[net-next,2/2] net: axienet: Add support for AXI 2.5G MAC

Commit Message

Gupta, Suraj Nov. 18, 2024, 8:18 a.m. UTC

Add AXI 2.5G MAC support, which is an incremental speed upgrade
of AXI 1G MAC and supports 2.5G speed only. "max-speed" DT property
is used in driver to distinguish 1G and 2.5G MACs of AXI 1G/2.5G IP.
If max-speed property is missing, 1G is assumed to support backward
compatibility.

Co-developed-by: Harini Katakam <[e-mail]>
Signed-off-by: Harini Katakam <[e-mail]>
Signed-off-by: Suraj Gupta <[e-mail]>
---
 drivers/net/ethernet/xilinx/xilinx_axienet.h  |  4 +++-
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 24 +++++++++++++++----
 2 files changed, 22 insertions(+), 6 deletions(-)

Comments

Radhey Shyam Pandey Nov. 18, 2024, 2:42 p.m. UTC | #1

> -----Original Message-----
> From: Suraj Gupta <[e-mail]>
> Sent: Monday, November 18, 2024 1:48 PM
> To: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Simek, Michal <[e-mail]>;
> [e-mail]; Pandey, Radhey Shyam
> <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]; linux-
> [e-mail]; git (AMD-Xilinx) <[e-mail]>; Katakam, Harini
> <[e-mail]>
> Subject: [PATCH net-next 2/2] net: axienet: Add support for AXI 2.5G MAC
> 
> Add AXI 2.5G MAC support, which is an incremental speed upgrade
> of AXI 1G MAC and supports 2.5G speed only. "max-speed" DT property
> is used in driver to distinguish 1G and 2.5G MACs of AXI 1G/2.5G IP.
> If max-speed property is missing, 1G is assumed to support backward
> compatibility.
> 
> Co-developed-by: Harini Katakam <[e-mail]>
> Signed-off-by: Harini Katakam <[e-mail]>
> Signed-off-by: Suraj Gupta <[e-mail]>

