Date:   Tue, 19 Sep 2023 15:21:37 +0200
From:   Lukasz Majewski <lukma@...x.de>
To:     <Parthiban.Veerasooran@...rochip.com>
Cc:     <andrew@...n.ch>, <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

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, &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’.

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@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists