* [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units
@ 2025-10-28 1:59 Yi Cong
2025-10-28 2:51 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Yi Cong @ 2025-10-28 1:59 UTC (permalink / raw)
To: Frank.Sae, andrew+netdev, hkallweit1, linux, davem
Cc: kuba, netdev, Yi Cong, stable
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units
2025-10-28 1:59 [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units Yi Cong
@ 2025-10-28 2:51 ` Andrew Lunn
2025-10-28 6:21 ` Yi Cong
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2025-10-28 2:51 UTC (permalink / raw)
To: Yi Cong
Cc: Frank.Sae, andrew+netdev, hkallweit1, linux, davem, kuba, netdev,
Yi Cong, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units
2025-10-28 2:51 ` Andrew Lunn
@ 2025-10-28 6:21 ` Yi Cong
2025-10-28 12:07 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Yi Cong @ 2025-10-28 6:21 UTC (permalink / raw)
To: andrew
Cc: Frank.Sae, andrew+netdev, cong.yi, davem, hkallweit1, kuba, linux,
netdev, stable, yicong
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units
2025-10-28 6:21 ` Yi Cong
@ 2025-10-28 12:07 ` Andrew Lunn
2025-10-28 12:19 ` Russell King (Oracle)
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2025-10-28 12:07 UTC (permalink / raw)
To: Yi Cong
Cc: Frank.Sae, andrew+netdev, davem, hkallweit1, kuba, linux, netdev,
stable, yicong
> > > #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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units
2025-10-28 12:07 ` Andrew Lunn
@ 2025-10-28 12:19 ` Russell King (Oracle)
2025-10-28 12:46 ` Yi Cong
0 siblings, 1 reply; 6+ messages in thread
From: Russell King (Oracle) @ 2025-10-28 12:19 UTC (permalink / raw)
To: Andrew Lunn
Cc: Yi Cong, Frank.Sae, andrew+netdev, davem, hkallweit1, kuba,
netdev, stable, yicong
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units
2025-10-28 12:19 ` Russell King (Oracle)
@ 2025-10-28 12:46 ` Yi Cong
0 siblings, 0 replies; 6+ messages in thread
From: Yi Cong @ 2025-10-28 12:46 UTC (permalink / raw)
To: linux; +Cc: Frank.Sae, andrew, davem, hkallweit1, kuba, netdev, stable,
yicong
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-28 12:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-28 1:59 [PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units Yi Cong
2025-10-28 2:51 ` Andrew Lunn
2025-10-28 6:21 ` Yi Cong
2025-10-28 12:07 ` Andrew Lunn
2025-10-28 12:19 ` Russell King (Oracle)
2025-10-28 12:46 ` Yi Cong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).