All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mvneta: add FIXED_PHY dependency
@ 2015-11-09 14:08 ` Arnd Bergmann
  0 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2015-11-09 14:08 UTC (permalink / raw)
  To: netdev
  Cc: Stas Sergeev, David S. Miller, Florian Fainelli,
	Thomas Petazzoni, linux-kernel, linux-arm-kernel

The fixed_phy infrastructure is done in a way that is optional,
by providing 'static inline' helper functions doing nothing in
include/linux/phy_fixed.h for all its APIs. However, three out
of the four users (DSA, BCMGENET, and SYSTEMPORT) always
'select FIXED_PHY', presumably because they need that.
MVNETA is the fourth one, and if that is built-in but FIXED_PHY
is configured as a loadable module, we get a link error:

drivers/built-in.o: In function `mvneta_fixed_link_update':
fpga-mgr.c:(.text+0x33ed80): undefined reference to `fixed_phy_update_state'

Presumably this driver has the same dependency as the others,
so this patch also uses 'select' to ensure that the fixed-phy
support is built-in.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 898b2970e2c9 ("mvneta: implement SGMII-based in-band link state signaling")
---
Found using ARM randconfig tests. An alternative here would be
to use 'depends on FIXED_PHY || FIXED_PHY=n', I picked the 'select'
approach for consistency.

Should we perhaps make 'FIXED_PHY' a silent option and remove the
inline helpers, based on the assumption that a driver that wants these
will not work without them?

diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
index 80af9ffce5ea..a1c862b4664d 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -44,6 +44,7 @@ config MVNETA
 	tristate "Marvell Armada 370/38x/XP network interface support"
 	depends on PLAT_ORION
 	select MVMDIO
+	select FIXED_PHY
 	---help---
 	  This driver supports the network interface units in the
 	  Marvell ARMADA XP, ARMADA 370 and ARMADA 38x SoC family.


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH] mvneta: add FIXED_PHY dependency
@ 2015-11-09 14:08 ` Arnd Bergmann
  0 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2015-11-09 14:08 UTC (permalink / raw)
  To: linux-arm-kernel

The fixed_phy infrastructure is done in a way that is optional,
by providing 'static inline' helper functions doing nothing in
include/linux/phy_fixed.h for all its APIs. However, three out
of the four users (DSA, BCMGENET, and SYSTEMPORT) always
'select FIXED_PHY', presumably because they need that.
MVNETA is the fourth one, and if that is built-in but FIXED_PHY
is configured as a loadable module, we get a link error:

drivers/built-in.o: In function `mvneta_fixed_link_update':
fpga-mgr.c:(.text+0x33ed80): undefined reference to `fixed_phy_update_state'

Presumably this driver has the same dependency as the others,
so this patch also uses 'select' to ensure that the fixed-phy
support is built-in.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 898b2970e2c9 ("mvneta: implement SGMII-based in-band link state signaling")
---
Found using ARM randconfig tests. An alternative here would be
to use 'depends on FIXED_PHY || FIXED_PHY=n', I picked the 'select'
approach for consistency.

Should we perhaps make 'FIXED_PHY' a silent option and remove the
inline helpers, based on the assumption that a driver that wants these
will not work without them?

diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
index 80af9ffce5ea..a1c862b4664d 100644
--- a/drivers/net/ethernet/marvell/Kconfig
+++ b/drivers/net/ethernet/marvell/Kconfig
@@ -44,6 +44,7 @@ config MVNETA
 	tristate "Marvell Armada 370/38x/XP network interface support"
 	depends on PLAT_ORION
 	select MVMDIO
+	select FIXED_PHY
 	---help---
 	  This driver supports the network interface units in the
 	  Marvell ARMADA XP, ARMADA 370 and ARMADA 38x SoC family.

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] mvneta: add FIXED_PHY dependency
  2015-11-09 14:08 ` Arnd Bergmann
@ 2015-11-09 16:36   ` David Miller
  -1 siblings, 0 replies; 20+ messages in thread
From: David Miller @ 2015-11-09 16:36 UTC (permalink / raw)
  To: arnd
  Cc: netdev, stsp, f.fainelli, thomas.petazzoni, linux-kernel,
	linux-arm-kernel

