Re: [RFC 2/3] net: Provide switchdev driver for NXP's More Than IP L2 switch

From: Vladimir Oltean <olteanv@gmail.com>
To: Lukasz Majewski <lukma@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>,
	"David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Madalin Bucur <madalin.bucur@oss.nxp.com>,
	Nicolas Ferre <nicolas.ferre@microchip.com>,
	Joakim Zhang <qiangqing.zhang@nxp.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	netdev@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
	Mark Einon <mark.einon@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC 2/3] net: Provide switchdev driver for NXP's More Than IP L2 switch
Date: Mon, 28 Jun 2021 15:48:35 +0300	[thread overview]
Message-ID: <20210628124835.zbuija3hwsnh2zmd@skbuf> (raw)
In-Reply-To: <20210628140526.7417fbf2@ktm>

On Mon, Jun 28, 2021 at 02:05:26PM +0200, Lukasz Majewski wrote:
> Hi Andrew,
>
> > > I do believe that I can just extend the L2 switch driver (fec_mtip.c
> > > file to be precise) to provide full blown L2 switch functionality
> > > without touching the legacy FEC more than in this patch set.
> > >
> > > Would you consider applying this patch series then?
> >
> > What is most important is the ABI. If something is merged now, we need
> > to ensure it does not block later refactoring to a clean new
> > driver. The DT binding is considered ABI. So the DT binding needs to
> > be like a traditional switchdev driver. Florian already pointed out,
> > you can use a binding very similar to DSA. ti,cpsw-switch.yaml is
> > another good example.
>
> The best I could get would be:
>
> &eth_switch {
> 	compatible = "imx,mtip-l2switch";
> 	reg = <0x800f8000 0x400>, <0x800fC000 0x4000>;
>
> 	interrupts = <100>;
> 	status = "okay";
>
> 	ethernet-ports {
> 		port1@1 {
> 			reg = <1>;
> 			label = "eth0";
> 			phys = <&mac0 0>;
> 		};
>
> 		port2@2 {
> 			reg = <2>;
> 			label = "eth1";
> 			phys = <&mac1 1>;
> 		};
> 	};
> };
>
> Which would abuse the "phys" properties usages - as 'mac[01]' are
> referring to ethernet controllers.
>
> On TI SoCs (e.g. am33xx-l4.dtsi) phys refer to some separate driver
> responsible for PHY management. On NXP this is integrated with FEC
> driver itself.

If we were really honest, the binding would need to be called

port@0 {
	puppet = <&mac0>;
};

port@1 {
	puppet = <&mac1>;
};

which speaks for itself as to why accepting "puppet master" drivers is
not really very compelling. I concur with the recommendation given by
Andrew and Florian to refactor FEC as a multi-port single driver.

> >
> > So before considering merging your changes, i would like to see a
> > usable binding.
> >
> > I also don't remember seeing support for STP. Without that, your
> > network has broadcast storm problems when there are loops. So i would
> > like to see the code needed to put ports into blocking, listening,
> > learning, and forwarding states.
> >
> > 	  Andrew

I cannot stress enough how important it is for us to see STP support and
consequently the ndo_start_xmit procedure for switch ports.
Let me see if I understand correctly. When the switch is enabled, eth0
sends packets towards both physical switch ports, and eth1 sends packets
towards none, but eth0 handles the link state of switch port 0, and eth1
handles the link state of switch port 1?