| Message ID | 1441936878-18290-2-git-send-email-horms+renesas@verge.net.au (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Simon Horman |
| Headers | show |
Le 09/10/15 19:01, Simon Horman a écrit : > Add a helper to allow ethernet drivers to limit the speed of a phy > (that they are attached to). > > This mainly involves factoring out the business-end of > of_set_phy_supported() and exporting a new symbol. > > This code seems to be open coded in several places, in several different > variants. > > This code is envisaged this will be used in situations where setting > the "max-speed" property is not appropriate, e.g. because the maximum > speed is not a property of the phy hardware. This looks good to me, one minor comment, see below: > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > --- > > v2 > * First post > --- > drivers/net/phy/phy_device.c | 52 ++++++++++++++++++++++++++++---------------- > include/linux/phy.h | 1 + > 2 files changed, 34 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index c0f211127274..d9a020095972 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1205,6 +1205,37 @@ static int gen10g_resume(struct phy_device *phydev) > return 0; > } > > +static void __set_phy_supported(struct phy_device *phydev, u32 max_speed) > +{ > + if (!IS_ENABLED(CONFIG_OF_MDIO)) > + return; I think that part should be moved to of_set_phy_supported(), since your are exporting phy_set_max_speed() which should therefore be available regardless of whether Device Tree is used. While you are it, it might be nice to either warn or return -ENOTSUPP if the speed does not match 10, 100 or 1000, but that might be worth a second patch. Thanks!
On Thu, Sep 10, 2015 at 09:14:34PM -0700, Florian Fainelli wrote: > Le 09/10/15 19:01, Simon Horman a écrit : > > Add a helper to allow ethernet drivers to limit the speed of a phy > > (that they are attached to). > > > > This mainly involves factoring out the business-end of > > of_set_phy_supported() and exporting a new symbol. > > > > This code seems to be open coded in several places, in several different > > variants. > > > > This code is envisaged this will be used in situations where setting > > the "max-speed" property is not appropriate, e.g. because the maximum > > speed is not a property of the phy hardware. > > This looks good to me, one minor comment, see below: > > > > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > > > --- > > > > v2 > > * First post > > --- > > drivers/net/phy/phy_device.c | 52 ++++++++++++++++++++++++++++---------------- > > include/linux/phy.h | 1 + > > 2 files changed, 34 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > > index c0f211127274..d9a020095972 100644 > > --- a/drivers/net/phy/phy_device.c > > +++ b/drivers/net/phy/phy_device.c > > @@ -1205,6 +1205,37 @@ static int gen10g_resume(struct phy_device *phydev) > > return 0; > > } > > > > +static void __set_phy_supported(struct phy_device *phydev, u32 max_speed) > > +{ > > + if (!IS_ENABLED(CONFIG_OF_MDIO)) > > + return; > > I think that part should be moved to of_set_phy_supported(), since your > are exporting phy_set_max_speed() which should therefore be available > regardless of whether Device Tree is used. Yes of course, silly me. > While you are it, it might be nice to either warn or return -ENOTSUPP if > the speed does not match 10, 100 or 1000, but that might be worth a > second patch. Sure, that sounds reasonable. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 9/11/2015 5:01 AM, Simon Horman wrote: > Add a helper to allow ethernet drivers to limit the speed of a phy > (that they are attached to). > > This mainly involves factoring out the business-end of > of_set_phy_supported() and exporting a new symbol. > > This code seems to be open coded in several places, in several different > variants. > > This code is envisaged this will be used in situations where setting > the "max-speed" property is not appropriate, e.g. because the maximum > speed is not a property of the phy hardware. > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > --- > > v2 > * First post > --- > drivers/net/phy/phy_device.c | 52 ++++++++++++++++++++++++++++---------------- > include/linux/phy.h | 1 + > 2 files changed, 34 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index c0f211127274..d9a020095972 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1205,6 +1205,37 @@ static int gen10g_resume(struct phy_device *phydev) > return 0; > } > > +static void __set_phy_supported(struct phy_device *phydev, u32 max_speed) > +{ > + if (!IS_ENABLED(CONFIG_OF_MDIO)) > + return; > + > + /* The default values for phydev->supported are provided by the PHY > + * driver "features" member, we want to reset to sane defaults fist s/fist/first/. > + * before supporting higher speeds. > + */ > + phydev->supported &= PHY_DEFAULT_FEATURES; > + > + switch (max_speed) { > + default: > + return; > + I don't think empty line is needed here. > + case SPEED_1000: > + phydev->supported |= PHY_1000BT_FEATURES; Need a comment like /* Fall thru */. > + case SPEED_100: > + phydev->supported |= PHY_100BT_FEATURES; And here. > + case SPEED_10: > + phydev->supported |= PHY_10BT_FEATURES; > + } > +} [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index c0f211127274..d9a020095972 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1205,6 +1205,37 @@ static int gen10g_resume(struct phy_device *phydev) return 0; } +static void __set_phy_supported(struct phy_device *phydev, u32 max_speed) +{ + if (!IS_ENABLED(CONFIG_OF_MDIO)) + return; + + /* The default values for phydev->supported are provided by the PHY + * driver "features" member, we want to reset to sane defaults fist + * before supporting higher speeds. + */ + phydev->supported &= PHY_DEFAULT_FEATURES; + + switch (max_speed) { + default: + return; + + case SPEED_1000: + phydev->supported |= PHY_1000BT_FEATURES; + case SPEED_100: + phydev->supported |= PHY_100BT_FEATURES; + case SPEED_10: + phydev->supported |= PHY_10BT_FEATURES; + } +} + +void phy_set_max_speed(struct phy_device *phydev, u32 max_speed) +{ + __set_phy_supported(phydev, max_speed); + phydev->advertising = phydev->supported; +} +EXPORT_SYMBOL(phy_set_max_speed); + static void of_set_phy_supported(struct phy_device *phydev) { struct device_node *node = phydev->dev.of_node; @@ -1216,25 +1247,8 @@ static void of_set_phy_supported(struct phy_device *phydev) if (!node) return; - if (!of_property_read_u32(node, "max-speed", &max_speed)) { - /* The default values for phydev->supported are provided by the PHY - * driver "features" member, we want to reset to sane defaults fist - * before supporting higher speeds. - */ - phydev->supported &= PHY_DEFAULT_FEATURES; - - switch (max_speed) { - default: - return; - - case SPEED_1000: - phydev->supported |= PHY_1000BT_FEATURES; - case SPEED_100: - phydev->supported |= PHY_100BT_FEATURES; - case SPEED_10: - phydev->supported |= PHY_10BT_FEATURES; - } - } + if (!of_property_read_u32(node, "max-speed", &max_speed)) + __set_phy_supported(phydev, max_speed); } /** diff --git a/include/linux/phy.h b/include/linux/phy.h index 962387a192f1..692b202a52af 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -794,6 +794,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd); int phy_start_interrupts(struct phy_device *phydev); void phy_print_status(struct phy_device *phydev); void phy_device_free(struct phy_device *phydev); +void phy_set_max_speed(struct phy_device *phydev, u32 max_speed); int phy_register_fixup(const char *bus_id, u32 phy_uid, u32 phy_uid_mask, int (*run)(struct phy_device *));
Add a helper to allow ethernet drivers to limit the speed of a phy (that they are attached to). This mainly involves factoring out the business-end of of_set_phy_supported() and exporting a new symbol. This code seems to be open coded in several places, in several different variants. This code is envisaged this will be used in situations where setting the "max-speed" property is not appropriate, e.g. because the maximum speed is not a property of the phy hardware. Signed-off-by: Simon Horman <horms+renesas@verge.net.au> --- v2 * First post --- drivers/net/phy/phy_device.c | 52 ++++++++++++++++++++++++++++---------------- include/linux/phy.h | 1 + 2 files changed, 34 insertions(+), 19 deletions(-)