linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f
@ 2020-05-12 10:46 Jisheng Zhang
  2020-05-12 18:43 ` Heiner Kallweit
  0 siblings, 1 reply; 11+ messages in thread
From: Jisheng Zhang @ 2020-05-12 10:46 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Russell King,
	David S. Miller
  Cc: netdev, linux-kernel

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().

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 drivers/net/phy/realtek.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 2d99e9de6ee1..398607268a3c 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -179,6 +179,10 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	u16 val_txdly, val_rxdly;
 	int ret;
 
+	ret = rtl8211f_ack_interrupt(phydev);
+	if (ret < 0)
+		return ret;
+
 	switch (phydev->interface) {
 	case PHY_INTERFACE_MODE_RGMII:
 		val_txdly = 0;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f
  2020-05-12 10:46 [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f Jisheng Zhang
@ 2020-05-12 18:43 ` Heiner Kallweit
  2020-05-13  6:51   ` Jisheng Zhang
  0 siblings, 1 reply; 11+ messages in thread
From: Heiner Kallweit @ 2020-05-12 18:43 UTC (permalink / raw)
  To: Jisheng Zhang, Andrew Lunn, Florian Fainelli, Russell King,
	David S. Miller
  Cc: netdev, linux-kernel

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?
I'm asking because you don't provide a Fixes tag and you don't
annotate your patch as net or net-next.
Once you provide more details we would also get an idea whether a
change would have to be made to phylib, because what you describe
doesn't seem to be specific to this one PHY model.

> 
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> ---
>  drivers/net/phy/realtek.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 2d99e9de6ee1..398607268a3c 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -179,6 +179,10 @@ static int rtl8211f_config_init(struct phy_device *phydev)
>  	u16 val_txdly, val_rxdly;
>  	int ret;
>  
> +	ret = rtl8211f_ack_interrupt(phydev);
> +	if (ret < 0)
> +		return ret;
> +
>  	switch (phydev->interface) {
>  	case PHY_INTERFACE_MODE_RGMII:
>  		val_txdly = 0;
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f
  2020-05-12 18:43 ` Heiner Kallweit
@ 2020-05-13  6:51   ` Jisheng Zhang
  2020-05-13 18:45     ` Heiner Kallweit
  0 siblings, 1 reply; 11+ messages in thread
From: Jisheng Zhang @ 2020-05-13  6:51 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Andrew Lunn, Florian Fainelli, Russell King, David S. Miller,
	netdev, linux-kernel

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.


> I'm asking because you don't provide a Fixes tag and you don't
> annotate your patch as net or net-next.

should be Fixes: 3447cf2e9a11 ("net/phy: Add support for Realtek RTL8211F")

> Once you provide more details we would also get an idea whether a
> change would have to be made to phylib, because what you describe
> doesn't seem to be specific to this one PHY model.

Nope, we don't need this change in phylib, this is specific to rtl8211f

Thanks,
Jisheng

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f
  2020-05-13  6:51   ` Jisheng Zhang
@ 2020-05-13 18:45     ` Heiner Kallweit
  2020-05-14  6:25       ` Jisheng Zhang
  0 siblings, 1 reply; 11+ messages in thread
From: Heiner Kallweit @ 2020-05-13 18:45 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Andrew Lunn, Florian Fainelli, Russell King, David S. Miller,
	netdev, linux-kernel

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?
One more thing to consider when implementing set_wol would be that the PHY
supports two WOL options:
1. INT/PMEB configured as PMEB
2. INT/PMEB configured as INT and WOL interrupt source active

> 
>> I'm asking because you don't provide a Fixes tag and you don't
>> annotate your patch as net or net-next.
> 
> should be Fixes: 3447cf2e9a11 ("net/phy: Add support for Realtek RTL8211F")
> 
>> Once you provide more details we would also get an idea whether a
>> change would have to be made to phylib, because what you describe
>> doesn't seem to be specific to this one PHY model.
> 
> Nope, we don't need this change in phylib, this is specific to rtl8211f
> 
> Thanks,
> Jisheng
> 
Heiner

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f
  2020-05-13 18:45     ` Heiner Kallweit
@ 2020-05-14  6:25       ` Jisheng Zhang
  2020-05-14 19:50         ` Heiner Kallweit
  0 siblings, 1 reply; 11+ messages in thread
From: Jisheng Zhang @ 2020-05-14  6:25 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Andrew Lunn, Florian Fainelli, Russell King, David S. Miller,
	netdev, linux-kernel

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?

> One more thing to consider when implementing set_wol would be that the PHY
> supports two WOL options:
> 1. INT/PMEB configured as PMEB
> 2. INT/PMEB configured as INT and WOL interrupt source active
> 
> >  
> >> I'm asking because you don't provide a Fixes tag and you don't
> >> annotate your patch as net or net-next.  
> >
> > should be Fixes: 3447cf2e9a11 ("net/phy: Add support for Realtek RTL8211F")
> >  
> >> Once you provide more details we would also get an idea whether a
> >> change would have to be made to phylib, because what you describe
> >> doesn't seem to be specific to this one PHY model.  
> >
> > Nope, we don't need this change in phylib, this is specific to rtl8211f
> >
> > Thanks,
> > Jisheng
> >  
> Heiner


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f
  2020-05-14  6:25       ` Jisheng Zhang
@ 2020-05-14 19:50         ` Heiner Kallweit
  2020-05-15  7:41           ` Jisheng Zhang
  0 siblings, 1 reply; 11+ messages in thread
From: Heiner Kallweit @ 2020-05-14 19:50 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Andrew Lunn, Florian Fainelli, Russell King, David S. Miller,
	netdev, linux-kernel

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).

>> One more thing to consider when implementing set_wol would be that the PHY
>> supports two WOL options:
>> 1. INT/PMEB configured as PMEB
>> 2. INT/PMEB configured as INT and WOL interrupt source active
>>
>>>  
>>>> I'm asking because you don't provide a Fixes tag and you don't
>>>> annotate your patch as net or net-next.  
>>>
>>> should be Fixes: 3447cf2e9a11 ("net/phy: Add support for Realtek RTL8211F")
>>>  
>>>> Once you provide more details we would also get an idea whether a
>>>> change would have to be made to phylib, because what you describe
>>>> doesn't seem to be specific to this one PHY model.  
>>>
>>> Nope, we don't need this change in phylib, this is specific to rtl8211f
>>>
>>> Thanks,
>>> Jisheng
>>>  
>> Heiner
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f
  2020-05-14 19:50         ` Heiner Kallweit
