| Message ID | 20240306085017.21731-5-Parthiban.Veerasooran@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface | expand |
> +/* Status Register #0 */ > +#define OA_TC6_REG_STATUS0 0x0008 > +#define STATUS0_RESETC BIT(6) /* Reset Complete */ > + > /* Control command header */ > #define OA_TC6_CTRL_HEADER_DATA_NOT_CTRL BIT(31) > #define OA_TC6_CTRL_HEADER_WRITE BIT(29) > @@ -24,6 +34,8 @@ > (OA_TC6_CTRL_MAX_REGISTERS *\ > OA_TC6_CTRL_REG_VALUE_SIZE) +\ > OA_TC6_CTRL_IGNORED_SIZE) > +#define STATUS0_RESETC_POLL_DELAY 5 > +#define STATUS0_RESETC_POLL_TIMEOUT 100 > > /* Internal structure for MAC-PHY drivers */ > struct oa_tc6 { > @@ -279,6 +291,39 @@ int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value) > } > EXPORT_SYMBOL_GPL(oa_tc6_write_register); > > +static int oa_tc6_read_sw_reset_status(struct oa_tc6 *tc6) > +{ > + u32 regval; > + int ret; > + > + ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, ®val); > + if (ret) > + return 0; > + > + return regval; The function name does not really fit what the function does. The function returns OA_TC6_REG_STATUS0. I assume it has more bits in it than just STATUS0_RESETC. So either this function should be called oa_tc6_read_status0, or you should mask regval with STATUS0_RESETC, so that it does actually return the sw reset status. > +static int oa_tc6_sw_reset_macphy(struct oa_tc6 *tc6) > +{ > + u32 regval = RESET_SWRESET; > + int ret; > + > + ret = oa_tc6_write_register(tc6, OA_TC6_REG_RESET, regval); > + if (ret) > + return ret; > + > + /* Poll for soft reset complete for every 5us until 100us timeout */ Is this 100us defined in the standard? It is pretty short. If it is not part of the standard, maybe set it to something much bigger? Also, polling every 5us is pretty quick. I doubt most systems can even sleep that short a time. So maybe 1ms between polls, and 1 second before -ETIMEDOUT? > + ret = readx_poll_timeout(oa_tc6_read_sw_reset_status, tc6, regval, > + regval & STATUS0_RESETC, > + STATUS0_RESETC_POLL_DELAY, > + STATUS0_RESETC_POLL_TIMEOUT); > + if (ret) > + return -ENODEV; > + > + /* Clear the reset complete status */ > + return oa_tc6_write_register(tc6, OA_TC6_REG_STATUS0, regval); Andrew
Hi Andrew, On 07/03/24 6:05 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> +/* Status Register #0 */ >> +#define OA_TC6_REG_STATUS0 0x0008 >> +#define STATUS0_RESETC BIT(6) /* Reset Complete */ >> + >> /* Control command header */ >> #define OA_TC6_CTRL_HEADER_DATA_NOT_CTRL BIT(31) >> #define OA_TC6_CTRL_HEADER_WRITE BIT(29) >> @@ -24,6 +34,8 @@ >> (OA_TC6_CTRL_MAX_REGISTERS *\ >> OA_TC6_CTRL_REG_VALUE_SIZE) +\ >> OA_TC6_CTRL_IGNORED_SIZE) >> +#define STATUS0_RESETC_POLL_DELAY 5 >> +#define STATUS0_RESETC_POLL_TIMEOUT 100 >> >> /* Internal structure for MAC-PHY drivers */ >> struct oa_tc6 { >> @@ -279,6 +291,39 @@ int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value) >> } >> EXPORT_SYMBOL_GPL(oa_tc6_write_register); >> >> +static int oa_tc6_read_sw_reset_status(struct oa_tc6 *tc6) >> +{ >> + u32 regval; >> + int ret; >> + >> + ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, ®val); >> + if (ret) >> + return 0; >> + >> + return regval; > > The function name does not really fit what the function does. The > function returns OA_TC6_REG_STATUS0. I assume it has more bits in it > than just STATUS0_RESETC. So either this function should be called > oa_tc6_read_status0, or you should mask regval with STATUS0_RESETC, so > that it does actually return the sw reset status. Ok, as we do the masking in the calling function readx_poll_timeout, as you suggested I will change the function name as oa_tc6_read_status0 in the next version. > >> +static int oa_tc6_sw_reset_macphy(struct oa_tc6 *tc6) >> +{ >> + u32 regval = RESET_SWRESET; >> + int ret; >> + >> + ret = oa_tc6_write_register(tc6, OA_TC6_REG_RESET, regval); >> + if (ret) >> + return ret; >> + >> + /* Poll for soft reset complete for every 5us until 100us timeout */ > > Is this 100us defined in the standard? It is pretty short. If it is > not part of the standard, maybe set it to something much bigger? No, the standard doesn't define this and it is not part of the standard. > > Also, polling every 5us is pretty quick. I doubt most systems can even > sleep that short a time. So maybe 1ms between polls, and 1 second > before -ETIMEDOUT? Ok, I agree with your proposal and will change the poll delay as 1ms with the poll timeout of 1 second. Best regards, Parthiban V > >> + ret = readx_poll_timeout(oa_tc6_read_sw_reset_status, tc6, regval, >> + regval & STATUS0_RESETC, >> + STATUS0_RESETC_POLL_DELAY, >> + STATUS0_RESETC_POLL_TIMEOUT); >> + if (ret) >> + return -ENODEV; >> + >> + /* Clear the reset complete status */ >> + return oa_tc6_write_register(tc6, OA_TC6_REG_STATUS0, regval); > > Andrew >
> >> +static int oa_tc6_read_sw_reset_status(struct oa_tc6 *tc6) > >> +{ > >> + u32 regval; > >> + int ret; > >> + > >> + ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, ®val); > >> + if (ret) > >> + return 0; > >> + > >> + return regval; > > > > The function name does not really fit what the function does. The > > function returns OA_TC6_REG_STATUS0. I assume it has more bits in it > > than just STATUS0_RESETC. So either this function should be called > > oa_tc6_read_status0, or you should mask regval with STATUS0_RESETC, so > > that it does actually return the sw reset status. > Ok, as we do the masking in the calling function readx_poll_timeout, as > you suggested I will change the function name as oa_tc6_read_status0 in > the next version. O.K. And i think some of the later patches can them make use of this generic function. Andrew
Hi Andrew, On 07/03/24 6:54 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>>> +static int oa_tc6_read_sw_reset_status(struct oa_tc6 *tc6) >>>> +{ >>>> + u32 regval; >>>> + int ret; >>>> + >>>> + ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, ®val); >>>> + if (ret) >>>> + return 0; >>>> + >>>> + return regval; >>> >>> The function name does not really fit what the function does. The >>> function returns OA_TC6_REG_STATUS0. I assume it has more bits in it >>> than just STATUS0_RESETC. So either this function should be called >>> oa_tc6_read_status0, or you should mask regval with STATUS0_RESETC, so >>> that it does actually return the sw reset status. >> Ok, as we do the masking in the calling function readx_poll_timeout, as >> you suggested I will change the function name as oa_tc6_read_status0 in >> the next version. > > O.K. And i think some of the later patches can them make use of this > generic function. Yes, might be helpful. Best regards, Parthiban V > > Andrew
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c index 35e377577ba4..e9ddc4ff7d0d 100644 --- a/drivers/net/ethernet/oa_tc6.c +++ b/drivers/net/ethernet/oa_tc6.c @@ -6,8 +6,18 @@ */ #include <linux/bitfield.h> +#include <linux/iopoll.h> #include <linux/oa_tc6.h> +/* OPEN Alliance TC6 registers */ +/* Reset Control and Status Register */ +#define OA_TC6_REG_RESET 0x0003 +#define RESET_SWRESET BIT(0) /* Software Reset */ + +/* Status Register #0 */ +#define OA_TC6_REG_STATUS0 0x0008 +#define STATUS0_RESETC BIT(6) /* Reset Complete */ + /* Control command header */ #define OA_TC6_CTRL_HEADER_DATA_NOT_CTRL BIT(31) #define OA_TC6_CTRL_HEADER_WRITE BIT(29) @@ -24,6 +34,8 @@ (OA_TC6_CTRL_MAX_REGISTERS *\ OA_TC6_CTRL_REG_VALUE_SIZE) +\ OA_TC6_CTRL_IGNORED_SIZE) +#define STATUS0_RESETC_POLL_DELAY 5 +#define STATUS0_RESETC_POLL_TIMEOUT 100 /* Internal structure for MAC-PHY drivers */ struct oa_tc6 { @@ -279,6 +291,39 @@ int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value) } EXPORT_SYMBOL_GPL(oa_tc6_write_register); +static int oa_tc6_read_sw_reset_status(struct oa_tc6 *tc6) +{ + u32 regval; + int ret; + + ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, ®val); + if (ret) + return 0; + + return regval; +} + +static int oa_tc6_sw_reset_macphy(struct oa_tc6 *tc6) +{ + u32 regval = RESET_SWRESET; + int ret; + + ret = oa_tc6_write_register(tc6, OA_TC6_REG_RESET, regval); + if (ret) + return ret; + + /* Poll for soft reset complete for every 5us until 100us timeout */ + ret = readx_poll_timeout(oa_tc6_read_sw_reset_status, tc6, regval, + regval & STATUS0_RESETC, + STATUS0_RESETC_POLL_DELAY, + STATUS0_RESETC_POLL_TIMEOUT); + if (ret) + return -ENODEV; + + /* Clear the reset complete status */ + return oa_tc6_write_register(tc6, OA_TC6_REG_STATUS0, regval); +} + /** * oa_tc6_init - allocates and initializes oa_tc6 structure. * @spi: device with which data will be exchanged. @@ -289,6 +334,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_write_register); struct oa_tc6 *oa_tc6_init(struct spi_device *spi) { struct oa_tc6 *tc6; + int ret; tc6 = devm_kzalloc(&spi->dev, sizeof(*tc6), GFP_KERNEL); if (!tc6) @@ -311,6 +357,13 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi) if (!tc6->spi_ctrl_rx_buf) return NULL; + ret = oa_tc6_sw_reset_macphy(tc6); + if (ret) { + dev_err(&tc6->spi->dev, + "MAC-PHY software reset failed: %d\n", ret); + return NULL; + } + return tc6; } EXPORT_SYMBOL_GPL(oa_tc6_init);
Reset complete bit is set when the MAC-PHY reset completes and ready for configuration. Additionally reset complete bit in the STS0 register has to be written by one upon reset complete to clear the interrupt. Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> --- drivers/net/ethernet/oa_tc6.c | 53 +++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)