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