[PATCH 0/3] Support tuning the RX sampling swap of the MAC.

[PATCH 0/3] Support tuning the RX sampling swap of the MAC.

[Yijie Yang]

The Ethernet MAC requires precise sampling times at Rx, but signals on the
Rx side after transmission on the board may vary due to different hardware
layouts. The RGMII_CONFIG2_RX_PROG_SWAP can be used to switch the sampling
occasion between the rising edge and falling edge of the clock to meet the
sampling requirements. Consequently, the configuration of this bit in the
Ethernet MAC can vary between boards, even if they are of the same version.
It should be adjustable rather than simply determined by the version. For
example, the MAC version is less than 3, but it needs to enable this bit.
Therefore, this patch set introduces a new flag for each board to control
whether to open it.
The dependency patch set detailed below has introduced and enabled an
Ethernet node that supports 1G speed on qcs615. The current patch set now
allows tuning of the MAC's RX swap, thereby supporting 10M and 100M speeds.

Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
---
This patch series depends on below patch series:
https://lore.kernel.org/all/20241118-dts_qcs615-v2-0-e62b924a3cbd@quicinc.com/

---
Yijie Yang (3):
      dt-bindings: net: stmmac: Tune rx sampling occasion
      net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615
      arm64: dts: qcom: qcs615-ride: Enable RX programmable swap on qcs615-ride

 .../devicetree/bindings/net/qcom,ethqos.yaml       |  6 ++++
 arch/arm64/boot/dts/qcom/qcs615-ride.dts           |  1 +
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 36 ++++++++++++----------
 3 files changed, 27 insertions(+), 16 deletions(-)
---
base-commit: 532900dbb7c1be5c9e6aab322d9af3a583888f25
change-id: 20241217-support_10m100m-16239916fa12
prerequisite-message-id: <20241118-dts_qcs615-v2-0-e62b924a3cbd@quicinc.com>
prerequisite-patch-id: ab55582f3bfce00f051fddd75bb66b2ef5e0677d
prerequisite-patch-id: 514acd303f0ef816ff6e61e59ecbaaff7f1b06ec

Best regards,
-- 
Yijie Yang <quic_yijiyang@quicinc.com>

[PATCH 1/3] dt-bindings: net: stmmac: Tune rx sampling occasion

[Yijie Yang]

Add documentation detailing the capability for tuning the RX sampling
occasion of the Qualcomm MAC. Introduce a new flag to indicate that a
board requires switching the timing of signal sampling, instead of relying
solely on the EMAC version in the driver code. For older DTS, the logic
for current boards is hard-coded in the driver code to ensure they
function as previously.

Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
---
 Documentation/devicetree/bindings/net/qcom,ethqos.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
index f117471fb06fb3b507df811d55d41d0b610ac15f..dab6d9ccfa9a0b64e314a4cca84f25b97f6cee40 100644
--- a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
@@ -78,6 +78,12 @@ properties:
   phy-names:
     const: serdes
 
+  qcom,rx-prog-swap:
+    type: boolean
+    description:
+      Swap the sampling occasion on the RX side. This can be used for
+      tuning when connected to a third-party PHY.
+
 required:
   - compatible
   - clocks

-- 
2.34.1

[PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Yijie Yang]

The sampling timing of the MAC varies per board due to differences in
hardware layout or peer PHY, even within the same version. For instance,
the EMAC version on the qcs615-ride board is below 3, but it requires
swapping the RX timing to enable 10M/100M speeds. Therefore, this setting
should be adjustable rather than solely determined by the version.
The RGMII_CONFIG2_RX_PROG_SWAP bit is used to switch the RX sampling
timing between the rising edge and the falling edge of the clock.
Consequently, the condition for setting this bit should be revised to
ensure correct data sampling.
The compatible string matching for 'qcom,sa8540p-ride' in this change is
intended to ensure ABI compatibility for DTS files that do not include the
new 'qcom,rx-prog-swap' property, allowing legacy boards to function as
before. This board is the only one among legacy boards using RGMII and
needs to enable RX swap.

Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
---
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 36 ++++++++++++----------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..cf9633489975bc89480d065ae723a92723a12b04 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -120,6 +120,7 @@ struct qcom_ethqos {
 	bool rgmii_config_loopback_en;
 	bool has_emac_ge_3;
 	bool needs_sgmii_loopback;
+	bool needs_rx_prog_swap;
 };
 
 static int rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
@@ -401,6 +402,7 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
 static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
 {
 	struct device *dev = &ethqos->pdev->dev;
+	int rx_prog_swap = 0;
 	int phase_shift;
 	int loopback;
 
@@ -421,6 +423,9 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
 	else
 		loopback = 0;
 
+	if (ethqos->needs_rx_prog_swap)
+		rx_prog_swap = RGMII_CONFIG2_RX_PROG_SWAP;
+
 	/* Select RGMII, write 0 to interface select */
 	rgmii_updatel(ethqos, RGMII_CONFIG_INTF_SEL,
 		      0, RGMII_IO_MACRO_CONFIG);
@@ -484,14 +489,8 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
 			      BIT(6), RGMII_IO_MACRO_CONFIG);
 		rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15,
 			      0, RGMII_IO_MACRO_CONFIG2);
-
-		if (ethqos->has_emac_ge_3)
-			rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
-				      RGMII_CONFIG2_RX_PROG_SWAP,
-				      RGMII_IO_MACRO_CONFIG2);
-		else
-			rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
-				      0, RGMII_IO_MACRO_CONFIG2);
+		rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, rx_prog_swap,
+			      RGMII_IO_MACRO_CONFIG2);
 
 		/* Write 0x5 to PRG_RCLK_DLY_CODE */
 		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE,
@@ -525,13 +524,9 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
 			      RGMII_IO_MACRO_CONFIG);
 		rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15,
 			      0, RGMII_IO_MACRO_CONFIG2);
-		if (ethqos->has_emac_ge_3)
-			rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
-				      RGMII_CONFIG2_RX_PROG_SWAP,
-				      RGMII_IO_MACRO_CONFIG2);
-		else
-			rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
-				      0, RGMII_IO_MACRO_CONFIG2);
+		rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, rx_prog_swap,
+			      RGMII_IO_MACRO_CONFIG2);
+
 		/* Write 0x5 to PRG_RCLK_DLY_CODE */
 		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE,
 			      (BIT(29) | BIT(27)), SDCC_HC_REG_DDR_CONFIG);
