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

[<Parthiban.Veerasooran@microchip.com>]

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, &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;
>> +     }
> 
> 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
>