Reviewed-by: Radhey Shyam Pandey <[e-mail]>
Thanks!
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet.h  |  4 +++-
>  .../net/ethernet/xilinx/xilinx_axienet_main.c | 24 +++++++++++++++----
>  2 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> index d64b8abcf018..ebad1c147aa2 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> @@ -274,7 +274,7 @@
>  #define XAE_EMMC_RX16BIT	0x01000000 /* 16 bit Rx client enable */
>  #define XAE_EMMC_LINKSPD_10	0x00000000 /* Link Speed mask for 10 Mbit */
>  #define XAE_EMMC_LINKSPD_100	0x40000000 /* Link Speed mask for
> 100 Mbit */
> -#define XAE_EMMC_LINKSPD_1000	0x80000000 /* Link Speed mask for
> 1000 Mbit */
> +#define XAE_EMMC_LINKSPD_1000_2500	0x80000000 /* Link Speed
> mask for 1000 or 2500 Mbit */
> 
>  /* Bit masks for Axi Ethernet PHYC register */
>  #define XAE_PHYC_SGMIILINKSPEED_MASK	0xC0000000 /* SGMII link
> speed mask*/
> @@ -542,6 +542,7 @@ struct skbuf_dma_descriptor {
>   * @tx_ring_tail: TX skb ring buffer tail index.
>   * @rx_ring_head: RX skb ring buffer head index.
>   * @rx_ring_tail: RX skb ring buffer tail index.
> + * @max_speed: Maximum possible MAC speed.
>   */
>  struct axienet_local {
>  	struct net_device *ndev;
> @@ -620,6 +621,7 @@ struct axienet_local {
>  	int tx_ring_tail;
>  	int rx_ring_head;
>  	int rx_ring_tail;
> +	u32 max_speed;
>  };
> 
>  /**
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 273ec5f70005..52a3703bd604 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -2388,6 +2388,7 @@ static struct phylink_pcs *axienet_mac_select_pcs(struct
> phylink_config *config,
>  	struct axienet_local *lp = netdev_priv(ndev);
> 
>  	if (interface == PHY_INTERFACE_MODE_1000BASEX ||
> +	    interface == PHY_INTERFACE_MODE_2500BASEX ||
>  	    interface ==  PHY_INTERFACE_MODE_SGMII)
>  		return &lp->pcs;
> 
> @@ -2421,8 +2422,9 @@ static void axienet_mac_link_up(struct phylink_config
> *config,
>  	emmc_reg &= ~XAE_EMMC_LINKSPEED_MASK;
> 
>  	switch (speed) {
> +	case SPEED_2500:
>  	case SPEED_1000:
> -		emmc_reg |= XAE_EMMC_LINKSPD_1000;
> +		emmc_reg |= XAE_EMMC_LINKSPD_1000_2500;
>  		break;
>  	case SPEED_100:
>  		emmc_reg |= XAE_EMMC_LINKSPD_100;
> @@ -2432,7 +2434,7 @@ static void axienet_mac_link_up(struct phylink_config
> *config,
>  		break;
>  	default:
>  		dev_err(&ndev->dev,
> -			"Speed other than 10, 100 or 1Gbps is not supported\n");
> +			"Speed other than 10, 100, 1Gbps or 2.5Gbps is not
> supported\n");
>  		break;
>  	}
> 
> @@ -2681,6 +2683,12 @@ static int axienet_probe(struct platform_device *pdev)
>  	lp->switch_x_sgmii = of_property_read_bool(pdev->dev.of_node,
>  						   "xlnx,switch-x-sgmii");
> 
> +	ret = of_property_read_u32(pdev->dev.of_node, "max-speed", &lp-
> >max_speed);
> +	if (ret) {
> +		lp->max_speed = SPEED_1000;
> +		netdev_warn(ndev, "Please upgrade your device tree to use max-
> speed\n");
> +	}
> +
>  	/* Start with the proprietary, and broken phy_type */
>  	ret = of_property_read_u32(pdev->dev.of_node, "xlnx,phy-type", &value);
>  	if (!ret) {
> @@ -2854,7 +2862,8 @@ static int axienet_probe(struct platform_device *pdev)
>  			 "error registering MDIO bus: %d\n", ret);
> 
>  	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
> -	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
> +	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX ||
> +	    lp->phy_mode == PHY_INTERFACE_MODE_2500BASEX) {
>  		np = of_parse_phandle(pdev->dev.of_node, "pcs-handle", 0);
>  		if (!np) {
>  			/* Deprecated: Always use "pcs-handle" for pcs_phy.
> @@ -2882,8 +2891,13 @@ static int axienet_probe(struct platform_device *pdev)
> 
>  	lp->phylink_config.dev = &ndev->dev;
>  	lp->phylink_config.type = PHYLINK_NETDEV;
> -	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE |
> MAC_ASYM_PAUSE |
> -		MAC_10FD | MAC_100FD | MAC_1000FD;
> +	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE |
> MAC_ASYM_PAUSE;
> +
> +	/* Set MAC capabilities based on MAC type */
> +	if (lp->max_speed == SPEED_1000)
> +		lp->phylink_config.mac_capabilities |= MAC_10FD | MAC_100FD |
> MAC_1000FD;
> +	else
> +		lp->phylink_config.mac_capabilities |= MAC_2500FD;
> 
>  	__set_bit(lp->phy_mode, lp->phylink_config.supported_interfaces);
>  	if (lp->switch_x_sgmii) {
> --
> 2.25.1

Russell King (Oracle) Nov. 18, 2024, 3:56 p.m. UTC | #2

On Mon, Nov 18, 2024 at 01:48:22PM +0530, Suraj Gupta wrote:
> Add AXI 2.5G MAC support, which is an incremental speed upgrade
> of AXI 1G MAC and supports 2.5G speed only. "max-speed" DT property
> is used in driver to distinguish 1G and 2.5G MACs of AXI 1G/2.5G IP.
> If max-speed property is missing, 1G is assumed to support backward
> compatibility.
> 
> Co-developed-by: Harini Katakam <[e-mail]>
> Signed-off-by: Harini Katakam <[e-mail]>
> Signed-off-by: Suraj Gupta <[e-mail]>
> ---

...

> -	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> -		MAC_10FD | MAC_100FD | MAC_1000FD;
> +	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
> +
> +	/* Set MAC capabilities based on MAC type */
> +	if (lp->max_speed == SPEED_1000)
> +		lp->phylink_config.mac_capabilities |= MAC_10FD | MAC_100FD | MAC_1000FD;
> +	else
> +		lp->phylink_config.mac_capabilities |= MAC_2500FD;

The MAC can only operate at (10M, 100M, 1G) _or_ 2.5G ?

Normally, max speeds can be limited using phylink_limit_mac_speed()
which will clear any MAC capabilities for speeds faster than the
speed specified.

Sean Anderson Nov. 18, 2024, 4 p.m. UTC | #3

On 11/18/24 10:56, Russell King (Oracle) wrote:
> On Mon, Nov 18, 2024 at 01:48:22PM +0530, Suraj Gupta wrote:
>> Add AXI 2.5G MAC support, which is an incremental speed upgrade
>> of AXI 1G MAC and supports 2.5G speed only. "max-speed" DT property
>> is used in driver to distinguish 1G and 2.5G MACs of AXI 1G/2.5G IP.
>> If max-speed property is missing, 1G is assumed to support backward
>> compatibility.
>> 
>> Co-developed-by: Harini Katakam <[e-mail]>
>> Signed-off-by: Harini Katakam <[e-mail]>
>> Signed-off-by: Suraj Gupta <[e-mail]>
>> ---
> 
> ...
> 
>> -	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
>> -		MAC_10FD | MAC_100FD | MAC_1000FD;
>> +	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
>> +
>> +	/* Set MAC capabilities based on MAC type */
>> +	if (lp->max_speed == SPEED_1000)
>> +		lp->phylink_config.mac_capabilities |= MAC_10FD | MAC_100FD | MAC_1000FD;
>> +	else
>> +		lp->phylink_config.mac_capabilities |= MAC_2500FD;
> 
> The MAC can only operate at (10M, 100M, 1G) _or_ 2.5G ?

It's a PCS limitation. It either does (1000Base-X and/or SGMII) OR
(2500Base-X). The MAC itself doesn't have this limitation AFAIK.

--Sean

> Normally, max speeds can be limited using phylink_limit_mac_speed()
> which will clear any MAC capabilities for speeds faster than the
> speed specified.
>

Russell King (Oracle) Nov. 18, 2024, 4:08 p.m. UTC | #4

On Mon, Nov 18, 2024 at 11:00:22AM -0500, Sean Anderson wrote:
> On 11/18/24 10:56, Russell King (Oracle) wrote:
> > On Mon, Nov 18, 2024 at 01:48:22PM +0530, Suraj Gupta wrote:
> >> Add AXI 2.5G MAC support, which is an incremental speed upgrade
> >> of AXI 1G MAC and supports 2.5G speed only. "max-speed" DT property
> >> is used in driver to distinguish 1G and 2.5G MACs of AXI 1G/2.5G IP.
> >> If max-speed property is missing, 1G is assumed to support backward
> >> compatibility.
> >> 
> >> Co-developed-by: Harini Katakam <[e-mail]>
> >> Signed-off-by: Harini Katakam <[e-mail]>
> >> Signed-off-by: Suraj Gupta <[e-mail]>
> >> ---
> > 
> > ...
> > 
> >> -	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> >> -		MAC_10FD | MAC_100FD | MAC_1000FD;
> >> +	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
> >> +
> >> +	/* Set MAC capabilities based on MAC type */
> >> +	if (lp->max_speed == SPEED_1000)
> >> +		lp->phylink_config.mac_capabilities |= MAC_10FD | MAC_100FD | MAC_1000FD;
> >> +	else
> >> +		lp->phylink_config.mac_capabilities |= MAC_2500FD;
> > 
> > The MAC can only operate at (10M, 100M, 1G) _or_ 2.5G ?
> 
> It's a PCS limitation. It either does (1000Base-X and/or SGMII) OR
> (2500Base-X). The MAC itself doesn't have this limitation AFAIK.

That means the patch is definitely wrong, and the proposed DT
change is also wrong.

If it's a limitation of the PCS, that limitation should be applied
via the PCS's .pcs_validate() method, not at the MAC level.

Andrew Lunn Nov. 19, 2024, 1:35 a.m. UTC | #5

On Mon, Nov 18, 2024 at 11:00:22AM -0500, Sean Anderson wrote:
> On 11/18/24 10:56, Russell King (Oracle) wrote:
> > On Mon, Nov 18, 2024 at 01:48:22PM +0530, Suraj Gupta wrote:
> >> Add AXI 2.5G MAC support, which is an incremental speed upgrade
> >> of AXI 1G MAC and supports 2.5G speed only. "max-speed" DT property
> >> is used in driver to distinguish 1G and 2.5G MACs of AXI 1G/2.5G IP.
> >> If max-speed property is missing, 1G is assumed to support backward
> >> compatibility.
> >> 
> >> Co-developed-by: Harini Katakam <[e-mail]>
> >> Signed-off-by: Harini Katakam <[e-mail]>
> >> Signed-off-by: Suraj Gupta <[e-mail]>
> >> ---
> > 
> > ...
> > 
> >> -	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> >> -		MAC_10FD | MAC_100FD | MAC_1000FD;
> >> +	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
> >> +
> >> +	/* Set MAC capabilities based on MAC type */
> >> +	if (lp->max_speed == SPEED_1000)
> >> +		lp->phylink_config.mac_capabilities |= MAC_10FD | MAC_100FD | MAC_1000FD;
> >> +	else
> >> +		lp->phylink_config.mac_capabilities |= MAC_2500FD;
> > 
> > The MAC can only operate at (10M, 100M, 1G) _or_ 2.5G ?
> 
> It's a PCS limitation. It either does (1000Base-X and/or SGMII) OR
> (2500Base-X). The MAC itself doesn't have this limitation AFAIK.


And can the PCS change between these modes? It is pretty typical to
use SGMII for 10/100/1G and then swap to 2500BaseX for 2.5G.

	Andrew

Gupta, Suraj Nov. 19, 2024, 10:28 a.m. UTC | #6

> -----Original Message-----
> From: Russell King <[e-mail]>
> Sent: Monday, November 18, 2024 9:39 PM
> To: Sean Anderson <[e-mail]>
> Cc: Gupta, Suraj <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Simek, Michal <[e-mail]>; Pandey, Radhey
> Shyam <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; git (AMD-Xilinx) <[e-mail]>; Katakam, Harini
> <[e-mail]>
> Subject: Re: [PATCH net-next 2/2] net: axienet: Add support for AXI 2.5G MAC
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> On Mon, Nov 18, 2024 at 11:00:22AM -0500, Sean Anderson wrote:
> > On 11/18/24 10:56, Russell King (Oracle) wrote:
> > > On Mon, Nov 18, 2024 at 01:48:22PM +0530, Suraj Gupta wrote:
> > >> Add AXI 2.5G MAC support, which is an incremental speed upgrade of
> > >> AXI 1G MAC and supports 2.5G speed only. "max-speed" DT property is
> > >> used in driver to distinguish 1G and 2.5G MACs of AXI 1G/2.5G IP.
> > >> If max-speed property is missing, 1G is assumed to support backward
> > >> compatibility.
> > >>
> > >> Co-developed-by: Harini Katakam <[e-mail]>
> > >> Signed-off-by: Harini Katakam <[e-mail]>
> > >> Signed-off-by: Suraj Gupta <[e-mail]>
> > >> ---
> > >
> > > ...
> > >
> > >> -  lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE |
> MAC_ASYM_PAUSE |
> > >> -          MAC_10FD | MAC_100FD | MAC_1000FD;
> > >> +  lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE |
> > >> + MAC_ASYM_PAUSE;
> > >> +
> > >> +  /* Set MAC capabilities based on MAC type */  if (lp->max_speed
> > >> + == SPEED_1000)
> > >> +          lp->phylink_config.mac_capabilities |= MAC_10FD |
> > >> + MAC_100FD | MAC_1000FD;  else
> > >> +          lp->phylink_config.mac_capabilities |= MAC_2500FD;
> > >
> > > The MAC can only operate at (10M, 100M, 1G) _or_ 2.5G ?
> >
> > It's a PCS limitation. It either does (1000Base-X and/or SGMII) OR
> > (2500Base-X). The MAC itself doesn't have this limitation AFAIK.
> 
> That means the patch is definitely wrong, and the proposed DT change is also
> wrong.
> 
> If it's a limitation of the PCS, that limitation should be applied via the PCS's
> .pcs_validate() method, not at the MAC level.
> 
As mentioned in IP PG (https://docs.amd.com/r/en-US/pg051-tri-mode-eth-mac/Ethernet-Overview#:~:text=Typical%20Ethernet%20Architecture-,MAC,-For%2010/100), it's limitation in MAC also.

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

Russell King (Oracle) Nov. 19, 2024, 1:18 p.m. UTC | #7

On Tue, Nov 19, 2024 at 10:28:48AM +0000, Gupta, Suraj wrote:
> > -----Original Message-----
> > From: Russell King <[e-mail]>
> > 
> > On Mon, Nov 18, 2024 at 11:00:22AM -0500, Sean Anderson wrote:
> > > On 11/18/24 10:56, Russell King (Oracle) wrote:
> > > > On Mon, Nov 18, 2024 at 01:48:22PM +0530, Suraj Gupta wrote:
> > > >> Add AXI 2.5G MAC support, which is an incremental speed upgrade of
> > > >> AXI 1G MAC and supports 2.5G speed only. "max-speed" DT property is
> > > >> used in driver to distinguish 1G and 2.5G MACs of AXI 1G/2.5G IP.
> > > >> If max-speed property is missing, 1G is assumed to support backward
> > > >> compatibility.
> > > >>
> > > >> Co-developed-by: Harini Katakam <[e-mail]>
> > > >> Signed-off-by: Harini Katakam <[e-mail]>
> > > >> Signed-off-by: Suraj Gupta <[e-mail]>
> > > >> ---
> > > >
> > > > ...
> > > >
> > > >> -  lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE |
> > MAC_ASYM_PAUSE |
> > > >> -          MAC_10FD | MAC_100FD | MAC_1000FD;
> > > >> +  lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE |
> > > >> + MAC_ASYM_PAUSE;
> > > >> +
> > > >> +  /* Set MAC capabilities based on MAC type */  if (lp->max_speed
> > > >> + == SPEED_1000)
> > > >> +          lp->phylink_config.mac_capabilities |= MAC_10FD |
> > > >> + MAC_100FD | MAC_1000FD;  else
> > > >> +          lp->phylink_config.mac_capabilities |= MAC_2500FD;
> > > >
> > > > The MAC can only operate at (10M, 100M, 1G) _or_ 2.5G ?
> > >
> > > It's a PCS limitation. It either does (1000Base-X and/or SGMII) OR
> > > (2500Base-X). The MAC itself doesn't have this limitation AFAIK.
> > 
> > That means the patch is definitely wrong, and the proposed DT change is also
> > wrong.
> > 
> > If it's a limitation of the PCS, that limitation should be applied via the PCS's
> > .pcs_validate() method, not at the MAC level.
> > 
> As mentioned in IP PG (https://docs.amd.com/r/en-US/pg051-tri-mode-eth-mac/Ethernet-Overview#:~:text=Typical%20Ethernet%20Architecture-,MAC,-For%2010/100), it's limitation in MAC also.

I'm not reading it as a limitation of the MAC.

The limitation stated there is that internal mode (GMII) is only
supported for 2.5Gbps speeds. At 2.5Gbps speeds, the clock rate is
increased from 125MHz to 312.5MHz (which makes it non-compliant
with 802.3-2008, because that version doesn't define 2.5Gbps speeds.)

So long as the clock rate and interface can be safely switched, I
don't see any reason to restrict the MAC itself to be either
10/100/1G _or_ 2.5G.

Note that 2.5G will only become available if it is supported by one
of the supported interface modes (e.g. 2500base-X). If the supported
interface modes do not include a mode that supports >1G, then 2.5G
won't be available even if MAC_2500FD is set in mac_capabilities.

Russell King (Oracle) Nov. 19, 2024, 3:12 p.m. UTC | #8

On Tue, Nov 19, 2024 at 01:18:57PM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 19, 2024 at 10:28:48AM +0000, Gupta, Suraj wrote:
> > > -----Original Message-----
> > > From: Russell King <[e-mail]>
> > > 
> > > On Mon, Nov 18, 2024 at 11:00:22AM -0500, Sean Anderson wrote:
> > > > On 11/18/24 10:56, Russell King (Oracle) wrote:
> > > > > On Mon, Nov 18, 2024 at 01:48:22PM +0530, Suraj Gupta wrote:
> > > > >> Add AXI 2.5G MAC support, which is an incremental speed upgrade of
> > > > >> AXI 1G MAC and supports 2.5G speed only. "max-speed" DT property is
> > > > >> used in driver to distinguish 1G and 2.5G MACs of AXI 1G/2.5G IP.
> > > > >> If max-speed property is missing, 1G is assumed to support backward
> > > > >> compatibility.
> > > > >>
> > > > >> Co-developed-by: Harini Katakam <[e-mail]>
> > > > >> Signed-off-by: Harini Katakam <[e-mail]>
> > > > >> Signed-off-by: Suraj Gupta <[e-mail]>
> > > > >> ---
> > > > >
> > > > > ...
> > > > >
> > > > >> -  lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE |
> > > MAC_ASYM_PAUSE |
> > > > >> -          MAC_10FD | MAC_100FD | MAC_1000FD;
> > > > >> +  lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE |
> > > > >> + MAC_ASYM_PAUSE;
> > > > >> +
> > > > >> +  /* Set MAC capabilities based on MAC type */  if (lp->max_speed
> > > > >> + == SPEED_1000)
> > > > >> +          lp->phylink_config.mac_capabilities |= MAC_10FD |
> > > > >> + MAC_100FD | MAC_1000FD;  else
> > > > >> +          lp->phylink_config.mac_capabilities |= MAC_2500FD;
> > > > >
> > > > > The MAC can only operate at (10M, 100M, 1G) _or_ 2.5G ?
> > > >
> > > > It's a PCS limitation. It either does (1000Base-X and/or SGMII) OR
> > > > (2500Base-X). The MAC itself doesn't have this limitation AFAIK.
> > > 
> > > That means the patch is definitely wrong, and the proposed DT change is also
> > > wrong.
> > > 
> > > If it's a limitation of the PCS, that limitation should be applied via the PCS's
> > > .pcs_validate() method, not at the MAC level.
> > > 
> > As mentioned in IP PG (https://docs.amd.com/r/en-US/pg051-tri-mode-eth-mac/Ethernet-Overview#:~:text=Typical%20Ethernet%20Architecture-,MAC,-For%2010/100), it's limitation in MAC also.
> 
> I'm not reading it as a limitation of the MAC.
> 
> The limitation stated there is that internal mode (GMII) is only
> supported for 2.5Gbps speeds. At 2.5Gbps speeds, the clock rate is
> increased from 125MHz to 312.5MHz (which makes it non-compliant
> with 802.3-2008, because that version doesn't define 2.5Gbps speeds.)
> 
> So long as the clock rate and interface can be safely switched, I
> don't see any reason to restrict the MAC itself to be either
> 10/100/1G _or_ 2.5G.
> 
> Note that 2.5G will only become available if it is supported by one
> of the supported interface modes (e.g. 2500base-X). If the supported
> interface modes do not include a mode that supports >1G, then 2.5G
> won't be available even if MAC_2500FD is set in mac_capabilities.

Reading further, PG047 which is the PCS, suggests that it can operate
at 10, 100, 1G, and 2.5G.

Moreover, what I read there is that where a PCS core supports 2.5G, it
can operate at 10, 100, 1G or 2.5G depending on the clock. Note 2 in
"Transceiver ports".

However, it doesn't support TBI at 2.5Gbps mode, only the 2500BASE-X
PMA/PMD.

Also states "The core operates at 125 MHz for the 1 Gbps data rate
(1.25Gbps line rate) and 312.5 MHz at 2.5 Gbps data rates (3.125 Gbps
line rate) in modes having device transceivers." These differences in
clocking are typical for systems that support 1G and 2.5G.

So, I'm still wondering what the limitation is. If the MAC transmit
clock can only run at 125MHz, or only at 312.5MHz, depending on the
design, then yes, it would be appropriate to limit the MAC to 1G
(and below) or 2.5G speeds.

However, if there's designs that allow the transmit clock to be
configured at run time, then the system supports both speeds.

Sean Anderson Nov. 19, 2024, 3:26 p.m. UTC | #9

On 11/18/24 20:35, Andrew Lunn wrote:
> On Mon, Nov 18, 2024 at 11:00:22AM -0500, Sean Anderson wrote:
>> On 11/18/24 10:56, Russell King (Oracle) wrote:
>> > On Mon, Nov 18, 2024 at 01:48:22PM +0530, Suraj Gupta wrote:
>> >> Add AXI 2.5G MAC support, which is an incremental speed upgrade
>> >> of AXI 1G MAC and supports 2.5G speed only. "max-speed" DT property
>> >> is used in driver to distinguish 1G and 2.5G MACs of AXI 1G/2.5G IP.
>> >> If max-speed property is missing, 1G is assumed to support backward
>> >> compatibility.
>> >> 
>> >> Co-developed-by: Harini Katakam <[e-mail]>
>> >> Signed-off-by: Harini Katakam <[e-mail]>
>> >> Signed-off-by: Suraj Gupta <[e-mail]>
>> >> ---
>> > 
>> > ...
>> > 
>> >> -	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
>> >> -		MAC_10FD | MAC_100FD | MAC_1000FD;
>> >> +	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
>> >> +
>> >> +	/* Set MAC capabilities based on MAC type */
>> >> +	if (lp->max_speed == SPEED_1000)
>> >> +		lp->phylink_config.mac_capabilities |= MAC_10FD | MAC_100FD | MAC_1000FD;
>> >> +	else
>> >> +		lp->phylink_config.mac_capabilities |= MAC_2500FD;
>> > 
>> > The MAC can only operate at (10M, 100M, 1G) _or_ 2.5G ?
>> 
>> It's a PCS limitation. It either does (1000Base-X and/or SGMII) OR
>> (2500Base-X). The MAC itself doesn't have this limitation AFAIK.
> 
> 
> And can the PCS change between these modes? It is pretty typical to
> use SGMII for 10/100/1G and then swap to 2500BaseX for 2.5G.

Not AFAIK. There's only a bit for switching between 1000Base-X and
SGMII. 2500Base-X is selected at synthesis time, and AIUI the serdes
settings are different.

--Sean

Russell King (Oracle) Nov. 19, 2024, 3:49 p.m. UTC | #10

On Tue, Nov 19, 2024 at 10:26:52AM -0500, Sean Anderson wrote:
> On 11/18/24 20:35, Andrew Lunn wrote:
> > On Mon, Nov 18, 2024 at 11:00:22AM -0500, Sean Anderson wrote:
> >> On 11/18/24 10:56, Russell King (Oracle) wrote:
> >> > On Mon, Nov 18, 2024 at 01:48:22PM +0530, Suraj Gupta wrote:
> >> >> Add AXI 2.5G MAC support, which is an incremental speed upgrade
> >> >> of AXI 1G MAC and supports 2.5G speed only. "max-speed" DT property
> >> >> is used in driver to distinguish 1G and 2.5G MACs of AXI 1G/2.5G IP.
> >> >> If max-speed property is missing, 1G is assumed to support backward
> >> >> compatibility.
> >> >> 
> >> >> Co-developed-by: Harini Katakam <[e-mail]>
> >> >> Signed-off-by: Harini Katakam <[e-mail]>
> >> >> Signed-off-by: Suraj Gupta <[e-mail]>
> >> >> ---
> >> > 
> >> > ...
> >> > 
> >> >> -	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> >> >> -		MAC_10FD | MAC_100FD | MAC_1000FD;
> >> >> +	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
> >> >> +
> >> >> +	/* Set MAC capabilities based on MAC type */
> >> >> +	if (lp->max_speed == SPEED_1000)
> >> >> +		lp->phylink_config.mac_capabilities |= MAC_10FD | MAC_100FD | MAC_1000FD;
> >> >> +	else
> >> >> +		lp->phylink_config.mac_capabilities |= MAC_2500FD;
> >> > 
> >> > The MAC can only operate at (10M, 100M, 1G) _or_ 2.5G ?
> >> 
> >> It's a PCS limitation. It either does (1000Base-X and/or SGMII) OR
> >> (2500Base-X). The MAC itself doesn't have this limitation AFAIK.
> > 
> > 
> > And can the PCS change between these modes? It is pretty typical to
> > use SGMII for 10/100/1G and then swap to 2500BaseX for 2.5G.
> 
> Not AFAIK. There's only a bit for switching between 1000Base-X and
> SGMII. 2500Base-X is selected at synthesis time, and AIUI the serdes
> settings are different.

Okay. First it was a PCS limitation. Then it was a MAC limitation. Now
it's a synthesis limitation.

I'm coming to the conclusion that those I'm communicating with don't
actually know, and are just throwing random thoughts out there.

Please do the research, and come back to me with a real and complete
answer, not some hand-wavey "it's a limitation of X, no it's a
limitation of Y, no it's a limitation of Z" which looks like no one
really knows the correct answer.

Just because the PCS doesn't have a bit that selects 2500base-X is
meaningless. 2500base-X is generally implemented by upclocking
1000base-X by 2.5x. Marvell does this at their Serdes, there is
no configuration at the MAC/PCS for 2.5G speeds.

The same is true of 10GBASE-R vs 5GBASE-R in Marvell - 5GBASE-R is
just the serdes clocking the MAC/PCS at half the rate that 10GBASE-R
would run at.

I suspect this Xilinx hardware is just the same - clock the transmit
path it at 62.5MHz, and you get 1G speeds. Clock it at 156.25MHz,
and you get 2.5G speeds.

Thanks.

Sean Anderson Nov. 19, 2024, 4:42 p.m. UTC | #11

On 11/19/24 10:49, Russell King (Oracle) wrote:
> On Tue, Nov 19, 2024 at 10:26:52AM -0500, Sean Anderson wrote:
>> On 11/18/24 20:35, Andrew Lunn wrote:
>> > On Mon, Nov 18, 2024 at 11:00:22AM -0500, Sean Anderson wrote:
>> >> On 11/18/24 10:56, Russell King (Oracle) wrote:
>> >> > On Mon, Nov 18, 2024 at 01:48:22PM +0530, Suraj Gupta wrote:
>> >> >> Add AXI 2.5G MAC support, which is an incremental speed upgrade
>> >> >> of AXI 1G MAC and supports 2.5G speed only. "max-speed" DT property
>> >> >> is used in driver to distinguish 1G and 2.5G MACs of AXI 1G/2.5G IP.
>> >> >> If max-speed property is missing, 1G is assumed to support backward
>> >> >> compatibility.
>> >> >> 
>> >> >> Co-developed-by: Harini Katakam <[e-mail]>
>> >> >> Signed-off-by: Harini Katakam <[e-mail]>
>> >> >> Signed-off-by: Suraj Gupta <[e-mail]>
>> >> >> ---
>> >> > 
>> >> > ...
>> >> > 
>> >> >> -	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
>> >> >> -		MAC_10FD | MAC_100FD | MAC_1000FD;
>> >> >> +	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
>> >> >> +
>> >> >> +	/* Set MAC capabilities based on MAC type */
>> >> >> +	if (lp->max_speed == SPEED_1000)
>> >> >> +		lp->phylink_config.mac_capabilities |= MAC_10FD | MAC_100FD | MAC_1000FD;
>> >> >> +	else
>> >> >> +		lp->phylink_config.mac_capabilities |= MAC_2500FD;
>> >> > 
>> >> > The MAC can only operate at (10M, 100M, 1G) _or_ 2.5G ?
>> >> 
>> >> It's a PCS limitation. It either does (1000Base-X and/or SGMII) OR
>> >> (2500Base-X). The MAC itself doesn't have this limitation AFAIK.
>> > 
>> > 
>> > And can the PCS change between these modes? It is pretty typical to
>> > use SGMII for 10/100/1G and then swap to 2500BaseX for 2.5G.
>> 
>> Not AFAIK. There's only a bit for switching between 1000Base-X and
>> SGMII. 2500Base-X is selected at synthesis time, and AIUI the serdes
>> settings are different.
> 
> Okay. First it was a PCS limitation. Then it was a MAC limitation. Now
> it's a synthesis limitation.
> 
> I'm coming to the conclusion that those I'm communicating with don't
> actually know, and are just throwing random thoughts out there.
> 
> Please do the research, and come back to me with a real and complete
> answer, not some hand-wavey "it's a limitation of X, no it's a
> limitation of Y, no it's a limitation of Z" which looks like no one
> really knows the correct answer.
> 
> Just because the PCS doesn't have a bit that selects 2500base-X is
> meaningless. 2500base-X is generally implemented by upclocking
> 1000base-X by 2.5x. Marvell does this at their Serdes, there is
> no configuration at the MAC/PCS for 2.5G speeds.
> 
> The same is true of 10GBASE-R vs 5GBASE-R in Marvell - 5GBASE-R is
> just the serdes clocking the MAC/PCS at half the rate that 10GBASE-R
> would run at.
> 
> I suspect this Xilinx hardware is just the same - clock the transmit
> path it at 62.5MHz, and you get 1G speeds. Clock it at 156.25MHz,
> and you get 2.5G speeds.

Hey, I'm just a l^Huser.

In the synthesis settings for the PCS, you can select

  - 1G
    - 1000BASEX
    - SGMII
    - BOTH
  - 2 5G
    - 2500 BASEX
    - 2.5G SGMII

(all of the above being exclusive choices)

In the synthesis settings for the MAC, you can select

  - 1 Gbps
    - Tri speed
    - 1000 Mbps
  - 2.5 Gbps
    - 2500 Mbps

(ditto)

I can't comment on what happens when you over/underclock the MAC or PCS.

--Sean

Patch

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h
index d64b8abcf018..ebad1c147aa2 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
@@ -274,7 +274,7 @@ 
 #define XAE_EMMC_RX16BIT	0x01000000 /* 16 bit Rx client enable */
 #define XAE_EMMC_LINKSPD_10	0x00000000 /* Link Speed mask for 10 Mbit */
 #define XAE_EMMC_LINKSPD_100	0x40000000 /* Link Speed mask for 100 Mbit */
-#define XAE_EMMC_LINKSPD_1000	0x80000000 /* Link Speed mask for 1000 Mbit */
+#define XAE_EMMC_LINKSPD_1000_2500	0x80000000 /* Link Speed mask for 1000 or 2500 Mbit */
 
 /* Bit masks for Axi Ethernet PHYC register */
 #define XAE_PHYC_SGMIILINKSPEED_MASK	0xC0000000 /* SGMII link speed mask*/
@@ -542,6 +542,7 @@  struct skbuf_dma_descriptor {
  * @tx_ring_tail: TX skb ring buffer tail index.
  * @rx_ring_head: RX skb ring buffer head index.
  * @rx_ring_tail: RX skb ring buffer tail index.
+ * @max_speed: Maximum possible MAC speed.
  */
 struct axienet_local {
 	struct net_device *ndev;
@@ -620,6 +621,7 @@  struct axienet_local {
 	int tx_ring_tail;
 	int rx_ring_head;
 	int rx_ring_tail;
+	u32 max_speed;
 };
 
 /**
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 273ec5f70005..52a3703bd604 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2388,6 +2388,7 @@  static struct phylink_pcs *axienet_mac_select_pcs(struct phylink_config *config,
 	struct axienet_local *lp = netdev_priv(ndev);
 
 	if (interface == PHY_INTERFACE_MODE_1000BASEX ||
+	    interface == PHY_INTERFACE_MODE_2500BASEX ||
 	    interface ==  PHY_INTERFACE_MODE_SGMII)
 		return &lp->pcs;
 
@@ -2421,8 +2422,9 @@  static void axienet_mac_link_up(struct phylink_config *config,
 	emmc_reg &= ~XAE_EMMC_LINKSPEED_MASK;
 
 	switch (speed) {
+	case SPEED_2500:
 	case SPEED_1000:
-		emmc_reg |= XAE_EMMC_LINKSPD_1000;
+		emmc_reg |= XAE_EMMC_LINKSPD_1000_2500;
 		break;
 	case SPEED_100:
 		emmc_reg |= XAE_EMMC_LINKSPD_100;
@@ -2432,7 +2434,7 @@  static void axienet_mac_link_up(struct phylink_config *config,
 		break;
 	default:
 		dev_err(&ndev->dev,
-			"Speed other than 10, 100 or 1Gbps is not supported\n");
+			"Speed other than 10, 100, 1Gbps or 2.5Gbps is not supported\n");
 		break;
 	}
 
@@ -2681,6 +2683,12 @@  static int axienet_probe(struct platform_device *pdev)
 	lp->switch_x_sgmii = of_property_read_bool(pdev->dev.of_node,
 						   "xlnx,switch-x-sgmii");
 
+	ret = of_property_read_u32(pdev->dev.of_node, "max-speed", &lp->max_speed);
+	if (ret) {
+		lp->max_speed = SPEED_1000;
+		netdev_warn(ndev, "Please upgrade your device tree to use max-speed\n");
+	}
+
 	/* Start with the proprietary, and broken phy_type */
 	ret = of_property_read_u32(pdev->dev.of_node, "xlnx,phy-type", &value);
 	if (!ret) {
@@ -2854,7 +2862,8 @@  static int axienet_probe(struct platform_device *pdev)
 			 "error registering MDIO bus: %d\n", ret);
 
 	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
-	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
+	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX ||
+	    lp->phy_mode == PHY_INTERFACE_MODE_2500BASEX) {
 		np = of_parse_phandle(pdev->dev.of_node, "pcs-handle", 0);
 		if (!np) {
 			/* Deprecated: Always use "pcs-handle" for pcs_phy.
@@ -2882,8 +2891,13 @@  static int axienet_probe(struct platform_device *pdev)
 
 	lp->phylink_config.dev = &ndev->dev;
 	lp->phylink_config.type = PHYLINK_NETDEV;
-	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
-		MAC_10FD | MAC_100FD | MAC_1000FD;
+	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
+
+	/* Set MAC capabilities based on MAC type */
+	if (lp->max_speed == SPEED_1000)
+		lp->phylink_config.mac_capabilities |= MAC_10FD | MAC_100FD | MAC_1000FD;
+	else
+		lp->phylink_config.mac_capabilities |= MAC_2500FD;
 
 	__set_bit(lp->phy_mode, lp->phylink_config.supported_interfaces);
 	if (lp->switch_x_sgmii) {