MIME-Version: 1.0
References: <20221222134844.lbzyx5hz7z5n763n@skbuf> <4263dc33-0344-16b6-df22-1db9718721b1@linaro.org>
 <20221223134459.6bmiidn4mp6mnggx@skbuf>
In-Reply-To: <20221223134459.6bmiidn4mp6mnggx@skbuf>
From:   Saravana Kannan <saravanak@google.com>
Date:   Mon, 6 Feb 2023 22:49:03 -0800
Message-ID: <CAGETcx8De_qm9hVtK5CznfWke9nmOfV8OcvAW6kmwyeb7APr=g@mail.gmail.com>
Subject: Re: Advice on MFD-style probing of DSA switch SoCs
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Lee Jones <lee@kernel.org>,
        Colin Foster <colin.foster@in-advantage.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Dec 23, 2022 at 5:44 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Fri, Dec 23, 2022 at 09:44:14AM +0100, Krzysztof Kozlowski wrote:
> > just trim the code... we do not need to scroll over unrelated pieces.
>
> ok
>
> > > However, the irq_domain/irqchip handling code in this case will go to
> > > drivers/net/dsa/, and it won't really be a "driver" (there is no struct
> >
> > Why? Devicetree hierarchy has nothing to do with Linux driver hierarchy
> > and nothing stops you from putting irqchip code in respective directory
> > for such DT. Your parent device can be MFD, can be same old DSA switch
> > driver etc. Several options.

Hi Vladimir,

I stumbled onto this thread when searching for some old emails between
us to refresh my memory on fw_devlink + DSA issues.


>
> True, in fact I've already migrated in my tree the drivers for
> nxp,sja1110-base-tx-mdio and nxp,sja1110-base-t1-mdio (which in the
> current bindings, are under ethernet-switch/mdios/mdio@N) to dedicated
> platform drivers under drivers/net/mdio/. The sja1105 driver will have
> to support old bindings as well, so code in sja1105_mdio.c which
> registers platform devices for MDIO nodes for compatibility will have to
> stay.
>
> But I don't want to keep doing that for other peripherals. The irqchip
> is not a child of the ethernet-switch, not in any sense at all. The
> ethernet-switch even has 2 IRQ lines which need to be provided by the
> irqchip, so there would be a circular dependency in the device tree
> description if the ethernet-switch was the parent.

I'm glad you are looking into this and agree how IRQ controllers are
independent of the rest of the ethernet-switch, etc.

> fw_devlink doesn't really like that, and has been causing problems for
> similar topologies with other DSA switches. There have been discussions
> with Saravana Kannan, and he proposed introducing a FWNODE_FLAG_BROKEN_PARENT
> flag, that says "don't create device links between a consumer and a
> supplier, if the consumer needs a resource from the supplier to probe,
> and the supplier needs to manually probe the consumer to finish its own
> probing".
> https://patchwork.kernel.org/project/netdevbpf/cover/20210826074526.825517-1-saravanak@google.com/

It did land as FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD and it's used for
PHYs. But yeah, it's not a great long term solution.

> That patch didn't really go anywhere to my knowledge, but I'd prefer to
> sidestep all that discussion about what constitutes a broken parent and
> what doesn't, and here, introducing an irqchip driver which is a fwnode
> child of the ethernet-switch driver seems like a big mistake, given past
> experience.

IMHO, the DSA is a logical device that's made up of many different
pieces of real hardware IP. IMHO an ideal solution would be something
like a dsa_bus type where we add a dsa_device. The dsa_device will
list all the necessary devices (IRQ, PHY, MDIO, etc -- they can be
wherever they want in DT) as its suppliers and when the dsa_device is
probed, it can assume all its suppliers are present and then do the
DSA initialization.

This would also solve the PHYs problem you stated earlier. So,
basically you'd move some of the dsa initialization code into the
dsa_probe() function of the dsa_bus.

Hope I'm making some sense. Let me know if you want to discuss this
further and I can try and provide more context and details.

Also, there's already a driver core feature that does just this --
component devices -- but the implementation is old and not so great
IMHO. Component device model can be done better using device links. I
want to refactor the component device framework to use device links,
but that's a problem for another time.

-Saravana