| Message ID | 20230505090558.2355-3-samin.guo@starfivetech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add motorcomm phy pad-driver-strength-cfg support | expand |
> #define YTPHY_DTS_OUTPUT_CLK_DIS 0 > @@ -1495,6 +1504,7 @@ static int yt8531_config_init(struct phy_device *phydev) > { > struct device_node *node = phydev->mdio.dev.of_node; > int ret; > + u32 ds, val; Reverse Christmas tree. Sort these longest first, shortest last. Otherwise this looks O.K. The only open question is if real unit should be used, uA, not some magic numbers. Lets see what the DT Maintainers say. Andrew
On 2023/5/5 17:05, Samin Guo wrote: > The motorcomm phy (YT8531) supports the ability to adjust the drive > strength of the rx_clk/rx_data, and the default strength may not be > suitable for all boards. So add configurable options to better match > the boards.(e.g. StarFive VisionFive 2) > > Signed-off-by: Samin Guo <samin.guo@starfivetech.com> > --- > drivers/net/phy/motorcomm.c | 46 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c > index 2fa5a90e073b..191650bb1454 100644 > --- a/drivers/net/phy/motorcomm.c > +++ b/drivers/net/phy/motorcomm.c > @@ -236,6 +236,7 @@ > */ > #define YTPHY_WCR_TYPE_PULSE BIT(0) > > +#define YTPHY_PAD_DRIVE_STRENGTH_REG 0xA010 > #define YTPHY_SYNCE_CFG_REG 0xA012 > #define YT8521_SCR_SYNCE_ENABLE BIT(5) > /* 1b0 output 25m clock > @@ -260,6 +261,14 @@ > #define YT8531_SCR_CLK_SRC_REF_25M 4 > #define YT8531_SCR_CLK_SRC_SSC_25M 5 > > +#define YT8531_RGMII_RXC_DS_DEFAULT 0x3 > +#define YT8531_RGMII_RXC_DS_MAX 0x7 > +#define YT8531_RGMII_RXC_DS GENMASK(15, 13) > +#define YT8531_RGMII_RXD_DS_DEFAULT 0x3 > +#define YT8531_RGMII_RXD_DS_MAX 0x7 > +#define YT8531_RGMII_RXD_DS_LOW GENMASK(5, 4) /* Bit 1/0 of rxd_ds */ > +#define YT8531_RGMII_RXD_DS_HI BIT(12) /* Bit 2 of rxd_ds */ YT8531_RGMII_xxx is bit define for YTPHY_PAD_DRIVE_STRENGTH_REG, so it is better to put it under the define of YTPHY_PAD_DRIVE_STRENGTH_REG. YT8531_RGMII_xxx bit define as reverse order: #define YTPHY_PAD_DRIVE_STRENGTH_REG 0xA010 #define YT8531_RGMII_RXC_DS GENMASK(15, 13) #define YT8531_RGMII_RXD_DS_HI BIT(12) /* Bit 2 of rxd_ds */ <------- #define YT8531_RGMII_RXD_DS_LOW GENMASK(5, 4) /* Bit 1/0 of rxd_ds */ ... > + > /* Extended Register end */ > > #define YTPHY_DTS_OUTPUT_CLK_DIS 0 > @@ -1495,6 +1504,7 @@ static int yt8531_config_init(struct phy_device *phydev) > { > struct device_node *node = phydev->mdio.dev.of_node; > int ret; > + u32 ds, val; > > ret = ytphy_rgmii_clk_delay_config_with_lock(phydev); > if (ret < 0) > @@ -1518,6 +1528,42 @@ static int yt8531_config_init(struct phy_device *phydev) > return ret; > } > > + ds = YT8531_RGMII_RXC_DS_DEFAULT; > + if (!of_property_read_u32(node, "motorcomm,rx-clk-driver-strength", &val)) { > + if (val > YT8531_RGMII_RXC_DS_MAX) > + return -EINVAL; > + > + ds = val; > + } > + > + ret = ytphy_modify_ext_with_lock(phydev, > + YTPHY_PAD_DRIVE_STRENGTH_REG, > + YT8531_RGMII_RXC_DS, > + FIELD_PREP(YT8531_RGMII_RXC_DS, ds)); > + if (ret < 0) > + return ret; > + > + ds = FIELD_PREP(YT8531_RGMII_RXD_DS_LOW, YT8531_RGMII_RXD_DS_DEFAULT); > + if (!of_property_read_u32(node, "motorcomm,rx-data-driver-strength", &val)) { > + if (val > YT8531_RGMII_RXD_DS_MAX) > + return -EINVAL; > + > + if (val > FIELD_MAX(YT8531_RGMII_RXD_DS_LOW)) { > + ds = val & FIELD_MAX(YT8531_RGMII_RXD_DS_LOW); > + ds = FIELD_PREP(YT8531_RGMII_RXD_DS_LOW, ds); > + ds |= YT8531_RGMII_RXD_DS_HI; > + } else { > + ds = FIELD_PREP(YT8531_RGMII_RXD_DS_LOW, val); > + } > + } > + > + ret = ytphy_modify_ext_with_lock(phydev, > + YTPHY_PAD_DRIVE_STRENGTH_REG, > + YT8531_RGMII_RXD_DS_LOW | YT8531_RGMII_RXD_DS_HI, > + ds); > + if (ret < 0) > + return ret; > + > return 0; > } >
data: Re: [PATCH v2 2/2] net: phy: motorcomm: Add pad drive strength cfg support From: Frank Sae <Frank.Sae@motor-comm.com> to: Samin Guo <samin.guo@starfivetech.com>, <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>, <netdev@vger.kernel.org>, Peter Geis <pgwipeout@gmail.com> data: 2023/5/6 > > > On 2023/5/5 17:05, Samin Guo wrote: >> The motorcomm phy (YT8531) supports the ability to adjust the drive >> strength of the rx_clk/rx_data, and the default strength may not be >> suitable for all boards. So add configurable options to better match >> the boards.(e.g. StarFive VisionFive 2) >> >> Signed-off-by: Samin Guo <samin.guo@starfivetech.com> >> --- >> drivers/net/phy/motorcomm.c | 46 +++++++++++++++++++++++++++++++++++++ >> 1 file changed, 46 insertions(+) >> >> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c >> index 2fa5a90e073b..191650bb1454 100644 >> --- a/drivers/net/phy/motorcomm.c >> +++ b/drivers/net/phy/motorcomm.c >> @@ -236,6 +236,7 @@ >> */ >> #define YTPHY_WCR_TYPE_PULSE BIT(0) >> >> +#define YTPHY_PAD_DRIVE_STRENGTH_REG 0xA010 >> #define YTPHY_SYNCE_CFG_REG 0xA012 >> #define YT8521_SCR_SYNCE_ENABLE BIT(5) >> /* 1b0 output 25m clock >> @@ -260,6 +261,14 @@ >> #define YT8531_SCR_CLK_SRC_REF_25M 4 >> #define YT8531_SCR_CLK_SRC_SSC_25M 5 >> >> +#define YT8531_RGMII_RXC_DS_DEFAULT 0x3 >> +#define YT8531_RGMII_RXC_DS_MAX 0x7 >> +#define YT8531_RGMII_RXC_DS GENMASK(15, 13) >> +#define YT8531_RGMII_RXD_DS_DEFAULT 0x3 >> +#define YT8531_RGMII_RXD_DS_MAX 0x7 >> +#define YT8531_RGMII_RXD_DS_LOW GENMASK(5, 4) /* Bit 1/0 of rxd_ds */ >> +#define YT8531_RGMII_RXD_DS_HI BIT(12) /* Bit 2 of rxd_ds */ > > > YT8531_RGMII_xxx is bit define for YTPHY_PAD_DRIVE_STRENGTH_REG, so it is better to put it under the define of YTPHY_PAD_DRIVE_STRENGTH_REG. > > YT8531_RGMII_xxx bit define as reverse order: > #define YTPHY_PAD_DRIVE_STRENGTH_REG 0xA010 > #define YT8531_RGMII_RXC_DS GENMASK(15, 13) > #define YT8531_RGMII_RXD_DS_HI BIT(12) /* Bit 2 of rxd_ds */ <------- > #define YT8531_RGMII_RXD_DS_LOW GENMASK(5, 4) /* Bit 1/0 of rxd_ds */ > ... > Hi Frank, Ok, will fix it next version. btw, do you have any information you can provide about Andrew's mention of using real unit uA/mA instead of magic numbers? (I couldn't find any information about current in the YT8531's datasheet other than the magic numbers.) Below is all the relevant information I found： Pad Drive Strength Cfg (EXT_0xA010) Bit | Symbol | Access | Default | Description 15:13 | Rgmii_sw_dr_rx | RW | 0x3 | Drive strenght of rx_clk pad. | 3'b111: strongest; 3'b000: weakest. 12 | Rgmii_sw_dr[2] | RW | 0x0 | Bit 2 of Rgmii_sw_dr[2:0], refer to ext A010[5:4] 5:4 | Rgmii_sw_dr[1:0] | RW | 0x3 | Bit 1 and 0 of Rgmii_sw_dr, Drive strenght of rxd/rx_ctl rgmii pad. | 3'b111: strongest; 3'b000: weakest Best regards, Samin >> + >> /* Extended Register end */ >> >> #define YTPHY_DTS_OUTPUT_CLK_DIS 0 >> @@ -1495,6 +1504,7 @@ static int yt8531_config_init(struct phy_device *phydev) >> { >> struct device_node *node = phydev->mdio.dev.of_node; >> int ret; >> + u32 ds, val; >> >> ret = ytphy_rgmii_clk_delay_config_with_lock(phydev); >> if (ret < 0) >> @@ -1518,6 +1528,42 @@ static int yt8531_config_init(struct phy_device *phydev) >> return ret; >> } >> >> + ds = YT8531_RGMII_RXC_DS_DEFAULT; >> + if (!of_property_read_u32(node, "motorcomm,rx-clk-driver-strength", &val)) { >> + if (val > YT8531_RGMII_RXC_DS_MAX) >> + return -EINVAL; >> + >> + ds = val; >> + } >> + >> + ret = ytphy_modify_ext_with_lock(phydev, >> + YTPHY_PAD_DRIVE_STRENGTH_REG, >> + YT8531_RGMII_RXC_DS, >> + FIELD_PREP(YT8531_RGMII_RXC_DS, ds)); >> + if (ret < 0) >> + return ret; >> + >> + ds = FIELD_PREP(YT8531_RGMII_RXD_DS_LOW, YT8531_RGMII_RXD_DS_DEFAULT); >> + if (!of_property_read_u32(node, "motorcomm,rx-data-driver-strength", &val)) { >> + if (val > YT8531_RGMII_RXD_DS_MAX) >> + return -EINVAL; >> + >> + if (val > FIELD_MAX(YT8531_RGMII_RXD_DS_LOW)) { >> + ds = val & FIELD_MAX(YT8531_RGMII_RXD_DS_LOW); >> + ds = FIELD_PREP(YT8531_RGMII_RXD_DS_LOW, ds); >> + ds |= YT8531_RGMII_RXD_DS_HI; >> + } else { >> + ds = FIELD_PREP(YT8531_RGMII_RXD_DS_LOW, val); >> + } >> + } >> + >> + ret = ytphy_modify_ext_with_lock(phydev, >> + YTPHY_PAD_DRIVE_STRENGTH_REG, >> + YT8531_RGMII_RXD_DS_LOW | YT8531_RGMII_RXD_DS_HI, >> + ds); >> + if (ret < 0) >> + return ret; >> + >> return 0; >> } >>
Re: [PATCH v2 2/2] net: phy: motorcomm: Add pad drive strength cfg support From: Andrew Lunn <andrew@lunn.ch> to: Samin Guo <samin.guo@starfivetech.com> data: 2023/5/5 >> #define YTPHY_DTS_OUTPUT_CLK_DIS 0 >> @@ -1495,6 +1504,7 @@ static int yt8531_config_init(struct phy_device *phydev) >> { >> struct device_node *node = phydev->mdio.dev.of_node; >> int ret; >> + u32 ds, val; > > Reverse Christmas tree. Sort these longest first, shortest last. > Thanks, will fix. > Otherwise this looks O.K. > > The only open question is if real unit should be used, uA, not some > magic numbers. Lets see what the DT Maintainers say. > > Andrew Hi Andrew, As I communicated with Frank, Motorcomm doesn't give specific units on their datasheet, except for magic numbers. Tried to ask Motorcomm last week, but it seems that they themselves do not know what the unit is and have no response so far. Below is all the relevant information I found： Pad Drive Strength Cfg (EXT_0xA010) Bit | Symbol | Access | Default | Description 15:13 | Rgmii_sw_dr_rx | RW | 0x3 | Drive strenght of rx_clk pad. | 3'b111: strongest; 3'b000: weakest. 12 | Rgmii_sw_dr[2] | RW | 0x0 | Bit 2 of Rgmii_sw_dr[2:0], refer to ext A010[5:4] 5:4 | Rgmii_sw_dr[1:0] | RW | 0x3 | Bit 1 and 0 of Rgmii_sw_dr, Drive strenght of rxd/rx_ctl rgmii pad. | 3'b111: strongest; 3'b000: weakest Best regards, Samin
Re: [PATCH v2 2/2] net: phy: motorcomm: Add pad drive strength cfg support From: Guo Samin <samin.guo@starfivetech.com> to: Andrew Lunn <andrew@lunn.ch> data: 2023/5/6 > > Re: [PATCH v2 2/2] net: phy: motorcomm: Add pad drive strength cfg support > From: Andrew Lunn <andrew@lunn.ch> > to: Samin Guo <samin.guo@starfivetech.com> > data: 2023/5/5 > >>> #define YTPHY_DTS_OUTPUT_CLK_DIS 0 >>> @@ -1495,6 +1504,7 @@ static int yt8531_config_init(struct phy_device *phydev) >>> { >>> struct device_node *node = phydev->mdio.dev.of_node; >>> int ret; >>> + u32 ds, val; >> >> Reverse Christmas tree. Sort these longest first, shortest last. >> > Thanks, will fix. >> Otherwise this looks O.K. >> >> The only open question is if real unit should be used, uA, not some >> magic numbers. Lets see what the DT Maintainers say. >> >> Andrew > > Hi Andrew, > > As I communicated with Frank, Motorcomm doesn't give specific units on their datasheet, except for magic numbers. > Tried to ask Motorcomm last week, but it seems that they themselves do not know what the unit is and have no response so far. > > > Below is all the relevant information I found： > > Pad Drive Strength Cfg (EXT_0xA010) > > Bit | Symbol | Access | Default | Description > 15:13 | Rgmii_sw_dr_rx | RW | 0x3 | Drive strenght of rx_clk pad. > | 3'b111: strongest; 3'b000: weakest. > > 12 | Rgmii_sw_dr[2] | RW | 0x0 | Bit 2 of Rgmii_sw_dr[2:0], refer to ext A010[5:4] > > 5:4 | Rgmii_sw_dr[1:0] | RW | 0x3 | Bit 1 and 0 of Rgmii_sw_dr, Drive strenght of rxd/rx_ctl rgmii pad. > | 3'b111: strongest; 3'b000: weakest > > > > Best regards, > Samin Hi Andrew, We tried contacting motorcomm again, but so far we haven't been able to get any more information about unit. Also, I found a similar configuration in Documentation/devicetree/bindings/net/qca,ar803x.yaml, and they also used the 'magic numbers': qca,clk-out-strength: description: Clock output driver strength. $ref: /schemas/types.yaml#/definitions/uint32 enum: [0, 1, 2] Best regards, Samin
diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c index 2fa5a90e073b..191650bb1454 100644 --- a/drivers/net/phy/motorcomm.c +++ b/drivers/net/phy/motorcomm.c @@ -236,6 +236,7 @@ */ #define YTPHY_WCR_TYPE_PULSE BIT(0) +#define YTPHY_PAD_DRIVE_STRENGTH_REG 0xA010 #define YTPHY_SYNCE_CFG_REG 0xA012 #define YT8521_SCR_SYNCE_ENABLE BIT(5) /* 1b0 output 25m clock @@ -260,6 +261,14 @@ #define YT8531_SCR_CLK_SRC_REF_25M 4 #define YT8531_SCR_CLK_SRC_SSC_25M 5 +#define YT8531_RGMII_RXC_DS_DEFAULT 0x3 +#define YT8531_RGMII_RXC_DS_MAX 0x7 +#define YT8531_RGMII_RXC_DS GENMASK(15, 13) +#define YT8531_RGMII_RXD_DS_DEFAULT 0x3 +#define YT8531_RGMII_RXD_DS_MAX 0x7 +#define YT8531_RGMII_RXD_DS_LOW GENMASK(5, 4) /* Bit 1/0 of rxd_ds */ +#define YT8531_RGMII_RXD_DS_HI BIT(12) /* Bit 2 of rxd_ds */ + /* Extended Register end */ #define YTPHY_DTS_OUTPUT_CLK_DIS 0 @@ -1495,6 +1504,7 @@ static int yt8531_config_init(struct phy_device *phydev) { struct device_node *node = phydev->mdio.dev.of_node; int ret; + u32 ds, val; ret = ytphy_rgmii_clk_delay_config_with_lock(phydev); if (ret < 0) @@ -1518,6 +1528,42 @@ static int yt8531_config_init(struct phy_device *phydev) return ret; } + ds = YT8531_RGMII_RXC_DS_DEFAULT; + if (!of_property_read_u32(node, "motorcomm,rx-clk-driver-strength", &val)) { + if (val > YT8531_RGMII_RXC_DS_MAX) + return -EINVAL; + + ds = val; + } + + ret = ytphy_modify_ext_with_lock(phydev, + YTPHY_PAD_DRIVE_STRENGTH_REG, + YT8531_RGMII_RXC_DS, + FIELD_PREP(YT8531_RGMII_RXC_DS, ds)); + if (ret < 0) + return ret; + + ds = FIELD_PREP(YT8531_RGMII_RXD_DS_LOW, YT8531_RGMII_RXD_DS_DEFAULT); + if (!of_property_read_u32(node, "motorcomm,rx-data-driver-strength", &val)) { + if (val > YT8531_RGMII_RXD_DS_MAX) + return -EINVAL; + + if (val > FIELD_MAX(YT8531_RGMII_RXD_DS_LOW)) { + ds = val & FIELD_MAX(YT8531_RGMII_RXD_DS_LOW); + ds = FIELD_PREP(YT8531_RGMII_RXD_DS_LOW, ds); + ds |= YT8531_RGMII_RXD_DS_HI; + } else { + ds = FIELD_PREP(YT8531_RGMII_RXD_DS_LOW, val); + } + } + + ret = ytphy_modify_ext_with_lock(phydev, + YTPHY_PAD_DRIVE_STRENGTH_REG, + YT8531_RGMII_RXD_DS_LOW | YT8531_RGMII_RXD_DS_HI, + ds); + if (ret < 0) + return ret; + return 0; }
The motorcomm phy (YT8531) supports the ability to adjust the drive strength of the rx_clk/rx_data, and the default strength may not be suitable for all boards. So add configurable options to better match the boards.(e.g. StarFive VisionFive 2) Signed-off-by: Samin Guo <samin.guo@starfivetech.com> --- drivers/net/phy/motorcomm.c | 46 +++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)