Hi Pali,
On Fri, 30 Dec 2022 at 10:13, Pali Rohár <p...@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 <p...@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 <p...@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