@@ -782,7 +777,7 @@ static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
 
 static int qcom_ethqos_probe(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node, *root;
 	const struct ethqos_emac_driver_data *data;
 	struct plat_stmmacenet_data *plat_dat;
 	struct stmmac_resources stmmac_res;
@@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 	ret = of_get_phy_mode(np, &ethqos->phy_mode);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
+
+	root = of_find_node_by_path("/");
+	if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
+		ethqos->needs_rx_prog_swap = true;
+	else
+		ethqos->needs_rx_prog_swap =
+			of_property_read_bool(np, "qcom,rx-prog-swap");
+	of_node_put(root);
+
 	switch (ethqos->phy_mode) {
 	case PHY_INTERFACE_MODE_RGMII:
 	case PHY_INTERFACE_MODE_RGMII_ID:

-- 
2.34.1

[PATCH 3/3] arm64: dts: qcom: qcs615-ride: Enable RX programmable swap on qcs615-ride

[Yijie Yang]

The timing of sampling at the RX side for qcs615-ride needs adjustment.
It varies from board to board.

Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qcs615-ride.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
index bfb5de4a0d440efece993dbf7a0001e001d5469b..f22a4a0b247a09bd1057b66203a34b666cd119a8 100644
--- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
+++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
@@ -206,6 +206,7 @@ &ethernet {
 	phy-handle = <&rgmii_phy>;
 	phy-mode = "rgmii";
 	max-speed = <1000>;
+	qcom,rx-prog-swap;
 
 	snps,mtl-rx-config = <&mtl_rx_setup>;
 	snps,mtl-tx-config = <&mtl_tx_setup>;

-- 
2.34.1

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Krzysztof Kozlowski]

On 25/12/2024 11:04, Yijie Yang wrote:

>  static int qcom_ethqos_probe(struct platform_device *pdev)
>  {
> -	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *np = pdev->dev.of_node, *root;
>  	const struct ethqos_emac_driver_data *data;
>  	struct plat_stmmacenet_data *plat_dat;
>  	struct stmmac_resources stmmac_res;
> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
> +
> +	root = of_find_node_by_path("/");
> +	if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))


Nope, your drivers are not supposed to poke root compatibles. Drop and
fix your driver to behave correctly for all existing devices.

> +		ethqos->needs_rx_prog_swap = true;
> +	else
> +		ethqos->needs_rx_prog_swap =
Best regards,
Krzysztof

Re: [PATCH 3/3] arm64: dts: qcom: qcs615-ride: Enable RX programmable swap on qcs615-ride

[Andrew Lunn]

On Wed, Dec 25, 2024 at 06:04:47PM +0800, Yijie Yang wrote:
> The timing of sampling at the RX side for qcs615-ride needs adjustment.
> It varies from board to board.
> 
> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs615-ride.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
> index bfb5de4a0d440efece993dbf7a0001e001d5469b..f22a4a0b247a09bd1057b66203a34b666cd119a8 100644
> --- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
> @@ -206,6 +206,7 @@ &ethernet {
>  	phy-handle = <&rgmii_phy>;
>  	phy-mode = "rgmii";
>  	max-speed = <1000>;
> +	qcom,rx-prog-swap;

I notice this board still has messed up rgmii delays, using phy-mode =
"rgmii", not "rgmii-id". How does com,rx-prog-swap interact with rgmii
delays? Is the sample point logic before or after the rgmii delay
logic in the MAC clock pipeline?

I think i also questioned max-speed = <1000>. Has this
arch/arm64/boot/dts/qcom/qcs615-ride.dts been merged?

	Andrew

Re: [PATCH 0/3] Support tuning the RX sampling swap of the MAC.

[Andrew Lunn]

On Wed, Dec 25, 2024 at 06:04:44PM +0800, Yijie Yang wrote:
> The Ethernet MAC requires precise sampling times at Rx, but signals on the
> Rx side after transmission on the board may vary due to different hardware
> layouts. The RGMII_CONFIG2_RX_PROG_SWAP can be used to switch the sampling
> occasion between the rising edge and falling edge of the clock to meet the
> sampling requirements.

The RGMII specification says that RD[3:0] pins are sampled on the
rising edge for bits 3:0 and falling edge for bits 7:4.

Given this is part of the standard, why would you want to do anything
else?

Is this maybe another symptom of having the RGMII delays messed up?

Anyway, i don't see a need for this property, unless you are working
with a PHY which breaks the RGMII standard, and has its clock
reversed?

	Andrew

Re: [PATCH 3/3] arm64: dts: qcom: qcs615-ride: Enable RX programmable swap on qcs615-ride

[Yijie Yang]

On 2024-12-26 01:38, Andrew Lunn wrote:
> On Wed, Dec 25, 2024 at 06:04:47PM +0800, Yijie Yang wrote:
>> The timing of sampling at the RX side for qcs615-ride needs adjustment.
>> It varies from board to board.
>>
>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/qcs615-ride.dts | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
>> index bfb5de4a0d440efece993dbf7a0001e001d5469b..f22a4a0b247a09bd1057b66203a34b666cd119a8 100644
>> --- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
>> +++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
>> @@ -206,6 +206,7 @@ &ethernet {
>>   	phy-handle = <&rgmii_phy>;
>>   	phy-mode = "rgmii";
>>   	max-speed = <1000>;
>> +	qcom,rx-prog-swap;
> 
> I notice this board still has messed up rgmii delays, using phy-mode =
> "rgmii", not "rgmii-id". How does com,rx-prog-swap interact with rgmii
> delays? Is the sample point logic before or after the rgmii delay
> logic in the MAC clock pipeline?

This patch set relies on an earlier version that has RGMII issues. The 
latter is still undergoing coding and verification. I will update this 
patch set once the RGMII issues are resolved and uploaded.

> 
> I think i also questioned max-speed = <1000>. Has this
> arch/arm64/boot/dts/qcom/qcs615-ride.dts been merged?

This will also be updated in the next version.

> 
> 	Andrew

-- 
Best Regards,
Yijie

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Yijie Yang]

On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
> On 25/12/2024 11:04, Yijie Yang wrote:
> 
>>   static int qcom_ethqos_probe(struct platform_device *pdev)
>>   {
>> -	struct device_node *np = pdev->dev.of_node;
>> +	struct device_node *np = pdev->dev.of_node, *root;
>>   	const struct ethqos_emac_driver_data *data;
>>   	struct plat_stmmacenet_data *plat_dat;
>>   	struct stmmac_resources stmmac_res;
>> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>   	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>   	if (ret)
>>   		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>> +
>> +	root = of_find_node_by_path("/");
>> +	if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
> 
> 
> Nope, your drivers are not supposed to poke root compatibles. Drop and
> fix your driver to behave correctly for all existing devices.
> 

Since this change introduces a new flag in the DTS, we must maintain ABI 
compatibility with the kernel. The new flag is specific to the board, so 
I need to ensure root nodes are matched to allow older boards to 
continue functioning as before. I'm happy to adopt that approach if 
there are any more elegant solutions.

>> +		ethqos->needs_rx_prog_swap = true;
>> +	else
>> +		ethqos->needs_rx_prog_swap =
> Best regards,
> Krzysztof

-- 
Best Regards,
Yijie

Re: [PATCH 0/3] Support tuning the RX sampling swap of the MAC.

[Yijie Yang]

On 2024-12-26 01:49, Andrew Lunn wrote:
> On Wed, Dec 25, 2024 at 06:04:44PM +0800, Yijie Yang wrote:
>> The Ethernet MAC requires precise sampling times at Rx, but signals on the
>> Rx side after transmission on the board may vary due to different hardware
>> layouts. The RGMII_CONFIG2_RX_PROG_SWAP can be used to switch the sampling
>> occasion between the rising edge and falling edge of the clock to meet the
>> sampling requirements.
> 
> The RGMII specification says that RD[3:0] pins are sampled on the
> rising edge for bits 3:0 and falling edge for bits 7:4.
> 
> Given this is part of the standard, why would you want to do anything
> else?
> 
> Is this maybe another symptom of having the RGMII delays messed up?
> 
> Anyway, i don't see a need for this property, unless you are working
> with a PHY which breaks the RGMII standard, and has its clock
> reversed?

Please correct me if there are any errors. As described in the Intel and 
TI design guidelines, Dual Data Rate (DDR), which samples at both edges 
of the clock, is primarily used for 1Gbps speeds. For 100Mbps and 10Mbps 
speeds, Single Data Rate (SDR), which samples at the rising edge of the 
clock, is typically adopted.
This patch set introduces such a flag mainly for 100M/10M speeds, as 
described in the cover letter.

> 
> 	Andrew

-- 
Best Regards,
Yijie

Re: [PATCH 0/3] Support tuning the RX sampling swap of the MAC.

[Andrew Lunn]

On Thu, Dec 26, 2024 at 11:06:48AM +0800, Yijie Yang wrote:
> 
> 
> On 2024-12-26 01:49, Andrew Lunn wrote:
> > On Wed, Dec 25, 2024 at 06:04:44PM +0800, Yijie Yang wrote:
> > > The Ethernet MAC requires precise sampling times at Rx, but signals on the
> > > Rx side after transmission on the board may vary due to different hardware
> > > layouts. The RGMII_CONFIG2_RX_PROG_SWAP can be used to switch the sampling
> > > occasion between the rising edge and falling edge of the clock to meet the
> > > sampling requirements.
> > 
> > The RGMII specification says that RD[3:0] pins are sampled on the
> > rising edge for bits 3:0 and falling edge for bits 7:4.
> > 
> > Given this is part of the standard, why would you want to do anything
> > else?
> > 
> > Is this maybe another symptom of having the RGMII delays messed up?
> > 
> > Anyway, i don't see a need for this property, unless you are working
> > with a PHY which breaks the RGMII standard, and has its clock
> > reversed?
> 
> Please correct me if there are any errors. As described in the Intel and TI
> design guidelines, Dual Data Rate (DDR), which samples at both edges of the
> clock, is primarily used for 1Gbps speeds. For 100Mbps and 10Mbps speeds,
> Single Data Rate (SDR), which samples at the rising edge of the clock, is
> typically adopted.

If it is typically adopted, why do you need to support falling edge?
Because we can is not a good reason. Do you have a board with a PHY
which requires falling edge for some reason?

	Andrew

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Andrew Lunn]

On Thu, Dec 26, 2024 at 10:29:45AM +0800, Yijie Yang wrote:
> 
> 
> On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
> > On 25/12/2024 11:04, Yijie Yang wrote:
> > 
> > >   static int qcom_ethqos_probe(struct platform_device *pdev)
> > >   {
> > > -	struct device_node *np = pdev->dev.of_node;
> > > +	struct device_node *np = pdev->dev.of_node, *root;
> > >   	const struct ethqos_emac_driver_data *data;
> > >   	struct plat_stmmacenet_data *plat_dat;
> > >   	struct stmmac_resources stmmac_res;
> > > @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
> > >   	ret = of_get_phy_mode(np, &ethqos->phy_mode);
> > >   	if (ret)
> > >   		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
> > > +
> > > +	root = of_find_node_by_path("/");
> > > +	if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
> > 
> > 
> > Nope, your drivers are not supposed to poke root compatibles. Drop and
> > fix your driver to behave correctly for all existing devices.
> > 
> 
> Since this change introduces a new flag in the DTS, we must maintain ABI
> compatibility with the kernel. The new flag is specific to the board, so I
> need to ensure root nodes are matched to allow older boards to continue
> functioning as before. I'm happy to adopt that approach if there are any
> more elegant solutions.

Why is it specific to this board? Does the board have a PHY which is
broken and requires this property? What we are missing are the details
needed to help you get to the correct way to solve the problem you are
facing.

	Andrew

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Krzysztof Kozlowski]

On 26/12/2024 03:29, Yijie Yang wrote:
> 
> 
> On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
>> On 25/12/2024 11:04, Yijie Yang wrote:
>>
>>>   static int qcom_ethqos_probe(struct platform_device *pdev)
>>>   {
>>> -	struct device_node *np = pdev->dev.of_node;
>>> +	struct device_node *np = pdev->dev.of_node, *root;
>>>   	const struct ethqos_emac_driver_data *data;
>>>   	struct plat_stmmacenet_data *plat_dat;
>>>   	struct stmmac_resources stmmac_res;
>>> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>   	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>>   	if (ret)
>>>   		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>> +
>>> +	root = of_find_node_by_path("/");
>>> +	if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
>>
>>
>> Nope, your drivers are not supposed to poke root compatibles. Drop and
>> fix your driver to behave correctly for all existing devices.
>>
> 
> Since this change introduces a new flag in the DTS, we must maintain ABI 
> compatibility with the kernel. The new flag is specific to the board, so 

It's not, I don't see it specific to the board in the bindings.

> I need to ensure root nodes are matched to allow older boards to 
> continue functioning as before. I'm happy to adopt that approach if 
> there are any more elegant solutions.

I don't think you understood the problem. Why you are not handling this
for my board, sa8775p-rideX and sa8225-pre-ride-yellow-shrimp?


Best regards,
Krzysztof

Re: [PATCH 0/3] Support tuning the RX sampling swap of the MAC.

[Rob Herring (Arm)]

On Wed, 25 Dec 2024 18:04:44 +0800, Yijie Yang wrote:
> The Ethernet MAC requires precise sampling times at Rx, but signals on the
> Rx side after transmission on the board may vary due to different hardware
> layouts. The RGMII_CONFIG2_RX_PROG_SWAP can be used to switch the sampling
> occasion between the rising edge and falling edge of the clock to meet the
> sampling requirements. Consequently, the configuration of this bit in the
> Ethernet MAC can vary between boards, even if they are of the same version.
> It should be adjustable rather than simply determined by the version. For
> example, the MAC version is less than 3, but it needs to enable this bit.
> Therefore, this patch set introduces a new flag for each board to control
> whether to open it.
> The dependency patch set detailed below has introduced and enabled an
> Ethernet node that supports 1G speed on qcs615. The current patch set now
> allows tuning of the MAC's RX swap, thereby supporting 10M and 100M speeds.
> 
> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
> ---
> This patch series depends on below patch series:
> https://lore.kernel.org/all/20241118-dts_qcs615-v2-0-e62b924a3cbd@quicinc.com/
> 
> ---
> Yijie Yang (3):
>       dt-bindings: net: stmmac: Tune rx sampling occasion
>       net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615
>       arm64: dts: qcom: qcs615-ride: Enable RX programmable swap on qcs615-ride
> 
>  .../devicetree/bindings/net/qcom,ethqos.yaml       |  6 ++++
>  arch/arm64/boot/dts/qcom/qcs615-ride.dts           |  1 +
>  .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 36 ++++++++++++----------
>  3 files changed, 27 insertions(+), 16 deletions(-)
> ---
> base-commit: 532900dbb7c1be5c9e6aab322d9af3a583888f25
> change-id: 20241217-support_10m100m-16239916fa12
> prerequisite-message-id: <20241118-dts_qcs615-v2-0-e62b924a3cbd@quicinc.com>
> prerequisite-patch-id: ab55582f3bfce00f051fddd75bb66b2ef5e0677d
> prerequisite-patch-id: 514acd303f0ef816ff6e61e59ecbaaff7f1b06ec
> 
> Best regards,
> --
> Yijie Yang <quic_yijiyang@quicinc.com>
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y qcom/qcs615-ride.dtb' for 20241225-support_10m100m-v1-0-4b52ef48b488@quicinc.com:

arch/arm64/boot/dts/qcom/qcs615-ride.dtb: ethernet@20000: compatible: ['qcom,qcs615-ethqos'] does not contain items matching the given schema
	from schema $id: http://devicetree.org/schemas/net/qcom,ethqos.yaml#
arch/arm64/boot/dts/qcom/qcs615-ride.dtb: ethernet@20000: snps,tso: False schema does not allow True
	from schema $id: http://devicetree.org/schemas/net/qcom,ethqos.yaml#
arch/arm64/boot/dts/qcom/qcs615-ride.dtb: ethernet@20000: compatible: 'oneOf' conditional failed, one must be fixed:
	['qcom,qcs615-ethqos'] is too short
	'qcom,qcs615-ethqos' is not one of ['qcom,qcs8300-ethqos']
	'qcom,qcs615-ethqos' is not one of ['qcom,qcs404-ethqos', 'qcom,sa8775p-ethqos', 'qcom,sc8280xp-ethqos', 'qcom,sm8150-ethqos']
	from schema $id: http://devicetree.org/schemas/net/qcom,ethqos.yaml#
arch/arm64/boot/dts/qcom/qcs615-ride.dtb: ethernet@20000: Unevaluated properties are not allowed ('compatible', 'max-speed', 'mdio', 'phy-handle', 'phy-mode', 'power-domains', 'resets', 'rx-fifo-depth', 'rx-queues-config', 'snps,mtl-rx-config', 'snps,mtl-tx-config', 'snps,pbl', 'snps,tso', 'tx-fifo-depth', 'tx-queues-config' were unexpected)
	from schema $id: http://devicetree.org/schemas/net/qcom,ethqos.yaml#

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Yijie Yang]

On 2024-12-27 01:21, Andrew Lunn wrote:
> On Thu, Dec 26, 2024 at 10:29:45AM +0800, Yijie Yang wrote:
>>
>>
>> On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
>>> On 25/12/2024 11:04, Yijie Yang wrote:
>>>
>>>>    static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>    {
>>>> -	struct device_node *np = pdev->dev.of_node;
>>>> +	struct device_node *np = pdev->dev.of_node, *root;
>>>>    	const struct ethqos_emac_driver_data *data;
>>>>    	struct plat_stmmacenet_data *plat_dat;
>>>>    	struct stmmac_resources stmmac_res;
>>>> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>    	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>>>    	if (ret)
>>>>    		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>>> +
>>>> +	root = of_find_node_by_path("/");
>>>> +	if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
>>>
>>>
>>> Nope, your drivers are not supposed to poke root compatibles. Drop and
>>> fix your driver to behave correctly for all existing devices.
>>>
>>
>> Since this change introduces a new flag in the DTS, we must maintain ABI
>> compatibility with the kernel. The new flag is specific to the board, so I
>> need to ensure root nodes are matched to allow older boards to continue
>> functioning as before. I'm happy to adopt that approach if there are any
>> more elegant solutions.
> 
> Why is it specific to this board? Does the board have a PHY which is
> broken and requires this property? What we are missing are the details
> needed to help you get to the correct way to solve the problem you are
> facing.
> 

Let me clarify why this bit is necessary and why it's board-specific. 
The RX programming swap bit can introduce a time delay of half a clock 
cycle. This bit, along with the clock delay adjustment functionality, is 
implemented by a module called 'IO Macro.' This is a Qualcomm-specific 
hardware design located between the MAC and PHY in the SoC, serving the 
RGMII interface. The bit works in conjunction with delay adjustment to 
meet the sampling requirements. The sampling of RX data is also handled 
by this module.

During the board design stage, the RGMII requirements may not have been 
strictly followed, leading to uncertainty in the relationship between 
the clock and data waveforms when they reach the IO Macro. This means 
the time delay introduced by the PC board may not be zero. Therefore, 
it's necessary for software developers to tune both the RX programming 
swap bit and the delay to ensure correct sampling.

> 	Andrew
> 

-- 
Best Regards,
Yijie

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Yijie Yang]

On 2024-12-27 15:03, Krzysztof Kozlowski wrote:
> On 26/12/2024 03:29, Yijie Yang wrote:
>>
>>
>> On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
>>> On 25/12/2024 11:04, Yijie Yang wrote:
>>>
>>>>    static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>    {
>>>> -	struct device_node *np = pdev->dev.of_node;
>>>> +	struct device_node *np = pdev->dev.of_node, *root;
>>>>    	const struct ethqos_emac_driver_data *data;
>>>>    	struct plat_stmmacenet_data *plat_dat;
>>>>    	struct stmmac_resources stmmac_res;
>>>> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>    	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>>>    	if (ret)
>>>>    		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>>> +
>>>> +	root = of_find_node_by_path("/");
>>>> +	if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
>>>
>>>
>>> Nope, your drivers are not supposed to poke root compatibles. Drop and
>>> fix your driver to behave correctly for all existing devices.
>>>
>>
>> Since this change introduces a new flag in the DTS, we must maintain ABI
>> compatibility with the kernel. The new flag is specific to the board, so
> 
> It's not, I don't see it specific to the board in the bindings.

I'm sorry for the confusion. This feature is not board-specific but 
rather a tunable option. All RGMII boards can choose whether to enable 
this bit in the DTS, so there are no restrictions in the binding.

> 
>> I need to ensure root nodes are matched to allow older boards to
>> continue functioning as before. I'm happy to adopt that approach if
>> there are any more elegant solutions.
> 
> I don't think you understood the problem. Why you are not handling this
> for my board, sa8775p-rideX and sa8225-pre-ride-yellow-shrimp?
> 

This feature is specifically for RGMII boards. The driver won't enable 
this bit if the DTS doesn't specify it. To handle compatibility, we need 
to identify legacy RGMII boards with MAC versions greater or equal to 3 
which require this bit to be enabled.
According to my knowledge, the SA8775P is of the SGMII type.

> 
> Best regards,
> Krzysztof

-- 
Best Regards,
Yijie

Re: [PATCH 0/3] Support tuning the RX sampling swap of the MAC.

[Yijie Yang]

On 2024-12-27 01:14, Andrew Lunn wrote:
> On Thu, Dec 26, 2024 at 11:06:48AM +0800, Yijie Yang wrote:
>>
>>
>> On 2024-12-26 01:49, Andrew Lunn wrote:
>>> On Wed, Dec 25, 2024 at 06:04:44PM +0800, Yijie Yang wrote:
>>>> The Ethernet MAC requires precise sampling times at Rx, but signals on the
>>>> Rx side after transmission on the board may vary due to different hardware
>>>> layouts. The RGMII_CONFIG2_RX_PROG_SWAP can be used to switch the sampling
>>>> occasion between the rising edge and falling edge of the clock to meet the
>>>> sampling requirements.
>>>
>>> The RGMII specification says that RD[3:0] pins are sampled on the
>>> rising edge for bits 3:0 and falling edge for bits 7:4.
>>>
>>> Given this is part of the standard, why would you want to do anything
>>> else?
>>>
>>> Is this maybe another symptom of having the RGMII delays messed up?
>>>
>>> Anyway, i don't see a need for this property, unless you are working
>>> with a PHY which breaks the RGMII standard, and has its clock
>>> reversed?
>>
>> Please correct me if there are any errors. As described in the Intel and TI
>> design guidelines, Dual Data Rate (DDR), which samples at both edges of the
>> clock, is primarily used for 1Gbps speeds. For 100Mbps and 10Mbps speeds,
>> Single Data Rate (SDR), which samples at the rising edge of the clock, is
>> typically adopted.
> 
> If it is typically adopted, why do you need to support falling edge?
> Because we can is not a good reason. Do you have a board with a PHY
> which requires falling edge for some reason?

It's an RX-side feature and is unrelated to the PHY. As I mentioned in 
another email, it's designed to introduce a half-clock cycle delay for 
correct sampling in the IO Macro.

> 
> 	Andrew
> 

-- 
Best Regards,
Yijie

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Andrew Lunn]

> > Why is it specific to this board? Does the board have a PHY which is
> > broken and requires this property? What we are missing are the details
> > needed to help you get to the correct way to solve the problem you are
> > facing.
> > 
> 
> Let me clarify why this bit is necessary and why it's board-specific. The RX
> programming swap bit can introduce a time delay of half a clock cycle. This
> bit, along with the clock delay adjustment functionality, is implemented by
> a module called 'IO Macro.' This is a Qualcomm-specific hardware design
> located between the MAC and PHY in the SoC, serving the RGMII interface. The
> bit works in conjunction with delay adjustment to meet the sampling
> requirements. The sampling of RX data is also handled by this module.
> 
> During the board design stage, the RGMII requirements may not have been
> strictly followed, leading to uncertainty in the relationship between the
> clock and data waveforms when they reach the IO Macro.

So this indicates any board might need this feature, not just this one
board. Putting the board name in the driver then does not scale.

> This means the time
> delay introduced by the PC board may not be zero. Therefore, it's necessary
> for software developers to tune both the RX programming swap bit and the
> delay to ensure correct sampling.

O.K. Now look at how other boards tune their delays. There are
standard properties for this:

        rx-internal-delay-ps:
          description:
            RGMII Receive Clock Delay defined in pico seconds. This is used for
            controllers that have configurable RX internal delays. If this
            property is present then the MAC applies the RX delay.
        tx-internal-delay-ps:
          description:
            RGMII Transmit Clock Delay defined in pico seconds. This is used for
            controllers that have configurable TX internal delays. If this
            property is present then the MAC applies the TX delay.

I think you can use these properties, maybe with an additional comment
in the binding. RGMII running at 1G has a clock of 125MHz. That is a
period of 8ns. So a half clock cycle delay is then 4ns.

So an rx-internal-delay-ps of 0-2000 means this clock invert should be
disabled. A rx-internal-delay-ps of 4000-6000 means the clock invert
should be enabled.

Now, ideally, you want the PHY to add the RGMII delays, that is what i
request all MAC/PHY pairs do, so we have a uniform setup across all
boards. So unless the PHY does not support RGMII delays, you would
expect rx-internal-delay-ps to be either just a small number of
picoseconds for fine tuning, or a small number of picoseconds + 4ns
for fine tuning.

This scales, since it can be used by an board with poor design, and it
does not require anything proprietary to Qualcomm, except the extended
range, and hopefully nobody except Qualcomms broken RDK will require
it, because obviously you will document the issue with the RDK and
tell customers how to correctly design their board to be RGMII
compliant with the clocks.

	Andrew

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Krzysztof Kozlowski]

On 08/01/2025 11:33, Yijie Yang wrote:
> 
> 
> On 2024-12-27 15:03, Krzysztof Kozlowski wrote:
>> On 26/12/2024 03:29, Yijie Yang wrote:
>>>
>>>
>>> On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
>>>> On 25/12/2024 11:04, Yijie Yang wrote:
>>>>
>>>>>    static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>>    {
>>>>> -	struct device_node *np = pdev->dev.of_node;
>>>>> +	struct device_node *np = pdev->dev.of_node, *root;
>>>>>    	const struct ethqos_emac_driver_data *data;
>>>>>    	struct plat_stmmacenet_data *plat_dat;
>>>>>    	struct stmmac_resources stmmac_res;
>>>>> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>>    	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>>>>    	if (ret)
>>>>>    		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>>>> +
>>>>> +	root = of_find_node_by_path("/");
>>>>> +	if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
>>>>
>>>>
>>>> Nope, your drivers are not supposed to poke root compatibles. Drop and
>>>> fix your driver to behave correctly for all existing devices.
>>>>
>>>
>>> Since this change introduces a new flag in the DTS, we must maintain ABI
>>> compatibility with the kernel. The new flag is specific to the board, so
>>
>> It's not, I don't see it specific to the board in the bindings.
> 
> I'm sorry for the confusion. This feature is not board-specific but 
> rather a tunable option. All RGMII boards can choose whether to enable 
> this bit in the DTS, so there are no restrictions in the binding.

If it is not specific to the board, I don't see why this cannot be
implied by compatible.

> 
>>
>>> I need to ensure root nodes are matched to allow older boards to
>>> continue functioning as before. I'm happy to adopt that approach if
>>> there are any more elegant solutions.
>>
>> I don't think you understood the problem. Why you are not handling this
>> for my board, sa8775p-rideX and sa8225-pre-ride-yellow-shrimp?
>>
> 
> This feature is specifically for RGMII boards. The driver won't enable 

So board specific?

> this bit if the DTS doesn't specify it. To handle compatibility, we need 

Do not describe us how drivers and DTS work. We all know.

> to identify legacy RGMII boards with MAC versions greater or equal to 3 
> which require this bit to be enabled.
> According to my knowledge, the SA8775P is of the SGMII type.


Best regards,
Krzysztof

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Yijie Yang]

On 2025-01-13 19:26, Krzysztof Kozlowski wrote:
> On 08/01/2025 11:33, Yijie Yang wrote:
>>
>>
>> On 2024-12-27 15:03, Krzysztof Kozlowski wrote:
>>> On 26/12/2024 03:29, Yijie Yang wrote:
>>>>
>>>>
>>>> On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
>>>>> On 25/12/2024 11:04, Yijie Yang wrote:
>>>>>
>>>>>>     static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>>>     {
>>>>>> -	struct device_node *np = pdev->dev.of_node;
>>>>>> +	struct device_node *np = pdev->dev.of_node, *root;
>>>>>>     	const struct ethqos_emac_driver_data *data;
>>>>>>     	struct plat_stmmacenet_data *plat_dat;
>>>>>>     	struct stmmac_resources stmmac_res;
>>>>>> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>>>     	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>>>>>     	if (ret)
>>>>>>     		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>>>>> +
>>>>>> +	root = of_find_node_by_path("/");
>>>>>> +	if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
>>>>>
>>>>>
>>>>> Nope, your drivers are not supposed to poke root compatibles. Drop and
>>>>> fix your driver to behave correctly for all existing devices.
>>>>>
>>>>
>>>> Since this change introduces a new flag in the DTS, we must maintain ABI
>>>> compatibility with the kernel. The new flag is specific to the board, so
>>>
>>> It's not, I don't see it specific to the board in the bindings.
>>
>> I'm sorry for the confusion. This feature is not board-specific but
>> rather a tunable option. All RGMII boards can choose whether to enable
>> this bit in the DTS, so there are no restrictions in the binding.
> 
> If it is not specific to the board, I don't see why this cannot be
> implied by compatible.
> 

Whether this bit should be enabled should be determined on a per-board 
basis, but it should be available for all RGMII-type boards. It should 
be left to the users to decide whether to enable this bit in the DTS 
file, rather than controlling its existence in the binding file, 
shouldn't it?

>>
>>>
>>>> I need to ensure root nodes are matched to allow older boards to
>>>> continue functioning as before. I'm happy to adopt that approach if
>>>> there are any more elegant solutions.
>>>
>>> I don't think you understood the problem. Why you are not handling this
>>> for my board, sa8775p-rideX and sa8225-pre-ride-yellow-shrimp?
>>>
>>
>> This feature is specifically for RGMII boards. The driver won't enable
> 
> So board specific?

It is 'phy-mode' specific, to be more precise.

> 
>> this bit if the DTS doesn't specify it. To handle compatibility, we need
> 
> Do not describe us how drivers and DTS work. We all know.

Sure, I will take care of it.

> 
>> to identify legacy RGMII boards with MAC versions greater or equal to 3
>> which require this bit to be enabled.
>> According to my knowledge, the SA8775P is of the SGMII type.
> 
> 
> Best regards,
> Krzysztof

-- 
Best Regards,
Yijie

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Yijie Yang]

On 2025-01-08 21:29, Andrew Lunn wrote:
>>> Why is it specific to this board? Does the board have a PHY which is
>>> broken and requires this property? What we are missing are the details
>>> needed to help you get to the correct way to solve the problem you are
>>> facing.
>>>
>>
>> Let me clarify why this bit is necessary and why it's board-specific. The RX
>> programming swap bit can introduce a time delay of half a clock cycle. This
>> bit, along with the clock delay adjustment functionality, is implemented by
>> a module called 'IO Macro.' This is a Qualcomm-specific hardware design
>> located between the MAC and PHY in the SoC, serving the RGMII interface. The
>> bit works in conjunction with delay adjustment to meet the sampling
>> requirements. The sampling of RX data is also handled by this module.
>>
>> During the board design stage, the RGMII requirements may not have been
>> strictly followed, leading to uncertainty in the relationship between the
>> clock and data waveforms when they reach the IO Macro.
> 
> So this indicates any board might need this feature, not just this one
> board. Putting the board name in the driver then does not scale.
> 

Should I ignore this if I choose to use the following standard properties?

>> This means the time
>> delay introduced by the PC board may not be zero. Therefore, it's necessary
>> for software developers to tune both the RX programming swap bit and the
>> delay to ensure correct sampling.
> 
> O.K. Now look at how other boards tune their delays. There are
> standard properties for this:
> 
>          rx-internal-delay-ps:
>            description:
>              RGMII Receive Clock Delay defined in pico seconds. This is used for
>              controllers that have configurable RX internal delays. If this
>              property is present then the MAC applies the RX delay.
>          tx-internal-delay-ps:
>            description:
>              RGMII Transmit Clock Delay defined in pico seconds. This is used for
>              controllers that have configurable TX internal delays. If this
>              property is present then the MAC applies the TX delay.
> 
> I think you can use these properties, maybe with an additional comment
> in the binding. RGMII running at 1G has a clock of 125MHz. That is a
> period of 8ns. So a half clock cycle delay is then 4ns.
> 
> So an rx-internal-delay-ps of 0-2000 means this clock invert should be
> disabled. A rx-internal-delay-ps of 4000-6000 means the clock invert
> should be enabled.

This board was designed to operate at different speed rates, not a fixed 
speed, and the clock rate varies for each speed. Thus, the delay 
introduced by inverting the clock is not fixed. Additionally, I noticed 
that some vendors apply the same routine for this property across all 
speeds in their driver code. Can this property be used just as a flag, 
regardless of its actual value?

> 
> Now, ideally, you want the PHY to add the RGMII delays, that is what i
> request all MAC/PHY pairs do, so we have a uniform setup across all
> boards. So unless the PHY does not support RGMII delays, you would
> expect rx-internal-delay-ps to be either just a small number of
> picoseconds for fine tuning, or a small number of picoseconds + 4ns
> for fine tuning.

The delay for both TX and RX sides is added by the MAC in the Qualcomm 
driver, which was introduced by the initial patch. I believe there may 
be a refactor in the future to ensure it follows the requirements.

> 
> This scales, since it can be used by an board with poor design, and it
> does not require anything proprietary to Qualcomm, except the extended
> range, and hopefully nobody except Qualcomms broken RDK will require
> it, because obviously you will document the issue with the RDK and
> tell customers how to correctly design their board to be RGMII
> compliant with the clocks.

Yes, I will make a note of it.

> 
> 	Andrew

-- 
Best Regards,
Yijie

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Andrew Lunn]

