[PATCH v2 0/2] Use NVMEM as reboot-mode write interface

[PATCH v2 0/2] Use NVMEM as reboot-mode write interface

[Han Nandor]

Description
-----------
Extend the reboot mode driver to use a NVMEM cell as writing interface.

Testing
-------
The testing is done by configuring DT from a custom board.
The NVMEM cell is configured in an RTC non-volatile memory.
Kernel: 4.14.60 (the patchset was rebased on kernel master)

DT configurations:
`
...
reboot-mode-nvmem@0 {
    compatible = "simple-mfd";
    reboot-mode {
        compatible = "nvmem-reboot-mode";
        nvmem-cells = <&reboot_mode>;
        nvmem-cell-names = "reboot-mode";

        mode-test       = <0x21969147>;
    };
};
...
reboot_mode: nvmem_reboot_mode@0 {
        reg = <0x00 0x4>;
};
...
`

1. Reboot the system using the command `reboot test`

2. Verify that kernel logs show that reboot was done in mode `test`:
PASS
`[  413.957172] reboot: Restarting system with command 'test' `

3. Stop in U-Boot and verify that mode `test` magic value is present
in RTCs non-volatile memory: PASS

Kernel: 5.1.0-rc3

1. Configure `arch/arm/configs/imx_v6_v7_defconfig` to contain 
`CONFIG_NVMEM_REBOOT_MODE=y`
2. Verify that Kernel compiles successful: PASS
`
make ARCH=arm CROSS_COMPILE=arm-linux-gnu- imx_v6_v7_defconfig zImage
...
CC      drivers/power/reset/nvmem-reboot-mode.o
...
Kernel: arch/arm/boot/zImage is ready
`
Changes since v1:
-----------------
 - split the documentation on a separate patch
 - add a missing header

Nandor Han (2):
  power: reset: nvmem-reboot-mode: use NVMEM as reboot mode write
    interface
  dt-bindings: power: reset: add document for NVMEM based reboot-mode

 .../power/reset/nvmem-reboot-mode.txt         | 32 ++++++++
 drivers/power/reset/Kconfig                   |  9 +++
 drivers/power/reset/Makefile                  |  1 +
 drivers/power/reset/nvmem-reboot-mode.c       | 76 +++++++++++++++++++
 4 files changed, 118 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.txt
 create mode 100644 drivers/power/reset/nvmem-reboot-mode.c

-- 
2.17.2

[PATCH v2 1/2] power: reset: nvmem-reboot-mode: use NVMEM as reboot mode write interface

[Han Nandor]

Add a new reboot mode write interface that is using an NVMEM cell
to store the reboot mode magic.

Signed-off-by: Nandor Han <nandor.han@vaisala.com>
---
 drivers/power/reset/Kconfig             |  9 +++
 drivers/power/reset/Makefile            |  1 +
 drivers/power/reset/nvmem-reboot-mode.c | 76 +++++++++++++++++++++++++
 3 files changed, 86 insertions(+)
 create mode 100644 drivers/power/reset/nvmem-reboot-mode.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index 6533aa560aa1..bb4a4e854f96 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -245,5 +245,14 @@ config POWER_RESET_SC27XX
 	  PMICs includes the SC2720, SC2721, SC2723, SC2730
 	  and SC2731 chips.
 
+config NVMEM_REBOOT_MODE
+	tristate "Generic NVMEM reboot mode driver"
+	select REBOOT_MODE
+	help
+	  Say y here will enable reboot mode driver. This will
+	  get reboot mode arguments and store it in a NVMEM cell,
+	  then the bootloader can read it and take different
+	  action according to the mode.
+
 endif
 
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index 0aebee954ac1..85da3198e4e0 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_POWER_RESET_ZX) += zx-reboot.o
 obj-$(CONFIG_REBOOT_MODE) += reboot-mode.o
 obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
 obj-$(CONFIG_POWER_RESET_SC27XX) += sc27xx-poweroff.o
