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.