Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default

[Saravana Kannan <saravanak@google.com>]

On Thu, Jan 21, 2021 at 8:04 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Wed, Jan 20, 2021 at 6:23 PM Saravana Kannan <saravanak@google.com> wrote:
> > On Wed, Jan 20, 2021 at 6:27 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, Jan 20, 2021 at 10:40 AM Geert Uytterhoeven
> > > <geert@linux-m68k.org> wrote:
> > > > On Tue, Jan 19, 2021 at 10:51 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > > On Tue, Jan 19, 2021 at 10:08 AM Saravana Kannan <saravanak@google.com> wrote:
> > > > > > On Tue, Jan 19, 2021 at 1:05 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > On Mon, Jan 18, 2021 at 10:19 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > > > > > On Mon, Jan 18, 2021 at 11:16 AM Geert Uytterhoeven
> > > > > > > > <geert@linux-m68k.org> wrote:
> > > > > > > > > On Mon, Jan 18, 2021 at 6:59 PM Marc Zyngier <maz@kernel.org> wrote:
> > > > > > > > > > On 2021-01-18 17:39, Geert Uytterhoeven wrote:
> > > > > > > > > > > On Fri, Dec 18, 2020 at 4:34 AM Saravana Kannan <saravanak@google.com>
> > > > > > > > > > > wrote:
> > > > > > > > > > >> Cyclic dependencies in some firmware was one of the last remaining
> > > > > > > > > > >> reasons fw_devlink=on couldn't be set by default. Now that cyclic
> > > > > > > > > > >> dependencies don't block probing, set fw_devlink=on by default.
> > > > > > > > > > >>
> > > > > > > > > > >> Setting fw_devlink=on by default brings a bunch of benefits
> > > > > > > > > > >> (currently,
> > > > > > > > > > >> only for systems with device tree firmware):
> > > > > > > > > > >> * Significantly cuts down deferred probes.
> > > > > > > > > > >> * Device probe is effectively attempted in graph order.
> > > > > > > > > > >> * Makes it much easier to load drivers as modules without having to
> > > > > > > > > > >>   worry about functional dependencies between modules (depmod is still
> > > > > > > > > > >>   needed for symbol dependencies).
> > > > > > > > > > >>
> > > > > > > > > > >> If this patch prevents some devices from probing, it's very likely due
> > > > > > > > > > >> to the system having one or more device drivers that "probe"/set up a
> > > > > > > > > > >> device (DT node with compatible property) without creating a struct
> > > > > > > > > > >> device for it.  If we hit such cases, the device drivers need to be
> > > > > > > > > > >> fixed so that they populate struct devices and probe them like normal
> > > > > > > > > > >> device drivers so that the driver core is aware of the devices and
> > > > > > > > > > >> their
> > > > > > > > > > >> status. See [1] for an example of such a case.
> > > > > > > > > > >>
> > > > > > > > > > >> [1] -
> > > > > > > > > > >> https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com/
> > > > > > > > > > >> Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > > > > > > >
> > > > > > > > > > > Shimoda-san reported that next-20210111 and later fail to boot
> > > > > > > > > > > on Renesas R-Car Gen3 platforms. No output is seen, unless earlycon
> > > > > > > > > > > is enabled.
> > > > > > > > > > >
> > > > > > > > > > > I have bisected this to commit e590474768f1cc04 ("driver core: Set
> > > > > > > > > > > fw_devlink=on by default").
>
> > > > > You'll need to convert drivers/soc/renesas/rcar-sysc.c into a platform
> > > > > driver. You already have a platform device created for it. So just go
> > > > > ahead and probe it with a platform driver. See what Marek did here
> > > > > [1].
> > > > >
> > > > > You probably had to implement it as an "initcall based driver"
> > > > > because you had to play initcall chicken to make sure the PD hardware
> > > > > was initialized before the consumers. With fw_devlink=on you won't
> > > > > have to worry about that. As an added benefit of implementing a proper
> > > > > platform driver, you can  actually implement runtime PM now, your
> > > > > suspend/resume would be more robust, etc.
> > > >
> > > > On R-Car H1, the system controller driver needs to be active before
> > > > secondary CPU setup, hence the early_initcall().
> > > > platform_bus_init() is called after that, so this is gonna need a split
> > > > initialization.  Or a dummy platform driver to make devlinks think
> > > > everything is fine ;-)
> >
> > I was wondering if you could still probe the "not needed by CPU" power
> > domains (if there are any) as devices. Using driver-core brings you
> > good things :)
>
>  1. That would mean splitting the driver in two parts, looping over the
>     tables twice, while everything can just be done in the first pass?
>
>  2. Which "good things" do you have in mind? Making the driver modular?
>     Ignoring the dependency for secondary CPU setup on R-Car H1, this
>     driver could indeed be modular on R-Car Gen2 and Gen3, as long as
>     the boot loader would pass a ramdisk with the module to the kernel.
>     The ramdisk could not be loaded in any other way, as all I/O
>     devices are part of a PM Domain, and thus depend on the SYSC driver.
>     Note that on some (non-R-Car) SoCs, the timers may be part of a PM
>     Domain, too.

