[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y49IBR8ByMQH6oVt@lunn.ch>
Date: Tue, 6 Dec 2022 14:47:49 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Piergiorgio Beruto <piergiorgio.beruto@...il.com>
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
> +// 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.
> +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.
> +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?
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...
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.
> +
> + 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?
> +
> + // 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.
Andrew
Powered by blists - more mailing lists