[PATCH 1/2] dm: core: add support for fallback drivers

[Tom Rini]

On Fri, May 03, 2024 at 05:53:11PM +0200, Caleb Connolly wrote:
> Hi all,
> 
> Sorry for the slow follow-up.
> 
> On 11/04/2024 04:37, Sean Anderson wrote:
> > On 4/10/24 15:44, Tom Rini wrote:
> > > On Wed, Apr 10, 2024 at 07:27:17PM +0200, Heinrich Schuchardt wrote:
> > > > 
> > > > 
> > > > Am 10. April 2024 19:06:57 MESZ schrieb Caleb Connolly
> > > > <caleb.connolly at linaro.org>:
> > > > > Introduce support for a uclass to provide a fallback/stub driver which
> > > > > can be used when no device is found for a given node. This might be
> > > > > useful for handling non-essential clock controllers like the RPMh on
> > > > > Qualcomm platforms, or during early bringup to get UART output before a
> > > > > real clock driver has been created.
> > > > > 
> > > > > Signed-off-by: Caleb Connolly <caleb.connolly at linaro.org>
> > > > > ---
> > > > > drivers/core/Kconfig  | 10 ++++++++++
> > > > > drivers/core/uclass.c | 24 +++++++++++++++++++++++-
> > > > > include/dm/uclass.h   |  3 +++
> > > > > 3 files changed, 36 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
> > > > > index 1081d61fcf01..09075b9b7a15 100644
> > > > > --- a/drivers/core/Kconfig
> > > > > +++ b/drivers/core/Kconfig
> > > > > @@ -466,5 +466,15 @@ config BOUNCE_BUFFER
> > > > > 
> > > > >       A second possible use of bounce buffers is their ability to
> > > > >       provide aligned buffers for DMA operations.
> > > > > 
> > > > > +menuconfig FALLBACK_DRIVERS
> > > > 
> > > > Wouldn't it be preferable to mark individual drivers as fallback
> > > > drivers in their declaration?
> > > > 
> > > > This would allow alternative fallback drivers and would not
> > > > require any definitions at uclass level.
> > > > 
> > > > Just by building a fallback driver you would enable the fallback
> > > > behavior for its uclass.
> > > 
> > > I think some of this is addressed in the cover letter. My concern /
> > > questions come down to I think just a matter of naming. Both "fallback"
> > > and "stub" are used at times. As a concept, we have some areas where we
> > > need a no-op driver because whereas the DT describes a relationship in
> > > the hardware, here in U-Boot we can just accept things as configured. To
> > > me "fallback" implies more functionality of the driver when it's really
> > > just a generic no-op driver to fill in the dependencies within the tree.
> > > Can we rename things a bit along those lines?
> 
> Yes, will do. I originally used "stub" but then decided "fallback" was
> better, but I think you're right that stub is the right way to go here. As I
> agree that "fallback" implies some kind of real implementation.
> > > 
> > 
> > I would rather just have a stub driver with compatibles for all clocks
> > we want
> > it to match. I think having a generic fallback driver could cause issues
> > in the
> > future if we need to switch over to using a real driver.
> 
> I don't think either approach is significantly better from a developer
> perspective. With my patches if there a driver with a matching compatible
> available then you will NEVER bind the stub, even if probe fails on the real
> driver. With your proposal we'd have to remember to remove the compatible
> from the stub driver or risk the race condition when binding drivers.
> 
> There is another subtle but important difference if we were to use a
> compatible list in the stub. It turns out that CONFIG_OF_LIVE affects the
> bind behaviour - U-Boot won't try to bind the children of a node with no
> driver when using the livetree. This is arguably more "correct", as usually
> child devices depend on their parent, so we save some cycles by not binding
> unsupported devices.
> 
> But, the RPM clock controller which I originally implemented this feature
> for is such a case, this is the DT (simplified for brevity):
> 
> rpm: remoteproc {
>     compatible = "qcom,sm6115-rpm-proc", "qcom,rpm-proc";
> 
>     glink-edge {
>         compatible = "qcom,glink-rpm";
> 
>         rpm_requests: rpm-requests {
>             compatible = "qcom,rpm-sm6115";
> 
>             rpmcc: clock-controller {
>                 compatible = "qcom,rpmcc-sm6115", "qcom,rpmcc";
>             };
>     };
> };
> 
> With livetree (which we use on qcom) we would need to stub not just the
> rpmcc but also the rpm-requests, the glink bus, and the remoteproc (yeah
> these platforms are a bit silly, I know XD).
> 
> However with the fallback solution as implemented in these patches, we
> totally sidestep this issue by directly binding the node on-demand,
> something we can only safely do for a stub driver as we don't need the
> parent devices.
> 
> So I can either:
> 
> 1) Make OF_LIVE behave like flattree and descend into all nodes (I don't
> know if this would cause any issues, but I think the livetree behaviour is
> more "correct" tbh).
> 2) Add stubs for all 3 of the parent nodes (and manually maintain a table of
> compatibles for clock drivers we want to stub).
> 3) Stick with this approach of binding the stub as a last resort.
> 
> Obviously I've already put the time into 3, and that would still be my
> preferred approach, but I'm open to other ideas.

I don't like option 1 either. But I don't see why option 2 is worrisome?
Yes, if adding a real driver for something we had stubbed out, we will
need to remove the compatible string for the stub. But that shouldn't
happen often? And we can put in a debug level or similar print so that
if someone is at the point of adding a new driver that had been stubbed
out and didn't remove the compatible stub to start with it should show
up pretty quick in looking at logs?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20240503/f41264de/attachment.sig>