[PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Simon Glass]

This breaks chromebook_coral and it is also not how things should work. If
a board needs to bind GPIOs as part of a pinctrl driver this can be done
during the bind step, if needed.

We cannot probe pinctrl devices when binding as a rule, since it cannot be
supported on some platforms.

The bind and probe steps are separate in U-Boot and they should remain
separate.

This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/pinctrl/pinctrl-uclass.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
index ce2d5ddf6d9..a1b85ca87e5 100644
--- a/drivers/pinctrl/pinctrl-uclass.c
+++ b/drivers/pinctrl/pinctrl-uclass.c
@@ -403,13 +403,6 @@ static int __maybe_unused pinctrl_post_bind(struct udevice *dev)
 {
 	const struct pinctrl_ops *ops = pinctrl_get_ops(dev);
 
-	/*
-	 * Make sure that the pinctrl driver gets probed after binding
-	 * as some pinctrl drivers also register the GPIO driver during
-	 * probe, and if they are not probed GPIO-s are not registered.
-	 */
-	dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
-
 	if (!ops) {
 		dev_dbg(dev, "ops is not set.  Do not bind.\n");
 		return -EINVAL;
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Pali Rohár]

On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
> This breaks chromebook_coral and it is also not how things should work. If
> a board needs to bind GPIOs as part of a pinctrl driver this can be done
> during the bind step, if needed.
> 
> We cannot probe pinctrl devices when binding as a rule, since it cannot be
> supported on some platforms.
> 
> The bind and probe steps are separate in U-Boot and they should remain
> separate.
> 
> This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.

Unfortunately reverting this patch would break other devices, mostly
A3720 based where pinctrl driver acts also as gpio driver. Because no
other caller then register gpio driver and so other drivers which parses
gpios from DT (which belongs to that gpio driver) will fail during
probe.

Also I think that pinctrl command would not work in this case if pinctrl
would not be probed.

> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  drivers/pinctrl/pinctrl-uclass.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
> index ce2d5ddf6d9..a1b85ca87e5 100644
> --- a/drivers/pinctrl/pinctrl-uclass.c
> +++ b/drivers/pinctrl/pinctrl-uclass.c
> @@ -403,13 +403,6 @@ static int __maybe_unused pinctrl_post_bind(struct udevice *dev)
>  {
>  	const struct pinctrl_ops *ops = pinctrl_get_ops(dev);
>  
> -	/*
> -	 * Make sure that the pinctrl driver gets probed after binding
> -	 * as some pinctrl drivers also register the GPIO driver during
> -	 * probe, and if they are not probed GPIO-s are not registered.
> -	 */
> -	dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
> -
>  	if (!ops) {
>  		dev_dbg(dev, "ops is not set.  Do not bind.\n");
>  		return -EINVAL;
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Simon Glass]

Hi Pali,

On Wed, 21 Dec 2022 at 17:02, Pali Rohár <pali@kernel.org> wrote:
>
> On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
> > This breaks chromebook_coral and it is also not how things should work. If
> > a board needs to bind GPIOs as part of a pinctrl driver this can be done
> > during the bind step, if needed.
> >
> > We cannot probe pinctrl devices when binding as a rule, since it cannot be
> > supported on some platforms.
> >
> > The bind and probe steps are separate in U-Boot and they should remain
> > separate.
> >
> > This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.
>
> Unfortunately reverting this patch would break other devices, mostly
> A3720 based where pinctrl driver acts also as gpio driver. Because no
> other caller then register gpio driver and so other drivers which parses
> gpios from DT (which belongs to that gpio driver) will fail during
> probe.

That is something to be sorted out for that platform. You can bind
GPIO devices in the pinctrl driver's bind() method as other SoCs do.
Even better, the device tree typically has that info in it, i.e. GPIO
subnodes within the pinctrl node.

Probing pinctrl in a bind function is unfortunately just wrong. It
will cause all sorts of problems, and perhaps already has.

>
> Also I think that pinctrl command would not work in this case if pinctrl
> would not be probed.

Devices are probed before use, including by commands.

This is quite important to fix before the release.

Regards,
Simon

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Pali Rohár]

On Friday 30 December 2022 09:30:27 Simon Glass wrote:
> Hi Pali,
> 
> On Wed, 21 Dec 2022 at 17:02, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
> > > This breaks chromebook_coral and it is also not how things should work. If
> > > a board needs to bind GPIOs as part of a pinctrl driver this can be done
> > > during the bind step, if needed.
> > >
> > > We cannot probe pinctrl devices when binding as a rule, since it cannot be
> > > supported on some platforms.
> > >
> > > The bind and probe steps are separate in U-Boot and they should remain
> > > separate.
> > >
> > > This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.
> >
> > Unfortunately reverting this patch would break other devices, mostly
> > A3720 based where pinctrl driver acts also as gpio driver. Because no
> > other caller then register gpio driver and so other drivers which parses
> > gpios from DT (which belongs to that gpio driver) will fail during
> > probe.
> 
> That is something to be sorted out for that platform. You can bind
> GPIO devices in the pinctrl driver's bind() method as other SoCs do.
> Even better, the device tree typically has that info in it, i.e. GPIO
> subnodes within the pinctrl node.
> 
> Probing pinctrl in a bind function is unfortunately just wrong. It
> will cause all sorts of problems, and perhaps already has.

Ok, so it means that drivers/pinctrl/mvebu/pinctrl-armada-37xx.c needs
to be refactored and fixed to handle these restrictions.

> > Also I think that pinctrl command would not work in this case if pinctrl
> > would not be probed.
> 
> Devices are probed before use, including by commands.
> 
> This is quite important to fix before the release.

Unfortunately in this time I do not have any a3720 board for testing.
Robert was able to easily trigger this issue and also introduced that
commit f9ec791b5e24 ("pinctrl: probe pinctrl drivers during post-bind").

So may I ask Robert to look at it? In past days I looked at powerpc
release issues and I do not have time for other things.

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Simon Glass]

Hi Pali,

On Fri, 30 Dec 2022 at 09:53, Pali Rohár <pali@kernel.org> wrote:
>
> On Friday 30 December 2022 09:30:27 Simon Glass wrote:
> > Hi Pali,
> >
> > On Wed, 21 Dec 2022 at 17:02, Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
> > > > This breaks chromebook_coral and it is also not how things should work. If
> > > > a board needs to bind GPIOs as part of a pinctrl driver this can be done
> > > > during the bind step, if needed.
> > > >
> > > > We cannot probe pinctrl devices when binding as a rule, since it cannot be
> > > > supported on some platforms.
> > > >
> > > > The bind and probe steps are separate in U-Boot and they should remain
> > > > separate.
> > > >
> > > > This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.
> > >
> > > Unfortunately reverting this patch would break other devices, mostly
> > > A3720 based where pinctrl driver acts also as gpio driver. Because no
> > > other caller then register gpio driver and so other drivers which parses
> > > gpios from DT (which belongs to that gpio driver) will fail during
> > > probe.
> >
> > That is something to be sorted out for that platform. You can bind
> > GPIO devices in the pinctrl driver's bind() method as other SoCs do.
> > Even better, the device tree typically has that info in it, i.e. GPIO
> > subnodes within the pinctrl node.
> >
> > Probing pinctrl in a bind function is unfortunately just wrong. It
> > will cause all sorts of problems, and perhaps already has.
>
> Ok, so it means that drivers/pinctrl/mvebu/pinctrl-armada-37xx.c needs
> to be refactored and fixed to handle these restrictions.

It is not a restriction. It is simply that binding and probing are
different things and we should not tie them together. It will just
become a nightmare for board bringup and other drivers.

>
> > > Also I think that pinctrl command would not work in this case if pinctrl
> > > would not be probed.
> >
> > Devices are probed before use, including by commands.
> >
> > This is quite important to fix before the release.
>
> Unfortunately in this time I do not have any a3720 board for testing.
> Robert was able to easily trigger this issue and also introduced that
> commit f9ec791b5e24 ("pinctrl: probe pinctrl drivers during post-bind").
>
> So may I ask Robert to look at it? In past days I looked at powerpc
> release issues and I do not have time for other things.

