[RFC PATCH net-next 0/5] net: macb: Add versal2 10GBE device support

[RFC PATCH net-next 0/5] net: macb: Add versal2 10GBE device support

[Vineeth Karumanchi]

10GBE IP is a HighSpeed mac that supports 1G, 2.5G, 5G, and 10G speeds
and has two PCS. It has USX PCS for higher speed and SGMII PCS for
lower speed transmission.

Auto-neg is disabled for this IP in versal2 device, hence "fixed-link"
at the specified speed is used. It is an expansion of the fixed-line speed
at 10G currently being implemented. These modifications are tested using
a HW board configuration without a PHY with QSFP.

The IP contains MDIO lines, and the goal is to use auto-neg in the PHY
and set the agreed-upon speed in the MAC. These adjustments will be made
in the following phase after the extra hardware is available.

Vineeth Karumanchi (5):
  dt-bindings: net: macb: Add support for versal2 10gbe device
  net: macb: Add versal2 10GBE device support
  net: macb: Update USX_CONTROL reg's bitfields and constants.
  net: macb: Configure High Speed Mac for given speed.
  net: macb: Get speed and link status runtime.

 .../devicetree/bindings/net/cdns,macb.yaml    |  1 +
 drivers/net/ethernet/cadence/macb.h           | 13 +++
 drivers/net/ethernet/cadence/macb_main.c      | 84 +++++++++++++++----
 3 files changed, 84 insertions(+), 14 deletions(-)

-- 
2.34.1

[RFC PATCH net-next 1/5] dt-bindings: net: macb: Add support for versal2 10gbe device

[Vineeth Karumanchi]

10GBE IP is a high speed mac supporting 10G, 5G, 2.5G and 1G speeds.
It has USX pcs for higher speed  and SGMII PCS for low transmission.

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 Documentation/devicetree/bindings/net/cdns,macb.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
index 3c30dd23cd4e..3870e4846e8a 100644
--- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
+++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
@@ -28,6 +28,7 @@ properties:
       - items:
           - enum:
               - xlnx,versal-gem       # Xilinx Versal
+              - amd,versal2-10gbe     # AMD Versal2 10gbe
               - xlnx,zynq-gem         # Xilinx Zynq-7xxx SoC
               - xlnx,zynqmp-gem       # Xilinx Zynq Ultrascale+ MPSoC
           - const: cdns,gem           # Generic
-- 
2.34.1

Re: [RFC PATCH net-next 1/5] dt-bindings: net: macb: Add support for versal2 10gbe device

[Krzysztof Kozlowski]

On 09/10/2024 07:39, Vineeth Karumanchi wrote:
> 10GBE IP is a high speed mac supporting 10G, 5G, 2.5G and 1G speeds.
> It has USX pcs for higher speed  and SGMII PCS for low transmission.
> 
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
>  Documentation/devicetree/bindings/net/cdns,macb.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> index 3c30dd23cd4e..3870e4846e8a 100644
> --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
> +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> @@ -28,6 +28,7 @@ properties:
>        - items:
>            - enum:
>                - xlnx,versal-gem       # Xilinx Versal
> +              - amd,versal2-10gbe     # AMD Versal2 10gbe

Keep the alphabetical order.

Best regards,
Krzysztof

[RFC PATCH net-next 2/5] net: macb: Add versal2 10GBE device support

[Vineeth Karumanchi]

Add 10GBE high-speed Mac support, it supports 10G, 5G, 2.5G and 1G speeds.
10GBE high speed Mac is an extension of the current 1G Mac in versal,
inheriting all its current features.

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 79db6cd01844..8f893f035289 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4951,6 +4951,16 @@ static const struct macb_config versal_config = {
 	.usrio = &macb_default_usrio,
 };
 
