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

[Saravana Kannan]

On Wed, Sep 22, 2021 at 9:31 AM Philipp Zabel <p.zabel at pengutronix.de> wrote:
>
> On Wed, 2021-09-22 at 08:38 -0700, Saravana Kannan wrote:
> [...]
> > 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.
>
> Oh, I wasn't aware. Yes,
> /sys/devices/platform/soc/2000000.bus/20d8000.reset-controller
> does exist.
>
> > > 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?
>
> There are a few functions exported from src.c (imx_enable_cpu,
> imx_set_cpu_arg/jump) that are used in hotplug.c and platsmp.c.
>
> > > > 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'll send a patch adding the platform driver, but could you elaborate on
> what could be done with the auto-generated reset-controller device
> instead?

Your platform driver would basically match with this auto generated
platform device. You would just change arch/arm/mach-imx/src.c to add
platform_driver and the probe() would pretty much just do something
like:

... probe(...)
{
        WARN_ON(!src_base);

        imx_reset_controller.of_node = pdev->dev.of_node;
        return reset_controller_register(&imx_reset_controller);
}

I don't think you need the IS_ENABLED() check since
reset_controller_register() is already stubbed out based on that.
You'll also leave the rest of the code in imx_src_int() since you'll
need those for your CPU reset to work.

Another option you could do instead of writing a platform driver is
something like this:
https://lore.kernel.org/lkml/20210205222644.2357303-8-saravanak@google.com/

But the benefit of writing a platform driver is that you'll also get
proper suspend/resume ordering (it might not matter in your case)
between the reset controller and it's non-CPU consumers.

I'm okay with you picking either option if you don't have any
suspend/resume ordering requirements between src vs its consumers, but
I think one of these options is better than the current patch that
just deletes the fwnode links.

-Saravana