Re: [PATCH] ARM: imx: drop devlinks to reset-controller node

[Saravana Kannan <saravanak@google.com>]

On Wed, Sep 22, 2021 at 7:12 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Saravana,
>
> (Cc: Michael)
>
> On Tue, 2021-09-21 at 09:47 -0700, Saravana Kannan wrote:
> > Hi Philipp,
> >
> > On Tue, Sep 21, 2021 at 8:05 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > Hi Saravana,
> > >
> > > On Tue, 2021-09-21 at 07:45 -0700, Saravana Kannan wrote:
> > > > On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > > Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add
> > > > > support for "resets" and "pwms""), the imx-drm driver fails to load
> > > > > due to forever dormant devlinks to the reset-controller node. This
> > > > > node is never associated with a struct device.
> > > > >
> > > > > Drop those links, the reset controller is always available.
> > > > >
> > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > > ---
> > > > >  arch/arm/mach-imx/src.c | 10 +++++++++-
> > > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c
> > > > > index 95fd1fbb0826..6924ac19a762 100644
> > > > > --- a/arch/arm/mach-imx/src.c
> > > > > +++ b/arch/arm/mach-imx/src.c
> > > > > @@ -178,9 +178,17 @@ void __init imx_src_init(void)
> > > > >         WARN_ON(!src_base);
> > > > >
> > > > >         imx_reset_controller.of_node = np;
> > > > > -       if (IS_ENABLED(CONFIG_RESET_CONTROLLER))
> > > > > +       if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) {
> > > > >                 reset_controller_register(&imx_reset_controller);
> > > > >
> > > > > +               /*
> > > > > +                * The reset-controller node is never associated with a struct
> > > > > +                * device, which trips up fw_devlink=on. As a workaround, drop
> > > > > +                * all devlinks to/from this node.
> > > > > +                */
> > > > > +               fw_devlink_purge_absent_suppliers(&np->fwnode);
> > > > > +       }
> > > > > +
> > > >
> > > > Hey Philipp,
> > > >
> > > > Can you point me to the .dts file that corresponds to this platform? I
> > > > want to take a closer look.
> > >
> > > Thank you, the relevant file is arch/arm/boot/dts/imx6qdl.dtsi, device
> > > nodes labeled as &src and &ipu1. This is for example included by
> > > arch/arm/boot/dts/imx6q-nitrogen6x.dts. There's a second &ipu2 added in
> > > arch/arm/boot/dts/imx6qd.dtsi as well.
> >
> > I took a closer look at the DT. Is there really no struct device for
> > this node?
>
> Yes, this node is handled by platform code in arch/arm/mach-imx/src.c as

Yes, I saw the node is handled in that file. I know it's not currently
a platform driver. I wasn't asking about that. I'm asking if a struct
device is created for the node or not. Creation of the struct device
is done automatically by the OF code and I don't see any code that
prevents the creation of the device.

Can you please run this on your device and that should answer my question"
# find /sys/devices/ -name *reset-controller*

I'm asking this question because there is a cleaner way to fix this
instead of what you are doing if there is actually a struct device
created for the node.

> the reset controller register also contains bits to bring up the CPUs.

Ok, I initially assumed that's why you were doing this, but I didn't
see this &src node (the reset controller) being referenced anywhere
else in DT. So how does that work?

>
> > Based on the DT, it looks like a device would automatically
> > get created for this node. So it looks like it's more of an issue with
> > the device not being probed by a real driver? Why can't you convert
> > the reset controller driver to a proper platform driver instead of
> > this patch? I think that's the right fix.
>
> In this case, I could indeed add a platform driver for the reset
> controller part. I'm not yet sure that this is always the right thing.

Can you dig into that more please? Instead of just deleting the fwnode
links? There are also ways to make sure you don't block early users
(before driver core is up) while still doing proper ordering for later
users (like ipu, etc). I'd like to fix this in the best way possibly
so that fw_devlink can do the best ordering it can.

> I just learned from Michael that there are other cases where fw_devlink
> follows of_graph links from capture devices to connector nodes that have
> a compatible, such as "composite-video-connector" or "svideo-connector",
> but no associated driver. These are just parsed to determine the
> connector type in drivers/media/v4l2-core/v4l2-fwnode.c. Would you
> suggest to create platform devices for these as well?

remote-endpoints always point to each other. So they create dependency
cycles and fw_devlink stops enforcing ordering between them (because
it's a cycle and it has no way to know which one depends on the other
for probing). So for this specific case, you should get away without
writing platform drivers.

But in general, if you have a compatible string, there really aren't
that many good reasons to not use a proper driver for it.

-Saravana

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel