[PATCH v6 0/4] add reboot mode driver

[PATCH v6 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 interfae
to store the boot mode in some place like special register
or ram , which can be read by the bootloader after system
reboot.

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


Changes in v6:
- fix a typo with "property"
- describe property "mask" more clear

Changes in v5:
- delete a unnecessary blank line in syscon-reboot-mode.txt
- rename mode-fastoboot to mode-bootloader in syscon-reboot-mode.txt
- rename macro BOOT_LOADER to BOOT_BL_DOWNLOAD, which gives a more clear mean
- use two blank space under help in Kconfig
- use unsigned int instead of int for member magic in struct mode_info

Changes in v4:
- remove mode-maskrom
- rename mode-fastboot to mode-bootloader to keep compatible with the exiting Android device
- make this driver depends on OF to avoid kbuild test error

Changes in v3:
- descirbe all reboot mode as properity instead of subnode
- scan multi properities
- add mask value for some platform which only use some bits of the register
  to store boot mode magic value
- descirbe all reboot mode as property instead of subnode
- add rk3036 support
- descirbe all reboot mode as properity instead of subnode

Changes in v2:
- move to dir drivers/power/reset/
- make syscon-reboot-mode a generic driver
- make this node as a subnode of PMU
- make this node as a subnode of 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 DT node
  ARM64: dts: rockchip: add syscon-reboot-mode DT node

 .../bindings/power/reset/reboot-mode.txt           |  24 +++++
 .../bindings/power/reset/syscon-reboot-mode.txt    |  35 +++++++
 arch/arm/boot/dts/rk3036.dtsi                      |  11 ++-
 arch/arm/boot/dts/rk3288.dtsi                      |  10 ++
 arch/arm/boot/dts/rk3xxx.dtsi                      |  12 ++-
 arch/arm64/boot/dts/rockchip/rk3368.dtsi           |  12 ++-
 drivers/power/reset/Kconfig                        |  13 +++
 drivers/power/reset/Makefile                       |   2 +
 drivers/power/reset/reboot-mode.c                  | 106 +++++++++++++++++++++
 drivers/power/reset/reboot-mode.h                  |   6 ++
 drivers/power/reset/syscon-reboot-mode.c           |  65 +++++++++++++
 include/dt-bindings/soc/rockchip_boot-mode.h       |  15 +++
 12 files changed, 308 insertions(+), 3 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

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

[Andy Yan]

add device tree bindings document for reboot-mode driver

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
Acked-by: Rob Herring <robh@kernel.org>

binding

---

Changes in v6:
- fix a typo with "property"
- describe property "mask" more clear

Changes in v5:
- delete a unnecessary blank line in syscon-reboot-mode.txt
- rename mode-fastoboot to mode-bootloader in syscon-reboot-mode.txt
- rename macro BOOT_LOADER to BOOT_BL_DOWNLOAD, which gives a more clear mean

Changes in v4:
- remove mode-maskrom
- rename mode-fastboot to mode-bootloader to keep compatible with the exiting Android device

Changes in v3:
- descirbe all reboot mode as properity instead of subnode

Changes in v2: None
Changes in v1: None

 .../bindings/power/reset/reboot-mode.txt           | 24 +++++++++++++++
 .../bindings/power/reset/syscon-reboot-mode.txt    | 35 ++++++++++++++++++++++
 2 files changed, 59 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..3457949
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
@@ -0,0 +1,24 @@
+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 the argument stored.
+
+All mode properties are vendor specific, it is a indication to tell
+the bootloder what to do when the system reboot, and should be named
+as mode-xxx = <magic> (xxx is mode name).
+
+- mode-normal: Normal reboot mode, system reboot with command "reboot".
+- mode-recovery: Android Recovery mode, it is a mode to format the device or update a new image.
+- mode-bootloader: Android fastboot mode, it's a mode to re-flash partitions on the Android based device.
+- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
+	       usually used in development.
+
+Example:
+	reboot-mode {
+		mode-normal = <BOOT_NORMAL>;
+		mode-recovery = <BOOT_RECOVERY>;
+		mode-bootloader = <BOOT_FASTBOOT>;
+		mode-loader = <BOOT_BL_DOWNLOAD>;
+	}
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..eb28469
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
@@ -0,0 +1,35 @@
+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)
+
+Optional property:
+- mask: bits mask of the bits in the register to store the reboot mode magic value,
+  default set to 0xffffffff if missing.
+
+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>;
+			mode-normal = <BOOT_NORMAL>;
+			mode-recovery = <BOOT_RECOVERY>;
+			mode-bootloader = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_BL_DOWNLOAD>;
+		};
+	};
-- 
1.9.1

