Re: pinctrl, probe order, and CONFIG_MODULES

[Phil Elwell <phil@raspberrypi.com>]

Hi Linus, Rob,

On Fri, 29 Aug 2025 at 23:43, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Aug 29, 2025 at 5:51 PM Phil Elwell <phil@raspberrypi.com> wrote:
> > On Fri, 29 Aug 2025 at 16:01, Rob Herring <robh@kernel.org> wrote:
> > > On Fri, Aug 29, 2025 at 9:52 AM Phil Elwell <phil@raspberrypi.com> wrote:
> > > > On Fri, 29 Aug 2025 at 15:44, Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > On Fri, Aug 29, 2025 at 8:35 AM Phil Elwell <phil@raspberrypi.com> wrote:
> > > > > >
> > > > > > Hi Rob,
> > > > > >
> > > > > > On Fri, 29 Aug 2025 at 14:25, Rob Herring <robh@kernel.org> wrote:
> > > > > > >
> > > > > > > On Fri, Aug 29, 2025 at 5:37 AM Phil Elwell <phil@raspberrypi.com> wrote:
> > > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > A Raspberry Pi user recently asked us why I2C wasn't working in their
> > > > > > > > custom kernel build. The primary change they had made was to trim the
> > > > > > > > number of enabled drivers, make them all built-in, and to remove
> > > > > > > > CONFIG_MODULES=y. The end result of this is that the pin muxing
> > > > > > > > required for I2C to work was not being applied, leaving the interface
> > > > > > > > talking to thin air.
> > > > > > > >
> > > > > > > > I eventually traced the failure back to this change:
> > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/pinctrl/devicetree.c?h=next-20250829&id=d19c5e79d46efdf89306be99f3c8824cf58e35f6
> > > > > > > > It introduces a requirement for CONFIG_MODULES to be enabled in order
> > > > > > > > to get an EPROBE_DEFER in the event that the pinctrl driver has not
> > > > > > > > yet been probed. Without CONFIG_MODULES, the pinctrl requirements are
> > > > > > > > waived. Removing the IS_ENABLED(CONFIG_MODULES) clause allows the
> > > > > > > > probing of the I2C driver to be deferred, fixing the user's problem.
> > > > > > > >
> > > > > > > > Is this requirement for supporting modules reasonable?
> > > > > > >
> > > > > > > That's not the requirement. If CONFIG_MODULES=n, then we only defer
> > > > > > > probes until late_initcall because after that point no more drivers
> > > > > > > are going to load. If CONFIG_MODULES=y, then deferring probe is based
> > > > > > > on a timeout which can be disabled.
> > > > > >
> > > > > > Thanks for replying. I'm probably missing something here, but if the
> > > > > > pinctrl and I2C drivers are both regular platform drivers, what is to
> > > > > > stop the I2C driver being probed first?
> > > > >
> > > > > Nothing, but it should defer unless you've reached late initcall or
> > > > > you've set "pinctrl-use-default".
> > > >
> > > > From the "next" tree:
> > > >
> > > > if (!np_pctldev || of_node_is_root(np_pctldev)) {
> > > >         of_node_put(np_pctldev);
> > > >         ret = -ENODEV;
> > > >         /* keep deferring if modules are enabled */
> > > >         if (IS_ENABLED(CONFIG_MODULES) && !allow_default && ret < 0)
> > > >                 ret = -EPROBE_DEFER;
> > > >         return ret;
> > > > }
> > > >
> > > > Unless CONFIG_MODULES=y you get ENODEV.
> > >
> > > Indeed, as 'ret = driver_deferred_probe_check_state(p->dev);' is gone due to:
> > >
> > > commit 24a026f85241a01bbcfe1b263caeeaa9a79bab40
> > > Author: Saravana Kannan <saravanak@google.com>
> > > Date:   Wed Jun 1 00:06:58 2022 -0700
> > >
> > >     pinctrl: devicetree: Delete usage of driver_deferred_probe_check_state()
> > >
> > >     Now that fw_devlink=on by default and fw_devlink supports
> > >     "pinctrl-[0-8]" property, the execution will never get to the point
> > >     where driver_deferred_probe_check_state() is called before the supplier
> > >     has probed successfully or before deferred probe timeout has expired.
> > >
> > >     So, delete the call and replace it with -ENODEV.
> > >
> > >     Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >     Signed-off-by: Saravana Kannan <saravanak@google.com>
> > >     Link: https://lore.kernel.org/r/20220601070707.3946847-3-saravanak@google.com
> > >     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >
> > >
> > > So fw_devlink will ensure that pinctrl probes first. Is that turned
> > > off by chance (kernel cmdline is the only way to disable).
> >
> > No, I don't think it's turned off. After a bit more digging I'm
> > starting to think that of_fwnode_add_links doesn't work in the pinctrl
> > case because the pinctrl-n properties are links to children of the
> > pinctrl controller, not the controller itself. Perhaps they're not
> > simple enough to be declared with DEFINE_SIMPLE_PROP and need a custom
> > parser that returns the parent instead?
>
> Have you tried setting this property in the pin controller?
>
> $ git grep link_consumers drivers/pinctrl/
> drivers/pinctrl/core.c: if (pctldev->desc->link_consumers)
> drivers/pinctrl/pinctrl-stmfx.c:        pctl->pctl_desc.link_consumers = true;
> drivers/pinctrl/stm32/pinctrl-stm32.c:  pctl->pctl_desc.link_consumers = true;
>
> Maybe it should be the default.
>
> 5+ years ago I patched something like this but thought it was
> too invasive:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/commit/?h=consumer-link-enforce&id=73441cf773ed91bff0e7f66614d391b2514188bf
>
> I don't know if it has something to do with it?

Using the information you've provided, further investigation uncovered
that the conversion of the supplier/consumer relationships from fwlink
to device link was rejecting many of the dependencies because an
ancestor was marked as initialised but with no device pointer. The
ancestor in this case is the RP1 I/O chip used on Raspberry Pi 5 - a
PCIe device whose content is described in Device Tree. Ensuring that
the device pointer is initialised allows the device link feature to
work as expected. It remains to be seen if the upstream RP1 driver
will need a similar patch for the no-modules corner case.

Thanks for your help,

Phil