+static const struct macb_config versal2_10gbe_config = {
+	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
+		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_QUEUE_DISABLE,
+	.dma_burst_length = 16,
+	.clk_init = macb_clk_init,
+	.init = init_reset_optional,
+	.jumbo_max_len = 10240,
+	.usrio = &macb_default_usrio,
+};
+
 static const struct of_device_id macb_dt_ids[] = {
 	{ .compatible = "cdns,at91sam9260-macb", .data = &at91sam9260_config },
 	{ .compatible = "cdns,macb" },
@@ -4974,6 +4984,7 @@ static const struct of_device_id macb_dt_ids[] = {
 	{ .compatible = "xlnx,zynqmp-gem", .data = &zynqmp_config},
 	{ .compatible = "xlnx,zynq-gem", .data = &zynq_config },
 	{ .compatible = "xlnx,versal-gem", .data = &versal_config},
+	{ .compatible = "amd,versal2-10gbe", .data = &versal2_10gbe_config},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, macb_dt_ids);
-- 
2.34.1

[RFC PATCH net-next 3/5] net: macb: Update USX_CONTROL reg's bitfields and constants.

[Vineeth Karumanchi]

New bitfeilds of USX_CONTROL register:
- GEM_RX_SYNC: RX Reset: Reset the receive datapath.

Constants of the bitfeilds in USX_CONTROL reg:
- HS_SPEED_*: Multiple speed constants of USX_SPEED bitfeild.
- MACB_SERDES_RATE_*: Multiple serdes rate constants of
  SERDES_RATE bitfeild.

Since MACB_SERDES_RATE_* and HS_SPEED_* are register constants,
move them to the header file.

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb.h      | 12 ++++++++++++
 drivers/net/ethernet/cadence/macb_main.c |  3 ---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 5740c98d8c9f..47e80fa72865 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -563,11 +563,23 @@
 #define GEM_RX_SCR_BYPASS_SIZE			1
 #define GEM_TX_SCR_BYPASS_OFFSET		8
 #define GEM_TX_SCR_BYPASS_SIZE			1
+#define GEM_RX_SYNC_RESET_OFFSET		2
+#define GEM_RX_SYNC_RESET_SIZE			1
 #define GEM_TX_EN_OFFSET			1
 #define GEM_TX_EN_SIZE				1
 #define GEM_SIGNAL_OK_OFFSET			0
 #define GEM_SIGNAL_OK_SIZE			1
 
+/* Constants for USX_CONTROL */
+#define HS_SPEED_10000M				4
+#define HS_SPEED_5000M				3
+#define HS_SPEED_2500M				2
+#define HS_SPEED_1000M				1
+#define MACB_SERDES_RATE_10G			1
+#define MACB_SERDES_RATE_5G			0
+#define MACB_SERDES_RATE_2_5G			0
+#define MACB_SERDES_RATE_1G			0
+
 /* Bitfields in USX_STATUS. */
 #define GEM_USX_BLOCK_LOCK_OFFSET		0
 #define GEM_USX_BLOCK_LOCK_SIZE			1
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 8f893f035289..3f9dc0b037c0 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -87,9 +87,6 @@ struct sifive_fu540_macb_mgmt {
 
 #define MACB_WOL_ENABLED		BIT(0)
 
-#define HS_SPEED_10000M			4
-#define MACB_SERDES_RATE_10G		1
-
 /* Graceful stop timeouts in us. We should allow up to
  * 1 frame time (10 Mbits/s, full-duplex, ignoring collisions)
  */
-- 
2.34.1

Re: [RFC PATCH net-next 3/5] net: macb: Update USX_CONTROL reg's bitfields and constants.

[Russell King (Oracle)]

On Wed, Oct 09, 2024 at 11:09:44AM +0530, Vineeth Karumanchi wrote:
> New bitfeilds of USX_CONTROL register:
> - GEM_RX_SYNC: RX Reset: Reset the receive datapath.
> 
> Constants of the bitfeilds in USX_CONTROL reg:
> - HS_SPEED_*: Multiple speed constants of USX_SPEED bitfeild.
> - MACB_SERDES_RATE_*: Multiple serdes rate constants of
>   SERDES_RATE bitfeild.
> 
> Since MACB_SERDES_RATE_* and HS_SPEED_* are register constants,
> move them to the header file.
> 
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>

...

> +/* Constants for USX_CONTROL */
> +#define HS_SPEED_10000M				4
> +#define HS_SPEED_5000M				3
> +#define HS_SPEED_2500M				2
> +#define HS_SPEED_1000M				1
> +#define MACB_SERDES_RATE_10G			1
> +#define MACB_SERDES_RATE_5G			0
> +#define MACB_SERDES_RATE_2_5G			0
> +#define MACB_SERDES_RATE_1G			0

I'm not sure having multiple definitions for the same value for the same
field makes sense. Maybe call it MACB_SERDES_RATE_5G_2G5_1G ?

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

[RFC PATCH net-next 4/5] net: macb: Configure High Speed Mac for given speed.

[Vineeth Karumanchi]

HS Mac configuration steps:
- Configure speed and serdes rate bits of USX_CONTROL register from
  user specified speed in the device-tree.
- Enable HS Mac for 5G and 10G speeds.
- Reset RX receive path to achieve USX block lock for the
  configured serdes rate.
- Wait for USX block lock synchronization.

Move the initialization instances to macb_usx_pcs_link_up().

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb.h      |  1 +
 drivers/net/ethernet/cadence/macb_main.c | 57 ++++++++++++++++++++----
 2 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 47e80fa72865..ed4edeac3a59 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -825,6 +825,7 @@
 	})
 
 #define MACB_READ_NSR(bp)	macb_readl(bp, NSR)
