diff mbox series

[RFC,2/2] net: phy: dp83869: fix mii mode when rgmii strap cfg is used

Message ID 20230425054429.3956535-3-s-vadapalli@ti.com (mailing list archive)
State New, archived
Headers show
Series DP83867/DP83869 Ethernet PHY workaround/fix | expand

Commit Message

Siddharth Vadapalli April 25, 2023, 5:44 a.m. UTC 
From: Grygorii Strashko <grygorii.strashko@ti.com>

The DP83869 PHY on TI's k3-am642-evm supports both MII and RGMII
interfaces and is configured by default to use RGMII interface (strap).
However, the board design allows switching dynamically to MII interface
for testing purposes by applying different set of pinmuxes.

To support switching to MII interface, update the DP83869 PHY driver to
configure OP_MODE_DECODE.RGMII_MII_SEL(bit 5) properly when MII PHY
interface mode is requested.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/net/phy/dp83869.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Andrew Lunn April 25, 2023, 12:18 p.m. UTC | #1 
On Tue, Apr 25, 2023 at 11:14:29AM +0530, Siddharth Vadapalli wrote:
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> The DP83869 PHY on TI's k3-am642-evm supports both MII and RGMII
> interfaces and is configured by default to use RGMII interface (strap).
> However, the board design allows switching dynamically to MII interface
> for testing purposes by applying different set of pinmuxes.

Only for testing? So nobody should actually design a board to use MII
and use software to change the interface from RGMII to MII?

This does not seem to be a fix, it is a new feature. So please submit
to net-next, in two weeks time when it opens again.

> @@ -692,8 +692,11 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>  	/* Below init sequence for each operational mode is defined in
>  	 * section 9.4.8 of the datasheet.
>  	 */
> +	phy_ctrl_val = dp83869->mode;
> +	if (phydev->interface == PHY_INTERFACE_MODE_MII)
> +		phy_ctrl_val |= DP83869_OP_MODE_MII;

Should there be some validation here with dp83869->mode?

DP83869_RGMII_COPPER_ETHERNET, DP83869_RGMII_SGMII_BRIDGE etc don't
make sense if MII is being used. DP83869_100M_MEDIA_CONVERT and maybe
DP83869_RGMII_100_BASE seem to be the only valid modes with MII?

	Andrew
Siddharth Vadapalli April 26, 2023, 5:49 a.m. UTC | #2 
On 25/04/23 17:48, Andrew Lunn wrote:
> On Tue, Apr 25, 2023 at 11:14:29AM +0530, Siddharth Vadapalli wrote:
>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>>
>> The DP83869 PHY on TI's k3-am642-evm supports both MII and RGMII
>> interfaces and is configured by default to use RGMII interface (strap).
>> However, the board design allows switching dynamically to MII interface
>> for testing purposes by applying different set of pinmuxes.
> 
> Only for testing? So nobody should actually design a board to use MII
> and use software to change the interface from RGMII to MII?
> 
> This does not seem to be a fix, it is a new feature. So please submit
> to net-next, in two weeks time when it opens again.

Sure. I will split this patch from the series and post the v2 of this patch with
the subject "RFC PATCH net-next" for requesting further feedback on this patch
if needed. Following that, I will post it to net-next as a new patch.

> 
>> @@ -692,8 +692,11 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>>  	/* Below init sequence for each operational mode is defined in
>>  	 * section 9.4.8 of the datasheet.
>>  	 */
>> +	phy_ctrl_val = dp83869->mode;
>> +	if (phydev->interface == PHY_INTERFACE_MODE_MII)
>> +		phy_ctrl_val |= DP83869_OP_MODE_MII;
> 
> Should there be some validation here with dp83869->mode?
> 
> DP83869_RGMII_COPPER_ETHERNET, DP83869_RGMII_SGMII_BRIDGE etc don't
> make sense if MII is being used. DP83869_100M_MEDIA_CONVERT and maybe
> DP83869_RGMII_100_BASE seem to be the only valid modes with MII?

The DP83869_OP_MODE_MII macro corresponds to BIT(5) which is the RGMII_MII_SEL
bit in the OP_MODE_DECODE register. If the RGMII_MII_SEL bit is set, MII mode is
selected. If the bit is cleared, which is the default value, RGMII mode is
selected. As pointed out by you, there are modes which aren't valid with MII
mode. However, a mode which isn't valid with RGMII mode (default value of the
RGMII_MII_SEL bit) also exists: DP83869_SGMII_COPPER_ETHERNET. For this reason,
I believe that setting the bit when MII mode is requested shouldn't cause any
issues.