+obj-$(CONFIG_NVMEM_REBOOT_MODE) += nvmem-reboot-mode.o
diff --git a/drivers/power/reset/nvmem-reboot-mode.c b/drivers/power/reset/nvmem-reboot-mode.c
new file mode 100644
index 000000000000..816cfdab16a7
--- /dev/null
+++ b/drivers/power/reset/nvmem-reboot-mode.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) Vaisala Oyj. All rights reserved.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/platform_device.h>
+#include <linux/reboot-mode.h>
+
+struct nvmem_reboot_mode {
+	struct reboot_mode_driver reboot;
+	struct nvmem_cell *cell;
+};
+
+static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot,
+				    unsigned int magic)
+{
+	int ret;
+	struct nvmem_reboot_mode *nvmem_rbm;
+
+	nvmem_rbm = container_of(reboot, struct nvmem_reboot_mode, reboot);
+
+	ret = nvmem_cell_write(nvmem_rbm->cell, &magic, sizeof(magic));
+	if (ret < 0)
+		dev_err(reboot->dev, "update reboot mode bits failed\n");
+
+	return ret;
+}
+
+static int nvmem_reboot_mode_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct nvmem_reboot_mode *nvmem_rbm;
+
+	nvmem_rbm = devm_kzalloc(&pdev->dev, sizeof(*nvmem_rbm), GFP_KERNEL);
+	if (!nvmem_rbm)
+		return -ENOMEM;
+
+	nvmem_rbm->reboot.dev = &pdev->dev;
+	nvmem_rbm->reboot.write = nvmem_reboot_mode_write;
+
+	nvmem_rbm->cell = devm_nvmem_cell_get(&pdev->dev, "reboot-mode");
+	if (IS_ERR(nvmem_rbm->cell)) {
+		dev_err(&pdev->dev, "failed to get the nvmem cell reboot-mode\n");
+		return PTR_ERR(nvmem_rbm->cell);
+	}
+
+	ret = devm_reboot_mode_register(&pdev->dev, &nvmem_rbm->reboot);
+	if (ret)
+		dev_err(&pdev->dev, "can't register reboot mode\n");
+
+	return ret;
+}
+
+static const struct of_device_id nvmem_reboot_mode_of_match[] = {
+	{ .compatible = "nvmem-reboot-mode" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, nvmem_reboot_mode_of_match);
+
+static struct platform_driver nvmem_reboot_mode_driver = {
+	.probe = nvmem_reboot_mode_probe,
+	.driver = {
+		.name = "nvmem-reboot-mode",
+		.of_match_table = nvmem_reboot_mode_of_match,
+	},
+};
+module_platform_driver(nvmem_reboot_mode_driver);
+
+MODULE_AUTHOR("Nandor Han <nandor.han@vaisala.com>");
+MODULE_DESCRIPTION("NVMEM reboot mode driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.2

[PATCH v2 2/2] dt-bindings: power: reset: add document for NVMEM based reboot-mode

[Han Nandor]

Add the device tree bindings document for the NVMEM based reboot-mode
driver.

Signed-off-by: Nandor Han <nandor.han@vaisala.com>
---
 .../power/reset/nvmem-reboot-mode.txt         | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.txt

diff --git a/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.txt
new file mode 100644
index 000000000000..2e1b86c31cb3
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.txt
@@ -0,0 +1,32 @@
+NVMEM reboot mode driver
+
+This driver gets reboot mode magic value from reboot-mode driver
+and stores it in a NVMEM cell named "reboot-mode". Then the bootloader
+can read it and take different action according to the magic
+value stored.
+
+This DT node should be represented as a sub-node of a "simple-mfd"
+node.
+
+Required properties:
+- compatible: should be "nvmem-reboot-mode".
+- nvmem-cells: A phandle to the reboot mode provided by a nvmem device.
+- nvmem-cell-names: Should be "reboot-mode".
+
+The rest of the properties should follow the generic reboot-mode description
+found in reboot-mode.txt
+
+Example:
+	reboot-mode-nvmem@0 {
+		compatible = "simple-mfd";
+		reboot-mode {
+			compatible = "nvmem-reboot-mode";
+			nvmem-cells = <&reboot_mode>;
+			nvmem-cell-names = "reboot-mode";
+
+			mode-normal     = <0xAAAA5501>;
+			mode-bootloader = <0xBBBB5500>;
+			mode-recovery   = <0xCCCC5502>;
+			mode-test       = <0xDDDD5503>;
+		};
+	};
-- 
2.17.2

Re: [PATCH v2 1/2] power: reset: nvmem-reboot-mode: use NVMEM as reboot mode write interface

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4470 bytes --]

On Thu, Apr 11, 2019 at 05:54:09AM +0000, Han Nandor wrote:
> Add a new reboot mode write interface that is using an NVMEM cell
> to store the reboot mode magic.
> 
> Signed-off-by: Nandor Han <nandor.han@vaisala.com>
> ---
>  drivers/power/reset/Kconfig             |  9 +++
>  drivers/power/reset/Makefile            |  1 +
>  drivers/power/reset/nvmem-reboot-mode.c | 76 +++++++++++++++++++++++++
>  3 files changed, 86 insertions(+)
>  create mode 100644 drivers/power/reset/nvmem-reboot-mode.c
> 
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index 6533aa560aa1..bb4a4e854f96 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -245,5 +245,14 @@ config POWER_RESET_SC27XX
>  	  PMICs includes the SC2720, SC2721, SC2723, SC2730
>  	  and SC2731 chips.
>  
> +config NVMEM_REBOOT_MODE
> +	tristate "Generic NVMEM reboot mode driver"
> +	select REBOOT_MODE
> +	help
> +	  Say y here will enable reboot mode driver. This will
> +	  get reboot mode arguments and store it in a NVMEM cell,
> +	  then the bootloader can read it and take different
> +	  action according to the mode.
> +
>  endif
>  
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index 0aebee954ac1..85da3198e4e0 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_POWER_RESET_ZX) += zx-reboot.o
>  obj-$(CONFIG_REBOOT_MODE) += reboot-mode.o
>  obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
>  obj-$(CONFIG_POWER_RESET_SC27XX) += sc27xx-poweroff.o
> +obj-$(CONFIG_NVMEM_REBOOT_MODE) += nvmem-reboot-mode.o
> diff --git a/drivers/power/reset/nvmem-reboot-mode.c b/drivers/power/reset/nvmem-reboot-mode.c
> new file mode 100644
> index 000000000000..816cfdab16a7
> --- /dev/null
> +++ b/drivers/power/reset/nvmem-reboot-mode.c
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) Vaisala Oyj. All rights reserved.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot-mode.h>
> +
> +struct nvmem_reboot_mode {
> +	struct reboot_mode_driver reboot;
> +	struct nvmem_cell *cell;
> +};
> +
> +static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot,
> +				    unsigned int magic)
> +{
> +	int ret;
> +	struct nvmem_reboot_mode *nvmem_rbm;
> +
> +	nvmem_rbm = container_of(reboot, struct nvmem_reboot_mode, reboot);
> +
> +	ret = nvmem_cell_write(nvmem_rbm->cell, &magic, sizeof(magic));
> +	if (ret < 0)
> +		dev_err(reboot->dev, "update reboot mode bits failed\n");
> +
> +	return ret;
> +}
> +
> +static int nvmem_reboot_mode_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct nvmem_reboot_mode *nvmem_rbm;
> +
> +	nvmem_rbm = devm_kzalloc(&pdev->dev, sizeof(*nvmem_rbm), GFP_KERNEL);
> +	if (!nvmem_rbm)
> +		return -ENOMEM;
> +
> +	nvmem_rbm->reboot.dev = &pdev->dev;
> +	nvmem_rbm->reboot.write = nvmem_reboot_mode_write;
> +
> +	nvmem_rbm->cell = devm_nvmem_cell_get(&pdev->dev, "reboot-mode");
> +	if (IS_ERR(nvmem_rbm->cell)) {
> +		dev_err(&pdev->dev, "failed to get the nvmem cell reboot-mode\n");
> +		return PTR_ERR(nvmem_rbm->cell);
> +	}
> +
> +	ret = devm_reboot_mode_register(&pdev->dev, &nvmem_rbm->reboot);
> +	if (ret)
> +		dev_err(&pdev->dev, "can't register reboot mode\n");
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id nvmem_reboot_mode_of_match[] = {
> +	{ .compatible = "nvmem-reboot-mode" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, nvmem_reboot_mode_of_match);
> +
> +static struct platform_driver nvmem_reboot_mode_driver = {
> +	.probe = nvmem_reboot_mode_probe,
> +	.driver = {
> +		.name = "nvmem-reboot-mode",
> +		.of_match_table = nvmem_reboot_mode_of_match,
> +	},
> +};
> +module_platform_driver(nvmem_reboot_mode_driver);
> +
> +MODULE_AUTHOR("Nandor Han <nandor.han@vaisala.com>");
> +MODULE_DESCRIPTION("NVMEM reboot mode driver");
> +MODULE_LICENSE("GPL v2");

I suppose as of bf7fbeeae6db "GPL v2" is deprecated, before it was
often read as GPL v2 only. In both cases it makes sense to just
use "GPL". Otherwise the driver looks fine to me, but I'm waiting
for Rob's review of the DT bindings before merging this.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] power: reset: nvmem-reboot-mode: use NVMEM as reboot mode write interface

[Nandor Han]

On 4/18/19 12:56 AM, Sebastian Reichel wrote:
> On Thu, Apr 11, 2019 at 05:54:09AM +0000, Han Nandor wrote:
>> Add a new reboot mode write interface that is using an NVMEM cell
>> to store the reboot mode magic.
>>
>> Signed-off-by: Nandor Han <nandor.han@vaisala.com>
>> ---

<snip>

>> +module_platform_driver(nvmem_reboot_mode_driver);
>> +
>> +MODULE_AUTHOR("Nandor Han <nandor.han@vaisala.com>");
>> +MODULE_DESCRIPTION("NVMEM reboot mode driver");
>> +MODULE_LICENSE("GPL v2");
> 
> I suppose as of bf7fbeeae6db "GPL v2" is deprecated, before it was
> often read as GPL v2 only. In both cases it makes sense to just
> use "GPL". Otherwise the driver looks fine to me, but I'm waiting
> for Rob's review of the DT bindings before merging this.
> 
> -- Sebastian
> 

Thanks Sebastian.

Do you want me to send a new revision with license changed to "GPL"?..or 
will you change it when it's merged?

Nandor

Re: [PATCH v2 1/2] power: reset: nvmem-reboot-mode: use NVMEM as reboot mode write interface

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]

