[PATCH v3 0/4] add reboot mode driver

[PATCH v3 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 v3:
- descirbe all reboot mode as properity instead of subnode
- add mask value for some platform which only use some bits of the register
  to store boot mode magic value
- add rk3036 support

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 node
  ARM64: dts: rockchip: add syscon-reboot-mode DT node

 .../bindings/power/reset/reboot-mode.txt           |  26 +++++
 .../bindings/power/reset/syscon-reboot-mode.txt    |  36 +++++++
 arch/arm/boot/dts/rk3036.dtsi                      |  10 +-
 arch/arm/boot/dts/rk3288.dtsi                      |  11 +++
 arch/arm/boot/dts/rk3xxx.dtsi                      |  13 ++-
 arch/arm64/boot/dts/rockchip/rk3368.dtsi           |  14 ++-
 drivers/power/reset/Kconfig                        |  16 ++++
 drivers/power/reset/Makefile                       |   2 +
 drivers/power/reset/reboot-mode.c                  | 105 +++++++++++++++++++++
 drivers/power/reset/reboot-mode.h                  |   6 ++
 drivers/power/reset/syscon-reboot-mode.c           |  64 +++++++++++++
 include/dt-bindings/soc/rockchip_boot-mode.h       |  30 ++++++
 12 files changed, 330 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 v3 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 v3:
- descirbe all reboot mode as properity instead of subnode
- add mask value for some platform which only use some bits of the register
  to store boot mode magic value
- add rk3036 support

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 node
  ARM64: dts: rockchip: add syscon-reboot-mode DT node

 .../bindings/power/reset/reboot-mode.txt           |  26 +++++
 .../bindings/power/reset/syscon-reboot-mode.txt    |  36 +++++++
 arch/arm/boot/dts/rk3036.dtsi                      |  10 +-
 arch/arm/boot/dts/rk3288.dtsi                      |  11 +++
 arch/arm/boot/dts/rk3xxx.dtsi                      |  13 ++-
 arch/arm64/boot/dts/rockchip/rk3368.dtsi           |  14 ++-
 drivers/power/reset/Kconfig                        |  16 ++++
 drivers/power/reset/Makefile                       |   2 +
 drivers/power/reset/reboot-mode.c                  | 105 +++++++++++++++++++++
 drivers/power/reset/reboot-mode.h                  |   6 ++
 drivers/power/reset/syscon-reboot-mode.c           |  64 +++++++++++++
 include/dt-bindings/soc/rockchip_boot-mode.h       |  30 ++++++
 12 files changed, 330 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 v3 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>

---

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           | 26 ++++++++++++++++
 .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
 2 files changed, 62 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..517080f
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
@@ -0,0 +1,26 @@
+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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.
+- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
+	       usually used in development.
+- mode-maskrom: It's a mode to download bootloader on Rockchip platform.
+
+Example:
+	reboot-mode {
+		mode-normal = <BOOT_NORMAL>;
+		mode-recovery = <BOOT_RECOVERY>;
+		mode-fastboot = <BOOT_FASTBOOT>;
+		mode-loader = <BOOT_LOADER>;
+		mode-maskrom = <BOOT_MASKROM>;
+	}
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..923c82b
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
@@ -0,0 +1,36 @@
+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 properity:
+- mask: the mask bits of the 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-fastboot = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_LOADER>;
+			mode-maskrom = <BOOT_MASKROM>;
+
+		};
+	};
-- 
1.9.1

[PATCH v3 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>

---

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           | 26 ++++++++++++++++
 .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
 2 files changed, 62 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..517080f
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
@@ -0,0 +1,26 @@
+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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.
+- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
+	       usually used in development.
+- mode-maskrom: It's a mode to download bootloader on Rockchip platform.
+
+Example:
+	reboot-mode {
+		mode-normal = <BOOT_NORMAL>;
+		mode-recovery = <BOOT_RECOVERY>;
+		mode-fastboot = <BOOT_FASTBOOT>;
+		mode-loader = <BOOT_LOADER>;
+		mode-maskrom = <BOOT_MASKROM>;
+	}
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..923c82b
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
@@ -0,0 +1,36 @@
+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 properity:
+- mask: the mask bits of the 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 at 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-fastboot = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_LOADER>;
+			mode-maskrom = <BOOT_MASKROM>;
+
+		};
+	};
-- 
1.9.1

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

[John Stultz]

On Tue, Feb 2, 2016 at 1:59 AM, Andy Yan <andy.yan@rock-chips.com> wrote:
> add device tree bindings document for reboot-mode driver
>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>
> ---
>
> 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           | 26 ++++++++++++++++
>  .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
>  2 files changed, 62 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..517080f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> @@ -0,0 +1,26 @@
> +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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.

One minor tweak here, on most Android devices (atleast most Nexus
devices) getting to fastboot is done via "reboot bootloader" not
"reboot fastboot".

If we are going to document the commands here to establish a standard,
I'd prefer we use use what existing userspace expects:

So across nexus devices, there's really two consistent commands:
"bootloader", and "recovery"

Nexus 6p:  https://android.googlesource.com/kernel/msm.git/+/android-msm-bullhead-3.10-marshmallow-mr1/drivers/power/reset/msm-poweroff.c#230
Nexus 9: https://android.googlesource.com/kernel/tegra.git/+/android-tegra-flounder-3.10-marshmallow-mr1/drivers/htc_debug/stability/reboot_params.c#86
Nexus 7: https://android.googlesource.com/kernel/msm.git/+/android-msm-flo-3.4-marshmallow-mr1/arch/arm/mach-msm/restart.c#273
Nexus 10: https://android.googlesource.com/kernel/exynos.git/+/android-exynos-manta-3.4-lollipop-mr1/arch/arm/mach-exynos/board-manta-power.c#422

While I don't object to having a duplicative mode-fastboot (it is more
clear as to what the command does), or the other oem specific modes, I
think we should make it clear that the Android userspace expects the
"bootloader" and "recovery" commands and document them here.

thanks
-john

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

[John Stultz]

On Tue, Feb 2, 2016 at 1:59 AM, Andy Yan <andy.yan@rock-chips.com> wrote:
> add device tree bindings document for reboot-mode driver
>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>
> ---
>
> 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           | 26 ++++++++++++++++
>  .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
>  2 files changed, 62 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..517080f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> @@ -0,0 +1,26 @@
> +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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.

One minor tweak here, on most Android devices (atleast most Nexus
devices) getting to fastboot is done via "reboot bootloader" not
"reboot fastboot".

If we are going to document the commands here to establish a standard,
I'd prefer we use use what existing userspace expects:

So across nexus devices, there's really two consistent commands:
"bootloader", and "recovery"

Nexus 6p:  https://android.googlesource.com/kernel/msm.git/+/android-msm-bullhead-3.10-marshmallow-mr1/drivers/power/reset/msm-poweroff.c#230
Nexus 9: https://android.googlesource.com/kernel/tegra.git/+/android-tegra-flounder-3.10-marshmallow-mr1/drivers/htc_debug/stability/reboot_params.c#86
Nexus 7: https://android.googlesource.com/kernel/msm.git/+/android-msm-flo-3.4-marshmallow-mr1/arch/arm/mach-msm/restart.c#273
Nexus 10: https://android.googlesource.com/kernel/exynos.git/+/android-exynos-manta-3.4-lollipop-mr1/arch/arm/mach-exynos/board-manta-power.c#422

While I don't object to having a duplicative mode-fastboot (it is more
clear as to what the command does), or the other oem specific modes, I
think we should make it clear that the Android userspace expects the
"bootloader" and "recovery" commands and document them here.

thanks
-john

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

[John Stultz]

On Tue, Feb 2, 2016 at 1:59 AM, Andy Yan <andy.yan@rock-chips.com> wrote:
> add device tree bindings document for reboot-mode driver
>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>
> ---
>
> 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           | 26 ++++++++++++++++
>  .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
>  2 files changed, 62 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..517080f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> @@ -0,0 +1,26 @@
> +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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.

One minor tweak here, on most Android devices (atleast most Nexus
devices) getting to fastboot is done via "reboot bootloader" not
"reboot fastboot".

If we are going to document the commands here to establish a standard,
I'd prefer we use use what existing userspace expects:

So across nexus devices, there's really two consistent commands:
"bootloader", and "recovery"

Nexus 6p:  https://android.googlesource.com/kernel/msm.git/+/android-msm-bullhead-3.10-marshmallow-mr1/drivers/power/reset/msm-poweroff.c#230
Nexus 9: https://android.googlesource.com/kernel/tegra.git/+/android-tegra-flounder-3.10-marshmallow-mr1/drivers/htc_debug/stability/reboot_params.c#86
Nexus 7: https://android.googlesource.com/kernel/msm.git/+/android-msm-flo-3.4-marshmallow-mr1/arch/arm/mach-msm/restart.c#273
Nexus 10: https://android.googlesource.com/kernel/exynos.git/+/android-exynos-manta-3.4-lollipop-mr1/arch/arm/mach-exynos/board-manta-power.c#422

While I don't object to having a duplicative mode-fastboot (it is more
clear as to what the command does), or the other oem specific modes, I
think we should make it clear that the Android userspace expects the
"bootloader" and "recovery" commands and document them here.

thanks
-john

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

[Rob Herring]

On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
> add device tree bindings document for reboot-mode driver
> 
> Signed-off-by: Andy Yan <andy.yan-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> 
> ---
> 
> 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           | 26 ++++++++++++++++
>  .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
>  2 files changed, 62 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..517080f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> @@ -0,0 +1,26 @@
> +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 values should be vendor specific. The property names should not. We 
can allow vendor specific ones, but we need to have a common set.

> +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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.
> +- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
> +	       usually used in development.
> +- mode-maskrom: It's a mode to download bootloader on Rockchip platform.
> +
> +Example:
> +	reboot-mode {
> +		mode-normal = <BOOT_NORMAL>;
> +		mode-recovery = <BOOT_RECOVERY>;
> +		mode-fastboot = <BOOT_FASTBOOT>;

I tend to agree with John on calling this mode-bootloader.

OTOH, fastboot is more specific about what the mode is. The name in DT 
and the userspace name don't necessarily have to be the same.

> +		mode-loader = <BOOT_LOADER>;

This one needs a better name. Maybe it should be 'rockchip,mode-loader' 
as it is vendor specific. Either way, loader is vague. Perhaps 
rockchip,mode-bl-download?

> +		mode-maskrom = <BOOT_MASKROM>;

I think this should be "mode-rom-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..923c82b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
> @@ -0,0 +1,36 @@
> +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.

Whether or not it is a simple-mfd or not depends on the syscon node.

> +
> +Required properties:
> +- compatible: should be "syscon-reboot-mode"
> +- offset: offset in the register map for the storage register (in bytes)
> +
> +Optional properity:
> +- mask: the mask bits of the 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-fastboot = <BOOT_FASTBOOT>;
> +			mode-loader = <BOOT_LOADER>;
> +			mode-maskrom = <BOOT_MASKROM>;
> +
> +		};
> +	};
> -- 
> 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
--
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 v3 1/4] dt-bindings: power: reset: add document for reboot-mode driver

[Rob Herring]

On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
> add device tree bindings document for reboot-mode driver
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> 
> ---
> 
> 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           | 26 ++++++++++++++++
>  .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
>  2 files changed, 62 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..517080f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> @@ -0,0 +1,26 @@
> +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 values should be vendor specific. The property names should not. We 
can allow vendor specific ones, but we need to have a common set.

> +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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.
> +- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
> +	       usually used in development.
> +- mode-maskrom: It's a mode to download bootloader on Rockchip platform.
> +
> +Example:
> +	reboot-mode {
> +		mode-normal = <BOOT_NORMAL>;
> +		mode-recovery = <BOOT_RECOVERY>;
> +		mode-fastboot = <BOOT_FASTBOOT>;

I tend to agree with John on calling this mode-bootloader.

OTOH, fastboot is more specific about what the mode is. The name in DT 
and the userspace name don't necessarily have to be the same.

> +		mode-loader = <BOOT_LOADER>;

This one needs a better name. Maybe it should be 'rockchip,mode-loader' 
as it is vendor specific. Either way, loader is vague. Perhaps 
rockchip,mode-bl-download?

> +		mode-maskrom = <BOOT_MASKROM>;

I think this should be "mode-rom-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..923c82b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
> @@ -0,0 +1,36 @@
> +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.

Whether or not it is a simple-mfd or not depends on the syscon node.

> +
> +Required properties:
> +- compatible: should be "syscon-reboot-mode"
> +- offset: offset in the register map for the storage register (in bytes)
> +
> +Optional properity:
> +- mask: the mask bits of the 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-fastboot = <BOOT_FASTBOOT>;
> +			mode-loader = <BOOT_LOADER>;
> +			mode-maskrom = <BOOT_MASKROM>;
> +
> +		};
> +	};
> -- 
> 1.9.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rob Herring]

On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
> add device tree bindings document for reboot-mode driver
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> 
> ---
> 
> 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           | 26 ++++++++++++++++
>  .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
>  2 files changed, 62 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..517080f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> @@ -0,0 +1,26 @@
> +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 values should be vendor specific. The property names should not. We 
can allow vendor specific ones, but we need to have a common set.

> +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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.
> +- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
> +	       usually used in development.
> +- mode-maskrom: It's a mode to download bootloader on Rockchip platform.
> +
> +Example:
> +	reboot-mode {
> +		mode-normal = <BOOT_NORMAL>;
> +		mode-recovery = <BOOT_RECOVERY>;
> +		mode-fastboot = <BOOT_FASTBOOT>;

I tend to agree with John on calling this mode-bootloader.

OTOH, fastboot is more specific about what the mode is. The name in DT 
and the userspace name don't necessarily have to be the same.

> +		mode-loader = <BOOT_LOADER>;

This one needs a better name. Maybe it should be 'rockchip,mode-loader' 
as it is vendor specific. Either way, loader is vague. Perhaps 
rockchip,mode-bl-download?

> +		mode-maskrom = <BOOT_MASKROM>;

I think this should be "mode-rom-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..923c82b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
> @@ -0,0 +1,36 @@
> +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.

Whether or not it is a simple-mfd or not depends on the syscon node.

> +
> +Required properties:
> +- compatible: should be "syscon-reboot-mode"
> +- offset: offset in the register map for the storage register (in bytes)
> +
> +Optional properity:
> +- mask: the mask bits of the 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 at 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-fastboot = <BOOT_FASTBOOT>;
> +			mode-loader = <BOOT_LOADER>;
> +			mode-maskrom = <BOOT_MASKROM>;
> +
> +		};
> +	};
> -- 
> 1.9.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[John Stultz]

