[RFC,0/8] Add support for 10G Ethernet SerDes on MT7988

  This series aims to add support for GMAC2 and GMAC3 of the MediaTek MT7988 SoC.
While the vendor SDK stuffs all this into their Ethernet driver, I've tried to
seperate things into a PHY driver, a PCS driver as well as changes to the
existing Ethernet and LynxI PCS driver.

                  +----------------+
+--------------+  |  USXGMII PCS   |   +------------------+
| Ethernet MAC +--+-------------+  +---+ PEXTP SerDes PHY |
+--------------+  |  SGMII PCS  |  |   +------------------+
                  +-------------+--+

Alltogether this allows using GMAC2 and GMAC3 with all possible interface modes,
including in-band-status if needed.

Daniel Golle (8):
  dt-bindings: phy: mediatek,xfi-pextp: add new bindings
  phy: add driver for MediaTek pextp 10GE SerDes PHY
  net: pcs: pcs-mtk-lynxi: use 2500Base-X without AN
  net: pcs: pcs-mtk-lynxi: allow calling with NULL advertising
  dt-bindings: net: pcs: add bindings for MediaTek USXGMII PCS
  net: pcs: add driver for MediaTek USXGMII PCS
  dt-bindings: net: mediatek,net: fix and complete mt7988-eth binding
  net: ethernet: mtk_eth_soc: add paths and SerDes modes for MT7988

 .../devicetree/bindings/net/mediatek,net.yaml | 171 ++++-
 .../bindings/net/pcs/mediatek,usxgmii.yaml    | 105 +++
 .../bindings/phy/mediatek,xfi-pextp.yaml      |  71 ++
 MAINTAINERS                                   |   3 +
 drivers/net/ethernet/mediatek/Kconfig         |  17 +
 drivers/net/ethernet/mediatek/mtk_eth_path.c  | 122 +++-
 drivers/net/ethernet/mediatek/mtk_eth_soc.c   | 178 ++++-
 drivers/net/ethernet/mediatek/mtk_eth_soc.h   | 105 ++-
 drivers/net/pcs/Kconfig                       |  10 +
 drivers/net/pcs/Makefile                      |   1 +
 drivers/net/pcs/pcs-mtk-lynxi.c               |  38 +-
 drivers/net/pcs/pcs-mtk-usxgmii.c             | 688 ++++++++++++++++++
 drivers/phy/mediatek/Kconfig                  |  11 +
 drivers/phy/mediatek/Makefile                 |   1 +
 drivers/phy/mediatek/phy-mtk-pextp.c          | 355 +++++++++
 include/linux/pcs/pcs-mtk-usxgmii.h           |  18 +
 16 files changed, 1813 insertions(+), 81 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/pcs/mediatek,usxgmii.yaml
 create mode 100644 Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml
 create mode 100644 drivers/net/pcs/pcs-mtk-usxgmii.c
 create mode 100644 drivers/phy/mediatek/phy-mtk-pextp.c
 create mode 100644 include/linux/pcs/pcs-mtk-usxgmii.h
  

> +  mediatek,usxgmii-performance-errata:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      USXGMII0 on MT7988 suffers from a performance problem in 10GBase-R
> +      mode which needs a work-around in the driver. The work-around is
> +      enabled using this flag.

Is there more details about this? I'm just wondering if this should be
based on the compatible, rather than a bool property.

      Andrew
  

Hi Andrew,

On Thu, Nov 09, 2023 at 10:55:55PM +0100, Andrew Lunn wrote:
> > +  mediatek,usxgmii-performance-errata:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      USXGMII0 on MT7988 suffers from a performance problem in 10GBase-R
> > +      mode which needs a work-around in the driver. The work-around is
> > +      enabled using this flag.
> 
> Is there more details about this? I'm just wondering if this should be
> based on the compatible, rather than a bool property.

The vendor sources where this is coming from are here:

https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/a500d94cd47e279015ce22947e1ce396a7516598%5E%21/#F0