@ 2020-05-15  7:41           ` Jisheng Zhang
  2020-05-15 16:18             ` Florian Fainelli
  0 siblings, 1 reply; 11+ messages in thread
From: Jisheng Zhang @ 2020-05-15  7:41 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Andrew Lunn, Florian Fainelli, Russell King, David S. Miller,
	netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f
  2020-05-15  7:41           ` Jisheng Zhang
@ 2020-05-15 16:18             ` Florian Fainelli
  2020-05-15 17:30               ` Heiner Kallweit
  0 siblings, 1 reply; 11+ messages in thread
From: Florian Fainelli @ 2020-05-15 16:18 UTC (permalink / raw)
  To: Jisheng Zhang, Heiner Kallweit
  Cc: Andrew Lunn, Russell King, David S. Miller, netdev, linux-kernel



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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f
  2020-05-15 16:18             ` Florian Fainelli
@ 2020-05-15 17:30               ` Heiner Kallweit
  2020-06-17  9:09                 ` Jisheng Zhang
  0 siblings, 1 reply; 11+ messages in thread
From: Heiner Kallweit @ 2020-05-15 17:30 UTC (permalink / raw)
  To: Florian Fainelli, Jisheng Zhang
  Cc: Andrew Lunn, Russell King, David S. Miller, netdev, linux-kernel

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().

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f
  2020-05-15 17:30               ` Heiner Kallweit
@ 2020-06-17  9:09                 ` Jisheng Zhang
  2020-06-21 20:16                   ` Heiner Kallweit
  0 siblings, 1 reply; 11+ messages in thread
From: Jisheng Zhang @ 2020-06-17  9:09 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Florian Fainelli, Andrew Lunn, Russell King, David S. Miller,
	netdev, linux-kernel

On Fri, 15 May 2020 19:30:38 +0200 Heiner Kallweit wrote:


> 
> 
> 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().

it's specific to RTL8211F from the following two PoV:
1. the PIN is shared between INTB and PMEB.
2. the PHY Register Accessible Interrupt is enabled by default

I didn't see such behaviors with other PHYs.

Thanks

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f
  2020-06-17  9:09                 ` Jisheng Zhang
@ 2020-06-21 20:16                   ` Heiner Kallweit
  0 siblings, 0 replies; 11+ messages in thread
From: Heiner Kallweit @ 2020-06-21 20:16 UTC (permalink / raw)
  To: Jisheng Zhang, Florian Fainelli, Andrew Lunn, Russell King
  Cc: David S. Miller, netdev, linux-kernel

On 17.06.2020 11:09, Jisheng Zhang wrote:
> On Fri, 15 May 2020 19:30:38 +0200 Heiner Kallweit wrote:
> 
> 
>>
>>
>> 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().
> 
> it's specific to RTL8211F from the following two PoV:
> 1. the PIN is shared between INTB and PMEB.
> 2. the PHY Register Accessible Interrupt is enabled by default
> 
If we clear the interrupt in config_init() and one interrupt source is
active per chip default, then wouldn't the irq pin be active soon again?

I was thinking about calling phy_disable_interrupts() in phy_init_hw(),
to have a defined init state, as we don't know in which state the PHY is
if the PHY driver is loaded. We shouldn't assume that it's the chip
power-on defaults, BIOS or boot loader could have changed this.
Or in case of dual-boot systems the other OS could leave the PHY in
whatever state.

This made me think about an issue we may have currently:
Interrupts are enabled in phy_request_interrupt() only. If the system
hibernates, then PHY may load power-on defaults on restore.
And mdio_bus_phy_restore() just calls phy_init_hw() and doesn't
care about interrupt config. Means after waking up from hibernation
we may have lost PHY interrupt config.

> I didn't see such behaviors with other PHYs.
> 
> Thanks
> 
Heiner

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-06-21 20:17 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-12 10:46 [PATCH] net: phy: realtek: clear interrupt during init for rtl8211f Jisheng Zhang
2020-05-12 18:43 ` Heiner Kallweit
2020-05-13  6:51   ` Jisheng Zhang
2020-05-13 18:45     ` Heiner Kallweit
2020-05-14  6:25       ` Jisheng Zhang
2020-05-14 19:50         ` Heiner Kallweit
2020-05-15  7:41           ` Jisheng Zhang
2020-05-15 16:18             ` Florian Fainelli
2020-05-15 17:30               ` Heiner Kallweit
2020-06-17  9:09                 ` Jisheng Zhang
2020-06-21 20:16                   ` Heiner Kallweit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).