| Message ID | 20230524144539.62618-5-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 |
On Wed, May 24, 2023 at 08:15:37PM +0530, Parthiban Veerasooran wrote: > As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete > status bit in the STS2 register has to be checked before proceeding to > the initial configuration. Reading STS2 register will also clear the > Reset Complete interrupt which is non-maskable. > > Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
> + /* Read STS2 register and check for the Reset Complete status to do the > + * init configuration. If the Reset Complete is not set, wait for 5us > + * and then read STS2 register again and check for Reset Complete status. > + * Still if it is failed then declare PHY reset error or else proceed > + * for the PHY initial register configuration. > + */ > + err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2); > + if (err < 0) > + return err; > + > + if (!(err & LAN867x_RESET_COMPLETE_STS)) { > + udelay(5); > + err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2); > + if (err < 0) > + return err; > + if (!(err & LAN867x_RESET_COMPLETE_STS)) { > + phydev_err(phydev, "PHY reset failed\n"); > + return -ENODEV; > + } > + } This comment explains exactly what the code does, which is also obvious from reading the code. A meaningful comment would be explaining why the state can change 5us later.
Hi Ramon, On 25/05/23 11:56 pm, Ramón Nordin Rodriguez wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> + /* Read STS2 register and check for the Reset Complete status to do the >> + * init configuration. If the Reset Complete is not set, wait for 5us >> + * and then read STS2 register again and check for Reset Complete status. >> + * Still if it is failed then declare PHY reset error or else proceed >> + * for the PHY initial register configuration. >> + */ >> + err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2); >> + if (err < 0) >> + return err; >> + >> + if (!(err & LAN867x_RESET_COMPLETE_STS)) { >> + udelay(5); >> + err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2); >> + if (err < 0) >> + return err; >> + if (!(err & LAN867x_RESET_COMPLETE_STS)) { >> + phydev_err(phydev, "PHY reset failed\n"); >> + return -ENODEV; >> + } >> + } > > This comment explains exactly what the code does, which is also obvious > from reading the code. A meaningful comment would be explaining why the > state can change 5us later. > As per design, LAN867x reset to be completed by 3us. Just for a safer side it is recommended to use 5us. With the assumption of more than 3us completion, the first read checks for the Reset Complete. If the config_init is more faster, then once again checks for it after 5us. As you mentioned, can we remove the existing block comment as it explains the code and add the above comment to explain 5us delay. What is your opinion on this proposal? Best Regards, Parthiban V
On Fri, May 26, 2023 at 06:00:08AM +0000, Parthiban.Veerasooran@microchip.com wrote: > Hi Ramon, > >> + /* Read STS2 register and check for the Reset Complete status to do the > >> + * init configuration. If the Reset Complete is not set, wait for 5us > >> + * and then read STS2 register again and check for Reset Complete status. > >> + * Still if it is failed then declare PHY reset error or else proceed > >> + * for the PHY initial register configuration. > >> + */ > > > > This comment explains exactly what the code does, which is also obvious > > from reading the code. A meaningful comment would be explaining why the > > state can change 5us later. > > > As per design, LAN867x reset to be completed by 3us. Just for a safer > side it is recommended to use 5us. With the assumption of more than 3us > completion, the first read checks for the Reset Complete. If the > config_init is more faster, then once again checks for it after 5us. > > As you mentioned, can we remove the existing block comment as it > explains the code and add the above comment to explain 5us delay. > What is your opinion on this proposal? > > Best Regards, > Parthiban V > I'd suggest the following /*The chip completes a reset in 3us, we might get here earlier than that, as an added margin we'll conditionally sleep 5us*/
Hi Ramon, On 26/05/23 12:44 pm, Ramón Nordin Rodriguez wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Fri, May 26, 2023 at 06:00:08AM +0000, Parthiban.Veerasooran@microchip.com wrote: >> Hi Ramon, >>>> + /* Read STS2 register and check for the Reset Complete status to do the >>>> + * init configuration. If the Reset Complete is not set, wait for 5us >>>> + * and then read STS2 register again and check for Reset Complete status. >>>> + * Still if it is failed then declare PHY reset error or else proceed >>>> + * for the PHY initial register configuration. >>>> + */ >>> >>> This comment explains exactly what the code does, which is also obvious >>> from reading the code. A meaningful comment would be explaining why the >>> state can change 5us later. >>> >> As per design, LAN867x reset to be completed by 3us. Just for a safer >> side it is recommended to use 5us. With the assumption of more than 3us >> completion, the first read checks for the Reset Complete. If the >> config_init is more faster, then once again checks for it after 5us. >> >> As you mentioned, can we remove the existing block comment as it >> explains the code and add the above comment to explain 5us delay. >> What is your opinion on this proposal? >> >> Best Regards, >> Parthiban V >> > > I'd suggest the following > /*The chip completes a reset in 3us, we might get here earlier than that, > as an added margin we'll conditionally sleep 5us*/ Ok I will use this proposal in the next version. Best Regards, Parthiban V
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c index 8f29d9802131..05a88b561993 100644 --- a/drivers/net/phy/microchip_t1s.c +++ b/drivers/net/phy/microchip_t1s.c @@ -14,6 +14,9 @@ #define LAN867X_REG_IRQ_1_CTL 0x001C #define LAN867X_REG_IRQ_2_CTL 0x001D +#define LAN867X_REG_STS2 0x0019 + +#define LAN867x_RESET_COMPLETE_STS BIT(11) /* The arrays below are pulled from the following table from AN1699 * Access MMD Address Value Mask @@ -65,6 +68,27 @@ static int lan867x_revb1_config_init(struct phy_device *phydev) int err; + /* Read STS2 register and check for the Reset Complete status to do the + * init configuration. If the Reset Complete is not set, wait for 5us + * and then read STS2 register again and check for Reset Complete status. + * Still if it is failed then declare PHY reset error or else proceed + * for the PHY initial register configuration. + */ + err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2); + if (err < 0) + return err; + + if (!(err & LAN867x_RESET_COMPLETE_STS)) { + udelay(5); + err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2); + if (err < 0) + return err; + if (!(err & LAN867x_RESET_COMPLETE_STS)) { + phydev_err(phydev, "PHY reset failed\n"); + return -ENODEV; + } + } + /* Read-Modified Write Pseudocode (from AN1699) * current_val = read_register(mmd, addr) // Read current register value * new_val = current_val AND (NOT mask) // Clear bit fields to be written
As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete status bit in the STS2 register has to be checked before proceeding to the initial configuration. Reading STS2 register will also clear the Reset Complete interrupt which is non-maskable. Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> --- drivers/net/phy/microchip_t1s.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)