On Thu, Jul 07, 2022 at 06:43:03PM +0300, Vladimir Oltean wrote:

On Thu, Jul 07, 2022 at 12:00:54PM +0100, Russell King (Oracle) wrote:

quoted

More importantly, we need your input on Ocelot, which you are listed as
a maintainer for, and Ocelot is the only DSA driver that does stuff
differently (due to the rate adapting PCS). It doesn't set
mac_capabilities, and therefore phylink_set_max_fixed_link() will not
work here.

Has Ocelot ever made use of this DSA feature where, when nothing is
specified for a CPU or DSA port, we use an effective fixed-link setup
with an interface mode that gives the highest speed? Or does this not
apply to this DSA driver?

Thanks.

I'm fine with both the ocelot and sja1105 drivers.

The ocelot driver has 3 users:

- felix_vsc9959 (arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi) on NXP
  LS1028A, where the CPU ports have and have always had a fixed-link
  node in the SoC dtsi. LS1028A based boards should include the SoC
  dtsi. If other board DT writers don't do that or if they delete the
  fixed-link node from the CPU ports, that's not my problem and I don't
  really want to help them.

- seville_vsc9953 (arch/powerpc/boot/dts/fsl/t1040si-post.dtsi) on NXP
  T1040. Same thing, embedded switch, not my fault if the fixed-link
  disappears from the SoC dtsi.

Great, so I'll mark ocelot is safe.

- Colin Foster's SPI-controlled VSC7512 (still downstream). He has an
  Ethernet cable connecting the CPU port to a Beaglebone Black, so he
  has a phy-handle on the CPU port, so definitely not nothing. I believe
  his work hasn't made it to production in any case, so enforcing
  validation now shouldn't bother him too much if at all.

Ok, thanks.

As for sja1105, there is DT validation that checks for the presence of
all required properties in sja1105_parse_ports_node().

Looking at those, it requires all of:

- a phy mode to be specified (as determined by of_get_phy_mode())
- a phy-handle or of_phy_is_fixed_link() to return true

otherwise it errors out.

There is some DT validation in felix_parse_ports_node() too, but it
doesn't check that all specifiers that phylink might use are there.

Phylink (correction, fwnode_get_phy_node() which is not part of phylink
anymore) will look for phy-handle, phy, or phy-device. This is I don't
see that there's any incompatibility between what the driver is doing
and what phylink does.

If there's a fixed-link property, then sja1105_parse_ports_node() is
happy, and so will phylink. If there's a phy-handle, the same is true.
If there's a "phy" or "phy-device" then sja1105_parse_ports_node()
errors out. That's completely fine.

"phy" and "phy-device" are the backwards compatibility for DT - I
believe one of them is the ePAPR specified property that we in Linux
have decided to only fall back on if there's not our more modern
"phy-handle" property.

It seems We have a lot of users of "phy" in DT today, so we can't drop
that from generic code such as phylink, but I haven't found any users
of "phy-device".

I'd really like to add some validation before I gain any involuntary
users, but all open-coded constructs I can come up with are clumsy.
What would you suggest, if I explicitly don't want to rely on
context-specific phylink interpretation of empty OF nodes, and rather
error out?

So I also don't see a problem - sja1105 rejects DTs that fail to
describe a port using at least one of a phy-handle, a fixed-link, or
a managed in-band link, and I don't think it needs to do further
validation, certainly not for the phy describing properties that
the kernel has chosen to deprecate for new implementations.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!