Re: [RFC PATCH net-next 2/6] net: ethernet: add mac-phy interrupt support with reset complete handling

From: Parthiban.Veerasooran
Date: Tue Sep 12 2023 - 08:29:32 EST

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@xxxxxxxxxxxxx>
>> ---
>> 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, &regval, 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,
>> + &regval, 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, &regval, 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, &regval, 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);
>>