[PATCH v2 0/4] add reboot mode driver

[PATCH v2 0/4] add reboot mode driver

[Andy Yan]

This driver parse the reboot commands like "reboot loader"
and "reboot recovery" to get a boot mode described in the
device tree , then call the corresponding  write interface
to store the boot mode magic value in some persistent storage
like special register or ram , which can be read by the bootloader
after system reboot.

This is commonly used on Android based devices, which in order
to reboot the device into fastboot or recovery mode.

Before this patch , I have try some hack on[0], and then found
John Stultz also doing the same work[1].

As John is busy these days, I go on with this work.

[0]https://patchwork.kernel.org/patch/7647751/
[1]https://patchwork.kernel.org/patch/7802391/

Changes in v2:
- move to dir drivers/power/reset/
- make syscon-reboot-mode a generic driver
- make this DT node as a subnode of PMU/PMUGRF

Changes in v1:
- fix the embarrassed compile warning
- correct the maskrom magic number
- check for the normal reboot
- correct the maskrom magic number
- use macro defined in rockchip_boot-mode.h for reboot-mode DT node

Andy Yan (4):
  dt-bindings: power: reset: add document for reboot-mode driver
  power: reset: add reboot mode driver
  ARM: dts: rockchip: add syscon-reboot-mode node
  ARM64: dts: rockchip: add syscon-reboot-mode DT node

 .../bindings/power/reset/reboot-mode.txt           |  41 +++++++++
 .../bindings/power/reset/syscon-reboot-mode.txt    |  52 +++++++++++
 arch/arm/boot/dts/rk3288.dtsi                      |  31 +++++++
 arch/arm/boot/dts/rk3xxx.dtsi                      |  33 ++++++-
 arch/arm64/boot/dts/rockchip/rk3368.dtsi           |  33 ++++++-
 drivers/power/reset/Kconfig                        |  16 ++++
 drivers/power/reset/Makefile                       |   2 +
 drivers/power/reset/reboot-mode.c                  | 100 +++++++++++++++++++++
 drivers/power/reset/reboot-mode.h                  |   6 ++
 drivers/power/reset/syscon-reboot-mode.c           |  62 +++++++++++++
 include/dt-bindings/soc/rockchip_boot-mode.h       |  30 +++++++
 11 files changed, 404 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt
 create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
 create mode 100644 drivers/power/reset/reboot-mode.c
 create mode 100644 drivers/power/reset/reboot-mode.h
 create mode 100644 drivers/power/reset/syscon-reboot-mode.c
 create mode 100644 include/dt-bindings/soc/rockchip_boot-mode.h

-- 
1.9.1



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/4] dt-bindings: power: reset: add document for reboot-mode driver

[Andy Yan]

add device tree binding document for reboot-mode driver

Signed-off-by: Andy Yan <andy.yan-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

---

Changes in v2: None
Changes in v1: None

 .../bindings/power/reset/reboot-mode.txt           | 41 +++++++++++++++++
 .../bindings/power/reset/syscon-reboot-mode.txt    | 52 ++++++++++++++++++++++
 2 files changed, 93 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt
 create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt

diff --git a/Documentation/devicetree/bindings/power/reset/reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
new file mode 100644
index 0000000..81d9f66
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
@@ -0,0 +1,41 @@
+Generic reboot mode core map driver
+
+This driver get reboot mode arguments and call the write
+interface to stores the magic value in special register
+or ram . Then the bootloader can read it and take different
+action according to the argument stored.
+
+Required properties:
+- compatible: only support "syscon-reboot-mode" now.
+
+Each mode is represented as a sub-node of reboot_mode:
+
+Subnode required properties:
+- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot".
+- loader,magic: magic number for the mode, this is vendor specific.
+
+Example:
+	reboot_mode {
+		compatible = "syscon-reboot-mode";
+		offset = <0x40>;
+
+		loader {
+			linux,mode = "loader";
+			loader,magic = <BOOT_LOADER>;
+		};
+
+		maskrom {
+			linux,mode = "maskrom";
+			loader,magic = <BOOT_MASKROM>;
+		};
+
+		recovery {
+			linux,mode = "recovery";
+			loader,magic = <BOOT_RECOVERY>;
+		};
+
+		fastboot {
+			linux,mode = "fastboot";
+			loader,magic = <BOOT_FASTBOOT>;
+		};
+        };
diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
new file mode 100644
index 0000000..6bce7dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
@@ -0,0 +1,52 @@
+SYSCON reboot mode driver
+
+This driver get reboot mode magic value form reboot-mode driver
+and stores it in a SYSCON mapped register. 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 "syscon", "simple-mfd"
+node.
+
+Required properties:
+- compatible: should be "syscon-reboot-mode"
+- offset: offset in the register map for the storage register (in bytes)
+
+The rest of the properties should follow the generic reboot-mode discription
+found in reboot-mode.txt
+
+Example:
+	pmu: pmu@20004000 {
+		compatible = "rockchip,rk3066-pmu", "syscon", "simple-mfd";
+		reg = <0x20004000 0x100>;
+
+		reboot-mode {
+			compatible = "syscon-reboot-mode";
+			offset = <0x40>;
+
+			normal {
+				linux,mode = "normal";
+				loader,magic = <BOOT_NORMAL>;
+			};
+
+			loader {
+				linux,mode = "loader";
+				loader,magic = <BOOT_LOADER>;
+			};
+
+			maskrom {
+				linux,mode = "maskrom";
+				loader,magic = <BOOT_MASKROM>;
+			};
+
+			recovery {
+				linux,mode = "recovery";
+				loader,magic = <BOOT_RECOVERY>;
+			};
+
+			fastboot {
+				linux,mode = "fastboot";
+				loader,magic = <BOOT_FASTBOOT>;
+			};
+		};
+	};
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/4] power: reset: add reboot mode driver

[Andy Yan]

This driver parse the reboot commands like "reboot loader"
and "reboot recovery" to get a boot mode described in the
device tree , then call the write interfae to store the boot
mode in some persistent storage  like special register or ram,
which can be read by the bootloader after system reboot, then
the bootloader can take different action according to the mode
stored.

This is commonly used on Android based devices, which in order
to reboot the device into fastboot or recovery mode.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

Changes in v2:
- move to dir drivers/power/reset/
- make syscon-reboot-mode a generic driver

Changes in v1:
- fix the embarrassed compile warning
- correct the maskrom magic number
- check for the normal reboot

 drivers/power/reset/Kconfig              |  16 +++++
 drivers/power/reset/Makefile             |   2 +
 drivers/power/reset/reboot-mode.c        | 100 +++++++++++++++++++++++++++++++
 drivers/power/reset/reboot-mode.h        |   6 ++
 drivers/power/reset/syscon-reboot-mode.c |  62 +++++++++++++++++++
 5 files changed, 186 insertions(+)
 create mode 100644 drivers/power/reset/reboot-mode.c
 create mode 100644 drivers/power/reset/reboot-mode.h
 create mode 100644 drivers/power/reset/syscon-reboot-mode.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index 1131cf7..2e6d873 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -173,5 +173,21 @@ config POWER_RESET_ZX
 	help
 	  Reboot support for ZTE SoCs.
 
+config REBOOT_MODE
+	tristate
+	depends on OF
+	help
+	 This driver will help to pass the system reboot mode
+	 to bootloader
+
+config SYSCON_REBOOT_MODE
+	bool "Generic SYSCON regmap 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 SYSCON mapped
+	 register, then the bootloader can read it to take different
+	 action according to the mode.
+
 endif
 
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index 096fa67..a63865b 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -20,3 +20,5 @@ obj-$(CONFIG_POWER_RESET_SYSCON) += syscon-reboot.o
 obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
 obj-$(CONFIG_POWER_RESET_RMOBILE) += rmobile-reset.o
 obj-$(CONFIG_POWER_RESET_ZX) += zx-reboot.o
