| Message ID | 20200512184601.40b1758a@xhacker.debian |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: realtek: clear interrupt during init for rtl8211f | expand |
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; >
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
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
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
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 >
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.
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.
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().
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
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
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;
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(+)