Message-ID: <da16882b-aea8-486b-5541-2919ff227458@microchip.com>
Date:   Tue, 19 Sep 2023 13:40:29 +0000
From:   <Parthiban.Veerasooran@...rochip.com>
To:     <lukma@...x.de>
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>, <andrew@...n.ch>
Subject: Re: [RFC PATCH net-next 2/6] net: ethernet: add mac-phy interrupt
 support with reset complete handling

Hi Lukasz,

Sorry for the delayed response. Regarding your issue, we just noticed 
that you have also filed an issue in our oa tc6 lib github page. Our oa 
tc6 lib for controllers developer Thorsten will get back to you on this. 
You can get the solution from there.

https://github.com/MicrochipTech/oa-tc6-lib/issues/14

Best Regards,
Parthiban V

On 13/09/23 6:56 pm, Lukasz Majewski wrote:
> Hi Andrew,
> 
>>> Just maybe mine small remark. IMHO the reset shall not pollute the
>>> IRQ hander. The RESETC is just set on the initialization phase and
>>> only then shall be served. Please correct me if I'm wrong, but it
>>> will not be handled during "normal" operation.
>>
>> This is something i also wondered. Maybe if the firmware in the
>> MAC-PHY crashes, burns, and a watchdog reset it, could it assert
>> RESETC? I think maybe a WARN_ON_ONCE() for RESETC in the interrupt
>> handler would be useful, but otherwise ignore it. Probe can then poll
>> during its reset.
>>
>>>> +				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);
>>>
>>> Is this enough to have the IRQ_N deasserted (i.e. pulled HIGH)?
>>>
>>> The documentation states it clearly that one also needs to set SYNC
>>> bit (BIT(15)) in the OA_CONFIG0 register (which would have the
>>> 0x8006 value).
>>>
>>> Mine problem is that even after writing 0x40 to OA_STATUS0 and
>>> 0x8006 to OA_CONFIG0 the IRQ_N is still LOW (it is pulled up via
>>> 10K resistor).
>>>
>>> (I'm able to read those registers and those show expected values)
>>
>> What does STATUS0 and STATUS1 contain?
> 
> STATUS0 => 0x40, which is expected.
> 
> Then I do write 0x40 to STATUS0 -> bit6 (RESETC) is R/W1C
> 
> After reading the same register - I do receive 0x00 (it has been
> cleared).
> 
> Then I write 0x8006 to OA_CONFIG0.
> 
> (Those two steps are regarded as "configuration" of LAN865x device in
> the documentation)
> 
> In this patch set -> the OA_COFIG0 also has the 0x6 added to indicate
> the SPI transfer chunk.
> 
> Dump of OA registers:
> {0x11, 0x7c1b3, 0x5e5, 0x0, 0x8006, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
> 0x3000, 0x1fbf, 0x3ffe0003, 0x0, 0x0}
> 
> Status 0 (0x8) -> 0x0
> Status 1 (0x9) -> 0x0
> 
>> That might be a dumb question,
>> i've not read the details for interrupt handling yet, but maybe there
>> is another interrupt pending? Or the interrupt mask needs writing?
> 
> All the interrupts on MASK{01} are masked.
> 
> Changing it to:
> sts &= ~(OA_IMASK0_TXPEM | OA_IMASK0_TXBOEM | OA_IMASK0_TXBUEM |
> OA_IMASK0_RXBOEM | OA_IMASK0_LOFEM | OA_IMASK0_HDREM
> 
> doesn't fix this problem.
> 
>>
>>> Was it on purpose to not use the RST_N pin to perform GPIO based
>>> reset?
>>>
>>> When I generate reset pulse (and keep it for low for > 5us) the
>>> IRQ_N gets high. After some time it gets low (as expected). But
>>> then it doesn't get high any more.
>>
>> Does the standard say RST_N is mandatory to be controlled by software?
>> I could imagine RST_N is tied to the board global reset when the power
>> supply is stable.
> 
> It can be GPIO controlled. However, it is not required. I've tied it to
> 3V3 and also left NC, but no change.
> 
>> Software reset is then used at probe time.
> 
> I've reconfigured the board to use only SW based reset (i.e. set bit0
> in OA_RESET - 0x3).
> 
>>
>> So this could be a board design decision. I can see this code getting
>> extended in the future, an optional gpiod passed to the core for it to
>> use.
> 
> I can omit the RST_N control. I'd just expect the IRQ_N to be high
> after reset.
> 
>>
>>>> msecs_to_jiffies(1));
>>>
>>> Please also clarify - does the LAN8651 require up to 1ms "settle
>>> down" (after reset) time before it gets operational again?
>>
>> If this is not part of the standard, it really should be in the MAC
>> driver, or configurable, since different devices might need different
>> delays. But ideally, if the status bit says it is good to go, i would
>> really expect it to be good to go. So this probably should be a
>> LAN8651 quirk.
> 
> The documentation is silent about the "settle down time". The only
> requirements is for RST_N assertion > 5us. However, when the IRQ_N goes
> low, and the interrupt is served - it happens that I cannot read ID
> from the chip via SPI.
> 
>>
>> 	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

Powered by blists - more mailing lists