Re: [net-next PATCH 02/14] net: phy: at803x: move disable WOL for 8031 from probe to config

[Christian Marangi]

On Wed, Nov 29, 2023 at 10:45:11AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 10:36:31AM +0100, Christian Marangi wrote:
> > On Wed, Nov 29, 2023 at 09:24:32AM +0000, Russell King (Oracle) wrote:
> > > On Wed, Nov 29, 2023 at 03:12:07AM +0100, Christian Marangi wrote:
> > > > Probe should be used only for DT parsing and allocate required priv, it
> > > > shouldn't touch regs, there is config_init for that.
> > > 
> > > I'm not sure where you get that idea from. PHY driver probe() functions
> > > are permitted to access registers to do any setup that they wish to.
> > > 
> > > config_init() is to configure the PHY for use with the network
> > > interface.
> > > 
> > > I think this patch is just noise rather than a cleanup.
> > >
> > 
> > I got it from here [1]
> > 
> > Also on every other driver probe was always used for allocation and
> > parsing so why deviates from this pattern here?
> 
> Untrue.
> 
> bcm54140_enable_monitoring() is called from bcm54140_probe_once()
> which in turn is called from bcm54140_probe().
> 
> dp83869_probe() calls dp83869_config_init(), rightly or wrongly.
> 
> lxt973_probe() fixes up the BMCR.
> 
> mv3310_probe() configures power-down modes, modifying registers.
> 
> mt7988_phy_probe() calls mt7988_phy_fix_leds_polarities() which
> modifies registers.
> 
> lan8814_probe() calls lan8814_ptp_init() which does a whole load of
> register writes.
> 
> lan88xx_probe() configures LEDs via register writes.
> 
> yt8521_probe() configures clocks via register modification.
> 
> I'm afraid this means your comment is demonstrably false.
>

Don't want to start a discussion and thanks a lot for pointing this
out. This is really to talk about this and not contradict you.

Yes it's not true, and I was wrong but still I watch other driver
outside PHY and normally probe should not do that kind of stuff.
(even the comments describing the use in phy.h doesn't say that it
should be used for configuring the PHY only once on discovery)

Watching some of the thing, I feel all of this is done in probe as it's
not called again on software reset (as it will call config_init again).

This looks like there is a missing feature here and maybe a chance to
improve this?

Wonder if adding an additional OP would be beneficial to this?
Was thinking to something like

.config_once ?

And add some comments that it's called only on PHY discovery?
It would be put right after the probe call in phy_core.

> > Also I think it was wrong from the start as on reset I think WoL is
> > not disabled again. (probe is not called)
> 
> On hardware reset, the 1588 register will re-enable the WoL pin, but
> that needs a hardware reset of the PHY to happen after probe() is
> called.
> 
> However, phy_probe() will only assert the reset signal _if_ an error
> occured during probing, not if probing was successful. So, a successful
> probe of this driver will not cause a hardware reset.
> 
> Also, hardware reset is optional. Do you know whether the platforms
> that use the separate WoL pin which this 1588 register controls also
> wire the reset signal such that it can be controlled by Linux?
> Probably not.
> 
> So, this register write will not be cleared by a hardware reset after
> a successful probe.
>

I just checked Datasheet, on HW reset it's enabled again and on SW reset
value is retained. So yes it must stay in probe as it will cause side
effect if WoL get enabled after... Really hope the config_once thing is
OK since it will make things much cleaner.

-- 
	Ansuel