[PATCH v2] drivers/net/phy: add driver for Microchip LAN867x 10BASE-T1S PHY

[PATCH v2] drivers/net/phy: add driver for Microchip LAN867x 10BASE-T1S PHY

[Ramón Nordin Rodriguez]

This patch adds support for the Microchip LAN867x 10BASE-T1S family
(LAN8670/1/2). The driver supports P2MP with PLCA.

Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
---
 drivers/net/phy/Kconfig         |   5 ++
 drivers/net/phy/Makefile        |   1 +
 drivers/net/phy/microchip_t1s.c | 136 ++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+)
 create mode 100644 drivers/net/phy/microchip_t1s.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 54874555c921..c12e30f83b4f 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -235,6 +235,11 @@ config MICREL_PHY
 	help
 	  Supports the KSZ9021, VSC8201, KS8001 PHYs.
 
+config MICROCHIP_T1S_PHY
+	tristate "Microchip 10BASE-T1S Ethernet PHY"
+	help
+		Currently supports the LAN8670, LAN8671, LAN8672
+
 config MICROCHIP_PHY
 	tristate "Microchip PHYs"
 	help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index b5138066ba04..64f649f2f62f 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -74,6 +74,7 @@ obj-$(CONFIG_MICREL_KS8995MA)	+= spi_ks8995.o
 obj-$(CONFIG_MICREL_PHY)	+= micrel.o
 obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
 obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.o
