[PATCH] net: phy: motorcomm: Fix the issue in the code regarding the incorrect use of time units
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:
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
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
#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
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?
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
linux-stable-mirror@lists.linaro.org
-
Andrew Lunn -
Russell King (Oracle) -
Yi Cong