Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1456054 > unrolled thread

[RFC][PATCH 0/4] SRAM based reboot reason driver for HiKey

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH 0/4] SRAM based reboot reason driver for HiKey John Stultz <john.stultz@linaro.org> - 2016-08-04 01:10 +0200
    [RFC][PATCH 1/4] drivers: sram: Have sram driver probe children nodes John Stultz <john.stultz@linaro.org> - 2016-08-04 01:10 +0200
    [RFC][PATCH 4/4] dts: hikey: Add hikey support for sram-reboot-mode John Stultz <john.stultz@linaro.org> - 2016-08-04 01:10 +0200
    Re: [RFC][PATCH 0/4] SRAM based reboot reason driver for HiKey Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-08-05 14:50 +0200
      Re: [RFC][PATCH 0/4] SRAM based reboot reason driver for HiKey John Stultz <john.stultz@linaro.org> - 2016-08-06 22:10 +0200
      Re: [RFC][PATCH 0/4] SRAM based reboot reason driver for HiKey Rob Herring <robh@kernel.org> - 2016-08-06 23:00 +0200
        Re: [RFC][PATCH 0/4] SRAM based reboot reason driver for HiKey Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-08-08 15:50 +0200

#1456054 — [RFC][PATCH 0/4] SRAM based reboot reason driver for HiKey

Now that Andy's reboot reason core driver has landed, I wanted
to resubmit a reworked version of my SRAM based reboot reason
driver.

This allows the kernel to communicate to the bootloader what mode
it should reboot to using some reserved memory.

Feedback would be very much appreciated!

thanks
-john

Cc: Andy Yan <andy.yan@rock-chips.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Heiko Stübner <heiko@sntech.de>
Cc: Caesar Wang <wxt@rock-chips.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Guodong Xu <guodong.xu@linaro.org>
Cc: Haojian Zhuang <haojian.zhuang@linaro.org>
Cc: Vishal Bhoj <vishal.bhoj@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: devicetree@vger.kernel.org
Cc: Android Kernel Team <kernel-team@android.com>

John Stultz (4):
  drivers: sram: Have sram driver probe children nodes
  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      | 35 ++++++++
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     | 22 ++++-
 drivers/misc/sram.c                                |  3 +
 drivers/power/reset/Kconfig                        | 10 +++
 drivers/power/reset/Makefile                       |  1 +
 drivers/power/reset/sram-reboot-mode.c             | 95 ++++++++++++++++++++++
 6 files changed, 165 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/power/reset/sram-reboot-mode.txt
 create mode 100644 drivers/power/reset/sram-reboot-mode.c

-- 
1.9.1

[toc] | [next] | [standalone]

#1456055 — [RFC][PATCH 1/4] drivers: sram: Have sram driver probe children nodes

In order to support sub-nodes with the sram driver,
have the sram driver call of_probe_default_children().

This will allow for supportting sram based reboot reasons.

Cc: Andy Yan <andy.yan@rock-chips.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Heiko Stübner <heiko@sntech.de>
Cc: Caesar Wang <wxt@rock-chips.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Guodong Xu <guodong.xu@linaro.org>
Cc: Haojian Zhuang <haojian.zhuang@linaro.org>
Cc: Vishal Bhoj <vishal.bhoj@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: devicetree@vger.kernel.org
Cc: Android Kernel Team <kernel-team@android.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/misc/sram.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index f84b53d..6830a79 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -23,6 +23,7 @@
 #include <linux/io.h>
 #include <linux/list_sort.h>
 #include <linux/of_address.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
@@ -384,6 +385,8 @@ static int sram_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sram);
 
+	of_platform_default_populate(pdev->dev.of_node,
+						NULL, &pdev->dev);
 	dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
 		gen_pool_size(sram->pool) / 1024, sram->virt_base);
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1456056 — [RFC][PATCH 4/4] dts: hikey: Add hikey support for sram-reboot-mode

Add support to hikey dts and defconfig for the
sram-reboot-mode driver.

The dts entries added here should really be generated
by the UEFI firmware, and not be static in the dts,
since one may be using different firmware on HiKey.
But this patch provides an example of how the
sram-reboot-mode entry would otherwise look.

Cc: Andy Yan <andy.yan@rock-chips.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Heiko Stübner <heiko@sntech.de>
Cc: Caesar Wang <wxt@rock-chips.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Guodong Xu <guodong.xu@linaro.org>
Cc: Haojian Zhuang <haojian.zhuang@linaro.org>
Cc: Vishal Bhoj <vishal.bhoj@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: devicetree@vger.kernel.org
Cc: Android Kernel Team <kernel-team@android.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index 593c7e4..62326ab 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -29,6 +29,7 @@
 	 * Reserve below regions from memory node:
 	 *
 	 *  0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
+	 *  0x05f0,1000 - 0x05f0,1fff: Reboot reason
 	 *  0x06df,f000 - 0x06df,ffff: Mailbox message data
 	 *  0x0740,f000 - 0x0740,ffff: MCU firmware section
 	 *  0x3e00,0000 - 0x3fff,ffff: OP-TEE
