RE: [PATCH net-next] net: phy: microchip_t1: Adds support for LAN887x phy
From: Divya.Koppera
Date: Fri Aug 09 2024 - 07:59:32 EST
Hi Russell,
Thanks for the review, please find my reply inline.
> -----Original Message-----
> From: Russell King <linux@xxxxxxxxxxxxxxx>
> Sent: Thursday, August 8, 2024 5:49 PM
> To: Divya Koppera - I30481 <Divya.Koppera@xxxxxxxxxxxxx>
> Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@xxxxxxxxxxxxx>;
> UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx>; andrew@xxxxxxx;
> hkallweit1@xxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next] net: phy: microchip_t1: Adds support for
> LAN887x phy
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Thu, Aug 08, 2024 at 08:29:16PM +0530, Divya Koppera wrote:
> > +static int lan887x_config_init(struct phy_device *phydev) {
> > + /* Disable pause frames */
> > + linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev-
> >supported);
> > + /* Disable asym pause */
> > + linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> > +phydev->supported);
>
> Why is this here? Pause frames are just like normal ethernet frames, they only
> have meaning to the MAC, not to the PHY.
>
> In any case, by the time the config_init() method has been called, the higher
> levels have already looked at phydev->supported and made decisions on
> what's there.
>
We tried to disable this in get_features.
These are set again in phy_probe API.
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/phy/phy_device.c#n3544
We will re-look into these settings while submitting auto-negotiation patch in future series.
> > +static int lan887x_config_aneg(struct phy_device *phydev) {
> > + int ret;
> > +
> > + /* First patch only supports 100Mbps and 1000Mbps force-mode.
> > + * T1 Auto-Negotiation (Clause 98 of IEEE 802.3) will be added later.
> > + */
> > + if (phydev->autoneg != AUTONEG_DISABLE) {
> > + /* PHY state is inconsistent due to ANEG Enable set
> > + * so we need to assign ANEG Disable for consistent behavior
> > + */
> > + phydev->autoneg = AUTONEG_DISABLE;
>
> If you clear phydev->supported's autoneg bit, then phylib ought to enforce
> this for you. Please check this rather than adding code to drivers.
Phylib is checking if advertisement is empty or not, but the feature is not verified against supported parameter.
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/phy/phy.c#n1092
But in the following statement phylib is updating advertising parameter.
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/phy/phy.c#n1113
This is making the feature enabled in driver, the right thing is to fix the library.
We will fix the phylib in next series.
>
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Thanks,
Divya