Message-ID: <CAPDyKFpYvinCB3t7aNp02_-=QNwQr+B4wWvtzAAzYi8rTwKkKQ@mail.gmail.com>
Date: Tue, 9 Jul 2024 17:20:46 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Geert Uytterhoeven <geert@...ux-m68k.org>, Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, 
	Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: Saravana Kannan <saravanak@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Jiri Slaby <jirislaby@...nel.org>, "Rafael J . Wysocki" <rafael@...nel.org>, Rob Herring <robh@...nel.org>, 
	Peng Fan <peng.fan@....com>, linux-pm@...r.kernel.org, linux-serial@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Devarsh Thakkar <devarsht@...com>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [PATCH/RFC 0/3] pmdomain: renesas: rmobile-sysc: Remove serial
 console handling

[...]

> > > >>>>
> > > >>>> However, if the serial port's clock or PM Domain is shared with another
> > > >>>> device, and that other device is runtime-suspended before the full
> > > >>>> serial driver has probed, the serial port's clock and/or PM Domain will
> > > >>>> be disabled inadvertently.  Any subsequent serial console output will
> > > >>>> cause a crash or system lock-up.  E.g. on R/SH-Mobile SoCs, the serial
> > > >>>> ports share their PM Domain with several other I/O devices.  After the
> > > >>>> use of pwm (Armadillo-800-EVA) or i2c (KZM-A9-GT) during early boot,
> > > >>>> before the full serial driver takes over, the PM Domain containing the
> > > >>>> early serial port is powered down, causing a lock-up when booted with
> > > >>>> "earlycon".
>
> Let's call this "Case B".
>
> > > >>>
> > > >>> Thanks for the detailed description of the problem! As pointed out in
> > > >>> regards to another similar recent patch [1], this is indeed a generic
> > > >>> problem, not limited to the serial console handling.
> > > >>>
> > > >>> At Linaro Connect a few weeks ago I followed up with Saravana from the
> > > >>> earlier discussions at LPC last fall. We now have a generic solution
> > > >>> for genpd drafted on plain paper, based on fw_devlink and the
> > > >>> ->sync_state() callback. I am currently working on the genpd series,
> > > >>> while Saravana will re-spin the series (can't find the link to the
> > > >>> last version) for the clock framework. Ideally, we want these things
> > > >>> to work in a very similar way.
> > > >>>
> > > >>> That said, allow me to post the series for genpd in a week or two to
> > > >>> see if it can solve your problem too, for the serial console.

I managed to hit the vacation period before I was able to post the
series. I will pick it up this week and hopefully should be able to
post something next week.

> > > >>
> > > >> Both the genpd and the clock solutions will make suppliers depend on all
> > > >> their consumers to be probed, right?
> > > >>
> > > >> I think it is a solution, and should be worked on, but it has the
> > > >> drawback that suppliers that have consumers that will possibly never be
> > > >> probed, will also never be able to turn off unused resources.
> > > >>
> > > >> This was specifically the case with the TI ti-sci pmdomain case I was
> > > >> looking at: the genpd driver (ti_sci_pm_domains.c) provides a lot of
> > > >> genpds for totally unrelated devices, and so if, e.g., you don't have or
> > > >> don't want to load a driver for the GPU, all PDs are affected.
> > > >>
> > > >> Even here the solutions you mention will help: instead of things getting
> > > >> broken because genpds get turned off while they are actually in use, the
> > > >> genpds will be kept enabled, thus fixing the breakage. Unfortunately,
> > > >> they'll be kept enabled forever.
> > > >>
> > > >> I've been ill for quite a while so I haven't had the chance to look at
> > > >> this more, but before that I was hacking around a bit with something I
> > > >> named .partial_sync_state(). .sync_state() gets called when all the
> > > >> consumers have probed, but .partial_sync_state() gets called when _a_
> > > >> consumer has been probed.
> > > >>
> > > >> For the .sync_state() things are easy for the driver, as it knows
> > > >> everything related has been probed, but for .partial_sync_state() the
> > > >> driver needs to track resources internally. .partial_sync_state() will
> > > >> tell the driver that a consumer device has probed, the driver can then
> > > >> find out which specific resources (genpds in my case) that consumer
> > > >> refers to, and then... Well, that's how far I got with my hacks =).
> > > >>
> > > >> So, I don't know if this .partial_sync_state() can even work, but I
> > > >> think we do need something more on top of the .sync_state().
> > > >
> > > > Thanks for the update!
> > > >
> > > > You certainly have a point, but rather than implementing some platform
> > > > specific method, I think we should be able enforce the call to
> > > > ->sync_state(), based upon some condition/timeout - and even if all
> > > > consumers haven't been probed.
> > >
> > > Hmm, I think that was already implemented in some of the serieses out
> > > there (or even in mainline already?), as I remember doing some
> > > experiments with it. I don't like it much, though.
> > >
> > > With a simple timeout, it'll always be just a bit too early for some
> > > user (nfs mount took a bit more time than expected -> board frozen).
> > >
> > > The only condition I can see that would somewhat work is a manual
> > > trigger from the userspace. The boot scripts could then signal the
> > > kernel when all the modules have been loaded and probably a suitable,
> > > platform/use case specific amount of time has passed to allow the
> > > drivers to probe.
> >
> > This is also already supported in mainline.
> >
> > Devices with sync_state() implementations (once Ulf adds it) will have
> > a state_synced file in sysfs. It shows where it has been called yet or
> > not. But you can also echo 1 into it to force the sync_state()
> > callback (only if it hasn't been called already). So, yeah, all
> > methods of handling this are available if you implement the
> > sync_state() callback.
> >
> > By default it's all strict (wait till all consumers probe
> > successfully). But you can set it to timeout (fw_devlink.sync_state).
> > And you also have the option I mentioned above that you can use with
> > both cases.
>
> So the idea is to disable unused genpds and clocks from the genpd
> resp. clock's driver .sync_state() callback, instead of from a late
> initcall?  That would indeed solve issues related to "Case A".
>
> However, how to solve "Case B"? Ignore disabling genpds or clocks
> before .sync_state() callback() has been called?
> That would cause issues for cases where the clock must be disabled,
> cfr.
>     "[PATCH RFC 0/3] Add clk_disable_unprepare_sync()"
>     https://lore.kernel.org/all/20240131160947.96171-1-biju.das.jz@bp.renesas.com/
>     "[PATCH v3 0/3] Add clk_poll_disable_unprepare()"
>     https://lore.kernel.org/linux-renesas-soc/20240318110842.41956-1-biju.das.jz@bp.renesas.com/
>

For genpd, the plan is to check the initial state of the PM domain. It
can be powered-on or powered-off and if it's powered-on, we should not
allow it to be powered-off until after ->sync_state() have been
called.

The similar approach is what Saravanna is trying to implement for
clocks, I think.

In the end, we simply need to try out these approaches to see if they
really work. Although, based on previous discussions (LKML +
F2F-conferences), I think there should be a good chance for us.

[...]

Kind regards
Uffe

Powered by blists - more mailing lists