From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 09 Nov 2015 15:08:57 +0100

> The fixed_phy infrastructure is done in a way that is optional,
> by providing 'static inline' helper functions doing nothing in
> include/linux/phy_fixed.h for all its APIs. However, three out
> of the four users (DSA, BCMGENET, and SYSTEMPORT) always
> 'select FIXED_PHY', presumably because they need that.
> MVNETA is the fourth one, and if that is built-in but FIXED_PHY
> is configured as a loadable module, we get a link error:
> 
> drivers/built-in.o: In function `mvneta_fixed_link_update':
> fpga-mgr.c:(.text+0x33ed80): undefined reference to `fixed_phy_update_state'
> 
> Presumably this driver has the same dependency as the others,
> so this patch also uses 'select' to ensure that the fixed-phy
> support is built-in.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 898b2970e2c9 ("mvneta: implement SGMII-based in-band link state signaling")
> ---
> Found using ARM randconfig tests. An alternative here would be
> to use 'depends on FIXED_PHY || FIXED_PHY=n', I picked the 'select'
> approach for consistency.
> 
> Should we perhaps make 'FIXED_PHY' a silent option and remove the
> inline helpers, based on the assumption that a driver that wants these
> will not work without them?

This seems reasonable to fix the problem for the time being, applied.

Thanks Arnd.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] mvneta: add FIXED_PHY dependency
@ 2015-11-09 16:36   ` David Miller
  0 siblings, 0 replies; 20+ messages in thread
From: David Miller @ 2015-11-09 16:36 UTC (permalink / raw)
  To: linux-arm-kernel

From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 09 Nov 2015 15:08:57 +0100

> The fixed_phy infrastructure is done in a way that is optional,
> by providing 'static inline' helper functions doing nothing in
> include/linux/phy_fixed.h for all its APIs. However, three out
> of the four users (DSA, BCMGENET, and SYSTEMPORT) always
> 'select FIXED_PHY', presumably because they need that.
> MVNETA is the fourth one, and if that is built-in but FIXED_PHY
> is configured as a loadable module, we get a link error:
> 
> drivers/built-in.o: In function `mvneta_fixed_link_update':
> fpga-mgr.c:(.text+0x33ed80): undefined reference to `fixed_phy_update_state'
> 
> Presumably this driver has the same dependency as the others,
> so this patch also uses 'select' to ensure that the fixed-phy
> support is built-in.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 898b2970e2c9 ("mvneta: implement SGMII-based in-band link state signaling")
> ---
> Found using ARM randconfig tests. An alternative here would be
> to use 'depends on FIXED_PHY || FIXED_PHY=n', I picked the 'select'
> approach for consistency.
> 
> Should we perhaps make 'FIXED_PHY' a silent option and remove the
> inline helpers, based on the assumption that a driver that wants these
> will not work without them?

This seems reasonable to fix the problem for the time being, applied.

Thanks Arnd.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] mvneta: add FIXED_PHY dependency
  2015-11-09 14:08 ` Arnd Bergmann
