Message-ID: <20210901084625.sqzh3oacwgdbhc7f@skbuf>
Date:   Wed, 1 Sep 2021 11:46:25 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, Len Brown <lenb@...nel.org>,
        Alvin Sipraga <ALSI@...g-olufsen.dk>, kernel-team@...roid.com,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        linux-acpi@...r.kernel.org
Subject: Re: [PATCH v1 1/2] driver core: fw_devlink: Add support for
 FWNODE_FLAG_BROKEN_PARENT

On Tue, Aug 31, 2021 at 07:00:58PM -0700, Saravana Kannan wrote:
> On Tue, Aug 31, 2021 at 4:18 PM Vladimir Oltean <olteanv@...il.com> wrote:
> >
> > On Wed, Sep 01, 2021 at 01:02:09AM +0200, Andrew Lunn wrote:
> > > Rev B is interesting because switch0 and switch1 got genphy, while
> > > switch2 got the correct Marvell PHY driver. switch2 PHYs don't have
> > > interrupt properties, so don't loop back to their parent device.
> >
> > This is interesting and not what I really expected to happen. It goes to
> > show that we really need more time to understand all the subtleties of
> > device dependencies before jumping on patching stuff.
> >
> > In case the DSA tree contains more than one switch, different things
> > will happen in dsa_register_switch().
> > The tree itself is only initialized when the last switch calls
> > dsa_register_switch(). All the other switches just mark themselves as
> > present and exit probing early. See this piece of code in dsa_tree_setup:
> >
> >         complete = dsa_tree_setup_routing_table(dst);
> >         if (!complete)
> >                 return 0;
> >
> > So it should be a general property of cross-chip DSA trees that all
> > switches except the last one will have the specific PHY driver probed
> > properly, and not the genphy.
> >
> > Because all (N - 1) switches of a tree exit early in dsa_register_switch,
> > they have successfully probed by the time the last switch brings up the
> > tree, and brings up the PHYs on behalf of every other switch.
> >
> > The last switch can connect to the PHY on behalf of the other switches
> > past their probe ending, and those PHYs should not defer probing because
> > their supplier is now probed. It is only that the last switch cannot
> > connect to the PHYs of its own ports.
> 
> I'm not saying this with any intention of making things easier for me
> (I'm not even sure it does). But your description about how multiple
> switches are handled by DSA has me even more convinced than before
> that DSA needs to use a component device model. This is like the
> textbook example for component devices.

In this example, I guess the component master would be the "struct dsa_switch_tree",
but there is no struct device associated with it.

How many "struct dsa_switch_tree" instances there are in a system
depends on whether OF is used or not.

If we use OF, the device tree needs to be parsed, and every unique first
cell (tree-id) of:
	dsa,member = <tree-id switch-id>;
constitutes a different "struct dsa_switch_tree".

If we do not use OF, the number of switch trees in a system is one, see dsa_switch_parse.

It seems to me like the compare function for component_match (where each
component is a "struct dsa_switch" should look at dev->of_node and parse
the "dsa,member" property again, and match on the same tree-id as the
component master itself?

There's also the question of how to do the component_match in a way that
also works for the pdata/non-OF based DSA systems (of which I have none to test).

All of this to move dsa_tree_setup() outside of the probe calling
context of any individual struct dsa_switch, and into the "bind" calling
context of the component master associated with the struct dsa_switch_tree.
This would allow the phy_connect()/phy_attach_direct() calls to find the
PHY device already bound to the specific driver, which would avoid
binding genphy as a last resort?

Two questions:

- Why would it now be more guaranteed that the PHY drivers are bound to
  the internal PHY devices exactly during the time span between events
  (a) Switch driver (a component of the switch tree) finishes probing
  (b) Switch tree (the component master) starts binding
  I mean in lack of any guarantee, we can still end up in a situation
  where the specific PHY driver still is not bound early enough to the
  internal PHY to be available by the time we call phylink_of_phy_connect,
  and we have all those component device goodies but they don't help.
  I'm sure I'm misunderstanding something but I don't know what.

- What if the internal PHY has other suppliers beyond the interrupt-parent?
  What if, say, it has a reset-gpios = <&gpio1>, where gpio1 is provided
  by some other thing on some other slow bus, which is equally slow (or
  slower) to probe to the DSA switch itself. So the temporary absence of
  this other supplier is causing the specific PHY driver to defer probing,
  just enough for a concurrent call to phylink_of_phy_connect -> phy_attach_direct
  to say "ok, I've waited enough and there is no driver, genphy it is".
  How would this be avoided? Or are you thinking of some kind of two-level
  component driver system:
  - the DSA switch is a component master, with components being its
    sub-devices such as internal PHYs etc
  - the DSA switch is also a component of the DSA switch tree
  But in that case, why is it even relevant to model the DSA switch tree
  probing as a component master, and why does it help? I don't care if
  it's a "textbook example" or not if it doesn't help.

Powered by blists - more mailing lists