| Message ID | 1373902450-11857-2-git-send-email-thomas.petazzoni@free-electrons.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello Thomas, 2013/7/15 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > Some Ethernet MACs have a "fixed link", and are not connected to a > normal MDIO-managed PHY device. For those situations, a Device Tree > binding allows to describe a "fixed link", as a "fixed-link" property > of the Ethernet device Device Tree node. > > This patch adds: > > * A documentation for the Device Tree property "fixed-link". > > * A of_phy_register_fixed_link() OF helper, which provided an OF node > that contains a "fixed-link" property, registers the corresponding > fixed PHY. > > * Removes the warning on the of_phy_connect_fixed_link() that says > new drivers should not use it, since Grant Likely indicated that > this "fixed-link" property is indeed the way to go. Some minor nitpicks below. Other than that: > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Reviewed-by: Florian Fainelli <florian@openwrt.org> > --- > .../devicetree/bindings/net/fixed-link.txt | 26 ++++++++++++++++ > drivers/of/of_mdio.c | 36 +++++++++++++++++++--- > include/linux/of_mdio.h | 10 ++++++ > 3 files changed, 68 insertions(+), 4 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt > > diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt > new file mode 100644 > index 0000000..25a009a > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/fixed-link.txt > @@ -0,0 +1,26 @@ > +Fixed link Device Tree binding > +------------------------------ > + > +Some Ethernet MACs have a "fixed link", and are not connected to a > +normal MDIO-managed PHY device. For those situations, a Device Tree > +binding allows to describe a "fixed link". You could also add the following: "this might be the case when using an Ethernet switch whose management interface is not accessible using the MDIO bus, or for specific PHYs where the control path of the PHY might be separated from its data path". > + > +Such a fixed link situation is described within an Ethernet device > +Device Tree node using a 'fixed-link' property, composed of 5 > +elements: > + > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in > + the system. You might want to detail that will be used to fake PHY identification registers 2 & 3. > + 2. The duplex (1 for full-duplex, 0 for half-duplex) > + 3. The speed (10, 100, 1000) > + 4. The pause setting (1 for enabled, 0 for disabled) > + 5. The asym pause setting (1 for enabled, 0 for disabled) > + > +Example: > + > +ethernet@0 { > + ... > + fixed-link = <1 1 1000 0 0>; > + ... > +}; > + > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index d5a57a9..66d5591 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -14,6 +14,7 @@ > #include <linux/netdevice.h> > #include <linux/err.h> > #include <linux/phy.h> > +#include <linux/phy_fixed.h> > #include <linux/of.h> > #include <linux/of_irq.h> > #include <linux/of_mdio.h> > @@ -215,10 +216,6 @@ EXPORT_SYMBOL(of_phy_connect); > * @dev: pointer to net_device claiming the phy > * @hndlr: Link state callback for the network device > * @iface: PHY data interface type > - * > - * This function is a temporary stop-gap and will be removed soon. It is > - * only to support the fs_enet, ucc_geth and gianfar Ethernet drivers. Do > - * not call this function from new drivers. > */ > struct phy_device *of_phy_connect_fixed_link(struct net_device *dev, > void (*hndlr)(struct net_device *), > @@ -247,3 +244,34 @@ struct phy_device *of_phy_connect_fixed_link(struct net_device *dev, > return IS_ERR(phy) ? NULL : phy; > } > EXPORT_SYMBOL(of_phy_connect_fixed_link); > + > +#if defined(CONFIG_FIXED_PHY) > +/** > + * of_phy_register_fixed_link - Parse fixed-link property and register a dummy phy > + * @np: pointer to the OF device node that contains the "fixed-link" > + * property for which a dummy phy should be registered. > + */ > +#define FIXED_LINK_PROPERTIES_COUNT 5 > +int of_phy_register_fixed_link(struct device_node *np) > +{ > + struct fixed_phy_status status = {}; > + u32 fixed_link_props[FIXED_LINK_PROPERTIES_COUNT]; > + int ret; > + > + ret = of_property_read_u32_array(np, "fixed-link", > + fixed_link_props, > + FIXED_LINK_PROPERTIES_COUNT); > + if (ret < 0) > + return ret; > + > + status.link = 1; > + status.duplex = fixed_link_props[1]; > + status.speed = fixed_link_props[2]; > + status.pause = fixed_link_props[3]; > + status.asym_pause = fixed_link_props[4]; > + > + return fixed_phy_add(PHY_POLL, fixed_link_props[0], > + &status); > +} > +EXPORT_SYMBOL(of_phy_register_fixed_link); > +#endif > diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h > index 8163107..bf6efea 100644 > --- a/include/linux/of_mdio.h > +++ b/include/linux/of_mdio.h > @@ -57,4 +57,14 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np) > } > #endif /* CONFIG_OF */ > > +#if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY) > +extern int of_phy_register_fixed_link(struct device_node *np); > +#else > +static inline int of_phy_register_fixed_link(struct device_node *np) > +{ > + return -ENOSYS; > +} > +#endif > + > + > #endif /* __LINUX_OF_MDIO_H */ > -- > 1.8.1.2 > -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thomas, On Mon, Jul 15, 2013 at 04:34:08PM +0100, Thomas Petazzoni wrote: > Some Ethernet MACs have a "fixed link", and are not connected to a > normal MDIO-managed PHY device. For those situations, a Device Tree > binding allows to describe a "fixed link", as a "fixed-link" property > of the Ethernet device Device Tree node. > > This patch adds: > > * A documentation for the Device Tree property "fixed-link". > > * A of_phy_register_fixed_link() OF helper, which provided an OF node > that contains a "fixed-link" property, registers the corresponding > fixed PHY. > > * Removes the warning on the of_phy_connect_fixed_link() that says > new drivers should not use it, since Grant Likely indicated that > this "fixed-link" property is indeed the way to go. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > .../devicetree/bindings/net/fixed-link.txt | 26 ++++++++++++++++ > drivers/of/of_mdio.c | 36 +++++++++++++++++++--- > include/linux/of_mdio.h | 10 ++++++ > 3 files changed, 68 insertions(+), 4 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt > > diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt > new file mode 100644 > index 0000000..25a009a > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/fixed-link.txt > @@ -0,0 +1,26 @@ > +Fixed link Device Tree binding > +------------------------------ > + > +Some Ethernet MACs have a "fixed link", and are not connected to a > +normal MDIO-managed PHY device. For those situations, a Device Tree > +binding allows to describe a "fixed link". Are partictular MACs fixed link, or can some either be either fixed link or wired to an MDIO-managed PHY? i.e. can we assume a given MAC is fixed-link from its compatible string? > + > +Such a fixed link situation is described within an Ethernet device > +Device Tree node using a 'fixed-link' property, composed of 5 > +elements: I'm not sure grouping these values together is the best way of handling this. It's rather opaque, and inflexible for future extension. > + > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in > + the system. Is there any reason this couldn't be allocated dynamically within the kernel as needed? I don't see why an arbitrary unique value should be a dt binding requirement; it seems like a leak of Linux implementation details. > + 2. The duplex (1 for full-duplex, 0 for half-duplex) Will this change for a given MAC? Could we not have a boolean property for each of these, and require one to be present? Possibly fixed-link-full-duplex / fix-link-half-duplex? > + 3. The speed (10, 100, 1000) fixed-link-speed? > + 4. The pause setting (1 for enabled, 0 for disabled) > + 5. The asym pause setting (1 for enabled, 0 for disabled) Boolean properties for both of these? Thanks, Mark. > + > +Example: > + > +ethernet@0 { > + ... > + fixed-link = <1 1 1000 0 0>; > + ... > +}; > + > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index d5a57a9..66d5591 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -14,6 +14,7 @@ > #include <linux/netdevice.h> > #include <linux/err.h> > #include <linux/phy.h> > +#include <linux/phy_fixed.h> > #include <linux/of.h> > #include <linux/of_irq.h> > #include <linux/of_mdio.h> > @@ -215,10 +216,6 @@ EXPORT_SYMBOL(of_phy_connect); > * @dev: pointer to net_device claiming the phy > * @hndlr: Link state callback for the network device > * @iface: PHY data interface type > - * > - * This function is a temporary stop-gap and will be removed soon. It is > - * only to support the fs_enet, ucc_geth and gianfar Ethernet drivers. Do > - * not call this function from new drivers. > */ > struct phy_device *of_phy_connect_fixed_link(struct net_device *dev, > void (*hndlr)(struct net_device *), > @@ -247,3 +244,34 @@ struct phy_device *of_phy_connect_fixed_link(struct net_device *dev, > return IS_ERR(phy) ? NULL : phy; > } > EXPORT_SYMBOL(of_phy_connect_fixed_link); > + > +#if defined(CONFIG_FIXED_PHY) > +/** > + * of_phy_register_fixed_link - Parse fixed-link property and register a dummy phy > + * @np: pointer to the OF device node that contains the "fixed-link" > + * property for which a dummy phy should be registered. > + */ > +#define FIXED_LINK_PROPERTIES_COUNT 5 > +int of_phy_register_fixed_link(struct device_node *np) > +{ > + struct fixed_phy_status status = {}; > + u32 fixed_link_props[FIXED_LINK_PROPERTIES_COUNT]; > + int ret; > + > + ret = of_property_read_u32_array(np, "fixed-link", > + fixed_link_props, > + FIXED_LINK_PROPERTIES_COUNT); > + if (ret < 0) > + return ret; > + > + status.link = 1; > + status.duplex = fixed_link_props[1]; > + status.speed = fixed_link_props[2]; > + status.pause = fixed_link_props[3]; > + status.asym_pause = fixed_link_props[4]; > + > + return fixed_phy_add(PHY_POLL, fixed_link_props[0], > + &status); > +} > +EXPORT_SYMBOL(of_phy_register_fixed_link); > +#endif > diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h > index 8163107..bf6efea 100644 > --- a/include/linux/of_mdio.h > +++ b/include/linux/of_mdio.h > @@ -57,4 +57,14 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np) > } > #endif /* CONFIG_OF */ > > +#if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY) > +extern int of_phy_register_fixed_link(struct device_node *np); > +#else > +static inline int of_phy_register_fixed_link(struct device_node *np) > +{ > + return -ENOSYS; > +} > +#endif > + > + > #endif /* __LINUX_OF_MDIO_H */ > -- > 1.8.1.2 > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 15, 2013 at 4:34 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Some Ethernet MACs have a "fixed link", and are not connected to a > normal MDIO-managed PHY device. For those situations, a Device Tree > binding allows to describe a "fixed link", as a "fixed-link" property > of the Ethernet device Device Tree node. > > This patch adds: > > * A documentation for the Device Tree property "fixed-link". > > * A of_phy_register_fixed_link() OF helper, which provided an OF node > that contains a "fixed-link" property, registers the corresponding > fixed PHY. > > * Removes the warning on the of_phy_connect_fixed_link() that says > new drivers should not use it, since Grant Likely indicated that > this "fixed-link" property is indeed the way to go. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > .../devicetree/bindings/net/fixed-link.txt | 26 ++++++++++++++++ > drivers/of/of_mdio.c | 36 +++++++++++++++++++--- > include/linux/of_mdio.h | 10 ++++++ > 3 files changed, 68 insertions(+), 4 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt > > diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt > new file mode 100644 > index 0000000..25a009a > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/fixed-link.txt > @@ -0,0 +1,26 @@ > +Fixed link Device Tree binding > +------------------------------ > + > +Some Ethernet MACs have a "fixed link", and are not connected to a > +normal MDIO-managed PHY device. For those situations, a Device Tree > +binding allows to describe a "fixed link". > + > +Such a fixed link situation is described within an Ethernet device > +Device Tree node using a 'fixed-link' property, composed of 5 > +elements: > + > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in > + the system. That's just loony! :) Regardless of existing code doing this, it is absolutely ridiculous to have it in the driver. The kernel should handle generating a phy id transparently. I'd rather mark this field as reserved in the binding and change the code to not care about it anymore. g. > + 2. The duplex (1 for full-duplex, 0 for half-duplex) > + 3. The speed (10, 100, 1000) > + 4. The pause setting (1 for enabled, 0 for disabled) > + 5. The asym pause setting (1 for enabled, 0 for disabled) > + > +Example: > + > +ethernet@0 { > + ... > + fixed-link = <1 1 1000 0 0>; > + ... > +}; > + > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index d5a57a9..66d5591 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -14,6 +14,7 @@ > #include <linux/netdevice.h> > #include <linux/err.h> > #include <linux/phy.h> > +#include <linux/phy_fixed.h> > #include <linux/of.h> > #include <linux/of_irq.h> > #include <linux/of_mdio.h> > @@ -215,10 +216,6 @@ EXPORT_SYMBOL(of_phy_connect); > * @dev: pointer to net_device claiming the phy > * @hndlr: Link state callback for the network device > * @iface: PHY data interface type > - * > - * This function is a temporary stop-gap and will be removed soon. It is > - * only to support the fs_enet, ucc_geth and gianfar Ethernet drivers. Do > - * not call this function from new drivers. > */ > struct phy_device *of_phy_connect_fixed_link(struct net_device *dev, > void (*hndlr)(struct net_device *), > @@ -247,3 +244,34 @@ struct phy_device *of_phy_connect_fixed_link(struct net_device *dev, > return IS_ERR(phy) ? NULL : phy; > } > EXPORT_SYMBOL(of_phy_connect_fixed_link); > + > +#if defined(CONFIG_FIXED_PHY) > +/** > + * of_phy_register_fixed_link - Parse fixed-link property and register a dummy phy > + * @np: pointer to the OF device node that contains the "fixed-link" > + * property for which a dummy phy should be registered. > + */ > +#define FIXED_LINK_PROPERTIES_COUNT 5 > +int of_phy_register_fixed_link(struct device_node *np) > +{ > + struct fixed_phy_status status = {}; > + u32 fixed_link_props[FIXED_LINK_PROPERTIES_COUNT]; > + int ret; > + > + ret = of_property_read_u32_array(np, "fixed-link", > + fixed_link_props, > + FIXED_LINK_PROPERTIES_COUNT); > + if (ret < 0) > + return ret; > + > + status.link = 1; > + status.duplex = fixed_link_props[1]; > + status.speed = fixed_link_props[2]; > + status.pause = fixed_link_props[3]; > + status.asym_pause = fixed_link_props[4]; > + > + return fixed_phy_add(PHY_POLL, fixed_link_props[0], > + &status); > +} > +EXPORT_SYMBOL(of_phy_register_fixed_link); > +#endif > diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h > index 8163107..bf6efea 100644 > --- a/include/linux/of_mdio.h > +++ b/include/linux/of_mdio.h > @@ -57,4 +57,14 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np) > } > #endif /* CONFIG_OF */ > > +#if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY) > +extern int of_phy_register_fixed_link(struct device_node *np); > +#else > +static inline int of_phy_register_fixed_link(struct device_node *np) > +{ > + return -ENOSYS; > +} > +#endif > + > + > #endif /* __LINUX_OF_MDIO_H */ > -- > 1.8.1.2 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, 2013/7/23 Mark Rutland <mark.rutland@arm.com>: [snip] >> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt >> @@ -0,0 +1,26 @@ >> +Fixed link Device Tree binding >> +------------------------------ >> + >> +Some Ethernet MACs have a "fixed link", and are not connected to a >> +normal MDIO-managed PHY device. For those situations, a Device Tree >> +binding allows to describe a "fixed link". > > Are partictular MACs fixed link, or can some either be either fixed link > or wired to an MDIO-managed PHY? i.e. can we assume a given MAC is > fixed-link from its compatible string? There are different use-cases out there: - you lack or do not want to have a proper switch/PHY library driver for an Ethernet switch, but that specific switch may still be accessible on the MDIO bus, yet you just want to expose a link UP from the CPU port perspective, that specific use case should dissapear in favor of a simplified PHY library driver which just eventually makes the link appear as UP from the CPU port perspective - you have absolutely no control over the PHY on the MDIO bus * for switches, it could be SPI, I2C, GPIO whatever, so to avoid any dependency, you might just want to let the link appear as UP * or you could have a very different PHY whose data-path is Ethernet, but the control-path is something else, e.g: MoCA (which is more the kind of use-case I am interested in) > >> + >> +Such a fixed link situation is described within an Ethernet device >> +Device Tree node using a 'fixed-link' property, composed of 5 >> +elements: > > I'm not sure grouping these values together is the best way of handling > this. It's rather opaque, and inflexible for future extension. Well, I proposed making them look like a "standard" Ethernet PHY node but this somehow got rejected, see at the very bottom. > >> + >> + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in >> + the system. > > Is there any reason this couldn't be allocated dynamically within the > kernel as needed? I don't see why an arbitrary unique value should be a > dt binding requirement; it seems like a leak of Linux implementation > details. Honestly, I do not really think actually letting this be configurable is a good idea because it would become some sort of user-space ABI. If we want to expose some PHY identification registers, we could use some ID2 and ID3 values which are not allocated to any vendor, or allocated to the Linux foundation (e.g; like what USB does with Linux HUBs). > >> + 2. The duplex (1 for full-duplex, 0 for half-duplex) > > Will this change for a given MAC? > > Could we not have a boolean property for each of these, and require one > to be present? > > Possibly fixed-link-full-duplex / fix-link-half-duplex? > >> + 3. The speed (10, 100, 1000) > > fixed-link-speed? > >> + 4. The pause setting (1 for enabled, 0 for disabled) >> + 5. The asym pause setting (1 for enabled, 0 for disabled) > > Boolean properties for both of these? > > Thanks, > Mark. > >> + >> +Example: >> + >> +ethernet@0 { >> + ... >> + fixed-link = <1 1 1000 0 0>; >> + ... >> +}; >> + The initial proposal that I had suggested was this: ethernet-phy@0 { reg = <0>; id = <0xdeadbeef>; speed = <1000>; full-duplex; pause; asym-pause; }; which looks more or less what you seem to want as well but this somehow makes it look like a real PHY device, so we could argue that this has nowhere to be in the Device Tree. Arguably there are a gazillions of other bindings which imho should also not be in a Device Tree. -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Grant Likely, On Tue, 23 Jul 2013 12:39:52 +0100, Grant Likely wrote: > > +Such a fixed link situation is described within an Ethernet device > > +Device Tree node using a 'fixed-link' property, composed of 5 > > +elements: > > + > > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in > > + the system. > > That's just loony! :) Regardless of existing code doing this, it is > absolutely ridiculous to have it in the driver. The kernel should > handle generating a phy id transparently. I'd rather mark this field > as reserved in the binding and change the code to not care about it > anymore. In fact, this value is used for two things: * As the PHY address on the fake "fixed" MDIO bus. * As the PHY identifier, as reported by the MII registers PHYS_ID1 (0x2) and PHYS_ID2 (0x3). I think this doesn't make sense, because the two things are completely unrelated. Ideally, we'd like the PHY identifier for fixed PHYs to be something fixed, identical for all fixed PHYs. The problem is finding an OUI and device number that is available for that, but maybe we can ask the OpenMoko people to allocate one (see http://wiki.openmoko.org/wiki/OUI). Then, the PHY address could be generated dynamically. This would require: * Adding a fixed_phy_create() function that internally uses fixed_phy_add(), but before that creates an unique PHY address for this newly created PHY. Those unique addresses will be generated by incrementing a global number of fixed PHYs, up to PHY_MAX_ADDR, which is the maximum number of fixed PHYs that can anyway be registered on the fixed MDIO bus. fixed_phy_create() would return this PHY address (positive) on success, or a negative error code on failure. * Change of_phy_register_fixed_link() to call fixed_phy_create() instead of fixed_phy_add() and make it return the PHY address allocated by fixed_phy_create(). * Add a of_phy_connect_fixed_link_direct() that is similar to of_phy_connect_fixed_link() but takes an additional PHY address as argument and uses that to generate the 'bus_id' used to find the phy_device. Grant, Mark, Florian, do you have other proposals? Thanks, Thomas Petazzoni
Dear Mark Rutland, On Tue, 23 Jul 2013 12:22:59 +0100, Mark Rutland wrote: > > +Some Ethernet MACs have a "fixed link", and are not connected to a > > +normal MDIO-managed PHY device. For those situations, a Device Tree > > +binding allows to describe a "fixed link". > > Are partictular MACs fixed link, or can some either be either fixed link > or wired to an MDIO-managed PHY? i.e. can we assume a given MAC is > fixed-link from its compatible string? No, you can't. The case that I have is that the mvneta Ethernet MAC (of Marvell 370/XP SOCs) is sometimes used with a MDIO-managed PHY, and sometimes used with a switch that isn't manageable at all and should be considered as a fixed link. So no, the compatible string of the Ethernet MAC cannot be used to determine whether we're wired fixed link or to a classical MDIO-managed PHY. > > +Such a fixed link situation is described within an Ethernet device > > +Device Tree node using a 'fixed-link' property, composed of 5 > > +elements: > > I'm not sure grouping these values together is the best way of handling > this. It's rather opaque, and inflexible for future extension. That's the DT binding that has been used by PowerPC platforms since several years, and I've simply re-used it. See 'git grep fixed-link arch/powerpc/boot/dts'. I have nothing against creating another DT binding, but for a start, I thought using existing bindings would be the best idea. > > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in > > + the system. > > Is there any reason this couldn't be allocated dynamically within the > kernel as needed? I don't see why an arbitrary unique value should be a > dt binding requirement; it seems like a leak of Linux implementation > details. As I pointed out in my reply to Grant, this value is used both as the PHY address on the fake fixed MDIO bus, and as the PHY identifier as reported by MII registers PHYS_ID1 and PHYS_ID2. If we can get assigned a proper PHY identifier that is used statically by the driver (it doesn't have to be different per fixed PHY instance), then we can allocate the PHY address dynamically. It requires a little bit of API changes but that's certainly doable. See my reply to Grant for a proposal about this. > > + 2. The duplex (1 for full-duplex, 0 for half-duplex) > > Will this change for a given MAC? > > Could we not have a boolean property for each of these, and require one > to be present? > > Possibly fixed-link-full-duplex / fix-link-half-duplex? > > > + 3. The speed (10, 100, 1000) > > fixed-link-speed? > > > + 4. The pause setting (1 for enabled, 0 for disabled) > > + 5. The asym pause setting (1 for enabled, 0 for disabled) > > Boolean properties for both of these? As Florian already answered, he already proposed something like this in the past, and it was rejected because a fixed PHY is not a piece of hardware and should therefore not be represented in the Device Tree. However, the fact that the MAC is not connected to a MDIO-manageable PHY but to some fixed-link thing is a property of the MAC hardware layout, and can therefore be expressed as a property of the MAC hardware. See the thread that starts at http://article.gmane.org/gmane.linux.network/275771, and specifically Grant answers to Florian suggestions of having DT nodes to represent fixed PHY: http://article.gmane.org/gmane.linux.network/276208. Grant's answer was: """ I think this discussion is going in the wrong direction. The concept of a dummy phy is really a Linux kernel internal detail. Creating some kind of dummy MDIO bus node does not describe the hardware. There is already support in the kernel for Ethernet MACs connected directly to a switch or other device. It is far better to describe how the MAC needs to be configured than to invent a non-existent phy. Search for "fixed-link" in the kernel tree to see how it is used. """ Best regards, Thomas
Hello Thomas, 2013/7/30 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > Dear Grant Likely, > > On Tue, 23 Jul 2013 12:39:52 +0100, Grant Likely wrote: > >> > +Such a fixed link situation is described within an Ethernet device >> > +Device Tree node using a 'fixed-link' property, composed of 5 >> > +elements: >> > + >> > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in >> > + the system. >> >> That's just loony! :) Regardless of existing code doing this, it is >> absolutely ridiculous to have it in the driver. The kernel should >> handle generating a phy id transparently. I'd rather mark this field >> as reserved in the binding and change the code to not care about it >> anymore. > > In fact, this value is used for two things: > > * As the PHY address on the fake "fixed" MDIO bus. > > * As the PHY identifier, as reported by the MII registers PHYS_ID1 > (0x2) and PHYS_ID2 (0x3). Right, so I would start with disambiguating the two and just forget about PHYS_ID1 and PHYS_ID2 for the moment since they probably do not need to be per-PHY configurable. > > I think this doesn't make sense, because the two things are completely > unrelated. Ideally, we'd like the PHY identifier for fixed PHYs to be > something fixed, identical for all fixed PHYs. The problem is finding > an OUI and device number that is available for that, but maybe we can > ask the OpenMoko people to allocate one (see > http://wiki.openmoko.org/wiki/OUI). Well that would be ideal indeed, but I am wondering if we cannot just go with some kind of magic value here anyway, regardless of allocations since this is not a real hardware device. How about the Linux Foundation? Is not that the same problem as with gadget USB devices which should have some sort of real MAC address for instance? > > Then, the PHY address could be generated dynamically. This would > require: > > * Adding a fixed_phy_create() function that internally uses > fixed_phy_add(), but before that creates an unique PHY address for > this newly created PHY. Those unique addresses will be generated by > incrementing a global number of fixed PHYs, up to PHY_MAX_ADDR, > which is the maximum number of fixed PHYs that can anyway be > registered on the fixed MDIO bus. Even though these are purely software PHY implementations, I believe that some sort of predictability would be welcome, so I would just use the phy_id argument passed to fixed_phy_add() as the address on the fixed MDIO bus like it is today. > > fixed_phy_create() would return this PHY address (positive) on > success, or a negative error code on failure. > > * Change of_phy_register_fixed_link() to call fixed_phy_create() > instead of fixed_phy_add() and make it return the PHY address > allocated by fixed_phy_create(). > > * Add a of_phy_connect_fixed_link_direct() that is similar to > of_phy_connect_fixed_link() but takes an additional PHY address as > argument and uses that to generate the 'bus_id' used to find the > phy_device. > > Grant, Mark, Florian, do you have other proposals? To sum up, let's just forget about the misuse of phy_id to fill in PHYS_ID1 and PHYS_ID2 registers and keep the existing code with a clear note that phy_id means the "PHY address on the fixed MDIO bus". -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Thomas, 2013/7/30 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > Dear Mark Rutland, > > On Tue, 23 Jul 2013 12:22:59 +0100, Mark Rutland wrote: > >> > +Some Ethernet MACs have a "fixed link", and are not connected to a >> > +normal MDIO-managed PHY device. For those situations, a Device Tree >> > +binding allows to describe a "fixed link". >> >> Are partictular MACs fixed link, or can some either be either fixed link >> or wired to an MDIO-managed PHY? i.e. can we assume a given MAC is >> fixed-link from its compatible string? > > No, you can't. The case that I have is that the mvneta Ethernet MAC > (of Marvell 370/XP SOCs) is sometimes used with a MDIO-managed PHY, and > sometimes used with a switch that isn't manageable at all and should be > considered as a fixed link. > > So no, the compatible string of the Ethernet MAC cannot be used to > determine whether we're wired fixed link or to a classical MDIO-managed > PHY. > >> > +Such a fixed link situation is described within an Ethernet device >> > +Device Tree node using a 'fixed-link' property, composed of 5 >> > +elements: >> >> I'm not sure grouping these values together is the best way of handling >> this. It's rather opaque, and inflexible for future extension. > > That's the DT binding that has been used by PowerPC platforms since > several years, and I've simply re-used it. See 'git grep fixed-link > arch/powerpc/boot/dts'. > > I have nothing against creating another DT binding, but for a start, I > thought using existing bindings would be the best idea. > >> > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in >> > + the system. >> >> Is there any reason this couldn't be allocated dynamically within the >> kernel as needed? I don't see why an arbitrary unique value should be a >> dt binding requirement; it seems like a leak of Linux implementation >> details. > > As I pointed out in my reply to Grant, this value is used both as the > PHY address on the fake fixed MDIO bus, and as the PHY identifier as > reported by MII registers PHYS_ID1 and PHYS_ID2. If we can get assigned > a proper PHY identifier that is used statically by the driver (it > doesn't have to be different per fixed PHY instance), then we can > allocate the PHY address dynamically. It requires a little bit of API > changes but that's certainly doable. See my reply to Grant for a > proposal about this. > >> > + 2. The duplex (1 for full-duplex, 0 for half-duplex) >> >> Will this change for a given MAC? >> >> Could we not have a boolean property for each of these, and require one >> to be present? >> >> Possibly fixed-link-full-duplex / fix-link-half-duplex? >> >> > + 3. The speed (10, 100, 1000) >> >> fixed-link-speed? >> >> > + 4. The pause setting (1 for enabled, 0 for disabled) >> > + 5. The asym pause setting (1 for enabled, 0 for disabled) >> >> Boolean properties for both of these? > > As Florian already answered, he already proposed something like this in > the past, and it was rejected because a fixed PHY is not a piece of > hardware and should therefore not be represented in the Device Tree. > However, the fact that the MAC is not connected to a MDIO-manageable > PHY but to some fixed-link thing is a property of the MAC hardware > layout, and can therefore be expressed as a property of the MAC > hardware. True, what I *did* not like about this "fixed-link" 5 integer property is that it does not present a consistent view of a PHY device and puts some properties in the Ethernet MAC node, while some other are in the Ethernet PHY node. From a hardware perspective, the Ethernet MAC and the Ethernet PHY are two pieces of hardware, so both should get their own node. That said, since "fixed PHY" devices are not connected on the MDIO bus, they cannot be represented as Ethernet PHY nodes as leafs of a parent MDIO bus node, so maybe what we could do is having the fixed PHY nodes child nodes of the Ethernet MAC but still make them look like a "relatively" conventional Ethernet PHY node? > > See the thread that starts at > http://article.gmane.org/gmane.linux.network/275771, and specifically > Grant answers to Florian suggestions of having DT nodes to represent > fixed PHY: http://article.gmane.org/gmane.linux.network/276208. Grant's > answer was: > > """ > I think this discussion is going in the wrong direction. The concept > of a dummy phy is really a Linux kernel internal detail. Creating some > kind of dummy MDIO bus node does not describe the hardware. There is > already support in the kernel for Ethernet MACs connected directly to > a switch or other device. It is far better to describe how the MAC > needs to be configured than to invent a non-existent phy. Search for > "fixed-link" in the kernel tree to see how it is used. > """ > > Best regards, > > Thomas > -- > Thomas Petazzoni, Free Electrons > Kernel, drivers, real-time and embedded Linux > development, consulting, training and support. > http://free-electrons.com
Dear Florian Fainelli, On Tue, 30 Jul 2013 11:05:04 +0100, Florian Fainelli wrote: > > In fact, this value is used for two things: > > > > * As the PHY address on the fake "fixed" MDIO bus. > > > > * As the PHY identifier, as reported by the MII registers PHYS_ID1 > > (0x2) and PHYS_ID2 (0x3). > > Right, so I would start with disambiguating the two and just forget > about PHYS_ID1 and PHYS_ID2 for the moment since they probably do not > need to be per-PHY configurable. Agreed. > > I think this doesn't make sense, because the two things are completely > > unrelated. Ideally, we'd like the PHY identifier for fixed PHYs to be > > something fixed, identical for all fixed PHYs. The problem is finding > > an OUI and device number that is available for that, but maybe we can > > ask the OpenMoko people to allocate one (see > > http://wiki.openmoko.org/wiki/OUI). > > Well that would be ideal indeed, but I am wondering if we cannot just > go with some kind of magic value here anyway, regardless of > allocations since this is not a real hardware device. Well, isn't that exactly what I'm proposing? Have a fixed value for PHYS_ID1 and PHYS_ID2, that is used for /all/ fixed PHYs. > How about the Linux Foundation? I am concerned about the time it would take to get a new OUI attributed. The OpenMoko OUI is said to be open for free and open source projects, I think the Linux kernel qualifies :) > Is not that the same problem as with gadget USB > devices which should have some sort of real MAC address for instance? They are either provided by the user or generated randomly, as far as I can see. > > Then, the PHY address could be generated dynamically. This would > > require: > > > > * Adding a fixed_phy_create() function that internally uses > > fixed_phy_add(), but before that creates an unique PHY address for > > this newly created PHY. Those unique addresses will be generated by > > incrementing a global number of fixed PHYs, up to PHY_MAX_ADDR, > > which is the maximum number of fixed PHYs that can anyway be > > registered on the fixed MDIO bus. > > Even though these are purely software PHY implementations, I believe > that some sort of predictability would be welcome, so I would just use > the phy_id argument passed to fixed_phy_add() as the address on the > fixed MDIO bus like it is today. Well, the whole starting point of this discussion is precisely that both Grant and Mark disliked this phy ID that had to be globally unique, and they wanted it to be dynamically allocated by the kernel, because the DT shouldn't have to care about such internal details. > > Grant, Mark, Florian, do you have other proposals? > > To sum up, let's just forget about the misuse of phy_id to fill in > PHYS_ID1 and PHYS_ID2 registers and keep the existing code with a > clear note that phy_id means the "PHY address on the fixed MDIO bus". Except that Grant and Mark point was precisely that the "PHY address on the fake MDIO bus" is not a description of the hardware, and therefore shouldn't be mentioned in the Device Tree but instead taken care of by the kernel itself. Best regards, Thomas
2013/7/30 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > Dear Florian Fainelli, > > On Tue, 30 Jul 2013 11:05:04 +0100, Florian Fainelli wrote: > >> > In fact, this value is used for two things: >> > >> > * As the PHY address on the fake "fixed" MDIO bus. >> > >> > * As the PHY identifier, as reported by the MII registers PHYS_ID1 >> > (0x2) and PHYS_ID2 (0x3). >> >> Right, so I would start with disambiguating the two and just forget >> about PHYS_ID1 and PHYS_ID2 for the moment since they probably do not >> need to be per-PHY configurable. > > Agreed. > >> > I think this doesn't make sense, because the two things are completely >> > unrelated. Ideally, we'd like the PHY identifier for fixed PHYs to be >> > something fixed, identical for all fixed PHYs. The problem is finding >> > an OUI and device number that is available for that, but maybe we can >> > ask the OpenMoko people to allocate one (see >> > http://wiki.openmoko.org/wiki/OUI). >> >> Well that would be ideal indeed, but I am wondering if we cannot just >> go with some kind of magic value here anyway, regardless of >> allocations since this is not a real hardware device. > > Well, isn't that exactly what I'm proposing? Have a fixed value for > PHYS_ID1 and PHYS_ID2, that is used for /all/ fixed PHYs. > >> How about the Linux Foundation? > > I am concerned about the time it would take to get a new OUI > attributed. The OpenMoko OUI is said to be open for free and open > source projects, I think the Linux kernel qualifies :) > >> Is not that the same problem as with gadget USB >> devices which should have some sort of real MAC address for instance? > > They are either provided by the user or generated randomly, as far as I > can see. > >> > Then, the PHY address could be generated dynamically. This would >> > require: >> > >> > * Adding a fixed_phy_create() function that internally uses >> > fixed_phy_add(), but before that creates an unique PHY address for >> > this newly created PHY. Those unique addresses will be generated by >> > incrementing a global number of fixed PHYs, up to PHY_MAX_ADDR, >> > which is the maximum number of fixed PHYs that can anyway be >> > registered on the fixed MDIO bus. >> >> Even though these are purely software PHY implementations, I believe >> that some sort of predictability would be welcome, so I would just use >> the phy_id argument passed to fixed_phy_add() as the address on the >> fixed MDIO bus like it is today. > > Well, the whole starting point of this discussion is precisely that > both Grant and Mark disliked this phy ID that had to be globally > unique, and they wanted it to be dynamically allocated by the kernel, > because the DT shouldn't have to care about such internal details. Ok, well, thinking about this some more, we do not need to be able to specify the fixed PHY fake MDIO address in the binding anyway. My concern was first with how to ensure that this would be relatively stable from an Ethernet driver and user-space perspective, but this will be stable if we allocate unique IDs in say, parsing/probing order anyway. > >> > Grant, Mark, Florian, do you have other proposals? >> >> To sum up, let's just forget about the misuse of phy_id to fill in >> PHYS_ID1 and PHYS_ID2 registers and keep the existing code with a >> clear note that phy_id means the "PHY address on the fixed MDIO bus". > > Except that Grant and Mark point was precisely that the "PHY address on > the fake MDIO bus" is not a description of the hardware, and therefore > shouldn't be mentioned in the Device Tree but instead taken care of by > the kernel itself. Right, that works for me now. -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thomas, On Tue, Jul 30, 2013 at 10:16:04AM +0100, Thomas Petazzoni wrote: > Dear Mark Rutland, > > On Tue, 23 Jul 2013 12:22:59 +0100, Mark Rutland wrote: > > > > +Some Ethernet MACs have a "fixed link", and are not connected to a > > > +normal MDIO-managed PHY device. For those situations, a Device Tree > > > +binding allows to describe a "fixed link". > > > > Are partictular MACs fixed link, or can some either be either fixed link > > or wired to an MDIO-managed PHY? i.e. can we assume a given MAC is > > fixed-link from its compatible string? > > No, you can't. The case that I have is that the mvneta Ethernet MAC > (of Marvell 370/XP SOCs) is sometimes used with a MDIO-managed PHY, and > sometimes used with a switch that isn't manageable at all and should be > considered as a fixed link. > > So no, the compatible string of the Ethernet MAC cannot be used to > determine whether we're wired fixed link or to a classical MDIO-managed > PHY. Ok. Thanks for the info. > > > > +Such a fixed link situation is described within an Ethernet device > > > +Device Tree node using a 'fixed-link' property, composed of 5 > > > +elements: > > > > I'm not sure grouping these values together is the best way of handling > > this. It's rather opaque, and inflexible for future extension. > > That's the DT binding that has been used by PowerPC platforms since > several years, and I've simply re-used it. See 'git grep fixed-link > arch/powerpc/boot/dts'. > > I have nothing against creating another DT binding, but for a start, I > thought using existing bindings would be the best idea. Sorry, I was not aware that was the case. I agree that reusing the existing binding is the right place to start. > > > > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in > > > + the system. > > > > Is there any reason this couldn't be allocated dynamically within the > > kernel as needed? I don't see why an arbitrary unique value should be a > > dt binding requirement; it seems like a leak of Linux implementation > > details. > > As I pointed out in my reply to Grant, this value is used both as the > PHY address on the fake fixed MDIO bus, and as the PHY identifier as > reported by MII registers PHYS_ID1 and PHYS_ID2. If we can get assigned > a proper PHY identifier that is used statically by the driver (it > doesn't have to be different per fixed PHY instance), then we can > allocate the PHY address dynamically. It requires a little bit of API > changes but that's certainly doable. See my reply to Grant for a > proposal about this. Ok. I must admit to not knowing enough about ethernet hardware to fully understand the implications here for the MII registers, but otherwise your proposal makes sense. > > > > + 2. The duplex (1 for full-duplex, 0 for half-duplex) > > > > Will this change for a given MAC? > > > > Could we not have a boolean property for each of these, and require one > > to be present? > > > > Possibly fixed-link-full-duplex / fix-link-half-duplex? > > > > > + 3. The speed (10, 100, 1000) > > > > fixed-link-speed? > > > > > + 4. The pause setting (1 for enabled, 0 for disabled) > > > + 5. The asym pause setting (1 for enabled, 0 for disabled) > > > > Boolean properties for both of these? > > As Florian already answered, he already proposed something like this in > the past, and it was rejected because a fixed PHY is not a piece of > hardware and should therefore not be represented in the Device Tree. > However, the fact that the MAC is not connected to a MDIO-manageable > PHY but to some fixed-link thing is a property of the MAC hardware > layout, and can therefore be expressed as a property of the MAC > hardware. I think it would certainly make sense to describe this as a property of the MAC. Thanks, Mark. > > See the thread that starts at > http://article.gmane.org/gmane.linux.network/275771, and specifically > Grant answers to Florian suggestions of having DT nodes to represent > fixed PHY: http://article.gmane.org/gmane.linux.network/276208. Grant's > answer was: > > """ > I think this discussion is going in the wrong direction. The concept > of a dummy phy is really a Linux kernel internal detail. Creating some > kind of dummy MDIO bus node does not describe the hardware. There is > already support in the kernel for Ethernet MACs connected directly to > a switch or other device. It is far better to describe how the MAC > needs to be configured than to invent a non-existent phy. Search for > "fixed-link" in the kernel tree to see how it is used. > """ > > Best regards, > > Thomas > -- > Thomas Petazzoni, Free Electrons > Kernel, drivers, real-time and embedded Linux > development, consulting, training and support. > http://free-electrons.com > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 30, 2013 at 10:07:05AM +0100, Thomas Petazzoni wrote: > Dear Grant Likely, > > On Tue, 23 Jul 2013 12:39:52 +0100, Grant Likely wrote: > > > > +Such a fixed link situation is described within an Ethernet device > > > +Device Tree node using a 'fixed-link' property, composed of 5 > > > +elements: > > > + > > > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in > > > + the system. > > > > That's just loony! :) Regardless of existing code doing this, it is > > absolutely ridiculous to have it in the driver. The kernel should > > handle generating a phy id transparently. I'd rather mark this field > > as reserved in the binding and change the code to not care about it > > anymore. > > In fact, this value is used for two things: > > * As the PHY address on the fake "fixed" MDIO bus. > > * As the PHY identifier, as reported by the MII registers PHYS_ID1 > (0x2) and PHYS_ID2 (0x3). > > I think this doesn't make sense, because the two things are completely > unrelated. Ideally, we'd like the PHY identifier for fixed PHYs to be > something fixed, identical for all fixed PHYs. The problem is finding > an OUI and device number that is available for that, but maybe we can > ask the OpenMoko people to allocate one (see > http://wiki.openmoko.org/wiki/OUI). > > Then, the PHY address could be generated dynamically. This would > require: > > * Adding a fixed_phy_create() function that internally uses > fixed_phy_add(), but before that creates an unique PHY address for > this newly created PHY. Those unique addresses will be generated by > incrementing a global number of fixed PHYs, up to PHY_MAX_ADDR, > which is the maximum number of fixed PHYs that can anyway be > registered on the fixed MDIO bus. > > fixed_phy_create() would return this PHY address (positive) on > success, or a negative error code on failure. > > * Change of_phy_register_fixed_link() to call fixed_phy_create() > instead of fixed_phy_add() and make it return the PHY address > allocated by fixed_phy_create(). > > * Add a of_phy_connect_fixed_link_direct() that is similar to > of_phy_connect_fixed_link() but takes an additional PHY address as > argument and uses that to generate the 'bus_id' used to find the > phy_device. > > Grant, Mark, Florian, do you have other proposals? The above sounds good to me. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 15, 2013 at 05:34:08PM +0200, Thomas Petazzoni wrote: > Some Ethernet MACs have a "fixed link", and are not connected to a > normal MDIO-managed PHY device. For those situations, a Device Tree > binding allows to describe a "fixed link", as a "fixed-link" property > of the Ethernet device Device Tree node. > > This patch adds: > > * A documentation for the Device Tree property "fixed-link". > > * A of_phy_register_fixed_link() OF helper, which provided an OF node > that contains a "fixed-link" property, registers the corresponding > fixed PHY. > > * Removes the warning on the of_phy_connect_fixed_link() that says > new drivers should not use it, since Grant Likely indicated that > this "fixed-link" property is indeed the way to go. > Any progress with this series? We have more and more boards here with exactly the same problem as Thomas has. For reasons stated below I don't like this binding, but still it would solve my problem. > +Fixed link Device Tree binding > +------------------------------ > + > +Some Ethernet MACs have a "fixed link", and are not connected to a > +normal MDIO-managed PHY device. For those situations, a Device Tree > +binding allows to describe a "fixed link". > + > +Such a fixed link situation is described within an Ethernet device > +Device Tree node using a 'fixed-link' property, composed of 5 > +elements: > + > + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in > + the system. > + 2. The duplex (1 for full-duplex, 0 for half-duplex) > + 3. The speed (10, 100, 1000) > + 4. The pause setting (1 for enabled, 0 for disabled) > + 5. The asym pause setting (1 for enabled, 0 for disabled) > + > +Example: > + > +ethernet@0 { > + ... > + fixed-link = <1 1 1000 0 0>; > + ... > +}; I must say I don't like this binding at all for two reasons. First the positional arguments make it impossible to add optional arguments to the link. Second the other side of the link is most likely a switch. Once this switch has its own node in the devicetree it seems like having a phandle to the switch here would be better. Sascha
Dear Sascha Hauer, On Mon, 12 Aug 2013 08:38:06 +0200, Sascha Hauer wrote: > > This patch adds: > > > > * A documentation for the Device Tree property "fixed-link". > > > > * A of_phy_register_fixed_link() OF helper, which provided an OF node > > that contains a "fixed-link" property, registers the corresponding > > fixed PHY. > > > > * Removes the warning on the of_phy_connect_fixed_link() that says > > new drivers should not use it, since Grant Likely indicated that > > this "fixed-link" property is indeed the way to go. > > > > Any progress with this series? I am not sure there really was a consensus yet on what the DT binding looks like. As soon as there is a consensus, I'm definitely willing to make progress on this series. > We have more and more boards here with exactly the same problem as > Thomas has. For reasons stated below I don't like this binding, but > still it would solve my problem. Ok. > > +Example: > > + > > +ethernet@0 { > > + ... > > + fixed-link = <1 1 1000 0 0>; > > + ... > > +}; > > I must say I don't like this binding at all for two reasons. As I explained, this binding was chosen for this RFC for two reasons: * It's the binding used on PowerPC platforms to represent fixed links. * It allows to encode all the informations into a single property, which avoids the need for a separate DT node for a "fake PHY", which isn't a representation of the hardware. > First the positional arguments make it impossible to add optional > arguments to the link. > > Second the other side of the link is most likely a switch. Once this > switch has its own node in the devicetree it seems like having a phandle > to the switch here would be better. So, in other words, what you're suggesting is something like: ethernet@0 { reg = <...>; interrupt = <...>; phy = <&phy0>; phy0: phy@0 { fixed-link; speed = <1000>; full-duplex; ... }; }; Or something else? Best regards, Thomas
On Mon, Aug 12, 2013 at 10:16:49AM +0200, Thomas Petazzoni wrote: > Dear Sascha Hauer, > > On Mon, 12 Aug 2013 08:38:06 +0200, Sascha Hauer wrote: > > > > This patch adds: > > > > > > * A documentation for the Device Tree property "fixed-link". > > > > > > * A of_phy_register_fixed_link() OF helper, which provided an OF node > > > that contains a "fixed-link" property, registers the corresponding > > > fixed PHY. > > > > > > * Removes the warning on the of_phy_connect_fixed_link() that says > > > new drivers should not use it, since Grant Likely indicated that > > > this "fixed-link" property is indeed the way to go. > > > > > > > Any progress with this series? > > I am not sure there really was a consensus yet on what the DT binding > looks like. As soon as there is a consensus, I'm definitely willing to > make progress on this series. > > > We have more and more boards here with exactly the same problem as > > Thomas has. For reasons stated below I don't like this binding, but > > still it would solve my problem. > > Ok. > > > > +Example: > > > + > > > +ethernet@0 { > > > + ... > > > + fixed-link = <1 1 1000 0 0>; > > > + ... > > > +}; > > > > I must say I don't like this binding at all for two reasons. > > As I explained, this binding was chosen for this RFC for two reasons: > > * It's the binding used on PowerPC platforms to represent fixed links. > * It allows to encode all the informations into a single property, > which avoids the need for a separate DT node for a "fake PHY", which > isn't a representation of the hardware. The fake phy is avoided by making the other side of the link what it really is: An ethernet switch. I'm currently not aware of a situation where a fixed link is needed and the other side is not a switch. And I can't think of a situation in which the other side of the other side of the fixed link really is pure 'virtual', I mean there always must be something connected, right? > > > First the positional arguments make it impossible to add optional > > arguments to the link. > > > > Second the other side of the link is most likely a switch. Once this > > switch has its own node in the devicetree it seems like having a phandle > > to the switch here would be better. > > So, in other words, what you're suggesting is something like: > > ethernet@0 { > reg = <...>; > interrupt = <...>; > phy = <&phy0>; > phy0: phy@0 { > fixed-link; > speed = <1000>; > full-duplex; > ... > }; > }; Yes, this looks good. ePAPR suggests naming the phy property "phy-handle" instead of just "phy", but that's just details. In case the phy really is a switch the phandle could just point to a i2c device instead of the ethernet node. Sascha
-- Christian Gmeiner, MSc 2013/8/12 Sascha Hauer <s.hauer@pengutronix.de>: > On Mon, Aug 12, 2013 at 10:16:49AM +0200, Thomas Petazzoni wrote: >> Dear Sascha Hauer, >> >> On Mon, 12 Aug 2013 08:38:06 +0200, Sascha Hauer wrote: >> >> > > This patch adds: >> > > >> > > * A documentation for the Device Tree property "fixed-link". >> > > >> > > * A of_phy_register_fixed_link() OF helper, which provided an OF node >> > > that contains a "fixed-link" property, registers the corresponding >> > > fixed PHY. >> > > >> > > * Removes the warning on the of_phy_connect_fixed_link() that says >> > > new drivers should not use it, since Grant Likely indicated that >> > > this "fixed-link" property is indeed the way to go. >> > > >> > >> > Any progress with this series? >> >> I am not sure there really was a consensus yet on what the DT binding >> looks like. As soon as there is a consensus, I'm definitely willing to >> make progress on this series. >> >> > We have more and more boards here with exactly the same problem as >> > Thomas has. For reasons stated below I don't like this binding, but >> > still it would solve my problem. >> >> Ok. >> >> > > +Example: >> > > + >> > > +ethernet@0 { >> > > + ... >> > > + fixed-link = <1 1 1000 0 0>; >> > > + ... >> > > +}; >> > >> > I must say I don't like this binding at all for two reasons. >> >> As I explained, this binding was chosen for this RFC for two reasons: >> >> * It's the binding used on PowerPC platforms to represent fixed links. >> * It allows to encode all the informations into a single property, >> which avoids the need for a separate DT node for a "fake PHY", which >> isn't a representation of the hardware. > > The fake phy is avoided by making the other side of the link what it > really is: An ethernet switch. I'm currently not aware of a situation > where a fixed link is needed and the other side is not a switch. And I > can't think of a situation in which the other side of the other side of > the fixed link really is pure 'virtual', I mean there always must be > something connected, right? > >> >> > First the positional arguments make it impossible to add optional >> > arguments to the link. >> > >> > Second the other side of the link is most likely a switch. Once this >> > switch has its own node in the devicetree it seems like having a phandle >> > to the switch here would be better. >> >> So, in other words, what you're suggesting is something like: >> >> ethernet@0 { >> reg = <...>; >> interrupt = <...>; >> phy = <&phy0>; >> phy0: phy@0 { >> fixed-link; >> speed = <1000>; >> full-duplex; >> ... >> }; >> }; > > Yes, this looks good. ePAPR suggests naming the phy property > "phy-handle" instead of just "phy", but that's just details. In case the > phy really is a switch the phandle could just point to a i2c device instead > of the ethernet node. > I have here a I.MX6 based board where I have the same issue that the MAC is directly connected to a switch. The switch has 5 phys (5 port 100mbt switch) but the phy5 is not physically connected to my FEC MAC as my hw guy has chosen to do it without it. In general it is not a bad idea and I got networking working in u-boot quite easily. http://patchwork.ozlabs.org/patch/266558/ Now I am interested in a solution and I think that a fixed-phy is the wrong way. I like the fixed-link solution more as it models the real world better. In my example I do not have any phy, but I have a fixed-link. &fec { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_enet_1>; fixed-link = <0 1 100 0 0>; status = "okay"; }; -- Christian Gmeiner, MSc -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013/8/21 Christian Gmeiner <christian.gmeiner@gmail.com>: > -- > Christian Gmeiner, MSc > > > 2013/8/12 Sascha Hauer <s.hauer@pengutronix.de>: >> On Mon, Aug 12, 2013 at 10:16:49AM +0200, Thomas Petazzoni wrote: >>> Dear Sascha Hauer, >>> >>> On Mon, 12 Aug 2013 08:38:06 +0200, Sascha Hauer wrote: >>> >>> > > This patch adds: >>> > > >>> > > * A documentation for the Device Tree property "fixed-link". >>> > > >>> > > * A of_phy_register_fixed_link() OF helper, which provided an OF node >>> > > that contains a "fixed-link" property, registers the corresponding >>> > > fixed PHY. >>> > > >>> > > * Removes the warning on the of_phy_connect_fixed_link() that says >>> > > new drivers should not use it, since Grant Likely indicated that >>> > > this "fixed-link" property is indeed the way to go. >>> > > >>> > >>> > Any progress with this series? >>> >>> I am not sure there really was a consensus yet on what the DT binding >>> looks like. As soon as there is a consensus, I'm definitely willing to >>> make progress on this series. >>> >>> > We have more and more boards here with exactly the same problem as >>> > Thomas has. For reasons stated below I don't like this binding, but >>> > still it would solve my problem. >>> >>> Ok. >>> >>> > > +Example: >>> > > + >>> > > +ethernet@0 { >>> > > + ... >>> > > + fixed-link = <1 1 1000 0 0>; >>> > > + ... >>> > > +}; >>> > >>> > I must say I don't like this binding at all for two reasons. >>> >>> As I explained, this binding was chosen for this RFC for two reasons: >>> >>> * It's the binding used on PowerPC platforms to represent fixed links. >>> * It allows to encode all the informations into a single property, >>> which avoids the need for a separate DT node for a "fake PHY", which >>> isn't a representation of the hardware. >> >> The fake phy is avoided by making the other side of the link what it >> really is: An ethernet switch. I'm currently not aware of a situation >> where a fixed link is needed and the other side is not a switch. And I >> can't think of a situation in which the other side of the other side of >> the fixed link really is pure 'virtual', I mean there always must be >> something connected, right? >> >>> >>> > First the positional arguments make it impossible to add optional >>> > arguments to the link. >>> > >>> > Second the other side of the link is most likely a switch. Once this >>> > switch has its own node in the devicetree it seems like having a phandle >>> > to the switch here would be better. >>> >>> So, in other words, what you're suggesting is something like: >>> >>> ethernet@0 { >>> reg = <...>; >>> interrupt = <...>; >>> phy = <&phy0>; >>> phy0: phy@0 { >>> fixed-link; >>> speed = <1000>; >>> full-duplex; >>> ... >>> }; >>> }; >> >> Yes, this looks good. ePAPR suggests naming the phy property >> "phy-handle" instead of just "phy", but that's just details. In case the >> phy really is a switch the phandle could just point to a i2c device instead >> of the ethernet node. >> > > I have here a I.MX6 based board where I have the same issue that the > MAC is directly > connected to a switch. The switch has 5 phys (5 port 100mbt switch) > but the phy5 is not > physically connected to my FEC MAC as my hw guy has chosen to do it without it. > In general it is not a bad idea and I got networking working in u-boot > quite easily. This is completely debatable and left to your own use case here. > > http://patchwork.ozlabs.org/patch/266558/ > > Now I am interested in a solution and I think that a fixed-phy is the wrong way. > I like the fixed-link solution more as it models the real world > better. In my example I > do not have any phy, but I have a fixed-link. > > &fec { > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_enet_1>; > fixed-link = <0 1 100 0 0>; > status = "okay"; > }; As was already raised before, this representation has a couple of issues: - not easily extendable - not self-explanatory - it transpires some internal OS concepts such as id/address to the binding On the other side I prefer the other representation for all of the opposite reasons above and it actually represents some kind of a PHY device which is not discoverable via usual means such as MDIO, but which still needs to be known by the Ethernet MAC to work correctly. To move forward, we really need a better argumentation than "I do not like it".
2013/8/12 Sascha Hauer <s.hauer@pengutronix.de>: > On Mon, Aug 12, 2013 at 10:16:49AM +0200, Thomas Petazzoni wrote: >> Dear Sascha Hauer, >> >> On Mon, 12 Aug 2013 08:38:06 +0200, Sascha Hauer wrote: >> >> > > This patch adds: >> > > >> > > * A documentation for the Device Tree property "fixed-link". >> > > >> > > * A of_phy_register_fixed_link() OF helper, which provided an OF node >> > > that contains a "fixed-link" property, registers the corresponding >> > > fixed PHY. >> > > >> > > * Removes the warning on the of_phy_connect_fixed_link() that says >> > > new drivers should not use it, since Grant Likely indicated that >> > > this "fixed-link" property is indeed the way to go. >> > > >> > >> > Any progress with this series? >> >> I am not sure there really was a consensus yet on what the DT binding >> looks like. As soon as there is a consensus, I'm definitely willing to >> make progress on this series. >> >> > We have more and more boards here with exactly the same problem as >> > Thomas has. For reasons stated below I don't like this binding, but >> > still it would solve my problem. >> >> Ok. >> >> > > +Example: >> > > + >> > > +ethernet@0 { >> > > + ... >> > > + fixed-link = <1 1 1000 0 0>; >> > > + ... >> > > +}; >> > >> > I must say I don't like this binding at all for two reasons. >> >> As I explained, this binding was chosen for this RFC for two reasons: >> >> * It's the binding used on PowerPC platforms to represent fixed links. >> * It allows to encode all the informations into a single property, >> which avoids the need for a separate DT node for a "fake PHY", which >> isn't a representation of the hardware. > > The fake phy is avoided by making the other side of the link what it > really is: An ethernet switch. I'm currently not aware of a situation > where a fixed link is needed and the other side is not a switch. There is such hardware out there, some platforms have a MoCA PHY which is responsible for the signaling/control path while the data-path can be connected to a slightly modified Ethernet MAC. > And I > can't think of a situation in which the other side of the other side of > the fixed link really is pure 'virtual', I mean there always must be > something connected, right? I agree, there is something on the other end in every case. > >> >> > First the positional arguments make it impossible to add optional >> > arguments to the link. >> > >> > Second the other side of the link is most likely a switch. Once this >> > switch has its own node in the devicetree it seems like having a phandle >> > to the switch here would be better. >> >> So, in other words, what you're suggesting is something like: >> >> ethernet@0 { >> reg = <...>; >> interrupt = <...>; >> phy = <&phy0>; >> phy0: phy@0 { >> fixed-link; >> speed = <1000>; >> full-duplex; >> ... >> }; >> }; > > Yes, this looks good. ePAPR suggests naming the phy property > "phy-handle" instead of just "phy", but that's just details. In case the > phy really is a switch the phandle could just point to a i2c device instead > of the ethernet node. I do like this representation better than the existing fixed-link property. -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt new file mode 100644 index 0000000..25a009a --- /dev/null +++ b/Documentation/devicetree/bindings/net/fixed-link.txt @@ -0,0 +1,26 @@ +Fixed link Device Tree binding +------------------------------ + +Some Ethernet MACs have a "fixed link", and are not connected to a +normal MDIO-managed PHY device. For those situations, a Device Tree +binding allows to describe a "fixed link". + +Such a fixed link situation is described within an Ethernet device +Device Tree node using a 'fixed-link' property, composed of 5 +elements: + + 1. A fake PHY ID, which must be unique accross all fixed-link PHYs in + the system. + 2. The duplex (1 for full-duplex, 0 for half-duplex) + 3. The speed (10, 100, 1000) + 4. The pause setting (1 for enabled, 0 for disabled) + 5. The asym pause setting (1 for enabled, 0 for disabled) + +Example: + +ethernet@0 { + ... + fixed-link = <1 1 1000 0 0>; + ... +}; + diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index d5a57a9..66d5591 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -14,6 +14,7 @@ #include <linux/netdevice.h> #include <linux/err.h> #include <linux/phy.h> +#include <linux/phy_fixed.h> #include <linux/of.h> #include <linux/of_irq.h> #include <linux/of_mdio.h> @@ -215,10 +216,6 @@ EXPORT_SYMBOL(of_phy_connect); * @dev: pointer to net_device claiming the phy * @hndlr: Link state callback for the network device * @iface: PHY data interface type - * - * This function is a temporary stop-gap and will be removed soon. It is - * only to support the fs_enet, ucc_geth and gianfar Ethernet drivers. Do - * not call this function from new drivers. */ struct phy_device *of_phy_connect_fixed_link(struct net_device *dev, void (*hndlr)(struct net_device *), @@ -247,3 +244,34 @@ struct phy_device *of_phy_connect_fixed_link(struct net_device *dev, return IS_ERR(phy) ? NULL : phy; } EXPORT_SYMBOL(of_phy_connect_fixed_link); + +#if defined(CONFIG_FIXED_PHY) +/** + * of_phy_register_fixed_link - Parse fixed-link property and register a dummy phy + * @np: pointer to the OF device node that contains the "fixed-link" + * property for which a dummy phy should be registered. + */ +#define FIXED_LINK_PROPERTIES_COUNT 5 +int of_phy_register_fixed_link(struct device_node *np) +{ + struct fixed_phy_status status = {}; + u32 fixed_link_props[FIXED_LINK_PROPERTIES_COUNT]; + int ret; + + ret = of_property_read_u32_array(np, "fixed-link", + fixed_link_props, + FIXED_LINK_PROPERTIES_COUNT); + if (ret < 0) + return ret; + + status.link = 1; + status.duplex = fixed_link_props[1]; + status.speed = fixed_link_props[2]; + status.pause = fixed_link_props[3]; + status.asym_pause = fixed_link_props[4]; + + return fixed_phy_add(PHY_POLL, fixed_link_props[0], + &status); +} +EXPORT_SYMBOL(of_phy_register_fixed_link); +#endif diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h index 8163107..bf6efea 100644 --- a/include/linux/of_mdio.h +++ b/include/linux/of_mdio.h @@ -57,4 +57,14 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np) } #endif /* CONFIG_OF */ +#if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY) +extern int of_phy_register_fixed_link(struct device_node *np); +#else +static inline int of_phy_register_fixed_link(struct device_node *np) +{ + return -ENOSYS; +} +#endif + + #endif /* __LINUX_OF_MDIO_H */
Some Ethernet MACs have a "fixed link", and are not connected to a normal MDIO-managed PHY device. For those situations, a Device Tree binding allows to describe a "fixed link", as a "fixed-link" property of the Ethernet device Device Tree node. This patch adds: * A documentation for the Device Tree property "fixed-link". * A of_phy_register_fixed_link() OF helper, which provided an OF node that contains a "fixed-link" property, registers the corresponding fixed PHY. * Removes the warning on the of_phy_connect_fixed_link() that says new drivers should not use it, since Grant Likely indicated that this "fixed-link" property is indeed the way to go. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- .../devicetree/bindings/net/fixed-link.txt | 26 ++++++++++++++++ drivers/of/of_mdio.c | 36 +++++++++++++++++++--- include/linux/of_mdio.h | 10 ++++++ 3 files changed, 68 insertions(+), 4 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt