| Message ID | 8d1e588f-72a4-ffff-f0f3-dbb071838a08@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: move getting (R)MII refclock to phylib | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 18 this patch: 18 |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 18 this patch: 18 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 62 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 3/24/23 11:05, Heiner Kallweit wrote: > Now that getting the reference clock has been moved to phylib, > we can remove it here. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> This is not the reference clock for bcm7xxx this is the SoC internal clock that feeds the Soc internal PHY.
On 24.03.2023 20:03, Florian Fainelli wrote: > On 3/24/23 11:05, Heiner Kallweit wrote: >> Now that getting the reference clock has been moved to phylib, >> we can remove it here. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > This is not the reference clock for bcm7xxx this is the SoC internal clock that feeds the Soc internal PHY. Ah, good to know. Then indeed we may have to allow drivers to disable this feature. Another aspect: When looking at ba4ee3c05365 "net: phy: bcm7xxx: request and manage GPHY clock" I stumbled across statement "PHY driver can be probed with the clocks turned off". I interpret this in a way that dynamic PHY detection doesn't work because the PHY ID registers aren't accessible before the PHY driver has been loaded and probed. Is this right? Should the MDIO bus driver enable the clock for the PHY?
On 3/24/23 12:50, Heiner Kallweit wrote: > On 24.03.2023 20:03, Florian Fainelli wrote: >> On 3/24/23 11:05, Heiner Kallweit wrote: >>> Now that getting the reference clock has been moved to phylib, >>> we can remove it here. >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> >> This is not the reference clock for bcm7xxx this is the SoC internal clock that feeds the Soc internal PHY. > > Ah, good to know. Then indeed we may have to allow drivers to disable this feature. > > Another aspect: When looking at ba4ee3c05365 "net: phy: bcm7xxx: request and manage GPHY clock" > I stumbled across statement "PHY driver can be probed with the clocks turned off". > I interpret this in a way that dynamic PHY detection doesn't work because the PHY ID > registers aren't accessible before the PHY driver has been loaded and probed. Is this right? Yes this is correct we actually probe with the clock turned off as we try to run as low power as possible upon boot. > Should the MDIO bus driver enable the clock for the PHY? > This is what I had done in our downstream MDIO bus driver initially and this was fine because we were guaranteed to use a specific MDIO bus driver since the PHY is integrated. Eventually when this landed upstream I went with specifying the Ethernet PHY compatible with the "ethernet-phy-idAAAA.BBBB" notation which forces the PHY library to match the compatible with the driver directly without requiring to read from MII_PHYSID1/2. The problems I saw with the MDIO bus approach was that: - you would have to enable the clock prior to scanning the bus which could be done in mii_bus::reset for a driver specific way of doing it, or directly in mdiobus_scan() and then you would have to balance the clock enable count within the PHY driver's probe function which required using __clk_is_enabled() to ensure the clock could be disabled later on when you unbind the PHY device from its driver or during remove, or suspend/resume - if the PHY device tree node specified multiple clocks, you would not necessarily know which one(s) to enable and which one(s) not to. Enabling all of them would be a waste of power and could also possibly create sequencing issues if we have a situation similar to the reference clock you are trying to address. Not enabling any would obviously not work at all. Using the "ethernet-phy-idAAAA.BBBB" ensured that the PHY driver could enable the clock(s) it needs and ensure that probe() and remove() would have balanced clock enable/disable calls.
On 24.03.2023 21:11, Florian Fainelli wrote: > On 3/24/23 12:50, Heiner Kallweit wrote: >> On 24.03.2023 20:03, Florian Fainelli wrote: >>> On 3/24/23 11:05, Heiner Kallweit wrote: >>>> Now that getting the reference clock has been moved to phylib, >>>> we can remove it here. >>>> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> >>> This is not the reference clock for bcm7xxx this is the SoC internal clock that feeds the Soc internal PHY. >> >> Ah, good to know. Then indeed we may have to allow drivers to disable this feature. >> >> Another aspect: When looking at ba4ee3c05365 "net: phy: bcm7xxx: request and manage GPHY clock" >> I stumbled across statement "PHY driver can be probed with the clocks turned off". >> I interpret this in a way that dynamic PHY detection doesn't work because the PHY ID >> registers aren't accessible before the PHY driver has been loaded and probed. Is this right? > > Yes this is correct we actually probe with the clock turned off as we try to run as low power as possible upon boot. > >> Should the MDIO bus driver enable the clock for the PHY? >> > > This is what I had done in our downstream MDIO bus driver initially and this was fine because we were guaranteed to use a specific MDIO bus driver since the PHY is integrated. > > Eventually when this landed upstream I went with specifying the Ethernet PHY compatible with the "ethernet-phy-idAAAA.BBBB" notation which forces the PHY library to match the compatible with the driver directly without requiring to read from MII_PHYSID1/2. > > The problems I saw with the MDIO bus approach was that: > > - you would have to enable the clock prior to scanning the bus which could be done in mii_bus::reset for a driver specific way of doing it, or directly in mdiobus_scan() and then you would have to balance the clock enable count within the PHY driver's probe function which required using __clk_is_enabled() to ensure the clock could be disabled later on when you unbind the PHY device from its driver or during remove, or suspend/resume > > - if the PHY device tree node specified multiple clocks, you would not necessarily know which one(s) to enable and which one(s) not to. Enabling all of them would be a waste of power and could also possibly create sequencing issues if we have a situation similar to the reference clock you are trying to address. Not enabling any would obviously not work at all. > > Using the "ethernet-phy-idAAAA.BBBB" ensured that the PHY driver could enable the clock(s) it needs and ensure that probe() and remove() would have balanced clock enable/disable calls. I see, thanks for the comprehensive explanation. If we need an additional PHY driver flag or other measures, then I wonder whether it's worth it to add refclock handling to phylib for two drivers. Maybe not.
On 3/24/23 14:16, Heiner Kallweit wrote: > On 24.03.2023 21:11, Florian Fainelli wrote: >> On 3/24/23 12:50, Heiner Kallweit wrote: >>> On 24.03.2023 20:03, Florian Fainelli wrote: >>>> On 3/24/23 11:05, Heiner Kallweit wrote: >>>>> Now that getting the reference clock has been moved to phylib, >>>>> we can remove it here. >>>>> >>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>>> >>>> This is not the reference clock for bcm7xxx this is the SoC internal clock that feeds the Soc internal PHY. >>> >>> Ah, good to know. Then indeed we may have to allow drivers to disable this feature. >>> >>> Another aspect: When looking at ba4ee3c05365 "net: phy: bcm7xxx: request and manage GPHY clock" >>> I stumbled across statement "PHY driver can be probed with the clocks turned off". >>> I interpret this in a way that dynamic PHY detection doesn't work because the PHY ID >>> registers aren't accessible before the PHY driver has been loaded and probed. Is this right? >> >> Yes this is correct we actually probe with the clock turned off as we try to run as low power as possible upon boot. >> >>> Should the MDIO bus driver enable the clock for the PHY? >>> >> >> This is what I had done in our downstream MDIO bus driver initially and this was fine because we were guaranteed to use a specific MDIO bus driver since the PHY is integrated. >> >> Eventually when this landed upstream I went with specifying the Ethernet PHY compatible with the "ethernet-phy-idAAAA.BBBB" notation which forces the PHY library to match the compatible with the driver directly without requiring to read from MII_PHYSID1/2. >> >> The problems I saw with the MDIO bus approach was that: >> >> - you would have to enable the clock prior to scanning the bus which could be done in mii_bus::reset for a driver specific way of doing it, or directly in mdiobus_scan() and then you would have to balance the clock enable count within the PHY driver's probe function which required using __clk_is_enabled() to ensure the clock could be disabled later on when you unbind the PHY device from its driver or during remove, or suspend/resume >> >> - if the PHY device tree node specified multiple clocks, you would not necessarily know which one(s) to enable and which one(s) not to. Enabling all of them would be a waste of power and could also possibly create sequencing issues if we have a situation similar to the reference clock you are trying to address. Not enabling any would obviously not work at all. >> >> Using the "ethernet-phy-idAAAA.BBBB" ensured that the PHY driver could enable the clock(s) it needs and ensure that probe() and remove() would have balanced clock enable/disable calls. > > I see, thanks for the comprehensive explanation. If we need an additional > PHY driver flag or other measures, then I wonder whether it's worth it > to add refclock handling to phylib for two drivers. Maybe not. Agreed that two drivers may not be that many. Situations like stmmac or other drivers where there may be a need for the PHY clock to run for the MAC's RX path to complete initializing might be solved using existing PHY library routines.
diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c index 75593e7d1..c608e0439 100644 --- a/drivers/net/phy/bcm7xxx.c +++ b/drivers/net/phy/bcm7xxx.c @@ -11,7 +11,6 @@ #include "bcm-phy-lib.h" #include <linux/bitops.h> #include <linux/brcmphy.h> -#include <linux/clk.h> #include <linux/mdio.h> /* Broadcom BCM7xxx internal PHY registers */ @@ -45,7 +44,6 @@ struct bcm7xxx_phy_priv { u64 *stats; - struct clk *clk; }; static int bcm7xxx_28nm_d0_afe_config_init(struct phy_device *phydev) @@ -825,14 +823,6 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev) if (!priv->stats) return -ENOMEM; - priv->clk = devm_clk_get_optional(&phydev->mdio.dev, NULL); - if (IS_ERR(priv->clk)) - return PTR_ERR(priv->clk); - - ret = clk_prepare_enable(priv->clk); - if (ret) - return ret; - /* Dummy read to a register to workaround an issue upon reset where the * internal inverter may not allow the first MDIO transaction to pass * the MDIO management controller and make us return 0xffff for such @@ -844,13 +834,6 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev) return ret; } -static void bcm7xxx_28nm_remove(struct phy_device *phydev) -{ - struct bcm7xxx_phy_priv *priv = phydev->priv; - - clk_disable_unprepare(priv->clk); -} - #define BCM7XXX_28NM_GPHY(_oui, _name) \ { \ .phy_id = (_oui), \ @@ -866,7 +849,6 @@ static void bcm7xxx_28nm_remove(struct phy_device *phydev) .get_strings = bcm_phy_get_strings, \ .get_stats = bcm7xxx_28nm_get_phy_stats, \ .probe = bcm7xxx_28nm_probe, \ - .remove = bcm7xxx_28nm_remove, \ } #define BCM7XXX_28NM_EPHY(_oui, _name) \ @@ -882,7 +864,6 @@ static void bcm7xxx_28nm_remove(struct phy_device *phydev) .get_strings = bcm_phy_get_strings, \ .get_stats = bcm7xxx_28nm_get_phy_stats, \ .probe = bcm7xxx_28nm_probe, \ - .remove = bcm7xxx_28nm_remove, \ .read_mmd = bcm7xxx_28nm_ephy_read_mmd, \ .write_mmd = bcm7xxx_28nm_ephy_write_mmd, \ } @@ -908,7 +889,6 @@ static void bcm7xxx_28nm_remove(struct phy_device *phydev) /* PHY_BASIC_FEATURES */ \ .flags = PHY_IS_INTERNAL, \ .probe = bcm7xxx_28nm_probe, \ - .remove = bcm7xxx_28nm_remove, \ .config_init = bcm7xxx_16nm_ephy_config_init, \ .config_aneg = genphy_config_aneg, \ .read_status = genphy_read_status, \
Now that getting the reference clock has been moved to phylib, we can remove it here. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/bcm7xxx.c | 20 -------------------- 1 file changed, 20 deletions(-)