Message ID | 1393415280-10227-1-git-send-email-ben.dooks@codethink.co.uk |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Ben Dooks <ben.dooks@codethink.co.uk> Date: Wed, 26 Feb 2014 11:48:00 +0000 > Add support for the led-mode property for the following PHYs > which have a single LED mode configuration value. > > KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and > KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4 > to control the LED configuration. > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, 2014-02-26 3:48 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>: [snip] > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 5a8993b..0c9e434 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -148,15 +148,52 @@ static int ks8737_config_intr(struct phy_device *phydev) > return rc < 0 ? rc : 0; > } > > +static int kszphy_setup_led(struct phy_device *phydev, > + unsigned int reg, unsigned int shift) > +{ > + > + struct device *dev = &phydev->dev; > + struct device_node *of_node = dev->of_node; > + int rc, temp; > + u32 val; > + > + if (!of_node && dev->parent->of_node) > + of_node = dev->parent->of_node; > + > + if (of_property_read_u32(of_node, "micrel,led-mode", &val)) > + return 0; This breaks non-OF configuration because of_read_property_read_u32() will return -ENOSYS, so you skip the LED register configuration entirely, is that intended? > + > + temp = phy_read(phydev, reg); > + if (temp < 0) > + return temp; > + > + temp &= 3 << shift; The compiler cannot verify that we are not overflowing, you might want to make sure that shift <= 14 (just in case) > + temp |= val << shift; > + rc = phy_write(phydev, reg, temp); > + > + return rc < 0 ? rc : 0; You could have; return phy_write(phydev, reg, temp); > +} > + > static int kszphy_config_init(struct phy_device *phydev) > { > return 0; > } > > +static int kszphy_config_init_led8041(struct phy_device *phydev) > +{ > + /* single led control, register 0x1e bits 15..14 */ > + return kszphy_setup_led(phydev, 0x1e, 14); > +} > + > static int ksz8021_config_init(struct phy_device *phydev) > { > - int rc; > const u16 val = KSZPHY_OMSO_B_CAST_OFF | KSZPHY_OMSO_RMII_OVERRIDE; > + int rc; > + > + rc = kszphy_setup_led(phydev, 0x1f, 4); > + if (rc) > + dev_err(&phydev->dev, "failed to set led mode\n"); > + > phy_write(phydev, MII_KSZPHY_OMSO, val); > rc = ksz_config_flags(phydev); > return rc < 0 ? rc : 0; > @@ -166,6 +203,10 @@ static int ks8051_config_init(struct phy_device *phydev) > { > int rc; > > + rc = kszphy_setup_led(phydev, 0x1f, 4); > + if (rc) > + dev_err(&phydev->dev, "failed to set led mode\n"); > + > rc = ksz_config_flags(phydev); > return rc < 0 ? rc : 0; > } > @@ -327,7 +368,7 @@ static struct phy_driver ksphy_driver[] = { > .features = (PHY_BASIC_FEATURES | SUPPORTED_Pause > | SUPPORTED_Asym_Pause), > .flags = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT, > - .config_init = kszphy_config_init, > + .config_init = kszphy_config_init_led8041, > .config_aneg = genphy_config_aneg, > .read_status = genphy_read_status, > .ack_interrupt = kszphy_ack_interrupt, > @@ -342,7 +383,7 @@ static struct phy_driver ksphy_driver[] = { > .features = PHY_BASIC_FEATURES | > SUPPORTED_Pause | SUPPORTED_Asym_Pause, > .flags = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT, > - .config_init = kszphy_config_init, > + .config_init = kszphy_config_init_led8041, > .config_aneg = genphy_config_aneg, > .read_status = genphy_read_status, > .ack_interrupt = kszphy_ack_interrupt, > @@ -371,7 +412,7 @@ static struct phy_driver ksphy_driver[] = { > .phy_id_mask = 0x00ffffff, > .features = (PHY_BASIC_FEATURES | SUPPORTED_Pause), > .flags = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT, > - .config_init = kszphy_config_init, > + .config_init = kszphy_config_init_led8041, > .config_aneg = genphy_config_aneg, > .read_status = genphy_read_status, > .ack_interrupt = kszphy_ack_interrupt, > -- > 1.8.5.3 >
On Wed, Feb 26, 2014 at 11:48:00AM +0000, Ben Dooks wrote: > Add support for the led-mode property for the following PHYs > which have a single LED mode configuration value. > > KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and > KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4 > to control the LED configuration. > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > Documentation/devicetree/bindings/net/micrel.txt | 18 +++++++++ > drivers/net/phy/micrel.c | 49 ++++++++++++++++++++++-- > 2 files changed, 63 insertions(+), 4 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/micrel.txt > > diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt > new file mode 100644 > index 0000000..98a3e61 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/micrel.txt > @@ -0,0 +1,18 @@ > +Micrel PHY properties. > + > +These properties cover the base properties Micrel PHYs. > + > +Optional properties: > + > + - micrel,led-mode : LED mode value to set for PHYs with configurable LEDs. > + > + Configure the LED mode with single value. The list of PHYs and > + the bits that are currently supported: > + > + KSZ8001: register 0x1e, bits 15..14 > + KSZ8041: register 0x1e, bits 15..14 > + KSZ8021: register 0x1f, bits 5..4 > + KSZ8031: register 0x1f, bits 5..4 > + KSZ8051: register 0x1f, bits 5..4 > + > + See the respective PHY datasheet for the mode values. What do these mean, roughly,, and why can the kernel not decide how to cnofigure these? In general we prefer to not place raw register values in the DT, and I'd like to know why we'd have to here. Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26/02/14 22:20, Florian Fainelli wrote: > Hi, > > 2014-02-26 3:48 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>: > > [snip] > >> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c >> index 5a8993b..0c9e434 100644 >> --- a/drivers/net/phy/micrel.c >> +++ b/drivers/net/phy/micrel.c >> @@ -148,15 +148,52 @@ static int ks8737_config_intr(struct phy_device *phydev) >> return rc < 0 ? rc : 0; >> } >> >> +static int kszphy_setup_led(struct phy_device *phydev, >> + unsigned int reg, unsigned int shift) >> +{ >> + >> + struct device *dev = &phydev->dev; >> + struct device_node *of_node = dev->of_node; >> + int rc, temp; >> + u32 val; >> + >> + if (!of_node && dev->parent->of_node) >> + of_node = dev->parent->of_node; >> + >> + if (of_property_read_u32(of_node, "micrel,led-mode", &val)) >> + return 0; > > This breaks non-OF configuration because of_read_property_read_u32() > will return -ENOSYS, so you skip the LED register configuration > entirely, is that intended? Yes, it is only here for OF case. I should however check if of_node is available for the !OF case. >> + >> + temp = phy_read(phydev, reg); >> + if (temp < 0) >> + return temp; >> + >> + temp &= 3 << shift; > > The compiler cannot verify that we are not overflowing, you might want > to make sure that shift <= 14 (just in case) Ok, should I add a WARN_ON(shift > 14) ? >> + temp |= val << shift; >> + rc = phy_write(phydev, reg, temp); >> + >> + return rc < 0 ? rc : 0; > > You could have; > > return phy_write(phydev, reg, temp); Thanks, will change to doing that.
On 27/02/14 09:15, Mark Rutland wrote: > On Wed, Feb 26, 2014 at 11:48:00AM +0000, Ben Dooks wrote: >> Add support for the led-mode property for the following PHYs >> which have a single LED mode configuration value. >> >> KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and >> KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4 >> to control the LED configuration. >> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> --- >> Documentation/devicetree/bindings/net/micrel.txt | 18 +++++++++ >> drivers/net/phy/micrel.c | 49 ++++++++++++++++++++++-- >> 2 files changed, 63 insertions(+), 4 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/net/micrel.txt >> >> diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt >> new file mode 100644 >> index 0000000..98a3e61 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/micrel.txt >> @@ -0,0 +1,18 @@ >> +Micrel PHY properties. >> + >> +These properties cover the base properties Micrel PHYs. >> + >> +Optional properties: >> + >> + - micrel,led-mode : LED mode value to set for PHYs with configurable LEDs. >> + >> + Configure the LED mode with single value. The list of PHYs and >> + the bits that are currently supported: >> + >> + KSZ8001: register 0x1e, bits 15..14 >> + KSZ8041: register 0x1e, bits 15..14 >> + KSZ8021: register 0x1f, bits 5..4 >> + KSZ8031: register 0x1f, bits 5..4 >> + KSZ8051: register 0x1f, bits 5..4 >> + >> + See the respective PHY datasheet for the mode values. > > What do these mean, roughly,, and why can the kernel not decide how to > cnofigure these? Board specific, in the case of the Lager one of the LEDs is connected to the ethernet mac block to indicate link, however the default mode is not for just "Link" so we have to change it. > In general we prefer to not place raw register values in the DT, and I'd > like to know why we'd have to here. I could copy out stuff from the data-sheet, but I was trying to avoid a copy and paste job.
Hi Ben, A bit of a late reply, I had missed this patch, sorry. On Thursday 27 February 2014 10:30:51 Ben Dooks wrote: > On 27/02/14 09:15, Mark Rutland wrote: > > On Wed, Feb 26, 2014 at 11:48:00AM +0000, Ben Dooks wrote: > >> Add support for the led-mode property for the following PHYs > >> which have a single LED mode configuration value. > >> > >> KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and > >> KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4 > >> to control the LED configuration. > >> > >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > >> --- > >> > >> Documentation/devicetree/bindings/net/micrel.txt | 18 +++++++++ > >> drivers/net/phy/micrel.c | 49 ++++++++++++++++-- > >> 2 files changed, 63 insertions(+), 4 deletions(-) > >> create mode 100644 Documentation/devicetree/bindings/net/micrel.txt > >> > >> diff --git a/Documentation/devicetree/bindings/net/micrel.txt > >> b/Documentation/devicetree/bindings/net/micrel.txt new file mode 100644 > >> index 0000000..98a3e61 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/net/micrel.txt > >> @@ -0,0 +1,18 @@ > >> +Micrel PHY properties. > >> + > >> +These properties cover the base properties Micrel PHYs. > >> + > >> +Optional properties: > >> + > >> + - micrel,led-mode : LED mode value to set for PHYs with configurable > >> LEDs. > >> + > >> + Configure the LED mode with single value. The list of PHYs > >> and > >> + the bits that are currently supported: > >> + > >> + KSZ8001: register 0x1e, bits 15..14 > >> + KSZ8041: register 0x1e, bits 15..14 > >> + KSZ8021: register 0x1f, bits 5..4 > >> + KSZ8031: register 0x1f, bits 5..4 > >> + KSZ8051: register 0x1f, bits 5..4 > >> + > >> + See the respective PHY datasheet for the mode values. > > > > What do these mean, roughly,, and why can the kernel not decide how to > > cnofigure these? > > Board specific, in the case of the Lager one of the LEDs is connected > to the ethernet mac block to indicate link, however the default mode > is not for just "Link" so we have to change it. > > > In general we prefer to not place raw register values in the DT, and I'd > > like to know why we'd have to here. > > I could copy out stuff from the data-sheet, but I was trying to avoid a > copy and paste job. I quite agree with Mark here, I would prefer not to list register values in DT bindings. However, the hardware hardware diversity doesn't help us abstracting LED modes. The following table summarizes LED usage options. Device Mode LED usage ----------------------------------------------------------------------------- 8001 (LED[3:0]) 00 Collision / Full-Duplex / Speed / Link/Activity 01 Activity / Full-Duplex/Collision / Speed / Link 10 Activity / Full-Duplex / 100Mbps Link / 10Mbps Link 11 Reserved 80[23]1 (LED[0]) 00 Link/Activity 01 Link 10 Reserved 11 Reserved 80[45]1 (LED[1:0]) 00 Speed / Link/Activity 01 Activity / Link 10 Reserved 11 Reserved While LED mode could be described by LED0 mode using "link-activity" or "link" strings for the 80[2345]1 chips, the 8001 makes that slightly more complex and shows that future chips might not conform to any scheme we come up with now. I thus have no strong preference for a string-based mode description over using an integer. However, if we keep the integer value, I wouldn't use the register value directly, but would instead use the mode field value as an integer in the 0-3 range. This would remove knowledge of the PHY control register layout from the DT node content, and bring more consistency to the values. Mark, what's your opinion on this ? I know that David has already applied the patch to his tree, but we can still fix this before v3.16. I can submit a patch.
Hi Ben, On Tuesday 18 March 2014 16:56:06 Laurent Pinchart wrote: > On Thursday 27 February 2014 10:30:51 Ben Dooks wrote: > > On 27/02/14 09:15, Mark Rutland wrote: > > > On Wed, Feb 26, 2014 at 11:48:00AM +0000, Ben Dooks wrote: > > >> Add support for the led-mode property for the following PHYs > > >> which have a single LED mode configuration value. > > >> > > >> KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and > > >> KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4 > > >> to control the LED configuration. > > >> > > >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > > >> --- > > >> > > >> Documentation/devicetree/bindings/net/micrel.txt | 18 +++++++++ > > >> drivers/net/phy/micrel.c | 49 ++++++++++++++-- > > >> 2 files changed, 63 insertions(+), 4 deletions(-) > > >> create mode 100644 Documentation/devicetree/bindings/net/micrel.txt > > >> > > >> diff --git a/Documentation/devicetree/bindings/net/micrel.txt > > >> b/Documentation/devicetree/bindings/net/micrel.txt new file mode 100644 > > >> index 0000000..98a3e61 > > >> --- /dev/null > > >> +++ b/Documentation/devicetree/bindings/net/micrel.txt > > >> @@ -0,0 +1,18 @@ > > >> +Micrel PHY properties. > > >> + > > >> +These properties cover the base properties Micrel PHYs. > > >> + > > >> +Optional properties: > > >> + > > >> + - micrel,led-mode : LED mode value to set for PHYs with configurable > > >> LEDs. > > >> + > > >> + Configure the LED mode with single value. The list of > > >> PHYs and > > >> + the bits that are currently supported: > > >> + > > >> + KSZ8001: register 0x1e, bits 15..14 > > >> + KSZ8041: register 0x1e, bits 15..14 > > >> + KSZ8021: register 0x1f, bits 5..4 > > >> + KSZ8031: register 0x1f, bits 5..4 > > >> + KSZ8051: register 0x1f, bits 5..4 > > >> + > > >> + See the respective PHY datasheet for the mode values. > > > > > > What do these mean, roughly,, and why can the kernel not decide how to > > > cnofigure these? > > > > Board specific, in the case of the Lager one of the LEDs is connected > > to the ethernet mac block to indicate link, however the default mode > > is not for just "Link" so we have to change it. > > > > > In general we prefer to not place raw register values in the DT, and I'd > > > like to know why we'd have to here. > > > > I could copy out stuff from the data-sheet, but I was trying to avoid a > > copy and paste job. > > I quite agree with Mark here, I would prefer not to list register values in > DT bindings. However, the hardware hardware diversity doesn't help us > abstracting LED modes. > > The following table summarizes LED usage options. > > Device Mode LED usage > ---------------------------------------------------------------------------- > 8001 (LED[3:0]) 00 Collision / Full-Duplex / Speed / Link/Activity > 01 Activity / Full-Duplex/Collision / Speed / Link > 10 Activity / Full-Duplex / 100Mbps Link / 10Mbps Link > 11 Reserved > 80[23]1 (LED[0]) 00 Link/Activity > 01 Link > 10 Reserved > 11 Reserved > 80[45]1 (LED[1:0]) 00 Speed / Link/Activity > 01 Activity / Link > 10 Reserved > 11 Reserved > > While LED mode could be described by LED0 mode using "link-activity" or > "link" strings for the 80[2345]1 chips, the 8001 makes that slightly more > complex and shows that future chips might not conform to any scheme we come > up with now. > > I thus have no strong preference for a string-based mode description over > using an integer. However, if we keep the integer value, I wouldn't use the > register value directly, but would instead use the mode field value as an > integer in the 0-3 range. This would remove knowledge of the PHY control > register layout from the DT node content, and bring more consistency to the > values. And I realize that this is what you've done already in the implementation :-/ I'll send a patch to clarify the DT bindings documentation if you don't mind, after hearing Mark's opinion on the issue. > Mark, what's your opinion on this ? I know that David has already applied > the patch to his tree, but we can still fix this before v3.16. I can submit > a patch.
On 18/03/14 17:11, Laurent Pinchart wrote: > Hi Ben, > > On Tuesday 18 March 2014 16:56:06 Laurent Pinchart wrote: >> On Thursday 27 February 2014 10:30:51 Ben Dooks wrote: >>> On 27/02/14 09:15, Mark Rutland wrote: >>>> On Wed, Feb 26, 2014 at 11:48:00AM +0000, Ben Dooks wrote: >>>>> Add support for the led-mode property for the following PHYs >>>>> which have a single LED mode configuration value. >>>>> >>>>> KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and >>>>> KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4 >>>>> to control the LED configuration. >>>>> >>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >>>>> --- >>>>> >>>>> Documentation/devicetree/bindings/net/micrel.txt | 18 +++++++++ >>>>> drivers/net/phy/micrel.c | 49 ++++++++++++++-- >>>>> 2 files changed, 63 insertions(+), 4 deletions(-) >>>>> create mode 100644 Documentation/devicetree/bindings/net/micrel.txt >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/net/micrel.txt >>>>> b/Documentation/devicetree/bindings/net/micrel.txt new file mode 100644 >>>>> index 0000000..98a3e61 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/net/micrel.txt >>>>> @@ -0,0 +1,18 @@ >>>>> +Micrel PHY properties. >>>>> + >>>>> +These properties cover the base properties Micrel PHYs. >>>>> + >>>>> +Optional properties: >>>>> + >>>>> + - micrel,led-mode : LED mode value to set for PHYs with configurable >>>>> LEDs. >>>>> + >>>>> + Configure the LED mode with single value. The list of >>>>> PHYs and >>>>> + the bits that are currently supported: >>>>> + >>>>> + KSZ8001: register 0x1e, bits 15..14 >>>>> + KSZ8041: register 0x1e, bits 15..14 >>>>> + KSZ8021: register 0x1f, bits 5..4 >>>>> + KSZ8031: register 0x1f, bits 5..4 >>>>> + KSZ8051: register 0x1f, bits 5..4 >>>>> + >>>>> + See the respective PHY datasheet for the mode values. >>>> >>>> What do these mean, roughly,, and why can the kernel not decide how to >>>> cnofigure these? >>> >>> Board specific, in the case of the Lager one of the LEDs is connected >>> to the ethernet mac block to indicate link, however the default mode >>> is not for just "Link" so we have to change it. >>> >>>> In general we prefer to not place raw register values in the DT, and I'd >>>> like to know why we'd have to here. >>> >>> I could copy out stuff from the data-sheet, but I was trying to avoid a >>> copy and paste job. >> >> I quite agree with Mark here, I would prefer not to list register values in >> DT bindings. However, the hardware hardware diversity doesn't help us >> abstracting LED modes. >> >> The following table summarizes LED usage options. >> >> Device Mode LED usage >> ---------------------------------------------------------------------------- >> 8001 (LED[3:0]) 00 Collision / Full-Duplex / Speed / Link/Activity >> 01 Activity / Full-Duplex/Collision / Speed / Link >> 10 Activity / Full-Duplex / 100Mbps Link / 10Mbps Link >> 11 Reserved >> 80[23]1 (LED[0]) 00 Link/Activity >> 01 Link >> 10 Reserved >> 11 Reserved >> 80[45]1 (LED[1:0]) 00 Speed / Link/Activity >> 01 Activity / Link >> 10 Reserved >> 11 Reserved >> >> While LED mode could be described by LED0 mode using "link-activity" or >> "link" strings for the 80[2345]1 chips, the 8001 makes that slightly more >> complex and shows that future chips might not conform to any scheme we come >> up with now. >> >> I thus have no strong preference for a string-based mode description over >> using an integer. However, if we keep the integer value, I wouldn't use the >> register value directly, but would instead use the mode field value as an >> integer in the 0-3 range. This would remove knowledge of the PHY control >> register layout from the DT node content, and bring more consistency to the >> values. > > And I realize that this is what you've done already in the implementation :-/ > I'll send a patch to clarify the DT bindings documentation if you don't mind, > after hearing Mark's opinion on the issue. Do we need strings for what is basically a single-setup post reset which it makes no sense for the user to ever update?
On Tuesday 18 March 2014 17:21:04 Ben Dooks wrote: > On 18/03/14 17:11, Laurent Pinchart wrote: > > On Tuesday 18 March 2014 16:56:06 Laurent Pinchart wrote: > >> On Thursday 27 February 2014 10:30:51 Ben Dooks wrote: > >>> On 27/02/14 09:15, Mark Rutland wrote: > >>>> On Wed, Feb 26, 2014 at 11:48:00AM +0000, Ben Dooks wrote: > >>>>> Add support for the led-mode property for the following PHYs > >>>>> which have a single LED mode configuration value. > >>>>> > >>>>> KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and > >>>>> KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4 > >>>>> to control the LED configuration. > >>>>> > >>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > >>>>> --- > >>>>> > >>>>> Documentation/devicetree/bindings/net/micrel.txt | 18 +++++++++ > >>>>> drivers/net/phy/micrel.c | 49 > >>>>> ++++++++++++++-- > >>>>> 2 files changed, 63 insertions(+), 4 deletions(-) > >>>>> create mode 100644 Documentation/devicetree/bindings/net/micrel.txt > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/net/micrel.txt > >>>>> b/Documentation/devicetree/bindings/net/micrel.txt new file mode > >>>>> 100644 > >>>>> index 0000000..98a3e61 > >>>>> --- /dev/null > >>>>> +++ b/Documentation/devicetree/bindings/net/micrel.txt > >>>>> @@ -0,0 +1,18 @@ > >>>>> +Micrel PHY properties. > >>>>> + > >>>>> +These properties cover the base properties Micrel PHYs. > >>>>> + > >>>>> +Optional properties: > >>>>> + > >>>>> + - micrel,led-mode : LED mode value to set for PHYs with configurable > >>>>> LEDs. > >>>>> + > >>>>> + Configure the LED mode with single value. The list of > >>>>> PHYs and > >>>>> + the bits that are currently supported: > >>>>> + > >>>>> + KSZ8001: register 0x1e, bits 15..14 > >>>>> + KSZ8041: register 0x1e, bits 15..14 > >>>>> + KSZ8021: register 0x1f, bits 5..4 > >>>>> + KSZ8031: register 0x1f, bits 5..4 > >>>>> + KSZ8051: register 0x1f, bits 5..4 > >>>>> + > >>>>> + See the respective PHY datasheet for the mode values. > >>>> > >>>> What do these mean, roughly,, and why can the kernel not decide how to > >>>> cnofigure these? > >>> > >>> Board specific, in the case of the Lager one of the LEDs is connected > >>> to the ethernet mac block to indicate link, however the default mode > >>> is not for just "Link" so we have to change it. > >>> > >>>> In general we prefer to not place raw register values in the DT, and > >>>> I'd > >>>> like to know why we'd have to here. > >>> > >>> I could copy out stuff from the data-sheet, but I was trying to avoid a > >>> copy and paste job. > >> > >> I quite agree with Mark here, I would prefer not to list register values > >> in > >> DT bindings. However, the hardware hardware diversity doesn't help us > >> abstracting LED modes. > >> > >> The following table summarizes LED usage options. > >> > >> Device Mode LED usage > >> ------------------------------------------------------------------------- > >> --- 8001 (LED[3:0]) 00 Collision / Full-Duplex / Speed / > >> Link/Activity>> > >> 01 Activity / Full-Duplex/Collision / Speed / Link > >> 10 Activity / Full-Duplex / 100Mbps Link / 10Mbps > >> Link > >> 11 Reserved > >> > >> 80[23]1 (LED[0]) 00 Link/Activity > >> 01 Link > >> 10 Reserved > >> 11 Reserved > >> > >> 80[45]1 (LED[1:0]) 00 Speed / Link/Activity > >> 01 Activity / Link > >> 10 Reserved > >> 11 Reserved > >> > >> While LED mode could be described by LED0 mode using "link-activity" or > >> "link" strings for the 80[2345]1 chips, the 8001 makes that slightly more > >> complex and shows that future chips might not conform to any scheme we > >> come up with now. > >> > >> I thus have no strong preference for a string-based mode description over > >> using an integer. However, if we keep the integer value, I wouldn't use > >> the register value directly, but would instead use the mode field value > >> as an integer in the 0-3 range. This would remove knowledge of the PHY > >> control register layout from the DT node content, and bring more > >> consistency to the values. > > > > And I realize that this is what you've done already in the implementation > > :-/ I'll send a patch to clarify the DT bindings documentation if you > > don't mind, after hearing Mark's opinion on the issue. > > Do we need strings for what is basically a single-setup post reset which it > makes no sense for the user to ever update? If it was a standard setting I would have voted to try and standardize the values. As it isn't, we probably don't. What would you think about defining macros for the LED mode values somewhere in include/dt-bindings/ ?
Hello. On 02/26/2014 02:48 PM, Ben Dooks wrote: > Add support for the led-mode property for the following PHYs > which have a single LED mode configuration value. > KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and > KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4 > to control the LED configuration. > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> [...] Missed this patch, unfortunately and now it has been merged already... doh. > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 5a8993b..0c9e434 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -148,15 +148,52 @@ static int ks8737_config_intr(struct phy_device *phydev) > return rc < 0 ? rc : 0; > } > > +static int kszphy_setup_led(struct phy_device *phydev, > + unsigned int reg, unsigned int shift) > +{ > + > + struct device *dev = &phydev->dev; > + struct device_node *of_node = dev->of_node; > + int rc, temp; > + u32 val; > + > + if (!of_node && dev->parent->of_node) > + of_node = dev->parent->of_node; > + > + if (of_property_read_u32(of_node, "micrel,led-mode", &val)) > + return 0; > + > + temp = phy_read(phydev, reg); > + if (temp < 0) > + return temp; > + > + temp &= 3 << shift; Hm, I guess you meant ~(3 << shift), else it wouldn't work as expected if the LED field is currently non-zero. Also, I think you didn't want to mask off unrelated bits. I'm surprised nobody has noticed this before... > + temp |= val << shift; > + rc = phy_write(phydev, reg, temp); > + > + return rc < 0 ? rc : 0; > +} > + > static int kszphy_config_init(struct phy_device *phydev) > { > return 0; > } > > +static int kszphy_config_init_led8041(struct phy_device *phydev) Why not ksz8041_config_init() like other names in this file? Why mention LED in the name at all? > +{ > + /* single led control, register 0x1e bits 15..14 */ > + return kszphy_setup_led(phydev, 0x1e, 14); > +} > + > static int ksz8021_config_init(struct phy_device *phydev) > { > - int rc; > const u16 val = KSZPHY_OMSO_B_CAST_OFF | KSZPHY_OMSO_RMII_OVERRIDE; > + int rc; > + > + rc = kszphy_setup_led(phydev, 0x1f, 4); > + if (rc) > + dev_err(&phydev->dev, "failed to set led mode\n"); LED. > + > phy_write(phydev, MII_KSZPHY_OMSO, val); > rc = ksz_config_flags(phydev); > return rc < 0 ? rc : 0; > @@ -166,6 +203,10 @@ static int ks8051_config_init(struct phy_device *phydev) > { > int rc; > > + rc = kszphy_setup_led(phydev, 0x1f, 4); > + if (rc) > + dev_err(&phydev->dev, "failed to set led mode\n"); LED. > + > rc = ksz_config_flags(phydev); > return rc < 0 ? rc : 0; > } WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/18/2014 11:39 PM, Sergei Shtylyov wrote: >> Add support for the led-mode property for the following PHYs >> which have a single LED mode configuration value. >> KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and >> KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4 >> to control the LED configuration. >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > [...] > Missed this patch, unfortunately and now it has been merged already... doh. Great work, BTW -- looks like you had to browse a lot of datasheets to do this patch. >> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c >> index 5a8993b..0c9e434 100644 >> --- a/drivers/net/phy/micrel.c >> +++ b/drivers/net/phy/micrel.c >> @@ -148,15 +148,52 @@ static int ks8737_config_intr(struct phy_device *phydev) >> return rc < 0 ? rc : 0; >> } >> >> +static int kszphy_setup_led(struct phy_device *phydev, >> + unsigned int reg, unsigned int shift) >> +{ >> + >> + struct device *dev = &phydev->dev; >> + struct device_node *of_node = dev->of_node; >> + int rc, temp; >> + u32 val; >> + >> + if (!of_node && dev->parent->of_node) >> + of_node = dev->parent->of_node; >> + >> + if (of_property_read_u32(of_node, "micrel,led-mode", &val)) >> + return 0; >> + >> + temp = phy_read(phydev, reg); >> + if (temp < 0) >> + return temp; >> + >> + temp &= 3 << shift; > Hm, I guess you meant ~(3 << shift), else it wouldn't work as expected if > the LED field is currently non-zero. Also, I think you didn't want to mask off > unrelated bits. I'm surprised nobody has noticed this before... OK, I'll go fix this myself. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt new file mode 100644 index 0000000..98a3e61 --- /dev/null +++ b/Documentation/devicetree/bindings/net/micrel.txt @@ -0,0 +1,18 @@ +Micrel PHY properties. + +These properties cover the base properties Micrel PHYs. + +Optional properties: + + - micrel,led-mode : LED mode value to set for PHYs with configurable LEDs. + + Configure the LED mode with single value. The list of PHYs and + the bits that are currently supported: + + KSZ8001: register 0x1e, bits 15..14 + KSZ8041: register 0x1e, bits 15..14 + KSZ8021: register 0x1f, bits 5..4 + KSZ8031: register 0x1f, bits 5..4 + KSZ8051: register 0x1f, bits 5..4 + + See the respective PHY datasheet for the mode values. diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 5a8993b..0c9e434 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -148,15 +148,52 @@ static int ks8737_config_intr(struct phy_device *phydev) return rc < 0 ? rc : 0; } +static int kszphy_setup_led(struct phy_device *phydev, + unsigned int reg, unsigned int shift) +{ + + struct device *dev = &phydev->dev; + struct device_node *of_node = dev->of_node; + int rc, temp; + u32 val; + + if (!of_node && dev->parent->of_node) + of_node = dev->parent->of_node; + + if (of_property_read_u32(of_node, "micrel,led-mode", &val)) + return 0; + + temp = phy_read(phydev, reg); + if (temp < 0) + return temp; + + temp &= 3 << shift; + temp |= val << shift; + rc = phy_write(phydev, reg, temp); + + return rc < 0 ? rc : 0; +} + static int kszphy_config_init(struct phy_device *phydev) { return 0; } +static int kszphy_config_init_led8041(struct phy_device *phydev) +{ + /* single led control, register 0x1e bits 15..14 */ + return kszphy_setup_led(phydev, 0x1e, 14); +} + static int ksz8021_config_init(struct phy_device *phydev) { - int rc; const u16 val = KSZPHY_OMSO_B_CAST_OFF | KSZPHY_OMSO_RMII_OVERRIDE; + int rc; + + rc = kszphy_setup_led(phydev, 0x1f, 4); + if (rc) + dev_err(&phydev->dev, "failed to set led mode\n"); + phy_write(phydev, MII_KSZPHY_OMSO, val); rc = ksz_config_flags(phydev); return rc < 0 ? rc : 0; @@ -166,6 +203,10 @@ static int ks8051_config_init(struct phy_device *phydev) { int rc; + rc = kszphy_setup_led(phydev, 0x1f, 4); + if (rc) + dev_err(&phydev->dev, "failed to set led mode\n"); + rc = ksz_config_flags(phydev); return rc < 0 ? rc : 0; } @@ -327,7 +368,7 @@ static struct phy_driver ksphy_driver[] = { .features = (PHY_BASIC_FEATURES | SUPPORTED_Pause | SUPPORTED_Asym_Pause), .flags = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT, - .config_init = kszphy_config_init, + .config_init = kszphy_config_init_led8041, .config_aneg = genphy_config_aneg, .read_status = genphy_read_status, .ack_interrupt = kszphy_ack_interrupt, @@ -342,7 +383,7 @@ static struct phy_driver ksphy_driver[] = { .features = PHY_BASIC_FEATURES | SUPPORTED_Pause | SUPPORTED_Asym_Pause, .flags = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT, - .config_init = kszphy_config_init, + .config_init = kszphy_config_init_led8041, .config_aneg = genphy_config_aneg, .read_status = genphy_read_status, .ack_interrupt = kszphy_ack_interrupt, @@ -371,7 +412,7 @@ static struct phy_driver ksphy_driver[] = { .phy_id_mask = 0x00ffffff, .features = (PHY_BASIC_FEATURES | SUPPORTED_Pause), .flags = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT, - .config_init = kszphy_config_init, + .config_init = kszphy_config_init_led8041, .config_aneg = genphy_config_aneg, .read_status = genphy_read_status, .ack_interrupt = kszphy_ack_interrupt,
Add support for the led-mode property for the following PHYs which have a single LED mode configuration value. KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4 to control the LED configuration. Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- Documentation/devicetree/bindings/net/micrel.txt | 18 +++++++++ drivers/net/phy/micrel.c | 49 ++++++++++++++++++++++-- 2 files changed, 63 insertions(+), 4 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/micrel.txt