diff mbox series

[v4,1/4] dt-bindings: net: phy: Add support for NXP TJA11xx

Message ID 20200313052252.25389-2-o.rempel@pengutronix.de
State Changes Requested, archived
Headers show
Series add TJA1102 support | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema success

Commit Message

Oleksij Rempel March 13, 2020, 5:22 a.m. UTC
Document the NXP TJA11xx PHY bindings.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 .../devicetree/bindings/net/nxp,tja11xx.yaml  | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/nxp,tja11xx.yaml

Comments

Florian Fainelli March 13, 2020, 6:02 p.m. UTC | #1
On 3/12/2020 10:22 PM, Oleksij Rempel wrote:
> Document the NXP TJA11xx PHY bindings.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../devicetree/bindings/net/nxp,tja11xx.yaml  | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> new file mode 100644
> index 000000000000..42be0255512b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP TJA11xx PHY
> +
> +maintainers:
> +  - Andrew Lunn <andrew@lunn.ch>
> +  - Florian Fainelli <f.fainelli@gmail.com>
> +  - Heiner Kallweit <hkallweit1@gmail.com>
> +
> +description:
> +  Bindings for NXP TJA11xx automotive PHYs
> +
> +allOf:
> +  - $ref: ethernet-phy.yaml#
> +
> +patternProperties:
> +  "^ethernet-phy@[0-9a-f]+$":
> +    type: object
> +    description: |
> +      Some packages have multiple PHYs. Secondary PHY should be defines as
> +      subnode of the first (parent) PHY.


There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
defined as 4 separate Ethernet PHY nodes and this would not be quite a
big stretch to represent them that way compared to how they are.

I would recommend doing the same thing and not bend the MDIO framework
to support the registration of "nested" Ethernet PHY nodes.
Andrew Lunn March 13, 2020, 6:10 p.m. UTC | #2
> > diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > new file mode 100644
> > index 000000000000..42be0255512b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP TJA11xx PHY
> > +
> > +maintainers:
> > +  - Andrew Lunn <andrew@lunn.ch>
> > +  - Florian Fainelli <f.fainelli@gmail.com>
> > +  - Heiner Kallweit <hkallweit1@gmail.com>
> > +
> > +description:
> > +  Bindings for NXP TJA11xx automotive PHYs
> > +
> > +allOf:
> > +  - $ref: ethernet-phy.yaml#
> > +
> > +patternProperties:
> > +  "^ethernet-phy@[0-9a-f]+$":
> > +    type: object
> > +    description: |
> > +      Some packages have multiple PHYs. Secondary PHY should be defines as
> > +      subnode of the first (parent) PHY.
> 
> 
> There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
> defined as 4 separate Ethernet PHY nodes and this would not be quite a
> big stretch to represent them that way compared to how they are.
> 
> I would recommend doing the same thing and not bend the MDIO framework
> to support the registration of "nested" Ethernet PHY nodes.

Hi Florian

The issue here is the missing PHY ID in the secondary PHY. Because of
that, the secondary does not probe in the normal way. We need the
primary to be involved to some degree. It needs to register it. What
i'm not so clear on is if it just needs to register it, or if these
sub nodes are actually needed, given the current code.

    Andrew
Oleksij Rempel March 13, 2020, 6:16 p.m. UTC | #3
On Fri, Mar 13, 2020 at 07:10:56PM +0100, Andrew Lunn wrote:
> > > diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > > new file mode 100644
> > > index 000000000000..42be0255512b
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > > @@ -0,0 +1,61 @@
> > > +# SPDX-License-Identifier: GPL-2.0+
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: NXP TJA11xx PHY
> > > +
> > > +maintainers:
> > > +  - Andrew Lunn <andrew@lunn.ch>
> > > +  - Florian Fainelli <f.fainelli@gmail.com>
> > > +  - Heiner Kallweit <hkallweit1@gmail.com>
> > > +
> > > +description:
> > > +  Bindings for NXP TJA11xx automotive PHYs
> > > +
> > > +allOf:
> > > +  - $ref: ethernet-phy.yaml#
> > > +
> > > +patternProperties:
> > > +  "^ethernet-phy@[0-9a-f]+$":
> > > +    type: object
> > > +    description: |
> > > +      Some packages have multiple PHYs. Secondary PHY should be defines as
> > > +      subnode of the first (parent) PHY.
> > 
> > 
> > There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
> > defined as 4 separate Ethernet PHY nodes and this would not be quite a
> > big stretch to represent them that way compared to how they are.
> > 
> > I would recommend doing the same thing and not bend the MDIO framework
> > to support the registration of "nested" Ethernet PHY nodes.
> 
> Hi Florian
> 
> The issue here is the missing PHY ID in the secondary PHY. Because of
> that, the secondary does not probe in the normal way. We need the
> primary to be involved to some degree. It needs to register it. What
> i'm not so clear on is if it just needs to register it, or if these
> sub nodes are actually needed, given the current code.

There are a bit more dependencies:
- PHY0 is responsible for health monitoring. If some thing wrong, it may
  shut down complete chip.
- We have shared reset. It make no sense to probe PHY1 before PHY0 with
  more controlling options will be probed
- It is possible bat dangerous to use PHY1 without PHY0.

Regards,
Oleksij
Florian Fainelli March 13, 2020, 6:20 p.m. UTC | #4
On 3/13/2020 11:16 AM, Oleksij Rempel wrote:
> On Fri, Mar 13, 2020 at 07:10:56PM +0100, Andrew Lunn wrote:
>>>> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
>>>> new file mode 100644
>>>> index 000000000000..42be0255512b
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
>>>> @@ -0,0 +1,61 @@
>>>> +# SPDX-License-Identifier: GPL-2.0+
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: NXP TJA11xx PHY
>>>> +
>>>> +maintainers:
>>>> +  - Andrew Lunn <andrew@lunn.ch>
>>>> +  - Florian Fainelli <f.fainelli@gmail.com>
>>>> +  - Heiner Kallweit <hkallweit1@gmail.com>
>>>> +
>>>> +description:
>>>> +  Bindings for NXP TJA11xx automotive PHYs
>>>> +
>>>> +allOf:
>>>> +  - $ref: ethernet-phy.yaml#
>>>> +
>>>> +patternProperties:
>>>> +  "^ethernet-phy@[0-9a-f]+$":
>>>> +    type: object
>>>> +    description: |
>>>> +      Some packages have multiple PHYs. Secondary PHY should be defines as
>>>> +      subnode of the first (parent) PHY.
>>>
>>>
>>> There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
>>> defined as 4 separate Ethernet PHY nodes and this would not be quite a
>>> big stretch to represent them that way compared to how they are.
>>>
>>> I would recommend doing the same thing and not bend the MDIO framework
>>> to support the registration of "nested" Ethernet PHY nodes.
>>
>> Hi Florian
>>
>> The issue here is the missing PHY ID in the secondary PHY. Because of
>> that, the secondary does not probe in the normal way. We need the
>> primary to be involved to some degree. It needs to register it. What
>> i'm not so clear on is if it just needs to register it, or if these
>> sub nodes are actually needed, given the current code.
> 
> There are a bit more dependencies:
> - PHY0 is responsible for health monitoring. If some thing wrong, it may
>   shut down complete chip.
> - We have shared reset. It make no sense to probe PHY1 before PHY0 with
>   more controlling options will be probed
> - It is possible bat dangerous to use PHY1 without PHY0.

probing is a software problem though. If we want to describe the PHY
package more correctly, we should be using a container node, something
like this maybe:

phy-package {
	compatible = "nxp,tja1102";

	ethernet-phy@4 {
		reg = <4>;
	};

	ethernet-phy@5 {
		reg = <5>;
	};
};
Oleksij Rempel March 13, 2020, 6:53 p.m. UTC | #5
On Fri, Mar 13, 2020 at 11:20:35AM -0700, Florian Fainelli wrote:
> 
> 
> On 3/13/2020 11:16 AM, Oleksij Rempel wrote:
> > On Fri, Mar 13, 2020 at 07:10:56PM +0100, Andrew Lunn wrote:
> >>>> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..42be0255512b
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> >>>> @@ -0,0 +1,61 @@
> >>>> +# SPDX-License-Identifier: GPL-2.0+
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: NXP TJA11xx PHY
> >>>> +
> >>>> +maintainers:
> >>>> +  - Andrew Lunn <andrew@lunn.ch>
> >>>> +  - Florian Fainelli <f.fainelli@gmail.com>
> >>>> +  - Heiner Kallweit <hkallweit1@gmail.com>
> >>>> +
> >>>> +description:
> >>>> +  Bindings for NXP TJA11xx automotive PHYs
> >>>> +
> >>>> +allOf:
> >>>> +  - $ref: ethernet-phy.yaml#
> >>>> +
> >>>> +patternProperties:
> >>>> +  "^ethernet-phy@[0-9a-f]+$":
> >>>> +    type: object
> >>>> +    description: |
> >>>> +      Some packages have multiple PHYs. Secondary PHY should be defines as
> >>>> +      subnode of the first (parent) PHY.
> >>>
> >>>
> >>> There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
> >>> defined as 4 separate Ethernet PHY nodes and this would not be quite a
> >>> big stretch to represent them that way compared to how they are.
> >>>
> >>> I would recommend doing the same thing and not bend the MDIO framework
> >>> to support the registration of "nested" Ethernet PHY nodes.
> >>
> >> Hi Florian
> >>
> >> The issue here is the missing PHY ID in the secondary PHY. Because of
> >> that, the secondary does not probe in the normal way. We need the
> >> primary to be involved to some degree. It needs to register it. What
> >> i'm not so clear on is if it just needs to register it, or if these
> >> sub nodes are actually needed, given the current code.
> > 
> > There are a bit more dependencies:
> > - PHY0 is responsible for health monitoring. If some thing wrong, it may
> >   shut down complete chip.
> > - We have shared reset. It make no sense to probe PHY1 before PHY0 with
> >   more controlling options will be probed
> > - It is possible bat dangerous to use PHY1 without PHY0.
> 
> probing is a software problem though. If we want to describe the PHY
> package more correctly, we should be using a container node, something
> like this maybe:
>
> phy-package {
> 	compatible = "nxp,tja1102";
> 
> 	ethernet-phy@4 {
> 		reg = <4>;
> 	};
> 
> 	ethernet-phy@5 {
> 		reg = <5>;
> 	};
> };

Yes, this is almost the same as it is currently done:

phy-package {
	reg = <4>;
 
 	ethernet-phy@5 {
 		reg = <5>;
 	};
};

Because the primary PHY0 can be autodetected by the bus scan.
But I have nothing against your suggestions. Please, some one should say the
last word here, how exactly it should be implemented?

Regards,
Oleksij
Oleksij Rempel March 17, 2020, 11:56 a.m. UTC | #6
On Fri, Mar 13, 2020 at 07:53:27PM +0100, Oleksij Rempel wrote:
> On Fri, Mar 13, 2020 at 11:20:35AM -0700, Florian Fainelli wrote:
> > 
> > 
> > On 3/13/2020 11:16 AM, Oleksij Rempel wrote:
> > > On Fri, Mar 13, 2020 at 07:10:56PM +0100, Andrew Lunn wrote:
> > >>>> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > >>>> new file mode 100644
> > >>>> index 000000000000..42be0255512b
> > >>>> --- /dev/null
> > >>>> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > >>>> @@ -0,0 +1,61 @@
> > >>>> +# SPDX-License-Identifier: GPL-2.0+
> > >>>> +%YAML 1.2
> > >>>> +---
> > >>>> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> > >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>>> +
> > >>>> +title: NXP TJA11xx PHY
> > >>>> +
> > >>>> +maintainers:
> > >>>> +  - Andrew Lunn <andrew@lunn.ch>
> > >>>> +  - Florian Fainelli <f.fainelli@gmail.com>
> > >>>> +  - Heiner Kallweit <hkallweit1@gmail.com>
> > >>>> +
> > >>>> +description:
> > >>>> +  Bindings for NXP TJA11xx automotive PHYs
> > >>>> +
> > >>>> +allOf:
> > >>>> +  - $ref: ethernet-phy.yaml#
> > >>>> +
> > >>>> +patternProperties:
> > >>>> +  "^ethernet-phy@[0-9a-f]+$":
> > >>>> +    type: object
> > >>>> +    description: |
> > >>>> +      Some packages have multiple PHYs. Secondary PHY should be defines as
> > >>>> +      subnode of the first (parent) PHY.
> > >>>
> > >>>
> > >>> There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
> > >>> defined as 4 separate Ethernet PHY nodes and this would not be quite a
> > >>> big stretch to represent them that way compared to how they are.
> > >>>
> > >>> I would recommend doing the same thing and not bend the MDIO framework
> > >>> to support the registration of "nested" Ethernet PHY nodes.
> > >>
> > >> Hi Florian
> > >>
> > >> The issue here is the missing PHY ID in the secondary PHY. Because of
> > >> that, the secondary does not probe in the normal way. We need the
> > >> primary to be involved to some degree. It needs to register it. What
> > >> i'm not so clear on is if it just needs to register it, or if these
> > >> sub nodes are actually needed, given the current code.
> > > 
> > > There are a bit more dependencies:
> > > - PHY0 is responsible for health monitoring. If some thing wrong, it may
> > >   shut down complete chip.
> > > - We have shared reset. It make no sense to probe PHY1 before PHY0 with
> > >   more controlling options will be probed
> > > - It is possible bat dangerous to use PHY1 without PHY0.
> > 
> > probing is a software problem though. If we want to describe the PHY
> > package more correctly, we should be using a container node, something
> > like this maybe:
> >
> > phy-package {
> > 	compatible = "nxp,tja1102";
> > 
> > 	ethernet-phy@4 {
> > 		reg = <4>;
> > 	};
> > 
> > 	ethernet-phy@5 {
> > 		reg = <5>;
> > 	};
> > };
> 
> Yes, this is almost the same as it is currently done:
> 
> phy-package {
> 	reg = <4>;
>  
>  	ethernet-phy@5 {
>  		reg = <5>;
>  	};
> };
> 
> Because the primary PHY0 can be autodetected by the bus scan.
> But I have nothing against your suggestions. Please, some one should say the
> last word here, how exactly it should be implemented?

ping,

Regards,
Oleksij
Florian Fainelli March 17, 2020, 7:48 p.m. UTC | #7
On 3/17/2020 4:56 AM, Oleksij Rempel wrote:
> On Fri, Mar 13, 2020 at 07:53:27PM +0100, Oleksij Rempel wrote:
>> On Fri, Mar 13, 2020 at 11:20:35AM -0700, Florian Fainelli wrote:
>>>
>>>
>>> On 3/13/2020 11:16 AM, Oleksij Rempel wrote:
>>>> On Fri, Mar 13, 2020 at 07:10:56PM +0100, Andrew Lunn wrote:
>>>>>>> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..42be0255512b
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
>>>>>>> @@ -0,0 +1,61 @@
>>>>>>> +# SPDX-License-Identifier: GPL-2.0+
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: NXP TJA11xx PHY
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Andrew Lunn <andrew@lunn.ch>
>>>>>>> +  - Florian Fainelli <f.fainelli@gmail.com>
>>>>>>> +  - Heiner Kallweit <hkallweit1@gmail.com>
>>>>>>> +
>>>>>>> +description:
>>>>>>> +  Bindings for NXP TJA11xx automotive PHYs
>>>>>>> +
>>>>>>> +allOf:
>>>>>>> +  - $ref: ethernet-phy.yaml#
>>>>>>> +
>>>>>>> +patternProperties:
>>>>>>> +  "^ethernet-phy@[0-9a-f]+$":
>>>>>>> +    type: object
>>>>>>> +    description: |
>>>>>>> +      Some packages have multiple PHYs. Secondary PHY should be defines as
>>>>>>> +      subnode of the first (parent) PHY.
>>>>>>
>>>>>>
>>>>>> There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
>>>>>> defined as 4 separate Ethernet PHY nodes and this would not be quite a
>>>>>> big stretch to represent them that way compared to how they are.
>>>>>>
>>>>>> I would recommend doing the same thing and not bend the MDIO framework
>>>>>> to support the registration of "nested" Ethernet PHY nodes.
>>>>>
>>>>> Hi Florian
>>>>>
>>>>> The issue here is the missing PHY ID in the secondary PHY. Because of
>>>>> that, the secondary does not probe in the normal way. We need the
>>>>> primary to be involved to some degree. It needs to register it. What
>>>>> i'm not so clear on is if it just needs to register it, or if these
>>>>> sub nodes are actually needed, given the current code.
>>>>
>>>> There are a bit more dependencies:
>>>> - PHY0 is responsible for health monitoring. If some thing wrong, it may
>>>>   shut down complete chip.
>>>> - We have shared reset. It make no sense to probe PHY1 before PHY0 with
>>>>   more controlling options will be probed
>>>> - It is possible bat dangerous to use PHY1 without PHY0.
>>>
>>> probing is a software problem though. If we want to describe the PHY
>>> package more correctly, we should be using a container node, something
>>> like this maybe:
>>>
>>> phy-package {
>>> 	compatible = "nxp,tja1102";
>>>
>>> 	ethernet-phy@4 {
>>> 		reg = <4>;
>>> 	};
>>>
>>> 	ethernet-phy@5 {
>>> 		reg = <5>;
>>> 	};
>>> };
>>
>> Yes, this is almost the same as it is currently done:
>>
>> phy-package {
>> 	reg = <4>;
>>  
>>  	ethernet-phy@5 {
>>  		reg = <5>;
>>  	};
>> };
>>
>> Because the primary PHY0 can be autodetected by the bus scan.
>> But I have nothing against your suggestions. Please, some one should say the
>> last word here, how exactly it should be implemented?

It's not for me to decide, I was hoping the Device Tree maintainers
could chime in, your current approach would certainly work although it
feels visually awkward.
Rob Herring (Arm) March 20, 2020, 11:05 p.m. UTC | #8
On Tue, Mar 17, 2020 at 12:48:47PM -0700, Florian Fainelli wrote:
> 
> 
> On 3/17/2020 4:56 AM, Oleksij Rempel wrote:
> > On Fri, Mar 13, 2020 at 07:53:27PM +0100, Oleksij Rempel wrote:
> >> On Fri, Mar 13, 2020 at 11:20:35AM -0700, Florian Fainelli wrote:
> >>>
> >>>
> >>> On 3/13/2020 11:16 AM, Oleksij Rempel wrote:
> >>>> On Fri, Mar 13, 2020 at 07:10:56PM +0100, Andrew Lunn wrote:
> >>>>>>> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> >>>>>>> new file mode 100644
> >>>>>>> index 000000000000..42be0255512b
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> >>>>>>> @@ -0,0 +1,61 @@
> >>>>>>> +# SPDX-License-Identifier: GPL-2.0+
> >>>>>>> +%YAML 1.2
> >>>>>>> +---
> >>>>>>> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>> +
> >>>>>>> +title: NXP TJA11xx PHY
> >>>>>>> +
> >>>>>>> +maintainers:
> >>>>>>> +  - Andrew Lunn <andrew@lunn.ch>
> >>>>>>> +  - Florian Fainelli <f.fainelli@gmail.com>
> >>>>>>> +  - Heiner Kallweit <hkallweit1@gmail.com>
> >>>>>>> +
> >>>>>>> +description:
> >>>>>>> +  Bindings for NXP TJA11xx automotive PHYs
> >>>>>>> +
> >>>>>>> +allOf:
> >>>>>>> +  - $ref: ethernet-phy.yaml#
> >>>>>>> +
> >>>>>>> +patternProperties:
> >>>>>>> +  "^ethernet-phy@[0-9a-f]+$":
> >>>>>>> +    type: object
> >>>>>>> +    description: |
> >>>>>>> +      Some packages have multiple PHYs. Secondary PHY should be defines as
> >>>>>>> +      subnode of the first (parent) PHY.
> >>>>>>
> >>>>>>
> >>>>>> There are QSGMII PHYs which have 4 PHYs embedded and AFAICT they are
> >>>>>> defined as 4 separate Ethernet PHY nodes and this would not be quite a
> >>>>>> big stretch to represent them that way compared to how they are.
> >>>>>>
> >>>>>> I would recommend doing the same thing and not bend the MDIO framework
> >>>>>> to support the registration of "nested" Ethernet PHY nodes.
> >>>>>
> >>>>> Hi Florian
> >>>>>
> >>>>> The issue here is the missing PHY ID in the secondary PHY. Because of
> >>>>> that, the secondary does not probe in the normal way. We need the
> >>>>> primary to be involved to some degree. It needs to register it. What
> >>>>> i'm not so clear on is if it just needs to register it, or if these
> >>>>> sub nodes are actually needed, given the current code.
> >>>>
> >>>> There are a bit more dependencies:
> >>>> - PHY0 is responsible for health monitoring. If some thing wrong, it may
> >>>>   shut down complete chip.
> >>>> - We have shared reset. It make no sense to probe PHY1 before PHY0 with
> >>>>   more controlling options will be probed
> >>>> - It is possible bat dangerous to use PHY1 without PHY0.
> >>>
> >>> probing is a software problem though. If we want to describe the PHY
> >>> package more correctly, we should be using a container node, something
> >>> like this maybe:
> >>>
> >>> phy-package {
> >>> 	compatible = "nxp,tja1102";
> >>>
> >>> 	ethernet-phy@4 {
> >>> 		reg = <4>;
> >>> 	};
> >>>
> >>> 	ethernet-phy@5 {
> >>> 		reg = <5>;
> >>> 	};
> >>> };
> >>
> >> Yes, this is almost the same as it is currently done:
> >>
> >> phy-package {
> >> 	reg = <4>;
> >>  
> >>  	ethernet-phy@5 {
> >>  		reg = <5>;
> >>  	};
> >> };
> >>
> >> Because the primary PHY0 can be autodetected by the bus scan.
> >> But I have nothing against your suggestions. Please, some one should say the
> >> last word here, how exactly it should be implemented?
> 
> It's not for me to decide, I was hoping the Device Tree maintainers
> could chime in, your current approach would certainly work although it
> feels visually awkward.

Something like this is what I'd do:

ethernet-phy@4 {
  compatible = "nxp,tja1102";
  reg = <4 5>;
};

Rob
Florian Fainelli March 22, 2020, 9:09 p.m. UTC | #9
On 3/20/2020 4:05 PM, Rob Herring wrote:
>>>> Because the primary PHY0 can be autodetected by the bus scan.
>>>> But I have nothing against your suggestions. Please, some one should say the
>>>> last word here, how exactly it should be implemented?
>>
>> It's not for me to decide, I was hoping the Device Tree maintainers
>> could chime in, your current approach would certainly work although it
>> feels visually awkward.
> 
> Something like this is what I'd do:
> 
> ethernet-phy@4 {
>   compatible = "nxp,tja1102";
>   reg = <4 5>;
> };

But the parent (MDIO bus controller) has #address-cells = 1 and
#size-cells = 0, so how can this be made to work without creating two
nodes or a first node encapsulating another one?
Rob Herring (Arm) March 23, 2020, 2:20 p.m. UTC | #10
On Sun, Mar 22, 2020 at 3:09 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 3/20/2020 4:05 PM, Rob Herring wrote:
> >>>> Because the primary PHY0 can be autodetected by the bus scan.
> >>>> But I have nothing against your suggestions. Please, some one should say the
> >>>> last word here, how exactly it should be implemented?
> >>
> >> It's not for me to decide, I was hoping the Device Tree maintainers
> >> could chime in, your current approach would certainly work although it
> >> feels visually awkward.
> >
> > Something like this is what I'd do:
> >
> > ethernet-phy@4 {
> >   compatible = "nxp,tja1102";
> >   reg = <4 5>;
> > };
>
> But the parent (MDIO bus controller) has #address-cells = 1 and
> #size-cells = 0, so how can this be made to work without creating two
> nodes or a first node encapsulating another one?

That is the size of the address, not how many addresses there are. If
the device has 2 addresses, then 2 address entries seems entirely
appropriate.

Rob
Rob Herring April 28, 2020, 5:30 p.m. UTC | #11
On Fri, Mar 13, 2020 at 12:23 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> Document the NXP TJA11xx PHY bindings.

Given the discussion, I'd marked this one as "changes requested"
expecting a new version to review the schema. And gmail decided to
make a new thread due to the extra 'RE:'. So it fell off my radar.

This schema is fundamentally broken as there's no way to match for
when to apply this schema. How do we find a NXP TJA11xx PHY? I suppose
we can look for 'ethernet-phy' with a child node 'ethernet-phy', but
then that would apply to any phy like this one. This needs a
compatible string IMO given it is non-standard.

>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../devicetree/bindings/net/nxp,tja11xx.yaml  | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> new file mode 100644
> index 000000000000..42be0255512b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: GPL-2.0+

Dual license new bindings:

(GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP TJA11xx PHY
> +
> +maintainers:
> +  - Andrew Lunn <andrew@lunn.ch>
> +  - Florian Fainelli <f.fainelli@gmail.com>
> +  - Heiner Kallweit <hkallweit1@gmail.com>
> +
> +description:
> +  Bindings for NXP TJA11xx automotive PHYs

Perhaps some information about how this phy is special.

> +
> +allOf:
> +  - $ref: ethernet-phy.yaml#

Not needed here as ethernet-phy.yaml already has a 'select' condition to apply.

> +
> +patternProperties:
> +  "^ethernet-phy@[0-9a-f]+$":
> +    type: object
> +    description: |
> +      Some packages have multiple PHYs. Secondary PHY should be defines as
> +      subnode of the first (parent) PHY.
> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 31
> +        description:
> +          The ID number for the child PHY. Should be +1 of parent PHY.
> +
> +    required:
> +      - reg
> +
> +examples:
> +  - |
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        tja1101_phy0: ethernet-phy@4 {
> +            reg = <0x4>;
> +        };
> +    };
> +  - |
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        tja1102_phy0: ethernet-phy@4 {
> +            reg = <0x4>;

> +            #address-cells = <1>;
> +            #size-cells = <0>;

These aren't documented.

> +
> +            tja1102_phy1: ethernet-phy@5 {
> +                reg = <0x5>;
> +            };
> +        };
> +    };
> --
> 2.25.1
>
Oleksij Rempel April 29, 2020, 4:38 a.m. UTC | #12
@Rob, thank you for the review.

@David, should I send fixes or reworked initial patches?

On Tue, Apr 28, 2020 at 12:30:06PM -0500, Rob Herring wrote:
> On Fri, Mar 13, 2020 at 12:23 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> >
> > Document the NXP TJA11xx PHY bindings.
> 
> Given the discussion, I'd marked this one as "changes requested"
> expecting a new version to review the schema. And gmail decided to
> make a new thread due to the extra 'RE:'. So it fell off my radar.
> 
> This schema is fundamentally broken as there's no way to match for
> when to apply this schema. How do we find a NXP TJA11xx PHY? I suppose
> we can look for 'ethernet-phy' with a child node 'ethernet-phy', but
> then that would apply to any phy like this one. This needs a
> compatible string IMO given it is non-standard.
> 
> >
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  .../devicetree/bindings/net/nxp,tja11xx.yaml  | 61 +++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > new file mode 100644
> > index 000000000000..42be0255512b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> 
> Dual license new bindings:
> 
> (GPL-2.0-only OR BSD-2-Clause)
> 
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP TJA11xx PHY
> > +
> > +maintainers:
> > +  - Andrew Lunn <andrew@lunn.ch>
> > +  - Florian Fainelli <f.fainelli@gmail.com>
> > +  - Heiner Kallweit <hkallweit1@gmail.com>
> > +
> > +description:
> > +  Bindings for NXP TJA11xx automotive PHYs
> 
> Perhaps some information about how this phy is special.
> 
> > +
> > +allOf:
> > +  - $ref: ethernet-phy.yaml#
> 
> Not needed here as ethernet-phy.yaml already has a 'select' condition to apply.
> 
> > +
> > +patternProperties:
> > +  "^ethernet-phy@[0-9a-f]+$":
> > +    type: object
> > +    description: |
> > +      Some packages have multiple PHYs. Secondary PHY should be defines as
> > +      subnode of the first (parent) PHY.
> > +
> > +    properties:
> > +      reg:
> > +        minimum: 0
> > +        maximum: 31
> > +        description:
> > +          The ID number for the child PHY. Should be +1 of parent PHY.
> > +
> > +    required:
> > +      - reg
> > +
> > +examples:
> > +  - |
> > +    mdio {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        tja1101_phy0: ethernet-phy@4 {
> > +            reg = <0x4>;
> > +        };
> > +    };
> > +  - |
> > +    mdio {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        tja1102_phy0: ethernet-phy@4 {
> > +            reg = <0x4>;
> 
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> 
> These aren't documented.
> 
> > +
> > +            tja1102_phy1: ethernet-phy@5 {
> > +                reg = <0x5>;
> > +            };
> > +        };
> > +    };
> > --
> > 2.25.1
> >
>
Florian Fainelli April 29, 2020, 4:45 a.m. UTC | #13
On 4/28/2020 9:38 PM, Oleksij Rempel wrote:
> @Rob, thank you for the review.
> 
> @David, should I send fixes or reworked initial patches?

You need to send incremental patches, once David applies the patches,
they are part of the git history for the trees he maintains.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
new file mode 100644
index 000000000000..42be0255512b
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
@@ -0,0 +1,61 @@ 
+# SPDX-License-Identifier: GPL-2.0+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/nxp,tja11xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP TJA11xx PHY
+
+maintainers:
+  - Andrew Lunn <andrew@lunn.ch>
+  - Florian Fainelli <f.fainelli@gmail.com>
+  - Heiner Kallweit <hkallweit1@gmail.com>
+
+description:
+  Bindings for NXP TJA11xx automotive PHYs
+
+allOf:
+  - $ref: ethernet-phy.yaml#
+
+patternProperties:
+  "^ethernet-phy@[0-9a-f]+$":
+    type: object
+    description: |
+      Some packages have multiple PHYs. Secondary PHY should be defines as
+      subnode of the first (parent) PHY.
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 31
+        description:
+          The ID number for the child PHY. Should be +1 of parent PHY.
+
+    required:
+      - reg
+
+examples:
+  - |
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        tja1101_phy0: ethernet-phy@4 {
+            reg = <0x4>;
+        };
+    };
+  - |
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        tja1102_phy0: ethernet-phy@4 {
+            reg = <0x4>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            tja1102_phy1: ethernet-phy@5 {
+                reg = <0x5>;
+            };
+        };
+    };