[PATCH] pstore/ram: Add ramoops support for the Flattened Device Tree.

[PATCH] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Bryan Freed]

When called with a non-zero of_node, fill out a new ramoops_platform_data
with data from the specified Flattened Device Tree node.
Update ramoops documentation with the new FDT interface.

Change-Id: Id8f9f0abc5b564375c1b6d5d30c92d57d76520b7
Signed-off-by: Bryan Freed <bfreed@chromium.org>
---
 Documentation/ramoops.txt |   19 +++++++++++++++++--
 fs/pstore/ram.c           |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
index 4ba7db2..9ed3ab7 100644
--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
 
 Sergiu Iordache <sergiu@chromium.org>
 
-Updated: 17 November 2011
+Updated: 5 June 2012
 
 0. Introduction
 
@@ -37,7 +37,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).
  2. Use a platform device and set the platform data. The parameters can then
@@ -70,6 +70,21 @@ if (ret) {
 	return ret;
 }
 
+ 3. Use the Flattened Device Tree to set the platform data.  For example:
+  arch/arm/boot/dts/$BOARD.dts:
+        ramoops {
+                compatible = "ramoops";
+                reg = <0x41f00000 0x00100000>;
+                record-size = <0x00020000>;
+                dump-oops = <1>;
+                ecc = <0>;
+        };
+ The "reg = <0x41f00000 0x00100000>" line specifies a mem_size of 1MB at
+  mem_address 0x41f00000.
+ The "record-size = <0x00020000>" line specifies a record size of 128KB.
+ The "dump-oops = <1>" line tells us to dump oopses as well as panics.
+ The "ecc = <0>" line tells us to turn off ECC protection.
+
 3. Dump format
 
 The data dump begins with a header, currently defined as "====" followed by a
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 9123cce..bf0f882 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -32,6 +32,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/pstore_ram.h>
+#include <linux/of_address.h>
 
 #define RAMOOPS_KERNMSG_HDR "===="
 #define MIN_MEM_SIZE 4096UL
@@ -199,10 +200,44 @@ static struct ramoops_context oops_cxt = {
 	},
 };
 
+static int __init of_ramoops_platform_data(struct device_node *node,
+					struct ramoops_platform_data *pdata)
+{
+	const __be32 *addrp;
+	const __be32 *prop;
+	u64 size;
+
+	addrp = of_get_address(node, 0, &size, NULL);
+	if (addrp == NULL)
+		return -EINVAL;
+	pdata->mem_address = of_translate_address(node, addrp);
+	pdata->mem_size = size;
+
+	prop = of_get_property(node, "record-size", NULL);
+	if (!prop)
+		return -EINVAL;
+	pdata->record_size = be32_to_cpup(prop);
+
+	prop = of_get_property(node, "dump-oops", NULL);
+	if (!prop)
+		pdata->dump_oops = 0;
+	else
+		pdata->dump_oops = be32_to_cpup(prop);
+
+	prop = of_get_property(node, "ecc", NULL);
+	if (!prop)
+		pdata->ecc = 0;
+	else
+		pdata->ecc = be32_to_cpup(prop);
+
+	return 0;
+}
+
 static int __init ramoops_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct ramoops_platform_data *pdata = pdev->dev.platform_data;
+	struct ramoops_platform_data of_pdata;
 	struct ramoops_context *cxt = &oops_cxt;
 	int err = -EINVAL;
 	int i;
@@ -213,6 +248,14 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	if (cxt->max_count)
 		goto fail_out;
 
+	if (pdev->dev.of_node) {
+		if (of_ramoops_platform_data(pdev->dev.of_node, &of_pdata)) {
+			pr_err("Invalid ramoops device tree data\n");
+			goto fail_out;
+		}
+		pdata = &of_pdata;
+	}
+
 	if (!pdata->mem_size || !pdata->record_size) {
 		pr_err("The memory size and the record size must be "
 			"non-zero\n");
-- 
1.7.7.3

Re: [PATCH] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Stephen Boyd]

On 06/05/12 12:59, Bryan Freed wrote:
> When called with a non-zero of_node, fill out a new ramoops_platform_data
> with data from the specified Flattened Device Tree node.
> Update ramoops documentation with the new FDT interface.
>
> Change-Id: Id8f9f0abc5b564375c1b6d5d30c92d57d76520b7
> Signed-off-by: Bryan Freed <bfreed@chromium.org>
> ---
>  Documentation/ramoops.txt |   19 +++++++++++++++++--
>  fs/pstore/ram.c           |   43 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+), 2 deletions(-)