@ 2015-11-09 16:42   ` Andrew Lunn
  -1 siblings, 0 replies; 20+ messages in thread
From: Andrew Lunn @ 2015-11-09 16:42 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: netdev, Thomas Petazzoni, Florian Fainelli, linux-kernel,
	Stas Sergeev, David S. Miller, linux-arm-kernel

On Mon, Nov 09, 2015 at 03:08:57PM +0100, Arnd Bergmann wrote:
> The fixed_phy infrastructure is done in a way that is optional,
> by providing 'static inline' helper functions doing nothing in
> include/linux/phy_fixed.h for all its APIs. However, three out
> of the four users (DSA, BCMGENET, and SYSTEMPORT) always
> 'select FIXED_PHY', presumably because they need that.

Hi Arnd

Need is probably too strong, it could be considered an optional
feature. If you don't have a fixed_phy property in your DT blob, you
don't need fixed phy support in your image.

> MVNETA is the fourth one, and if that is built-in but FIXED_PHY
> is configured as a loadable module, we get a link error:
> 
> drivers/built-in.o: In function `mvneta_fixed_link_update':
> fpga-mgr.c:(.text+0x33ed80): undefined reference to `fixed_phy_update_state'
> 
> Presumably this driver has the same dependency as the others,
> so this patch also uses 'select' to ensure that the fixed-phy
> support is built-in.

This will work, and is uniform with the other instances. But maybe a
more correct fix is to ensure fixed-phy is never a module when there
is a builtin user.

> Should we perhaps make 'FIXED_PHY' a silent option and remove the
> inline helpers, based on the assumption that a driver that wants these
> will not work without them?

I suppose it comes down to, are we allowed to optionally implement
part of the DT binding?

     Andrew

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] mvneta: add FIXED_PHY dependency
@ 2015-11-09 16:42   ` Andrew Lunn
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Lunn @ 2015-11-09 16:42 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Nov 09, 2015 at 03:08:57PM +0100, Arnd Bergmann wrote:
> The fixed_phy infrastructure is done in a way that is optional,
> by providing 'static inline' helper functions doing nothing in
> include/linux/phy_fixed.h for all its APIs. However, three out
> of the four users (DSA, BCMGENET, and SYSTEMPORT) always
> 'select FIXED_PHY', presumably because they need that.

Hi Arnd

Need is probably too strong, it could be considered an optional
feature. If you don't have a fixed_phy property in your DT blob, you
don't need fixed phy support in your image.

> MVNETA is the fourth one, and if that is built-in but FIXED_PHY
> is configured as a loadable module, we get a link error:
> 
> drivers/built-in.o: In function `mvneta_fixed_link_update':
> fpga-mgr.c:(.text+0x33ed80): undefined reference to `fixed_phy_update_state'
> 
> Presumably this driver has the same dependency as the others,
> so this patch also uses 'select' to ensure that the fixed-phy
> support is built-in.

This will work, and is uniform with the other instances. But maybe a
more correct fix is to ensure fixed-phy is never a module when there
is a builtin user.

> Should we perhaps make 'FIXED_PHY' a silent option and remove the
> inline helpers, based on the assumption that a driver that wants these
> will not work without them?

I suppose it comes down to, are we allowed to optionally implement
part of the DT binding?

     Andrew

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] mvneta: add FIXED_PHY dependency
  2015-11-09 16:42   ` Andrew Lunn
@ 2015-11-09 16:57     ` Arnd Bergmann
  -1 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2015-11-09 16:57 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Thomas Petazzoni, Florian Fainelli, linux-kernel,
	Stas Sergeev, David S. Miller, linux-arm-kernel

On Monday 09 November 2015 17:42:32 Andrew Lunn wrote:
> On Mon, Nov 09, 2015 at 03:08:57PM +0100, Arnd Bergmann wrote:
> > The fixed_phy infrastructure is done in a way that is optional,
> > by providing 'static inline' helper functions doing nothing in
> > include/linux/phy_fixed.h for all its APIs. However, three out
> > of the four users (DSA, BCMGENET, and SYSTEMPORT) always
> > 'select FIXED_PHY', presumably because they need that.
> 
> Hi Arnd
> 
> Need is probably too strong, it could be considered an optional
> feature. If you don't have a fixed_phy property in your DT blob, you
> don't need fixed phy support in your image.

Ok, I see.

> > MVNETA is the fourth one, and if that is built-in but FIXED_PHY
> > is configured as a loadable module, we get a link error:
> > 
> > drivers/built-in.o: In function `mvneta_fixed_link_update':
> > fpga-mgr.c:(.text+0x33ed80): undefined reference to `fixed_phy_update_state'
> > 
> > Presumably this driver has the same dependency as the others,
> > so this patch also uses 'select' to ensure that the fixed-phy
> > support is built-in.
> 
> This will work, and is uniform with the other instances. But maybe a
> more correct fix is to ensure fixed-phy is never a module when there
> is a builtin user.

That is hard to express with Kconfig. The alternative I listed instead
guarantees that CONFIG_MVNETA cannot be set to 'y' whenever FIXED_PHY=m.
For all practical purposes that has the same effect.

