From: Grygorii Strashko <grygorii.strashko@ti.com> To: Kishon Vijay Abraham I <kishon@ti.com>, "David S. Miller" <davem@davemloft.net>, <netdev@vger.kernel.org>, Tony Lindgren <tony@atomide.com>, Rob Herring <robh+dt@kernel.org> Cc: Sekhar Nori <nsekhar@ti.com>, <linux-kernel@vger.kernel.org>, <linux-omap@vger.kernel.org>, <devicetree@vger.kernel.org> Subject: Re: [RFC PATCH 01/11] phy: core add phy_set_netif_mode() api Date: Tue, 9 Oct 2018 17:43:00 -0500 [thread overview] Message-ID: <18219d97-f86e-27ee-a116-a8febc3dcd24@ti.com> (raw) In-Reply-To: <54ba13bb-95e1-1519-a30f-8da198447489@ti.com> 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. -- regards, -grygorii
WARNING: multiple messages have this Message-ID (diff)
From: Grygorii Strashko <grygorii.strashko@ti.com> To: Kishon Vijay Abraham I <kishon@ti.com>, "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org, Tony Lindgren <tony@atomide.com>, Rob Herring <robh+dt@kernel.org> Cc: Sekhar Nori <nsekhar@ti.com>, linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [RFC PATCH 01/11] phy: core add phy_set_netif_mode() api Date: Tue, 9 Oct 2018 17:43:00 -0500 [thread overview] Message-ID: <18219d97-f86e-27ee-a116-a8febc3dcd24@ti.com> (raw) In-Reply-To: <54ba13bb-95e1-1519-a30f-8da198447489@ti.com> 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. -- regards, -grygorii
next prev parent reply other threads:[~2018-10-09 22:43 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-10-08 23:49 [RFC PATCH 00/11] net: ethernet: ti: cpsw: replace cpsw-phy-sel with phy driver Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 01/11] phy: core add phy_set_netif_mode() api Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-09 5:22 ` Kishon Vijay Abraham I 2018-10-09 5:22 ` Kishon Vijay Abraham I 2018-10-09 22:43 ` Grygorii Strashko [this message] 2018-10-09 22:43 ` Grygorii Strashko 2018-10-25 10:05 ` Kishon Vijay Abraham I 2018-10-25 10:05 ` Kishon Vijay Abraham I 2018-10-08 23:49 ` [RFC PATCH 02/11] dt-bindings: phy: add cpsw port interface mode selection phy bindings Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-09 14:40 ` Tony Lindgren 2018-10-09 20:10 ` Grygorii Strashko 2018-10-09 20:10 ` Grygorii Strashko 2018-10-09 20:30 ` Tony Lindgren 2018-10-09 22:04 ` Grygorii Strashko 2018-10-09 22:04 ` Grygorii Strashko 2018-10-09 22:07 ` Tony Lindgren 2018-10-17 15:39 ` Rob Herring 2018-10-08 23:49 ` [RFC PATCH 03/11] phy: ti: introduce phy-gmii-sel driver Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-09 0:39 ` Andrew Lunn 2018-10-09 20:22 ` Grygorii Strashko 2018-10-09 20:22 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 04/11] dt-bindings: net: ti: cpsw: switch to use phy-gmii-sel phy Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-17 15:41 ` Rob Herring 2018-10-08 23:49 ` [RFC PATCH 05/11] net: ethernet: ti: cpsw: add support for port interface mode selection phy Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-09 0:50 ` Andrew Lunn 2018-10-09 20:28 ` Grygorii Strashko 2018-10-09 20:28 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 06/11] ARM: dts: dra7: switch to use phy-gmii-sel Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 07/11] ARM: dts: dm814x: " Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 08/11] ARM: dts: am4372: " Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 09/11] ARM: dts: am335x: " Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 10/11] dt-bindings: net: ti: deprecate cpsw-phy-sel bindings Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-17 15:41 ` Rob Herring 2018-10-17 15:41 ` Rob Herring 2018-10-08 23:49 ` [RFC PATCH 11/11] net: ethernet: ti: cpsw: deprecate cpsw-phy-sel driver Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-09 14:36 ` [RFC PATCH 00/11] net: ethernet: ti: cpsw: replace cpsw-phy-sel with phy driver Tony Lindgren 2018-10-09 21:12 ` Grygorii Strashko 2018-10-09 21:12 ` Grygorii Strashko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=18219d97-f86e-27ee-a116-a8febc3dcd24@ti.com \ --to=grygorii.strashko@ti.com \ --cc=davem@davemloft.net \ --cc=devicetree@vger.kernel.org \ --cc=kishon@ti.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=nsekhar@ti.com \ --cc=robh+dt@kernel.org \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.