That driver has a few FIXMEs in it and could use a look anyway. I see
that the gpio controller is a subnode of pinctrl anyway. Adding a
compatible string for it would fix the problem  just like that, and
remove a lot of ugly code in the driver. This Linux-centric nature of
device tree bindings really is infuriating:

/* FIXME: Use livtree and check the result of device_bind() below */
fdt_for_each_subnode(subnode, blob, node) {
    if (fdtdec_get_bool(blob, subnode, "gpio-controller")) {
        ret = 0;
        break;
    }
};
if (ret)
   return ret;


Failing that it just needs a bind() method that calls
armada_37xx_gpiochip_register()

Regards,
Simon

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Pali Rohár]

On Friday 30 December 2022 10:00:11 Simon Glass wrote:
> Hi Pali,
> 
> On Fri, 30 Dec 2022 at 09:53, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Friday 30 December 2022 09:30:27 Simon Glass wrote:
> > > Hi Pali,
> > >
> > > On Wed, 21 Dec 2022 at 17:02, Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
> > > > > This breaks chromebook_coral and it is also not how things should work. If
> > > > > a board needs to bind GPIOs as part of a pinctrl driver this can be done
> > > > > during the bind step, if needed.
> > > > >
> > > > > We cannot probe pinctrl devices when binding as a rule, since it cannot be
> > > > > supported on some platforms.
> > > > >
> > > > > The bind and probe steps are separate in U-Boot and they should remain
> > > > > separate.
> > > > >
> > > > > This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.
> > > >
> > > > Unfortunately reverting this patch would break other devices, mostly
> > > > A3720 based where pinctrl driver acts also as gpio driver. Because no
> > > > other caller then register gpio driver and so other drivers which parses
> > > > gpios from DT (which belongs to that gpio driver) will fail during
> > > > probe.
> > >
> > > That is something to be sorted out for that platform. You can bind
> > > GPIO devices in the pinctrl driver's bind() method as other SoCs do.
> > > Even better, the device tree typically has that info in it, i.e. GPIO
> > > subnodes within the pinctrl node.
> > >
> > > Probing pinctrl in a bind function is unfortunately just wrong. It
> > > will cause all sorts of problems, and perhaps already has.
> >
> > Ok, so it means that drivers/pinctrl/mvebu/pinctrl-armada-37xx.c needs
> > to be refactored and fixed to handle these restrictions.
> 
> It is not a restriction. It is simply that binding and probing are
> different things and we should not tie them together. It will just
> become a nightmare for board bringup and other drivers.
> 
> >
> > > > Also I think that pinctrl command would not work in this case if pinctrl
> > > > would not be probed.
> > >
> > > Devices are probed before use, including by commands.
> > >
> > > This is quite important to fix before the release.
> >
> > Unfortunately in this time I do not have any a3720 board for testing.
> > Robert was able to easily trigger this issue and also introduced that
> > commit f9ec791b5e24 ("pinctrl: probe pinctrl drivers during post-bind").
> >
> > So may I ask Robert to look at it? In past days I looked at powerpc
> > release issues and I do not have time for other things.
> 
> That driver has a few FIXMEs in it and could use a look anyway. I see
> that the gpio controller is a subnode of pinctrl anyway. Adding a
> compatible string for it would fix the problem  just like that, and
> remove a lot of ugly code in the driver. This Linux-centric nature of
> device tree bindings really is infuriating:

All a3720 DTS files are 1:1 copied from Linux. So if problem is in DTS
file then it should be discussed with Linux dt/mvebu maintainers. This
year I fixed U-Boot code to handle Linux a3720 DTS files and then copied
DTS files from Linux. So it is not a good idea to have again different
DTS file for u-boot and different for kernel.

> /* FIXME: Use livtree and check the result of device_bind() below */
> fdt_for_each_subnode(subnode, blob, node) {
>     if (fdtdec_get_bool(blob, subnode, "gpio-controller")) {
>         ret = 0;
>         break;
>     }
> };
> if (ret)
>    return ret;
> 
> 
> Failing that it just needs a bind() method that calls
> armada_37xx_gpiochip_register()
> 
> Regards,
> Simon

Seems that it is not such simple. armada_37xx_gpiochip_register()
depends on initialized and bound pinctrl part of driver. So before
binding gpio you need to bind pinctrl as they share internal structures.

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Simon Glass]

Hi Pali,

On Fri, 30 Dec 2022 at 10:13, Pali Rohár <pali@kernel.org> wrote:
>
> On Friday 30 December 2022 10:00:11 Simon Glass wrote:
> > Hi Pali,
> >
> > On Fri, 30 Dec 2022 at 09:53, Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Friday 30 December 2022 09:30:27 Simon Glass wrote:
> > > > Hi Pali,
> > > >
> > > > On Wed, 21 Dec 2022 at 17:02, Pali Rohár <pali@kernel.org> wrote:
> > > > >
> > > > > On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
> > > > > > This breaks chromebook_coral and it is also not how things should work. If
> > > > > > a board needs to bind GPIOs as part of a pinctrl driver this can be done
> > > > > > during the bind step, if needed.
> > > > > >
> > > > > > We cannot probe pinctrl devices when binding as a rule, since it cannot be
> > > > > > supported on some platforms.
> > > > > >
> > > > > > The bind and probe steps are separate in U-Boot and they should remain
> > > > > > separate.
> > > > > >
> > > > > > This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.
> > > > >
> > > > > Unfortunately reverting this patch would break other devices, mostly
> > > > > A3720 based where pinctrl driver acts also as gpio driver. Because no
> > > > > other caller then register gpio driver and so other drivers which parses
> > > > > gpios from DT (which belongs to that gpio driver) will fail during
> > > > > probe.
> > > >
> > > > That is something to be sorted out for that platform. You can bind
> > > > GPIO devices in the pinctrl driver's bind() method as other SoCs do.
> > > > Even better, the device tree typically has that info in it, i.e. GPIO
> > > > subnodes within the pinctrl node.
> > > >
> > > > Probing pinctrl in a bind function is unfortunately just wrong. It
> > > > will cause all sorts of problems, and perhaps already has.
> > >
> > > Ok, so it means that drivers/pinctrl/mvebu/pinctrl-armada-37xx.c needs
> > > to be refactored and fixed to handle these restrictions.
> >
> > It is not a restriction. It is simply that binding and probing are
> > different things and we should not tie them together. It will just
> > become a nightmare for board bringup and other drivers.
> >
> > >
> > > > > Also I think that pinctrl command would not work in this case if pinctrl
> > > > > would not be probed.
> > > >
> > > > Devices are probed before use, including by commands.
> > > >
> > > > This is quite important to fix before the release.
> > >
> > > Unfortunately in this time I do not have any a3720 board for testing.
> > > Robert was able to easily trigger this issue and also introduced that
> > > commit f9ec791b5e24 ("pinctrl: probe pinctrl drivers during post-bind").
> > >
> > > So may I ask Robert to look at it? In past days I looked at powerpc
> > > release issues and I do not have time for other things.
> >
> > That driver has a few FIXMEs in it and could use a look anyway. I see
> > that the gpio controller is a subnode of pinctrl anyway. Adding a
> > compatible string for it would fix the problem  just like that, and
> > remove a lot of ugly code in the driver. This Linux-centric nature of
> > device tree bindings really is infuriating:
>
> All a3720 DTS files are 1:1 copied from Linux. So if problem is in DTS
> file then it should be discussed with Linux dt/mvebu maintainers. This
> year I fixed U-Boot code to handle Linux a3720 DTS files and then copied
> DTS files from Linux. So it is not a good idea to have again different
> DTS file for u-boot and different for kernel.
>
> > /* FIXME: Use livtree and check the result of device_bind() below */
> > fdt_for_each_subnode(subnode, blob, node) {
> >     if (fdtdec_get_bool(blob, subnode, "gpio-controller")) {
> >         ret = 0;
> >         break;
> >     }
> > };
> > if (ret)
> >    return ret;
> >
> >
> > Failing that it just needs a bind() method that calls
> > armada_37xx_gpiochip_register()
> >
> > Regards,
> > Simon
>
> Seems that it is not such simple. armada_37xx_gpiochip_register()
> depends on initialized and bound pinctrl part of driver. So before
> binding gpio you need to bind pinctrl as they share internal structures.

