| Message ID | 20241001123734.1667581-3-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 |
On Tue, 1 Oct 2024 18:07:29 +0530 Parthiban Veerasooran wrote: > + cfg_results[0] = FIELD_PREP(GENMASK(15, 10), (9 + offsets[0]) & 0x3F) | > + FIELD_PREP(GENMASK(15, 4), (14 + offsets[0]) & 0x3F) | > + 0x03; > + cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F); It's really strange to OR together FIELD_PREP()s with overlapping fields. What's going on here? 15:10 and 15:4 ranges overlap, then there is 0x3 hardcoded, with no fields size definition. Could you clarify and preferably name as many of the constants as possible? Also why are you masking the result of the sum with 0x3f? Can the result not fit? Is that safe or should we error out? > + ret &= GENMASK(4, 0); ? if (ret & BIT(4)) GENMASK() is nice but naming the fields would be even nicer.. What's 3:0, what's 4:4 ?
Hi Jakub, Thanks for your review comments. On 05/10/24 12:20 am, Jakub Kicinski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Tue, 1 Oct 2024 18:07:29 +0530 Parthiban Veerasooran wrote: >> + cfg_results[0] = FIELD_PREP(GENMASK(15, 10), (9 + offsets[0]) & 0x3F) | >> + FIELD_PREP(GENMASK(15, 4), (14 + offsets[0]) & 0x3F) | >> + 0x03; >> + cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F); > > It's really strange to OR together FIELD_PREP()s with overlapping > fields. What's going on here? 15:10 and 15:4 ranges overlap, then > there is 0x3 hardcoded, with no fields size definition. This calculation has been implemented based on the logic provided in the configuration application note (AN1760) released with the product. Please refer the link [1] below for more info. As mentioned in the AN1760 document, "it provides guidance on how to configure the LAN8650/1 internal PHY for optimal performance in 10BASE-T1S networks." Unfortunately we don't have any other information on those each and every parameters and constants used for the calculation. They are all derived by design team to bring up the device to the nominal state. It is also mentioned as, "The following parameters must be calculated from the device configuration parameters mentioned above to use for the configuration of the registers." uint16 cfgparam1 = (uint16) (((9 + offset1) & 0x3F) << 10) | (uint16) (((14 + offset1) & 0x3F) << 4) | 0x03 uint16 cfgparam2 = (uint16) (((40 + offset2) & 0x3F) << 10) This is the reason why the above logic has been implemented. > Could you clarify and preferably name as many of the constants > as possible? I would like to do that but as I mentioned above there is no info on those constants in the application note. > > Also why are you masking the result of the sum with 0x3f? > Can the result not fit? Is that safe or should we error out? Hope the above info clarifies this as well. > >> + ret &= GENMASK(4, 0); > ? if (ret & BIT(4)) > > GENMASK() is nice but naming the fields would be even nicer.. > What's 3:0, what's 4:4 ? As per the information provided in the application note, the offset value expected range is from -5 to 15. Offsets are stored as signed 5-bit values in the addresses 0x04 and 0x08. So 0x1F is used to mask the 5-bit value and if the 4th bit is set then the value from 27 to 31 will be considered as -ve value from -5 to -1. I think adding the above comment in the above code snippet will clarify the need. What do you think? Link: [1]: https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ApplicationNotes/ApplicationNotes/LAN8650-1-Configuration-Appnote-60001760.pdf Best regards, Parthiban V > -- > pw-bot: cr
On Mon, 7 Oct 2024 07:51:36 +0000 Parthiban.Veerasooran@microchip.com wrote: > On 05/10/24 12:20 am, Jakub Kicinski wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Tue, 1 Oct 2024 18:07:29 +0530 Parthiban Veerasooran wrote: > >> + cfg_results[0] = FIELD_PREP(GENMASK(15, 10), (9 + offsets[0]) & 0x3F) | > >> + FIELD_PREP(GENMASK(15, 4), (14 + offsets[0]) & 0x3F) | > >> + 0x03; > >> + cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F); > > > > It's really strange to OR together FIELD_PREP()s with overlapping > > fields. What's going on here? 15:10 and 15:4 ranges overlap, then > > there is 0x3 hardcoded, with no fields size definition. > This calculation has been implemented based on the logic provided in the > configuration application note (AN1760) released with the product. > Please refer the link [1] below for more info. > > As mentioned in the AN1760 document, "it provides guidance on how to > configure the LAN8650/1 internal PHY for optimal performance in > 10BASE-T1S networks." Unfortunately we don't have any other information > on those each and every parameters and constants used for the > calculation. They are all derived by design team to bring up the device > to the nominal state. > > It is also mentioned as, "The following parameters must be calculated > from the device configuration parameters mentioned above to use for the > configuration of the registers." > > uint16 cfgparam1 = (uint16) (((9 + offset1) & 0x3F) << 10) | (uint16) > (((14 + offset1) & 0x3F) << 4) | 0x03 > uint16 cfgparam2 = (uint16) (((40 + offset2) & 0x3F) << 10) > > This is the reason why the above logic has been implemented. In this case the code should simply be: cfg_results[0] = FIELD_PREP(GENMASK(15, 10), 9 + offsets[0]) | FIELD_PREP(GENMASK(9, 4), 14 + offsets[0]) | the fields are clearly 6b each. FILED_PREP() already masks. > > Could you clarify and preferably name as many of the constants > > as possible? > I would like to do that but as I mentioned above there is no info on > those constants in the application note. > > > > Also why are you masking the result of the sum with 0x3f? > > Can the result not fit? Is that safe or should we error out? > Hope the above info clarifies this as well. > > > >> + ret &= GENMASK(4, 0); > > ? if (ret & BIT(4)) > > > > GENMASK() is nice but naming the fields would be even nicer.. > > What's 3:0, what's 4:4 ? > As per the information provided in the application note, the offset > value expected range is from -5 to 15. Offsets are stored as signed > 5-bit values in the addresses 0x04 and 0x08. So 0x1F is used to mask the > 5-bit value and if the 4th bit is set then the value from 27 to 31 will > be considered as -ve value from -5 to -1. > > I think adding the above comment in the above code snippet will clarify > the need. What do you think? Oh yes, a comment, e.g. /* 5-bit signed value, sign extend */ would help a lot, thanks!
Hi Jakub, On 07/10/24 9:30 pm, Jakub Kicinski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, 7 Oct 2024 07:51:36 +0000 Parthiban.Veerasooran@microchip.com > wrote: >> On 05/10/24 12:20 am, Jakub Kicinski wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On Tue, 1 Oct 2024 18:07:29 +0530 Parthiban Veerasooran wrote: >>>> + cfg_results[0] = FIELD_PREP(GENMASK(15, 10), (9 + offsets[0]) & 0x3F) | >>>> + FIELD_PREP(GENMASK(15, 4), (14 + offsets[0]) & 0x3F) | >>>> + 0x03; >>>> + cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F); >>> >>> It's really strange to OR together FIELD_PREP()s with overlapping >>> fields. What's going on here? 15:10 and 15:4 ranges overlap, then >>> there is 0x3 hardcoded, with no fields size definition. >> This calculation has been implemented based on the logic provided in the >> configuration application note (AN1760) released with the product. >> Please refer the link [1] below for more info. >> >> As mentioned in the AN1760 document, "it provides guidance on how to >> configure the LAN8650/1 internal PHY for optimal performance in >> 10BASE-T1S networks." Unfortunately we don't have any other information >> on those each and every parameters and constants used for the >> calculation. They are all derived by design team to bring up the device >> to the nominal state. >> >> It is also mentioned as, "The following parameters must be calculated >> from the device configuration parameters mentioned above to use for the >> configuration of the registers." >> >> uint16 cfgparam1 = (uint16) (((9 + offset1) & 0x3F) << 10) | (uint16) >> (((14 + offset1) & 0x3F) << 4) | 0x03 >> uint16 cfgparam2 = (uint16) (((40 + offset2) & 0x3F) << 10) >> >> This is the reason why the above logic has been implemented. > > In this case the code should simply be: > > cfg_results[0] = FIELD_PREP(GENMASK(15, 10), 9 + offsets[0]) | > FIELD_PREP(GENMASK(9, 4), 14 + offsets[0]) | > > the fields are clearly 6b each. FILED_PREP() already masks. Ah ok, thanks for the input. I will take care in other places as well. > >>> Could you clarify and preferably name as many of the constants >>> as possible? >> I would like to do that but as I mentioned above there is no info on >> those constants in the application note. >>> >>> Also why are you masking the result of the sum with 0x3f? >>> Can the result not fit? Is that safe or should we error out? >> Hope the above info clarifies this as well. >>> >>>> + ret &= GENMASK(4, 0); >>> ? if (ret & BIT(4)) >>> >>> GENMASK() is nice but naming the fields would be even nicer.. >>> What's 3:0, what's 4:4 ? >> As per the information provided in the application note, the offset >> value expected range is from -5 to 15. Offsets are stored as signed >> 5-bit values in the addresses 0x04 and 0x08. So 0x1F is used to mask the >> 5-bit value and if the 4th bit is set then the value from 27 to 31 will >> be considered as -ve value from -5 to -1. >> >> I think adding the above comment in the above code snippet will clarify >> the need. What do you think? > > Oh yes, a comment, e.g. /* 5-bit signed value, sign extend */ > would help a lot, thanks! Sure, I will add this comment in the code snippet. Thanks. Best regards, Parthiban V
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c index 24f992aba7d7..37eb89da08d6 100644 --- a/drivers/net/phy/microchip_t1s.c +++ b/drivers/net/phy/microchip_t1s.c @@ -59,29 +59,45 @@ 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, +/* LAN865x Rev.B0 configuration parameters from AN1760 + * As per the Configuration Application Note AN1760 published in the below link, + * https://www.microchip.com/en-us/application-notes/an1760 + * Revision F (DS60001760G - June 2024) + */ +static const u32 lan865x_revb0_fixup_registers[17] = { + 0x00D0, 0x00E0, 0x00E9, 0x00F5, + 0x00F4, 0x00F8, 0x00F9, 0x0081, + 0x0091, 0x0043, 0x0044, 0x0045, + 0x0053, 0x0054, 0x0055, 0x0040, + 0x0050, +}; + +static const u16 lan865x_revb0_fixup_values[17] = { + 0x3F31, 0xC000, 0x9E50, 0x1CF8, + 0xC020, 0xB900, 0x4E53, 0x0080, + 0x9660, 0x00FF, 0xFFFF, 0x0000, + 0x00FF, 0xFFFF, 0x0000, 0x0002, + 0x0002, +}; + +static const u16 lan865x_revb0_fixup_cfg_regs[2] = { + 0x0084, 0x008A, +}; + +static const u32 lan865x_revb0_sqi_fixup_regs[12] = { 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, +static const u16 lan865x_revb0_sqi_fixup_values[12] = { 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 +static const u16 lan865x_revb0_sqi_fixup_cfg_regs[3] = { + 0x00AD, 0x00AE, 0x00AF, }; /* Pulled from AN1760 describing 'indirect read' @@ -121,6 +137,8 @@ static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[]) ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]); if (ret < 0) return ret; + + ret &= GENMASK(4, 0); if (ret & BIT(4)) offsets[i] = ret | 0xE0; else @@ -163,59 +181,88 @@ static int lan865x_write_cfg_params(struct phy_device *phydev, return 0; } -static int lan865x_setup_cfgparam(struct phy_device *phydev) +static int lan865x_setup_cfgparam(struct phy_device *phydev, s8 offsets[]) { u16 cfg_results[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)]; u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)]; - s8 offsets[2]; int ret; - ret = lan865x_generate_cfg_offsets(phydev, offsets); + ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs, + cfg_params, ARRAY_SIZE(cfg_params)); if (ret) return ret; - ret = lan865x_read_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs, + cfg_results[0] = FIELD_PREP(GENMASK(15, 10), (9 + offsets[0]) & 0x3F) | + FIELD_PREP(GENMASK(15, 4), (14 + offsets[0]) & 0x3F) | + 0x03; + cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F); + + return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs, + cfg_results, ARRAY_SIZE(cfg_results)); +} + +static int lan865x_setup_sqi_cfgparam(struct phy_device *phydev, s8 offsets[]) +{ + u16 cfg_results[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)]; + u16 cfg_params[ARRAY_SIZE(lan865x_revb0_sqi_fixup_cfg_regs)]; + int ret; + + ret = lan865x_read_cfg_params(phydev, lan865x_revb0_sqi_fixup_cfg_regs, cfg_params, ARRAY_SIZE(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]); + cfg_results[0] = FIELD_PREP(GENMASK(15, 8), (5 + offsets[0]) & 0x3F) | + ((9 + offsets[0]) & 0x3F); + cfg_results[1] = FIELD_PREP(GENMASK(15, 8), (9 + offsets[0]) & 0x3F) | + ((14 + offsets[0]) & 0x3F); + cfg_results[2] = FIELD_PREP(GENMASK(15, 8), (17 + offsets[0]) & 0x3F) | + ((22 + offsets[0]) & 0x3F); - return lan865x_write_cfg_params(phydev, lan865x_revb0_fixup_cfg_regs, + return lan865x_write_cfg_params(phydev, + lan865x_revb0_sqi_fixup_cfg_regs, cfg_results, ARRAY_SIZE(cfg_results)); } static int lan865x_revb0_config_init(struct phy_device *phydev) { + s8 offsets[2]; int ret; /* Reference to AN1760 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf */ + ret = lan865x_generate_cfg_offsets(phydev, offsets); + if (ret) + return ret; + 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; + + if (i == 1) { + ret = lan865x_setup_cfgparam(phydev, offsets); + if (ret) + return ret; + } } - /* Function to calculate and write the configuration parameters in the - * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760) - */ - return lan865x_setup_cfgparam(phydev); + + ret = lan865x_setup_sqi_cfgparam(phydev, offsets); + if (ret) + return ret; + + for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_sqi_fixup_regs); i++) { + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, + lan865x_revb0_sqi_fixup_regs[i], + lan865x_revb0_sqi_fixup_values[i]); + if (ret) + return ret; + } + + return 0; } static int lan867x_revb1_config_init(struct phy_device *phydev)
Update the new/improved initial settings from the latest configuration application note AN1760 released for LAN8650/1 Rev.B0 Revision F (DS60001760G - June 2024). https://www.microchip.com/en-us/application-notes/an1760 Signed-off-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com> --- drivers/net/phy/microchip_t1s.c | 119 ++++++++++++++++++++++---------- 1 file changed, 83 insertions(+), 36 deletions(-)