Hi Andy,

Am Dienstag, 1. Dezember 2015, 23:10:15 schrieb Andy Yan:

2015-11-23 21:15 GMT+08:00 Andy Yan [off-list ref]:

quoted

2015-11-20 9:58 GMT+08:00 Rob Herring [off-list ref]:

quoted

On Thu, Nov 19, 2015 at 7:16 PM, Andy Yan [off-list ref]

wrote:

quoted

On 2015年11月19日 12:35, Heiko Stuebner wrote:

quoted

Am Donnerstag, 19. November 2015, 09:17:37 schrieb Andy Yan:

quoted

On 2015年11月19日 06:59, Rob Herring wrote:

quoted

On Wed, Nov 18, 2015 at 05:53:30PM +0800, Andy Yan wrote:

quoted

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

quoted

quoted

quoted

quoted

just to bind to drivers. Whatever &pmu is is probably what should

have

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

       which register to use to store the reboot mode. Because

rockchip

quoted

quoted

quoted

       use different register file to store the reboot mode on

different

quoted

quoted

quoted

       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,

quoted

quoted

quoted

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

quoted

quoted

maybe simply do the

        platform_device_register_simple("rockchip-reboot", -1, NULL,

0);

quoted

quoted

in that common code?

Though I'm not yet sure how to get the platform-data. I guess one

option

quoted

quoted

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

quoted

quoted

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

quoted

    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

quoted

soc

    in the future and the soc use a different register here, we need

modify

quoted

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

quoted

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
--

    Do you mean I should implement the DT node like this？
   
   diff --git a/arch/arm/boot/dts/rk3xxx.dtsi

b/arch/arm/boot/dts/rk3xxx.dtsi
index 7b14d7a..1735d09 100644

--- a/arch/arm/boot/dts/rk3xxx.dtsi
+++ b/arch/arm/boot/dts/rk3xxx.dtsi

@@ -103,12 +103,6 @@

                };
        
        };

-       reboot {
-               compatible = "rockchip,reboot";
-               rockchip,regmap = <&pmu>;
-               offset = <0x40>;
-       };
-

        xin24m: oscillator {
        
                compatible = "fixed-clock";
                clock-frequency = <24000000>;

@@ -249,7 +243,11 @@

        pmu: pmu@20004000 {
        
                compatible = "rockchip,rk3066-pmu", "syscon";

-               reg = <0x20004000 0x100>;
+              reg = <0x20004000 0x100>;
+              reboot {
+                       compatible = "rockchip,reboot";
+                       offset = <0x40>;
+               };

        };

diff --git a/arch/arm64/boot/dts/rockchip/rk3368.dtsi

b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
index cd02229..8a9837a 100644

--- a/arch/arm64/boot/dts/rockchip/rk3368.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3368.dtsi

@@ -202,12 +202,6 @@

                method = "smc";
        
        };

-       reboot {
-               compatible = "rockchip,reboot";
-               rockchip,regmap = <&pmugrf>;
-               offset = <0x200>;
-       };
-

        timer {
        
                compatible = "arm,armv8-timer";
                interrupts = <GIC_PPI 13

@@ -493,6 +487,10 @@

        pmugrf: syscon@ff738000 {
        
                compatible = "rockchip,rk3368-pmugrf", "syscon";

    compatible = "rockchip,rk3368-pmugrf", "syscon", "simple-mfd";

quoted

                reg = <0x0 0xff738000 0x0 0x1000>;

+              reboot {
+                       compatible = "rockchip,reboot";
+                       offset = <0x200>;
+               };

        };

  Is there any further suggestion for this? If not, I will send the V4 with
the DT node as a subnode in PMU or PMUGRF.

I guess Rob is the authority on this, but I'm not sure on the "devicetree 
describes hardware" level.

On the one hand it is not really a hardware-device, but on the other hand it 
is a firmware-interface (like psci etc, that's already in the devicetree 
elsewhere), so I'd guess it should be ok.


Heiko
--
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