| Message ID | ZD7YzBhzlEBHrEPC@builder (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | drivers/net/phy: add driver for Microchip LAN867x 10BASE-T1S PHY | expand |
> +config LAN867X_PHY > + tristate "Microchip 10BASE-T1S Ethernet PHY" > + help > + Currently supports the LAN8670, LAN8671, LAN8672 > + This file is sorted by tristate string, so it should come before tristate "Microchip PHYs" > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index b5138066ba04..a12c2f296297 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -78,6 +78,7 @@ 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_LAN867X_PHY) += lan867x.o And this is sorted by CONFIG_ so should appear after CONFIG_INTEL_XWAY_PHY. > 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/lan867x.c b/drivers/net/phy/lan867x.c Maybe call it microchip_t1s.c ? That sort of fits with the pattern of the current files: microchip.c microchip_t1.c Microchip drivers don't really have a consistent naming, because they keep buying other vendors, like vitesse, Microsemi, Micrel/Kendin... > +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 masks[12] = { > + 0x0E03, 0x0300, 0xFFC0, 0x000F, > + 0xF800, 0x801C, 0x1FFF, 0xFFFF, > + 0x0600, 0x7F00, 0x2000, 0xFFFF, > + }; > + > + const int values[12] = { > + 0x0002, 0x0000, 0x3380, 0x0006, > + 0xC000, 0x801C, 0x033F, 0x0404, > + 0x0600, 0x2400, 0x2000, 0x7F80, > + }; > + > + int err; > + int reg; > + int reg_value; netdev uses reverse christmas tree. That is, variables should be sorted with the longest lines first, shorted last. > + /* 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; > + } Maybe phy_modify_mmd(). However, that skips the write if the value is not changed by the mask and set value. > +static int lan867x_config_interrupt(struct phy_device *phydev) > +{ > + /* None of the interrupts in the lan867x phy seem relevant. > + * Other phys inspect the link status and call phy_trigger_machine > + * on change. > + * This phy does not support link status, and thus has no interrupt > + * for it either. > + * So we'll just disable all interrupts instead. > + */ It interrupts are pointless, just don't provide the functions. phylib will then poll. > +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; Not that polling gives anything useful! Andrew
On Tue, Apr 18, 2023 at 08:56:47PM +0200, Andrew Lunn wrote: > > +config LAN867X_PHY > > + tristate "Microchip 10BASE-T1S Ethernet PHY" > > + help > > + Currently supports the LAN8670, LAN8671, LAN8672 > > + > > This file is sorted by tristate string, so it should come before > tristate "Microchip PHYs" > Ah, I missed that, fixing it in the next patch version. > > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > > index b5138066ba04..a12c2f296297 100644 > > --- a/drivers/net/phy/Makefile > > +++ b/drivers/net/phy/Makefile > > @@ -78,6 +78,7 @@ 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_LAN867X_PHY) += lan867x.o > > And this is sorted by CONFIG_ so should appear after > CONFIG_INTEL_XWAY_PHY. > Same thing here, will fix it. > > 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/lan867x.c b/drivers/net/phy/lan867x.c > > Maybe call it microchip_t1s.c ? That sort of fits with the pattern of > the current files: > > microchip.c > microchip_t1.c > > Microchip drivers don't really have a consistent naming, because they > keep buying other vendors, like vitesse, Microsemi, Micrel/Kendin... > Totally agree with the name suggestion. > > +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 masks[12] = { > > + 0x0E03, 0x0300, 0xFFC0, 0x000F, > > + 0xF800, 0x801C, 0x1FFF, 0xFFFF, > > + 0x0600, 0x7F00, 0x2000, 0xFFFF, > > + }; > > + > > + const int values[12] = { > > + 0x0002, 0x0000, 0x3380, 0x0006, > > + 0xC000, 0x801C, 0x033F, 0x0404, > > + 0x0600, 0x2400, 0x2000, 0x7F80, > > + }; > > + > > + int err; > > + int reg; > > + int reg_value; > > netdev uses reverse christmas tree. That is, variables should be > sorted with the longest lines first, shorted last. > Missed that in the style guide, will fix it. > > + /* 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; > > + } > > Maybe phy_modify_mmd(). However, that skips the write if the value is > not changed by the mask and set value. > I've reached out to Microchip regarding this and asked them if a write is required even if the resulting value is not modified. I suggest we leave this as is, and if they respond that it's not required to write on unmodified I will submit a patch that removes this block and uses phy_modify_mmd instead. In breif I don't feel confident that I can verify that I've achieved 'optimal perfomance' as they refer to it as and would like to get feedback from the manufacturer. > > +static int lan867x_config_interrupt(struct phy_device *phydev) > > +{ > > + /* None of the interrupts in the lan867x phy seem relevant. > > + * Other phys inspect the link status and call phy_trigger_machine > > + * on change. > > + * This phy does not support link status, and thus has no interrupt > > + * for it either. > > + * So we'll just disable all interrupts instead. > > + */ > > It interrupts are pointless, just don't provide the functions. phylib > will then poll. > Gotcha, I'll remove the func. > > +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; > > Not that polling gives anything useful! > > Andrew :) Great feedback! Trying to get a new patch out today
Hi Ramon, Good day...! This is Parthiban from Microchip. Thanks for your patches for the Microchip LAN867x 10BASE-T1S PHY. We really appreciate your effort on this. For your kind information, we are already working for the driver which supports all the 10BASE-T1S PHYs from Microchip and doing internal review of those driver patches to mainline. These patches are going to reach mainline in couple of days. It is very unfortunate that we two are working on the same task at the same time without knowing each other. The architecture of your patch is similar to our current implementation. However to be able to support also the upcoming 10BASE-T1S products e.g., the LAN865x 10BASE-T1S MAC-PHY, additional functionalities have to be implemented. In order to avoid unnecessary/redundant work on both sides, we would like to collaborate with you on this topic and have a sync outside of this mailing list before going forward. Best Regards, Parthiban V On 18/04/23 11:22 pm, Ramón Nordin Rodriguez wrote: > [Some people who received this message don't often get email from ramon.nordin.rodriguez@ferroamp.se. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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/lan867x.c | 144 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 150 insertions(+) > create mode 100644 drivers/net/phy/lan867x.c > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index 54874555c921..63ba7f51087e 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -284,6 +284,11 @@ config NCN26000_PHY > Currently supports the NCN26000 10BASE-T1S Industrial PHY > with MII interface. > > +config LAN867X_PHY > + tristate "Microchip 10BASE-T1S Ethernet PHY" > + help > + Currently supports the LAN8670, LAN8671, LAN8672 > + > config AT803X_PHY > tristate "Qualcomm Atheros AR803X PHYs and QCA833x PHYs" > depends on REGULATOR > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index b5138066ba04..a12c2f296297 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -78,6 +78,7 @@ 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_LAN867X_PHY) += lan867x.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/lan867x.c b/drivers/net/phy/lan867x.c > new file mode 100644 > index 000000000000..c861c46ed06b > --- /dev/null > +++ b/drivers/net/phy/lan867x.c > @@ -0,0 +1,144 @@ > +// 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 masks[12] = { > + 0x0E03, 0x0300, 0xFFC0, 0x000F, > + 0xF800, 0x801C, 0x1FFF, 0xFFFF, > + 0x0600, 0x7F00, 0x2000, 0xFFFF, > + }; > + > + const int values[12] = { > + 0x0002, 0x0000, 0x3380, 0x0006, > + 0xC000, 0x801C, 0x033F, 0x0404, > + 0x0600, 0x2400, 0x2000, 0x7F80, > + }; > + > + int err; > + int reg; > + int reg_value; > + > + /* 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; > + } > + > + return 0; > +} > + > +static int lan867x_config_interrupt(struct phy_device *phydev) > +{ > + /* None of the interrupts in the lan867x phy seem relevant. > + * Other phys inspect the link status and call phy_trigger_machine > + * on change. > + * This phy does not support link status, and thus has no interrupt > + * for it either. > + * So we'll just disable all interrupts instead. > + */ > + > + int err; > + > + err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_1_CTL, 0xFFFF); > + if (err) > + 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, > + .config_intr = lan867x_config_interrupt, > + .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 >
On Wed, Apr 19, 2023 at 02:40:29PM +0000, Parthiban.Veerasooran@microchip.com wrote: > Hi Ramon, > > Good day...! This is Parthiban from Microchip. > > Thanks for your patches for the Microchip LAN867x 10BASE-T1S PHY. We > really appreciate your effort on this. > > For your kind information, we are already working for the driver which > supports all the 10BASE-T1S PHYs from Microchip and doing internal > review of those driver patches to mainline. These patches are going to > reach mainline in couple of days. It is very unfortunate that we two are > working on the same task at the same time without knowing each other. > > The architecture of your patch is similar to our current implementation. > However to be able to support also the upcoming 10BASE-T1S products > e.g., the LAN865x 10BASE-T1S MAC-PHY, additional functionalities have to > be implemented. In order to avoid unnecessary/redundant work on both > sides, we would like to collaborate with you on this topic and have a > sync outside of this mailing list before going forward. Hi Parthiban Please review version 2 of the patch which was posted today. Is there anything in that patch which is actually wrong? I don't like the idea of dropping a patch, because a vendor comes out of, maybe unintentional, stealth mode, and asks for their version to be used, not somebody else's. For me this is especially important for a new contributor. My preferred way forward is to merge Ramon's code, and then you can build on it with additional features to support other family members. Please don't get me wrong, i find it great you are supporting your own devices. Not many vendors do. But Linux is a community, we have to respect each others work, other members of the community. Andrew FYI: Do you have any other drivers in the pipeline you want to announce, just to avoid this happening again.
On 19/04/23 8:40 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, Apr 19, 2023 at 02:40:29PM +0000, Parthiban.Veerasooran@microchip.com wrote: >> Hi Ramon, >> >> Good day...! This is Parthiban from Microchip. >> >> Thanks for your patches for the Microchip LAN867x 10BASE-T1S PHY. We >> really appreciate your effort on this. >> >> For your kind information, we are already working for the driver which >> supports all the 10BASE-T1S PHYs from Microchip and doing internal >> review of those driver patches to mainline. These patches are going to >> reach mainline in couple of days. It is very unfortunate that we two are >> working on the same task at the same time without knowing each other. >> >> The architecture of your patch is similar to our current implementation. >> However to be able to support also the upcoming 10BASE-T1S products >> e.g., the LAN865x 10BASE-T1S MAC-PHY, additional functionalities have to >> be implemented. In order to avoid unnecessary/redundant work on both >> sides, we would like to collaborate with you on this topic and have a >> sync outside of this mailing list before going forward. > > Hi Parthiban > > Please review version 2 of the patch which was posted today. Is there > anything in that patch which is actually wrong? > > I don't like the idea of dropping a patch, because a vendor comes out > of, maybe unintentional, stealth mode, and asks for their version to > be used, not somebody else's. For me this is especially important for > a new contributor. > > My preferred way forward is to merge Ramon's code, and then you can > build on it with additional features to support other family > members. > > Please don't get me wrong, i find it great you are supporting your own > devices. Not many vendors do. But Linux is a community, we have to > respect each others work, other members of the community. > > Andrew > > FYI: Do you have any other drivers in the pipeline you want to > announce, just to avoid this happening again. Hi Andrew, Thanks a lot for your reply and clarification. Sure I will also participate in reviewing the patches including v2. I fully agree with you and also I really appreciate Ramon's effort on this which shows how much interest he has on our product and driver development. 1. Add support for LAN8650/1 10BASE-T1S MAC-PHY's PHY in the microchip_t1s.c driver which is being mainlined by Ramon. 2. Add generic driver support for the OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface. 3. Add driver support for LAN8650/1 10BASE-T1S MAC-PHY. Note: 2nd and 3rd will be in a single patch series. Above product link: https://www.microchip.com/en-us/product/lan8650 As I communicated before in the below email, we have the above drivers in the pipeline to mainline. Hi Andrew, Thanks a lot for your support. I will check with our colleagues suggested by you and get back to mainline again. Best Regards, Parthiban V On 11/03/23 11:15 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Allan > > It has been a long time since we talked, maybe 2019 at the Linux > Plumbers conference.... And then PTP discussions etc. > > It seems like Sparx5 is going well, along with felix, seville, etc. > > On Fri, Mar 10, 2023 at 11:13:23AM +0000, Parthiban.Veerasooran@microchip.com wrote: >> Hi All, >> >> I would like to add Microchip's LAN865x 10BASE-T1S MAC-PHY driver >> support to Linux kernel. >> (Product link: https://www.microchip.com/en-us/product/LAN8650) >> >> The LAN8650 combines a Media Access Controller (MAC) and an Ethernet PHY >> to access 10BASE‑T1S networks. The common standard Serial Peripheral >> Interface (SPI) is used so that the transfer of Ethernet packets and >> LAN8650 control/status commands are performed over a single, serial >> interface. >> >> Ethernet packets are segmented and transferred over the serial interface >> according to the OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface >> specification designed by TC6. >> (link: https://www.opensig.org/Automotive-Ethernet-Specifications/) >> The serial interface protocol can simultaneously transfer both transmit >> and receive packets between the host and the LAN8650. >> >> Basically the driver comprises of two parts. One part is to interface >> with networking subsystem and SPI subsystem. The other part is a TC6 >> state machine which implements the Ethernet packets segmentation >> according to OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface >> specification. >> >> The idea behind the TC6 state machine implementation is to make it as a >> generic library and platform independent. A set of API's provided by >> this TC6 state machine library can be used by the 10BASE-T1x MAC-PHY >> drivers to segment the Ethernet packets according to the OPEN Alliance >> 10BASE‑T1x MAC‑PHY Serial Interface specification. >> >> With the above information, kindly provide your valuable feedback on my >> below queries. >> >> Can we keep this TC6 state machine within the LAN865x driver or as a >> separate generic library accessible for other 10BASE-T1x MAC-PHY drivers >> as well? >> >> If you recommend to have that as a separate generic library then could >> you please advice on what is the best way to do that in kernel? > > Microchip is getting more and more involved in mainline. Jakub > publishes some developers statistics for netdev: > > https://lwn.net/Articles/918007/ > > It shows Microchip are near the top for code contributions. Which is > great. However, as a reviewer, i see the quality really varies. Given > how active Microchip is within Linux, the netdev community, and to > some extent Linux as a whole, expects a company like Microchip to > build up its internal resources to offer training and Mentoring to > mainline developers, rather than expect the community to do that > work. Does such a thing exist within Microchip? Could you point > Parthiban towards a mentor who can help guide the work adding generic > support for the OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface and > the LAN8650/1 specific bits? If not, could Steen Hegelund or Horatiu > Vultur make some time available to be a mentor? > > Thanks > Andrew
On 20/04/23 11:03 am, Parthiban Veerasooran wrote: > On 19/04/23 8:40 pm, Andrew Lunn wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> On Wed, Apr 19, 2023 at 02:40:29PM +0000, >> Parthiban.Veerasooran@microchip.com wrote: >>> Hi Ramon, >>> >>> Good day...! This is Parthiban from Microchip. >>> >>> Thanks for your patches for the Microchip LAN867x 10BASE-T1S PHY. We >>> really appreciate your effort on this. >>> >>> For your kind information, we are already working for the driver which >>> supports all the 10BASE-T1S PHYs from Microchip and doing internal >>> review of those driver patches to mainline. These patches are going to >>> reach mainline in couple of days. It is very unfortunate that we two are >>> working on the same task at the same time without knowing each other. >>> >>> The architecture of your patch is similar to our current implementation. >>> However to be able to support also the upcoming 10BASE-T1S products >>> e.g., the LAN865x 10BASE-T1S MAC-PHY, additional functionalities have to >>> be implemented. In order to avoid unnecessary/redundant work on both >>> sides, we would like to collaborate with you on this topic and have a >>> sync outside of this mailing list before going forward. >> >> Hi Parthiban >> >> Please review version 2 of the patch which was posted today. Is there >> anything in that patch which is actually wrong? >> >> I don't like the idea of dropping a patch, because a vendor comes out >> of, maybe unintentional, stealth mode, and asks for their version to >> be used, not somebody else's. For me this is especially important for >> a new contributor. >> >> My preferred way forward is to merge Ramon's code, and then you can >> build on it with additional features to support other family >> members. >> >> Please don't get me wrong, i find it great you are supporting your own >> devices. Not many vendors do. But Linux is a community, we have to >> respect each others work, other members of the community. >> >> Andrew >> >> FYI: Do you have any other drivers in the pipeline you want to >> announce, just to avoid this happening again. > > Hi Andrew, > > Thanks a lot for your reply and clarification. > > Sure I will also participate in reviewing the patches including v2. > > I fully agree with you and also I really appreciate Ramon's effort on > this which shows how much interest he has on our product and driver > development. > > 1. Add support for LAN8650/1 10BASE-T1S MAC-PHY's PHY in the > microchip_t1s.c driver which is being mainlined by Ramon. > 2. Add generic driver support for the OPEN Alliance 10BASE-T1x > MAC-PHY Serial Interface. > 3. Add driver support for LAN8650/1 10BASE-T1S MAC-PHY. > > Note: 2nd and 3rd will be in a single patch series. > > Above product link: https://www.microchip.com/en-us/product/lan8650 > > As I communicated before in the below email, we have the above drivers > in the pipeline to mainline. Hi Andrew, Sorry, forgot to add one more task in the above pipeline. Please find the updated list below, 1. Add support for Microchip LAN8650/1 Rev.B0 10BASE-T1S MAC-PHY's PHY in the microchip_t1s.c driver which is being mainlined by Ramon (which will be used by the below LAN8650/1 Rev.B0 10BASE-T1S MAC-PHY driver). 2. Add support for Microchip LAN8670/1/2 Rev.C1 10BASE-T1S PHY in the microchip_t1s.c driver which is being mainlined by Ramon. 3. Add generic driver support for the OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface (which will be used by the below LAN8650/1 10BASE-T1S MAC-PHY driver). 4. Add driver support for LAN8650/1 Rev.B0 10BASE-T1S MAC-PHY. We are already working on the above drivers and going for internal review to get the drivers mainlined in the upstream. Best Regards, Parthiban V > > Hi Andrew, > > Thanks a lot for your support. I will check with our colleagues > suggested by you and get back to mainline again. > > Best Regards, > Parthiban V > On 11/03/23 11:15 pm, Andrew Lunn wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > > > Hi Allan > > > > It has been a long time since we talked, maybe 2019 at the Linux > > Plumbers conference.... And then PTP discussions etc. > > > > It seems like Sparx5 is going well, along with felix, seville, etc. > > > > On Fri, Mar 10, 2023 at 11:13:23AM +0000, > Parthiban.Veerasooran@microchip.com wrote: > >> Hi All, > >> > >> I would like to add Microchip's LAN865x 10BASE-T1S MAC-PHY driver > >> support to Linux kernel. > >> (Product link: https://www.microchip.com/en-us/product/LAN8650) > >> > >> The LAN8650 combines a Media Access Controller (MAC) and an Ethernet > PHY > >> to access 10BASE‑T1S networks. The common standard Serial Peripheral > >> Interface (SPI) is used so that the transfer of Ethernet packets and > >> LAN8650 control/status commands are performed over a single, serial > >> interface. > >> > >> Ethernet packets are segmented and transferred over the serial > interface > >> according to the OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface > >> specification designed by TC6. > >> (link: https://www.opensig.org/Automotive-Ethernet-Specifications/) > >> The serial interface protocol can simultaneously transfer both transmit > >> and receive packets between the host and the LAN8650. > >> > >> Basically the driver comprises of two parts. One part is to interface > >> with networking subsystem and SPI subsystem. The other part is a TC6 > >> state machine which implements the Ethernet packets segmentation > >> according to OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface > >> specification. > >> > >> The idea behind the TC6 state machine implementation is to make it as a > >> generic library and platform independent. A set of API's provided by > >> this TC6 state machine library can be used by the 10BASE-T1x MAC-PHY > >> drivers to segment the Ethernet packets according to the OPEN Alliance > >> 10BASE‑T1x MAC‑PHY Serial Interface specification. > >> > >> With the above information, kindly provide your valuable feedback on my > >> below queries. > >> > >> Can we keep this TC6 state machine within the LAN865x driver or as a > >> separate generic library accessible for other 10BASE-T1x MAC-PHY > drivers > >> as well? > >> > >> If you recommend to have that as a separate generic library then could > >> you please advice on what is the best way to do that in kernel? > > > > Microchip is getting more and more involved in mainline. Jakub > > publishes some developers statistics for netdev: > > > > https://lwn.net/Articles/918007/ > > > > It shows Microchip are near the top for code contributions. Which is > > great. However, as a reviewer, i see the quality really varies. Given > > how active Microchip is within Linux, the netdev community, and to > > some extent Linux as a whole, expects a company like Microchip to > > build up its internal resources to offer training and Mentoring to > > mainline developers, rather than expect the community to do that > > work. Does such a thing exist within Microchip? Could you point > > Parthiban towards a mentor who can help guide the work adding generic > > support for the OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface and > > the LAN8650/1 specific bits? If not, could Steen Hegelund or Horatiu > > Vultur make some time available to be a mentor? > > > > Thanks > > Andrew >
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 54874555c921..63ba7f51087e 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -284,6 +284,11 @@ config NCN26000_PHY Currently supports the NCN26000 10BASE-T1S Industrial PHY with MII interface. +config LAN867X_PHY + tristate "Microchip 10BASE-T1S Ethernet PHY" + help + Currently supports the LAN8670, LAN8671, LAN8672 + config AT803X_PHY tristate "Qualcomm Atheros AR803X PHYs and QCA833x PHYs" depends on REGULATOR diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index b5138066ba04..a12c2f296297 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -78,6 +78,7 @@ 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_LAN867X_PHY) += lan867x.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/lan867x.c b/drivers/net/phy/lan867x.c new file mode 100644 index 000000000000..c861c46ed06b --- /dev/null +++ b/drivers/net/phy/lan867x.c @@ -0,0 +1,144 @@ +// 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 masks[12] = { + 0x0E03, 0x0300, 0xFFC0, 0x000F, + 0xF800, 0x801C, 0x1FFF, 0xFFFF, + 0x0600, 0x7F00, 0x2000, 0xFFFF, + }; + + const int values[12] = { + 0x0002, 0x0000, 0x3380, 0x0006, + 0xC000, 0x801C, 0x033F, 0x0404, + 0x0600, 0x2400, 0x2000, 0x7F80, + }; + + int err; + int reg; + int reg_value; + + /* 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; + } + + return 0; +} + +static int lan867x_config_interrupt(struct phy_device *phydev) +{ + /* None of the interrupts in the lan867x phy seem relevant. + * Other phys inspect the link status and call phy_trigger_machine + * on change. + * This phy does not support link status, and thus has no interrupt + * for it either. + * So we'll just disable all interrupts instead. + */ + + int err; + + err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_1_CTL, 0xFFFF); + if (err) + 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, + .config_intr = lan867x_config_interrupt, + .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");
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/lan867x.c | 144 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+) create mode 100644 drivers/net/phy/lan867x.c