[net-next 0/4] Add AST2600 RGMII delay into ftgmac100

[net-next 0/4] Add AST2600 RGMII delay into ftgmac100

[Jacky Chou]

In AST2600, the RGMII delay is configured in SCU.
Add the properties according ethernet-controller.yaml into AST2600 dts
and dtsi, and add these in the ftgmac100 driver and yaml to configure and
describe how to use.
Now, just support for AST2600 and the other platforms will be ignored
the RGMII delay setting according to compatible.

Jacky Chou (4):
  ARM: dts: aspeed-g6:add scu to mac for RGMII delay
  ARM: dts: ast2600-evb: add default RGMII delay
  dt-bindings: net: ftgmac100: add rgmii delay properties
  net: ftgmac100: add RGMII delay for AST2600

 .../bindings/net/faraday,ftgmac100.yaml       | 16 +++-
 .../boot/dts/aspeed/aspeed-ast2600-evb.dts    | 12 +++
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi       |  4 +
 drivers/net/ethernet/faraday/ftgmac100.c      | 88 +++++++++++++++++++
 drivers/net/ethernet/faraday/ftgmac100.h      | 12 +++
 5 files changed, 131 insertions(+), 1 deletion(-)

-- 
2.34.1

[net-next 1/4] ARM: dts: aspeed-g6:add scu to mac for RGMII delay

[Jacky Chou]

The RGMII delay of AST2600 MAC is configured in SCU
register, so add scu regmap into mac node.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 8ed715bd53aa..17e979d616dc 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -236,6 +236,7 @@ mac0: ethernet@1e660000 {
 			reg = <0x1e660000 0x180>;
 			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&syscon ASPEED_CLK_GATE_MAC1CLK>;
+			scu = <&syscon>;
 			status = "disabled";
 		};
 
@@ -244,6 +245,7 @@ mac1: ethernet@1e680000 {
 			reg = <0x1e680000 0x180>;
 			interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&syscon ASPEED_CLK_GATE_MAC2CLK>;
+			scu = <&syscon>;
 			status = "disabled";
 		};
 
@@ -252,6 +254,7 @@ mac2: ethernet@1e670000 {
 			reg = <0x1e670000 0x180>;
 			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&syscon ASPEED_CLK_GATE_MAC3CLK>;
+			scu = <&syscon>;
 			status = "disabled";
 		};
 
@@ -260,6 +263,7 @@ mac3: ethernet@1e690000 {
 			reg = <0x1e690000 0x180>;
 			interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&syscon ASPEED_CLK_GATE_MAC4CLK>;
+			scu = <&syscon>;
 			status = "disabled";
 		};
 
-- 
2.34.1

[net-next 2/4] ARM: dts: ast2600-evb: add default RGMII delay

[Jacky Chou]

Use tx-internal-delay-ps and rx-internal-delay-ps to
configure the RGMII delay on MAC.
And add default value for AST2600 MAC in dts.
Refer to faraday,ftgmac100yaml to know how to configure
the RGMII delay.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
index de83c0eb1d6e..1db1f2a02d91 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
@@ -126,6 +126,9 @@ &mac0 {
 	phy-mode = "rgmii-rxid";
 	phy-handle = <&ethphy0>;
 
+	tx-internal-delay-ps = <16>;
+	rx-internal-delay-ps = <10>;
+
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_rgmii1_default>;
 };
@@ -137,6 +140,9 @@ &mac1 {
 	phy-mode = "rgmii-rxid";
 	phy-handle = <&ethphy1>;
 
+	tx-internal-delay-ps = <16>;
+	rx-internal-delay-ps = <10>;
+
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_rgmii2_default>;
 };
@@ -147,6 +153,9 @@ &mac2 {
 	phy-mode = "rgmii";
 	phy-handle = <&ethphy2>;
 
+	tx-internal-delay-ps = <8>;
+	rx-internal-delay-ps = <4>;
+
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_rgmii3_default>;
 };
@@ -157,6 +166,9 @@ &mac3 {
 	phy-mode = "rgmii";
 	phy-handle = <&ethphy3>;
 
+	tx-internal-delay-ps = <8>;
+	rx-internal-delay-ps = <4>;
+
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_rgmii4_default>;
 };