The fixed-phy support isn't very big (around 2KB), so I wonder how
relevant that optimization is.

> > Should we perhaps make 'FIXED_PHY' a silent option and remove the
> > inline helpers, based on the assumption that a driver that wants these
> > will not work without them?
> 
> I suppose it comes down to, are we allowed to optionally implement
> part of the DT binding?

I'm not sure what you are asking. A lot of DT bindings have both
optional and mandatory properties. For mvneta, the "phy" and "phy-mode"
properties are listed as mandatory, so the driver can safely assume
that they are always present. If there are reasons to leave them out,
and for the driver to handle that case correctly, the binding
should be updated to mark them as optional.

	Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] mvneta: add FIXED_PHY dependency
@ 2015-11-09 16:57     ` Arnd Bergmann
  0 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2015-11-09 16:57 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 09 November 2015 17:42:32 Andrew Lunn wrote:
> On Mon, Nov 09, 2015 at 03:08:57PM +0100, Arnd Bergmann wrote:
> > The fixed_phy infrastructure is done in a way that is optional,
> > by providing 'static inline' helper functions doing nothing in
> > include/linux/phy_fixed.h for all its APIs. However, three out
> > of the four users (DSA, BCMGENET, and SYSTEMPORT) always
> > 'select FIXED_PHY', presumably because they need that.
> 
> Hi Arnd
> 
> Need is probably too strong, it could be considered an optional
> feature. If you don't have a fixed_phy property in your DT blob, you
> don't need fixed phy support in your image.

Ok, I see.

> > MVNETA is the fourth one, and if that is built-in but FIXED_PHY
> > is configured as a loadable module, we get a link error:
> > 
> > drivers/built-in.o: In function `mvneta_fixed_link_update':
> > fpga-mgr.c:(.text+0x33ed80): undefined reference to `fixed_phy_update_state'
> > 
> > Presumably this driver has the same dependency as the others,
> > so this patch also uses 'select' to ensure that the fixed-phy
> > support is built-in.
> 
> This will work, and is uniform with the other instances. But maybe a
> more correct fix is to ensure fixed-phy is never a module when there
> is a builtin user.

That is hard to express with Kconfig. The alternative I listed instead
guarantees that CONFIG_MVNETA cannot be set to 'y' whenever FIXED_PHY=m.
For all practical purposes that has the same effect.

The fixed-phy support isn't very big (around 2KB), so I wonder how
relevant that optimization is.

> > Should we perhaps make 'FIXED_PHY' a silent option and remove the
> > inline helpers, based on the assumption that a driver that wants these
> > will not work without them?
> 
> I suppose it comes down to, are we allowed to optionally implement
> part of the DT binding?

I'm not sure what you are asking. A lot of DT bindings have both
optional and mandatory properties. For mvneta, the "phy" and "phy-mode"
properties are listed as mandatory, so the driver can safely assume
that they are always present. If there are reasons to leave them out,
and for the driver to handle that case correctly, the binding
should be updated to mark them as optional.

	Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] mvneta: add FIXED_PHY dependency
  2015-11-09 16:57     ` Arnd Bergmann
