diff mbox

[v3,2/5] dt-bindings: soc: add document for rockchip reboot notifier driver

Message ID 1447840410-19794-1-git-send-email-andy.yan@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andy Yan Nov. 18, 2015, 9:53 a.m. UTC
Add devicetree binding document for rockchip reboot nofifier driver

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

---

Changes in v3:
 - add dt binding

Changes in v2: None

 .../bindings/soc/rockchip/rockchip-reboot.txt          | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/rockchip/rockchip-reboot.txt

Comments

Rob Herring Nov. 18, 2015, 10:59 p.m. UTC | #1
On Wed, Nov 18, 2015 at 05:53:30PM +0800, Andy Yan wrote:
> Add devicetree binding document for rockchip reboot nofifier driver

Just reading the subject this is way too specific to the Linux driver 
needs rather than a h/w description. Please don't create fake DT nodes 
just to bind to drivers. Whatever &pmu is is probably what should have 
the DT node. Let the driver for it create child devices if you need 
that.

Rob
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> 
> ---
> 
> Changes in v3:
>  - add dt binding
> 
> Changes in v2: None
> 
>  .../bindings/soc/rockchip/rockchip-reboot.txt          | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/rockchip/rockchip-reboot.txt
> 
> diff --git a/Documentation/devicetree/bindings/soc/rockchip/rockchip-reboot.txt b/Documentation/devicetree/bindings/soc/rockchip/rockchip-reboot.txt
> new file mode 100644
> index 0000000..6f69c8d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/rockchip/rockchip-reboot.txt
> @@ -0,0 +1,18 @@
> +Rockchip reboot notifier driver
> +
> +This driver get reboot mode arguments from userspace
> +and stores it in special register. Then the bootloader
> +will read it and take different action according the
> +argument stored.
> +
> +Required properties:
> +- compatible: should be "rockchip,reboot"
> +- regmap: this is phandle to the register map node
> +- offset: offset in the register map for the storage register (in bytes)
> +
> +Examples:
> +	reboot {
> +	   compatible = "rockchip,reboot";
> +	   regmap = <&pmu>;
> +	   offset = <0x94>;
> +	};
> -- 
> 1.9.1
> 
>
Andy Yan Nov. 19, 2015, 1:17 a.m. UTC | #2
Hi Rob:

On 2015?11?19? 06:59, Rob Herring wrote:
> On Wed, Nov 18, 2015 at 05:53:30PM +0800, Andy Yan wrote:
>> Add devicetree binding document for rockchip reboot nofifier driver
> Just reading the subject this is way too specific to the Linux driver
> needs rather than a h/w description. Please don't create fake DT nodes
> just to bind to drivers. Whatever &pmu is is probably what should have
> the DT node. Let the driver for it create child devices if you need
> that.

     This is note a fake DT nodes, we really need it to tell the driver
      which register to use to store the reboot mode. Because rockchip
      use different register file to store the reboot mode on different
      platform, on rk3066,rk3188, rk3288,it use  one of the PMU 
register, on
      the incoming RK3036, it use one of the GRF register, and it use 
one  of
      the PMUGRF register for arm64 platform rk3368. On the other hand, the
      PMU/GRF/PMUGRF register file are mapped as "syscon", then referenced
      by other DT nodes by phandle. So maybe let it as a separate DT 
node here
      is better.

> Rob
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>
>> ---
>>
>> Changes in v3:
>>   - add dt binding
>>
>> Changes in v2: None
>>
>>   .../bindings/soc/rockchip/rockchip-reboot.txt          | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/rockchip/rockchip-reboot.txt
>>
>> diff --git a/Documentation/devicetree/bindings/soc/rockchip/rockchip-reboot.txt b/Documentation/devicetree/bindings/soc/rockchip/rockchip-reboot.txt
>> new file mode 100644
>> index 0000000..6f69c8d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/rockchip/rockchip-reboot.txt
>> @@ -0,0 +1,18 @@
>> +Rockchip reboot notifier driver
>> +
>> +This driver get reboot mode arguments from userspace
>> +and stores it in special register. Then the bootloader
>> +will read it and take different action according the
>> +argument stored.
>> +
>> +Required properties:
>> +- compatible: should be "rockchip,reboot"
>> +- regmap: this is phandle to the register map node
>> +- offset: offset in the register map for the storage register (in bytes)
>> +
>> +Examples:
>> +	reboot {
>> +	   compatible = "rockchip,reboot";
>> +	   regmap = <&pmu>;
>> +	   offset = <0x94>;
>> +	};
>> -- 
>> 1.9.1
>>
>>
>
>
Heiko Stübner Nov. 19, 2015, 4:35 a.m. UTC | #3
Hi Andy,