+#define MACB_READ_USX_STATUS(bp)	gem_readl(bp, USX_STATUS)
 
 /* struct macb_dma_desc - Hardware DMA descriptor
  * @addr: DMA address of data buffer
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 3f9dc0b037c0..7beb775a0bd7 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -94,6 +94,7 @@ struct sifive_fu540_macb_mgmt {
 #define MACB_PM_TIMEOUT  100 /* ms */
 
 #define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
+#define GEM_SYNC_TIMEOUT	2500000 /* in usecs */
 
 /* DMA buffer descriptor might be different size
  * depends on hardware configuration:
@@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 				 int duplex)
 {
 	struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
-	u32 config;
+	u32 speed_val, serdes_rate, config;
+	bool hs_mac = false;
+
+	switch (speed) {
+	case SPEED_1000:
+		speed_val = HS_SPEED_1000M;
+		serdes_rate = MACB_SERDES_RATE_1G;
+		break;
+	case SPEED_2500:
+		speed_val = HS_SPEED_2500M;
+		serdes_rate = MACB_SERDES_RATE_2_5G;
+		break;
+	case SPEED_5000:
+		speed_val = HS_SPEED_5000M;
+		serdes_rate = MACB_SERDES_RATE_5G;
+		hs_mac = true;
+		break;
+	case SPEED_10000:
+		speed_val = HS_SPEED_10000M;
+		serdes_rate = MACB_SERDES_RATE_10G;
+		hs_mac = true;
+		break;
+	default:
+		netdev_err(bp->dev, "Specified speed not supported\n");
+		return;
+	}
+
+	/* Enable HS MAC for high speeds */
+	if (hs_mac) {
+		config = macb_or_gem_readl(bp, NCR);
+		config |= GEM_BIT(ENABLE_HS_MAC);
+		macb_or_gem_writel(bp, NCR, config);
+	}
+
+	/* Configure HS MAC for specified speed */
+	config = gem_readl(bp, HS_MAC_CONFIG);
+	config = GEM_BFINS(HS_MAC_SPEED, speed_val, config);
+	gem_writel(bp, HS_MAC_CONFIG, config);
 
 	config = gem_readl(bp, USX_CONTROL);
-	config = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, config);
-	config = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, config);
+	config = GEM_BFINS(SERDES_RATE, serdes_rate, config);
+	config = GEM_BFINS(USX_CTRL_SPEED, speed_val, config);
 	config &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
+	config |= GEM_BIT(RX_SYNC_RESET);
+	gem_writel(bp, USX_CONTROL, config);
+	mdelay(250);
+	config &= ~GEM_BIT(RX_SYNC_RESET);
 	config |= GEM_BIT(TX_EN);
 	gem_writel(bp, USX_CONTROL, config);