@ 2015-11-09 17:08       ` Russell King - ARM Linux
  -1 siblings, 0 replies; 20+ messages in thread
From: Russell King - ARM Linux @ 2015-11-09 17:08 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andrew Lunn, Thomas Petazzoni, Florian Fainelli, netdev,
	linux-kernel, Stas Sergeev, David S. Miller, linux-arm-kernel

On Mon, Nov 09, 2015 at 05:57:43PM +0100, Arnd Bergmann wrote:
> I'm not sure what you are asking. A lot of DT bindings have both
> optional and mandatory properties. For mvneta, the "phy" and "phy-mode"
> properties are listed as mandatory, so the driver can safely assume
> that they are always present. If there are reasons to leave them out,
> and for the driver to handle that case correctly, the binding
> should be updated to mark them as optional.

They are "optional" because when you're using a DSA switch, you don't
specify a PHY (because, there isn't one).  For example, this is what
I'm using with an Armada 388 board with a Marvell DSA switch.  The
DSA does not appear as a PHY, and no node in the DSA stanza can be
referenced for a phy entry in the ethernet device's stanza.

                        eth1: ethernet@30000 {
                                compatible = "marvell,armada-370-neta";
                                reg = <0x30000 0x4000>;
                                interrupts-extended = <&mpic 10>;
                                clocks = <&gateclk 3>;
                                managed = "in-band-status";
                                phy-mode = "sgmii";
                                status = "okay";
                        };

        dsa@0 {
                compatible = "marvell,dsa";
                dsa,ethernet = <&eth1>;
                dsa,mii-bus = <&mdio>;
                pinctrl-0 = <&clearfog_dsa0_clk_pins &clearfog_dsa0_pins>;
                pinctrl-names = "default";
                #address-cells = <2>;
                #size-cells = <0>;

                switch@0 {
		...
		};
	};

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] mvneta: add FIXED_PHY dependency
@ 2015-11-09 17:08       ` Russell King - ARM Linux
  0 siblings, 0 replies; 20+ messages in thread
From: Russell King - ARM Linux @ 2015-11-09 17:08 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Nov 09, 2015 at 05:57:43PM +0100, Arnd Bergmann wrote:
> I'm not sure what you are asking. A lot of DT bindings have both
> optional and mandatory properties. For mvneta, the "phy" and "phy-mode"
> properties are listed as mandatory, so the driver can safely assume
> that they are always present. If there are reasons to leave them out,
> and for the driver to handle that case correctly, the binding
> should be updated to mark them as optional.

They are "optional" because when you're using a DSA switch, you don't
specify a PHY (because, there isn't one).  For example, this is what
I'm using with an Armada 388 board with a Marvell DSA switch.  The
DSA does not appear as a PHY, and no node in the DSA stanza can be
referenced for a phy entry in the ethernet device's stanza.

                        eth1: ethernet at 30000 {
                                compatible = "marvell,armada-370-neta";
                                reg = <0x30000 0x4000>;
                                interrupts-extended = <&mpic 10>;
                                clocks = <&gateclk 3>;
                                managed = "in-band-status";
                                phy-mode = "sgmii";
                                status = "okay";
                        };

        dsa at 0 {
                compatible = "marvell,dsa";
                dsa,ethernet = <&eth1>;
                dsa,mii-bus = <&mdio>;
                pinctrl-0 = <&clearfog_dsa0_clk_pins &clearfog_dsa0_pins>;
                pinctrl-names = "default";
                #address-cells = <2>;
                #size-cells = <0>;

                switch at 0 {
		...
		};
	};

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] mvneta: add FIXED_PHY dependency
  2015-11-09 16:57     ` Arnd Bergmann
@ 2015-11-09 17:08       ` Andrew Lunn
  -1 siblings, 0 replies; 20+ messages in thread
From: Andrew Lunn @ 2015-11-09 17:08 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: netdev, Thomas Petazzoni, Florian Fainelli, linux-kernel,
	Stas Sergeev, David S. Miller, linux-arm-kernel

> > I suppose it comes down to, are we allowed to optionally implement
> > part of the DT binding?
> 
> I'm not sure what you are asking. A lot of DT bindings have both
> optional and mandatory properties. For mvneta, the "phy" and "phy-mode"
> properties are listed as mandatory, so the driver can safely assume
> that they are always present. If there are reasons to leave them out,
> and for the driver to handle that case correctly, the binding
> should be updated to mark them as optional.

Hi Arnd

You are looking at it from the perspective of the driver. I was
meaning from the perspective of the DT blob. Can be blob assume the
driver implements all of the binding, all of the time?

