| Message ID | 4225885.EYXTHjRPmX@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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 = <ð1>; dsa,mii-bus = <&mdio>; pinctrl-0 = <&clearfog_dsa0_clk_pins &clearfog_dsa0_pins>; pinctrl-names = "default"; #address-cells = <2>; #size-cells = <0>; switch@0 { ... }; };
> > 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
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
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
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.
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.
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.
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?