On Fri, Jan 13, 2023 at 12:43 AM Simon Glass <s...@chromium.org> wrote:
>
> Hi,
>
> On Tue, 3 Jan 2023 at 10:05, Simon Glass <s...@chromium.org> wrote:
> >
> > Hi Robert,
> >
> > On Fri, 30 Dec 2022 at 13:26, Robert Marko <robert.ma...@sartura.hr> wrote:
> > >
> > >
> > >
> > > On Fri, Dec 30, 2022 at 8:02 PM Simon Glass <s...@chromium.org> wrote:
> > >>
> > >> 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."
> > >
> > >
> > > 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.ma...@sartura.hr
Web: www.sartura.hr