You use fixed-phy when the MAC is connected to a switch, not a phy. Or
when the MAC is connected to an SFP module. The driver can currently
be built to not implement the fixed-phy party of the binding. Is that
O.K. from the perspective of the DT blob? Or should the driver always
implement all of the binding, in which these NOP stubs should be
removed and fixed phy always be enabled for the drivers that use it.

	Andrew

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] mvneta: add FIXED_PHY dependency
@ 2015-11-09 17:08       ` Andrew Lunn
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Lunn @ 2015-11-09 17:08 UTC (permalink / raw)
  To: linux-arm-kernel

> > I suppose it comes down to, are we allowed to optionally implement
> > part of the DT binding?
> 
> I'm not sure what you are asking. A lot of DT bindings have both
> optional and mandatory properties. For mvneta, the "phy" and "phy-mode"
> properties are listed as mandatory, so the driver can safely assume
> that they are always present. If there are reasons to leave them out,
> and for the driver to handle that case correctly, the binding
> should be updated to mark them as optional.

Hi Arnd

You are looking at it from the perspective of the driver. I was
meaning from the perspective of the DT blob. Can be blob assume the
driver implements all of the binding, all of the time?

You use fixed-phy when the MAC is connected to a switch, not a phy. Or
when the MAC is connected to an SFP module. The driver can currently
be built to not implement the fixed-phy party of the binding. Is that
O.K. from the perspective of the DT blob? Or should the driver always
implement all of the binding, in which these NOP stubs should be
removed and fixed phy always be enabled for the drivers that use it.

	Andrew

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] mvneta: add FIXED_PHY dependency
  2015-11-09 17:08       ` Russell King - ARM Linux
@ 2015-11-09 17:12         ` Arnd Bergmann
  -1 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2015-11-09 17:12 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Russell King - ARM Linux, Thomas Petazzoni, Andrew Lunn,
	Florian Fainelli, netdev, linux-kernel, Stas Sergeev,
	David S. Miller

On Monday 09 November 2015 17:08:34 Russell King - ARM Linux wrote:
> They are "optional" because when you're using a DSA switch, you don't
> specify a PHY (because, there isn't one).  For example, this is what
> I'm using with an Armada 388 board with a Marvell DSA switch.  The
> DSA does not appear as a PHY, and no node in the DSA stanza can be
> referenced for a phy entry in the ethernet device's stanza.
> 
>                         eth1: ethernet@30000 {
>                                 compatible = "marvell,armada-370-neta";
>                                 reg = <0x30000 0x4000>;
>                                 interrupts-extended = <&mpic 10>;
>                                 clocks = <&gateclk 3>;
>                                 managed = "in-band-status";
>                                 phy-mode = "sgmii";
>                                 status = "okay";
>                         };
> 
> 

Ok, then it would be nice to change the binding to reflect that,
and also document the "managed" property there.

	Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] mvneta: add FIXED_PHY dependency
@ 2015-11-09 17:12         ` Arnd Bergmann
  0 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2015-11-09 17:12 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 09 November 2015 17:08:34 Russell King - ARM Linux wrote:
> They are "optional" because when you're using a DSA switch, you don't
> specify a PHY (because, there isn't one).  For example, this is what
> I'm using with an Armada 388 board with a Marvell DSA switch.  The
> DSA does not appear as a PHY, and no node in the DSA stanza can be
> referenced for a phy entry in the ethernet device's stanza.
> 
>                         eth1: ethernet at 30000 {
>                                 compatible = "marvell,armada-370-neta";
>                                 reg = <0x30000 0x4000>;
>                                 interrupts-extended = <&mpic 10>;
>                                 clocks = <&gateclk 3>;
>                                 managed = "in-band-status";
>                                 phy-mode = "sgmii";
>                                 status = "okay";
>                         };
> 
> 

Ok, then it would be nice to change the binding to reflect that,
and also document the "managed" property there.

	Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] mvneta: add FIXED_PHY dependency
  2015-11-09 17:08       ` Andrew Lunn
@ 2015-11-09 17:14         ` Arnd Bergmann
  -1 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2015-11-09 17:14 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Thomas Petazzoni, Florian Fainelli, linux-kernel,
	Stas Sergeev, David S. Miller, linux-arm-kernel

On Monday 09 November 2015 18:08:49 Andrew Lunn wrote:
> > > I suppose it comes down to, are we allowed to optionally implement
> > > part of the DT binding?
> > 
> > I'm not sure what you are asking. A lot of DT bindings have both
> > optional and mandatory properties. For mvneta, the "phy" and "phy-mode"
> > properties are listed as mandatory, so the driver can safely assume
> > that they are always present. If there are reasons to leave them out,
> > and for the driver to handle that case correctly, the binding
> > should be updated to mark them as optional.
> 
> Hi Arnd
> 
> You are looking at it from the perspective of the driver. I was
> meaning from the perspective of the DT blob. Can be blob assume the
> driver implements all of the binding, all of the time?

