RFC: add init-regs for phy nodes

RFC: add init-regs for phy nodes

[Ben Dooks]

I have run into an issue where the KSZ8041 does not get
initialised properly on the Renesas R8A7790 lager board.

This patch adds the ability to set arbitrary registers
on initialisation of a phy from informaiton in the fdt.

Feedback welcome.

[PATCH] net: add init-regs for of_phy support

[Ben Dooks]

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>
 
 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, &reg);
+		while (ptr != NULL) {
+			ptr = of_prop_next_u32(prop, ptr, &reg);
+			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, &reg);
+		}
+	}
+
+	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

Re: [PATCH] net: add init-regs for of_phy support

[Mark Rutland]

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, &reg);
> +		while (ptr != NULL) {
> +			ptr = of_prop_next_u32(prop, ptr, &reg);
> +			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, &reg);
> +		}
> +	}
> +
> +	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
> 

Re: [PATCH] net: add init-regs for of_phy support

[Ben Dooks]

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.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] net: add init-regs for of_phy support

[Mark Rutland]

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.

Re: [PATCH] net: add init-regs for of_phy support

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] net: add init-regs for of_phy support

[Ben Dooks]

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, &reg);
>> +		while (ptr != NULL) {
>> +			ptr = of_prop_next_u32(prop, ptr, &reg);

oops, bad rebase fixup, this should have been deleted.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] net: add init-regs for of_phy support

[Florian Fainelli]

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, &reg);
> +               while (ptr != NULL) {
> +                       ptr = of_prop_next_u32(prop, ptr, &reg);
> +                       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, &reg);
> +               }
> +       }
> +
> +       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



-- 
Florian

Re: [PATCH] net: add init-regs for of_phy support

[Ben Dooks]

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.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] net: add init-regs for of_phy support

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH] net: add init-regs for of_phy support

[Mark Rutland]

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.

Re: [PATCH] net: add init-regs for of_phy support

[David Miller]

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

Re: [PATCH] net: add init-regs for of_phy support

[Sergei Shtylyov]

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

Re: [PATCH] net: add init-regs for of_phy support

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH] net: add init-regs for of_phy support

[Sergei Shtylyov]

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

Re: [PATCH] net: add init-regs for of_phy support

[Florian Fainelli]

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
-- 
Florian

Re: [PATCH] net: add init-regs for of_phy support

[Ben Dooks]

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.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] net: add init-regs for of_phy support

[Mark Rutland]

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.

Re: [PATCH] net: add init-regs for of_phy support

[Sergei Shtylyov]

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

Re: [PATCH] net: add init-regs for of_phy support

[Mark Rutland]

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.