On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
>> add device tree bindings document for reboot-mode driver
>>
>> Signed-off-by: Andy Yan <andy.yan-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>
>> ---
>>
>> 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           | 26 ++++++++++++++++
>>  .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
>>  2 files changed, 62 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..517080f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> @@ -0,0 +1,26 @@
>> +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 values should be vendor specific. The property names should not. We
> can allow vendor specific ones, but we need to have a common set.
>
>> +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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.
>> +- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
>> +            usually used in development.
>> +- mode-maskrom: It's a mode to download bootloader on Rockchip platform.
>> +
>> +Example:
>> +     reboot-mode {
>> +             mode-normal = <BOOT_NORMAL>;
>> +             mode-recovery = <BOOT_RECOVERY>;
>> +             mode-fastboot = <BOOT_FASTBOOT>;
>
> I tend to agree with John on calling this mode-bootloader.
>
> OTOH, fastboot is more specific about what the mode is. The name in DT
> and the userspace name don't necessarily have to be the same.

Wait. This is a bit confusing. The utility of adding a property name
and using that name be the reboot command parsed for made sense
(compared to earlier versions which had command strings) as it made
the dts more terse.   But it sounds here like you're suggesting we
should have some logic in the driver that translates "reboot fastboot"
to mode-bootloader or vice versa.

>> +             mode-loader = <BOOT_LOADER>;
>
> This one needs a better name. Maybe it should be 'rockchip,mode-loader'
> as it is vendor specific. Either way, loader is vague. Perhaps
> rockchip,mode-bl-download?

Hrm. So how what reboot command do you expect to trigger that?

Though I do like the vendor specific prefix here, as it clarifies how
universal the command is (and its easy add standard commands if they
become more established and remap things in the future).

>
>> +             mode-maskrom = <BOOT_MASKROM>;
>
> I think this should be "mode-rom-download".

I think one of the difficult things here is that there's no real
standards for all bios/bootloader modes. So they are somewhat
firmware/bootloader/device specific, and thus we need something that
is flexible enough to allow lots of different modes to be easily
specified.  That said, this does expose a userspace interface (though
one could argue kernel ABI doesn't cross reboots :) so we should try
to have some consistency so the same userspace can work on various
devices.

I do think the "bootloader" and "recovery" arguments are somewhat
defacto standards, well established on most android devices.

I think here the concern is rockchip probably has some userspace that
is already using "reboot maskrom" or "reboot loader" for their own
uses. And its a bit of a pain to ask that userspace to be reworked to
use "reboot rom-download" or "reboot rockchip,rom-download" depending
on how we try to deal with these.  (Granted, non-upstream interfaces
aren't official, so that is their risk somewhat, but we avoid being
smug about that :)

Another part of the issue is there isn't really a way to probe for
reboot cmd capability here. As much as I'd rather not complicate
things, one couldn't easily extend existing userspace to work with
current kernels as well as future kernels, since the reboot with an
invalid command won't fail. The machine still resets. So you can't try
one and fallback to the other.

Maybe there needs to be a sysfs entry with the list of the supported commands?

thanks
-john

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

[John Stultz]

On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
>> add device tree bindings document for reboot-mode driver
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>
>> ---
>>
>> 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           | 26 ++++++++++++++++
>>  .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
>>  2 files changed, 62 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..517080f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> @@ -0,0 +1,26 @@
>> +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 values should be vendor specific. The property names should not. We
> can allow vendor specific ones, but we need to have a common set.
>
>> +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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.
>> +- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
>> +            usually used in development.
>> +- mode-maskrom: It's a mode to download bootloader on Rockchip platform.
>> +
>> +Example:
>> +     reboot-mode {
>> +             mode-normal = <BOOT_NORMAL>;
>> +             mode-recovery = <BOOT_RECOVERY>;
>> +             mode-fastboot = <BOOT_FASTBOOT>;
>
> I tend to agree with John on calling this mode-bootloader.
>
> OTOH, fastboot is more specific about what the mode is. The name in DT
> and the userspace name don't necessarily have to be the same.

Wait. This is a bit confusing. The utility of adding a property name
and using that name be the reboot command parsed for made sense
(compared to earlier versions which had command strings) as it made
the dts more terse.   But it sounds here like you're suggesting we
should have some logic in the driver that translates "reboot fastboot"
to mode-bootloader or vice versa.

>> +             mode-loader = <BOOT_LOADER>;
>
> This one needs a better name. Maybe it should be 'rockchip,mode-loader'
> as it is vendor specific. Either way, loader is vague. Perhaps
> rockchip,mode-bl-download?

Hrm. So how what reboot command do you expect to trigger that?

Though I do like the vendor specific prefix here, as it clarifies how
universal the command is (and its easy add standard commands if they
become more established and remap things in the future).

>
>> +             mode-maskrom = <BOOT_MASKROM>;
>
> I think this should be "mode-rom-download".

I think one of the difficult things here is that there's no real
standards for all bios/bootloader modes. So they are somewhat
firmware/bootloader/device specific, and thus we need something that
is flexible enough to allow lots of different modes to be easily
specified.  That said, this does expose a userspace interface (though
one could argue kernel ABI doesn't cross reboots :) so we should try
to have some consistency so the same userspace can work on various
devices.

I do think the "bootloader" and "recovery" arguments are somewhat
defacto standards, well established on most android devices.

I think here the concern is rockchip probably has some userspace that
is already using "reboot maskrom" or "reboot loader" for their own
uses. And its a bit of a pain to ask that userspace to be reworked to
use "reboot rom-download" or "reboot rockchip,rom-download" depending
on how we try to deal with these.  (Granted, non-upstream interfaces
aren't official, so that is their risk somewhat, but we avoid being
smug about that :)

Another part of the issue is there isn't really a way to probe for
reboot cmd capability here. As much as I'd rather not complicate
things, one couldn't easily extend existing userspace to work with
current kernels as well as future kernels, since the reboot with an
invalid command won't fail. The machine still resets. So you can't try
one and fallback to the other.

Maybe there needs to be a sysfs entry with the list of the supported commands?

thanks
-john

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

[John Stultz]

On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
>> add device tree bindings document for reboot-mode driver
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>
>> ---
>>
>> 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           | 26 ++++++++++++++++
>>  .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
>>  2 files changed, 62 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..517080f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> @@ -0,0 +1,26 @@
>> +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 values should be vendor specific. The property names should not. We
> can allow vendor specific ones, but we need to have a common set.
>
>> +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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.
>> +- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
>> +            usually used in development.
>> +- mode-maskrom: It's a mode to download bootloader on Rockchip platform.
>> +
>> +Example:
>> +     reboot-mode {
>> +             mode-normal = <BOOT_NORMAL>;
>> +             mode-recovery = <BOOT_RECOVERY>;
>> +             mode-fastboot = <BOOT_FASTBOOT>;
>
> I tend to agree with John on calling this mode-bootloader.
>
> OTOH, fastboot is more specific about what the mode is. The name in DT
> and the userspace name don't necessarily have to be the same.

Wait. This is a bit confusing. The utility of adding a property name
and using that name be the reboot command parsed for made sense
(compared to earlier versions which had command strings) as it made
the dts more terse.   But it sounds here like you're suggesting we
should have some logic in the driver that translates "reboot fastboot"
to mode-bootloader or vice versa.

>> +             mode-loader = <BOOT_LOADER>;
>
> This one needs a better name. Maybe it should be 'rockchip,mode-loader'
> as it is vendor specific. Either way, loader is vague. Perhaps
> rockchip,mode-bl-download?

Hrm. So how what reboot command do you expect to trigger that?

Though I do like the vendor specific prefix here, as it clarifies how
universal the command is (and its easy add standard commands if they
become more established and remap things in the future).

>
>> +             mode-maskrom = <BOOT_MASKROM>;
>
> I think this should be "mode-rom-download".

I think one of the difficult things here is that there's no real
standards for all bios/bootloader modes. So they are somewhat
firmware/bootloader/device specific, and thus we need something that
is flexible enough to allow lots of different modes to be easily
specified.  That said, this does expose a userspace interface (though
one could argue kernel ABI doesn't cross reboots :) so we should try
to have some consistency so the same userspace can work on various
devices.

I do think the "bootloader" and "recovery" arguments are somewhat
defacto standards, well established on most android devices.

I think here the concern is rockchip probably has some userspace that
is already using "reboot maskrom" or "reboot loader" for their own
uses. And its a bit of a pain to ask that userspace to be reworked to
use "reboot rom-download" or "reboot rockchip,rom-download" depending
on how we try to deal with these.  (Granted, non-upstream interfaces
aren't official, so that is their risk somewhat, but we avoid being
smug about that :)

Another part of the issue is there isn't really a way to probe for
reboot cmd capability here. As much as I'd rather not complicate
things, one couldn't easily extend existing userspace to work with
current kernels as well as future kernels, since the reboot with an
invalid command won't fail. The machine still resets. So you can't try
one and fallback to the other.

Maybe there needs to be a sysfs entry with the list of the supported commands?

thanks
-john

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

[Rob Herring]

On Thu, Feb 04, 2016 at 03:46:15PM -0800, John Stultz wrote:
> On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
> > On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
> >> add device tree bindings document for reboot-mode driver
> >>
> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> >>
> >> ---
> >>
> >> 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           | 26 ++++++++++++++++
> >>  .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
> >>  2 files changed, 62 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..517080f
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> >> @@ -0,0 +1,26 @@
> >> +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 values should be vendor specific. The property names should not. We
> > can allow vendor specific ones, but we need to have a common set.
> >
> >> +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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.
> >> +- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
> >> +            usually used in development.
> >> +- mode-maskrom: It's a mode to download bootloader on Rockchip platform.
> >> +
> >> +Example:
> >> +     reboot-mode {
> >> +             mode-normal = <BOOT_NORMAL>;
> >> +             mode-recovery = <BOOT_RECOVERY>;
> >> +             mode-fastboot = <BOOT_FASTBOOT>;
> >
> > I tend to agree with John on calling this mode-bootloader.
> >
> > OTOH, fastboot is more specific about what the mode is. The name in DT
> > and the userspace name don't necessarily have to be the same.
> 
> Wait. This is a bit confusing. The utility of adding a property name
> and using that name be the reboot command parsed for made sense
> (compared to earlier versions which had command strings) as it made
> the dts more terse.   But it sounds here like you're suggesting we
> should have some logic in the driver that translates "reboot fastboot"
> to mode-bootloader or vice versa.

I said early on the DT names and kernel-userspace names should not 
necessarily be linked. They can be, but we shouldn't require that.

My concern with mode-bootloader is what if you can boot into multiple 
bootloader modes. Say USB mass storage is one option. "bootloader" is 
not real specific.

> >> +             mode-loader = <BOOT_LOADER>;
> >
> > This one needs a better name. Maybe it should be 'rockchip,mode-loader'
> > as it is vendor specific. Either way, loader is vague. Perhaps
> > rockchip,mode-bl-download?
> 
> Hrm. So how what reboot command do you expect to trigger that?

Whatever your OS has defined to map to that.

We could just decide the kernel will strip <vendor> and 'mode-' and 
match commands against what remains.

> Though I do like the vendor specific prefix here, as it clarifies how
> universal the command is (and its easy add standard commands if they
> become more established and remap things in the future).
> 
> >
> >> +             mode-maskrom = <BOOT_MASKROM>;
> >
> > I think this should be "mode-rom-download".
> 
> I think one of the difficult things here is that there's no real
> standards for all bios/bootloader modes. So they are somewhat
> firmware/bootloader/device specific, and thus we need something that
> is flexible enough to allow lots of different modes to be easily
> specified.  That said, this does expose a userspace interface (though
> one could argue kernel ABI doesn't cross reboots :) so we should try
> to have some consistency so the same userspace can work on various
> devices.

There is: UEFI. Boot mode efivars are standard. But then they are pretty 
much PC oriented though. It is more which device to boot off of, but 
there is network boot or boot to bios setup.

> I do think the "bootloader" and "recovery" arguments are somewhat
> defacto standards, well established on most android devices.

Yes, otherwise I'd be completely against "mode-bootloader" as the 
property.

> I think here the concern is rockchip probably has some userspace that
> is already using "reboot maskrom" or "reboot loader" for their own
> uses. And its a bit of a pain to ask that userspace to be reworked to

Perhaps those are only for development and change would not be so 
painful.

> use "reboot rom-download" or "reboot rockchip,rom-download" depending
> on how we try to deal with these.  (Granted, non-upstream interfaces
> aren't official, so that is their risk somewhat, but we avoid being
> smug about that :)

Or vendor specific modes require vendor specific translation in the 
kernel. 

To some extent we need to design what is right and worry about future 
devices rather than cater to past devices. There's always some 
compromise. What would you design ignoring the existing conditions. 
Start there and then figure out how to make it work with current 
designs.

> Another part of the issue is there isn't really a way to probe for
> reboot cmd capability here. As much as I'd rather not complicate
> things, one couldn't easily extend existing userspace to work with
> current kernels as well as future kernels, since the reboot with an
> invalid command won't fail. The machine still resets. So you can't try
> one and fallback to the other.

Well, that's nice. Maybe we should change that? Or we're stuck with 
that ABI?

> Maybe there needs to be a sysfs entry with the list of the supported commands?

You can just read the DT. Although, the problem then is what happens 
when we move to the next firmware interface. We see that some with 
devices having userspace dependencies on ATAGS.

Rob

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

[Rob Herring]

On Thu, Feb 04, 2016 at 03:46:15PM -0800, John Stultz wrote:
> On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
> > On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
> >> add device tree bindings document for reboot-mode driver
> >>
> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> >>
> >> ---
> >>
> >> 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           | 26 ++++++++++++++++
> >>  .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
> >>  2 files changed, 62 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..517080f
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> >> @@ -0,0 +1,26 @@
> >> +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 values should be vendor specific. The property names should not. We
> > can allow vendor specific ones, but we need to have a common set.
> >
> >> +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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.
> >> +- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
> >> +            usually used in development.
> >> +- mode-maskrom: It's a mode to download bootloader on Rockchip platform.
> >> +
> >> +Example:
> >> +     reboot-mode {
> >> +             mode-normal = <BOOT_NORMAL>;
> >> +             mode-recovery = <BOOT_RECOVERY>;
> >> +             mode-fastboot = <BOOT_FASTBOOT>;
> >
> > I tend to agree with John on calling this mode-bootloader.
> >
> > OTOH, fastboot is more specific about what the mode is. The name in DT
> > and the userspace name don't necessarily have to be the same.
> 
> Wait. This is a bit confusing. The utility of adding a property name
> and using that name be the reboot command parsed for made sense
> (compared to earlier versions which had command strings) as it made
> the dts more terse.   But it sounds here like you're suggesting we
> should have some logic in the driver that translates "reboot fastboot"
> to mode-bootloader or vice versa.

I said early on the DT names and kernel-userspace names should not 
necessarily be linked. They can be, but we shouldn't require that.

My concern with mode-bootloader is what if you can boot into multiple 
bootloader modes. Say USB mass storage is one option. "bootloader" is 
not real specific.

> >> +             mode-loader = <BOOT_LOADER>;
> >
> > This one needs a better name. Maybe it should be 'rockchip,mode-loader'
> > as it is vendor specific. Either way, loader is vague. Perhaps
> > rockchip,mode-bl-download?
> 
> Hrm. So how what reboot command do you expect to trigger that?

Whatever your OS has defined to map to that.

We could just decide the kernel will strip <vendor> and 'mode-' and 
match commands against what remains.

> Though I do like the vendor specific prefix here, as it clarifies how
> universal the command is (and its easy add standard commands if they
> become more established and remap things in the future).
> 
> >
> >> +             mode-maskrom = <BOOT_MASKROM>;
> >
> > I think this should be "mode-rom-download".
> 
> I think one of the difficult things here is that there's no real
> standards for all bios/bootloader modes. So they are somewhat
> firmware/bootloader/device specific, and thus we need something that
> is flexible enough to allow lots of different modes to be easily
> specified.  That said, this does expose a userspace interface (though
> one could argue kernel ABI doesn't cross reboots :) so we should try
> to have some consistency so the same userspace can work on various
> devices.

There is: UEFI. Boot mode efivars are standard. But then they are pretty 
much PC oriented though. It is more which device to boot off of, but 
there is network boot or boot to bios setup.

> I do think the "bootloader" and "recovery" arguments are somewhat
> defacto standards, well established on most android devices.

Yes, otherwise I'd be completely against "mode-bootloader" as the 
property.

> I think here the concern is rockchip probably has some userspace that
> is already using "reboot maskrom" or "reboot loader" for their own
> uses. And its a bit of a pain to ask that userspace to be reworked to

Perhaps those are only for development and change would not be so 
painful.

> use "reboot rom-download" or "reboot rockchip,rom-download" depending
> on how we try to deal with these.  (Granted, non-upstream interfaces
> aren't official, so that is their risk somewhat, but we avoid being
> smug about that :)

Or vendor specific modes require vendor specific translation in the 
kernel. 

To some extent we need to design what is right and worry about future 
devices rather than cater to past devices. There's always some 
compromise. What would you design ignoring the existing conditions. 
Start there and then figure out how to make it work with current 
designs.

> Another part of the issue is there isn't really a way to probe for
> reboot cmd capability here. As much as I'd rather not complicate
> things, one couldn't easily extend existing userspace to work with
> current kernels as well as future kernels, since the reboot with an
> invalid command won't fail. The machine still resets. So you can't try
> one and fallback to the other.

Well, that's nice. Maybe we should change that? Or we're stuck with 
that ABI?

> Maybe there needs to be a sysfs entry with the list of the supported commands?

You can just read the DT. Although, the problem then is what happens 
when we move to the next firmware interface. We see that some with 
devices having userspace dependencies on ATAGS.

Rob

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

[Rob Herring]

On Thu, Feb 04, 2016 at 03:46:15PM -0800, John Stultz wrote:
> On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
> > On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
> >> add device tree bindings document for reboot-mode driver
> >>
> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> >>
> >> ---
> >>
> >> 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           | 26 ++++++++++++++++
> >>  .../bindings/power/reset/syscon-reboot-mode.txt    | 36 ++++++++++++++++++++++
> >>  2 files changed, 62 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..517080f
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
> >> @@ -0,0 +1,26 @@
> >> +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 values should be vendor specific. The property names should not. We
> > can allow vendor specific ones, but we need to have a common set.
> >
> >> +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-fastboot: Android fastboot mode, it's a mode to  re-flash partitions on the device.
> >> +- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
> >> +            usually used in development.
> >> +- mode-maskrom: It's a mode to download bootloader on Rockchip platform.
> >> +
> >> +Example:
> >> +     reboot-mode {
> >> +             mode-normal = <BOOT_NORMAL>;
> >> +             mode-recovery = <BOOT_RECOVERY>;
> >> +             mode-fastboot = <BOOT_FASTBOOT>;
> >
> > I tend to agree with John on calling this mode-bootloader.
> >
> > OTOH, fastboot is more specific about what the mode is. The name in DT
> > and the userspace name don't necessarily have to be the same.
> 
> Wait. This is a bit confusing. The utility of adding a property name
> and using that name be the reboot command parsed for made sense
> (compared to earlier versions which had command strings) as it made
> the dts more terse.   But it sounds here like you're suggesting we
> should have some logic in the driver that translates "reboot fastboot"
> to mode-bootloader or vice versa.

I said early on the DT names and kernel-userspace names should not 
necessarily be linked. They can be, but we shouldn't require that.

My concern with mode-bootloader is what if you can boot into multiple 
bootloader modes. Say USB mass storage is one option. "bootloader" is 
not real specific.

> >> +             mode-loader = <BOOT_LOADER>;
> >
> > This one needs a better name. Maybe it should be 'rockchip,mode-loader'
> > as it is vendor specific. Either way, loader is vague. Perhaps
> > rockchip,mode-bl-download?
> 
> Hrm. So how what reboot command do you expect to trigger that?

Whatever your OS has defined to map to that.

We could just decide the kernel will strip <vendor> and 'mode-' and 
match commands against what remains.

> Though I do like the vendor specific prefix here, as it clarifies how
> universal the command is (and its easy add standard commands if they
> become more established and remap things in the future).
> 
> >
> >> +             mode-maskrom = <BOOT_MASKROM>;
> >
> > I think this should be "mode-rom-download".
> 
> I think one of the difficult things here is that there's no real
> standards for all bios/bootloader modes. So they are somewhat
> firmware/bootloader/device specific, and thus we need something that
> is flexible enough to allow lots of different modes to be easily
> specified.  That said, this does expose a userspace interface (though
> one could argue kernel ABI doesn't cross reboots :) so we should try
> to have some consistency so the same userspace can work on various
> devices.

There is: UEFI. Boot mode efivars are standard. But then they are pretty 
much PC oriented though. It is more which device to boot off of, but 
there is network boot or boot to bios setup.

> I do think the "bootloader" and "recovery" arguments are somewhat
> defacto standards, well established on most android devices.

Yes, otherwise I'd be completely against "mode-bootloader" as the 
property.

> I think here the concern is rockchip probably has some userspace that
> is already using "reboot maskrom" or "reboot loader" for their own
> uses. And its a bit of a pain to ask that userspace to be reworked to

Perhaps those are only for development and change would not be so 
painful.

> use "reboot rom-download" or "reboot rockchip,rom-download" depending
> on how we try to deal with these.  (Granted, non-upstream interfaces
> aren't official, so that is their risk somewhat, but we avoid being
> smug about that :)

Or vendor specific modes require vendor specific translation in the 
kernel. 

To some extent we need to design what is right and worry about future 
devices rather than cater to past devices. There's always some 
compromise. What would you design ignoring the existing conditions. 
Start there and then figure out how to make it work with current 
designs.

> Another part of the issue is there isn't really a way to probe for
> reboot cmd capability here. As much as I'd rather not complicate
> things, one couldn't easily extend existing userspace to work with
> current kernels as well as future kernels, since the reboot with an
> invalid command won't fail. The machine still resets. So you can't try
> one and fallback to the other.

Well, that's nice. Maybe we should change that? Or we're stuck with 
that ABI?

> Maybe there needs to be a sysfs entry with the list of the supported commands?

You can just read the DT. Although, the problem then is what happens 
when we move to the next firmware interface. We see that some with 
devices having userspace dependencies on ATAGS.

Rob

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

[John Stultz]

On Thu, Feb 4, 2016 at 8:35 PM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Feb 04, 2016 at 03:46:15PM -0800, John Stultz wrote:
>> On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
>> > On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
>> >> +Example:
>> >> +     reboot-mode {
>> >> +             mode-normal = <BOOT_NORMAL>;
>> >> +             mode-recovery = <BOOT_RECOVERY>;
>> >> +             mode-fastboot = <BOOT_FASTBOOT>;
>> >
>> > I tend to agree with John on calling this mode-bootloader.
>> >
>> > OTOH, fastboot is more specific about what the mode is. The name in DT
>> > and the userspace name don't necessarily have to be the same.
>>
>> Wait. This is a bit confusing. The utility of adding a property name
>> and using that name be the reboot command parsed for made sense
>> (compared to earlier versions which had command strings) as it made
>> the dts more terse.   But it sounds here like you're suggesting we
>> should have some logic in the driver that translates "reboot fastboot"
>> to mode-bootloader or vice versa.
>
> I said early on the DT names and kernel-userspace names should not
> necessarily be linked. They can be, but we shouldn't require that.

Sigh. Ok. It seemed it was due to earlier comments (maybe from others,
but I thought it was you), that we moved from specifying a command
string, to using the label. But if you think the label name and the
commands shouldn't be linked, it seems like we should re-introduce
that. No?

Unless your thinking we need some sort of static in-kernel mapping of
commands to label names? But that just seems painfully indirect for
little gain ("Its obvious! For that mode, you use this term here, and
that different term over there!").

> My concern with mode-bootloader is what if you can boot into multiple
> bootloader modes. Say USB mass storage is one option. "bootloader" is
> not real specific.

True. But as I think we agreed below, "bootloader" and "recovery" are
basically defacto standards, and I think it would be a bad idea to try
to declare all the existing android tooling and docs wrong just
because the command is vague, technically.


>> >> +             mode-loader = <BOOT_LOADER>;
>> >
>> > This one needs a better name. Maybe it should be 'rockchip,mode-loader'
>> > as it is vendor specific. Either way, loader is vague. Perhaps
>> > rockchip,mode-bl-download?
>>
>> Hrm. So how what reboot command do you expect to trigger that?
>
> Whatever your OS has defined to map to that.
>
> We could just decide the kernel will strip <vendor> and 'mode-' and
> match commands against what remains.

That part sounds sane, although I do think having vendor prefixes are
reasonable for actual commands as well.


>> I think one of the difficult things here is that there's no real
>> standards for all bios/bootloader modes. So they are somewhat
>> firmware/bootloader/device specific, and thus we need something that
>> is flexible enough to allow lots of different modes to be easily
>> specified.  That said, this does expose a userspace interface (though
>> one could argue kernel ABI doesn't cross reboots :) so we should try
>> to have some consistency so the same userspace can work on various
>> devices.
>
> There is: UEFI. Boot mode efivars are standard. But then they are pretty
> much PC oriented though. It is more which device to boot off of, but
> there is network boot or boot to bios setup.

Well, there's a partial standard there.  I'm told for android on x86,
there is no UEFI standard way to communicate rebooting to fastboot or
recovery. Every device does its own device specific driver.


>> I do think the "bootloader" and "recovery" arguments are somewhat
>> defacto standards, well established on most android devices.
>
> Yes, otherwise I'd be completely against "mode-bootloader" as the
> property.

Ok.

>> I think here the concern is rockchip probably has some userspace that
>> is already using "reboot maskrom" or "reboot loader" for their own
>> uses. And its a bit of a pain to ask that userspace to be reworked to
>
> Perhaps those are only for development and change would not be so
> painful.

Andy: Can you comment here? How critical are the specific commands
you've used here for your userspace?


>> use "reboot rom-download" or "reboot rockchip,rom-download" depending
>> on how we try to deal with these.  (Granted, non-upstream interfaces
>> aren't official, so that is their risk somewhat, but we avoid being
>> smug about that :)
>
> Or vendor specific modes require vendor specific translation in the
> kernel.

True.


> To some extent we need to design what is right and worry about future
> devices rather than cater to past devices. There's always some
> compromise. What would you design ignoring the existing conditions.
> Start there and then figure out how to make it work with current
> designs.

Fair enough. I just want to make sure we're not getting too caught up
with design purity and are willing the meet the world where it is.

>> Another part of the issue is there isn't really a way to probe for
>> reboot cmd capability here. As much as I'd rather not complicate
>> things, one couldn't easily extend existing userspace to work with
>> current kernels as well as future kernels, since the reboot with an
>> invalid command won't fail. The machine still resets. So you can't try
>> one and fallback to the other.
>
> Well, that's nice. Maybe we should change that? Or we're stuck with
> that ABI?

Maybe? I'm not sure what might have trouble with reboot failing if it
sticks any extra noise in the reboot command.
I'd probably lean towards sticking with the existing behavior.

>> Maybe there needs to be a sysfs entry with the list of the supported commands?
>
> You can just read the DT. Although, the problem then is what happens
> when we move to the next firmware interface. We see that some with
> devices having userspace dependencies on ATAGS.

Heh. I guess. Thought I suspect "Just read the DT to sort out the
available reboot modes" is probably not what most userspace wants to
hear. :)

thanks
-john

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

[John Stultz]

On Thu, Feb 4, 2016 at 8:35 PM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Feb 04, 2016 at 03:46:15PM -0800, John Stultz wrote:
>> On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
>> > On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
>> >> +Example:
>> >> +     reboot-mode {
>> >> +             mode-normal = <BOOT_NORMAL>;
>> >> +             mode-recovery = <BOOT_RECOVERY>;
>> >> +             mode-fastboot = <BOOT_FASTBOOT>;
>> >
>> > I tend to agree with John on calling this mode-bootloader.
>> >
>> > OTOH, fastboot is more specific about what the mode is. The name in DT
>> > and the userspace name don't necessarily have to be the same.
>>
>> Wait. This is a bit confusing. The utility of adding a property name
>> and using that name be the reboot command parsed for made sense
>> (compared to earlier versions which had command strings) as it made
>> the dts more terse.   But it sounds here like you're suggesting we
>> should have some logic in the driver that translates "reboot fastboot"
>> to mode-bootloader or vice versa.
>
> I said early on the DT names and kernel-userspace names should not
> necessarily be linked. They can be, but we shouldn't require that.

Sigh. Ok. It seemed it was due to earlier comments (maybe from others,
but I thought it was you), that we moved from specifying a command
string, to using the label. But if you think the label name and the
commands shouldn't be linked, it seems like we should re-introduce
that. No?

Unless your thinking we need some sort of static in-kernel mapping of
commands to label names? But that just seems painfully indirect for
little gain ("Its obvious! For that mode, you use this term here, and
that different term over there!").

> My concern with mode-bootloader is what if you can boot into multiple
> bootloader modes. Say USB mass storage is one option. "bootloader" is
> not real specific.

True. But as I think we agreed below, "bootloader" and "recovery" are
basically defacto standards, and I think it would be a bad idea to try
to declare all the existing android tooling and docs wrong just
because the command is vague, technically.


>> >> +             mode-loader = <BOOT_LOADER>;
>> >
>> > This one needs a better name. Maybe it should be 'rockchip,mode-loader'
>> > as it is vendor specific. Either way, loader is vague. Perhaps
>> > rockchip,mode-bl-download?
>>
>> Hrm. So how what reboot command do you expect to trigger that?
>
> Whatever your OS has defined to map to that.
>
> We could just decide the kernel will strip <vendor> and 'mode-' and
> match commands against what remains.

That part sounds sane, although I do think having vendor prefixes are
reasonable for actual commands as well.


>> I think one of the difficult things here is that there's no real
>> standards for all bios/bootloader modes. So they are somewhat
>> firmware/bootloader/device specific, and thus we need something that
>> is flexible enough to allow lots of different modes to be easily
>> specified.  That said, this does expose a userspace interface (though
>> one could argue kernel ABI doesn't cross reboots :) so we should try
>> to have some consistency so the same userspace can work on various
>> devices.
>
> There is: UEFI. Boot mode efivars are standard. But then they are pretty
> much PC oriented though. It is more which device to boot off of, but
> there is network boot or boot to bios setup.

Well, there's a partial standard there.  I'm told for android on x86,
there is no UEFI standard way to communicate rebooting to fastboot or
recovery. Every device does its own device specific driver.


>> I do think the "bootloader" and "recovery" arguments are somewhat
>> defacto standards, well established on most android devices.
>
> Yes, otherwise I'd be completely against "mode-bootloader" as the
> property.

Ok.

>> I think here the concern is rockchip probably has some userspace that
>> is already using "reboot maskrom" or "reboot loader" for their own
>> uses. And its a bit of a pain to ask that userspace to be reworked to
>
> Perhaps those are only for development and change would not be so
> painful.

Andy: Can you comment here? How critical are the specific commands
you've used here for your userspace?


>> use "reboot rom-download" or "reboot rockchip,rom-download" depending
>> on how we try to deal with these.  (Granted, non-upstream interfaces
>> aren't official, so that is their risk somewhat, but we avoid being
>> smug about that :)
>
> Or vendor specific modes require vendor specific translation in the
> kernel.

True.


> To some extent we need to design what is right and worry about future
> devices rather than cater to past devices. There's always some
> compromise. What would you design ignoring the existing conditions.
> Start there and then figure out how to make it work with current
> designs.

Fair enough. I just want to make sure we're not getting too caught up
with design purity and are willing the meet the world where it is.

>> Another part of the issue is there isn't really a way to probe for
>> reboot cmd capability here. As much as I'd rather not complicate
>> things, one couldn't easily extend existing userspace to work with
>> current kernels as well as future kernels, since the reboot with an
>> invalid command won't fail. The machine still resets. So you can't try
>> one and fallback to the other.
>
> Well, that's nice. Maybe we should change that? Or we're stuck with
> that ABI?

Maybe? I'm not sure what might have trouble with reboot failing if it
sticks any extra noise in the reboot command.
I'd probably lean towards sticking with the existing behavior.

>> Maybe there needs to be a sysfs entry with the list of the supported commands?
>
> You can just read the DT. Although, the problem then is what happens
> when we move to the next firmware interface. We see that some with
> devices having userspace dependencies on ATAGS.

Heh. I guess. Thought I suspect "Just read the DT to sort out the
available reboot modes" is probably not what most userspace wants to
hear. :)

thanks
-john

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

[John Stultz]

On Thu, Feb 4, 2016 at 8:35 PM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Feb 04, 2016 at 03:46:15PM -0800, John Stultz wrote:
>> On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
>> > On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
>> >> +Example:
>> >> +     reboot-mode {
>> >> +             mode-normal = <BOOT_NORMAL>;
>> >> +             mode-recovery = <BOOT_RECOVERY>;
>> >> +             mode-fastboot = <BOOT_FASTBOOT>;
>> >
>> > I tend to agree with John on calling this mode-bootloader.
>> >
>> > OTOH, fastboot is more specific about what the mode is. The name in DT
>> > and the userspace name don't necessarily have to be the same.
>>
>> Wait. This is a bit confusing. The utility of adding a property name
>> and using that name be the reboot command parsed for made sense
>> (compared to earlier versions which had command strings) as it made
>> the dts more terse.   But it sounds here like you're suggesting we
>> should have some logic in the driver that translates "reboot fastboot"
>> to mode-bootloader or vice versa.
>
> I said early on the DT names and kernel-userspace names should not
> necessarily be linked. They can be, but we shouldn't require that.

Sigh. Ok. It seemed it was due to earlier comments (maybe from others,
but I thought it was you), that we moved from specifying a command
string, to using the label. But if you think the label name and the
commands shouldn't be linked, it seems like we should re-introduce
that. No?

Unless your thinking we need some sort of static in-kernel mapping of
commands to label names? But that just seems painfully indirect for
little gain ("Its obvious! For that mode, you use this term here, and
that different term over there!").

> My concern with mode-bootloader is what if you can boot into multiple
> bootloader modes. Say USB mass storage is one option. "bootloader" is
> not real specific.

True. But as I think we agreed below, "bootloader" and "recovery" are
basically defacto standards, and I think it would be a bad idea to try
to declare all the existing android tooling and docs wrong just
because the command is vague, technically.


>> >> +             mode-loader = <BOOT_LOADER>;
>> >
>> > This one needs a better name. Maybe it should be 'rockchip,mode-loader'
>> > as it is vendor specific. Either way, loader is vague. Perhaps
>> > rockchip,mode-bl-download?
>>
>> Hrm. So how what reboot command do you expect to trigger that?
>
> Whatever your OS has defined to map to that.
>
> We could just decide the kernel will strip <vendor> and 'mode-' and
> match commands against what remains.

That part sounds sane, although I do think having vendor prefixes are
reasonable for actual commands as well.


>> I think one of the difficult things here is that there's no real
>> standards for all bios/bootloader modes. So they are somewhat
>> firmware/bootloader/device specific, and thus we need something that
>> is flexible enough to allow lots of different modes to be easily
>> specified.  That said, this does expose a userspace interface (though
>> one could argue kernel ABI doesn't cross reboots :) so we should try
>> to have some consistency so the same userspace can work on various
>> devices.
>
> There is: UEFI. Boot mode efivars are standard. But then they are pretty
> much PC oriented though. It is more which device to boot off of, but
> there is network boot or boot to bios setup.

Well, there's a partial standard there.  I'm told for android on x86,
there is no UEFI standard way to communicate rebooting to fastboot or
recovery. Every device does its own device specific driver.


>> I do think the "bootloader" and "recovery" arguments are somewhat
>> defacto standards, well established on most android devices.
>
> Yes, otherwise I'd be completely against "mode-bootloader" as the
> property.

Ok.

>> I think here the concern is rockchip probably has some userspace that
>> is already using "reboot maskrom" or "reboot loader" for their own
>> uses. And its a bit of a pain to ask that userspace to be reworked to
>
> Perhaps those are only for development and change would not be so
> painful.

Andy: Can you comment here? How critical are the specific commands
you've used here for your userspace?


>> use "reboot rom-download" or "reboot rockchip,rom-download" depending
>> on how we try to deal with these.  (Granted, non-upstream interfaces
>> aren't official, so that is their risk somewhat, but we avoid being
>> smug about that :)
>
> Or vendor specific modes require vendor specific translation in the
> kernel.

True.


> To some extent we need to design what is right and worry about future
> devices rather than cater to past devices. There's always some
> compromise. What would you design ignoring the existing conditions.
> Start there and then figure out how to make it work with current
> designs.

Fair enough. I just want to make sure we're not getting too caught up
with design purity and are willing the meet the world where it is.

>> Another part of the issue is there isn't really a way to probe for
>> reboot cmd capability here. As much as I'd rather not complicate
>> things, one couldn't easily extend existing userspace to work with
>> current kernels as well as future kernels, since the reboot with an
>> invalid command won't fail. The machine still resets. So you can't try
>> one and fallback to the other.
>
> Well, that's nice. Maybe we should change that? Or we're stuck with
> that ABI?

Maybe? I'm not sure what might have trouble with reboot failing if it
sticks any extra noise in the reboot command.
I'd probably lean towards sticking with the existing behavior.

>> Maybe there needs to be a sysfs entry with the list of the supported commands?
>
> You can just read the DT. Although, the problem then is what happens
> when we move to the next firmware interface. We see that some with
> devices having userspace dependencies on ATAGS.

Heh. I guess. Thought I suspect "Just read the DT to sort out the
available reboot modes" is probably not what most userspace wants to
hear. :)

thanks
-john

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

[Rob Herring]

On Thu, Feb 04, 2016 at 09:03:44PM -0800, John Stultz wrote:
> On Thu, Feb 4, 2016 at 8:35 PM, Rob Herring <robh@kernel.org> wrote:
> > On Thu, Feb 04, 2016 at 03:46:15PM -0800, John Stultz wrote:
> >> On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
> >> > On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
> >> >> +Example:
> >> >> +     reboot-mode {
> >> >> +             mode-normal = <BOOT_NORMAL>;
> >> >> +             mode-recovery = <BOOT_RECOVERY>;
> >> >> +             mode-fastboot = <BOOT_FASTBOOT>;
> >> >
> >> > I tend to agree with John on calling this mode-bootloader.
> >> >
> >> > OTOH, fastboot is more specific about what the mode is. The name in DT
> >> > and the userspace name don't necessarily have to be the same.
> >>
> >> Wait. This is a bit confusing. The utility of adding a property name
> >> and using that name be the reboot command parsed for made sense
> >> (compared to earlier versions which had command strings) as it made
> >> the dts more terse.   But it sounds here like you're suggesting we
> >> should have some logic in the driver that translates "reboot fastboot"
> >> to mode-bootloader or vice versa.
> >
> > I said early on the DT names and kernel-userspace names should not
> > necessarily be linked. They can be, but we shouldn't require that.
> 
> Sigh. Ok. It seemed it was due to earlier comments (maybe from others,
> but I thought it was you), that we moved from specifying a command
> string, to using the label. But if you think the label name and the
> commands shouldn't be linked, it seems like we should re-introduce
> that. No?
> 
> Unless your thinking we need some sort of static in-kernel mapping of
> commands to label names? But that just seems painfully indirect for
> little gain ("Its obvious! For that mode, you use this term here, and
> that different term over there!").

Tying it to a Linux ABI makes the binding Linux specific. I don't have a 
problem that the strings happen to be the same, but we should have some 
understanding that they may not be and allow for that.

> > My concern with mode-bootloader is what if you can boot into multiple
> > bootloader modes. Say USB mass storage is one option. "bootloader" is
> > not real specific.
> 
> True. But as I think we agreed below, "bootloader" and "recovery" are
> basically defacto standards, and I think it would be a bad idea to try
> to declare all the existing android tooling and docs wrong just
> because the command is vague, technically.

Okay, as long as they are clearly documented what they mean.


> >> >> +             mode-loader = <BOOT_LOADER>;
> >> >
> >> > This one needs a better name. Maybe it should be 'rockchip,mode-loader'
> >> > as it is vendor specific. Either way, loader is vague. Perhaps
> >> > rockchip,mode-bl-download?
> >>
> >> Hrm. So how what reboot command do you expect to trigger that?
> >
> > Whatever your OS has defined to map to that.
> >
> > We could just decide the kernel will strip <vendor> and 'mode-' and
> > match commands against what remains.
> 
> That part sounds sane, although I do think having vendor prefixes are
> reasonable for actual commands as well.

Well, you could still have "rockchip,mode-rockchip-bl-download"... 

We can bikeshed that when get there.

The other way random custom modes could be done is just allow the raw 
value to be passed from userspace converting the string to a number. 
Then we have no abstraction rather than half way abstracting it.

> >> I think one of the difficult things here is that there's no real
> >> standards for all bios/bootloader modes. So they are somewhat
> >> firmware/bootloader/device specific, and thus we need something that
> >> is flexible enough to allow lots of different modes to be easily
> >> specified.  That said, this does expose a userspace interface (though
> >> one could argue kernel ABI doesn't cross reboots :) so we should try
> >> to have some consistency so the same userspace can work on various
> >> devices.
> >
> > There is: UEFI. Boot mode efivars are standard. But then they are pretty
> > much PC oriented though. It is more which device to boot off of, but
> > there is network boot or boot to bios setup.
> 
> Well, there's a partial standard there.  I'm told for android on x86,
> there is no UEFI standard way to communicate rebooting to fastboot or
> recovery. Every device does its own device specific driver.

So much for standards. However, while these specific modes have not been 
standardized, there is a set of standard modes and these could have been 
added to the existing mechanism. So there at least exists some model to 
draw inspiration from.

Rob

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

[Rob Herring]

On Thu, Feb 04, 2016 at 09:03:44PM -0800, John Stultz wrote:
> On Thu, Feb 4, 2016 at 8:35 PM, Rob Herring <robh@kernel.org> wrote:
> > On Thu, Feb 04, 2016 at 03:46:15PM -0800, John Stultz wrote:
> >> On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
> >> > On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
> >> >> +Example:
> >> >> +     reboot-mode {
> >> >> +             mode-normal = <BOOT_NORMAL>;
> >> >> +             mode-recovery = <BOOT_RECOVERY>;
> >> >> +             mode-fastboot = <BOOT_FASTBOOT>;
> >> >
> >> > I tend to agree with John on calling this mode-bootloader.
> >> >
> >> > OTOH, fastboot is more specific about what the mode is. The name in DT
> >> > and the userspace name don't necessarily have to be the same.
> >>
> >> Wait. This is a bit confusing. The utility of adding a property name
> >> and using that name be the reboot command parsed for made sense
> >> (compared to earlier versions which had command strings) as it made
> >> the dts more terse.   But it sounds here like you're suggesting we
> >> should have some logic in the driver that translates "reboot fastboot"
> >> to mode-bootloader or vice versa.
> >
> > I said early on the DT names and kernel-userspace names should not
> > necessarily be linked. They can be, but we shouldn't require that.
> 
> Sigh. Ok. It seemed it was due to earlier comments (maybe from others,
> but I thought it was you), that we moved from specifying a command
> string, to using the label. But if you think the label name and the
> commands shouldn't be linked, it seems like we should re-introduce
> that. No?
> 
> Unless your thinking we need some sort of static in-kernel mapping of
> commands to label names? But that just seems painfully indirect for
> little gain ("Its obvious! For that mode, you use this term here, and
> that different term over there!").

Tying it to a Linux ABI makes the binding Linux specific. I don't have a 
problem that the strings happen to be the same, but we should have some 
understanding that they may not be and allow for that.

> > My concern with mode-bootloader is what if you can boot into multiple
> > bootloader modes. Say USB mass storage is one option. "bootloader" is
> > not real specific.
> 
> True. But as I think we agreed below, "bootloader" and "recovery" are
> basically defacto standards, and I think it would be a bad idea to try
> to declare all the existing android tooling and docs wrong just
> because the command is vague, technically.

Okay, as long as they are clearly documented what they mean.


> >> >> +             mode-loader = <BOOT_LOADER>;
> >> >
> >> > This one needs a better name. Maybe it should be 'rockchip,mode-loader'
> >> > as it is vendor specific. Either way, loader is vague. Perhaps
> >> > rockchip,mode-bl-download?
> >>
> >> Hrm. So how what reboot command do you expect to trigger that?
> >
> > Whatever your OS has defined to map to that.
> >
> > We could just decide the kernel will strip <vendor> and 'mode-' and
> > match commands against what remains.
> 
> That part sounds sane, although I do think having vendor prefixes are
> reasonable for actual commands as well.

Well, you could still have "rockchip,mode-rockchip-bl-download"... 

We can bikeshed that when get there.

The other way random custom modes could be done is just allow the raw 
value to be passed from userspace converting the string to a number. 
Then we have no abstraction rather than half way abstracting it.

> >> I think one of the difficult things here is that there's no real
> >> standards for all bios/bootloader modes. So they are somewhat
> >> firmware/bootloader/device specific, and thus we need something that
> >> is flexible enough to allow lots of different modes to be easily
> >> specified.  That said, this does expose a userspace interface (though
> >> one could argue kernel ABI doesn't cross reboots :) so we should try
> >> to have some consistency so the same userspace can work on various
> >> devices.
> >
> > There is: UEFI. Boot mode efivars are standard. But then they are pretty
> > much PC oriented though. It is more which device to boot off of, but
> > there is network boot or boot to bios setup.
> 
> Well, there's a partial standard there.  I'm told for android on x86,
> there is no UEFI standard way to communicate rebooting to fastboot or
> recovery. Every device does its own device specific driver.

So much for standards. However, while these specific modes have not been 
standardized, there is a set of standard modes and these could have been 
added to the existing mechanism. So there at least exists some model to 
draw inspiration from.

Rob

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

[Rob Herring]

On Thu, Feb 04, 2016 at 09:03:44PM -0800, John Stultz wrote:
> On Thu, Feb 4, 2016 at 8:35 PM, Rob Herring <robh@kernel.org> wrote:
> > On Thu, Feb 04, 2016 at 03:46:15PM -0800, John Stultz wrote:
> >> On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
> >> > On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
> >> >> +Example:
> >> >> +     reboot-mode {
> >> >> +             mode-normal = <BOOT_NORMAL>;
> >> >> +             mode-recovery = <BOOT_RECOVERY>;
> >> >> +             mode-fastboot = <BOOT_FASTBOOT>;
> >> >
> >> > I tend to agree with John on calling this mode-bootloader.
> >> >
> >> > OTOH, fastboot is more specific about what the mode is. The name in DT
> >> > and the userspace name don't necessarily have to be the same.
> >>
> >> Wait. This is a bit confusing. The utility of adding a property name
> >> and using that name be the reboot command parsed for made sense
> >> (compared to earlier versions which had command strings) as it made
> >> the dts more terse.   But it sounds here like you're suggesting we
> >> should have some logic in the driver that translates "reboot fastboot"
> >> to mode-bootloader or vice versa.
> >
> > I said early on the DT names and kernel-userspace names should not
> > necessarily be linked. They can be, but we shouldn't require that.
> 
> Sigh. Ok. It seemed it was due to earlier comments (maybe from others,
> but I thought it was you), that we moved from specifying a command
> string, to using the label. But if you think the label name and the
> commands shouldn't be linked, it seems like we should re-introduce
> that. No?
> 
> Unless your thinking we need some sort of static in-kernel mapping of
> commands to label names? But that just seems painfully indirect for
> little gain ("Its obvious! For that mode, you use this term here, and
> that different term over there!").

Tying it to a Linux ABI makes the binding Linux specific. I don't have a 
problem that the strings happen to be the same, but we should have some 
understanding that they may not be and allow for that.

> > My concern with mode-bootloader is what if you can boot into multiple
> > bootloader modes. Say USB mass storage is one option. "bootloader" is
> > not real specific.
> 
> True. But as I think we agreed below, "bootloader" and "recovery" are
> basically defacto standards, and I think it would be a bad idea to try
> to declare all the existing android tooling and docs wrong just
> because the command is vague, technically.

Okay, as long as they are clearly documented what they mean.


> >> >> +             mode-loader = <BOOT_LOADER>;
> >> >
> >> > This one needs a better name. Maybe it should be 'rockchip,mode-loader'
> >> > as it is vendor specific. Either way, loader is vague. Perhaps
> >> > rockchip,mode-bl-download?
> >>
> >> Hrm. So how what reboot command do you expect to trigger that?
> >
> > Whatever your OS has defined to map to that.
> >
> > We could just decide the kernel will strip <vendor> and 'mode-' and
> > match commands against what remains.
> 
> That part sounds sane, although I do think having vendor prefixes are
> reasonable for actual commands as well.

Well, you could still have "rockchip,mode-rockchip-bl-download"... 

We can bikeshed that when get there.

The other way random custom modes could be done is just allow the raw 
value to be passed from userspace converting the string to a number. 
Then we have no abstraction rather than half way abstracting it.

> >> I think one of the difficult things here is that there's no real
> >> standards for all bios/bootloader modes. So they are somewhat
> >> firmware/bootloader/device specific, and thus we need something that
> >> is flexible enough to allow lots of different modes to be easily
> >> specified.  That said, this does expose a userspace interface (though
> >> one could argue kernel ABI doesn't cross reboots :) so we should try
> >> to have some consistency so the same userspace can work on various
> >> devices.
> >
> > There is: UEFI. Boot mode efivars are standard. But then they are pretty
> > much PC oriented though. It is more which device to boot off of, but
> > there is network boot or boot to bios setup.
> 
> Well, there's a partial standard there.  I'm told for android on x86,
> there is no UEFI standard way to communicate rebooting to fastboot or
> recovery. Every device does its own device specific driver.

So much for standards. However, while these specific modes have not been 
standardized, there is a set of standard modes and these could have been 
added to the existing mechanism. So there at least exists some model to 
draw inspiration from.

Rob

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

[Andy Yan]

Hi Rob & John:

On 2016年02月05日 13:03, John Stultz wrote:
> On Thu, Feb 4, 2016 at 8:35 PM, Rob Herring <robh@kernel.org> wrote:
>> On Thu, Feb 04, 2016 at 03:46:15PM -0800, John Stultz wrote:
>>> On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
>>>> On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
>>>>> +Example:
>>>>> +     reboot-mode {
>>>>> +             mode-normal = <BOOT_NORMAL>;
>>>>> +             mode-recovery = <BOOT_RECOVERY>;
>>>>> +             mode-fastboot = <BOOT_FASTBOOT>;
>>>> I tend to agree with John on calling this mode-bootloader.
>>>>
>>>> OTOH, fastboot is more specific about what the mode is. The name in DT
>>>> and the userspace name don't necessarily have to be the same.
>>> Wait. This is a bit confusing. The utility of adding a property name
>>> and using that name be the reboot command parsed for made sense
>>> (compared to earlier versions which had command strings) as it made
>>> the dts more terse.   But it sounds here like you're suggesting we
>>> should have some logic in the driver that translates "reboot fastboot"
>>> to mode-bootloader or vice versa.
>> I said early on the DT names and kernel-userspace names should not
>> necessarily be linked. They can be, but we shouldn't require that.
> Sigh. Ok. It seemed it was due to earlier comments (maybe from others,
> but I thought it was you), that we moved from specifying a command
> string, to using the label. But if you think the label name and the
> commands shouldn't be linked, it seems like we should re-introduce
> that. No?
>
> Unless your thinking we need some sort of static in-kernel mapping of
> commands to label names? But that just seems painfully indirect for
> little gain ("Its obvious! For that mode, you use this term here, and
> that different term over there!").
>
>> My concern with mode-bootloader is what if you can boot into multiple
>> bootloader modes. Say USB mass storage is one option. "bootloader" is
>> not real specific.
> True. But as I think we agreed below, "bootloader" and "recovery" are
> basically defacto standards, and I think it would be a bad idea to try
> to declare all the existing android tooling and docs wrong just
> because the command is vague, technically.
>
>
>>>>> +             mode-loader = <BOOT_LOADER>;
>>>> This one needs a better name. Maybe it should be 'rockchip,mode-loader'
>>>> as it is vendor specific. Either way, loader is vague. Perhaps
>>>> rockchip,mode-bl-download?
>>> Hrm. So how what reboot command do you expect to trigger that?
>> Whatever your OS has defined to map to that.
>>
>> We could just decide the kernel will strip <vendor> and 'mode-' and
>> match commands against what remains.
> That part sounds sane, although I do think having vendor prefixes are
> reasonable for actual commands as well.
>
>
>>> I think one of the difficult things here is that there's no real
>>> standards for all bios/bootloader modes. So they are somewhat
>>> firmware/bootloader/device specific, and thus we need something that
>>> is flexible enough to allow lots of different modes to be easily
>>> specified.  That said, this does expose a userspace interface (though
>>> one could argue kernel ABI doesn't cross reboots :) so we should try
>>> to have some consistency so the same userspace can work on various
>>> devices.
>> There is: UEFI. Boot mode efivars are standard. But then they are pretty
>> much PC oriented though. It is more which device to boot off of, but
>> there is network boot or boot to bios setup.
> Well, there's a partial standard there.  I'm told for android on x86,
> there is no UEFI standard way to communicate rebooting to fastboot or
> recovery. Every device does its own device specific driver.
>
>
>>> I do think the "bootloader" and "recovery" arguments are somewhat
>>> defacto standards, well established on most android devices.
>> Yes, otherwise I'd be completely against "mode-bootloader" as the
>> property.
> Ok.
>
>>> I think here the concern is rockchip probably has some userspace that
>>> is already using "reboot maskrom" or "reboot loader" for their own
>>> uses. And its a bit of a pain to ask that userspace to be reworked to
>> Perhaps those are only for development and change would not be so
>> painful.
> Andy: Can you comment here? How critical are the specific commands
> you've used here for your userspace?
>
     Have some discussion with my colleague, we decided remove the" 
maskrom" mode in next version.
   Actually, this mode is not so used so often.
     But "reboot loader" is frequently used  in development by all the 
rockchip platform related people, even
some widely used factory tools use this command.So we really hope it can 
compatible with the existing tools
and user experience.
>>> use "reboot rom-download" or "reboot rockchip,rom-download" depending
>>> on how we try to deal with these.  (Granted, non-upstream interfaces
>>> aren't official, so that is their risk somewhat, but we avoid being
>>> smug about that :)
>> Or vendor specific modes require vendor specific translation in the
>> kernel.
> True.
>
>
>> To some extent we need to design what is right and worry about future
>> devices rather than cater to past devices. There's always some
>> compromise. What would you design ignoring the existing conditions.
>> Start there and then figure out how to make it work with current
>> designs.
> Fair enough. I just want to make sure we're not getting too caught up
> with design purity and are willing the meet the world where it is.
>
>>> Another part of the issue is there isn't really a way to probe for
>>> reboot cmd capability here. As much as I'd rather not complicate
>>> things, one couldn't easily extend existing userspace to work with
>>> current kernels as well as future kernels, since the reboot with an
>>> invalid command won't fail. The machine still resets. So you can't try
>>> one and fallback to the other.
>> Well, that's nice. Maybe we should change that? Or we're stuck with
>> that ABI?
> Maybe? I'm not sure what might have trouble with reboot failing if it
> sticks any extra noise in the reboot command.
> I'd probably lean towards sticking with the existing behavior.
>
>>> Maybe there needs to be a sysfs entry with the list of the supported commands?
>> You can just read the DT. Although, the problem then is what happens
>> when we move to the next firmware interface. We see that some with
>> devices having userspace dependencies on ATAGS.
> Heh. I guess. Thought I suspect "Just read the DT to sort out the
> available reboot modes" is probably not what most userspace wants to
> hear. :)
>
> thanks
> -john
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>

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

[Andy Yan]

Hi Rob & John:

On 2016年02月05日 13:03, John Stultz wrote:
> On Thu, Feb 4, 2016 at 8:35 PM, Rob Herring <robh@kernel.org> wrote:
>> On Thu, Feb 04, 2016 at 03:46:15PM -0800, John Stultz wrote:
>>> On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
>>>> On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
>>>>> +Example:
>>>>> +     reboot-mode {
>>>>> +             mode-normal = <BOOT_NORMAL>;
>>>>> +             mode-recovery = <BOOT_RECOVERY>;
>>>>> +             mode-fastboot = <BOOT_FASTBOOT>;
>>>> I tend to agree with John on calling this mode-bootloader.
>>>>
>>>> OTOH, fastboot is more specific about what the mode is. The name in DT
>>>> and the userspace name don't necessarily have to be the same.
>>> Wait. This is a bit confusing. The utility of adding a property name
>>> and using that name be the reboot command parsed for made sense
>>> (compared to earlier versions which had command strings) as it made
>>> the dts more terse.   But it sounds here like you're suggesting we
>>> should have some logic in the driver that translates "reboot fastboot"
>>> to mode-bootloader or vice versa.
>> I said early on the DT names and kernel-userspace names should not
>> necessarily be linked. They can be, but we shouldn't require that.
> Sigh. Ok. It seemed it was due to earlier comments (maybe from others,
> but I thought it was you), that we moved from specifying a command
> string, to using the label. But if you think the label name and the
> commands shouldn't be linked, it seems like we should re-introduce
> that. No?
>
> Unless your thinking we need some sort of static in-kernel mapping of
> commands to label names? But that just seems painfully indirect for
> little gain ("Its obvious! For that mode, you use this term here, and
> that different term over there!").
>
>> My concern with mode-bootloader is what if you can boot into multiple
>> bootloader modes. Say USB mass storage is one option. "bootloader" is
>> not real specific.
> True. But as I think we agreed below, "bootloader" and "recovery" are
> basically defacto standards, and I think it would be a bad idea to try
> to declare all the existing android tooling and docs wrong just
> because the command is vague, technically.
>
>
>>>>> +             mode-loader = <BOOT_LOADER>;
>>>> This one needs a better name. Maybe it should be 'rockchip,mode-loader'
>>>> as it is vendor specific. Either way, loader is vague. Perhaps
>>>> rockchip,mode-bl-download?
>>> Hrm. So how what reboot command do you expect to trigger that?
>> Whatever your OS has defined to map to that.
>>
>> We could just decide the kernel will strip <vendor> and 'mode-' and
>> match commands against what remains.
> That part sounds sane, although I do think having vendor prefixes are
> reasonable for actual commands as well.
>
>
>>> I think one of the difficult things here is that there's no real
>>> standards for all bios/bootloader modes. So they are somewhat
>>> firmware/bootloader/device specific, and thus we need something that
>>> is flexible enough to allow lots of different modes to be easily
>>> specified.  That said, this does expose a userspace interface (though
>>> one could argue kernel ABI doesn't cross reboots :) so we should try
>>> to have some consistency so the same userspace can work on various
>>> devices.
>> There is: UEFI. Boot mode efivars are standard. But then they are pretty
>> much PC oriented though. It is more which device to boot off of, but
>> there is network boot or boot to bios setup.
> Well, there's a partial standard there.  I'm told for android on x86,
> there is no UEFI standard way to communicate rebooting to fastboot or
> recovery. Every device does its own device specific driver.
>
>
>>> I do think the "bootloader" and "recovery" arguments are somewhat
>>> defacto standards, well established on most android devices.
>> Yes, otherwise I'd be completely against "mode-bootloader" as the
>> property.
> Ok.
>
>>> I think here the concern is rockchip probably has some userspace that
>>> is already using "reboot maskrom" or "reboot loader" for their own
>>> uses. And its a bit of a pain to ask that userspace to be reworked to
>> Perhaps those are only for development and change would not be so
>> painful.
> Andy: Can you comment here? How critical are the specific commands
> you've used here for your userspace?
>
     Have some discussion with my colleague, we decided remove the" 
maskrom" mode in next version.
   Actually, this mode is not so used so often.
     But "reboot loader" is frequently used  in development by all the 
rockchip platform related people, even
some widely used factory tools use this command.So we really hope it can 
compatible with the existing tools
and user experience.
>>> use "reboot rom-download" or "reboot rockchip,rom-download" depending
>>> on how we try to deal with these.  (Granted, non-upstream interfaces
>>> aren't official, so that is their risk somewhat, but we avoid being
>>> smug about that :)
>> Or vendor specific modes require vendor specific translation in the
>> kernel.
> True.
>
>
>> To some extent we need to design what is right and worry about future
>> devices rather than cater to past devices. There's always some
>> compromise. What would you design ignoring the existing conditions.
>> Start there and then figure out how to make it work with current
>> designs.
> Fair enough. I just want to make sure we're not getting too caught up
> with design purity and are willing the meet the world where it is.
>
>>> Another part of the issue is there isn't really a way to probe for
>>> reboot cmd capability here. As much as I'd rather not complicate
>>> things, one couldn't easily extend existing userspace to work with
>>> current kernels as well as future kernels, since the reboot with an
>>> invalid command won't fail. The machine still resets. So you can't try
>>> one and fallback to the other.
>> Well, that's nice. Maybe we should change that? Or we're stuck with
>> that ABI?
> Maybe? I'm not sure what might have trouble with reboot failing if it
> sticks any extra noise in the reboot command.
> I'd probably lean towards sticking with the existing behavior.
>
>>> Maybe there needs to be a sysfs entry with the list of the supported commands?
>> You can just read the DT. Although, the problem then is what happens
>> when we move to the next firmware interface. We see that some with
>> devices having userspace dependencies on ATAGS.
> Heh. I guess. Thought I suspect "Just read the DT to sort out the
> available reboot modes" is probably not what most userspace wants to
> hear. :)
>
> thanks
> -john
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>

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

[Andy Yan]

Hi Rob & John:

On 2016?02?05? 13:03, John Stultz wrote:
> On Thu, Feb 4, 2016 at 8:35 PM, Rob Herring <robh@kernel.org> wrote:
>> On Thu, Feb 04, 2016 at 03:46:15PM -0800, John Stultz wrote:
>>> On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
>>>> On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
>>>>> +Example:
>>>>> +     reboot-mode {
>>>>> +             mode-normal = <BOOT_NORMAL>;
>>>>> +             mode-recovery = <BOOT_RECOVERY>;
>>>>> +             mode-fastboot = <BOOT_FASTBOOT>;
>>>> I tend to agree with John on calling this mode-bootloader.
>>>>
>>>> OTOH, fastboot is more specific about what the mode is. The name in DT
>>>> and the userspace name don't necessarily have to be the same.
>>> Wait. This is a bit confusing. The utility of adding a property name
>>> and using that name be the reboot command parsed for made sense
>>> (compared to earlier versions which had command strings) as it made
>>> the dts more terse.   But it sounds here like you're suggesting we
>>> should have some logic in the driver that translates "reboot fastboot"
>>> to mode-bootloader or vice versa.
>> I said early on the DT names and kernel-userspace names should not
>> necessarily be linked. They can be, but we shouldn't require that.
> Sigh. Ok. It seemed it was due to earlier comments (maybe from others,
> but I thought it was you), that we moved from specifying a command
> string, to using the label. But if you think the label name and the
> commands shouldn't be linked, it seems like we should re-introduce
> that. No?
>
> Unless your thinking we need some sort of static in-kernel mapping of
> commands to label names? But that just seems painfully indirect for
> little gain ("Its obvious! For that mode, you use this term here, and
> that different term over there!").
>
>> My concern with mode-bootloader is what if you can boot into multiple
>> bootloader modes. Say USB mass storage is one option. "bootloader" is
>> not real specific.
> True. But as I think we agreed below, "bootloader" and "recovery" are
> basically defacto standards, and I think it would be a bad idea to try
> to declare all the existing android tooling and docs wrong just
> because the command is vague, technically.
>
>
>>>>> +             mode-loader = <BOOT_LOADER>;
>>>> This one needs a better name. Maybe it should be 'rockchip,mode-loader'
>>>> as it is vendor specific. Either way, loader is vague. Perhaps
>>>> rockchip,mode-bl-download?
>>> Hrm. So how what reboot command do you expect to trigger that?
>> Whatever your OS has defined to map to that.
>>
>> We could just decide the kernel will strip <vendor> and 'mode-' and
>> match commands against what remains.
> That part sounds sane, although I do think having vendor prefixes are
> reasonable for actual commands as well.
>
>
>>> I think one of the difficult things here is that there's no real
>>> standards for all bios/bootloader modes. So they are somewhat
>>> firmware/bootloader/device specific, and thus we need something that
>>> is flexible enough to allow lots of different modes to be easily
>>> specified.  That said, this does expose a userspace interface (though
>>> one could argue kernel ABI doesn't cross reboots :) so we should try
>>> to have some consistency so the same userspace can work on various
>>> devices.
>> There is: UEFI. Boot mode efivars are standard. But then they are pretty
>> much PC oriented though. It is more which device to boot off of, but
>> there is network boot or boot to bios setup.
> Well, there's a partial standard there.  I'm told for android on x86,
> there is no UEFI standard way to communicate rebooting to fastboot or
> recovery. Every device does its own device specific driver.
>
>
>>> I do think the "bootloader" and "recovery" arguments are somewhat
>>> defacto standards, well established on most android devices.
>> Yes, otherwise I'd be completely against "mode-bootloader" as the
>> property.
> Ok.
>
>>> I think here the concern is rockchip probably has some userspace that
>>> is already using "reboot maskrom" or "reboot loader" for their own
>>> uses. And its a bit of a pain to ask that userspace to be reworked to
>> Perhaps those are only for development and change would not be so
>> painful.
> Andy: Can you comment here? How critical are the specific commands
> you've used here for your userspace?
>
     Have some discussion with my colleague, we decided remove the" 
maskrom" mode in next version.
   Actually, this mode is not so used so often.
     But "reboot loader" is frequently used  in development by all the 
rockchip platform related people, even
some widely used factory tools use this command.So we really hope it can 
compatible with the existing tools
and user experience.
>>> use "reboot rom-download" or "reboot rockchip,rom-download" depending
>>> on how we try to deal with these.  (Granted, non-upstream interfaces
>>> aren't official, so that is their risk somewhat, but we avoid being
>>> smug about that :)
>> Or vendor specific modes require vendor specific translation in the
>> kernel.
> True.
>
>
>> To some extent we need to design what is right and worry about future
>> devices rather than cater to past devices. There's always some
>> compromise. What would you design ignoring the existing conditions.
>> Start there and then figure out how to make it work with current
>> designs.
> Fair enough. I just want to make sure we're not getting too caught up
> with design purity and are willing the meet the world where it is.
>
>>> Another part of the issue is there isn't really a way to probe for
>>> reboot cmd capability here. As much as I'd rather not complicate
>>> things, one couldn't easily extend existing userspace to work with
>>> current kernels as well as future kernels, since the reboot with an
>>> invalid command won't fail. The machine still resets. So you can't try
>>> one and fallback to the other.
>> Well, that's nice. Maybe we should change that? Or we're stuck with
>> that ABI?
> Maybe? I'm not sure what might have trouble with reboot failing if it
> sticks any extra noise in the reboot command.
> I'd probably lean towards sticking with the existing behavior.
>
>>> Maybe there needs to be a sysfs entry with the list of the supported commands?
>> You can just read the DT. Although, the problem then is what happens
>> when we move to the next firmware interface. We see that some with
>> devices having userspace dependencies on ATAGS.
> Heh. I guess. Thought I suspect "Just read the DT to sort out the
> available reboot modes" is probably not what most userspace wants to
> hear. :)
>
> thanks
> -john
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>

[PATCH v3 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>
Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

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              |  16 +++++
 drivers/power/reset/Makefile             |   2 +
 drivers/power/reset/reboot-mode.c        | 105 +++++++++++++++++++++++++++++++
 drivers/power/reset/reboot-mode.h        |   6 ++
 drivers/power/reset/syscon-reboot-mode.c |  64 +++++++++++++++++++
 5 files changed, 193 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..69ba640
--- /dev/null
+++ b/drivers/power/reset/reboot-mode.c
@@ -0,0 +1,105 @@
+/*
+ * 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];
+	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..8571df3
--- /dev/null
+++ b/drivers/power/reset/syscon-reboot-mode.c
@@ -0,0 +1,64 @@
+/*
+ * 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 v3 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>
Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

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              |  16 +++++
 drivers/power/reset/Makefile             |   2 +
 drivers/power/reset/reboot-mode.c        | 105 +++++++++++++++++++++++++++++++
 drivers/power/reset/reboot-mode.h        |   6 ++
 drivers/power/reset/syscon-reboot-mode.c |  64 +++++++++++++++++++
 5 files changed, 193 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..69ba640
--- /dev/null
+++ b/drivers/power/reset/reboot-mode.c
@@ -0,0 +1,105 @@
+/*
+ * 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];
+	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..8571df3
--- /dev/null
+++ b/drivers/power/reset/syscon-reboot-mode.c
@@ -0,0 +1,64 @@
+/*
+ * 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

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

[Moritz Fischer]

Hi Andy,

looks good to me. Feel free to add my Reviewed-by

On Tue, Feb 2, 2016 at 2:02 AM, Andy Yan <andy.yan@rock-chips.com> wrote:

> This is commonly used on Android based devices, in order to
> reboot the device into fastboot or recovery mode.
>
Reviewed-by: Moritz Fischer <moritz.fischer@ettus.com>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

Cheers,

Moritz

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

[Moritz Fischer]

Hi Andy,

looks good to me. Feel free to add my Reviewed-by

On Tue, Feb 2, 2016 at 2:02 AM, Andy Yan <andy.yan@rock-chips.com> wrote:

> This is commonly used on Android based devices, in order to
> reboot the device into fastboot or recovery mode.
>
Reviewed-by: Moritz Fischer <moritz.fischer@ettus.com>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

Cheers,

Moritz

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

[Moritz Fischer]

Hi Andy,

looks good to me. Feel free to add my Reviewed-by

On Tue, Feb 2, 2016 at 2:02 AM, Andy Yan <andy.yan@rock-chips.com> wrote:

> This is commonly used on Android based devices, in order to
> reboot the device into fastboot or recovery mode.
>
Reviewed-by: Moritz Fischer <moritz.fischer@ettus.com>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

Cheers,

Moritz

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

[kbuild test robot]

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

Hi Andy,

[auto build test ERROR on rockchip/for-next]
[also build test ERROR on v4.5-rc2 next-20160203]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Andy-Yan/add-reboot-mode-driver/20160202-181909
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
config: x86_64-randconfig-r0-02031912 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

warning: (SYSCON_REBOOT_MODE) selects REBOOT_MODE which has unmet direct dependencies (POWER_RESET && OF)
   drivers/power/reset/reboot-mode.c: In function 'reboot_mode_register':
>> drivers/power/reset/reboot-mode.c:79:2: error: implicit declaration of function 'for_each_property_of_node' [-Werror=implicit-function-declaration]
     for_each_property_of_node(dev->of_node, prop) {
     ^
>> drivers/power/reset/reboot-mode.c:79:48: error: expected ';' before '{' token
     for_each_property_of_node(dev->of_node, prop) {
                                                   ^
>> drivers/power/reset/reboot-mode.c:70:9: warning: unused variable 'len' [-Wunused-variable]
     size_t len = strlen(PREFIX);
            ^
>> drivers/power/reset/reboot-mode.c:68:20: warning: unused variable 'info' [-Wunused-variable]
     struct mode_info *info;
                       ^
   drivers/power/reset/reboot-mode.c: At top level:
   drivers/power/reset/reboot-mode.c:51:12: warning: 'reboot_mode_notify' defined but not used [-Wunused-function]
    static int reboot_mode_notify(struct notifier_block *this,
               ^
   cc1: some warnings being treated as errors

vim +/for_each_property_of_node +79 drivers/power/reset/reboot-mode.c

    62		return NOTIFY_DONE;
    63	}
    64	
    65	int reboot_mode_register(struct device *dev, int (*write)(int))
    66	{
    67		struct reboot_mode_driver *reboot;
  > 68		struct mode_info *info;
    69		struct property *prop;
  > 70		size_t len = strlen(PREFIX);
    71		int ret;
    72	
    73		reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
    74		if (!reboot)
    75			return -ENOMEM;
    76	
    77		reboot->write = write;
    78		INIT_LIST_HEAD(&reboot->head);
  > 79		for_each_property_of_node(dev->of_node, prop) {
    80			if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, len))
    81				continue;
    82			info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24266 bytes --]

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

[kbuild test robot]

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

Hi Andy,

[auto build test ERROR on rockchip/for-next]
[also build test ERROR on v4.5-rc2 next-20160203]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Andy-Yan/add-reboot-mode-driver/20160202-181909
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
config: x86_64-randconfig-r0-02031912 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

warning: (SYSCON_REBOOT_MODE) selects REBOOT_MODE which has unmet direct dependencies (POWER_RESET && OF)
   drivers/power/reset/reboot-mode.c: In function 'reboot_mode_register':
>> drivers/power/reset/reboot-mode.c:79:2: error: implicit declaration of function 'for_each_property_of_node' [-Werror=implicit-function-declaration]
     for_each_property_of_node(dev->of_node, prop) {
     ^
>> drivers/power/reset/reboot-mode.c:79:48: error: expected ';' before '{' token
     for_each_property_of_node(dev->of_node, prop) {
                                                   ^
>> drivers/power/reset/reboot-mode.c:70:9: warning: unused variable 'len' [-Wunused-variable]
     size_t len = strlen(PREFIX);
            ^
>> drivers/power/reset/reboot-mode.c:68:20: warning: unused variable 'info' [-Wunused-variable]
     struct mode_info *info;
                       ^
   drivers/power/reset/reboot-mode.c: At top level:
   drivers/power/reset/reboot-mode.c:51:12: warning: 'reboot_mode_notify' defined but not used [-Wunused-function]
    static int reboot_mode_notify(struct notifier_block *this,
               ^
   cc1: some warnings being treated as errors

vim +/for_each_property_of_node +79 drivers/power/reset/reboot-mode.c

    62		return NOTIFY_DONE;
    63	}
    64	
    65	int reboot_mode_register(struct device *dev, int (*write)(int))
    66	{
    67		struct reboot_mode_driver *reboot;
  > 68		struct mode_info *info;
    69		struct property *prop;
  > 70		size_t len = strlen(PREFIX);
    71		int ret;
    72	
    73		reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
    74		if (!reboot)
    75			return -ENOMEM;
    76	
    77		reboot->write = write;
    78		INIT_LIST_HEAD(&reboot->head);
  > 79		for_each_property_of_node(dev->of_node, prop) {
    80			if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, len))
    81				continue;
    82			info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24266 bytes --]

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

[kbuild test robot]

Hi Andy,

[auto build test ERROR on rockchip/for-next]
[also build test ERROR on v4.5-rc2 next-20160203]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Andy-Yan/add-reboot-mode-driver/20160202-181909
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
config: x86_64-randconfig-r0-02031912 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

warning: (SYSCON_REBOOT_MODE) selects REBOOT_MODE which has unmet direct dependencies (POWER_RESET && OF)
   drivers/power/reset/reboot-mode.c: In function 'reboot_mode_register':
>> drivers/power/reset/reboot-mode.c:79:2: error: implicit declaration of function 'for_each_property_of_node' [-Werror=implicit-function-declaration]
     for_each_property_of_node(dev->of_node, prop) {
     ^
>> drivers/power/reset/reboot-mode.c:79:48: error: expected ';' before '{' token
     for_each_property_of_node(dev->of_node, prop) {
                                                   ^
>> drivers/power/reset/reboot-mode.c:70:9: warning: unused variable 'len' [-Wunused-variable]
     size_t len = strlen(PREFIX);
            ^
>> drivers/power/reset/reboot-mode.c:68:20: warning: unused variable 'info' [-Wunused-variable]
     struct mode_info *info;
                       ^
   drivers/power/reset/reboot-mode.c: At top level:
   drivers/power/reset/reboot-mode.c:51:12: warning: 'reboot_mode_notify' defined but not used [-Wunused-function]
    static int reboot_mode_notify(struct notifier_block *this,
               ^
   cc1: some warnings being treated as errors

vim +/for_each_property_of_node +79 drivers/power/reset/reboot-mode.c

    62		return NOTIFY_DONE;
    63	}
    64	
    65	int reboot_mode_register(struct device *dev, int (*write)(int))
    66	{
    67		struct reboot_mode_driver *reboot;
  > 68		struct mode_info *info;
    69		struct property *prop;
  > 70		size_t len = strlen(PREFIX);
    71		int ret;
    72	
    73		reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
    74		if (!reboot)
    75			return -ENOMEM;
    76	
    77		reboot->write = write;
    78		INIT_LIST_HEAD(&reboot->head);
  > 79		for_each_property_of_node(dev->of_node, prop) {
    80			if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, len))
    81				continue;
    82			info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 24266 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160203/17f6916f/attachment-0001.obj>

[PATCH v3 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 platform

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

---

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                | 11 ++++++++++
 arch/arm/boot/dts/rk3xxx.dtsi                | 13 +++++++++++-
 include/dt-bindings/soc/rockchip_boot-mode.h | 30 ++++++++++++++++++++++++++++
 4 files changed, 63 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..64527c6 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-fastboot = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_LOADER>;
+		};
 	};
 
 	acodec: acodec-ana@20030000 {
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 8ac49f3..4d06d84 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,16 @@
 				clocks = <&cru ACLK_GPU>;
 			};
 		};
+
+		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>;
+		};
 	};
 
 	sgrf: syscon@ff740000 {
diff --git a/arch/arm/boot/dts/rk3xxx.dtsi b/arch/arm/boot/dts/rk3xxx.dtsi
index 99eeea7..d21ed43 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,18 @@
 	};
 
 	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-fastboot = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_LOADER>;
+			mode-maskrom = <BOOT_MASKROM>;
+		};
 	};
 
 	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 v3 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 platform

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

---

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                | 11 ++++++++++
 arch/arm/boot/dts/rk3xxx.dtsi                | 13 +++++++++++-
 include/dt-bindings/soc/rockchip_boot-mode.h | 30 ++++++++++++++++++++++++++++
 4 files changed, 63 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..64527c6 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 at 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-fastboot = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_LOADER>;
+		};
 	};
 
 	acodec: acodec-ana at 20030000 {
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 8ac49f3..4d06d84 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,16 @@
 				clocks = <&cru ACLK_GPU>;
 			};
 		};
+
+		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>;
+		};
 	};
 
 	sgrf: syscon at ff740000 {
diff --git a/arch/arm/boot/dts/rk3xxx.dtsi b/arch/arm/boot/dts/rk3xxx.dtsi
index 99eeea7..d21ed43 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,18 @@
 	};
 
 	pmu: pmu at 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-fastboot = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_LOADER>;
+			mode-maskrom = <BOOT_MASKROM>;
+		};
 	};
 
 	grf: grf at 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 v3 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 v3:
- descirbe all reboot mode as property 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 | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3368.dtsi b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
index 122777b..5213713 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,19 @@
 	};
 
 	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-fastboot = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_LOADER>;
+			mode-maskrom = <BOOT_MASKROM>;
+
+		};
 	};
 
 	cru: clock-controller@ff760000 {
-- 
1.9.1

[PATCH v3 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 v3:
- descirbe all reboot mode as property 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 | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3368.dtsi b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
index 122777b..5213713 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,19 @@
 	};
 
 	pmugrf: syscon at 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-fastboot = <BOOT_FASTBOOT>;
+			mode-loader = <BOOT_LOADER>;
+			mode-maskrom = <BOOT_MASKROM>;
+
+		};
 	};
 
 	cru: clock-controller at ff760000 {
-- 
1.9.1

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

[Shawn Lin]

On 2016/2/2 18:13, Andy Yan wrote:
> Add syscon-reboot-mode driver DT node for rk3368 platform
>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>

Work fine with my rk3368-sheep board :)

Tested-by: Shawn Lin <shawn.lin@rock-chips.com>

> ---
>
> Changes in v3:
> - descirbe all reboot mode as property 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 | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3368.dtsi b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
> index 122777b..5213713 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,19 @@
>   	};
>
>   	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-fastboot = <BOOT_FASTBOOT>;
> +			mode-loader = <BOOT_LOADER>;
> +			mode-maskrom = <BOOT_MASKROM>;
> +
> +		};
>   	};
>
>   	cru: clock-controller@ff760000 {
>


-- 
Best Regards
Shawn Lin

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

[Shawn Lin]

On 2016/2/2 18:13, Andy Yan wrote:
> Add syscon-reboot-mode driver DT node for rk3368 platform
>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>

Work fine with my rk3368-sheep board :)

Tested-by: Shawn Lin <shawn.lin@rock-chips.com>

> ---
>
> Changes in v3:
> - descirbe all reboot mode as property 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 | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3368.dtsi b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
> index 122777b..5213713 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,19 @@
>   	};
>
>   	pmugrf: syscon at 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-fastboot = <BOOT_FASTBOOT>;
> +			mode-loader = <BOOT_LOADER>;
> +			mode-maskrom = <BOOT_MASKROM>;
> +
> +		};
>   	};
>
>   	cru: clock-controller at ff760000 {
>


-- 
Best Regards
Shawn Lin