| Message ID | 1392642484-19938-2-git-send-email-ben.dooks@codethink.co.uk |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Feb 17, 2014 at 01:08:04PM +0000, Ben Dooks wrote: > Add new init-regs field for of_phy nodes and make sure these > get applied when the phy is configured. > > This allows any phy node in an fdt to initialise registers > that may not be set as standard by the driver at initialisation > time, such as LED controls. Why not have a driver for the particular PHY? If it's not standard we don't need to pretend it is. If it has some extensions then the standard compatible string can be a fallback entry in the compatible list. I think allocating a compatible string and handling it in the kernel is better than having arbitrary register poke values in the dt. Thanks, Mark. > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > Documentation/devicetree/bindings/net/phy.txt | 12 ++++++ > drivers/net/phy/phy_device.c | 59 ++++++++++++++++++++++++++- > 2 files changed, 70 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt > index 58307d0..48d8ded 100644 > --- a/Documentation/devicetree/bindings/net/phy.txt > +++ b/Documentation/devicetree/bindings/net/phy.txt > @@ -20,6 +20,8 @@ Optional Properties: > assume clause 22. The compatible list may also contain other > elements. > - max-speed: Maximum PHY supported speed (10, 100, 1000...) > +- init-regs: Set of registers to modify at initialisation as a > + a set of <register set clear> > > Example: > > @@ -29,3 +31,13 @@ ethernet-phy@0 { > interrupts = <35 1>; > reg = <0>; > }; > + > +ethernet-phy@0 { > + compatible = "ethernet-phy-ieee802.3-c22"; > + interrupt-parent = <40000>; > + interrupts = <35 1>; > + reg = <0>; > + > + /* set KSZ8041 LED mode bits correctly */ > + init-reg = <0x1e 0x4000 0xc000>; > +}; > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 82514e7..6741cdb 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -33,6 +33,7 @@ > #include <linux/mdio.h> > #include <linux/io.h> > #include <linux/uaccess.h> > +#include <linux/of.h> > > #include <asm/irq.h> > > @@ -532,6 +533,57 @@ static int phy_poll_reset(struct phy_device *phydev) > return 0; > } > > +#ifdef CONFIG_OF > +static int of_phy_configure(struct phy_device *phydev) > +{ > + struct device *dev = &phydev->dev; > + struct device_node *of_node = dev->of_node; > + struct property *prop; > + const __be32 *ptr; > + u32 reg, set, clear; > + int len; > + int val; > + > + if (!of_node) > + of_node = dev->parent->of_node; > + if (!of_node) > + return 0; > + > + prop = of_find_property(of_node, "init-regs", &len); > + if (prop) { > + if (len % (sizeof(__be32) * 3)) { > + dev_err(dev, "init-regs not multiple of 3 entries\n"); > + return -EINVAL; > + } > + > + ptr = of_prop_next_u32(prop, ptr, ®); > + while (ptr != NULL) { > + ptr = of_prop_next_u32(prop, ptr, ®); > + ptr = of_prop_next_u32(prop, ptr, &set); > + ptr = of_prop_next_u32(prop, ptr, &clear); > + > + val = phy_read(phydev, reg); > + if (val < 0) { > + dev_err(dev, "failed to read %d\n", reg); > + return val; > + } > + > + val &= ~clear; > + val |= set; > + phy_write(phydev, reg, val); > + > + dev_info(dev, "set d to %04x\n", reg, val); > + > + ptr = of_prop_next_u32(prop, ptr, ®); > + } > + } > + > + return 0; > +} > +#else > +static inline int of_phy_configure(struct phy_device *phydev) { return 0; } > +#endif > + > int phy_init_hw(struct phy_device *phydev) > { > int ret; > @@ -551,7 +603,12 @@ int phy_init_hw(struct phy_device *phydev) > if (ret < 0) > return ret; > > - return phydev->drv->config_init(phydev); > + ret = phydev->drv->config_init(phydev); > + > + if (ret == 0) > + ret = of_phy_configure(phydev); > + > + return ret; > } > EXPORT_SYMBOL(phy_init_hw); > > -- > 1.8.5.3 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17/02/14 13:44, Mark Rutland wrote: > On Mon, Feb 17, 2014 at 01:08:04PM +0000, Ben Dooks wrote: >> Add new init-regs field for of_phy nodes and make sure these >> get applied when the phy is configured. >> >> This allows any phy node in an fdt to initialise registers >> that may not be set as standard by the driver at initialisation >> time, such as LED controls. > > Why not have a driver for the particular PHY? If it's not standard we > don't need to pretend it is. If it has some extensions then the standard > compatible string can be a fallback entry in the compatible list. I was trying to provide some useful and reasonably generic code to setup PHYs without having to add specific arguments to each of them. I could have added something like ksz8041,led-mode1 = <1> and updated the micrel driver.
On 17/02/14 13:44, Mark Rutland wrote: > On Mon, Feb 17, 2014 at 01:08:04PM +0000, Ben Dooks wrote: >> Add new init-regs field for of_phy nodes and make sure these >> get applied when the phy is configured. >> >> This allows any phy node in an fdt to initialise registers >> that may not be set as standard by the driver at initialisation >> time, such as LED controls. > > Why not have a driver for the particular PHY? If it's not standard we > don't need to pretend it is. If it has some extensions then the standard > compatible string can be a fallback entry in the compatible list. > > I think allocating a compatible string and handling it in the kernel is > better than having arbitrary register poke values in the dt. >> + prop = of_find_property(of_node, "init-regs", &len); >> + if (prop) { >> + if (len % (sizeof(__be32) * 3)) { >> + dev_err(dev, "init-regs not multiple of 3 entries\n"); >> + return -EINVAL; >> + } >> + >> + ptr = of_prop_next_u32(prop, ptr, ®); >> + while (ptr != NULL) { >> + ptr = of_prop_next_u32(prop, ptr, ®); oops, bad rebase fixup, this should have been deleted.
On Mon, Feb 17, 2014 at 01:50:28PM +0000, Ben Dooks wrote: > On 17/02/14 13:44, Mark Rutland wrote: > > On Mon, Feb 17, 2014 at 01:08:04PM +0000, Ben Dooks wrote: > >> Add new init-regs field for of_phy nodes and make sure these > >> get applied when the phy is configured. > >> > >> This allows any phy node in an fdt to initialise registers > >> that may not be set as standard by the driver at initialisation > >> time, such as LED controls. > > > > Why not have a driver for the particular PHY? If it's not standard we > > don't need to pretend it is. If it has some extensions then the standard > > compatible string can be a fallback entry in the compatible list. > > I was trying to provide some useful and reasonably generic code > to setup PHYs without having to add specific arguments to each of > them. > > I could have added something like ksz8041,led-mode1 = <1> and > updated the micrel driver. Perhaps. It depends on precisely what the property is describing. I think describing the hardware and letting Linux figure out what to do is better than giving it a set of opaque instructions for it to blindly follow. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ben, 2014-02-17 5:08 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>: > Add new init-regs field for of_phy nodes and make sure these > get applied when the phy is configured. > > This allows any phy node in an fdt to initialise registers > that may not be set as standard by the driver at initialisation > time, such as LED controls. > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > Documentation/devicetree/bindings/net/phy.txt | 12 ++++++ > drivers/net/phy/phy_device.c | 59 ++++++++++++++++++++++++++- > 2 files changed, 70 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt > index 58307d0..48d8ded 100644 > --- a/Documentation/devicetree/bindings/net/phy.txt > +++ b/Documentation/devicetree/bindings/net/phy.txt > @@ -20,6 +20,8 @@ Optional Properties: > assume clause 22. The compatible list may also contain other > elements. > - max-speed: Maximum PHY supported speed (10, 100, 1000...) > +- init-regs: Set of registers to modify at initialisation as a > + a set of <register set clear> Should be: "micrel,led-control-init-val" or something like that. first cell is the register address, according to the IEEE 802.3 clause 22 second cell is the set bitmask to apply to the register address specified in the first cell third cell is the clear bitmask to apply to the register address specified in the second cell I would rather see this as a specific PHY node DT property for setting the LED control register, because this is totally non-standard and you are touching a proprietary register here. > > Example: > > @@ -29,3 +31,13 @@ ethernet-phy@0 { > interrupts = <35 1>; > reg = <0>; > }; > + > +ethernet-phy@0 { > + compatible = "ethernet-phy-ieee802.3-c22"; > + interrupt-parent = <40000>; > + interrupts = <35 1>; > + reg = <0>; > + > + /* set KSZ8041 LED mode bits correctly */ > + init-reg = <0x1e 0x4000 0xc000>; > +}; > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 82514e7..6741cdb 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -33,6 +33,7 @@ > #include <linux/mdio.h> > #include <linux/io.h> > #include <linux/uaccess.h> > +#include <linux/of.h> > > #include <asm/irq.h> > > @@ -532,6 +533,57 @@ static int phy_poll_reset(struct phy_device *phydev) > return 0; > } > > +#ifdef CONFIG_OF > +static int of_phy_configure(struct phy_device *phydev) > +{ > + struct device *dev = &phydev->dev; > + struct device_node *of_node = dev->of_node; > + struct property *prop; > + const __be32 *ptr; > + u32 reg, set, clear; > + int len; > + int val; This does not belong in the generic PHY code unless we are very clear on what we want to do, and how to do it, which I do not think we are yet. What exactly is needed here: - fixing up some design mistake? - accounting for a specific board design? In any case a PHY fixup would do the job for you. > + > + if (!of_node) > + of_node = dev->parent->of_node; > + if (!of_node) > + return 0; > + > + prop = of_find_property(of_node, "init-regs", &len); > + if (prop) { > + if (len % (sizeof(__be32) * 3)) { > + dev_err(dev, "init-regs not multiple of 3 entries\n"); > + return -EINVAL; > + } > + > + ptr = of_prop_next_u32(prop, ptr, ®); > + while (ptr != NULL) { > + ptr = of_prop_next_u32(prop, ptr, ®); > + ptr = of_prop_next_u32(prop, ptr, &set); > + ptr = of_prop_next_u32(prop, ptr, &clear); > + > + val = phy_read(phydev, reg); > + if (val < 0) { > + dev_err(dev, "failed to read %d\n", reg); > + return val; > + } > + > + val &= ~clear; > + val |= set; > + phy_write(phydev, reg, val); > + > + dev_info(dev, "set d to %04x\n", reg, val); > + > + ptr = of_prop_next_u32(prop, ptr, ®); > + } > + } > + > + return 0; > +} > +#else > +static inline int of_phy_configure(struct phy_device *phydev) { return 0; } > +#endif > + > int phy_init_hw(struct phy_device *phydev) > { > int ret; > @@ -551,7 +603,12 @@ int phy_init_hw(struct phy_device *phydev) > if (ret < 0) > return ret; > > - return phydev->drv->config_init(phydev); > + ret = phydev->drv->config_init(phydev); > + > + if (ret == 0) > + ret = of_phy_configure(phydev); > + > + return ret; > } > EXPORT_SYMBOL(phy_init_hw); > > -- > 1.8.5.3 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17/02/14 17:33, Florian Fainelli wrote: > Hi Ben, > > 2014-02-17 5:08 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>: >> Add new init-regs field for of_phy nodes and make sure these >> get applied when the phy is configured. >> >> This allows any phy node in an fdt to initialise registers >> that may not be set as standard by the driver at initialisation >> time, such as LED controls. >> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> --- >> Documentation/devicetree/bindings/net/phy.txt | 12 ++++++ >> drivers/net/phy/phy_device.c | 59 ++++++++++++++++++++++++++- >> 2 files changed, 70 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt >> index 58307d0..48d8ded 100644 >> --- a/Documentation/devicetree/bindings/net/phy.txt >> +++ b/Documentation/devicetree/bindings/net/phy.txt >> @@ -20,6 +20,8 @@ Optional Properties: >> assume clause 22. The compatible list may also contain other >> elements. >> - max-speed: Maximum PHY supported speed (10, 100, 1000...) >> +- init-regs: Set of registers to modify at initialisation as a >> + a set of <register set clear> > > Should be: > > "micrel,led-control-init-val" or something like that. > > first cell is the register address, according to the IEEE 802.3 clause 22 > second cell is the set bitmask to apply to the register address > specified in the first cell > third cell is the clear bitmask to apply to the register address > specified in the second cell > > I would rather see this as a specific PHY node DT property for setting > the LED control register, because this is totally non-standard and you > are touching a proprietary register here. I'd rather stay with this than splattering lots and lots of phy specific additions to each phy driver. This has the plus it lets board developers set registers in case of board specific initialisation values that are not already in the drivers.
2014-02-17 9:44 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>: > On 17/02/14 17:33, Florian Fainelli wrote: >> >> Hi Ben, >> >> 2014-02-17 5:08 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>: >>> >>> Add new init-regs field for of_phy nodes and make sure these >>> get applied when the phy is configured. >>> >>> This allows any phy node in an fdt to initialise registers >>> that may not be set as standard by the driver at initialisation >>> time, such as LED controls. >>> >>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >>> --- >>> Documentation/devicetree/bindings/net/phy.txt | 12 ++++++ >>> drivers/net/phy/phy_device.c | 59 >>> ++++++++++++++++++++++++++- >>> 2 files changed, 70 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/net/phy.txt >>> b/Documentation/devicetree/bindings/net/phy.txt >>> index 58307d0..48d8ded 100644 >>> --- a/Documentation/devicetree/bindings/net/phy.txt >>> +++ b/Documentation/devicetree/bindings/net/phy.txt >>> @@ -20,6 +20,8 @@ Optional Properties: >>> assume clause 22. The compatible list may also contain other >>> elements. >>> - max-speed: Maximum PHY supported speed (10, 100, 1000...) >>> +- init-regs: Set of registers to modify at initialisation as a >>> + a set of <register set clear> >> >> >> Should be: >> >> "micrel,led-control-init-val" or something like that. >> >> first cell is the register address, according to the IEEE 802.3 clause 22 >> second cell is the set bitmask to apply to the register address >> specified in the first cell >> third cell is the clear bitmask to apply to the register address >> specified in the second cell >> >> I would rather see this as a specific PHY node DT property for setting >> the LED control register, because this is totally non-standard and you >> are touching a proprietary register here. > > > I'd rather stay with this than splattering lots and lots of > phy specific additions to each phy driver. And I would rather not do it because: - Device Tree related problems are already hard enough to debug that we do not want DT to act as a firmware and provide values for Linux to digest without even a bit of parsing and validation - this is not capture by a PHY fixup, and will not be reported as such in /sys/class/mdio_bus/*/*/phy_has_fixups - this covers the one time initialization that might be needed, but does not cover the suspend/resume/reset times where this is also needed - this is vendor-specific and sort of calls for being moved into the PHY driver > > This has the plus it lets board developers set registers in > case of board specific initialisation values that are not > already in the drivers. And this bloats the code for the other 99% people that do not use things like this. Do not get me wrong, Ethernet PHYs never work as we want them to, but we libphy comes with PHY fixups callbacks which allows you to capture quirks and will ensure that they are run again at the right time in the PHY state machine cycle.
On Mon, Feb 17, 2014 at 05:44:02PM +0000, Ben Dooks wrote: > On 17/02/14 17:33, Florian Fainelli wrote: > > Hi Ben, > > > > 2014-02-17 5:08 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>: > >> Add new init-regs field for of_phy nodes and make sure these > >> get applied when the phy is configured. > >> > >> This allows any phy node in an fdt to initialise registers > >> that may not be set as standard by the driver at initialisation > >> time, such as LED controls. > >> > >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > >> --- > >> Documentation/devicetree/bindings/net/phy.txt | 12 ++++++ > >> drivers/net/phy/phy_device.c | 59 ++++++++++++++++++++++++++- > >> 2 files changed, 70 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt > >> index 58307d0..48d8ded 100644 > >> --- a/Documentation/devicetree/bindings/net/phy.txt > >> +++ b/Documentation/devicetree/bindings/net/phy.txt > >> @@ -20,6 +20,8 @@ Optional Properties: > >> assume clause 22. The compatible list may also contain other > >> elements. > >> - max-speed: Maximum PHY supported speed (10, 100, 1000...) > >> +- init-regs: Set of registers to modify at initialisation as a > >> + a set of <register set clear> > > > > Should be: > > > > "micrel,led-control-init-val" or something like that. > > > > first cell is the register address, according to the IEEE 802.3 clause 22 > > second cell is the set bitmask to apply to the register address > > specified in the first cell > > third cell is the clear bitmask to apply to the register address > > specified in the second cell > > > > I would rather see this as a specific PHY node DT property for setting > > the LED control register, because this is totally non-standard and you > > are touching a proprietary register here. > > I'd rather stay with this than splattering lots and lots of > phy specific additions to each phy driver. For something that's PHY-specific anyway? Should we remove the rest of the "phy specific additions" that constitute a driver? > This has the plus it lets board developers set registers in > case of board specific initialisation values that are not > already in the drivers. This also has the minus that it lets board developers set registers in arbitrary ways, creating new bugs that will require more effort to work around in drivers. If you update the DTs to describe the hardware to the kernel, then the kernel can later be updated to handle specific PHYs better. That cannot happen if you try to hide information from the kernel by giving it a list of arbitrary magic numbers. Drivers are the place such things should go. The DT is not a place for shoddy bytecode. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Mark Rutland <mark.rutland@arm.com> Date: Mon, 17 Feb 2014 18:04:15 +0000 > If you update the DTs to describe the hardware to the kernel, then the > kernel can later be updated to handle specific PHYs better. That cannot > happen if you try to hide information from the kernel by giving it a > list of arbitrary magic numbers. > > Drivers are the place such things should go. The DT is not a place for > shoddy bytecode. +1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-02-17 13:33 GMT-08:00 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>: > Hello. > > > On 02/17/2014 08:33 PM, Florian Fainelli wrote: > >>> Add new init-regs field for of_phy nodes and make sure these >>> get applied when the phy is configured. > > >>> This allows any phy node in an fdt to initialise registers >>> that may not be set as standard by the driver at initialisation >>> time, such as LED controls. > > >>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >>> --- >>> Documentation/devicetree/bindings/net/phy.txt | 12 ++++++ >>> drivers/net/phy/phy_device.c | 59 >>> ++++++++++++++++++++++++++- >>> 2 files changed, 70 insertions(+), 1 deletion(-) > > >>> diff --git a/Documentation/devicetree/bindings/net/phy.txt >>> b/Documentation/devicetree/bindings/net/phy.txt >>> index 58307d0..48d8ded 100644 >>> --- a/Documentation/devicetree/bindings/net/phy.txt >>> +++ b/Documentation/devicetree/bindings/net/phy.txt >>> @@ -20,6 +20,8 @@ Optional Properties: >>> assume clause 22. The compatible list may also contain other >>> elements. >>> - max-speed: Maximum PHY supported speed (10, 100, 1000...) >>> +- init-regs: Set of registers to modify at initialisation as a >>> + a set of <register set clear> > > >> Should be: > > >> "micrel,led-control-init-val" or something like that. > > >> first cell is the register address, according to the IEEE 802.3 clause 22 >> second cell is the set bitmask to apply to the register address >> specified in the first cell >> third cell is the clear bitmask to apply to the register address >> specified in the second cell > > >> I would rather see this as a specific PHY node DT property for setting >> the LED control register, because this is totally non-standard and you >> are touching a proprietary register here. > > > Yes, I agree. > > >>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >>> index 82514e7..6741cdb 100644 >>> --- a/drivers/net/phy/phy_device.c >>> +++ b/drivers/net/phy/phy_device.c > > [...] > >>> @@ -532,6 +533,57 @@ static int phy_poll_reset(struct phy_device *phydev) >>> return 0; >>> } >>> >>> +#ifdef CONFIG_OF >>> +static int of_phy_configure(struct phy_device *phydev) >>> +{ >>> + struct device *dev = &phydev->dev; >>> + struct device_node *of_node = dev->of_node; >>> + struct property *prop; >>> + const __be32 *ptr; >>> + u32 reg, set, clear; >>> + int len; >>> + int val; > > >> This does not belong in the generic PHY code unless we are very clear >> on what we want to do, and how to do it, which I do not think we are >> yet. What exactly is needed here: > > >> - fixing up some design mistake? >> - accounting for a specific board design? > > > Kind of both. This was invented to defy the necessity of having platform > fixup in the DT case (where there should be no board file to place it into). > I have already described that platform fixup necessary on the Renesas > Lager/Koelsch boards where the LED0 signat is connected to ETH_LINK signal > on the SoC and the PHY reset sets the LED control bits to default 0 which > means that LED0 will be LINK/ACTIVITY signal and thus blink on activity and > cause ETH_LINK to bounce off/on after each packet. > > >> In any case a PHY fixup would do the job for you. > > > Not in any case. In case of DT we have no place for it, so should invent > something involving DT. How is DT different than any machine probing mechanism here? The way to involve DT is to do the following: if (of_machine_is_compatible("renesas,foo-board-with-broken-micrel-phy")) phy_register_fixup(&foo_board_with_broken_micrel_phy); If your machine compatible string does not allow you to uniquely identify your machine, this is a DT problem, as this should really be the case. If you do not want to add this code to wherever this is relevant in arch/arm/mach-shmobile/board-*.c, neither is drivers/net/phy/phy_device.c this the place to add it. Dealing with quirks applying to industry standard blocks is to update the relevant driver, based on information provided by the specifically affected systems. Failure to identify either of those correctly is a problem that must not lead to a generic "let's override PHY registers from DT" type of solution. As usual, mechanism vs policy applies even more when DT is involved.
2014-02-17 14:08 GMT-08:00 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>: > On 02/17/2014 11:48 PM, Florian Fainelli wrote: > >>>>> Add new init-regs field for of_phy nodes and make sure these >>>>> get applied when the phy is configured. > > >>>>> This allows any phy node in an fdt to initialise registers >>>>> that may not be set as standard by the driver at initialisation >>>>> time, such as LED controls. > > >>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > > [...] > > >>>>> diff --git a/drivers/net/phy/phy_device.c >>>>> b/drivers/net/phy/phy_device.c >>>>> index 82514e7..6741cdb 100644 >>>>> --- a/drivers/net/phy/phy_device.c >>>>> +++ b/drivers/net/phy/phy_device.c > > >>> [...] > > >>>>> @@ -532,6 +533,57 @@ static int phy_poll_reset(struct phy_device >>>>> *phydev) >>>>> return 0; >>>>> } >>>>> >>>>> +#ifdef CONFIG_OF >>>>> +static int of_phy_configure(struct phy_device *phydev) >>>>> +{ >>>>> + struct device *dev = &phydev->dev; >>>>> + struct device_node *of_node = dev->of_node; >>>>> + struct property *prop; >>>>> + const __be32 *ptr; >>>>> + u32 reg, set, clear; >>>>> + int len; >>>>> + int val; > > >>>> This does not belong in the generic PHY code unless we are very clear >>>> on what we want to do, and how to do it, which I do not think we are >>>> yet. What exactly is needed here: > > >>>> - fixing up some design mistake? >>>> - accounting for a specific board design? > > >>> Kind of both. This was invented to defy the necessity of having >>> platform >>> fixup in the DT case (where there should be no board file to place it >>> into). >>> I have already described that platform fixup necessary on the Renesas >>> Lager/Koelsch boards where the LED0 signat is connected to ETH_LINK >>> signal >>> on the SoC and the PHY reset sets the LED control bits to default 0 which >>> means that LED0 will be LINK/ACTIVITY signal and thus blink on activity >>> and >>> cause ETH_LINK to bounce off/on after each packet. > > >>>> In any case a PHY fixup would do the job for you. > > >>> Not in any case. In case of DT we have no place for it, so should >>> invent >>> something involving DT. > > >> How is DT different than any machine probing mechanism here? > > > There supposed to be no board files. The purpose of DT is to get rid of > the board files, at least on ARM. > > >> The way to involve DT is to do the following: > > >> if (of_machine_is_compatible("renesas,foo-board-with-broken-micrel-phy")) >> phy_register_fixup(&foo_board_with_broken_micrel_phy); > > > Where are you suggesting to place such code? > arch/arm/mach-shmobile/setup-*.c? Somewhere along those lines, I am not familiar at all with the SH Mobile line of SoCs and how the DT/non-DT code used to look like. Although I would be naively thinking that hooking this into the init_machine() callback for the DT machine descriptor would do the job. > > >> If your machine compatible string does not allow you to uniquely >> identify your machine, this is a DT problem, as this should really be >> the case. If you do not want to add this code to wherever this is >> relevant in arch/arm/mach-shmobile/board-*.c, > > > There just should be no such file for DT case. There is still a generic file which catches all SH Mobile machines and registers some peripherals, as far as I look e.g; board-marzen.c that one is still doing a bunch of platform_device_register_full() calls. Even if the DT board file was extremely generic to the point where it contains nothing, adding a custom init_machine() callback which registers PHY fixups would not be too crazy. > > >> neither is drivers/net/phy/phy_device.c this the place to add it. > > > Hey, I wasn't arguing with that! :-) > > >> Dealing with quirks applying to industry standard blocks is to update >> the relevant driver, based on information provided by the specifically >> affected systems. Failure to identify either of those correctly is a >> problem that must not lead to a generic "let's override PHY registers >> from DT" type of solution. > > >> As usual, mechanism vs policy applies even more when DT is involved. > > > Ah, so you're suggesting placing the fixup code in the driver itself? > That's a bit strange for the platform specific code, but would do I guess... PHY fixups are slightly different from say, traditional HW fixups, what I meant here was that the general use case for quirks is: - board code detects the faulty hardware and sets a flag that gets passed down the relevant driver - the relevant driver checks for this flag to enable such a thing For the specific PHY devices, there are actually two ways to deal with this: - register a PHY fixup somewhere (TBD where this somewhere is) - set the phydev->dev_flags (just like what the TG3 driver does for instance
Hello. On 02/17/2014 08:33 PM, Florian Fainelli wrote: >> Add new init-regs field for of_phy nodes and make sure these >> get applied when the phy is configured. >> This allows any phy node in an fdt to initialise registers >> that may not be set as standard by the driver at initialisation >> time, such as LED controls. >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> --- >> Documentation/devicetree/bindings/net/phy.txt | 12 ++++++ >> drivers/net/phy/phy_device.c | 59 ++++++++++++++++++++++++++- >> 2 files changed, 70 insertions(+), 1 deletion(-) >> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt >> index 58307d0..48d8ded 100644 >> --- a/Documentation/devicetree/bindings/net/phy.txt >> +++ b/Documentation/devicetree/bindings/net/phy.txt >> @@ -20,6 +20,8 @@ Optional Properties: >> assume clause 22. The compatible list may also contain other >> elements. >> - max-speed: Maximum PHY supported speed (10, 100, 1000...) >> +- init-regs: Set of registers to modify at initialisation as a >> + a set of <register set clear> > Should be: > "micrel,led-control-init-val" or something like that. > first cell is the register address, according to the IEEE 802.3 clause 22 > second cell is the set bitmask to apply to the register address > specified in the first cell > third cell is the clear bitmask to apply to the register address > specified in the second cell > I would rather see this as a specific PHY node DT property for setting > the LED control register, because this is totally non-standard and you > are touching a proprietary register here. Yes, I agree. >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index 82514e7..6741cdb 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c [...] >> @@ -532,6 +533,57 @@ static int phy_poll_reset(struct phy_device *phydev) >> return 0; >> } >> >> +#ifdef CONFIG_OF >> +static int of_phy_configure(struct phy_device *phydev) >> +{ >> + struct device *dev = &phydev->dev; >> + struct device_node *of_node = dev->of_node; >> + struct property *prop; >> + const __be32 *ptr; >> + u32 reg, set, clear; >> + int len; >> + int val; > This does not belong in the generic PHY code unless we are very clear > on what we want to do, and how to do it, which I do not think we are > yet. What exactly is needed here: > - fixing up some design mistake? > - accounting for a specific board design? Kind of both. This was invented to defy the necessity of having platform fixup in the DT case (where there should be no board file to place it into). I have already described that platform fixup necessary on the Renesas Lager/Koelsch boards where the LED0 signat is connected to ETH_LINK signal on the SoC and the PHY reset sets the LED control bits to default 0 which means that LED0 will be LINK/ACTIVITY signal and thus blink on activity and cause ETH_LINK to bounce off/on after each packet. > In any case a PHY fixup would do the job for you. Not in any case. In case of DT we have no place for it, so should invent something involving DT. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/17/2014 11:48 PM, Florian Fainelli wrote: >>>> Add new init-regs field for of_phy nodes and make sure these >>>> get applied when the phy is configured. >>>> This allows any phy node in an fdt to initialise registers >>>> that may not be set as standard by the driver at initialisation >>>> time, such as LED controls. >>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> [...] >>>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >>>> index 82514e7..6741cdb 100644 >>>> --- a/drivers/net/phy/phy_device.c >>>> +++ b/drivers/net/phy/phy_device.c >> [...] >>>> @@ -532,6 +533,57 @@ static int phy_poll_reset(struct phy_device *phydev) >>>> return 0; >>>> } >>>> >>>> +#ifdef CONFIG_OF >>>> +static int of_phy_configure(struct phy_device *phydev) >>>> +{ >>>> + struct device *dev = &phydev->dev; >>>> + struct device_node *of_node = dev->of_node; >>>> + struct property *prop; >>>> + const __be32 *ptr; >>>> + u32 reg, set, clear; >>>> + int len; >>>> + int val; >>> This does not belong in the generic PHY code unless we are very clear >>> on what we want to do, and how to do it, which I do not think we are >>> yet. What exactly is needed here: >>> - fixing up some design mistake? >>> - accounting for a specific board design? >> Kind of both. This was invented to defy the necessity of having platform >> fixup in the DT case (where there should be no board file to place it into). >> I have already described that platform fixup necessary on the Renesas >> Lager/Koelsch boards where the LED0 signat is connected to ETH_LINK signal >> on the SoC and the PHY reset sets the LED control bits to default 0 which >> means that LED0 will be LINK/ACTIVITY signal and thus blink on activity and >> cause ETH_LINK to bounce off/on after each packet. >>> In any case a PHY fixup would do the job for you. >> Not in any case. In case of DT we have no place for it, so should invent >> something involving DT. > How is DT different than any machine probing mechanism here? There supposed to be no board files. The purpose of DT is to get rid of the board files, at least on ARM. > The way to involve DT is to do the following: > if (of_machine_is_compatible("renesas,foo-board-with-broken-micrel-phy")) > phy_register_fixup(&foo_board_with_broken_micrel_phy); Where are you suggesting to place such code? arch/arm/mach-shmobile/setup-*.c? > If your machine compatible string does not allow you to uniquely > identify your machine, this is a DT problem, as this should really be > the case. If you do not want to add this code to wherever this is > relevant in arch/arm/mach-shmobile/board-*.c, There just should be no such file for DT case. > neither is drivers/net/phy/phy_device.c this the place to add it. Hey, I wasn't arguing with that! :-) > Dealing with quirks applying to industry standard blocks is to update > the relevant driver, based on information provided by the specifically > affected systems. Failure to identify either of those correctly is a > problem that must not lead to a generic "let's override PHY registers > from DT" type of solution. > As usual, mechanism vs policy applies even more when DT is involved. Ah, so you're suggesting placing the fixup code in the driver itself? That's a bit strange for the platform specific code, but would do I guess... WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17/02/14 20:48, Florian Fainelli wrote: [snip] >>> - fixing up some design mistake? >>> - accounting for a specific board design? >> >> >> Kind of both. This was invented to defy the necessity of having platform >> fixup in the DT case (where there should be no board file to place it into). >> I have already described that platform fixup necessary on the Renesas >> Lager/Koelsch boards where the LED0 signat is connected to ETH_LINK signal >> on the SoC and the PHY reset sets the LED control bits to default 0 which >> means that LED0 will be LINK/ACTIVITY signal and thus blink on activity and >> cause ETH_LINK to bounce off/on after each packet. >> >> >>> In any case a PHY fixup would do the job for you. >> >> >> Not in any case. In case of DT we have no place for it, so should invent >> something involving DT. > > How is DT different than any machine probing mechanism here? The way > to involve DT is to do the following: > > if (of_machine_is_compatible("renesas,foo-board-with-broken-micrel-phy")) > phy_register_fixup(&foo_board_with_broken_micrel_phy); Oh yes, but now I have to do that for Linux, for $BSD, and for anything else I want to run on the device. I thought dt was meant to allow us to describe the hardware. If this is the case, let's just call this linuxtree and let everyone else get on with their own thing again. See also comment below. > If your machine compatible string does not allow you to uniquely > identify your machine, this is a DT problem, as this should really be > the case. If you do not want to add this code to wherever this is > relevant in arch/arm/mach-shmobile/board-*.c, neither is > drivers/net/phy/phy_device.c this the place to add it. So where should it be added? If we keep piling stuff into board files in arch/arm.... then we're just back to the pre-dt case and going to keep getting shouted at. > Dealing with quirks applying to industry standard blocks is to update > the relevant driver, based on information provided by the specifically > affected systems. Failure to identify either of those correctly is a > problem that must not lead to a generic "let's override PHY registers > from DT" type of solution. > > As usual, mechanism vs policy applies even more when DT is involved. There's an industry standard for the access method, but every PHY seems to have different extra setup registers for their own cases. I will leave this out here in case anyone else finds it useful, there may be a case where there are PHYs that need an amount of register initialisation and this code may be smaller than putting a pile of dt properties in.
On Tue, Feb 18, 2014 at 08:16:46AM +0000, Ben Dooks wrote: > On 17/02/14 20:48, Florian Fainelli wrote: > > [snip] > > >>> - fixing up some design mistake? > >>> - accounting for a specific board design? > >> > >> > >> Kind of both. This was invented to defy the necessity of having platform > >> fixup in the DT case (where there should be no board file to place it into). > >> I have already described that platform fixup necessary on the Renesas > >> Lager/Koelsch boards where the LED0 signat is connected to ETH_LINK signal > >> on the SoC and the PHY reset sets the LED control bits to default 0 which > >> means that LED0 will be LINK/ACTIVITY signal and thus blink on activity and > >> cause ETH_LINK to bounce off/on after each packet. > >> > >> > >>> In any case a PHY fixup would do the job for you. > >> > >> > >> Not in any case. In case of DT we have no place for it, so should invent > >> something involving DT. > > > > How is DT different than any machine probing mechanism here? The way > > to involve DT is to do the following: > > > > if (of_machine_is_compatible("renesas,foo-board-with-broken-micrel-phy")) > > phy_register_fixup(&foo_board_with_broken_micrel_phy); > > Oh yes, but now I have to do that for Linux, for $BSD, and for > anything else I want to run on the device. I thought dt was meant > to allow us to describe the hardware. It does allow you to describe the hardware. Arbitrary register writes aren't a description of the hardware, they're a sequence of instructions that tells the OS nothing about the hardware and limit the ability of an OS to do something different that might be better. It's already the case that the OS has to have some knowledge of the hardware that's implicit in a binding. We don't expect to have to include bytecode to tell the OS how to poke a particular UART when it can figure that out from a compatible string. > If this is the case, let's just call this linuxtree and let everyone > else get on with their own thing again. This doesn't follow at all. Any OS needs to have some understanding of the hardware it will try to poke. Describing a specific sequence of writes in a DT is no more operating system independent than identifying the hardware and expecting the OS to have a driver for it. The requirements aren't any more suited to an individual OS in either case. > > See also comment below. > > > If your machine compatible string does not allow you to uniquely > > identify your machine, this is a DT problem, as this should really be > > the case. If you do not want to add this code to wherever this is > > relevant in arch/arm/mach-shmobile/board-*.c, neither is > > drivers/net/phy/phy_device.c this the place to add it. > > So where should it be added? If we keep piling stuff into board files > in arch/arm.... then we're just back to the pre-dt case and going to > keep getting shouted at. The general trend has been to allocate new compatible strings for components and let individual drivers handle this. As far as I can see your case doesn't involve any components external to the PHY, so should probably live in a PHY driver. The PHY can have a specific compatible string with the generic string as a fallback (if it works to some degree without special poking). I don't see that we need anything board-specific. > > > Dealing with quirks applying to industry standard blocks is to update > > the relevant driver, based on information provided by the specifically > > affected systems. Failure to identify either of those correctly is a > > problem that must not lead to a generic "let's override PHY registers > > from DT" type of solution. > > > > As usual, mechanism vs policy applies even more when DT is involved. > > There's an industry standard for the access method, but every PHY seems > to have different extra setup registers for their own cases. So? Have a driver for each PHY, or fixups for each PHY in a shared driver. Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 02/18/2014 02:54 PM, Mark Rutland wrote: >> [snip] >>>>> - fixing up some design mistake? >>>>> - accounting for a specific board design? >>>> Kind of both. This was invented to defy the necessity of having platform >>>> fixup in the DT case (where there should be no board file to place it into). >>>> I have already described that platform fixup necessary on the Renesas >>>> Lager/Koelsch boards where the LED0 signat is connected to ETH_LINK signal >>>> on the SoC and the PHY reset sets the LED control bits to default 0 which >>>> means that LED0 will be LINK/ACTIVITY signal and thus blink on activity and >>>> cause ETH_LINK to bounce off/on after each packet. >>>>> In any case a PHY fixup would do the job for you. >>>> Not in any case. In case of DT we have no place for it, so should invent >>>> something involving DT. >>> How is DT different than any machine probing mechanism here? The way >>> to involve DT is to do the following: >>> if (of_machine_is_compatible("renesas,foo-board-with-broken-micrel-phy")) >>> phy_register_fixup(&foo_board_with_broken_micrel_phy); >> Oh yes, but now I have to do that for Linux, for $BSD, and for >> anything else I want to run on the device. I thought dt was meant >> to allow us to describe the hardware. > It does allow you to describe the hardware. Arbitrary register writes > aren't a description of the hardware, they're a sequence of instructions > that tells the OS nothing about the hardware and limit the ability of an > OS to do something different that might be better. > It's already the case that the OS has to have some knowledge of the > hardware that's implicit in a binding. We don't expect to have to > include bytecode to tell the OS how to poke a particular UART when it > can figure that out from a compatible string. >> If this is the case, let's just call this linuxtree and let everyone >> else get on with their own thing again. > This doesn't follow at all. Any OS needs to have some understanding of > the hardware it will try to poke. Describing a specific sequence of > writes in a DT is no more operating system independent than identifying > the hardware and expecting the OS to have a driver for it. The > requirements aren't any more suited to an individual OS in either case. >> See also comment below. >>> If your machine compatible string does not allow you to uniquely >>> identify your machine, this is a DT problem, as this should really be >>> the case. If you do not want to add this code to wherever this is >>> relevant in arch/arm/mach-shmobile/board-*.c, neither is >>> drivers/net/phy/phy_device.c this the place to add it. >> So where should it be added? If we keep piling stuff into board files >> in arch/arm.... then we're just back to the pre-dt case and going to >> keep getting shouted at. > The general trend has been to allocate new compatible strings for > components and let individual drivers handle this. > As far as I can see your case doesn't involve any components external to > the PHY, so should probably live in a PHY driver. The PHY can have a It does involve LEDs which should function in the way described by their labels, and it does involve SoC for which ETH_LINK signal should remain stable and not bouncing after each packet. > specific compatible string with the generic string as a fallback (if it > works to some degree without special poking). It can but that doesn't solve this issue in any way. The issue is board specific, not only PHY specific. > I don't see that we need anything board-specific. Did you read the text substantially above this in this mail for more complete description of the issue? We're trying to emulate the PHY *platform* fixup here which didn't belong with the PHY driver. [...] > Cheers, > Mark. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 18, 2014 at 05:00:03PM +0000, Sergei Shtylyov wrote: > Hello. > > On 02/18/2014 02:54 PM, Mark Rutland wrote: > > >> [snip] > > >>>>> - fixing up some design mistake? > >>>>> - accounting for a specific board design? > > >>>> Kind of both. This was invented to defy the necessity of having platform > >>>> fixup in the DT case (where there should be no board file to place it into). > >>>> I have already described that platform fixup necessary on the Renesas > >>>> Lager/Koelsch boards where the LED0 signat is connected to ETH_LINK signal > >>>> on the SoC and the PHY reset sets the LED control bits to default 0 which > >>>> means that LED0 will be LINK/ACTIVITY signal and thus blink on activity and > >>>> cause ETH_LINK to bounce off/on after each packet. > > >>>>> In any case a PHY fixup would do the job for you. > > >>>> Not in any case. In case of DT we have no place for it, so should invent > >>>> something involving DT. > > >>> How is DT different than any machine probing mechanism here? The way > >>> to involve DT is to do the following: > > >>> if (of_machine_is_compatible("renesas,foo-board-with-broken-micrel-phy")) > >>> phy_register_fixup(&foo_board_with_broken_micrel_phy); > > >> Oh yes, but now I have to do that for Linux, for $BSD, and for > >> anything else I want to run on the device. I thought dt was meant > >> to allow us to describe the hardware. > > > It does allow you to describe the hardware. Arbitrary register writes > > aren't a description of the hardware, they're a sequence of instructions > > that tells the OS nothing about the hardware and limit the ability of an > > OS to do something different that might be better. > > > It's already the case that the OS has to have some knowledge of the > > hardware that's implicit in a binding. We don't expect to have to > > include bytecode to tell the OS how to poke a particular UART when it > > can figure that out from a compatible string. > > >> If this is the case, let's just call this linuxtree and let everyone > >> else get on with their own thing again. > > > This doesn't follow at all. Any OS needs to have some understanding of > > the hardware it will try to poke. Describing a specific sequence of > > writes in a DT is no more operating system independent than identifying > > the hardware and expecting the OS to have a driver for it. The > > requirements aren't any more suited to an individual OS in either case. > > >> See also comment below. > > >>> If your machine compatible string does not allow you to uniquely > >>> identify your machine, this is a DT problem, as this should really be > >>> the case. If you do not want to add this code to wherever this is > >>> relevant in arch/arm/mach-shmobile/board-*.c, neither is > >>> drivers/net/phy/phy_device.c this the place to add it. > > >> So where should it be added? If we keep piling stuff into board files > >> in arch/arm.... then we're just back to the pre-dt case and going to > >> keep getting shouted at. > > > The general trend has been to allocate new compatible strings for > > components and let individual drivers handle this. > > > As far as I can see your case doesn't involve any components external to > > the PHY, so should probably live in a PHY driver. The PHY can have a > > It does involve LEDs which should function in the way described by their > labels, and it does involve SoC for which ETH_LINK signal should remain stable > and not bouncing after each packet. Ah, I see I misunderstood. > > > specific compatible string with the generic string as a fallback (if it > > works to some degree without special poking). > > It can but that doesn't solve this issue in any way. The issue is board > specific, not only PHY specific. Sure. So additional properties are required. > > > I don't see that we need anything board-specific. > > Did you read the text substantially above this in this mail for more > complete description of the issue? We're trying to emulate the PHY *platform* > fixup here which didn't belong with the PHY driver. Apologies, I was indeed mistaken. Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ben, On Monday 17 February 2014 14:12:22 Mark Rutland wrote: > On Mon, Feb 17, 2014 at 01:50:28PM +0000, Ben Dooks wrote: > > On 17/02/14 13:44, Mark Rutland wrote: > > > On Mon, Feb 17, 2014 at 01:08:04PM +0000, Ben Dooks wrote: > > >> Add new init-regs field for of_phy nodes and make sure these > > >> get applied when the phy is configured. > > >> > > >> This allows any phy node in an fdt to initialise registers > > >> that may not be set as standard by the driver at initialisation > > >> time, such as LED controls. > > > > > > Why not have a driver for the particular PHY? If it's not standard we > > > don't need to pretend it is. If it has some extensions then the standard > > > compatible string can be a fallback entry in the compatible list. > > > > I was trying to provide some useful and reasonably generic code > > to setup PHYs without having to add specific arguments to each of > > them. > > > > I could have added something like ksz8041,led-mode1 = <1> and > > updated the micrel driver. > > Perhaps. It depends on precisely what the property is describing. > > I think describing the hardware and letting Linux figure out what to do > is better than giving it a set of opaque instructions for it to blindly > follow. I second that, I think a LED mode property would be better in this case.
diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt index 58307d0..48d8ded 100644 --- a/Documentation/devicetree/bindings/net/phy.txt +++ b/Documentation/devicetree/bindings/net/phy.txt @@ -20,6 +20,8 @@ Optional Properties: assume clause 22. The compatible list may also contain other elements. - max-speed: Maximum PHY supported speed (10, 100, 1000...) +- init-regs: Set of registers to modify at initialisation as a + a set of <register set clear> Example: @@ -29,3 +31,13 @@ ethernet-phy@0 { interrupts = <35 1>; reg = <0>; }; + +ethernet-phy@0 { + compatible = "ethernet-phy-ieee802.3-c22"; + interrupt-parent = <40000>; + interrupts = <35 1>; + reg = <0>; + + /* set KSZ8041 LED mode bits correctly */ + init-reg = <0x1e 0x4000 0xc000>; +}; diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 82514e7..6741cdb 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -33,6 +33,7 @@ #include <linux/mdio.h> #include <linux/io.h> #include <linux/uaccess.h> +#include <linux/of.h> #include <asm/irq.h> @@ -532,6 +533,57 @@ static int phy_poll_reset(struct phy_device *phydev) return 0; } +#ifdef CONFIG_OF +static int of_phy_configure(struct phy_device *phydev) +{ + struct device *dev = &phydev->dev; + struct device_node *of_node = dev->of_node; + struct property *prop; + const __be32 *ptr; + u32 reg, set, clear; + int len; + int val; + + if (!of_node) + of_node = dev->parent->of_node; + if (!of_node) + return 0; + + prop = of_find_property(of_node, "init-regs", &len); + if (prop) { + if (len % (sizeof(__be32) * 3)) { + dev_err(dev, "init-regs not multiple of 3 entries\n"); + return -EINVAL; + } + + ptr = of_prop_next_u32(prop, ptr, ®); + while (ptr != NULL) { + ptr = of_prop_next_u32(prop, ptr, ®); + ptr = of_prop_next_u32(prop, ptr, &set); + ptr = of_prop_next_u32(prop, ptr, &clear); + + val = phy_read(phydev, reg); + if (val < 0) { + dev_err(dev, "failed to read %d\n", reg); + return val; + } + + val &= ~clear; + val |= set; + phy_write(phydev, reg, val); + + dev_info(dev, "set d to %04x\n", reg, val); + + ptr = of_prop_next_u32(prop, ptr, ®); + } + } + + return 0; +} +#else +static inline int of_phy_configure(struct phy_device *phydev) { return 0; } +#endif + int phy_init_hw(struct phy_device *phydev) { int ret; @@ -551,7 +603,12 @@ int phy_init_hw(struct phy_device *phydev) if (ret < 0) return ret; - return phydev->drv->config_init(phydev); + ret = phydev->drv->config_init(phydev); + + if (ret == 0) + ret = of_phy_configure(phydev); + + return ret; } EXPORT_SYMBOL(phy_init_hw);
Add new init-regs field for of_phy nodes and make sure these get applied when the phy is configured. This allows any phy node in an fdt to initialise registers that may not be set as standard by the driver at initialisation time, such as LED controls. Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- Documentation/devicetree/bindings/net/phy.txt | 12 ++++++ drivers/net/phy/phy_device.c | 59 ++++++++++++++++++++++++++- 2 files changed, 70 insertions(+), 1 deletion(-)