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

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

[Jacky Chou]

This patch series adds support for configuring RGMII internal delays for the
Aspeed AST2600 FTGMAC100 Ethernet MACs. It introduces new compatible strings to
distinguish between MAC0/1 and MAC2/3, as their delay chains and configuration
units differ.
The device tree bindings are updated to restrict the allowed phy-mode and delay
properties for each MAC type. Corresponding changes are made to the device tree
source files and the FTGMAC100 driver to support the new delay configuration.

Summary of changes:
- dt-bindings: net: ftgmac100: Add conditional schema for AST2600 MAC0/1 and
  MAC2/3, restrict delay properties, and require SCU phandle.
- ARM: dts: aspeed-g6: Add ethernet aliases to indentify the index of
  MAC.
- ARM: dts: aspeed-ast2600-evb: Add new compatibles, scu handle and
  rx/tx-internal-delay-ps properties and update phy-mode for MACs.
- net: ftgmac100: Add driver support for configuring RGMII delay for AST2600
  MACs via SCU.

This enables precise RGMII timing configuration for AST2600-based platforms,
improving interoperability with various PHYs

---
v3:
 - Add new item on compatible property for new compatible strings
 - Remove the new compatible and scu handle of MAC from aspeed-g6.dtsi
 - Add new compatible and scu handle to MAC node in
   aspeed-ast2600-evb.dts
 - Change all phy-mode of MACs to "rgmii-id"
 - Keep "aspeed,ast2600-mac" compatible in ftgmac100.c and configure the
   rgmii delay with "aspeed,ast2600-mac01" and "aspeed,ast2600-mac23"
v2:
 - added new compatible strings for MAC0/1 and MAC2/3
 - updated device tree bindings to restrict phy-mode and delay properties
 - refactored driver code to handle rgmii delay configuration
---

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>

---
Jacky Chou (4):
      dt-bindings: net: ftgmac100: Add delay properties for AST2600
      ARM: dts: aspeed-g6: Add ethernet alise
      ARM: dts: aspeed: ast2600-evb: Configure RGMII delay for MAC
      net: ftgmac100: Add RGMII delay support for AST2600

 .../devicetree/bindings/net/faraday,ftgmac100.yaml |  50 ++++++++++
 arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts    |  28 +++++-
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi            |   4 +
 drivers/net/ethernet/faraday/ftgmac100.c           | 110 +++++++++++++++++++++
 drivers/net/ethernet/faraday/ftgmac100.h           |  15 +++
 5 files changed, 203 insertions(+), 4 deletions(-)
---
base-commit: 01cc760632b875c4ad0d8fec0b0c01896b8a36d4
change-id: 20251031-rgmii_delay_2600-a00b0248c7e6

Best regards,
-- 
Jacky Chou <jacky_chou@aspeedtech.com>

[PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Jacky Chou]

Create the new compatibles to identify AST2600 MAC0/1 and MAC3/4.
Add conditional schema constraints for Aspeed AST2600 MAC controllers:
- For "aspeed,ast2600-mac01", require rx/tx-internal-delay-ps properties
  with 45ps step.
- For "aspeed,ast2600-mac23", require rx/tx-internal-delay-ps properties
  with 250ps step.
- Both require the "scu" property.
Other compatible values remain unrestricted.

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

diff --git a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
index d14410018bcf..de646e7e3bca 100644
--- a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
+++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
@@ -19,6 +19,12 @@ properties:
               - aspeed,ast2500-mac
               - aspeed,ast2600-mac
           - const: faraday,ftgmac100
+      - items:
+          - enum:
+              - aspeed,ast2600-mac01
+              - aspeed,ast2600-mac23
+          - const: aspeed,ast2600-mac
+          - const: faraday,ftgmac100
 
   reg:
     maxItems: 1
@@ -69,6 +75,12 @@ properties:
   mdio:
     $ref: /schemas/net/mdio.yaml#
 
+  scu:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the SCU (System Control Unit) syscon node for Aspeed platform.
+      This reference is used by the MAC controller to configure the RGMII delays.
+
 required:
   - compatible
   - reg
@@ -88,6 +100,44 @@ allOf:
     else:
       properties:
         resets: false
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: aspeed,ast2600-mac01
+    then:
+      properties:
+        rx-internal-delay-ps:
+          minimum: 0
+          maximum: 1395
+          multipleOf: 45
+        tx-internal-delay-ps:
+          minimum: 0
+          maximum: 1395
+          multipleOf: 45
+      required:
+        - scu
+        - rx-internal-delay-ps
+        - tx-internal-delay-ps
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: aspeed,ast2600-mac23
+    then:
+      properties:
+        rx-internal-delay-ps:
+          minimum: 0
+          maximum: 7750
+          multipleOf: 250
+        tx-internal-delay-ps:
+          minimum: 0
+          maximum: 7750
+          multipleOf: 250
+      required:
+        - scu
+        - rx-internal-delay-ps
+        - tx-internal-delay-ps
 
 unevaluatedProperties: false
 

-- 
2.34.1

[PATCH net-next v3 2/4] ARM: dts: aspeed-g6: Add ethernet alise

[Jacky Chou]

For RGMII delay configuration, MAC0 and MAC1 use register SCU0x340,
while MAC2 and MAC3 use SCU0x350.
The Ethernet aliases are added to help identify the corresponding
MAC index.

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 f8662c8ac089..03ad566a4ce8 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -40,6 +40,10 @@ aliases {
 		mdio1 = &mdio1;
 		mdio2 = &mdio2;
 		mdio3 = &mdio3;
+		ethernet0 = &mac0;
+		ethernet1 = &mac1;
+		ethernet2 = &mac2;
+		ethernet3 = &mac3;
 	};
 
 

-- 
2.34.1

[PATCH net-next v3 3/4] ARM: dts: aspeed: ast2600-evb: Configure RGMII delay for MAC

[Jacky Chou]

This change sets the rx-internal-delay-ps and tx-internal-delay-ps
properties to control the RGMII signal delay.
The phy-mode for MAC0–MAC3 is updated to "rgmii-id" to enable TX/RX
internal delay on the PHY and disable the corresponding delay
on the MAC.

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

