| Message ID | 20191025040041.6210-3-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: bcm: nsp: gpio improvements (hopefully) | expand |
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>; >
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>; > > > >
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>; > > > > > > >
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>;
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(-)