[2/2] ARM: dts: NSP: avoid unnecessary probe deferrals
diff mbox series

Message ID 20191025040041.6210-3-chris.packham@alliedtelesis.co.nz
State New
Headers show
Series
  • ARM: bcm: nsp: gpio improvements (hopefully)
Related show

Commit Message

Chris Packham Oct. 25, 2019, 4 a.m. UTC
The pinctrl node is used by the gpioa node. Which may have more
descendants at a board level. If the pinctrl node isn't probed first the
gpio is deferred and anything that needs a gpio pin on that chip is also
deferred.

Normally we and nodes in the device tree to be listed in their natural
memory mapped address order but putting the pinctrl node first avoids
the deferral of numerous devices so make an exception in this case.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 arch/arm/boot/dts/bcm-nsp.dtsi | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Florian Fainelli Oct. 25, 2019, 5:26 p.m. UTC | #1
On 10/24/19 9:00 PM, Chris Packham wrote:
> The pinctrl node is used by the gpioa node. Which may have more
> descendants at a board level. If the pinctrl node isn't probed first the
> gpio is deferred and anything that needs a gpio pin on that chip is also
> deferred.

If what you care is to optimize your boot flow such that no re-probing
occurs, maybe another solution to look at is to re-order the order in
which subsystems are initialized or built (_initcall changes or
drivers/Makefile changes), because changing Device Tree certainly does
not scale over platforms and I recall Rob indicating that he wanted to
introduce randomized platform_device creation from
of_platform_bus_populate() at one point or another.

> 
> Normally we and nodes in the device tree to be listed in their natural
> memory mapped address order but putting the pinctrl node first avoids
> the deferral of numerous devices so make an exception in this case.

That is a workaround more than a real solution, though I understand why
you would to do that. One downside is that the entries are no longer in
incrementing register address order and that is visually disturbing and
who knows, maybe a drive by contributor whose pet project will be to
order the Device Tree entries by incrementing addresses will change that
in the future...

> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  arch/arm/boot/dts/bcm-nsp.dtsi | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
> index da6d70f09ef1..dd7a65743c08 100644
> --- a/arch/arm/boot/dts/bcm-nsp.dtsi
> +++ b/arch/arm/boot/dts/bcm-nsp.dtsi
> @@ -172,6 +172,13 @@
>  		#address-cells = <1>;
>  		#size-cells = <1>;
>  
> +		pinctrl: pinctrl@3f1c0 {
> +			compatible = "brcm,nsp-pinmux";
> +			reg = <0x3f1c0 0x04>,
> +			      <0x30028 0x04>,
> +			      <0x3f408 0x04>;
> +		};
> +
>  		gpioa: gpio@20 {
>  			compatible = "brcm,nsp-gpio-a";
>  			reg = <0x0020 0x70>,
> @@ -458,13 +465,6 @@
>  					     "sata2";
>  		};
>  
> -		pinctrl: pinctrl@3f1c0 {
> -			compatible = "brcm,nsp-pinmux";
> -			reg = <0x3f1c0 0x04>,
> -			      <0x30028 0x04>,
> -			      <0x3f408 0x04>;
> -		};
> -
>  		thermal: thermal@3f2c0 {
>  			compatible = "brcm,ns-thermal";
>  			reg = <0x3f2c0 0x10>;
>
Chris Packham Oct. 28, 2019, 8:21 p.m. UTC | #2
On Fri, 2019-10-25 at 10:26 -0700, Florian Fainelli wrote:
> On 10/24/19 9:00 PM, Chris Packham wrote:
> > The pinctrl node is used by the gpioa node. Which may have more
> > descendants at a board level. If the pinctrl node isn't probed first the
> > gpio is deferred and anything that needs a gpio pin on that chip is also
> > deferred.
> 
> If what you care is to optimize your boot flow such that no re-probing
> occurs, maybe another solution to look at is to re-order the order in
> which subsystems are initialized or built (_initcall changes or
> drivers/Makefile changes), because changing Device Tree certainly does
> not scale over platforms and I recall Rob indicating that he wanted to
> introduce randomized platform_device creation from
> of_platform_bus_populate() at one point or another.
> 

Hmm. I might be missing something. pinctrl-nsp-gpio.c uses
arch_initcall_sync() and pinctrl-nsp-mux.c uses arch_initcall() so in
theory they are already in the right order.

> > 
> > Normally we and nodes in the device tree to be listed in their natural
> > memory mapped address order but putting the pinctrl node first avoids
> > the deferral of numerous devices so make an exception in this case.
> 
> That is a workaround more than a real solution, though I understand why
> you would to do that. One downside is that the entries are no longer in
> incrementing register address order and that is visually disturbing and
> who knows, maybe a drive by contributor whose pet project will be to
> order the Device Tree entries by incrementing addresses will change that
> in the future...
> 

I guess really what's needed is something that understands phandles and
tries to produce a dependency tree based on that.

> > 
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > ---
> >  arch/arm/boot/dts/bcm-nsp.dtsi | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
> > index da6d70f09ef1..dd7a65743c08 100644
> > --- a/arch/arm/boot/dts/bcm-nsp.dtsi
> > +++ b/arch/arm/boot/dts/bcm-nsp.dtsi
> > @@ -172,6 +172,13 @@
> >  		#address-cells = <1>;
> >  		#size-cells = <1>;
> >  
> > +		pinctrl: pinctrl@3f1c0 {
> > +			compatible = "brcm,nsp-pinmux";
> > +			reg = <0x3f1c0 0x04>,
> > +			      <0x30028 0x04>,
> > +			      <0x3f408 0x04>;
> > +		};
> > +
> >  		gpioa: gpio@20 {
> >  			compatible = "brcm,nsp-gpio-a";
> >  			reg = <0x0020 0x70>,
> > @@ -458,13 +465,6 @@
> >  					     "sata2";
> >  		};
> >  
> > -		pinctrl: pinctrl@3f1c0 {
> > -			compatible = "brcm,nsp-pinmux";
> > -			reg = <0x3f1c0 0x04>,
> > -			      <0x30028 0x04>,
> > -			      <0x3f408 0x04>;
> > -		};
> > -
> >  		thermal: thermal@3f2c0 {
> >  			compatible = "brcm,ns-thermal";
> >  			reg = <0x3f2c0 0x10>;
> > 
> 
>
Chris Packham Oct. 28, 2019, 9:44 p.m. UTC | #3
On Tue, 2019-10-29 at 09:21 +1300, Chris Packham wrote:
> On Fri, 2019-10-25 at 10:26 -0700, Florian Fainelli wrote:
> > On 10/24/19 9:00 PM, Chris Packham wrote:
> > > The pinctrl node is used by the gpioa node. Which may have more
> > > descendants at a board level. If the pinctrl node isn't probed first the
> > > gpio is deferred and anything that needs a gpio pin on that chip is also
> > > deferred.
> > 
> > If what you care is to optimize your boot flow such that no re-probing
> > occurs, maybe another solution to look at is to re-order the order in
> > which subsystems are initialized or built (_initcall changes or
> > drivers/Makefile changes), because changing Device Tree certainly does
> > not scale over platforms and I recall Rob indicating that he wanted to
> > introduce randomized platform_device creation from
> > of_platform_bus_populate() at one point or another.
> > 
> 
> Hmm. I might be missing something. pinctrl-nsp-gpio.c uses
> arch_initcall_sync() and pinctrl-nsp-mux.c uses arch_initcall() so in
> theory they are already in the right order.
> 

Actually the init calls are made in the required order w.r.t each
other. But they are both before of_platform_populate, so it's back to
the device tree being the determining factor for when the probe()
functions are run.

With the current kernel I get

nsp_pinmux_init:
nsp_gpio_init:
OF: of_platform_populate:
OF: of_platform_bus_create: /axi@18000000/gpio@20
nsp_gpio_probe:
gpiochip_add_data_with_key: GPIOs 480..511 (18000020.gpio) failed to
register, -517
nsp-gpio-a 18000020.gpio: unable to add GPIO chip
OF: of_platform_bus_create: /axi@18000000/pinctrl@3f1c0
nsp_pinmux_probe:
... much later ...
nsp_gpio_probe:

Would it be acceptable to change the init calls to device_initcall()
and device_initcall_sync()? pinctrl-nsp-mux.c could even be converted
to (builtin_)platform_driver.

> > > 
> > > Normally we and nodes in the device tree to be listed in their natural
> > > memory mapped address order but putting the pinctrl node first avoids
> > > the deferral of numerous devices so make an exception in this case.
> > 
> > That is a workaround more than a real solution, though I understand why
> > you would to do that. One downside is that the entries are no longer in
> > incrementing register address order and that is visually disturbing and
> > who knows, maybe a drive by contributor whose pet project will be to
> > order the Device Tree entries by incrementing addresses will change that
> > in the future...
> > 
> 
> I guess really what's needed is something that understands phandles and
> tries to produce a dependency tree based on that.
> 
> > > 
> > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > > ---
> > >  arch/arm/boot/dts/bcm-nsp.dtsi | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
> > > index da6d70f09ef1..dd7a65743c08 100644
> > > --- a/arch/arm/boot/dts/bcm-nsp.dtsi
> > > +++ b/arch/arm/boot/dts/bcm-nsp.dtsi
> > > @@ -172,6 +172,13 @@
> > >  		#address-cells = <1>;
> > >  		#size-cells = <1>;
> > >  
> > > +		pinctrl: pinctrl@3f1c0 {
> > > +			compatible = "brcm,nsp-pinmux";
> > > +			reg = <0x3f1c0 0x04>,
> > > +			      <0x30028 0x04>,
> > > +			      <0x3f408 0x04>;
> > > +		};
> > > +
> > >  		gpioa: gpio@20 {
> > >  			compatible = "brcm,nsp-gpio-a";
> > >  			reg = <0x0020 0x70>,
> > > @@ -458,13 +465,6 @@
> > >  					     "sata2";
> > >  		};
> > >  
> > > -		pinctrl: pinctrl@3f1c0 {
> > > -			compatible = "brcm,nsp-pinmux";
> > > -			reg = <0x3f1c0 0x04>,
> > > -			      <0x30028 0x04>,
> > > -			      <0x3f408 0x04>;
> > > -		};
> > > -
> > >  		thermal: thermal@3f2c0 {
> > >  			compatible = "brcm,ns-thermal";
> > >  			reg = <0x3f2c0 0x10>;
> > > 
> > 
> >

Patch
diff mbox series

diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
index da6d70f09ef1..dd7a65743c08 100644
--- a/arch/arm/boot/dts/bcm-nsp.dtsi
+++ b/arch/arm/boot/dts/bcm-nsp.dtsi
@@ -172,6 +172,13 @@ 
 		#address-cells = <1>;
 		#size-cells = <1>;
 
+		pinctrl: pinctrl@3f1c0 {
+			compatible = "brcm,nsp-pinmux";
+			reg = <0x3f1c0 0x04>,
+			      <0x30028 0x04>,
+			      <0x3f408 0x04>;
+		};
+
 		gpioa: gpio@20 {
 			compatible = "brcm,nsp-gpio-a";
 			reg = <0x0020 0x70>,
@@ -458,13 +465,6 @@ 
 					     "sata2";
 		};
 
-		pinctrl: pinctrl@3f1c0 {
-			compatible = "brcm,nsp-pinmux";
-			reg = <0x3f1c0 0x04>,
-			      <0x30028 0x04>,
-			      <0x3f408 0x04>;
-		};
-
 		thermal: thermal@3f2c0 {
 			compatible = "brcm,ns-thermal";
 			reg = <0x3f2c0 0x10>;