-- 
2.34.1

[net-next 3/4] dt-bindings: net: ftgmac100: add rgmii delay properties

[Jacky Chou]

Add tx-internal-delay-ps and rx-internal-delay-ps to
configure the RGMII delay for MAC. According to
ethernet-controller.yaml, they use for RGMII TX and RX delay.

In Aspeed desgin, the RGMII delay is a number of ps as unit to
set delay, do not use one ps as unit. The values are different
from each MAC. So, here describes the property values
as index to configure corresponding scu register.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 .../bindings/net/faraday,ftgmac100.yaml          | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
index 55d6a8379025..c5904aa84e05 100644
--- a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
+++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
@@ -66,6 +66,20 @@ properties:
     type: boolean
     deprecated: true
 
+  rx-internal-delay-ps:
+    description:
+       Setting this property to a non-zero number sets the RX internal delay
+       for the MAC. Use this property value as a index not a ps unit to
+       configure the corresponding delay register field. And the index range is
+       0 to 63.
+
+  tx-internal-delay-ps:
+    description:
+       Setting this property to a non-zero number sets the TX internal delay
+       for the MAC. Use this property value as a index not a ps unit to
+       configure the corresponding delay register field. And the index range is
+       0 to 63.
+
   mdio:
     $ref: /schemas/net/mdio.yaml#
 
@@ -102,4 +116,4 @@ examples:
                 reg = <1>;
             };
         };
-    };
+    };
\ No newline at end of file
-- 
2.34.1

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

[Jacky Chou]

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;
+
+	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 */
-- 
2.34.1

Re: [net-next 3/4] dt-bindings: net: ftgmac100: add rgmii delay properties

[Rob Herring (Arm)]

On Mon, 17 Mar 2025 10:59:21 +0800, Jacky Chou wrote:
> Add tx-internal-delay-ps and rx-internal-delay-ps to
> configure the RGMII delay for MAC. According to
> ethernet-controller.yaml, they use for RGMII TX and RX delay.
> 
> In Aspeed desgin, the RGMII delay is a number of ps as unit to
> set delay, do not use one ps as unit. The values are different
> from each MAC. So, here describes the property values
> as index to configure corresponding scu register.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  .../bindings/net/faraday,ftgmac100.yaml          | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml:71:8: [warning] wrong indentation: expected 6 but found 7 (indentation)
./Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml:78:8: [warning] wrong indentation: expected 6 but found 7 (indentation)
./Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml:119:7: [error] no new line character at the end of file (new-line-at-end-of-file)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250317025922.1526937-4-jacky_chou@aspeedtech.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [net-next 3/4] dt-bindings: net: ftgmac100: add rgmii delay properties

[Krzysztof Kozlowski]

On 17/03/2025 03:59, Jacky Chou wrote:
> Add tx-internal-delay-ps and rx-internal-delay-ps to

Please wrap code according to the preferred limit expressed in Kernel
coding style (checkpatch is not a coding style description, but only a
tool).  However don't wrap blindly (see Kernel coding style).

> configure the RGMII delay for MAC. According to
> ethernet-controller.yaml, they use for RGMII TX and RX delay.
> 
> In Aspeed desgin, the RGMII delay is a number of ps as unit to
> set delay, do not use one ps as unit. The values are different
> from each MAC. So, here describes the property values
> as index to configure corresponding scu register.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---

...

>    mdio:
>      $ref: /schemas/net/mdio.yaml#
>  
> @@ -102,4 +116,4 @@ examples:
>                  reg = <1>;
>              };
>          };
> -    };
> +    };
> \ No newline at end of file


This was neither tested nor reviewed by you before sending.


Best regards,
Krzysztof

回覆: [net-next 3/4] dt-bindings: net: ftgmac100: add rgmii delay properties

[Jacky Chou]

Hi Krzysztof
> > Add tx-internal-delay-ps and rx-internal-delay-ps to
> 
> Please wrap code according to the preferred limit expressed in Kernel coding
> style (checkpatch is not a coding style description, but only a tool).  However
> don't wrap blindly (see Kernel coding style).
> >    mdio:
> >      $ref: /schemas/net/mdio.yaml#
> >
> > @@ -102,4 +116,4 @@ examples:
> >                  reg = <1>;
> >              };
> >          };
> > -    };
> > +    };
> > \ No newline at end of file
> 
> 
> This was neither tested nor reviewed by you before sending.
> 

