Re: [PATCH net-next] net: mvneta: Fix validation of 2.5G HSGMII without comphy

["Marek Behún" <kabel@kernel.org>]

On Sun, 15 Nov 2020 03:26:01 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Hi Russell,
> 
> On 15.11.20 02:02, Russell King - ARM Linux admin wrote:
> > On Sun, Nov 15, 2020 at 01:41:51AM +0100, Andreas Färber wrote:  
> >> Commit 1a642ca7f38992b086101fe204a1ae3c90ed8016 (net: ethernet: mvneta:
> >> Add 2500BaseX support for SoCs without comphy) added support for 2500BaseX.
> >>
> >> In case a comphy is not provided, mvneta_validate()'s check
> >>   state->interface == PHY_INTERFACE_MODE_2500BASEX
> >> could never be true (it would've returned with empty bitmask before),
> >> so that 2500baseT_Full and 2500baseX_Full do net get added to the mask.  
> > 
> > This makes me nervous. It was intentional that if there is no comphy
> > configured in DT for SoCs such as Armada 388, then there is no support
> > to switch between 1G and 2.5G speed. Unfortunately, the configuration
> > of the comphy is up to the board to do, not the SoC .dtsi, so we can't
> > rely on there being a comphy on Armada 388 systems.  
> 
> Please note that the if clause at the beginning of the validate function
> does not check for comphy at all. So even with comphy, if there is a
> code path that attempts to validate state 2500BASEX, it is broken, too.
> 
> Do you consider the modification of both ifs (validate and power_up) as
> correct? Should they be split off from my main _NA change you discuss?
> 
> > So, one of the side effects of this patch is it incorrectly opens up
> > the possibility of allowing 2.5G support on Armada 388 without a comphy
> > configured.
> > 
> > We really need a better way to solve this; just relying on the lack of
> > comphy and poking at a register that has no effect on Armada 388 to
> > select 2.5G speed while allowing 1G and 2.5G to be arbitarily chosen
> > doesn't sound like a good idea to me.  
> [snip]
> 
> May I add that the comphy needs better documentation?
> 
> Marek and I independently came up with <&comphy5 2> in the end, but the
> DT binding doesn't explain what the index is supposed to be and how I
> might figure it out. It cost me days of reading U-Boot and kernel
> sources and playing around with values until I had the working one.
> 
> Might be helpful to have a symbolic dt-bindings #define for this 2.
> 

The gbe mux number is described in Armada 385 documentation. Yes, maybe
we should add these defines somewhere, but certainly we should not
declare ability of 2500baseX if comphy is not present and the interface
is not configured to 2500baseX by default.

I propose putting this just into the dt binding documentation. No need
for macros IMO, especially since these muxes may be different on each
SOC.

Marek