Where are you seeing that? From what I can tell it just binds the GPIO
driver. It doesn't probe it. So long as you bind the GPIO driver in
armada_37xx_pinctrl_bind() it should be equivalent.

And don't forget the root cause of this whole problem is Linux-centrix
DT bindings.

Regards,
Simon

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4932 bytes --]

On Fri, Dec 30, 2022 at 11:47:29AM -0600, Simon Glass wrote:
> Hi Pali,
> 
> On Fri, 30 Dec 2022 at 10:13, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Friday 30 December 2022 10:00:11 Simon Glass wrote:
> > > Hi Pali,
> > >
> > > On Fri, 30 Dec 2022 at 09:53, Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Friday 30 December 2022 09:30:27 Simon Glass wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Wed, 21 Dec 2022 at 17:02, Pali Rohár <pali@kernel.org> wrote:
> > > > > >
> > > > > > On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
> > > > > > > This breaks chromebook_coral and it is also not how things should work. If
> > > > > > > a board needs to bind GPIOs as part of a pinctrl driver this can be done
> > > > > > > during the bind step, if needed.
> > > > > > >
> > > > > > > We cannot probe pinctrl devices when binding as a rule, since it cannot be
> > > > > > > supported on some platforms.
> > > > > > >
> > > > > > > The bind and probe steps are separate in U-Boot and they should remain
> > > > > > > separate.
> > > > > > >
> > > > > > > This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.
> > > > > >
> > > > > > Unfortunately reverting this patch would break other devices, mostly
> > > > > > A3720 based where pinctrl driver acts also as gpio driver. Because no
> > > > > > other caller then register gpio driver and so other drivers which parses
> > > > > > gpios from DT (which belongs to that gpio driver) will fail during
> > > > > > probe.
> > > > >
> > > > > That is something to be sorted out for that platform. You can bind
> > > > > GPIO devices in the pinctrl driver's bind() method as other SoCs do.
> > > > > Even better, the device tree typically has that info in it, i.e. GPIO
> > > > > subnodes within the pinctrl node.
> > > > >
> > > > > Probing pinctrl in a bind function is unfortunately just wrong. It
> > > > > will cause all sorts of problems, and perhaps already has.
> > > >
> > > > Ok, so it means that drivers/pinctrl/mvebu/pinctrl-armada-37xx.c needs
> > > > to be refactored and fixed to handle these restrictions.
> > >
> > > It is not a restriction. It is simply that binding and probing are
> > > different things and we should not tie them together. It will just
> > > become a nightmare for board bringup and other drivers.
> > >
> > > >
> > > > > > Also I think that pinctrl command would not work in this case if pinctrl
> > > > > > would not be probed.
> > > > >
> > > > > Devices are probed before use, including by commands.
> > > > >
> > > > > This is quite important to fix before the release.
> > > >
> > > > Unfortunately in this time I do not have any a3720 board for testing.
> > > > Robert was able to easily trigger this issue and also introduced that
> > > > commit f9ec791b5e24 ("pinctrl: probe pinctrl drivers during post-bind").
> > > >
> > > > So may I ask Robert to look at it? In past days I looked at powerpc
> > > > release issues and I do not have time for other things.
> > >
> > > That driver has a few FIXMEs in it and could use a look anyway. I see
> > > that the gpio controller is a subnode of pinctrl anyway. Adding a
> > > compatible string for it would fix the problem  just like that, and
> > > remove a lot of ugly code in the driver. This Linux-centric nature of
> > > device tree bindings really is infuriating:
> >
> > All a3720 DTS files are 1:1 copied from Linux. So if problem is in DTS
> > file then it should be discussed with Linux dt/mvebu maintainers. This
> > year I fixed U-Boot code to handle Linux a3720 DTS files and then copied
> > DTS files from Linux. So it is not a good idea to have again different
> > DTS file for u-boot and different for kernel.
> >
> > > /* FIXME: Use livtree and check the result of device_bind() below */
> > > fdt_for_each_subnode(subnode, blob, node) {
> > >     if (fdtdec_get_bool(blob, subnode, "gpio-controller")) {
> > >         ret = 0;
> > >         break;
> > >     }
> > > };
> > > if (ret)
> > >    return ret;
> > >
> > >
> > > Failing that it just needs a bind() method that calls
> > > armada_37xx_gpiochip_register()
> > >
> > > Regards,
> > > Simon
> >
> > Seems that it is not such simple. armada_37xx_gpiochip_register()
> > depends on initialized and bound pinctrl part of driver. So before
> > binding gpio you need to bind pinctrl as they share internal structures.
> 
> Where are you seeing that? From what I can tell it just binds the GPIO
> driver. It doesn't probe it. So long as you bind the GPIO driver in
> armada_37xx_pinctrl_bind() it should be equivalent.
> 
> And don't forget the root cause of this whole problem is Linux-centrix
> DT bindings.

And perhaps something in the -u-boot.dtsi for now will let us resolve
this for the release, and see what changes upstream is or isn't
interested in taking at this point.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Pali Rohár]

On Friday 30 December 2022 11:47:29 Simon Glass wrote:
> Hi Pali,
> 
> On Fri, 30 Dec 2022 at 10:13, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Friday 30 December 2022 10:00:11 Simon Glass wrote:
> > > Hi Pali,
> > >
> > > On Fri, 30 Dec 2022 at 09:53, Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Friday 30 December 2022 09:30:27 Simon Glass wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Wed, 21 Dec 2022 at 17:02, Pali Rohár <pali@kernel.org> wrote:
> > > > > >
> > > > > > On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
> > > > > > > This breaks chromebook_coral and it is also not how things should work. If
> > > > > > > a board needs to bind GPIOs as part of a pinctrl driver this can be done
> > > > > > > during the bind step, if needed.
> > > > > > >
> > > > > > > We cannot probe pinctrl devices when binding as a rule, since it cannot be
> > > > > > > supported on some platforms.
> > > > > > >
> > > > > > > The bind and probe steps are separate in U-Boot and they should remain
> > > > > > > separate.
> > > > > > >
> > > > > > > This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.
> > > > > >
> > > > > > Unfortunately reverting this patch would break other devices, mostly
> > > > > > A3720 based where pinctrl driver acts also as gpio driver. Because no
> > > > > > other caller then register gpio driver and so other drivers which parses
> > > > > > gpios from DT (which belongs to that gpio driver) will fail during
> > > > > > probe.
> > > > >
> > > > > That is something to be sorted out for that platform. You can bind
> > > > > GPIO devices in the pinctrl driver's bind() method as other SoCs do.
> > > > > Even better, the device tree typically has that info in it, i.e. GPIO
> > > > > subnodes within the pinctrl node.
> > > > >
> > > > > Probing pinctrl in a bind function is unfortunately just wrong. It
> > > > > will cause all sorts of problems, and perhaps already has.
> > > >
> > > > Ok, so it means that drivers/pinctrl/mvebu/pinctrl-armada-37xx.c needs
> > > > to be refactored and fixed to handle these restrictions.
> > >
> > > It is not a restriction. It is simply that binding and probing are
> > > different things and we should not tie them together. It will just
> > > become a nightmare for board bringup and other drivers.
> > >
> > > >
> > > > > > Also I think that pinctrl command would not work in this case if pinctrl
> > > > > > would not be probed.
> > > > >
> > > > > Devices are probed before use, including by commands.
> > > > >
> > > > > This is quite important to fix before the release.
> > > >
> > > > Unfortunately in this time I do not have any a3720 board for testing.
> > > > Robert was able to easily trigger this issue and also introduced that
> > > > commit f9ec791b5e24 ("pinctrl: probe pinctrl drivers during post-bind").
> > > >
> > > > So may I ask Robert to look at it? In past days I looked at powerpc
> > > > release issues and I do not have time for other things.
> > >
> > > That driver has a few FIXMEs in it and could use a look anyway. I see
> > > that the gpio controller is a subnode of pinctrl anyway. Adding a
> > > compatible string for it would fix the problem  just like that, and
> > > remove a lot of ugly code in the driver. This Linux-centric nature of
> > > device tree bindings really is infuriating:
> >
> > All a3720 DTS files are 1:1 copied from Linux. So if problem is in DTS
> > file then it should be discussed with Linux dt/mvebu maintainers. This
> > year I fixed U-Boot code to handle Linux a3720 DTS files and then copied
> > DTS files from Linux. So it is not a good idea to have again different
> > DTS file for u-boot and different for kernel.
> >
> > > /* FIXME: Use livtree and check the result of device_bind() below */
> > > fdt_for_each_subnode(subnode, blob, node) {
> > >     if (fdtdec_get_bool(blob, subnode, "gpio-controller")) {
> > >         ret = 0;
> > >         break;
> > >     }
> > > };
> > > if (ret)
> > >    return ret;
> > >
> > >
> > > Failing that it just needs a bind() method that calls
> > > armada_37xx_gpiochip_register()
> > >
> > > Regards,
> > > Simon
> >
> > Seems that it is not such simple. armada_37xx_gpiochip_register()
> > depends on initialized and bound pinctrl part of driver. So before
> > binding gpio you need to bind pinctrl as they share internal structures.
> 
> Where are you seeing that? From what I can tell it just binds the GPIO
> driver. It doesn't probe it. So long as you bind the GPIO driver in
> armada_37xx_pinctrl_bind() it should be equivalent.

