[PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units

[PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units

[Yi Cong]

From: Yi Cong <yicong@kylinos.cn>

Currently, NS (nanoseconds) is being used, but according to the datasheet,
the correct unit should be PS (picoseconds).

Fixes: 4869a146cd60 ("net: phy: Add BIT macro for Motorcomm yt8521/yt8531 gigabit ethernet phy")
Cc: stable@vger.kernel.org
Signed-off-by: Yi Cong <yicong@kylinos.cn>
---
 drivers/net/phy/motorcomm.c | 102 ++++++++++++++++++------------------
 1 file changed, 51 insertions(+), 51 deletions(-)

diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
index a3593e663059..81491c71e75b 100644
--- a/drivers/net/phy/motorcomm.c
+++ b/drivers/net/phy/motorcomm.c
@@ -171,7 +171,7 @@
  * 1b1 enable 1.9ns rxc clock delay
  */
 #define YT8521_CCR_RXC_DLY_EN			BIT(8)
-#define YT8521_CCR_RXC_DLY_1_900_NS		1900
+#define YT8521_CCR_RXC_DLY_1_900_PS		1900
 
 #define YT8521_CCR_MODE_SEL_MASK		(BIT(2) | BIT(1) | BIT(0))
 #define YT8521_CCR_MODE_UTP_TO_RGMII		0
@@ -196,22 +196,22 @@
 #define YT8521_RC1R_RX_DELAY_MASK		GENMASK(13, 10)
 #define YT8521_RC1R_FE_TX_DELAY_MASK		GENMASK(7, 4)
 #define YT8521_RC1R_GE_TX_DELAY_MASK		GENMASK(3, 0)
-#define YT8521_RC1R_RGMII_0_000_NS		0
-#define YT8521_RC1R_RGMII_0_150_NS		1
-#define YT8521_RC1R_RGMII_0_300_NS		2
-#define YT8521_RC1R_RGMII_0_450_NS		3
-#define YT8521_RC1R_RGMII_0_600_NS		4
-#define YT8521_RC1R_RGMII_0_750_NS		5
-#define YT8521_RC1R_RGMII_0_900_NS		6
-#define YT8521_RC1R_RGMII_1_050_NS		7
-#define YT8521_RC1R_RGMII_1_200_NS		8
-#define YT8521_RC1R_RGMII_1_350_NS		9
-#define YT8521_RC1R_RGMII_1_500_NS		10
-#define YT8521_RC1R_RGMII_1_650_NS		11
-#define YT8521_RC1R_RGMII_1_800_NS		12
-#define YT8521_RC1R_RGMII_1_950_NS		13
-#define YT8521_RC1R_RGMII_2_100_NS		14
-#define YT8521_RC1R_RGMII_2_250_NS		15
+#define YT8521_RC1R_RGMII_0_000_PS		0
+#define YT8521_RC1R_RGMII_0_150_PS		1
+#define YT8521_RC1R_RGMII_0_300_PS		2
+#define YT8521_RC1R_RGMII_0_450_PS		3
+#define YT8521_RC1R_RGMII_0_600_PS		4
+#define YT8521_RC1R_RGMII_0_750_PS		5
+#define YT8521_RC1R_RGMII_0_900_PS		6
+#define YT8521_RC1R_RGMII_1_050_PS		7
+#define YT8521_RC1R_RGMII_1_200_PS		8
+#define YT8521_RC1R_RGMII_1_350_PS		9
+#define YT8521_RC1R_RGMII_1_500_PS		10
+#define YT8521_RC1R_RGMII_1_650_PS		11
+#define YT8521_RC1R_RGMII_1_800_PS		12
+#define YT8521_RC1R_RGMII_1_950_PS		13
+#define YT8521_RC1R_RGMII_2_100_PS		14
+#define YT8521_RC1R_RGMII_2_250_PS		15
 
 /* LED CONFIG */
 #define YT8521_MAX_LEDS				3
@@ -800,40 +800,40 @@ struct ytphy_cfg_reg_map {
 
 static const struct ytphy_cfg_reg_map ytphy_rgmii_delays[] = {
 	/* for tx delay / rx delay with YT8521_CCR_RXC_DLY_EN is not set. */
-	{ 0,	YT8521_RC1R_RGMII_0_000_NS },
-	{ 150,	YT8521_RC1R_RGMII_0_150_NS },
-	{ 300,	YT8521_RC1R_RGMII_0_300_NS },
-	{ 450,	YT8521_RC1R_RGMII_0_450_NS },
-	{ 600,	YT8521_RC1R_RGMII_0_600_NS },
-	{ 750,	YT8521_RC1R_RGMII_0_750_NS },
-	{ 900,	YT8521_RC1R_RGMII_0_900_NS },
-	{ 1050,	YT8521_RC1R_RGMII_1_050_NS },
-	{ 1200,	YT8521_RC1R_RGMII_1_200_NS },
-	{ 1350,	YT8521_RC1R_RGMII_1_350_NS },
-	{ 1500,	YT8521_RC1R_RGMII_1_500_NS },
-	{ 1650,	YT8521_RC1R_RGMII_1_650_NS },
-	{ 1800,	YT8521_RC1R_RGMII_1_800_NS },
-	{ 1950,	YT8521_RC1R_RGMII_1_950_NS },	/* default tx/rx delay */
-	{ 2100,	YT8521_RC1R_RGMII_2_100_NS },
-	{ 2250,	YT8521_RC1R_RGMII_2_250_NS },
+	{ 0,	YT8521_RC1R_RGMII_0_000_PS },
+	{ 150,	YT8521_RC1R_RGMII_0_150_PS },
+	{ 300,	YT8521_RC1R_RGMII_0_300_PS },
+	{ 450,	YT8521_RC1R_RGMII_0_450_PS },
+	{ 600,	YT8521_RC1R_RGMII_0_600_PS },
+	{ 750,	YT8521_RC1R_RGMII_0_750_PS },
+	{ 900,	YT8521_RC1R_RGMII_0_900_PS },
+	{ 1050,	YT8521_RC1R_RGMII_1_050_PS },
+	{ 1200,	YT8521_RC1R_RGMII_1_200_PS },
+	{ 1350,	YT8521_RC1R_RGMII_1_350_PS },
+	{ 1500,	YT8521_RC1R_RGMII_1_500_PS },
+	{ 1650,	YT8521_RC1R_RGMII_1_650_PS },
+	{ 1800,	YT8521_RC1R_RGMII_1_800_PS },
+	{ 1950,	YT8521_RC1R_RGMII_1_950_PS },	/* default tx/rx delay */
+	{ 2100,	YT8521_RC1R_RGMII_2_100_PS },
+	{ 2250,	YT8521_RC1R_RGMII_2_250_PS },
 
 	/* only for rx delay with YT8521_CCR_RXC_DLY_EN is set. */
-	{ 0    + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_0_000_NS },
-	{ 150  + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_0_150_NS },
-	{ 300  + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_0_300_NS },
-	{ 450  + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_0_450_NS },
-	{ 600  + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_0_600_NS },
-	{ 750  + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_0_750_NS },
-	{ 900  + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_0_900_NS },
-	{ 1050 + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_1_050_NS },
-	{ 1200 + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_1_200_NS },
-	{ 1350 + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_1_350_NS },
-	{ 1500 + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_1_500_NS },
-	{ 1650 + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_1_650_NS },
-	{ 1800 + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_1_800_NS },
-	{ 1950 + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_1_950_NS },
-	{ 2100 + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_2_100_NS },
-	{ 2250 + YT8521_CCR_RXC_DLY_1_900_NS,	YT8521_RC1R_RGMII_2_250_NS }
+	{ 0    + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_0_000_PS },
+	{ 150  + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_0_150_PS },
+	{ 300  + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_0_300_PS },
+	{ 450  + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_0_450_PS },
+	{ 600  + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_0_600_PS },
+	{ 750  + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_0_750_PS },
+	{ 900  + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_0_900_PS },
+	{ 1050 + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_1_050_PS },
+	{ 1200 + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_1_200_PS },
+	{ 1350 + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_1_350_PS },
+	{ 1500 + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_1_500_PS },
+	{ 1650 + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_1_650_PS },
+	{ 1800 + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_1_800_PS },
+	{ 1950 + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_1_950_PS },
+	{ 2100 + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_2_100_PS },
+	{ 2250 + YT8521_CCR_RXC_DLY_1_900_PS,	YT8521_RC1R_RGMII_2_250_PS }
 };
 
 static u32 ytphy_get_delay_reg_value(struct phy_device *phydev,
@@ -890,10 +890,10 @@ static int ytphy_rgmii_clk_delay_config(struct phy_device *phydev)
 	rx_reg = ytphy_get_delay_reg_value(phydev, "rx-internal-delay-ps",
 					   ytphy_rgmii_delays, tb_size,
 					   &rxc_dly_en,
-					   YT8521_RC1R_RGMII_1_950_NS);
+					   YT8521_RC1R_RGMII_1_950_PS);
 	tx_reg = ytphy_get_delay_reg_value(phydev, "tx-internal-delay-ps",
 					   ytphy_rgmii_delays, tb_size, NULL,
-					   YT8521_RC1R_RGMII_1_950_NS);
+					   YT8521_RC1R_RGMII_1_950_PS);
 
 	switch (phydev->interface) {
 	case PHY_INTERFACE_MODE_RGMII:
-- 
2.25.1

Re: [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units

[Andrew Lunn]

On Tue, Oct 28, 2025 at 09:59:23AM +0800, Yi Cong wrote:
> From: Yi Cong <yicong@kylinos.cn>
> 
> Currently, NS (nanoseconds) is being used, but according to the datasheet,
> the correct unit should be PS (picoseconds).
> 
> Fixes: 4869a146cd60 ("net: phy: Add BIT macro for Motorcomm yt8521/yt8531 gigabit ethernet phy")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yi Cong <yicong@kylinos.cn>
> ---
>  drivers/net/phy/motorcomm.c | 102 ++++++++++++++++++------------------
>  1 file changed, 51 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index a3593e663059..81491c71e75b 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c
> @@ -171,7 +171,7 @@
>   * 1b1 enable 1.9ns rxc clock delay
>   */
>  #define YT8521_CCR_RXC_DLY_EN			BIT(8)
> -#define YT8521_CCR_RXC_DLY_1_900_NS		1900
> +#define YT8521_CCR_RXC_DLY_1_900_PS		1900

This could be down to interpretation.

#define YT8521_CCR_RXC_DLY_1.900_NS		1900

would be technically correct, but not valid for cpp(1). So the . is
replaced with a _ .

#define YT8521_CCR_RXC_DLY_1900_PS		1900

would also be correct, but that is not what you have in your patch,
you leave the _ in place.

    Andrew

---
pw-bot: cr

Re: [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units

[Yi Cong]

On Tue, 28 Oct 2025 03:51:04 +0100, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Oct 28, 2025 at 09:59:23AM +0800, Yi Cong wrote:
> > From: Yi Cong <yicong@kylinos.cn>
> >
> > Currently, NS (nanoseconds) is being used, but according to the datasheet,
> > the correct unit should be PS (picoseconds).
> >
> > Fixes: 4869a146cd60 ("net: phy: Add BIT macro for Motorcomm yt8521/yt8531 gigabit ethernet phy")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Yi Cong <yicong@kylinos.cn>
> > ---
> >  drivers/net/phy/motorcomm.c | 102 ++++++++++++++++++------------------
> >  1 file changed, 51 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> > index a3593e663059..81491c71e75b 100644
> > --- a/drivers/net/phy/motorcomm.c
> > +++ b/drivers/net/phy/motorcomm.c
> > @@ -171,7 +171,7 @@
> >   * 1b1 enable 1.9ns rxc clock delay
> >   */
> >  #define YT8521_CCR_RXC_DLY_EN			BIT(8)
> > -#define YT8521_CCR_RXC_DLY_1_900_NS		1900
> > +#define YT8521_CCR_RXC_DLY_1_900_PS		1900
>
> This could be down to interpretation.
>
> #define YT8521_CCR_RXC_DLY_1.900_NS		1900
>
> would be technically correct, but not valid for cpp(1). So the . is
> replaced with a _ .
>
> #define YT8521_CCR_RXC_DLY_1900_PS		1900
>
> would also be correct, but that is not what you have in your patch,
> you leave the _ in place.

Alright, I didn't realize that 1_950 represents 1.950;
I thought the underscores were used for code neatness,
making numbers like 900 and 1050 the same length, for example:
#define YT8521_RC1R_RGMII_0_900_PS
#define YT8521_RC1R_RGMII_1_050_PS

In that case, is my patch still necessary?
Or should I instead follow your suggestion above and change them to something like:
#define YT8521_RC1R_RGMII_900_PS
#define YT8521_RC1R_RGMII_1050_PS

Regards,
    Yi Cong

Re: [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units

[Andrew Lunn]

> > >  #define YT8521_CCR_RXC_DLY_EN			BIT(8)
> > > -#define YT8521_CCR_RXC_DLY_1_900_NS		1900
> > > +#define YT8521_CCR_RXC_DLY_1_900_PS		1900
> >
> > This could be down to interpretation.
> >
> > #define YT8521_CCR_RXC_DLY_1.900_NS		1900
> >
> > would be technically correct, but not valid for cpp(1). So the . is
> > replaced with a _ .
> >
> > #define YT8521_CCR_RXC_DLY_1900_PS		1900
> >
> > would also be correct, but that is not what you have in your patch,
> > you leave the _ in place.
> 
> Alright, I didn't realize that 1_950 represents 1.950;
> I thought the underscores were used for code neatness,
> making numbers like 900 and 1050 the same length, for example:
> #define YT8521_RC1R_RGMII_0_900_PS
> #define YT8521_RC1R_RGMII_1_050_PS
> 
> In that case, is my patch still necessary?

I think it is unnecessary.

If you want, you could add a comment which explains that the _ should
be read as a .  However, this does appear elsewhere in Linux, it is
one of those things you learn with time.

    Andrew

---
pw-bot: cr

Re: [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units

[Russell King (Oracle)]

On Tue, Oct 28, 2025 at 01:07:34PM +0100, Andrew Lunn wrote:
> > > >  #define YT8521_CCR_RXC_DLY_EN			BIT(8)
> > > > -#define YT8521_CCR_RXC_DLY_1_900_NS		1900
> > > > +#define YT8521_CCR_RXC_DLY_1_900_PS		1900
> > >
> > > This could be down to interpretation.
> > >
> > > #define YT8521_CCR_RXC_DLY_1.900_NS		1900
> > >
> > > would be technically correct, but not valid for cpp(1). So the . is
> > > replaced with a _ .
> > >
> > > #define YT8521_CCR_RXC_DLY_1900_PS		1900
> > >
> > > would also be correct, but that is not what you have in your patch,
> > > you leave the _ in place.
> > 
> > Alright, I didn't realize that 1_950 represents 1.950;
> > I thought the underscores were used for code neatness,
> > making numbers like 900 and 1050 the same length, for example:
> > #define YT8521_RC1R_RGMII_0_900_PS
> > #define YT8521_RC1R_RGMII_1_050_PS
> > 
> > In that case, is my patch still necessary?
> 
> I think it is unnecessary.
> 
> If you want, you could add a comment which explains that the _ should
> be read as a .  However, this does appear elsewhere in Linux, it is
> one of those things you learn with time.

Hang on.

Is the "1900" 1.9ns or 1.9ps ?

If YT8521_CCR_RXC_DLY_1_900_NS means 1.9ns, and the value is in ps,
then surely if it's being renamed to _PS, then it _must_ become
YT8521_CCR_RXC_DLY_1900_NS, because 1.900ps is wrong?

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

Re: [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units

[Yi Cong]

On Tue, 28 Oct 2025 12:19:32 +0000, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>
> On Tue, Oct 28, 2025 at 01:07:34PM +0100, Andrew Lunn wrote:
> > > > >  #define YT8521_CCR_RXC_DLY_EN			BIT(8)
> > > > > -#define YT8521_CCR_RXC_DLY_1_900_NS		1900
> > > > > +#define YT8521_CCR_RXC_DLY_1_900_PS		1900
> > > >
> > > > This could be down to interpretation.
> > > >
> > > > #define YT8521_CCR_RXC_DLY_1.900_NS		1900
> > > >
> > > > would be technically correct, but not valid for cpp(1). So the . is
> > > > replaced with a _ .
> > > >
> > > > #define YT8521_CCR_RXC_DLY_1900_PS		1900
> > > >
> > > > would also be correct, but that is not what you have in your patch,
> > > > you leave the _ in place.
> > >
> > > Alright, I didn't realize that 1_950 represents 1.950;
> > > I thought the underscores were used for code neatness,
> > > making numbers like 900 and 1050 the same length, for example:
> > > #define YT8521_RC1R_RGMII_0_900_PS
> > > #define YT8521_RC1R_RGMII_1_050_PS
> > >
> > > In that case, is my patch still necessary?
> >
> > I think it is unnecessary.
> >
> > If you want, you could add a comment which explains that the _ should
> > be read as a .  However, this does appear elsewhere in Linux, it is
> > one of those things you learn with time.
>
> Hang on.
>
> Is the "1900" 1.9ns or 1.9ps ?
>
> If YT8521_CCR_RXC_DLY_1_900_NS means 1.9ns, and the value is in ps,
> then surely if it's being renamed to _PS, then it _must_ become
> YT8521_CCR_RXC_DLY_1900_NS, because 1.900ps is wrong?

According to the information I obtained from the manufacturer,
the unit in the register is PS.
In the code, both 1900_PS and 1_900_NS are correct,as they both
represent 1900ps (=1.9ns).
Therefore, there is no need to change the existing 1_900_NS.


Regards,
    Yi Cong