That question is not really relevant: the DT describes the hardware,
it doesn't matter whether there are drivers for all the bits or
whether all properties are read.

> You use fixed-phy when the MAC is connected to a switch, not a phy. Or
> when the MAC is connected to an SFP module. The driver can currently
> be built to not implement the fixed-phy party of the binding. Is that
> O.K. from the perspective of the DT blob? Or should the driver always
> implement all of the binding, in which these NOP stubs should be
> removed and fixed phy always be enabled for the drivers that use it.

Sure, that is ok.

	Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] mvneta: add FIXED_PHY dependency
@ 2015-11-09 17:14         ` Arnd Bergmann
  0 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2015-11-09 17:14 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 09 November 2015 18:08:49 Andrew Lunn wrote:
> > > I suppose it comes down to, are we allowed to optionally implement
> > > part of the DT binding?
> > 
> > I'm not sure what you are asking. A lot of DT bindings have both
> > optional and mandatory properties. For mvneta, the "phy" and "phy-mode"
> > properties are listed as mandatory, so the driver can safely assume
> > that they are always present. If there are reasons to leave them out,
> > and for the driver to handle that case correctly, the binding
> > should be updated to mark them as optional.
> 
> Hi Arnd
> 
> You are looking at it from the perspective of the driver. I was
> meaning from the perspective of the DT blob. Can be blob assume the
> driver implements all of the binding, all of the time?

That question is not really relevant: the DT describes the hardware,
it doesn't matter whether there are drivers for all the bits or
whether all properties are read.

> You use fixed-phy when the MAC is connected to a switch, not a phy. Or
> when the MAC is connected to an SFP module. The driver can currently
> be built to not implement the fixed-phy party of the binding. Is that
> O.K. from the perspective of the DT blob? Or should the driver always
> implement all of the binding, in which these NOP stubs should be
> removed and fixed phy always be enabled for the drivers that use it.

Sure, that is ok.

	Arnd

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] mvneta: add FIXED_PHY dependency
  2015-11-09 17:12         ` Arnd Bergmann
@ 2015-11-09 17:31           ` Russell King - ARM Linux
  -1 siblings, 0 replies; 20+ messages in thread
From: Russell King - ARM Linux @ 2015-11-09 17:31 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel, Thomas Petazzoni, Andrew Lunn,
	Florian Fainelli, netdev, linux-kernel, Stas Sergeev,
	David S. Miller

On Mon, Nov 09, 2015 at 06:12:02PM +0100, Arnd Bergmann wrote:
> On Monday 09 November 2015 17:08:34 Russell King - ARM Linux wrote:
> > They are "optional" because when you're using a DSA switch, you don't
> > specify a PHY (because, there isn't one).  For example, this is what
> > I'm using with an Armada 388 board with a Marvell DSA switch.  The
> > DSA does not appear as a PHY, and no node in the DSA stanza can be
> > referenced for a phy entry in the ethernet device's stanza.
> > 
> >                         eth1: ethernet@30000 {
> >                                 compatible = "marvell,armada-370-neta";
> >                                 reg = <0x30000 0x4000>;
> >                                 interrupts-extended = <&mpic 10>;
> >                                 clocks = <&gateclk 3>;
> >                                 managed = "in-band-status";
> >                                 phy-mode = "sgmii";
> >                                 status = "okay";
> >                         };
> > 
> > 
> 
> Ok, then it would be nice to change the binding to reflect that,
> and also document the "managed" property there.

"managed" is already documented.  See
Documentation/devicetree/bindings/net/ethernet.txt:

- managed: string, specifies the PHY management type. Supported values are:
  "auto", "in-band-status". "auto" is the default, it usess MDIO for
  management if fixed-link is not specified.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] mvneta: add FIXED_PHY dependency
@ 2015-11-09 17:31           ` Russell King - ARM Linux
  0 siblings, 0 replies; 20+ messages in thread
