Message-ID: <Y49THkXZdLBR6Mxv@gvm01>
Date:   Tue, 6 Dec 2022 15:35:10 +0100
From:   Piergiorgio Beruto <piergiorgio.beruto@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [PATCH v4 net-next 5/5] drivers/net/phy: add driver for the
 onsemi NCN26000 10BASE-T1S PHY

Hi Andrew,
thank you for your review. Please, see my answers below.

Thanks,
Piergiorgio

On Tue, Dec 06, 2022 at 02:47:49PM +0100, Andrew Lunn wrote:
> > +// module parameter: if set, the link status is derived from the PLCA status
> > +// default: false
> > +static bool link_status_plca;
> > +module_param(link_status_plca, bool, 0644);
> 
> No module parameters, they are considered a bad user interface.
OK, as you see I'm a bit "outdated" :-)
What would be the alternative? There is a bunch of vendor-specific PHY
features that I would like to expose at some point (e.g. regulation of
TX voltage levels). Thanks!
 
> > +static int ncn26000_get_features(struct phy_device *phydev)
> > +{
> > +	linkmode_zero(phydev->supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_MII_BIT, phydev->supported);
> > +
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
> > +			 phydev->supported);
> > +
> > +	linkmode_copy(phydev->advertising, phydev->supported);
> 
> That should not be needed.
> 
> Also, look at PHY_BASIC_T1_FEATURES, and how it is used in
> microchip_t1.c.
In principle, I agree. But I have a problem with this specific chip, two
actually...

1. The chip does -not- set the MDIO_PMA_EXTABLE while it should. This
has been fixed in new versions of the chip, but for now, the bit is 0
so genphy_c45_baset1_able() reports 'false'.

2. This PHY is one of the PHYs that emulates AN (we discussed about this
earlier), but it does not actually implement it.

Therefore, I thought to just force the capabilities. In the future, I
could read the chip ID/version and make a decision based on that (force
or use the standard c45 functions).

Doe it make sense to you?

> > +static int ncn26000_read_status(struct phy_device *phydev)
> > +{
> > +	// The NCN26000 reports NCN26000_LINK_STATUS_BIT if the link status of
> > +	// the PHY is up. It further reports the logical AND of the link status
> > +	// and the PLCA status in the BMSR_LSTATUS bit. Thus, report the link
> > +	// status by testing the appropriate BMSR bit according to the module's
> > +	// parameter configuration.
> > +	const int lstatus_flag = link_status_plca ?
> > +		BMSR_LSTATUS : NCN26000_BMSR_LINK_STATUS_BIT;
> > +
> > +	int ret;
> > +
> > +	ret = phy_read(phydev, MII_BMSR);
> > +	if (unlikely(ret < 0))
> > +		return ret;
> > +
> > +	// update link status
> > +	phydev->link = (ret & lstatus_flag) ? 1 : 0;
> 
> What about the latching behaviour of LSTATUS?
See further down.

> 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L2289
> 
> > +
> > +	// handle more IRQs here
> 
> You are not in an IRQ handler...
Right, this is just a left-over when I moved the code from the ISR to
this functions. Fixed.

> You should also be setting speed and duplex. I don't think they are
> guaranteed to have any specific value if you don't set them.
Ah, I got that before, but I removed it after comment from Russell
asking me not to do this. Testing on my HW, this seems to work, although
I'm not sure whether this is correct or it is working 'by chance' ?

> > +
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
> > +{
> > +	const struct ncn26000_priv *const priv = phydev->priv;
> > +	int ret;
> > +
> > +	// clear the latched bits in MII_BMSR
> > +	phy_read(phydev, MII_BMSR);
> 
> Why?
That was my ugly handling of the status double-read...
See the pacth below! I copied part of the code you suggested.

> 
> > +
> > +	// read and aknowledge the IRQ status register
> > +	ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> > +
> > +	if (unlikely(ret < 0) || (ret & priv->enabled_irqs) == 0)
> 
> How does NCN26000_REG_IRQ_STATUS work? Can it have bits set which are
> not in NCN26000_REG_IRQ_CTL ? That does happen sometimes, but is
> pretty unusual. If not, you don't need to track priv->enabled_irqs,
> just ensure ret is not 0.
It has a single bit that is not maskable. That would be the reset event
bit, which is triggered if the chip goes through a spurious reset. Since
I did not want to handle this in this first version of the driver, I
just masked it this way.
Thoughts?


diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c
index 9e02c5c55244..198539b7ee66 100644
--- a/drivers/net/phy/ncn26000.c
+++ b/drivers/net/phy/ncn26000.c
@@ -92,15 +92,27 @@ static int ncn26000_read_status(struct phy_device *phydev)

        int ret;

+       /* The link state is latched low so that momentary link
+        * drops can be detected. Do not double-read the status
+        * in polling mode to detect such short link drops except
+        * the link was already down.
+        */
+       if (!phy_polling_mode(phydev) || !phydev->link) {
+               ret = phy_read(phydev, MII_BMSR);
+               if (ret < 0)
+                       return ret;
+               else if (ret & lstatus_flag)
+                       goto upd_link;
+       }
+
        ret = phy_read(phydev, MII_BMSR);
        if (unlikely(ret < 0))
                return ret;

+upd_link:
        // update link status
        phydev->link = (ret & lstatus_flag) ? 1 : 0;

-       // handle more IRQs here
-
        return 0;
 }

@@ -109,9 +121,6 @@ static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
        const struct ncn26000_priv *const priv = phydev->priv;
        int ret;

-       // clear the latched bits in MII_BMSR
-       phy_read(phydev, MII_BMSR);
-
        // read and aknowledge the IRQ status register
        ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);

Powered by blists - more mailing lists