[net-next,1/3] net: phy: Add binding for vendor specific C45 MDIO address space

Message ID 20180417090233.21548-2-vicentiu.galanopulo@nxp.com
State New
Headers show
Series
  • net: phy: Enable C45 vendor specific MDIO register addr space
Related show

Commit Message

Vicentiu Galanopulo April 17, 2018, 9:02 a.m.
The extra property enables the discovery on the MDIO bus
of the PHYs which have a vendor specific address space
for accessing the C45 MDIO registers.

Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>
---
 Documentation/devicetree/bindings/net/phy.txt | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Andrew Lunn April 17, 2018, 12:53 p.m. | #1
On Tue, Apr 17, 2018 at 04:02:31AM -0500, Vicentiu Galanopulo wrote:
> The extra property enables the discovery on the MDIO bus
> of the PHYs which have a vendor specific address space
> for accessing the C45 MDIO registers.
> 
> Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>

Hi Vicentiu

I think binding is O.K, but the implementation needs work. So 

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
--
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 Fainelli April 17, 2018, 6:18 p.m. | #2
On 04/17/2018 02:02 AM, Vicentiu Galanopulo wrote:
> The extra property enables the discovery on the MDIO bus
> of the PHYs which have a vendor specific address space
> for accessing the C45 MDIO registers.
> 
> Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>
> ---
>  Documentation/devicetree/bindings/net/phy.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> index d2169a5..82692e2 100644
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -61,6 +61,11 @@ Optional Properties:
>  - reset-deassert-us: Delay after the reset was deasserted in microseconds.
>    If this property is missing the delay will be skipped.
>  
> +- dev-addr: If set, it indicates the device address of the PHY to be used
> +  when accessing the C45 PHY registers over MDIO. It is used for vendor specific
> +  register space addresses that do no conform to standard address for the MDIO
> +  registers (e.g. MMD30)

Rob made that comment earlier, and I have to ask again now, why don't we
have the Clause 45 PHY binding be modified such that you have a reg
property that has #address-size = 2? This should be entirely backwards
compatible, but it would allow you to specify that device address in a
more traditional way.

> +
>  Example:
>  
>  ethernet-phy@0 {
> @@ -72,4 +77,5 @@ ethernet-phy@0 {
>  	reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
>  	reset-assert-us = <1000>;
>  	reset-deassert-us = <2000>;
> +	dev-addr = <0x1e>;
>  };
>
Andrew Lunn April 17, 2018, 6:27 p.m. | #3
On Tue, Apr 17, 2018 at 11:18:20AM -0700, Florian Fainelli wrote:
> On 04/17/2018 02:02 AM, Vicentiu Galanopulo wrote:
> > The extra property enables the discovery on the MDIO bus
> > of the PHYs which have a vendor specific address space
> > for accessing the C45 MDIO registers.
> > 
> > Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/net/phy.txt | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> > index d2169a5..82692e2 100644
> > --- a/Documentation/devicetree/bindings/net/phy.txt
> > +++ b/Documentation/devicetree/bindings/net/phy.txt
> > @@ -61,6 +61,11 @@ Optional Properties:
> >  - reset-deassert-us: Delay after the reset was deasserted in microseconds.
> >    If this property is missing the delay will be skipped.
> >  
> > +- dev-addr: If set, it indicates the device address of the PHY to be used
> > +  when accessing the C45 PHY registers over MDIO. It is used for vendor specific
> > +  register space addresses that do no conform to standard address for the MDIO
> > +  registers (e.g. MMD30)
> 
> Rob made that comment earlier, and I have to ask again now, why don't we
> have the Clause 45 PHY binding be modified such that you have a reg
> property that has #address-size = 2? This should be entirely backwards
> compatible, but it would allow you to specify that device address in a
> more traditional way.

Hi Florian

I think we might get into trouble when we have both c22 and c45 on the
same bus. Two different reg formats. I would have to try it and see to
be sure.

     Andrew
--
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 Fainelli April 17, 2018, 6:29 p.m. | #4
On 04/17/2018 11:27 AM, Andrew Lunn wrote:
> On Tue, Apr 17, 2018 at 11:18:20AM -0700, Florian Fainelli wrote:
>> On 04/17/2018 02:02 AM, Vicentiu Galanopulo wrote:
>>> The extra property enables the discovery on the MDIO bus
>>> of the PHYs which have a vendor specific address space
>>> for accessing the C45 MDIO registers.
>>>
>>> Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>
>>> ---
>>>  Documentation/devicetree/bindings/net/phy.txt | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
>>> index d2169a5..82692e2 100644
>>> --- a/Documentation/devicetree/bindings/net/phy.txt
>>> +++ b/Documentation/devicetree/bindings/net/phy.txt
>>> @@ -61,6 +61,11 @@ Optional Properties:
>>>  - reset-deassert-us: Delay after the reset was deasserted in microseconds.
>>>    If this property is missing the delay will be skipped.
>>>  
>>> +- dev-addr: If set, it indicates the device address of the PHY to be used
>>> +  when accessing the C45 PHY registers over MDIO. It is used for vendor specific
>>> +  register space addresses that do no conform to standard address for the MDIO
>>> +  registers (e.g. MMD30)
>>
>> Rob made that comment earlier, and I have to ask again now, why don't we
>> have the Clause 45 PHY binding be modified such that you have a reg
>> property that has #address-size = 2? This should be entirely backwards
>> compatible, but it would allow you to specify that device address in a
>> more traditional way.
> 
> Hi Florian
> 
> I think we might get into trouble when we have both c22 and c45 on the
> same bus. Two different reg formats. I would have to try it and see to
> be sure.

Hum indeed, we would no longer be able to mix and match on the same MDIO
bus, unless we give C22 PHYs a "fake" second cell. Disregard that idea
then, and let's stick with 'dev-addr'.
Rob Herring April 24, 2018, 1:14 p.m. | #5
On Tue, Apr 17, 2018 at 11:29:27AM -0700, Florian Fainelli wrote:
> On 04/17/2018 11:27 AM, Andrew Lunn wrote:
> > On Tue, Apr 17, 2018 at 11:18:20AM -0700, Florian Fainelli wrote:
> >> On 04/17/2018 02:02 AM, Vicentiu Galanopulo wrote:
> >>> The extra property enables the discovery on the MDIO bus
> >>> of the PHYs which have a vendor specific address space
> >>> for accessing the C45 MDIO registers.
> >>>
> >>> Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/net/phy.txt | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> >>> index d2169a5..82692e2 100644
> >>> --- a/Documentation/devicetree/bindings/net/phy.txt
> >>> +++ b/Documentation/devicetree/bindings/net/phy.txt
> >>> @@ -61,6 +61,11 @@ Optional Properties:
> >>>  - reset-deassert-us: Delay after the reset was deasserted in microseconds.
> >>>    If this property is missing the delay will be skipped.
> >>>  
> >>> +- dev-addr: If set, it indicates the device address of the PHY to be used
> >>> +  when accessing the C45 PHY registers over MDIO. It is used for vendor specific
> >>> +  register space addresses that do no conform to standard address for the MDIO
> >>> +  registers (e.g. MMD30)
> >>
> >> Rob made that comment earlier, and I have to ask again now, why don't we
> >> have the Clause 45 PHY binding be modified such that you have a reg
> >> property that has #address-size = 2? This should be entirely backwards
> >> compatible, but it would allow you to specify that device address in a
> >> more traditional way.

#address-size is not a valid property. I think #address-cells is what 
was meant here. However, the statement is still not right either. The 
number of cells is how many cells to encode an address, not how many 
addresses you have.

> > 
> > Hi Florian
> > 
> > I think we might get into trouble when we have both c22 and c45 on the
> > same bus. Two different reg formats. I would have to try it and see to
> > be sure.
>
> Hum indeed, we would no longer be able to mix and match on the same MDIO
> bus, unless we give C22 PHYs a "fake" second cell. Disregard that idea
> then, and let's stick with 'dev-addr'.

The format would be the same (1 cell for an address), you'd just have 1 
address for c22 and 2 addresses for c45 devices which is perfectly fine.

However, as I mentioned on the previous version, I'm okay with dev-addr 
given it seems to be a quirk that isn't always needed.

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

Patch

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index d2169a5..82692e2 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -61,6 +61,11 @@  Optional Properties:
 - reset-deassert-us: Delay after the reset was deasserted in microseconds.
   If this property is missing the delay will be skipped.
 
+- dev-addr: If set, it indicates the device address of the PHY to be used
+  when accessing the C45 PHY registers over MDIO. It is used for vendor specific
+  register space addresses that do no conform to standard address for the MDIO
+  registers (e.g. MMD30)
+
 Example:
 
 ethernet-phy@0 {
@@ -72,4 +77,5 @@  ethernet-phy@0 {
 	reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
 	reset-assert-us = <1000>;
 	reset-deassert-us = <2000>;
+	dev-addr = <0x1e>;
 };