| Message ID | 20231023154649.45931-3-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 |
> + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, true); > + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, false); Just looking at this, it is not clear what these true/false mean. Maybe add some #defines #define TC6_READ true #define TC6_WRITE false #define TC6_PROTECTED true #define TC6_UNPROTECTED false > + if (ret) > + return ret; > + > + /* The chip completes a reset in 3us, we might get here earlier than > + * that, as an added margin we'll conditionally sleep 5us. > + */ > + udelay(5); > + > + ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false); > + if (ret) > + return ret; > + > + /* Check for reset complete interrupt status */ > + if (regval & RESETC) { > + regval = RESETC; People don't always agree, but i found STATUS0_RESETC easier to see you have the correct bit for the register you just read. Andrew
Hi Andrew, On 24/10/23 4:13 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, true); >> + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, false); > > Just looking at this, it is not clear what these true/false mean. Maybe add some #defines > > #define TC6_READ true > #define TC6_WRITE false > #define TC6_PROTECTED true > #define TC6_UNPROTECTED false Sure will do this. > >> + if (ret) >> + return ret; >> + >> + /* The chip completes a reset in 3us, we might get here earlier than >> + * that, as an added margin we'll conditionally sleep 5us. >> + */ >> + udelay(5); >> + >> + ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false); >> + if (ret) >> + return ret; >> + >> + /* Check for reset complete interrupt status */ >> + if (regval & RESETC) { >> + regval = RESETC; > > People don't always agree, but i found STATUS0_RESETC easier to see > you have the correct bit for the register you just read. Do you want me to define STATUS0_RESETC instead of RESETC or is my understanding wrong? Best Regards, Parthiban V > > Andrew
Hi Andrew, On 24/10/23 4:13 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, true); >> + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, false); > > Just looking at this, it is not clear what these true/false mean. Maybe add some #defines > > #define TC6_READ true > #define TC6_WRITE false > #define TC6_PROTECTED true > #define TC6_UNPROTECTED false Sure will do this. > >> + if (ret) >> + return ret; >> + >> + /* The chip completes a reset in 3us, we might get here earlier than >> + * that, as an added margin we'll conditionally sleep 5us. >> + */ >> + udelay(5); >> + >> + ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false); >> + if (ret) >> + return ret; >> + >> + /* Check for reset complete interrupt status */ >> + if (regval & RESETC) { >> + regval = RESETC; > > People don't always agree, but i found STATUS0_RESETC easier to see > you have the correct bit for the register you just read. Do you want me to define STATUS0_RESETC instead of RESETC or is my understanding is wrong? Best Regards, Parthiban V > > Andrew
> >> + ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false); > >> + if (ret) > >> + return ret; > >> + > >> + /* Check for reset complete interrupt status */ > >> + if (regval & RESETC) { > >> + regval = RESETC; > > > > People don't always agree, but i found STATUS0_RESETC easier to see > > you have the correct bit for the register you just read. > Do you want me to define STATUS0_RESETC instead of RESETC or is my > understanding wrong? Correct, STATUS0_RESETC. It avoids silly typos like: ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false); if (ret) return ret; /* Check for reset complete interrupt status */ if (regval & RESET) { regval = RESETC; where RESET is a valid register name, but not a bit. Or say: ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false); if (ret) return ret; /* Check for reset complete interrupt status */ if (regval & SWRESET) { regval = STATUS0_; where SWRESET is a valid bit, but not for STATUS0. I've made silly mistakes like this, and learnt that good naming helps to avoid it. Andrew
Hi Andrew, On 27/10/23 1:31 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>>> + ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + /* Check for reset complete interrupt status */ >>>> + if (regval & RESETC) { >>>> + regval = RESETC; >>> >>> People don't always agree, but i found STATUS0_RESETC easier to see >>> you have the correct bit for the register you just read. >> Do you want me to define STATUS0_RESETC instead of RESETC or is my >> understanding wrong? > > Correct, STATUS0_RESETC. It avoids silly typos like: > > ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false); > if (ret) > return ret; > > /* Check for reset complete interrupt status */ > if (regval & RESET) { > regval = RESETC; > > where RESET is a valid register name, but not a bit. Or say: > > ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false); > if (ret) > return ret; > > /* Check for reset complete interrupt status */ > if (regval & SWRESET) { > regval = STATUS0_; > > where SWRESET is a valid bit, but not for STATUS0. > > I've made silly mistakes like this, and learnt that good naming helps > to avoid it. Ok, thanks for the detailed explanation. Best Regards, Parthiban V > > Andrew
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c index acedc327b05e..e4457569135f 100644 --- a/drivers/net/ethernet/oa_tc6.c +++ b/drivers/net/ethernet/oa_tc6.c @@ -6,6 +6,8 @@ */ #include <linux/bitfield.h> +#include <linux/interrupt.h> +#include <linux/delay.h> #include <linux/oa_tc6.h> /* Opaque structure for MACPHY drivers */ @@ -169,10 +171,15 @@ static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len, if (ret) return ret; - /* Check echoed/received control reply for errors */ - ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, prote); - if (ret) - return ret; + /* In case of reset write, the echoed control command doesn't have any + * valid data. So no need to check for errors. + */ + if (addr != RESET) { + /* Check echoed/received control reply for errors */ + ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, prote); + if (ret) + return ret; + } if (!wnr) { /* Copy read data from the rx data in case of ctrl read */ @@ -191,6 +198,49 @@ static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len, return ret; } +static int oa_tc6_sw_reset(struct oa_tc6 *tc6) +{ + u32 regval; + int ret; + + /* Perform software reset with both protected and unprotected control + * commands because the driver doesn't know the current status of the + * MAC-PHY. + */ + regval = SWRESET; + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, true); + if (ret) + return ret; + + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, false); + if (ret) + return ret; + + /* The chip completes a reset in 3us, we might get here earlier than + * that, as an added margin we'll conditionally sleep 5us. + */ + udelay(5); + + ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false); + if (ret) + return ret; + + /* Check for reset complete interrupt status */ + if (regval & RESETC) { + regval = RESETC; + /* SPI host should write RESETC bit with one to + * clear the reset interrupt status. + */ + ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, true, false); + if (ret) + return ret; + } else { + return -ENODEV; + } + + return 0; +} + /** * oa_tc6_write_register - function for writing a MACPHY register. * @tc6: oa_tc6 struct. @@ -279,6 +329,12 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote) tc6->spi = spi; tc6->prote = prote; + /* Perform MAC-PHY software reset */ + if (oa_tc6_sw_reset(tc6)) { + dev_err(&spi->dev, "MAC-PHY software reset failed\n"); + return NULL; + } + return tc6; } EXPORT_SYMBOL_GPL(oa_tc6_init); diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h index 6284828bda42..8a838499da97 100644 --- a/include/linux/oa_tc6.h +++ b/include/linux/oa_tc6.h @@ -21,6 +21,15 @@ #define TC6_FTR_SIZE 4 /* Ctrl command footer size ss per OA */ #define TC6_CTRL_BUF_SIZE 1032 /* Max ctrl buffer size for 128 regs */ +/* Open Alliance TC6 Standard Control and Status Registers */ +/* Reset Control and Status Register */ +#define RESET 0x0003 +#define SWRESET BIT(0) /* Software Reset */ + +/* Status Register #0 */ +#define STATUS0 0x0008 +#define RESETC BIT(6) /* Reset Complete */ + struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote); int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 val); int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 *val);
Reset complete bit is set when the MAC-PHY reset completes and ready for configuration. When it is set, it will generate a non-maskable interrupt to alert the SPI host. 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 | 64 ++++++++++++++++++++++++++++++++--- include/linux/oa_tc6.h | 9 +++++ 2 files changed, 69 insertions(+), 4 deletions(-)