* [RFC PATCHv2 0/4] Add DT support for fixed PHYs @ 2013-09-06 15:18 Thomas Petazzoni 2013-09-06 15:18 ` [RFC PATCHv2 1/4] net: phy: decouple PHY id and PHY address in fixed PHY driver Thomas Petazzoni ` (5 more replies) 0 siblings, 6 replies; 21+ messages in thread From: Thomas Petazzoni @ 2013-09-06 15:18 UTC (permalink / raw) To: David S. Miller, netdev, devicetree Cc: Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia, linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner Hello, Here is a second version of the patch set that adds a Device Tree binding and the related code to support fixed PHYs. Marked as RFC, this patch set is obviously not intended for merging in 3.12. Since the first version, the changes have been: * Instead of using a 'fixed-link' property inside the Ethernet device DT node, with a fairly cryptic succession of integer values, we now use a PHY subnode under the Ethernet device DT node, with explicit properties to configure the duplex, speed, pause and other PHY properties. * The PHY address is automatically allocated by the kernel and no longer visible in the Device Tree binding. * The PHY device is created directly when the network driver calls of_phy_connect_fixed_link(), and associated to the PHY DT node, which allows the existing of_phy_connect() function to work, without the need to use the deprecated of_phy_connect_fixed_link(). The things I am not entirely happy with yet are: * The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a properly reserved vendor/device identifier, but it isn't clear how to get one allocated for this purpose. * The fixed_phy_register() function in drivers/net/phy/fixed.c has some OF references. So ideally, I would have preferred to put this code in drivers/of/of_mdio.c, but to call get_phy_device(), we need a reference to the mii_bus structure that represents the fixed MDIO bus. * There is some error management missing in fixed_phy_register(), but it can certainly be added easily. This RFC is meant to sort out the general idea. Thanks, Thomas Thomas Petazzoni (4): net: phy: decouple PHY id and PHY address in fixed PHY driver net: phy: extend fixed driver with fixed_phy_register() of: provide a binding for fixed link PHYs net: mvneta: add support for fixed links .../devicetree/bindings/net/fixed-link.txt | 34 ++++++++++++ .../bindings/net/marvell-armada-370-neta.txt | 4 +- drivers/net/ethernet/marvell/mvneta.c | 10 ++-- drivers/net/phy/fixed.c | 63 ++++++++++++++++++---- drivers/of/of_mdio.c | 24 +++++++++ include/linux/of_mdio.h | 15 ++++++ include/linux/phy_fixed.h | 11 ++++ 7 files changed, 145 insertions(+), 16 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt -- 1.8.1.2 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCHv2 1/4] net: phy: decouple PHY id and PHY address in fixed PHY driver 2013-09-06 15:18 [RFC PATCHv2 0/4] Add DT support for fixed PHYs Thomas Petazzoni @ 2013-09-06 15:18 ` Thomas Petazzoni 2013-09-06 15:18 ` [RFC PATCHv2 2/4] net: phy: extend fixed driver with fixed_phy_register() Thomas Petazzoni ` (4 subsequent siblings) 5 siblings, 0 replies; 21+ messages in thread From: Thomas Petazzoni @ 2013-09-06 15:18 UTC (permalink / raw) To: David S. Miller, netdev, devicetree Cc: Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia, linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner Until now, the fixed_phy_add() function was taking as argument 'phy_id', which was used both as the PHY address on the fake fixed MDIO bus, and as the PHY id, as available in the MII_PHYSID1 and MII_PHYSID2 registers. However, those two informations are completely unrelated. This patch decouples them. The PHY id of fixed PHYs is hardcoded to be 0xdeadbeef. Ideally, a really reserved value would be nicer, but there doesn't seem to be an easy of making sure a dummy value can be assigned to the Linux kernel for such usage. The PHY address remains passed by the caller of phy_fixed_add(). Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- drivers/net/phy/fixed.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c index ba55adf..0f02403 100644 --- a/drivers/net/phy/fixed.c +++ b/drivers/net/phy/fixed.c @@ -31,7 +31,7 @@ struct fixed_mdio_bus { }; struct fixed_phy { - int id; + int addr; u16 regs[MII_REGS_NUM]; struct phy_device *phydev; struct fixed_phy_status status; @@ -104,8 +104,8 @@ static int fixed_phy_update_regs(struct fixed_phy *fp) if (fp->status.asym_pause) lpa |= LPA_PAUSE_ASYM; - fp->regs[MII_PHYSID1] = fp->id >> 16; - fp->regs[MII_PHYSID2] = fp->id; + fp->regs[MII_PHYSID1] = 0xdead; + fp->regs[MII_PHYSID2] = 0xbeef; fp->regs[MII_BMSR] = bmsr; fp->regs[MII_BMCR] = bmcr; @@ -115,7 +115,7 @@ static int fixed_phy_update_regs(struct fixed_phy *fp) return 0; } -static int fixed_mdio_read(struct mii_bus *bus, int phy_id, int reg_num) +static int fixed_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num) { struct fixed_mdio_bus *fmb = bus->priv; struct fixed_phy *fp; @@ -124,7 +124,7 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_id, int reg_num) return -1; list_for_each_entry(fp, &fmb->phys, node) { - if (fp->id == phy_id) { + if (fp->addr == phy_addr) { /* Issue callback if user registered it. */ if (fp->link_update) { fp->link_update(fp->phydev->attached_dev, @@ -138,7 +138,7 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_id, int reg_num) return 0xFFFF; } -static int fixed_mdio_write(struct mii_bus *bus, int phy_id, int reg_num, +static int fixed_mdio_write(struct mii_bus *bus, int phy_addr, int reg_num, u16 val) { return 0; @@ -160,7 +160,7 @@ int fixed_phy_set_link_update(struct phy_device *phydev, return -EINVAL; list_for_each_entry(fp, &fmb->phys, node) { - if (fp->id == phydev->phy_id) { + if (fp->addr == phydev->addr) { fp->link_update = link_update; fp->phydev = phydev; return 0; @@ -171,7 +171,7 @@ int fixed_phy_set_link_update(struct phy_device *phydev, } EXPORT_SYMBOL_GPL(fixed_phy_set_link_update); -int fixed_phy_add(unsigned int irq, int phy_id, +int fixed_phy_add(unsigned int irq, int phy_addr, struct fixed_phy_status *status) { int ret; @@ -184,9 +184,9 @@ int fixed_phy_add(unsigned int irq, int phy_id, memset(fp->regs, 0xFF, sizeof(fp->regs[0]) * MII_REGS_NUM); - fmb->irqs[phy_id] = irq; + fmb->irqs[phy_addr] = irq; - fp->id = phy_id; + fp->addr = phy_addr; fp->status = *status; ret = fixed_phy_update_regs(fp); -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [RFC PATCHv2 2/4] net: phy: extend fixed driver with fixed_phy_register() 2013-09-06 15:18 [RFC PATCHv2 0/4] Add DT support for fixed PHYs Thomas Petazzoni 2013-09-06 15:18 ` [RFC PATCHv2 1/4] net: phy: decouple PHY id and PHY address in fixed PHY driver Thomas Petazzoni @ 2013-09-06 15:18 ` Thomas Petazzoni 2013-09-06 15:18 ` [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs Thomas Petazzoni ` (3 subsequent siblings) 5 siblings, 0 replies; 21+ messages in thread From: Thomas Petazzoni @ 2013-09-06 15:18 UTC (permalink / raw) To: David S. Miller, netdev, devicetree Cc: Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia, linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner The existing fixed_phy_add() function has several drawbacks that prevents it from being used as is for OF-based declaration of fixed PHYs: * The address of the PHY on the fake bus needs to be passed, while a dynamic allocation is desired. * Since the phy_device instantiation is post-poned until the next mdiobus scan, there is no way to associate the fixed PHY with its OF node, which later prevents of_phy_connect() from finding this fixed PHY from a given OF node. To solve this, this commit introduces fixed_phy_register(), which will allocate an available PHY address, add the PHY using fixed_phy_add() and instantiate the phy_device structure associated with the provided OF node. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- drivers/net/phy/fixed.c | 43 +++++++++++++++++++++++++++++++++++++++++++ include/linux/phy_fixed.h | 11 +++++++++++ 2 files changed, 54 insertions(+) diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c index 0f02403..3f9412b 100644 --- a/drivers/net/phy/fixed.c +++ b/drivers/net/phy/fixed.c @@ -21,6 +21,7 @@ #include <linux/phy_fixed.h> #include <linux/err.h> #include <linux/slab.h> +#include <linux/of.h> #define MII_REGS_NUM 29 @@ -203,6 +204,48 @@ err_regs: } EXPORT_SYMBOL_GPL(fixed_phy_add); +static int phy_fixed_addr; +static DEFINE_SPINLOCK(phy_fixed_addr_lock); + +int fixed_phy_register(unsigned int irq, + struct fixed_phy_status *status, + struct device_node *np) +{ + struct fixed_mdio_bus *fmb = &platform_fmb; + struct phy_device *phy; + int phy_addr; + int ret; + + /* Get the next available PHY address, up to PHY_MAX_ADDR */ + spin_lock(&phy_fixed_addr_lock); + if (phy_fixed_addr == PHY_MAX_ADDR) { + spin_unlock(&phy_fixed_addr_lock); + return -ENOSPC; + } + phy_addr = phy_fixed_addr++; + spin_unlock(&phy_fixed_addr_lock); + + ret = fixed_phy_add(PHY_POLL, phy_addr, status); + if (ret < 0) + return ret; + + phy = get_phy_device(fmb->mii_bus, phy_addr, false); + if (!phy || IS_ERR(phy)) + return -EINVAL; + + of_node_get(np); + phy->dev.of_node = np; + + ret = phy_device_register(phy); + if (ret) { + phy_device_free(phy); + of_node_put(np); + return ret; + } + + return 0; +} + static int __init fixed_mdio_bus_init(void) { struct fixed_mdio_bus *fmb = &platform_fmb; diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h index 509d8f5..902e8a1 100644 --- a/include/linux/phy_fixed.h +++ b/include/linux/phy_fixed.h @@ -9,15 +9,26 @@ struct fixed_phy_status { int asym_pause; }; +struct device_node; + #ifdef CONFIG_FIXED_PHY extern int fixed_phy_add(unsigned int irq, int phy_id, struct fixed_phy_status *status); +extern int fixed_phy_register(unsigned int irq, + struct fixed_phy_status *status, + struct device_node *np); #else static inline int fixed_phy_add(unsigned int irq, int phy_id, struct fixed_phy_status *status) { return -ENODEV; } +extern int fixed_phy_register(unsigned int irq, + struct fixed_phy_status *status, + struct device_node *np) +{ + return -ENODEV; +} #endif /* CONFIG_FIXED_PHY */ /* -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs 2013-09-06 15:18 [RFC PATCHv2 0/4] Add DT support for fixed PHYs Thomas Petazzoni 2013-09-06 15:18 ` [RFC PATCHv2 1/4] net: phy: decouple PHY id and PHY address in fixed PHY driver Thomas Petazzoni 2013-09-06 15:18 ` [RFC PATCHv2 2/4] net: phy: extend fixed driver with fixed_phy_register() Thomas Petazzoni @ 2013-09-06 15:18 ` Thomas Petazzoni [not found] ` <1378480701-12908-4-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2013-11-12 1:43 ` Florian Fainelli 2013-09-06 15:18 ` [RFC PATCHv2 4/4] net: mvneta: add support for fixed links Thomas Petazzoni ` (2 subsequent siblings) 5 siblings, 2 replies; 21+ messages in thread From: Thomas Petazzoni @ 2013-09-06 15:18 UTC (permalink / raw) To: David S. Miller, netdev, devicetree Cc: Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia, linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner 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" using a special PHY node. This patch adds: * A documentation for the fixed PHY Device Tree binding. * An of_phy_is_fixed_link() function that an Ethernet driver can call on its PHY phandle to find out whether it's a fixed link PHY or not. It should typically be used to know if of_phy_register_fixed_link() should be called. * An of_phy_register_fixed_link() function that instantiates the fixed PHY into the PHY subsystem, so that when the driver calls of_phy_connect(), the PHY device associated to the OF node will be found. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- .../devicetree/bindings/net/fixed-link.txt | 34 ++++++++++++++++++++++ drivers/of/of_mdio.c | 24 +++++++++++++++ include/linux/of_mdio.h | 15 ++++++++++ 3 files changed, 73 insertions(+) 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..9f2a1a50 --- /dev/null +++ b/Documentation/devicetree/bindings/net/fixed-link.txt @@ -0,0 +1,34 @@ +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 by creating a PHY node as a +sub-node of an Ethernet device, with the following properties: + +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a + fixed link PHY. +* 'speed' (integer, mandatory), to indicate the link speed. Accepted + values are 10, 100 and 1000 +* 'full-duplex' (boolean, optional), to indicate that full duplex is + used. When absent, half duplex is assumed. +* 'pause' (boolean, optional), to indicate that pause should be + enabled. +* 'asym-pause' (boolean, optional), to indicate that asym_pause should + be enabled. + +Example: + +ethernet@0 { + ... + phy = <&phy0>; + phy0: phy@0 { + fixed-link; + speed = <1000>; + full-duplex; + }; + ... +}; + diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index d5a57a9..0507f8f 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> @@ -247,3 +248,26 @@ 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) +bool of_phy_is_fixed_link(struct device_node *np) +{ + return of_property_read_bool(np, "fixed-link"); +} +EXPORT_SYMBOL(of_phy_is_fixed_link); + +int of_phy_register_fixed_link(struct device_node *np) +{ + struct fixed_phy_status status = {}; + + status.link = 1; + status.duplex = of_property_read_bool(np, "full-duplex"); + if (of_property_read_u32(np, "speed", &status.speed)) + return -EINVAL; + status.pause = of_property_read_bool(np, "pause"); + status.asym_pause = of_property_read_bool(np, "asym-pause"); + + return fixed_phy_register(PHY_POLL, &status, np); +} +EXPORT_SYMBOL(of_phy_register_fixed_link); +#endif diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h index 8163107..2f535ee 100644 --- a/include/linux/of_mdio.h +++ b/include/linux/of_mdio.h @@ -57,4 +57,19 @@ 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); +extern bool of_phy_is_fixed_link(struct device_node *np); +#else +static inline int of_phy_register_fixed_link(struct device_node *np) +{ + return -ENOSYS; +} +static inline bool of_phy_is_fixed_link(struct device_node *np) +{ + return false; +} +#endif + + #endif /* __LINUX_OF_MDIO_H */ -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
[parent not found: <1378480701-12908-4-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs [not found] ` <1378480701-12908-4-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2013-09-18 4:29 ` Grant Likely [not found] ` <20130918042923.5D845C42CF7-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org> 0 siblings, 1 reply; 21+ messages in thread From: Grant Likely @ 2013-09-18 4:29 UTC (permalink / raw) To: Thomas Petazzoni, David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA Cc: Lior Amsalem, Mark Rutland, Sascha Hauer, Christian Gmeiner, Ezequiel Garcia, Gregory Clement, Florian Fainelli, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r On Fri, 6 Sep 2013 17:18:20 +0200, Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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" using a special PHY node. > > This patch adds: > > * A documentation for the fixed PHY Device Tree binding. > > * An of_phy_is_fixed_link() function that an Ethernet driver can call > on its PHY phandle to find out whether it's a fixed link PHY or > not. It should typically be used to know if > of_phy_register_fixed_link() should be called. > > * An of_phy_register_fixed_link() function that instantiates the > fixed PHY into the PHY subsystem, so that when the driver calls > of_phy_connect(), the PHY device associated to the OF node will be > found. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> Hi Thomas, The implemenation in this series looks like it is in good shape, so I'll restrict my comments to be binding... > --- > .../devicetree/bindings/net/fixed-link.txt | 34 ++++++++++++++++++++++ > drivers/of/of_mdio.c | 24 +++++++++++++++ > include/linux/of_mdio.h | 15 ++++++++++ > 3 files changed, 73 insertions(+) > 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..9f2a1a50 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/fixed-link.txt > @@ -0,0 +1,34 @@ > +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 by creating a PHY node as a > +sub-node of an Ethernet device, with the following properties: > + > +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a > + fixed link PHY. > +* 'speed' (integer, mandatory), to indicate the link speed. Accepted > + values are 10, 100 and 1000 > +* 'full-duplex' (boolean, optional), to indicate that full duplex is > + used. When absent, half duplex is assumed. > +* 'pause' (boolean, optional), to indicate that pause should be > + enabled. > +* 'asym-pause' (boolean, optional), to indicate that asym_pause should > + be enabled. I understand what you're trying to do here, but it causes a troublesome leakage of implementation detail into the binding, making the whole thing look very odd. This binding tries to make a fixed link look exactly like a real PHY even to the point of including a phandle to the phy. But having a phandle to a node which is *always* a direct child of the MAC node is redundant and a rather looney. Yes, doing it that way makes it easy for of_phy_find_device() to be transparent for fixed link, but that should *not* drive bindings, especially when that makes the binding really rather weird. Second, this new binding doesn't provide anything over and above the existing fixed-link binding. It may not be pretty, but it is estabilshed. That said, I do agree that the current Linux implementation is not good because it cannot handle a fixed-link property transparently. That's a deficiency in the Linux implementation and it should be fixed. of_phy_connect() currently requires the phy phandle to be passed in. Part of the reason it was done this way is that some drivers connect to multiple 'phys'. A soulition could be to make the phy handle optional. If it is empty then go looking for either a phy-device or fixed-link property. Otherwise use the provided node. g. > + > +Example: > + > +ethernet@0 { > + ... > + phy = <&phy0>; > + phy0: phy@0 { > + fixed-link; > + speed = <1000>; > + full-duplex; > + }; > + ... > +}; > + > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index d5a57a9..0507f8f 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> > @@ -247,3 +248,26 @@ 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) > +bool of_phy_is_fixed_link(struct device_node *np) > +{ > + return of_property_read_bool(np, "fixed-link"); > +} > +EXPORT_SYMBOL(of_phy_is_fixed_link); > + > +int of_phy_register_fixed_link(struct device_node *np) > +{ > + struct fixed_phy_status status = {}; > + > + status.link = 1; > + status.duplex = of_property_read_bool(np, "full-duplex"); > + if (of_property_read_u32(np, "speed", &status.speed)) > + return -EINVAL; > + status.pause = of_property_read_bool(np, "pause"); > + status.asym_pause = of_property_read_bool(np, "asym-pause"); > + > + return fixed_phy_register(PHY_POLL, &status, np); > +} > +EXPORT_SYMBOL(of_phy_register_fixed_link); > +#endif > diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h > index 8163107..2f535ee 100644 > --- a/include/linux/of_mdio.h > +++ b/include/linux/of_mdio.h > @@ -57,4 +57,19 @@ 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); > +extern bool of_phy_is_fixed_link(struct device_node *np); > +#else > +static inline int of_phy_register_fixed_link(struct device_node *np) > +{ > + return -ENOSYS; > +} > +static inline bool of_phy_is_fixed_link(struct device_node *np) > +{ > + return false; > +} > +#endif > + > + > #endif /* __LINUX_OF_MDIO_H */ > -- > 1.8.1.2 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <20130918042923.5D845C42CF7-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>]
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs [not found] ` <20130918042923.5D845C42CF7-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org> @ 2013-09-18 9:21 ` Florian Fainelli [not found] ` <CAGVrzcbVTenhVC4tQznJFqVpO08ruxLyy1ZiLmw6Bu1=3zbGZg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2013-09-18 16:11 ` Thomas Petazzoni 1 sibling, 1 reply; 21+ messages in thread From: Florian Fainelli @ 2013-09-18 9:21 UTC (permalink / raw) To: Grant Likely Cc: Thomas Petazzoni, David S. Miller, netdev, devicetree-u79uwXL29TY76Z2rM5mHXA, Lior Amsalem, Mark Rutland, Sascha Hauer, Christian Gmeiner, Ezequiel Garcia, Gregory Clement, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Hello, 2013/9/18 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>: > On Fri, 6 Sep 2013 17:18:20 +0200, Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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" using a special PHY node. >> >> This patch adds: >> >> * A documentation for the fixed PHY Device Tree binding. >> >> * An of_phy_is_fixed_link() function that an Ethernet driver can call >> on its PHY phandle to find out whether it's a fixed link PHY or >> not. It should typically be used to know if >> of_phy_register_fixed_link() should be called. >> >> * An of_phy_register_fixed_link() function that instantiates the >> fixed PHY into the PHY subsystem, so that when the driver calls >> of_phy_connect(), the PHY device associated to the OF node will be >> found. >> >> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> > > Hi Thomas, > > The implemenation in this series looks like it is in good shape, so I'll > restrict my comments to be binding... > >> --- >> .../devicetree/bindings/net/fixed-link.txt | 34 ++++++++++++++++++++++ >> drivers/of/of_mdio.c | 24 +++++++++++++++ >> include/linux/of_mdio.h | 15 ++++++++++ >> 3 files changed, 73 insertions(+) >> 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..9f2a1a50 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt >> @@ -0,0 +1,34 @@ >> +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 by creating a PHY node as a >> +sub-node of an Ethernet device, with the following properties: >> + >> +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a >> + fixed link PHY. >> +* 'speed' (integer, mandatory), to indicate the link speed. Accepted >> + values are 10, 100 and 1000 >> +* 'full-duplex' (boolean, optional), to indicate that full duplex is >> + used. When absent, half duplex is assumed. >> +* 'pause' (boolean, optional), to indicate that pause should be >> + enabled. >> +* 'asym-pause' (boolean, optional), to indicate that asym_pause should >> + be enabled. > > I understand what you're trying to do here, but it causes a troublesome > leakage of implementation detail into the binding, making the whole > thing look very odd. This binding tries to make a fixed link look > exactly like a real PHY even to the point of including a phandle to the > phy. But having a phandle to a node which is *always* a direct child of > the MAC node is redundant and a rather looney. Yes, doing it that way > makes it easy for of_phy_find_device() to be transparent for fixed link, > but that should *not* drive bindings, especially when that makes the > binding really rather weird. This is not exactly true in the sense that the "new" binding just re-shuffles the properties representation into something that is clearer and more extendible but there is not much difference in the semantics. > > Second, this new binding doesn't provide anything over and above the > existing fixed-link binding. It may not be pretty, but it is > estabilshed. In fact it does, the old one is obscure and not easily extendable because we rely on an integer array to represent the various properties, so at least this new one makes it easy to extend the binding to support a possibly new fixed-link property. Being able to deprecate a fundamentaly badly designed binding should still be a prerogative, software is flexible and can deal with both with little cost. > > That said, I do agree that the current Linux implementation is not good > because it cannot handle a fixed-link property transparently. That's a > deficiency in the Linux implementation and it should be fixed. > of_phy_connect() currently requires the phy phandle to be passed in. > Part of the reason it was done this way is that some drivers connect to > multiple 'phys'. A soulition could be to make the phy handle optional. > If it is empty then go looking for either a phy-device or fixed-link > property. Otherwise use the provided node. I do not quite follow you on this one, and I fear we might be leaking some Linux probing heuristic into Device Tree bindings by implicitely saying "not including a PHY phandle means connecting to a fixed-PHY link." This would also dramatically change the current behavior for most drivers where they might refuse probing if no corresponding PHY device node is present and they are not designed to connect to a fixed PHY one. -- Florian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <CAGVrzcbVTenhVC4tQznJFqVpO08ruxLyy1ZiLmw6Bu1=3zbGZg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs [not found] ` <CAGVrzcbVTenhVC4tQznJFqVpO08ruxLyy1ZiLmw6Bu1=3zbGZg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2013-09-18 15:00 ` Grant Likely [not found] ` <20130918150031.D9034C42CDF-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org> 0 siblings, 1 reply; 21+ messages in thread From: Grant Likely @ 2013-09-18 15:00 UTC (permalink / raw) To: Florian Fainelli Cc: Thomas Petazzoni, David S. Miller, netdev, devicetree-u79uwXL29TY76Z2rM5mHXA, Lior Amsalem, Mark Rutland, Sascha Hauer, Christian Gmeiner, Ezequiel Garcia, Gregory Clement, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r On Wed, 18 Sep 2013 10:21:11 +0100, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > Hello, > > 2013/9/18 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>: > > On Fri, 6 Sep 2013 17:18:20 +0200, Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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" using a special PHY node. > >> > >> This patch adds: > >> > >> * A documentation for the fixed PHY Device Tree binding. > >> > >> * An of_phy_is_fixed_link() function that an Ethernet driver can call > >> on its PHY phandle to find out whether it's a fixed link PHY or > >> not. It should typically be used to know if > >> of_phy_register_fixed_link() should be called. > >> > >> * An of_phy_register_fixed_link() function that instantiates the > >> fixed PHY into the PHY subsystem, so that when the driver calls > >> of_phy_connect(), the PHY device associated to the OF node will be > >> found. > >> > >> Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> > > > > Hi Thomas, > > > > The implemenation in this series looks like it is in good shape, so I'll > > restrict my comments to be binding... > > > >> --- > >> .../devicetree/bindings/net/fixed-link.txt | 34 ++++++++++++++++++++++ > >> drivers/of/of_mdio.c | 24 +++++++++++++++ > >> include/linux/of_mdio.h | 15 ++++++++++ > >> 3 files changed, 73 insertions(+) > >> 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..9f2a1a50 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt > >> @@ -0,0 +1,34 @@ > >> +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 by creating a PHY node as a > >> +sub-node of an Ethernet device, with the following properties: > >> + > >> +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a > >> + fixed link PHY. > >> +* 'speed' (integer, mandatory), to indicate the link speed. Accepted > >> + values are 10, 100 and 1000 > >> +* 'full-duplex' (boolean, optional), to indicate that full duplex is > >> + used. When absent, half duplex is assumed. > >> +* 'pause' (boolean, optional), to indicate that pause should be > >> + enabled. > >> +* 'asym-pause' (boolean, optional), to indicate that asym_pause should > >> + be enabled. > > > > I understand what you're trying to do here, but it causes a troublesome > > leakage of implementation detail into the binding, making the whole > > thing look very odd. This binding tries to make a fixed link look > > exactly like a real PHY even to the point of including a phandle to the > > phy. But having a phandle to a node which is *always* a direct child of > > the MAC node is redundant and a rather looney. Yes, doing it that way > > makes it easy for of_phy_find_device() to be transparent for fixed link, > > but that should *not* drive bindings, especially when that makes the > > binding really rather weird. > > This is not exactly true in the sense that the "new" binding just > re-shuffles the properties representation into something that is > clearer and more extendible but there is not much difference in the > semantics. That's not my point in the above paragraph. My point is a binding that consists of a phandle to a node that is always a direct child is goofy and wrong. > > Second, this new binding doesn't provide anything over and above the > > existing fixed-link binding. It may not be pretty, but it is > > estabilshed. > > In fact it does, the old one is obscure and not easily extendable > because we rely on an integer array to represent the various > properties, so at least this new one makes it easy to extend the > binding to support a possibly new fixed-link property. Being able to > deprecate a fundamentaly badly designed binding should still be a > prerogative, software is flexible and can deal with both with little > cost. I understand that, but consistency is also important. I don't see a proposal for a new feature for the binding in this patch. Without a really compelling reason to change a binding that works (even if it is opaque) I cannot agree to changing it. > > That said, I do agree that the current Linux implementation is not good > > because it cannot handle a fixed-link property transparently. That's a > > deficiency in the Linux implementation and it should be fixed. > > of_phy_connect() currently requires the phy phandle to be passed in. > > Part of the reason it was done this way is that some drivers connect to > > multiple 'phys'. A soulition could be to make the phy handle optional. > > If it is empty then go looking for either a phy-device or fixed-link > > property. Otherwise use the provided node. > > I do not quite follow you on this one, and I fear we might be leaking > some Linux probing heuristic into Device Tree bindings by implicitely > saying "not including a PHY phandle means connecting to a fixed-PHY > link." This would also dramatically change the current behavior for > most drivers where they might refuse probing if no corresponding PHY > device node is present and they are not designed to connect to a fixed > PHY one. Drivers we can change and fix. There aren't very may call sites affected so I'm not overly worried about it. Also, I was making a suggestion on how to fix it, but a suggestion is not a fully formed patch. The issue you raise would of course need to be addressed. g. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <20130918150031.D9034C42CDF-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>]
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs [not found] ` <20130918150031.D9034C42CDF-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org> @ 2013-09-19 6:36 ` Sascha Hauer 0 siblings, 0 replies; 21+ messages in thread From: Sascha Hauer @ 2013-09-19 6:36 UTC (permalink / raw) To: Grant Likely Cc: Florian Fainelli, Thomas Petazzoni, David S. Miller, netdev, devicetree-u79uwXL29TY76Z2rM5mHXA, Lior Amsalem, Mark Rutland, Christian Gmeiner, Ezequiel Garcia, Gregory Clement, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r On Wed, Sep 18, 2013 at 10:00:31AM -0500, Grant Likely wrote: > On Wed, 18 Sep 2013 10:21:11 +0100, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > > > > > > I understand what you're trying to do here, but it causes a troublesome > > > leakage of implementation detail into the binding, making the whole > > > thing look very odd. This binding tries to make a fixed link look > > > exactly like a real PHY even to the point of including a phandle to the > > > phy. But having a phandle to a node which is *always* a direct child of > > > the MAC node is redundant and a rather looney. Yes, doing it that way > > > makes it easy for of_phy_find_device() to be transparent for fixed link, > > > but that should *not* drive bindings, especially when that makes the > > > binding really rather weird. > > > > This is not exactly true in the sense that the "new" binding just > > re-shuffles the properties representation into something that is > > clearer and more extendible but there is not much difference in the > > semantics. > > That's not my point in the above paragraph. My point is a binding that > consists of a phandle to a node that is always a direct child is goofy > and wrong. It's not necessarily a direct child. Most of these fixed links are really ethernet switches. These are (mostly) i2c devices which are under their corresponding i2c bus node. Using a phandle from the ethernet MAC to the port of a switch not only provides the link information, but also the information to which port of the switch the MAC is connected. Another situation is that some SoCs have a MDIO bus external to the MAC and possibly shared for multiple ethernet MACs. This also requires a phandle from the MAC to the MDIO bus. So we have the situation that we need a phandle from the MAC to something that provides a link. For consistency it would be good to always use a phandle instead of having an inflexible 'fixed-link' property. You're right, the binding doesn't provide anything new, but I think it straightens things up for the future. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs [not found] ` <20130918042923.5D845C42CF7-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org> 2013-09-18 9:21 ` Florian Fainelli @ 2013-09-18 16:11 ` Thomas Petazzoni 2013-10-25 4:40 ` Florian Fainelli 1 sibling, 1 reply; 21+ messages in thread From: Thomas Petazzoni @ 2013-09-18 16:11 UTC (permalink / raw) To: Grant Likely Cc: David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, Lior Amsalem, Mark Rutland, Sascha Hauer, Christian Gmeiner, Ezequiel Garcia, Gregory Clement, Florian Fainelli, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Dear Grant Likely, On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote: > I understand what you're trying to do here, but it causes a troublesome > leakage of implementation detail into the binding, making the whole > thing look very odd. This binding tries to make a fixed link look > exactly like a real PHY even to the point of including a phandle to the > phy. But having a phandle to a node which is *always* a direct child of > the MAC node is redundant and a rather looney. Yes, doing it that way > makes it easy for of_phy_find_device() to be transparent for fixed link, > but that should *not* drive bindings, especially when that makes the > binding really rather weird. > > Second, this new binding doesn't provide anything over and above the > existing fixed-link binding. It may not be pretty, but it is > estabilshed. Have you followed the past discussions about this patch set? Basically the *only* feedback I received on RFCv1 is that the fixed-link property sucks, and everybody (including the known Device Tree binding maintainer Mark Rutland) suggested to not use the fixed-link mechanism. See http://article.gmane.org/gmane.linux.network/276932, where Mark said: "" I'm not sure grouping these values together is the best way of handling this. It's rather opaque, and inflexible for future extension. "" So, please DT maintainers, tell me what you want. I honestly don't care whether fixed-link or a separate node is chosen. However, I do care about being dragged around between two solutions just because the former DT maintainer and the new DT maintainers do not agree. Thanks, 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 devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs 2013-09-18 16:11 ` Thomas Petazzoni @ 2013-10-25 4:40 ` Florian Fainelli 2013-11-12 12:37 ` Grant Likely 0 siblings, 1 reply; 21+ messages in thread From: Florian Fainelli @ 2013-10-25 4:40 UTC (permalink / raw) To: Thomas Petazzoni Cc: Grant Likely, David S. Miller, netdev, devicetree, Lior Amsalem, Mark Rutland, Sascha Hauer, Christian Gmeiner, Ezequiel Garcia, Gregory Clement, linux-arm-kernel Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit : > Dear Grant Likely, > > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote: > > I understand what you're trying to do here, but it causes a troublesome > > leakage of implementation detail into the binding, making the whole > > thing look very odd. This binding tries to make a fixed link look > > exactly like a real PHY even to the point of including a phandle to the > > phy. But having a phandle to a node which is *always* a direct child of > > the MAC node is redundant and a rather looney. Yes, doing it that way > > makes it easy for of_phy_find_device() to be transparent for fixed link, > > but that should *not* drive bindings, especially when that makes the > > binding really rather weird. > > > > Second, this new binding doesn't provide anything over and above the > > existing fixed-link binding. It may not be pretty, but it is > > estabilshed. > > Have you followed the past discussions about this patch set? Basically > the *only* feedback I received on RFCv1 is that the fixed-link property > sucks, and everybody (including the known Device Tree binding > maintainer Mark Rutland) suggested to not use the fixed-link mechanism. > See http://article.gmane.org/gmane.linux.network/276932, where Mark > said: > > "" > I'm not sure grouping these values together is the best way of handling > this. It's rather opaque, and inflexible for future extension. > "" > > So, please DT maintainers, tell me what you want. I honestly don't care > whether fixed-link or a separate node is chosen. However, I do care > about being dragged around between two solutions just because the > former DT maintainer and the new DT maintainers do not agree. Since I would like to move forward so I can one day use that binding in a real-life product, I am going to go for Mark's side which happens to be how I want the binding to look like. Do we all agree that the new binding is just way better than the old one? In light of the recent unstable DT ABI discussion, we can still continue parsing the old one for the sake of compatibility. -- Florian ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs 2013-10-25 4:40 ` Florian Fainelli @ 2013-11-12 12:37 ` Grant Likely 2013-11-12 16:29 ` Mark Rutland 0 siblings, 1 reply; 21+ messages in thread From: Grant Likely @ 2013-11-12 12:37 UTC (permalink / raw) To: Florian Fainelli, Thomas Petazzoni Cc: David S. Miller, netdev, devicetree, Lior Amsalem, Mark Rutland, Sascha Hauer, Christian Gmeiner, Ezequiel Garcia, Gregory Clement, linux-arm-kernel On Fri, 25 Oct 2013 05:40:57 +0100, Florian Fainelli <florian@openwrt.org> wrote: > Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit : > > Dear Grant Likely, > > > > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote: > > > I understand what you're trying to do here, but it causes a troublesome > > > leakage of implementation detail into the binding, making the whole > > > thing look very odd. This binding tries to make a fixed link look > > > exactly like a real PHY even to the point of including a phandle to the > > > phy. But having a phandle to a node which is *always* a direct child of > > > the MAC node is redundant and a rather looney. Yes, doing it that way > > > makes it easy for of_phy_find_device() to be transparent for fixed link, > > > but that should *not* drive bindings, especially when that makes the > > > binding really rather weird. > > > > > > Second, this new binding doesn't provide anything over and above the > > > existing fixed-link binding. It may not be pretty, but it is > > > estabilshed. > > > > Have you followed the past discussions about this patch set? Basically > > the *only* feedback I received on RFCv1 is that the fixed-link property > > sucks, and everybody (including the known Device Tree binding > > maintainer Mark Rutland) suggested to not use the fixed-link mechanism. > > See http://article.gmane.org/gmane.linux.network/276932, where Mark > > said: > > > > "" > > I'm not sure grouping these values together is the best way of handling > > this. It's rather opaque, and inflexible for future extension. > > "" > > > > So, please DT maintainers, tell me what you want. I honestly don't care > > whether fixed-link or a separate node is chosen. However, I do care > > about being dragged around between two solutions just because the > > former DT maintainer and the new DT maintainers do not agree. I've been sleepy on this issue, which limits how much I can push. I'll say one more thing on the issue (below) and then leave the decision up to Mark. I trust him and he knows what he is doing. > Since I would like to move forward so I can one day use that binding in a > real-life product, I am going to go for Mark's side which happens to be how I > want the binding to look like. > > Do we all agree that the new binding is just way better than the old one? In > light of the recent unstable DT ABI discussion, we can still continue parsing > the old one for the sake of compatibility. Regardless of what you want it to look like, does the old binding work for your purposes? If yes then use it. The only valid reason for creating a new binding is if the old one doesn't work for a specific (not theoretical) use case. g. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs 2013-11-12 12:37 ` Grant Likely @ 2013-11-12 16:29 ` Mark Rutland 2013-11-12 17:40 ` Florian Fainelli 0 siblings, 1 reply; 21+ messages in thread From: Mark Rutland @ 2013-11-12 16:29 UTC (permalink / raw) To: Grant Likely, Florian Fainelli Cc: Thomas Petazzoni, David S. Miller, netdev, devicetree, Lior Amsalem, Sascha Hauer, Christian Gmeiner, Ezequiel Garcia, Gregory Clement, linux-arm-kernel Hi Florian, Grant, On Tue, Nov 12, 2013 at 12:37:46PM +0000, Grant Likely wrote: > On Fri, 25 Oct 2013 05:40:57 +0100, Florian Fainelli <florian@openwrt.org> wrote: > > Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit : > > > Dear Grant Likely, > > > > > > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote: > > > > I understand what you're trying to do here, but it causes a troublesome > > > > leakage of implementation detail into the binding, making the whole > > > > thing look very odd. This binding tries to make a fixed link look > > > > exactly like a real PHY even to the point of including a phandle to the > > > > phy. But having a phandle to a node which is *always* a direct child of > > > > the MAC node is redundant and a rather looney. Yes, doing it that way > > > > makes it easy for of_phy_find_device() to be transparent for fixed link, > > > > but that should *not* drive bindings, especially when that makes the > > > > binding really rather weird. > > > > > > > > Second, this new binding doesn't provide anything over and above the > > > > existing fixed-link binding. It may not be pretty, but it is > > > > estabilshed. > > > > > > Have you followed the past discussions about this patch set? Basically > > > the *only* feedback I received on RFCv1 is that the fixed-link property > > > sucks, and everybody (including the known Device Tree binding > > > maintainer Mark Rutland) suggested to not use the fixed-link mechanism. > > > See http://article.gmane.org/gmane.linux.network/276932, where Mark > > > said: > > > > > > "" > > > I'm not sure grouping these values together is the best way of handling > > > this. It's rather opaque, and inflexible for future extension. > > > "" > > > > > > So, please DT maintainers, tell me what you want. I honestly don't care > > > whether fixed-link or a separate node is chosen. However, I do care > > > about being dragged around between two solutions just because the > > > former DT maintainer and the new DT maintainers do not agree. > > I've been sleepy on this issue, which limits how much I can push. I'll > say one more thing on the issue (below) and then leave the decision up > to Mark. I trust him and he knows what he is doing. > > > Since I would like to move forward so I can one day use that binding in a > > real-life product, I am going to go for Mark's side which happens to be how I > > want the binding to look like. > > > > Do we all agree that the new binding is just way better than the old one? In > > light of the recent unstable DT ABI discussion, we can still continue parsing > > the old one for the sake of compatibility. > > Regardless of what you want it to look like, does the old binding work > for your purposes? If yes then use it. The only valid reason for > creating a new binding is if the old one doesn't work for a specific > (not theoretical) use case. I think the issue here was that I am not versed in the history of all of the existing bindings. While I'm not keen on the existing fixed-link property and I think it should be done differently were it being created from scratch today, as Grant has pointed out we're already supporting it today, and adding a new binding is going to make the code handling it more complex. If fixed-link works for your use case today, then use fixed-link. If we have a valid reason to create a new binding, we should. At the moment I think the only egregious portion of the binding is the globally unique fake PHY id, and if that causes issues we should be able to assign IDs within Linux ignoring the values in the DT, or reorganise things such that the arbitrary ID doesn't matter. If there are configurations we need to support that the fixed-link property cannot encode, then I think we should go ahead with the binding style that you are proposing today. However, we don't need to go with it right away, and we can continue to support fixed-link regardless. I apologise for the lack of consistency here, and I'm sorry that I've delayed this series for so long. Thanks, Mark. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs 2013-11-12 16:29 ` Mark Rutland @ 2013-11-12 17:40 ` Florian Fainelli 0 siblings, 0 replies; 21+ messages in thread From: Florian Fainelli @ 2013-11-12 17:40 UTC (permalink / raw) To: Mark Rutland Cc: Grant Likely, Thomas Petazzoni, Lior Amsalem, devicetree, netdev, Sascha Hauer, Christian Gmeiner, Ezequiel Garcia, Gregory Clement, David S. Miller, linux-arm-kernel 2013/11/12 Mark Rutland <mark.rutland@arm.com>: > Hi Florian, Grant, > > On Tue, Nov 12, 2013 at 12:37:46PM +0000, Grant Likely wrote: >> On Fri, 25 Oct 2013 05:40:57 +0100, Florian Fainelli <florian@openwrt.org> wrote: >> > Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit : >> > > Dear Grant Likely, >> > > >> > > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote: >> > > > I understand what you're trying to do here, but it causes a troublesome >> > > > leakage of implementation detail into the binding, making the whole >> > > > thing look very odd. This binding tries to make a fixed link look >> > > > exactly like a real PHY even to the point of including a phandle to the >> > > > phy. But having a phandle to a node which is *always* a direct child of >> > > > the MAC node is redundant and a rather looney. Yes, doing it that way >> > > > makes it easy for of_phy_find_device() to be transparent for fixed link, >> > > > but that should *not* drive bindings, especially when that makes the >> > > > binding really rather weird. >> > > > >> > > > Second, this new binding doesn't provide anything over and above the >> > > > existing fixed-link binding. It may not be pretty, but it is >> > > > estabilshed. >> > > >> > > Have you followed the past discussions about this patch set? Basically >> > > the *only* feedback I received on RFCv1 is that the fixed-link property >> > > sucks, and everybody (including the known Device Tree binding >> > > maintainer Mark Rutland) suggested to not use the fixed-link mechanism. >> > > See http://article.gmane.org/gmane.linux.network/276932, where Mark >> > > said: >> > > >> > > "" >> > > I'm not sure grouping these values together is the best way of handling >> > > this. It's rather opaque, and inflexible for future extension. >> > > "" >> > > >> > > So, please DT maintainers, tell me what you want. I honestly don't care >> > > whether fixed-link or a separate node is chosen. However, I do care >> > > about being dragged around between two solutions just because the >> > > former DT maintainer and the new DT maintainers do not agree. >> >> I've been sleepy on this issue, which limits how much I can push. I'll >> say one more thing on the issue (below) and then leave the decision up >> to Mark. I trust him and he knows what he is doing. >> >> > Since I would like to move forward so I can one day use that binding in a >> > real-life product, I am going to go for Mark's side which happens to be how I >> > want the binding to look like. >> > >> > Do we all agree that the new binding is just way better than the old one? In >> > light of the recent unstable DT ABI discussion, we can still continue parsing >> > the old one for the sake of compatibility. >> >> Regardless of what you want it to look like, does the old binding work >> for your purposes? If yes then use it. The only valid reason for >> creating a new binding is if the old one doesn't work for a specific >> (not theoretical) use case. > > I think the issue here was that I am not versed in the history of all of > the existing bindings. While I'm not keen on the existing fixed-link > property and I think it should be done differently were it being created > from scratch today, as Grant has pointed out we're already supporting it > today, and adding a new binding is going to make the code handling it > more complex. > > If fixed-link works for your use case today, then use fixed-link. Like I said in an earlier response to Thomas yesterday, it does not cover everything, most importantly, the existing "fixed-link" property does not allow the representation of direct MAC to MAC connections where you still need to specify the MII connection type for it to work properly. This is something that can be easily provided by the new fixed PHY binding, but less easily by the existing "fixed-link" property. We cannot extend "fixed-link" to contain a 6th integer because "phy-mode" or "phy-connection-type" is a string. My options here are pretty simple: - use the existing "fixed-link" property even though it is not nice, it does the job - add a supplemental "connection-type" property and parse it in my driver, which is where it is going to be used - go on with the new proposed Device Tree binding > > If we have a valid reason to create a new binding, we should. At the > moment I think the only egregious portion of the binding is the globally > unique fake PHY id, and if that causes issues we should be able to > assign IDs within Linux ignoring the values in the DT, or reorganise > things such that the arbitrary ID doesn't matter. I think we all agreed that the PHY ID (the term should be clarified as it tends to either mean PHY address or PHY OUI) had to be gone, and this is what Thomas did in this serie. Whichever binding we choose, it should be fairly easy to keep the "PHY ID" integer in the "fixed-link" property but make the code ignore it (with possibly a big fat warning for a transition period). > > If there are configurations we need to support that the fixed-link > property cannot encode, then I think we should go ahead with the binding > style that you are proposing today. However, we don't need to go with it > right away, and we can continue to support fixed-link regardless. Well, yes, the lack of "connection-type" representation is a blocker for some hardware configurations where two Ethernet MACs are connected one to each other. Whether this deserves a new incarnation of the "fixed PHY" Device Tree binding is left for discussion. > > I apologise for the lack of consistency here, and I'm sorry that I've > delayed this series for so long. -- Florian ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs 2013-09-06 15:18 ` [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs Thomas Petazzoni [not found] ` <1378480701-12908-4-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2013-11-12 1:43 ` Florian Fainelli 1 sibling, 0 replies; 21+ messages in thread From: Florian Fainelli @ 2013-11-12 1:43 UTC (permalink / raw) To: Thomas Petazzoni Cc: David S. Miller, netdev, devicetree, Lior Amsalem, Gregory Clement, Ezequiel Garcia, linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner Hello Thomas, 2013/9/6 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". > + > +Such a fixed link situation is described by creating a PHY node as a > +sub-node of an Ethernet device, with the following properties: > + > +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a > + fixed link PHY. > +* 'speed' (integer, mandatory), to indicate the link speed. Accepted > + values are 10, 100 and 1000 'max-speed' might be better here to match ePAPR v1.1 (if we do care, 'speed') works for me too. > +* 'full-duplex' (boolean, optional), to indicate that full duplex is > + used. When absent, half duplex is assumed. > +* 'pause' (boolean, optional), to indicate that pause should be > + enabled. > +* 'asym-pause' (boolean, optional), to indicate that asym_pause should > + be enabled. We also need to add a property: 'connection-type' which can be any of 'mii', 'rgmii' etc... When operating Ethernet devices with Ethernet devices connected back to back, it might be required to configure the Ethernet MAC with an appropriate connection type. Note that I picked 'connection-type' here because this the ePAPR v1.1 terminology. Now the good thing is that it is a new "feature" wrt. the old binding. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCHv2 4/4] net: mvneta: add support for fixed links 2013-09-06 15:18 [RFC PATCHv2 0/4] Add DT support for fixed PHYs Thomas Petazzoni ` (2 preceding siblings ...) 2013-09-06 15:18 ` [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs Thomas Petazzoni @ 2013-09-06 15:18 ` Thomas Petazzoni 2013-09-06 20:42 ` [RFC PATCHv2 0/4] Add DT support for fixed PHYs Florian Fainelli 2013-09-11 6:42 ` Sascha Hauer 5 siblings, 0 replies; 21+ messages in thread From: Thomas Petazzoni @ 2013-09-06 15:18 UTC (permalink / raw) To: David S. Miller, netdev, devicetree Cc: Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia, linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner Following the introduction of of_phy_register_fixed_link(), this patch introduces fixed link support in the mvneta driver, for Marvell Armada 370/XP SOCs. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- .../devicetree/bindings/net/marvell-armada-370-neta.txt | 4 ++-- drivers/net/ethernet/marvell/mvneta.c | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt index 859a6fa..4d07d4e 100644 --- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt +++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt @@ -4,8 +4,8 @@ Required properties: - compatible: should be "marvell,armada-370-neta". - reg: address and length of the register set for the device. - interrupts: interrupt for the device -- phy: A phandle to a phy node defining the PHY address (as the reg - property, a single integer). +- phy: A phandle to the PHY node describing the PHY to which this + Ethernet controller is connected to. - phy-mode: The interface between the SoC and the PHY (a string that of_get_phy_mode() can understand) - clocks: a pointer to the reference clock for this device. diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index b017818..6da1516 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -2711,10 +2711,12 @@ static int mvneta_probe(struct platform_device *pdev) } phy_node = of_parse_phandle(dn, "phy", 0); - if (!phy_node) { - dev_err(&pdev->dev, "no associated PHY\n"); - err = -ENODEV; - goto err_free_irq; + if (of_phy_is_fixed_link(phy_node)) { + err = of_phy_register_fixed_link(phy_node); + if (err < 0) { + dev_err(&pdev->dev, "cannot register fixed PHY\n"); + goto err_free_irq; + } } phy_mode = of_get_phy_mode(dn); -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [RFC PATCHv2 0/4] Add DT support for fixed PHYs 2013-09-06 15:18 [RFC PATCHv2 0/4] Add DT support for fixed PHYs Thomas Petazzoni ` (3 preceding siblings ...) 2013-09-06 15:18 ` [RFC PATCHv2 4/4] net: mvneta: add support for fixed links Thomas Petazzoni @ 2013-09-06 20:42 ` Florian Fainelli 2013-09-07 10:27 ` Thomas Petazzoni 2013-09-11 6:42 ` Sascha Hauer 5 siblings, 1 reply; 21+ messages in thread From: Florian Fainelli @ 2013-09-06 20:42 UTC (permalink / raw) To: Thomas Petazzoni Cc: David S. Miller, netdev, devicetree, Lior Amsalem, Gregory Clement, Ezequiel Garcia, linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner Hello Thomas, Le vendredi 6 septembre 2013 17:18:17 Thomas Petazzoni a écrit : > Hello, > > Here is a second version of the patch set that adds a Device Tree > binding and the related code to support fixed PHYs. Marked as RFC, > this patch set is obviously not intended for merging in 3.12. Thanks a lot for continuing on this work, I really like the state of it now. > > Since the first version, the changes have been: > > * Instead of using a 'fixed-link' property inside the Ethernet device > DT node, with a fairly cryptic succession of integer values, we now > use a PHY subnode under the Ethernet device DT node, with explicit > properties to configure the duplex, speed, pause and other PHY > properties. > > * The PHY address is automatically allocated by the kernel and no > longer visible in the Device Tree binding. > > * The PHY device is created directly when the network driver calls > of_phy_connect_fixed_link(), and associated to the PHY DT node, > which allows the existing of_phy_connect() function to work, > without the need to use the deprecated of_phy_connect_fixed_link(). > > The things I am not entirely happy with yet are: > > * The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a > properly reserved vendor/device identifier, but it isn't clear how > to get one allocated for this purpose. Right, we should try to get something better, but we obviously cannot use an already allocated OUI for this. Can we ask the Linux foundation or a Linux- friendly company to allocate one maybe? > > * The fixed_phy_register() function in drivers/net/phy/fixed.c has > some OF references. So ideally, I would have preferred to put this > code in drivers/of/of_mdio.c, but to call get_phy_device(), we need > a reference to the mii_bus structure that represents the fixed MDIO > bus. This is not a big deal, not everything in drivers/ is consistent with this, and making the fixed MDIO bus globally accessible does not sound too great. > > * There is some error management missing in fixed_phy_register(), but > it can certainly be added easily. This RFC is meant to sort out the > general idea. Do you think you could add these to got beyond the RFC state? The patchset as it currently is fine with me if you can address these. -- Florian ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCHv2 0/4] Add DT support for fixed PHYs 2013-09-06 20:42 ` [RFC PATCHv2 0/4] Add DT support for fixed PHYs Florian Fainelli @ 2013-09-07 10:27 ` Thomas Petazzoni 0 siblings, 0 replies; 21+ messages in thread From: Thomas Petazzoni @ 2013-09-07 10:27 UTC (permalink / raw) To: Florian Fainelli Cc: David S. Miller, netdev, devicetree, Lior Amsalem, Gregory Clement, Ezequiel Garcia, linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner Dear Florian Fainelli, On Fri, 06 Sep 2013 21:42:42 +0100, Florian Fainelli wrote: > > Here is a second version of the patch set that adds a Device Tree > > binding and the related code to support fixed PHYs. Marked as RFC, > > this patch set is obviously not intended for merging in 3.12. > > Thanks a lot for continuing on this work, I really like the state of it now. Thanks for your feedback. > > Since the first version, the changes have been: > > > > * Instead of using a 'fixed-link' property inside the Ethernet device > > DT node, with a fairly cryptic succession of integer values, we now > > use a PHY subnode under the Ethernet device DT node, with explicit > > properties to configure the duplex, speed, pause and other PHY > > properties. > > > > * The PHY address is automatically allocated by the kernel and no > > longer visible in the Device Tree binding. > > > > * The PHY device is created directly when the network driver calls > > of_phy_connect_fixed_link(), and associated to the PHY DT node, > > which allows the existing of_phy_connect() function to work, > > without the need to use the deprecated of_phy_connect_fixed_link(). > > > > The things I am not entirely happy with yet are: > > > > * The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a > > properly reserved vendor/device identifier, but it isn't clear how > > to get one allocated for this purpose. > > Right, we should try to get something better, but we obviously cannot use an > already allocated OUI for this. Can we ask the Linux foundation or a Linux- > friendly company to allocate one maybe? According to http://standards.ieee.org/develop/regauth/oui/oui.txt, the Linux Foundation doesn't seem to own any OUI. Should we simply be contacting some random companies in this list? > > * The fixed_phy_register() function in drivers/net/phy/fixed.c has > > some OF references. So ideally, I would have preferred to put this > > code in drivers/of/of_mdio.c, but to call get_phy_device(), we need > > a reference to the mii_bus structure that represents the fixed MDIO > > bus. > > This is not a big deal, not everything in drivers/ is consistent with this, > and making the fixed MDIO bus globally accessible does not sound too great. Indeed. > > * There is some error management missing in fixed_phy_register(), but > > it can certainly be added easily. This RFC is meant to sort out the > > general idea. > > Do you think you could add these to got beyond the RFC state? The patchset as > it currently is fine with me if you can address these. Sure, it shouldn't be too difficult. In the mean time, I'm interested in hearing comments from other people, especially from the Device Tree bindings maintainers: while the internal implementation details can always be fixed later on, the DT binding should obviously get an approval from the DT maintainers. Thanks, Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCHv2 0/4] Add DT support for fixed PHYs 2013-09-06 15:18 [RFC PATCHv2 0/4] Add DT support for fixed PHYs Thomas Petazzoni ` (4 preceding siblings ...) 2013-09-06 20:42 ` [RFC PATCHv2 0/4] Add DT support for fixed PHYs Florian Fainelli @ 2013-09-11 6:42 ` Sascha Hauer [not found] ` <20130911064248.GI30088-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 5 siblings, 1 reply; 21+ messages in thread From: Sascha Hauer @ 2013-09-11 6:42 UTC (permalink / raw) To: Thomas Petazzoni Cc: David S. Miller, netdev, devicetree, Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia, linux-arm-kernel, Mark Rutland, Christian Gmeiner On Fri, Sep 06, 2013 at 05:18:17PM +0200, Thomas Petazzoni wrote: > Hello, > > Here is a second version of the patch set that adds a Device Tree > binding and the related code to support fixed PHYs. Marked as RFC, > this patch set is obviously not intended for merging in 3.12. > > Since the first version, the changes have been: > > * Instead of using a 'fixed-link' property inside the Ethernet device > DT node, with a fairly cryptic succession of integer values, we now > use a PHY subnode under the Ethernet device DT node, with explicit > properties to configure the duplex, speed, pause and other PHY > properties. > > * The PHY address is automatically allocated by the kernel and no > longer visible in the Device Tree binding. > > * The PHY device is created directly when the network driver calls > of_phy_connect_fixed_link(), and associated to the PHY DT node, > which allows the existing of_phy_connect() function to work, > without the need to use the deprecated of_phy_connect_fixed_link(). > > The things I am not entirely happy with yet are: > > * The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a > properly reserved vendor/device identifier, but it isn't clear how > to get one allocated for this purpose. > > * The fixed_phy_register() function in drivers/net/phy/fixed.c has > some OF references. So ideally, I would have preferred to put this > code in drivers/of/of_mdio.c, but to call get_phy_device(), we need > a reference to the mii_bus structure that represents the fixed MDIO > bus. > > * There is some error management missing in fixed_phy_register(), but > it can certainly be added easily. This RFC is meant to sort out the > general idea. +1 for the general idea. This really looks good now. I've not much more to say. Maybe someone from the devicetree corner has a few words for the binding? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <20130911064248.GI30088-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]
* Re: [RFC PATCHv2 0/4] Add DT support for fixed PHYs [not found] ` <20130911064248.GI30088-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> @ 2013-09-25 7:12 ` Christian Gmeiner [not found] ` <CAH9NwWfBGHmZ+HfUndeh18NW+HyZ=c82W=O_4hJSOH-oZuM9jA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 21+ messages in thread From: Christian Gmeiner @ 2013-09-25 7:12 UTC (permalink / raw) To: Thomas Petazzoni, David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Mark Rutland, Christian Gmeiner Hi >> Hello, >> >> Here is a second version of the patch set that adds a Device Tree >> binding and the related code to support fixed PHYs. Marked as RFC, >> this patch set is obviously not intended for merging in 3.12. >> >> Since the first version, the changes have been: >> >> * Instead of using a 'fixed-link' property inside the Ethernet device >> DT node, with a fairly cryptic succession of integer values, we now >> use a PHY subnode under the Ethernet device DT node, with explicit >> properties to configure the duplex, speed, pause and other PHY >> properties. >> >> * The PHY address is automatically allocated by the kernel and no >> longer visible in the Device Tree binding. >> >> * The PHY device is created directly when the network driver calls >> of_phy_connect_fixed_link(), and associated to the PHY DT node, >> which allows the existing of_phy_connect() function to work, >> without the need to use the deprecated of_phy_connect_fixed_link(). >> >> The things I am not entirely happy with yet are: >> >> * The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a >> properly reserved vendor/device identifier, but it isn't clear how >> to get one allocated for this purpose. >> >> * The fixed_phy_register() function in drivers/net/phy/fixed.c has >> some OF references. So ideally, I would have preferred to put this >> code in drivers/of/of_mdio.c, but to call get_phy_device(), we need >> a reference to the mii_bus structure that represents the fixed MDIO >> bus. >> >> * There is some error management missing in fixed_phy_register(), but >> it can certainly be added easily. This RFC is meant to sort out the >> general idea. > > +1 for the general idea. This really looks good now. I've not much more > to say. Maybe someone from the devicetree corner has a few words for the > binding? > I tested the whole series with an I.MX6D board with the FEC driver: fec 2188000.ethernet eth0: Freescale FEC PHY driver [Generic PHY] (mii_bus:phy_addr=fixed-0:00, irq=-1) For me binding looks nice and I hope to see this patch series in 3.13. Tested-by: Christian Gmeiner <christian.gmeiner-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> -- Christian Gmeiner, MSc -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <CAH9NwWfBGHmZ+HfUndeh18NW+HyZ=c82W=O_4hJSOH-oZuM9jA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC PATCHv2 0/4] Add DT support for fixed PHYs [not found] ` <CAH9NwWfBGHmZ+HfUndeh18NW+HyZ=c82W=O_4hJSOH-oZuM9jA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2014-02-10 10:30 ` Christian Gmeiner 2014-02-10 12:09 ` Thomas Petazzoni 0 siblings, 1 reply; 21+ messages in thread From: Christian Gmeiner @ 2014-02-10 10:30 UTC (permalink / raw) To: Thomas Petazzoni, David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Mark Rutland, Christian Gmeiner 2013-09-25 9:12 GMT+02:00 Christian Gmeiner <christian.gmeiner-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>: > Hi > >>> Hello, >>> >>> Here is a second version of the patch set that adds a Device Tree >>> binding and the related code to support fixed PHYs. Marked as RFC, >>> this patch set is obviously not intended for merging in 3.12. >>> >>> Since the first version, the changes have been: >>> >>> * Instead of using a 'fixed-link' property inside the Ethernet device >>> DT node, with a fairly cryptic succession of integer values, we now >>> use a PHY subnode under the Ethernet device DT node, with explicit >>> properties to configure the duplex, speed, pause and other PHY >>> properties. >>> >>> * The PHY address is automatically allocated by the kernel and no >>> longer visible in the Device Tree binding. >>> >>> * The PHY device is created directly when the network driver calls >>> of_phy_connect_fixed_link(), and associated to the PHY DT node, >>> which allows the existing of_phy_connect() function to work, >>> without the need to use the deprecated of_phy_connect_fixed_link(). >>> >>> The things I am not entirely happy with yet are: >>> >>> * The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a >>> properly reserved vendor/device identifier, but it isn't clear how >>> to get one allocated for this purpose. >>> >>> * The fixed_phy_register() function in drivers/net/phy/fixed.c has >>> some OF references. So ideally, I would have preferred to put this >>> code in drivers/of/of_mdio.c, but to call get_phy_device(), we need >>> a reference to the mii_bus structure that represents the fixed MDIO >>> bus. >>> >>> * There is some error management missing in fixed_phy_register(), but >>> it can certainly be added easily. This RFC is meant to sort out the >>> general idea. >> >> +1 for the general idea. This really looks good now. I've not much more >> to say. Maybe someone from the devicetree corner has a few words for the >> binding? >> > > I tested the whole series with an I.MX6D board with the FEC driver: > fec 2188000.ethernet eth0: Freescale FEC PHY driver [Generic PHY] > (mii_bus:phy_addr=fixed-0:00, irq=-1) > > For me binding looks nice and I hope to see this patch series in 3.13. > > Tested-by: Christian Gmeiner <christian.gmeiner-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Is there any update on this patch series? -- Christian Gmeiner, MSc https://soundcloud.com/christian-gmeiner -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCHv2 0/4] Add DT support for fixed PHYs 2014-02-10 10:30 ` Christian Gmeiner @ 2014-02-10 12:09 ` Thomas Petazzoni 0 siblings, 0 replies; 21+ messages in thread From: Thomas Petazzoni @ 2014-02-10 12:09 UTC (permalink / raw) To: Christian Gmeiner Cc: David S. Miller, netdev, devicetree, Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia, linux-arm-kernel, Mark Rutland Dear Christian Gmeiner, On Mon, 10 Feb 2014 11:30:30 +0100, Christian Gmeiner wrote: > >> +1 for the general idea. This really looks good now. I've not much more > >> to say. Maybe someone from the devicetree corner has a few words for the > >> binding? > >> > > > > I tested the whole series with an I.MX6D board with the FEC driver: > > fec 2188000.ethernet eth0: Freescale FEC PHY driver [Generic PHY] > > (mii_bus:phy_addr=fixed-0:00, irq=-1) > > > > For me binding looks nice and I hope to see this patch series in 3.13. > > > > Tested-by: Christian Gmeiner <christian.gmeiner@gmail.com> > > Is there any update on this patch series? I'll try to send a new version in the next few days. It's still part of my TODO list. Thanks! Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2014-02-10 12:09 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-09-06 15:18 [RFC PATCHv2 0/4] Add DT support for fixed PHYs Thomas Petazzoni 2013-09-06 15:18 ` [RFC PATCHv2 1/4] net: phy: decouple PHY id and PHY address in fixed PHY driver Thomas Petazzoni 2013-09-06 15:18 ` [RFC PATCHv2 2/4] net: phy: extend fixed driver with fixed_phy_register() Thomas Petazzoni 2013-09-06 15:18 ` [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs Thomas Petazzoni [not found] ` <1378480701-12908-4-git-send-email-thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2013-09-18 4:29 ` Grant Likely [not found] ` <20130918042923.5D845C42CF7-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org> 2013-09-18 9:21 ` Florian Fainelli [not found] ` <CAGVrzcbVTenhVC4tQznJFqVpO08ruxLyy1ZiLmw6Bu1=3zbGZg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2013-09-18 15:00 ` Grant Likely [not found] ` <20130918150031.D9034C42CDF-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org> 2013-09-19 6:36 ` Sascha Hauer 2013-09-18 16:11 ` Thomas Petazzoni 2013-10-25 4:40 ` Florian Fainelli 2013-11-12 12:37 ` Grant Likely 2013-11-12 16:29 ` Mark Rutland 2013-11-12 17:40 ` Florian Fainelli 2013-11-12 1:43 ` Florian Fainelli 2013-09-06 15:18 ` [RFC PATCHv2 4/4] net: mvneta: add support for fixed links Thomas Petazzoni 2013-09-06 20:42 ` [RFC PATCHv2 0/4] Add DT support for fixed PHYs Florian Fainelli 2013-09-07 10:27 ` Thomas Petazzoni 2013-09-11 6:42 ` Sascha Hauer [not found] ` <20130911064248.GI30088-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 2013-09-25 7:12 ` Christian Gmeiner [not found] ` <CAH9NwWfBGHmZ+HfUndeh18NW+HyZ=c82W=O_4hJSOH-oZuM9jA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2014-02-10 10:30 ` Christian Gmeiner 2014-02-10 12:09 ` Thomas Petazzoni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).