[PATCH] board: sl28: add DSA support for variant 2

[Michael Walle]

Am 2021-02-17 00:55, schrieb Vladimir Oltean:
> On Wed, Feb 17, 2021 at 12:29:33AM +0100, Michael Walle wrote:
>> Am 2021-02-16 23:53, schrieb Vladimir Oltean:
>> > On Tue, Feb 16, 2021 at 11:25:25PM +0100, Michael Walle wrote:
>> > > Now that u-boot gained DSA support, and it is already enabled for the
>> > > kontron_sl28 board, add the last missing piece and enable the
>> > > corresponding devices it in the device tree.
>> > >
>> > > Signed-off-by: Michael Walle <michael at walle.cc>
>> > > ---
>> > >  .../arm/dts/fsl-ls1028a-kontron-sl28-var2.dts | 46
>> > > +++++++++++++++++++
>> > >  1 file changed, 46 insertions(+)
>> > >
>> > > diff --git a/arch/arm/dts/fsl-ls1028a-kontron-sl28-var2.dts
>> > > b/arch/arm/dts/fsl-ls1028a-kontron-sl28-var2.dts
>> > > index 1ea1265bcf..39280cd1c7 100644
>> > > --- a/arch/arm/dts/fsl-ls1028a-kontron-sl28-var2.dts
>> > > +++ b/arch/arm/dts/fsl-ls1028a-kontron-sl28-var2.dts
>> > > @@ -15,6 +15,12 @@
>> > >  / {
>> > >  	model = "Kontron SMARC-sAL28 (TSN-on-module)";
>> > >  	compatible = "kontron,sl28-var2", "kontron,sl28", "fsl,ls1028a";
>> > > +
>> > > +	aliases {
>> > > +		eth0 = &mscc_felix_port0;
>> > > +		eth1 = &mscc_felix_port1;
>> > > +		eth2 = &enetc2;
>> >
>> > The way DSA is intended to be used is that the alias for the DSA master
>> > (host port) comes first, then the alias for switch ports.
>> >
>> > The reasoning is that U-Boot has MAC address inheritance rules. If
>> > ethNaddr (where N is the sequence id of the Ethernet udevice
>> > corresponding to a switch port) is not defined in the env, then the MAC
>> > address is inherited from the master interface.
>> >
>> > But the address cannot be inherited unless the master has already been
>> > probed. So it must be probed first, so it needs to have a lower numbered
>> > alias.
>> 
>> Hm. I reordered the aliases because ethaddr should be the MAC
>> address for the first port and eth1addr the one for the second
>> port.
>> 
>> Every board has a pool of MAC addresses and the vendor version of
>> u-boot will populate the environment accoring to the pool. I.e.
>> there will be ethaddr, eth1addr, .. ethNaddr. This is still missing
>> in the vanilla bootloader (no EEPROM decode and no access to the
>> SPI OTP area yet). So for now, the user will have to supply the
>> ethNaddr variables.
> 
> The DSA master is an Ethernet port too which needs a MAC address of its
> own, and a board vendor should think about what MAC address it wants to
> assign to it.
> 
> In the case of NXP LS1028A, the MAC address assigned to eno2 doesn't
> really matter, since the tagging protocol used by the Felix switch
> shifts the MAC addresses to the right, replacing them with the DSA
> header plus some prefix/padding. That prefix maps over the Ethernet
> header in such a way that ENETC believes it's seeing broadcast frames
> (ff:ff:ff:ff:ff:ff), so it accepts them regardless of the MAC address
> that was assigned to eno2 and therefore used for RX filtering.
> 
> But not all switches are like that. The vast majority either have a DSA
> header before the EtherType, or as a tail tag. So the source & 
> destination
> MAC addresses are not shifted, and whatever the MAC address the front
> panel port claims, the DSA master will perceive the same MAC address
> too. This implies that the sanest approach for these switches is to let
> DSA inherit the MAC address from the master. If they have a unique
> MAC address, someone would have to put the DSA master in promiscuous
> mode, something which we do not do currently.
> 
>> So it should be OK in principle. But if - for some reason - there
>> is only one ethaddr, it won't work, right?
> 
> The code should be more expressive than my words:
> 
> 	/*
> 	 * Inherit port's hwaddr from the DSA master, unless the port already
> 	 * has a unique MAC address specified in the environment.
> 	 */
> 	eth_env_get_enetaddr_by_index("eth", dev_seq(pdev), env_enetaddr);
> 	if (!is_zero_ethaddr(env_enetaddr))
> 		return 0;
> 
> 	master = dsa_get_master(dev);
> 	if (!master)
> 		return 0;
> 
> 	master_pdata = dev_get_plat(master);
> 	eth_pdata = dev_get_plat(pdev);
> 	memcpy(eth_pdata->enetaddr, master_pdata->enetaddr, ARP_HLEN);
> 	eth_env_set_enetaddr_by_index("eth", dev_seq(pdev),
> 				      master_pdata->enetaddr);
> 
> If the environment is populated with only one ethaddr, that will work.
> DSA calls eth_env_set_enetaddr_by_index automatically as you can see.
> So next time you run saveenv, eth1addr and eth2addr will automagically
> have the value of ethaddr too. They can be overridden too, and that
> works as well as long as what I said above holds true (the master does
> not need to go into promiscuous mode in order to receive packets with a
> different MAC address from the encapsulated packet's perspective).
> 
>> Unfortunately, I don't see a way to make both work:
>>  (1) the first port should have ethaddr and the second ethaddr
>>  (2) it should work with just one ethaddr
> 
> I don't understand "the first port should have ethaddr and the second
> ethaddr".

This should read "the second eth1addr".

> You mean "the first and the second switch port should have the
> same ethaddr variable"? But that's the same as point 2, isn't it?

Consider this:
> The way DSA is intended to be used is that the alias for the DSA master
> (host port) comes first, then the alias for switch ports.

So, the internal port must have an alias (and thus an ethNaddr)
smaller than the actual ports. So there is no way that the first
port has "ethaddr" and the second one "eth1addr". Keep in mind,
that ethNaddr will automatically set by the board code and without
knowing anything about the actual ethernet. It will just say,
"here have these MAC addresses for the first N network devices".
But there is also a rule that the first two MAC addresses are
used for the two external ports.

If that wasn't clear before, we are using the switch as kind
of a port extender, thus we have two _different_ MAC addresses.

-- 
-michael