armada_37xx_gpiochip_register() calls device_bind() for
&armada_37xx_gpio_driver which probe method armada_37xx_gpio_probe() and
ops &armada_37xx_gpio_ops callbacks access a37xx pinctrl internal
structures.

So I'm not sure if there is an issue or not. But for sure a37xx gpio
must be probed after a37xx pinctrl is probed because a37xx pinctrl probe
function fills internal a37xx pinctrl strucutre used by a37xx gpio probe
function.

> And don't forget the root cause of this whole problem is Linux-centrix
> DT bindings.
> 
> Regards,
> Simon

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Simon Glass]

Hi Pali,

On Fri, 30 Dec 2022 at 12:02, Pali Rohár <pali@kernel.org> wrote:
>
> On Friday 30 December 2022 11:47:29 Simon Glass wrote:
> > Hi Pali,
> >
> > On Fri, 30 Dec 2022 at 10:13, Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Friday 30 December 2022 10:00:11 Simon Glass wrote:
> > > > Hi Pali,
> > > >
> > > > On Fri, 30 Dec 2022 at 09:53, Pali Rohár <pali@kernel.org> wrote:
> > > > >
> > > > > On Friday 30 December 2022 09:30:27 Simon Glass wrote:
> > > > > > Hi Pali,
> > > > > >
> > > > > > On Wed, 21 Dec 2022 at 17:02, Pali Rohár <pali@kernel.org> wrote:
> > > > > > >
> > > > > > > On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
> > > > > > > > This breaks chromebook_coral and it is also not how things should work. If
> > > > > > > > a board needs to bind GPIOs as part of a pinctrl driver this can be done
> > > > > > > > during the bind step, if needed.
> > > > > > > >
> > > > > > > > We cannot probe pinctrl devices when binding as a rule, since it cannot be
> > > > > > > > supported on some platforms.
> > > > > > > >
> > > > > > > > The bind and probe steps are separate in U-Boot and they should remain
> > > > > > > > separate.
> > > > > > > >
> > > > > > > > This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.
> > > > > > >
> > > > > > > Unfortunately reverting this patch would break other devices, mostly
> > > > > > > A3720 based where pinctrl driver acts also as gpio driver. Because no
> > > > > > > other caller then register gpio driver and so other drivers which parses
> > > > > > > gpios from DT (which belongs to that gpio driver) will fail during
> > > > > > > probe.
> > > > > >
> > > > > > That is something to be sorted out for that platform. You can bind
> > > > > > GPIO devices in the pinctrl driver's bind() method as other SoCs do.
> > > > > > Even better, the device tree typically has that info in it, i.e. GPIO
> > > > > > subnodes within the pinctrl node.
> > > > > >
> > > > > > Probing pinctrl in a bind function is unfortunately just wrong. It
> > > > > > will cause all sorts of problems, and perhaps already has.
> > > > >
> > > > > Ok, so it means that drivers/pinctrl/mvebu/pinctrl-armada-37xx.c needs
> > > > > to be refactored and fixed to handle these restrictions.
> > > >
> > > > It is not a restriction. It is simply that binding and probing are
> > > > different things and we should not tie them together. It will just
> > > > become a nightmare for board bringup and other drivers.
> > > >
> > > > >
> > > > > > > Also I think that pinctrl command would not work in this case if pinctrl
> > > > > > > would not be probed.
> > > > > >
> > > > > > Devices are probed before use, including by commands.
> > > > > >
> > > > > > This is quite important to fix before the release.
> > > > >
> > > > > Unfortunately in this time I do not have any a3720 board for testing.
> > > > > Robert was able to easily trigger this issue and also introduced that
> > > > > commit f9ec791b5e24 ("pinctrl: probe pinctrl drivers during post-bind").
> > > > >
> > > > > So may I ask Robert to look at it? In past days I looked at powerpc
> > > > > release issues and I do not have time for other things.
> > > >
> > > > That driver has a few FIXMEs in it and could use a look anyway. I see
> > > > that the gpio controller is a subnode of pinctrl anyway. Adding a
> > > > compatible string for it would fix the problem  just like that, and
> > > > remove a lot of ugly code in the driver. This Linux-centric nature of
> > > > device tree bindings really is infuriating:
> > >
> > > All a3720 DTS files are 1:1 copied from Linux. So if problem is in DTS
> > > file then it should be discussed with Linux dt/mvebu maintainers. This
> > > year I fixed U-Boot code to handle Linux a3720 DTS files and then copied
> > > DTS files from Linux. So it is not a good idea to have again different
> > > DTS file for u-boot and different for kernel.

That was not my suggestion. I would simply like the bindings in Linux
to be more explicit, rather than having driver code manually written
to do what the device tree is supposed to do.

> > >
> > > > /* FIXME: Use livtree and check the result of device_bind() below */
> > > > fdt_for_each_subnode(subnode, blob, node) {
> > > >     if (fdtdec_get_bool(blob, subnode, "gpio-controller")) {
> > > >         ret = 0;
> > > >         break;
> > > >     }
> > > > };
> > > > if (ret)
> > > >    return ret;
> > > >
> > > >
> > > > Failing that it just needs a bind() method that calls
> > > > armada_37xx_gpiochip_register()
> > > >
> > > > Regards,
> > > > Simon
> > >
> > > Seems that it is not such simple. armada_37xx_gpiochip_register()
> > > depends on initialized and bound pinctrl part of driver. So before
> > > binding gpio you need to bind pinctrl as they share internal structures.
> >
> > Where are you seeing that? From what I can tell it just binds the GPIO
> > driver. It doesn't probe it. So long as you bind the GPIO driver in
> > armada_37xx_pinctrl_bind() it should be equivalent.
>
> armada_37xx_gpiochip_register() calls device_bind() for
> &armada_37xx_gpio_driver which probe method armada_37xx_gpio_probe() and
> ops &armada_37xx_gpio_ops callbacks access a37xx pinctrl internal
> structures.

Yes but probing is different from binding, please see [1]