+obj-$(CONFIG_MICROCHIP_T1S_PHY) += microchip_t1s.o
 obj-$(CONFIG_MICROSEMI_PHY)	+= mscc/
 obj-$(CONFIG_MOTORCOMM_PHY)	+= motorcomm.o
 obj-$(CONFIG_NATIONAL_PHY)	+= national.o
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
new file mode 100644
index 000000000000..edb50ce63c63
--- /dev/null
+++ b/drivers/net/phy/microchip_t1s.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for Microchip 10BASE-T1S LAN867X PHY
+ *
+ * Support: Microchip Phys:
+ *  lan8670, lan8671, lan8672
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+
+#define PHY_ID_LAN867X 0x0007C160
+
+#define LAN867X_REG_IRQ_1_CTL 0x001C
+#define LAN867X_REG_IRQ_2_CTL 0x001D
+
+static int lan867x_config_init(struct phy_device *phydev)
+{
+	/* HW quirk: Microchip states in the application note (AN1699) for the phy
+	 * that a set of read-modify-write (rmw) operations has to be performed
+	 * on a set of seemingly magic registers.
+	 * The result of these operations is just described as 'optimal performance'
+	 * Microchip gives no explanation as to what these mmd regs do,
+	 * in fact they are marked as reserved in the datasheet.
+	 */
+
+	/* The arrays below are pulled from the following table from AN1699
+	 * Access MMD Address Value Mask
+	 * RMW 0x1F 0x00D0 0x0002 0x0E03
+	 * RMW 0x1F 0x00D1 0x0000 0x0300
+	 * RMW 0x1F 0x0084 0x3380 0xFFC0
+	 * RMW 0x1F 0x0085 0x0006 0x000F
+	 * RMW 0x1F 0x008A 0xC000 0xF800
+	 * RMW 0x1F 0x0087 0x801C 0x801C
+	 * RMW 0x1F 0x0088 0x033F 0x1FFF
+	 * W   0x1F 0x008B 0x0404 ------
+	 * RMW 0x1F 0x0080 0x0600 0x0600
+	 * RMW 0x1F 0x00F1 0x2400 0x7F00
+	 * RMW 0x1F 0x0096 0x2000 0x2000
+	 * W   0x1F 0x0099 0x7F80 ------
+	 */
+
+	const int registers[12] = {
+		0x00D0, 0x00D1, 0x0084, 0x0085,
+		0x008A, 0x0087, 0x0088, 0x008B,
+		0x0080, 0x00F1, 0x0096, 0x0099,
+	};
+
+	const int values[12] = {
+		0x0002, 0x0000, 0x3380, 0x0006,
+		0xC000, 0x801C, 0x033F, 0x0404,
+		0x0600, 0x2400, 0x2000, 0x7F80,
+	};
+
+	const int masks[12] = {
+		0x0E03, 0x0300, 0xFFC0, 0x000F,
+		0xF800, 0x801C, 0x1FFF, 0xFFFF,
+		0x0600, 0x7F00, 0x2000, 0xFFFF,
+	};
+
+	int reg_value;
+	int err;
+	int reg;
+
+	/* Read-Modified Write Pseudocode (from AN1699)
+	 * current_val = read_register(mmd, addr) // Read current register value
+	 * new_val = current_val AND (NOT mask) // Clear bit fields to be written
+	 * new_val = new_val OR value // Set bits
+	 * write_register(mmd, addr, new_val) // Write back updated register value
+	 */
+	for (int i = 0; i < ARRAY_SIZE(registers); i++) {
+		reg = registers[i];
+		reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg);
+		reg_value &= ~masks[i];
+		reg_value |= values[i];
+		err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value);
+		if (err != 0)
+			return err;
+	}
+
+	/* None of the interrupts in the lan867x phy seem relevant.
+	 * Other phys inspect the link status and call phy_trigger_machine
+	 * in the interrupt handler.
+	 * This phy does not support link status, and thus has no interrupt
+	 * for it either.
+	 * So we'll just disable all interrupts on the chip.
+	 */
+	err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_1_CTL, 0xFFFF);
+	if (err != 0)
+		return err;
+	err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_2_CTL, 0xFFFF);
+
+	return err;
+}
+
+static int lan867x_read_status(struct phy_device *phydev)
+{
+	/* The phy has some limitations, namely:
+	 *  - always reports link up
+	 *  - only supports 10MBit half duplex
+	 *  - does not support auto negotiate
+	 */
+	phydev->link = 1;
+	phydev->duplex = DUPLEX_HALF;
+	phydev->speed = SPEED_10;
+	phydev->autoneg = AUTONEG_DISABLE;
+
+	return 0;
+}
+
+static struct phy_driver lan867x_driver[] = {
+	{
+		PHY_ID_MATCH_MODEL(PHY_ID_LAN867X),
+		.name               = "LAN867X",
+		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
+		.config_init        = lan867x_config_init,
+		.read_status        = lan867x_read_status,
+		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
+		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
+		.get_plca_status    = genphy_c45_plca_get_status,
+	}
+};
+
+module_phy_driver(lan867x_driver);
+
+static struct mdio_device_id __maybe_unused tbl[] = {
+	{ PHY_ID_MATCH_MODEL(PHY_ID_LAN867X) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, tbl);
+
+MODULE_DESCRIPTION("Microchip 10BASE-T1S lan867x Phy driver");
+MODULE_AUTHOR("Ramón Nordin Rodriguez");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH v2] drivers/net/phy: add driver for Microchip LAN867x 10BASE-T1S PHY

[Andrew Lunn]

On Wed, Apr 19, 2023 at 01:16:39PM +0200, Ramón Nordin Rodriguez wrote:
> This patch adds support for the Microchip LAN867x 10BASE-T1S family
> (LAN8670/1/2). The driver supports P2MP with PLCA.

It is normal to list here what has changed since the last
version. That gives reviewers an idea what comments have been address,
and if any have been missed.

> Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2] drivers/net/phy: add driver for Microchip LAN867x 10BASE-T1S PHY

[Vladimir Oltean]

