[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZqH3uTvOrytNHp+9@HYD-DK-UNGSW21.microchip.com>
Date: Thu, 25 Jul 2024 12:29:05 +0530
From: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
To: Andrew Lunn <andrew@...n.ch>
CC: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>, <netdev@...r.kernel.org>,
<davem@...emloft.net>, <kuba@...nel.org>, <hkallweit1@...il.com>,
<linux@...linux.org.uk>, <edumazet@...gle.com>, <pabeni@...hat.com>,
<horatiu.vultur@...rochip.com>, <linux-kernel@...r.kernel.org>,
<UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net] net: phy: micrel: Fix the KSZ9131 MDI-X status issue
Hi Andrew,
Thank you for review the patch.
The 07/18/2024 17:04, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Fri, Jul 12, 2024 at 04:46:48PM +0530, Raju Lakkaraju wrote:
> > Access information about Auto mdix completion and pair selection from the
> > KSZ9131's Auto/MDI/MDI-X status register
>
> Please explain what the broken behaviour is. How would i know i need
> this patch?
>
Ok. I will add broken behaviour details.
> You have not included a Cc: stable tag. Does that mean this does not
> bother anybody and so does not need backporting?
>
Ok. I will add "Cc: stable tag"
> > Fixes: b64e6a8794d9 ("net: phy: micrel: Add PHY Auto/MDI/MDI-X set driver for KSZ9131")
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
> > ---
> > drivers/net/phy/micrel.c | 23 ++++++++++++++++++-----
> > 1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> > index ebafedde0ab7..fddc1b91ba7f 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -1438,6 +1438,9 @@ static int ksz9131_config_init(struct phy_device *phydev)
> > #define MII_KSZ9131_AUTO_MDIX 0x1C
> > #define MII_KSZ9131_AUTO_MDI_SET BIT(7)
> > #define MII_KSZ9131_AUTO_MDIX_SWAP_OFF BIT(6)
> > +#define MII_KSZ9131_DIG_AXAN_STS 0x14
> > +#define MII_KSZ9131_DIG_AXAN_STS_LINK_DET BIT(14)
> > +#define MII_KSZ9131_DIG_AXAN_STS_A_SELECT BIT(12)
> >
> > static int ksz9131_mdix_update(struct phy_device *phydev)
> > {
> > @@ -1452,14 +1455,24 @@ static int ksz9131_mdix_update(struct phy_device *phydev)
> > phydev->mdix_ctrl = ETH_TP_MDI;
> > else
> > phydev->mdix_ctrl = ETH_TP_MDI_X;
> > +
> > + phydev->mdix = phydev->mdix_ctrl;
>
> This seems a bit odd. phydev->mdix_ctrl is what the user wants to
> happen. This is generally ETH_TP_MDI_AUTO, meaning the PHY should
> figure it out. It can be ETH_TP_MDI_X, or ETH_TP_MDI which forces the
> configuration. phydev->mdix is what it has ended up using.
>
I agree. I will fix it.
> So the code above first seems to change what the user asked for. This
> is likely to replace ETH_TP_MDI_AUTO with one of the fixed modes,
> which will then break when the user replaces a crossed cable with a
> straight cable, and the forced mode is then wrong.
>
> Setting mdix to mdix_ctrl then seems wrong. In most cases, you are
> going to get ETH_TP_MDI_AUTO, when in fact you should be returning
> what the PHY has decided on, ETH_TP_MDI_X, ETH_TP_MDI, or
> ETH_TP_MDI_INVALID because the link is down.
Yes. I agree with you.
I will fix it in next version of patch.
>
> Maybe genphy_c45_read_mdix() will help you. It simply reads a PHY
> status register, sets phydev->mdix and it is done.
>
Ok
> Andrew
--
Thanks,
Raju
Powered by blists - more mailing lists