>
> So I'm not sure if there is an issue or not. But for sure a37xx gpio
> must be probed after a37xx pinctrl is probed because a37xx pinctrl probe
> function fills internal a37xx pinctrl strucutre used by a37xx gpio probe
> function.

Yes, children are probed after parents, as in docs:

"All parent devices are probed. It is not possible to activate a
device unless its predecessors (all the way up to the root device) are
activated. This means (for example) that an I2C driver will require
that its bus be activated."

>
> > And don't forget the root cause of this whole problem is Linux-centrix
> > DT bindings.

Regards,
Simon

[1] https://u-boot.readthedocs.io/en/latest/develop/driver-model/design.html#driver-lifecycle

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Robert Marko]

On Fri, Dec 30, 2022 at 8:02 PM Simon Glass <sjg@chromium.org> wrote:

> Hi Pali,
>
> On Fri, 30 Dec 2022 at 12:02, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Friday 30 December 2022 11:47:29 Simon Glass wrote:
> > > Hi Pali,
> > >
> > > On Fri, 30 Dec 2022 at 10:13, Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Friday 30 December 2022 10:00:11 Simon Glass wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Fri, 30 Dec 2022 at 09:53, Pali Rohár <pali@kernel.org> wrote:
> > > > > >
> > > > > > On Friday 30 December 2022 09:30:27 Simon Glass wrote:
> > > > > > > Hi Pali,
> > > > > > >
> > > > > > > On Wed, 21 Dec 2022 at 17:02, Pali Rohár <pali@kernel.org>
> wrote:
> > > > > > > >
> > > > > > > > On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
> > > > > > > > > This breaks chromebook_coral and it is also not how things
> should work. If
> > > > > > > > > a board needs to bind GPIOs as part of a pinctrl driver
> this can be done
> > > > > > > > > during the bind step, if needed.
> > > > > > > > >
> > > > > > > > > We cannot probe pinctrl devices when binding as a rule,
> since it cannot be
> > > > > > > > > supported on some platforms.
> > > > > > > > >
> > > > > > > > > The bind and probe steps are separate in U-Boot and they
> should remain
> > > > > > > > > separate.
> > > > > > > > >
> > > > > > > > > This reverts commit
> f9ec791b5e24378b71590877499f8683d5f54dac.
> > > > > > > >
> > > > > > > > Unfortunately reverting this patch would break other
> devices, mostly
> > > > > > > > A3720 based where pinctrl driver acts also as gpio driver.
> Because no
> > > > > > > > other caller then register gpio driver and so other drivers
> which parses
> > > > > > > > gpios from DT (which belongs to that gpio driver) will fail
> during
> > > > > > > > probe.
> > > > > > >
> > > > > > > That is something to be sorted out for that platform. You can
> bind
> > > > > > > GPIO devices in the pinctrl driver's bind() method as other
> SoCs do.
> > > > > > > Even better, the device tree typically has that info in it,
> i.e. GPIO
> > > > > > > subnodes within the pinctrl node.
> > > > > > >
> > > > > > > Probing pinctrl in a bind function is unfortunately just
> wrong. It
> > > > > > > will cause all sorts of problems, and perhaps already has.
> > > > > >
> > > > > > Ok, so it means that drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> needs
> > > > > > to be refactored and fixed to handle these restrictions.
> > > > >
> > > > > It is not a restriction. It is simply that binding and probing are
> > > > > different things and we should not tie them together. It will just
> > > > > become a nightmare for board bringup and other drivers.
> > > > >
> > > > > >
> > > > > > > > Also I think that pinctrl command would not work in this
> case if pinctrl
> > > > > > > > would not be probed.
> > > > > > >
> > > > > > > Devices are probed before use, including by commands.
> > > > > > >
> > > > > > > This is quite important to fix before the release.
> > > > > >
> > > > > > Unfortunately in this time I do not have any a3720 board for
> testing.
> > > > > > Robert was able to easily trigger this issue and also introduced
> that
> > > > > > commit f9ec791b5e24 ("pinctrl: probe pinctrl drivers during
> post-bind").
> > > > > >
> > > > > > So may I ask Robert to look at it? In past days I looked at
> powerpc
> > > > > > release issues and I do not have time for other things.
> > > > >
> > > > > That driver has a few FIXMEs in it and could use a look anyway. I
> see
> > > > > that the gpio controller is a subnode of pinctrl anyway. Adding a
> > > > > compatible string for it would fix the problem  just like that, and
> > > > > remove a lot of ugly code in the driver. This Linux-centric nature
> of
> > > > > device tree bindings really is infuriating:
> > > >
> > > > All a3720 DTS files are 1:1 copied from Linux. So if problem is in
> DTS
> > > > file then it should be discussed with Linux dt/mvebu maintainers.
> This
> > > > year I fixed U-Boot code to handle Linux a3720 DTS files and then
> copied
> > > > DTS files from Linux. So it is not a good idea to have again
> different
> > > > DTS file for u-boot and different for kernel.
>
> That was not my suggestion. I would simply like the bindings in Linux
> to be more explicit, rather than having driver code manually written
> to do what the device tree is supposed to do.
>
> > > >
> > > > > /* FIXME: Use livtree and check the result of device_bind() below
> */
> > > > > fdt_for_each_subnode(subnode, blob, node) {
> > > > >     if (fdtdec_get_bool(blob, subnode, "gpio-controller")) {
> > > > >         ret = 0;
> > > > >         break;
> > > > >     }
> > > > > };
> > > > > if (ret)
> > > > >    return ret;
> > > > >
> > > > >
> > > > > Failing that it just needs a bind() method that calls
> > > > > armada_37xx_gpiochip_register()
> > > > >
> > > > > Regards,
> > > > > Simon
> > > >
> > > > Seems that it is not such simple. armada_37xx_gpiochip_register()
> > > > depends on initialized and bound pinctrl part of driver. So before
> > > > binding gpio you need to bind pinctrl as they share internal
> structures.
> > >
> > > Where are you seeing that? From what I can tell it just binds the GPIO
> > > driver. It doesn't probe it. So long as you bind the GPIO driver in
> > > armada_37xx_pinctrl_bind() it should be equivalent.
> >
> > armada_37xx_gpiochip_register() calls device_bind() for
> > &armada_37xx_gpio_driver which probe method armada_37xx_gpio_probe() and
> > ops &armada_37xx_gpio_ops callbacks access a37xx pinctrl internal
> > structures.
>
> Yes but probing is different from binding, please see [1]
>
> >
> > So I'm not sure if there is an issue or not. But for sure a37xx gpio
> > must be probed after a37xx pinctrl is probed because a37xx pinctrl probe
> > function fills internal a37xx pinctrl strucutre used by a37xx gpio probe
> > function.
>
> Yes, children are probed after parents, as in docs:
>
> "All parent devices are probed. It is not possible to activate a
> device unless its predecessors (all the way up to the root device) are
> activated. This means (for example) that an I2C driver will require
> that its bus be activated."
>

Hi Simon,
Finally, catching up with emails.

The issue here is that there is nothing probing the pinctrl driver as no
pinmuxing on that
controller is being done and so GPIO doesn't get probed as well which in
turn is breaking
networking as Methode eDPU board only has SFP ports and TX disable remains
active since
the networking driver cannot toggle the GPIO as it's not registered.

Other than inventing a pinmux node and attaching it so that controller gets
probed I dont see
how to solve it within the current U-boot scope.

Regards,
Robert

>
> >
> > > And don't forget the root cause of this whole problem is Linux-centrix
> > > DT bindings.
>
> Regards,
> Simon
>
> [1]
> https://u-boot.readthedocs.io/en/latest/develop/driver-model/design.html#driver-lifecycle
>


-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email:  <http://goog_1951392730>robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Simon Glass]

Hi Robert,

On Fri, 30 Dec 2022 at 13:26, Robert Marko <robert.marko@sartura.hr> wrote:
>
>
>
> On Fri, Dec 30, 2022 at 8:02 PM Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Pali,
>>
>> On Fri, 30 Dec 2022 at 12:02, Pali Rohár <pali@kernel.org> wrote:
>> >
>> > On Friday 30 December 2022 11:47:29 Simon Glass wrote:
>> > > Hi Pali,
>> > >
>> > > On Fri, 30 Dec 2022 at 10:13, Pali Rohár <pali@kernel.org> wrote:
>> > > >
>> > > > On Friday 30 December 2022 10:00:11 Simon Glass wrote:
>> > > > > Hi Pali,
>> > > > >
>> > > > > On Fri, 30 Dec 2022 at 09:53, Pali Rohár <pali@kernel.org> wrote:
>> > > > > >
>> > > > > > On Friday 30 December 2022 09:30:27 Simon Glass wrote:
>> > > > > > > Hi Pali,
>> > > > > > >
>> > > > > > > On Wed, 21 Dec 2022 at 17:02, Pali Rohár <pali@kernel.org> wrote:
>> > > > > > > >
>> > > > > > > > On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
>> > > > > > > > > This breaks chromebook_coral and it is also not how things should work. If
>> > > > > > > > > a board needs to bind GPIOs as part of a pinctrl driver this can be done
>> > > > > > > > > during the bind step, if needed.
>> > > > > > > > >
>> > > > > > > > > We cannot probe pinctrl devices when binding as a rule, since it cannot be
>> > > > > > > > > supported on some platforms.
>> > > > > > > > >
>> > > > > > > > > The bind and probe steps are separate in U-Boot and they should remain
>> > > > > > > > > separate.
>> > > > > > > > >
>> > > > > > > > > This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.
>> > > > > > > >
>> > > > > > > > Unfortunately reverting this patch would break other devices, mostly
>> > > > > > > > A3720 based where pinctrl driver acts also as gpio driver. Because no
>> > > > > > > > other caller then register gpio driver and so other drivers which parses
>> > > > > > > > gpios from DT (which belongs to that gpio driver) will fail during
>> > > > > > > > probe.
>> > > > > > >
>> > > > > > > That is something to be sorted out for that platform. You can bind
>> > > > > > > GPIO devices in the pinctrl driver's bind() method as other SoCs do.
>> > > > > > > Even better, the device tree typically has that info in it, i.e. GPIO
>> > > > > > > subnodes within the pinctrl node.
>> > > > > > >
>> > > > > > > Probing pinctrl in a bind function is unfortunately just wrong. It
>> > > > > > > will cause all sorts of problems, and perhaps already has.
>> > > > > >
>> > > > > > Ok, so it means that drivers/pinctrl/mvebu/pinctrl-armada-37xx.c needs
>> > > > > > to be refactored and fixed to handle these restrictions.
>> > > > >
>> > > > > It is not a restriction. It is simply that binding and probing are
>> > > > > different things and we should not tie them together. It will just
>> > > > > become a nightmare for board bringup and other drivers.
>> > > > >
>> > > > > >
>> > > > > > > > Also I think that pinctrl command would not work in this case if pinctrl
>> > > > > > > > would not be probed.
>> > > > > > >
>> > > > > > > Devices are probed before use, including by commands.
>> > > > > > >
>> > > > > > > This is quite important to fix before the release.
>> > > > > >
>> > > > > > Unfortunately in this time I do not have any a3720 board for testing.
>> > > > > > Robert was able to easily trigger this issue and also introduced that
>> > > > > > commit f9ec791b5e24 ("pinctrl: probe pinctrl drivers during post-bind").
>> > > > > >
>> > > > > > So may I ask Robert to look at it? In past days I looked at powerpc
>> > > > > > release issues and I do not have time for other things.
>> > > > >
>> > > > > That driver has a few FIXMEs in it and could use a look anyway. I see
>> > > > > that the gpio controller is a subnode of pinctrl anyway. Adding a
>> > > > > compatible string for it would fix the problem  just like that, and
>> > > > > remove a lot of ugly code in the driver. This Linux-centric nature of
>> > > > > device tree bindings really is infuriating:
>> > > >
>> > > > All a3720 DTS files are 1:1 copied from Linux. So if problem is in DTS
>> > > > file then it should be discussed with Linux dt/mvebu maintainers. This
>> > > > year I fixed U-Boot code to handle Linux a3720 DTS files and then copied
>> > > > DTS files from Linux. So it is not a good idea to have again different
>> > > > DTS file for u-boot and different for kernel.
>>
>> That was not my suggestion. I would simply like the bindings in Linux
>> to be more explicit, rather than having driver code manually written
>> to do what the device tree is supposed to do.
>>
>> > > >
>> > > > > /* FIXME: Use livtree and check the result of device_bind() below */
>> > > > > fdt_for_each_subnode(subnode, blob, node) {
>> > > > >     if (fdtdec_get_bool(blob, subnode, "gpio-controller")) {
>> > > > >         ret = 0;
>> > > > >         break;
>> > > > >     }
>> > > > > };
>> > > > > if (ret)
>> > > > >    return ret;
>> > > > >
>> > > > >
>> > > > > Failing that it just needs a bind() method that calls
>> > > > > armada_37xx_gpiochip_register()
>> > > > >
>> > > > > Regards,
>> > > > > Simon
>> > > >
>> > > > Seems that it is not such simple. armada_37xx_gpiochip_register()
>> > > > depends on initialized and bound pinctrl part of driver. So before
>> > > > binding gpio you need to bind pinctrl as they share internal structures.
>> > >
>> > > Where are you seeing that? From what I can tell it just binds the GPIO
>> > > driver. It doesn't probe it. So long as you bind the GPIO driver in
>> > > armada_37xx_pinctrl_bind() it should be equivalent.
>> >
>> > armada_37xx_gpiochip_register() calls device_bind() for
>> > &armada_37xx_gpio_driver which probe method armada_37xx_gpio_probe() and
>> > ops &armada_37xx_gpio_ops callbacks access a37xx pinctrl internal
>> > structures.
>>
>> Yes but probing is different from binding, please see [1]
>>
>> >
>> > So I'm not sure if there is an issue or not. But for sure a37xx gpio
>> > must be probed after a37xx pinctrl is probed because a37xx pinctrl probe
>> > function fills internal a37xx pinctrl strucutre used by a37xx gpio probe
>> > function.
>>
>> Yes, children are probed after parents, as in docs:
>>
>> "All parent devices are probed. It is not possible to activate a
>> device unless its predecessors (all the way up to the root device) are
>> activated. This means (for example) that an I2C driver will require
>> that its bus be activated."
>
>
> Hi Simon,
> Finally, catching up with emails.
>
> The issue here is that there is nothing probing the pinctrl driver as no pinmuxing on that
> controller is being done and so GPIO doesn't get probed as well which in turn is breaking
> networking as Methode eDPU board only has SFP ports and TX disable remains active since
> the networking driver cannot toggle the GPIO as it's not registered.
>
> Other than inventing a pinmux node and attaching it so that controller gets probed I dont see
> how to solve it within the current U-boot scope.

Did you see my comments above? You only need to change it to do things
in the pinctrl bind() method, instead of the probe() method. It should
be pretty easy...let me know if you get stuck.

Regards,
Simon

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Simon Glass]