[PATCH v6 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 place like special register or sram , 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, in order to
reboot the device into fastboot or recovery mode.

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: Moritz Fischer <moritz.fischer@ettus.com>
Tested-by: John Stultz <john.stultz@linaro.org>
Acked-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

Changes in v6: None
Changes in v5:
- use two blank space under help in Kconfig
- use unsigned int instead of int for member magic in struct mode_info

Changes in v4:
- make this driver depends on OF to avoid kbuild test error

Changes in v3:
- scan multi properities
- add mask value for some platform which only use some bits of the register
  to store boot mode magic value

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              |  13 ++++
 drivers/power/reset/Makefile             |   2 +
 drivers/power/reset/reboot-mode.c        | 106 +++++++++++++++++++++++++++++++
 drivers/power/reset/reboot-mode.h        |   6 ++
 drivers/power/reset/syscon-reboot-mode.c |  65 +++++++++++++++++++
 5 files changed, 192 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..cf50630 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -173,5 +173,18 @@ config POWER_RESET_ZX
 	help
 	  Reboot support for ZTE SoCs.
 
+config REBOOT_MODE
+	tristate
+
+config SYSCON_REBOOT_MODE
+	bool "Generic SYSCON regmap reboot mode driver"
+	depends on OF
+	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..9aa7b80
--- /dev/null
+++ b/drivers/power/reset/reboot-mode.c
@@ -0,0 +1,106 @@
+/*
+ * 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"
+
+#define PREFIX "mode-"
+
+struct mode_info {
+	char mode[32];
+	unsigned 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 property *prop;
+	size_t len = strlen(PREFIX);
+	int ret;
+
+	reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
+	if (!reboot)
+		return -ENOMEM;
+
+	reboot->write = write;
+	INIT_LIST_HEAD(&reboot->head);
+	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);
+	}
+	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..44ed34f
--- /dev/null
+++ b/drivers/power/reset/reboot-mode.h
@@ -0,0 +1,6 @@
+#ifndef __REBOOT_MODE_H__
+#define __REBOOT_MODE_H__
+
+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..d1521d8
--- /dev/null
+++ b/drivers/power/reset/syscon-reboot-mode.c
@@ -0,0 +1,65 @@
+/*
+ * 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 u32 mask = 0xffffffff;
+
+static int syscon_reboot_mode_write(int magic)
+{
+	regmap_update_bits(map, offset, mask, 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;
+	of_property_read_u32(pdev->dev.of_node, "mask", &mask);
+	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 v6 3/4] ARM: dts: rockchip: add syscon-reboot-mode DT 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/rk3036/rk3288 based platform

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3:
- descirbe all reboot mode as property instead of subnode
- add rk3036 support

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/rk3036.dtsi                | 11 ++++++++++-
 arch/arm/boot/dts/rk3288.dtsi                | 10 ++++++++++
 arch/arm/boot/dts/rk3xxx.dtsi                | 12 +++++++++++-
 include/dt-bindings/soc/rockchip_boot-mode.h | 15 +++++++++++++++
 4 files changed, 46 insertions(+), 2 deletions(-)
 create mode 100644 include/dt-bindings/soc/rockchip_boot-mode.h

diff --git a/arch/arm/boot/dts/rk3036.dtsi b/arch/arm/boot/dts/rk3036.dtsi
index b9567c1..4011c2e 100644
--- a/arch/arm/boot/dts/rk3036.dtsi
+++ b/arch/arm/boot/dts/rk3036.dtsi
@@ -43,6 +43,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/rockchip.h>
 #include <dt-bindings/clock/rk3036-cru.h>
+#include <dt-bindings/soc/rockchip_boot-mode.h>
 #include "skeleton.dtsi"
 
 / {
@@ -261,8 +262,16 @@
 	};
 
 	grf: syscon@20008000 {
-		compatible = "rockchip,rk3036-grf", "syscon";
+		compatible = "rockchip,rk3036-grf", "syscon", "simple-mfd";
 		reg = <0x20008000 0x1000>;
+		reboot-mode {
+			compatible = "syscon-reboot-mode";
+			offset = <0x1d8>;
+			mode-normal = <BOOT_NORMAL>;
+			mode-recovery = <BOOT_RECOVERY>;
+			mode-bootloader = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_BL_DOWNLOAD>;
+		};
 	};
 
 	acodec: acodec-ana@20030000 {
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 8ac49f3..9aa7d73 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"
 
 / {
@@ -713,6 +714,15 @@
 				clocks = <&cru ACLK_GPU>;
 			};
 		};
+
+		reboot-mode {
+			compatible = "syscon-reboot-mode";
+			offset = <0x94>;
+			mode-normal = <BOOT_NORMAL>;
+			mode-recovery = <BOOT_RECOVERY>;
+			mode-bootloader = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_BL_DOWNLOAD>;
+		};
 	};
 
 	sgrf: syscon@ff740000 {
diff --git a/arch/arm/boot/dts/rk3xxx.dtsi b/arch/arm/boot/dts/rk3xxx.dtsi
index 99eeea7..f8f661f 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"
 
 / {
@@ -243,8 +244,17 @@
 	};
 
 	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>;
+			mode-normal = <BOOT_NORMAL>;
+			mode-recovery = <BOOT_RECOVERY>;
+			mode-bootloader = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_BL_DOWNLOAD>;
+		};
 	};
 
 	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..ae7c867
--- /dev/null
+++ b/include/dt-bindings/soc/rockchip_boot-mode.h
@@ -0,0 +1,15 @@
+#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 bootloader rockusb mode */
+#define BOOT_BL_DOWNLOAD	(REBOOT_FLAG + 1)
+/* enter recovery */
+#define BOOT_RECOVERY		(REBOOT_FLAG + 3)
+ /* enter fastboot mode */
+#define BOOT_FASTBOOT		(REBOOT_FLAG + 9)
+
+#endif
-- 
1.9.1

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

[Andy Yan]

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

Tested-by: Caesar Wang <caesar.upstream@gmail.com>
Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3:
- descirbe all reboot mode as properity instead of subnode

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

Changes in v1: None

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

diff --git a/arch/arm64/boot/dts/rockchip/rk3368.dtsi b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
index 122777b..df062ea 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>
 #include <dt-bindings/thermal/thermal.h>
 
 / {
@@ -554,8 +555,17 @@
 	};
 
 	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>;
+			mode-normal = <BOOT_NORMAL>;
+			mode-recovery = <BOOT_RECOVERY>;
+			mode-bootloader = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_BL_DOWNLOAD>;
+		};
 	};
 
 	cru: clock-controller@ff760000 {
-- 
1.9.1

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

[Krzysztof Kozlowski]

Interesting idea. While looking at the code I stumped over few things below:

On Tue, Mar 22, 2016 at 8:36 PM, Andy Yan <andy.yan@rock-chips.com> wrote:
> add device tree bindings document for reboot-mode driver

s/add/Add/
Plus a full-stop at the end of sentence.

>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> Acked-by: Rob Herring <robh@kernel.org>
>
> binding

What is that?

>
> ---
>
> Changes in v6:
> - fix a typo with "property"
> - describe property "mask" more clear
>
> Changes in v5:
> - delete a unnecessary blank line in syscon-reboot-mode.txt
> - rename mode-fastoboot to mode-bootloader in syscon-reboot-mode.txt
> - rename macro BOOT_LOADER to BOOT_BL_DOWNLOAD, which gives a more clear mean
>
> Changes in v4:
> - remove mode-maskrom
> - rename mode-fastboot to mode-bootloader to keep compatible with the exiting Android device
>
> Changes in v3:
> - descirbe all reboot mode as properity instead of subnode
>
> Changes in v2: None
> Changes in v1: None
>
>  .../bindings/power/reset/reboot-mode.txt           | 24 +++++++++++++++
>  .../bindings/power/reset/syscon-reboot-mode.txt    | 35 ++++++++++++++++++++++
>  2 files changed, 59 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..3457949
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> @@ -0,0 +1,24 @@
> +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
s/stores/store/
> +or ram . Then the bootloader can read it and take different

s/ram ./ram./

> +action according the argument stored.

according to

> +
> +All mode properties are vendor specific, it is a indication to tell
> +the bootloder what to do when the system reboot, and should be named

s/bootloder/bootloader/
s/reboot/reboots/

> +as mode-xxx = <magic> (xxx is mode name).
> +

These are examples? If yes then add a note, like:

+For example modes common on Android platform:

> +- mode-normal: Normal reboot mode, system reboot with command "reboot".
> +- mode-recovery: Android Recovery mode, it is a mode to format the device or update a new image.
> +- mode-bootloader: Android fastboot mode, it's a mode to re-flash partitions on the Android based device.
> +- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
> +              usually used in development.

The "bootloader" and "loader" examples are very confusing. Maybe just
name the first one as "mode-fastboot" and second as
"rockchip-download"? The term "loader" does not look for me as
anything related to uploading/downloading new firmware.

> +
> +Example:
> +       reboot-mode {
> +               mode-normal = <BOOT_NORMAL>;
> +               mode-recovery = <BOOT_RECOVERY>;
> +               mode-bootloader = <BOOT_FASTBOOT>;
> +               mode-loader = <BOOT_BL_DOWNLOAD>;
> +       }
> 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..eb28469
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
> @@ -0,0 +1,35 @@
> +SYSCON reboot mode driver
> +
> +This driver get reboot mode magic value form reboot-mode driver

s/get/gets/

> +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)
> +
> +Optional property:
> +- mask: bits mask of the bits in the register to store the reboot mode magic value,
> +  default set to 0xffffffff if missing.
> +
> +The rest of the properties should follow the generic reboot-mode discription

