| Message ID | 20230908142919.14849-3-Parthiban.Veerasooran@microchip.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface | expand |
> + /* Register MAC-PHY interrupt service routine */ > + ret = devm_request_irq(&spi->dev, spi->irq, macphy_irq, 0, "macphy int", > + tc6); It looks like using threaded interrupts could save a lot of complexity. Let the IRQ core handle all the tasklet stuff for you. Andrew
> Register MAC-PHY interrupt and handle reset complete interrupt. Reset > complete bit is set when the MAC-PHY reset complete 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 | 141 ++++++++++++++++++++++++++++++++-- > include/linux/oa_tc6.h | 16 +++- > 2 files changed, 150 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c > index 613cf034430a..0019f70345b6 100644 > --- a/drivers/net/ethernet/oa_tc6.c > +++ b/drivers/net/ethernet/oa_tc6.c > @@ -6,6 +6,7 @@ > */ > > #include <linux/bitfield.h> > +#include <linux/interrupt.h> > #include <linux/oa_tc6.h> > > static int oa_tc6_spi_transfer(struct spi_device *spi, u8 *ptx, u8 *prx, > @@ -160,10 +161,16 @@ int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len, > if (ret) > goto err_ctrl; > > - /* Check echoed/received control reply */ > - ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, ctrl_prot); > - if (ret) > - goto err_ctrl; > + /* In case of reset write, the echoed control command doesn't have any > + * valid data. So no need to check for error. > + */ > + if (addr != OA_TC6_RESET) { > + /* Check echoed/received control reply */ > + ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, > + ctrl_prot); > + if (ret) > + goto err_ctrl; > + } > > if (!wnr) { > /* Copy read data from the rx data in case of ctrl read */ > @@ -186,6 +193,88 @@ int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len, > return ret; > } > > +static int oa_tc6_handler(void *data) > +{ > + struct oa_tc6 *tc6 = data; > + u32 regval; > + int ret; > + > + while (likely(!kthread_should_stop())) { > + wait_event_interruptible(tc6->tc6_wq, tc6->int_flag || > + kthread_should_stop()); > + if (tc6->int_flag) { > + tc6->int_flag = false; > + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_STS0, ®val, 1, > + false, false); > + if (ret) { > + dev_err(&tc6->spi->dev, "Failed to read STS0\n"); > + continue; > + } > + /* 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, OA_TC6_STS0, > + ®val, 1, true, > + false); > + if (ret) { > + dev_err(&tc6->spi->dev, > + "Failed to write STS0\n"); > + continue; > + } > + complete(&tc6->rst_complete); > + } > + } > + } > + return 0; > +} > + > +static irqreturn_t macphy_irq(int irq, void *dev_id) > +{ > + struct oa_tc6 *tc6 = dev_id; > + > + /* Wake tc6 task to perform interrupt action */ > + tc6->int_flag = true; > + wake_up_interruptible(&tc6->tc6_wq); > + > + return IRQ_HANDLED; > +} > + > +static int oa_tc6_sw_reset(struct oa_tc6 *tc6) > +{ > + long timeleft; > + 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 = SW_RESET; > + reinit_completion(&tc6->rst_complete); > + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, true, false); > + if (ret) { > + dev_err(&tc6->spi->dev, "RESET register write failed\n"); > + return ret; > + } > + > + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, true, true); > + if (ret) { > + dev_err(&tc6->spi->dev, "RESET register write failed\n"); > + return ret; > + } > + timeleft = wait_for_completion_interruptible_timeout(&tc6->rst_complete, > + msecs_to_jiffies(1)); > + if (timeleft <= 0) { > + dev_err(&tc6->spi->dev, "MAC-PHY reset failed\n"); > + return -ENODEV; > + } > + > + return 0; > +} > + > int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len) > { > return oa_tc6_perform_ctrl(tc6, addr, val, len, true, tc6->ctrl_prot); > @@ -201,6 +290,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_read_register); > struct oa_tc6 *oa_tc6_init(struct spi_device *spi) > { > struct oa_tc6 *tc6; > + int ret; > > if (!spi) > return NULL; > @@ -211,12 +301,51 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi) > > tc6->spi = spi; > > + /* Used for triggering the OA TC6 task */ > + init_waitqueue_head(&tc6->tc6_wq); > + > + init_completion(&tc6->rst_complete); > + > + /* This task performs the SPI transfer */ > + tc6->tc6_task = kthread_run(oa_tc6_handler, tc6, "OA TC6 Task"); > + if (IS_ERR(tc6->tc6_task)) > + goto err_tc6_task; > + > + /* Set the highest priority to the tc6 task as it is time critical */ > + sched_set_fifo(tc6->tc6_task); > + > + /* Register MAC-PHY interrupt service routine */ > + ret = devm_request_irq(&spi->dev, spi->irq, macphy_irq, 0, "macphy int", > + tc6); > + if ((ret != -ENOTCONN) && ret < 0) { > + dev_err(&spi->dev, "Error attaching macphy irq %d\n", ret); > + goto err_macphy_irq; > + } > + > + /* Perform MAC-PHY software reset */ > + if (oa_tc6_sw_reset(tc6)) > + goto err_macphy_reset; > + > return tc6; > + > +err_macphy_reset: > + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6); > +err_macphy_irq: > + kthread_stop(tc6->tc6_task); > +err_tc6_task: > + kfree(tc6); > + return NULL; > } > EXPORT_SYMBOL_GPL(oa_tc6_init); > > -void oa_tc6_deinit(struct oa_tc6 *tc6) > +int oa_tc6_deinit(struct oa_tc6 *tc6) > { > - kfree(tc6); > + int ret; > + > + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6); > + ret = kthread_stop(tc6->tc6_task); kthread_stop() will the result of threadfn(). Here mean that if threadfn() return non-zero, deinit() will fail. But the KTHREAD_SHOULD_STOP already be set. And oa_tc6_handler() will end. Please check it is what you want. > + if (!ret) > + kfree(tc6); > + return ret; > } > EXPORT_SYMBOL_GPL(oa_tc6_deinit); > diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h > index 5e0a58ab1dcd..315f061c2dfe 100644 > --- a/include/linux/oa_tc6.h > +++ b/include/linux/oa_tc6.h > @@ -17,15 +17,29 @@ > #define CTRL_HDR_LEN GENMASK(7, 1) /* Length */ > #define CTRL_HDR_P BIT(0) /* Parity Bit */ > > +/* Open Alliance TC6 Standard Control and Status Registers */ > +#define OA_TC6_RESET 0x0003 /* Reset Control and Status Register */ > +#define OA_TC6_STS0 0x0008 /* Status Register #0 */ > + > +/* RESET register field */ > +#define SW_RESET BIT(0) /* Software Reset */ > + > +/* STATUS0 register field */ > +#define RESETC BIT(6) /* Reset Complete */ > + > #define TC6_HDR_SIZE 4 /* Ctrl command header size as per OA */ > #define TC6_FTR_SIZE 4 /* Ctrl command footer size ss per OA */ > > struct oa_tc6 { > struct spi_device *spi; > bool ctrl_prot; > + struct task_struct *tc6_task; > + wait_queue_head_t tc6_wq; > + bool int_flag; > + struct completion rst_complete; > }; > > struct oa_tc6 *oa_tc6_init(struct spi_device *spi); > -void oa_tc6_deinit(struct oa_tc6 *tc6); > +int oa_tc6_deinit(struct oa_tc6 *tc6); > int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 value[], u8 len); > int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 value[], u8 len); >
> > +int oa_tc6_deinit(struct oa_tc6 *tc6) > > { > > - kfree(tc6); > > + int ret; > > + > > + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6); > > + ret = kthread_stop(tc6->tc6_task); > > kthread_stop() will the result of threadfn(). Here mean that if threadfn() > return non-zero, deinit() will fail. But the KTHREAD_SHOULD_STOP already be set. > And oa_tc6_handler() will end. Please check it is what you want. Hi Ziyang Please trim emails when replying to just the relevant text. Otherwise you need to page down, page down, page down again and again and might miss parts of your reply. Andrew
Hi Ziyang, On 11/09/23 6:21 pm, Ziyang Xuan (William) wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> Register MAC-PHY interrupt and handle reset complete interrupt. Reset >> complete bit is set when the MAC-PHY reset complete 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 | 141 ++++++++++++++++++++++++++++++++-- >> include/linux/oa_tc6.h | 16 +++- >> 2 files changed, 150 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c >> index 613cf034430a..0019f70345b6 100644 >> --- a/drivers/net/ethernet/oa_tc6.c >> +++ b/drivers/net/ethernet/oa_tc6.c >> @@ -6,6 +6,7 @@ >> */ >> >> #include <linux/bitfield.h> >> +#include <linux/interrupt.h> >> #include <linux/oa_tc6.h> >> >> static int oa_tc6_spi_transfer(struct spi_device *spi, u8 *ptx, u8 *prx, >> @@ -160,10 +161,16 @@ int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len, >> if (ret) >> goto err_ctrl; >> >> - /* Check echoed/received control reply */ >> - ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, ctrl_prot); >> - if (ret) >> - goto err_ctrl; >> + /* In case of reset write, the echoed control command doesn't have any >> + * valid data. So no need to check for error. >> + */ >> + if (addr != OA_TC6_RESET) { >> + /* Check echoed/received control reply */ >> + ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, >> + ctrl_prot); >> + if (ret) >> + goto err_ctrl; >> + } >> >> if (!wnr) { >> /* Copy read data from the rx data in case of ctrl read */ >> @@ -186,6 +193,88 @@ int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len, >> return ret; >> } >> >> +static int oa_tc6_handler(void *data) >> +{ >> + struct oa_tc6 *tc6 = data; >> + u32 regval; >> + int ret; >> + >> + while (likely(!kthread_should_stop())) { >> + wait_event_interruptible(tc6->tc6_wq, tc6->int_flag || >> + kthread_should_stop()); >> + if (tc6->int_flag) { >> + tc6->int_flag = false; >> + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_STS0, ®val, 1, >> + false, false); >> + if (ret) { >> + dev_err(&tc6->spi->dev, "Failed to read STS0\n"); >> + continue; >> + } >> + /* 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, OA_TC6_STS0, >> + ®val, 1, true, >> + false); >> + if (ret) { >> + dev_err(&tc6->spi->dev, >> + "Failed to write STS0\n"); >> + continue; >> + } >> + complete(&tc6->rst_complete); >> + } >> + } >> + } >> + return 0; >> +} >> + >> +static irqreturn_t macphy_irq(int irq, void *dev_id) >> +{ >> + struct oa_tc6 *tc6 = dev_id; >> + >> + /* Wake tc6 task to perform interrupt action */ >> + tc6->int_flag = true; >> + wake_up_interruptible(&tc6->tc6_wq); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int oa_tc6_sw_reset(struct oa_tc6 *tc6) >> +{ >> + long timeleft; >> + 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 = SW_RESET; >> + reinit_completion(&tc6->rst_complete); >> + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, true, false); >> + if (ret) { >> + dev_err(&tc6->spi->dev, "RESET register write failed\n"); >> + return ret; >> + } >> + >> + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, true, true); >> + if (ret) { >> + dev_err(&tc6->spi->dev, "RESET register write failed\n"); >> + return ret; >> + } >> + timeleft = wait_for_completion_interruptible_timeout(&tc6->rst_complete, >> + msecs_to_jiffies(1)); >> + if (timeleft <= 0) { >> + dev_err(&tc6->spi->dev, "MAC-PHY reset failed\n"); >> + return -ENODEV; >> + } >> + >> + return 0; >> +} >> + >> int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len) >> { >> return oa_tc6_perform_ctrl(tc6, addr, val, len, true, tc6->ctrl_prot); >> @@ -201,6 +290,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_read_register); >> struct oa_tc6 *oa_tc6_init(struct spi_device *spi) >> { >> struct oa_tc6 *tc6; >> + int ret; >> >> if (!spi) >> return NULL; >> @@ -211,12 +301,51 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi) >> >> tc6->spi = spi; >> >> + /* Used for triggering the OA TC6 task */ >> + init_waitqueue_head(&tc6->tc6_wq); >> + >> + init_completion(&tc6->rst_complete); >> + >> + /* This task performs the SPI transfer */ >> + tc6->tc6_task = kthread_run(oa_tc6_handler, tc6, "OA TC6 Task"); >> + if (IS_ERR(tc6->tc6_task)) >> + goto err_tc6_task; >> + >> + /* Set the highest priority to the tc6 task as it is time critical */ >> + sched_set_fifo(tc6->tc6_task); >> + >> + /* Register MAC-PHY interrupt service routine */ >> + ret = devm_request_irq(&spi->dev, spi->irq, macphy_irq, 0, "macphy int", >> + tc6); >> + if ((ret != -ENOTCONN) && ret < 0) { >> + dev_err(&spi->dev, "Error attaching macphy irq %d\n", ret); >> + goto err_macphy_irq; >> + } >> + >> + /* Perform MAC-PHY software reset */ >> + if (oa_tc6_sw_reset(tc6)) >> + goto err_macphy_reset; >> + >> return tc6; >> + >> +err_macphy_reset: >> + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6); >> +err_macphy_irq: >> + kthread_stop(tc6->tc6_task); >> +err_tc6_task: >> + kfree(tc6); >> + return NULL; >> } >> EXPORT_SYMBOL_GPL(oa_tc6_init); >> >> -void oa_tc6_deinit(struct oa_tc6 *tc6) >> +int oa_tc6_deinit(struct oa_tc6 *tc6) >> { >> - kfree(tc6); >> + int ret; >> + >> + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6); >> + ret = kthread_stop(tc6->tc6_task); > > kthread_stop() will the result of threadfn(). Here mean that if threadfn() > return non-zero, deinit() will fail. But the KTHREAD_SHOULD_STOP already be set. > And oa_tc6_handler() will end. Please check it is what you want. Ok will handle it properly in the next revision. Best Regards, Parthiban V > >> + if (!ret) >> + kfree(tc6); >> + return ret; >> } >> EXPORT_SYMBOL_GPL(oa_tc6_deinit); >> diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h >> index 5e0a58ab1dcd..315f061c2dfe 100644 >> --- a/include/linux/oa_tc6.h >> +++ b/include/linux/oa_tc6.h >> @@ -17,15 +17,29 @@ >> #define CTRL_HDR_LEN GENMASK(7, 1) /* Length */ >> #define CTRL_HDR_P BIT(0) /* Parity Bit */ >> >> +/* Open Alliance TC6 Standard Control and Status Registers */ >> +#define OA_TC6_RESET 0x0003 /* Reset Control and Status Register */ >> +#define OA_TC6_STS0 0x0008 /* Status Register #0 */ >> + >> +/* RESET register field */ >> +#define SW_RESET BIT(0) /* Software Reset */ >> + >> +/* STATUS0 register field */ >> +#define RESETC BIT(6) /* Reset Complete */ >> + >> #define TC6_HDR_SIZE 4 /* Ctrl command header size as per OA */ >> #define TC6_FTR_SIZE 4 /* Ctrl command footer size ss per OA */ >> >> struct oa_tc6 { >> struct spi_device *spi; >> bool ctrl_prot; >> + struct task_struct *tc6_task; >> + wait_queue_head_t tc6_wq; >> + bool int_flag; >> + struct completion rst_complete; >> }; >> >> struct oa_tc6 *oa_tc6_init(struct spi_device *spi); >> -void oa_tc6_deinit(struct oa_tc6 *tc6); >> +int oa_tc6_deinit(struct oa_tc6 *tc6); >> int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 value[], u8 len); >> int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 value[], u8 len); >>
Hi Andrew, Thank you for reviewing the patch. On 09/09/23 7:09 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> + /* Register MAC-PHY interrupt service routine */ >> + ret = devm_request_irq(&spi->dev, spi->irq, macphy_irq, 0, "macphy int", >> + tc6); > > It looks like using threaded interrupts could save a lot of > complexity. Let the IRQ core handle all the tasklet stuff for you. Ok. If I understand correctly, I have to use devm_request_threaded_irq() instead of devm_request_irq() and let the thread handler registered with the devm_request_threaded_irq() function to perform interrupt activity directly? Best Regards, Parthiban V > > Andrew
> Ok. If I understand correctly, I have to use devm_request_threaded_irq() > instead of devm_request_irq() and let the thread handler registered with > the devm_request_threaded_irq() function to perform interrupt activity > directly? Yes. I've not looked at all the patches yet, but if the work queue is not used for anything else, you should be able to remove it, and let the IRQ core handle all the threading for you. Andrew
> +static int oa_tc6_sw_reset(struct oa_tc6 *tc6) > +{ > + long timeleft; > + 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 = SW_RESET; > + reinit_completion(&tc6->rst_complete); > + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, true, false); > + if (ret) { > + dev_err(&tc6->spi->dev, "RESET register write failed\n"); > + return ret; > + } > + > + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, true, true); > + if (ret) { > + dev_err(&tc6->spi->dev, "RESET register write failed\n"); > + return ret; > + } > + timeleft = wait_for_completion_interruptible_timeout(&tc6->rst_complete, > + msecs_to_jiffies(1)); > + if (timeleft <= 0) { > + dev_err(&tc6->spi->dev, "MAC-PHY reset failed\n"); > + return -ENODEV; > + } This seems a bit messy and complex. I assume reset is performed once during probe, and never again? So i wonder if it would be cleaner to actually just poll for the reset to complete? You can then remove all this completion code, and the interrupt handler gets simpler? > + /* Register MAC-PHY interrupt service routine */ > + ret = devm_request_irq(&spi->dev, spi->irq, macphy_irq, 0, "macphy int", > + tc6); > + if ((ret != -ENOTCONN) && ret < 0) { > + dev_err(&spi->dev, "Error attaching macphy irq %d\n", ret); > + goto err_macphy_irq; > + } Why is -ENOTCONN special? A comment would be good here. > -void oa_tc6_deinit(struct oa_tc6 *tc6) > +int oa_tc6_deinit(struct oa_tc6 *tc6) > { > - kfree(tc6); > + int ret; > + > + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6); > + ret = kthread_stop(tc6->tc6_task); > + if (!ret) > + kfree(tc6); > + return ret; > } What is the MAC driver supposed to do if this fails? But this problem probably goes away once you use a threaded interrupt handler. w> +/* Open Alliance TC6 Standard Control and Status Registers */ > +#define OA_TC6_RESET 0x0003 /* Reset Control and Status Register */ > +#define OA_TC6_STS0 0x0008 /* Status Register #0 */ Please use the same name as the standard. It use STATUS0, so OA_TC6_STATUS0. Please make sure all your defines follow the standard. > + > +/* RESET register field */ > +#define SW_RESET BIT(0) /* Software Reset */ It is pretty normal to put #defines for a register members after the #define for the register itself: #define OA_TC6_RESET 0x0003 /* Reset Control and Status Register */ #define OA_TC6_RESET_SWRESET BIT(0) #define OA_TC6_STATUS0 0x0008 /* Status Register #0 */ #define OA_TC6_STATUS0_RESETC BIT(6) /* Reset Complete */ The naming like this also helps avoid mixups. Andrew
Hi Parthiban, > Register MAC-PHY interrupt and handle reset complete interrupt. Reset > complete bit is set when the MAC-PHY reset complete 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. I'm using the MicroE module with LAN8651 device connected to nucleo STM32G4 microcontroller. Maybe not directly related to Linux, but I would like to ask for some clarification. > > Signed-off-by: Parthiban Veerasooran > <Parthiban.Veerasooran@microchip.com> --- > drivers/net/ethernet/oa_tc6.c | 141 > ++++++++++++++++++++++++++++++++-- include/linux/oa_tc6.h | > 16 +++- 2 files changed, 150 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/oa_tc6.c > b/drivers/net/ethernet/oa_tc6.c index 613cf034430a..0019f70345b6 > 100644 --- a/drivers/net/ethernet/oa_tc6.c > +++ b/drivers/net/ethernet/oa_tc6.c > @@ -6,6 +6,7 @@ > */ > > #include <linux/bitfield.h> > +#include <linux/interrupt.h> > #include <linux/oa_tc6.h> > > static int oa_tc6_spi_transfer(struct spi_device *spi, u8 *ptx, u8 > *prx, @@ -160,10 +161,16 @@ int oa_tc6_perform_ctrl(struct oa_tc6 > *tc6, u32 addr, u32 val[], u8 len, if (ret) > goto err_ctrl; > > - /* Check echoed/received control reply */ > - ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, > ctrl_prot); > - if (ret) > - goto err_ctrl; > + /* In case of reset write, the echoed control command > doesn't have any > + * valid data. So no need to check for error. > + */ > + if (addr != OA_TC6_RESET) { > + /* Check echoed/received control reply */ > + ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, > wnr, > + ctrl_prot); > + if (ret) > + goto err_ctrl; > + } > > if (!wnr) { > /* Copy read data from the rx data in case of ctrl > read */ @@ -186,6 +193,88 @@ int oa_tc6_perform_ctrl(struct oa_tc6 > *tc6, u32 addr, u32 val[], u8 len, return ret; > } > > +static int oa_tc6_handler(void *data) > +{ > + struct oa_tc6 *tc6 = data; > + u32 regval; > + int ret; > + > + while (likely(!kthread_should_stop())) { > + wait_event_interruptible(tc6->tc6_wq, tc6->int_flag > || > + kthread_should_stop()); > + if (tc6->int_flag) { > + tc6->int_flag = false; > + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_STS0, > ®val, 1, > + false, false); > + if (ret) { > + dev_err(&tc6->spi->dev, "Failed to > read STS0\n"); > + continue; > + } > + /* Check for reset complete interrupt status > */ > + if (regval & RESETC) { Just maybe mine small remark. IMHO the reset shall not pollute the IRQ hander. The RESETC is just set on the initialization phase and only then shall be served. Please correct me if I'm wrong, but it will not be handled during "normal" operation. > + regval = RESETC; > + /* SPI host should write RESETC bit > with one to > + * clear the reset interrupt status. > + */ > + ret = oa_tc6_perform_ctrl(tc6, > OA_TC6_STS0, > + ®val, > 1, true, > + false); Is this enough to have the IRQ_N deasserted (i.e. pulled HIGH)? The documentation states it clearly that one also needs to set SYNC bit (BIT(15)) in the OA_CONFIG0 register (which would have the 0x8006 value). Mine problem is that even after writing 0x40 to OA_STATUS0 and 0x8006 to OA_CONFIG0 the IRQ_N is still LOW (it is pulled up via 10K resistor). (I'm able to read those registers and those show expected values) > + if (ret) { > + dev_err(&tc6->spi->dev, > + "Failed to write > STS0\n"); > + continue; > + } > + complete(&tc6->rst_complete); > + } > + } > + } > + return 0; > +} > + > +static irqreturn_t macphy_irq(int irq, void *dev_id) > +{ > + struct oa_tc6 *tc6 = dev_id; > + > + /* Wake tc6 task to perform interrupt action */ > + tc6->int_flag = true; > + wake_up_interruptible(&tc6->tc6_wq); > + > + return IRQ_HANDLED; > +} > + > +static int oa_tc6_sw_reset(struct oa_tc6 *tc6) > +{ > + long timeleft; > + 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 = SW_RESET; > + reinit_completion(&tc6->rst_complete); > + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, > true, false); > + if (ret) { > + dev_err(&tc6->spi->dev, "RESET register write > failed\n"); > + return ret; > + } > + > + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, > true, true); > + if (ret) { > + dev_err(&tc6->spi->dev, "RESET register write > failed\n"); > + return ret; > + } > + timeleft = Was it on purpose to not use the RST_N pin to perform GPIO based reset? When I generate reset pulse (and keep it for low for > 5us) the IRQ_N gets high. After some time it gets low (as expected). But then it doesn't get high any more. > wait_for_completion_interruptible_timeout(&tc6->rst_complete, > + > msecs_to_jiffies(1)); Please also clarify - does the LAN8651 require up to 1ms "settle down" (after reset) time before it gets operational again? > + if (timeleft <= 0) { > + dev_err(&tc6->spi->dev, "MAC-PHY reset failed\n"); > + return -ENODEV; > + } > + > + return 0; > +} > + > int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 val[], > u8 len) { > return oa_tc6_perform_ctrl(tc6, addr, val, len, true, > tc6->ctrl_prot); @@ -201,6 +290,7 @@ > EXPORT_SYMBOL_GPL(oa_tc6_read_register); struct oa_tc6 > *oa_tc6_init(struct spi_device *spi) { > struct oa_tc6 *tc6; > + int ret; > > if (!spi) > return NULL; > @@ -211,12 +301,51 @@ struct oa_tc6 *oa_tc6_init(struct spi_device > *spi) > tc6->spi = spi; > > + /* Used for triggering the OA TC6 task */ > + init_waitqueue_head(&tc6->tc6_wq); > + > + init_completion(&tc6->rst_complete); > + > + /* This task performs the SPI transfer */ > + tc6->tc6_task = kthread_run(oa_tc6_handler, tc6, "OA TC6 > Task"); > + if (IS_ERR(tc6->tc6_task)) > + goto err_tc6_task; > + > + /* Set the highest priority to the tc6 task as it is time > critical */ > + sched_set_fifo(tc6->tc6_task); > + > + /* Register MAC-PHY interrupt service routine */ > + ret = devm_request_irq(&spi->dev, spi->irq, macphy_irq, 0, > "macphy int", > + tc6); > + if ((ret != -ENOTCONN) && ret < 0) { > + dev_err(&spi->dev, "Error attaching macphy irq > %d\n", ret); > + goto err_macphy_irq; > + } > + > + /* Perform MAC-PHY software reset */ > + if (oa_tc6_sw_reset(tc6)) > + goto err_macphy_reset; > + > return tc6; > + > +err_macphy_reset: > + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6); > +err_macphy_irq: > + kthread_stop(tc6->tc6_task); > +err_tc6_task: > + kfree(tc6); > + return NULL; > } > EXPORT_SYMBOL_GPL(oa_tc6_init); > > -void oa_tc6_deinit(struct oa_tc6 *tc6) > +int oa_tc6_deinit(struct oa_tc6 *tc6) > { > - kfree(tc6); > + int ret; > + > + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6); > + ret = kthread_stop(tc6->tc6_task); > + if (!ret) > + kfree(tc6); > + return ret; > } > EXPORT_SYMBOL_GPL(oa_tc6_deinit); > diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h > index 5e0a58ab1dcd..315f061c2dfe 100644 > --- a/include/linux/oa_tc6.h > +++ b/include/linux/oa_tc6.h > @@ -17,15 +17,29 @@ > #define CTRL_HDR_LEN GENMASK(7, 1) /* Length */ > #define CTRL_HDR_P BIT(0) /* Parity Bit */ > > +/* Open Alliance TC6 Standard Control and Status Registers */ > +#define OA_TC6_RESET 0x0003 /* Reset Control > and Status Register */ +#define OA_TC6_STS0 0x0008 > /* Status Register #0 */ + > +/* RESET register field */ > +#define SW_RESET BIT(0) /* Software Reset */ > + > +/* STATUS0 register field */ > +#define RESETC BIT(6) /* Reset > Complete */ + > #define TC6_HDR_SIZE 4 /* Ctrl command header > size as per OA */ #define TC6_FTR_SIZE 4 /* > Ctrl command footer size ss per OA */ > struct oa_tc6 { > struct spi_device *spi; > bool ctrl_prot; > + struct task_struct *tc6_task; > + wait_queue_head_t tc6_wq; > + bool int_flag; > + struct completion rst_complete; > }; > > struct oa_tc6 *oa_tc6_init(struct spi_device *spi); > -void oa_tc6_deinit(struct oa_tc6 *tc6); > +int oa_tc6_deinit(struct oa_tc6 *tc6); > int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 value[], > u8 len); int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 > value[], u8 len); Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
> Just maybe mine small remark. IMHO the reset shall not pollute the IRQ > hander. The RESETC is just set on the initialization phase and only > then shall be served. Please correct me if I'm wrong, but it will not > be handled during "normal" operation. This is something i also wondered. Maybe if the firmware in the MAC-PHY crashes, burns, and a watchdog reset it, could it assert RESETC? I think maybe a WARN_ON_ONCE() for RESETC in the interrupt handler would be useful, but otherwise ignore it. Probe can then poll during its reset. > > + regval = RESETC; > > + /* SPI host should write RESETC bit > > with one to > > + * clear the reset interrupt status. > > + */ > > + ret = oa_tc6_perform_ctrl(tc6, > > OA_TC6_STS0, > > + ®val, > > 1, true, > > + false); > > Is this enough to have the IRQ_N deasserted (i.e. pulled HIGH)? > > The documentation states it clearly that one also needs to set SYNC bit > (BIT(15)) in the OA_CONFIG0 register (which would have the 0x8006 value). > > Mine problem is that even after writing 0x40 to OA_STATUS0 and 0x8006 > to OA_CONFIG0 the IRQ_N is still LOW (it is pulled up via 10K resistor). > > (I'm able to read those registers and those show expected values) What does STATUS0 and STATUS1 contain? That might be a dumb question, i've not read the details for interrupt handling yet, but maybe there is another interrupt pending? Or the interrupt mask needs writing? > Was it on purpose to not use the RST_N pin to perform GPIO based reset? > > When I generate reset pulse (and keep it for low for > 5us) the IRQ_N > gets high. After some time it gets low (as expected). But then it > doesn't get high any more. Does the standard say RST_N is mandatory to be controlled by software? I could imagine RST_N is tied to the board global reset when the power supply is stable. Software reset is then used at probe time. So this could be a board design decision. I can see this code getting extended in the future, an optional gpiod passed to the core for it to use. > > msecs_to_jiffies(1)); > > Please also clarify - does the LAN8651 require up to 1ms "settle down" > (after reset) time before it gets operational again? If this is not part of the standard, it really should be in the MAC driver, or configurable, since different devices might need different delays. But ideally, if the status bit says it is good to go, i would really expect it to be good to go. So this probably should be a LAN8651 quirk. Andrew
Hi Andrew, > > Just maybe mine small remark. IMHO the reset shall not pollute the > > IRQ hander. The RESETC is just set on the initialization phase and > > only then shall be served. Please correct me if I'm wrong, but it > > will not be handled during "normal" operation. > > This is something i also wondered. Maybe if the firmware in the > MAC-PHY crashes, burns, and a watchdog reset it, could it assert > RESETC? I think maybe a WARN_ON_ONCE() for RESETC in the interrupt > handler would be useful, but otherwise ignore it. Probe can then poll > during its reset. > > > > + regval = RESETC; > > > + /* SPI host should write RESETC > > > bit with one to > > > + * clear the reset interrupt > > > status. > > > + */ > > > + ret = oa_tc6_perform_ctrl(tc6, > > > OA_TC6_STS0, > > > + > > > ®val, 1, true, > > > + > > > false); > > > > Is this enough to have the IRQ_N deasserted (i.e. pulled HIGH)? > > > > The documentation states it clearly that one also needs to set SYNC > > bit (BIT(15)) in the OA_CONFIG0 register (which would have the > > 0x8006 value). > > > > Mine problem is that even after writing 0x40 to OA_STATUS0 and > > 0x8006 to OA_CONFIG0 the IRQ_N is still LOW (it is pulled up via > > 10K resistor). > > > > (I'm able to read those registers and those show expected values) > > What does STATUS0 and STATUS1 contain? STATUS0 => 0x40, which is expected. Then I do write 0x40 to STATUS0 -> bit6 (RESETC) is R/W1C After reading the same register - I do receive 0x00 (it has been cleared). Then I write 0x8006 to OA_CONFIG0. (Those two steps are regarded as "configuration" of LAN865x device in the documentation) In this patch set -> the OA_COFIG0 also has the 0x6 added to indicate the SPI transfer chunk. Dump of OA registers: {0x11, 0x7c1b3, 0x5e5, 0x0, 0x8006, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x3000, 0x1fbf, 0x3ffe0003, 0x0, 0x0} Status 0 (0x8) -> 0x0 Status 1 (0x9) -> 0x0 > That might be a dumb question, > i've not read the details for interrupt handling yet, but maybe there > is another interrupt pending? Or the interrupt mask needs writing? All the interrupts on MASK{01} are masked. Changing it to: sts &= ~(OA_IMASK0_TXPEM | OA_IMASK0_TXBOEM | OA_IMASK0_TXBUEM | OA_IMASK0_RXBOEM | OA_IMASK0_LOFEM | OA_IMASK0_HDREM doesn't fix this problem. > > > Was it on purpose to not use the RST_N pin to perform GPIO based > > reset? > > > > When I generate reset pulse (and keep it for low for > 5us) the > > IRQ_N gets high. After some time it gets low (as expected). But > > then it doesn't get high any more. > > Does the standard say RST_N is mandatory to be controlled by software? > I could imagine RST_N is tied to the board global reset when the power > supply is stable. It can be GPIO controlled. However, it is not required. I've tied it to 3V3 and also left NC, but no change. > Software reset is then used at probe time. I've reconfigured the board to use only SW based reset (i.e. set bit0 in OA_RESET - 0x3). > > So this could be a board design decision. I can see this code getting > extended in the future, an optional gpiod passed to the core for it to > use. I can omit the RST_N control. I'd just expect the IRQ_N to be high after reset. > > > > msecs_to_jiffies(1)); > > > > Please also clarify - does the LAN8651 require up to 1ms "settle > > down" (after reset) time before it gets operational again? > > If this is not part of the standard, it really should be in the MAC > driver, or configurable, since different devices might need different > delays. But ideally, if the status bit says it is good to go, i would > really expect it to be good to go. So this probably should be a > LAN8651 quirk. The documentation is silent about the "settle down time". The only requirements is for RST_N assertion > 5us. However, when the IRQ_N goes low, and the interrupt is served - it happens that I cannot read ID from the chip via SPI. > > Andrew Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Andrew, On 13/09/23 7:49 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> Ok. If I understand correctly, I have to use devm_request_threaded_irq() >> instead of devm_request_irq() and let the thread handler registered with >> the devm_request_threaded_irq() function to perform interrupt activity >> directly? > > Yes. I've not looked at all the patches yet, but if the work queue is > not used for anything else, you should be able to remove it, and let > the IRQ core handle all the threading for you. Sure, will implement it. Thanks. Best Regards, Parthiban V > > Andrew >
On 13/09/23 8:09 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> +static int oa_tc6_sw_reset(struct oa_tc6 *tc6) >> +{ >> + long timeleft; >> + 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 = SW_RESET; >> + reinit_completion(&tc6->rst_complete); >> + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, true, false); >> + if (ret) { >> + dev_err(&tc6->spi->dev, "RESET register write failed\n"); >> + return ret; >> + } >> + >> + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, true, true); >> + if (ret) { >> + dev_err(&tc6->spi->dev, "RESET register write failed\n"); >> + return ret; >> + } >> + timeleft = wait_for_completion_interruptible_timeout(&tc6->rst_complete, >> + msecs_to_jiffies(1)); >> + if (timeleft <= 0) { >> + dev_err(&tc6->spi->dev, "MAC-PHY reset failed\n"); >> + return -ENODEV; >> + } > > This seems a bit messy and complex. I assume reset is performed once > during probe, and never again? So i wonder if it would be cleaner to > actually just poll for the reset to complete? You can then remove all > this completion code, and the interrupt handler gets simpler? Ok the spec says the below, that's why I implemented like this. 9.2.8.8 RESETC Reset Complete. This bit is set when the MAC-PHY reset is complete and ready for configuration. When it is set, it will generate a non-maskable interrupt assertion on IRQn to alert the SPI host. Additionally, setting of the RESETC bit shall also set EXST = 1 in the receive data footer until this bit is cleared by action of the SPI host writing a ‘1’. Yes, I agree that the reset is performed once in the beginning. So I will poll for the completion and remove this block in the next revision. > >> + /* Register MAC-PHY interrupt service routine */ >> + ret = devm_request_irq(&spi->dev, spi->irq, macphy_irq, 0, "macphy int", >> + tc6); >> + if ((ret != -ENOTCONN) && ret < 0) { >> + dev_err(&spi->dev, "Error attaching macphy irq %d\n", ret); >> + goto err_macphy_irq; >> + } > > Why is -ENOTCONN special? A comment would be good here. Ah, it is a mistake. I supposed to use, if (ret) I will correct it in the next version. > >> -void oa_tc6_deinit(struct oa_tc6 *tc6) >> +int oa_tc6_deinit(struct oa_tc6 *tc6) >> { >> - kfree(tc6); >> + int ret; >> + >> + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6); >> + ret = kthread_stop(tc6->tc6_task); >> + if (!ret) >> + kfree(tc6); >> + return ret; >> } > > What is the MAC driver supposed to do if this fails? > > But this problem probably goes away once you use a threaded interrupt > handler. Yes, I agree. Will do that. > > w> +/* Open Alliance TC6 Standard Control and Status Registers */ >> +#define OA_TC6_RESET 0x0003 /* Reset Control and Status Register */ >> +#define OA_TC6_STS0 0x0008 /* Status Register #0 */ > > Please use the same name as the standard. It use STATUS0, so > OA_TC6_STATUS0. Please make sure all your defines follow the standard. Yes sure. > >> + >> +/* RESET register field */ >> +#define SW_RESET BIT(0) /* Software Reset */ > > It is pretty normal to put #defines for a register members after the > #define for the register itself: > > #define OA_TC6_RESET 0x0003 /* Reset Control and Status Register */ > #define OA_TC6_RESET_SWRESET BIT(0) > > #define OA_TC6_STATUS0 0x0008 /* Status Register #0 */ > #define OA_TC6_STATUS0_RESETC BIT(6) /* Reset Complete */ > > The naming like this also helps avoid mixups. Ok, I will follow this in the next version. Best Regards, Parthiban V > > Andrew >
Hi Parthiban, > On 13/09/23 8:09 am, Andrew Lunn wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > >> +static int oa_tc6_sw_reset(struct oa_tc6 *tc6) > >> +{ > >> + long timeleft; > >> + 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 = SW_RESET; > >> + reinit_completion(&tc6->rst_complete); > >> + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, > >> true, false); > >> + if (ret) { > >> + dev_err(&tc6->spi->dev, "RESET register write > >> failed\n"); > >> + return ret; > >> + } > >> + > >> + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, > >> true, true); > >> + if (ret) { > >> + dev_err(&tc6->spi->dev, "RESET register write > >> failed\n"); > >> + return ret; > >> + } > >> + timeleft = > >> wait_for_completion_interruptible_timeout(&tc6->rst_complete, > >> + > >> msecs_to_jiffies(1)); > >> + if (timeleft <= 0) { > >> + dev_err(&tc6->spi->dev, "MAC-PHY reset failed\n"); > >> + return -ENODEV; > >> + } > > > > This seems a bit messy and complex. I assume reset is performed once > > during probe, and never again? So i wonder if it would be cleaner to > > actually just poll for the reset to complete? You can then remove > > all this completion code, and the interrupt handler gets simpler? > Ok the spec says the below, that's why I implemented like this. > > 9.2.8.8 RESETC > Reset Complete. This bit is set when the MAC-PHY reset is complete > and ready for configuration. When it is set, it will generate a > non-maskable interrupt assertion on IRQn to alert the SPI host. > Additionally, setting of the RESETC bit shall also set EXST = 1 in > the receive data footer until this bit is cleared by action of the > SPI host writing a ‘1’. If you don't mind - I would like to ask some extra questions: 1. Could you share which silicon revision of LAN8651 (rev 1 = B0 or rev 2 = B1) are your using? 2. Do you use 10k Ohm pull up resistor between VDD and the IRQ_N line? 3. Are you using any standard development board with LAN865x device? Could you share how do you connect reset and irq lines and which CPU do you use? Thanks in advance for your help. > > Yes, I agree that the reset is performed once in the beginning. So I > will poll for the completion and remove this block in the next > revision. > > > >> + /* Register MAC-PHY interrupt service routine */ > >> + ret = devm_request_irq(&spi->dev, spi->irq, macphy_irq, 0, > >> "macphy int", > >> + tc6); > >> + if ((ret != -ENOTCONN) && ret < 0) { > >> + dev_err(&spi->dev, "Error attaching macphy irq > >> %d\n", ret); > >> + goto err_macphy_irq; > >> + } > > > > Why is -ENOTCONN special? A comment would be good here. > Ah, it is a mistake. I supposed to use, > > if (ret) > > I will correct it in the next version. > > > >> -void oa_tc6_deinit(struct oa_tc6 *tc6) > >> +int oa_tc6_deinit(struct oa_tc6 *tc6) > >> { > >> - kfree(tc6); > >> + int ret; > >> + > >> + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6); > >> + ret = kthread_stop(tc6->tc6_task); > >> + if (!ret) > >> + kfree(tc6); > >> + return ret; > >> } > > > > What is the MAC driver supposed to do if this fails? > > > > But this problem probably goes away once you use a threaded > > interrupt handler. > Yes, I agree. Will do that. > > > > w> +/* Open Alliance TC6 Standard Control and Status Registers */ > >> +#define OA_TC6_RESET 0x0003 /* Reset Control and Status > >> Register */ +#define OA_TC6_STS0 0x0008 /* Status > >> Register #0 */ > > > > Please use the same name as the standard. It use STATUS0, so > > OA_TC6_STATUS0. Please make sure all your defines follow the > > standard. > Yes sure. > > > >> + > >> +/* RESET register field */ > >> +#define SW_RESET BIT(0) /* Software Reset */ > > > > It is pretty normal to put #defines for a register members after the > > #define for the register itself: > > > > #define OA_TC6_RESET 0x0003 /* Reset Control and Status > > Register */ #define OA_TC6_RESET_SWRESET BIT(0) > > > > #define OA_TC6_STATUS0 0x0008 /* Status Register #0 */ > > #define OA_TC6_STATUS0_RESETC BIT(6) /* Reset > > Complete */ > > > > The naming like this also helps avoid mixups. > Ok, I will follow this in the next version. > > Best Regards, > Parthiban V > > > > Andrew > > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Lukasz, Sorry for the delayed response. Regarding your issue, we just noticed that you have also filed an issue in our oa tc6 lib github page. Our oa tc6 lib for controllers developer Thorsten will get back to you on this. You can get the solution from there. https://github.com/MicrochipTech/oa-tc6-lib/issues/14 Best Regards, Parthiban V On 13/09/23 6:56 pm, Lukasz Majewski wrote: > Hi Andrew, > >>> Just maybe mine small remark. IMHO the reset shall not pollute the >>> IRQ hander. The RESETC is just set on the initialization phase and >>> only then shall be served. Please correct me if I'm wrong, but it >>> will not be handled during "normal" operation. >> >> This is something i also wondered. Maybe if the firmware in the >> MAC-PHY crashes, burns, and a watchdog reset it, could it assert >> RESETC? I think maybe a WARN_ON_ONCE() for RESETC in the interrupt >> handler would be useful, but otherwise ignore it. Probe can then poll >> during its reset. >> >>>> + regval = RESETC; >>>> + /* SPI host should write RESETC >>>> bit with one to >>>> + * clear the reset interrupt >>>> status. >>>> + */ >>>> + ret = oa_tc6_perform_ctrl(tc6, >>>> OA_TC6_STS0, >>>> + >>>> ®val, 1, true, >>>> + >>>> false); >>> >>> Is this enough to have the IRQ_N deasserted (i.e. pulled HIGH)? >>> >>> The documentation states it clearly that one also needs to set SYNC >>> bit (BIT(15)) in the OA_CONFIG0 register (which would have the >>> 0x8006 value). >>> >>> Mine problem is that even after writing 0x40 to OA_STATUS0 and >>> 0x8006 to OA_CONFIG0 the IRQ_N is still LOW (it is pulled up via >>> 10K resistor). >>> >>> (I'm able to read those registers and those show expected values) >> >> What does STATUS0 and STATUS1 contain? > > STATUS0 => 0x40, which is expected. > > Then I do write 0x40 to STATUS0 -> bit6 (RESETC) is R/W1C > > After reading the same register - I do receive 0x00 (it has been > cleared). > > Then I write 0x8006 to OA_CONFIG0. > > (Those two steps are regarded as "configuration" of LAN865x device in > the documentation) > > In this patch set -> the OA_COFIG0 also has the 0x6 added to indicate > the SPI transfer chunk. > > Dump of OA registers: > {0x11, 0x7c1b3, 0x5e5, 0x0, 0x8006, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, > 0x3000, 0x1fbf, 0x3ffe0003, 0x0, 0x0} > > Status 0 (0x8) -> 0x0 > Status 1 (0x9) -> 0x0 > >> That might be a dumb question, >> i've not read the details for interrupt handling yet, but maybe there >> is another interrupt pending? Or the interrupt mask needs writing? > > All the interrupts on MASK{01} are masked. > > Changing it to: > sts &= ~(OA_IMASK0_TXPEM | OA_IMASK0_TXBOEM | OA_IMASK0_TXBUEM | > OA_IMASK0_RXBOEM | OA_IMASK0_LOFEM | OA_IMASK0_HDREM > > doesn't fix this problem. > >> >>> Was it on purpose to not use the RST_N pin to perform GPIO based >>> reset? >>> >>> When I generate reset pulse (and keep it for low for > 5us) the >>> IRQ_N gets high. After some time it gets low (as expected). But >>> then it doesn't get high any more. >> >> Does the standard say RST_N is mandatory to be controlled by software? >> I could imagine RST_N is tied to the board global reset when the power >> supply is stable. > > It can be GPIO controlled. However, it is not required. I've tied it to > 3V3 and also left NC, but no change. > >> Software reset is then used at probe time. > > I've reconfigured the board to use only SW based reset (i.e. set bit0 > in OA_RESET - 0x3). > >> >> So this could be a board design decision. I can see this code getting >> extended in the future, an optional gpiod passed to the core for it to >> use. > > I can omit the RST_N control. I'd just expect the IRQ_N to be high > after reset. > >> >>>> msecs_to_jiffies(1)); >>> >>> Please also clarify - does the LAN8651 require up to 1ms "settle >>> down" (after reset) time before it gets operational again? >> >> If this is not part of the standard, it really should be in the MAC >> driver, or configurable, since different devices might need different >> delays. But ideally, if the status bit says it is good to go, i would >> really expect it to be good to go. So this probably should be a >> LAN8651 quirk. > > The documentation is silent about the "settle down time". The only > requirements is for RST_N assertion > 5us. However, when the IRQ_N goes > low, and the interrupt is served - it happens that I cannot read ID > from the chip via SPI. > >> >> Andrew > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Parthiban, > Hi Lukasz, > > Sorry for the delayed response. Regarding your issue, we just noticed > that you have also filed an issue in our oa tc6 lib github page. Our > oa tc6 lib for controllers developer Thorsten will get back to you on > this. You can get the solution from there. > > https://github.com/MicrochipTech/oa-tc6-lib/issues/14 Yes. This is filled by me :-) I will reach Thorsten directly. Thanks for the reply. > > Best Regards, > Parthiban V > > On 13/09/23 6:56 pm, Lukasz Majewski wrote: > > Hi Andrew, > > > >>> Just maybe mine small remark. IMHO the reset shall not pollute the > >>> IRQ hander. The RESETC is just set on the initialization phase and > >>> only then shall be served. Please correct me if I'm wrong, but it > >>> will not be handled during "normal" operation. > >> > >> This is something i also wondered. Maybe if the firmware in the > >> MAC-PHY crashes, burns, and a watchdog reset it, could it assert > >> RESETC? I think maybe a WARN_ON_ONCE() for RESETC in the interrupt > >> handler would be useful, but otherwise ignore it. Probe can then > >> poll during its reset. > >> > >>>> + regval = RESETC; > >>>> + /* SPI host should write RESETC > >>>> bit with one to > >>>> + * clear the reset interrupt > >>>> status. > >>>> + */ > >>>> + ret = oa_tc6_perform_ctrl(tc6, > >>>> OA_TC6_STS0, > >>>> + > >>>> ®val, 1, true, > >>>> + > >>>> false); > >>> > >>> Is this enough to have the IRQ_N deasserted (i.e. pulled HIGH)? > >>> > >>> The documentation states it clearly that one also needs to set > >>> SYNC bit (BIT(15)) in the OA_CONFIG0 register (which would have > >>> the 0x8006 value). > >>> > >>> Mine problem is that even after writing 0x40 to OA_STATUS0 and > >>> 0x8006 to OA_CONFIG0 the IRQ_N is still LOW (it is pulled up via > >>> 10K resistor). > >>> > >>> (I'm able to read those registers and those show expected values) > >>> > >> > >> What does STATUS0 and STATUS1 contain? > > > > STATUS0 => 0x40, which is expected. > > > > Then I do write 0x40 to STATUS0 -> bit6 (RESETC) is R/W1C > > > > After reading the same register - I do receive 0x00 (it has been > > cleared). > > > > Then I write 0x8006 to OA_CONFIG0. > > > > (Those two steps are regarded as "configuration" of LAN865x device > > in the documentation) > > > > In this patch set -> the OA_COFIG0 also has the 0x6 added to > > indicate the SPI transfer chunk. > > > > Dump of OA registers: > > {0x11, 0x7c1b3, 0x5e5, 0x0, 0x8006, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, > > 0x3000, 0x1fbf, 0x3ffe0003, 0x0, 0x0} > > > > Status 0 (0x8) -> 0x0 > > Status 1 (0x9) -> 0x0 > > > >> That might be a dumb question, > >> i've not read the details for interrupt handling yet, but maybe > >> there is another interrupt pending? Or the interrupt mask needs > >> writing? > > > > All the interrupts on MASK{01} are masked. > > > > Changing it to: > > sts &= ~(OA_IMASK0_TXPEM | OA_IMASK0_TXBOEM | OA_IMASK0_TXBUEM | > > OA_IMASK0_RXBOEM | OA_IMASK0_LOFEM | OA_IMASK0_HDREM > > > > doesn't fix this problem. > > > >> > >>> Was it on purpose to not use the RST_N pin to perform GPIO based > >>> reset? > >>> > >>> When I generate reset pulse (and keep it for low for > 5us) the > >>> IRQ_N gets high. After some time it gets low (as expected). But > >>> then it doesn't get high any more. > >> > >> Does the standard say RST_N is mandatory to be controlled by > >> software? I could imagine RST_N is tied to the board global reset > >> when the power supply is stable. > > > > It can be GPIO controlled. However, it is not required. I've tied > > it to 3V3 and also left NC, but no change. > > > >> Software reset is then used at probe time. > > > > I've reconfigured the board to use only SW based reset (i.e. set > > bit0 in OA_RESET - 0x3). > > > >> > >> So this could be a board design decision. I can see this code > >> getting extended in the future, an optional gpiod passed to the > >> core for it to use. > > > > I can omit the RST_N control. I'd just expect the IRQ_N to be high > > after reset. > > > >> > >>>> msecs_to_jiffies(1)); > >>> > >>> Please also clarify - does the LAN8651 require up to 1ms "settle > >>> down" (after reset) time before it gets operational again? > >> > >> If this is not part of the standard, it really should be in the MAC > >> driver, or configurable, since different devices might need > >> different delays. But ideally, if the status bit says it is good > >> to go, i would really expect it to be good to go. So this probably > >> should be a LAN8651 quirk. > > > > The documentation is silent about the "settle down time". The only > > requirements is for RST_N assertion > 5us. However, when the IRQ_N > > goes low, and the interrupt is served - it happens that I cannot > > read ID from the chip via SPI. > > > >> > >> Andrew > > > > Best regards, > > > > Lukasz Majewski > > > > -- > > > > DENX Software Engineering GmbH, Managing Director: Erika Unter > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, > > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > > lukma@denx.de > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c index 613cf034430a..0019f70345b6 100644 --- a/drivers/net/ethernet/oa_tc6.c +++ b/drivers/net/ethernet/oa_tc6.c @@ -6,6 +6,7 @@ */ #include <linux/bitfield.h> +#include <linux/interrupt.h> #include <linux/oa_tc6.h> static int oa_tc6_spi_transfer(struct spi_device *spi, u8 *ptx, u8 *prx, @@ -160,10 +161,16 @@ int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len, if (ret) goto err_ctrl; - /* Check echoed/received control reply */ - ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, ctrl_prot); - if (ret) - goto err_ctrl; + /* In case of reset write, the echoed control command doesn't have any + * valid data. So no need to check for error. + */ + if (addr != OA_TC6_RESET) { + /* Check echoed/received control reply */ + ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, + ctrl_prot); + if (ret) + goto err_ctrl; + } if (!wnr) { /* Copy read data from the rx data in case of ctrl read */ @@ -186,6 +193,88 @@ int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len, return ret; } +static int oa_tc6_handler(void *data) +{ + struct oa_tc6 *tc6 = data; + u32 regval; + int ret; + + while (likely(!kthread_should_stop())) { + wait_event_interruptible(tc6->tc6_wq, tc6->int_flag || + kthread_should_stop()); + if (tc6->int_flag) { + tc6->int_flag = false; + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_STS0, ®val, 1, + false, false); + if (ret) { + dev_err(&tc6->spi->dev, "Failed to read STS0\n"); + continue; + } + /* 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, OA_TC6_STS0, + ®val, 1, true, + false); + if (ret) { + dev_err(&tc6->spi->dev, + "Failed to write STS0\n"); + continue; + } + complete(&tc6->rst_complete); + } + } + } + return 0; +} + +static irqreturn_t macphy_irq(int irq, void *dev_id) +{ + struct oa_tc6 *tc6 = dev_id; + + /* Wake tc6 task to perform interrupt action */ + tc6->int_flag = true; + wake_up_interruptible(&tc6->tc6_wq); + + return IRQ_HANDLED; +} + +static int oa_tc6_sw_reset(struct oa_tc6 *tc6) +{ + long timeleft; + 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 = SW_RESET; + reinit_completion(&tc6->rst_complete); + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, true, false); + if (ret) { + dev_err(&tc6->spi->dev, "RESET register write failed\n"); + return ret; + } + + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1, true, true); + if (ret) { + dev_err(&tc6->spi->dev, "RESET register write failed\n"); + return ret; + } + timeleft = wait_for_completion_interruptible_timeout(&tc6->rst_complete, + msecs_to_jiffies(1)); + if (timeleft <= 0) { + dev_err(&tc6->spi->dev, "MAC-PHY reset failed\n"); + return -ENODEV; + } + + return 0; +} + int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len) { return oa_tc6_perform_ctrl(tc6, addr, val, len, true, tc6->ctrl_prot); @@ -201,6 +290,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_read_register); struct oa_tc6 *oa_tc6_init(struct spi_device *spi) { struct oa_tc6 *tc6; + int ret; if (!spi) return NULL; @@ -211,12 +301,51 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi) tc6->spi = spi; + /* Used for triggering the OA TC6 task */ + init_waitqueue_head(&tc6->tc6_wq); + + init_completion(&tc6->rst_complete); + + /* This task performs the SPI transfer */ + tc6->tc6_task = kthread_run(oa_tc6_handler, tc6, "OA TC6 Task"); + if (IS_ERR(tc6->tc6_task)) + goto err_tc6_task; + + /* Set the highest priority to the tc6 task as it is time critical */ + sched_set_fifo(tc6->tc6_task); + + /* Register MAC-PHY interrupt service routine */ + ret = devm_request_irq(&spi->dev, spi->irq, macphy_irq, 0, "macphy int", + tc6); + if ((ret != -ENOTCONN) && ret < 0) { + dev_err(&spi->dev, "Error attaching macphy irq %d\n", ret); + goto err_macphy_irq; + } + + /* Perform MAC-PHY software reset */ + if (oa_tc6_sw_reset(tc6)) + goto err_macphy_reset; + return tc6; + +err_macphy_reset: + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6); +err_macphy_irq: + kthread_stop(tc6->tc6_task); +err_tc6_task: + kfree(tc6); + return NULL; } EXPORT_SYMBOL_GPL(oa_tc6_init); -void oa_tc6_deinit(struct oa_tc6 *tc6) +int oa_tc6_deinit(struct oa_tc6 *tc6) { - kfree(tc6); + int ret; + + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6); + ret = kthread_stop(tc6->tc6_task); + if (!ret) + kfree(tc6); + return ret; } EXPORT_SYMBOL_GPL(oa_tc6_deinit); diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h index 5e0a58ab1dcd..315f061c2dfe 100644 --- a/include/linux/oa_tc6.h +++ b/include/linux/oa_tc6.h @@ -17,15 +17,29 @@ #define CTRL_HDR_LEN GENMASK(7, 1) /* Length */ #define CTRL_HDR_P BIT(0) /* Parity Bit */ +/* Open Alliance TC6 Standard Control and Status Registers */ +#define OA_TC6_RESET 0x0003 /* Reset Control and Status Register */ +#define OA_TC6_STS0 0x0008 /* Status Register #0 */ + +/* RESET register field */ +#define SW_RESET BIT(0) /* Software Reset */ + +/* STATUS0 register field */ +#define RESETC BIT(6) /* Reset Complete */ + #define TC6_HDR_SIZE 4 /* Ctrl command header size as per OA */ #define TC6_FTR_SIZE 4 /* Ctrl command footer size ss per OA */ struct oa_tc6 { struct spi_device *spi; bool ctrl_prot; + struct task_struct *tc6_task; + wait_queue_head_t tc6_wq; + bool int_flag; + struct completion rst_complete; }; struct oa_tc6 *oa_tc6_init(struct spi_device *spi); -void oa_tc6_deinit(struct oa_tc6 *tc6); +int oa_tc6_deinit(struct oa_tc6 *tc6); int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 value[], u8 len); int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 value[], u8 len);
Register MAC-PHY interrupt and handle reset complete interrupt. Reset complete bit is set when the MAC-PHY reset complete 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 | 141 ++++++++++++++++++++++++++++++++-- include/linux/oa_tc6.h | 16 +++- 2 files changed, 150 insertions(+), 7 deletions(-)