diff --git a/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
index de83c0eb1d6e..a65568e637bd 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
@@ -121,44 +121,64 @@ ethphy3: ethernet-phy@0 {
 };
 
 &mac0 {
+	compatible = "aspeed,ast2600-mac01", "aspeed,ast2600-mac", "faraday,ftgmac100";
 	status = "okay";
 
-	phy-mode = "rgmii-rxid";
+	phy-mode = "rgmii-id";
 	phy-handle = <&ethphy0>;
 
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_rgmii1_default>;
+
+	rx-internal-delay-ps = <0>;
+	tx-internal-delay-ps = <0>;
+	scu = <&syscon>;
 };
 
 
 &mac1 {
+	compatible = "aspeed,ast2600-mac01", "aspeed,ast2600-mac", "faraday,ftgmac100";
 	status = "okay";
 
-	phy-mode = "rgmii-rxid";
+	phy-mode = "rgmii-id";
 	phy-handle = <&ethphy1>;
 
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_rgmii2_default>;
+
+	rx-internal-delay-ps = <0>;
+	tx-internal-delay-ps = <0>;
+	scu = <&syscon>;
 };
 
 &mac2 {
+	compatible = "aspeed,ast2600-mac23", "aspeed,ast2600-mac", "faraday,ftgmac100";
 	status = "okay";
 
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 	phy-handle = <&ethphy2>;
 
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_rgmii3_default>;
+
+	rx-internal-delay-ps = <0>;
+	tx-internal-delay-ps = <0>;
+	scu = <&syscon>;
 };
 
 &mac3 {
+	compatible = "aspeed,ast2600-mac23", "aspeed,ast2600-mac", "faraday,ftgmac100";
 	status = "okay";
 
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 	phy-handle = <&ethphy3>;
 
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_rgmii4_default>;
+
+	rx-internal-delay-ps = <0>;
+	tx-internal-delay-ps = <0>;
+	scu = <&syscon>;
 };
 
 &emmc_controller {

-- 
2.34.1

[PATCH net-next v3 4/4] net: ftgmac100: Add RGMII delay support for AST2600

[Jacky Chou]

On the AST2600 platform, the RGMII delay is controlled via the
SCU registers. The delay chain configuration differs between MAC0/1
and MAC2/3, even though all four MACs use a 32-stage delay chain.
+------+----------+-----------+-------------+-------------+
|      |Delay Unit|Delay Stage|TX Edge Stage|RX Edge Stage|
+------+----------+-----------+-------------+-------------+
|MAC0/1|     45 ps|        32 |           0 |           0 |
+------+----------+-----------+-------------+-------------+
|MAC2/3|    250 ps|        32 |           0 |          26 |
+------+----------+-----------+-------------+-------------+
For MAC2/3, the "no delay" condition starts from stage 26.
Setting the RX delay stage to 26 means that no additional RX
delay is applied.
Here lists the RX delay setting of MAC2/3 below.
26 -> 0   ns, 27 -> 0.25 ns, ... , 31 -> 1.25 ns,
0  -> 1.5 ns, 1  -> 1.75 ns, ... , 25 -> 7.75 ns

Therefore, we calculate the delay stage from the
rx-internal-delay-ps of MAC2/3 to add 26. If the stage is equel
to or bigger than 32, the delay stage will be mask 0x1f to get
the correct setting.
The delay chain is like a ring for configuration.
Example for the rx-internal-delay-ps of MAC2/3 is 2000 ps,
we will get the delay stage is 2.

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

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index a863f7841210..bc83ef079095 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -26,6 +26,9 @@
 #include <linux/if_vlan.h>
 #include <linux/of_net.h>
 #include <linux/phy_fixed.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/bitfield.h>
 #include <net/ip.h>
 #include <net/ncsi.h>
 
@@ -1833,6 +1836,108 @@ static bool ftgmac100_has_child_node(struct device_node *np, const char *name)
 	return ret;
 }
 
+static int ftgmac100_set_ast2600_rgmii_delay(struct platform_device *pdev,
+					     u32 rgmii_tx_delay,
+					     u32 rgmii_rx_delay)
+{
+	struct device_node *np = pdev->dev.of_node;
+	u32 rgmii_delay_unit;
+	struct regmap *scu;
+	int dly_mask;
+	int dly_reg;
+	int id;
+
+	scu = syscon_regmap_lookup_by_phandle(np, "scu");
+	if (IS_ERR(scu)) {
+		dev_err(&pdev->dev, "failed to get scu");
+		return PTR_ERR(scu);
+	}
+
+	id = of_alias_get_id(np, "ethernet");
+	if (id < 0 || id > 3) {
+		dev_err(&pdev->dev, "get wrong alise id %d\n", id);
+		return -EINVAL;
+	}
+
+	if (of_device_is_compatible(np, "aspeed,ast2600-mac01")) {
+		dly_reg = AST2600_MAC01_CLK_DLY;
+		rgmii_delay_unit = AST2600_MAC01_CLK_DLY_UNIT;
+	} else if (of_device_is_compatible(np, "aspeed,ast2600-mac23")) {
+		dly_reg = AST2600_MAC23_CLK_DLY;
+		rgmii_delay_unit = AST2600_MAC23_CLK_DLY_UNIT;
+	}
+
+	rgmii_tx_delay = DIV_ROUND_CLOSEST(rgmii_tx_delay, rgmii_delay_unit);
+	if (rgmii_tx_delay >= 32) {
+		dev_err(&pdev->dev,
+			"The index %u of TX delay setting is out of range\n",
+			rgmii_tx_delay);
+		return -EINVAL;
+	}
+
+	rgmii_rx_delay = DIV_ROUND_CLOSEST(rgmii_rx_delay, rgmii_delay_unit);
+	if (rgmii_rx_delay >= 32) {
+		dev_err(&pdev->dev,
+			"The index %u of RX delay setting is out of range\n",
+			rgmii_rx_delay);
+		return -EINVAL;
+	}
+
+	/* Due to the hardware design reason, for MAC23 on AST2600, the zero
+	 * delay ns on RX is configured by setting value 0x1a.
+	 * List as below:
+	 * 0x1a -> 0   ns, 0x1b -> 0.25 ns, ... , 0x1f -> 1.25 ns,
+	 * 0x00 -> 1.5 ns, 0x01 -> 1.75 ns, ... , 0x19 -> 7.75 ns, 0x1a -> 0 ns
+	 */
+	if (of_device_is_compatible(np, "aspeed,ast2600-mac23"))
+		rgmii_rx_delay = (AST2600_MAC23_RX_DLY_0_NS + rgmii_rx_delay) &
+				 AST2600_MAC_TX_RX_DLY_MASK;
+
+	if (id == 0 || id == 2) {
+		dly_mask = ASPEED_MAC0_2_TX_DLY | ASPEED_MAC0_2_RX_DLY;
+		rgmii_tx_delay = FIELD_PREP(ASPEED_MAC0_2_TX_DLY, rgmii_tx_delay);
+		rgmii_rx_delay = FIELD_PREP(ASPEED_MAC0_2_RX_DLY, rgmii_rx_delay);
+	} else {
+		dly_mask = ASPEED_MAC1_3_TX_DLY | ASPEED_MAC1_3_RX_DLY;
+		rgmii_tx_delay = FIELD_PREP(ASPEED_MAC1_3_TX_DLY, rgmii_tx_delay);
+		rgmii_rx_delay = FIELD_PREP(ASPEED_MAC1_3_RX_DLY, rgmii_rx_delay);
+	}
+
+	regmap_update_bits(scu, dly_reg, dly_mask, rgmii_tx_delay | rgmii_rx_delay);
+
+	return 0;
+}
+
+static int ftgmac100_set_internal_delay(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	u32 rgmii_tx_delay;
+	u32 rgmii_rx_delay;
+	int err;
+
+	if (!(of_device_is_compatible(np, "aspeed,ast2600-mac01") ||
+	      of_device_is_compatible(np, "aspeed,ast2600-mac23")))
+		return 0;
+
+	err = of_property_read_u32(np, "tx-internal-delay-ps", &rgmii_tx_delay);
+	if (err) {
+		dev_err(&pdev->dev, "failed to get tx-internal-delay-ps\n");
+		return err;
+	}
+
+	err = of_property_read_u32(np, "rx-internal-delay-ps", &rgmii_rx_delay);
+	if (err) {
+		dev_err(&pdev->dev, "failed to get tx-internal-delay-ps\n");
+		return err;
+	}
+
+	err = ftgmac100_set_ast2600_rgmii_delay(pdev,
+						rgmii_tx_delay,
+						rgmii_rx_delay);
+
+	return err;
+}
+
 static int ftgmac100_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -2004,6 +2109,11 @@ 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 compatibles */
