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

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

[Andreas Färber]

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.

U-Boot v2020.10 on Turris Omnia dumps this table:

 | Lane # | Speed |  Type       |
 --------------------------------
 |   0    |   6   | SATA0       |
 |   1    |   5   | USB3 HOST0  |
 |   2    |   5   | PCIe1       |
 |   3    |   5   | USB3 HOST1  |
 |   4    |   5   | PCIe2       |
 |   5    |   0   | SGMII2      |
 --------------------------------

But IIUC the Linux comphy driver doesn't take any type ID as argument
but rather an index into a table of "ports" which specifies another
index to apply or look up? Terribly indirect and magic to non-experts.

As a DT contributor I would need the binding to tell me that it's an
enum with 2 meaning SGMII2. Not even the max of 2 is documented. The
Linux driver talks of ports, but I don't see that term used in U-Boot,
and U-Boot APIs appeared to pass very different args in the board code.

The binding also still needs to be converted to YAML before we can
extend it with any better explanations. (And before you suggest it:
Since I obviously don't fully understand that hardware, I'm the wrong
person to attempt documenting it. The 38x comphy seems not mentioned in
MAINTAINERS, only the 3700 one.)

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

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

[Marek Behún]

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

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

[Russell King - ARM Linux admin]

On Sun, Nov 15, 2020 at 03:26:01AM +0100, Andreas Färber 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.

I'm afraid you are mistaken.  phy_interface_mode_is_8023z() covers
both 1000BASEX and 2500BASEX.

> 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?

Sorry, don't understand this comment.

> > 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?
> 
> As a DT contributor I would need the binding to tell me that it's an
> enum with 2 meaning SGMII2. Not even the max of 2 is documented. The
> Linux driver talks of ports, but I don't see that term used in U-Boot,
> and U-Boot APIs appeared to pass very different args in the board code.

It would be nice if the comphy documentation described this parameter
in detail for all comphys, but alas it hasn't been.

This is described in the binding for Armada 38x
Documentation/devicetree/bindings/phy/phy-armada38x-comphy.txt:

- #phy-cells : from the generic phy bindings, must be 1. Defines the
               input port to use for a given comphy lane.

This came from the phy-mvebu-comphy.txt, which was maintained by
bootlin/free-electrons, and uses the same wording.

For the Armada 38x comphy, it is the internal port number for the
ethernet controller when the comphy is connected to ethernet. So,
0 is ethernet@70000, 1 is ethernet@30000, 2 is ethernet@34000. It
actually defines an index into a table in the comphy driver which is
used to look up the "Common PHYs Selectors Register" setting for each
comphy, and also describes the bit index in an undocumented
configuration register for the ethernet port. Look at Table 1479
in the public Armada-38x-Functional-Spec-U0A document (which can be
found via google.) You'll see it talks about XXX PortN. This is the
"N" value.

The Armada 38x comphy Linux driver does not support non-ethernet
interfaces (since this was all I needed it for) but the hardware does
support SATA and USB. The driver can be regarded as incomplete, but it
is not necessary for it to be complete. These are not described in the
DT for the SoC since historically they have not needed to be, since
u-boot does all the setup there - and to add them would require a lot
of effort to ensure that they were correct for every board. That's
dangerous to do when we don't have a driver making use of that
information; there would be no validation that it is correct.

> The binding also still needs to be converted to YAML before we can
> extend it with any better explanations. (And before you suggest it:
> Since I obviously don't fully understand that hardware, I'm the wrong
> person to attempt documenting it. The 38x comphy seems not mentioned in
> MAINTAINERS, only the 3700 one.)

I'm not sure how we'd better describe it TBH. It is the input port
number as described by the SoC documentation.

I suppose we could say:

	0 for ethernet@70000
	1 for ethernet@30000
	2 for ethernet@34000
	0 for usb@...
	1 for usb@...
	0 for pci@...
	1 for pci@...
	... etc ...

But then what if we have that IP re-used on a different SoC where the
devices are at different addresses in the SoC. So that would mean we
would need to list out the devices for every SoC in the comphy
documentation, which is hardly practical.

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