Am Donnerstag, 19. November 2015, 09:17:37 schrieb Andy Yan:
> Hi Rob:
> 
> On 2015?11?19? 06:59, Rob Herring wrote:
> > On Wed, Nov 18, 2015 at 05:53:30PM +0800, Andy Yan wrote:
> >> Add devicetree binding document for rockchip reboot nofifier driver
> > Just reading the subject this is way too specific to the Linux driver
> > needs rather than a h/w description. Please don't create fake DT nodes
> > just to bind to drivers. Whatever &pmu is is probably what should have
> > the DT node. Let the driver for it create child devices if you need
> > that.
> 
>      This is note a fake DT nodes, we really need it to tell the driver
>       which register to use to store the reboot mode. Because rockchip
>       use different register file to store the reboot mode on different
>       platform, on rk3066,rk3188, rk3288,it use  one of the PMU 
> register, on
>       the incoming RK3036, it use one of the GRF register, and it use 
> one  of
>       the PMUGRF register for arm64 platform rk3368. On the other hand, the
>       PMU/GRF/PMUGRF register file are mapped as "syscon", then referenced
>       by other DT nodes by phandle. So maybe let it as a separate DT 
> node here
>       is better.

or alternatively we could do something similar to what the bl-switcher 
cupfreq-driver does. Take a look at

drivers/cpufreq/arm_big_little.c
drivers/clk/clk-mb86s7x.c

We already have the core restart-handler code in the clock-tree, so could 
maybe simply do the
	platform_device_register_simple("rockchip-reboot", -1, NULL, 0);
in that common code?

