linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Saravana Kannan <saravanak@google.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Jonathan Corbet <corbet@lwn.net>, Len Brown <lenb@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Rob Herring <robh+dt@kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	Linux Doc Mailing List <linux-doc@vger.kernel.org>,
	linux-acpi@vger.kernel.org,
	clang-built-linux <clang-built-linux@googlegroups.com>,
	David Collins <collinsd@codeaurora.org>,
	Android Kernel Team <kernel-team@android.com>,
	kbuild test robot <lkp@intel.com>
Subject: Re: [PATCH v11 3/6] of: property: Add functional dependency link from DT bindings
Date: Tue, 8 Oct 2019 11:57:49 -0700	[thread overview]
Message-ID: <CAGETcx-FjF+bktBgL6h0ORH3MU4vMM9JNC2oB8Myn8KqvnmhDg@mail.gmail.com> (raw)
In-Reply-To: <20191008145304.2BD54205F4@mail.kernel.org>

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.

> >
> > > > +static int of_link_property(struct device *dev, struct device_node *con_np,
> > > > +                            const char *prop_name)
> > > > +{
> > > > +       struct device_node *phandle;
> > > > +       const struct supplier_bindings *s = bindings;
> > > > +       unsigned int i = 0;
> > > > +       bool matched = false;
> > > > +       int ret = 0;
> > > > +
> > > > +       /* Do not stop at first failed link, link all available suppliers. */
> > > > +       while (!matched && s->parse_prop) {
> > > > +               while ((phandle = s->parse_prop(con_np, prop_name, i))) {
> > > > +                       matched = true;
> > > > +                       i++;
> > > > +                       if (of_link_to_phandle(dev, phandle) == -EAGAIN)
> > > > +                               ret = -EAGAIN;
> > >
> > > And don't break?
> >
> > There was comments before about how this is not needed.  Frank asked
> > that the comment be removed.  And now you point it out again :)
> >
> > Look at the comment a few lines up, we have to go through all of the
> > suppliers.
> >
>
> Ok. The comment tells me what is happening but it misses the essential
> part which is _why_ we must make links to each supplier and return
> -EAGAIN.

To be clear the -EAGAIN is only if any of the linking fails.

The reason was already discussion in the email thread [1] but I agree
it needs to be documented.

I thought I had documented the _why_ in the documentation for
fwnode.add_links(), but it's not there. I'll check to make sure I
didn't capture it elsewhere and if not, I'll update fwnode.add_links
documentation.

To copy-paste the discussion from the earlier thread:

"Actually, there is a point for this. Say Device-C depends on suppliers
Device-S1 and Device-S2 and they are listed in DT in that order.

Say, S1 gets populated after late_initcall_sync but S2 is probes way
before that. If I don't continue past a "failed linking" to S1 and
also link up to S2, then S2 will get a sync_state() callback before C
is probed. So I have to go through all possible suppliers and [link] as many
as possible."

-Saravana

[1] - https://lore.kernel.org/lkml/CAGETcx-hCrUvY5whZBihueqqCxmF3oDjFybjmoo3JUu87iiiEw@mail.gmail.com/

  reply	other threads:[~2019-10-08 18:58 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-04 21:11 [PATCH v11 0/6] Solve postboot supplier cleanup and optimize probe ordering Saravana Kannan
2019-09-04 21:11 ` [PATCH v11 1/6] driver core: Add fwnode_to_dev() to look up device from fwnode Saravana Kannan
2019-09-04 21:11 ` [PATCH v11 2/6] driver core: Add support for linking devices during device addition Saravana Kannan
2019-09-04 21:11 ` [PATCH v11 3/6] of: property: Add functional dependency link from DT bindings Saravana Kannan
2019-09-11 10:29   ` Stephen Boyd
2019-10-04 15:37     ` Greg Kroah-Hartman
2019-10-04 23:46       ` Saravana Kannan
2019-10-08 14:53       ` Stephen Boyd
2019-10-08 18:57         ` Saravana Kannan [this message]
2019-10-16 20:15           ` Stephen Boyd
2019-09-04 21:11 ` [PATCH v11 4/6] driver core: Add sync_state driver/bus callback Saravana Kannan
2019-09-04 21:11 ` [PATCH v11 5/6] of/platform: Pause/resume sync state during init and of_platform_populate() Saravana Kannan
2019-09-04 21:11 ` [PATCH v11 6/6] of: property: Create device links for all child-supplier depencencies Saravana Kannan
2019-09-11 10:34 ` [PATCH v11 0/6] Solve postboot supplier cleanup and optimize probe ordering Stephen Boyd
2019-10-04 15:34 ` Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGETcx-FjF+bktBgL6h0ORH3MU4vMM9JNC2oB8Myn8KqvnmhDg@mail.gmail.com \
    --to=saravanak@google.com \
    --cc=clang-built-linux@googlegroups.com \
    --cc=collinsd@codeaurora.org \
    --cc=corbet@lwn.net \
    --cc=devicetree@vger.kernel.org \
    --cc=frowand.list@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kernel-team@android.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lkp@intel.com \
    --cc=mark.rutland@arm.com \
    --cc=rafael@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).