Sorry. I will notice and confirm all of patches in next version.
Thank you for your reminder.

Jacky

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

[Maxime Chevallier]

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

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

[Jacky Chou]

> > +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

Got it. I will modify these in next version.

> > +	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 ?

I use the base address to identify the MAC index because it is the only fixed value 
that is paired with the MAC.
If I create a property to identify the MAC index and someone use the wrong value, 
the driver will configure the wrong register field.
Therefore, I use the base address as MAC index, and it is very clear which MAC is 
configured.

> 
> > +		}
> > +	} 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/microchi
> p/sparx5/lan969x/lan969x_rgmii.c#L51
> 

Agreed. Thank you for your information.
I will adjust this part to use ps unit to translate to the relevant index in next version.

Jacky

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

[Russell King (Oracle)]

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.

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

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

[Jacky Chou]

> > 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.

Thank you for your information.
The RGMII delay of AST2600 has a lot of steps can be configured.
And the delay value of each MAC has different.
Each MAC has 64 steps to configure RGMII TX/RX delay.
Therefore, I use these properties as index to configure the register to reduce 
computing in driver.
Or I do not use the property that defined in ethernet-controller.yaml, I 
create the property that defined by myself?

Thanks,
Jacky

Re: [net-next 3/4] dt-bindings: net: ftgmac100: add rgmii delay properties

[Andrew Lunn]

> In Aspeed desgin, the RGMII delay is a number of ps as unit to
> set delay, do not use one ps as unit. The values are different
> from each MAC. So, here describes the property values
> as index to configure corresponding scu register.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  .../bindings/net/faraday,ftgmac100.yaml          | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> index 55d6a8379025..c5904aa84e05 100644
> --- a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> +++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> @@ -66,6 +66,20 @@ properties:
>      type: boolean
>      deprecated: true
>  
> +  rx-internal-delay-ps:
> +    description:
> +       Setting this property to a non-zero number sets the RX internal delay
> +       for the MAC. Use this property value as a index not a ps unit to
> +       configure the corresponding delay register field. And the index range is
> +       0 to 63.

You have to use picoseconds here. As with all DT binding, you use SI
units, and the driver then converts them to whatever value you need to
poke into the register.
	
    Andrew

---
pw-bot: cr

Re: [net-next 1/4] ARM: dts: aspeed-g6:add scu to mac for RGMII delay

[Andrew Lunn]

On Mon, Mar 17, 2025 at 10:59:19AM +0800, Jacky Chou wrote:
> The RGMII delay of AST2600 MAC is configured in SCU
> register, so add scu regmap into mac node.

Don't you need to add the scu to the binding?

	Andrew

Re: 回覆: [net-next 4/4] net: ftgmac100: add RGMII delay for AST2600

[Andrew Lunn]

> 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

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

[Andrew Lunn]

> +	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

Re: [net-next 2/4] ARM: dts: ast2600-evb: add default RGMII delay

[Andrew Lunn]

