[PATCH stblinux.git 1/2] dt-bindings: firmware: add Broadcom's NVRAM memory mapping

[PATCH stblinux.git 1/2] dt-bindings: firmware: add Broadcom's NVRAM memory mapping

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

NVRAM structure contains device data and can be accessed using MMIO.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../bindings/firmware/brcm,nvram.yaml         | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/brcm,nvram.yaml

diff --git a/Documentation/devicetree/bindings/firmware/brcm,nvram.yaml b/Documentation/devicetree/bindings/firmware/brcm,nvram.yaml
new file mode 100644
index 000000000000..12af8e2e7c9c
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/brcm,nvram.yaml
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/firmware/brcm,nvram.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Broadcom's NVRAM
+
+maintainers:
+  - Rafał Miłecki <rafal@milecki.pl>
+
+description: |
+  NVRAM is a structure containing device specific environment variables.
+  It is used for storing device configuration, booting parameters and
+  calibration data.
+
+  It's required very early in booting process and so is made available
+  using memory mapping.
+
+  NVRAM can be found on Broadcom BCM47xx MIPS, Northstar ARM Cortex-A9
+  and some more devices.
+
+properties:
+  compatible:
+    const: brcm,nvram
+
+  reg:
+    description: memory region with NVRAM data
+    maxItems: 1
+
+required:
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    nvram@1e000000 {
+         compatible = "brcm,nvram";
+         reg = <0x1e000000 0x10000>;
+    };
-- 
2.26.2

[PATCH stblinux.git 2/2] firmware: bcm47xx_nvram: support platform device "brcm,nvram"

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Add support for platform device providing mapping resource. This allows
reading NVRAM based on DT mapping binding. It's required for devices
that boot depending on NVRAM stored setup and provides early access to
NVRAM data.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
bcm47xx_nvram driver was originally added through MIPS tree, but this
change doesn't affect BCM47XX (MIPS) as it doesn't use DT. It targets
ARCH_BCM_5301X so I suggest this goes through the stblinux.git tree.
---
 drivers/firmware/broadcom/bcm47xx_nvram.c | 55 +++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/firmware/broadcom/bcm47xx_nvram.c b/drivers/firmware/broadcom/bcm47xx_nvram.c
index 835ece9c00f1..d5d19dd1b9e1 100644
--- a/drivers/firmware/broadcom/bcm47xx_nvram.c
+++ b/drivers/firmware/broadcom/bcm47xx_nvram.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/mtd/mtd.h>
+#include <linux/platform_device.h>
 #include <linux/bcm47xx_nvram.h>
 
 #define NVRAM_MAGIC			0x48534C46	/* 'FLSH' */
@@ -162,6 +163,60 @@ static int nvram_init(void)
 	return -ENXIO;
 }
 
