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

["Heiko Stübner" <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>]

Hi Andy,

Am Dienstag, 1. Dezember 2015, 23:10:15 schrieb Andy Yan:
> 2015-11-23 21:15 GMT+08:00 Andy Yan <andyshrk-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > 2015-11-20 9:58 GMT+08:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> >> On Thu, Nov 19, 2015 at 7:16 PM, Andy Yan <andy.yan-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> >> 
> >> 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
> >> --
> >> 
> >     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";


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