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