On Wed, Apr 19, 2023 at 01:16:39PM +0200, Ramón Nordin Rodriguez wrote:
> This patch adds support for the Microchip LAN867x 10BASE-T1S family
> (LAN8670/1/2). The driver supports P2MP with PLCA.
> 
> Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
> ---
>  drivers/net/phy/Kconfig         |   5 ++
>  drivers/net/phy/Makefile        |   1 +
>  drivers/net/phy/microchip_t1s.c | 136 ++++++++++++++++++++++++++++++++
>  3 files changed, 142 insertions(+)
>  create mode 100644 drivers/net/phy/microchip_t1s.c
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 54874555c921..c12e30f83b4f 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -235,6 +235,11 @@ config MICREL_PHY
>  	help
>  	  Supports the KSZ9021, VSC8201, KS8001 PHYs.
>  
> +config MICROCHIP_T1S_PHY
> +	tristate "Microchip 10BASE-T1S Ethernet PHY"
> +	help
> +		Currently supports the LAN8670, LAN8671, LAN8672

I believe there is an explicit convention documented somewhere (can't
remember where, sorry) that help texts are indented with 2 spaces from
the "help" word. Just like above, basically.

> +
>  config MICROCHIP_PHY
>  	tristate "Microchip PHYs"
>  	help
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index b5138066ba04..64f649f2f62f 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -74,6 +74,7 @@ obj-$(CONFIG_MICREL_KS8995MA)	+= spi_ks8995.o
>  obj-$(CONFIG_MICREL_PHY)	+= micrel.o
>  obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
>  obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.o
> +obj-$(CONFIG_MICROCHIP_T1S_PHY) += microchip_t1s.o
>  obj-$(CONFIG_MICROSEMI_PHY)	+= mscc/
>  obj-$(CONFIG_MOTORCOMM_PHY)	+= motorcomm.o
>  obj-$(CONFIG_NATIONAL_PHY)	+= national.o
> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> new file mode 100644
> index 000000000000..edb50ce63c63
> --- /dev/null
> +++ b/drivers/net/phy/microchip_t1s.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Microchip 10BASE-T1S LAN867X PHY
> + *
> + * Support: Microchip Phys:

Supports Microchip PHYs:

> + *  lan8670, lan8671, lan8672
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +
> +#define PHY_ID_LAN867X 0x0007C160
> +
> +#define LAN867X_REG_IRQ_1_CTL 0x001C
> +#define LAN867X_REG_IRQ_2_CTL 0x001D
> +
> +static int lan867x_config_init(struct phy_device *phydev)
> +{
> +	/* HW quirk: Microchip states in the application note (AN1699) for the phy
> +	 * that a set of read-modify-write (rmw) operations has to be performed
> +	 * on a set of seemingly magic registers.
> +	 * The result of these operations is just described as 'optimal performance'
> +	 * Microchip gives no explanation as to what these mmd regs do,
> +	 * in fact they are marked as reserved in the datasheet.
> +	 */
> +
> +	/* The arrays below are pulled from the following table from AN1699
> +	 * Access MMD Address Value Mask
> +	 * RMW 0x1F 0x00D0 0x0002 0x0E03
> +	 * RMW 0x1F 0x00D1 0x0000 0x0300
> +	 * RMW 0x1F 0x0084 0x3380 0xFFC0
> +	 * RMW 0x1F 0x0085 0x0006 0x000F
> +	 * RMW 0x1F 0x008A 0xC000 0xF800
> +	 * RMW 0x1F 0x0087 0x801C 0x801C
> +	 * RMW 0x1F 0x0088 0x033F 0x1FFF
> +	 * W   0x1F 0x008B 0x0404 ------
> +	 * RMW 0x1F 0x0080 0x0600 0x0600
> +	 * RMW 0x1F 0x00F1 0x2400 0x7F00
> +	 * RMW 0x1F 0x0096 0x2000 0x2000
> +	 * W   0x1F 0x0099 0x7F80 ------
> +	 */
> +
> +	const int registers[12] = {
> +		0x00D0, 0x00D1, 0x0084, 0x0085,
> +		0x008A, 0x0087, 0x0088, 0x008B,
> +		0x0080, 0x00F1, 0x0096, 0x0099,
> +	};
> +
> +	const int values[12] = {
> +		0x0002, 0x0000, 0x3380, 0x0006,
> +		0xC000, 0x801C, 0x033F, 0x0404,
> +		0x0600, 0x2400, 0x2000, 0x7F80,
> +	};
> +
> +	const int masks[12] = {
> +		0x0E03, 0x0300, 0xFFC0, 0x000F,
> +		0xF800, 0x801C, 0x1FFF, 0xFFFF,
> +		0x0600, 0x7F00, 0x2000, 0xFFFF,
> +	};

Kernel stack space eventually runs out, and defining large-ish arrays as
local variables is always a problem. These don't appear to be large
enough to cause problems, but it is a good practice to make constants go
to the .rodata section, and that would be achieved by moving them to the
global scope of this file (don't forget to make them static) and to give
them more specific names (lan867x_fixup_regs, lan867x_fixup_values,
lan867x_fixup_masks). Alternatively, if you insist on making them
visible just to the scope of lan867x_config_init(), there is a way of
doing that by making the local variables "static". That is essentially
just syntactic sugar (they aren't put on stack) and is used more rarely.

> +
> +	int reg_value;
> +	int err;
> +	int reg;
> +
> +	/* Read-Modified Write Pseudocode (from AN1699)
> +	 * current_val = read_register(mmd, addr) // Read current register value
> +	 * new_val = current_val AND (NOT mask) // Clear bit fields to be written
> +	 * new_val = new_val OR value // Set bits
> +	 * write_register(mmd, addr, new_val) // Write back updated register value
> +	 */
> +	for (int i = 0; i < ARRAY_SIZE(registers); i++) {
> +		reg = registers[i];
> +		reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg);
> +		reg_value &= ~masks[i];
> +		reg_value |= values[i];
> +		err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value);
> +		if (err != 0)
> +			return err;
> +	}
> +
> +	/* None of the interrupts in the lan867x phy seem relevant.
> +	 * Other phys inspect the link status and call phy_trigger_machine
> +	 * in the interrupt handler.
> +	 * This phy does not support link status, and thus has no interrupt
> +	 * for it either.
> +	 * So we'll just disable all interrupts on the chip.
> +	 */
> +	err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_1_CTL, 0xFFFF);
> +	if (err != 0)
> +		return err;
> +	err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_2_CTL, 0xFFFF);
> +
> +	return err;

