Date:   Sun, 19 Feb 2023 10:35:00 +0300
From:   Arınç ÜNAL <arinc.unal@...nc9.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Frank Wunderlich <frank-w@...lic-files.de>,
        netdev <netdev@...r.kernel.org>, erkin.bozoglu@...ont.com,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: Choose a default DSA CPU port

On 18.02.2023 23:52, Vladimir Oltean wrote:
> Hi Arınç,
> 
> On Sat, Feb 18, 2023 at 08:07:53PM +0300, Arınç ÜNAL wrote:
>> Hey there folks,
>>
>> The problem is this. Frank and I have got a Bananapi BPI-R2 with MT7623 SoC.
>> The port5 of MT7530 switch is wired to gmac1 of the SoC. Port6 is wired to
>> gmac0. Since DSA sets the first CPU port it finds on the devicetree, port5
>> becomes the CPU port for all DSA slaves.
>>
>> But we'd prefer port6 since it uses trgmii while port5 uses rgmii. There are
>> also some performance issues with the port5 - gmac1 link.
>>
>> Now we could change it manually on userspace if the DSA subdriver supported
>> changing the DSA master.
>>
>> I'd like to find a solution which would work for the cases of; the driver
>> not supporting changing the DSA master, or saving the effort of manually
>> changing it on userspace.
>>
>> The solution that came to my mind:
>>
>> Introduce a DT property to designate a CPU port as the default CPU port.
>> If this property exists on a CPU port, that port becomes the CPU port for
>> all DSA slaves.
>> If it doesn't exist, fallback to the first-found-cpu-port method.
>>
>> Frank doesn't like this idea:
>>
>>> maybe define the default cpu in driver which gets picked up by core
>> (define port6 as default if available).
>>> Imho additional dts-propperty is wrong approch...it should be handled by
>> driver. But cpu-port-selection is currently done in dsa-core which makes it
>> a bit difficult.
>>
>> What are your thoughts?
>>
>> Arınç
> 
> Before making any change, I believe that the first step is to be in agreement
> as to what the problem is.
> 
> The current DSA device tree binding has always chosen the numerically first
> CPU port as the default CPU port. This was also the case at the time when the
> mt7530 driver was accepted upstream. Clearly there are cases where this choice
> is not the optimal choice (from the perspective of an outside observer).
> For example when another CPU port has a higher link speed. But it's not
> exactly obvious that higher link speed is always better. If you have a CPU
> port at a higher link speed than the user ports, but it doesn't have flow
> control, then the choice is practically worse than the CPU port which operates
> at the same link speed as user ports.
> 
> So the choice between RGMII and TRGMII is not immediately obvious to some code
> which should take this decision automatically. And this complicates things
> a lot. If there is no downside to having the kernel take a decision automatically,
> generally I don't have a problem taking it. But here, I would like to hear
> some strong arguments why port 6 is preferable over port 5.

I'm leaving this to Frank to explain.

> 
> If there are strong reasons to consider port 6 a primary CPU port and
> port 5 a secondary one, then there is also a very valid concern of forward
> compatibility between the mt7530 driver and device trees from the future
> (with multiple CPU ports defined). The authors of the mt7530 driver must have
> been aware of the DSA binding implementation's choice of selecting the
> first CPU port as the default, but they decided to hide their head in
> the sand and pretend like this issue will never crop up. The driver has
> not been coded up to accept port 5 as a valid CPU port until very recently.
> What should have been done (*assuming strong arguments*) is that
> dsa_tree_setup_default_cpu() should have been modified from day one of
> mt7530 driver introduction, such that the driver has a way of specifying
> a preferred default CPU port.
> 
> In other words, the fact that the CPU port becomes port 5 when booting
> on a new device tree is equally a problem for current kernels as it is
> for past LTS kernels. I would like this to be treated seriously, and
> current + stable kernels should behave in a reasonable manner with
> device trees from the future, before support for multiple CPU ports is
> added to mt7530. Forcing users to change device trees in lockstep with
> the kernel is not something that I want to maintain and support, if user
> questions and issues do crop up.
> 
> Since this wasn't done, the only thing we're left with is to retroactively
> add this functionality to pick a preferred default CPU port, as patches
> to "net" which get backported to stable kernels. Given enough forethought
> in the mt7530 driver development, this should not have been necessary,
> but here we are.
> 
> Now that I expressed this point of view, let me comment on why your
> proposal, Arınç, solves exactly nothing.
> 
> If you add a device tree property for the preferred CPU port, you
> haven't solved any of the compatibility problems:
> 
> - old kernels + new device trees will not have the logic to interpret
>    that device tree property
> - old device trees + new kernels will not have that device tree property
> 
> so... yeah.

Makes perfect sense. I always make the assumption that once the DTs on 
the kernel source code is updated, it will be used everywhere, which is 
just not the case.

Arınç

Powered by blists - more mailing lists