Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
On Wed, 2013-11-06 at 12:34 +1100, Alistair Popple wrote: On Tue, 5 Nov 2013 23:11:50 Ben Hutchings wrote: On Wed, 2013-11-06 at 06:54 +1100, Benjamin Herrenschmidt wrote: [snip] It's an SoC bit so there's little point making it generally selectable by the user. I think a better way to do this is: config IBM_EMAC_RGMII_WOL bool IBM EMAC RGMII wake-on-LAN support depends on MY_WONDERFUL_NEW_SOC || COMPILE_TEST default y if MY_WONDERFUL_NEW_SOC Then anyone making an API change that affects this driver can check that it still complies. The method used in this patch is the same as what is currently used by the other IBM EMAC PHY interfaces (eg. config IBM_EMAC_ZMII etc). I'm happy to send a patch to update all of those as well for consistency but that would mean adding what each platform requires into EMACS Kconfig as well. Personally I think it is nicer to keep the definitions of what each platform requires in one place (ie. arch/powerpc/platforms/44x/Kconfig) as it is consistent with what we do for other 44x drivers, however I am happy to use the above method if people think it's better. Yes, I see your point. Alternatively we could do something like this: config IBM_EMAC_RGMII_WOL bool default y if COMPILE_TEST default n This would leave the platform dependencies as they are currently but still allow compile testing. It still shouldn't default to y in that case. Instead you can make the symbol conditionally configurable: config IBM_EMAC_RGMII_WOL bool IBM EMAC RGMII wake-on-LAN support if COMPILE_TEST and then select this from your platform Kconfig as you intended. (There is no need to put 'default n' as that's implicit for a configurable symbol. But it doesn't hurt either.) Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
On Wed, 6 Nov 2013 16:40:10 Ben Hutchings wrote: On Wed, 2013-11-06 at 12:34 +1100, Alistair Popple wrote: On Tue, 5 Nov 2013 23:11:50 Ben Hutchings wrote: On Wed, 2013-11-06 at 06:54 +1100, Benjamin Herrenschmidt wrote: [snip] It's an SoC bit so there's little point making it generally selectable by the user. I think a better way to do this is: config IBM_EMAC_RGMII_WOL bool IBM EMAC RGMII wake-on-LAN support depends on MY_WONDERFUL_NEW_SOC || COMPILE_TEST default y if MY_WONDERFUL_NEW_SOC Then anyone making an API change that affects this driver can check that it still complies. The method used in this patch is the same as what is currently used by the other IBM EMAC PHY interfaces (eg. config IBM_EMAC_ZMII etc). I'm happy to send a patch to update all of those as well for consistency but that would mean adding what each platform requires into EMACS Kconfig as well. Personally I think it is nicer to keep the definitions of what each platform requires in one place (ie. arch/powerpc/platforms/44x/Kconfig) as it is consistent with what we do for other 44x drivers, however I am happy to use the above method if people think it's better. Yes, I see your point. Alternatively we could do something like this: config IBM_EMAC_RGMII_WOL bool default y if COMPILE_TEST default n This would leave the platform dependencies as they are currently but still allow compile testing. It still shouldn't default to y in that case. Instead you can make the symbol conditionally configurable: config IBM_EMAC_RGMII_WOL bool IBM EMAC RGMII wake-on-LAN support if COMPILE_TEST and then select this from your platform Kconfig as you intended. That looks reasonable - I will include it in the next version of the patch series. Thanks. (There is no need to put 'default n' as that's implicit for a configurable symbol. But it doesn't hurt either.) Ben. Alistair ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
On Tue, 2013-11-05 at 16:31 +1100, Alistair Popple wrote: [...] --- a/drivers/net/ethernet/ibm/emac/Kconfig +++ b/drivers/net/ethernet/ibm/emac/Kconfig @@ -55,6 +55,10 @@ config IBM_EMAC_RGMII bool default n +config IBM_EMAC_RGMII_WOL + bool + default n + [...] So no-one can even build-test this at present! Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
On Tue, 2013-11-05 at 18:16 +, Ben Hutchings wrote: On Tue, 2013-11-05 at 16:31 +1100, Alistair Popple wrote: [...] --- a/drivers/net/ethernet/ibm/emac/Kconfig +++ b/drivers/net/ethernet/ibm/emac/Kconfig @@ -55,6 +55,10 @@ config IBM_EMAC_RGMII bool default n +config IBM_EMAC_RGMII_WOL + bool + default n + [...] So no-one can even build-test this at present! Patch 7/7 adds the select to the platform, isn't that sufficient ? It's an SoC bit so there's little point making it generally selectable by the user. Alistair: The commit name should be different, it's not a PHY you are adding, it's a PHY interface (the PHY itself is off chip). Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
[snip]
+/* RGMII bridge supports only GMII/TBI and RGMII/RTBI PHYs */
+static inline int rgmii_valid_mode(int phy_mode)
+{
+ return phy_mode == PHY_MODE_GMII ||
+ phy_mode == PHY_MODE_MII ||
+ phy_mode == PHY_MODE_RGMII ||
+ phy_mode == PHY_MODE_TBI ||
+ phy_mode == PHY_MODE_RTBI;
+}
+
+static inline const char *rgmii_mode_name(int mode)
+{
+ switch (mode) {
+ case PHY_MODE_RGMII:
+ return RGMII;
+ case PHY_MODE_TBI:
+ return TBI;
+ case PHY_MODE_GMII:
+ return GMII;
+ case PHY_MODE_MII:
+ return MII;
+ case PHY_MODE_RTBI:
+ return RTBI;
+ default:
+ BUG();
+ }
Any reasons why you are duplicating what is available in
drivers/of/of_net.c ::of_get_phy_mode()?
--
Florian
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
On Wed, 2013-11-06 at 06:54 +1100, Benjamin Herrenschmidt wrote: On Tue, 2013-11-05 at 18:16 +, Ben Hutchings wrote: On Tue, 2013-11-05 at 16:31 +1100, Alistair Popple wrote: [...] --- a/drivers/net/ethernet/ibm/emac/Kconfig +++ b/drivers/net/ethernet/ibm/emac/Kconfig @@ -55,6 +55,10 @@ config IBM_EMAC_RGMII bool default n +config IBM_EMAC_RGMII_WOL + bool + default n + [...] So no-one can even build-test this at present! Patch 7/7 adds the select to the platform, isn't that sufficient ? I suppose so, but I don't think that went to netdev. It's an SoC bit so there's little point making it generally selectable by the user. I think a better way to do this is: config IBM_EMAC_RGMII_WOL bool IBM EMAC RGMII wake-on-LAN support depends on MY_WONDERFUL_NEW_SOC || COMPILE_TEST default y if MY_WONDERFUL_NEW_SOC Then anyone making an API change that affects this driver can check that it still complies. Ben. Alistair: The commit name should be different, it's not a PHY you are adding, it's a PHY interface (the PHY itself is off chip). -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
On Tue, 5 Nov 2013 10:47:22 Florian Fainelli wrote:
[snip]
+/* RGMII bridge supports only GMII/TBI and RGMII/RTBI PHYs */
+static inline int rgmii_valid_mode(int phy_mode)
+{
+ return phy_mode == PHY_MODE_GMII ||
+ phy_mode == PHY_MODE_MII ||
+ phy_mode == PHY_MODE_RGMII ||
+ phy_mode == PHY_MODE_TBI ||
+ phy_mode == PHY_MODE_RTBI;
+}
+
+static inline const char *rgmii_mode_name(int mode)
+{
+ switch (mode) {
+ case PHY_MODE_RGMII:
+ return RGMII;
+ case PHY_MODE_TBI:
+ return TBI;
+ case PHY_MODE_GMII:
+ return GMII;
+ case PHY_MODE_MII:
+ return MII;
+ case PHY_MODE_RTBI:
+ return RTBI;
+ default:
+ BUG();
+ }
Any reasons why you are duplicating what is available in
drivers/of/of_net.c ::of_get_phy_mode()?
Unless I'm missing something of_get_phy_mode() is going the other way.
rgmii_mode_name() is converting PHY_MODE_* into a human-readable string. I
couldn't find any obvious kernel method to do this but maybe I missed it?
Regards,
Alistair
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
2013/11/5 Alistair Popple alist...@popple.id.au:
On Tue, 5 Nov 2013 10:47:22 Florian Fainelli wrote:
[snip]
+/* RGMII bridge supports only GMII/TBI and RGMII/RTBI PHYs */
+static inline int rgmii_valid_mode(int phy_mode)
+{
+ return phy_mode == PHY_MODE_GMII ||
+ phy_mode == PHY_MODE_MII ||
+ phy_mode == PHY_MODE_RGMII ||
+ phy_mode == PHY_MODE_TBI ||
+ phy_mode == PHY_MODE_RTBI;
+}
+
+static inline const char *rgmii_mode_name(int mode)
+{
+ switch (mode) {
+ case PHY_MODE_RGMII:
+ return RGMII;
+ case PHY_MODE_TBI:
+ return TBI;
+ case PHY_MODE_GMII:
+ return GMII;
+ case PHY_MODE_MII:
+ return MII;
+ case PHY_MODE_RTBI:
+ return RTBI;
+ default:
+ BUG();
+ }
Any reasons why you are duplicating what is available in
drivers/of/of_net.c ::of_get_phy_mode()?
Unless I'm missing something of_get_phy_mode() is going the other way.
rgmii_mode_name() is converting PHY_MODE_* into a human-readable string. I
couldn't find any obvious kernel method to do this but maybe I missed it?
Right, rgmii_mode_name() just has informative purposes and should be
removed, I would suggest using standard device tree bindings property
(phy-mode) anyway such that you could use of_get_phy_mode() and use
phy_interface_t types.
--
Florian
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
On Tue, 5 Nov 2013 23:11:50 Ben Hutchings wrote: On Wed, 2013-11-06 at 06:54 +1100, Benjamin Herrenschmidt wrote: [snip] It's an SoC bit so there's little point making it generally selectable by the user. I think a better way to do this is: config IBM_EMAC_RGMII_WOL bool IBM EMAC RGMII wake-on-LAN support depends on MY_WONDERFUL_NEW_SOC || COMPILE_TEST default y if MY_WONDERFUL_NEW_SOC Then anyone making an API change that affects this driver can check that it still complies. The method used in this patch is the same as what is currently used by the other IBM EMAC PHY interfaces (eg. config IBM_EMAC_ZMII etc). I'm happy to send a patch to update all of those as well for consistency but that would mean adding what each platform requires into EMACS Kconfig as well. Personally I think it is nicer to keep the definitions of what each platform requires in one place (ie. arch/powerpc/platforms/44x/Kconfig) as it is consistent with what we do for other 44x drivers, however I am happy to use the above method if people think it's better. Alternatively we could do something like this: config IBM_EMAC_RGMII_WOL bool default y if COMPILE_TEST default n This would leave the platform dependencies as they are currently but still allow compile testing. Regards, Alistair Ben. Alistair: The commit name should be different, it's not a PHY you are adding, it's a PHY interface (the PHY itself is off chip). ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
On Tue, 5 Nov 2013 16:16:08 Florian Fainelli wrote: [snip] 2013/11/5 Alistair Popple alist...@popple.id.au: Any reasons why you are duplicating what is available in drivers/of/of_net.c ::of_get_phy_mode()? Unless I'm missing something of_get_phy_mode() is going the other way. rgmii_mode_name() is converting PHY_MODE_* into a human-readable string. I couldn't find any obvious kernel method to do this but maybe I missed it? Right, rgmii_mode_name() just has informative purposes and should be removed, I would suggest using standard device tree bindings property (phy-mode) anyway such that you could use of_get_phy_mode() and use phy_interface_t types. Ok, that's what is currently done in the core IBM EMAC driver. As this is used just for informative purposes I will remove it. Thanks. Regards, Alistair ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
On Wed, 2013-11-06 at 12:38 +1100, Alistair Popple wrote: Right, rgmii_mode_name() just has informative purposes and should be removed, I would suggest using standard device tree bindings property (phy-mode) anyway such that you could use of_get_phy_mode() and use phy_interface_t types. Ok, that's what is currently done in the core IBM EMAC driver. As this is used just for informative purposes I will remove it. Thanks. Why ? Information is useful... Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
2013/11/5 Benjamin Herrenschmidt b...@kernel.crashing.org:
On Wed, 2013-11-06 at 12:38 +1100, Alistair Popple wrote:
Right, rgmii_mode_name() just has informative purposes and should be
removed, I would suggest using standard device tree bindings
property
(phy-mode) anyway such that you could use of_get_phy_mode() and use
phy_interface_t types.
Ok, that's what is currently done in the core IBM EMAC driver. As this
is used
just for informative purposes I will remove it. Thanks.
Why ? Information is useful...
Not the way they are currently handled:
+ /* Check if we need to attach to a RGMII */
+ if (!rgmii_valid_mode(mode)) {
+ dev_err(ofdev-dev, unsupported settings !\n);
Considering that nothing useful is being printed, that or nothing at
all really looks identical to me.
--
Florian
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
The IBM Akebono board uses a different ethernet PHY that has wake on lan (WOL) support with the IBM emac. This patch adds suppot to the IBM emac driver for this new PHY. At this stage the wake on lan functionality has not been implemented. Signed-off-by: Alistair Popple alist...@popple.id.au Cc: David S. Miller da...@davemloft.net Cc: net...@vger.kernel.org --- .../devicetree/bindings/powerpc/4xx/emac.txt |9 + drivers/net/ethernet/ibm/emac/Kconfig |4 + drivers/net/ethernet/ibm/emac/Makefile |1 + drivers/net/ethernet/ibm/emac/core.c | 50 +++- drivers/net/ethernet/ibm/emac/core.h | 12 + drivers/net/ethernet/ibm/emac/rgmii_wol.c | 262 drivers/net/ethernet/ibm/emac/rgmii_wol.h | 62 + 7 files changed, 394 insertions(+), 6 deletions(-) create mode 100644 drivers/net/ethernet/ibm/emac/rgmii_wol.c create mode 100644 drivers/net/ethernet/ibm/emac/rgmii_wol.h diff --git a/Documentation/devicetree/bindings/powerpc/4xx/emac.txt b/Documentation/devicetree/bindings/powerpc/4xx/emac.txt index 712baf6..9928d9d 100644 --- a/Documentation/devicetree/bindings/powerpc/4xx/emac.txt +++ b/Documentation/devicetree/bindings/powerpc/4xx/emac.txt @@ -61,6 +61,8 @@ Fox Axon: present, whatever value is appropriate for each EMAC, that is the content of the current (bogus) phy-port property. +- rgmii-wol-device : 1 cell, required iff conntect to a RGMII in the WKUP + power domain. phandle of the RGMII-WOL device node. Optional properties: - phy-address : 1 cell, optional, MDIO address of the PHY. If absent, @@ -146,3 +148,10 @@ available. For Axon: 0x012a + iv) RGMII-WOL node + +Required properties: +- compatible : compatible list, containing 2 entries, first is + ibm,rgmii-wol-CHIP where CHIP is the host ASIC (like + EMAC) and the second is ibm,rgmii-wol. +- reg: registers mapping diff --git a/drivers/net/ethernet/ibm/emac/Kconfig b/drivers/net/ethernet/ibm/emac/Kconfig index 3f44a30..7425c27 100644 --- a/drivers/net/ethernet/ibm/emac/Kconfig +++ b/drivers/net/ethernet/ibm/emac/Kconfig @@ -55,6 +55,10 @@ config IBM_EMAC_RGMII bool default n +config IBM_EMAC_RGMII_WOL + bool + default n + config IBM_EMAC_TAH bool default n diff --git a/drivers/net/ethernet/ibm/emac/Makefile b/drivers/net/ethernet/ibm/emac/Makefile index eba2183..8843803 100644 --- a/drivers/net/ethernet/ibm/emac/Makefile +++ b/drivers/net/ethernet/ibm/emac/Makefile @@ -7,5 +7,6 @@ obj-$(CONFIG_IBM_EMAC) += ibm_emac.o ibm_emac-y := mal.o core.o phy.o ibm_emac-$(CONFIG_IBM_EMAC_ZMII) += zmii.o ibm_emac-$(CONFIG_IBM_EMAC_RGMII) += rgmii.o +ibm_emac-$(CONFIG_IBM_EMAC_RGMII_WOL) += rgmii_wol.o ibm_emac-$(CONFIG_IBM_EMAC_TAH) += tah.o ibm_emac-$(CONFIG_IBM_EMAC_DEBUG) += debug.o diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c index 6b5c722..fc1a775 100644 --- a/drivers/net/ethernet/ibm/emac/core.c +++ b/drivers/net/ethernet/ibm/emac/core.c @@ -630,6 +630,8 @@ static int emac_configure(struct emac_instance *dev) if (emac_has_feature(dev, EMAC_FTR_HAS_RGMII)) rgmii_set_speed(dev-rgmii_dev, dev-rgmii_port, dev-phy.speed); + if (emac_has_feature(dev, EMAC_FTR_HAS_RGMII_WOL)) + rgmii_wol_set_speed(dev-rgmii_wol_dev, dev-phy.speed); if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII)) zmii_set_speed(dev-zmii_dev, dev-zmii_port, dev-phy.speed); @@ -797,6 +799,8 @@ static int __emac_mdio_read(struct emac_instance *dev, u8 id, u8 reg) zmii_get_mdio(dev-zmii_dev, dev-zmii_port); if (emac_has_feature(dev, EMAC_FTR_HAS_RGMII)) rgmii_get_mdio(dev-rgmii_dev, dev-rgmii_port); + if (emac_has_feature(dev, EMAC_FTR_HAS_RGMII_WOL)) + rgmii_wol_get_mdio(dev-rgmii_wol_dev); /* Wait for management interface to become idle */ n = 20; @@ -844,6 +848,8 @@ static int __emac_mdio_read(struct emac_instance *dev, u8 id, u8 reg) DBG2(dev, mdio_read - %04x NL, r); err = 0; bail: + if (emac_has_feature(dev, EMAC_FTR_HAS_RGMII_WOL)) + rgmii_wol_put_mdio(dev-rgmii_wol_dev); if (emac_has_feature(dev, EMAC_FTR_HAS_RGMII)) rgmii_put_mdio(dev-rgmii_dev, dev-rgmii_port); if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII)) @@ -869,6 +875,8 @@ static void __emac_mdio_write(struct emac_instance *dev, u8 id, u8 reg, zmii_get_mdio(dev-zmii_dev, dev-zmii_port); if (emac_has_feature(dev, EMAC_FTR_HAS_RGMII)) rgmii_get_mdio(dev-rgmii_dev,