And I'm afraid this is as much detail as it gets. And yes, we could
also base this on the compatible and just have two different ones for
the two PEXTP instances found in MT7988.
Let me know your conclusion in that regard.


Cheers


Daniel
  

On Thu, Nov 09, 2023 at 09:50:55PM +0000, Daniel Golle wrote:
> Add bindings for the MediaTek PEXTP Ethernet SerDes PHY found in the
> MediaTek MT7988 SoC which can operate at various interfaces modes:
> 
>  * USXGMII
>  * 10GBase-R
>  * 5GBase-R
>  * 2500Base-X
>  * 1000Base-X
>  * Cisco SGMII (MAC side)
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  .../bindings/phy/mediatek,xfi-pextp.yaml      | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml b/Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml
> new file mode 100644
> index 0000000000000..948d5031af1e3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/mediatek,xfi-pextp.yaml
> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/mediatek,xfi-pextp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek XFI PEXTP SerDes PHY
> +
> +maintainers:
> +  - Daniel Golle <daniel@makrotopia.org>
> +
> +description: |

Don't need '|' here.

> +  The MediaTek XFI PEXTP SerDes PHY provides the physical SerDes lanes
> +  used by the MediaTek USXGMII PCS.
> +
> +properties:
> +  $nodename:
> +    pattern: "^phy@[0-9a-f]+$"
> +
> +  compatible:
> +    const: mediatek,mt7988-xfi-pextp
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: XFI PHY clock
> +
> +  resets:
> +    items:
> +      - description: PEXTP reset
> +
> +  mediatek,usxgmii-performance-errata:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      USXGMII0 on MT7988 suffers from a performance problem in 10GBase-R
> +      mode which needs a work-around in the driver. The work-around is
> +      enabled using this flag.
> +
> +  "#phy-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - resets
> +  - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mediatek,mt7988-clk.h>
> +    #include <dt-bindings/reset/mediatek,mt7988-resets.h>
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      xfi_pextp0: phy@11f20000 {

Drop unused labels.

> +        compatible = "mediatek,mt7988-xfi-pextp";
> +        reg = <0 0x11f20000 0 0x10000>;
> +        clocks = <&topckgen CLK_TOP_XFI_PHY_0_XTAL_SEL>;
> +        resets = <&watchdog MT7988_TOPRGU_XFI_PEXTP0_GRST>;
> +        mediatek,usxgmii-performance-errata;
> +        #phy-cells = <0>;
> +      };
> +    };
> +
> +...
> -- 
> 2.42.1
  

On Do, 2023-11-09 at 21:52 +0000, Daniel Golle wrote:
> MT7988 comes with a built-in 2.5G PHY as well as SerDes lanes to
> connect external PHYs or transceivers in USXGMII, 10GBase-R, 5GBase-R,
> 2500Base-X, 1000Base-X and Cisco SGMII interface modes.
> 
> Implement support for configuring for the new paths to SerDes interfaces
> and the internal 2.5G PHY.
> 
> Add USXGMII PCS driver for 10GBase-R, 5GBase-R and USXGMII mode, and
> setup the new PHYA on MT7988 to access the also still existing old
> LynxI PCS for 1000Base-X, 2500Base-X and Cisco SGMII PCS interface
> modes.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
[...]
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> index 9ae3b8a71d0e6..ba5998ef7965e 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> @@ -15,6 +15,7 @@
>  #include <linux/u64_stats_sync.h>
>  #include <linux/refcount.h>
>  #include <linux/phylink.h>
> +#include <linux/reset.h>

I can't see what this is required for?

regards
Philipp
  

On Thu, Nov 09, 2023 at 09:51:57PM +0000, Daniel Golle wrote:
> Add driver for USXGMII PCS found in the MediaTek MT7988 SoC and supporting
> USXGMII, 10GBase-R and 5GBase-R interface modes. In order to support
> Cisco SGMII, 1000Base-X and 2500Base-X via the also present LynxI PCS
> create a wrapped PCS taking care of the components shared between the
> new USXGMII PCS and the legacy LynxI PCS.

