net: dsa: microchip: look for phy-mode in port nodes

Documentation/devicetree/bindings/net/dsa/dsa.txt says that the phy-mode
property should be specified on port nodes. However, the microchip
drivers read it from the switch node.

Let the driver use the per-port property and fall back to the old
location with a warning.

Fix in-tree users.

Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
Link: https://lore.kernel.org/netdev/20200617082235.GA1523@laureti-dev/
---
 arch/arm/boot/dts/at91-sama5d2_icp.dts |  2 +-
 drivers/net/dsa/microchip/ksz8795.c    | 17 +++++++++++-----
 drivers/net/dsa/microchip/ksz9477.c    | 28 +++++++++++++++++---------
 drivers/net/dsa/microchip/ksz_common.c | 13 +++++++++++-
 drivers/net/dsa/microchip/ksz_common.h |  3 ++-
 5 files changed, 45 insertions(+), 18 deletions(-)

On Tue, Jul 14, 2020 at 02:08:28PM +0200, Helmut Grohne wrote:
> Documentation/devicetree/bindings/net/dsa/dsa.txt says that the phy-mode
> property should be specified on port nodes. However, the microchip
> drivers read it from the switch node.
> 
> Let the driver use the per-port property and fall back to the old
> location with a warning.
> 
> Fix in-tree users.

Hi Helmut

I think this change is more complex than it needs to be. Only the CPU
port supports different interface modes. So i don't see the need to
handle both dev->interface and p->interface. Just first search
ksz_switch_register() first look in the cpu port node, and if not
found go back to the old location. The rest of the code can stay the
same.

	Andrew

Hi Andrew,

Thank you for the quick reply.

On Wed, Jul 15, 2020 at 12:27:16AM +0200, Andrew Lunn wrote:
> I think this change is more complex than it needs to be. Only the CPU
> port supports different interface modes. So i don't see the need to
> handle both dev->interface and p->interface. Just first search
> ksz_switch_register() first look in the cpu port node, and if not
> found go back to the old location. The rest of the code can stay the
> same.

The driver supports (among others) the KSZ9897R, which comes with two
MAC ports supporting[1, page 8] RGMII/RMII/MII (ports 6 and 7). Both of
these can be connected to a CPU, so they can both operate as CPU ports
in principle.

However, one can only enable tail tagging for one of them at a time[1,
page 39]. As the current driver expects tail tagging to be enabled on
CPU ports, it doesn't work as is with the driver. It could still be used
to form a ring of switches such that a single failing switch would leave
two chains of switches attached to the CPU. This kind of failover seems
to be part of the DSA vision (but I fail to find a reference at the
moment).

For these reasons, I think that there can be multiple CPU ports in
future. Now there is a trade-off. Either we further encode the
assumption of there being only a single CPU port more deeply into the
driver (as it already does assume that) or we can take the opportunity
to already lift it here with the vision for runtime reconfiguration of
switch topologies.

You seem to be in favour of more deeply encoding the "there can be only
one CPU port" assumption. Based on that assumption, the rest of what you
write makes very much sense to me. Is that the direction to go?

Helmut

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9897R-Data-Sheet-DS00002330D.pdf

On Wed, Jul 15, 2020 at 09:31:12AM +0200, Helmut Grohne wrote:
> You seem to be in favour of more deeply encoding the "there can be only
> one CPU port" assumption. Based on that assumption, the rest of what you
> write makes very much sense to me. Is that the direction to go?

From what i understand, there is only one port which can do RGMII. It
does not really matter if that is the CPU port, or a user
port. Ideally, whatever port it is, should have the phy-mode property
in its port node.

How you store that information until you need it is up to the
driver. But KISS is generally best, reuse what you have, unless there
is a good reason to change it. If you see this code being reused when
more than one port supports RGMII, then adding a per port members
makes sense. But if that is unlikely, keep with the global.

      Andrew

On Wed, Jul 15, 2020 at 03:00:46PM +0200, Andrew Lunn wrote:
> On Wed, Jul 15, 2020 at 09:31:12AM +0200, Helmut Grohne wrote:
> > You seem to be in favour of more deeply encoding the "there can be only
> > one CPU port" assumption. Based on that assumption, the rest of what you
> > write makes very much sense to me. Is that the direction to go?
> 
> From what i understand, there is only one port which can do RGMII. It

This is not universally true. It does hold for a number of smaller chips
including KSZ9893, but it does not hold for e.g. KSZ9897R as explained
in my previous mail on this matter.

I think that this is the sole point of disagreement here. If we assume
that there only ever is one CPU (or user) port, then I agree with the
rest of what you wrote. However, my understanding is that this premise
is violated by larger devices that are partially supported by this
driver.

> does not really matter if that is the CPU port, or a user
> port. Ideally, whatever port it is, should have the phy-mode property
> in its port node.
> 
> How you store that information until you need it is up to the
> driver. But KISS is generally best, reuse what you have, unless there
> is a good reason to change it. If you see this code being reused when
> more than one port supports RGMII, then adding a per port members
> makes sense. But if that is unlikely, keep with the global.

I've prepared a patch based one the one-CPU-port assumption. It really
becomes way simpler that way. I'd like to give it a little more testing
before sending it.

Given the point of discussion I think that the assumption is a
reasonable trade-off, because you can support larger devices with
multiple RGMII-capable ports with this driver as long as you only use
one of them as CPU port. If someone ever wants to use multiple CPU ports
(not me), more significant changes are needed anyway. Partially
supporting this runs afoul KISS as you say.

Helmut

On Thu, Jul 16, 2020 at 09:00:00AM +0200, Helmut Grohne wrote:
> I've prepared a patch based one the one-CPU-port assumption. It really
> becomes way simpler that way. I'd like to give it a little more testing
> before sending it.

I'm sorry, but it is not that simple. Testing revealed a fatal flaw.

At the time ksz_switch_register is called, dev->cpu_port is not
necessarily initialized. For ksz8795, it is initialized during detect
and that is fine. For ksz9477, it is initialized in
ksz9477_config_cpu_port, which comes much too late. The ksz9477 driver
actually handles the case where we choose a CPU port and selects it
using dsa_is_cpu_port (which derives this information from the device
tree during dsa_register_switch).

So in ksz_switch_register, I have no good way of knowing which port will
end up being the CPU port. Options include:
 * Replicating the logic from ksz9477_config_cpu_port. I expect that
   doing this would make the driver harder to maintain.
 * Move the cpu_port computation from ksz9477_config_cpu_port to
   ksz9477_switch_detect. I don't think this would work, because we
   presently call detect before dsa_register_switch, which parses the
   device tree. During detect, dsa_is_cpu_port is not usable.
 * Add a function to ksz_common.c that looks up the phy mode for a port
   from the device tree on demand. That way, ksz9477_config_cpu_port
   could call into the common code instead of reading a ksz_device
   property. It would defer parsing the device tree though and it could
   issue the warning multiple times.
 * Stick with my previous patch that stores the phy mode per-port.

Given the above, the last option seems least bad to me. Do you agree?

Helmut