> @@ -147,6 +153,9 @@ &mac2 {
>  	phy-mode = "rgmii";
>  	phy-handle = <&ethphy2>;
>  
> +	tx-internal-delay-ps = <8>;
> +	rx-internal-delay-ps = <4>;
> +

Ideally you want:

	phy-mode = "rgmii-id";
	tx-internal-delay-ps = <0>;
	rx-internal-delay-ps = <0>;

Since 'rgmii-id' correctly describes the hardware.

	Andrew

Re: [net-next 3/4] dt-bindings: net: ftgmac100: add rgmii delay properties

[Krzysztof Kozlowski]

On 17/03/2025 13:44, Andrew Lunn wrote:
>> diff --git a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
>> index 55d6a8379025..c5904aa84e05 100644
>> --- a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
>> +++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
>> @@ -66,6 +66,20 @@ properties:
>>      type: boolean
>>      deprecated: true
>>  
>> +  rx-internal-delay-ps:
>> +    description:
>> +       Setting this property to a non-zero number sets the RX internal delay
>> +       for the MAC. Use this property value as a index not a ps unit to
>> +       configure the corresponding delay register field. And the index range is
>> +       0 to 63.
> 
> You have to use picoseconds here. As with all DT binding, you use SI
> units, and the driver then converts them to whatever value you need to
> poke into the register.
Ykes, I did notice that when skimming through the patch. Such a sneaky
way to pretend you conform to the bindings but eh, not really, I will do
it my way.

I think reviewing Aspeed code takes a lot, a lot of our time. It's not
only about this patchset but several others.

Maybe it is time for Aspeed to perform intensive internal review, before
they post to the mailing lists? Several other companies do it, more or less.

Best regards,
Krzysztof

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

[Jacky Chou]

Hi Andrew,

Thank you for your reply.

> > 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?

There are fixed delay values by step. I list below.
AST2600 MAC0/1 one step delay = 45 ps
AST2600 MAC2/3 one step delay = 250 ps
I calculate all step and emulate them.
The dt-binding will be like below.
rx-internal-delay-ps:
    description:
      Setting this property to a non-zero number sets the RX internal delay
      for the MAC. ... skip ...
    enum:
      [45, 90, 135, 180, 225, 250, 270, 315, 360, 405, 450, 495, 500, 540, 585, 630, 675, 
       720, 750, 765, 810, 855, 900, 945, 990, 1000, 1035, 1080, 1125, 1170, 1215, 1250, 
       1260, 1305, 1350, 1395, 1440, 1500, 1750, 2000, 2250, 2500, 2750, 3000, 3250, 3500, 
       3750, 4000, 4250, 4500, 4750, 5000, 5250, 5500, 5750, 6000, 6250, 6500, 6750, 7000, 
       7250, 7500, 7750, 8000]

I will modify using calculating delay value in next version.

Thanks,
Jacky

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

[Jacky Chou]

Hi Andrew,

Thank you for your reply.

> > +	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'.

Why can't the MAC add internal delay to RGMII? Is it necessary to add on PHY side?

> 
> > +		/* 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.

Agreed.
I just show warning here, because sometimes the RGMII delay value will configure at bootloader.
Therefore, it shows message and ignore the delay value when it got wrong.

I will add check for this in next version.

Thanks,
Jacky

回覆: [net-next 2/4] ARM: dts: ast2600-evb: add default RGMII delay

[Jacky Chou]

Hi Andrew,

Thank you for your reply.

> >  	phy-mode = "rgmii";
> >  	phy-handle = <&ethphy2>;
> >
> > +	tx-internal-delay-ps = <8>;
> > +	rx-internal-delay-ps = <4>;
> > +
> 
> Ideally you want:
> 
> 	phy-mode = "rgmii-id";
> 	tx-internal-delay-ps = <0>;
> 	rx-internal-delay-ps = <0>;
> 
> Since 'rgmii-id' correctly describes the hardware.

I still confuse about ethernet-controller.yaml.
It lists 'rgmi', 'rgmii-rxid', 'rgmii-txid' and 'rgmii-id'.

ethernet-controller.yaml
...
      # RX and TX delays are added by the MAC when required
      - rgmii

      # RGMII with internal RX and TX delays provided by the PHY,
      # the MAC should not add the RX or TX delays in this case
      - rgmii-id

      # RGMII with internal RX delay provided by the PHY, the MAC
      # should not add an RX delay in this case
      - rgmii-rxid

      # RGMII with internal TX delay provided by the PHY, the MAC
      # should not add an TX delay in this case
      - rgmii-txid
...

It seems if MAC has ability to add delay in MAC internal, driver can use these
values to describes the hardware design.

I know this topic had been discussed. I thought for a while to find a solution, but I 
cannot still understand why 'rgmii-id' is correct for HW?

Thanks,
Jacky

回覆: [net-next 1/4] ARM: dts: aspeed-g6:add scu to mac for RGMII delay

[Jacky Chou]

Hi Andrew

Thank you for your reply.

> > The RGMII delay of AST2600 MAC is configured in SCU register, so add
> > scu regmap into mac node.
> 
> Don't you need to add the scu to the binding?

Agreed.
I will add this 'scu' property into dt-binding in next version.

Thanks,
Jacky

Re: 回覆: [net-next 4/4] net: ftgmac100: add RGMII delay for AST2600

[Andrew Lunn]

On Tue, Mar 18, 2025 at 10:46:58AM +0000, Jacky Chou wrote:
> Hi Andrew,
> 
> Thank you for your reply.
> 
> > > +	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'.
> 
> Why can't the MAC add internal delay to RGMII? Is it necessary to add on PHY side?

The MAC could, but that is not the point. You need to explain how you
are going to solve the mess you have in DT, why all aspeed boards have
the wrong phy-mode. You need to fix that, and i will continue to NACK
new boards until the correct rgmii-id value can be used to indicate
there do not have extra long clock lines on the PCB.

> > > +		/* 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.
> 
> Agreed.
> I just show warning here, because sometimes the RGMII delay value will configure at bootloader.

That is a different issue. If there is a value in DT, it must be
valid, fail the probe otherwise.

	Andrew

Re: 回覆: 回覆: [net-next 4/4] net: ftgmac100: add RGMII delay for AST2600

[Andrew Lunn]

On Tue, Mar 18, 2025 at 05:34:08AM +0000, Jacky Chou wrote:
> Hi Andrew,
> 
> Thank you for your reply.
> 
> > > 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?
> 
> There are fixed delay values by step. I list below.
> AST2600 MAC0/1 one step delay = 45 ps
> AST2600 MAC2/3 one step delay = 250 ps

That is messy.

> I calculate all step and emulate them.
> The dt-binding will be like below.
> rx-internal-delay-ps:
>     description:
>       Setting this property to a non-zero number sets the RX internal delay
>       for the MAC. ... skip ...
>     enum:
>       [45, 90, 135, 180, 225, 250, 270, 315, 360, 405, 450, 495, 500, 540, 585, 630, 675, 
>        720, 750, 765, 810, 855, 900, 945, 990, 1000, 1035, 1080, 1125, 1170, 1215, 1250, 
>        1260, 1305, 1350, 1395, 1440, 1500, 1750, 2000, 2250, 2500, 2750, 3000, 3250, 3500, 
>        3750, 4000, 4250, 4500, 4750, 5000, 5250, 5500, 5750, 6000, 6250, 6500, 6750, 7000, 
>        7250, 7500, 7750, 8000]

Can the hardware do 0 ps?

So this list is a superset of both 45ps and 250ps steps?

Lets see what the DT Maintainers say, but it could be you need two
different compatibles for mac0/1 to mac2/3 because they are not
actually compatible! You can then have a list per compatible.

	Andrew

Re: 回覆: [net-next 2/4] ARM: dts: ast2600-evb: add default RGMII delay

[Andrew Lunn]

On Tue, Mar 18, 2025 at 11:00:27AM +0000, Jacky Chou wrote:
> Hi Andrew,
> 
> Thank you for your reply.
> 
> > >  	phy-mode = "rgmii";
> > >  	phy-handle = <&ethphy2>;
> > >
> > > +	tx-internal-delay-ps = <8>;
> > > +	rx-internal-delay-ps = <4>;
> > > +
> > 
> > Ideally you want:
> > 
> > 	phy-mode = "rgmii-id";
> > 	tx-internal-delay-ps = <0>;
> > 	rx-internal-delay-ps = <0>;
> > 
> > Since 'rgmii-id' correctly describes the hardware.
> 
> I still confuse about ethernet-controller.yaml.
> It lists 'rgmi', 'rgmii-rxid', 'rgmii-txid' and 'rgmii-id'.

DT describes the board. Does the board add the 2ns delay via extra
long clock lines? If yes, use rgmii. If the MAC/PHY pair need to add
the 2ns delay, use rgmii-id.

If the MAC/PHY pair is adding the delay, the DT says nothing about how
they add the delay.

The general rule is the PHY adds the delay. If you look at
drivers/net/phy/*.c, every PHY that implements RGMII support both
PHY_INTERFACE_MODE_RGMII_ID and PHY_INTERFACE_MODE_RGMII. There is no
reason not to follow ever other MAC/PHY pair and have the PHY add the
delay. The MAC can then do fine tuning if needed, adding small delays.

	Andrew

Re: 回覆: 回覆: [net-next 4/4] net: ftgmac100: add RGMII delay for AST2600

[Krzysztof Kozlowski]

On 18/03/2025 14:51, Andrew Lunn wrote:
> On Tue, Mar 18, 2025 at 05:34:08AM +0000, Jacky Chou wrote:
>> Hi Andrew,
>>
>> Thank you for your reply.
>>
>>>> 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?
>>
>> There are fixed delay values by step. I list below.
>> AST2600 MAC0/1 one step delay = 45 ps
>> AST2600 MAC2/3 one step delay = 250 ps
> 
> That is messy.
> 
>> I calculate all step and emulate them.
>> The dt-binding will be like below.
>> rx-internal-delay-ps:
>>     description:
>>       Setting this property to a non-zero number sets the RX internal delay
>>       for the MAC. ... skip ...
>>     enum:
>>       [45, 90, 135, 180, 225, 250, 270, 315, 360, 405, 450, 495, 500, 540, 585, 630, 675, 
>>        720, 750, 765, 810, 855, 900, 945, 990, 1000, 1035, 1080, 1125, 1170, 1215, 1250, 
>>        1260, 1305, 1350, 1395, 1440, 1500, 1750, 2000, 2250, 2500, 2750, 3000, 3250, 3500, 
>>        3750, 4000, 4250, 4500, 4750, 5000, 5250, 5500, 5750, 6000, 6250, 6500, 6750, 7000, 
>>        7250, 7500, 7750, 8000]
> 
> Can the hardware do 0 ps?
> 
> So this list is a superset of both 45ps and 250ps steps?

git grep multipleOf:

e.g.
oneOf:
 - minimum: 45
   maximum: ...
   multipleOf: 45
 - minimum: 1500
   maximum: ...
   multipleOf: 250

> 
> Lets see what the DT Maintainers say, but it could be you need two
> different compatibles for mac0/1 to mac2/3 because they are not
> actually compatible! You can then have a list per compatible.
If this is the only, *only* difference, then just go with vendor
property matching register value... but oh, wait, how person reading and
writing the DTS would understand if "0x2" means 90 ps or 1750 ps? I
don't see how the original binding was helping here in total. Just
moving the burden from driver developer to DTS developer. :/

If different instances are not the same, means the devices are not the
same, so two compatibles seem reasonable.

Best regards,
Krzysztof

回覆: 回覆: [net-next 2/4] ARM: dts: ast2600-evb: add default RGMII delay

[Jacky Chou]

Hi Andrew,


> DT describes the board. Does the board add the 2ns delay via extra long clock
> lines? If yes, use rgmii. If the MAC/PHY pair need to add the 2ns delay, use
> rgmii-id.
> 
> If the MAC/PHY pair is adding the delay, the DT says nothing about how they
> add the delay.
> 
> The general rule is the PHY adds the delay. If you look at drivers/net/phy/*.c,
> every PHY that implements RGMII support both
> PHY_INTERFACE_MODE_RGMII_ID and PHY_INTERFACE_MODE_RGMII. There
> is no reason not to follow ever other MAC/PHY pair and have the PHY add the
> delay. The MAC can then do fine tuning if needed, adding small delays.

Thank you for your reply and information.
We will discuss internal and modify in next version.

Thanks,
Jacky

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

[Jacky Chou]

Hi Krzysztof,

> 
> git grep multipleOf:
> 
> e.g.
> oneOf:
>  - minimum: 45
>    maximum: ...
>    multipleOf: 45
>  - minimum: 1500
>    maximum: ...
>    multipleOf: 250
> 
> >
> > Lets see what the DT Maintainers say, but it could be you need two
> > different compatibles for mac0/1 to mac2/3 because they are not
> > actually compatible! You can then have a list per compatible.
> If this is the only, *only* difference, then just go with vendor property matching
> register value... but oh, wait, how person reading and writing the DTS would
> understand if "0x2" means 90 ps or 1750 ps? I don't see how the original
> binding was helping here in total. Just moving the burden from driver
> developer to DTS developer. :/
> 
> If different instances are not the same, means the devices are not the same, so
> two compatibles seem reasonable.

Thank you for your reply and information.
We will modify this section by adding more descriptions and referencing other yaml.

Thanks,
Jacky