After Rob pointed out there's a non-ancient version of the devicetree bindings for pstore/ramoops, I figured it would be worth migrating over to using it.
So this series adds pstore/ramoops support to hikey, and its much more tidy compared to the last patch for this.
Would love any extra feedback and review.
The reserved ramoops address is mostly chosen at random, and while the bootloader doesn't seen to clear that memory, I suspect we will need a change to make sure that memory is protected by the bootloader and UEFI applications don't scribble over it. So help there would be appreciated!
thanks -john
Cc: Rob Herring rob.herring@linaro.org Cc: Arnd Bergmann arnd.bergmann@linaro.org Cc: Haojian Zhuang haojian.zhuang@linaro.org Cc: Guodong Xu guodong.xu@linaro.org Cc: Vishal Bhoj vishal.bhoj@linaro.org
Greg Hackmann (1): pstore-ram: add Device Tree bindings
John Stultz (1): hikey: dts: Add pstore support for HiKey
Documentation/ramoops.txt | 6 +- arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 13 +++ arch/arm64/configs/hikey_defconfig | 7 ++ fs/pstore/ram.c | 114 ++++++++++++++++++++++++- 4 files changed, 134 insertions(+), 6 deletions(-)
From: Greg Hackmann ghackmann@google.com
ramoops is one of the remaining places where ARM vendors still rely on board-specific shims. Device Tree lets us replace those shims with generic code.
These bindings mirror the ramoops module parameters, with two small differences:
(1) dump_oops becomes an optional "no-dump-oops" property, since ramoops sets dump_oops=1 by default.
(2) mem_type=1 becomes the more self-explanatory "unbuffered" property.
Cc: Rob Herring rob.herring@linaro.org Cc: Arnd Bergmann arnd.bergmann@linaro.org Cc: Haojian Zhuang haojian.zhuang@linaro.org Cc: Guodong Xu guodong.xu@linaro.org Cc: Vishal Bhoj vishal.bhoj@linaro.org Signed-off-by: Greg Hackmann ghackmann@google.com [jstultz: Backported to 4.1] Signed-off-by: John Stultz john.stultz@linaro.org --- Documentation/ramoops.txt | 6 ++- fs/pstore/ram.c | 114 ++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 114 insertions(+), 6 deletions(-)
diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt index 5d86756..9264bca 100644 --- a/Documentation/ramoops.txt +++ b/Documentation/ramoops.txt @@ -45,7 +45,7 @@ corrupt, but usually it is restorable.
2. Setting the parameters
-Setting the ramoops parameters can be done in 2 different manners: +Setting the ramoops parameters can be done in 3 different manners: 1. Use the module parameters (which have the names of the variables described as before). For quick debugging, you can also reserve parts of memory during boot @@ -54,7 +54,9 @@ Setting the ramoops parameters can be done in 2 different manners: kernel to use only the first 128 MB of memory, and place ECC-protected ramoops region at 128 MB boundary: "mem=128M ramoops.mem_address=0x8000000 ramoops.ecc=1" - 2. Use a platform device and set the platform data. The parameters can then + 2. Use Device Tree bindings, as described in + Documentation/device-tree/bindings/misc/ramoops.txt. + 3. Use a platform device and set the platform data. The parameters can then be set through that platform data. An example of doing that is:
#include <linux/pstore_ram.h> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c index 257dec9..6a89024 100644 --- a/fs/pstore/ram.c +++ b/fs/pstore/ram.c @@ -34,6 +34,8 @@ #include <linux/slab.h> #include <linux/compiler.h> #include <linux/pstore_ram.h> +#include <linux/of.h> +#include <linux/of_address.h>
#define RAMOOPS_KERNMSG_HDR "====" #define MIN_MEM_SIZE 4096UL @@ -450,15 +452,112 @@ void notrace ramoops_console_write_buf(const char *buf, size_t size) persistent_ram_write(cxt->cprz, buf, size); }
+static int ramoops_parse_dt_size(struct platform_device *pdev, + const char *propname, unsigned long *val) +{ + u64 val64; + int ret; + + ret = of_property_read_u64(pdev->dev.of_node, propname, &val64); + if (ret == -EINVAL) { + *val = 0; + return 0; + } else if (ret != 0) { + dev_err(&pdev->dev, "failed to parse property %s: %d\n", + propname, ret); + return ret; + } + + if (val64 > ULONG_MAX) { + dev_err(&pdev->dev, "invalid %s %llu\n", propname, val64); + return -EOVERFLOW; + } + + *val = val64; + return 0; +} + +static int ramoops_parse_dt(struct platform_device *pdev, + struct ramoops_platform_data *pdata) +{ + struct device_node *of_node = pdev->dev.of_node; + struct device_node *mem_region; + struct resource res; + u32 ecc_size; + int ret; + + dev_dbg(&pdev->dev, "using Device Tree\n"); + + mem_region = of_parse_phandle(of_node, "memory-region", 0); + if (!mem_region) { + dev_err(&pdev->dev, "no memory-region phandle\n"); + return -ENODEV; + } + + ret = of_address_to_resource(mem_region, 0, &res); + of_node_put(mem_region); + if (ret) { + dev_err(&pdev->dev, "failed to translate memory-region to resource: %d\n", + ret); + return ret; + } + + pdata->mem_size = resource_size(&res); + pdata->mem_address = res.start; + pdata->mem_type = of_property_read_bool(of_node, "unbuffered"); + pdata->dump_oops = of_property_read_bool(of_node, "dump-oops"); + + ret = ramoops_parse_dt_size(pdev, "record-size", &pdata->record_size); + if (ret < 0) + return ret; + + ret = ramoops_parse_dt_size(pdev, "console-size", &pdata->console_size); + if (ret < 0) + return ret; + + ret = ramoops_parse_dt_size(pdev, "ftrace-size", &pdata->ftrace_size); + if (ret < 0) + return ret; + + ret = ramoops_parse_dt_size(pdev, "pmsg-size", &pdata->pmsg_size); + if (ret < 0) + return ret; + + ret = of_property_read_u32(of_node, "ecc-size", &ecc_size); + if (ret == 0) { + if (ecc_size > INT_MAX) { + dev_err(&pdev->dev, "invalid ecc-size %u\n", ecc_size); + return -EOVERFLOW; + } + pdata->ecc_info.ecc_size = ecc_size; + } else if (ret != -EINVAL) { + return ret; + } + + return 0; +} + static int ramoops_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct ramoops_platform_data *pdata = pdev->dev.platform_data; + struct ramoops_platform_data *pdata = platform_get_drvdata(pdev); struct ramoops_context *cxt = &oops_cxt; size_t dump_mem_sz; phys_addr_t paddr; int err = -EINVAL;
+ if (dev->of_node && !pdata) { + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) { + err = -ENOMEM; + goto fail_out; + } + + err = ramoops_parse_dt(pdev, pdata); + if (err < 0) + goto fail_out; + } + /* Only a single ramoops area allowed at a time, so fail extra * probes. */ @@ -553,6 +652,7 @@ static int ramoops_probe(struct platform_device *pdev) cxt->size, (unsigned long long)cxt->phys_addr, cxt->ecc_info.ecc_size, cxt->ecc_info.block_size);
+ platform_set_drvdata(pdev, pdata); return 0;
fail_buf: @@ -591,11 +691,17 @@ static int __exit ramoops_remove(struct platform_device *pdev) return -EBUSY; }
+static const struct of_device_id dt_match[] = { + { .compatible = "ramoops" }, + {} +}; + static struct platform_driver ramoops_driver = { - .probe = ramoops_probe, + .probe = ramoops_probe, .remove = __exit_p(ramoops_remove), - .driver = { - .name = "ramoops", + .driver = { + .name = "ramoops", + .of_match_table = dt_match, }, };
This patch reserves some memory in the DTS and sets up a pstore device tree node to enable pstore support on HiKey.
This also enables the needed config options in the defconfig.
Cc: Rob Herring rob.herring@linaro.org Cc: Arnd Bergmann arnd.bergmann@linaro.org Cc: Haojian Zhuang haojian.zhuang@linaro.org Cc: Guodong Xu guodong.xu@linaro.org Cc: Vishal Bhoj vishal.bhoj@linaro.org Signed-off-by: John Stultz john.stultz@linaro.org --- arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 13 +++++++++++++ arch/arm64/configs/hikey_defconfig | 7 +++++++ 2 files changed, 20 insertions(+)
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts index f0cd5fa..36d300f 100644 --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts @@ -61,6 +61,19 @@ no-map; reg = <0x0 0x06dff000 0x0 0x00001000>; /* Mailbox message buf */ }; + + pstore: pstore@0x21f00000 { + no-map; + reg = <0x0 0x21f00000 0x0 0x00100000>; /* pstore/ramoops buffer */ + }; + }; + + ramoops { + compatible = "ramoops"; + memory-region = <&pstore>; + record-size = <0x0 0x00020000>; + console-size = <0x0 0x00020000>; + ftrace-size = <0x0 0x00020000>; };
reboot_reason: reboot-reason@05f01000 { diff --git a/arch/arm64/configs/hikey_defconfig b/arch/arm64/configs/hikey_defconfig index e3cd02d..c8c6511 100644 --- a/arch/arm64/configs/hikey_defconfig +++ b/arch/arm64/configs/hikey_defconfig @@ -354,6 +354,7 @@ CONFIG_ANDROID=y CONFIG_ANDROID_BINDER_IPC=y CONFIG_REBOOT_REASON_SRAM=y CONFIG_EFI_VARS=y +CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE=y CONFIG_EXT4_FS=y CONFIG_EXT4_FS_SECURITY=y CONFIG_BTRFS_FS=y @@ -371,6 +372,10 @@ CONFIG_TMPFS=y CONFIG_HUGETLBFS=y CONFIG_SQUASHFS=y CONFIG_SQUASHFS_XZ=y +CONFIG_PSTORE=y +CONFIG_PSTORE_CONSOLE=y +CONFIG_PSTORE_PMSG=y +CONFIG_PSTORE_RAM=y CONFIG_NFS_FS=y # CONFIG_NFS_V2 is not set CONFIG_NFS_V3_ACL=y @@ -385,6 +390,8 @@ CONFIG_PRINTK_TIME=y CONFIG_DEBUG_INFO=y CONFIG_MAGIC_SYSRQ=y CONFIG_LOCKUP_DETECTOR=y +CONFIG_PANIC_ON_OOPS=y +CONFIG_PANIC_TIMEOUT=10 # CONFIG_SCHED_DEBUG is not set CONFIG_SCHEDSTATS=y CONFIG_TIMER_STATS=y
On Tuesday 05 January 2016 20:53:50 John Stultz wrote:
From: Greg Hackmann ghackmann@google.com
ramoops is one of the remaining places where ARM vendors still rely on board-specific shims. Device Tree lets us replace those shims with generic code.
These bindings mirror the ramoops module parameters, with two small differences:
(1) dump_oops becomes an optional "no-dump-oops" property, since ramoops sets dump_oops=1 by default.
The code has a "dump-oops" property, not "no-dump-oops".
(2) mem_type=1 becomes the more self-explanatory "unbuffered" property.
There should also be a DT binding document that describes the valid properties.
@@ -450,15 +452,112 @@ void notrace ramoops_console_write_buf(const char *buf, size_t size) persistent_ram_write(cxt->cprz, buf, size); } +static int ramoops_parse_dt_size(struct platform_device *pdev,
const char *propname, unsigned long *val)+{
- u64 val64;
- int ret;
- ret = of_property_read_u64(pdev->dev.of_node, propname, &val64);
- if (ret == -EINVAL) {
*val = 0;return 0;- } else if (ret != 0) {
dev_err(&pdev->dev, "failed to parse property %s: %d\n",propname, ret);return ret;- }
- if (val64 > ULONG_MAX) {
dev_err(&pdev->dev, "invalid %s %llu\n", propname, val64);return -EOVERFLOW;- }
- *val = val64;
- return 0;
+}
Should this perhaps use the normal address parser using #address-cells and #size-cells, rather than hardcoding 64-bit?
+static int ramoops_parse_dt(struct platform_device *pdev,
struct ramoops_platform_data *pdata)+{
- struct device_node *of_node = pdev->dev.of_node;
- struct device_node *mem_region;
- struct resource res;
- u32 ecc_size;
- int ret;
- dev_dbg(&pdev->dev, "using Device Tree\n");
- mem_region = of_parse_phandle(of_node, "memory-region", 0);
- if (!mem_region) {
dev_err(&pdev->dev, "no memory-region phandle\n");return -ENODEV;- }
- ret = of_address_to_resource(mem_region, 0, &res);
- of_node_put(mem_region);
- if (ret) {
dev_err(&pdev->dev, "failed to translate memory-region to resource: %d\n",ret);return ret;- }
This is a rather odd way of doing it. Why is the ramoops block not a device under the memory region itself?
Arnd
On Thu, Jan 7, 2016 at 10:45 AM, Arnd Bergmann arnd@arndb.de wrote:
On Tuesday 05 January 2016 20:53:50 John Stultz wrote:
From: Greg Hackmann ghackmann@google.com
ramoops is one of the remaining places where ARM vendors still rely on board-specific shims. Device Tree lets us replace those shims with generic code.
These bindings mirror the ramoops module parameters, with two small differences:
(1) dump_oops becomes an optional "no-dump-oops" property, since ramoops sets dump_oops=1 by default.
The code has a "dump-oops" property, not "no-dump-oops".
(2) mem_type=1 becomes the more self-explanatory "unbuffered" property.
There should also be a DT binding document that describes the valid properties.
There is. I think John dropped it. You should review the version on lkml.
[...]
+static int ramoops_parse_dt(struct platform_device *pdev,
struct ramoops_platform_data *pdata)+{
struct device_node *of_node = pdev->dev.of_node;struct device_node *mem_region;struct resource res;u32 ecc_size;int ret;dev_dbg(&pdev->dev, "using Device Tree\n");mem_region = of_parse_phandle(of_node, "memory-region", 0);if (!mem_region) {dev_err(&pdev->dev, "no memory-region phandle\n");return -ENODEV;}ret = of_address_to_resource(mem_region, 0, &res);of_node_put(mem_region);if (ret) {dev_err(&pdev->dev, "failed to translate memory-region to resource: %d\n",ret);return ret;}This is a rather odd way of doing it. Why is the ramoops block not a device under the memory region itself?
Probably so that a platform device will be created. Your suggestion is probably a better way to go. We also have to consider pstore in onchip RAM buffers. We really need a way to instantiate platform drivers from DT yet don't map 1-1 with a device node and a compatible string (I'm looking at you, cpufreq drivers). I attempted something before but anything creating platform device in an initcall gets shot down.
Rob
Sorry for the delay in putting together V3 of this patch; I'll send that out today based on Rob's and Arnd's feedback.
On 01/07/2016 08:45 AM, Arnd Bergmann wrote:
(1) dump_oops becomes an optional "no-dump-oops" property, since ramoops sets dump_oops=1 by default.
The code has a "dump-oops" property, not "no-dump-oops".
Will fix in V3. (This is meant to be no-dump-oops as documented, not dump-oops as implemented.)
Should this perhaps use the normal address parser using #address-cells and #size-cells, rather than hardcoding 64-bit?
These sizes don't have an address associated with them.
On Thu, Jan 7, 2016 at 9:03 AM, Rob Herring rob.herring@linaro.org wrote:
On Thu, Jan 7, 2016 at 10:45 AM, Arnd Bergmann arnd@arndb.de wrote:
On Tuesday 05 January 2016 20:53:50 John Stultz wrote:
(1) dump_oops becomes an optional "no-dump-oops" property, since ramoops sets dump_oops=1 by default.
The code has a "dump-oops" property, not "no-dump-oops".
(2) mem_type=1 becomes the more self-explanatory "unbuffered" property.
There should also be a DT binding document that describes the valid properties.
There is. I think John dropped it. You should review the version on lkml.
Not intentionally, apologies. I must have missed the add after doing fixups (the patch didn't apply cleanly when backported).
And sorry to Greg for the extra noise here. I was hoping to get some internal review before committing for 96boards, and forgot you'd be pulled into the cc by git send-email. But hopefully the feedback is useful and glad to hear a v3 is in the works. :)
thanks -john
-
Arnd Bergmann -
Greg Hackmann -
John Stultz -
Rob Herring