diff mbox

phy: micrel: add of configuration for LED mode

Message ID 1393415280-10227-1-git-send-email-ben.dooks@codethink.co.uk
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Ben Dooks Feb. 26, 2014, 11:48 a.m. UTC 
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

Comments

David Miller Feb. 26, 2014, 10 p.m. UTC | #1 
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
Florian Fainelli Feb. 26, 2014, 10:20 p.m. UTC | #2 
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
>
Mark Rutland Feb. 27, 2014, 9:15 a.m. UTC | #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
Ben Dooks Feb. 27, 2014, 10:22 a.m. UTC | #4 
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.
Ben Dooks Feb. 27, 2014, 10:30 a.m. UTC | #5 
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.
Laurent Pinchart March 18, 2014, 3:56 p.m. UTC | #6 
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.
Laurent Pinchart March 18, 2014, 4:11 p.m. UTC | #7 
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.
Ben Dooks March 18, 2014, 4:21 p.m. UTC | #8 
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?
Laurent Pinchart March 18, 2014, 4:31 p.m. UTC | #9 
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/ ?
Sergei Shtylyov March 18, 2014, 8:39 p.m. UTC | #10 
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
Sergei Shtylyov March 18, 2014, 11:15 p.m. UTC | #11 
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 mbox

Patch

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,