| Message ID | 20230522113331.36872-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 Mon, May 22, 2023 at 05:03:29PM +0530, Parthiban Veerasooran wrote: > As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete > status bit in the STS2 register to be checked before proceeding for the > initial configuration. Is this the unmaskable interrupt status bit which needs clearing? There is no mention of interrupts here. Andrew
Hi Andrew, On 22/05/23 6:13 pm, Andrew Lunn 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:29PM +0530, Parthiban Veerasooran wrote: >> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete >> status bit in the STS2 register to be checked before proceeding for the >> initial configuration. > > Is this the unmaskable interrupt status bit which needs clearing? Yes, it is non-maskable interrupt. > There is no mention of interrupts here. The device will assert the Reset Complete (RESETC) bit in the Status 2 (STS2) register to indicate that it has completed its internal initialization and is ready for configuration. As the Reset Complete status is non-maskable, the IRQ_N pin will always be asserted and driven low following a device reset. Upon reading of the Status 2 register, the pending Reset Complete status bit will be automatically cleared causing the IRQ_N pin to be released and pulled high again. Do you think it makes sense to add these explanation regarding the reset and interrupt behavior with the above comment for a better understanding? Note: This explanation is pulled from DS-LAN8670-1-2-60001573C.pdf Best Regards, Parthiban V > > Andrew
On Tue, May 23, 2023 at 05:30:06AM +0000, Parthiban.Veerasooran@microchip.com wrote: > Hi Andrew, > > On 22/05/23 6:13 pm, Andrew Lunn 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:29PM +0530, Parthiban Veerasooran wrote: > >> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete > >> status bit in the STS2 register to be checked before proceeding for the > >> initial configuration. > > > > Is this the unmaskable interrupt status bit which needs clearing? > Yes, it is non-maskable interrupt. > > There is no mention of interrupts here. > The device will assert the Reset Complete (RESETC) bit in the Status 2 > (STS2) register to indicate that it has completed its internal > initialization and is ready for configuration. As the Reset Complete > status is non-maskable, the IRQ_N pin will always be asserted and driven > low following a device reset. Upon reading of the Status 2 register, the > pending Reset Complete status bit will be automatically cleared causing > the IRQ_N pin to be released and pulled high again. > > Do you think it makes sense to add these explanation regarding the reset > and interrupt behavior with the above comment for a better understanding? Comments should explain 'Why?'. At the moment, it is not clear why you are reading the status. The discussion so far has been about clearing the interrupt, not about checking it has actually finished its internal reset. So i think you should be mentioning interrupts somewhere. Especially since this is a rather odd behaviour. Andrew
Hi Andrew, On 23/05/23 5:53 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Tue, May 23, 2023 at 05:30:06AM +0000, Parthiban.Veerasooran@microchip.com wrote: >> Hi Andrew, >> >> On 22/05/23 6:13 pm, Andrew Lunn 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:29PM +0530, Parthiban Veerasooran wrote: >>>> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete >>>> status bit in the STS2 register to be checked before proceeding for the >>>> initial configuration. >>> >>> Is this the unmaskable interrupt status bit which needs clearing? >> Yes, it is non-maskable interrupt. >>> There is no mention of interrupts here. >> The device will assert the Reset Complete (RESETC) bit in the Status 2 >> (STS2) register to indicate that it has completed its internal >> initialization and is ready for configuration. As the Reset Complete >> status is non-maskable, the IRQ_N pin will always be asserted and driven >> low following a device reset. Upon reading of the Status 2 register, the >> pending Reset Complete status bit will be automatically cleared causing >> the IRQ_N pin to be released and pulled high again. >> >> Do you think it makes sense to add these explanation regarding the reset >> and interrupt behavior with the above comment for a better understanding? > > Comments should explain 'Why?'. At the moment, it is not clear why you > are reading the status. The discussion so far has been about clearing > the interrupt, not about checking it has actually finished its > internal reset. So i think you should be mentioning interrupts > somewhere. Especially since this is a rather odd behaviour. Shall I update the above commit message like below, As per the datasheet DS-LAN8670-1-2-60001573C.pdf, during the Power ON Reset (POR), the Reset Complete status bit in the STS2 register to be checked before proceeding for the initial configuration. Reading STS2 register will also clear the Reset Complete interrupt which is non-maskable. Or still I have a misunderstanding here? Best Regards, Parthiban V > > Andrew
Hi Andrew, Please ignore the previous reply for this comment and consider this one. On 23/05/23 5:53 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Tue, May 23, 2023 at 05:30:06AM +0000, Parthiban.Veerasooran@microchip.com wrote: >> Hi Andrew, >> >> On 22/05/23 6:13 pm, Andrew Lunn 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:29PM +0530, Parthiban Veerasooran wrote: >>>> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, the Reset Complete >>>> status bit in the STS2 register to be checked before proceeding for the >>>> initial configuration. >>> >>> Is this the unmaskable interrupt status bit which needs clearing? >> Yes, it is non-maskable interrupt. >>> There is no mention of interrupts here. >> The device will assert the Reset Complete (RESETC) bit in the Status 2 >> (STS2) register to indicate that it has completed its internal >> initialization and is ready for configuration. As the Reset Complete >> status is non-maskable, the IRQ_N pin will always be asserted and driven >> low following a device reset. Upon reading of the Status 2 register, the >> pending Reset Complete status bit will be automatically cleared causing >> the IRQ_N pin to be released and pulled high again. >> >> Do you think it makes sense to add these explanation regarding the reset >> and interrupt behavior with the above comment for a better understanding? > > Comments should explain 'Why?'. At the moment, it is not clear why you > are reading the status. The discussion so far has been about clearing > the interrupt, not about checking it has actually finished its > internal reset. So i think you should be mentioning interrupts > somewhere. Especially since this is a rather odd behaviour. Shall I update the above commit message like below? As per the datasheet DS-LAN8670-1-2-60001573C.pdf, during the Power ON Reset(POR)/Hard Reset/Soft Reset, the Reset Complete status bit in the STS2 register to be checked before proceeding for the initial configuration. Reading STS2 register will also clear the Reset Complete interrupt which is non-maskable. Or still I have a misunderstanding here? Best Regards, Parthiban V > > Andrew
> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, during the Power ON > Reset(POR)/Hard Reset/Soft Reset, the Reset Complete status bit in the > STS2 register to be checked before proceeding for the initial 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. Otherwise, this is O.K. Andrew
Hi Andrew, On 24/05/23 5:33 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> As per the datasheet DS-LAN8670-1-2-60001573C.pdf, during the Power ON >> Reset(POR)/Hard Reset/Soft Reset, the Reset Complete status bit in the >> STS2 register to be checked before proceeding for the initial > > 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. > > Otherwise, this is O.K. Thanks for your feedback. I will prepare the next version and send for the review. Best Regards, Parthiban V > > Andrew >
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c index 869c7f403ea1..2f22a1954c09 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 to be checked before proceeding for the initial configuration. Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> --- drivers/net/phy/microchip_t1s.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)