+		err = ftgmac100_set_internal_delay(pdev);
+		if (err)
+			goto err_phy_connect;
 	}
 
 	/* Default ring sizes */
diff --git a/drivers/net/ethernet/faraday/ftgmac100.h b/drivers/net/ethernet/faraday/ftgmac100.h
index 4968f6f0bdbc..6a2a9159bee4 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.h
+++ b/drivers/net/ethernet/faraday/ftgmac100.h
@@ -271,4 +271,19 @@ struct ftgmac100_rxdes {
 #define FTGMAC100_RXDES1_UDP_CHKSUM_ERR	(1 << 26)
 #define FTGMAC100_RXDES1_IP_CHKSUM_ERR	(1 << 27)
 
+/* Aspeed SCU */
+#define AST2600_MAC01_CLK_DLY	0x340
+#define AST2600_MAC23_CLK_DLY	0x350
+#define AST2600_MAC01_CLK_DLY_UNIT	45	/* ps */
+#define AST2600_MAC01_TX_DLY_0_NS	0
+#define AST2600_MAC01_RX_DLY_0_NS	0
+#define AST2600_MAC23_CLK_DLY_UNIT	250	/* ps */
+#define AST2600_MAC23_TX_DLY_0_NS	0
+#define AST2600_MAC23_RX_DLY_0_NS	0x1a
+#define AST2600_MAC_TX_RX_DLY_MASK	0x1f
+#define ASPEED_MAC0_2_TX_DLY		GENMASK(5, 0)
+#define ASPEED_MAC0_2_RX_DLY		GENMASK(17, 12)
+#define ASPEED_MAC1_3_TX_DLY		GENMASK(11, 6)
+#define ASPEED_MAC1_3_RX_DLY		GENMASK(23, 18)
+
 #endif /* __FTGMAC100_H */

-- 
2.34.1

Re: [PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Andrew Lunn]

On Mon, Nov 03, 2025 at 03:39:16PM +0800, Jacky Chou wrote:
> Create the new compatibles to identify AST2600 MAC0/1 and MAC3/4.
> Add conditional schema constraints for Aspeed AST2600 MAC controllers:
> - For "aspeed,ast2600-mac01", require rx/tx-internal-delay-ps properties
>   with 45ps step.
> - For "aspeed,ast2600-mac23", require rx/tx-internal-delay-ps properties
>   with 250ps step.
> - Both require the "scu" property.
> Other compatible values remain unrestricted.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  .../devicetree/bindings/net/faraday,ftgmac100.yaml | 50 ++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> index d14410018bcf..de646e7e3bca 100644
> --- a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> +++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> @@ -19,6 +19,12 @@ properties:
>                - aspeed,ast2500-mac
>                - aspeed,ast2600-mac
>            - const: faraday,ftgmac100
> +      - items:
> +          - enum:
> +              - aspeed,ast2600-mac01
> +              - aspeed,ast2600-mac23
> +          - const: aspeed,ast2600-mac
> +          - const: faraday,ftgmac100
>  
>    reg:
>      maxItems: 1
> @@ -69,6 +75,12 @@ properties:
>    mdio:
>      $ref: /schemas/net/mdio.yaml#
>  
> +  scu:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the SCU (System Control Unit) syscon node for Aspeed platform.
> +      This reference is used by the MAC controller to configure the RGMII delays.
> +
>  required:
>    - compatible
>    - reg
> @@ -88,6 +100,44 @@ allOf:
>      else:
>        properties:
>          resets: false
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2600-mac01
> +    then:
> +      properties:
> +        rx-internal-delay-ps:
> +          minimum: 0
> +          maximum: 1395
> +          multipleOf: 45

I would add a default: 0

> +        tx-internal-delay-ps:
> +          minimum: 0
> +          maximum: 1395
> +          multipleOf: 45

and also here.

> +      required:
> +        - scu
> +        - rx-internal-delay-ps
> +        - tx-internal-delay-ps

and then these are not required, but optional.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2600-mac23
> +    then:
> +      properties:
> +        rx-internal-delay-ps:
> +          minimum: 0
> +          maximum: 7750
> +          multipleOf: 250
> +        tx-internal-delay-ps:
> +          minimum: 0
> +          maximum: 7750
> +          multipleOf: 250
> +      required:
> +        - scu
> +        - rx-internal-delay-ps
> +        - tx-internal-delay-ps

Same again here.

	Andrew

Re: [PATCH net-next v3 4/4] net: ftgmac100: Add RGMII delay support for AST2600

[kernel test robot]

Hi Jacky,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 01cc760632b875c4ad0d8fec0b0c01896b8a36d4]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacky-Chou/dt-bindings-net-ftgmac100-Add-delay-properties-for-AST2600/20251103-154258
base:   01cc760632b875c4ad0d8fec0b0c01896b8a36d4
patch link:    https://lore.kernel.org/r/20251103-rgmii_delay_2600-v3-4-e2af2656f7d7%40aspeedtech.com
patch subject: [PATCH net-next v3 4/4] net: ftgmac100: Add RGMII delay support for AST2600
config: arm-defconfig (https://download.01.org/0day-ci/archive/20251104/202511041023.QGAHaAZ3-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project d2625a438020ad35330cda29c3def102c1687b1b)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251104/202511041023.QGAHaAZ3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511041023.QGAHaAZ3-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/faraday/ftgmac100.c:1865:13: warning: variable 'rgmii_delay_unit' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    1865 |         } else if (of_device_is_compatible(np, "aspeed,ast2600-mac23")) {
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/faraday/ftgmac100.c:1870:53: note: uninitialized use occurs here
    1870 |         rgmii_tx_delay = DIV_ROUND_CLOSEST(rgmii_tx_delay, rgmii_delay_unit);
         |                                                            ^~~~~~~~~~~~~~~~
   drivers/net/ethernet/faraday/ftgmac100.c:1865:9: note: remove the 'if' if its condition is always true
    1865 |         } else if (of_device_is_compatible(np, "aspeed,ast2600-mac23")) {
         |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/faraday/ftgmac100.c:1844:22: note: initialize the variable 'rgmii_delay_unit' to silence this warning
    1844 |         u32 rgmii_delay_unit;
         |                             ^
         |                              = 0
   1 warning generated.


vim +1865 drivers/net/ethernet/faraday/ftgmac100.c

  1838	
  1839	static int ftgmac100_set_ast2600_rgmii_delay(struct platform_device *pdev,
  1840						     u32 rgmii_tx_delay,
  1841						     u32 rgmii_rx_delay)
  1842	{
  1843		struct device_node *np = pdev->dev.of_node;
  1844		u32 rgmii_delay_unit;
  1845		struct regmap *scu;
  1846		int dly_mask;
  1847		int dly_reg;
  1848		int id;
  1849	
  1850		scu = syscon_regmap_lookup_by_phandle(np, "scu");
  1851		if (IS_ERR(scu)) {
  1852			dev_err(&pdev->dev, "failed to get scu");
  1853			return PTR_ERR(scu);
  1854		}
  1855	
  1856		id = of_alias_get_id(np, "ethernet");
  1857		if (id < 0 || id > 3) {
  1858			dev_err(&pdev->dev, "get wrong alise id %d\n", id);
  1859			return -EINVAL;
  1860		}
  1861	
  1862		if (of_device_is_compatible(np, "aspeed,ast2600-mac01")) {
  1863			dly_reg = AST2600_MAC01_CLK_DLY;
  1864			rgmii_delay_unit = AST2600_MAC01_CLK_DLY_UNIT;
> 1865		} else if (of_device_is_compatible(np, "aspeed,ast2600-mac23")) {
  1866			dly_reg = AST2600_MAC23_CLK_DLY;
  1867			rgmii_delay_unit = AST2600_MAC23_CLK_DLY_UNIT;
  1868		}
  1869	
  1870		rgmii_tx_delay = DIV_ROUND_CLOSEST(rgmii_tx_delay, rgmii_delay_unit);
  1871		if (rgmii_tx_delay >= 32) {
  1872			dev_err(&pdev->dev,
  1873				"The index %u of TX delay setting is out of range\n",
  1874				rgmii_tx_delay);
  1875			return -EINVAL;
  1876		}
  1877	
  1878		rgmii_rx_delay = DIV_ROUND_CLOSEST(rgmii_rx_delay, rgmii_delay_unit);
  1879		if (rgmii_rx_delay >= 32) {
  1880			dev_err(&pdev->dev,
  1881				"The index %u of RX delay setting is out of range\n",
  1882				rgmii_rx_delay);
  1883			return -EINVAL;
  1884		}
  1885	
  1886		/* Due to the hardware design reason, for MAC23 on AST2600, the zero
  1887		 * delay ns on RX is configured by setting value 0x1a.
  1888		 * List as below:
  1889		 * 0x1a -> 0   ns, 0x1b -> 0.25 ns, ... , 0x1f -> 1.25 ns,
  1890		 * 0x00 -> 1.5 ns, 0x01 -> 1.75 ns, ... , 0x19 -> 7.75 ns, 0x1a -> 0 ns
  1891		 */
  1892		if (of_device_is_compatible(np, "aspeed,ast2600-mac23"))
  1893			rgmii_rx_delay = (AST2600_MAC23_RX_DLY_0_NS + rgmii_rx_delay) &
  1894					 AST2600_MAC_TX_RX_DLY_MASK;
  1895	
  1896		if (id == 0 || id == 2) {
  1897			dly_mask = ASPEED_MAC0_2_TX_DLY | ASPEED_MAC0_2_RX_DLY;
  1898			rgmii_tx_delay = FIELD_PREP(ASPEED_MAC0_2_TX_DLY, rgmii_tx_delay);
  1899			rgmii_rx_delay = FIELD_PREP(ASPEED_MAC0_2_RX_DLY, rgmii_rx_delay);
  1900		} else {
  1901			dly_mask = ASPEED_MAC1_3_TX_DLY | ASPEED_MAC1_3_RX_DLY;
  1902			rgmii_tx_delay = FIELD_PREP(ASPEED_MAC1_3_TX_DLY, rgmii_tx_delay);
  1903			rgmii_rx_delay = FIELD_PREP(ASPEED_MAC1_3_RX_DLY, rgmii_rx_delay);
  1904		}
  1905	
  1906		regmap_update_bits(scu, dly_reg, dly_mask, rgmii_tx_delay | rgmii_rx_delay);
  1907	
  1908		return 0;
  1909	}
  1910	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v3 3/4] ARM: dts: aspeed: ast2600-evb: Configure RGMII delay for MAC