+obj-$(CONFIG_REBOOT_MODE) += reboot-mode.o
+obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
new file mode 100644
index 0000000..92b7b4d
--- /dev/null
+++ b/drivers/power/reset/reboot-mode.c
@@ -0,0 +1,100 @@
+/*
+ * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/reboot.h>
+#include "reboot-mode.h"
+
+struct mode_info {
+	const char *mode;
+	int magic;
+	struct list_head list;
+};
+
+struct reboot_mode_driver {
+	struct list_head head;
+	int (*write)(int magic);
+	struct notifier_block reboot_notifier;
+};
+
+static int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
+				 const char *cmd)
+{
+	const char *normal = "normal";
+	int magic = 0;
+	struct mode_info *info;
+
+	if (!cmd)
+		cmd = normal;
+
+	list_for_each_entry(info, &reboot->head, list) {
+		if (!strcmp(info->mode, cmd)) {
+			magic = info->magic;
+			break;
+		}
+	}
+
+	return magic;
+}
+
+static int reboot_mode_notify(struct notifier_block *this,
+			      unsigned long mode, void *cmd)
+{
+	struct reboot_mode_driver *reboot;
+	int magic;
+
+	reboot = container_of(this, struct reboot_mode_driver, reboot_notifier);
+	magic = get_reboot_mode_magic(reboot, cmd);
+	if (magic)
+		reboot->write(magic);
+
+	return NOTIFY_DONE;
+}
+
+int reboot_mode_register(struct device *dev, int (*write)(int))
+{
+	struct reboot_mode_driver *reboot;
+	struct mode_info *info;
+	struct device_node *child;
+	int ret;
+
+	reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
+	if (!reboot)
+		return -ENOMEM;
+
+	reboot->write = write;
+	INIT_LIST_HEAD(&reboot->head);
+	for_each_child_of_node(dev->of_node, child) {
+		info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+		if (!info)
+			return -ENOMEM;
+		info->mode = of_get_property(child, "linux,mode", NULL);
+		if (of_property_read_u32(child, "loader,magic", &info->magic)) {
+			dev_err(dev, "reboot mode %s without magic number\n",
+				info->mode);
+			devm_kfree(dev, info);
+			continue;
+		}
+		list_add_tail(&info->list, &reboot->head);
+	}
+	reboot->reboot_notifier.notifier_call = reboot_mode_notify;
+	ret = register_reboot_notifier(&reboot->reboot_notifier);
+	if (ret)
+		dev_err(dev, "can't register reboot notifier\n");
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(reboot_mode_register);
+
+MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com");
+MODULE_DESCRIPTION("System reboot mode driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/power/reset/reboot-mode.h b/drivers/power/reset/reboot-mode.h
new file mode 100644
index 0000000..76646e7
--- /dev/null
+++ b/drivers/power/reset/reboot-mode.h
@@ -0,0 +1,6 @@
+#ifndef __REBOOT_MODE_H__
+#define __REBOOT_MODE_H__
+
+extern int reboot_mode_register(struct device *dev, int (*write)(int));
+
+#endif
diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
new file mode 100644
index 0000000..b9370f3
--- /dev/null
+++ b/drivers/power/reset/syscon-reboot-mode.c
@@ -0,0 +1,62 @@
+/*
+ * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include "reboot-mode.h"
+
+static struct regmap *map;
+static u32 offset;
+
+static int syscon_reboot_mode_write(int magic)
+{
+	regmap_write(map, offset, magic);
+
+	return 0;
+}
+
+static int syscon_reboot_mode_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	map = syscon_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+	if (of_property_read_u32(pdev->dev.of_node, "offset", &offset))
+		return -EINVAL;
+	ret = reboot_mode_register(&pdev->dev, syscon_reboot_mode_write);
+	if (ret)
+		dev_err(&pdev->dev, "can't register reboot mode\n");
+
+	return ret;
+}
+
+static const struct of_device_id syscon_reboot_mode_of_match[] = {
+	{ .compatible = "syscon-reboot-mode" },
+	{}
+};
+
+static struct platform_driver syscon_reboot_mode_driver = {
+	.probe = syscon_reboot_mode_probe,
+	.driver = {
+		.name = "syscon-reboot-mode",
+		.of_match_table = syscon_reboot_mode_of_match,
+	},
+};
+module_platform_driver(syscon_reboot_mode_driver);
+
+MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com");
+MODULE_DESCRIPTION("SYSCON reboot mode driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v2 3/4] ARM: dts: rockchip: add syscon-reboot-mode node

[Andy Yan]

Rockchip platform use a SYSCON mapped register store
the reboot mode magic value for bootloader to use when
system reboot. So add syscon-reboot-mode driver DT node
for rk3xxx,rk3288 platform

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

Changes in v2:
- make this node as a subnode of PMU

Changes in v1:
- correct the maskrom magic number
- use macro defined in rockchip_boot-mode.h for reboot-mode DT node

 arch/arm/boot/dts/rk3288.dtsi                | 31 ++++++++++++++++++++++++++
 arch/arm/boot/dts/rk3xxx.dtsi                | 33 +++++++++++++++++++++++++++-
 include/dt-bindings/soc/rockchip_boot-mode.h | 30 +++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/soc/rockchip_boot-mode.h

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 04ea209..4e49fb7 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -45,6 +45,7 @@
 #include <dt-bindings/clock/rk3288-cru.h>
 #include <dt-bindings/thermal/thermal.h>
 #include <dt-bindings/power/rk3288-power.h>
+#include <dt-bindings/soc/rockchip_boot-mode.h>
 #include "skeleton.dtsi"
 
 / {
@@ -712,6 +713,36 @@
 				clocks = <&cru ACLK_GPU>;
 			};
 		};
+
+		reboot-mode {
+			compatible = "syscon-reboot-mode";
+			offset = <0x94>;
+
+			normal {
+				linux,mode = "normal";
+				loader,magic = <BOOT_NORMAL>;
+			};
+
+			loader {
+				linux,mode = "loader";
+				loader,magic = <BOOT_LOADER>;
+			};
+
+			maskrom {
+				linux,mode = "maskrom";
+				loader,magic = <BOOT_MASKROM>;
+			};
+
+			recovery {
+				linux,mode = "recovery";
+				loader,magic = <BOOT_RECOVERY>;
+			};
+
+			fastboot {
+				linux,mode = "fastboot";
+				loader,magic = <BOOT_FASTBOOT>;
+			};
+		};
 	};
 
 	sgrf: syscon@ff740000 {
diff --git a/arch/arm/boot/dts/rk3xxx.dtsi b/arch/arm/boot/dts/rk3xxx.dtsi
index 4497d28..58af546 100644
--- a/arch/arm/boot/dts/rk3xxx.dtsi
+++ b/arch/arm/boot/dts/rk3xxx.dtsi
@@ -43,6 +43,7 @@
 
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/soc/rockchip_boot-mode.h>
 #include "skeleton.dtsi"
 
 / {
@@ -242,8 +243,38 @@
 	};
 
 	pmu: pmu@20004000 {
-		compatible = "rockchip,rk3066-pmu", "syscon";
+		compatible = "rockchip,rk3066-pmu", "syscon", "simple-mfd";
 		reg = <0x20004000 0x100>;
+
+		reboot-mode {
+			compatible = "syscon-reboot-mode";
+			offset = <0x40>;
+
+			normal {
+				linux,mode = "normal";
+				loader,magic = <BOOT_NORMAL>;
+			};
+
+			loader {
+				linux,mode = "loader";
+				loader,magic = <BOOT_LOADER>;
+			};
+
+			maskrom {
+				linux,mode = "maskrom";
+				loader,magic = <BOOT_MASKROM>;
+			};
+
+			recovery {
+				linux,mode = "recovery";
+				loader,magic = <BOOT_RECOVERY>;
+			};
+
+			fastboot {
+				linux,mode = "fastboot";
+				loader,magic = <BOOT_FASTBOOT>;
+			};
+		};
 	};
 
 	grf: grf@20008000 {
diff --git a/include/dt-bindings/soc/rockchip_boot-mode.h b/include/dt-bindings/soc/rockchip_boot-mode.h
new file mode 100644
index 0000000..eedf113
--- /dev/null
+++ b/include/dt-bindings/soc/rockchip_boot-mode.h
@@ -0,0 +1,30 @@
+#ifndef __ROCKCHIP_BOOT_MODE_H
+#define __ROCKCHIP_BOOT_MODE_H
+
+/*high 24 bits is tag, low 8 bits is type*/
+#define	REBOOT_FLAG		0x5242C300
+/* normal boot */
+#define	BOOT_NORMAL		(REBOOT_FLAG + 0)
+/* enter loader rockusb mode */
+#define	BOOT_LOADER		(REBOOT_FLAG + 1)
+/* enter maskrom rockusb mode */
+#define	BOOT_MASKROM		(0xEF08A53C)
+/* enter recovery */
+#define	BOOT_RECOVERY		(REBOOT_FLAG + 3)
+/* do not enter recover */
+#define	BOOT_NORECOVER		(REBOOT_FLAG + 4)
+/* boot second OS*/
+#define	BOOT_SECONDOS		(REBOOT_FLAG + 5)
+/* enter recover and wipe data. */
+#define	BOOT_WIPEDATA		(REBOOT_FLAG + 6)
+/* enter recover and wipe all data. */
+#define	BOOT_WIPEALL		(REBOOT_FLAG + 7)
+/* check firmware img with backup part*/
+#define	BOOT_CHECKIMG		(REBOOT_FLAG + 8)
+ /* enter fast boot mode */
+#define	BOOT_FASTBOOT		(REBOOT_FLAG + 9)
+#define	BOOT_SECUREBOOT_DISABLE	(REBOOT_FLAG + 10)
+/* enter charge mode */
+#define	BOOT_CHARGING		(REBOOT_FLAG + 11)
+
+#endif
-- 
1.9.1

[PATCH v2 4/4] ARM64: dts: rockchip: add syscon-reboot-mode DT node

[Andy Yan]

Add syscon-reboot-mode driver DT node for rk3368 platform

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

Changes in v2:
- make this node as a subnode of pmugrf

Changes in v1: None

 arch/arm64/boot/dts/rockchip/rk3368.dtsi | 33 +++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3368.dtsi b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
index cc093a4..20997a2 100644
--- a/arch/arm64/boot/dts/rockchip/rk3368.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
@@ -45,6 +45,7 @@
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/rockchip.h>
+#include <dt-bindings/soc/rockchip_boot-mode.h>
 
 / {
 	compatible = "rockchip,rk3368";
@@ -485,8 +486,38 @@
 	};
 
 	pmugrf: syscon@ff738000 {
-		compatible = "rockchip,rk3368-pmugrf", "syscon";
+		compatible = "rockchip,rk3368-pmugrf", "syscon", "simple-mfd";
 		reg = <0x0 0xff738000 0x0 0x1000>;
+
+		reboot-mode {
+			compatible = "syscon-reboot-mode";
+			offset = <0x200>;
+
+			normal {
+				linux,mode = "normal";
+				loader,magic = <BOOT_NORMAL>;
+			};
+
+			loader {
+				linux,mode = "loader";
+				loader,magic = <BOOT_LOADER>;
+			};
+
+			maskrom {
+				linux,mode = "maskrom";
+				loader,magic = <BOOT_MASKROM>;
+			};
+
+			recovery {
+				linux,mode = "recovery";
+				loader,magic = <BOOT_RECOVERY>;
+			};
+
+			fastboot {
+				linux,mode = "fastboot";
+				loader,magic = <BOOT_FASTBOOT>;
+			};
+		};
 	};
 
 	cru: clock-controller@ff760000 {
-- 
1.9.1

Re: [PATCH v2 0/4] add reboot mode driver

[Caesar Wang]

Hi ,

在 2016年01月12日 19:27, Andy Yan 写道:
> This driver parse the reboot commands like "reboot loader"
> and "reboot recovery" to get a boot mode described in the
> device tree , then call the corresponding  write interface
> to store the boot mode magic value in some persistent storage
> like special register or ram , which can be read by the bootloader
> after system reboot.
>
> This is commonly used on Android based devices, which in order
> to reboot the device into fastboot or recovery mode.
>
> Before this patch , I have try some hack on[0], and then found
> John Stultz also doing the same work[1].
>
> As John is busy these days, I go on with this work.
>
> [0]https://patchwork.kernel.org/patch/7647751/
> [1]https://patchwork.kernel.org/patch/7802391/
>
> Changes in v2:
> - move to dir drivers/power/reset/
> - make syscon-reboot-mode a generic driver
> - make this DT node as a subnode of PMU/PMUGRF
>
> Changes in v1:
> - fix the embarrassed compile warning
> - correct the maskrom magic number
> - check for the normal reboot
> - correct the maskrom magic number
> - use macro defined in rockchip_boot-mode.h for reboot-mode DT node
>
> Andy Yan (4):
>    dt-bindings: power: reset: add document for reboot-mode driver
>    power: reset: add reboot mode driver
>    ARM: dts: rockchip: add syscon-reboot-mode node
>    ARM64: dts: rockchip: add syscon-reboot-mode DT node

This series patches are verified by my github based on kernel 4.4 release.
https://github.com/Caesar-github/rockchip/commits/kylin-develop4.4

reboot loader, reboot recovery, reboot fastboot..... these are okay for 
rk3288, rk3368. rk3036 Socs on my hand board.
(Note: the loader need that jeffy send patches to support uboot on 
upstream,  that's also okay if you are using the RK loader from Rockchip)

I just send the patch to support the rk3036 dts. 
<https://patchwork.kernel.org/patch/8021581/>

So feel free add my test tags:

Tested-by: Caesar Wang <wxt@rock-chips.com>

>
>   .../bindings/power/reset/reboot-mode.txt           |  41 +++++++++
>   .../bindings/power/reset/syscon-reboot-mode.txt    |  52 +++++++++++
>   arch/arm/boot/dts/rk3288.dtsi                      |  31 +++++++
>   arch/arm/boot/dts/rk3xxx.dtsi                      |  33 ++++++-
>   arch/arm64/boot/dts/rockchip/rk3368.dtsi           |  33 ++++++-
>   drivers/power/reset/Kconfig                        |  16 ++++
>   drivers/power/reset/Makefile                       |   2 +
>   drivers/power/reset/reboot-mode.c                  | 100 +++++++++++++++++++++
>   drivers/power/reset/reboot-mode.h                  |   6 ++
>   drivers/power/reset/syscon-reboot-mode.c           |  62 +++++++++++++
>   include/dt-bindings/soc/rockchip_boot-mode.h       |  30 +++++++
>   11 files changed, 404 insertions(+), 2 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>   create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
>   create mode 100644 drivers/power/reset/reboot-mode.c
>   create mode 100644 drivers/power/reset/reboot-mode.h
>   create mode 100644 drivers/power/reset/syscon-reboot-mode.c
>   create mode 100644 include/dt-bindings/soc/rockchip_boot-mode.h
>


-- 
Thanks,
Caesar

Re: [PATCH v2 2/4] power: reset: add reboot mode driver

[John Stultz]

On Tue, Jan 12, 2016 at 3:31 AM, Andy Yan <andy.yan@rock-chips.com> wrote:
> This driver parse the reboot commands like "reboot loader"
> and "reboot recovery" to get a boot mode described in the
> device tree , then call the write interfae to store the boot
> mode in some persistent storage  like special register or ram,
> which can be read by the bootloader after system reboot, then
> the bootloader can take different action according to the mode
> stored.
>
> This is commonly used on Android based devices, which in order
> to reboot the device into fastboot or recovery mode.
>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>


Hey Andy!
  Thanks for keeping this work going! I've just successfully reworked
my Nexus7 tree to use your implementation (using the syscon version,
setting up a syscon for IMEM as Bjorn had requested earlier).

All is working well so far!

The one thing I was working on supporting with my own version that
seems to be missing here are for devices that use string based codes,
rather then magic numbers.

This was mostly a theoretical issue. I think the Galaxy Nexus used it,
and when I was looking at some of the HTC devices, they support a text
based reason along with the magic code, but at least in some
implementations the text mode isn't used, so I suspect there its just
for extra debugging.  So this may not be critical to solve until
someone tries to add support for such a device.

Anyway, I'm going to look at porting this to the HiKey board next
(which just uses reserved ram, not syscon), so I'll try to do a an
SRAM driver implementaiton to see how that goes.

thanks!
-john

Re: [PATCH v2 1/4] dt-bindings: power: reset: add document for reboot-mode driver

[John Stultz]

On Tue, Jan 12, 2016 at 3:29 AM, Andy Yan <andy.yan@rock-chips.com> wrote:
> add device tree binding document for reboot-mode driver
>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>
> ---
>
> Changes in v2: None
> Changes in v1: None
>
>  .../bindings/power/reset/reboot-mode.txt           | 41 +++++++++++++++++
>  .../bindings/power/reset/syscon-reboot-mode.txt    | 52 ++++++++++++++++++++++
>  2 files changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
>
> diff --git a/Documentation/devicetree/bindings/power/reset/reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> new file mode 100644
> index 0000000..81d9f66
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> @@ -0,0 +1,41 @@
> +Generic reboot mode core map driver
> +
> +This driver get reboot mode arguments and call the write
> +interface to stores the magic value in special register
> +or ram . Then the bootloader can read it and take different
> +action according to the argument stored.
> +
> +Required properties:
> +- compatible: only support "syscon-reboot-mode" now.
> +
> +Each mode is represented as a sub-node of reboot_mode:
> +
> +Subnode required properties:
> +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot".
> +- loader,magic: magic number for the mode, this is vendor specific.
> +
> +Example:
> +       reboot_mode {
> +               compatible = "syscon-reboot-mode";
> +               offset = <0x40>;
> +
> +               loader {
> +                       linux,mode = "loader";
> +                       loader,magic = <BOOT_LOADER>;
> +               };
> +
> +               maskrom {
> +                       linux,mode = "maskrom";
> +                       loader,magic = <BOOT_MASKROM>;
> +               };
> +
> +               recovery {
> +                       linux,mode = "recovery";
> +                       loader,magic = <BOOT_RECOVERY>;
> +               };
> +
> +               fastboot {
> +                       linux,mode = "fastboot";
> +                       loader,magic = <BOOT_FASTBOOT>;
> +               };
> +        };


So one minor thought here. While the commands are somewhat vendor
specific, would it be a good idea for the example commands to match
the common commands on Android devices?  For example, usually
"bootloader" is what gets you into fastboot mode on nexus devices.

thanks
-john

Re: [PATCH v2 1/4] dt-bindings: power: reset: add document for reboot-mode driver

[Andy Yan]

Hi John:

On 2016年01月16日 06:41, John Stultz wrote:
> On Tue, Jan 12, 2016 at 3:29 AM, Andy Yan <andy.yan-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>> add device tree binding document for reboot-mode driver
>>
>> Signed-off-by: Andy Yan <andy.yan-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>
>> ---
>>
>> Changes in v2: None
>> Changes in v1: None
>>
>>   .../bindings/power/reset/reboot-mode.txt           | 41 +++++++++++++++++
>>   .../bindings/power/reset/syscon-reboot-mode.txt    | 52 ++++++++++++++++++++++
>>   2 files changed, 93 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>>   create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> new file mode 100644
>> index 0000000..81d9f66
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> @@ -0,0 +1,41 @@
>> +Generic reboot mode core map driver
>> +
>> +This driver get reboot mode arguments and call the write
>> +interface to stores the magic value in special register
>> +or ram . Then the bootloader can read it and take different
>> +action according to the argument stored.
>> +
>> +Required properties:
>> +- compatible: only support "syscon-reboot-mode" now.
>> +
>> +Each mode is represented as a sub-node of reboot_mode:
>> +
>> +Subnode required properties:
>> +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot".
>> +- loader,magic: magic number for the mode, this is vendor specific.
>> +
>> +Example:
>> +       reboot_mode {
>> +               compatible = "syscon-reboot-mode";
>> +               offset = <0x40>;
>> +
>> +               loader {
>> +                       linux,mode = "loader";
>> +                       loader,magic = <BOOT_LOADER>;
>> +               };
>> +
>> +               maskrom {
>> +                       linux,mode = "maskrom";
>> +                       loader,magic = <BOOT_MASKROM>;
>> +               };
>> +
>> +               recovery {
>> +                       linux,mode = "recovery";
>> +                       loader,magic = <BOOT_RECOVERY>;
>> +               };
>> +
>> +               fastboot {
>> +                       linux,mode = "fastboot";
>> +                       loader,magic = <BOOT_FASTBOOT>;
>> +               };
>> +        };
>
> So one minor thought here. While the commands are somewhat vendor
> specific, would it be a good idea for the example commands to match
> the common commands on Android devices?  For example, usually
> "bootloader" is what gets you into fastboot mode on nexus devices.
>
> thanks
> -john
>
>
>
>

   When I run the "adb help" , I got the following message:
   adb reboot [bootloader|recovery] - reboots the device, optionally 
into the bootloader or recovery program
   adb reboot-bootloader        - reboots the device into the bootloader

   It  only says "adb reboot-bootloader" will reboot the device into 
bootloader, not specific to fastboot.

   I add @Simon from Google here, maybe he can give some suggestions.



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] power: reset: add reboot mode driver

[Andy Yan]

Hi John:

On 2016年01月16日 04:27, John Stultz wrote:
> On Tue, Jan 12, 2016 at 3:31 AM, Andy Yan <andy.yan@rock-chips.com> wrote:
>> This driver parse the reboot commands like "reboot loader"
>> and "reboot recovery" to get a boot mode described in the
>> device tree , then call the write interfae to store the boot
>> mode in some persistent storage  like special register or ram,
>> which can be read by the bootloader after system reboot, then
>> the bootloader can take different action according to the mode
>> stored.
>>
>> This is commonly used on Android based devices, which in order
>> to reboot the device into fastboot or recovery mode.
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>
> Hey Andy!
>    Thanks for keeping this work going! I've just successfully reworked
> my Nexus7 tree to use your implementation (using the syscon version,
> setting up a syscon for IMEM as Bjorn had requested earlier).
>
> All is working well so far!
>
> The one thing I was working on supporting with my own version that
> seems to be missing here are for devices that use string based codes,
> rather then magic numbers.
>
> This was mostly a theoretical issue. I think the Galaxy Nexus used it,
> and when I was looking at some of the HTC devices, they support a text
> based reason along with the magic code, but at least in some
> implementations the text mode isn't used, so I suspect there its just
> for extra debugging.  So this may not be critical to solve until
> someone tries to add support for such a device.
>
> Anyway, I'm going to look at porting this to the HiKey board next
> (which just uses reserved ram, not syscon), so I'll try to do a an
> SRAM driver implementaiton to see how that goes.
>
> thanks!
> -john
>
>
>
>

  Very glad to hear news from you. And welcome your SRAM based driver 
implementation.

Re: [PATCH v2 1/4] dt-bindings: power: reset: add document for reboot-mode driver

[Rob Herring]

On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote:
> add device tree binding document for reboot-mode driver
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> 
> ---
> 
> Changes in v2: None
> Changes in v1: None
> 
>  .../bindings/power/reset/reboot-mode.txt           | 41 +++++++++++++++++
>  .../bindings/power/reset/syscon-reboot-mode.txt    | 52 ++++++++++++++++++++++
>  2 files changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> new file mode 100644
> index 0000000..81d9f66
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> @@ -0,0 +1,41 @@
> +Generic reboot mode core map driver
> +
> +This driver get reboot mode arguments and call the write
> +interface to stores the magic value in special register
> +or ram . Then the bootloader can read it and take different
> +action according to the argument stored.
> +
> +Required properties:
> +- compatible: only support "syscon-reboot-mode" now.
> +
> +Each mode is represented as a sub-node of reboot_mode:
> +
> +Subnode required properties:
> +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot".
> +- loader,magic: magic number for the mode, this is vendor specific.
> +
> +Example:
> +	reboot_mode {

reboot-mode instead please.

> +		compatible = "syscon-reboot-mode";
> +		offset = <0x40>;

This doc by itself is a little confusing. For example, is a child of the 
syscon node? I would remove offset (and perhaps compatible) from this 
example.

> +
> +		loader {
> +			linux,mode = "loader";
> +			loader,magic = <BOOT_LOADER>;
> +		};

Sorry, my previous suggestion was not clear. I'm suggesting get rid of 
the subnodes and just do properties like this:

loader = <BOOT_LOADER>;
maskrom = <BOOT_MASKROM>;

That's the same amount of information unless node names and linux,mode 
values are going to diverge. Do they need to? I can't see a reason.


We need to be clear what loader means. More specifically, it is boot 
into bootloader shell. 

> +
> +		maskrom {

In theory, the bootrom could have multiple modes. This typically means a 
USB download mode. So perhaps a more precise name would be 
"rom-download". 

In chips I'm familiar with the bootrom mode is selected via a different 
mechanism than the secondary bootloader modes, but I suppose the same 
mechanism could be used.

> +			linux,mode = "maskrom";
> +			loader,magic = <BOOT_MASKROM>;
> +		};
> +
> +		recovery {
> +			linux,mode = "recovery";
> +			loader,magic = <BOOT_RECOVERY>;
> +		};
> +
> +		fastboot {
> +			linux,mode = "fastboot";
> +			loader,magic = <BOOT_FASTBOOT>;
> +		};

Re: [PATCH v2 1/4] dt-bindings: power: reset: add document for reboot-mode driver

[John Stultz]

On Wed, Jan 20, 2016 at 10:28 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote:
>> add device tree binding document for reboot-mode driver
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>
>> ---
>>
>> Changes in v2: None
>> Changes in v1: None
>>
>>  .../bindings/power/reset/reboot-mode.txt           | 41 +++++++++++++++++
>>  .../bindings/power/reset/syscon-reboot-mode.txt    | 52 ++++++++++++++++++++++
>>  2 files changed, 93 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> new file mode 100644
>> index 0000000..81d9f66
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> @@ -0,0 +1,41 @@
>> +Generic reboot mode core map driver
>> +
>> +This driver get reboot mode arguments and call the write
>> +interface to stores the magic value in special register
>> +or ram . Then the bootloader can read it and take different
>> +action according to the argument stored.
>> +
>> +Required properties:
>> +- compatible: only support "syscon-reboot-mode" now.
>> +
>> +Each mode is represented as a sub-node of reboot_mode:
>> +
>> +Subnode required properties:
>> +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot".
>> +- loader,magic: magic number for the mode, this is vendor specific.
>> +
>> +Example:
>> +     reboot_mode {
>
> reboot-mode instead please.
>
>> +             compatible = "syscon-reboot-mode";
>> +             offset = <0x40>;
>
> This doc by itself is a little confusing. For example, is a child of the
> syscon node? I would remove offset (and perhaps compatible) from this
> example.
>
>> +
>> +             loader {
>> +                     linux,mode = "loader";
>> +                     loader,magic = <BOOT_LOADER>;
>> +             };
>
> Sorry, my previous suggestion was not clear. I'm suggesting get rid of
> the subnodes and just do properties like this:
>
> loader = <BOOT_LOADER>;
> maskrom = <BOOT_MASKROM>;
>
> That's the same amount of information unless node names and linux,mode
> values are going to diverge. Do they need to? I can't see a reason.


It seems like devices already have a number of various different
vendor specific commands.  So this sort of flexibility helps us adapt
the driver to different hardware/system environments (which may use
different conventions then what Android commonly uses).

Unless I'm misunderstanding you and you're instead suggesting we can
dynamically parse the command mode from the node name?

That said, I agree we should try to push vendors to using the same
conventions when they are providing the same functionality. But I'm
not sure we should enforce that by making vendors introduce a new dt
binding for every new reboot command they want to implement.

thanks
-john

Re: [PATCH v2 1/4] dt-bindings: power: reset: add document for reboot-mode driver

[Rob Herring]

On Wed, Jan 20, 2016 at 12:47 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Jan 20, 2016 at 10:28 AM, Rob Herring <robh@kernel.org> wrote:
>> On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote:
>>> add device tree binding document for reboot-mode driver
>>>
>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>>
>>> ---
>>>
>>> Changes in v2: None
>>> Changes in v1: None
>>>
>>>  .../bindings/power/reset/reboot-mode.txt           | 41 +++++++++++++++++
>>>  .../bindings/power/reset/syscon-reboot-mode.txt    | 52 ++++++++++++++++++++++
>>>  2 files changed, 93 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>>>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/reset/reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>>> new file mode 100644
>>> index 0000000..81d9f66
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>>> @@ -0,0 +1,41 @@
>>> +Generic reboot mode core map driver
>>> +
>>> +This driver get reboot mode arguments and call the write
>>> +interface to stores the magic value in special register
>>> +or ram . Then the bootloader can read it and take different
>>> +action according to the argument stored.
>>> +
>>> +Required properties:
>>> +- compatible: only support "syscon-reboot-mode" now.
>>> +
>>> +Each mode is represented as a sub-node of reboot_mode:
>>> +
>>> +Subnode required properties:
>>> +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot".
>>> +- loader,magic: magic number for the mode, this is vendor specific.
>>> +
>>> +Example:
>>> +     reboot_mode {
>>
>> reboot-mode instead please.
>>
>>> +             compatible = "syscon-reboot-mode";
>>> +             offset = <0x40>;
>>
>> This doc by itself is a little confusing. For example, is a child of the
>> syscon node? I would remove offset (and perhaps compatible) from this
>> example.
>>
>>> +
>>> +             loader {
>>> +                     linux,mode = "loader";
>>> +                     loader,magic = <BOOT_LOADER>;
>>> +             };
>>
>> Sorry, my previous suggestion was not clear. I'm suggesting get rid of
>> the subnodes and just do properties like this:
>>
>> loader = <BOOT_LOADER>;
>> maskrom = <BOOT_MASKROM>;
>>
>> That's the same amount of information unless node names and linux,mode
>> values are going to diverge. Do they need to? I can't see a reason.
>
>
> It seems like devices already have a number of various different
> vendor specific commands.  So this sort of flexibility helps us adapt
> the driver to different hardware/system environments (which may use
> different conventions then what Android commonly uses).
>
> Unless I'm misunderstanding you and you're instead suggesting we can
> dynamically parse the command mode from the node name?

Yes, except it would be the property name. The driver could either
iterate over the properties of the reboot-mode node or lookup the
property when it has the reboot reason string. We generally like to
parse everything up front at probe, but the latter would be somewhat
easier to implement. Perhaps we would want to prefix nodes with
"mode-" or something.

Vendors can certainly still add their own (e.g.
vendor,halt-and-catch-fire = <0xdead>).

The question here really is will we ever need additional data for each
mode (in the DT) besides "magic". We can do some amount of extending
the property like allowing more that single 32-bit value. We can't mix
strings and integers though. If we don't need that, then lets go with
the more concise approach. This isn't really an evolving area.
Selecting boot mode/device has existed for a long time on PCs (it is
fixed list in the IPMI spec for example). So I can't imagine that we'd
have to extend this, but I'd like to hear thoughts on it. You had
mentioned the Galaxy Nexus having a value and a string, but I'd find
it surprising if the bootloader actually uses the string.

> That said, I agree we should try to push vendors to using the same
> conventions when they are providing the same functionality. But I'm
> not sure we should enforce that by making vendors introduce a new dt
> binding for every new reboot command they want to implement.

Either way they need to document it. It's either a new linux,mode
value or a new property name. Vendors are free to add whatever they
want, but for upstream it has to go thru review.

Rob

Re: [PATCH v2 1/4] dt-bindings: power: reset: add document for reboot-mode driver

[John Stultz]

On Wed, Jan 20, 2016 at 11:53 AM, Rob Herring <robh@kernel.org> wrote:
> On Wed, Jan 20, 2016 at 12:47 PM, John Stultz <john.stultz@linaro.org> wrote:
> The question here really is will we ever need additional data for each
> mode (in the DT) besides "magic". We can do some amount of extending
> the property like allowing more that single 32-bit value. We can't mix
> strings and integers though. If we don't need that, then lets go with
> the more concise approach. This isn't really an evolving area.
> Selecting boot mode/device has existed for a long time on PCs (it is
> fixed list in the IPMI spec for example). So I can't imagine that we'd
> have to extend this, but I'd like to hear thoughts on it. You had
> mentioned the Galaxy Nexus having a value and a string, but I'd find
> it surprising if the bootloader actually uses the string.

So I've been told the galaxy nexus and possibly other older TI
hardware used only a string.

And the HTC implementations I've seen have code for both a string and
command to be placed, but usually don't actually copy anything to the
string address (which suggest its not used by the bootloader, as you
note).

Either way, we'd have to extend or implement a new driver, but that's
a bridge I think we can cross later when we see such hardware support
upstream (rather then try to create a super driver that handles
everything a creative firmware developer could come up with).

thanks
-john

Re: [PATCH v2 1/4] dt-bindings: power: reset: add document for reboot-mode driver

[Andy Yan]

Hi Rob:
    thanks for your review.
On 2016年01月21日 02:28, Rob Herring wrote:
> On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote:
>> add device tree binding document for reboot-mode driver
>>
>> Signed-off-by: Andy Yan <andy.yan-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>
>> ---
>>
>> Changes in v2: None
>> Changes in v1: None
>>
>>   .../bindings/power/reset/reboot-mode.txt           | 41 +++++++++++++++++
>>   .../bindings/power/reset/syscon-reboot-mode.txt    | 52 ++++++++++++++++++++++
>>   2 files changed, 93 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>>   create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> new file mode 100644
>> index 0000000..81d9f66
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> @@ -0,0 +1,41 @@
>> +Generic reboot mode core map driver
>> +
>> +This driver get reboot mode arguments and call the write
>> +interface to stores the magic value in special register
>> +or ram . Then the bootloader can read it and take different
>> +action according to the argument stored.
>> +
>> +Required properties:
>> +- compatible: only support "syscon-reboot-mode" now.
>> +
>> +Each mode is represented as a sub-node of reboot_mode:
>> +
>> +Subnode required properties:
>> +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot".
>> +- loader,magic: magic number for the mode, this is vendor specific.
>> +
>> +Example:
>> +	reboot_mode {
> reboot-mode instead please.
>

     Sorry, I have already correct it in DT file, forget it here. It 
will be changed in next version.

>> +		compatible = "syscon-reboot-mode";
>> +		offset = <0x40>;
> This doc by itself is a little confusing. For example, is a child of the
> syscon node? I would remove offset (and perhaps compatible) from this
> example.

    Yes, is a child of a syscon mapped node. For example, Rockchip 
platform use a register of PMU(rk3066/rk3288) or GRF(rk3036), PMU and 
GRF are aleady mapped by syscon.
    offset and compatible are used by write interface driver like 
syscon-reboot-mode.c. If you don't like it appear in the core map doc, I 
will move it to the syscon-reboot-mode.txt?
>> +
>> +		loader {
>> +			linux,mode = "loader";
>> +			loader,magic = <BOOT_LOADER>;
>> +		};
> Sorry, my previous suggestion was not clear. I'm suggesting get rid of
> the subnodes and just do properties like this:
>
> loader = <BOOT_LOADER>;
> maskrom = <BOOT_MASKROM>;
>
> That's the same amount of information unless node names and linux,mode
> values are going to diverge. Do they need to? I can't see a reason.

     Because the command"linux,mode" and value"loader,magic" is vendor 
specific. I don't know what commands and how many mode other platform 
will use. So as John says in his reply, this sort of flexibility help us 
adapt the driver to different hardware/system environments.
>
> We need to be clear what loader means. More specifically, it is boot
> into bootloader shell.
     Actually, Rockchip platform will reboot into a bootloader download 
mode with this command. This mode can download faster than maskrom 
download mode.
>> +
>> +		maskrom {
> In theory, the bootrom could have multiple modes. This typically means a
> USB download mode. So perhaps a more precise name would be
> "rom-download".
>
> In chips I'm familiar with the bootrom mode is selected via a different
> mechanism than the secondary bootloader modes, but I suppose the same
> mechanism could be used.
    Yes , they use the same mechanism.
>> +			linux,mode = "maskrom";
>> +			loader,magic = <BOOT_MASKROM>;
>> +		};
>> +
>> +		recovery {
>> +			linux,mode = "recovery";
>> +			loader,magic = <BOOT_RECOVERY>;
>> +		};
>> +
>> +		fastboot {
>> +			linux,mode = "fastboot";
>> +			loader,magic = <BOOT_FASTBOOT>;
>> +		};
>
>
>


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] power: reset: add reboot mode driver

[Matthias Brugger]

On 12/01/16 12:31, Andy Yan wrote:
> This driver parse the reboot commands like "reboot loader"
> and "reboot recovery" to get a boot mode described in the
> device tree , then call the write interfae to store the boot
> mode in some persistent storage  like special register or ram,
> which can be read by the bootloader after system reboot, then
> the bootloader can take different action according to the mode
> stored.
>
> This is commonly used on Android based devices, which in order
> to reboot the device into fastboot or recovery mode.
>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>
> ---

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

>
> Changes in v2:
> - move to dir drivers/power/reset/
> - make syscon-reboot-mode a generic driver
>
> Changes in v1:
> - fix the embarrassed compile warning
> - correct the maskrom magic number
> - check for the normal reboot
>
>   drivers/power/reset/Kconfig              |  16 +++++
>   drivers/power/reset/Makefile             |   2 +
>   drivers/power/reset/reboot-mode.c        | 100 +++++++++++++++++++++++++++++++
>   drivers/power/reset/reboot-mode.h        |   6 ++
>   drivers/power/reset/syscon-reboot-mode.c |  62 +++++++++++++++++++
>   5 files changed, 186 insertions(+)
>   create mode 100644 drivers/power/reset/reboot-mode.c
>   create mode 100644 drivers/power/reset/reboot-mode.h
>   create mode 100644 drivers/power/reset/syscon-reboot-mode.c
>
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index 1131cf7..2e6d873 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -173,5 +173,21 @@ config POWER_RESET_ZX
>   	help
>   	  Reboot support for ZTE SoCs.
>
> +config REBOOT_MODE
> +	tristate
> +	depends on OF
> +	help
> +	 This driver will help to pass the system reboot mode
> +	 to bootloader
> +
> +config SYSCON_REBOOT_MODE
> +	bool "Generic SYSCON regmap 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 SYSCON mapped
> +	 register, then the bootloader can read it to take different
> +	 action according to the mode.
> +
>   endif
>
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index 096fa67..a63865b 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -20,3 +20,5 @@ obj-$(CONFIG_POWER_RESET_SYSCON) += syscon-reboot.o
>   obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
>   obj-$(CONFIG_POWER_RESET_RMOBILE) += rmobile-reset.o
>   obj-$(CONFIG_POWER_RESET_ZX) += zx-reboot.o
> +obj-$(CONFIG_REBOOT_MODE) += reboot-mode.o
> +obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> new file mode 100644
> index 0000000..92b7b4d
> --- /dev/null
> +++ b/drivers/power/reset/reboot-mode.c
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/reboot.h>
> +#include "reboot-mode.h"
> +
> +struct mode_info {
> +	const char *mode;
> +	int magic;
> +	struct list_head list;
> +};
> +
> +struct reboot_mode_driver {
> +	struct list_head head;
> +	int (*write)(int magic);
> +	struct notifier_block reboot_notifier;
> +};
> +
> +static int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
> +				 const char *cmd)
> +{
> +	const char *normal = "normal";
> +	int magic = 0;
> +	struct mode_info *info;
> +
> +	if (!cmd)
> +		cmd = normal;
> +
> +	list_for_each_entry(info, &reboot->head, list) {
> +		if (!strcmp(info->mode, cmd)) {
> +			magic = info->magic;
> +			break;
> +		}
> +	}
> +
> +	return magic;
> +}
> +
> +static int reboot_mode_notify(struct notifier_block *this,
> +			      unsigned long mode, void *cmd)
> +{
> +	struct reboot_mode_driver *reboot;
> +	int magic;
> +
> +	reboot = container_of(this, struct reboot_mode_driver, reboot_notifier);
> +	magic = get_reboot_mode_magic(reboot, cmd);
> +	if (magic)
> +		reboot->write(magic);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +int reboot_mode_register(struct device *dev, int (*write)(int))
> +{
> +	struct reboot_mode_driver *reboot;
> +	struct mode_info *info;
> +	struct device_node *child;
> +	int ret;
> +
> +	reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
> +	if (!reboot)
> +		return -ENOMEM;
> +
> +	reboot->write = write;
> +	INIT_LIST_HEAD(&reboot->head);
> +	for_each_child_of_node(dev->of_node, child) {
> +		info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +		if (!info)
> +			return -ENOMEM;
> +		info->mode = of_get_property(child, "linux,mode", NULL);
> +		if (of_property_read_u32(child, "loader,magic", &info->magic)) {
> +			dev_err(dev, "reboot mode %s without magic number\n",
> +				info->mode);
> +			devm_kfree(dev, info);
> +			continue;
> +		}
> +		list_add_tail(&info->list, &reboot->head);
> +	}
> +	reboot->reboot_notifier.notifier_call = reboot_mode_notify;
> +	ret = register_reboot_notifier(&reboot->reboot_notifier);
> +	if (ret)
> +		dev_err(dev, "can't register reboot notifier\n");
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(reboot_mode_register);
> +
> +MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com");
> +MODULE_DESCRIPTION("System reboot mode driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/power/reset/reboot-mode.h b/drivers/power/reset/reboot-mode.h
> new file mode 100644
> index 0000000..76646e7
> --- /dev/null
> +++ b/drivers/power/reset/reboot-mode.h
> @@ -0,0 +1,6 @@
> +#ifndef __REBOOT_MODE_H__
> +#define __REBOOT_MODE_H__
> +
> +extern int reboot_mode_register(struct device *dev, int (*write)(int));
> +
> +#endif
> diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
> new file mode 100644
> index 0000000..b9370f3
> --- /dev/null
> +++ b/drivers/power/reset/syscon-reboot-mode.c
> @@ -0,0 +1,62 @@
> +/*
> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include "reboot-mode.h"
> +
> +static struct regmap *map;
> +static u32 offset;
> +
> +static int syscon_reboot_mode_write(int magic)
> +{
> +	regmap_write(map, offset, magic);
> +
> +	return 0;
> +}
> +
> +static int syscon_reboot_mode_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +	if (of_property_read_u32(pdev->dev.of_node, "offset", &offset))
> +		return -EINVAL;
> +	ret = reboot_mode_register(&pdev->dev, syscon_reboot_mode_write);
> +	if (ret)
> +		dev_err(&pdev->dev, "can't register reboot mode\n");
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id syscon_reboot_mode_of_match[] = {
> +	{ .compatible = "syscon-reboot-mode" },
> +	{}
> +};
> +
> +static struct platform_driver syscon_reboot_mode_driver = {
> +	.probe = syscon_reboot_mode_probe,
> +	.driver = {
> +		.name = "syscon-reboot-mode",
> +		.of_match_table = syscon_reboot_mode_of_match,
> +	},
> +};
> +module_platform_driver(syscon_reboot_mode_driver);
> +
> +MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com");
> +MODULE_DESCRIPTION("SYSCON reboot mode driver");
> +MODULE_LICENSE("GPL v2");
>

Re: [PATCH v2 3/4] ARM: dts: rockchip: add syscon-reboot-mode node

[Matthias Brugger]

On 12/01/16 12:32, Andy Yan wrote:
> Rockchip platform use a SYSCON mapped register store
> the reboot mode magic value for bootloader to use when
> system reboot. So add syscon-reboot-mode driver DT node
> for rk3xxx,rk3288 platform
>
> Signed-off-by: Andy Yan <andy.yan-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>
> ---

Reviewed-by: Matthias Brugger <matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

>
> Changes in v2:
> - make this node as a subnode of PMU
>
> Changes in v1:
> - correct the maskrom magic number
> - use macro defined in rockchip_boot-mode.h for reboot-mode DT node
>
>   arch/arm/boot/dts/rk3288.dtsi                | 31 ++++++++++++++++++++++++++
>   arch/arm/boot/dts/rk3xxx.dtsi                | 33 +++++++++++++++++++++++++++-
>   include/dt-bindings/soc/rockchip_boot-mode.h | 30 +++++++++++++++++++++++++
>   3 files changed, 93 insertions(+), 1 deletion(-)
>   create mode 100644 include/dt-bindings/soc/rockchip_boot-mode.h
>
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 04ea209..4e49fb7 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -45,6 +45,7 @@
>   #include <dt-bindings/clock/rk3288-cru.h>
>   #include <dt-bindings/thermal/thermal.h>
>   #include <dt-bindings/power/rk3288-power.h>
> +#include <dt-bindings/soc/rockchip_boot-mode.h>
>   #include "skeleton.dtsi"
>
>   / {
> @@ -712,6 +713,36 @@
>   				clocks = <&cru ACLK_GPU>;
>   			};
>   		};
> +
> +		reboot-mode {
> +			compatible = "syscon-reboot-mode";
> +			offset = <0x94>;
> +
> +			normal {
> +				linux,mode = "normal";
> +				loader,magic = <BOOT_NORMAL>;
> +			};
> +
> +			loader {
> +				linux,mode = "loader";
> +				loader,magic = <BOOT_LOADER>;
> +			};
> +
> +			maskrom {
> +				linux,mode = "maskrom";
> +				loader,magic = <BOOT_MASKROM>;
> +			};
> +
> +			recovery {
> +				linux,mode = "recovery";
> +				loader,magic = <BOOT_RECOVERY>;
> +			};
> +
> +			fastboot {
> +				linux,mode = "fastboot";
> +				loader,magic = <BOOT_FASTBOOT>;
> +			};
> +		};
>   	};
>
>   	sgrf: syscon@ff740000 {
> diff --git a/arch/arm/boot/dts/rk3xxx.dtsi b/arch/arm/boot/dts/rk3xxx.dtsi
> index 4497d28..58af546 100644
> --- a/arch/arm/boot/dts/rk3xxx.dtsi
> +++ b/arch/arm/boot/dts/rk3xxx.dtsi
> @@ -43,6 +43,7 @@
>
>   #include <dt-bindings/interrupt-controller/irq.h>
>   #include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/soc/rockchip_boot-mode.h>
>   #include "skeleton.dtsi"
>
>   / {
> @@ -242,8 +243,38 @@
>   	};
>
>   	pmu: pmu@20004000 {
> -		compatible = "rockchip,rk3066-pmu", "syscon";
> +		compatible = "rockchip,rk3066-pmu", "syscon", "simple-mfd";
>   		reg = <0x20004000 0x100>;
> +
> +		reboot-mode {
> +			compatible = "syscon-reboot-mode";
> +			offset = <0x40>;
> +
> +			normal {
> +				linux,mode = "normal";
> +				loader,magic = <BOOT_NORMAL>;
> +			};
> +
> +			loader {
> +				linux,mode = "loader";
> +				loader,magic = <BOOT_LOADER>;
> +			};
> +
> +			maskrom {
> +				linux,mode = "maskrom";
> +				loader,magic = <BOOT_MASKROM>;
> +			};
> +
> +			recovery {
> +				linux,mode = "recovery";
> +				loader,magic = <BOOT_RECOVERY>;
> +			};
> +
> +			fastboot {
> +				linux,mode = "fastboot";
> +				loader,magic = <BOOT_FASTBOOT>;
> +			};
> +		};
>   	};
>
>   	grf: grf@20008000 {
> diff --git a/include/dt-bindings/soc/rockchip_boot-mode.h b/include/dt-bindings/soc/rockchip_boot-mode.h
> new file mode 100644
> index 0000000..eedf113
> --- /dev/null
> +++ b/include/dt-bindings/soc/rockchip_boot-mode.h
> @@ -0,0 +1,30 @@
> +#ifndef __ROCKCHIP_BOOT_MODE_H
> +#define __ROCKCHIP_BOOT_MODE_H
> +
> +/*high 24 bits is tag, low 8 bits is type*/
> +#define	REBOOT_FLAG		0x5242C300
> +/* normal boot */
> +#define	BOOT_NORMAL		(REBOOT_FLAG + 0)
> +/* enter loader rockusb mode */
> +#define	BOOT_LOADER		(REBOOT_FLAG + 1)
> +/* enter maskrom rockusb mode */
> +#define	BOOT_MASKROM		(0xEF08A53C)
> +/* enter recovery */
> +#define	BOOT_RECOVERY		(REBOOT_FLAG + 3)
> +/* do not enter recover */
> +#define	BOOT_NORECOVER		(REBOOT_FLAG + 4)
> +/* boot second OS*/
> +#define	BOOT_SECONDOS		(REBOOT_FLAG + 5)
> +/* enter recover and wipe data. */
> +#define	BOOT_WIPEDATA		(REBOOT_FLAG + 6)
> +/* enter recover and wipe all data. */
> +#define	BOOT_WIPEALL		(REBOOT_FLAG + 7)
> +/* check firmware img with backup part*/
> +#define	BOOT_CHECKIMG		(REBOOT_FLAG + 8)
> + /* enter fast boot mode */
> +#define	BOOT_FASTBOOT		(REBOOT_FLAG + 9)
> +#define	BOOT_SECUREBOOT_DISABLE	(REBOOT_FLAG + 10)
> +/* enter charge mode */
> +#define	BOOT_CHARGING		(REBOOT_FLAG + 11)
> +
> +#endif
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] dt-bindings: power: reset: add document for reboot-mode driver

[Rob Herring]

On Thu, Jan 21, 2016 at 02:27:57PM +0800, Andy Yan wrote:
> Hi Rob:
>    thanks for your review.
> On 2016年01月21日 02:28, Rob Herring wrote:
> >On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote:
> >>add device tree binding document for reboot-mode driver
> >>
> >>Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> >>
> >>---
> >>
> >>Changes in v2: None
> >>Changes in v1: None
> >>
> >>  .../bindings/power/reset/reboot-mode.txt           | 41 +++++++++++++++++
> >>  .../bindings/power/reset/syscon-reboot-mode.txt    | 52 ++++++++++++++++++++++
> >>  2 files changed, 93 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> >>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
> >>
> >>diff --git a/Documentation/devicetree/bindings/power/reset/reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> >>new file mode 100644
> >>index 0000000..81d9f66
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> >>@@ -0,0 +1,41 @@
> >>+Generic reboot mode core map driver

[...]

> >>+		compatible = "syscon-reboot-mode";
> >>+		offset = <0x40>;
> >This doc by itself is a little confusing. For example, is a child of the
> >syscon node? I would remove offset (and perhaps compatible) from this
> >example.
> 
>    Yes, is a child of a syscon mapped node. For example, Rockchip platform
> use a register of PMU(rk3066/rk3288) or GRF(rk3036), PMU and GRF are aleady
> mapped by syscon.
>    offset and compatible are used by write interface driver like
> syscon-reboot-mode.c. If you don't like it appear in the core map doc, I
> will move it to the syscon-reboot-mode.txt?

Yes, try to make this doc stand on its own. It will obviously be 
incomplete lacking information on where in the DT it goes. So perhaps a 
note stating reboot-mode node location is defined in platform specific 
binding docs.

> >>+
> >>+		loader {
> >>+			linux,mode = "loader";
> >>+			loader,magic = <BOOT_LOADER>;
> >>+		};
> >Sorry, my previous suggestion was not clear. I'm suggesting get rid of
> >the subnodes and just do properties like this:
> >
> >loader = <BOOT_LOADER>;
> >maskrom = <BOOT_MASKROM>;
> >
> >That's the same amount of information unless node names and linux,mode
> >values are going to diverge. Do they need to? I can't see a reason.
> 
>     Because the command"linux,mode" and value"loader,magic" is vendor
> specific. I don't know what commands and how many mode other platform will
> use. So as John says in his reply, this sort of flexibility help us adapt
> the driver to different hardware/system environments.

The only part of "reboot to fastboot" that is vendor specific would be 
the magic value. While we can have custom modes, we should standardize 
the common ones as much as possible. As I pointed out in my reply to 
John, we can still support vendor specific modes with just a property.

> >
> >We need to be clear what loader means. More specifically, it is boot
> >into bootloader shell.
>     Actually, Rockchip platform will reboot into a bootloader download mode
> with this command. This mode can download faster than maskrom download mode.

My point is proven. I assumed one thing and you meant something else. 
Doesn't matter what the mode is, just needs to be clear.

Rob

Re: [PATCH v2 1/4] dt-bindings: power: reset: add document for reboot-mode driver

[Andy Yan]

Hi Rob:

On 2016年01月26日 01:11, Rob Herring wrote:
> On Thu, Jan 21, 2016 at 02:27:57PM +0800, Andy Yan wrote:
>> Hi Rob:
>>     thanks for your review.
>> On 2016年01月21日 02:28, Rob Herring wrote:
>>> On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote:
>>>> add device tree binding document for reboot-mode driver
>>>>
>>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>>>
>>>> ---
>>>>
>>>> Changes in v2: None
>>>> Changes in v1: None
>>>>
>>>>   .../bindings/power/reset/reboot-mode.txt           | 41 +++++++++++++++++
>>>>   .../bindings/power/reset/syscon-reboot-mode.txt    | 52 ++++++++++++++++++++++
>>>>   2 files changed, 93 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>>>>   create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/power/reset/reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>>>> new file mode 100644
>>>> index 0000000..81d9f66
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>>>> @@ -0,0 +1,41 @@
>>>> +Generic reboot mode core map driver
> [...]
>
>>>> +		compatible = "syscon-reboot-mode";
>>>> +		offset = <0x40>;
>>> This doc by itself is a little confusing. For example, is a child of the
>>> syscon node? I would remove offset (and perhaps compatible) from this
>>> example.
>>     Yes, is a child of a syscon mapped node. For example, Rockchip platform
>> use a register of PMU(rk3066/rk3288) or GRF(rk3036), PMU and GRF are aleady
>> mapped by syscon.
>>     offset and compatible are used by write interface driver like
>> syscon-reboot-mode.c. If you don't like it appear in the core map doc, I
>> will move it to the syscon-reboot-mode.txt?
> Yes, try to make this doc stand on its own. It will obviously be
> incomplete lacking information on where in the DT it goes. So perhaps a
> note stating reboot-mode node location is defined in platform specific
> binding docs.
>
>>>> +
>>>> +		loader {
>>>> +			linux,mode = "loader";
>>>> +			loader,magic = <BOOT_LOADER>;
>>>> +		};
>>> Sorry, my previous suggestion was not clear. I'm suggesting get rid of
>>> the subnodes and just do properties like this:
>>>
>>> loader = <BOOT_LOADER>;
>>> maskrom = <BOOT_MASKROM>;
>>>
>>> That's the same amount of information unless node names and linux,mode
>>> values are going to diverge. Do they need to? I can't see a reason.
>>      Because the command"linux,mode" and value"loader,magic" is vendor
>> specific. I don't know what commands and how many mode other platform will
>> use. So as John says in his reply, this sort of flexibility help us adapt
>> the driver to different hardware/system environments.
> The only part of "reboot to fastboot" that is vendor specific would be
> the magic value. While we can have custom modes, we should standardize
> the common ones as much as possible. As I pointed out in my reply to
> John, we can still support vendor specific modes with just a property.

     Based your reply to John, I rebuild the code like bellow, I hope this
     is what you mean.

     DTS file:
     reboot-mode {
                         compatible = "syscon-reboot-mode";
                         offset = <0x94>;
                         mode-normal = <BOOT_NORMAL>;
                         mode-recovery = <BOOT_RECOVERY>;
                         mode-fastboot = <BOOT_FASTBOOT>;
                         mode-loader = <BOOT_LOADER>;
                         mode-maskrom = <BOOT_MASKROM>;
                 };


    driver:

     #define PREFIX "mode-"

     struct property *prop;
     size_t len = strlen(PREFIX);
     for_each_property_of_node(dev->of_node, prop) {
                 if (len > strlen(prop->name) || strncmp(prop->name, 
PREFIX, len))
                         continue;
                 info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
                 if (!info)
                         return -ENOMEM;
                 strcpy(info->mode, prop->name + len);
                 if (of_property_read_u32(dev->of_node, prop->name, 
&info->magic)) {
                         dev_err(dev, "reboot mode %s without magic 
number\n",
                                 info->mode);
                         devm_kfree(dev, info);
                         continue;
                 }
                 list_add_tail(&info->list, &reboot->head);
         }


>>> We need to be clear what loader means. More specifically, it is boot
>>> into bootloader shell.
>>      Actually, Rockchip platform will reboot into a bootloader download mode
>> with this command. This mode can download faster than maskrom download mode.
> My point is proven. I assumed one thing and you meant something else.
> Doesn't matter what the mode is, just needs to be clear.
>
> Rob
>
>
>

Re: [PATCH v2 2/4] power: reset: add reboot mode driver

[Moritz Fischer]

Hi Andy,

On Tue, Jan 12, 2016 at 12:31 PM, Andy Yan <andy.yan-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> This driver parse the reboot commands like "reboot loader"
> and "reboot recovery" to get a boot mode described in the
> device tree , then call the write interfae to store the boot
> mode in some persistent storage  like special register or ram,
> which can be read by the bootloader after system reboot, then
> the bootloader can take different action according to the mode
> stored.
>
> This is commonly used on Android based devices, which in order
> to reboot the device into fastboot or recovery mode.
>
> Signed-off-by: Andy Yan <andy.yan-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>
> ---
>
> Changes in v2:
> - move to dir drivers/power/reset/
> - make syscon-reboot-mode a generic driver
>
> Changes in v1:
> - fix the embarrassed compile warning
> - correct the maskrom magic number
> - check for the normal reboot
>
>  drivers/power/reset/Kconfig              |  16 +++++
>  drivers/power/reset/Makefile             |   2 +
>  drivers/power/reset/reboot-mode.c        | 100 +++++++++++++++++++++++++++++++
>  drivers/power/reset/reboot-mode.h        |   6 ++
>  drivers/power/reset/syscon-reboot-mode.c |  62 +++++++++++++++++++
>  5 files changed, 186 insertions(+)
>  create mode 100644 drivers/power/reset/reboot-mode.c
>  create mode 100644 drivers/power/reset/reboot-mode.h
>  create mode 100644 drivers/power/reset/syscon-reboot-mode.c
>
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index 1131cf7..2e6d873 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -173,5 +173,21 @@ config POWER_RESET_ZX
>         help
>           Reboot support for ZTE SoCs.
>
> +config REBOOT_MODE
> +       tristate
> +       depends on OF
> +       help
> +        This driver will help to pass the system reboot mode
> +        to bootloader
> +
> +config SYSCON_REBOOT_MODE
> +       bool "Generic SYSCON regmap 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 SYSCON mapped
> +        register, then the bootloader can read it to take different
> +        action according to the mode.
> +
>  endif
>
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index 096fa67..a63865b 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -20,3 +20,5 @@ obj-$(CONFIG_POWER_RESET_SYSCON) += syscon-reboot.o
>  obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
>  obj-$(CONFIG_POWER_RESET_RMOBILE) += rmobile-reset.o
>  obj-$(CONFIG_POWER_RESET_ZX) += zx-reboot.o
> +obj-$(CONFIG_REBOOT_MODE) += reboot-mode.o
> +obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> new file mode 100644
> index 0000000..92b7b4d
> --- /dev/null
> +++ b/drivers/power/reset/reboot-mode.c
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/reboot.h>
> +#include "reboot-mode.h"
> +
> +struct mode_info {
> +       const char *mode;
> +       int magic;
> +       struct list_head list;
> +};
> +
> +struct reboot_mode_driver {
> +       struct list_head head;
> +       int (*write)(int magic);
> +       struct notifier_block reboot_notifier;
> +};
> +
> +static int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
> +                                const char *cmd)
> +{
> +       const char *normal = "normal";
> +       int magic = 0;
> +       struct mode_info *info;
> +
> +       if (!cmd)
> +               cmd = normal;
> +
> +       list_for_each_entry(info, &reboot->head, list) {
> +               if (!strcmp(info->mode, cmd)) {
> +                       magic = info->magic;
> +                       break;
> +               }
> +       }
> +
> +       return magic;
> +}
> +
> +static int reboot_mode_notify(struct notifier_block *this,
> +                             unsigned long mode, void *cmd)
> +{
> +       struct reboot_mode_driver *reboot;
> +       int magic;
> +
> +       reboot = container_of(this, struct reboot_mode_driver, reboot_notifier);
> +       magic = get_reboot_mode_magic(reboot, cmd);
> +       if (magic)
> +               reboot->write(magic);
> +
> +       return NOTIFY_DONE;
> +}
> +
> +int reboot_mode_register(struct device *dev, int (*write)(int))
> +{
> +       struct reboot_mode_driver *reboot;
> +       struct mode_info *info;
> +       struct device_node *child;
> +       int ret;
> +
> +       reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
> +       if (!reboot)
> +               return -ENOMEM;
> +
> +       reboot->write = write;
> +       INIT_LIST_HEAD(&reboot->head);
> +       for_each_child_of_node(dev->of_node, child) {
> +               info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +               if (!info)
> +                       return -ENOMEM;
> +               info->mode = of_get_property(child, "linux,mode", NULL);
> +               if (of_property_read_u32(child, "loader,magic", &info->magic)) {
> +                       dev_err(dev, "reboot mode %s without magic number\n",
> +                               info->mode);
> +                       devm_kfree(dev, info);
> +                       continue;
> +               }
> +               list_add_tail(&info->list, &reboot->head);
> +       }
> +       reboot->reboot_notifier.notifier_call = reboot_mode_notify;
> +       ret = register_reboot_notifier(&reboot->reboot_notifier);
> +       if (ret)
> +               dev_err(dev, "can't register reboot notifier\n");
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(reboot_mode_register);
> +
> +MODULE_AUTHOR("Andy Yan <andy.yan-TNX95d0MmH7DzftRWevZcw@public.gmane.org");
> +MODULE_DESCRIPTION("System reboot mode driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/power/reset/reboot-mode.h b/drivers/power/reset/reboot-mode.h
> new file mode 100644
> index 0000000..76646e7
> --- /dev/null
> +++ b/drivers/power/reset/reboot-mode.h
> @@ -0,0 +1,6 @@
> +#ifndef __REBOOT_MODE_H__
> +#define __REBOOT_MODE_H__
> +
> +extern int reboot_mode_register(struct device *dev, int (*write)(int));
> +
> +#endif
> diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
> new file mode 100644
> index 0000000..b9370f3
> --- /dev/null
> +++ b/drivers/power/reset/syscon-reboot-mode.c
> @@ -0,0 +1,62 @@
> +/*
> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include "reboot-mode.h"
> +
> +static struct regmap *map;
> +static u32 offset;
> +
> +static int syscon_reboot_mode_write(int magic)
> +{
> +       regmap_write(map, offset, magic);

See suggestion below. I think using regmap_update_bits() would be even
more useful for cases where you have other stuff in that register, too.

> +
> +       return 0;
> +}
> +
> +static int syscon_reboot_mode_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +
> +       map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> +       if (IS_ERR(map))
> +               return PTR_ERR(map);
> +       if (of_property_read_u32(pdev->dev.of_node, "offset", &offset))
> +               return -EINVAL;

I think it would be worthwhile to have a mask value here, that you can use
with regmap_update_bits()

+           if (of_property_read_u32(pdev->dev.of_node, "mask", &mask))
+                   return -EINVAL;


Cheers,

Moritz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] power: reset: add reboot mode driver

[Andy Yan]

Hi Moritz:

On 2016年01月27日 18:53, Moritz Fischer wrote:
> Hi Andy,
>
> On Tue, Jan 12, 2016 at 12:31 PM, Andy Yan <andy.yan@rock-chips.com> wrote:
>> This driver parse the reboot commands like "reboot loader"
>> and "reboot recovery" to get a boot mode described in the
>> device tree , then call the write interfae to store the boot
>> mode in some persistent storage  like special register or ram,
>> which can be read by the bootloader after system reboot, then
>> the bootloader can take different action according to the mode
>> stored.
>>
>> This is commonly used on Android based devices, which in order
>> to reboot the device into fastboot or recovery mode.
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - move to dir drivers/power/reset/
>> - make syscon-reboot-mode a generic driver
>>
>> Changes in v1:
>> - fix the embarrassed compile warning
>> - correct the maskrom magic number
>> - check for the normal reboot
>>
>>   drivers/power/reset/Kconfig              |  16 +++++
>>   drivers/power/reset/Makefile             |   2 +
>>   drivers/power/reset/reboot-mode.c        | 100 +++++++++++++++++++++++++++++++
>>   drivers/power/reset/reboot-mode.h        |   6 ++
>>   drivers/power/reset/syscon-reboot-mode.c |  62 +++++++++++++++++++
>>   5 files changed, 186 insertions(+)
>>   create mode 100644 drivers/power/reset/reboot-mode.c
>>   create mode 100644 drivers/power/reset/reboot-mode.h
>>   create mode 100644 drivers/power/reset/syscon-reboot-mode.c
>>
>> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
>> index 1131cf7..2e6d873 100644
>> --- a/drivers/power/reset/Kconfig
>> +++ b/drivers/power/reset/Kconfig
>> @@ -173,5 +173,21 @@ config POWER_RESET_ZX
>>          help
>>            Reboot support for ZTE SoCs.
>>
>> +config REBOOT_MODE
>> +       tristate
>> +       depends on OF
>> +       help
>> +        This driver will help to pass the system reboot mode
>> +        to bootloader
>> +
>> +config SYSCON_REBOOT_MODE
>> +       bool "Generic SYSCON regmap 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 SYSCON mapped
>> +        register, then the bootloader can read it to take different
>> +        action according to the mode.
>> +
>>   endif
>>
>> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
>> index 096fa67..a63865b 100644
>> --- a/drivers/power/reset/Makefile
>> +++ b/drivers/power/reset/Makefile
>> @@ -20,3 +20,5 @@ obj-$(CONFIG_POWER_RESET_SYSCON) += syscon-reboot.o
>>   obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
>>   obj-$(CONFIG_POWER_RESET_RMOBILE) += rmobile-reset.o
>>   obj-$(CONFIG_POWER_RESET_ZX) += zx-reboot.o
>> +obj-$(CONFIG_REBOOT_MODE) += reboot-mode.o
>> +obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
>> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
>> new file mode 100644
>> index 0000000..92b7b4d
>> --- /dev/null
>> +++ b/drivers/power/reset/reboot-mode.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#include <linux/device.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/reboot.h>
>> +#include "reboot-mode.h"
>> +
>> +struct mode_info {
>> +       const char *mode;
>> +       int magic;
>> +       struct list_head list;
>> +};
>> +
>> +struct reboot_mode_driver {
>> +       struct list_head head;
>> +       int (*write)(int magic);
>> +       struct notifier_block reboot_notifier;
>> +};
>> +
>> +static int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
>> +                                const char *cmd)
>> +{
>> +       const char *normal = "normal";
>> +       int magic = 0;
>> +       struct mode_info *info;
>> +
>> +       if (!cmd)
>> +               cmd = normal;
>> +
>> +       list_for_each_entry(info, &reboot->head, list) {
>> +               if (!strcmp(info->mode, cmd)) {
>> +                       magic = info->magic;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       return magic;
>> +}
>> +
>> +static int reboot_mode_notify(struct notifier_block *this,
>> +                             unsigned long mode, void *cmd)
>> +{
>> +       struct reboot_mode_driver *reboot;
>> +       int magic;
>> +
>> +       reboot = container_of(this, struct reboot_mode_driver, reboot_notifier);
>> +       magic = get_reboot_mode_magic(reboot, cmd);
>> +       if (magic)
>> +               reboot->write(magic);
>> +
>> +       return NOTIFY_DONE;
>> +}
>> +
>> +int reboot_mode_register(struct device *dev, int (*write)(int))
>> +{
>> +       struct reboot_mode_driver *reboot;
>> +       struct mode_info *info;
>> +       struct device_node *child;
>> +       int ret;
>> +
>> +       reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
>> +       if (!reboot)
>> +               return -ENOMEM;
>> +
>> +       reboot->write = write;
>> +       INIT_LIST_HEAD(&reboot->head);
>> +       for_each_child_of_node(dev->of_node, child) {
>> +               info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>> +               if (!info)
>> +                       return -ENOMEM;
>> +               info->mode = of_get_property(child, "linux,mode", NULL);
>> +               if (of_property_read_u32(child, "loader,magic", &info->magic)) {
>> +                       dev_err(dev, "reboot mode %s without magic number\n",
>> +                               info->mode);
>> +                       devm_kfree(dev, info);
>> +                       continue;
>> +               }
>> +               list_add_tail(&info->list, &reboot->head);
>> +       }
>> +       reboot->reboot_notifier.notifier_call = reboot_mode_notify;
>> +       ret = register_reboot_notifier(&reboot->reboot_notifier);
>> +       if (ret)
>> +               dev_err(dev, "can't register reboot notifier\n");
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(reboot_mode_register);
>> +
>> +MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com");
>> +MODULE_DESCRIPTION("System reboot mode driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/power/reset/reboot-mode.h b/drivers/power/reset/reboot-mode.h
>> new file mode 100644
>> index 0000000..76646e7
>> --- /dev/null
>> +++ b/drivers/power/reset/reboot-mode.h
>> @@ -0,0 +1,6 @@
>> +#ifndef __REBOOT_MODE_H__
>> +#define __REBOOT_MODE_H__
>> +
>> +extern int reboot_mode_register(struct device *dev, int (*write)(int));
>> +
>> +#endif
>> diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
>> new file mode 100644
>> index 0000000..b9370f3
>> --- /dev/null
>> +++ b/drivers/power/reset/syscon-reboot-mode.c
>> @@ -0,0 +1,62 @@
>> +/*
>> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reboot.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +#include "reboot-mode.h"
>> +
>> +static struct regmap *map;
>> +static u32 offset;
>> +
>> +static int syscon_reboot_mode_write(int magic)
>> +{
>> +       regmap_write(map, offset, magic);
> See suggestion below. I think using regmap_update_bits() would be even
> more useful for cases where you have other stuff in that register, too.
>
>> +
>> +       return 0;
>> +}
>> +
>> +static int syscon_reboot_mode_probe(struct platform_device *pdev)
>> +{
>> +       int ret;
>> +
>> +       map = syscon_node_to_regmap(pdev->dev.parent->of_node);
>> +       if (IS_ERR(map))
>> +               return PTR_ERR(map);
>> +       if (of_property_read_u32(pdev->dev.of_node, "offset", &offset))
>> +               return -EINVAL;
> I think it would be worthwhile to have a mask value here, that you can use
> with regmap_update_bits()

     That sounds reasonable,  I already fond some drivers like tegra[0]
     only use some bits to store the mode value. So I will add it in V3.

    [0] drivers/soc/tegra/pmc.c
>
> +           if (of_property_read_u32(pdev->dev.of_node, "mask", &mask))
> +                   return -EINVAL;
>
>
> Cheers,
>
> Moritz
>
>
>