Add support for the onsemi NCN26000 10BASE-T1S industrial Ethernet PHY.
The driver supports Point-to-Multipoint operation without
auto-negotiation and with link control handling. The PLCA RS support
will be included on a separate patch.
Signed-off-by: Piergiorgio Beruto <[email protected]>
---
MAINTAINERS | 7 ++
drivers/net/phy/Kconfig | 7 ++
drivers/net/phy/Makefile | 1 +
drivers/net/phy/ncn26000.c | 193 +++++++++++++++++++++++++++++++++++++
4 files changed, 208 insertions(+)
create mode 100644 drivers/net/phy/ncn26000.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7952243e4b43..f07527baf321 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15357,6 +15357,13 @@ L: [email protected]
S: Maintained
F: arch/mips/boot/dts/ralink/omega2p.dts
+ONSEMI ETHERNET PHY DRIVERS
+M: Piergiorgio Beruto <[email protected]>
+L: [email protected]
+S: Supported
+W: http://www.onsemi.com
+F: drivers/net/phy/ncn*
+
OP-TEE DRIVER
M: Jens Wiklander <[email protected]>
L: [email protected]
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index af00cf44cd97..7c466830c611 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -267,6 +267,13 @@ config NATIONAL_PHY
help
Currently supports the DP83865 PHY.
+config NCN26000_PHY
+ tristate "onsemi 10BASE-T1S Ethernet PHY"
+ help
+ Adds support for the onsemi 10BASE-T1S Ethernet PHY.
+ Currently supports the NCN26000 10BASE-T1S Industrial PHY
+ with MII interface.
+
config NXP_C45_TJA11XX_PHY
tristate "NXP C45 TJA11XX PHYs"
depends on PTP_1588_CLOCK_OPTIONAL
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index f7138d3c896b..b5138066ba04 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_MICROCHIP_T1_PHY) += microchip_t1.o
obj-$(CONFIG_MICROSEMI_PHY) += mscc/
obj-$(CONFIG_MOTORCOMM_PHY) += motorcomm.o
obj-$(CONFIG_NATIONAL_PHY) += national.o
+obj-$(CONFIG_NCN26000_PHY) += ncn26000.o
obj-$(CONFIG_NXP_C45_TJA11XX_PHY) += nxp-c45-tja11xx.o
obj-$(CONFIG_NXP_TJA11XX_PHY) += nxp-tja11xx.o
obj-$(CONFIG_QSEMI_PHY) += qsemi.o
diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c
new file mode 100644
index 000000000000..65a34edc5b20
--- /dev/null
+++ b/drivers/net/phy/ncn26000.c
@@ -0,0 +1,193 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+/*
+ * Driver for Analog Devices Industrial Ethernet T1L PHYs
+ *
+ * Copyright 2020 Analog Devices Inc.
+ */
+#include <linux/kernel.h>
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mii.h>
+#include <linux/phy.h>
+#include <linux/property.h>
+
+#define PHY_ID_NCN26000 0x180FF5A1
+
+#define NCN26000_REG_IRQ_CTL ((u16)16)
+#define NCN26000_REG_IRQ_STATUS ((u16)17)
+
+#define NCN26000_IRQ_LINKST_BIT ((u16)1)
+#define NCN26000_IRQ_PLCAST_BIT ((u16)(1 << 1))
+#define NCN26000_IRQ_LJABBER_BIT ((u16)(1 << 2))
+#define NCN26000_IRQ_RJABBER_BIT ((u16)(1 << 3))
+#define NCN26000_IRQ_RJABBER_BIT ((u16)(1 << 3))
+#define NCN26000_IRQ_PLCAREC_BIT ((u16)(1 << 4))
+#define NCN26000_IRQ_PHYSCOL_BIT ((u16)(1 << 5))
+
+struct ncn26000_priv {
+ u16 enabled_irqs;
+};
+
+static int ncn26000_config_init(struct phy_device *phydev)
+{
+ // TODO: add vendor-specific tuning (ENI, CMC, ...)
+ return 0;
+}
+
+static int ncn26000_enable(struct phy_device *phydev)
+{
+ phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+ phydev->mdix = ETH_TP_MDI;
+ phydev->pause = 0;
+ phydev->asym_pause = 0;
+ phydev->speed = SPEED_10;
+ phydev->duplex = DUPLEX_HALF;
+
+ // bring up the link (link_ctrl is mapped to BMCR_ANENABLE)
+ // clear also ISOLATE mode and Collision Test
+ return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
+}
+
+static int ncn26000_soft_reset(struct phy_device *phydev)
+{
+ int ret;
+
+ ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
+
+ if (ret != 0)
+ return ret;
+
+ return phy_read_poll_timeout(phydev,
+ MII_BMCR,
+ ret,
+ !(ret & BMCR_RESET),
+ 500,
+ 20000,
+ true);
+}
+
+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);
+ return 0;
+}
+
+static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
+{
+ const struct ncn26000_priv *const priv = phydev->priv;
+ u16 events;
+ int ret;
+
+ // read and aknowledge the IRQ status register
+ ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
+
+ if (unlikely(ret < 0))
+ return IRQ_NONE;
+
+ events = (u16)ret & priv->enabled_irqs;
+ if (events == 0)
+ return IRQ_NONE;
+
+ if (events & NCN26000_IRQ_LINKST_BIT) {
+ ret = phy_read(phydev, MII_BMSR);
+
+ if (unlikely(ret < 0)) {
+ phydev_err(phydev,
+ "error reading the status register (%d)\n",
+ ret);
+
+ return IRQ_NONE;
+ }
+
+ phydev->link = ((u16)ret & BMSR_ANEGCOMPLETE) ? 1 : 0;
+ }
+
+ // handle more IRQs here
+
+ phy_trigger_machine(phydev);
+ return IRQ_HANDLED;
+}
+
+static int ncn26000_config_intr(struct phy_device *phydev)
+{
+ int ret;
+ struct ncn26000_priv *priv = phydev->priv;
+
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+ // acknowledge IRQs
+ ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
+ if (ret < 0)
+ return ret;
+
+ // get link status notifications
+ priv->enabled_irqs = NCN26000_IRQ_LINKST_BIT;
+ } else {
+ // disable all IRQs
+ priv->enabled_irqs = 0;
+ }
+
+ ret = phy_write(phydev, NCN26000_REG_IRQ_CTL, priv->enabled_irqs);
+ if (ret != 0)
+ return ret;
+
+ return 0;
+}
+
+static int ncn26000_probe(struct phy_device *phydev)
+{
+ struct device *dev = &phydev->mdio.dev;
+ struct ncn26000_priv *priv;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ phydev->priv = priv;
+
+ return 0;
+}
+
+static void ncn26000_remove(struct phy_device *phydev)
+{
+ struct device *dev = &phydev->mdio.dev;
+ struct ncn26000_priv *priv = phydev->priv;
+
+ // free the private structure pointer
+ devm_kfree(dev, priv);
+}
+
+static struct phy_driver ncn26000_driver[] = {
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_NCN26000),
+ .name = "NCN26000",
+ .probe = ncn26000_probe,
+ .remove = ncn26000_remove,
+ .get_features = ncn26000_get_features,
+ .config_init = ncn26000_config_init,
+ .soft_reset = ncn26000_soft_reset,
+ .config_intr = ncn26000_config_intr,
+ .config_aneg = ncn26000_enable,
+ .handle_interrupt = ncn26000_handle_interrupt,
+ },
+};
+
+module_phy_driver(ncn26000_driver);
+
+static struct mdio_device_id __maybe_unused ncn26000_tbl[] = {
+ { PHY_ID_MATCH_MODEL(PHY_ID_NCN26000) },
+ { }
+};
+
+MODULE_DEVICE_TABLE(mdio, ncn26000_tbl);
+MODULE_AUTHOR("Piergiorgio Beruto");
+MODULE_DESCRIPTION("onsemi 10BASE-T1S PHY driver");
+MODULE_LICENSE("Dual BSD/GPL");
--
2.35.1
Hi,
On Sun, Dec 04, 2022 at 03:31:33AM +0100, Piergiorgio Beruto wrote:
> diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c
> new file mode 100644
> index 000000000000..65a34edc5b20
> --- /dev/null
> +++ b/drivers/net/phy/ncn26000.c
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/*
> + * Driver for Analog Devices Industrial Ethernet T1L PHYs
> + *
> + * Copyright 2020 Analog Devices Inc.
> + */
> +#include <linux/kernel.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mii.h>
> +#include <linux/phy.h>
> +#include <linux/property.h>
> +
> +#define PHY_ID_NCN26000 0x180FF5A1
> +
> +#define NCN26000_REG_IRQ_CTL ((u16)16)
> +#define NCN26000_REG_IRQ_STATUS ((u16)17)
> +
> +#define NCN26000_IRQ_LINKST_BIT ((u16)1)
> +#define NCN26000_IRQ_PLCAST_BIT ((u16)(1 << 1))
> +#define NCN26000_IRQ_LJABBER_BIT ((u16)(1 << 2))
> +#define NCN26000_IRQ_RJABBER_BIT ((u16)(1 << 3))
> +#define NCN26000_IRQ_RJABBER_BIT ((u16)(1 << 3))
> +#define NCN26000_IRQ_PLCAREC_BIT ((u16)(1 << 4))
> +#define NCN26000_IRQ_PHYSCOL_BIT ((u16)(1 << 5))
There isn't much point in having the casts to u16 here. Also,
BIT() is useful for identifying single bits.
> +static int ncn26000_enable(struct phy_device *phydev)
> +{
This is actually the config_aneg() implementation, it should be named
as such.
> + phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> + phydev->mdix = ETH_TP_MDI;
> + phydev->pause = 0;
> + phydev->asym_pause = 0;
> + phydev->speed = SPEED_10;
> + phydev->duplex = DUPLEX_HALF;
Is this initialisation actually necessary?
> +
> + // bring up the link (link_ctrl is mapped to BMCR_ANENABLE)
> + // clear also ISOLATE mode and Collision Test
> + return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
You always use AN even when ethtool turns off AN? If AN is mandatory,
it seems there should be some way that phylib can force that to be
the case.
> +}
> +
> +static int ncn26000_soft_reset(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
> +
> + if (ret != 0)
> + return ret;
> +
> + return phy_read_poll_timeout(phydev,
> + MII_BMCR,
> + ret,
> + !(ret & BMCR_RESET),
> + 500,
> + 20000,
> + true);
Isn't this just genphy_reset() ?
> +}
> +
> +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);
> + return 0;
> +}
> +
> +static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
> +{
> + const struct ncn26000_priv *const priv = phydev->priv;
> + u16 events;
> + int ret;
> +
> + // read and aknowledge the IRQ status register
> + ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> +
> + if (unlikely(ret < 0))
> + return IRQ_NONE;
> +
> + events = (u16)ret & priv->enabled_irqs;
> + if (events == 0)
> + return IRQ_NONE;
> +
> + if (events & NCN26000_IRQ_LINKST_BIT) {
> + ret = phy_read(phydev, MII_BMSR);
> +
> + if (unlikely(ret < 0)) {
> + phydev_err(phydev,
> + "error reading the status register (%d)\n",
> + ret);
> +
> + return IRQ_NONE;
> + }
> +
> + phydev->link = ((u16)ret & BMSR_ANEGCOMPLETE) ? 1 : 0;
1. aneg_complete shouldn't be used to set phydev->link.
2. phydev->link should be updated in the read_status() function, which
the state machine will call. Setting it here without taking the lock
introduces races.
> + }
> +
> + // handle more IRQs here
> +
> + phy_trigger_machine(phydev);
> + return IRQ_HANDLED;
> +}
> +
> +static int ncn26000_config_intr(struct phy_device *phydev)
> +{
> + int ret;
> + struct ncn26000_priv *priv = phydev->priv;
> +
> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> + // acknowledge IRQs
> + ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + // get link status notifications
> + priv->enabled_irqs = NCN26000_IRQ_LINKST_BIT;
> + } else {
> + // disable all IRQs
> + priv->enabled_irqs = 0;
> + }
> +
> + ret = phy_write(phydev, NCN26000_REG_IRQ_CTL, priv->enabled_irqs);
> + if (ret != 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int ncn26000_probe(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + struct ncn26000_priv *priv;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + phydev->priv = priv;
> +
> + return 0;
> +}
> +
> +static void ncn26000_remove(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + struct ncn26000_priv *priv = phydev->priv;
> +
> + // free the private structure pointer
> + devm_kfree(dev, priv);
No need to call devm_kfree() - the point of devm_*() is that resources
are automatically released.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> > +
> > + // bring up the link (link_ctrl is mapped to BMCR_ANENABLE)
> > + // clear also ISOLATE mode and Collision Test
> > + return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
>
> You always use AN even when ethtool turns off AN? If AN is mandatory,
> it seems there should be some way that phylib can force that to be
> the case.
Hi Russell
The comment is trying to explain that the bit BMCR_ANENABLE does not
actually mean Enable Autoneg. Since AN is not supported by T1S PHYs,
and the standard, some vendors have repurposed this bit.
Maybe we need BMCR_T1S_LINK_CTRL, local to this driver.
Andrew
> On Sun, Dec 04, 2022 at 03:31:33AM +0100, Piergiorgio Beruto wrote:
> > +static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
> > +{
> > + const struct ncn26000_priv *const priv = phydev->priv;
> > + u16 events;
> > + int ret;
> > +
> > + // read and aknowledge the IRQ status register
> > + ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> > +
> > + if (unlikely(ret < 0))
> > + return IRQ_NONE;
> > +
> > + events = (u16)ret & priv->enabled_irqs;
> > + if (events == 0)
> > + return IRQ_NONE;
> > +
> > + if (events & NCN26000_IRQ_LINKST_BIT) {
> > + ret = phy_read(phydev, MII_BMSR);
> > +
> > + if (unlikely(ret < 0)) {
> > + phydev_err(phydev,
> > + "error reading the status register (%d)\n",
> > + ret);
> > +
> > + return IRQ_NONE;
> > + }
> > +
> > + phydev->link = ((u16)ret & BMSR_ANEGCOMPLETE) ? 1 : 0;
Hi Piergiorgio
Interrupt handling in PHY don't follow the usual model. Historically,
PHYs were always polled once per second. The read_status() function
gets called to report the current status of the PHY. Interrupt are
just used to indicate that poll should happen now. All the handler
needs to do is clear the interrupt line so it can be safely reenabled
and not cause an interrupt storm, and call phy_trigger_machine() to
trigger the poll.
Andrew
> > > +static int ncn26000_enable(struct phy_device *phydev)
> > > +{
> >
> > This is actually the config_aneg() implementation, it should be named
> > as such.
> I can certainly rename it, however I did this for a reason. The NCN26000
> only supports P2MP mode. Therefore, it does not support AN (this is
> clearly indicated in the IEEE specifications as well).
>
> However, it is my understanding that the config_aneg() callback is
> invoked also for PHYs that do not support AN, and this is actually the
> only way to set a link_control bit to have the PHY enable the PMA/PCS
> functions. So I thought to call this function "enable" to make it clear
> we're not really implementing autoneg, but link_control.
Anybody familiar with PHY drivers knows the name is not ideal, but
when they see config_aneg() they have a good idea what it does without
having to look at the code. All PHY drivers should have the same basic
structure, naming etc, just to make knowledge transfer between drivers
easy, maintenance easy, etc.
Andrew
On Sun, Dec 04, 2022 at 07:58:07PM +0100, Andrew Lunn wrote:
> > > > +static int ncn26000_enable(struct phy_device *phydev)
> > > > +{
> > >
> > > This is actually the config_aneg() implementation, it should be named
> > > as such.
> > I can certainly rename it, however I did this for a reason. The NCN26000
> > only supports P2MP mode. Therefore, it does not support AN (this is
> > clearly indicated in the IEEE specifications as well).
> >
> > However, it is my understanding that the config_aneg() callback is
> > invoked also for PHYs that do not support AN, and this is actually the
> > only way to set a link_control bit to have the PHY enable the PMA/PCS
> > functions. So I thought to call this function "enable" to make it clear
> > we're not really implementing autoneg, but link_control.
>
> Anybody familiar with PHY drivers knows the name is not ideal, but
> when they see config_aneg() they have a good idea what it does without
> having to look at the code. All PHY drivers should have the same basic
> structure, naming etc, just to make knowledge transfer between drivers
> easy, maintenance easy, etc.
Fair enough, I'll change the name in the next patchset version
On Sun, Dec 04, 2022 at 04:52:04PM +0000, Russell King (Oracle) wrote:
> Hi,
>
> On Sun, Dec 04, 2022 at 03:31:33AM +0100, Piergiorgio Beruto wrote:
> > diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c
> > new file mode 100644
> > index 000000000000..65a34edc5b20
> > --- /dev/null
> > +++ b/drivers/net/phy/ncn26000.c
> > @@ -0,0 +1,193 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > +/*
> > + * Driver for Analog Devices Industrial Ethernet T1L PHYs
> > + *
> > + * Copyright 2020 Analog Devices Inc.
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/mii.h>
> > +#include <linux/phy.h>
> > +#include <linux/property.h>
> > +
> > +#define PHY_ID_NCN26000 0x180FF5A1
> > +
> > +#define NCN26000_REG_IRQ_CTL ((u16)16)
> > +#define NCN26000_REG_IRQ_STATUS ((u16)17)
> > +
> > +#define NCN26000_IRQ_LINKST_BIT ((u16)1)
> > +#define NCN26000_IRQ_PLCAST_BIT ((u16)(1 << 1))
> > +#define NCN26000_IRQ_LJABBER_BIT ((u16)(1 << 2))
> > +#define NCN26000_IRQ_RJABBER_BIT ((u16)(1 << 3))
> > +#define NCN26000_IRQ_RJABBER_BIT ((u16)(1 << 3))
> > +#define NCN26000_IRQ_PLCAREC_BIT ((u16)(1 << 4))
> > +#define NCN26000_IRQ_PHYSCOL_BIT ((u16)(1 << 5))
>
> There isn't much point in having the casts to u16 here. Also,
> BIT() is useful for identifying single bits.
Ok, I'll fix, thanks.
> > +static int ncn26000_enable(struct phy_device *phydev)
> > +{
>
> This is actually the config_aneg() implementation, it should be named
> as such.
I can certainly rename it, however I did this for a reason. The NCN26000
only supports P2MP mode. Therefore, it does not support AN (this is
clearly indicated in the IEEE specifications as well).
However, it is my understanding that the config_aneg() callback is
invoked also for PHYs that do not support AN, and this is actually the
only way to set a link_control bit to have the PHY enable the PMA/PCS
functions. So I thought to call this function "enable" to make it clear
we're not really implementing autoneg, but link_control.
But as I said, I am not strongly biased towards this name, I just wanted
to let you know the rationale behind my choice.
Please let me know if you wish to reconsider or you still prefer to
rename it.
> > + phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> > + phydev->mdix = ETH_TP_MDI;
> > + phydev->pause = 0;
> > + phydev->asym_pause = 0;
> > + phydev->speed = SPEED_10;
> > + phydev->duplex = DUPLEX_HALF;
>
> Is this initialisation actually necessary?
To be honest, I am not sure. Reading the code for genphy_c45_read_pma()
and genphy_c45_read_status() I can see those variables are set. In my
case, the driver is -not- invoking those functions, therefore I thought
this initialization should be needed. If not, I can certainly remove it.
Advices?
> > +
> > + // bring up the link (link_ctrl is mapped to BMCR_ANENABLE)
> > + // clear also ISOLATE mode and Collision Test
> > + return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
>
> You always use AN even when ethtool turns off AN? If AN is mandatory,
> it seems there should be some way that phylib can force that to be
> the case.
I need to explain this better. The NCN26000, as I said earlier, does
-not- support AN. However, it re-uses the AN bit to implement the
link_control function (described in the IEEE specifications). Therefore,
setting AN on this PHY actually means bringing up the link.
I don't know if it could be better to add a define (specific for this
PHY) for the link_control bit and set it == BMCR_ANENABLE? Would that be
more clear for the reader?
> > +}
> > +
> > +static int ncn26000_soft_reset(struct phy_device *phydev)
> > +{
> > + int ret;
> > +
> > + ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
> > +
> > + if (ret != 0)
> > + return ret;
> > +
> > + return phy_read_poll_timeout(phydev,
> > + MII_BMCR,
> > + ret,
> > + !(ret & BMCR_RESET),
> > + 500,
> > + 20000,
> > + true);
>
> Isn't this just genphy_reset() ?
Right, this was a leftover. I substituted with genphy_soft_reset() and
indeed it works just fine. Thanks for noticing.
> > +}
> > +
> > +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);
> > + return 0;
> > +}
> > +
> > +static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
> > +{
> > + const struct ncn26000_priv *const priv = phydev->priv;
> > + u16 events;
> > + int ret;
> > +
> > + // read and aknowledge the IRQ status register
> > + ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> > +
> > + if (unlikely(ret < 0))
> > + return IRQ_NONE;
> > +
> > + events = (u16)ret & priv->enabled_irqs;
> > + if (events == 0)
> > + return IRQ_NONE;
> > +
> > + if (events & NCN26000_IRQ_LINKST_BIT) {
> > + ret = phy_read(phydev, MII_BMSR);
> > +
> > + if (unlikely(ret < 0)) {
> > + phydev_err(phydev,
> > + "error reading the status register (%d)\n",
> > + ret);
> > +
> > + return IRQ_NONE;
> > + }
> > +
> > + phydev->link = ((u16)ret & BMSR_ANEGCOMPLETE) ? 1 : 0;
>
> 1. aneg_complete shouldn't be used to set phydev->link.
> 2. phydev->link should be updated in the read_status() function, which
> the state machine will call. Setting it here without taking the lock
> introduces races.
Same as before. AN complete is used as an extended link status
indication for this PHY, considering the PLCA status as well. It is not
the result of AN (which is not supported).
> > + }
> > +
> > + // handle more IRQs here
> > +
> > + phy_trigger_machine(phydev);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int ncn26000_config_intr(struct phy_device *phydev)
> > +{
> > + int ret;
> > + struct ncn26000_priv *priv = phydev->priv;
> > +
> > + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> > + // acknowledge IRQs
> > + ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> > + if (ret < 0)
> > + return ret;
> > +
> > + // get link status notifications
> > + priv->enabled_irqs = NCN26000_IRQ_LINKST_BIT;
> > + } else {
> > + // disable all IRQs
> > + priv->enabled_irqs = 0;
> > + }
> > +
> > + ret = phy_write(phydev, NCN26000_REG_IRQ_CTL, priv->enabled_irqs);
> > + if (ret != 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int ncn26000_probe(struct phy_device *phydev)
> > +{
> > + struct device *dev = &phydev->mdio.dev;
> > + struct ncn26000_priv *priv;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + phydev->priv = priv;
> > +
> > + return 0;
> > +}
> > +
> > +static void ncn26000_remove(struct phy_device *phydev)
> > +{
> > + struct device *dev = &phydev->mdio.dev;
> > + struct ncn26000_priv *priv = phydev->priv;
> > +
> > + // free the private structure pointer
> > + devm_kfree(dev, priv);
>
> No need to call devm_kfree() - the point of devm_*() is that resources
> are automatically released.
>
> Thanks.
Got it, Thanks!
On Sun, Dec 04, 2022 at 07:00:38PM +0100, Andrew Lunn wrote:
> > On Sun, Dec 04, 2022 at 03:31:33AM +0100, Piergiorgio Beruto wrote:
> > > +static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
> > > +{
> > > + const struct ncn26000_priv *const priv = phydev->priv;
> > > + u16 events;
> > > + int ret;
> > > +
> > > + // read and aknowledge the IRQ status register
> > > + ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> > > +
> > > + if (unlikely(ret < 0))
> > > + return IRQ_NONE;
> > > +
> > > + events = (u16)ret & priv->enabled_irqs;
> > > + if (events == 0)
> > > + return IRQ_NONE;
> > > +
> > > + if (events & NCN26000_IRQ_LINKST_BIT) {
> > > + ret = phy_read(phydev, MII_BMSR);
> > > +
> > > + if (unlikely(ret < 0)) {
> > > + phydev_err(phydev,
> > > + "error reading the status register (%d)\n",
> > > + ret);
> > > +
> > > + return IRQ_NONE;
> > > + }
> > > +
> > > + phydev->link = ((u16)ret & BMSR_ANEGCOMPLETE) ? 1 : 0;
>
> Hi Piergiorgio
>
> Interrupt handling in PHY don't follow the usual model. Historically,
> PHYs were always polled once per second. The read_status() function
> gets called to report the current status of the PHY. Interrupt are
> just used to indicate that poll should happen now. All the handler
> needs to do is clear the interrupt line so it can be safely reenabled
> and not cause an interrupt storm, and call phy_trigger_machine() to
> trigger the poll.
To be honest, I sort of realized that. But I was confused again by the
read_status() function description. It looked to me it was not invoked
when AN is not supported, but I'm hearing this is not the case.
I'll rework this part.
Thanks
Piergiorgio