Though I'm not yet sure how to get the platform-data. I guess one option would 
be to do things like the 3288 suspend code does (arch/arm/mach-rockchip/pm.c 
at the bottom), by having the per-soc-data in the driver and then matching 
against the pmu. Because the pmu is not part of the clock controller binding 
(and probably also shouldn't be).


Heiko
Thierry Reding Nov. 19, 2015, 12:56 p.m. UTC | #4
On Thu, Nov 19, 2015 at 09:17:37AM +0800, Andy Yan wrote:
> Hi Rob:
> 
> On 2015?11?19? 06:59, Rob Herring wrote:
> >On Wed, Nov 18, 2015 at 05:53:30PM +0800, Andy Yan wrote:
> >>Add devicetree binding document for rockchip reboot nofifier driver
> >Just reading the subject this is way too specific to the Linux driver
> >needs rather than a h/w description. Please don't create fake DT nodes
> >just to bind to drivers. Whatever &pmu is is probably what should have
> >the DT node. Let the driver for it create child devices if you need
> >that.
> 
>     This is note a fake DT nodes, we really need it to tell the driver
>      which register to use to store the reboot mode. Because rockchip
>      use different register file to store the reboot mode on different
>      platform, on rk3066,rk3188, rk3288,it use  one of the PMU register, on
>      the incoming RK3036, it use one of the GRF register, and it use one  of
>      the PMUGRF register for arm64 platform rk3368. On the other hand, the
>      PMU/GRF/PMUGRF register file are mapped as "syscon", then referenced
>      by other DT nodes by phandle. So maybe let it as a separate DT node
> here
>      is better.

In that case you should probably implement a reboot notifier in each of
the drivers you list and depending on the generation of the SoC. You can
easily parameterize this by matching on the compatible string.

Thierry
Thierry Reding Nov. 19, 2015, 3:30 p.m. UTC | #5
On Thu, Nov 19, 2015 at 09:39:02PM +0800, Andy Yan wrote:
> Hi Thierry:
> 
> 2015-11-19 20:56 GMT+08:00 Thierry Reding <treding@nvidia.com>:
> 
> > On Thu, Nov 19, 2015 at 09:17:37AM +0800, Andy Yan wrote:
> > > Hi Rob:
> > >
> > > On 2015?11?19? 06:59, Rob Herring wrote:
> > > >On Wed, Nov 18, 2015 at 05:53:30PM +0800, Andy Yan wrote:
> > > >>Add devicetree binding document for rockchip reboot nofifier driver
> > > >Just reading the subject this is way too specific to the Linux driver
> > > >needs rather than a h/w description. Please don't create fake DT nodes
> > > >just to bind to drivers. Whatever &pmu is is probably what should have
> > > >the DT node. Let the driver for it create child devices if you need
> > > >that.
> > >
> > >     This is note a fake DT nodes, we really need it to tell the driver
> > >      which register to use to store the reboot mode. Because rockchip
> > >      use different register file to store the reboot mode on different
> > >      platform, on rk3066,rk3188, rk3288,it use  one of the PMU register,
> > on
> > >      the incoming RK3036, it use one of the GRF register, and it use
> > one  of
> > >      the PMUGRF register for arm64 platform rk3368. On the other hand,
> > the
> > >      PMU/GRF/PMUGRF register file are mapped as "syscon", then referenced
> > >      by other DT nodes by phandle. So maybe let it as a separate DT node
> > > here
> > >      is better.
> >
> > In that case you should probably implement a reboot notifier in each of
> > the drivers you list and depending on the generation of the SoC. You can
> > easily parameterize this by matching on the compatible string.
> >
> > Thierry
> >
> 
>  There is no rockchip specific driver for PMU/GRF/PMUGRF register file I
> list above? they
>  use the generic driver “syscon”

Well, just go and write specific drivers, then.

Thierry
Andy Yan Nov. 20, 2015, 1:16 a.m. UTC | #6
Hi Heiko:

On 2015?11?19? 12:35, Heiko Stuebner wrote:
> Hi Andy,
>
> Am Donnerstag, 19. November 2015, 09:17:37 schrieb Andy Yan:
>> Hi Rob:
>>
>> On 2015?11?19? 06:59, Rob Herring wrote:
>>> On Wed, Nov 18, 2015 at 05:53:30PM +0800, Andy Yan wrote:
>>>> Add devicetree binding document for rockchip reboot nofifier driver
>>> Just reading the subject this is way too specific to the Linux driver
>>> needs rather than a h/w description. Please don't create fake DT nodes
>>> just to bind to drivers. Whatever &pmu is is probably what should have
>>> the DT node. Let the driver for it create child devices if you need
>>> that.
>>       This is note a fake DT nodes, we really need it to tell the driver
>>        which register to use to store the reboot mode. Because rockchip
>>        use different register file to store the reboot mode on different
>>        platform, on rk3066,rk3188, rk3288,it use  one of the PMU
>> register, on
>>        the incoming RK3036, it use one of the GRF register, and it use
>> one  of
>>        the PMUGRF register for arm64 platform rk3368. On the other hand, the
>>        PMU/GRF/PMUGRF register file are mapped as "syscon", then referenced
>>        by other DT nodes by phandle. So maybe let it as a separate DT
>> node here
>>        is better.
> or alternatively we could do something similar to what the bl-switcher
> cupfreq-driver does. Take a look at
>
> drivers/cpufreq/arm_big_little.c
> drivers/clk/clk-mb86s7x.c
>
> We already have the core restart-handler code in the clock-tree, so could
> maybe simply do the
> 	platform_device_register_simple("rockchip-reboot", -1, NULL, 0);
> in that common code?
>
> Though I'm not yet sure how to get the platform-data. I guess one option would
> be to do things like the 3288 suspend code does (arch/arm/mach-rockchip/pm.c
> at the bottom), by having the per-soc-data in the driver and then matching
> against the pmu. Because the pmu is not part of the clock controller binding
> (and probably also shouldn't be).
>
>
> Heiko
>
>
>
>
>
    Thanks for your suggestion.
     I have read the code you list above, if we implement the reboot 
notifier
     driver like this, the driver need to add much more code to find the 
platform
     data(like arch/arm/mach-rockhcip/pm.c), what's more, if we have a 
new soc
     in the future and the soc use a different register here, we need 
modify the
     driver to add a new platform data again, this will bring additional 
work.

     Use the DT node pass the register will make the driver code simple 
and clear.
     Is there any hurt to put this information in the DT?

     Andy
Rob Herring Nov. 20, 2015, 1:58 a.m. UTC | #7
On Thu, Nov 19, 2015 at 7:16 PM, Andy Yan <andy.yan@rock-chips.com> wrote:
> On 2015?11?19? 12:35, Heiko Stuebner wrote:
>> Am Donnerstag, 19. November 2015, 09:17:37 schrieb Andy Yan:
>>> On 2015?11?19? 06:59, Rob Herring wrote:
>>>> On Wed, Nov 18, 2015 at 05:53:30PM +0800, Andy Yan wrote:
>>>>>
>>>>> Add devicetree binding document for rockchip reboot nofifier driver
>>>>
>>>> Just reading the subject this is way too specific to the Linux driver
>>>> needs rather than a h/w description. Please don't create fake DT nodes
>>>> just to bind to drivers. Whatever &pmu is is probably what should have
>>>> the DT node. Let the driver for it create child devices if you need
>>>> that.
>>>
>>>       This is note a fake DT nodes, we really need it to tell the driver
>>>        which register to use to store the reboot mode. Because rockchip
>>>        use different register file to store the reboot mode on different
>>>        platform, on rk3066,rk3188, rk3288,it use  one of the PMU
>>> register, on
>>>        the incoming RK3036, it use one of the GRF register, and it use
>>> one  of
>>>        the PMUGRF register for arm64 platform rk3368. On the other hand,
>>> the
>>>        PMU/GRF/PMUGRF register file are mapped as "syscon", then
>>> referenced
>>>        by other DT nodes by phandle. So maybe let it as a separate DT
>>> node here
>>>        is better.
>>
>> or alternatively we could do something similar to what the bl-switcher
>> cupfreq-driver does. Take a look at
>>
>> drivers/cpufreq/arm_big_little.c
>> drivers/clk/clk-mb86s7x.c
>>
>> We already have the core restart-handler code in the clock-tree, so could
>> maybe simply do the
>>         platform_device_register_simple("rockchip-reboot", -1, NULL, 0);
>> in that common code?
>>
>> Though I'm not yet sure how to get the platform-data. I guess one option
>> would
>> be to do things like the 3288 suspend code does
>> (arch/arm/mach-rockchip/pm.c
>> at the bottom), by having the per-soc-data in the driver and then matching
>> against the pmu. Because the pmu is not part of the clock controller
>> binding
>> (and probably also shouldn't be).
>>
>    Thanks for your suggestion.
>     I have read the code you list above, if we implement the reboot notifier
>     driver like this, the driver need to add much more code to find the
> platform
>     data(like arch/arm/mach-rockhcip/pm.c), what's more, if we have a new
> soc
>     in the future and the soc use a different register here, we need modify
> the
>     driver to add a new platform data again, this will bring additional
> work.
>
>     Use the DT node pass the register will make the driver code simple and
> clear.
>     Is there any hurt to put this information in the DT?

Add the data you need to the PMU node. Then the PMU driver can get it
and pass to the child driver.

Rob
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/soc/rockchip/rockchip-reboot.txt b/Documentation/devicetree/bindings/soc/rockchip/rockchip-reboot.txt
new file mode 100644
index 0000000..6f69c8d
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/rockchip/rockchip-reboot.txt
@@ -0,0 +1,18 @@ 
+Rockchip reboot notifier driver
+
+This driver get reboot mode arguments from userspace
+and stores it in special register. Then the bootloader
+will read it and take different action according the
+argument stored.
+
+Required properties:
+- compatible: should be "rockchip,reboot"
+- regmap: this is phandle to the register map node
+- offset: offset in the register map for the storage register (in bytes)
+
+Examples:
+	reboot {
+	   compatible = "rockchip,reboot";
+	   regmap = <&pmu>;
+	   offset = <0x94>;
+	};