[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <852a61a5-8c15-1b5a-bea0-2f0d936722df@microchip.com>
Date: Tue, 19 Sep 2023 13:07:44 +0000
From: <Parthiban.Veerasooran@...rochip.com>
To: <andrew@...n.ch>
CC: <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>, <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>
Subject: Re: [RFC PATCH net-next 2/6] net: ethernet: add mac-phy interrupt
support with reset complete handling
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
>
Powered by blists - more mailing lists