return phy_write_mmd()?

> +}
> +
> +static int lan867x_read_status(struct phy_device *phydev)
> +{
> +	/* The phy has some limitations, namely:
> +	 *  - always reports link up
> +	 *  - only supports 10MBit half duplex
> +	 *  - does not support auto negotiate
> +	 */
> +	phydev->link = 1;
> +	phydev->duplex = DUPLEX_HALF;
> +	phydev->speed = SPEED_10;
> +	phydev->autoneg = AUTONEG_DISABLE;

Sounds really suboptimal if phylib has to poll once per second to find
out static information. Does the PHY have an architectural limitation in
that it does not report link status? Is it a T1S thing? Something should
change here, but I'm not really sure what. A PHY that doesn't report
link status seems... strange?

> +
> +	return 0;
> +}
> +
> +static struct phy_driver lan867x_driver[] = {
> +	{
> +		PHY_ID_MATCH_MODEL(PHY_ID_LAN867X),
> +		.name               = "LAN867X",
> +		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
> +		.config_init        = lan867x_config_init,
> +		.read_status        = lan867x_read_status,
> +		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
> +		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
> +		.get_plca_status    = genphy_c45_plca_get_status,
> +	}
> +};
> +
> +module_phy_driver(lan867x_driver);
> +
> +static struct mdio_device_id __maybe_unused tbl[] = {
> +	{ PHY_ID_MATCH_MODEL(PHY_ID_LAN867X) },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(mdio, tbl);
> +
> +MODULE_DESCRIPTION("Microchip 10BASE-T1S lan867x Phy driver");
> +MODULE_AUTHOR("Ramón Nordin Rodriguez");
> +MODULE_LICENSE("GPL");
> -- 
> 2.39.2
> 

Re: [PATCH v2] drivers/net/phy: add driver for Microchip LAN867x 10BASE-T1S PHY

[Andrew Lunn]

> > +static int lan867x_read_status(struct phy_device *phydev)
> > +{
> > +	/* The phy has some limitations, namely:
> > +	 *  - always reports link up
> > +	 *  - only supports 10MBit half duplex
> > +	 *  - does not support auto negotiate
> > +	 */
> > +	phydev->link = 1;
> > +	phydev->duplex = DUPLEX_HALF;
> > +	phydev->speed = SPEED_10;
> > +	phydev->autoneg = AUTONEG_DISABLE;
> 
> Sounds really suboptimal if phylib has to poll once per second to find
> out static information. Does the PHY have an architectural limitation in
> that it does not report link status? Is it a T1S thing? Something should
> change here, but I'm not really sure what. A PHY that doesn't report
> link status seems... strange?

Hi Vladimir

I thought that as well. I skimmed the data sheet, and i don't see
anything. You can maybe imply there is no link by looking for PLCA
beacons, but i don't know how reliable that is. You are not forced to
use PLCA, so there might not be any, but communication is still
possible.

The other T1S PHY, NCN26000, does have link status. So i don't think
it is a T1S thing.

Also, the typical use case of T1S is automotive. There are no RJ45
plug/sockets to be unplugged. It is an industrial connector. And i
expect such applications don't really care about L2 connectivity.  L7
either works, or it does not.

My thinking was, the overhead of a pointless poll once per second is
minimal, not enough to justify a change to the core.

	 Andrew

Re: [PATCH v2] drivers/net/phy: add driver for Microchip LAN867x 10BASE-T1S PHY

[Ramón Nordin Rodriguez]

On Wed, Apr 19, 2023 at 04:39:13PM +0200, Andrew Lunn wrote:
> On Wed, Apr 19, 2023 at 01:16:39PM +0200, Ramón Nordin Rodriguez wrote:
> > This patch adds support for the Microchip LAN867x 10BASE-T1S family
> > (LAN8670/1/2). The driver supports P2MP with PLCA.
> 
> It is normal to list here what has changed since the last
> version. That gives reviewers an idea what comments have been address,
> and if any have been missed.
> 

Sorry for missing the obvious, is it acceptable to ammend a list of
changes changes in this reply, or would you prefer a V3 submission?

Changes:
    v2:
- Removed mentioning of not supporting auto-negotiation from commit
  message
- Renamed file drivers/net/phy/lan867x.c ->
  drivers/net/phy/microchip_t1s.c
- Renamed Kconfig option to reflect implementation filename (from
  LAN867X_PHY to MICROCHIP_T1S_PHY)
- Moved entry in drivers/net/phy/KConfig to correct sort order
- Moved entry in drivers/net/phy/Makefile to correct sort order
- Moved variable declarations to conform to reverse christmas tree order
  (in func lan867x_config_init)
- Moved register write to disable chip interrupts to func lan867x_config_init, when omitting the irq disable all togheter I got null pointer dereference, see the call trace below:

    Call Trace:
     <TASK>
     phy_interrupt+0xa8/0xf0 [libphy]
     irq_thread_fn+0x1c/0x60
     irq_thread+0xf7/0x1c0
     ? __pfx_irq_thread_dtor+0x10/0x10
     ? __pfx_irq_thread+0x10/0x10
     kthread+0xe6/0x110
     ? __pfx_kthread+0x10/0x10
     ret_from_fork+0x29/0x50
     </TASK>

- Removed func lan867x_config_interrupt and removed the member
  .config_intr from the phy_driver struct

Testing:
This has been tested with ethtool --set/get-plca-cfg and verified on an
oscilloscope where it was observed that:
- The PLCA beacon was enabled/disabled when setting the node-id to 0/not
  0
- The PLCA beacon is transmitted with the expected frequency when
  changing max nodes
- Two devices using the evaluation board EVB-LAN8670-USB could ping each
  other

Request for comments:
Now that the module has changed name, maybe the funcs lan867x_config_init
and lan867x_read_status should change name to
microchip_t1s_config_init/read_status?


> > Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
>     Andrew