diff mbox series

[net-next,4/4] net: ftgmac100: add RGMII delay for AST2600

Message ID 20250317025922.1526937-5-jacky_chou@aspeedtech.com (mailing list archive)
State New
Headers show
Series Add AST2600 RGMII delay into ftgmac100 | expand

Commit Message

Jacky Chou March 17, 2025, 2:59 a.m. UTC 
Use rx-internal-delay-ps and tx-internal-delay-ps
properties to configue the RGMII delay into corresponding register of
scu. Currently, the ftgmac100 driver only configures on AST2600 and will
be by pass the other platforms.
The details are in faraday,ftgmac100.yaml.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 88 ++++++++++++++++++++++++
 drivers/net/ethernet/faraday/ftgmac100.h | 12 ++++
 2 files changed, 100 insertions(+)

Comments

Maxime Chevallier March 17, 2025, 8:53 a.m. UTC | #1 
Hi,

On Mon, 17 Mar 2025 10:59:22 +0800
Jacky Chou <jacky_chou@aspeedtech.com> wrote:

> Use rx-internal-delay-ps and tx-internal-delay-ps
> properties to configue the RGMII delay into corresponding register of
> scu. Currently, the ftgmac100 driver only configures on AST2600 and will
> be by pass the other platforms.
> The details are in faraday,ftgmac100.yaml.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  drivers/net/ethernet/faraday/ftgmac100.c | 88 ++++++++++++++++++++++++
>  drivers/net/ethernet/faraday/ftgmac100.h | 12 ++++
>  2 files changed, 100 insertions(+)
> 
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 17ec35e75a65..ea2061488cba 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -27,6 +27,9 @@
>  #include <linux/phy_fixed.h>
>  #include <net/ip.h>
>  #include <net/ncsi.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/bitfield.h>
>  
>  #include "ftgmac100.h"
>  
> @@ -1812,6 +1815,88 @@ static bool ftgmac100_has_child_node(struct device_node *np, const char *name)
>  	return ret;
>  }
>  
> +static void ftgmac100_set_internal_delay(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct net_device *netdev;
> +	struct ftgmac100 *priv;
> +	struct regmap *scu;
> +	u32 rgmii_tx_delay, rgmii_rx_delay;
> +	u32 dly_reg, tx_dly_mask, rx_dly_mask;
> +	int tx, rx;

Please use the reverse christmas tree notation, sorting declarations by
descending line length

> +	netdev = platform_get_drvdata(pdev);
> +	priv = netdev_priv(netdev);
> +
> +	tx = of_property_read_u32(np, "tx-internal-delay-ps", &rgmii_tx_delay);
> +	rx = of_property_read_u32(np, "rx-internal-delay-ps", &rgmii_rx_delay);
> +
> +	if (of_device_is_compatible(np, "aspeed,ast2600-mac")) {
> +		/* According to mac base address to get mac index */
> +		switch (priv->res->start) {
> +		case 0x1e660000:
> +			dly_reg = AST2600_MAC12_CLK_DLY;
> +			tx_dly_mask = AST2600_MAC1_TX_DLY;
> +			rx_dly_mask = AST2600_MAC1_RX_DLY;
> +			rgmii_tx_delay = FIELD_PREP(AST2600_MAC1_TX_DLY, rgmii_tx_delay);
> +			rgmii_rx_delay = FIELD_PREP(AST2600_MAC1_RX_DLY, rgmii_rx_delay);
> +			break;
> +		case 0x1e680000:
> +			dly_reg = AST2600_MAC12_CLK_DLY;
> +			tx_dly_mask = AST2600_MAC2_TX_DLY;
> +			rx_dly_mask = AST2600_MAC2_RX_DLY;
> +			rgmii_tx_delay = FIELD_PREP(AST2600_MAC2_TX_DLY, rgmii_tx_delay);
> +			rgmii_rx_delay = FIELD_PREP(AST2600_MAC2_RX_DLY, rgmii_rx_delay);
> +			break;
> +		case 0x1e670000:
> +			dly_reg = AST2600_MAC34_CLK_DLY;
> +			tx_dly_mask = AST2600_MAC3_TX_DLY;
> +			rx_dly_mask = AST2600_MAC3_RX_DLY;
> +			rgmii_tx_delay = FIELD_PREP(AST2600_MAC3_TX_DLY, rgmii_tx_delay);
> +			rgmii_rx_delay = FIELD_PREP(AST2600_MAC3_RX_DLY, rgmii_rx_delay);
> +			break;
> +		case 0x1e690000:
> +			dly_reg = AST2600_MAC34_CLK_DLY;
> +			tx_dly_mask = AST2600_MAC4_TX_DLY;
> +			rx_dly_mask = AST2600_MAC4_RX_DLY;
> +			rgmii_tx_delay = FIELD_PREP(AST2600_MAC4_TX_DLY, rgmii_tx_delay);
> +			rgmii_rx_delay = FIELD_PREP(AST2600_MAC4_RX_DLY, rgmii_rx_delay);
> +			break;
> +		default:
> +			dev_warn(&pdev->dev, "Invalid mac base address");
> +			return;

There has to be a better way that directly looking up the base address.
Maybe you need an extra DT property ?

> +		}
> +	} else {
> +		return;
> +	}
> +
> +	scu = syscon_regmap_lookup_by_phandle(np, "scu");
> +	if (IS_ERR(scu)) {
> +		dev_warn(&pdev->dev, "failed to map scu base");
> +		return;
> +	}
> +
> +	if (!tx) {
> +		/* Use tx-internal-delay-ps as index to configure tx delay
> +		 * into scu register.
> +		 */

So this goes completely against the naming of the property. It has the
-ps suffix, so you would expect to have picoseconds values passed, and
not an arbiraty index.

Take a look at other drivers, you should accept picseconds values from
these properties, then compute the relevant index in the driver. That
index should be something internal to your driver.

An example here :

https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_rgmii.c#L51

> +		if (rgmii_tx_delay > 64)
> +			dev_warn(&pdev->dev, "Get invalid tx delay value");
> +		else
> +			regmap_update_bits(scu, dly_reg, tx_dly_mask, rgmii_tx_delay);
> +	}
> +
> +	if (!rx) {
> +		/* Use rx-internal-delay-ps as index to configure rx delay
> +		 * into scu register.
> +		 */
> +		if (rgmii_tx_delay > 64)
> +			dev_warn(&pdev->dev, "Get invalid rx delay value");
> +		else
> +			regmap_update_bits(scu, dly_reg, rx_dly_mask, rgmii_rx_delay);
> +	}
> +}
> +
>  static int ftgmac100_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
> @@ -1977,6 +2062,9 @@ static int ftgmac100_probe(struct platform_device *pdev)
>  		if (of_device_is_compatible(np, "aspeed,ast2600-mac"))
>  			iowrite32(FTGMAC100_TM_DEFAULT,
>  				  priv->base + FTGMAC100_OFFSET_TM);
> +
> +		/* Configure RGMII delay if there are the corresponding properties */
> +		ftgmac100_set_internal_delay(pdev);
>  	}
>  
>  	/* Default ring sizes */
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.h b/drivers/net/ethernet/faraday/ftgmac100.h
> index 4968f6f0bdbc..d464d287502c 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.h
> +++ b/drivers/net/ethernet/faraday/ftgmac100.h
> @@ -271,4 +271,16 @@ struct ftgmac100_rxdes {
>  #define FTGMAC100_RXDES1_UDP_CHKSUM_ERR	(1 << 26)
>  #define FTGMAC100_RXDES1_IP_CHKSUM_ERR	(1 << 27)
>  
> +/* Aspeed SCU */
> +#define AST2600_MAC12_CLK_DLY	0x340
> +#define AST2600_MAC1_TX_DLY		GENMASK(5, 0)
> +#define AST2600_MAC1_RX_DLY		GENMASK(17, 12)
> +#define AST2600_MAC2_TX_DLY		GENMASK(11, 6)
> +#define AST2600_MAC2_RX_DLY		GENMASK(23, 18)
> +#define AST2600_MAC34_CLK_DLY	0x350
> +#define AST2600_MAC3_TX_DLY		AST2600_MAC1_TX_DLY
> +#define AST2600_MAC3_RX_DLY		AST2600_MAC1_RX_DLY
> +#define AST2600_MAC4_TX_DLY		AST2600_MAC2_TX_DLY
> +#define AST2600_MAC4_RX_DLY		AST2600_MAC2_RX_DLY
> +
>  #endif /* __FTGMAC100_H */

Maxime
Russell King (Oracle) March 17, 2025, 11:09 a.m. UTC | #2 
On Mon, Mar 17, 2025 at 09:53:33AM +0100, Maxime Chevallier wrote:
> So this goes completely against the naming of the property. It has the
> -ps suffix, so you would expect to have picoseconds values passed, and
> not an arbiraty index.
> 
> Take a look at other drivers, you should accept picseconds values from
> these properties, then compute the relevant index in the driver. That
> index should be something internal to your driver.
> 
> An example here :
> 
> https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_rgmii.c#L51

Another example would be drivers/net/phy/adin.c::adin_get_reg_value()
and associated functions - these lookup a DT property and then look
that up in a table to convert it to a register value.

I suspect that's something which could become generic, as I suspect
most hardware isn't going to accept a picosecond value, but be a
choice of N different options.
Andrew Lunn March 17, 2025, 12:52 p.m. UTC | #3 
> The RGMII delay of AST2600 has a lot of steps can be configured.

Are they uniformly space? Then it should be a simple formula to
calculate? Or a lookup table?

	Andrew
Andrew Lunn March 17, 2025, 1:03 p.m. UTC | #4 
> +	u32 rgmii_tx_delay, rgmii_rx_delay;
> +	u32 dly_reg, tx_dly_mask, rx_dly_mask;
> +	int tx, rx;
> +
> +	netdev = platform_get_drvdata(pdev);
> +	priv = netdev_priv(netdev);
> +
> +	tx = of_property_read_u32(np, "tx-internal-delay-ps", &rgmii_tx_delay);
> +	rx = of_property_read_u32(np, "rx-internal-delay-ps", &rgmii_rx_delay);

> +	if (!tx) {

The documentation for of_property_read_u32() says:

 * Return: 0 on success, -EINVAL if the property does not exist,
 * -ENODATA if property does not have a value, and -EOVERFLOW if the
 * property data isn't large enough.

You need to handle EINVAL different to the other errors, which are
real errors and should fail the probe.

The commit message, and probably the binding needs to document what
happens when the properties are not in the DT blob. This needs to be
part of the bigger picture of how you are going to sort out the mess
with existing .dts files listing 'rgmii' when in fact they should be
'rgmii-id'.

> +		/* Use tx-internal-delay-ps as index to configure tx delay
> +		 * into scu register.
> +		 */
> +		if (rgmii_tx_delay > 64)
> +			dev_warn(&pdev->dev, "Get invalid tx delay value");

Return EINVAL and fail the probe.

	Andrew
diff mbox series

Patch

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 17ec35e75a65..ea2061488cba 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -27,6 +27,9 @@ 
 #include <linux/phy_fixed.h>
 #include <net/ip.h>
 #include <net/ncsi.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/bitfield.h>
 
 #include "ftgmac100.h"
 
@@ -1812,6 +1815,88 @@  static bool ftgmac100_has_child_node(struct device_node *np, const char *name)
 	return ret;
 }
 
+static void ftgmac100_set_internal_delay(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct net_device *netdev;
+	struct ftgmac100 *priv;
+	struct regmap *scu;
+	u32 rgmii_tx_delay, rgmii_rx_delay;
+	u32 dly_reg, tx_dly_mask, rx_dly_mask;
+	int tx, rx;
+
+	netdev = platform_get_drvdata(pdev);
+	priv = netdev_priv(netdev);
+
+	tx = of_property_read_u32(np, "tx-internal-delay-ps", &rgmii_tx_delay);
+	rx = of_property_read_u32(np, "rx-internal-delay-ps", &rgmii_rx_delay);
+
+	if (of_device_is_compatible(np, "aspeed,ast2600-mac")) {
+		/* According to mac base address to get mac index */
+		switch (priv->res->start) {
+		case 0x1e660000:
+			dly_reg = AST2600_MAC12_CLK_DLY;
+			tx_dly_mask = AST2600_MAC1_TX_DLY;
+			rx_dly_mask = AST2600_MAC1_RX_DLY;
+			rgmii_tx_delay = FIELD_PREP(AST2600_MAC1_TX_DLY, rgmii_tx_delay);
+			rgmii_rx_delay = FIELD_PREP(AST2600_MAC1_RX_DLY, rgmii_rx_delay);
+			break;
+		case 0x1e680000:
+			dly_reg = AST2600_MAC12_CLK_DLY;
+			tx_dly_mask = AST2600_MAC2_TX_DLY;
+			rx_dly_mask = AST2600_MAC2_RX_DLY;
+			rgmii_tx_delay = FIELD_PREP(AST2600_MAC2_TX_DLY, rgmii_tx_delay);
+			rgmii_rx_delay = FIELD_PREP(AST2600_MAC2_RX_DLY, rgmii_rx_delay);
+			break;
+		case 0x1e670000:
+			dly_reg = AST2600_MAC34_CLK_DLY;
+			tx_dly_mask = AST2600_MAC3_TX_DLY;
+			rx_dly_mask = AST2600_MAC3_RX_DLY;
+			rgmii_tx_delay = FIELD_PREP(AST2600_MAC3_TX_DLY, rgmii_tx_delay);
+			rgmii_rx_delay = FIELD_PREP(AST2600_MAC3_RX_DLY, rgmii_rx_delay);
+			break;
+		case 0x1e690000:
+			dly_reg = AST2600_MAC34_CLK_DLY;
+			tx_dly_mask = AST2600_MAC4_TX_DLY;
+			rx_dly_mask = AST2600_MAC4_RX_DLY;
+			rgmii_tx_delay = FIELD_PREP(AST2600_MAC4_TX_DLY, rgmii_tx_delay);
+			rgmii_rx_delay = FIELD_PREP(AST2600_MAC4_RX_DLY, rgmii_rx_delay);
+			break;
+		default:
+			dev_warn(&pdev->dev, "Invalid mac base address");
+			return;
+		}
+	} else {
+		return;
+	}
+
+	scu = syscon_regmap_lookup_by_phandle(np, "scu");
+	if (IS_ERR(scu)) {
+		dev_warn(&pdev->dev, "failed to map scu base");
+		return;
+	}
+
+	if (!tx) {
+		/* Use tx-internal-delay-ps as index to configure tx delay
+		 * into scu register.
+		 */
+		if (rgmii_tx_delay > 64)
+			dev_warn(&pdev->dev, "Get invalid tx delay value");
+		else
+			regmap_update_bits(scu, dly_reg, tx_dly_mask, rgmii_tx_delay);
+	}
+
+	if (!rx) {
+		/* Use rx-internal-delay-ps as index to configure rx delay
+		 * into scu register.
+		 */
+		if (rgmii_tx_delay > 64)
+			dev_warn(&pdev->dev, "Get invalid rx delay value");
+		else
+			regmap_update_bits(scu, dly_reg, rx_dly_mask, rgmii_rx_delay);
+	}
+}
+
 static int ftgmac100_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -1977,6 +2062,9 @@  static int ftgmac100_probe(struct platform_device *pdev)
 		if (of_device_is_compatible(np, "aspeed,ast2600-mac"))
 			iowrite32(FTGMAC100_TM_DEFAULT,
 				  priv->base + FTGMAC100_OFFSET_TM);
+
+		/* Configure RGMII delay if there are the corresponding properties */
+		ftgmac100_set_internal_delay(pdev);
 	}
 
 	/* Default ring sizes */
