This patch adds 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 PHY also features
PLCA for improving performance in P2MP mode.
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 ed626cbdf5af..09f0bfa3ae64 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..9e02c5c55244
--- /dev/null
+++ b/drivers/net/phy/ncn26000.c
@@ -0,0 +1,193 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+/*
+ * Driver for the onsemi 10BASE-T1S NCN26000 PHYs family.
+ *
+ * Copyright 2022 onsemi
+ */
+#include <linux/kernel.h>
+#include <linux/bitfield.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mii.h>
+#include <linux/phy.h>
+
+#include "mdio-open-alliance.h"
+
+#define PHY_ID_NCN26000 0x180FF5A1
+
+#define NCN26000_REG_IRQ_CTL 16
+#define NCN26000_REG_IRQ_STATUS 17
+
+// the NCN26000 maps link_ctrl to BMCR_ANENABLE
+#define NCN26000_BCMR_LINK_CTRL_BIT BMCR_ANENABLE
+
+// the NCN26000 maps link_status to BMSR_ANEGCOMPLETE
+#define NCN26000_BMSR_LINK_STATUS_BIT BMSR_ANEGCOMPLETE
+
+#define NCN26000_IRQ_LINKST_BIT BIT(0)
+#define NCN26000_IRQ_PLCAST_BIT BIT(1)
+#define NCN26000_IRQ_LJABBER_BIT BIT(2)
+#define NCN26000_IRQ_RJABBER_BIT BIT(3)
+#define NCN26000_IRQ_PLCAREC_BIT BIT(4)
+#define NCN26000_IRQ_PHYSCOL_BIT BIT(5)
+
+#define TO_TMR_DEFAULT 32
+
+struct ncn26000_priv {
+ u16 enabled_irqs;
+};
+
+// 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);
+
+// driver callbacks
+
+static int ncn26000_config_init(struct phy_device *phydev)
+{
+ /* HW bug workaround: the default value of the PLCA TO_TIMER should be
+ * 32, where the current version of NCN26000 reports 24. This will be
+ * fixed in future PHY versions. For the time being, we force the
+ * correct default here.
+ */
+ return phy_write_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_TOTMR,
+ TO_TMR_DEFAULT);
+}
+
+static int ncn26000_config_aneg(struct phy_device *phydev)
+{
+ // Note: the NCN26000 supports only P2MP link mode. Therefore, AN is not
+ // supported. However, this function is invoked by phylib to enable the
+ // PHY, regardless of the AN support.
+ phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+ phydev->mdix = ETH_TP_MDI;
+
+ // bring up the link
+ return phy_write(phydev, MII_BMCR, NCN26000_BCMR_LINK_CTRL_BIT);
+}
+
+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 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;
+
+ // handle more IRQs here
+
+ 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);
+
+ // read and aknowledge the IRQ status register
+ ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
+
+ if (unlikely(ret < 0) || (ret & priv->enabled_irqs) == 0)
+ return IRQ_NONE;
+
+ 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 struct phy_driver ncn26000_driver[] = {
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_NCN26000),
+ .name = "NCN26000",
+ .probe = ncn26000_probe,
+ .get_features = ncn26000_get_features,
+ .config_init = ncn26000_config_init,
+ .config_intr = ncn26000_config_intr,
+ .config_aneg = ncn26000_config_aneg,
+ .read_status = ncn26000_read_status,
+ .handle_interrupt = ncn26000_handle_interrupt,
+ .get_plca_cfg = genphy_c45_plca_get_cfg,
+ .set_plca_cfg = genphy_c45_plca_set_cfg,
+ .get_plca_status = genphy_c45_plca_get_status,
+ .soft_reset = genphy_soft_reset,
+ },
+};
+
+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 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);
> +// 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
On Tue, Dec 06, 2022 at 03:35:10PM +0100, Piergiorgio Beruto wrote:
> On Tue, Dec 06, 2022 at 02:47:49PM +0100, Andrew Lunn wrote:
> > > +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' ?
I asked you to get rid of them in the config function, which was
setting them to "unknown" values. I thought I explained why it was
wrong to set them there - but again...
If you force the values in the config function, then when userspace
does a read-modify-write of the settings via ethtool, you will end
up wiping out the PHYs link settings, despite maybe nothing having
actually been changed. It is also incorrect to set them in the
config function, because those writes to those variables can race
with users reading them - the only place they should be set by a
PHY driver is in the .read_status method.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> 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!
TX voltages sound like they are board specific, so use DT properties.
For runtime properties, look at phy tunables.
Andrew
On Tue, Dec 06, 2022 at 02:57:04PM +0000, Russell King (Oracle) wrote:
> On Tue, Dec 06, 2022 at 03:35:10PM +0100, Piergiorgio Beruto wrote:
> > On Tue, Dec 06, 2022 at 02:47:49PM +0100, Andrew Lunn wrote:
> > > > +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' ?
>
> I asked you to get rid of them in the config function, which was
> setting them to "unknown" values. I thought I explained why it was
> wrong to set them there - but again...
>
> If you force the values in the config function, then when userspace
> does a read-modify-write of the settings via ethtool, you will end
> up wiping out the PHYs link settings, despite maybe nothing having
> actually been changed. It is also incorrect to set them in the
> config function, because those writes to those variables can race
> with users reading them - the only place they should be set by a
> PHY driver is in the .read_status method.
Ok, I must have misunderstood what the problem was. This is clear to me
now, I'm going to add this back in the read_status() method.
Thanks,
Piergiorgio
On Tue, Dec 06, 2022 at 05:50:13PM +0100, Andrew Lunn wrote:
> > 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!
>
> TX voltages sound like they are board specific, so use DT properties.
>
> For runtime properties, look at phy tunables.
Ok, but as far as I understand tunables, these need to be "generalized".
I was wondering if there is some interface (sysfs / proc / other) to set
parameters which are very specific to a PHY implementation?
Thanks,
Piergiorgio
> I was wondering if there is some interface (sysfs / proc / other) to set
> parameters which are very specific to a PHY implementation?
Please describe what they are, and in what context you need them. Then
we can decide on the correct API.
In general, the OS is there to abstract over the hardware so they all
look the same. We don't want anything specific to the PHY.
Andrew
On Tue, Dec 06, 2022 at 08:12:02PM +0100, Andrew Lunn wrote:
> > I was wondering if there is some interface (sysfs / proc / other) to set
> > parameters which are very specific to a PHY implementation?
>
> Please describe what they are, and in what context you need them. Then
> we can decide on the correct API.
>
> In general, the OS is there to abstract over the hardware so they all
> look the same. We don't want anything specific to the PHY.
That's clear, let me explain.
Enable of enhanced-noise-immunity mode
- This trades off CSMA/CD performance for noise immunity. It could be a
static setting, but the user may want to conditionally enable it
depending on application decisions. E.g. some people may want to
enable/disable this when using CSMA/CD as a fallback in case the PLCA
coordinator disappears. Of course, there are better ways of doing
this, but it is a possible use-case that some people want to use.
Tuning of internal impedance to match the line/MDI
- This is really board dependent, so DT seems good to me
Tuning of PMA filters to optimize SNR
- same as above?
Tuning of TX voltage levels
- I am not 100% sure that is static (DT) but for the time being it could
be considered as such. It basically trades-off EMI (immunity) for EME
(emissions).
Topology Discovery
- This is a special mode to detect the physical distance among nodes on
the multi-drop cable. It is also being standardized in the OPEN
Alliance, but for the time being, it is proprietary. I think it will
require some kernel support as a protocol is also involved (but not
standardized, yet).
Multi-putrpose I/Os (LED, GPIO, SFD detect).
- I know the kernel already has the infrastructure for those functions
(not sure about SFD) so I assume this could be some DT work and some
code to configure the MUX to achieve the specific function.
Selection of link status triggers
- This is what I was trying to achieve with the module parameter. i.e.,
the link status can be a simple on/off based on the link_control
setting (this is what it is for CSMA/CD as there is no link concept)
or it could be masked by PLCA status whrn PLCA is enabled. This is a
design choice of the user. In the former case, you don't get a link
down if the PHY automatically go back to CSMA/CD as a result of PLCA
status being 0. In the latter case you get a link down until PLCA is
up & running, preventing the application to send data before time.
Thanks,
Piergiorgio
What might also play a role here is the application these PHYs are
used in. Automotive. So the devices are generally 'appliances'. They
have one purpose, rather than being general purpose. So i expect the
network is very static.
DT is supposed to describe the hardware, not really configuration of
the hardware. But a lot of this is very hardware near so should be
O.K. in DT.
> Enable of enhanced-noise-immunity mode
> - This trades off CSMA/CD performance for noise immunity. It could be a
> static setting, but the user may want to conditionally enable it
> depending on application decisions. E.g. some people may want to
> enable/disable this when using CSMA/CD as a fallback in case the PLCA
> coordinator disappears. Of course, there are better ways of doing
> this, but it is a possible use-case that some people want to use.
I would probably leave this one for the moment, until somebody really
needs it. It also sounds generic, not specific to this PHY.
> Tuning of internal impedance to match the line/MDI
> - This is really board dependent, so DT seems good to me
DT sounds good. It also sounds generic, not specific to this PHY.
> Tuning of PMA filters to optimize SNR
> - same as above?
Also sounds generic?
> Tuning of TX voltage levels
> - I am not 100% sure that is static (DT) but for the time being it could
> be considered as such. It basically trades-off EMI (immunity) for EME
> (emissions).
There has been general interest in this for a while, but nobody has
implemented it. Marvell PHYs allow you to change the amplitude for
example, which i've seen used to work around EMC issues. Part of the
problem here is keeping within the standard, and dealing with the
dynamic behaviour of the network. Somebody might reduce the voltage
and it works. And then the cable is swapped, to a longer cable, and it
stops working because the voltage is too low. So there was discussion
about on link down, the setting reverts back to default, to ensure it
works when the link comes back up again. A PHY tunable was suggested
for this. But it needs thinking about. For automotive/appliance
setting, maybe this can be simplified, since you don't expect the
cabling to change.
> Topology Discovery
> - This is a special mode to detect the physical distance among nodes on
> the multi-drop cable. It is also being standardized in the OPEN
> Alliance, but for the time being, it is proprietary. I think it will
> require some kernel support as a protocol is also involved (but not
> standardized, yet).
This might fall under cable test. It can already return cable length,
which normally means length along cable to fault, but it is a bit
ambiguous, so can also mean just length. Adding an extra attribute it
could easily mean length to node. Proprietary vs standard does not
matter, since cable testing in general is proprietary. You just need
to see how you can plug it into the current API.
> Multi-putrpose I/Os (LED, GPIO, SFD detect).
> - I know the kernel already has the infrastructure for those functions
> (not sure about SFD) so I assume this could be some DT work and some
> code to configure the MUX to achieve the specific function.
LEDs are a long long story, which has not yet reached its
conclusion. But they will end up as being Linux LEDs, which can be
accelerated using hardware. GPIOs are just linux GPIOs. You might have
some DT to indicate how the external pin is wired, pinmux. But pinmux
is also standardized within Linux. I should point out that many PHY
silicons support GPIOs, but nobody has ever used the capability in
Mainline Linux.
> Selection of link status triggers
> - This is what I was trying to achieve with the module parameter. i.e.,
> the link status can be a simple on/off based on the link_control
> setting (this is what it is for CSMA/CD as there is no link concept)
> or it could be masked by PLCA status whrn PLCA is enabled. This is a
> design choice of the user. In the former case, you don't get a link
> down if the PHY automatically go back to CSMA/CD as a result of PLCA
> status being 0. In the latter case you get a link down until PLCA is
> up & running, preventing the application to send data before time.
This is an interesting one. I don't know of any other link technology
which operates like this. The link is either up, or down. There is
support for downshift, when a T4 phy discovers a broken pair and swaps
from 1G to 100Mbps using two of the working pairs. You get down/up
events when this happens, and you can see the actual speed as opposed
to the expected speed.
I'm not sure i would actually make this configurable. Some
applications might be happy with best effort, will others want to wait
for PLCA to be active. There is already a link state netlink message
which gets broadcast to userspace when ever some property of the link
changes. I would add PLCA status to it. Applications then just need to
listen for the message.
Also, you need real use cases. Linux is not a collection of SDKs
bolted together. Vendor crap SDKs tend to have an API for everything
the hardware can do, and 90% of it is never used. That just adds
maintenance burden. We push back on this in mainline. We only add
stuff when there is somebody who needs it. So having the vendor
provide the core driver is great, but it is better that a user of the
device added the needed bells and whistles, for their real use case.
Andrew