| Message ID | 20231129021219.20914-3-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: at803x: cleanup + split | expand |
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.
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? Also I think it was wrong from the start as on reset I think WoL is not disabled again. (probe is not called) [1] https://elixir.bootlin.com/linux/latest/source/include/linux/phy.h#L916
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. > 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.
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.
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().
> if (phydev->drv->phy_id == ATH8031_PHY_ID) { > + /* Disable WoL in 1588 register which is enabled > + * by default > + */ > + ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, > + AT803X_PHY_MMD3_WOL_CTRL, > + AT803X_WOL_EN, 0); > + if (ret) > + return ret; > + Maybe it comes later in the patch series, but i would actually add a at8031_probe() which calls the common at803x_probe() and then does this WoL stuff. I don't see any reason to have just one probe, with if (phydev->drv->phy_id == ATH8031_PHY_ID) { in it. Andrew
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index ef203b0807e5..b32ff82240dc 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -886,15 +886,6 @@ static int at803x_probe(struct phy_device *phydev) priv->is_fiber = true; break; } - - /* Disable WoL in 1588 register which is enabled - * by default - */ - ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, - AT803X_PHY_MMD3_WOL_CTRL, - AT803X_WOL_EN, 0); - if (ret) - return ret; } return 0; @@ -1008,6 +999,15 @@ static int at803x_config_init(struct phy_device *phydev) int ret; if (phydev->drv->phy_id == ATH8031_PHY_ID) { + /* Disable WoL in 1588 register which is enabled + * by default + */ + ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, + AT803X_PHY_MMD3_WOL_CTRL, + AT803X_WOL_EN, 0); + if (ret) + return ret; + /* Some bootloaders leave the fiber page selected. * Switch to the appropriate page (fiber or copper), as otherwise we * read the PHY capabilities from the wrong page.
Probe should be used only for DT parsing and allocate required priv, it shouldn't touch regs, there is config_init for that. Move the WOL disable call from probe to config_init to follow this rule and keep code tidy. No behaviour is done as the mode was disabled only if phy_read succeeded in probe and this is translated as the first action done in config_init (called only if probe returns 0) Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- drivers/net/phy/at803x.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)