| Message ID | 20181008234949.15416-2-grygorii.strashko@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: ethernet: ti: cpsw: replace cpsw-phy-sel with phy driver | expand |
Hi Grygorii, On Tuesday 09 October 2018 05:19 AM, Grygorii Strashko wrote: > Add new API phy_set_netif_mode(struct phy *phy, phy_interface_t mode) and > new PHY operation callback .set_netif_mode() which intended to be implemnte > by PHY drivers which supports Network interrfaces mode selection. Both > accepts phy_interface_t vlaue as input parameter. > > Cc: Kishon Vijay Abraham I <kishon@ti.com> > Cc: Tony Lindgren <tony@atomide.com> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > drivers/phy/phy-core.c | 15 +++++++++++++++ > include/linux/phy/phy.h | 12 ++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > index 35fd38c..d9aba1a 100644 > --- a/drivers/phy/phy-core.c > +++ b/drivers/phy/phy-core.c > @@ -377,6 +377,21 @@ int phy_set_mode(struct phy *phy, enum phy_mode mode) > } > EXPORT_SYMBOL_GPL(phy_set_mode); > > +int phy_set_netif_mode(struct phy *phy, phy_interface_t mode) > +{ > + int ret; > + > + if (!phy || !phy->ops->set_netif_mode) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->set_netif_mode(phy, mode); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_set_netif_mode); We should try to add only generic PHY APIs and not subsystem specific APIs. In this case I think phy_set_mode should suffice. Thanks Kishon > + > int phy_reset(struct phy *phy) > { > int ret; > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index 9713aeb..bc73d2b 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -17,6 +17,7 @@ > #include <linux/err.h> > #include <linux/of.h> > #include <linux/device.h> > +#include <linux/phy.h> > #include <linux/pm_runtime.h> > #include <linux/regulator/consumer.h> > > @@ -49,6 +50,7 @@ enum phy_mode { > * @power_on: powering on the phy > * @power_off: powering off the phy > * @set_mode: set the mode of the phy > + * @set_netif_mode: set the mode of the net interface phy > * @reset: resetting the phy > * @calibrate: calibrate the phy > * @owner: the module owner containing the ops > @@ -59,6 +61,7 @@ struct phy_ops { > int (*power_on)(struct phy *phy); > int (*power_off)(struct phy *phy); > int (*set_mode)(struct phy *phy, enum phy_mode mode); > + int (*set_netif_mode)(struct phy *phy, phy_interface_t mode); > int (*reset)(struct phy *phy); > int (*calibrate)(struct phy *phy); > struct module *owner; > @@ -163,6 +166,7 @@ int phy_exit(struct phy *phy); > int phy_power_on(struct phy *phy); > int phy_power_off(struct phy *phy); > int phy_set_mode(struct phy *phy, enum phy_mode mode); > +int phy_set_netif_mode(struct phy *phy, phy_interface_t mode); > static inline enum phy_mode phy_get_mode(struct phy *phy) > { > return phy->attrs.mode; > @@ -283,6 +287,14 @@ static inline int phy_set_mode(struct phy *phy, enum phy_mode mode) > return -ENOSYS; > } > > +static inline int phy_set_netif_mode(struct phy *phy, > + phy_interface_t mode) > +{ > + if (!phy) > + return 0; > + return -ENOTSUPP; > +} > + > static inline enum phy_mode phy_get_mode(struct phy *phy) > { > return PHY_MODE_INVALID; >
On 10/09/2018 12:22 AM, Kishon Vijay Abraham I wrote: > Hi Grygorii, > > On Tuesday 09 October 2018 05:19 AM, Grygorii Strashko wrote: >> Add new API phy_set_netif_mode(struct phy *phy, phy_interface_t mode) and >> new PHY operation callback .set_netif_mode() which intended to be implemnte >> by PHY drivers which supports Network interrfaces mode selection. Both >> accepts phy_interface_t vlaue as input parameter. >> >> Cc: Kishon Vijay Abraham I <kishon@ti.com> >> Cc: Tony Lindgren <tony@atomide.com> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- >> drivers/phy/phy-core.c | 15 +++++++++++++++ >> include/linux/phy/phy.h | 12 ++++++++++++ >> 2 files changed, 27 insertions(+) >> >> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c >> index 35fd38c..d9aba1a 100644 >> --- a/drivers/phy/phy-core.c >> +++ b/drivers/phy/phy-core.c >> @@ -377,6 +377,21 @@ int phy_set_mode(struct phy *phy, enum phy_mode mode) >> } >> EXPORT_SYMBOL_GPL(phy_set_mode); >> >> +int phy_set_netif_mode(struct phy *phy, phy_interface_t mode) >> +{ >> + int ret; >> + >> + if (!phy || !phy->ops->set_netif_mode) >> + return 0; >> + >> + mutex_lock(&phy->mutex); >> + ret = phy->ops->set_netif_mode(phy, mode); >> + mutex_unlock(&phy->mutex); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(phy_set_netif_mode); > > We should try to add only generic PHY APIs and not subsystem specific APIs. In > this case I think phy_set_mode should suffice. This is what I've had in mind first, but all my guts argued against it after I've tried: diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index bc73d2b..961b156 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -41,6 +41,14 @@ enum phy_mode { PHY_MODE_10GKR, PHY_MODE_UFS_HS_A, PHY_MODE_UFS_HS_B, + PHY_MODE_MODE_MII, + PHY_MODE_MODE_GMII, + PHY_MODE_MODE_SGMII, + PHY_MODE_MODE_RMII, + PHY_MODE_MODE_RGMII, + PHY_MODE_MODE_RGMII_ID, + PHY_MODE_MODE_RGMII_RXID, + PHY_MODE_MODE_RGMII_TXID, }; above introduces ugly constants duplication and required every network phy driver to maintain conversation table phy_interface_t -> enum phy_mode. More over, if above change happens third time (first time PHY_MODE_SGMII/PHY_MODE_10GKR were added, second - PHY_MODE_2500SGMII) it will never ends (there are ~15 more items only in phy_interface_t). As result, enum phy_mode might became a un-maintainable monster. So, as per above, and considering that Network subsystem is based on standards (phy_interface_t lists standard intf) I've tried to add separate PHY API. As an idea: - seems it could be reasonable to introduce PHY_MODE_NETWORK (or PHY_MODE_ETHERNET) and add generic phy_set_submode(struct phy *phy, long submode). So, single functional PHY device can just use phy_set_submode() and multi-functional devices (like serdes which can be muxed between PCIe, USB, NET), can use: phy_set_mode(PHY_MODE_ETHERNET) phy_set_submode(X); Any way, if you still agree to just add new above phy_modes - i'll redo patches.
Hi, On Wednesday 10 October 2018 04:13 AM, Grygorii Strashko wrote: > > > On 10/09/2018 12:22 AM, Kishon Vijay Abraham I wrote: >> Hi Grygorii, >> >> On Tuesday 09 October 2018 05:19 AM, Grygorii Strashko wrote: >>> Add new API phy_set_netif_mode(struct phy *phy, phy_interface_t mode) and >>> new PHY operation callback .set_netif_mode() which intended to be implemnte >>> by PHY drivers which supports Network interrfaces mode selection. Both >>> accepts phy_interface_t vlaue as input parameter. >>> >>> Cc: Kishon Vijay Abraham I <kishon@ti.com> >>> Cc: Tony Lindgren <tony@atomide.com> >>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>> --- >>> drivers/phy/phy-core.c | 15 +++++++++++++++ >>> include/linux/phy/phy.h | 12 ++++++++++++ >>> 2 files changed, 27 insertions(+) >>> >>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c >>> index 35fd38c..d9aba1a 100644 >>> --- a/drivers/phy/phy-core.c >>> +++ b/drivers/phy/phy-core.c >>> @@ -377,6 +377,21 @@ int phy_set_mode(struct phy *phy, enum phy_mode mode) >>> } >>> EXPORT_SYMBOL_GPL(phy_set_mode); >>> >>> +int phy_set_netif_mode(struct phy *phy, phy_interface_t mode) >>> +{ >>> + int ret; >>> + >>> + if (!phy || !phy->ops->set_netif_mode) >>> + return 0; >>> + >>> + mutex_lock(&phy->mutex); >>> + ret = phy->ops->set_netif_mode(phy, mode); >>> + mutex_unlock(&phy->mutex); >>> + >>> + return ret; >>> +} >>> +EXPORT_SYMBOL_GPL(phy_set_netif_mode); >> >> We should try to add only generic PHY APIs and not subsystem specific APIs. In >> this case I think phy_set_mode should suffice. > > > This is what I've had in mind first, but all my guts argued against it after I've tried: > > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index bc73d2b..961b156 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -41,6 +41,14 @@ enum phy_mode { > PHY_MODE_10GKR, > PHY_MODE_UFS_HS_A, > PHY_MODE_UFS_HS_B, > + PHY_MODE_MODE_MII, > + PHY_MODE_MODE_GMII, > + PHY_MODE_MODE_SGMII, > + PHY_MODE_MODE_RMII, > + PHY_MODE_MODE_RGMII, > + PHY_MODE_MODE_RGMII_ID, > + PHY_MODE_MODE_RGMII_RXID, > + PHY_MODE_MODE_RGMII_TXID, > }; > > above introduces ugly constants duplication and required every network phy driver > to maintain conversation table phy_interface_t -> enum phy_mode. > More over, if above change happens third time (first time PHY_MODE_SGMII/PHY_MODE_10GKR were added, > second - PHY_MODE_2500SGMII) it will never ends (there are ~15 more items only in phy_interface_t). > As result, enum phy_mode might became a un-maintainable monster. > > So, as per above, and considering that Network subsystem is based on standards (phy_interface_t lists standard intf) > I've tried to add separate PHY API. > > As an idea: > - seems it could be reasonable to introduce PHY_MODE_NETWORK (or PHY_MODE_ETHERNET) and > add generic phy_set_submode(struct phy *phy, long submode). > > So, single functional PHY device can just use phy_set_submode() and > multi-functional devices (like serdes which can be muxed between PCIe, USB, NET), can use: > > phy_set_mode(PHY_MODE_ETHERNET) > phy_set_submode(X); Agreed on the constant duplication comment above. We can modify set_mode to take submode as an additional parameter and fix all the users of phy_set_mode. int phy_set_mode(struct phy *phy, enum phy_mode mode, int submode) Thanks Kishon
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index 35fd38c..d9aba1a 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -377,6 +377,21 @@ int phy_set_mode(struct phy *phy, enum phy_mode mode) } EXPORT_SYMBOL_GPL(phy_set_mode); +int phy_set_netif_mode(struct phy *phy, phy_interface_t mode) +{ + int ret; + + if (!phy || !phy->ops->set_netif_mode) + return 0; + + mutex_lock(&phy->mutex); + ret = phy->ops->set_netif_mode(phy, mode); + mutex_unlock(&phy->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(phy_set_netif_mode); + int phy_reset(struct phy *phy) { int ret; diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index 9713aeb..bc73d2b 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -17,6 +17,7 @@ #include <linux/err.h> #include <linux/of.h> #include <linux/device.h> +#include <linux/phy.h> #include <linux/pm_runtime.h> #include <linux/regulator/consumer.h> @@ -49,6 +50,7 @@ enum phy_mode { * @power_on: powering on the phy * @power_off: powering off the phy * @set_mode: set the mode of the phy + * @set_netif_mode: set the mode of the net interface phy * @reset: resetting the phy * @calibrate: calibrate the phy * @owner: the module owner containing the ops @@ -59,6 +61,7 @@ struct phy_ops { int (*power_on)(struct phy *phy); int (*power_off)(struct phy *phy); int (*set_mode)(struct phy *phy, enum phy_mode mode); + int (*set_netif_mode)(struct phy *phy, phy_interface_t mode); int (*reset)(struct phy *phy); int (*calibrate)(struct phy *phy); struct module *owner; @@ -163,6 +166,7 @@ int phy_exit(struct phy *phy); int phy_power_on(struct phy *phy); int phy_power_off(struct phy *phy); int phy_set_mode(struct phy *phy, enum phy_mode mode); +int phy_set_netif_mode(struct phy *phy, phy_interface_t mode); static inline enum phy_mode phy_get_mode(struct phy *phy) { return phy->attrs.mode; @@ -283,6 +287,14 @@ static inline int phy_set_mode(struct phy *phy, enum phy_mode mode) return -ENOSYS; } +static inline int phy_set_netif_mode(struct phy *phy, + phy_interface_t mode) +{ + if (!phy) + return 0; + return -ENOTSUPP; +} + static inline enum phy_mode phy_get_mode(struct phy *phy) { return PHY_MODE_INVALID;
Add new API phy_set_netif_mode(struct phy *phy, phy_interface_t mode) and new PHY operation callback .set_netif_mode() which intended to be implemnte by PHY drivers which supports Network interrfaces mode selection. Both accepts phy_interface_t vlaue as input parameter. Cc: Kishon Vijay Abraham I <kishon@ti.com> Cc: Tony Lindgren <tony@atomide.com> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- drivers/phy/phy-core.c | 15 +++++++++++++++ include/linux/phy/phy.h | 12 ++++++++++++ 2 files changed, 27 insertions(+)