Re: DSA using cpsw and lan9303

["Måns Rullgård" <mans@mansr.com>]

Vladimir Oltean <olteanv@gmail.com> writes:

> On Wed, Feb 16, 2022 at 04:26:34PM +0200, Vladimir Oltean wrote:
>> On Wed, Feb 16, 2022 at 02:23:24PM +0000, Måns Rullgård wrote:
>> > Vladimir Oltean <olteanv@gmail.com> writes:
>> > 
>> > > On Wed, Feb 16, 2022 at 01:17:47PM +0000, Måns Rullgård wrote:
>> > >> > Some complaints about accessing the CPU port as dsa_to_port(chip->ds, 0),
>> > >> > but it's not the first place in this driver where that is done.
>> > >> 
>> > >> What would be the proper way to do it?
>> > >
>> > > Generally speaking:
>> > >
>> > > 	struct dsa_port *cpu_dp;
>> > >
>> > > 	dsa_switch_for_each_cpu_port(cpu_dp, ds)
>> > > 		break;
>> > >
>> > > 	// use cpu_dp
>> > >
>> > > If your code runs after dsa_tree_setup_default_cpu(), which contains the
>> > > "DSA: tree %d has no CPU port\n" check, you don't even need to check
>> > > whether cpu_dp was found or not - it surely was. Everything that runs
>> > > after dsa_register_switch() has completed successfully - for example the
>> > > DSA ->setup() method - qualifies here.
>> > 
>> > In this particular driver, the setup function contains this:
>> > 
>> > 	/* Make sure that port 0 is the cpu port */
>> > 	if (!dsa_is_cpu_port(ds, 0)) {
>> > 		dev_err(chip->dev, "port 0 is not the CPU port\n");
>> > 		return -EINVAL;
>> > 	}
>> > 
>> > I take this to mean that port 0 is guaranteed to be the cpu port.  Of
>> > course, it can't hurt to be thorough just in case that check is ever
>> > removed.
>> 
>> Yes, I saw that, and I said that there are other places in the driver
>> that assume port 0 is the CPU port. Although I don't know why that is,
>> if the switch can only operate like that, etc. I just pointed out how it
>> would be preferable to get a hold of the CPU port in a regular DSA
>> driver without any special constraints.
>
> Ah, silly me, I should have paid more attention on where you're actually
> inserting the code. You could have done:
>
> static int lan9303_port_enable(struct dsa_switch *ds, int port,
> 			       struct phy_device *phy)
> {
> 	struct dsa_port *dp = dsa_to_port(ds, port);
> 	struct lan9303 *chip = ds->priv;
>
> 	if (!dsa_port_is_user(dp))
> 		return 0;
>
> 	vlan_vid_add(dp->cpu_dp->master, htons(ETH_P_8021Q), port);
>
> 	return lan9303_enable_processing_port(chip, port);
> }
>
> the advantage being that if this driver ever supports the remapping of
> the CPU port, or multiple CPU ports, this logic wouldn't need to be
> changed, as it also conveys the user-to-CPU port affinity.

The LAN9303 has (R)MII for port 0 and internal PHYs for ports 1/2, so
there's really only one sensible way to connect it, even though the
switch core has identical functionality for all ports.

-- 
Måns Rullgård