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

[Heiner Kallweit]

On 15.05.2020 18:18, Florian Fainelli wrote:
> 
> 
> 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.
> 
First it's about the justification of the change as such, and second about the
question whether the change should be in the driver or in phylib.

Acking interrupts we do already if the PHY is configured for interrupt mode,
we call phy_clear_interrupt() at the beginning of phy_enable_interrupts()
and at the end of phy_disable_interrupts().
When using polling mode there is no strict need to ack interrupts.
If we say however that interrupts should be acked in general, then I think
it's not specific to RTL8211F, but it's something for phylib. Most likely
we would have to add a call to phy_clear_interrupt() to phy_init_hw().