> 
> 	Andrew
Andrew Lunn April 26, 2023, 12:41 p.m. UTC | #3 
> >> @@ -692,8 +692,11 @@ static int dp83869_configure_mode(struct phy_device *phydev,
> >>  	/* Below init sequence for each operational mode is defined in
> >>  	 * section 9.4.8 of the datasheet.
> >>  	 */
> >> +	phy_ctrl_val = dp83869->mode;
> >> +	if (phydev->interface == PHY_INTERFACE_MODE_MII)
> >> +		phy_ctrl_val |= DP83869_OP_MODE_MII;
> > 
> > Should there be some validation here with dp83869->mode?
> > 
> > DP83869_RGMII_COPPER_ETHERNET, DP83869_RGMII_SGMII_BRIDGE etc don't
> > make sense if MII is being used. DP83869_100M_MEDIA_CONVERT and maybe
> > DP83869_RGMII_100_BASE seem to be the only valid modes with MII?
> 
> The DP83869_OP_MODE_MII macro corresponds to BIT(5) which is the RGMII_MII_SEL
> bit in the OP_MODE_DECODE register. If the RGMII_MII_SEL bit is set, MII mode is
> selected. If the bit is cleared, which is the default value, RGMII mode is
> selected. As pointed out by you, there are modes which aren't valid with MII
> mode. However, a mode which isn't valid with RGMII mode (default value of the
> RGMII_MII_SEL bit) also exists: DP83869_SGMII_COPPER_ETHERNET. For this reason,
> I believe that setting the bit when MII mode is requested shouldn't cause any
> issues.

If you say so. I was just thinking you could give the poor software
engineer a hint the hardware engineer has put on strapping resistors
which means the PHY is not going to work.

      Andrew
Siddharth Vadapalli April 27, 2023, 4:08 a.m. UTC | #4 
On 26/04/23 18:11, Andrew Lunn wrote:
>>>> @@ -692,8 +692,11 @@ static int dp83869_configure_mode(struct phy_device *phydev,
>>>>  	/* Below init sequence for each operational mode is defined in
>>>>  	 * section 9.4.8 of the datasheet.
>>>>  	 */
>>>> +	phy_ctrl_val = dp83869->mode;
>>>> +	if (phydev->interface == PHY_INTERFACE_MODE_MII)
>>>> +		phy_ctrl_val |= DP83869_OP_MODE_MII;
>>>
>>> Should there be some validation here with dp83869->mode?
>>>
>>> DP83869_RGMII_COPPER_ETHERNET, DP83869_RGMII_SGMII_BRIDGE etc don't
>>> make sense if MII is being used. DP83869_100M_MEDIA_CONVERT and maybe
>>> DP83869_RGMII_100_BASE seem to be the only valid modes with MII?
>>
>> The DP83869_OP_MODE_MII macro corresponds to BIT(5) which is the RGMII_MII_SEL
>> bit in the OP_MODE_DECODE register. If the RGMII_MII_SEL bit is set, MII mode is
>> selected. If the bit is cleared, which is the default value, RGMII mode is
>> selected. As pointed out by you, there are modes which aren't valid with MII
>> mode. However, a mode which isn't valid with RGMII mode (default value of the
>> RGMII_MII_SEL bit) also exists: DP83869_SGMII_COPPER_ETHERNET. For this reason,
>> I believe that setting the bit when MII mode is requested shouldn't cause any
>> issues.
> 
> If you say so. I was just thinking you could give the poor software
> engineer a hint the hardware engineer has put on strapping resistors
> which means the PHY is not going to work.

I understand now. I will update this patch to add a print if the MII mode is not
valid with the configured "dp83869->mode". Would you suggest using a dev_err()
or a dev_dbg()?

Thank you for the feedback on this series.
Andrew Lunn April 27, 2023, 12:04 p.m. UTC | #5 
> > If you say so. I was just thinking you could give the poor software
> > engineer a hint the hardware engineer has put on strapping resistors
> > which means the PHY is not going to work.
> 
> I understand now. I will update this patch to add a print if the MII mode is not
> valid with the configured "dp83869->mode". Would you suggest using a dev_err()
> or a dev_dbg()?

dev_err(). And you can return -EINVAL when asked to set the interface
mode.

	Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 9ab5eff502b7..8dbc502bcd9e 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -692,8 +692,11 @@  static int dp83869_configure_mode(struct phy_device *phydev,
 	/* Below init sequence for each operational mode is defined in
 	 * section 9.4.8 of the datasheet.
 	 */
+	phy_ctrl_val = dp83869->mode;
+	if (phydev->interface == PHY_INTERFACE_MODE_MII)
+		phy_ctrl_val |= DP83869_OP_MODE_MII;
 	ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_OP_MODE,
-			    dp83869->mode);
+			    phy_ctrl_val);
 	if (ret)
 		return ret;