| Message ID | 1447840410-19794-1-git-send-email-andy.yan@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > >
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 >> >> > >
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
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
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
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
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 --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>; + };
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