Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Dec 2022 15:34:27 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        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
Subject: Re: Advice on MFD-style probing of DSA switch SoCs
Message-ID: <Y6Rq8+wYpDkGGbYs@lunn.ch>
References: <20221222134844.lbzyx5hz7z5n763n@skbuf>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221222134844.lbzyx5hz7z5n763n@skbuf>
Precedence: bulk

> I think that doesn't scale very well either, so I was looking into
> transitioning the sja1105 bindings to something similar to what Colin
> Foster has done with vsc7512 (ocelot). For this switch, new-style
> bindings would look like this:

Have you looked at probe ordering issues? MFD devices i've played with
are very independent. They are a bunch of IP blocks sharing a bus. A
switch however is very interconnected.

> 
> 	soc@2 {
> 		compatible = "nxp,sja1110-soc";
> 		reg = <2>;
> 		spi-max-frequency = <4000000>;
> 		spi-cpol;
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 
> 		sw2: ethernet-switch@0 {
> 			compatible = "nxp,sja1110a";
> 			reg = <0x000000 0x400000>;
> 			resets = <&sw2_rgu SJA1110_RGU_ETHSW_RST>;
> 			dsa,member = <0 1>;
> 
> 			ethernet-ports {
> 				#address-cells = <1>;
> 				#size-cells = <0>;

...

> 
> 				port@3 {
> 					reg = <3>;
> 					label = "1ge_p2";
> 					phy-mode = "rgmii-id";
> 					phy-handle = <&sw2_mii3_phy>;
> 				};


So for the switch to probe, the PHY needs to probe first.

> 		mdio@704000 {
> 			compatible = "nxp,sja1110-base-t1-mdio";
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			reg = <0x704000 0x1000>;
> 
> 			sw2_port5_base_t1_phy: ethernet-phy@1 {
> 				compatible = "ethernet-phy-ieee802.3-c45";
> 				reg = <0x1>;
> 				interrupts-extended = <&slir2 SJA1110_IRQ_CBT1_PHY1>;
> 			};

For the PHY to probe requires that the interrupt controller probes first.


> 		slir2: interrupt-controller@711fe0 {
> 			compatible = "nxp,sja1110-acu-slir";
> 			reg = <0x711fe0 0x10>;
> 			interrupt-controller;
> 			#interrupt-cells = <1>;
> 			interrupt-parent = <&gpio 10>;
> 		};

and the interrupt controller requires its parent gpio controller
probes first. I assume this is the host SOC GPIO controller, not the
switches GPIO controller.

> 		sw2_rgu: reset@718000 {
> 			compatible = "nxp,sja1110-rgu";
> 			reg = <0x718000 0x1000>;
> 			#reset-cells = <1>;
> 		};

and presumably something needs to hit the reset at some point? Will
there be DT phandles to this?

> 
> 		sw2_cgu: clock-controller@719000 {
> 			compatible = "nxp,sja1110-cgu";
> 			reg = <0x719000 0x1000>;
> 			#clock-cells = <1>;
> 		};

and phandles to the clock driver?

Before doing too much in this direction, i would want to be sure you
have sufficient control of ordering and the DT loops are not too
complex, that the MFD core and the driver core can actually get
everything probed.

The current way of doing it, with the drivers embedded inside the DSA
driver is that DT blob only exposes what needs to be seen outside of
the DSA driver. And the driver has full control over the order it
probes its internal sub drivers, so ensuring they are probed in the
correct order, and the linking is done in C, not DT, were again the
driver is in full control.

I do however agree that being able to split sub drivers out of the
main driver is a good idea, and putting them in the appropriated
subsystem would help with code review.

Maybe the media subsystem has some pointers how to do this. It also
has complex devices made up from lots of sub devices.

    Andrew