| Message ID | 20201115004151.12899-1-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net: mvneta: Fix validation of 2.5G HSGMII without comphy | expand |
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. 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. Clearly there are differences in mvneta hardware in different SoCs. Maybe they should have used different compatibles, so the driver can know which variant of the hardware it is dealing with, rather than relying on presence/lack of comphy. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
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)
On Sun, 15 Nov 2020 01:41:51 +0100 Andreas Färber <afaerber@suse.de> wrote: > - if (pp->comphy || state->interface == PHY_INTERFACE_MODE_2500BASEX) { > + if (pp->comphy || state->interface == PHY_INTERFACE_MODE_2500BASEX > + || state->interface == PHY_INTERFACE_MODE_NA) { > phylink_set(mask, 2500baseT_Full); > phylink_set(mask, 2500baseX_Full); > } No, this will cause, on systems without comphy described, phylink to think that 2500baseX/T is possible. But without comphy how can it be configured? Marek
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
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!
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 54b0bf574c05..c5016036de3a 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -3812,10 +3812,11 @@ static void mvneta_validate(struct phylink_config *config, struct mvneta_port *pp = netdev_priv(ndev); __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; - /* We only support QSGMII, SGMII, 802.3z and RGMII modes */ + /* We only support QSGMII, SGMII, HSGMII, 802.3z and RGMII modes */ if (state->interface != PHY_INTERFACE_MODE_NA && state->interface != PHY_INTERFACE_MODE_QSGMII && state->interface != PHY_INTERFACE_MODE_SGMII && + state->interface != PHY_INTERFACE_MODE_2500BASEX && !phy_interface_mode_is_8023z(state->interface) && !phy_interface_mode_is_rgmii(state->interface)) { bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); @@ -3834,7 +3835,8 @@ static void mvneta_validate(struct phylink_config *config, phylink_set(mask, 1000baseT_Full); phylink_set(mask, 1000baseX_Full); } - if (pp->comphy || state->interface == PHY_INTERFACE_MODE_2500BASEX) { + if (pp->comphy || state->interface == PHY_INTERFACE_MODE_2500BASEX + || state->interface == PHY_INTERFACE_MODE_NA) { phylink_set(mask, 2500baseT_Full); phylink_set(mask, 2500baseX_Full); } @@ -5038,6 +5040,7 @@ static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode) if (phy_mode != PHY_INTERFACE_MODE_QSGMII && phy_mode != PHY_INTERFACE_MODE_SGMII && + phy_mode != PHY_INTERFACE_MODE_2500BASEX && !phy_interface_mode_is_8023z(phy_mode) && !phy_interface_mode_is_rgmii(phy_mode)) return -EINVAL;
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 causes phylink_sfp_config() to fail validation of 2.5G SFP support. Address this by adding 2500baseX_Full and 2500baseT_Full to the mask for state->interface == PHY_INTERFACE_MODE_NA as well. Also handle PHY_INTERFACE_MODE_2500BASEX in two checks for allowed modes and update a comment. Tested with 2.5G and 1G SFPs on Turris Omnia before assigning comphy. Fixes: 1a642ca7f389 ("net: ethernet: mvneta: Add 2500BaseX support for SoCs without comphy") Cc: Sascha Hauer <s.hauer@pengutronix.de> Cc: David S. Miller <davem@davemloft.net> Cc: Marek Behún <kabel@kernel.org> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Uwe Kleine-König <uwe@kleine-koenig.org> Cc: Jason Cooper <jason@lakedaemon.net> Cc: Gregory CLEMENT <gregory.clement@bootlin.com> Signed-off-by: Andreas Färber <afaerber@suse.de> --- drivers/net/ethernet/marvell/mvneta.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) -- 2.28.0