"Good things" like being able to implement runtime pm, suspend/resume
robustness (due to device links). There were a few more benefits I had
in mind when I wrote it, but I don't remember what it was.

The double pass itself is not that big of a deal IMHO. It probably
adds less than a millisecond.

>
> > > > So basically all producer DT drivers not using a platform (or e.g. i2c)
> > > > driver are now broken?
> > > > Including all clock drivers using CLK_OF_DECLARE()?
> > >
> > > Oh, of_link_to_phandle() ignores device nodes where OF_POPULATED
> > > is set, and of_clk_init() sets that flag.  So rcar-sysc should do so, too.
> > > Patch sent.
> > > >     $ git grep -L "\<[a-z0-9]*_driver\>" -- $(git grep -l
> > > > "\.compatible\>") | wc -l
> > > >     249
> > > >
> > > > (includes false positives)
> > > >
> > > > I doubt they'll all get fixed for v5.12, as we're already at rc4...
> > >
> > > Still more than 100 drivers to fix?
> >
> > Not fully sure what the grep is trying to catch, but fw_devlink
> > supports devices on any bus (i2c, platform, pci, etc). So that's not a
> > problem. It'll be a problem when a struct device is never created for
> > a real device. Or if it's created, but never probed.
>
> The grep tries to catch drivers using DT matching (i.e. matching ".compatible")
> and not using a driver model driver (i.e. not matching "*_driver").

Ah TIL about -L and -l. Thanks.

> > I'm also looking into a bunch of other options for fallback when
> > fw_devlink=on doesn't work. Too much to explain here -- patches are
> > easier :)
>
> I gave it a try on all Renesas platforms I have local access to:

Thanks a lot! Really appreciate the testing and reporting.

>
>   - R-Car Gen2/Gen3:
>     Setting OF_POPULATED in the rcar-sysc driver[1] made my standard
>     config boot again.  Remaining issues:
>       - CONFIG_IPMMU_VMSA=n hangs: supplier fe990000.iommu not ready
>       - CONFIG_RCAR_DMAC=n hangs: supplier e7310000.dma-controller not ready
>         Note that Ethernet does not use the R-Car DMAC, so DHCP works.
>         Nevertheless, after that everything hangs, and the board does not
>         respond to pings anymore
>     Both IOMMU and DMAC dependencies are optional, hence should be dropped
>     at late boot (late_initcall?).

Yeah, I'm looking into a good/clean way of handling optional
suppliers. There are a bunch of corner cases I need to consider. But
in the end, I need to have it behave as closely as possible to
fw_devlink=permissive.

>
>   - SH-Mobile AG5 and R-Mobile APE6:
>     The rmobile-sysc driver is similar to the rcar-sysc driver, and does
>     not use a platform device.
>     Still, it works, because all dependencies on the System Controller
>     become unblocked when the rmobile-reset driver binds against the
>     "renesas,sysc-rmobile" device.  Obviously it would fail if no
>     support for that driver is included in your kernel...

Yeah, IMHO two real drivers (not stubs) for a single device tree node
is wrong/weird at a high level. I'd think one should be a child of the
other. But too late to fix that DT now.

Does it make sense for the rmobile-sysc driver to create a new
platform device and have the rmobule-reset bind to that instead? And
then you can bind a stub driver to the "renesas,sysc-rmobile" device?
I know this can be handled by whatever solution I come up with for the
IOMMU case, but that doesn't seem right for this case. We don't have
to decide on this now, but that's my current view.

>   - R-Mobile A1:
>     Also using the rmobile-sysc driver.
>     However, this is a single core Cortex-A9, i.e. it does not have an
>     ARM architectured timer (like R-Mobile APE6) or Cortex-A9 Global
>     Timer (like SH-Mobile AG5).  The timer used (TMU) is located in a PM
>     Domain controlled by the rmobile-sysc driver, and driver
>     initialization is postponed beyond the point where something relies
>     on a working timer, causing a hang.
>
>     Setting OF_POPULATED (like in my fix for the rcar-sysc driver) fixes
>     this, but prevents the rmobile-reset driver from binding against the
>     same device node, so the reset handling will have to be incorporated
>     into the rmobile-sysc driver (and will thus be registered very
>     early).

Or you can do the "create a child device" option I suggested above.

>   - RZ/A1 and RZ/A2:
>     These are not affected, as the timer used (OSTM) is not a platform
>     driver, but uses TIMER_OF_DECLARE().
>     Note that the RZ/A2 clock driver uses split initialization:
>       1. Early (timer) clocks are initialized from CLK_OF_DECLARE_DRIVER,
>       2. Other clocks are initialized by platform_driver_probe() from a
>          subsys_initcall.
>     If the OSTM driver would be a platform_driver, it would block on the
>     block dependency.  Setting the OF_POPULATED flag in the clock driver
>     would not work: while that flag would unblock probing of the timer
>     driver, it would also prevent the second part of the clock driver
>     initialization.

So this looks like it's all working fine, right? Yeah, I already took
into account the *OF*_DECLARE macros when I wrote this and was aware
of the split driver implementations. So hopefully this all works out
fine.

> Now, back to the things I was supposed to work on this week ;-)

Really appreciate all this testing and feedback!

-Saravana