Hi,

On Thu, Apr 18, 2019 at 09:29:08AM +0300, Nandor Han wrote:
> On 4/18/19 12:56 AM, Sebastian Reichel wrote:
> > On Thu, Apr 11, 2019 at 05:54:09AM +0000, Han Nandor wrote:
> > > Add a new reboot mode write interface that is using an NVMEM cell
> > > to store the reboot mode magic.
> > > 
> > > Signed-off-by: Nandor Han <nandor.han@vaisala.com>
> > > ---
> 
> <snip>
> 
> > > +module_platform_driver(nvmem_reboot_mode_driver);
> > > +
> > > +MODULE_AUTHOR("Nandor Han <nandor.han@vaisala.com>");
> > > +MODULE_DESCRIPTION("NVMEM reboot mode driver");
> > > +MODULE_LICENSE("GPL v2");
> > 
> > I suppose as of bf7fbeeae6db "GPL v2" is deprecated, before it was
> > often read as GPL v2 only. In both cases it makes sense to just
> > use "GPL". Otherwise the driver looks fine to me, but I'm waiting
> > for Rob's review of the DT bindings before merging this.
> > 
> > -- Sebastian
> > 
> 
> Thanks Sebastian.
> 
> Do you want me to send a new revision with license changed to "GPL"?..or
> will you change it when it's merged?

I prefer to get a follow-up patch for things dealing with copyright/license
details.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]