[Andrew Lunn]

> +	scu = <&syscon>;

This property can be moved into the .dtsi file. It should not matter
if .dts files are using the old compatible, it will be ignored. But
having it in the .dtsi makes it easier to .dts files changing to the
new compatible.

	Andrew

Re: [PATCH net-next v3 4/4] net: ftgmac100: Add RGMII delay support for AST2600

[Andrew Lunn]

> +	rgmii_tx_delay = DIV_ROUND_CLOSEST(rgmii_tx_delay, rgmii_delay_unit);
> +	if (rgmii_tx_delay >= 32) {
> +		dev_err(&pdev->dev,
> +			"The index %u of TX delay setting is out of range\n",
> +			rgmii_tx_delay);

The index is not really interesting here, it is not something a DT
author uses. It is the delay in ps in the .dts file which is too big.

       Andrew

Re: [PATCH net-next v3 3/4] ARM: dts: aspeed: ast2600-evb: Configure RGMII delay for MAC

[Andrew Lunn]

On Mon, Nov 03, 2025 at 03:39:18PM +0800, Jacky Chou wrote:
> This change sets the rx-internal-delay-ps and tx-internal-delay-ps
> properties to control the RGMII signal delay.
> The phy-mode for MAC0–MAC3 is updated to "rgmii-id" to enable TX/RX
> internal delay on the PHY and disable the corresponding delay
> on the MAC.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts | 28 +++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
> index de83c0eb1d6e..a65568e637bd 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
> +++ b/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
> @@ -121,44 +121,64 @@ ethphy3: ethernet-phy@0 {
>  };
>  
>  &mac0 {
> +	compatible = "aspeed,ast2600-mac01", "aspeed,ast2600-mac", "faraday,ftgmac100";

Is it really compatible to aspeed,ast2600-mac? If a driver binds to
that, not aspeed,ast2600-mac01, doesn't that imply the bootloader
delays are still in use, so phy-mode will be wrong?

I think you should only list aspeed,ast2600-mac01. If somebody uses
this DT blob on an old kernel, then you won't get an ethernet
interface, rather than a not working ethernet interface, which is
probably preferable.

	Andrew

Re: [PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Andrew Lunn]

On Mon, Nov 03, 2025 at 03:39:16PM +0800, Jacky Chou wrote:
> Create the new compatibles to identify AST2600 MAC0/1 and MAC3/4.
> Add conditional schema constraints for Aspeed AST2600 MAC controllers:
> - For "aspeed,ast2600-mac01", require rx/tx-internal-delay-ps properties
>   with 45ps step.
> - For "aspeed,ast2600-mac23", require rx/tx-internal-delay-ps properties
>   with 250ps step.
> - Both require the "scu" property.
> Other compatible values remain unrestricted.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  .../devicetree/bindings/net/faraday,ftgmac100.yaml | 50 ++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> index d14410018bcf..de646e7e3bca 100644
> --- a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> +++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> @@ -19,6 +19,12 @@ properties:
>                - aspeed,ast2500-mac
>                - aspeed,ast2600-mac

I don't know if it is possible, but it would be good to mark
aspeed,ast2600-mac as deprecated.

I also think some comments would be good, explaining how
aspeed,ast2600-mac01 and aspeed,ast2600-mac23 differ from
aspeed,ast2600-mac, and why you should use them.

	Andrew

[PATCH net-next v3 4/4] net: ftgmac100: Add RGMII delay support for AST2600

[Jacky Chou]

Hi Andrew,

Thank you for your reply.

> > +	rgmii_tx_delay = DIV_ROUND_CLOSEST(rgmii_tx_delay,
> rgmii_delay_unit);
> > +	if (rgmii_tx_delay >= 32) {
> > +		dev_err(&pdev->dev,
> > +			"The index %u of TX delay setting is out of range\n",
> > +			rgmii_tx_delay);
> 
> The index is not really interesting here, it is not something a DT author uses. It
> is the delay in ps in the .dts file which is too big.
> 

Agreed - the "index" isn't meaningful to DT authors.
I'll modify the error message to display the actual TX/RX delay value in picoseconds 
instead of the internal index, so it's clearer which DT value is too large.

Thanks,
Jacky

回覆: [PATCH net-next v3 3/4] ARM: dts: aspeed: ast2600-evb: Configure RGMII delay for MAC

[Jacky Chou]

Hi Andrew,

> > diff --git a/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
> > b/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
> > index de83c0eb1d6e..a65568e637bd 100644
> > --- a/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
> > +++ b/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
> > @@ -121,44 +121,64 @@ ethphy3: ethernet-phy@0 {  };
> >
> >  &mac0 {
> > +	compatible = "aspeed,ast2600-mac01", "aspeed,ast2600-mac",
> > +"faraday,ftgmac100";
> 
> Is it really compatible to aspeed,ast2600-mac? If a driver binds to that, not
> aspeed,ast2600-mac01, doesn't that imply the bootloader delays are still in use,
> so phy-mode will be wrong?
> 
> I think you should only list aspeed,ast2600-mac01. If somebody uses this DT
> blob on an old kernel, then you won't get an ethernet interface, rather than a
> not working ethernet interface, which is probably preferable.
> 

Thanks for pointing that out.
Agreed. I'll update the patch to only use "aspeed,ast2600-mac01" as the compatible 
String like below, so that only the new driver version binds.
I'll also update the dt-binding yaml.

&mac0 {
	compatible = "aspeed,ast2600-mac01", "faraday,ftgmac100";
	...
};

Thanks,
Jacky

[PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Jacky Chou]

Hi Andrew,

Thank you for your reply.

> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: aspeed,ast2600-mac01
> > +    then:
> > +      properties:
> > +        rx-internal-delay-ps:
> > +          minimum: 0
> > +          maximum: 1395
> > +          multipleOf: 45
> 
> I would add a default: 0
> 

Agreed.
I will add it in next version.

> > +        tx-internal-delay-ps:
> > +          minimum: 0
> > +          maximum: 1395
> > +          multipleOf: 45
> 
> and also here.
> 

Agreed.

> > +      required:
> > +        - scu
> > +        - rx-internal-delay-ps
> > +        - tx-internal-delay-ps
> 
> and then these are not required, but optional.
> 

Configure the tx/rx delay in the scu register.
At least, the scu handle must be required.

Here I have one question.
In v3 patches series, if the ftgmac driver cannot find one of 
tx-internal-delay-ps and rx-internal-delay-ps, it will return error in probe 
stage.
If here is optional, does it means that just add warning and not return error when
lack one of them and use the default to configure? Or not configure tx/rx delay just
return success in probe stage?

Thanks,
Jacky

回覆: [PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Jacky Chou]

> > diff --git
> > a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> > b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> > index d14410018bcf..de646e7e3bca 100644
> > --- a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> > +++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> > @@ -19,6 +19,12 @@ properties:
> >                - aspeed,ast2500-mac
> >                - aspeed,ast2600-mac
> 
> I don't know if it is possible, but it would be good to mark aspeed,ast2600-mac
> as deprecated.
> 
> I also think some comments would be good, explaining how
> aspeed,ast2600-mac01 and aspeed,ast2600-mac23 differ from
> aspeed,ast2600-mac, and why you should use them.
> 

Thanks for the suggestion.
We keep "aspeed,ast2600-mac" in the compatible list mainly for backward compatibility.
There are already many existing device trees and systems using this string.
Removing or deprecating it right now might break those setups.

However, I agree that adding comments to clarify the differences between "aspeed,ast2600-mac",
"aspeed,ast2600-mac01", and "aspeed,ast2600-mac23" would be helpful for new users. 

In the future, if someone submits a new DTS for an AST2600-based platform,
I think they should add the new compatible string and properly describe the TX/RX delay and 
phy-mode properties in their DTS.

Thanks,
Jacky

Re: [PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Krzysztof Kozlowski]

On Mon, Nov 03, 2025 at 03:39:16PM +0800, Jacky Chou wrote:
> Create the new compatibles to identify AST2600 MAC0/1 and MAC3/4.
> Add conditional schema constraints for Aspeed AST2600 MAC controllers:
> - For "aspeed,ast2600-mac01", require rx/tx-internal-delay-ps properties
>   with 45ps step.
> - For "aspeed,ast2600-mac23", require rx/tx-internal-delay-ps properties
>   with 250ps step.

That difference does not justify different compatibles. Basically you
said they have same programming model, just different hardware
characteristics, so same compatible.

Best regards,
Krzysztof

Re: [PATCH net-next v3 0/4] Add AST2600 RGMII delay into ftgmac100

[Krzysztof Kozlowski]

On Mon, Nov 03, 2025 at 03:39:15PM +0800, Jacky Chou wrote:
> This patch series adds support for configuring RGMII internal delays for the
> Aspeed AST2600 FTGMAC100 Ethernet MACs. It introduces new compatible strings to
> distinguish between MAC0/1 and MAC2/3, as their delay chains and configuration
> units differ.
> The device tree bindings are updated to restrict the allowed phy-mode and delay
> properties for each MAC type. Corresponding changes are made to the device tree
> source files and the FTGMAC100 driver to support the new delay configuration.
> 
> Summary of changes:
> - dt-bindings: net: ftgmac100: Add conditional schema for AST2600 MAC0/1 and
>   MAC2/3, restrict delay properties, and require SCU phandle.
> - ARM: dts: aspeed-g6: Add ethernet aliases to indentify the index of
>   MAC.
> - ARM: dts: aspeed-ast2600-evb: Add new compatibles, scu handle and
>   rx/tx-internal-delay-ps properties and update phy-mode for MACs.
> - net: ftgmac100: Add driver support for configuring RGMII delay for AST2600
>   MACs via SCU.
> 
> This enables precise RGMII timing configuration for AST2600-based platforms,
> improving interoperability with various PHYs
> 
> ---
> v3:
>  - Add new item on compatible property for new compatible strings
>  - Remove the new compatible and scu handle of MAC from aspeed-g6.dtsi
>  - Add new compatible and scu handle to MAC node in
>    aspeed-ast2600-evb.dts
>  - Change all phy-mode of MACs to "rgmii-id"
>  - Keep "aspeed,ast2600-mac" compatible in ftgmac100.c and configure the
>    rgmii delay with "aspeed,ast2600-mac01" and "aspeed,ast2600-mac23"
> v2:
>  - added new compatible strings for MAC0/1 and MAC2/3
>  - updated device tree bindings to restrict phy-mode and delay properties
>  - refactored driver code to handle rgmii delay configuration

That's b4 managed change, so where are the lorelinks? Why are you
removing them?

Since you decided to drop them making it difficult for me to find
previous revisions, I will not bother to look at background of this
patchset to understand why you did that way and just NAK the binding.

Next time, make it easy for reviewers, not intentionally difficult.

Best regards,
Krzysztof

回覆: [PATCH net-next v3 0/4] Add AST2600 RGMII delay into ftgmac100

[Jacky Chou]

Hi Krzysztof

> > ---
> > v3:
> >  - Add new item on compatible property for new compatible strings
> >  - Remove the new compatible and scu handle of MAC from aspeed-g6.dtsi
> >  - Add new compatible and scu handle to MAC node in
> >    aspeed-ast2600-evb.dts
> >  - Change all phy-mode of MACs to "rgmii-id"
> >  - Keep "aspeed,ast2600-mac" compatible in ftgmac100.c and configure the
> >    rgmii delay with "aspeed,ast2600-mac01" and "aspeed,ast2600-mac23"
> > v2:
> >  - added new compatible strings for MAC0/1 and MAC2/3
> >  - updated device tree bindings to restrict phy-mode and delay
> > properties
> >  - refactored driver code to handle rgmii delay configuration
> 
> That's b4 managed change, so where are the lorelinks? Why are you removing
> them?
> 
> Since you decided to drop them making it difficult for me to find previous
> revisions, I will not bother to look at background of this patchset to understand
> why you did that way and just NAK the binding.
> 
> Next time, make it easy for reviewers, not intentionally difficult.
> 

Thanks for pointing that out.
The missing lore links were due to my attempt to use b4 for sending this version, 
and some information was unintentionally dropped.
Starting from the next revision, I’ll make sure to include all the proper lore links 
and metadata generated by b4 send.

Sorry about the missing lore links in this version. and thank you for the review.

Thanks,
Jacky

[PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Jacky Chou]

Hi Krzysztof

Thank you for your reply.

> > Create the new compatibles to identify AST2600 MAC0/1 and MAC3/4.
> > Add conditional schema constraints for Aspeed AST2600 MAC controllers:
> > - For "aspeed,ast2600-mac01", require rx/tx-internal-delay-ps properties
> >   with 45ps step.
> > - For "aspeed,ast2600-mac23", require rx/tx-internal-delay-ps properties
> >   with 250ps step.
> 
> That difference does not justify different compatibles. Basically you said they
> have same programming model, just different hardware characteristics, so
> same compatible.
> 

This change was originally based on feedback from a previous review discussion.
At that time, another reviewer suggested introducing separate compatibles for 
MAC0/1 and MAC2/3 on AST2600, since the delay characteristics differ and they 
might not be fully compatible.

Thanks,
Jacky

Re: [PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Krzysztof Kozlowski]

On 04/11/2025 10:54, Jacky Chou wrote:
> Hi Krzysztof
> 
> Thank you for your reply.
> 
>>> Create the new compatibles to identify AST2600 MAC0/1 and MAC3/4.
>>> Add conditional schema constraints for Aspeed AST2600 MAC controllers:
>>> - For "aspeed,ast2600-mac01", require rx/tx-internal-delay-ps properties
>>>   with 45ps step.
>>> - For "aspeed,ast2600-mac23", require rx/tx-internal-delay-ps properties
>>>   with 250ps step.
>>
>> That difference does not justify different compatibles. Basically you said they
>> have same programming model, just different hardware characteristics, so
>> same compatible.
>>
> 
> This change was originally based on feedback from a previous review discussion.
> At that time, another reviewer suggested introducing separate compatibles for 
> MAC0/1 and MAC2/3 on AST2600, since the delay characteristics differ and they 
> might not be fully compatible.


Your commit msg does not provide enough of rationale for that.
Difference in DTS properties is rather a counter argument for having
separate compatibles. That's why you have these properties - to mark the
difference.

Best regards,
Krzysztof

Re: [PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Andrew Lunn]

On Tue, Nov 04, 2025 at 05:14:41AM +0000, Jacky Chou wrote:
> Hi Andrew,
> 
> Thank you for your reply.
> 
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: aspeed,ast2600-mac01
> > > +    then:
> > > +      properties:
> > > +        rx-internal-delay-ps:
> > > +          minimum: 0
> > > +          maximum: 1395
> > > +          multipleOf: 45
> > 
> > I would add a default: 0
> > 
> 
> Agreed.
> I will add it in next version.
> 
> > > +        tx-internal-delay-ps:
> > > +          minimum: 0
> > > +          maximum: 1395
> > > +          multipleOf: 45
> > 
> > and also here.
> > 
> 
> Agreed.
> 
> > > +      required:
> > > +        - scu
> > > +        - rx-internal-delay-ps
> > > +        - tx-internal-delay-ps
> > 
> > and then these are not required, but optional.
> > 
> 
> Configure the tx/rx delay in the scu register.
> At least, the scu handle must be required.

Sorry, i was unclear. By says 'and then', i was trying to chain it to
the previous comment, that the delays should default to 0. With
defaults set, rx-internal-delay-ps and tx-internal-delay-ps become
optional. I agree scu is required.

> Here I have one question.
> In v3 patches series, if the ftgmac driver cannot find one of 
> tx-internal-delay-ps and rx-internal-delay-ps, it will return error in probe 
> stage.
> If here is optional, does it means that just add warning and not return error when
> lack one of them and use the default to configure? Or not configure tx/rx delay just
> return success in probe stage?

Once you add the default statement, it is clear what delay should be
added if they property is not listed, 0. No warning is needed.

What you should find in the end is that most boards just set the new
compatible and 'rgmii-id', and need nothing else. Only badly designed
boards tend to need tx-internal-delay-ps and rx-internal-delay-ps.

	Andrew

Re: 回覆: [PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Andrew Lunn]

On Tue, Nov 04, 2025 at 05:22:59AM +0000, Jacky Chou wrote:
> > > diff --git
> > > a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> > > b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> > > index d14410018bcf..de646e7e3bca 100644
> > > --- a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> > > +++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> > > @@ -19,6 +19,12 @@ properties:
> > >                - aspeed,ast2500-mac
> > >                - aspeed,ast2600-mac
> > 
> > I don't know if it is possible, but it would be good to mark aspeed,ast2600-mac
> > as deprecated.
> > 
> > I also think some comments would be good, explaining how
> > aspeed,ast2600-mac01 and aspeed,ast2600-mac23 differ from
> > aspeed,ast2600-mac, and why you should use them.
> > 
> 
> Thanks for the suggestion.
> We keep "aspeed,ast2600-mac" in the compatible list mainly for backward compatibility.
> There are already many existing device trees and systems using this string.
> Removing or deprecating it right now might break those setups.

I'm not saying remove it. I'm just saying mark it as deprecated. For
properties you add an extra attribute, e.g.

https://elixir.bootlin.com/linux/v6.17.7/source/Documentation/devicetree/bindings/net/snps,dwmac.yaml#L433

but for a compatible, i've no idea if YAML supports it. However,
snps,dwmac.yam actually places st,spear600-gmac at the end of the list
after a comment. Maybe that is the best way to do this, and in the
comment you can explain what it gets wrong and why you should not use
it.

> In the future, if someone submits a new DTS for an AST2600-based platform,
> I think they should add the new compatible string and properly describe the TX/RX delay and 
> phy-mode properties in their DTS.

Yes, i agree. It would be good if you can keep on out of for such
patches, and make review comments. I assume you will also update the
vendor documentation with this recommendation.

	Andrew

[PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Jacky Chou]

> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            const: aspeed,ast2600-mac01
> > > > +    then:
> > > > +      properties:
> > > > +        rx-internal-delay-ps:
> > > > +          minimum: 0
> > > > +          maximum: 1395
> > > > +          multipleOf: 45
> > >
> > > I would add a default: 0
> > >
> >
> > Agreed.
> > I will add it in next version.
> >
> > > > +        tx-internal-delay-ps:
> > > > +          minimum: 0
> > > > +          maximum: 1395
> > > > +          multipleOf: 45
> > >
> > > and also here.
> > >
> >
> > Agreed.
> >
> > > > +      required:
> > > > +        - scu
> > > > +        - rx-internal-delay-ps
> > > > +        - tx-internal-delay-ps
> > >
> > > and then these are not required, but optional.
> > >
> >
> > Configure the tx/rx delay in the scu register.
> > At least, the scu handle must be required.
> 
> Sorry, i was unclear. By says 'and then', i was trying to chain it to the previous
> comment, that the delays should default to 0. With defaults set,
> rx-internal-delay-ps and tx-internal-delay-ps become optional. I agree scu is
> required.
> 
> > Here I have one question.
> > In v3 patches series, if the ftgmac driver cannot find one of
> > tx-internal-delay-ps and rx-internal-delay-ps, it will return error in
> > probe stage.
> > If here is optional, does it means that just add warning and not
> > return error when lack one of them and use the default to configure?
> > Or not configure tx/rx delay just return success in probe stage?
> 
> Once you add the default statement, it is clear what delay should be added if
> they property is not listed, 0. No warning is needed.
> 
> What you should find in the end is that most boards just set the new
> compatible and 'rgmii-id', and need nothing else. Only badly designed boards
> tend to need tx-internal-delay-ps and rx-internal-delay-ps.
> 

Thanks for the clarification and detailed explanation.

In the next version, I'll mark "aspeed,ast2600-mac" as deprecated and continue 
using "aspeed,ast2600-mac01" and "aspeed,ast2600-mac23".
I'll also add the default delay values for tx/rx-internal-delay-ps.

In the driver, if those properties are missing, it will use the default values for configuration.
Additionally, I'll treat aspeed-ast2600-evb.dts as a new board using the new
compatible and 'rgmii-id'.

Thanks again for your helpful feedback.

Thanks,
Jacky

[PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Jacky Chou]

> >>> Create the new compatibles to identify AST2600 MAC0/1 and MAC3/4.
> >>> Add conditional schema constraints for Aspeed AST2600 MAC controllers:
> >>> - For "aspeed,ast2600-mac01", require rx/tx-internal-delay-ps properties
> >>>   with 45ps step.
> >>> - For "aspeed,ast2600-mac23", require rx/tx-internal-delay-ps properties
> >>>   with 250ps step.
> >>
> >> That difference does not justify different compatibles. Basically you
> >> said they have same programming model, just different hardware
> >> characteristics, so same compatible.
> >>
> >
> > This change was originally based on feedback from a previous review
> discussion.
> > At that time, another reviewer suggested introducing separate
> > compatibles for
> > MAC0/1 and MAC2/3 on AST2600, since the delay characteristics differ
> > and they might not be fully compatible.
> 
> 
> Your commit msg does not provide enough of rationale for that.
> Difference in DTS properties is rather a counter argument for having separate
> compatibles. That's why you have these properties - to mark the difference.
> 

Actually, on the AST2600 there are two dies, and each die has its own MAC.
The MACs on these two dies indeed have different delay configurations.

Previously, the driver did not configure these delays — they were set earlier during 
the bootloader stage. Now, I’m planning to use the properties defined in 
ethernet-controller.yaml to configure these delays properly within the driver.

Since these legacy settings have been used for quite some time, I’d like to deprecate 
the old compatible and clearly distinguish that the AST2600 contains two different 
MACs. Future platforms based on the AST2600 will use the new compatibles with 
the correct PHY and delay configurations.

In the next revision, I’ll also include a detailed description explaining the reason for 
this change.

Thanks again for your review.

Thanks,
Jacky

Re: [PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Krzysztof Kozlowski]

On 06/11/2025 06:41, Jacky Chou wrote:
>>>>> Create the new compatibles to identify AST2600 MAC0/1 and MAC3/4.
>>>>> Add conditional schema constraints for Aspeed AST2600 MAC controllers:
>>>>> - For "aspeed,ast2600-mac01", require rx/tx-internal-delay-ps properties
>>>>>   with 45ps step.
>>>>> - For "aspeed,ast2600-mac23", require rx/tx-internal-delay-ps properties
>>>>>   with 250ps step.
>>>>
>>>> That difference does not justify different compatibles. Basically you
>>>> said they have same programming model, just different hardware
>>>> characteristics, so same compatible.
>>>>
>>>
>>> This change was originally based on feedback from a previous review
>> discussion.
>>> At that time, another reviewer suggested introducing separate
>>> compatibles for
>>> MAC0/1 and MAC2/3 on AST2600, since the delay characteristics differ
>>> and they might not be fully compatible.
>>
>>
>> Your commit msg does not provide enough of rationale for that.
>> Difference in DTS properties is rather a counter argument for having separate
>> compatibles. That's why you have these properties - to mark the difference.
>>
> 
> Actually, on the AST2600 there are two dies, and each die has its own MAC.
> The MACs on these two dies indeed have different delay configurations.

Is this the logic like: we have multiple snps,dw-apb-uart UARTs on the
device, so we need snps,dw-apb-uart-1, snps,dw-apb-uart-2 and
snps,dw-apb-uart-3?

> 
> Previously, the driver did not configure these delays — they were set earlier during 
> the bootloader stage. Now, I’m planning to use the properties defined in 
> ethernet-controller.yaml to configure these delays properly within the driver.
> 
> Since these legacy settings have been used for quite some time, I’d like to deprecate 
> the old compatible and clearly distinguish that the AST2600 contains two different 
> MACs. Future platforms based on the AST2600 will use the new compatibles with 
> the correct PHY and delay configurations.

Why are you repeating the same? So I will repeat the same. You need to
provide rationale why different compatible is justified. Difference in
delay itself is not the enough. Please write concise answer based on
device programming model differences or other rules expressed in writing
bindings or numerous presentations.

Best regards,
Krzysztof

[PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Jacky Chou]

> >>>>> Create the new compatibles to identify AST2600 MAC0/1 and MAC3/4.
> >>>>> Add conditional schema constraints for Aspeed AST2600 MAC
> controllers:
> >>>>> - For "aspeed,ast2600-mac01", require rx/tx-internal-delay-ps properties
> >>>>>   with 45ps step.
> >>>>> - For "aspeed,ast2600-mac23", require rx/tx-internal-delay-ps properties
> >>>>>   with 250ps step.
> >>>>
> >>>> That difference does not justify different compatibles. Basically
> >>>> you said they have same programming model, just different hardware
> >>>> characteristics, so same compatible.
> >>>>
> >>>
> >>> This change was originally based on feedback from a previous review
> >> discussion.
> >>> At that time, another reviewer suggested introducing separate
> >>> compatibles for
> >>> MAC0/1 and MAC2/3 on AST2600, since the delay characteristics differ
> >>> and they might not be fully compatible.
> >>
> >>
> >> Your commit msg does not provide enough of rationale for that.
> >> Difference in DTS properties is rather a counter argument for having
> >> separate compatibles. That's why you have these properties - to mark the
> difference.
> >>
> >
> > Actually, on the AST2600 there are two dies, and each die has its own MAC.
> > The MACs on these two dies indeed have different delay configurations.
> 
> Is this the logic like: we have multiple snps,dw-apb-uart UARTs on the device,
> so we need snps,dw-apb-uart-1, snps,dw-apb-uart-2 and snps,dw-apb-uart-3?
> 

You are right. That doesn't make sense..

> >
> > Previously, the driver did not configure these delays - they were set
> > earlier during the bootloader stage. Now, I'm planning to use the
> > properties defined in ethernet-controller.yaml to configure these delays
> properly within the driver.
> >
> > Since these legacy settings have been used for quite some time, I'd
> > like to deprecate the old compatible and clearly distinguish that the
> > AST2600 contains two different MACs. Future platforms based on the
> > AST2600 will use the new compatibles with the correct PHY and delay
> configurations.
> 
> Why are you repeating the same? So I will repeat the same. You need to
> provide rationale why different compatible is justified. Difference in delay itself
> is not the enough. Please write concise answer based on device programming
> model differences or other rules expressed in writing bindings or numerous
> presentations.
> 

I plan to remove the new compatible entry used to identify these MACs and instead 
add a new property to specify the delay step value.
However, I have one question I'd like to discuss with you.

There are four MACs in the AST2600. In the DT bindings and DTS files, what would be 
the recommended way to identify which MAC is which?
In version 3 of my patches, I used the aliases in the DTSI file to allow the driver to get 
the MAC index.

Do you think this is a good approach? Or would it be better to create a new property 
in the DTSI to explicitly configure the index and identify each MAC?

Thanks for your time and suggestions.

Thanks,
Jacky

Re: [PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Andrew Lunn]

> There are four MACs in the AST2600. In the DT bindings and DTS files, what would be 
> the recommended way to identify which MAC is which?
> In version 3 of my patches, I used the aliases in the DTSI file to allow the driver to get 
> the MAC index.

It is a bit ugly, but you are working around broken behaviour, so
sometimes you need to accept ugly. The addresses are fixed. You know
1e660000 is mac0, 1e680000 is mac1, etc. Put the addresses into the
driver, for compatible aspeed,ast2600-mac.

	Andrew

[PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Jacky Chou]

Hi Andrew

> > There are four MACs in the AST2600. In the DT bindings and DTS files,
> > what would be the recommended way to identify which MAC is which?
> > In version 3 of my patches, I used the aliases in the DTSI file to
> > allow the driver to get the MAC index.
> 
> It is a bit ugly, but you are working around broken behaviour, so sometimes you
> need to accept ugly. The addresses are fixed. You know
> 1e660000 is mac0, 1e680000 is mac1, etc. Put the addresses into the driver,
> for compatible aspeed,ast2600-mac.
> 

I used this fixed address as MAC index in the first version of this series.
But the other reviewer mentioned maybe there has the other better way to 
identify index.
https://lore.kernel.org/all/20250317095229.6f8754dd@fedora.home/
I find the "aliase", on preparing the v2 and v3, I think it may be a way to
do that. But I am not sure.
So, I would like to confirm the other good way before submitting the next
version.

Thanks,
Jacky

Re: [PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Andrew Lunn]

On Fri, Nov 07, 2025 at 01:12:08AM +0000, Jacky Chou wrote:
> Hi Andrew
> 
> > > There are four MACs in the AST2600. In the DT bindings and DTS files,
> > > what would be the recommended way to identify which MAC is which?
> > > In version 3 of my patches, I used the aliases in the DTSI file to
> > > allow the driver to get the MAC index.
> > 
> > It is a bit ugly, but you are working around broken behaviour, so sometimes you
> > need to accept ugly. The addresses are fixed. You know
> > 1e660000 is mac0, 1e680000 is mac1, etc. Put the addresses into the driver,
> > for compatible aspeed,ast2600-mac.
> > 
> 
> I used this fixed address as MAC index in the first version of this series.
> But the other reviewer mentioned maybe there has the other better way to 
> identify index.
> https://lore.kernel.org/all/20250317095229.6f8754dd@fedora.home/
> I find the "aliase", on preparing the v2 and v3, I think it may be a way to
> do that. But I am not sure.
> So, I would like to confirm the other good way before submitting the next
> version.

The problem with alias is that it normally a board property, in the
.dts file. A board might want a different mapping, which could then
break delays.

	Andrew

[PATCH net-next v3 1/4] dt-bindings: net: ftgmac100: Add delay properties for AST2600

[Jacky Chou]

> > > > There are four MACs in the AST2600. In the DT bindings and DTS
> > > > files, what would be the recommended way to identify which MAC is
> which?
> > > > In version 3 of my patches, I used the aliases in the DTSI file to
> > > > allow the driver to get the MAC index.
> > >
> > > It is a bit ugly, but you are working around broken behaviour, so
> > > sometimes you need to accept ugly. The addresses are fixed. You know
> > > 1e660000 is mac0, 1e680000 is mac1, etc. Put the addresses into the
> > > driver, for compatible aspeed,ast2600-mac.
> > >
> >
> > I used this fixed address as MAC index in the first version of this series.
> > But the other reviewer mentioned maybe there has the other better way
> > to identify index.
> > https://lore.kernel.org/all/20250317095229.6f8754dd@fedora.home/
> > I find the "aliase", on preparing the v2 and v3, I think it may be a
> > way to do that. But I am not sure.
> > So, I would like to confirm the other good way before submitting the
> > next version.
> 
> The problem with alias is that it normally a board property, in the .dts file. A
> board might want a different mapping, which could then break delays.
> 

Agreed. I looked through ethernet-controller.yaml, but it doesn't seem to have 
any property that fits our needs.
I will use the fixed-address value from the reg property to identify the index of 
MACs to configure the RGMII delay in next version.

Thanks,
Jacky