| Message ID | 20231127104045.96722-3-ramon.nordin.rodriguez@ferroamp.se (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: microchip_t1s: additional phy support and collision detect handling | expand |
> #define PHY_ID_LAN867X_REVB1 0x0007C162 > +#define PHY_ID_LAN867X_REVC1 0x0007C164 So there is a gap in the revisions. Maybe a B2 exists? > +static int lan867x_revc1_read_fixup_value(struct phy_device *phydev, u16 addr) > +{ > + int regval; > + /* The AN pretty much just states 'trust us' regarding these magic vals */ > + const u16 magic_or = 0xE0; > + const u16 magic_reg_mask = 0x1F; > + const u16 magic_check_mask = 0x10; Reverse christmass tree please. Longest first, shorted last. > + regval = lan865x_revb0_indirect_read(phydev, addr); > + if (regval < 0) > + return regval; > + > + regval &= magic_reg_mask; > + > + return (regval & magic_check_mask) ? regval | magic_or : regval; > +} > + > +static int lan867x_revc1_config_init(struct phy_device *phydev) > +{ > + int err; > + int regval; > + u16 override0; > + u16 override1; > + const u16 override_addr0 = 0x4; > + const u16 override_addr1 = 0x8; > + const u8 index_to_override0 = 2; > + const u8 index_to_override1 = 3; Same here. > + > + err = lan867x_wait_for_reset_complete(phydev); > + if (err) > + return err; > + > + /* The application note specifies a super convenient process > + * where 2 of the fixup regs needs a write with a value that is > + * a modified result of another reg read. > + * Enjoy the magic show. > + */ I really do hope that by revision D1 they get the firmware sorted out so none of this undocumented magic is needed. Andrew
On Mon, Nov 27, 2023 at 02:37:41PM +0100, Andrew Lunn wrote: > > #define PHY_ID_LAN867X_REVB1 0x0007C162 > > +#define PHY_ID_LAN867X_REVC1 0x0007C164 > > So there is a gap in the revisions. Maybe a B2 exists? The datasheet lists A0, B1 and C1, seems like Microchip removes the application notes for old revisions, so no way that I can see to add the init-fixup for A0. I'm guessing there is a rev.c0 that was never released to the public. > > + const u16 magic_or = 0xE0; > > + const u16 magic_reg_mask = 0x1F; > > + const u16 magic_check_mask = 0x10; > > Reverse christmass tree please. Longest first, shorted last. My bad, I was just thinking 'christmas tree' forgot about the reverse. I'll fix that. > > + int err; > > + int regval; > > + u16 override0; > > + u16 override1; > > + const u16 override_addr0 = 0x4; > > + const u16 override_addr1 = 0x8; > > + const u8 index_to_override0 = 2; > > + const u8 index_to_override1 = 3; > > Same here. I'll fix this. > > > + > > + err = lan867x_wait_for_reset_complete(phydev); > > + if (err) > > + return err; > > + > > + /* The application note specifies a super convenient process > > + * where 2 of the fixup regs needs a write with a value that is > > + * a modified result of another reg read. > > + * Enjoy the magic show. > > + */ > > I really do hope that by revision D1 they get the firmware sorted out > so none of this undocumented magic is needed. > > Andrew Really do hope so..
Subject: Re: [PATCH 2/3] net: microchip_t1s: add support for LAN867x Rev.C1
Hi,
> So there is a gap in the revisions. Maybe a B2 exists?
Actually, probably not. Some search gives this datasheet:
https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/DataSheets/LAN8670-1-2-Data-Sheet-60001573.pdf
And page 2 (table 1) shows only revisions A0 (rev0), B1, (rev2), C1 (rev4).
Not sure about why only even revision numbers are released ?
Page 193 (table 10-1) also shows only B1 and C1. So you can be confident that only those exist.
@Ramón, thank you for your work on this driver!
Félix Piédallu
> > So there is a gap in the revisions. Maybe a B2 exists? > > Actually, probably not. Some search gives this datasheet: > > https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/DataSheets/LAN8670-1-2-Data-Sheet-60001573.pdf > > And page 2 (table 1) shows only revisions A0 (rev0), B1, (rev2), C1 (rev4). > Not sure about why only even revision numbers are released ? > > Page 193 (table 10-1) also shows only B1 and C1. So you can be confident that only those exist. > Thanks for clearing that up! > @Ramón, thank you for your work on this driver! Much appreciated R
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c index ace2bf35a18a..db84d850b165 100644 --- a/drivers/net/phy/microchip_t1s.c +++ b/drivers/net/phy/microchip_t1s.c @@ -12,6 +12,7 @@ #include <linux/phy.h> #define PHY_ID_LAN867X_REVB1 0x0007C162 +#define PHY_ID_LAN867X_REVC1 0x0007C164 #define PHY_ID_LAN865X_REVB0 0x0007C1B3 #define LAN867X_REG_STS2 0x0019 @@ -59,6 +60,22 @@ static const u16 lan867x_revb1_fixup_masks[12] = { 0x0600, 0x7F00, 0x2000, 0xFFFF, }; +static const u16 lan867x_revc1_fixup_registers[12] = { + 0x00D0, 0x00E0, 0x0084, 0x008A, + 0x00E9, 0x00F5, 0x00F4, 0x00F8, + 0x00F9, 0x0081, 0x0091, 0x0093, +}; + +/* Index 2 & 3 will not be used, these are runtime populated/calculated. + * It makes the code a lot easier to read having this arr the same len + * as the _fixup_registers arr though + */ +static const u16 lan867x_revc1_fixup_values[12] = { + 0x3F31, 0xC000, 0xFFFF, 0xFFFF, + 0x9E50, 0x1CF8, 0xC020, 0x9B00, + 0x4E53, 0x0080, 0x9660, 0x06E9, +}; + /* LAN865x Rev.B0 configuration parameters from AN1760 */ static const u32 lan865x_revb0_fixup_registers[28] = { 0x0091, 0x0081, 0x0043, 0x0044, @@ -263,6 +280,74 @@ static int lan867x_revb1_config_init(struct phy_device *phydev) return 0; } +static int lan867x_revc1_read_fixup_value(struct phy_device *phydev, u16 addr) +{ + int regval; + /* The AN pretty much just states 'trust us' regarding these magic vals */ + const u16 magic_or = 0xE0; + const u16 magic_reg_mask = 0x1F; + const u16 magic_check_mask = 0x10; + + regval = lan865x_revb0_indirect_read(phydev, addr); + if (regval < 0) + return regval; + + regval &= magic_reg_mask; + + return (regval & magic_check_mask) ? regval | magic_or : regval; +} + +static int lan867x_revc1_config_init(struct phy_device *phydev) +{ + int err; + int regval; + u16 override0; + u16 override1; + const u16 override_addr0 = 0x4; + const u16 override_addr1 = 0x8; + const u8 index_to_override0 = 2; + const u8 index_to_override1 = 3; + + err = lan867x_wait_for_reset_complete(phydev); + if (err) + return err; + + /* The application note specifies a super convenient process + * where 2 of the fixup regs needs a write with a value that is + * a modified result of another reg read. + * Enjoy the magic show. + */ + regval = lan867x_revc1_read_fixup_value(phydev, override_addr0); + if (regval < 0) + return regval; + override0 = ((regval + 9) << 10) | ((regval + 14) << 4) | 0x3; + + regval = lan867x_revc1_read_fixup_value(phydev, override_addr1); + if (regval < 0) + return regval; + override1 = (regval + 40) << 10; + + for (int i = 0; i < ARRAY_SIZE(lan867x_revc1_fixup_registers); i++) { + /* The hardcoded */ + if (i == index_to_override0) + err = phy_write_mmd(phydev, MDIO_MMD_VEND2, + lan867x_revc1_fixup_registers[i], + override0); + else if (i == index_to_override1) + err = phy_write_mmd(phydev, MDIO_MMD_VEND2, + lan867x_revc1_fixup_registers[i], + override1); + else + err = phy_write_mmd(phydev, MDIO_MMD_VEND2, + lan867x_revc1_fixup_registers[i], + lan867x_revc1_fixup_values[i]); + if (err) + return err; + } + + return 0; +} + static int lan86xx_read_status(struct phy_device *phydev) { /* The phy has some limitations, namely: @@ -289,6 +374,16 @@ static struct phy_driver microchip_t1s_driver[] = { .set_plca_cfg = genphy_c45_plca_set_cfg, .get_plca_status = genphy_c45_plca_get_status, }, + { + PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVC1), + .name = "LAN867X Rev.C1", + .features = PHY_BASIC_T1S_P2MP_FEATURES, + .config_init = lan867x_revc1_config_init, + .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", @@ -305,6 +400,7 @@ module_phy_driver(microchip_t1s_driver); static struct mdio_device_id __maybe_unused tbl[] = { { PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) }, + { PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVC1) }, { PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0) }, { } };