Hi,

On Tue, 3 Jan 2023 at 10:05, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Robert,
>
> On Fri, 30 Dec 2022 at 13:26, Robert Marko <robert.marko@sartura.hr> wrote:
> >
> >
> >
> > On Fri, Dec 30, 2022 at 8:02 PM Simon Glass <sjg@chromium.org> wrote:
> >>
> >> Hi Pali,
> >>
> >> On Fri, 30 Dec 2022 at 12:02, Pali Rohár <pali@kernel.org> wrote:
> >> >
> >> > On Friday 30 December 2022 11:47:29 Simon Glass wrote:
> >> > > Hi Pali,
> >> > >
> >> > > On Fri, 30 Dec 2022 at 10:13, Pali Rohár <pali@kernel.org> wrote:
> >> > > >
> >> > > > On Friday 30 December 2022 10:00:11 Simon Glass wrote:
> >> > > > > Hi Pali,
> >> > > > >
> >> > > > > On Fri, 30 Dec 2022 at 09:53, Pali Rohár <pali@kernel.org> wrote:
> >> > > > > >
> >> > > > > > On Friday 30 December 2022 09:30:27 Simon Glass wrote:
> >> > > > > > > Hi Pali,
> >> > > > > > >
> >> > > > > > > On Wed, 21 Dec 2022 at 17:02, Pali Rohár <pali@kernel.org> wrote:
> >> > > > > > > >
> >> > > > > > > > On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
> >> > > > > > > > > This breaks chromebook_coral and it is also not how things should work. If
> >> > > > > > > > > a board needs to bind GPIOs as part of a pinctrl driver this can be done
> >> > > > > > > > > during the bind step, if needed.
> >> > > > > > > > >
> >> > > > > > > > > We cannot probe pinctrl devices when binding as a rule, since it cannot be
> >> > > > > > > > > supported on some platforms.
> >> > > > > > > > >
> >> > > > > > > > > The bind and probe steps are separate in U-Boot and they should remain
> >> > > > > > > > > separate.
> >> > > > > > > > >
> >> > > > > > > > > This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.
> >> > > > > > > >
> >> > > > > > > > Unfortunately reverting this patch would break other devices, mostly
> >> > > > > > > > A3720 based where pinctrl driver acts also as gpio driver. Because no
> >> > > > > > > > other caller then register gpio driver and so other drivers which parses
> >> > > > > > > > gpios from DT (which belongs to that gpio driver) will fail during
> >> > > > > > > > probe.
> >> > > > > > >
> >> > > > > > > That is something to be sorted out for that platform. You can bind
> >> > > > > > > GPIO devices in the pinctrl driver's bind() method as other SoCs do.
> >> > > > > > > Even better, the device tree typically has that info in it, i.e. GPIO
> >> > > > > > > subnodes within the pinctrl node.
> >> > > > > > >
> >> > > > > > > Probing pinctrl in a bind function is unfortunately just wrong. It
> >> > > > > > > will cause all sorts of problems, and perhaps already has.
> >> > > > > >
> >> > > > > > Ok, so it means that drivers/pinctrl/mvebu/pinctrl-armada-37xx.c needs
> >> > > > > > to be refactored and fixed to handle these restrictions.
> >> > > > >
> >> > > > > It is not a restriction. It is simply that binding and probing are
> >> > > > > different things and we should not tie them together. It will just
> >> > > > > become a nightmare for board bringup and other drivers.
> >> > > > >
> >> > > > > >
> >> > > > > > > > Also I think that pinctrl command would not work in this case if pinctrl
> >> > > > > > > > would not be probed.
> >> > > > > > >
> >> > > > > > > Devices are probed before use, including by commands.
> >> > > > > > >
> >> > > > > > > This is quite important to fix before the release.
> >> > > > > >
> >> > > > > > Unfortunately in this time I do not have any a3720 board for testing.
> >> > > > > > Robert was able to easily trigger this issue and also introduced that
> >> > > > > > commit f9ec791b5e24 ("pinctrl: probe pinctrl drivers during post-bind").
> >> > > > > >
> >> > > > > > So may I ask Robert to look at it? In past days I looked at powerpc
> >> > > > > > release issues and I do not have time for other things.
> >> > > > >
> >> > > > > That driver has a few FIXMEs in it and could use a look anyway. I see
> >> > > > > that the gpio controller is a subnode of pinctrl anyway. Adding a
> >> > > > > compatible string for it would fix the problem  just like that, and
> >> > > > > remove a lot of ugly code in the driver. This Linux-centric nature of
> >> > > > > device tree bindings really is infuriating:
> >> > > >
> >> > > > All a3720 DTS files are 1:1 copied from Linux. So if problem is in DTS
> >> > > > file then it should be discussed with Linux dt/mvebu maintainers. This
> >> > > > year I fixed U-Boot code to handle Linux a3720 DTS files and then copied
> >> > > > DTS files from Linux. So it is not a good idea to have again different
> >> > > > DTS file for u-boot and different for kernel.
> >>
> >> That was not my suggestion. I would simply like the bindings in Linux
> >> to be more explicit, rather than having driver code manually written
> >> to do what the device tree is supposed to do.
> >>
> >> > > >
> >> > > > > /* FIXME: Use livtree and check the result of device_bind() below */
> >> > > > > fdt_for_each_subnode(subnode, blob, node) {
> >> > > > >     if (fdtdec_get_bool(blob, subnode, "gpio-controller")) {
> >> > > > >         ret = 0;
> >> > > > >         break;
> >> > > > >     }
> >> > > > > };
> >> > > > > if (ret)
> >> > > > >    return ret;
> >> > > > >
> >> > > > >
> >> > > > > Failing that it just needs a bind() method that calls
> >> > > > > armada_37xx_gpiochip_register()
> >> > > > >
> >> > > > > Regards,
> >> > > > > Simon
> >> > > >
> >> > > > Seems that it is not such simple. armada_37xx_gpiochip_register()
> >> > > > depends on initialized and bound pinctrl part of driver. So before
> >> > > > binding gpio you need to bind pinctrl as they share internal structures.
> >> > >
> >> > > Where are you seeing that? From what I can tell it just binds the GPIO
> >> > > driver. It doesn't probe it. So long as you bind the GPIO driver in
> >> > > armada_37xx_pinctrl_bind() it should be equivalent.
> >> >
> >> > armada_37xx_gpiochip_register() calls device_bind() for
> >> > &armada_37xx_gpio_driver which probe method armada_37xx_gpio_probe() and
> >> > ops &armada_37xx_gpio_ops callbacks access a37xx pinctrl internal
> >> > structures.
> >>
> >> Yes but probing is different from binding, please see [1]
> >>
> >> >
> >> > So I'm not sure if there is an issue or not. But for sure a37xx gpio
> >> > must be probed after a37xx pinctrl is probed because a37xx pinctrl probe
> >> > function fills internal a37xx pinctrl strucutre used by a37xx gpio probe
> >> > function.
> >>
> >> Yes, children are probed after parents, as in docs:
> >>
> >> "All parent devices are probed. It is not possible to activate a
> >> device unless its predecessors (all the way up to the root device) are
> >> activated. This means (for example) that an I2C driver will require
> >> that its bus be activated."
> >
> >
> > Hi Simon,
> > Finally, catching up with emails.
> >
> > The issue here is that there is nothing probing the pinctrl driver as no pinmuxing on that
> > controller is being done and so GPIO doesn't get probed as well which in turn is breaking
> > networking as Methode eDPU board only has SFP ports and TX disable remains active since
> > the networking driver cannot toggle the GPIO as it's not registered.
> >
> > Other than inventing a pinmux node and attaching it so that controller gets probed I dont see
> > how to solve it within the current U-boot scope.
>
> Did you see my comments above? You only need to change it to do things
> in the pinctrl bind() method, instead of the probe() method. It should
> be pretty easy...let me know if you get stuck.
>