> > So this indicates any board might need this feature, not just this one
> > board. Putting the board name in the driver then does not scale.
> > 
> 
> Should I ignore this if I choose to use the following standard properties?

You should always follow standard properties unless they don't
work. And if they don't work, your commit message needs to explain why
they don't work forcing your to do something special.

> > > This means the time
> > > delay introduced by the PC board may not be zero. Therefore, it's necessary
> > > for software developers to tune both the RX programming swap bit and the
> > > delay to ensure correct sampling.
> > 
> > O.K. Now look at how other boards tune their delays. There are
> > standard properties for this:
> > 
> >          rx-internal-delay-ps:
> >            description:
> >              RGMII Receive Clock Delay defined in pico seconds. This is used for
> >              controllers that have configurable RX internal delays. If this
> >              property is present then the MAC applies the RX delay.
> >          tx-internal-delay-ps:
> >            description:
> >              RGMII Transmit Clock Delay defined in pico seconds. This is used for
> >              controllers that have configurable TX internal delays. If this
> >              property is present then the MAC applies the TX delay.
> > 
> > I think you can use these properties, maybe with an additional comment
> > in the binding. RGMII running at 1G has a clock of 125MHz. That is a
> > period of 8ns. So a half clock cycle delay is then 4ns.
> > 
> > So an rx-internal-delay-ps of 0-2000 means this clock invert should be
> > disabled. A rx-internal-delay-ps of 4000-6000 means the clock invert
> > should be enabled.
> 
> This board was designed to operate at different speed rates, not a fixed
> speed, and the clock rate varies for each speed. Thus, the delay introduced
> by inverting the clock is not fixed. Additionally, I noticed that some
> vendors apply the same routine for this property across all speeds in their
> driver code. Can this property be used just as a flag, regardless of its
> actual value?