+static int brcm_nvram_probe(struct platform_device *pdev)
+{
+	struct nvram_header __iomem *header;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	void __iomem *mmio;
+	size_t copy_len;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "Failed to get resource\n");
+		return -ENODEV;
+	}
+
+	mmio = ioremap(res->start, resource_size(res));
+	if (!mmio)
+		return -ENOMEM;
+
+	header = (struct nvram_header *)mmio;
+	copy_len = DIV_ROUND_UP(sizeof(*header) + header->len, 4);
+	if (header->magic != NVRAM_MAGIC) {
+		dev_err(dev, "No NVRAM found at %pR\n", res);
+		return -EPROTO;
+	} else if (copy_len > resource_size(res)) {
+		dev_err(dev, "NVRAM size exceeds %pR\n", res);
+		return -ERANGE;
+	} else if (copy_len >= NVRAM_SPACE) {
+		dev_err(dev, "NVRAM size exceeds buffer size %d\n", NVRAM_SPACE);
+		return -ENOMEM;
+	}
+
+	__ioread32_copy(nvram_buf, mmio, copy_len);
+	nvram_buf[NVRAM_SPACE - 1] = '\0';
+
+	iounmap(mmio);
+
+	return 0;
+}
+
+static const struct of_device_id brcm_nvram_of_match[] = {
+	{ .compatible = "brcm,nvram "},
+	{},
+};
+
+static struct platform_driver brcm_nvram_driver = {
+	.driver = {
+		.name = "brcm_nvram",
+		.of_match_table = brcm_nvram_of_match,
+	},
+	.probe	= brcm_nvram_probe,
+};
+
+module_platform_driver(brcm_nvram_driver);
+
 int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len)
 {
 	char *var, *value, *end, *eq;
-- 
2.26.2

Re: [PATCH stblinux.git 2/2] firmware: bcm47xx_nvram: support platform device "brcm,nvram"

[Florian Fainelli]

On 3/1/21 11:44 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Add support for platform device providing mapping resource. This allows
> reading NVRAM based on DT mapping binding. It's required for devices
> that boot depending on NVRAM stored setup and provides early access to
> NVRAM data.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> bcm47xx_nvram driver was originally added through MIPS tree, but this
> change doesn't affect BCM47XX (MIPS) as it doesn't use DT. It targets
> ARCH_BCM_5301X so I suggest this goes through the stblinux.git tree.

Can you see if this change can be replaced by the nvmem-rmem work that
Nicolas recently did to support something similar for the Raspberry Pi 4:

https://lkml.org/lkml/2021/1/29/235

> ---
>  drivers/firmware/broadcom/bcm47xx_nvram.c | 55 +++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/firmware/broadcom/bcm47xx_nvram.c b/drivers/firmware/broadcom/bcm47xx_nvram.c
> index 835ece9c00f1..d5d19dd1b9e1 100644
> --- a/drivers/firmware/broadcom/bcm47xx_nvram.c
> +++ b/drivers/firmware/broadcom/bcm47xx_nvram.c
> @@ -13,6 +13,7 @@
>  #include <linux/kernel.h>
>  #include <linux/string.h>
>  #include <linux/mtd/mtd.h>
> +#include <linux/platform_device.h>
>  #include <linux/bcm47xx_nvram.h>
>  
>  #define NVRAM_MAGIC			0x48534C46	/* 'FLSH' */
> @@ -162,6 +163,60 @@ static int nvram_init(void)
>  	return -ENXIO;
>  }
>  
> +static int brcm_nvram_probe(struct platform_device *pdev)
> +{
> +	struct nvram_header __iomem *header;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	void __iomem *mmio;
> +	size_t copy_len;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "Failed to get resource\n");
> +		return -ENODEV;
> +	}
> +
> +	mmio = ioremap(res->start, resource_size(res));
> +	if (!mmio)
> +		return -ENOMEM;
> +
> +	header = (struct nvram_header *)mmio;
> +	copy_len = DIV_ROUND_UP(sizeof(*header) + header->len, 4);
> +	if (header->magic != NVRAM_MAGIC) {
> +		dev_err(dev, "No NVRAM found at %pR\n", res);
> +		return -EPROTO;
> +	} else if (copy_len > resource_size(res)) {
> +		dev_err(dev, "NVRAM size exceeds %pR\n", res);
> +		return -ERANGE;
> +	} else if (copy_len >= NVRAM_SPACE) {
> +		dev_err(dev, "NVRAM size exceeds buffer size %d\n", NVRAM_SPACE);
> +		return -ENOMEM;
> +	}
> +
> +	__ioread32_copy(nvram_buf, mmio, copy_len);
> +	nvram_buf[NVRAM_SPACE - 1] = '\0';
> +
> +	iounmap(mmio);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id brcm_nvram_of_match[] = {
> +	{ .compatible = "brcm,nvram "},
> +	{},
> +};
> +
> +static struct platform_driver brcm_nvram_driver = {
> +	.driver = {
> +		.name = "brcm_nvram",
> +		.of_match_table = brcm_nvram_of_match,
> +	},
> +	.probe	= brcm_nvram_probe,
> +};
> +
> +module_platform_driver(brcm_nvram_driver);
> +
>  int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len)
>  {
>  	char *var, *value, *end, *eq;
> 


-- 
Florian

Re: [PATCH stblinux.git 2/2] firmware: bcm47xx_nvram: support platform device "brcm,nvram"

[Rafał Miłecki]

On 02.03.2021 17:59, Florian Fainelli wrote:
> On 3/1/21 11:44 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Add support for platform device providing mapping resource. This allows
>> reading NVRAM based on DT mapping binding. It's required for devices
>> that boot depending on NVRAM stored setup and provides early access to
>> NVRAM data.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> bcm47xx_nvram driver was originally added through MIPS tree, but this
>> change doesn't affect BCM47XX (MIPS) as it doesn't use DT. It targets
>> ARCH_BCM_5301X so I suggest this goes through the stblinux.git tree.
> 
> Can you see if this change can be replaced by the nvmem-rmem work that
> Nicolas recently did to support something similar for the Raspberry Pi 4:
> 
> https://lkml.org/lkml/2021/1/29/235

I don't think it fits my case.

It's a reserved memory binding/driver which refers to the system memory.
In NVRAM case we need to do a mapping. I think it's different?

nvmem-rmem registers NVMEM device without providing any cells. It also
doesn't understand NVRAM data structure. I guess nvmem-rmem only exposes
NVMEM for user-space access. I need to access NVRAM to e.g. detect boot
parameters in kernel code.

I was thinking for a moment about treating NVRAM like a NVMEM but NVRAM
doesn't seem to fit current design and kernel API. NVMEM assumes that
every cell has a specific offset and size. Reading NVRAM should be
based on string keys (nof offsets). See nvmem_reg_read_t for details.

This won't make a huge difference I think, but for a slightly cleaner
design I could probably have NVRAM devices without cells and make it
setup NVRAM. Let me see if I can code that.

Re: [PATCH stblinux.git 1/2] dt-bindings: firmware: add Broadcom's NVRAM memory mapping

[Rob Herring]

On Tue, Mar 02, 2021 at 08:44:04AM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> NVRAM structure contains device data and can be accessed using MMIO.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  .../bindings/firmware/brcm,nvram.yaml         | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/brcm,nvram.yaml
> 
> diff --git a/Documentation/devicetree/bindings/firmware/brcm,nvram.yaml b/Documentation/devicetree/bindings/firmware/brcm,nvram.yaml
> new file mode 100644
> index 000000000000..12af8e2e7c9c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/brcm,nvram.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/firmware/brcm,nvram.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Broadcom's NVRAM
> +
> +maintainers:
> +  - Rafał Miłecki <rafal@milecki.pl>
> +
> +description: |
> +  NVRAM is a structure containing device specific environment variables.
> +  It is used for storing device configuration, booting parameters and
> +  calibration data.

The structure of the data is fully discoverable just from a genericish 
'brcm,nvram'?

And it's a dedicated memory outside of regular RAM?

> +
> +  It's required very early in booting process and so is made available
> +  using memory mapping.
> +
> +  NVRAM can be found on Broadcom BCM47xx MIPS, Northstar ARM Cortex-A9
> +  and some more devices.
> +
> +properties:
> +  compatible:
> +    const: brcm,nvram
> +
> +  reg:
> +    description: memory region with NVRAM data
> +    maxItems: 1
> +
> +required:
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    nvram@1e000000 {
> +         compatible = "brcm,nvram";
> +         reg = <0x1e000000 0x10000>;
> +    };
> -- 
> 2.26.2
> 

Re: [PATCH stblinux.git 1/2] dt-bindings: firmware: add Broadcom's NVRAM memory mapping

[Rafał Miłecki]

On 08.03.2021 19:43, Rob Herring wrote:
> On Tue, Mar 02, 2021 at 08:44:04AM +0100, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> NVRAM structure contains device data and can be accessed using MMIO.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>   .../bindings/firmware/brcm,nvram.yaml         | 41 +++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/firmware/brcm,nvram.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/brcm,nvram.yaml b/Documentation/devicetree/bindings/firmware/brcm,nvram.yaml
>> new file mode 100644
>> index 000000000000..12af8e2e7c9c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/firmware/brcm,nvram.yaml
>> @@ -0,0 +1,41 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/firmware/brcm,nvram.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> +
>> +title: Broadcom's NVRAM
>> +
>> +maintainers:
>> +  - Rafał Miłecki <rafal@milecki.pl>
>> +
>> +description: |
>> +  NVRAM is a structure containing device specific environment variables.
>> +  It is used for storing device configuration, booting parameters and
>> +  calibration data.
> 
> The structure of the data is fully discoverable just from a genericish
> 'brcm,nvram'?

Yes, NVRAM structure is a header (with magic and length) and a list of
key-value pairs separated by \0. If you map memory at given address you
should verify magic and start reading key-value pairs.

Content example: foo=bar\0baz=qux\0quux(...)

There is no predefined order of pairs, set of keys or anything similar I
could think of. I can't think of anything more worth describing in DT.


> And it's a dedicated memory outside of regular RAM?

Yes

Re: [PATCH stblinux.git 1/2] dt-bindings: firmware: add Broadcom's NVRAM memory mapping

[Rafał Miłecki]

On 08.03.2021 22:37, Rafał Miłecki wrote:
> On 08.03.2021 19:43, Rob Herring wrote:
>> On Tue, Mar 02, 2021 at 08:44:04AM +0100, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> NVRAM structure contains device data and can be accessed using MMIO.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>>   .../bindings/firmware/brcm,nvram.yaml         | 41 +++++++++++++++++++
>>>   1 file changed, 41 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/firmware/brcm,nvram.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/firmware/brcm,nvram.yaml b/Documentation/devicetree/bindings/firmware/brcm,nvram.yaml
>>> new file mode 100644
>>> index 000000000000..12af8e2e7c9c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/firmware/brcm,nvram.yaml
>>> @@ -0,0 +1,41 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: "http://devicetree.org/schemas/firmware/brcm,nvram.yaml#"
>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>> +
>>> +title: Broadcom's NVRAM
>>> +
>>> +maintainers:
>>> +  - Rafał Miłecki <rafal@milecki.pl>
>>> +
>>> +description: |
>>> +  NVRAM is a structure containing device specific environment variables.
>>> +  It is used for storing device configuration, booting parameters and
>>> +  calibration data.
>>
>> The structure of the data is fully discoverable just from a genericish
>> 'brcm,nvram'?
> 
> Yes, NVRAM structure is a header (with magic and length) and a list of
> key-value pairs separated by \0. If you map memory at given address you
> should verify magic and start reading key-value pairs.
> 
> Content example: foo=bar\0baz=qux\0quux(...)
> 
> There is no predefined order of pairs, set of keys or anything similar I
> could think of. I can't think of anything more worth describing in DT.

Ah, I've just realized, I'm replying to the "firmware" binding patch.

Florian suggested to look at NVMEM subsystem instead. Please kindly check
[PATCH V2 1/2] dt-bindings: nvmem: add Broadcom's NVRAM