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

On Wed, Sep 22, 2021 at 9:31 AM Philipp Zabel [off-list ref] wrote:

On Wed, 2021-09-22 at 08:38 -0700, Saravana Kannan wrote:
[...]

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

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/ (local)

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

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