Maybe you should go read the RGMII standard, and then think about how
your hardware actually works.

RGMII always has a variable clock, with different clock speeds for
10/100/1G. So your board design is just plain normal, not
special. Does the standard talk about different delays for different
speeds? As you say, other drivers apply the same delay for all
speeds. Why should your hardware be special?

RGMII has been around for 25 years. Do you really think your RGMII
implementation needs something special which no other implementation
has needed in the last 25 years?

> > Now, ideally, you want the PHY to add the RGMII delays, that is what i
> > request all MAC/PHY pairs do, so we have a uniform setup across all
> > boards. So unless the PHY does not support RGMII delays, you would
> > expect rx-internal-delay-ps to be either just a small number of
> > picoseconds for fine tuning, or a small number of picoseconds + 4ns
> > for fine tuning.
> 
> The delay for both TX and RX sides is added by the MAC in the Qualcomm
> driver, which was introduced by the initial patch. I believe there may be a
> refactor in the future to ensure it follows the requirements.
 
You can do it in the MAC. But you probably want to clearly document
this, that your design is different to > 95% of systems in Linux which
have the PHY do the delays.

	Andrew

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Yijie Yang]

On 2025-01-21 00:49, Andrew Lunn wrote:
>>> So this indicates any board might need this feature, not just this one
>>> board. Putting the board name in the driver then does not scale.
>>>
>>
>> Should I ignore this if I choose to use the following standard properties?
> 
> You should always follow standard properties unless they don't
> work. And if they don't work, your commit message needs to explain why
> they don't work forcing your to do something special.
> 
>>>> This means the time
>>>> delay introduced by the PC board may not be zero. Therefore, it's necessary
>>>> for software developers to tune both the RX programming swap bit and the
>>>> delay to ensure correct sampling.
>>>
>>> O.K. Now look at how other boards tune their delays. There are
>>> standard properties for this:
>>>
>>>           rx-internal-delay-ps:
>>>             description:
>>>               RGMII Receive Clock Delay defined in pico seconds. This is used for
>>>               controllers that have configurable RX internal delays. If this
>>>               property is present then the MAC applies the RX delay.
>>>           tx-internal-delay-ps:
>>>             description:
>>>               RGMII Transmit Clock Delay defined in pico seconds. This is used for
>>>               controllers that have configurable TX internal delays. If this
>>>               property is present then the MAC applies the TX delay.
>>>
>>> I think you can use these properties, maybe with an additional comment
>>> in the binding. RGMII running at 1G has a clock of 125MHz. That is a
>>> period of 8ns. So a half clock cycle delay is then 4ns.
>>>
>>> So an rx-internal-delay-ps of 0-2000 means this clock invert should be
>>> disabled. A rx-internal-delay-ps of 4000-6000 means the clock invert
>>> should be enabled.
>>
>> This board was designed to operate at different speed rates, not a fixed
>> speed, and the clock rate varies for each speed. Thus, the delay introduced
>> by inverting the clock is not fixed. Additionally, I noticed that some
>> vendors apply the same routine for this property across all speeds in their
>> driver code. Can this property be used just as a flag, regardless of its
>> actual value?
> 
> Maybe you should go read the RGMII standard, and then think about how
> your hardware actually works.
> 
> RGMII always has a variable clock, with different clock speeds for
> 10/100/1G. So your board design is just plain normal, not
> special. Does the standard talk about different delays for different
> speeds? As you say, other drivers apply the same delay for all
> speeds. Why should your hardware be special?
> 
> RGMII has been around for 25 years. Do you really think your RGMII
> implementation needs something special which no other implementation
> has needed in the last 25 years?

I do not intend to violate the regulations of the RGMII standard and aim 
to maintain the same delay across all speeds. But the RX programming 
swap bit can only introduce a delay of 180 degrees. Should I assume the 
1G speed clock to calculate and determine if this bit should be enabled 
for all speeds?

> 
>>> Now, ideally, you want the PHY to add the RGMII delays, that is what i
>>> request all MAC/PHY pairs do, so we have a uniform setup across all
>>> boards. So unless the PHY does not support RGMII delays, you would
>>> expect rx-internal-delay-ps to be either just a small number of
>>> picoseconds for fine tuning, or a small number of picoseconds + 4ns
>>> for fine tuning.
>>
>> The delay for both TX and RX sides is added by the MAC in the Qualcomm
>> driver, which was introduced by the initial patch. I believe there may be a
>> refactor in the future to ensure it follows the requirements.
>   
> You can do it in the MAC. But you probably want to clearly document
> this, that your design is different to > 95% of systems in Linux which
> have the PHY do the delays.
> 
> 	Andrew

-- 
Best Regards,
Yijie

Re: [PATCH 2/3] net: stmmac: qcom-ethqos: Enable RX programmable swap on qcs615

[Andrew Lunn]

> > Maybe you should go read the RGMII standard, and then think about how
> > your hardware actually works.
> > 
> > RGMII always has a variable clock, with different clock speeds for
> > 10/100/1G. So your board design is just plain normal, not
> > special. Does the standard talk about different delays for different
> > speeds? As you say, other drivers apply the same delay for all
> > speeds. Why should your hardware be special?
> > 
> > RGMII has been around for 25 years. Do you really think your RGMII
> > implementation needs something special which no other implementation
> > has needed in the last 25 years?
> 
> I do not intend to violate the regulations of the RGMII standard and aim to
> maintain the same delay across all speeds. But the RX programming swap bit
> can only introduce a delay of 180 degrees. Should I assume the 1G speed
> clock to calculate and determine if this bit should be enabled for all
> speeds?

Lets rewind a bit.

The RGMII standard specified which edge you sample on. Since it is
defined, no other driver has a configuration like this, they just
setup there hardware to be standards compliant.

Why do you need the ability to break the standard, and sample on the
wrong edge?

I can think of two reasons:

1) You have a PHY which is broken, it also samples on the wrong
edge. This is a workaround for that broken PHY.

2) You have a board with a clock line driver inserted between the
RGMII output pins and the PHY, and this line driver includes a NOT?
This line driver is causing the clocking to break the RGMII
standard. You are working around this broken board design by getting
the MAC to invert the clock.

Is there a third reason?

Lets first understand the details of why you need to be able to invert
the clock.

	Andrew