Re: [RFC][PATCH 0/3] SRAM reboot mode driver

[Vladimir Zapolskiy]

Hi John,

On 27.01.2016 02:37, John Stultz wrote:
> This patchset extends on Andy Yan's reboot mode driver
> work from here: https://lkml.org/lkml/2016/1/12/315
> 
> It adds reboot mode/reason support for devices that use
> an SRAM location to communicate with the bootloader.
> 
> Doing this via an SRAM subnode was a suggestion from
> Arnd, but I worry this implementation isn't yet ideal,
> since I spent quite a bit of time futzing with getting
> the sram dts entry to work properly. So I suspect there
> will be a number of suggestions for improvements.

sorry, I missed that discussion, what are the problems you've
encountered?
Probably SRAM driver is ready to serve your purposes?

I believe I'm screwing up some HiSilicon specifics, but here
is a rough draft, which you may find helpful:

1. Add SRAM node with defined reboot-reason area:

---8<---
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index 8185251..c35f5ed 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -31,6 +31,20 @@
 		device_type = "memory";
 		reg = <0x0 0x0 0x0 0x40000000>;
 	};
+
+	sram@5f01000 {
+		compatible = "mmio-sram";
+		reg = <0x0 0x05f01000 0x0 0x00001000>;
+		ranges = <0x0 0x0 0x05f01000 0x00001000>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		reboot-reason: reboot-reason {
+			reg = <0x0 0x4>;
+			pool;
+		};
+	};
 };

 &uart2 {
---8<---

2. Reference reboot-reason node from reboot mode driver's node,
probably you need to add an optional property (here "reason-storage") to its
binding:

---8<---
		reboot-mode {
			compatible = "syscon-reboot-mode";
[snip]
			reason-storage = <&reboot-reason>;
		}
---8<---

3. In reboot-mode driver read or write to that SRAM partition:

---8<---
	storage = of_gen_pool_get(reboot_mode_np, "reason-storage", 0);
	if (!storage)
		goto no_storage;

	size = gen_pool_size(storage);
        base = gen_pool_alloc(storage, pool_size);
	if (!base)
		return -EINVAL;

	writel(0x77665501, base);
	gen_pool_free(storage, base, size);

no_storage:
	return 0;
---8<---

There is one noticeable benefit of this approach - you don't need
to write another driver and you don't need to add another DT binding.

With best wishes,
Vladimir

> Again, this series dependson Andy's patch set above,
> but also was developed & tested against the 4.1 based
> hikey tree, so at least the hikey dts patch won't apply.
> I'm mostly sending this out for just a rough initial
> review of the dts and conceptual usage of sram subnodes.
> 
> Also, it was pointed out that the hikey dts entry for
> this really should be added by the UEFI firmware, since
> alternative bootloaders may be used which do not support
> this feature. So the hikey dts patch isn't likely to ever
> go upstream, but its a useful illustration for how other
> devices might use this driver.
> 
> Cc: Andy Yan <andy.yan@xxxxxxxxxxxxxx>
> Cc: Rob Herring <robh@xxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Thierry Reding <treding@xxxxxxxxxx>
> Cc: Heiko StÃbner <heiko@xxxxxxxxx>
> Cc: Caesar Wang <wxt@xxxxxxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Guodong Xu <guodong.xu@xxxxxxxxxx>
> Cc: Haojian Zhuang <haojian.zhuang@xxxxxxxxxx>
> Cc: Vishal Bhoj <vishal.bhoj@xxxxxxxxxx>
> Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx
> Cc: Android Kernel Team <kernel-team@xxxxxxxxxxx>
> 
> John Stultz (3):
>   dt-bindings: power: reset: Add document for sram-reboot-mode driver
>   power: reset: Add sram-reboot-mode driver
>   dts: hikey: Add hikey support for sram-reboot-mode
> 
>  .../bindings/power/reset/sram-reboot-mode.txt      | 47 +++++++++++++++
>  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     | 36 ++++++++++++
>  arch/arm64/configs/hikey_defconfig                 |  3 +
>  drivers/power/reset/Kconfig                        |  9 +++
>  drivers/power/reset/Makefile                       |  1 +
>  drivers/power/reset/sram-reboot-mode.c             | 66 ++++++++++++++++++++++
>  6 files changed, 162 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/sram-reboot-mode.txt
>  create mode 100644 drivers/power/reset/sram-reboot-mode.c
>