Message ID | 19c1e5c4-2f86-a3ff-e971-a0098c605b3f@gmail.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | [net-next] net: phy: Add support for asking the PHY its abilities | expand |
On Sat, Feb 09, 2019 at 03:24:47PM +0100, Heiner Kallweit wrote: > From: Andrew Lunn <andrew@lunn.ch> > Add support for runtime determination of what the PHY supports, by > adding a new function to the phy driver. The get_features call should > set the phydev->supported member with the features the PHY supports. > It is only called if phydrv->features is NULL. > > This requires minor changes to pause. The PHY driver should not set > pause abilities, except for when it has odd cause capabilities, e.g. > pause cannot be disabled. With this change, phydev->supported already > contains the drivers abilities, including pause. So rather than > considering phydrv->features, look at the phydev->supported, and > enable pause if neither of the pause bits are already set. Hi Heiner Ah, cool, these are the patches i was asking for, when you asked about splitting Maxime's patches. There is one more in my tree which converts the marvell10g to using this. I think that should be posted as well. It makes it clear how this should be used, and it replaces one of the patches in Maxime's set. > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > [hkallweit1@gmail.com: fixed small checkpatch complaint in one comment] Thanks. Andrew
From: Heiner Kallweit <hkallweit1@gmail.com> Date: Sat, 9 Feb 2019 15:24:47 +0100 > From: Andrew Lunn <andrew@lunn.ch> > Add support for runtime determination of what the PHY supports, by > adding a new function to the phy driver. The get_features call should > set the phydev->supported member with the features the PHY supports. > It is only called if phydrv->features is NULL. > > This requires minor changes to pause. The PHY driver should not set > pause abilities, except for when it has odd cause capabilities, e.g. > pause cannot be disabled. With this change, phydev->supported already > contains the drivers abilities, including pause. So rather than > considering phydrv->features, look at the phydev->supported, and > enable pause if neither of the pause bits are already set. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > [hkallweit1@gmail.com: fixed small checkpatch complaint in one comment] > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Applied.
On 09.02.2019 17:35, Andrew Lunn wrote: > On Sat, Feb 09, 2019 at 03:24:47PM +0100, Heiner Kallweit wrote: >> From: Andrew Lunn <andrew@lunn.ch> >> Add support for runtime determination of what the PHY supports, by >> adding a new function to the phy driver. The get_features call should >> set the phydev->supported member with the features the PHY supports. >> It is only called if phydrv->features is NULL. >> >> This requires minor changes to pause. The PHY driver should not set >> pause abilities, except for when it has odd cause capabilities, e.g. >> pause cannot be disabled. With this change, phydev->supported already >> contains the drivers abilities, including pause. So rather than >> considering phydrv->features, look at the phydev->supported, and >> enable pause if neither of the pause bits are already set. > > Hi Heiner > > Ah, cool, these are the patches i was asking for, when you asked > about splitting Maxime's patches. There is one more in my tree which > converts the marvell10g to using this. I think that should be posted > as well. It makes it clear how this should be used, and it replaces > one of the patches in Maxime's set. > I know, it's patch 15 in your series ;) And I'm aware that usually new core functionality is acceptable only if it comes together with a user, to avoid having billions of orphaned good ideas in the code. I focused on the core here to not get lost in all the new stuff, and to provide Maxime with some direction for adjusting his series. >> Signed-off-by: Andrew Lunn <andrew@lunn.ch> >> [hkallweit1@gmail.com: fixed small checkpatch complaint in one comment] > > Thanks. > > Andrew > Heiner
> I know, it's patch 15 in your series ;) And I'm aware that usually new > core functionality is acceptable only if it comes together with a user, > to avoid having billions of orphaned good ideas in the code. > I focused on the core here to not get lost in all the new stuff, and to > provide Maxime with some direction for adjusting his series. I'm just trying to avoid Maxime reimplementing something when we already have a patch: https://github.com/lunn/linux/commit/07e3fa8f183f05a09d969a9378534da35238eeb9 Maxime, feel free to cherry-pick this into your series. Andrew
On 09.02.2019 20:42, Andrew Lunn wrote: >> I know, it's patch 15 in your series ;) And I'm aware that usually new >> core functionality is acceptable only if it comes together with a user, >> to avoid having billions of orphaned good ideas in the code. >> I focused on the core here to not get lost in all the new stuff, and to >> provide Maxime with some direction for adjusting his series. > > I'm just trying to avoid Maxime reimplementing something when we > already have a patch: > Sure, I didn't mean Maxime should re-implement things we have in the pipe. I meant it more in a way that he gets an idea in which direction we're moving. > https://github.com/lunn/linux/commit/07e3fa8f183f05a09d969a9378534da35238eeb9 > > Maxime, feel free to cherry-pick this into your series. > I'll submit this one too. > Andrew > Heiner Oh, and I just saw: When we talk about/with somebody, we should add him to the mail addressee list.
On 09.02.2019 20:50, Heiner Kallweit wrote: > On 09.02.2019 20:42, Andrew Lunn wrote: >>> I know, it's patch 15 in your series ;) And I'm aware that usually new >>> core functionality is acceptable only if it comes together with a user, >>> to avoid having billions of orphaned good ideas in the code. >>> I focused on the core here to not get lost in all the new stuff, and to >>> provide Maxime with some direction for adjusting his series. >> >> I'm just trying to avoid Maxime reimplementing something when we >> already have a patch: >> > Sure, I didn't mean Maxime should re-implement things we have in the pipe. > I meant it more in a way that he gets an idea in which direction we're moving. > >> https://github.com/lunn/linux/commit/07e3fa8f183f05a09d969a9378534da35238eeb9 >> >> Maxime, feel free to cherry-pick this into your series. >> > I'll submit this one too. > Just saw that this patch depends on patch 5 of Maxime's series. And it needs a small change because the generic function was renamed to genphy_c45_pma_read_abilities(). So indeed it may be better if Maxime adds this patch to his series. One comment regarding the pause bits in Maxime's patch 5: If no pause bit is set the core automatically sets both. So we have to do this neither in the marvell10g driver nor in the generic read-abilities function. >> Andrew >> Heiner > Heiner > > Oh, and I just saw: When we talk about/with somebody, we should add him to the > mail addressee list. >
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 92b7a71df..8573d17ec 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2236,7 +2236,14 @@ static int phy_probe(struct device *dev) * a controller will attach, and may modify one * or both of these values */ - linkmode_copy(phydev->supported, phydrv->features); + if (phydrv->features) { + linkmode_copy(phydev->supported, phydrv->features); + } else { + err = phydrv->get_features(phydev); + if (err) + goto out; + } + of_set_phy_supported(phydev); linkmode_copy(phydev->advertising, phydev->supported); @@ -2256,20 +2263,8 @@ static int phy_probe(struct device *dev) * (e.g. hardware erratum) where the driver wants to set only one * of these bits. */ - if (test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydrv->features) || - test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydrv->features)) { - linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, - phydev->supported); - linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, - phydev->supported); - if (test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydrv->features)) - linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, - phydev->supported); - if (test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, - phydrv->features)) - linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, - phydev->supported); - } else { + if (!test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported) && + !test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported)) { linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported); linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, @@ -2315,7 +2310,11 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner) { int retval; - if (WARN_ON(!new_driver->features)) { + /* Either the features are hard coded, or dynamically + * determine. It cannot be both or neither + */ + if (WARN_ON((!new_driver->features && !new_driver->get_features) || + (new_driver->features && new_driver->get_features))) { pr_err("%s: Driver features are missing\n", new_driver->name); return -EINVAL; } diff --git a/include/linux/phy.h b/include/linux/phy.h index f41bf651f..d2ffae992 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -502,6 +502,12 @@ struct phy_driver { */ int (*probe)(struct phy_device *phydev); + /* + * Probe the hardware to determine what abilities it has. + * Should only set phydev->supported. + */ + int (*get_features)(struct phy_device *phydev); + /* PHY Power Management */ int (*suspend)(struct phy_device *phydev); int (*resume)(struct phy_device *phydev);