Can you document the binding in Documentation/devicetree/bindings/* too?

> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 9123cce..bf0f882 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -213,6 +248,14 @@ static int __init ramoops_probe(struct platform_device *pdev)
>  	if (cxt->max_count)
>  		goto fail_out;
>  
> +	if (pdev->dev.of_node) {
> +		if (of_ramoops_platform_data(pdev->dev.of_node, &of_pdata)) {
> +			pr_err("Invalid ramoops device tree data\n");

dev_err()?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Bryan Freed]

On Wed, Jun 6, 2012 at 10:16 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>
> On 06/05/12 12:59, Bryan Freed wrote:
> > When called with a non-zero of_node, fill out a new
> > ramoops_platform_data
> > with data from the specified Flattened Device Tree node.
> > Update ramoops documentation with the new FDT interface.
> >
> > Change-Id: Id8f9f0abc5b564375c1b6d5d30c92d57d76520b7
> > Signed-off-by: Bryan Freed <bfreed@chromium.org>
> > ---
> >  Documentation/ramoops.txt |   19 +++++++++++++++++--
> >  fs/pstore/ram.c           |   43
> > +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 60 insertions(+), 2 deletions(-)
>
> Can you document the binding in Documentation/devicetree/bindings/* too?

Good point, Stephen.  Will do...

>
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index 9123cce..bf0f882 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -213,6 +248,14 @@ static int __init ramoops_probe(struct
> > platform_device *pdev)
> >       if (cxt->max_count)
> >               goto fail_out;
> >
> > +     if (pdev->dev.of_node) {
> > +             if (of_ramoops_platform_data(pdev->dev.of_node,
> > &of_pdata)) {
> > +                     pr_err("Invalid ramoops device tree data\n");
>
> dev_err()?

I feel dev_err() would be a step in the wrong direction, but I do not
really know the merits of one vs the other.
In looking through all of fs/*, I do not see any
dev_{err|warn|crit|alert|emerg|notice}() calls other than the two
dev_err() calls added to this file (fs/pstore/ram.c) just a few weeks
ago.
I feel more comfortable sticking with this file's "pr_err" convention.
 If that is incorrect, another change should be submitted to change
them all to dev_err().  In this respect, I think Anton's change of May
17 (896fc1f0 in my repo) should have followed the pr_err convention.

Anyone have a strong feeling on dev_err vs pr_err and how/if this
transition should occur?

bryan.

> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>

Re: [PATCH] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Anton Vorontsov]

On Wed, Jun 06, 2012 at 11:00:27AM -0700, Bryan Freed wrote:
[...]
> Anyone have a strong feeling on dev_err vs pr_err and how/if this
> transition should occur?

If you have a device, it's better to use dev_*(), since that way
you'll get more information in the logs, e.g. which exact device
caused the issue.

But this makes sense only for drivers that support multiple
devices. Not the case for pstore now, though this might change
in the future. (E.g. we might have several pstore nodes some day.)

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Olof Johansson]

Hi,

On Tue, Jun 5, 2012 at 12:59 PM, Bryan Freed <bfreed@chromium.org> wrote:
> When called with a non-zero of_node, fill out a new ramoops_platform_data
> with data from the specified Flattened Device Tree node.
> Update ramoops documentation with the new FDT interface.
>
> Change-Id: Id8f9f0abc5b564375c1b6d5d30c92d57d76520b7
> Signed-off-by: Bryan Freed <bfreed@chromium.org>
> ---
>  Documentation/ramoops.txt |   19 +++++++++++++++++--
>  fs/pstore/ram.c           |   43 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
> index 4ba7db2..9ed3ab7 100644
> --- a/Documentation/ramoops.txt
> +++ b/Documentation/ramoops.txt
> @@ -3,7 +3,7 @@ Ramoops oops/panic logger
>
>  Sergiu Iordache <sergiu@chromium.org>
>
> -Updated: 17 November 2011
> +Updated: 5 June 2012
>
>  0. Introduction
>
> @@ -37,7 +37,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).
>  2. Use a platform device and set the platform data. The parameters can then
> @@ -70,6 +70,21 @@ if (ret) {
>        return ret;
>  }
>
> + 3. Use the Flattened Device Tree to set the platform data.  For example:
> +  arch/arm/boot/dts/$BOARD.dts:
> +        ramoops {
> +                compatible = "ramoops";
> +                reg = <0x41f00000 0x00100000>;
> +                record-size = <0x00020000>;
> +                dump-oops = <1>;
> +                ecc = <0>;
> +        };
> + The "reg = <0x41f00000 0x00100000>" line specifies a mem_size of 1MB at
> +  mem_address 0x41f00000.
> + The "record-size = <0x00020000>" line specifies a record size of 128KB.
> + The "dump-oops = <1>" line tells us to dump oopses as well as panics.
> + The "ecc = <0>" line tells us to turn off ECC protection.

Device tree properties are binary just by being present or not. So
there is no need to have values on these -- to turn "dump-oops" off,
don't include the property. Same for ecc.

But to take a step up, to be honest I don't think "dump-oops" belongs
here. The device tree is supposed to define hardware. In this case,
what truly needs to be defined for that is the base address, and
_possibly_ the record size used.

The rest are software attributes. For dump-oops, I propose using
sysctl/module parameters instead. That is true for ECC as well.

So, I would just remove those pieces from the device tree binding and
parsing, and focus on the memory range and record size. The rest can
be configured after booting (or on the commandline) if needed.

Also, the device-tree documentation should be added under
Documentation/devicetree/bindings as well (or maybe only there, and
referenced from the ramoops.txt docs).


Thanks!

-Olof