| Message ID | 20230522113331.36872-7-Parthiban.Veerasooran@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | microchip_t1s: Update on Microchip 10BASE-T1S PHY driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
> +static int lan865x_setup_cfgparam(struct phy_device *phydev) > +{ > + u16 cfg_results[5]; > + u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)]; > + s8 offsets[2]; > + int ret; Reverse Christmas tree please. > + > + ret = lan865x_generate_cfg_offsets(phydev, offsets); > + if (ret) > + return ret; > + > + ret = lan865x_read_cfg_params(phydev, cfg_params); Is this doing a read from fuses? Is anything documented about this? What the values mean? Would a board designer ever need to use different values? Or is this just a case of 'trust us', you don't need to understand this magic. > + if (ret) > + return ret; > + > + cfg_results[0] = (cfg_params[0] & 0x000F) | > + FIELD_PREP(GENMASK(15, 10), 9 + offsets[0]) | > + FIELD_PREP(GENMASK(15, 4), 14 + offsets[0]); > + cfg_results[1] = (cfg_params[1] & 0x03FF) | > + FIELD_PREP(GENMASK(15, 10), 40 + offsets[1]); > + cfg_results[2] = (cfg_params[2] & 0xC0C0) | > + FIELD_PREP(GENMASK(15, 8), 5 + offsets[0]) | > + (9 + offsets[0]); > + cfg_results[3] = (cfg_params[3] & 0xC0C0) | > + FIELD_PREP(GENMASK(15, 8), 9 + offsets[0]) | > + (14 + offsets[0]); > + cfg_results[4] = (cfg_params[4] & 0xC0C0) | > + FIELD_PREP(GENMASK(15, 8), 17 + offsets[0]) | > + (22 + offsets[0]); > + > + return lan865x_write_cfg_params(phydev, cfg_results); > +} Andrew
On Mon, May 22, 2023 at 05:03:31PM +0530, Parthiban Veerasooran wrote: > Add 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. As > LAN867X and LAN865X are using the same function for the read_status, > rename the function as lan86xx_read_status. > > Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> Hi Parthiban, thanks for your patch. Some minor nits from my side. ... > +/* This is pulled straigt from AN1760 from 'calulation of offset 1' & > + * 'calculation of offset 2' > + */ nit: s/straigt/straight/ > +static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[2]) > +{ > + const u16 fixup_regs[2] = {0x0004, 0x0008}; > + int ret; > + > + for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) { > + ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]); > + if (ret < 0) > + return ret; > + if (ret & BIT(4)) > + offsets[i] = ret | 0xE0; > + else > + offsets[i] = ret; > + } > + > + return 0; > +} ... > +static int lan865x_setup_cfgparam(struct phy_device *phydev) > +{ > + u16 cfg_results[5]; > + u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)]; > + s8 offsets[2]; > + int ret; nit: Please use reverse xmas tree order - longest line to shortest - for local variable declarations in networking code. ...
Hi Simon, Thanks for your interest in reviewing my patches. On 22/05/23 8:32 pm, Simon Horman wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, May 22, 2023 at 05:03:31PM +0530, Parthiban Veerasooran wrote: >> Add 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. As >> LAN867X and LAN865X are using the same function for the read_status, >> rename the function as lan86xx_read_status. >> >> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> > > Hi Parthiban, > > thanks for your patch. > Some minor nits from my side. > > ... > >> +/* This is pulled straigt from AN1760 from 'calulation of offset 1' & >> + * 'calculation of offset 2' >> + */ > > nit: s/straigt/straight/ Ah yes, surely will correct it in the next version. > >> +static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[2]) >> +{ >> + const u16 fixup_regs[2] = {0x0004, 0x0008}; >> + int ret; >> + >> + for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) { >> + ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]); >> + if (ret < 0) >> + return ret; >> + if (ret & BIT(4)) >> + offsets[i] = ret | 0xE0; >> + else >> + offsets[i] = ret; >> + } >> + >> + return 0; >> +} > > ... > >> +static int lan865x_setup_cfgparam(struct phy_device *phydev) >> +{ >> + u16 cfg_results[5]; >> + u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)]; >> + s8 offsets[2]; >> + int ret; > > nit: Please use reverse xmas tree order - longest line to shortest - > for local variable declarations in networking code. Yes understood, surely will correct it in the next version. Best Regards, Parthiban V > > ...
Hi Andrew, On 22/05/23 6:26 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> +static int lan865x_setup_cfgparam(struct phy_device *phydev) >> +{ >> + u16 cfg_results[5]; >> + u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)]; >> + s8 offsets[2]; >> + int ret; > > Reverse Christmas tree please. Ah yes, surely will correct it in the next version. > >> + >> + ret = lan865x_generate_cfg_offsets(phydev, offsets); >> + if (ret) >> + return ret; >> + >> + ret = lan865x_read_cfg_params(phydev, cfg_params); > > Is this doing a read from fuses? Is anything documented about this? > What the values mean? Would a board designer ever need to use > different values? Or is this just a case of 'trust us', you don't need > to understand this magic. Yes, it is a read from fuses and those values are specific/unique for each PHY chip. Those values are calculated based on some characteristics of the PHY chip behavior for optimal performance and they are fused in the PHY chip for the driver to configure it during the initialization. This is done in the production/testing stage of the PHY chip. As it is specific to PHY chip, a board designer doesn't have any influence on this and need not to worry about it. Unfortunately they can't be documented anywhere as they are design specific. So simply 'trust us'. Best Regards, Parthiban V > >> + if (ret) >> + return ret; >> + >> + cfg_results[0] = (cfg_params[0] & 0x000F) | >> + FIELD_PREP(GENMASK(15, 10), 9 + offsets[0]) | >> + FIELD_PREP(GENMASK(15, 4), 14 + offsets[0]); >> + cfg_results[1] = (cfg_params[1] & 0x03FF) | >> + FIELD_PREP(GENMASK(15, 10), 40 + offsets[1]); >> + cfg_results[2] = (cfg_params[2] & 0xC0C0) | >> + FIELD_PREP(GENMASK(15, 8), 5 + offsets[0]) | >> + (9 + offsets[0]); >> + cfg_results[3] = (cfg_params[3] & 0xC0C0) | >> + FIELD_PREP(GENMASK(15, 8), 9 + offsets[0]) | >> + (14 + offsets[0]); >> + cfg_results[4] = (cfg_params[4] & 0xC0C0) | >> + FIELD_PREP(GENMASK(15, 8), 17 + offsets[0]) | >> + (22 + offsets[0]); >> + >> + return lan865x_write_cfg_params(phydev, cfg_results); >> +} > > > Andrew >
> > Is this doing a read from fuses? Is anything documented about this? > > What the values mean? Would a board designer ever need to use > > different values? Or is this just a case of 'trust us', you don't need > > to understand this magic. > Yes, it is a read from fuses and those values are specific/unique for > each PHY chip. Those values are calculated based on some characteristics > of the PHY chip behavior for optimal performance and they are fused in > the PHY chip for the driver to configure it during the initialization. > This is done in the production/testing stage of the PHY chip. As it is > specific to PHY chip, a board designer doesn't have any influence on > this and need not to worry about it. Unfortunately they can't be > documented anywhere as they are design specific. So simply 'trust us'. O.K. Please consider for future generations that you move all this magic into the PHY firmware. There does not seem to be any reason the OS needs to know about this. Andrew
Hi Andrew, On 23/05/23 5:59 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>> Is this doing a read from fuses? Is anything documented about this? >>> What the values mean? Would a board designer ever need to use >>> different values? Or is this just a case of 'trust us', you don't need >>> to understand this magic. >> Yes, it is a read from fuses and those values are specific/unique for >> each PHY chip. Those values are calculated based on some characteristics >> of the PHY chip behavior for optimal performance and they are fused in >> the PHY chip for the driver to configure it during the initialization. >> This is done in the production/testing stage of the PHY chip. As it is >> specific to PHY chip, a board designer doesn't have any influence on >> this and need not to worry about it. Unfortunately they can't be >> documented anywhere as they are design specific. So simply 'trust us'. > > O.K. Please consider for future generations that you move all this > magic into the PHY firmware. There does not seem to be any reason the > OS needs to know about this. Thanks for the feedback. Best Regards, Parthiban V > > Andrew
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c index a612f7867df8..385d97d47cb4 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,11 +12,19 @@ #include <linux/phy.h> #define PHY_ID_LAN867X_REVB1 0x0007C162 +#define PHY_ID_LAN865X_REVB0 0x0007C1B3 #define LAN867X_REG_STS2 0x0019 #define LAN867x_RESET_COMPLETE_STS BIT(11) +#define LAN865X_REG_CFGPARAM_ADDR 0x00D8 +#define LAN865X_REG_CFGPARAM_DATA 0x00D9 +#define LAN865X_REG_CFGPARAM_CTRL 0x00DA +#define LAN865X_REG_STS2 0x0019 + +#define LAN865X_CFGPARAM_READ_ENABLE BIT(1) + /* The arrays below are pulled from the following table from AN1699 * Access MMD Address Value Mask * RMW 0x1F 0x00D0 0x0002 0x0E03 @@ -50,6 +59,164 @@ static const u16 lan867x_revb1_fixup_masks[12] = { 0x0600, 0x7F00, 0x2000, 0xFFFF, }; +/* LAN865x Rev.B0 configuration parameters from AN1760 */ +static const u32 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 u16 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, +}; + +static const u16 lan865x_revb0_fixup_cfg_regs[5] = { + 0x0084, 0x008A, 0x00AD, 0x00AE, 0x00AF +}; + +/* 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, LAN865X_REG_CFGPARAM_ADDR, + addr); + if (ret) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN865X_REG_CFGPARAM_CTRL, + LAN865X_CFGPARAM_READ_ENABLE); + if (ret) + return ret; + + return phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN865X_REG_CFGPARAM_DATA); +} + +/* 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]) +{ + const u16 fixup_regs[2] = {0x0004, 0x0008}; + int ret; + + for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) { + ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]); + if (ret < 0) + return ret; + if (ret & BIT(4)) + offsets[i] = ret | 0xE0; + else + offsets[i] = ret; + } + + return 0; +} + +static int lan865x_read_cfg_params(struct phy_device *phydev, u16 cfg_params[]) +{ + int ret; + + for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) { + ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, + lan865x_revb0_fixup_cfg_regs[i]); + if (ret < 0) + return ret; + cfg_params[i] = (u16)ret; + } + + return 0; +} + +static int lan865x_write_cfg_params(struct phy_device *phydev, u16 cfg_params[]) +{ + int ret; + + for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) { + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, + lan865x_revb0_fixup_cfg_regs[i], + cfg_params[i]); + if (ret) + return ret; + } + + return 0; +} + +static int lan865x_setup_cfgparam(struct phy_device *phydev) +{ + u16 cfg_results[5]; + u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)]; + s8 offsets[2]; + int ret; + + ret = lan865x_generate_cfg_offsets(phydev, offsets); + if (ret) + return ret; + + ret = lan865x_read_cfg_params(phydev, cfg_params); + if (ret) + return ret; + + cfg_results[0] = (cfg_params[0] & 0x000F) | + FIELD_PREP(GENMASK(15, 10), 9 + offsets[0]) | + FIELD_PREP(GENMASK(15, 4), 14 + offsets[0]); + cfg_results[1] = (cfg_params[1] & 0x03FF) | + FIELD_PREP(GENMASK(15, 10), 40 + offsets[1]); + cfg_results[2] = (cfg_params[2] & 0xC0C0) | + FIELD_PREP(GENMASK(15, 8), 5 + offsets[0]) | + (9 + offsets[0]); + cfg_results[3] = (cfg_params[3] & 0xC0C0) | + FIELD_PREP(GENMASK(15, 8), 9 + offsets[0]) | + (14 + offsets[0]); + cfg_results[4] = (cfg_params[4] & 0xC0C0) | + FIELD_PREP(GENMASK(15, 8), 17 + offsets[0]) | + (22 + offsets[0]); + + return lan865x_write_cfg_params(phydev, cfg_results); +} + +static int lan865x_revb0_config_init(struct phy_device *phydev) +{ + int ret; + + /* Reference to AN1760 + * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf + */ + for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) { + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, + lan865x_revb0_fixup_registers[i], + lan865x_revb0_fixup_values[i]); + 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; + + return 0; +} + static int lan867x_revb1_config_init(struct phy_device *phydev) { /* HW quirk: Microchip states in the application note (AN1699) for the phy @@ -105,7 +272,7 @@ static int lan867x_revb1_config_init(struct phy_device *phydev) return 0; } -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 @@ -126,17 +293,28 @@ static struct phy_driver microchip_t1s_driver[] = { .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 = genphy_c45_plca_set_cfg, + .get_plca_status = genphy_c45_plca_get_status, + }, }; 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_LAN865X_REVB0) }, { } };
Add 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. As LAN867X and LAN865X are using the same function for the read_status, rename the function as lan86xx_read_status. Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> --- drivers/net/phy/microchip_t1s.c | 184 +++++++++++++++++++++++++++++++- 1 file changed, 181 insertions(+), 3 deletions(-)