@@ -36,11 +37,30 @@
 	memory@0 {
 		device_type = "memory";
 		reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
-		      <0x00000000 0x05f00000 0x00000000 0x00eff000>,
+		      <0x00000000 0x05f00000 0x00000000 0x00001000>,
+		      <0x00000000 0x05f02000 0x00000000 0x00efd000>,
 		      <0x00000000 0x06e00000 0x00000000 0x0060f000>,
 		      <0x00000000 0x07410000 0x00000000 0x36bf0000>;
 	};
 
+	sram@5f01000 {
+		compatible = "mmio-sram";
+		reg = <0x0 0x05f01000 0x0 0x00001000>;
+		ranges = <0x0 0x0 0x05f01000 0x00001000>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		reboot-mode@0 {
+			compatible = "sram-reboot-mode";
+			reg = <0x0 0x4>;
+
+			mode-normal	= <0x77665501>;
+			mode-bootloader	= <0x77665500>;
+			mode-recovery	= <0x77665502>;
+		};
+	};
+
 	soc {
 		spi0: spi@f7106000 {
 			status = "ok";
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1457081

Hi John,

On 08/04/2016 02:05 AM, John Stultz wrote:
> Now that Andy's reboot reason core driver has landed, I wanted
> to resubmit a reworked version of my SRAM based reboot reason
> driver.
>
> This allows the kernel to communicate to the bootloader what mode
> it should reboot to using some reserved memory.
>
> Feedback would be very much appreciated!

in my opinion the taken approach is wrong, and I've already explained
why and how to rework your driver to shrink the change, please see
https://lkml.org/lkml/2016/1/27/133

In this case I think that a SRAM device node should just contain
a plain description of partitions, compatible = "sram-reboot-mode" is
clearly not a device on "SRAM bus", it is not a device at all, so
please let's separate policy from mechanism

Because my proposed alternative approach separates policy from
mechanism, it for instanse allows to avoid overlappings on SRAM areas,
and still other drivers may serve as consumers of partitions on SRAM.

Please add me to Cc list when you send the next version of the driver.

With best wishes,
Vladimir

> thanks
> -john
>
> Cc: Andy Yan <andy.yan@rock-chips.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Heiko Stübner <heiko@sntech.de>
> Cc: Caesar Wang <wxt@rock-chips.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Guodong Xu <guodong.xu@linaro.org>
> Cc: Haojian Zhuang <haojian.zhuang@linaro.org>
> Cc: Vishal Bhoj <vishal.bhoj@linaro.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: devicetree@vger.kernel.org
> Cc: Android Kernel Team <kernel-team@android.com>
>
> John Stultz (4):
>   drivers: sram: Have sram driver probe children nodes
>   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      | 35 ++++++++
>  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     | 22 ++++-
>  drivers/misc/sram.c                                |  3 +
>  drivers/power/reset/Kconfig                        | 10 +++
>  drivers/power/reset/Makefile                       |  1 +
>  drivers/power/reset/sram-reboot-mode.c             | 95 ++++++++++++++++++++++
>  6 files changed, 165 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/sram-reboot-mode.txt
>  create mode 100644 drivers/power/reset/sram-reboot-mode.c
>

[toc] | [prev] | [next] | [standalone]

#1457302

On Fri, Aug 5, 2016 at 3:37 PM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Aug 5, 2016 at 7:46 AM, Vladimir Zapolskiy
> <vladimir_zapolskiy@mentor.com> wrote:
>> Hi John,
>>
>> On 08/04/2016 02:05 AM, John Stultz wrote:
>>>
>>> Now that Andy's reboot reason core driver has landed, I wanted
>>> to resubmit a reworked version of my SRAM based reboot reason
>>> driver.
>>>
>>> This allows the kernel to communicate to the bootloader what mode
>>> it should reboot to using some reserved memory.
>>>
>>> Feedback would be very much appreciated!
>>
>>
>> in my opinion the taken approach is wrong, and I've already explained
>> why and how to rework your driver to shrink the change, please see
>> https://lkml.org/lkml/2016/1/27/133
>>
>> In this case I think that a SRAM device node should just contain
>> a plain description of partitions, compatible = "sram-reboot-mode" is
>> clearly not a device on "SRAM bus", it is not a device at all, so
>> please let's separate policy from mechanism
>
> Having a 2nd node for the driver is still not a device on a bus. It
> adds unneeded complexity to the binding IMO.
>
> The current approach also follows the model ramoops is using. Right
> now it's using reserved-memory, but that could easily be extended to
> SRAM region as well.
>
>> Because my proposed alternative approach separates policy from
>> mechanism, it for instanse allows to avoid overlappings on SRAM areas,
>> and still other drivers may serve as consumers of partitions on SRAM.
>
> You could still have multiple consumers and having a compatible string
> doesn't necessarily imply a driver. Though multiple consumers without
> something arbitrating access sounds like broken design to me.

So after running into some issues implementing the feedback that Bjorn
suggested, I realized we were going to need to not only extend the
sram driver to probe children, but we'd also have to make it a mfd so
it wouldn't reserve the entire range and the reboot reason driver
could map the memory.

That on top of the fact that we're already duplicating much of the
syscon-reboot-mode driver to work on sram, I decided to just start
over and use the syscon driver, which works fine here. All that is
needed is just adding it to the dts.

I know that its not exactly correct usage of the syscon driver, but it
starts to feel crazy almost completely duplicating the syscon driver
just to have it named sram.

thanks
-john

[toc] | [prev] | [next] | [standalone]

#1457354

On Fri, Aug 5, 2016 at 7:46 AM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:
> Hi John,
>
> On 08/04/2016 02:05 AM, John Stultz wrote:
>>
>> Now that Andy's reboot reason core driver has landed, I wanted
>> to resubmit a reworked version of my SRAM based reboot reason
>> driver.
>>
>> This allows the kernel to communicate to the bootloader what mode
>> it should reboot to using some reserved memory.
>>
>> Feedback would be very much appreciated!
>
>
> in my opinion the taken approach is wrong, and I've already explained
> why and how to rework your driver to shrink the change, please see
> https://lkml.org/lkml/2016/1/27/133
>
> In this case I think that a SRAM device node should just contain
> a plain description of partitions, compatible = "sram-reboot-mode" is
> clearly not a device on "SRAM bus", it is not a device at all, so
> please let's separate policy from mechanism

Having a 2nd node for the driver is still not a device on a bus. It
adds unneeded complexity to the binding IMO.

The current approach also follows the model ramoops is using. Right
now it's using reserved-memory, but that could easily be extended to
SRAM region as well.

> Because my proposed alternative approach separates policy from
> mechanism, it for instanse allows to avoid overlappings on SRAM areas,
> and still other drivers may serve as consumers of partitions on SRAM.

You could still have multiple consumers and having a compatible string
doesn't necessarily imply a driver. Though multiple consumers without
something arbitrating access sounds like broken design to me.

Rob

[toc] | [prev] | [next] | [standalone]

#1457803

Hi Rob,

On 08/06/2016 01:37 AM, Rob Herring wrote:
> On Fri, Aug 5, 2016 at 7:46 AM, Vladimir Zapolskiy
> <vladimir_zapolskiy@mentor.com> wrote:
>> Hi John,
>>
>> On 08/04/2016 02:05 AM, John Stultz wrote:
>>>
>>> Now that Andy's reboot reason core driver has landed, I wanted
>>> to resubmit a reworked version of my SRAM based reboot reason
>>> driver.
>>>
>>> This allows the kernel to communicate to the bootloader what mode
>>> it should reboot to using some reserved memory.
>>>
>>> Feedback would be very much appreciated!
>>
>>
>> in my opinion the taken approach is wrong, and I've already explained
>> why and how to rework your driver to shrink the change, please see
>> https://lkml.org/lkml/2016/1/27/133
>>
>> In this case I think that a SRAM device node should just contain
>> a plain description of partitions, compatible = "sram-reboot-mode" is
>> clearly not a device on "SRAM bus", it is not a device at all, so
>> please let's separate policy from mechanism
>
> Having a 2nd node for the driver is still not a device on a bus. It
> adds unneeded complexity to the binding IMO.

What second node for the driver do you mean here? If you reference
a reset/syscon driver then there should be only a property pointing
to a partition on SRAM, similar case is found in CODA driver, see
Documentation/devicetree/bindings/media/coda.txt and in my short term
plans to do the same for lpc-eth driver.

And a node which describes an area on SRAM is generally needed in both
cases, however note that with my approach techincally it is possible to
specify the entire SRAM device as a target partition, sometimes it is
sufficient but here it is not wanted, because there will be no control
on offset/size of the particular data stored on SRAM. The essential
part is the meaning of this added second node, either it is a reserved
partition (= unified definition independently on consumers) or
a description with a compatible property for some arbitrary device.
Why zoo of compatibles under SRAM node should be accepted? Why SRAM
should be converted to a bus type device? Should be the same done
with e.g. MTD or NVMEM devices? IMHO clear separation between data
proiders and data consumers should be preserved if possible, and here
it appears to be a simpler solution for the given technical problem.

> The current approach also follows the model ramoops is using. Right
> now it's using reserved-memory, but that could easily be extended to
> SRAM region as well.
>
>> Because my proposed alternative approach separates policy from
>> mechanism, it for instanse allows to avoid overlappings on SRAM areas,
>> and still other drivers may serve as consumers of partitions on SRAM.
>
> You could still have multiple consumers and having a compatible string
> doesn't necessarily imply a driver. Though multiple consumers without
> something arbitrating access sounds like broken design to me.
>

Not in this case, the interface to SRAM partitions and/or SRAM as
a whole deliberately assumes that a memory area is shared among all
consumers in sense of a memory pool, data resides within the
given area but it is not inter-shared among consumers.

--
With best wishes,
Vladimir

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web