+
+	if (readx_poll_timeout(MACB_READ_USX_STATUS, bp, config, config & GEM_BIT(USX_BLOCK_LOCK),
+			       1, GEM_SYNC_TIMEOUT))
+		netdev_err(bp->dev, "USX PCS block lock not achieved\n");
 }
 
 static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
@@ -662,7 +708,6 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
 			ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
 		} else if (state->interface == PHY_INTERFACE_MODE_10GBASER) {
 			ctrl |= GEM_BIT(PCSSEL);
-			ncr |= GEM_BIT(ENABLE_HS_MAC);
 		} else if (bp->caps & MACB_CAPS_MIIONRGMII &&
 			   bp->phy_interface == PHY_INTERFACE_MODE_MII) {
 			ncr |= MACB_BIT(MIIONRGMII);
@@ -766,10 +811,6 @@ static void macb_mac_link_up(struct phylink_config *config,
 
 	macb_or_gem_writel(bp, NCFGR, ctrl);
 
-	if (bp->phy_interface == PHY_INTERFACE_MODE_10GBASER)
-		gem_writel(bp, HS_MAC_CONFIG, GEM_BFINS(HS_MAC_SPEED, HS_SPEED_10000M,
-							gem_readl(bp, HS_MAC_CONFIG)));
-
 	spin_unlock_irqrestore(&bp->lock, flags);
 
 	if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC))
-- 
2.34.1

Re: [RFC PATCH net-next 4/5] net: macb: Configure High Speed Mac for given speed.

[Maxime Chevallier]

Hello,

On Wed, 9 Oct 2024 11:09:45 +0530
Vineeth Karumanchi <vineeth.karumanchi@amd.com> wrote:

> HS Mac configuration steps:
> - Configure speed and serdes rate bits of USX_CONTROL register from
>   user specified speed in the device-tree.
> - Enable HS Mac for 5G and 10G speeds.
> - Reset RX receive path to achieve USX block lock for the
>   configured serdes rate.
> - Wait for USX block lock synchronization.
> 
> Move the initialization instances to macb_usx_pcs_link_up().
> 
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>

[...]

>  
>  /* DMA buffer descriptor might be different size
>   * depends on hardware configuration:
> @@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
>  				 int duplex)
>  {
>  	struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
> -	u32 config;
> +	u32 speed_val, serdes_rate, config;
> +	bool hs_mac = false;
> +
> +	switch (speed) {
> +	case SPEED_1000:
> +		speed_val = HS_SPEED_1000M;
> +		serdes_rate = MACB_SERDES_RATE_1G;
> +		break;
> +	case SPEED_2500:
> +		speed_val = HS_SPEED_2500M;
> +		serdes_rate = MACB_SERDES_RATE_2_5G;
> +		break;
> +	case SPEED_5000:
> +		speed_val = HS_SPEED_5000M;
> +		serdes_rate = MACB_SERDES_RATE_5G;
> +		hs_mac = true;
> +		break;

You support some new speeds and modes, so you also need to update :

 - The macb_select_pcs() code, as right now it will return NULL for any
mode that isn't 10GBaseR or SGMII, so for 2500/5000 speeds, that
probably won't work. And for 1000, the default PCS will be used and not
USX

 - the phylink mac_capabilities, so far 2500 and 5000 speeds aren't
reported as supported.

 - the phylink supported_interfaces, I suppose the IP uses 2500BaseX
and 5GBaseT ? or maybe some usxgmii flavors ?

> +	case SPEED_10000:
> +		speed_val = HS_SPEED_10000M;
> +		serdes_rate = MACB_SERDES_RATE_10G;
> +		hs_mac = true;
> +		break;
> +	default:
> +		netdev_err(bp->dev, "Specified speed not supported\n");
> +		return;
> +	}
> +
> +	/* Enable HS MAC for high speeds */
> +	if (hs_mac) {
> +		config = macb_or_gem_readl(bp, NCR);
> +		config |= GEM_BIT(ENABLE_HS_MAC);
> +		macb_or_gem_writel(bp, NCR, config);
> +	}

