[PATCH] mvneta: add FIXED_PHY dependency

[PATCH] mvneta: add FIXED_PHY dependency

[Arnd Bergmann]

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.

Re: [PATCH] mvneta: add FIXED_PHY dependency

[David Miller]

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.

Re: [PATCH] mvneta: add FIXED_PHY dependency

[Andrew Lunn]

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

Re: [PATCH] mvneta: add FIXED_PHY dependency

[Arnd Bergmann]

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

Re: [PATCH] mvneta: add FIXED_PHY dependency

[Russell King - ARM Linux]

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.

Re: [PATCH] mvneta: add FIXED_PHY dependency

[Arnd Bergmann]

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

Re: [PATCH] mvneta: add FIXED_PHY dependency

[Russell King - ARM Linux]

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.

Re: [PATCH] mvneta: add FIXED_PHY dependency

[Andrew Lunn]

> > 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

Re: [PATCH] mvneta: add FIXED_PHY dependency

[Arnd Bergmann]

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

Re: [PATCH] mvneta: add FIXED_PHY dependency

[Russell King - ARM Linux]

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.