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

[Russell King (Oracle)]

On Wed, Nov 29, 2023 at 12:03:14PM +0100, Christian Marangi wrote:
> 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.

The phylib comment deviates from standard practice elsewhere in the
kernel, where it is totally fine to do hardware setup in the probe()
function. So I would suggest just updating the comment on probe() to
remove the idea that one shouldn't be doing this. I think it's totally
fine to be doing setup in probe().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!