It looks like you moved the MAC selection between HS MAC and non-HS MAC
from the phylink .mac_config to PCS config.

This configuration is indeed a MAC-side configuration from what I
understand, you shouldn't need to set that in PCS code. Maybe instead,
check the interface mode in macb_mac_config, and look if you're in
5GBaseR / 10GBaseR to select the MAC ?

Thanks,

Maxime

Re: [RFC PATCH net-next 4/5] net: macb: Configure High Speed Mac for given speed.

[Karumanchi, Vineeth]

Hi Maxime,

On 10/9/2024 12:06 PM, Maxime Chevallier wrote:
> Hello,
>
> On Wed, 9 Oct 2024 11:09:45 +0530
> Vineeth Karumanchi <vineeth.karumanchi@amd.com> wrote:
>
>> HS Mac configuration steps:
>> - Configure speed and serdes rate bits of USX_CONTROL register from
>>    user specified speed in the device-tree.
>> - Enable HS Mac for 5G and 10G speeds.
>> - Reset RX receive path to achieve USX block lock for the
>>    configured serdes rate.
>> - Wait for USX block lock synchronization.
>>
>> Move the initialization instances to macb_usx_pcs_link_up().
>>
>> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> [...]
>
>>   
>>   /* DMA buffer descriptor might be different size
>>    * depends on hardware configuration:
>> @@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
>>   				 int duplex)
>>   {
>>   	struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
>> -	u32 config;
>> +	u32 speed_val, serdes_rate, config;
>> +	bool hs_mac = false;
>> +
>> +	switch (speed) {
>> +	case SPEED_1000:
>> +		speed_val = HS_SPEED_1000M;
>> +		serdes_rate = MACB_SERDES_RATE_1G;
>> +		break;
>> +	case SPEED_2500:
>> +		speed_val = HS_SPEED_2500M;
>> +		serdes_rate = MACB_SERDES_RATE_2_5G;
>> +		break;
>> +	case SPEED_5000:
>> +		speed_val = HS_SPEED_5000M;
>> +		serdes_rate = MACB_SERDES_RATE_5G;
>> +		hs_mac = true;
>> +		break;
> You support some new speeds and modes, so you also need to update :
>
>   - The macb_select_pcs() code, as right now it will return NULL for any
> mode that isn't 10GBaseR or SGMII, so for 2500/5000 speeds, that
> probably won't work. And for 1000, the default PCS will be used and not
> USX
>
>   - the phylink mac_capabilities, so far 2500 and 5000 speeds aren't
> reported as supported.
>
>   - the phylink supported_interfaces, I suppose the IP uses 2500BaseX
> and 5GBaseT ? or maybe some usxgmii flavors ?

with 10GBaseR, ethtool is showing multiple speed support. ( 1G, 2.5G, 5G 
and 10G )
The only check I see with 10GbaseR is max speed shouldn't be greater 
than 10G, which
suits the IP requirement.

>> +	case SPEED_10000:
>> +		speed_val = HS_SPEED_10000M;
>> +		serdes_rate = MACB_SERDES_RATE_10G;
>> +		hs_mac = true;
>> +		break;
>> +	default:
>> +		netdev_err(bp->dev, "Specified speed not supported\n");
>> +		return;
>> +	}
>> +
>> +	/* Enable HS MAC for high speeds */
>> +	if (hs_mac) {
>> +		config = macb_or_gem_readl(bp, NCR);
>> +		config |= GEM_BIT(ENABLE_HS_MAC);
>> +		macb_or_gem_writel(bp, NCR, config);
>> +	}
> It looks like you moved the MAC selection between HS MAC and non-HS MAC
> from the phylink .mac_config to PCS config.
>
> This configuration is indeed a MAC-side configuration from what I
> understand, you shouldn't need to set that in PCS code. Maybe instead,
> check the interface mode in macb_mac_config, and look if you're in
> 5GBaseR / 10GBaseR to select the MAC ?

Yes, agreed!

As our current HW setup doesn't have AN and PHY, to support speed change 
using ethtool
I have moved the above MAC config into PCS. I will explore on setting 
MAC config based on speed
instead of interface in  macb_mac_config().

Please let me know your thoughts and comments.

-- 
🙏 vineeth

[...]

Re: [RFC PATCH net-next 4/5] net: macb: Configure High Speed Mac for given speed.

[Russell King (Oracle)]

On Wed, Oct 09, 2024 at 11:09:45AM +0530, Vineeth Karumanchi wrote:
> HS Mac configuration steps:
> - Configure speed and serdes rate bits of USX_CONTROL register from
>   user specified speed in the device-tree.
> - Enable HS Mac for 5G and 10G speeds.
> - Reset RX receive path to achieve USX block lock for the
>   configured serdes rate.
> - Wait for USX block lock synchronization.
> 
> Move the initialization instances to macb_usx_pcs_link_up().

It only partly moves stuff there, creating what I can only call a mess
which probably doesn't work correctly.

Please consider the MAC and PCS as two separate boxes - register
settings controlled in one box should not be touched by the other box.

For example, macb_mac_config() now does this:

        old_ncr = ncr = macb_or_gem_readl(bp, NCR);
...
        } else if (macb_is_gem(bp)) {
...
                ncr &= ~GEM_BIT(ENABLE_HS_MAC);
...
        if (old_ncr ^ ncr)
                macb_or_gem_writel(bp, NCR, ncr);

meanwhile:

> @@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
>  				 int duplex)
>  {
>  	struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
...
> +	/* Enable HS MAC for high speeds */
> +	if (hs_mac) {
> +		config = macb_or_gem_readl(bp, NCR);
> +		config |= GEM_BIT(ENABLE_HS_MAC);
> +		macb_or_gem_writel(bp, NCR, config);
> +	}

Arguably, the time that this would happen is when the interface mode
changes which would cause a full reconfiguration and thus both of
these functions will be called, but it's not easy to follow that's
what is going on here.

It also looks like you're messing with MAC registers in the PCS code,
setting the MAC speed there. Are the PCS and MAC so integrated together
that abstracting the PCS into its own separate code block leads to
problems?

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

Re: [RFC PATCH net-next 4/5] net: macb: Configure High Speed Mac for given speed.

[Karumanchi, Vineeth]

Hi Russel,

On 10/9/2024 2:49 PM, Russell King (Oracle) wrote:
> On Wed, Oct 09, 2024 at 11:09:45AM +0530, Vineeth Karumanchi wrote:
>> HS Mac configuration steps:
>> - Configure speed and serdes rate bits of USX_CONTROL register from
>>    user specified speed in the device-tree.
>> - Enable HS Mac for 5G and 10G speeds.
>> - Reset RX receive path to achieve USX block lock for the
>>    configured serdes rate.
>> - Wait for USX block lock synchronization.
>>
>> Move the initialization instances to macb_usx_pcs_link_up().
> It only partly moves stuff there, creating what I can only call a mess
> which probably doesn't work correctly.
>
> Please consider the MAC and PCS as two separate boxes - register
> settings controlled in one box should not be touched by the other box.
>
> For example, macb_mac_config() now does this:
>
>          old_ncr = ncr = macb_or_gem_readl(bp, NCR);
> ...
>          } else if (macb_is_gem(bp)) {
> ...
>                  ncr &= ~GEM_BIT(ENABLE_HS_MAC);
> ...
>          if (old_ncr ^ ncr)
>                  macb_or_gem_writel(bp, NCR, ncr);
>
> meanwhile:
>
>> @@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
>>   				 int duplex)
>>   {
>>   	struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
> ...
>> +	/* Enable HS MAC for high speeds */
>> +	if (hs_mac) {
>> +		config = macb_or_gem_readl(bp, NCR);
>> +		config |= GEM_BIT(ENABLE_HS_MAC);
>> +		macb_or_gem_writel(bp, NCR, config);
>> +	}
> Arguably, the time that this would happen is when the interface mode
> changes which would cause a full reconfiguration and thus both of
> these functions will be called, but it's not easy to follow that's
> what is going on here.
>
> It also looks like you're messing with MAC registers in the PCS code,
> setting the MAC speed there. Are the PCS and MAC so integrated together
> that abstracting the PCS into its own separate code block leads to
> problems?

Agreed, Since our current hardware configuration lacks AN and PHY, I've 
relocated the ENABLE_HS_MAC configuration into PCS to
allow speed changes using ethtool. When more hardware with a PHY that 
supports AN becomes available,
the phylink will invoke macb_mac_config() with the communicated speed 
(phylinkstate->speed).
It is possible to make ENABLE_HS_MAC conditional based on speed.

Currently, for fixed-link, will keep the earlier implementation.

Please let me know your thoughts and comments.

-- 
🙏 vineeth

Re: [RFC PATCH net-next 4/5] net: macb: Configure High Speed Mac for given speed.

[Russell King (Oracle)]

On Thu, Oct 10, 2024 at 07:39:16PM +0530, Karumanchi, Vineeth wrote:
> Hi Russel,
> 
> On 10/9/2024 2:49 PM, Russell King (Oracle) wrote:
> > It also looks like you're messing with MAC registers in the PCS code,
> > setting the MAC speed there. Are the PCS and MAC so integrated together
> > that abstracting the PCS into its own separate code block leads to
> > problems?
> 
> Agreed, Since our current hardware configuration lacks AN and PHY, I've
> relocated the ENABLE_HS_MAC configuration into PCS to
> allow speed changes using ethtool. When more hardware with a PHY that
> supports AN becomes available,
> the phylink will invoke macb_mac_config() with the communicated speed
> (phylinkstate->speed).

Where are you getting that idea from, because that has not been true for
a good number of years - and it's been stated in the phylink
documentation for a very long time.

I've killed all the code references to ->speed in all mac_config()
implementations, and I've even gone to the extent of now ensuring that
all mac_config() methods will _always_ be called with state->speed
set to SPEED_UNKNOWN, so no one can make any useful determinations
from that.

If people continue to insist on using this, then I'll have no option
but to make a disruptive API change, making mac_config() take an
explicit set of arguments for the items that it should have access
to.

> Currently, for fixed-link, will keep the earlier implementation.

I want phylink users to be correct and easy to understand - because
I maintain phylink, and that means I need to understand the code
that makes use of its facilities. So, want to see phylink methods
implemented properly. If they aren't going to be implemented
properly, then I will ask that the driver ceases to use phylink
quite simply because it makes _my_ maintenance more difficult
when drivers don't implement phylink methods correctly.

The choice is: implement phylink methods properly or don't use
phylink.

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

[RFC PATCH net-next 5/5] net: macb: Get speed and link status runtime.

[Vineeth Karumanchi]

- Update speed value by reading HS_SPEED_MAC register.
- Update link status by reading NSR_LINK bit of NSR register
  for slower speeds (1G and 2.5G) and USX_BLOCK_LOCK bit
  of USX_STATUS register for higher speeds.

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 7beb775a0bd7..48ba19a76418 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -624,14 +624,21 @@ static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
 				   struct phylink_link_state *state)
 {
 	struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
+	u32 hs_mac_map[] = {SPEED_UNKNOWN, SPEED_1000, SPEED_2500,
+			    SPEED_5000, SPEED_10000};
 	u32 val;
 
-	state->speed = SPEED_10000;
 	state->duplex = 1;
 	state->an_complete = 1;
 
-	val = gem_readl(bp, USX_STATUS);
-	state->link = !!(val & GEM_BIT(USX_BLOCK_LOCK));
+	val = gem_readl(bp, HS_MAC_CONFIG);
+	val = GEM_BFEXT(HS_MAC_SPEED, val);
+	state->speed = hs_mac_map[val];
+
+	state->link = (state->speed < SPEED_5000) ?
+		!!(macb_readl(bp, NSR) & MACB_BIT(NSR_LINK)) :
+		!!(gem_readl(bp, USX_STATUS) & GEM_BIT(USX_BLOCK_LOCK));
+
 	val = gem_readl(bp, NCFGR);
 	if (val & GEM_BIT(PAE))
 		state->pause = MLO_PAUSE_RX;
-- 
2.34.1