s/discription/description/

Please, run the spell check...

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

Cool work! Few comments below.


On Tue, Mar 22, 2016 at 8:37 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 place like special register or sram , 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, in order to
> reboot the device into fastboot or recovery mode.
>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> Reviewed-by: Moritz Fischer <moritz.fischer@ettus.com>
> Tested-by: John Stultz <john.stultz@linaro.org>
> Acked-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>
> ---
>
> Changes in v6: None
> Changes in v5:
> - use two blank space under help in Kconfig
> - use unsigned int instead of int for member magic in struct mode_info
>
> Changes in v4:
> - make this driver depends on OF to avoid kbuild test error
>
> Changes in v3:
> - scan multi properities
> - add mask value for some platform which only use some bits of the register
>   to store boot mode magic value
>
> 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              |  13 ++++
>  drivers/power/reset/Makefile             |   2 +
>  drivers/power/reset/reboot-mode.c        | 106 +++++++++++++++++++++++++++++++
>  drivers/power/reset/reboot-mode.h        |   6 ++
>  drivers/power/reset/syscon-reboot-mode.c |  65 +++++++++++++++++++
>  5 files changed, 192 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..cf50630 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -173,5 +173,18 @@ config POWER_RESET_ZX
>         help
>           Reboot support for ZTE SoCs.
>
> +config REBOOT_MODE
> +       tristate
> +
> +config SYSCON_REBOOT_MODE
> +       bool "Generic SYSCON regmap reboot mode driver"
> +       depends on OF
> +       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..9aa7b80
> --- /dev/null
> +++ b/drivers/power/reset/reboot-mode.c
> @@ -0,0 +1,106 @@
> +/*
> + * 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"
> +
> +#define PREFIX "mode-"
> +
> +struct mode_info {
> +       char mode[32];
> +       unsigned 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;

In absence of 'normal' mode (it is not described as required property)
the magic will be '0'. It would be nice to document that in bindings.
Imagine someone forgets about this and will wonder why 0x0 is written
to his precious register on normal reboot...

It would be nice to document that 'mode-normal' has a special
(hard-coded) meaning.

> +}
> +
> +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 property *prop;
> +       size_t len = strlen(PREFIX);
> +       int ret;
> +
> +       reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
> +       if (!reboot)
> +               return -ENOMEM;
> +
> +       reboot->write = write;
> +       INIT_LIST_HEAD(&reboot->head);
> +       for_each_property_of_node(dev->of_node, prop) {
> +               if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, len))
> +                       continue;

New line please for readability.

> +               info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +               if (!info)
> +                       return -ENOMEM;

Ditto.

> +               strcpy(info->mode, prop->name + len);

Ehm, and how do you protect that name of mode is shorter than 32 characters?

> +               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);
> +       }
> +       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..44ed34f
> --- /dev/null
> +++ b/drivers/power/reset/reboot-mode.h
> @@ -0,0 +1,6 @@
> +#ifndef __REBOOT_MODE_H__
> +#define __REBOOT_MODE_H__
> +
> +int reboot_mode_register(struct device *dev, int (*write)(int));

Documentation would be appreciated. Although it is local header but
you decoupled them and you are exporting the function.

> +
> +#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..d1521d8
> --- /dev/null
> +++ b/drivers/power/reset/syscon-reboot-mode.c
> @@ -0,0 +1,65 @@
> +/*
> + * 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>

I don't see the need of of_address or am I missing something?

> +#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 u32 mask = 0xffffffff;

Why this cannot be part of state container? I suppose usually there
will be only one reboot mode handler, but:
1. It is always nice to make things self-contained, so code could be
easily used in different contexts. Depending on static variables
prevents that.
2. How do you protect against double binding (two reboot mode device
nodes in DT)? The second binding (put by mistake... or whatever
reason) will overwrite data used by previous device.

> +
> +static int syscon_reboot_mode_write(int magic)
> +{
> +       regmap_update_bits(map, offset, mask, magic);
> +
> +       return 0;

Why ignoring return value of regmap_update_bits?

> +}
> +
> +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);

Empty line for readability.

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

Empty line for readability.

Best regards,
Krzysztof

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

[Andy Yan]

Hi Krzysztof:

On 2016年03月24日 10:27, Krzysztof Kozlowski wrote:
> Interesting idea. While looking at the code I stumped over few things below:
>
> On Tue, Mar 22, 2016 at 8:36 PM, Andy Yan <andy.yan@rock-chips.com> wrote:
>> add device tree bindings document for reboot-mode driver
> s/add/Add/
> Plus a full-stop at the end of sentence.
     okay, I will do it in next version.
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>>
>> binding
> What is that?

     This will be removed
>
>> ---
>>
>> Changes in v6:
>> - fix a typo with "property"
>> - describe property "mask" more clear
>>
>> Changes in v5:
>> - delete a unnecessary blank line in syscon-reboot-mode.txt
>> - rename mode-fastoboot to mode-bootloader in syscon-reboot-mode.txt
>> - rename macro BOOT_LOADER to BOOT_BL_DOWNLOAD, which gives a more clear mean
>>
>> Changes in v4:
>> - remove mode-maskrom
>> - rename mode-fastboot to mode-bootloader to keep compatible with the exiting Android device
>>
>> Changes in v3:
>> - descirbe all reboot mode as properity instead of subnode
>>
>> Changes in v2: None
>> Changes in v1: None
>>
>>   .../bindings/power/reset/reboot-mode.txt           | 24 +++++++++++++++
>>   .../bindings/power/reset/syscon-reboot-mode.txt    | 35 ++++++++++++++++++++++
>>   2 files changed, 59 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..3457949
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> @@ -0,0 +1,24 @@
>> +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
> s/stores/store/
>> +or ram . Then the bootloader can read it and take different
> s/ram ./ram./
>
>> +action according the argument stored.
> according to
>
>> +
>> +All mode properties are vendor specific, it is a indication to tell
>> +the bootloder what to do when the system reboot, and should be named
> s/bootloder/bootloader/
> s/reboot/reboots/
>
>> +as mode-xxx = <magic> (xxx is mode name).
>> +
> These are examples? If yes then add a note, like:
>
> +For example modes common on Android platform:

     Thanks for your carefully review, all these will be fixed in next 
version.
>> +- mode-normal: Normal reboot mode, system reboot with command "reboot".
>> +- mode-recovery: Android Recovery mode, it is a mode to format the device or update a new image.
>> +- mode-bootloader: Android fastboot mode, it's a mode to re-flash partitions on the Android based device.
>> +- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
>> +              usually used in development.
> The "bootloader" and "loader" examples are very confusing. Maybe just
> name the first one as "mode-fastboot" and second as
> "rockchip-download"? The term "loader" does not look for me as
> anything related to uploading/downloading new firmware.

     About these two modes, we have a long discussion with Rob and John [0]
     We keep them as what you see now to keep compatibility with exiting 
devices.

  [0] https://lkml.org/lkml/2016/2/2/128
>
>> +
>> +Example:
>> +       reboot-mode {
>> +               mode-normal = <BOOT_NORMAL>;
>> +               mode-recovery = <BOOT_RECOVERY>;
>> +               mode-bootloader = <BOOT_FASTBOOT>;
>> +               mode-loader = <BOOT_BL_DOWNLOAD>;
>> +       }
>> 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..eb28469
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
>> @@ -0,0 +1,35 @@
>> +SYSCON reboot mode driver
>> +
>> +This driver get reboot mode magic value form reboot-mode driver
> s/get/gets/
>
>> +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)
>> +
>> +Optional property:
>> +- mask: bits mask of the bits in the register to store the reboot mode magic value,
>> +  default set to 0xffffffff if missing.
>> +
>> +The rest of the properties should follow the generic reboot-mode discription
> s/discription/description/
>
> Please, run the spell check...
>
> Best regards,
> Krzysztof
>
>
>
>

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

[Krzysztof Kozlowski]

On 24.03.2016 16:27, Andy Yan wrote:
>>> +- mode-normal: Normal reboot mode, system reboot with command "reboot".
>>> +- mode-recovery: Android Recovery mode, it is a mode to format the
>>> device or update a new image.
>>> +- mode-bootloader: Android fastboot mode, it's a mode to re-flash
>>> partitions on the Android based device.
>>> +- mode-loader: A bootloader mode, it's a mode used to download image
>>> on Rockchip platform,
>>> +              usually used in development.
>> The "bootloader" and "loader" examples are very confusing. Maybe just
>> name the first one as "mode-fastboot" and second as
>> "rockchip-download"? The term "loader" does not look for me as
>> anything related to uploading/downloading new firmware.
> 
>     About these two modes, we have a long discussion with Rob and John [0]
>     We keep them as what you see now to keep compatibility with exiting
> devices.
> 
>  [0] https://lkml.org/lkml/2016/2/2/128

To me these names are non-obvious and I would rather expect choosing
meaningful names when putting something into the mainline. The user
(Android) will use this mainline kernel release in a year or more (3
years for vendors?), so it will have plenty of time to adopt. :)

But this was already discussed and you got ack from Rob, so let's move
on. I don't mind.

Best regards,
Krzysztof

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

[Andy Yan]

Hi Krzystof:

On 2016年03月24日 10:50, Krzysztof Kozlowski wrote:
> Cool work! Few comments below.
>
>
> On Tue, Mar 22, 2016 at 8:37 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 place like special register or sram , 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, in order to
>> reboot the device into fastboot or recovery mode.
>>
>> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
>> Reviewed-by: Moritz Fischer <moritz.fischer@ettus.com>
>> Tested-by: John Stultz <john.stultz@linaro.org>
>> Acked-by: John Stultz <john.stultz@linaro.org>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>
>> ---
>>
>> Changes in v6: None
>> Changes in v5:
>> - use two blank space under help in Kconfig
>> - use unsigned int instead of int for member magic in struct mode_info
>>
>> Changes in v4:
>> - make this driver depends on OF to avoid kbuild test error
>>
>> Changes in v3:
>> - scan multi properities
>> - add mask value for some platform which only use some bits of the register
>>    to store boot mode magic value
>>
>> 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              |  13 ++++
>>   drivers/power/reset/Makefile             |   2 +
>>   drivers/power/reset/reboot-mode.c        | 106 +++++++++++++++++++++++++++++++
>>   drivers/power/reset/reboot-mode.h        |   6 ++
>>   drivers/power/reset/syscon-reboot-mode.c |  65 +++++++++++++++++++
>>   5 files changed, 192 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..cf50630 100644
>> --- a/drivers/power/reset/Kconfig
>> +++ b/drivers/power/reset/Kconfig
>> @@ -173,5 +173,18 @@ config POWER_RESET_ZX
>>          help
>>            Reboot support for ZTE SoCs.
>>
>> +config REBOOT_MODE
>> +       tristate
>> +
>> +config SYSCON_REBOOT_MODE
>> +       bool "Generic SYSCON regmap reboot mode driver"
>> +       depends on OF
>> +       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..9aa7b80
>> --- /dev/null
>> +++ b/drivers/power/reset/reboot-mode.c
>> @@ -0,0 +1,106 @@
>> +/*
>> + * 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"
>> +
>> +#define PREFIX "mode-"
>> +
>> +struct mode_info {
>> +       char mode[32];
>> +       unsigned 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;
> In absence of 'normal' mode (it is not described as required property)
> the magic will be '0'. It would be nice to document that in bindings.
> Imagine someone forgets about this and will wonder why 0x0 is written
> to his precious register on normal reboot...

     If the magic value is '0', we won't touch the register, please see 
reboot_mode_notify bellow.
> It would be nice to document that 'mode-normal' has a special
> (hard-coded) meaning.
>
>> +}
>> +
>> +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 property *prop;
>> +       size_t len = strlen(PREFIX);
>> +       int ret;
>> +
>> +       reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
>> +       if (!reboot)
>> +               return -ENOMEM;
>> +
>> +       reboot->write = write;
>> +       INIT_LIST_HEAD(&reboot->head);
>> +       for_each_property_of_node(dev->of_node, prop) {
>> +               if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, len))
>> +                       continue;
> New line please for readability.

     Okay
>
>> +               info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>> +               if (!info)
>> +                       return -ENOMEM;
> Ditto.

     Okay
>
>> +               strcpy(info->mode, prop->name + len);
> Ehm, and how do you protect that name of mode is shorter than 32 characters?

     How about 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);
>> +       }
>> +       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..44ed34f
>> --- /dev/null
>> +++ b/drivers/power/reset/reboot-mode.h
>> @@ -0,0 +1,6 @@
>> +#ifndef __REBOOT_MODE_H__
>> +#define __REBOOT_MODE_H__
>> +
>> +int reboot_mode_register(struct device *dev, int (*write)(int));
> Documentation would be appreciated. Although it is local header but
> you decoupled them and you are exporting the function.

     I think I should remove the EXPORT_SYMBOL_GPL.
>
>> +
>> +#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..d1521d8
>> --- /dev/null
>> +++ b/drivers/power/reset/syscon-reboot-mode.c
>> @@ -0,0 +1,65 @@
>> +/*
>> + * 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>
> I don't see the need of of_address or am I missing something?

     Yes, it useless, I will remove it.
>
>> +#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 u32 mask = 0xffffffff;
> Why this cannot be part of state container? I suppose usually there
> will be only one reboot mode handler, but:
> 1. It is always nice to make things self-contained, so code could be
> easily used in different contexts. Depending on static variables
> prevents that.
> 2. How do you protect against double binding (two reboot mode device
> nodes in DT)? The second binding (put by mistake... or whatever
> reason) will overwrite data used by previous device.

     Okay, I will make this self-contained.
>> +
>> +static int syscon_reboot_mode_write(int magic)
>> +{
>> +       regmap_update_bits(map, offset, mask, magic);
>> +
>> +       return 0;
> Why ignoring return value of regmap_update_bits?

     Okay, I will check it.
>
>> +}
>> +
>> +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);
> Empty line for readability.
     ok
>> +       if (of_property_read_u32(pdev->dev.of_node, "offset", &offset))
>> +               return -EINVAL;
> Empty line for readability.
     ok
>
> Best regards,
> Krzysztof
>
>
>
>

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

[Krzysztof Kozlowski]

On 24.03.2016 17:03, Andy Yan wrote:
> Hi Krzystof:

(...)

>>> +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;
>> In absence of 'normal' mode (it is not described as required property)
>> the magic will be '0'. It would be nice to document that in bindings.
>> Imagine someone forgets about this and will wonder why 0x0 is written
>> to his precious register on normal reboot...
> 
>     If the magic value is '0', we won't touch the register, please see
> reboot_mode_notify bellow.

Ah, indeed... so we cannot use value of '0' for magic (e.g. to clear any
existing value for normal reboot)?

>> It would be nice to document that 'mode-normal' has a special
>> (hard-coded) meaning.
>>
>>> +}
>>> +
>>> +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 property *prop;
>>> +       size_t len = strlen(PREFIX);
>>> +       int ret;
>>> +
>>> +       reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
>>> +       if (!reboot)
>>> +               return -ENOMEM;
>>> +
>>> +       reboot->write = write;
>>> +       INIT_LIST_HEAD(&reboot->head);
>>> +       for_each_property_of_node(dev->of_node, prop) {
>>> +               if (len > strlen(prop->name) || strncmp(prop->name,
>>> PREFIX, len))
>>> +                       continue;
>> New line please for readability.
> 
>     Okay
>>
>>> +               info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>>> +               if (!info)
>>> +                       return -ENOMEM;
>> Ditto.
> 
>     Okay
>>
>>> +               strcpy(info->mode, prop->name + len);
>> Ehm, and how do you protect that name of mode is shorter than 32
>> characters?
> 
>     How about info->mode = prop->name + len ?

I don't get your answer.
As fair as I read the code, the prop->name can be very long and you are
copying it from 5 character. If the name of the mode has bazillion
characters then again my question: how do you protect that it will fit
in 32 bytes of 'mode'?


>>
>>> +               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);
>>> +       }
>>> +       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..44ed34f
>>> --- /dev/null
>>> +++ b/drivers/power/reset/reboot-mode.h
>>> @@ -0,0 +1,6 @@
>>> +#ifndef __REBOOT_MODE_H__
>>> +#define __REBOOT_MODE_H__
>>> +
>>> +int reboot_mode_register(struct device *dev, int (*write)(int));
>> Documentation would be appreciated. Although it is local header but
>> you decoupled them and you are exporting the function.
> 
>     I think I should remove the EXPORT_SYMBOL_GPL.

You need to export symbols which are located in modules. Is it located
in module?

Best regards,
Krzysztof

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

[Andy Yan]

Hi Krzysztof :

On 2016年03月28日 14:34, Krzysztof Kozlowski wrote:
> On 24.03.2016 17:03, Andy Yan wrote:
>> Hi Krzystof:
> (...)
>
>>>> +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;
>>> In absence of 'normal' mode (it is not described as required property)
>>> the magic will be '0'. It would be nice to document that in bindings.
>>> Imagine someone forgets about this and will wonder why 0x0 is written
>>> to his precious register on normal reboot...
>>      If the magic value is '0', we won't touch the register, please see
>> reboot_mode_notify bellow.
> Ah, indeed... so we cannot use value of '0' for magic (e.g. to clear any
> existing value for normal reboot)?

     It seems that the value '0' cannot be used.
>>> It would be nice to document that 'mode-normal' has a special
>>> (hard-coded) meaning.
>>>
>>>> +}
>>>> +
>>>> +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 property *prop;
>>>> +       size_t len = strlen(PREFIX);
>>>> +       int ret;
>>>> +
>>>> +       reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
>>>> +       if (!reboot)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       reboot->write = write;
>>>> +       INIT_LIST_HEAD(&reboot->head);
>>>> +       for_each_property_of_node(dev->of_node, prop) {
>>>> +               if (len > strlen(prop->name) || strncmp(prop->name,
>>>> PREFIX, len))
>>>> +                       continue;
>>> New line please for readability.
>>      Okay
>>>> +               info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>>>> +               if (!info)
>>>> +                       return -ENOMEM;
>>> Ditto.
>>      Okay
>>>> +               strcpy(info->mode, prop->name + len);
>>> Ehm, and how do you protect that name of mode is shorter than 32
>>> characters?
>>      How about info->mode = prop->name + len ?
> I don't get your answer.
> As fair as I read the code, the prop->name can be very long and you are
> copying it from 5 character. If the name of the mode has bazillion
> characters then again my question: how do you protect that it will fit
> in 32 bytes of 'mode'?

     What I mean is set info->mode as a pointer point to prop->name + len

    struct mode_info {
             char *mode;
             ..........
             .........
    }

    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);
>>>> +       }
>>>> +       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..44ed34f
>>>> --- /dev/null
>>>> +++ b/drivers/power/reset/reboot-mode.h
>>>> @@ -0,0 +1,6 @@
>>>> +#ifndef __REBOOT_MODE_H__
>>>> +#define __REBOOT_MODE_H__
>>>> +
>>>> +int reboot_mode_register(struct device *dev, int (*write)(int));
>>> Documentation would be appreciated. Although it is local header but
>>> you decoupled them and you are exporting the function.
>>      I think I should remove the EXPORT_SYMBOL_GPL.
> You need to export symbols which are located in modules. Is it located
> in module?
>
> Best regards,
> Krzysztof
>
>
>
>
>

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

[Krzysztof Kozlowski]

On 28.03.2016 16:40, Andy Yan wrote:
> Hi Krzysztof :
> 
> On 2016年03月28日 14:34, Krzysztof Kozlowski wrote:
>> On 24.03.2016 17:03, Andy Yan wrote:
>>> Hi Krzystof:
>> (...)
>>
>>>>> +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;
>>>> In absence of 'normal' mode (it is not described as required property)
>>>> the magic will be '0'. It would be nice to document that in bindings.
>>>> Imagine someone forgets about this and will wonder why 0x0 is written
>>>> to his precious register on normal reboot...
>>>      If the magic value is '0', we won't touch the register, please see
>>> reboot_mode_notify bellow.
>> Ah, indeed... so we cannot use value of '0' for magic (e.g. to clear any
>> existing value for normal reboot)?
> 
>     It seems that the value '0' cannot be used.

How about mentioning it in bindings documentation?

(...)

>>>>> +               strcpy(info->mode, prop->name + len);
>>>> Ehm, and how do you protect that name of mode is shorter than 32
>>>> characters?
>>>      How about info->mode = prop->name + len ?
>> I don't get your answer.
>> As fair as I read the code, the prop->name can be very long and you are
>> copying it from 5 character. If the name of the mode has bazillion
>> characters then again my question: how do you protect that it will fit
>> in 32 bytes of 'mode'?
> 
>     What I mean is set info->mode as a pointer point to prop->name + len
> 
>    struct mode_info {
>             char *mode;
>             ..........
>             .........
>    }
> 
>    info->mode = prop->name + len

Ahh, I get it. Then I guess you should also do of_node_get() and
of_node_put()... and use kstrdup_const(). Looks good but I am not
familiar with overlays and life-cycle of OF nodes (documentation for the
life-cycle is a todo list item: Documentation/devicetree/todo.txt).

Best regards,
Krzysztof

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

[Andy Yan]

Hi Krzysztof:

On 2016年03月24日 10:50, Krzysztof Kozlowski wrote:
> Cool work! Few comments below.
>
>
> On Tue, Mar 22, 2016 at 8:37 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 place like special register or sram , 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, in order to
>> reboot the device into fastboot or recovery mode.
>>
>> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
>> Reviewed-by: Moritz Fischer <moritz.fischer@ettus.com>
>> Tested-by: John Stultz <john.stultz@linaro.org>
>> Acked-by: John Stultz <john.stultz@linaro.org>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>
>> ---
>>
>> Changes in v6: None
>> Changes in v5:
>> - use two blank space under help in Kconfig
>> - use unsigned int instead of int for member magic in struct mode_info
>>
>> Changes in v4:
>> - make this driver depends on OF to avoid kbuild test error
>>
>> Changes in v3:
>> - scan multi properities
>> - add mask value for some platform which only use some bits of the register
>>    to store boot mode magic value
>>
>> 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              |  13 ++++
>>   drivers/power/reset/Makefile             |   2 +
>>   drivers/power/reset/reboot-mode.c        | 106 +++++++++++++++++++++++++++++++
>>   drivers/power/reset/reboot-mode.h        |   6 ++
>>   drivers/power/reset/syscon-reboot-mode.c |  65 +++++++++++++++++++
>>   5 files changed, 192 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..cf50630 100644
>> --- a/drivers/power/reset/Kconfig
>> +++ b/drivers/power/reset/Kconfig
>> @@ -173,5 +173,18 @@ config POWER_RESET_ZX
>>          help
>>            Reboot support for ZTE SoCs.
>>
>> +config REBOOT_MODE
>> +       tristate
>> +
>> +config SYSCON_REBOOT_MODE
>> +       bool "Generic SYSCON regmap reboot mode driver"
>> +       depends on OF
>> +       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..9aa7b80
>> --- /dev/null
>> +++ b/drivers/power/reset/reboot-mode.c
>> @@ -0,0 +1,106 @@
>> +/*
>> + * 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"
>> +
>> +#define PREFIX "mode-"
>> +
>> +struct mode_info {
>> +       char mode[32];
>> +       unsigned 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;
> In absence of 'normal' mode (it is not described as required property)
> the magic will be '0'. It would be nice to document that in bindings.
> Imagine someone forgets about this and will wonder why 0x0 is written
> to his precious register on normal reboot...
>
> It would be nice to document that 'mode-normal' has a special
> (hard-coded) meaning.
>
>> +}
>> +
>> +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 property *prop;
>> +       size_t len = strlen(PREFIX);
>> +       int ret;
>> +
>> +       reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
>> +       if (!reboot)
>> +               return -ENOMEM;
>> +
>> +       reboot->write = write;
>> +       INIT_LIST_HEAD(&reboot->head);
>> +       for_each_property_of_node(dev->of_node, prop) {
>> +               if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, len))
>> +                       continue;
> New line please for readability.
>
>> +               info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>> +               if (!info)
>> +                       return -ENOMEM;
> Ditto.
>
>> +               strcpy(info->mode, prop->name + len);
> Ehm, and how do you protect that name of mode is shorter than 32 characters?
>
>> +               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);
>> +       }
>> +       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..44ed34f
>> --- /dev/null
>> +++ b/drivers/power/reset/reboot-mode.h
>> @@ -0,0 +1,6 @@
>> +#ifndef __REBOOT_MODE_H__
>> +#define __REBOOT_MODE_H__
>> +
>> +int reboot_mode_register(struct device *dev, int (*write)(int));
> Documentation would be appreciated. Although it is local header but
> you decoupled them and you are exporting the function.

    You want this driver being a module in V9, so you may also want
  I document this function when a exporting it. Where I should write the
  documentation, and would you please give me some example?
>> +
>> +#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..d1521d8
>> --- /dev/null
>> +++ b/drivers/power/reset/syscon-reboot-mode.c
>> @@ -0,0 +1,65 @@
>> +/*
>> + * 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>
> I don't see the need of of_address or am I missing something?
>
>> +#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 u32 mask = 0xffffffff;
> Why this cannot be part of state container? I suppose usually there
> will be only one reboot mode handler, but:
> 1. It is always nice to make things self-contained, so code could be
> easily used in different contexts. Depending on static variables
> prevents that.
> 2. How do you protect against double binding (two reboot mode device
> nodes in DT)? The second binding (put by mistake... or whatever
> reason) will overwrite data used by previous device.
>
>> +
>> +static int syscon_reboot_mode_write(int magic)
>> +{
>> +       regmap_update_bits(map, offset, mask, magic);
>> +
>> +       return 0;
> Why ignoring return value of regmap_update_bits?
>
>> +}
>> +
>> +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);
> Empty line for readability.
>
>> +       if (of_property_read_u32(pdev->dev.of_node, "offset", &offset))
>> +               return -EINVAL;
> Empty line for readability.
>
> Best regards,
> Krzysztof
>
>
>
>

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

[Krzysztof Kozlowski]

On 06/21/2016 01:01 PM, Andy Yan wrote:

(...)

>>> +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..44ed34f
>>> --- /dev/null
>>> +++ b/drivers/power/reset/reboot-mode.h
>>> @@ -0,0 +1,6 @@
>>> +#ifndef __REBOOT_MODE_H__
>>> +#define __REBOOT_MODE_H__
>>> +
>>> +int reboot_mode_register(struct device *dev, int (*write)(int));
>> Documentation would be appreciated. Although it is local header but
>> you decoupled them and you are exporting the function.
> 
>    You want this driver being a module in V9, so you may also want
>  I document this function when a exporting it. Where I should write the
>  documentation, and would you please give me some example?

Follow kernel-doc (kernel-doc-nano-HOWTO.txt). There is an example. Put
the documentation of functions next to them in source file (not header).

Best regards,
Krzysztof