| Message ID | 20230426114655.93672-3-Parthiban.Veerasooran@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add driver support for Microchip LAN865X Rev.B0 Internal PHYs | expand |
On Wed, Apr 26, 2023 at 05:16:55PM +0530, Parthiban Veerasooran wrote: > This patch adds support for the Microchip LAN865x Rev.B0 10BASE-T1S > Internal PHYs (LAN8650/1). The LAN865x combines a Media Access Controller > (MAC) and an internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S > networks. > > Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> > --- > drivers/net/phy/microchip_t1s.c | 200 ++++++++++++++++++++++++++++++-- > 1 file changed, 191 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c > index 793fb0210605..3d8d285b3c34 100644 > --- a/drivers/net/phy/microchip_t1s.c > +++ b/drivers/net/phy/microchip_t1s.c > @@ -4,6 +4,7 @@ > * > * Support: Microchip Phys: > * lan8670/1/2 Rev.B1 > + * lan8650/1 Rev.B0 Internal PHYs > */ > > #include <linux/kernel.h> > @@ -11,9 +12,10 @@ > #include <linux/phy.h> > > #define PHY_ID_LAN867X_REVB1 0x0007C162 > +#define PHY_ID_LAN865X_REVB0 0x0007C1B3 > > -#define LAN867X_REG_IRQ_1_CTL 0x001C > -#define LAN867X_REG_IRQ_2_CTL 0x001D > +#define LAN86XX_REG_IRQ_1_CTL 0x001C > +#define LAN86XX_REG_IRQ_2_CTL 0x001D > > /* The arrays below are pulled from the following table from AN1699 > * Access MMD Address Value Mask > @@ -49,6 +51,174 @@ static const int lan867x_revb1_fixup_masks[12] = { > 0x0600, 0x7F00, 0x2000, 0xFFFF, > }; > > +/* LAN865x Rev.B0 configuration parameters from AN1760 > + */ > +static const int lan865x_revb0_fixup_registers[28] = { > + 0x0091, 0x0081, 0x0043, 0x0044, > + 0x0045, 0x0053, 0x0054, 0x0055, > + 0x0040, 0x0050, 0x00D0, 0x00E9, > + 0x00F5, 0x00F4, 0x00F8, 0x00F9, > + 0x00B0, 0x00B1, 0x00B2, 0x00B3, > + 0x00B4, 0x00B5, 0x00B6, 0x00B7, > + 0x00B8, 0x00B9, 0x00BA, 0x00BB, > +}; > + > +static const int lan865x_revb0_fixup_values[28] = { > + 0x9660, 0x00C0, 0x00FF, 0xFFFF, > + 0x0000, 0x00FF, 0xFFFF, 0x0000, > + 0x0002, 0x0002, 0x5F21, 0x9E50, > + 0x1CF8, 0xC020, 0x9B00, 0x4E53, > + 0x0103, 0x0910, 0x1D26, 0x002A, > + 0x0103, 0x070D, 0x1720, 0x0027, > + 0x0509, 0x0E13, 0x1C25, 0x002B, > +}; > + > +/* indirect read pseudocode from AN1760 > + * write_register(0x4, 0x00D8, addr) > + * write_register(0x4, 0x00DA, 0x2) > + * return (int8)(read_register(0x4, 0x00D9)) > + */ I suggest this comment block is slightly changed to /* Pulled from AN1760 describing 'indirect read' * * write_register(0x4, 0x00D8, addr) * write_register(0x4, 0x00DA, 0x2) * return (int8)(read_register(0x4, 0x00D9)) * * 0x4 refers to memory map selector 4, which maps to MDIO_MMD_VEND2 */ > +static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr) > +{ > + int ret; > + > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xD8, addr); > + if (ret) > + return ret; > + > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xDA, 0x0002); > + if (ret) > + return ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0xD9); > + if (ret < 0) > + return ret; > + > + return ret; > +} The func itself might get a bit more readable by naming the magic regs and value, below is a suggestion. static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr) { int ret; static const u16 fixup_w0_reg = 0x00D8; static const u16 fixup_r0_reg = 0x00D9; static const u16 fixup_w1_val = 0x0002; static const u16 fixup_w1_reg = 0x00DA; ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, fixup_w0_reg, addr); if (ret) return ret; ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, fixup_w1_reg, fixup_w1_val); if (ret) return ret; return phy_read_mmd(phydev, MDIO_MMD_VEND2, fixup_r0_reg); } > + > +static int lan865x_setup_cfgparam(struct phy_device *phydev) > +{ > + s8 offset1; > + s8 offset2; > + s8 value; > + u16 cfgparam; > + int ret; > + > + ret = lan865x_revb0_indirect_read(phydev, 0x0004); > + if (ret < 0) > + return ret; > + value = (s8)ret; > + /* Calculation of configuration offset 1 from AN1760 > + */ > + if ((value & 0x10) != 0) > + offset1 = value | 0xE0; > + else > + offset1 = value; > + > + ret = lan865x_revb0_indirect_read(phydev, 0x0008); > + if (ret < 0) > + return ret; > + value = (s8)ret; > + /* Calculation of configuration offset 2 from AN1760 > + */ > + if ((value & 0x10) != 0) > + offset2 = value | 0xE0; > + else > + offset2 = value; > + > + /* calculate and write the configuration parameters in the > + * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760) > + */ > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x0084); > + if (ret < 0) > + return ret; > + cfgparam = (ret & 0xF) | (((9 + offset1) << 10) | > + ((14 + offset1) << 4)); > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x84, cfgparam); > + if (ret) > + return ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x008A); > + if (ret < 0) > + return ret; > + cfgparam = (ret & 0x3FF) | ((40 + offset2) << 10); > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x8A, cfgparam); > + if (ret) > + return ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AD); > + if (ret < 0) > + return ret; > + cfgparam = (ret & 0xC0C0) | (((5 + offset1) << 8) | (9 + offset1)); > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAD, cfgparam); > + if (ret) > + return ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AE); > + if (ret < 0) > + return ret; > + cfgparam = (ret & 0xC0C0) | (((9 + offset1) << 8) | (14 + offset1)); > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAE, cfgparam); > + if (ret) > + return ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AF); > + if (ret < 0) > + return ret; > + cfgparam = (ret & 0xC0C0) | (((17 + offset1) << 8) | (22 + offset1)); > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAF, cfgparam); > +} > + I suggest this func is broken up into multiple funcs, for the sake of readability. My suggestion is less efficient and would require allocating some arrays which might not be an ideal solution, but then readability would be my primary concern here. //this array should be placed at the top of the file with the other arrs static const u16 lan865x_revb0_fixup_cfg_regs[5] = { 0x84, 0x8A, 0xAD, 0xAE, 0xAF, }; /* This is pulled straigt from AN1760 from 'calulation of offset 1' & 'calculation of offset 2' */ static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[2]) { u16 fixup_regs[2] = {0x0004, 0x0008}; int val; for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) { val = (s8)lan865x_revb0_indirect_read(phydev, fixup_regs[i]); if (val < 0) return val; if ((val & 0x10) != 0) offsets[i] = val | 0xE0; else offsets[i] = val; } return 0; } static int lan865x_read_cfg_params(struct phy_device *phydev, s16 cfg_params[5]) { int err; for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) { err = phy_read_mmd(phydev, MDIO_MMD_VEND2, lan865x_revb0_fixup_cfg_regs[i]); if (err < 0) return err; cfg_params[i] = (u16)err; } return 0; } static int lan865x_write_cfg_params(struct phy_device *phydev, u16 cfg_params[5]) { int err; for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) { err = phy_write_mmd(phydev, MDIO_MMD_VEND2, lan865x_revb0_fixup_cfg_regs[i], cfg_params[i]); if (err < 0) return err; } return 0; } static int lan865x_setup_cfgparam(struct phy_device *phydev) { u16 cfg_results[5]; u16 cfg_params[5]; s8 offsets[2]; int err; err = lan865x_generate_cfg_offsets(phydev, offsets); if (err < 0) return err; err = lan865x_read_cfg_params(phydev, cfg_params); if (err < 0) return err; // This block computes the magic values that goes into the magic cfg regs. // Maybe there is some way of compacting this into a loop, but this seems like // as readable as it's going to get. cfg_results[0] = (cfg_params[0] & 0xF) | (((9 + offsets[0]) << 10) | ((14 + offsets[0]) << 4)); cfg_results[1] = (cfg_params[1] & 0x3FF) | ((40 + offsets[1]) << 10); cfg_results[2] = (cfg_params[2] & 0xC0C0) | (((5 + offsets[0]) << 8) | (9 + offsets[0])); cfg_results[3] = (cfg_params[3] & 0xC0C0) | (((9 + offsets[0]) << 8) | (14 + offsets[0])); cfg_results[4] = (cfg_params[4] & 0xC0C0) | (((17 + offsets[0]) << 8) | (22 + offsets[0])); return lan865x_write_cfg_params(phydev, cfg_results); } > +static int lan865x_revb0_config_init(struct phy_device *phydev) > +{ > + int addr; > + int value; > + int ret; > + > + /* As per AN1760, the below configuration applies only to the LAN8650/1 > + * hardware revision Rev.B0. > + */ I think this is implied by having it the device specific init func, you can probably drop this comment. > + for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) { > + addr = lan865x_revb0_fixup_registers[i]; > + value = lan865x_revb0_fixup_values[i]; > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, addr, value); > + if (ret) > + return ret; > + } > + /* function to calculate and write the configuration parameters in the > + * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760) > + */ > + ret = lan865x_setup_cfgparam(phydev); > + if (ret < 0) > + return ret; The loop and the call to lan865x_setup_cfgparam deviates from AN1760, in the AN the writes to the cfg regs are mixed into the writes to the fixup regs. Is this really intended and safe? I'm guessing this is done out of convenience, if so I'd suggest adding a comment along the lines // The writes to the fixup and cfg regs deviate from the recommendation // in AN1760, where the writes are intermixed. This is done out of // convenience but works I'm guessing you've tested it :) > + > + /* disable all the interrupts > + */ Maybe a motivation about why would be helpful here > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF); > + if (ret) > + return ret; > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF); > +} > + > +static int lan865x_revb0_plca_set_cfg(struct phy_device *phydev, > + const struct phy_plca_cfg *plca_cfg) > +{ > + int ret; > + > + ret = genphy_c45_plca_set_cfg(phydev, plca_cfg); > + if (ret) > + return ret; > + > + /* Disable the collision detection when PLCA is enabled and enable > + * collision detection when CSMA/CD mode is enabled. > + */ > + if (plca_cfg->enabled) > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0000); > + else > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0083); This register is marked as reserved in the datasheet, is this the intended reg? If it is intended a more detailed comment about what it does would be nice, since no one outside microchip will be able to verify it. Also is something similar relevant for the lan867x phys? > +} > + > static int lan867x_revb1_config_init(struct phy_device *phydev) > { > /* HW quirk: Microchip states in the application note (AN1699) for the phy > @@ -90,13 +260,13 @@ static int lan867x_revb1_config_init(struct phy_device *phydev) > * 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); > + err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF); > if (err != 0) > return err; > - return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_2_CTL, 0xFFFF); > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF); > } > > -static int lan867x_read_status(struct phy_device *phydev) > +static int lan86xx_read_status(struct phy_device *phydev) > { > /* The phy has some limitations, namely: > * - always reports link up > @@ -111,23 +281,34 @@ static int lan867x_read_status(struct phy_device *phydev) > return 0; > } > > -static struct phy_driver lan867x_revb1_driver[] = { > +static struct phy_driver lan86xx_driver[] = { > { > PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1), > .name = "LAN867X Rev.B1", > .features = PHY_BASIC_T1S_P2MP_FEATURES, > .config_init = lan867x_revb1_config_init, > - .read_status = lan867x_read_status, > + .read_status = lan86xx_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, > - } > + }, > + { > + PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0), > + .name = "LAN865X Rev.B0 Internal Phy", > + .features = PHY_BASIC_T1S_P2MP_FEATURES, > + .config_init = lan865x_revb0_config_init, > + .read_status = lan86xx_read_status, > + .get_plca_cfg = genphy_c45_plca_get_cfg, > + .set_plca_cfg = lan865x_revb0_plca_set_cfg, > + .get_plca_status = genphy_c45_plca_get_status, > + }, > }; > > -module_phy_driver(lan867x_revb1_driver); > +module_phy_driver(lan86xx_driver); > > static struct mdio_device_id __maybe_unused tbl[] = { > { PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) }, > + { PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0) }, > { } > }; > > @@ -135,4 +316,5 @@ MODULE_DEVICE_TABLE(mdio, tbl); > > MODULE_DESCRIPTION("Microchip 10BASE-T1S Phy driver"); > MODULE_AUTHOR("Ramón Nordin Rodriguez"); > +MODULE_AUTHOR("Parthiban Veerasooran <parthiban.veerasooran@microchip.com>"); > MODULE_LICENSE("GPL"); > -- > 2.34.1 >
The 04/26/2023 17:16, Parthiban Veerasooran wrote: Hi Parthiban, > > This patch adds support for the Microchip LAN865x Rev.B0 10BASE-T1S > Internal PHYs (LAN8650/1). The LAN865x combines a Media Access Controller > (MAC) and an internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S > networks. I really thought that first we will have an internal review as we discussed before sending this out! > > Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> > --- > drivers/net/phy/microchip_t1s.c | 200 ++++++++++++++++++++++++++++++-- > 1 file changed, 191 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c > index 793fb0210605..3d8d285b3c34 100644 > --- a/drivers/net/phy/microchip_t1s.c > +++ b/drivers/net/phy/microchip_t1s.c > @@ -4,6 +4,7 @@ > * > * Support: Microchip Phys: > * lan8670/1/2 Rev.B1 > + * lan8650/1 Rev.B0 Internal PHYs > */ > > #include <linux/kernel.h> > @@ -11,9 +12,10 @@ > #include <linux/phy.h> > > #define PHY_ID_LAN867X_REVB1 0x0007C162 > +#define PHY_ID_LAN865X_REVB0 0x0007C1B3 > > -#define LAN867X_REG_IRQ_1_CTL 0x001C > -#define LAN867X_REG_IRQ_2_CTL 0x001D > +#define LAN86XX_REG_IRQ_1_CTL 0x001C > +#define LAN86XX_REG_IRQ_2_CTL 0x001D > > /* The arrays below are pulled from the following table from AN1699 > * Access MMD Address Value Mask > @@ -49,6 +51,174 @@ static const int lan867x_revb1_fixup_masks[12] = { > 0x0600, 0x7F00, 0x2000, 0xFFFF, > }; > > +/* LAN865x Rev.B0 configuration parameters from AN1760 > + */ You can put */ on previous line. > +static const int lan865x_revb0_fixup_registers[28] = { > + 0x0091, 0x0081, 0x0043, 0x0044, > + 0x0045, 0x0053, 0x0054, 0x0055, > + 0x0040, 0x0050, 0x00D0, 0x00E9, > + 0x00F5, 0x00F4, 0x00F8, 0x00F9, > + 0x00B0, 0x00B1, 0x00B2, 0x00B3, > + 0x00B4, 0x00B5, 0x00B6, 0x00B7, > + 0x00B8, 0x00B9, 0x00BA, 0x00BB, > +}; > + > +static const int lan865x_revb0_fixup_values[28] = { Can't this be u16? As this is used only by phy_write_mmd which gets an u16. > + 0x9660, 0x00C0, 0x00FF, 0xFFFF, > + 0x0000, 0x00FF, 0xFFFF, 0x0000, > + 0x0002, 0x0002, 0x5F21, 0x9E50, > + 0x1CF8, 0xC020, 0x9B00, 0x4E53, > + 0x0103, 0x0910, 0x1D26, 0x002A, > + 0x0103, 0x070D, 0x1720, 0x0027, > + 0x0509, 0x0E13, 0x1C25, 0x002B, > +}; > + > +/* indirect read pseudocode from AN1760 In the entire file, sometimes when you write the comment you start with a lower case, sometimes with an upper case, please be consistent. > + * write_register(0x4, 0x00D8, addr) > + * write_register(0x4, 0x00DA, 0x2) > + * return (int8)(read_register(0x4, 0x00D9)) > + */ > +static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr) > +{ > + int ret; > + > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xD8, addr); > + if (ret) > + return ret; > + > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xDA, 0x0002); > + if (ret) > + return ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0xD9); You can return phy_read_mmd, there is no point to check the ret here. > + if (ret < 0) > + return ret; > + > + return ret; > +} > + > +static int lan865x_setup_cfgparam(struct phy_device *phydev) > +{ > + s8 offset1; > + s8 offset2; > + s8 value; > + u16 cfgparam; > + int ret; Please use reverse christmas notation > + > + ret = lan865x_revb0_indirect_read(phydev, 0x0004); > + if (ret < 0) > + return ret; > + value = (s8)ret; > + /* Calculation of configuration offset 1 from AN1760 > + */ Again, you can put */ on the previous line. There other places in the file, I will not mention all of them. > + if ((value & 0x10) != 0) > + offset1 = value | 0xE0; > + else > + offset1 = value; > + > + ret = lan865x_revb0_indirect_read(phydev, 0x0008); > + if (ret < 0) > + return ret; > + value = (s8)ret; > + /* Calculation of configuration offset 2 from AN1760 > + */ > + if ((value & 0x10) != 0) > + offset2 = value | 0xE0; > + else > + offset2 = value; > + > + /* calculate and write the configuration parameters in the > + * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760) > + */ > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x0084); > + if (ret < 0) > + return ret; > + cfgparam = (ret & 0xF) | (((9 + offset1) << 10) | > + ((14 + offset1) << 4)); What about using FIELD_PREP to skip all <<? > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x84, cfgparam); > + if (ret) > + return ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x008A); Small thing, here you use 0x008A, and few lines bellow only 0X8A, please be consistent > + if (ret < 0) > + return ret; > + cfgparam = (ret & 0x3FF) | ((40 + offset2) << 10); > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x8A, cfgparam); > + if (ret) > + return ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AD); > + if (ret < 0) > + return ret; > + cfgparam = (ret & 0xC0C0) | (((5 + offset1) << 8) | (9 + offset1)); > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAD, cfgparam); > + if (ret) > + return ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AE); > + if (ret < 0) > + return ret; > + cfgparam = (ret & 0xC0C0) | (((9 + offset1) << 8) | (14 + offset1)); > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAE, cfgparam); > + if (ret) > + return ret; > + > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AF); > + if (ret < 0) > + return ret; > + cfgparam = (ret & 0xC0C0) | (((17 + offset1) << 8) | (22 + offset1)); > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAF, cfgparam); There are quite a lot of hardcoded values in the previous code, can you add some comments what they mean, or add defines for them. > +} > + > +static int lan865x_revb0_config_init(struct phy_device *phydev) > +{ > + int addr; > + int value; > + int ret; Please use reverse x-mas > + > + /* As per AN1760, the below configuration applies only to the LAN8650/1 > + * hardware revision Rev.B0. > + */ > + for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) { > + addr = lan865x_revb0_fixup_registers[i]; > + value = lan865x_revb0_fixup_values[i]; Doesn't seem that you need the variable value here. > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, addr, value); > + if (ret) > + return ret; > + } > + /* function to calculate and write the configuration parameters in the > + * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760) > + */ > + ret = lan865x_setup_cfgparam(phydev); > + if (ret < 0) > + return ret; > + > + /* disable all the interrupts > + */ > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF); > + if (ret) > + return ret; > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF); You can use GENMASK instead of 0xFFFF > +} > + > +static int lan865x_revb0_plca_set_cfg(struct phy_device *phydev, > + const struct phy_plca_cfg *plca_cfg) > +{ > + int ret; > + > + ret = genphy_c45_plca_set_cfg(phydev, plca_cfg); > + if (ret) > + return ret; > + > + /* Disable the collision detection when PLCA is enabled and enable > + * collision detection when CSMA/CD mode is enabled. > + */ > + if (plca_cfg->enabled) > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0000); > + else > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0083); > +} > + > static int lan867x_revb1_config_init(struct phy_device *phydev) > { > /* HW quirk: Microchip states in the application note (AN1699) for the phy > @@ -90,13 +260,13 @@ static int lan867x_revb1_config_init(struct phy_device *phydev) > * 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); > + err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF); > if (err != 0) > return err; > - return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_2_CTL, 0xFFFF); > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF); > } > > -static int lan867x_read_status(struct phy_device *phydev) > +static int lan86xx_read_status(struct phy_device *phydev) > { > /* The phy has some limitations, namely: > * - always reports link up > @@ -111,23 +281,34 @@ static int lan867x_read_status(struct phy_device *phydev) > return 0; > } > > -static struct phy_driver lan867x_revb1_driver[] = { > +static struct phy_driver lan86xx_driver[] = { > { > PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1), > .name = "LAN867X Rev.B1", > .features = PHY_BASIC_T1S_P2MP_FEATURES, > .config_init = lan867x_revb1_config_init, > - .read_status = lan867x_read_status, > + .read_status = lan86xx_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, > - } > + }, > + { > + PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0), > + .name = "LAN865X Rev.B0 Internal Phy", > + .features = PHY_BASIC_T1S_P2MP_FEATURES, > + .config_init = lan865x_revb0_config_init, > + .read_status = lan86xx_read_status, > + .get_plca_cfg = genphy_c45_plca_get_cfg, > + .set_plca_cfg = lan865x_revb0_plca_set_cfg, > + .get_plca_status = genphy_c45_plca_get_status, > + }, > }; > > -module_phy_driver(lan867x_revb1_driver); > +module_phy_driver(lan86xx_driver); > > static struct mdio_device_id __maybe_unused tbl[] = { > { PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) }, > + { PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0) }, > { } > }; > > @@ -135,4 +316,5 @@ MODULE_DEVICE_TABLE(mdio, tbl); > > MODULE_DESCRIPTION("Microchip 10BASE-T1S Phy driver"); > MODULE_AUTHOR("Ramón Nordin Rodriguez"); > +MODULE_AUTHOR("Parthiban Veerasooran <parthiban.veerasooran@microchip.com>"); > MODULE_LICENSE("GPL"); > -- > 2.34.1 >
> > +/* indirect read pseudocode from AN1760 > > + * write_register(0x4, 0x00D8, addr) > > + * write_register(0x4, 0x00DA, 0x2) > > + * return (int8)(read_register(0x4, 0x00D9)) > > + */ > > I suggest this comment block is slightly changed to > > /* Pulled from AN1760 describing 'indirect read' > * > * write_register(0x4, 0x00D8, addr) > * write_register(0x4, 0x00DA, 0x2) > * return (int8)(read_register(0x4, 0x00D9)) > * > * 0x4 refers to memory map selector 4, which maps to MDIO_MMD_VEND2 > */ > > > +static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr) > > +{ > > + int ret; > > + > > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xD8, addr); > > + if (ret) > > + return ret; > > + > > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xDA, 0x0002); > > + if (ret) > > + return ret; > > + > > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0xD9); > > + if (ret < 0) > > + return ret; > > + > > + return ret; > > +} It is unclear to me how 0x4 maps to MDIO_MMD_VEND2, which is 31. Why not just describe it in terms of MMD read/write? > The func itself might get a bit more readable by naming the magic regs > and value, below is a suggestion. > > static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr) > { > int ret; > static const u16 fixup_w0_reg = 0x00D8; > static const u16 fixup_r0_reg = 0x00D9; > static const u16 fixup_w1_val = 0x0002; > static const u16 fixup_w1_reg = 0x00DA; #defines would be normal, not variables. And i guess 0x0002 is actually BIT(1). And it probably means something like START? 0xD8 is some sort of address register, so i would put ADDR in the name. 0xD9 appears to be a control register, so CTRL. And 0xDA is a data register? So these could be give more descriptive names, just by my reverse engineering it. With the actual data sheet, i'm expect somebody could do better. > > +static int lan865x_revb0_config_init(struct phy_device *phydev) > > +{ > > + int addr; > > + int value; > > + int ret; > > + > > + /* As per AN1760, the below configuration applies only to the LAN8650/1 > > + * hardware revision Rev.B0. > > + */ > > I think this is implied by having it the device specific init func, you > can probably drop this comment. A reference to AN1760 somewhere in the driver would be good, to help people understand why this magic is needed. Does AN1760 actually explain the magic, or just say you need to do this to make it work, Trust Us(tm). Andrew
> -#define LAN867X_REG_IRQ_1_CTL 0x001C > -#define LAN867X_REG_IRQ_2_CTL 0x001D > +#define LAN86XX_REG_IRQ_1_CTL 0x001C > +#define LAN86XX_REG_IRQ_2_CTL 0x001D This patch is pretty big. Please split to LAN867X to LAN86XX rename out, to make the patches smaller and easier to review. > +static int lan865x_revb0_plca_set_cfg(struct phy_device *phydev, > + const struct phy_plca_cfg *plca_cfg) > +{ > + int ret; > + > + ret = genphy_c45_plca_set_cfg(phydev, plca_cfg); > + if (ret) > + return ret; > + > + /* Disable the collision detection when PLCA is enabled and enable > + * collision detection when CSMA/CD mode is enabled. > + */ > + if (plca_cfg->enabled) > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0000); > + else > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0083); > +} > + This could be in a patch of its own, with a good commit message explaining why it is needed. Once you replace the magic numbers with #defines, the comment becomes pointless. But what is missing is the answer to the question Why? Andrew
> + /* disable all the interrupts > + */ > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF); > + if (ret) > + return ret; > + return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF); This is also something which could be in a patch of its own, with an explanation in the commit message. You said the device will generate an interrupt after reset whatever. So it would be good to document this odd behaviour. Also, should you actually clear the pending interrupt, as well as disable interrupts? I assume there is an interrupt status register? It would typically be clear on read, or write 1 to clear a specific interrupt? Andrew
Hi Horatiu, Thanks for reviewing my patches. Please see my comments below. Best Regards, Parthiban V On 27/04/23 2:51 am, Horatiu Vultur wrote: > The 04/26/2023 17:16, Parthiban Veerasooran wrote: > > Hi Parthiban, > >> >> This patch adds support for the Microchip LAN865x Rev.B0 10BASE-T1S >> Internal PHYs (LAN8650/1). The LAN865x combines a Media Access Controller >> (MAC) and an internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S >> networks. > > I really thought that first we will have an internal review as we > discussed before sending this out! As the initial version of the microchip 10BASE-T1S PHY driver is already in the mainline with lan867x support, I sent these patches directly to apply on top of that. Definitely for the upcoming new drivers in the pipeline, I will approach for the internal review with you first before going to the mainline. Sorry if I had a miscommunication here. Also please let me know if you have other proposal? > >> >> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> >> --- >> drivers/net/phy/microchip_t1s.c | 200 ++++++++++++++++++++++++++++++-- >> 1 file changed, 191 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c >> index 793fb0210605..3d8d285b3c34 100644 >> --- a/drivers/net/phy/microchip_t1s.c >> +++ b/drivers/net/phy/microchip_t1s.c >> @@ -4,6 +4,7 @@ >> * >> * Support: Microchip Phys: >> * lan8670/1/2 Rev.B1 >> + * lan8650/1 Rev.B0 Internal PHYs >> */ >> >> #include <linux/kernel.h> >> @@ -11,9 +12,10 @@ >> #include <linux/phy.h> >> >> #define PHY_ID_LAN867X_REVB1 0x0007C162 >> +#define PHY_ID_LAN865X_REVB0 0x0007C1B3 >> >> -#define LAN867X_REG_IRQ_1_CTL 0x001C >> -#define LAN867X_REG_IRQ_2_CTL 0x001D >> +#define LAN86XX_REG_IRQ_1_CTL 0x001C >> +#define LAN86XX_REG_IRQ_2_CTL 0x001D >> >> /* The arrays below are pulled from the following table from AN1699 >> * Access MMD Address Value Mask >> @@ -49,6 +51,174 @@ static const int lan867x_revb1_fixup_masks[12] = { >> 0x0600, 0x7F00, 0x2000, 0xFFFF, >> }; >> >> +/* LAN865x Rev.B0 configuration parameters from AN1760 >> + */ > > You can put */ on previous line. Sure, will do. > >> +static const int lan865x_revb0_fixup_registers[28] = { >> + 0x0091, 0x0081, 0x0043, 0x0044, >> + 0x0045, 0x0053, 0x0054, 0x0055, >> + 0x0040, 0x0050, 0x00D0, 0x00E9, >> + 0x00F5, 0x00F4, 0x00F8, 0x00F9, >> + 0x00B0, 0x00B1, 0x00B2, 0x00B3, >> + 0x00B4, 0x00B5, 0x00B6, 0x00B7, >> + 0x00B8, 0x00B9, 0x00BA, 0x00BB, >> +}; >> + >> +static const int lan865x_revb0_fixup_values[28] = { > > Can't this be u16? As this is used only by phy_write_mmd which gets an > u16. yes, will do it. > >> + 0x9660, 0x00C0, 0x00FF, 0xFFFF, >> + 0x0000, 0x00FF, 0xFFFF, 0x0000, >> + 0x0002, 0x0002, 0x5F21, 0x9E50, >> + 0x1CF8, 0xC020, 0x9B00, 0x4E53, >> + 0x0103, 0x0910, 0x1D26, 0x002A, >> + 0x0103, 0x070D, 0x1720, 0x0027, >> + 0x0509, 0x0E13, 0x1C25, 0x002B, >> +}; >> + >> +/* indirect read pseudocode from AN1760 > > In the entire file, sometimes when you write the comment you start with > a lower case, sometimes with an upper case, please be consistent. Sure, I will. > >> + * write_register(0x4, 0x00D8, addr) >> + * write_register(0x4, 0x00DA, 0x2) >> + * return (int8)(read_register(0x4, 0x00D9)) >> + */ >> +static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr) >> +{ >> + int ret; >> + >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xD8, addr); >> + if (ret) >> + return ret; >> + >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xDA, 0x0002); >> + if (ret) >> + return ret; >> + >> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0xD9); > > You can return phy_read_mmd, there is no point to check the ret here. Ah good idea. > >> + if (ret < 0) >> + return ret; >> + >> + return ret; >> +} >> + >> +static int lan865x_setup_cfgparam(struct phy_device *phydev) >> +{ >> + s8 offset1; >> + s8 offset2; >> + s8 value; >> + u16 cfgparam; >> + int ret; > > Please use reverse christmas notation Sure, I will. > >> + >> + ret = lan865x_revb0_indirect_read(phydev, 0x0004); >> + if (ret < 0) >> + return ret; >> + value = (s8)ret; >> + /* Calculation of configuration offset 1 from AN1760 >> + */ > > Again, you can put */ on the previous line. There other places in the > file, I will not mention all of them. Sure, I will. > >> + if ((value & 0x10) != 0) >> + offset1 = value | 0xE0; >> + else >> + offset1 = value; >> + >> + ret = lan865x_revb0_indirect_read(phydev, 0x0008); >> + if (ret < 0) >> + return ret; >> + value = (s8)ret; >> + /* Calculation of configuration offset 2 from AN1760 >> + */ >> + if ((value & 0x10) != 0) >> + offset2 = value | 0xE0; >> + else >> + offset2 = value; >> + >> + /* calculate and write the configuration parameters in the >> + * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760) >> + */ >> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x0084); >> + if (ret < 0) >> + return ret; >> + cfgparam = (ret & 0xF) | (((9 + offset1) << 10) | >> + ((14 + offset1) << 4)); > > What about using FIELD_PREP to skip all <<? Ok noted. > >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x84, cfgparam); >> + if (ret) >> + return ret; >> + >> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x008A); > > Small thing, here you use 0x008A, and few lines bellow only 0X8A, please > be consistent Sure, I will. > >> + if (ret < 0) >> + return ret; >> + cfgparam = (ret & 0x3FF) | ((40 + offset2) << 10); >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x8A, cfgparam); >> + if (ret) >> + return ret; >> + >> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AD); >> + if (ret < 0) >> + return ret; >> + cfgparam = (ret & 0xC0C0) | (((5 + offset1) << 8) | (9 + offset1)); >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAD, cfgparam); >> + if (ret) >> + return ret; >> + >> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AE); >> + if (ret < 0) >> + return ret; >> + cfgparam = (ret & 0xC0C0) | (((9 + offset1) << 8) | (14 + offset1)); >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAE, cfgparam); >> + if (ret) >> + return ret; >> + >> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AF); >> + if (ret < 0) >> + return ret; >> + cfgparam = (ret & 0xC0C0) | (((17 + offset1) << 8) | (22 + offset1)); >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAF, cfgparam); > > There are quite a lot of hardcoded values in the previous code, can you add > some comments what they mean, or add defines for them. Sure, I will check this. > >> +} >> + >> +static int lan865x_revb0_config_init(struct phy_device *phydev) >> +{ >> + int addr; >> + int value; >> + int ret; > > Please use reverse x-mas Ok noted. > >> + >> + /* As per AN1760, the below configuration applies only to the LAN8650/1 >> + * hardware revision Rev.B0. >> + */ >> + for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) { >> + addr = lan865x_revb0_fixup_registers[i]; >> + value = lan865x_revb0_fixup_values[i]; > > Doesn't seem that you need the variable value here. Ok noted. > >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, addr, value); >> + if (ret) >> + return ret; >> + } >> + /* function to calculate and write the configuration parameters in the >> + * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760) >> + */ >> + ret = lan865x_setup_cfgparam(phydev); >> + if (ret < 0) >> + return ret; >> + >> + /* disable all the interrupts >> + */ >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF); >> + if (ret) >> + return ret; >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF); > > You can use GENMASK instead of 0xFFFF Ok noted. > >> +} >> + >> +static int lan865x_revb0_plca_set_cfg(struct phy_device *phydev, >> + const struct phy_plca_cfg *plca_cfg) >> +{ >> + int ret; >> + >> + ret = genphy_c45_plca_set_cfg(phydev, plca_cfg); >> + if (ret) >> + return ret; >> + >> + /* Disable the collision detection when PLCA is enabled and enable >> + * collision detection when CSMA/CD mode is enabled. >> + */ >> + if (plca_cfg->enabled) >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0000); >> + else >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0083); >> +} >> + >> static int lan867x_revb1_config_init(struct phy_device *phydev) >> { >> /* HW quirk: Microchip states in the application note (AN1699) for the phy >> @@ -90,13 +260,13 @@ static int lan867x_revb1_config_init(struct phy_device *phydev) >> * 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); >> + err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF); >> if (err != 0) >> return err; >> - return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_2_CTL, 0xFFFF); >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF); >> } >> >> -static int lan867x_read_status(struct phy_device *phydev) >> +static int lan86xx_read_status(struct phy_device *phydev) >> { >> /* The phy has some limitations, namely: >> * - always reports link up >> @@ -111,23 +281,34 @@ static int lan867x_read_status(struct phy_device *phydev) >> return 0; >> } >> >> -static struct phy_driver lan867x_revb1_driver[] = { >> +static struct phy_driver lan86xx_driver[] = { >> { >> PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1), >> .name = "LAN867X Rev.B1", >> .features = PHY_BASIC_T1S_P2MP_FEATURES, >> .config_init = lan867x_revb1_config_init, >> - .read_status = lan867x_read_status, >> + .read_status = lan86xx_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, >> - } >> + }, >> + { >> + PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0), >> + .name = "LAN865X Rev.B0 Internal Phy", >> + .features = PHY_BASIC_T1S_P2MP_FEATURES, >> + .config_init = lan865x_revb0_config_init, >> + .read_status = lan86xx_read_status, >> + .get_plca_cfg = genphy_c45_plca_get_cfg, >> + .set_plca_cfg = lan865x_revb0_plca_set_cfg, >> + .get_plca_status = genphy_c45_plca_get_status, >> + }, >> }; >> >> -module_phy_driver(lan867x_revb1_driver); >> +module_phy_driver(lan86xx_driver); >> >> static struct mdio_device_id __maybe_unused tbl[] = { >> { PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) }, >> + { PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0) }, >> { } >> }; >> >> @@ -135,4 +316,5 @@ MODULE_DEVICE_TABLE(mdio, tbl); >> >> MODULE_DESCRIPTION("Microchip 10BASE-T1S Phy driver"); >> MODULE_AUTHOR("Ramón Nordin Rodriguez"); >> +MODULE_AUTHOR("Parthiban Veerasooran <parthiban.veerasooran@microchip.com>"); >> MODULE_LICENSE("GPL"); >> -- >> 2.34.1 >> >
Hi Andrew, Thanks for reviewing the patches. Please see my comments below. Best Regards, Parthiban V On 27/04/23 3:13 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>> +/* indirect read pseudocode from AN1760 >>> + * write_register(0x4, 0x00D8, addr) >>> + * write_register(0x4, 0x00DA, 0x2) >>> + * return (int8)(read_register(0x4, 0x00D9)) >>> + */ >> >> I suggest this comment block is slightly changed to >> >> /* Pulled from AN1760 describing 'indirect read' >> * >> * write_register(0x4, 0x00D8, addr) >> * write_register(0x4, 0x00DA, 0x2) >> * return (int8)(read_register(0x4, 0x00D9)) >> * >> * 0x4 refers to memory map selector 4, which maps to MDIO_MMD_VEND2 >> */ >> >>> +static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr) >>> +{ >>> + int ret; >>> + >>> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xD8, addr); >>> + if (ret) >>> + return ret; >>> + >>> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xDA, 0x0002); >>> + if (ret) >>> + return ret; >>> + >>> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0xD9); >>> + if (ret < 0) >>> + return ret; >>> + >>> + return ret; >>> +} > > It is unclear to me how 0x4 maps to MDIO_MMD_VEND2, which is 31. > > Why not just describe it in terms of MMD read/write? This is an internal 10BASE-T1S PHY of LAN865x MAC-PHY. LAN865x has SPI to interface with Host. The integrated PHY vendor specific registers are located within Memory Map Selector 4 (MMS4). This MMS4 will be used as a base if the MAC driver wants to access the integrated PHY vendor specific registers directly through SPI without PHY driver. Is it clear? or do you need more information? > >> The func itself might get a bit more readable by naming the magic regs >> and value, below is a suggestion. >> >> static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr) >> { >> int ret; >> static const u16 fixup_w0_reg = 0x00D8; >> static const u16 fixup_r0_reg = 0x00D9; >> static const u16 fixup_w1_val = 0x0002; >> static const u16 fixup_w1_reg = 0x00DA; > > #defines would be normal, not variables. > > And i guess 0x0002 is actually BIT(1). And it probably means something > like START? 0xD8 is some sort of address register, so i would put ADDR > in the name. 0xD9 appears to be a control register, so CTRL. And 0xDA > is a data register? So these could be give more descriptive names, > just by my reverse engineering it. With the actual data sheet, i'm > expect somebody could do better. > >>> +static int lan865x_revb0_config_init(struct phy_device *phydev) >>> +{ >>> + int addr; >>> + int value; >>> + int ret; >>> + >>> + /* As per AN1760, the below configuration applies only to the LAN8650/1 >>> + * hardware revision Rev.B0. >>> + */ >> >> I think this is implied by having it the device specific init func, you >> can probably drop this comment. > > A reference to AN1760 somewhere in the driver would be good, to help Do you mean to give a web link to that document like below? https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf > people understand why this magic is needed. Does AN1760 actually > explain the magic, or just say you need to do this to make it work, Trust Us(tm). Unfortunately it doesn't explain what those init settings but we need to do this to work as expected. As you said, Trust Us. > > Andrew
Hi Ramon, Thanks for showing your interest in reviewing my patches. Please see my replies below, Best Regards, Parthiban V On 27/04/23 2:23 am, Ramón Nordin Rodriguez wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, Apr 26, 2023 at 05:16:55PM +0530, Parthiban Veerasooran wrote: >> This patch adds support for the Microchip LAN865x Rev.B0 10BASE-T1S >> Internal PHYs (LAN8650/1). The LAN865x combines a Media Access Controller >> (MAC) and an internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S >> networks. >> >> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> >> --- >> drivers/net/phy/microchip_t1s.c | 200 ++++++++++++++++++++++++++++++-- >> 1 file changed, 191 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c >> index 793fb0210605..3d8d285b3c34 100644 >> --- a/drivers/net/phy/microchip_t1s.c >> +++ b/drivers/net/phy/microchip_t1s.c >> @@ -4,6 +4,7 @@ >> * >> * Support: Microchip Phys: >> * lan8670/1/2 Rev.B1 >> + * lan8650/1 Rev.B0 Internal PHYs >> */ >> >> #include <linux/kernel.h> >> @@ -11,9 +12,10 @@ >> #include <linux/phy.h> >> >> #define PHY_ID_LAN867X_REVB1 0x0007C162 >> +#define PHY_ID_LAN865X_REVB0 0x0007C1B3 >> >> -#define LAN867X_REG_IRQ_1_CTL 0x001C >> -#define LAN867X_REG_IRQ_2_CTL 0x001D >> +#define LAN86XX_REG_IRQ_1_CTL 0x001C >> +#define LAN86XX_REG_IRQ_2_CTL 0x001D >> >> /* The arrays below are pulled from the following table from AN1699 >> * Access MMD Address Value Mask >> @@ -49,6 +51,174 @@ static const int lan867x_revb1_fixup_masks[12] = { >> 0x0600, 0x7F00, 0x2000, 0xFFFF, >> }; >> >> +/* LAN865x Rev.B0 configuration parameters from AN1760 >> + */ >> +static const int lan865x_revb0_fixup_registers[28] = { >> + 0x0091, 0x0081, 0x0043, 0x0044, >> + 0x0045, 0x0053, 0x0054, 0x0055, >> + 0x0040, 0x0050, 0x00D0, 0x00E9, >> + 0x00F5, 0x00F4, 0x00F8, 0x00F9, >> + 0x00B0, 0x00B1, 0x00B2, 0x00B3, >> + 0x00B4, 0x00B5, 0x00B6, 0x00B7, >> + 0x00B8, 0x00B9, 0x00BA, 0x00BB, >> +}; >> + >> +static const int lan865x_revb0_fixup_values[28] = { >> + 0x9660, 0x00C0, 0x00FF, 0xFFFF, >> + 0x0000, 0x00FF, 0xFFFF, 0x0000, >> + 0x0002, 0x0002, 0x5F21, 0x9E50, >> + 0x1CF8, 0xC020, 0x9B00, 0x4E53, >> + 0x0103, 0x0910, 0x1D26, 0x002A, >> + 0x0103, 0x070D, 0x1720, 0x0027, >> + 0x0509, 0x0E13, 0x1C25, 0x002B, >> +}; >> + >> +/* indirect read pseudocode from AN1760 >> + * write_register(0x4, 0x00D8, addr) >> + * write_register(0x4, 0x00DA, 0x2) >> + * return (int8)(read_register(0x4, 0x00D9)) >> + */ > > I suggest this comment block is slightly changed to > > /* Pulled from AN1760 describing 'indirect read' > * > * write_register(0x4, 0x00D8, addr) > * write_register(0x4, 0x00DA, 0x2) > * return (int8)(read_register(0x4, 0x00D9)) > * > * 0x4 refers to memory map selector 4, which maps to MDIO_MMD_VEND2 > *Sure, I will. > >> +static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr) >> +{ >> + int ret; >> + >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xD8, addr); >> + if (ret) >> + return ret; >> + >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xDA, 0x0002); >> + if (ret) >> + return ret; >> + >> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0xD9); >> + if (ret < 0) >> + return ret; >> + >> + return ret; >> +} > > The func itself might get a bit more readable by naming the magic regs > and value, below is a suggestion. > Ok noted. > static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr) > { > int ret; > static const u16 fixup_w0_reg = 0x00D8; > static const u16 fixup_r0_reg = 0x00D9; > static const u16 fixup_w1_val = 0x0002; > static const u16 fixup_w1_reg = 0x00DA; > > ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, fixup_w0_reg, addr); > if (ret) > return ret; > > ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, fixup_w1_reg, fixup_w1_val); > if (ret) > return ret; > > return phy_read_mmd(phydev, MDIO_MMD_VEND2, fixup_r0_reg); > } > Ok but I will consider Andrew's comment as well in the later email. >> + >> +static int lan865x_setup_cfgparam(struct phy_device *phydev) >> +{ >> + s8 offset1; >> + s8 offset2; >> + s8 value; >> + u16 cfgparam; >> + int ret; >> + >> + ret = lan865x_revb0_indirect_read(phydev, 0x0004); >> + if (ret < 0) >> + return ret; >> + value = (s8)ret; >> + /* Calculation of configuration offset 1 from AN1760 >> + */ >> + if ((value & 0x10) != 0) >> + offset1 = value | 0xE0; >> + else >> + offset1 = value; >> + >> + ret = lan865x_revb0_indirect_read(phydev, 0x0008); >> + if (ret < 0) >> + return ret; >> + value = (s8)ret; >> + /* Calculation of configuration offset 2 from AN1760 >> + */ >> + if ((value & 0x10) != 0) >> + offset2 = value | 0xE0; >> + else >> + offset2 = value; >> + >> + /* calculate and write the configuration parameters in the >> + * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760) >> + */ >> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x0084); >> + if (ret < 0) >> + return ret; >> + cfgparam = (ret & 0xF) | (((9 + offset1) << 10) | >> + ((14 + offset1) << 4)); >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x84, cfgparam); >> + if (ret) >> + return ret; >> + >> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x008A); >> + if (ret < 0) >> + return ret; >> + cfgparam = (ret & 0x3FF) | ((40 + offset2) << 10); >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x8A, cfgparam); >> + if (ret) >> + return ret; >> + >> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AD); >> + if (ret < 0) >> + return ret; >> + cfgparam = (ret & 0xC0C0) | (((5 + offset1) << 8) | (9 + offset1)); >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAD, cfgparam); >> + if (ret) >> + return ret; >> + >> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AE); >> + if (ret < 0) >> + return ret; >> + cfgparam = (ret & 0xC0C0) | (((9 + offset1) << 8) | (14 + offset1)); >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAE, cfgparam); >> + if (ret) >> + return ret; >> + >> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AF); >> + if (ret < 0) >> + return ret; >> + cfgparam = (ret & 0xC0C0) | (((17 + offset1) << 8) | (22 + offset1)); >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAF, cfgparam); >> +} >> + > > I suggest this func is broken up into multiple funcs, for the sake of > readability. My suggestion is less efficient and would require > allocating some arrays which might not be an ideal solution, but then > readability would be my primary concern here. > > //this array should be placed at the top of the file with the other arrs > static const u16 lan865x_revb0_fixup_cfg_regs[5] = { 0x84, 0x8A, 0xAD, 0xAE, 0xAF, }; > > /* This is pulled straigt from AN1760 from 'calulation of offset 1' & 'calculation of offset 2' > */ > static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[2]) > { > u16 fixup_regs[2] = {0x0004, 0x0008}; > int val; > > for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) { > val = (s8)lan865x_revb0_indirect_read(phydev, fixup_regs[i]); > if (val < 0) > return val; > if ((val & 0x10) != 0) > offsets[i] = val | 0xE0; > else > offsets[i] = val; > } > > return 0; > } > > static int lan865x_read_cfg_params(struct phy_device *phydev, s16 cfg_params[5]) > { > int err; > > for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) { > err = phy_read_mmd(phydev, MDIO_MMD_VEND2, lan865x_revb0_fixup_cfg_regs[i]); > if (err < 0) > return err; > cfg_params[i] = (u16)err; > } > > return 0; > } > > static int lan865x_write_cfg_params(struct phy_device *phydev, u16 cfg_params[5]) > { > int err; > > for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) { > err = phy_write_mmd(phydev, MDIO_MMD_VEND2, lan865x_revb0_fixup_cfg_regs[i], cfg_params[i]); > if (err < 0) > return err; > } > > return 0; > } > > static int lan865x_setup_cfgparam(struct phy_device *phydev) > { > u16 cfg_results[5]; > u16 cfg_params[5]; > s8 offsets[2]; > int err; > > err = lan865x_generate_cfg_offsets(phydev, offsets); > if (err < 0) > return err; > err = lan865x_read_cfg_params(phydev, cfg_params); > if (err < 0) > return err; > > // This block computes the magic values that goes into the magic cfg regs. > // Maybe there is some way of compacting this into a loop, but this seems like > // as readable as it's going to get. > cfg_results[0] = (cfg_params[0] & 0xF) | (((9 + offsets[0]) << 10) | > ((14 + offsets[0]) << 4)); > cfg_results[1] = (cfg_params[1] & 0x3FF) | ((40 + offsets[1]) << 10); > cfg_results[2] = (cfg_params[2] & 0xC0C0) | (((5 + offsets[0]) << 8) | (9 + offsets[0])); > cfg_results[3] = (cfg_params[3] & 0xC0C0) | (((9 + offsets[0]) << 8) | (14 + offsets[0])); > cfg_results[4] = (cfg_params[4] & 0xC0C0) | (((17 + offsets[0]) << 8) | (22 + offsets[0])); > > return lan865x_write_cfg_params(phydev, cfg_results); > } > Sure, I will consider this proposal as well. Thanks a lot for your effort on this. >> +static int lan865x_revb0_config_init(struct phy_device *phydev) >> +{ >> + int addr; >> + int value; >> + int ret; >> + >> + /* As per AN1760, the below configuration applies only to the LAN8650/1 >> + * hardware revision Rev.B0. >> + */ > > I think this is implied by having it the device specific init func, you > can probably drop this comment. Ok, I will consider Andrew's proposal as well in the later email. > >> + for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) { >> + addr = lan865x_revb0_fixup_registers[i]; >> + value = lan865x_revb0_fixup_values[i]; >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, addr, value); >> + if (ret) >> + return ret; >> + } >> + /* function to calculate and write the configuration parameters in the >> + * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760) >> + */ >> + ret = lan865x_setup_cfgparam(phydev); >> + if (ret < 0) >> + return ret; > > The loop and the call to lan865x_setup_cfgparam deviates from AN1760, in > the AN the writes to the cfg regs are mixed into the writes to the fixup > regs. Is this really intended and safe? > I'm guessing this is done out of convenience, if so I'd suggest adding a > comment along the lines > > // The writes to the fixup and cfg regs deviate from the recommendation > // in AN1760, where the writes are intermixed. This is done out of > // convenience but works > > I'm guessing you've tested it :) > Yes, we had this discussion locally and confirmed any order of this settings will work and even I tested it. >> + >> + /* disable all the interrupts >> + */ > > Maybe a motivation about why would be helpful here I noticed, there is a slight change in handling the interrupt between lan867x and lan865x PHYs. I will double check and update the code accordingly. > >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF); >> + if (ret) >> + return ret; >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF); >> +} >> + >> +static int lan865x_revb0_plca_set_cfg(struct phy_device *phydev, >> + const struct phy_plca_cfg *plca_cfg) >> +{ >> + int ret; >> + >> + ret = genphy_c45_plca_set_cfg(phydev, plca_cfg); >> + if (ret) >> + return ret; >> + >> + /* Disable the collision detection when PLCA is enabled and enable >> + * collision detection when CSMA/CD mode is enabled. >> + */ >> + if (plca_cfg->enabled) >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0000); >> + else >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0083); > > This register is marked as reserved in the datasheet, is this the > intended reg? Currently I am in contact with the design team for the clarification that why it is not documented in the datasheet. Will get more info soon. > If it is intended a more detailed comment about what it does would be > nice, since no one outside microchip will be able to verify it. > Also is something similar relevant for the lan867x phys? > Yes you are right it is applicable for lan867x as well. >> +} >> + >> static int lan867x_revb1_config_init(struct phy_device *phydev) >> { >> /* HW quirk: Microchip states in the application note (AN1699) for the phy >> @@ -90,13 +260,13 @@ static int lan867x_revb1_config_init(struct phy_device *phydev) >> * 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); >> + err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF); >> if (err != 0) >> return err; >> - return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_2_CTL, 0xFFFF); >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF); >> } >> >> -static int lan867x_read_status(struct phy_device *phydev) >> +static int lan86xx_read_status(struct phy_device *phydev) >> { >> /* The phy has some limitations, namely: >> * - always reports link up >> @@ -111,23 +281,34 @@ static int lan867x_read_status(struct phy_device *phydev) >> return 0; >> } >> >> -static struct phy_driver lan867x_revb1_driver[] = { >> +static struct phy_driver lan86xx_driver[] = { >> { >> PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1), >> .name = "LAN867X Rev.B1", >> .features = PHY_BASIC_T1S_P2MP_FEATURES, >> .config_init = lan867x_revb1_config_init, >> - .read_status = lan867x_read_status, >> + .read_status = lan86xx_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, >> - } >> + }, >> + { >> + PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0), >> + .name = "LAN865X Rev.B0 Internal Phy", >> + .features = PHY_BASIC_T1S_P2MP_FEATURES, >> + .config_init = lan865x_revb0_config_init, >> + .read_status = lan86xx_read_status, >> + .get_plca_cfg = genphy_c45_plca_get_cfg, >> + .set_plca_cfg = lan865x_revb0_plca_set_cfg, >> + .get_plca_status = genphy_c45_plca_get_status, >> + }, >> }; >> >> -module_phy_driver(lan867x_revb1_driver); >> +module_phy_driver(lan86xx_driver); >> >> static struct mdio_device_id __maybe_unused tbl[] = { >> { PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) }, >> + { PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0) }, >> { } >> }; >> >> @@ -135,4 +316,5 @@ MODULE_DEVICE_TABLE(mdio, tbl); >> >> MODULE_DESCRIPTION("Microchip 10BASE-T1S Phy driver"); >> MODULE_AUTHOR("Ramón Nordin Rodriguez"); >> +MODULE_AUTHOR("Parthiban Veerasooran <parthiban.veerasooran@microchip.com>"); >> MODULE_LICENSE("GPL"); >> -- >> 2.34.1 >>
On 27/04/23 3:22 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> -#define LAN867X_REG_IRQ_1_CTL 0x001C >> -#define LAN867X_REG_IRQ_2_CTL 0x001D >> +#define LAN86XX_REG_IRQ_1_CTL 0x001C >> +#define LAN86XX_REG_IRQ_2_CTL 0x001D > > This patch is pretty big. Please split to LAN867X to LAN86XX rename > out, to make the patches smaller and easier to reviewSure, will do it in the next version. > >> +static int lan865x_revb0_plca_set_cfg(struct phy_device *phydev, >> + const struct phy_plca_cfg *plca_cfg) >> +{ >> + int ret; >> + >> + ret = genphy_c45_plca_set_cfg(phydev, plca_cfg); >> + if (ret) >> + return ret; >> + >> + /* Disable the collision detection when PLCA is enabled and enable >> + * collision detection when CSMA/CD mode is enabled. >> + */ >> + if (plca_cfg->enabled) >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0000); >> + else >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0083); >> +} >> + > > This could be in a patch of its own, with a good commit message > explaining why it is needed. Sure, as Ramon asked in his review comment this is also needed for lan867x rev.b1 as well. So will make it as a separate patch before lan865x rev.b0 patch. > > Once you replace the magic numbers with #defines, the comment becomes > pointless. But what is missing is the answer to the question Why? As I commented in previous email to Ramon, I am in contact with design team for the clarification that why it is not documented. Will get more info soon. > > Andrew >
Hi Andrew, On 27/04/23 3:32 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> + /* disable all the interrupts >> + */ >> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF); >> + if (ret) >> + return ret; >> + return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF); > > This is also something which could be in a patch of its own, with an Ok noted. > explanation in the commit message. You said the device will generate > an interrupt after reset whatever. So it would be good to document > this odd behaviour. Also, should you actually clear the pending > interrupt, as well as disable interrupts? I assume there is an > interrupt status register? It would typically be clear on read, or > write 1 to clear a specific interrupt? Yes, I checked in the datasheet timing diagram and it is recommended to clear the "reset done" interrupt in the lan867x rev.b1 before going for the initial configuration. It may not be needed for lan865x. I noticed, there is a slight change in handling the interrupt between lan867x and lan865x PHYs. I will double check and update the code accordingly. Best Regards, Parthiban V > > Andrew
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c index 793fb0210605..3d8d285b3c34 100644 --- a/drivers/net/phy/microchip_t1s.c +++ b/drivers/net/phy/microchip_t1s.c @@ -4,6 +4,7 @@ * * Support: Microchip Phys: * lan8670/1/2 Rev.B1 + * lan8650/1 Rev.B0 Internal PHYs */ #include <linux/kernel.h> @@ -11,9 +12,10 @@ #include <linux/phy.h> #define PHY_ID_LAN867X_REVB1 0x0007C162 +#define PHY_ID_LAN865X_REVB0 0x0007C1B3 -#define LAN867X_REG_IRQ_1_CTL 0x001C -#define LAN867X_REG_IRQ_2_CTL 0x001D +#define LAN86XX_REG_IRQ_1_CTL 0x001C +#define LAN86XX_REG_IRQ_2_CTL 0x001D /* The arrays below are pulled from the following table from AN1699 * Access MMD Address Value Mask @@ -49,6 +51,174 @@ static const int lan867x_revb1_fixup_masks[12] = { 0x0600, 0x7F00, 0x2000, 0xFFFF, }; +/* LAN865x Rev.B0 configuration parameters from AN1760 + */ +static const int lan865x_revb0_fixup_registers[28] = { + 0x0091, 0x0081, 0x0043, 0x0044, + 0x0045, 0x0053, 0x0054, 0x0055, + 0x0040, 0x0050, 0x00D0, 0x00E9, + 0x00F5, 0x00F4, 0x00F8, 0x00F9, + 0x00B0, 0x00B1, 0x00B2, 0x00B3, + 0x00B4, 0x00B5, 0x00B6, 0x00B7, + 0x00B8, 0x00B9, 0x00BA, 0x00BB, +}; + +static const int lan865x_revb0_fixup_values[28] = { + 0x9660, 0x00C0, 0x00FF, 0xFFFF, + 0x0000, 0x00FF, 0xFFFF, 0x0000, + 0x0002, 0x0002, 0x5F21, 0x9E50, + 0x1CF8, 0xC020, 0x9B00, 0x4E53, + 0x0103, 0x0910, 0x1D26, 0x002A, + 0x0103, 0x070D, 0x1720, 0x0027, + 0x0509, 0x0E13, 0x1C25, 0x002B, +}; + +/* indirect read pseudocode from AN1760 + * write_register(0x4, 0x00D8, addr) + * write_register(0x4, 0x00DA, 0x2) + * return (int8)(read_register(0x4, 0x00D9)) + */ +static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr) +{ + int ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xD8, addr); + if (ret) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xDA, 0x0002); + if (ret) + return ret; + + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0xD9); + if (ret < 0) + return ret; + + return ret; +} + +static int lan865x_setup_cfgparam(struct phy_device *phydev) +{ + s8 offset1; + s8 offset2; + s8 value; + u16 cfgparam; + int ret; + + ret = lan865x_revb0_indirect_read(phydev, 0x0004); + if (ret < 0) + return ret; + value = (s8)ret; + /* Calculation of configuration offset 1 from AN1760 + */ + if ((value & 0x10) != 0) + offset1 = value | 0xE0; + else + offset1 = value; + + ret = lan865x_revb0_indirect_read(phydev, 0x0008); + if (ret < 0) + return ret; + value = (s8)ret; + /* Calculation of configuration offset 2 from AN1760 + */ + if ((value & 0x10) != 0) + offset2 = value | 0xE0; + else + offset2 = value; + + /* calculate and write the configuration parameters in the + * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760) + */ + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x0084); + if (ret < 0) + return ret; + cfgparam = (ret & 0xF) | (((9 + offset1) << 10) | + ((14 + offset1) << 4)); + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x84, cfgparam); + if (ret) + return ret; + + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x008A); + if (ret < 0) + return ret; + cfgparam = (ret & 0x3FF) | ((40 + offset2) << 10); + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x8A, cfgparam); + if (ret) + return ret; + + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AD); + if (ret < 0) + return ret; + cfgparam = (ret & 0xC0C0) | (((5 + offset1) << 8) | (9 + offset1)); + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAD, cfgparam); + if (ret) + return ret; + + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AE); + if (ret < 0) + return ret; + cfgparam = (ret & 0xC0C0) | (((9 + offset1) << 8) | (14 + offset1)); + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAE, cfgparam); + if (ret) + return ret; + + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AF); + if (ret < 0) + return ret; + cfgparam = (ret & 0xC0C0) | (((17 + offset1) << 8) | (22 + offset1)); + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAF, cfgparam); +} + +static int lan865x_revb0_config_init(struct phy_device *phydev) +{ + int addr; + int value; + int ret; + + /* As per AN1760, the below configuration applies only to the LAN8650/1 + * hardware revision Rev.B0. + */ + for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) { + addr = lan865x_revb0_fixup_registers[i]; + value = lan865x_revb0_fixup_values[i]; + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, addr, value); + if (ret) + return ret; + } + /* function to calculate and write the configuration parameters in the + * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760) + */ + ret = lan865x_setup_cfgparam(phydev); + if (ret < 0) + return ret; + + /* disable all the interrupts + */ + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF); + if (ret) + return ret; + return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF); +} + +static int lan865x_revb0_plca_set_cfg(struct phy_device *phydev, + const struct phy_plca_cfg *plca_cfg) +{ + int ret; + + ret = genphy_c45_plca_set_cfg(phydev, plca_cfg); + if (ret) + return ret; + + /* Disable the collision detection when PLCA is enabled and enable + * collision detection when CSMA/CD mode is enabled. + */ + if (plca_cfg->enabled) + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0000); + else + return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0083); +} + static int lan867x_revb1_config_init(struct phy_device *phydev) { /* HW quirk: Microchip states in the application note (AN1699) for the phy @@ -90,13 +260,13 @@ static int lan867x_revb1_config_init(struct phy_device *phydev) * 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); + err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF); if (err != 0) return err; - return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_2_CTL, 0xFFFF); + return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF); } -static int lan867x_read_status(struct phy_device *phydev) +static int lan86xx_read_status(struct phy_device *phydev) { /* The phy has some limitations, namely: * - always reports link up @@ -111,23 +281,34 @@ static int lan867x_read_status(struct phy_device *phydev) return 0; } -static struct phy_driver lan867x_revb1_driver[] = { +static struct phy_driver lan86xx_driver[] = { { PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1), .name = "LAN867X Rev.B1", .features = PHY_BASIC_T1S_P2MP_FEATURES, .config_init = lan867x_revb1_config_init, - .read_status = lan867x_read_status, + .read_status = lan86xx_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, - } + }, + { + PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0), + .name = "LAN865X Rev.B0 Internal Phy", + .features = PHY_BASIC_T1S_P2MP_FEATURES, + .config_init = lan865x_revb0_config_init, + .read_status = lan86xx_read_status, + .get_plca_cfg = genphy_c45_plca_get_cfg, + .set_plca_cfg = lan865x_revb0_plca_set_cfg, + .get_plca_status = genphy_c45_plca_get_status, + }, }; -module_phy_driver(lan867x_revb1_driver); +module_phy_driver(lan86xx_driver); static struct mdio_device_id __maybe_unused tbl[] = { { PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) }, + { PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0) }, { } }; @@ -135,4 +316,5 @@ MODULE_DEVICE_TABLE(mdio, tbl); MODULE_DESCRIPTION("Microchip 10BASE-T1S Phy driver"); MODULE_AUTHOR("Ramón Nordin Rodriguez"); +MODULE_AUTHOR("Parthiban Veerasooran <parthiban.veerasooran@microchip.com>"); MODULE_LICENSE("GPL");
This patch adds support for the Microchip LAN865x Rev.B0 10BASE-T1S Internal PHYs (LAN8650/1). The LAN865x combines a Media Access Controller (MAC) and an internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S networks. Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> --- drivers/net/phy/microchip_t1s.c | 200 ++++++++++++++++++++++++++++++-- 1 file changed, 191 insertions(+), 9 deletions(-)