Re: [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f

[Florian Fainelli]

On 5/15/2020 12:41 AM, Jisheng Zhang wrote:
> On Thu, 14 May 2020 21:50:53 +0200 Heiner Kallweit wrote:
> 
>>
>>
>> On 14.05.2020 08:25, Jisheng Zhang wrote:
>>> On Wed, 13 May 2020 20:45:13 +0200 Heiner Kallweit wrote:
>>>  
>>>>
>>>> On 13.05.2020 08:51, Jisheng Zhang wrote:  
>>>>> Hi,
>>>>>
>>>>> On Tue, 12 May 2020 20:43:40 +0200 Heiner Kallweit wrote:
>>>>>  
>>>>>>
>>>>>>
>>>>>> On 12.05.2020 12:46, Jisheng Zhang wrote:  
>>>>>>> The PHY Register Accessible Interrupt is enabled by default, so
>>>>>>> there's such an interrupt during init. In PHY POLL mode case, the
>>>>>>> INTB/PMEB pin is alway active, it is not good. Clear the interrupt by
>>>>>>> calling rtl8211f_ack_interrupt().  
>>>>>>
>>>>>> As you say "it's not good" w/o elaborating a little bit more on it:
>>>>>> Do you face any actual issue? Or do you just think that it's not nice?  
>>>>>
>>>>>
>>>>> The INTB/PMEB pin can be used in two different modes:
>>>>> INTB: used for interrupt
>>>>> PMEB: special mode for Wake-on-LAN
>>>>>
>>>>> The PHY Register Accessible Interrupt is enabled by
>>>>> default, there's always such an interrupt during the init. In PHY POLL mode
>>>>> case, the pin is always active. If platforms plans to use the INTB/PMEB pin
>>>>> as WOL, then the platform will see WOL active. It's not good.
>>>>>  
>>>> The platform should listen to this pin only once WOL has been configured and
>>>> the pin has been switched to PMEB function. For the latter you first would
>>>> have to implement the set_wol callback in the PHY driver.
>>>> Or where in which code do you plan to switch the pin function to PMEB?  
>>>
>>> I think it's better to switch the pin function in set_wol callback. But this
>>> is another story. No matter WOL has been configured or not, keeping the
>>> INTB/PMEB pin active is not good. what do you think?
>>>  
>>
>> It shouldn't hurt (at least it didn't hurt for the last years), because no
>> listener should listen to the pin w/o having it configured before.
>> So better extend the PHY driver first (set_wol, ..), and then do the follow-up
>> platform changes (e.g. DT config of a connected GPIO).
> 
> There are two sides involved here: the listener, it should not listen to the pin
> as you pointed out; the phy side, this patch tries to make the phy side
> behave normally -- not keep the INTB/PMEB pin always active. The listener
> side behaves correctly doesn't mean the phy side could keep the pin active.
> 
> When .set_wol isn't implemented, this patch could make the system suspend/resume
> work properly.
> 
> PS: even with set_wol implemented as configure the pin mode, I think we
> still need to clear the interrupt for phy poll mode either in set_wol
> or as this patch does.

I agree with Jisheng here, Heiner, is there a reason you are pushing
back on the change? Acknowledging prior interrupts while configuring the
PHY is a common and established practice.
-- 
Florian