diff --git a/drivers/net/ethernet/faraday/ftgmac100.h b/drivers/net/ethernet/faraday/ftgmac100.h
index 4968f6f0bdbc..d464d287502c 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.h
+++ b/drivers/net/ethernet/faraday/ftgmac100.h
@@ -271,4 +271,16 @@  struct ftgmac100_rxdes {
 #define FTGMAC100_RXDES1_UDP_CHKSUM_ERR	(1 << 26)
 #define FTGMAC100_RXDES1_IP_CHKSUM_ERR	(1 << 27)
 
+/* Aspeed SCU */
+#define AST2600_MAC12_CLK_DLY	0x340
+#define AST2600_MAC1_TX_DLY		GENMASK(5, 0)
+#define AST2600_MAC1_RX_DLY		GENMASK(17, 12)
+#define AST2600_MAC2_TX_DLY		GENMASK(11, 6)
+#define AST2600_MAC2_RX_DLY		GENMASK(23, 18)
+#define AST2600_MAC34_CLK_DLY	0x350
+#define AST2600_MAC3_TX_DLY		AST2600_MAC1_TX_DLY
+#define AST2600_MAC3_RX_DLY		AST2600_MAC1_RX_DLY
+#define AST2600_MAC4_TX_DLY		AST2600_MAC2_TX_DLY
+#define AST2600_MAC4_RX_DLY		AST2600_MAC2_RX_DLY
+
 #endif /* __FTGMAC100_H */