From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1447840044-19689-1-git-send-email-andy.yan@rock-chips.com> <20151118225904.GA5429@rob-hp-laptop> <564D2331.8070503@rock-chips.com> <2082288.pD4oKxtaXL@phil> <564E7473.9080509@rock-chips.com> Date: Tue, 1 Dec 2015 23:10:15 +0800 Message-ID: Subject: Re: [PATCH v3 2/5] dt-bindings: soc: add document for rockchip reboot notifier driver From: Andy Yan Content-Type: multipart/alternative; boundary=047d7bdc1c0c1dc7850525d78e1e To: Rob Herring Cc: Andy Yan , Heiko Stuebner , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , Kumar Gala , Russell King - ARM Linux , "open list:ARM/Rockchip SoC..." , Ian Campbell , Pawel Moll , Mark Rutland , "linux-arm-kernel@lists.infradead.org" , Kevin Hilman , Thierry Reding , Caesar Wang , Simon Glass , Ben Chan List-ID: --047d7bdc1c0c1dc7850525d78e1e Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi Rob=E3=80=81Heiko=EF=BC=9A 2015-11-23 21:15 GMT+08:00 Andy Yan : > Hi Rob=EF=BC=9A > > 2015-11-20 9:58 GMT+08:00 Rob Herring : > >> On Thu, Nov 19, 2015 at 7:16 PM, Andy Yan >> wrote: >> > On 2015=E5=B9=B411=E6=9C=8819=E6=97=A5 12:35, Heiko Stuebner wrote: >> >> Am Donnerstag, 19. November 2015, 09:17:37 schrieb Andy Yan: >> >>> On 2015=E5=B9=B411=E6=9C=8819=E6=97=A5 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 drive= r >> >>>> >> >>>> Just reading the subject this is way too specific to the Linux driv= er >> >>>> 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 u= se >> >>> 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 D= T >> >>> node here >> >>> is better. >> >> >> >> or alternatively we could do something similar to what the bl-switche= r >> >> 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 th= e >> > 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 additiona= l >> > 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=EF=BC=9F > > 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 =3D "rockchip,reboot"; > - rockchip,regmap =3D <&pmu>; > - offset =3D <0x40>; > - }; > - > xin24m: oscillator { > compatible =3D "fixed-clock"; > clock-frequency =3D <24000000>; > @@ -249,7 +243,11 @@ > > pmu: pmu@20004000 { > compatible =3D "rockchip,rk3066-pmu", "syscon"; > - reg =3D <0x20004000 0x100>; > + reg =3D <0x20004000 0x100>; > + reboot { > + compatible =3D "rockchip,reboot"; > + offset =3D <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 =3D "smc"; > }; > > - reboot { > - compatible =3D "rockchip,reboot"; > - rockchip,regmap =3D <&pmugrf>; > - offset =3D <0x200>; > - }; > - > timer { > compatible =3D "arm,armv8-timer"; > interrupts =3D @@ -493,6 +487,10 @@ > pmugrf: syscon@ff738000 { > compatible =3D "rockchip,rk3368-pmugrf", "syscon"; > reg =3D <0x0 0xff738000 0x0 0x1000>; > + reboot { > + compatible =3D "rockchip,reboot"; > + offset =3D <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. Andy --047d7bdc1c0c1dc7850525d78e1e Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Rob=E3=80=81Heiko=EF=BC=9A

2015-11-23 21:15 GMT+08:0= 0 Andy Yan <andyshrk@gmail.com>:
Hi Rob=EF=BC=9A

2015-11-20 9:58 GMT+0= 8:00 Rob Herring <robh@kernel.org>:
= On Thu, Nov 19, 2015 at 7:16 PM, Andy Yan <andy.yan@rock-chips.com> wrote:
> On 2015=E5=B9=B411=E6=9C=8819=E6=97=A5 12:35, Heiko Stuebner wrote: >> Am Donnerstag, 19. November 2015, 09:17:37 schrieb An= dy Yan:
>>> On 2015=E5=B9=B411=E6=9C=8819=E6=97=A5 06:59,= Rob Herring wrote:
>>>> On Wed, Nov 18, 2015 at 05:53:30PM +0800, Andy Yan wrote:<= br> >>>>>
>>>>> Add devicetree binding document for rockchip reboot no= fifier driver
>>>>
>>>> Just reading the subject this is way too specific to the L= inux driver
>>>> needs rather than a h/w description. Please don't crea= te 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.
>>>
>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0This is note a fake DT nodes, we rea= lly need it to tell the driver
>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 which register to use to store the = reboot mode. Because rockchip
>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 use different register file to stor= e the reboot mode on different
>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 platform, on rk3066,rk3188, rk3288,= it use=C2=A0 one of the PMU
>>> register, on
>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 the incoming RK3036, it use one of = the GRF register, and it use
>>> one=C2=A0 of
>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 the PMUGRF register for arm64 platf= orm rk3368. On the other hand,
>>> the
>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 PMU/GRF/PMUGRF register file are ma= pped as "syscon", then
>>> referenced
>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 by other DT nodes by phandle. So ma= ybe let it as a separate DT
>>> node here
>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 is better.
>>
>> or alternatively we could do something similar to what the bl-swit= cher
>> 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, s= o could
>> maybe simply do the
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0platform_device_register_simple(&= quot;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 controll= er
>> binding
>> (and probably also shouldn't be).
>>
>=C2=A0 =C2=A0 Thanks for your suggestion.
>=C2=A0 =C2=A0 =C2=A0I have read the code you list above, if we implemen= t the reboot notifier
>=C2=A0 =C2=A0 =C2=A0driver like this, the driver need to add much more = code to find the
> platform
>=C2=A0 =C2=A0 =C2=A0data(like arch/arm/mach-rockhcip/pm.c), what's = more, if we have a new
> soc
>=C2=A0 =C2=A0 =C2=A0in the future and the soc use a different register = here, we need modify
> the
>=C2=A0 =C2=A0 =C2=A0driver to add a new platform data again, this will = bring additional
> work.
>
>=C2=A0 =C2=A0 =C2=A0Use the DT node pass the register will make the dri= ver code simple and
> clear.
>=C2=A0 =C2=A0 =C2=A0Is 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 i= t
and pass to the child driver.

Rob
--

=C2=A0
=C2=A0=C2=A0=C2=A0 Do you=C2=A0mean I should implement the= DT node like this=EF=BC=9F

=C2=A0=C2=A0 diff --git a/arch/arm/boot/dts/rk3xxx.dt= si 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 @@
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };
=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 };
=C2=A0
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 reboo= t {
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 compatible =3D "rockchip,reboot";
-=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 rockchip,regmap =3D <&pmu>;
-=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 offset =3D <0x= 40>;
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };
-
=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 xin24m: oscillator {
=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 compa= tible =3D "fixed-clock";
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 clock-frequency =3D = <24000000>;
@@ -249,7 +243,11 @@
=C2=A0
=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 pmu: pmu@20004000 {
=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 compatible = =3D "rockchip,rk3066-pmu", "syscon";
-=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 re= g =3D <0x20004000 0x100>;
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0reg =3D <0x20004000 0x100&g= t;;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0
+=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 reboot {
+=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 compatible =3D "ro= ckchip,reboot";
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 offset =3D <0x40>;
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };
=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };

diff --git a/arch/arm64/boot/dts/rockchip/rk336= 8.dtsi b/arch/arm64/boot/dts/rockchip/rk3368.dtsi
index cd02229..8a9837a= 100644
--- a/arch/arm64/boot/dts/rockchip/rk3368.dtsi
+++ b/arch/arm= 64/boot/dts/rockchip/rk3368.dtsi
@@ -202,12 +202,6 @@
=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= method =3D "smc";
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = };
=C2=A0
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 reboot {
-=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 compatible =3D "rockchip,reboot";
-=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rockchip,re= gmap =3D <&pmugrf>;
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 offset =3D <0x200>;
= -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };
-
=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 timer {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 compatible =3D "arm,armv= 8-timer";
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 interrupts =3D <GIC_PPI 13
@@ -493,= 6 +487,10 @@
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pmugrf: syscon@f= f738000 {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 compatible =3D "rockchip,rk3368-pmugrf&= quot;, "syscon";
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 reg =3D <0x0 0xff738000 0x= 0 0x1000>;
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0reboot {
+=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 compatible =3D "rockchip,reboot";<= br>+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 offse= t =3D <0x200>;
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 };
=C2=A0


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

Andy

=
--047d7bdc1c0c1dc7850525d78e1e--