What is the actual hardware setup here?

From what I can tell, it's something like this:

         .---- LynxI PCS ----.
 MAC ---+                     +--- PEXP --- external
         `--- USXGMII PCS ---'

Where PEXP is the serdes, handled by the drivers/phy layer in the
kernel. This is not an unusual setup, but we don't have the serdes PHY
controlled by the PCS driver.

You seem to be combining the whole lot into one driver, which seems
rather odd.

I would suggest that the serdes PHY is handled in the MAC driver, using
the mac_prepare(), mac_config() and mac_finish() methods, as well as
other parts of the driver:

- when the netdev is opened, call phy_power_on(pextp)
- when the netdev is closed, call phy_power_off(pextp)
- in mac_prepare(), if the interface has changed, call phy_reset(pextp)
- in mac_finish(), if the interface has changed, update your recorded
  interface mode to detect future changes in either mac_prepare() or
  mac_finish(), and call phy_set_mode_ext(pextp, PHY_MODE_ETHERNET,
  interface).

That will move most of what seems to be duplicated between the two PCS
instances out of the PCS driver and to MAC level, and then the PCS parts
become more about just driving the PCS hardware and nothing beyond that.
More specifically, the wrapping's only function then is to deal with the
sgmii reset. What exactly is that reset signal controlling? The reset to
the LynxI PCS or something else?

If you don't do that (and I prefer that you _do_ the above), then the
following comments apply to the code here:

1. the use of phy_power_on() without any calls to phy_power_off().
   These are counted calls, and after the first call to phy_power_on(),
   the only effect will be to increase the enable-counts of any
   associated regulator and the power count. So, basically you're
   missing calls to phy_power_off(). I suggest a call to phy_power_off()
   in the pcs_disable() function.

2. calling phy_power_on() in pcs_config() is entirely unnecessary.
   pcs_config() will not be called unless pcs_enable() has _already_
   been called, so the call to phy_power_on() in the pcs_enable()
   function is entirely sufficient.

With these two fixed, it means that the pextp PHY will be powered up
when one of the pcs_enable() functions is called, and powered down
when one of the pcs_disable() functions is called.

3. the complicated reset sequence, which is basically:
   - phy_reset(pextp)
   - reset_control_assert(sgmii or xfi reset)
   - *sleep* 100-500us (yes, sleep)
   - reset_control_deassert(sgmii or xfi reset)
   - *delay* 10ms (not sleep, but spin wait)
   If we are in a schedulable context (which the usleep_range() suggests
   we are) then why bother sleeping for the short delay, and
   spin-waiting for the longer delay? A bit of consistency seems to be
   needed here.

4. really needs to explain why it's necessary to repeatedly call the
   pcs_config() function at each get_state() if the link is down.

   Note that with the code the way it is, phy_power_on() will be
   repeatedly called, and at some point the "power_count" will overflow
   which would probably be bad. The counting in the regulator core will
   probably also overflow as well. So this is bad.

   Apart from the overflow issue, the only thing I can see that this
   achieves is to call the core of the pcs_config function. In the case
   of the lynxi, calling its pcs_config() repeatedly with the same
   parameters. Looking at pcs-mtk-lynxi.c, I can't see what this would
   achieve.

With the above issues dealt with, from the point of view of the lynxi /
sgmii code, the only things I can see that the wrapping achieves are:

a) when pcs_enable() is called, call phy_power_on(pextp)
b) when pcs_disable() is called, call phy_power_offpextp)
c) when pcs_config() is called, if the interface has changed:
   i)  call phy_reset() and assert/deassert the "sgmii" reset before
       calling the lynxi PCS
   ii) call phy_set_mode_ext(pextp) for the new interface mode after
       calling the lynxi PCS

I haven't picked through the usxgmii code completely, so I'm not
specifically commenting on it, although some of the above applies
there as well.

Thanks.