Re: [PATCH v11 3/6] of: property: Add functional dependency link from DT bindings

[Stephen Boyd <sboyd@kernel.org>]

Quoting Saravana Kannan (2019-10-08 11:57:49)
> On Tue, Oct 8, 2019 at 7:53 AM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Greg Kroah-Hartman (2019-10-04 08:37:50)
> > > On Wed, Sep 11, 2019 at 03:29:25AM -0700, Stephen Boyd wrote:
> > > > Quoting Saravana Kannan (2019-09-04 14:11:22)
> > > > > +       int ret = 0;
> > > > > +       struct device_node *tmp_np = sup_np;
> > > > > +
> > > > > +       of_node_get(sup_np);
> > > > > +       /*
> > > > > +        * Find the device node that contains the supplier phandle.  It may be
> > > > > +        * @sup_np or it may be an ancestor of @sup_np.
> > > > > +        */
> > > > > +       while (sup_np && !of_find_property(sup_np, "compatible", NULL))
> > > > > +               sup_np = of_get_next_parent(sup_np);
> > > >
> > > > I don't get this. This is assuming that drivers are only probed for
> > > > device nodes that have a compatible string? What about drivers that make
> > > > sub-devices for clk support that have drivers in drivers/clk/ that then
> > > > attach at runtime later? This happens sometimes for MFDs that want to
> > > > split the functionality across the driver tree to the respective
> > > > subsystems.
> > >
> > > For that, the link would not be there, correct?
> >
> > The parent device (MFD) would have the links because that is the device
> > node with the provider property like '#clock-cells'. The child clk
> > device that's populated by the MFD would be the one actually providing
> > the clk via a driver that may probe any time later, or never, depending
> > on if the clk driver is configured as a module or not. I fail to see how
> > this will work for these cases.
> >
> > Is this logic there to find the parent of a regulator phandle and match
> > that to some driver? It looks like it.
> 
> In the case of an MFD creating "fake" children devices, the parent MFD
> device's driver is responsible for handling the sync state callback.
> It'll get the sync_state callback after all the child devices'
> consumers have probed. The MFD driver will need to do the sync state
> clean up for the children devices or pass it on to the child devices'
> drivers (whatever makes sense for that specific MFD) by whatever means
> those specific drivers talk to each other (direct calls, registering
> callbacks, etc).
> 
> If they are real sub-devices, then they should really be captured in
> DT as child devices and then the child device's drivers will get the
> sync state callback directly.

It seems sort of hand-wavy at the moment. Is the plan to actually
implement this for MFDs that are doing these things? It's really hard to
understand this patch series without any actual users.

From my perspective using driver probe as the signal that some resource
like clks or regulators has been consumed and configured into the proper
state is completely wrong. It makes a large assumption that driver probe
is actually putting the device into some state that has taken over
ownership of the device state by the time probe returns. That isn't
always the case when you consider things like the display or GPU don't
do much until their device is opened by userspace.

It would be better to involve the various kernel frameworks in this
decision by having those frameworks intercept the acquisition of the
resources they provide and track consumers to the point where we can be
certain all consumers have requested and configured the resources they
need to operate properly without something go wrong. Maybe we need
drivers to indicate this to frameworks somehow so that we don't turn the
regulator off for the screen when the screen driver probes but the GPU
driver hasn't started drawing anything there because userspace isn't
running yet?

I'm trying to take a step back and understand the bigger picture here.
From what I can tell we're trying to answer the question "When have all
the consumers of this resource put their constraints in place?" This is
because we want to actively cleanup resources that have been left on or
misconfigured by bootloader/firmware code but we can't be certain when
to do that and if we should do that at all. Is that right?