From: Russell King - ARM Linux @ 2015-11-09 17:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Nov 09, 2015 at 06:12:02PM +0100, Arnd Bergmann wrote:
> On Monday 09 November 2015 17:08:34 Russell King - ARM Linux wrote:
> > They are "optional" because when you're using a DSA switch, you don't
> > specify a PHY (because, there isn't one).  For example, this is what
> > I'm using with an Armada 388 board with a Marvell DSA switch.  The
> > DSA does not appear as a PHY, and no node in the DSA stanza can be
> > referenced for a phy entry in the ethernet device's stanza.
> > 
> >                         eth1: ethernet at 30000 {
> >                                 compatible = "marvell,armada-370-neta";
> >                                 reg = <0x30000 0x4000>;
> >                                 interrupts-extended = <&mpic 10>;
> >                                 clocks = <&gateclk 3>;
> >                                 managed = "in-band-status";
> >                                 phy-mode = "sgmii";
> >                                 status = "okay";
> >                         };
> > 
> > 
> 
> Ok, then it would be nice to change the binding to reflect that,
> and also document the "managed" property there.

"managed" is already documented.  See
Documentation/devicetree/bindings/net/ethernet.txt:

- managed: string, specifies the PHY management type. Supported values are:
  "auto", "in-band-status". "auto" is the default, it usess MDIO for
  management if fixed-link is not specified.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] mvneta: add FIXED_PHY dependency
  2015-11-09 17:08       ` Andrew Lunn
@ 2015-11-09 17:33         ` Russell King - ARM Linux
  -1 siblings, 0 replies; 20+ messages in thread
From: Russell King - ARM Linux @ 2015-11-09 17:33 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Arnd Bergmann, Thomas Petazzoni, Florian Fainelli, netdev,
	linux-kernel, Stas Sergeev, David S. Miller, linux-arm-kernel

On Mon, Nov 09, 2015 at 06:08:49PM +0100, Andrew Lunn wrote:
> You use fixed-phy when the MAC is connected to a switch, not a phy. Or
> when the MAC is connected to an SFP module.

... hopefully not for much longer.  Once -rc1 is out, I'll sort out
posting my phylink and SFP module hotplug support as a RFC.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] mvneta: add FIXED_PHY dependency
@ 2015-11-09 17:33         ` Russell King - ARM Linux
  0 siblings, 0 replies; 20+ messages in thread
From: Russell King - ARM Linux @ 2015-11-09 17:33 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Nov 09, 2015 at 06:08:49PM +0100, Andrew Lunn wrote:
> You use fixed-phy when the MAC is connected to a switch, not a phy. Or
> when the MAC is connected to an SFP module.

... hopefully not for much longer.  Once -rc1 is out, I'll sort out
posting my phylink and SFP module hotplug support as a RFC.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2015-11-09 17:33 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-09 14:08 [PATCH] mvneta: add FIXED_PHY dependency Arnd Bergmann
2015-11-09 14:08 ` Arnd Bergmann
2015-11-09 16:36 ` David Miller
2015-11-09 16:36   ` David Miller
2015-11-09 16:42 ` Andrew Lunn
2015-11-09 16:42   ` Andrew Lunn
2015-11-09 16:57   ` Arnd Bergmann
2015-11-09 16:57     ` Arnd Bergmann
2015-11-09 17:08     ` Russell King - ARM Linux
2015-11-09 17:08       ` Russell King - ARM Linux
2015-11-09 17:12       ` Arnd Bergmann
2015-11-09 17:12         ` Arnd Bergmann
2015-11-09 17:31         ` Russell King - ARM Linux
2015-11-09 17:31           ` Russell King - ARM Linux
2015-11-09 17:08     ` Andrew Lunn
2015-11-09 17:08       ` Andrew Lunn
2015-11-09 17:14       ` Arnd Bergmann
2015-11-09 17:14         ` Arnd Bergmann
2015-11-09 17:33       ` Russell King - ARM Linux
2015-11-09 17:33         ` Russell King - ARM Linux

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.