Date: Tue, 12 Sep 2023 12:28:53 +0000
From: <Parthiban.Veerasooran@...rochip.com>
To: <william.xuanziyang@...wei.com>
CC: <netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
	<Horatiu.Vultur@...rochip.com>, <Woojung.Huh@...rochip.com>,
	<Nicolas.Ferre@...rochip.com>, <UNGLinuxDriver@...rochip.com>,
	<Thorsten.Kummermehr@...rochip.com>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
	<conor+dt@...nel.org>, <corbet@....net>, <Steen.Hegelund@...rochip.com>,
	<rdunlap@...radead.org>, <horms@...nel.org>, <casper.casan@...il.com>,
	<andrew@...n.ch>
Subject: Re: [RFC PATCH net-next 2/6] net: ethernet: add mac-phy interrupt
 support with reset complete handling

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@...rochip.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, &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);
>>

Powered by blists - more mailing lists