Applied to u-boot/dm

Please can you look at this before the next release?

Regards,
Simon

Re: [PATCH] dm: pinctrl: Revert "pinctrl: probe pinctrl drivers during post-bind"

[Robert Marko]

On Fri, Jan 13, 2023 at 12:43 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Tue, 3 Jan 2023 at 10:05, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Robert,
> >
> > On Fri, 30 Dec 2022 at 13:26, Robert Marko <robert.marko@sartura.hr> wrote:
> > >
> > >
> > >
> > > On Fri, Dec 30, 2022 at 8:02 PM Simon Glass <sjg@chromium.org> wrote:
> > >>
> > >> Hi Pali,
> > >>
> > >> On Fri, 30 Dec 2022 at 12:02, Pali Rohár <pali@kernel.org> wrote:
> > >> >
> > >> > On Friday 30 December 2022 11:47:29 Simon Glass wrote:
> > >> > > Hi Pali,
> > >> > >
> > >> > > On Fri, 30 Dec 2022 at 10:13, Pali Rohár <pali@kernel.org> wrote:
> > >> > > >
> > >> > > > On Friday 30 December 2022 10:00:11 Simon Glass wrote:
> > >> > > > > Hi Pali,
> > >> > > > >
> > >> > > > > On Fri, 30 Dec 2022 at 09:53, Pali Rohár <pali@kernel.org> wrote:
> > >> > > > > >
> > >> > > > > > On Friday 30 December 2022 09:30:27 Simon Glass wrote:
> > >> > > > > > > Hi Pali,
> > >> > > > > > >
> > >> > > > > > > On Wed, 21 Dec 2022 at 17:02, Pali Rohár <pali@kernel.org> wrote:
> > >> > > > > > > >
> > >> > > > > > > > On Wednesday 21 December 2022 07:27:39 Simon Glass wrote:
> > >> > > > > > > > > This breaks chromebook_coral and it is also not how things should work. If
> > >> > > > > > > > > a board needs to bind GPIOs as part of a pinctrl driver this can be done
> > >> > > > > > > > > during the bind step, if needed.
> > >> > > > > > > > >
> > >> > > > > > > > > We cannot probe pinctrl devices when binding as a rule, since it cannot be
> > >> > > > > > > > > supported on some platforms.
> > >> > > > > > > > >
> > >> > > > > > > > > The bind and probe steps are separate in U-Boot and they should remain
> > >> > > > > > > > > separate.
> > >> > > > > > > > >
> > >> > > > > > > > > This reverts commit f9ec791b5e24378b71590877499f8683d5f54dac.
> > >> > > > > > > >
> > >> > > > > > > > Unfortunately reverting this patch would break other devices, mostly
> > >> > > > > > > > A3720 based where pinctrl driver acts also as gpio driver. Because no
> > >> > > > > > > > other caller then register gpio driver and so other drivers which parses
> > >> > > > > > > > gpios from DT (which belongs to that gpio driver) will fail during
> > >> > > > > > > > probe.
> > >> > > > > > >
> > >> > > > > > > That is something to be sorted out for that platform. You can bind
> > >> > > > > > > GPIO devices in the pinctrl driver's bind() method as other SoCs do.
> > >> > > > > > > Even better, the device tree typically has that info in it, i.e. GPIO
> > >> > > > > > > subnodes within the pinctrl node.
> > >> > > > > > >
> > >> > > > > > > Probing pinctrl in a bind function is unfortunately just wrong. It
> > >> > > > > > > will cause all sorts of problems, and perhaps already has.
> > >> > > > > >
> > >> > > > > > Ok, so it means that drivers/pinctrl/mvebu/pinctrl-armada-37xx.c needs
> > >> > > > > > to be refactored and fixed to handle these restrictions.
> > >> > > > >
> > >> > > > > It is not a restriction. It is simply that binding and probing are
> > >> > > > > different things and we should not tie them together. It will just
> > >> > > > > become a nightmare for board bringup and other drivers.
> > >> > > > >
> > >> > > > > >
> > >> > > > > > > > Also I think that pinctrl command would not work in this case if pinctrl
> > >> > > > > > > > would not be probed.
> > >> > > > > > >
> > >> > > > > > > Devices are probed before use, including by commands.
> > >> > > > > > >
> > >> > > > > > > This is quite important to fix before the release.
> > >> > > > > >
> > >> > > > > > Unfortunately in this time I do not have any a3720 board for testing.
> > >> > > > > > Robert was able to easily trigger this issue and also introduced that
> > >> > > > > > commit f9ec791b5e24 ("pinctrl: probe pinctrl drivers during post-bind").
> > >> > > > > >
> > >> > > > > > So may I ask Robert to look at it? In past days I looked at powerpc
> > >> > > > > > release issues and I do not have time for other things.
> > >> > > > >
> > >> > > > > That driver has a few FIXMEs in it and could use a look anyway. I see
> > >> > > > > that the gpio controller is a subnode of pinctrl anyway. Adding a
> > >> > > > > compatible string for it would fix the problem  just like that, and
> > >> > > > > remove a lot of ugly code in the driver. This Linux-centric nature of
> > >> > > > > device tree bindings really is infuriating:
> > >> > > >
> > >> > > > All a3720 DTS files are 1:1 copied from Linux. So if problem is in DTS
> > >> > > > file then it should be discussed with Linux dt/mvebu maintainers. This
> > >> > > > year I fixed U-Boot code to handle Linux a3720 DTS files and then copied
> > >> > > > DTS files from Linux. So it is not a good idea to have again different
> > >> > > > DTS file for u-boot and different for kernel.
> > >>
> > >> That was not my suggestion. I would simply like the bindings in Linux
> > >> to be more explicit, rather than having driver code manually written
> > >> to do what the device tree is supposed to do.
> > >>
> > >> > > >
> > >> > > > > /* FIXME: Use livtree and check the result of device_bind() below */
> > >> > > > > fdt_for_each_subnode(subnode, blob, node) {
> > >> > > > >     if (fdtdec_get_bool(blob, subnode, "gpio-controller")) {
> > >> > > > >         ret = 0;
> > >> > > > >         break;
> > >> > > > >     }
> > >> > > > > };
> > >> > > > > if (ret)
> > >> > > > >    return ret;
> > >> > > > >
> > >> > > > >
> > >> > > > > Failing that it just needs a bind() method that calls
> > >> > > > > armada_37xx_gpiochip_register()
> > >> > > > >
> > >> > > > > Regards,
> > >> > > > > Simon
> > >> > > >
> > >> > > > Seems that it is not such simple. armada_37xx_gpiochip_register()
> > >> > > > depends on initialized and bound pinctrl part of driver. So before
> > >> > > > binding gpio you need to bind pinctrl as they share internal structures.
> > >> > >
> > >> > > Where are you seeing that? From what I can tell it just binds the GPIO
> > >> > > driver. It doesn't probe it. So long as you bind the GPIO driver in
> > >> > > armada_37xx_pinctrl_bind() it should be equivalent.
> > >> >
> > >> > armada_37xx_gpiochip_register() calls device_bind() for
> > >> > &armada_37xx_gpio_driver which probe method armada_37xx_gpio_probe() and
> > >> > ops &armada_37xx_gpio_ops callbacks access a37xx pinctrl internal
> > >> > structures.
> > >>
> > >> Yes but probing is different from binding, please see [1]
> > >>
> > >> >
> > >> > So I'm not sure if there is an issue or not. But for sure a37xx gpio
> > >> > must be probed after a37xx pinctrl is probed because a37xx pinctrl probe
> > >> > function fills internal a37xx pinctrl strucutre used by a37xx gpio probe
> > >> > function.
> > >>
> > >> Yes, children are probed after parents, as in docs:
> > >>
> > >> "All parent devices are probed. It is not possible to activate a
> > >> device unless its predecessors (all the way up to the root device) are
> > >> activated. This means (for example) that an I2C driver will require
> > >> that its bus be activated."
> > >
> > >
> > > Hi Simon,
> > > Finally, catching up with emails.
> > >
> > > The issue here is that there is nothing probing the pinctrl driver as no pinmuxing on that
> > > controller is being done and so GPIO doesn't get probed as well which in turn is breaking
> > > networking as Methode eDPU board only has SFP ports and TX disable remains active since
> > > the networking driver cannot toggle the GPIO as it's not registered.
> > >
> > > Other than inventing a pinmux node and attaching it so that controller gets probed I dont see
> > > how to solve it within the current U-boot scope.
> >
> > Did you see my comments above? You only need to change it to do things
> > in the pinctrl bind() method, instead of the probe() method. It should
> > be pretty easy...let me know if you get stuck.
> >
>
> Applied to u-boot/dm
>
> Please can you look at this before the next release?

Hi, Simon.

I will take a look this week to get it fixed.

Regards,
Robert
>
> Regards,
> Simon



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr