* [PATCH v4 0/2] Add support for QSGMII mode @ 2022-09-01 8:55 Siddharth Vadapalli 2022-09-01 8:55 ` [PATCH v4 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 Siddharth Vadapalli 2022-09-01 8:55 ` [PATCH v4 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200 Siddharth Vadapalli 0 siblings, 2 replies; 9+ messages in thread From: Siddharth Vadapalli @ 2022-09-01 8:55 UTC (permalink / raw) To: robh+dt, lee.jones, krzysztof.kozlowski, krzysztof.kozlowski+dt, kishon, vkoul, dan.carpenter, grygorii.strashko, rogerq Cc: devicetree, linux-kernel, linux-phy, s-vadapalli Add compatible for J7200 CPSW5G. Add support for QSGMII mode in phy-gmii-sel driver for CPSW5G in J7200. Change log: v3 -> v4: 1. Update $ref to /schemas/phy/phy-provider.yaml in Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml. 2. Update commit message for the "phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200" patch, describing the reason for defining the property "ti,qsgmii-main-ports" as an array. 3. Add a check in drivers/phy/ti/phy-gmii-sel.c to ensure that the value of the variable "main_ports" is within bounds. If the property "ti,qsgmii-main-ports" is either not mentioned in the devicetree or the value of the property is out of bounds, in both these cases, "main_ports" defaults to 1. 4. Use the function "of_property_read_u32()" instead of the function "of_property_read_u32_array()" in drivers/phy/ti/phy-gmii-sel.c. v2 -> v3: 1. Add $ref to "phy@[0-9a-f]+$" pattern property in Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml. 2. Restrict the optional ti,qsgmii-main-ports property to ti,j7200-cpsw5g-phy-gmii-sel property by adding an else statement and disallowing it for other compatibles. 3. Move the "items" constraint for the ti,qsgmii-main-ports property to the place the property is defined. v1 -> v2: 1. Rename ti,enet-ctrl-qsgmii as ti,qsgmii-main-ports. 2. Change ti,qsgmii-main-ports property from bitmask to integer. 3. Update commit message with property name as ti,qsgmii-main-ports. v3: https://lore.kernel.org/r/20220822065631.27933-1-s-vadapalli@ti.com/ v2: https://lore.kernel.org/r/20220816055848.111482-1-s-vadapalli@ti.com/ v1: https://lore.kernel.org/r/20220531111221.22963-1-s-vadapalli@ti.com/ Siddharth Vadapalli (2): dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200 .../mfd/ti,j721e-system-controller.yaml | 6 +++ .../bindings/phy/ti,phy-gmii-sel.yaml | 30 +++++++++++- drivers/phy/ti/phy-gmii-sel.c | 47 +++++++++++++++++-- 3 files changed, 79 insertions(+), 4 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-09-01 8:55 [PATCH v4 0/2] Add support for QSGMII mode Siddharth Vadapalli @ 2022-09-01 8:55 ` Siddharth Vadapalli 2022-09-01 15:21 ` Krzysztof Kozlowski 2022-09-01 8:55 ` [PATCH v4 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200 Siddharth Vadapalli 1 sibling, 1 reply; 9+ messages in thread From: Siddharth Vadapalli @ 2022-09-01 8:55 UTC (permalink / raw) To: robh+dt, lee.jones, krzysztof.kozlowski, krzysztof.kozlowski+dt, kishon, vkoul, dan.carpenter, grygorii.strashko, rogerq Cc: devicetree, linux-kernel, linux-phy, s-vadapalli TI's J7200 SoC supports additional PHY modes like QSGMII and SGMII that are not supported on earlier SoCs. Add a compatible for it. Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- .../mfd/ti,j721e-system-controller.yaml | 6 ++++ .../bindings/phy/ti,phy-gmii-sel.yaml | 30 ++++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml index 1aeac43cad92..802374e7645f 100644 --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml @@ -54,6 +54,12 @@ patternProperties: description: Clock provider for TI EHRPWM nodes. + "phy@[0-9a-f]+$": + type: object + $ref: /schemas/phy/phy-provider.yaml + description: + This is the register to set phy mode through phy-gmii-sel driver. + required: - compatible - reg diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml index ff8a6d9eb153..0ffb97f1a77c 100644 --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml @@ -53,12 +53,24 @@ properties: - ti,am43xx-phy-gmii-sel - ti,dm814-phy-gmii-sel - ti,am654-phy-gmii-sel + - ti,j7200-cpsw5g-phy-gmii-sel reg: maxItems: 1 '#phy-cells': true + ti,qsgmii-main-ports: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: | + Required only for QSGMII mode. Array to select the port for + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB + ports automatically. Any one of the 4 CPSW5G ports can act as the + main port with the rest of them being the QSGMII_SUB ports. + items: + minimum: 1 + maximum: 4 + allOf: - if: properties: @@ -73,6 +85,22 @@ allOf: '#phy-cells': const: 1 description: CPSW port number (starting from 1) + - if: + properties: + compatible: + contains: + enum: + - ti,j7200-cpsw5g-phy-gmii-sel + then: + properties: + '#phy-cells': + const: 1 + description: CPSW port number (starting from 1) + ti,qsgmii-main-ports: + maxItems: 1 + else: + properties: + ti,qsgmii-main-ports: false - if: properties: compatible: @@ -97,7 +125,7 @@ additionalProperties: false examples: - | - phy_gmii_sel: phy-gmii-sel@650 { + phy_gmii_sel: phy@650 { compatible = "ti,am3352-phy-gmii-sel"; reg = <0x650 0x4>; #phy-cells = <2>; -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-09-01 8:55 ` [PATCH v4 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 Siddharth Vadapalli @ 2022-09-01 15:21 ` Krzysztof Kozlowski 2022-09-02 6:09 ` Siddharth Vadapalli 0 siblings, 1 reply; 9+ messages in thread From: Krzysztof Kozlowski @ 2022-09-01 15:21 UTC (permalink / raw) To: Siddharth Vadapalli, robh+dt, lee.jones, krzysztof.kozlowski+dt, kishon, vkoul, dan.carpenter, grygorii.strashko, rogerq Cc: devicetree, linux-kernel, linux-phy On 01/09/2022 11:55, Siddharth Vadapalli wrote: > TI's J7200 SoC supports additional PHY modes like QSGMII and SGMII > that are not supported on earlier SoCs. Add a compatible for it. > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > .../mfd/ti,j721e-system-controller.yaml | 6 ++++ > .../bindings/phy/ti,phy-gmii-sel.yaml | 30 ++++++++++++++++++- > 2 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml > index 1aeac43cad92..802374e7645f 100644 > --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml > +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml > @@ -54,6 +54,12 @@ patternProperties: > description: > Clock provider for TI EHRPWM nodes. > > + "phy@[0-9a-f]+$": > + type: object > + $ref: /schemas/phy/phy-provider.yaml You need instead ref to specific device bindings/schema. Probably to /schemas/phy/ti,phy-gmii-sel.yaml# This was entirely different in v3, so your change is very confusing. > + description: > + This is the register to set phy mode through phy-gmii-sel driver. I don't understand the description. Please focus on the hardware not some drivers - what is here? Phy for something? > + > required: > - compatible > - reg > diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml > index ff8a6d9eb153..0ffb97f1a77c 100644 > --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml > +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml > @@ -53,12 +53,24 @@ properties: > - ti,am43xx-phy-gmii-sel > - ti,dm814-phy-gmii-sel > - ti,am654-phy-gmii-sel > + - ti,j7200-cpsw5g-phy-gmii-sel > > reg: > maxItems: 1 > > '#phy-cells': true > > + ti,qsgmii-main-ports: > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: | > + Required only for QSGMII mode. Array to select the port for > + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB > + ports automatically. Any one of the 4 CPSW5G ports can act as the > + main port with the rest of them being the QSGMII_SUB ports. > + items: > + minimum: 1 > + maximum: 4 > + > allOf: > - if: > properties: > @@ -73,6 +85,22 @@ allOf: > '#phy-cells': > const: 1 > description: CPSW port number (starting from 1) Blank line > + - if: > + properties: > + compatible: > + contains: > + enum: > + - ti,j7200-cpsw5g-phy-gmii-sel > + then: > + properties: > + '#phy-cells': > + const: 1 > + description: CPSW port number (starting from 1) > + ti,qsgmii-main-ports: > + maxItems: 1 It does not really make sense to limit items here, in the context of this patch. You got a comment for it already. Your patch should make sense on its own. > + else: > + properties: > + ti,qsgmii-main-ports: false Blank line > - if: > properties: > compatible: > @@ -97,7 +125,7 @@ additionalProperties: false > > examples: > - | > - phy_gmii_sel: phy-gmii-sel@650 { > + phy_gmii_sel: phy@650 { Split cleanup into separate patch. > compatible = "ti,am3352-phy-gmii-sel"; > reg = <0x650 0x4>; > #phy-cells = <2>; Best regards, Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-09-01 15:21 ` Krzysztof Kozlowski @ 2022-09-02 6:09 ` Siddharth Vadapalli 2022-09-05 13:09 ` Krzysztof Kozlowski 0 siblings, 1 reply; 9+ messages in thread From: Siddharth Vadapalli @ 2022-09-02 6:09 UTC (permalink / raw) To: Krzysztof Kozlowski, krzysztof.kozlowski+dt Cc: robh+dt, lee.jones, kishon, vkoul, dan.carpenter, grygorii.strashko, rogerq, devicetree, linux-kernel, linux-phy, s-vadapalli Hello Krzysztof, On 01/09/22 20:51, Krzysztof Kozlowski wrote: > On 01/09/2022 11:55, Siddharth Vadapalli wrote: >> TI's J7200 SoC supports additional PHY modes like QSGMII and SGMII >> that are not supported on earlier SoCs. Add a compatible for it. >> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >> --- >> .../mfd/ti,j721e-system-controller.yaml | 6 ++++ >> .../bindings/phy/ti,phy-gmii-sel.yaml | 30 ++++++++++++++++++- >> 2 files changed, 35 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >> index 1aeac43cad92..802374e7645f 100644 >> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >> @@ -54,6 +54,12 @@ patternProperties: >> description: >> Clock provider for TI EHRPWM nodes. >> >> + "phy@[0-9a-f]+$": >> + type: object >> + $ref: /schemas/phy/phy-provider.yaml > > You need instead ref to specific device bindings/schema. Probably to > /schemas/phy/ti,phy-gmii-sel.yaml# Thank you for the clarification. I will update $ref to "/schemas/phy/ti,phy-gmii-sel.yaml#" in the v5 series. > > This was entirely different in v3, so your change is very confusing. I had misunderstood Rob's comment in the v3 patch. I had initially provided the relative path to the bindings file ti,phy-gmii-sel.yaml in the v3 patch. When Rob commented "/schemas/phy/..", I misunderstood and thought that I had to point $ref to a generic phy-provider schema present in the dt-schema repo and thus, in this v4 patch, I had updated $ref accordingly. > >> + description: >> + This is the register to set phy mode through phy-gmii-sel driver. > > I don't understand the description. Please focus on the hardware not > some drivers - what is here? Phy for something? I will fix the description, updating it to the following: "Address of the CTRLMMR_ENETx_CTRL registers which are used to configure the phy-mode of the CPSW MAC ports." Please let me know if the above description is fine. > >> + >> required: >> - compatible >> - reg >> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >> index ff8a6d9eb153..0ffb97f1a77c 100644 >> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >> @@ -53,12 +53,24 @@ properties: >> - ti,am43xx-phy-gmii-sel >> - ti,dm814-phy-gmii-sel >> - ti,am654-phy-gmii-sel >> + - ti,j7200-cpsw5g-phy-gmii-sel >> >> reg: >> maxItems: 1 >> >> '#phy-cells': true >> >> + ti,qsgmii-main-ports: >> + $ref: /schemas/types.yaml#/definitions/uint32-array >> + description: | >> + Required only for QSGMII mode. Array to select the port for >> + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB >> + ports automatically. Any one of the 4 CPSW5G ports can act as the >> + main port with the rest of them being the QSGMII_SUB ports. >> + items: >> + minimum: 1 >> + maximum: 4 >> + >> allOf: >> - if: >> properties: >> @@ -73,6 +85,22 @@ allOf: >> '#phy-cells': >> const: 1 >> description: CPSW port number (starting from 1) > > Blank line I will fix this in the v5 series. > > >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - ti,j7200-cpsw5g-phy-gmii-sel >> + then: >> + properties: >> + '#phy-cells': >> + const: 1 >> + description: CPSW port number (starting from 1) >> + ti,qsgmii-main-ports: >> + maxItems: 1 > > It does not really make sense to limit items here, in the context of > this patch. You got a comment for it already. Your patch should make > sense on its own. I had defined the property as an array because there are more than one QSGMII main ports for other devices for which I will be posting the patches. I had planned to reuse this property, with "maxItems: 2" in the future patches for other compatibles. However, as suggested by you, I will change the property to a uint32 instead of uint32-array in this series. Later, in my future patches for other devices, I will change it back to a uint32-array when I reuse the property. > >> + else: >> + properties: >> + ti,qsgmii-main-ports: false > > Blank line I will fix this in the v5 series. > >> - if: >> properties: >> compatible: >> @@ -97,7 +125,7 @@ additionalProperties: false >> >> examples: >> - | >> - phy_gmii_sel: phy-gmii-sel@650 { >> + phy_gmii_sel: phy@650 { > > Split cleanup into separate patch. I will do so. Thank you for reviewing the patch. Regards, Siddharth. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-09-02 6:09 ` Siddharth Vadapalli @ 2022-09-05 13:09 ` Krzysztof Kozlowski 2022-09-06 5:02 ` Siddharth Vadapalli 0 siblings, 1 reply; 9+ messages in thread From: Krzysztof Kozlowski @ 2022-09-05 13:09 UTC (permalink / raw) To: Siddharth Vadapalli, krzysztof.kozlowski+dt Cc: robh+dt, lee.jones, kishon, vkoul, dan.carpenter, grygorii.strashko, rogerq, devicetree, linux-kernel, linux-phy On 02/09/2022 08:09, Siddharth Vadapalli wrote: > Hello Krzysztof, > > On 01/09/22 20:51, Krzysztof Kozlowski wrote: >> On 01/09/2022 11:55, Siddharth Vadapalli wrote: >>> TI's J7200 SoC supports additional PHY modes like QSGMII and SGMII >>> that are not supported on earlier SoCs. Add a compatible for it. >>> >>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>> --- >>> .../mfd/ti,j721e-system-controller.yaml | 6 ++++ >>> .../bindings/phy/ti,phy-gmii-sel.yaml | 30 ++++++++++++++++++- >>> 2 files changed, 35 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>> index 1aeac43cad92..802374e7645f 100644 >>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>> @@ -54,6 +54,12 @@ patternProperties: >>> description: >>> Clock provider for TI EHRPWM nodes. >>> >>> + "phy@[0-9a-f]+$": >>> + type: object >>> + $ref: /schemas/phy/phy-provider.yaml >> >> You need instead ref to specific device bindings/schema. Probably to >> /schemas/phy/ti,phy-gmii-sel.yaml# > > Thank you for the clarification. I will update $ref to > "/schemas/phy/ti,phy-gmii-sel.yaml#" in the v5 series. > >> >> This was entirely different in v3, so your change is very confusing. > > I had misunderstood Rob's comment in the v3 patch. I had initially > provided the relative path to the bindings file ti,phy-gmii-sel.yaml in > the v3 patch. When Rob commented "/schemas/phy/..", I misunderstood and > thought that I had to point $ref to a generic phy-provider schema > present in the dt-schema repo and thus, in this v4 patch, I had updated > $ref accordingly. > >> >>> + description: >>> + This is the register to set phy mode through phy-gmii-sel driver. >> >> I don't understand the description. Please focus on the hardware not >> some drivers - what is here? Phy for something? > > I will fix the description, updating it to the following: > "Address of the CTRLMMR_ENETx_CTRL registers which are used to configure > the phy-mode of the CPSW MAC ports." > > Please let me know if the above description is fine. Hm, but that's a phy node, not address of register... Isn't this a phy node representing the phy of the CPSW MAC ports? > >> >>> + >>> required: >>> - compatible >>> - reg >>> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>> index ff8a6d9eb153..0ffb97f1a77c 100644 >>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>> @@ -53,12 +53,24 @@ properties: >>> - ti,am43xx-phy-gmii-sel >>> - ti,dm814-phy-gmii-sel >>> - ti,am654-phy-gmii-sel >>> + - ti,j7200-cpsw5g-phy-gmii-sel >>> >>> reg: >>> maxItems: 1 >>> >>> '#phy-cells': true >>> >>> + ti,qsgmii-main-ports: >>> + $ref: /schemas/types.yaml#/definitions/uint32-array >>> + description: | >>> + Required only for QSGMII mode. Array to select the port for >>> + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB >>> + ports automatically. Any one of the 4 CPSW5G ports can act as the >>> + main port with the rest of them being the QSGMII_SUB ports. >>> + items: >>> + minimum: 1 >>> + maximum: 4 >>> + >>> allOf: >>> - if: >>> properties: >>> @@ -73,6 +85,22 @@ allOf: >>> '#phy-cells': >>> const: 1 >>> description: CPSW port number (starting from 1) >> >> Blank line > > I will fix this in the v5 series. > >> >> >>> + - if: >>> + properties: >>> + compatible: >>> + contains: >>> + enum: >>> + - ti,j7200-cpsw5g-phy-gmii-sel >>> + then: >>> + properties: >>> + '#phy-cells': >>> + const: 1 >>> + description: CPSW port number (starting from 1) >>> + ti,qsgmii-main-ports: >>> + maxItems: 1 >> >> It does not really make sense to limit items here, in the context of >> this patch. You got a comment for it already. Your patch should make >> sense on its own. > > I had defined the property as an array because there are more than one > QSGMII main ports for other devices for which I will be posting the > patches. I had planned to reuse this property, with "maxItems: 2" in the > future patches for other compatibles. However, as suggested by you, I > will change the property to a uint32 instead of uint32-array in this > series. Later, in my future patches for other devices, I will change it > back to a uint32-array when I reuse the property. Wait, no. You should not change the property. This should be uint32-array, because you will extend it soon, just maxItems must be defined in top-level place. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-09-05 13:09 ` Krzysztof Kozlowski @ 2022-09-06 5:02 ` Siddharth Vadapalli 2022-09-06 7:03 ` Krzysztof Kozlowski 0 siblings, 1 reply; 9+ messages in thread From: Siddharth Vadapalli @ 2022-09-06 5:02 UTC (permalink / raw) To: Krzysztof Kozlowski, krzysztof.kozlowski+dt Cc: robh+dt, lee.jones, kishon, vkoul, dan.carpenter, grygorii.strashko, rogerq, devicetree, linux-kernel, linux-phy, s-vadapalli Hello Krzysztof, On 05/09/22 18:39, Krzysztof Kozlowski wrote: > On 02/09/2022 08:09, Siddharth Vadapalli wrote: >> Hello Krzysztof, >> >> On 01/09/22 20:51, Krzysztof Kozlowski wrote: >>> On 01/09/2022 11:55, Siddharth Vadapalli wrote: >>>> TI's J7200 SoC supports additional PHY modes like QSGMII and SGMII >>>> that are not supported on earlier SoCs. Add a compatible for it. >>>> >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>> --- >>>> .../mfd/ti,j721e-system-controller.yaml | 6 ++++ >>>> .../bindings/phy/ti,phy-gmii-sel.yaml | 30 ++++++++++++++++++- >>>> 2 files changed, 35 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>> index 1aeac43cad92..802374e7645f 100644 >>>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>> @@ -54,6 +54,12 @@ patternProperties: >>>> description: >>>> Clock provider for TI EHRPWM nodes. >>>> >>>> + "phy@[0-9a-f]+$": >>>> + type: object >>>> + $ref: /schemas/phy/phy-provider.yaml >>> >>> You need instead ref to specific device bindings/schema. Probably to >>> /schemas/phy/ti,phy-gmii-sel.yaml# >> >> Thank you for the clarification. I will update $ref to >> "/schemas/phy/ti,phy-gmii-sel.yaml#" in the v5 series. >> >>> >>> This was entirely different in v3, so your change is very confusing. >> >> I had misunderstood Rob's comment in the v3 patch. I had initially >> provided the relative path to the bindings file ti,phy-gmii-sel.yaml in >> the v3 patch. When Rob commented "/schemas/phy/..", I misunderstood and >> thought that I had to point $ref to a generic phy-provider schema >> present in the dt-schema repo and thus, in this v4 patch, I had updated >> $ref accordingly. >> >>> >>>> + description: >>>> + This is the register to set phy mode through phy-gmii-sel driver. >>> >>> I don't understand the description. Please focus on the hardware not >>> some drivers - what is here? Phy for something? >> >> I will fix the description, updating it to the following: >> "Address of the CTRLMMR_ENETx_CTRL registers which are used to configure >> the phy-mode of the CPSW MAC ports." >> >> Please let me know if the above description is fine. > > Hm, but that's a phy node, not address of register... Isn't this a phy > node representing the phy of the CPSW MAC ports? Despite it being a phy node, the phy-gmii-sel driver actually uses this node to obtain the address of the CTRLMMR_ENETx_CTRL registers which correspond to the CPSW MAC configuration and are therefore unrelated to the PHY. Please let me know if my suggested description would be fine. > >> >>> >>>> + >>>> required: >>>> - compatible >>>> - reg >>>> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>> index ff8a6d9eb153..0ffb97f1a77c 100644 >>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>> @@ -53,12 +53,24 @@ properties: >>>> - ti,am43xx-phy-gmii-sel >>>> - ti,dm814-phy-gmii-sel >>>> - ti,am654-phy-gmii-sel >>>> + - ti,j7200-cpsw5g-phy-gmii-sel >>>> >>>> reg: >>>> maxItems: 1 >>>> >>>> '#phy-cells': true >>>> >>>> + ti,qsgmii-main-ports: >>>> + $ref: /schemas/types.yaml#/definitions/uint32-array >>>> + description: | >>>> + Required only for QSGMII mode. Array to select the port for >>>> + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB >>>> + ports automatically. Any one of the 4 CPSW5G ports can act as the >>>> + main port with the rest of them being the QSGMII_SUB ports. >>>> + items: >>>> + minimum: 1 >>>> + maximum: 4 >>>> + >>>> allOf: >>>> - if: >>>> properties: >>>> @@ -73,6 +85,22 @@ allOf: >>>> '#phy-cells': >>>> const: 1 >>>> description: CPSW port number (starting from 1) >>> >>> Blank line >> >> I will fix this in the v5 series. >> >>> >>> >>>> + - if: >>>> + properties: >>>> + compatible: >>>> + contains: >>>> + enum: >>>> + - ti,j7200-cpsw5g-phy-gmii-sel >>>> + then: >>>> + properties: >>>> + '#phy-cells': >>>> + const: 1 >>>> + description: CPSW port number (starting from 1) >>>> + ti,qsgmii-main-ports: >>>> + maxItems: 1 >>> >>> It does not really make sense to limit items here, in the context of >>> this patch. You got a comment for it already. Your patch should make >>> sense on its own. >> >> I had defined the property as an array because there are more than one >> QSGMII main ports for other devices for which I will be posting the >> patches. I had planned to reuse this property, with "maxItems: 2" in the >> future patches for other compatibles. However, as suggested by you, I >> will change the property to a uint32 instead of uint32-array in this >> series. Later, in my future patches for other devices, I will change it >> back to a uint32-array when I reuse the property. > > Wait, no. You should not change the property. This should be > uint32-array, because you will extend it soon, just maxItems must be > defined in top-level place. Thank you for clarifying. I will move "maxItems: 1" to the top where "ti,qsgmii-main-ports" property is first defined, and continue using the property as a uint32-array. Regards, Siddharth. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-09-06 5:02 ` Siddharth Vadapalli @ 2022-09-06 7:03 ` Krzysztof Kozlowski 2022-09-06 10:29 ` Siddharth Vadapalli 0 siblings, 1 reply; 9+ messages in thread From: Krzysztof Kozlowski @ 2022-09-06 7:03 UTC (permalink / raw) To: Siddharth Vadapalli, krzysztof.kozlowski+dt Cc: robh+dt, lee.jones, kishon, vkoul, dan.carpenter, grygorii.strashko, rogerq, devicetree, linux-kernel, linux-phy On 06/09/2022 07:02, Siddharth Vadapalli wrote: >>> >>> Please let me know if the above description is fine. >> >> Hm, but that's a phy node, not address of register... Isn't this a phy >> node representing the phy of the CPSW MAC ports? > > Despite it being a phy node, the phy-gmii-sel driver actually uses this > node to obtain the address of the CTRLMMR_ENETx_CTRL registers which > correspond to the CPSW MAC configuration and are therefore unrelated to > the PHY. Please let me know if my suggested description would be fine. Either I miss some more pieces or this is wrong design. The phy node should not be used to pass some addresses somewhere. It is used to define a device which will be instantiated (as parent is simple-mfd). If you use it only to obtain some address, not to describe child device, then this is wrong property type. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-09-06 7:03 ` Krzysztof Kozlowski @ 2022-09-06 10:29 ` Siddharth Vadapalli 0 siblings, 0 replies; 9+ messages in thread From: Siddharth Vadapalli @ 2022-09-06 10:29 UTC (permalink / raw) To: Krzysztof Kozlowski, krzysztof.kozlowski+dt Cc: robh+dt, lee.jones, kishon, vkoul, dan.carpenter, grygorii.strashko, rogerq, devicetree, linux-kernel, linux-phy, linux-arm-kernel, s-vadapalli Hello Krzysztof, On 06/09/22 12:33, Krzysztof Kozlowski wrote: > On 06/09/2022 07:02, Siddharth Vadapalli wrote: >>>> >>>> Please let me know if the above description is fine. >>> >>> Hm, but that's a phy node, not address of register... Isn't this a phy >>> node representing the phy of the CPSW MAC ports? >> >> Despite it being a phy node, the phy-gmii-sel driver actually uses this >> node to obtain the address of the CTRLMMR_ENETx_CTRL registers which >> correspond to the CPSW MAC configuration and are therefore unrelated to >> the PHY. Please let me know if my suggested description would be fine. > > Either I miss some more pieces or this is wrong design. The phy node > should not be used to pass some addresses somewhere. It is used to > define a device which will be instantiated (as parent is simple-mfd). If > you use it only to obtain some address, not to describe child device, > then this is wrong property type. Sorry for describing it incompletely, and at some places incorrectly. Yes, you were right when you initially mentioned that the phy node corresponds to the phy of the ethernet MAC ports. I had incorrectly understood the term "phy" there as the external Layer-1 ethernet phy.The phy node corresponds to the phy used by the ethernet MAC, based on the phy-mode configured. The am65-cpsw-nuss driver which is responsible for ethernet MAC, requires the ethernet MAC's phy to be configured by the phy-gmii-sel driver. Thus, the phy node corresponds to an actual phy (ethernet MAC's PHY). I plan on updating the description for the phy pattern property to the following: "The phy node corresponding to the ethernet MAC." Regards, Siddharth. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200 2022-09-01 8:55 [PATCH v4 0/2] Add support for QSGMII mode Siddharth Vadapalli 2022-09-01 8:55 ` [PATCH v4 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 Siddharth Vadapalli @ 2022-09-01 8:55 ` Siddharth Vadapalli 1 sibling, 0 replies; 9+ messages in thread From: Siddharth Vadapalli @ 2022-09-01 8:55 UTC (permalink / raw) To: robh+dt, lee.jones, krzysztof.kozlowski, krzysztof.kozlowski+dt, kishon, vkoul, dan.carpenter, grygorii.strashko, rogerq Cc: devicetree, linux-kernel, linux-phy, s-vadapalli Each of the CPSW5G ports in J7200 support additional modes like QSGMII. Add a new compatible for J7200 to support the additional modes. In TI's J7200, each of the CPSW5G ethernet interfaces can act as a QSGMII or QSGMII-SUB port. The QSGMII interface is responsible for performing auto-negotiation between the MAC and the PHY while the rest of the interfaces are designated as QSGMII-SUB interfaces, indicating that they will not be taking part in the auto-negotiation process. To indicate the interface which will serve as the main QSGMII interface, add a property "ti,qsgmii-main-ports", whose value indicates the port number of the interface which shall serve as the main QSGMII interface. The rest of the interfaces are then assigned QSGMII-SUB mode by default. The property "ti,qsgmii-main-ports" is used to configure the CTRLMMR_ENETx_CTRL register. Depending on the device, it is possible for more than one QSGMII main port to exist. Thus, the property "ti,qsgmii-main-ports" is defined as an array of values in order to reuse the property for other devices. Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- drivers/phy/ti/phy-gmii-sel.c | 47 ++++++++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c index d0ab69750c6b..0bcfd6d96b4d 100644 --- a/drivers/phy/ti/phy-gmii-sel.c +++ b/drivers/phy/ti/phy-gmii-sel.c @@ -22,6 +22,12 @@ #define AM33XX_GMII_SEL_MODE_RMII 1 #define AM33XX_GMII_SEL_MODE_RGMII 2 +/* J72xx SoC specific definitions for the CONTROL port */ +#define J72XX_GMII_SEL_MODE_QSGMII 4 +#define J72XX_GMII_SEL_MODE_QSGMII_SUB 6 + +#define PHY_GMII_PORT(n) BIT((n) - 1) + enum { PHY_GMII_SEL_PORT_MODE = 0, PHY_GMII_SEL_RGMII_ID_MODE, @@ -43,6 +49,7 @@ struct phy_gmii_sel_soc_data { u32 features; const struct reg_field (*regfields)[PHY_GMII_SEL_LAST]; bool use_of_data; + u64 extra_modes; }; struct phy_gmii_sel_priv { @@ -53,6 +60,7 @@ struct phy_gmii_sel_priv { struct phy_gmii_sel_phy_priv *if_phys; u32 num_ports; u32 reg_offset; + u32 qsgmii_main_ports; }; static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode) @@ -88,10 +96,17 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode) gmii_sel_mode = AM33XX_GMII_SEL_MODE_MII; break; + case PHY_INTERFACE_MODE_QSGMII: + if (!(soc_data->extra_modes & BIT(PHY_INTERFACE_MODE_QSGMII))) + goto unsupported; + if (if_phy->priv->qsgmii_main_ports & BIT(if_phy->id - 1)) + gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII; + else + gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII_SUB; + break; + default: - dev_warn(dev, "port%u: unsupported mode: \"%s\"\n", - if_phy->id, phy_modes(submode)); - return -EINVAL; + goto unsupported; } if_phy->phy_if_mode = submode; @@ -123,6 +138,11 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode) } return 0; + +unsupported: + dev_warn(dev, "port%u: unsupported mode: \"%s\"\n", + if_phy->id, phy_modes(submode)); + return -EINVAL; } static const @@ -188,6 +208,13 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_soc_am654 = { .regfields = phy_gmii_sel_fields_am654, }; +static const +struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = { + .use_of_data = true, + .regfields = phy_gmii_sel_fields_am654, + .extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII), +}; + static const struct of_device_id phy_gmii_sel_id_table[] = { { .compatible = "ti,am3352-phy-gmii-sel", @@ -209,6 +236,10 @@ static const struct of_device_id phy_gmii_sel_id_table[] = { .compatible = "ti,am654-phy-gmii-sel", .data = &phy_gmii_sel_soc_am654, }, + { + .compatible = "ti,j7200-cpsw5g-phy-gmii-sel", + .data = &phy_gmii_sel_cpsw5g_soc_j7200, + }, {} }; MODULE_DEVICE_TABLE(of, phy_gmii_sel_id_table); @@ -350,6 +381,7 @@ static int phy_gmii_sel_probe(struct platform_device *pdev) struct device_node *node = dev->of_node; const struct of_device_id *of_id; struct phy_gmii_sel_priv *priv; + u32 main_ports = 1; int ret; of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node); @@ -363,6 +395,15 @@ static int phy_gmii_sel_probe(struct platform_device *pdev) priv->dev = &pdev->dev; priv->soc_data = of_id->data; priv->num_ports = priv->soc_data->num_ports; + of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports); + /* + * Ensure that main_ports is within bounds. If the property + * ti,qsgmii-main-ports is not mentioned, or the value mentioned + * is out of bounds, default to 1. + */ + if (main_ports < 1 || main_ports > 4) + main_ports = 1; + priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports); priv->regmap = syscon_node_to_regmap(node->parent); if (IS_ERR(priv->regmap)) { -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-09-06 10:30 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-01 8:55 [PATCH v4 0/2] Add support for QSGMII mode Siddharth Vadapalli 2022-09-01 8:55 ` [PATCH v4 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 Siddharth Vadapalli 2022-09-01 15:21 ` Krzysztof Kozlowski 2022-09-02 6:09 ` Siddharth Vadapalli 2022-09-05 13:09 ` Krzysztof Kozlowski 2022-09-06 5:02 ` Siddharth Vadapalli 2022-09-06 7:03 ` Krzysztof Kozlowski 2022-09-06 10:29 ` Siddharth Vadapalli 2022-09-01 8:55 ` [PATCH v4 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200 Siddharth Vadapalli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).