* [PATCH 0/2] Add support for QSGMII mode @ 2022-05-31 11:12 Siddharth Vadapalli 2022-05-31 11:12 ` [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 Siddharth Vadapalli 2022-05-31 11:12 ` [PATCH 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200 Siddharth Vadapalli 0 siblings, 2 replies; 11+ messages in thread From: Siddharth Vadapalli @ 2022-05-31 11:12 UTC (permalink / raw) To: robh+dt, lee.jones, krzysztof.kozlowski+dt, kishon, vkoul, dan.carpenter, grygorii.strashko, rogerq Cc: devicetree, linux-kernel, linux-phy, Siddharth Vadapalli Add compatible for J7200 CPSW5G. Add support for QSGMII mode in phy-gmii-sel driver for CPSW5G in J7200. 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 | 5 +++ .../bindings/phy/ti,phy-gmii-sel.yaml | 24 +++++++++++- drivers/phy/ti/phy-gmii-sel.c | 39 +++++++++++++++++-- 3 files changed, 64 insertions(+), 4 deletions(-) -- 2.36.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-05-31 11:12 [PATCH 0/2] Add support for QSGMII mode Siddharth Vadapalli @ 2022-05-31 11:12 ` Siddharth Vadapalli 2022-05-31 11:45 ` Roger Quadros 2022-05-31 11:12 ` [PATCH 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200 Siddharth Vadapalli 1 sibling, 1 reply; 11+ messages in thread From: Siddharth Vadapalli @ 2022-05-31 11:12 UTC (permalink / raw) To: robh+dt, lee.jones, krzysztof.kozlowski+dt, kishon, vkoul, dan.carpenter, grygorii.strashko, rogerq Cc: devicetree, linux-kernel, linux-phy, Siddharth 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 | 5 ++++ .../bindings/phy/ti,phy-gmii-sel.yaml | 24 ++++++++++++++++++- 2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644 --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml @@ -48,6 +48,11 @@ patternProperties: description: This is the SERDES lane control mux. + "phy@[0-9a-f]+$": + type: object + 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..7427758451e7 100644 --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml @@ -53,12 +53,21 @@ 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,enet-ctrl-qsgmii: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + Required only for QSGMII mode. Bitmask to select the port for + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB + ports automatically. Any of the 4 CPSW5G ports can act as the + main port with the rest of them being the QSGMII_SUB ports. + allOf: - if: properties: @@ -73,6 +82,19 @@ 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,enet-ctrl-qsgmii: + enum: [1, 2, 4, 8] - if: properties: compatible: @@ -97,7 +119,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.36.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-05-31 11:12 ` [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 Siddharth Vadapalli @ 2022-05-31 11:45 ` Roger Quadros 2022-06-01 6:01 ` Siddharth Vadapalli 0 siblings, 1 reply; 11+ messages in thread From: Roger Quadros @ 2022-05-31 11:45 UTC (permalink / raw) To: Siddharth Vadapalli, robh+dt, lee.jones, krzysztof.kozlowski+dt, kishon, vkoul, dan.carpenter, grygorii.strashko Cc: devicetree, linux-kernel, linux-phy Hi Siddharth, On 31/05/2022 14:12, 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 | 5 ++++ > .../bindings/phy/ti,phy-gmii-sel.yaml | 24 ++++++++++++++++++- > 2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644 > --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml > +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml > @@ -48,6 +48,11 @@ patternProperties: > description: > This is the SERDES lane control mux. > > + "phy@[0-9a-f]+$": > + type: object > + description: > + This is the register to set phy mode through phy-gmii-sel driver. > + Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all. > 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..7427758451e7 100644 > --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml > +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml > @@ -53,12 +53,21 @@ properties: > - ti,am43xx-phy-gmii-sel > - ti,dm814-phy-gmii-sel > - ti,am654-phy-gmii-sel > + - ti,j7200-cpsw5g-phy-gmii-sel Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming. > > reg: > maxItems: 1 > > '#phy-cells': true > > + ti,enet-ctrl-qsgmii: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: | > + Required only for QSGMII mode. Bitmask to select the port for > + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB > + ports automatically. Any of the 4 CPSW5G ports can act as the > + main port with the rest of them being the QSGMII_SUB ports. > + This is weird way of doing things. The Ethernet controller driver already knows which mode the port is supposed to operate. e.g. +&cpsw0_port1 { + phy-handle = <&cpsw5g_phy0>; + phy-mode = "qsgmii"; + mac-address = [00 00 00 00 00 00]; + phys = <&cpsw0_phy_gmii_sel 1>; +}; + +&cpsw0_port2 { + phy-handle = <&cpsw5g_phy1>; + phy-mode = "qsgmii-sub"; + mac-address = [00 00 00 00 00 00]; + phys = <&cpsw0_phy_gmii_sel 2>; And it can convey the mode to the PHY driver via phy_ops->set_mode. So you should be depending on that instead of adding this new property. > allOf: > - if: > properties: > @@ -73,6 +82,19 @@ 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,enet-ctrl-qsgmii: > + enum: [1, 2, 4, 8] > - if: > properties: > compatible: > @@ -97,7 +119,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>; cheers, -roger ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-05-31 11:45 ` Roger Quadros @ 2022-06-01 6:01 ` Siddharth Vadapalli 2022-06-01 9:38 ` Roger Quadros 0 siblings, 1 reply; 11+ messages in thread From: Siddharth Vadapalli @ 2022-06-01 6:01 UTC (permalink / raw) To: Roger Quadros, robh+dt, lee.jones, krzysztof.kozlowski+dt, kishon, vkoul, dan.carpenter, grygorii.strashko Cc: devicetree, linux-kernel, linux-phy Hello Roger, On 31/05/22 17:15, Roger Quadros wrote: > Hi Siddharth, > > On 31/05/2022 14:12, 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 | 5 ++++ >> .../bindings/phy/ti,phy-gmii-sel.yaml | 24 ++++++++++++++++++- >> 2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644 >> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >> @@ -48,6 +48,11 @@ patternProperties: >> description: >> This is the SERDES lane control mux. >> >> + "phy@[0-9a-f]+$": >> + type: object >> + description: >> + This is the register to set phy mode through phy-gmii-sel driver. >> + > > Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all. The property has to be mentioned in order to pass: make dtbs_check. > >> 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..7427758451e7 100644 >> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >> @@ -53,12 +53,21 @@ properties: >> - ti,am43xx-phy-gmii-sel >> - ti,dm814-phy-gmii-sel >> - ti,am654-phy-gmii-sel >> + - ti,j7200-cpsw5g-phy-gmii-sel > > Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming. In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name of the compatible. > >> >> reg: >> maxItems: 1 >> >> '#phy-cells': true >> >> + ti,enet-ctrl-qsgmii: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: | >> + Required only for QSGMII mode. Bitmask to select the port for >> + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB >> + ports automatically. Any of the 4 CPSW5G ports can act as the >> + main port with the rest of them being the QSGMII_SUB ports. >> + > > This is weird way of doing things. > > The Ethernet controller driver already knows which mode the port is > supposed to operate. From the ethernet driver perspective, there is no difference between the QSGMII or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel driver configures CPSW MAC registers differently depending on the mode being QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to identify the QSGMII main port and the rest are configured in CPSW MAC as QSGMII-SUB ports. > > e.g. > +&cpsw0_port1 { > + phy-handle = <&cpsw5g_phy0>; > + phy-mode = "qsgmii"; > + mac-address = [00 00 00 00 00 00]; > + phys = <&cpsw0_phy_gmii_sel 1>; > +}; > + > +&cpsw0_port2 { > + phy-handle = <&cpsw5g_phy1>; > + phy-mode = "qsgmii-sub"; > + mac-address = [00 00 00 00 00 00]; > + phys = <&cpsw0_phy_gmii_sel 2>; > > And it can convey the mode to the PHY driver via phy_ops->set_mode. > So you should be depending on that instead of adding this new property. QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB has to be introduced to the kernel. Additionally, all existing phy drivers will have to be updated to recognize the new phy mode. Since the QSGMII-SUB mode is TI specific, it was decided that it would be better to add a new property in TI specific files for identifying the QSGMII main port and treating the rest as QSGMII-SUB ports. Thanks, Siddharth. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-06-01 6:01 ` Siddharth Vadapalli @ 2022-06-01 9:38 ` Roger Quadros 2022-06-01 11:27 ` Siddharth Vadapalli 0 siblings, 1 reply; 11+ messages in thread From: Roger Quadros @ 2022-06-01 9:38 UTC (permalink / raw) To: Siddharth Vadapalli, robh+dt, lee.jones, krzysztof.kozlowski+dt, kishon, vkoul, dan.carpenter, grygorii.strashko Cc: devicetree, linux-kernel, linux-phy Siddharth, On 01/06/2022 09:01, Siddharth Vadapalli wrote: > Hello Roger, > > On 31/05/22 17:15, Roger Quadros wrote: >> Hi Siddharth, >> >> On 31/05/2022 14:12, 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 | 5 ++++ >>> .../bindings/phy/ti,phy-gmii-sel.yaml | 24 ++++++++++++++++++- >>> 2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644 >>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>> @@ -48,6 +48,11 @@ patternProperties: >>> description: >>> This is the SERDES lane control mux. >>> >>> + "phy@[0-9a-f]+$": >>> + type: object >>> + description: >>> + This is the register to set phy mode through phy-gmii-sel driver. >>> + >> >> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all. > > The property has to be mentioned in order to pass: make dtbs_check. > >> >>> 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..7427758451e7 100644 >>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>> @@ -53,12 +53,21 @@ properties: >>> - ti,am43xx-phy-gmii-sel >>> - ti,dm814-phy-gmii-sel >>> - ti,am654-phy-gmii-sel >>> + - ti,j7200-cpsw5g-phy-gmii-sel >> >> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming. > > In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While > CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added > with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for > CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name > of the compatible. Here we are talking about the PHY driver (phy-gmii-sel) and not the MAC (CPSW2G / CPSW5G) Does this PHY on J7200 always support QSGMII mode? if yes then embedding "cpsw5g" in compatible is wrong. You need to use a different compatible in CPSW driver and make sure CPSW2G doesn't initiate QSGMII mode. > >> >>> >>> reg: >>> maxItems: 1 >>> >>> '#phy-cells': true >>> >>> + ti,enet-ctrl-qsgmii: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: | >>> + Required only for QSGMII mode. Bitmask to select the port for >>> + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB >>> + ports automatically. Any of the 4 CPSW5G ports can act as the >>> + main port with the rest of them being the QSGMII_SUB ports. >>> + >> >> This is weird way of doing things. >> >> The Ethernet controller driver already knows which mode the port is >> supposed to operate. > > From the ethernet driver perspective, there is no difference between the QSGMII > or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel > driver configures CPSW MAC registers differently depending on the mode being > QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to > identify the QSGMII main port and the rest are configured in CPSW MAC as > QSGMII-SUB ports. > >> >> e.g. >> +&cpsw0_port1 { >> + phy-handle = <&cpsw5g_phy0>; >> + phy-mode = "qsgmii"; >> + mac-address = [00 00 00 00 00 00]; >> + phys = <&cpsw0_phy_gmii_sel 1>; >> +}; >> + >> +&cpsw0_port2 { >> + phy-handle = <&cpsw5g_phy1>; >> + phy-mode = "qsgmii-sub"; >> + mac-address = [00 00 00 00 00 00]; >> + phys = <&cpsw0_phy_gmii_sel 2>; >> >> And it can convey the mode to the PHY driver via phy_ops->set_mode. >> So you should be depending on that instead of adding this new property. > > QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with > the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB > has to be introduced to the kernel. Additionally, all existing phy drivers will > have to be updated to recognize the new phy mode. > > Since the QSGMII-SUB mode is TI specific, it was decided that it would be better > to add a new property in TI specific files for identifying the QSGMII main port > and treating the rest as QSGMII-SUB ports. Who decides which port should be MAIN and which should be SUB? Can all ports be MAIN? Can all ports be SUB or there has to be at least one MAIN? Just trying to understand if there can be a solution without introducing a new custom property. cheers, -roger ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-06-01 9:38 ` Roger Quadros @ 2022-06-01 11:27 ` Siddharth Vadapalli 2022-06-03 8:48 ` Roger Quadros 0 siblings, 1 reply; 11+ messages in thread From: Siddharth Vadapalli @ 2022-06-01 11:27 UTC (permalink / raw) To: Roger Quadros, robh+dt, lee.jones, krzysztof.kozlowski+dt, kishon, vkoul, dan.carpenter, grygorii.strashko Cc: devicetree, linux-kernel, linux-phy, s-vadapalli Hello Roger, On 01/06/22 15:08, Roger Quadros wrote: > Siddharth, > > On 01/06/2022 09:01, Siddharth Vadapalli wrote: >> Hello Roger, >> >> On 31/05/22 17:15, Roger Quadros wrote: >>> Hi Siddharth, >>> >>> On 31/05/2022 14:12, 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 | 5 ++++ >>>> .../bindings/phy/ti,phy-gmii-sel.yaml | 24 ++++++++++++++++++- >>>> 2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644 >>>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>> @@ -48,6 +48,11 @@ patternProperties: >>>> description: >>>> This is the SERDES lane control mux. >>>> >>>> + "phy@[0-9a-f]+$": >>>> + type: object >>>> + description: >>>> + This is the register to set phy mode through phy-gmii-sel driver. >>>> + >>> >>> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all. >> >> The property has to be mentioned in order to pass: make dtbs_check. >> >>> >>>> 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..7427758451e7 100644 >>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>> @@ -53,12 +53,21 @@ properties: >>>> - ti,am43xx-phy-gmii-sel >>>> - ti,dm814-phy-gmii-sel >>>> - ti,am654-phy-gmii-sel >>>> + - ti,j7200-cpsw5g-phy-gmii-sel >>> >>> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming. >> >> In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While >> CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added >> with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for >> CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name >> of the compatible. > > Here we are talking about the PHY driver (phy-gmii-sel) and not the MAC (CPSW2G / CPSW5G) > Does this PHY on J7200 always support QSGMII mode? if yes then embedding "cpsw5g" in compatible is wrong. The PHY on J7200 is part of the Add-On Ethernet card. It is possible to connect RGMII, QSGMII and SGMII PHY. The CPSW5G MAC supports all these modes. With the current patch, I am adding just QSGMII mode as an extra mode, but in a future patch, I will be adding SGMII also as an extra mode. For this reason, CPSW5G is being mentioned in the compatible name, to differentiate supported modes for CPSW2G and CPSW5G. Also, the phy-gmii-sel driver actually configures CPSW MAC registers and not the PHY. > > You need to use a different compatible in CPSW driver and make sure CPSW2G doesn't initiate QSGMII mode. Yes, I will add a check there too by using a different compatible in the CPSW driver, but shouldn't the phy-gmii-sel driver also have a check to ensure that it doesn't try configuring QSGMII mode for CPSW2G? > >> >>> >>>> >>>> reg: >>>> maxItems: 1 >>>> >>>> '#phy-cells': true >>>> >>>> + ti,enet-ctrl-qsgmii: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + description: | >>>> + Required only for QSGMII mode. Bitmask to select the port for >>>> + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB >>>> + ports automatically. Any of the 4 CPSW5G ports can act as the >>>> + main port with the rest of them being the QSGMII_SUB ports. >>>> + >>> >>> This is weird way of doing things. >>> >>> The Ethernet controller driver already knows which mode the port is >>> supposed to operate. >> >> From the ethernet driver perspective, there is no difference between the QSGMII >> or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel >> driver configures CPSW MAC registers differently depending on the mode being >> QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to >> identify the QSGMII main port and the rest are configured in CPSW MAC as >> QSGMII-SUB ports. >> >>> >>> e.g. >>> +&cpsw0_port1 { >>> + phy-handle = <&cpsw5g_phy0>; >>> + phy-mode = "qsgmii"; >>> + mac-address = [00 00 00 00 00 00]; >>> + phys = <&cpsw0_phy_gmii_sel 1>; >>> +}; >>> + >>> +&cpsw0_port2 { >>> + phy-handle = <&cpsw5g_phy1>; >>> + phy-mode = "qsgmii-sub"; >>> + mac-address = [00 00 00 00 00 00]; >>> + phys = <&cpsw0_phy_gmii_sel 2>; >>> >>> And it can convey the mode to the PHY driver via phy_ops->set_mode. >>> So you should be depending on that instead of adding this new property. >> >> QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with >> the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB >> has to be introduced to the kernel. Additionally, all existing phy drivers will >> have to be updated to recognize the new phy mode. >> >> Since the QSGMII-SUB mode is TI specific, it was decided that it would be better >> to add a new property in TI specific files for identifying the QSGMII main port >> and treating the rest as QSGMII-SUB ports. > > Who decides which port should be MAIN and which should be SUB? Can all ports be MAIN? > Can all ports be SUB or there has to be at least one MAIN? All 4 ports in CPSW5G have the capability to be the MAIN port, with the only restriction being that only one of them should be the MAIN port at a time. The role of the CPSW5G ports is decided based on what PHY port each of the CPSW5G ports connects to. MAIN port of CPSW5G MAC is responsible for auto-negotiation with the PHY port on the PHY which supports auto-negotiation. Thus, there can and should be only one MAIN port. Thanks, Siddharth. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-06-01 11:27 ` Siddharth Vadapalli @ 2022-06-03 8:48 ` Roger Quadros 2022-06-03 10:49 ` Siddharth Vadapalli 0 siblings, 1 reply; 11+ messages in thread From: Roger Quadros @ 2022-06-03 8:48 UTC (permalink / raw) To: Siddharth Vadapalli, robh+dt, lee.jones, krzysztof.kozlowski+dt, kishon, vkoul, dan.carpenter, grygorii.strashko Cc: devicetree, linux-kernel, linux-phy Hi Siddharth, On 01/06/2022 14:27, Siddharth Vadapalli wrote: > Hello Roger, > > On 01/06/22 15:08, Roger Quadros wrote: >> Siddharth, >> >> On 01/06/2022 09:01, Siddharth Vadapalli wrote: >>> Hello Roger, >>> >>> On 31/05/22 17:15, Roger Quadros wrote: >>>> Hi Siddharth, >>>> >>>> On 31/05/2022 14:12, 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 | 5 ++++ >>>>> .../bindings/phy/ti,phy-gmii-sel.yaml | 24 ++++++++++++++++++- >>>>> 2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644 >>>>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>>> @@ -48,6 +48,11 @@ patternProperties: >>>>> description: >>>>> This is the SERDES lane control mux. >>>>> >>>>> + "phy@[0-9a-f]+$": >>>>> + type: object >>>>> + description: >>>>> + This is the register to set phy mode through phy-gmii-sel driver. >>>>> + >>>> >>>> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all. >>> >>> The property has to be mentioned in order to pass: make dtbs_check. >>> >>>> >>>>> 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..7427758451e7 100644 >>>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>>> @@ -53,12 +53,21 @@ properties: >>>>> - ti,am43xx-phy-gmii-sel >>>>> - ti,dm814-phy-gmii-sel >>>>> - ti,am654-phy-gmii-sel >>>>> + - ti,j7200-cpsw5g-phy-gmii-sel >>>> >>>> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming. >>> >>> In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While >>> CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added >>> with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for >>> CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name >>> of the compatible. >> >> Here we are talking about the PHY driver (phy-gmii-sel) and not the MAC (CPSW2G / CPSW5G) >> Does this PHY on J7200 always support QSGMII mode? if yes then embedding "cpsw5g" in compatible is wrong. > > The PHY on J7200 is part of the Add-On Ethernet card. It is possible to connect > RGMII, QSGMII and SGMII PHY. The CPSW5G MAC supports all these modes. With the > current patch, I am adding just QSGMII mode as an extra mode, but in a future > patch, I will be adding SGMII also as an extra mode. For this reason, CPSW5G is > being mentioned in the compatible name, to differentiate supported modes for > CPSW2G and CPSW5G. Also, the phy-gmii-sel driver actually configures CPSW MAC > registers and not the PHY. phy-gmii-sel configures CTRL MMR register right? How does it configure CPSW MAC register? Anyways, I just looked at the TRM and there are in fact separate phy-gmii-sel (ENET_CTRL) registers for CPSW2g and CPSW5g. So they warrant for separate compatibles as they are not identical. > >> >> You need to use a different compatible in CPSW driver and make sure CPSW2G doesn't initiate QSGMII mode. > > Yes, I will add a check there too by using a different compatible in the CPSW > driver, but shouldn't the phy-gmii-sel driver also have a check to ensure that > it doesn't try configuring QSGMII mode for CPSW2G? Yes, additional check in phy-gmii-sel driver is fine. > >> >>> >>>> >>>>> >>>>> reg: >>>>> maxItems: 1 >>>>> >>>>> '#phy-cells': true >>>>> >>>>> + ti,enet-ctrl-qsgmii: >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>> + description: | >>>>> + Required only for QSGMII mode. Bitmask to select the port for >>>>> + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB >>>>> + ports automatically. Any of the 4 CPSW5G ports can act as the >>>>> + main port with the rest of them being the QSGMII_SUB ports. >>>>> + >>>> >>>> This is weird way of doing things. >>>> >>>> The Ethernet controller driver already knows which mode the port is >>>> supposed to operate. >>> >>> From the ethernet driver perspective, there is no difference between the QSGMII >>> or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel >>> driver configures CPSW MAC registers differently depending on the mode being You mean the ENET_CTRL register in CTRL_MMR space? >>> QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to >>> identify the QSGMII main port and the rest are configured in CPSW MAC as >>> QSGMII-SUB ports. >>> >>>> >>>> e.g. >>>> +&cpsw0_port1 { >>>> + phy-handle = <&cpsw5g_phy0>; >>>> + phy-mode = "qsgmii"; >>>> + mac-address = [00 00 00 00 00 00]; >>>> + phys = <&cpsw0_phy_gmii_sel 1>; >>>> +}; >>>> + >>>> +&cpsw0_port2 { >>>> + phy-handle = <&cpsw5g_phy1>; >>>> + phy-mode = "qsgmii-sub"; >>>> + mac-address = [00 00 00 00 00 00]; >>>> + phys = <&cpsw0_phy_gmii_sel 2>; >>>> >>>> And it can convey the mode to the PHY driver via phy_ops->set_mode. >>>> So you should be depending on that instead of adding this new property. >>> >>> QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with >>> the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB >>> has to be introduced to the kernel. Additionally, all existing phy drivers will >>> have to be updated to recognize the new phy mode. >>> >>> Since the QSGMII-SUB mode is TI specific, it was decided that it would be better >>> to add a new property in TI specific files for identifying the QSGMII main port >>> and treating the rest as QSGMII-SUB ports. >> >> Who decides which port should be MAIN and which should be SUB? Can all ports be MAIN? >> Can all ports be SUB or there has to be at least one MAIN? > > All 4 ports in CPSW5G have the capability to be the MAIN port, with the only > restriction being that only one of them should be the MAIN port at a time. The > role of the CPSW5G ports is decided based on what PHY port each of the CPSW5G > ports connects to. OK, then instead of using bitmask and property being named "ti,enet-ctrl-qsgmii", why not just say "ti,qsgmii-main-port" = <main_port_number>; Also do some sanity check when getting that property. > > MAIN port of CPSW5G MAC is responsible for auto-negotiation with the PHY port on > the PHY which supports auto-negotiation. Thus, there can and should be only one > MAIN port. > > Thanks, > Siddharth. cheers, -roger ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-06-03 8:48 ` Roger Quadros @ 2022-06-03 10:49 ` Siddharth Vadapalli 2022-06-03 12:36 ` Roger Quadros 0 siblings, 1 reply; 11+ messages in thread From: Siddharth Vadapalli @ 2022-06-03 10:49 UTC (permalink / raw) To: Roger Quadros Cc: robh+dt, lee.jones, krzysztof.kozlowski+dt, vkoul, dan.carpenter, kishon, grygorii.strashko, devicetree, linux-kernel, linux-phy Hello Roger, On 03/06/22 14:18, Roger Quadros wrote: > Hi Siddharth, > > On 01/06/2022 14:27, Siddharth Vadapalli wrote: >> Hello Roger, >> >> On 01/06/22 15:08, Roger Quadros wrote: >>> Siddharth, >>> >>> On 01/06/2022 09:01, Siddharth Vadapalli wrote: >>>> Hello Roger, >>>> >>>> On 31/05/22 17:15, Roger Quadros wrote: >>>>> Hi Siddharth, >>>>> >>>>> On 31/05/2022 14:12, 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 | 5 ++++ >>>>>> .../bindings/phy/ti,phy-gmii-sel.yaml | 24 ++++++++++++++++++- >>>>>> 2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644 >>>>>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>>>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>>>> @@ -48,6 +48,11 @@ patternProperties: >>>>>> description: >>>>>> This is the SERDES lane control mux. >>>>>> >>>>>> + "phy@[0-9a-f]+$": >>>>>> + type: object >>>>>> + description: >>>>>> + This is the register to set phy mode through phy-gmii-sel driver. >>>>>> + >>>>> >>>>> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all. >>>> >>>> The property has to be mentioned in order to pass: make dtbs_check. >>>> >>>>> >>>>>> 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..7427758451e7 100644 >>>>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>>>> @@ -53,12 +53,21 @@ properties: >>>>>> - ti,am43xx-phy-gmii-sel >>>>>> - ti,dm814-phy-gmii-sel >>>>>> - ti,am654-phy-gmii-sel >>>>>> + - ti,j7200-cpsw5g-phy-gmii-sel >>>>> >>>>> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming. >>>> >>>> In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While >>>> CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added >>>> with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for >>>> CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name >>>> of the compatible. >>> >>> Here we are talking about the PHY driver (phy-gmii-sel) and not the MAC (CPSW2G / CPSW5G) >>> Does this PHY on J7200 always support QSGMII mode? if yes then embedding "cpsw5g" in compatible is wrong. >> >> The PHY on J7200 is part of the Add-On Ethernet card. It is possible to connect >> RGMII, QSGMII and SGMII PHY. The CPSW5G MAC supports all these modes. With the >> current patch, I am adding just QSGMII mode as an extra mode, but in a future >> patch, I will be adding SGMII also as an extra mode. For this reason, CPSW5G is >> being mentioned in the compatible name, to differentiate supported modes for >> CPSW2G and CPSW5G. Also, the phy-gmii-sel driver actually configures CPSW MAC >> registers and not the PHY. > > phy-gmii-sel configures CTRL MMR register right? How does it configure CPSW MAC register? > > Anyways, I just looked at the TRM and there are in fact separate phy-gmii-sel (ENET_CTRL) > registers for CPSW2g and CPSW5g. So they warrant for separate compatibles as they are > not identical. By CPSW MAC registers being configured, I meant that the configuration being done is for the MAC and not for the PHY. As per the TRM, for CPSW2G, the CTRLMMR_MCU_ENET_CTRL register is configured and for CPSW5G, the CTRLMMR_ENETx_CTRL registers are configured, with x ranging from 1 to 4 (corresponding to the 4 ports of CPSW5G). These registers configure the CPSW MAC (CPSW2G/CPSW5G) and not the PHY. For this reason, I think that it would be appropriate to use cpsw5g in the compatible name, to indicate which CTRLMMR registers are being configured. >> >>> >>> You need to use a different compatible in CPSW driver and make sure CPSW2G doesn't initiate QSGMII mode. >> >> Yes, I will add a check there too by using a different compatible in the CPSW >> driver, but shouldn't the phy-gmii-sel driver also have a check to ensure that >> it doesn't try configuring QSGMII mode for CPSW2G? > > Yes, additional check in phy-gmii-sel driver is fine. > >> >>> >>>> >>>>> >>>>>> >>>>>> reg: >>>>>> maxItems: 1 >>>>>> >>>>>> '#phy-cells': true >>>>>> >>>>>> + ti,enet-ctrl-qsgmii: >>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>> + description: | >>>>>> + Required only for QSGMII mode. Bitmask to select the port for >>>>>> + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB >>>>>> + ports automatically. Any of the 4 CPSW5G ports can act as the >>>>>> + main port with the rest of them being the QSGMII_SUB ports. >>>>>> + >>>>> >>>>> This is weird way of doing things. >>>>> >>>>> The Ethernet controller driver already knows which mode the port is >>>>> supposed to operate. >>>> >>>> From the ethernet driver perspective, there is no difference between the QSGMII >>>> or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel >>>> driver configures CPSW MAC registers differently depending on the mode being > > You mean the ENET_CTRL register in CTRL_MMR space? Yes I am referring to the CTRLMMR_ENETx_CTRL registers as per the J7200 TRM, corresponding to the CPSW5G MAC. > >>>> QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to >>>> identify the QSGMII main port and the rest are configured in CPSW MAC as >>>> QSGMII-SUB ports. >>>> >>>>> >>>>> e.g. >>>>> +&cpsw0_port1 { >>>>> + phy-handle = <&cpsw5g_phy0>; >>>>> + phy-mode = "qsgmii"; >>>>> + mac-address = [00 00 00 00 00 00]; >>>>> + phys = <&cpsw0_phy_gmii_sel 1>; >>>>> +}; >>>>> + >>>>> +&cpsw0_port2 { >>>>> + phy-handle = <&cpsw5g_phy1>; >>>>> + phy-mode = "qsgmii-sub"; >>>>> + mac-address = [00 00 00 00 00 00]; >>>>> + phys = <&cpsw0_phy_gmii_sel 2>; >>>>> >>>>> And it can convey the mode to the PHY driver via phy_ops->set_mode. >>>>> So you should be depending on that instead of adding this new property. >>>> >>>> QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with >>>> the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB >>>> has to be introduced to the kernel. Additionally, all existing phy drivers will >>>> have to be updated to recognize the new phy mode. >>>> >>>> Since the QSGMII-SUB mode is TI specific, it was decided that it would be better >>>> to add a new property in TI specific files for identifying the QSGMII main port >>>> and treating the rest as QSGMII-SUB ports. >>> >>> Who decides which port should be MAIN and which should be SUB? Can all ports be MAIN? >>> Can all ports be SUB or there has to be at least one MAIN? >> >> All 4 ports in CPSW5G have the capability to be the MAIN port, with the only >> restriction being that only one of them should be the MAIN port at a time. The >> role of the CPSW5G ports is decided based on what PHY port each of the CPSW5G >> ports connects to. > > OK, then instead of using bitmask and property being named "ti,enet-ctrl-qsgmii", why not > just say "ti,qsgmii-main-port" = <main_port_number>; I plan to send patches for J721e device which has CPSW9G (8 external ports) MAC. CPSW9G can work with two sets of QSGMII interfaces (4 ports + 4 ports). Thus, using a bitmask for the QSGMII main port will help identify the QSGMII main port across both sets of QSGMII interfaces. The bitmask in case of J721e CPSW9G will consider the first 4 bits for the first interface's 4 ports and the next 4 bits for the second interface's 4 ports. In this manner, it will be possible to extend it for 8 port CPSW9G MAC as well, without having to add a new property for the second QSGMII interface. > > Also do some sanity check when getting that property. To ensure that multiple QSGMII ports are not declared as the main port, the "ti,enet-ctrl-qsgmii" property has been declared as an enum: [1,2,4,8]. If a different value other than the value in enum were to be used, then "make dtbs_check" would raise an error. This will prevent configuring multiple QSGMII ports as the main port at once. Also, in the phy-gmii-sel driver, a default value of 1 is being assigned to the variable that will store the value corresponding to the ti,enet-ctrl-qsgmii property from the device tree, thereby treating the first CPSW5G port as the QSGMII main port by default. Thanks, Siddharth. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-06-03 10:49 ` Siddharth Vadapalli @ 2022-06-03 12:36 ` Roger Quadros 2022-06-06 9:39 ` Siddharth Vadapalli 0 siblings, 1 reply; 11+ messages in thread From: Roger Quadros @ 2022-06-03 12:36 UTC (permalink / raw) To: Siddharth Vadapalli Cc: robh+dt, lee.jones, krzysztof.kozlowski+dt, vkoul, dan.carpenter, kishon, grygorii.strashko, devicetree, linux-kernel, linux-phy Siddharth, On 03/06/2022 13:49, Siddharth Vadapalli wrote: > Hello Roger, > > On 03/06/22 14:18, Roger Quadros wrote: >> Hi Siddharth, >> >> On 01/06/2022 14:27, Siddharth Vadapalli wrote: >>> Hello Roger, >>> >>> On 01/06/22 15:08, Roger Quadros wrote: >>>> Siddharth, >>>> >>>> On 01/06/2022 09:01, Siddharth Vadapalli wrote: >>>>> Hello Roger, >>>>> >>>>> On 31/05/22 17:15, Roger Quadros wrote: >>>>>> Hi Siddharth, >>>>>> >>>>>> On 31/05/2022 14:12, 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 | 5 ++++ >>>>>>> .../bindings/phy/ti,phy-gmii-sel.yaml | 24 ++++++++++++++++++- >>>>>>> 2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644 >>>>>>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>>>>> @@ -48,6 +48,11 @@ patternProperties: >>>>>>> description: >>>>>>> This is the SERDES lane control mux. >>>>>>> >>>>>>> + "phy@[0-9a-f]+$": >>>>>>> + type: object >>>>>>> + description: >>>>>>> + This is the register to set phy mode through phy-gmii-sel driver. >>>>>>> + >>>>>> >>>>>> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all. >>>>> >>>>> The property has to be mentioned in order to pass: make dtbs_check. >>>>> >>>>>> >>>>>>> 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..7427758451e7 100644 >>>>>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>>>>> @@ -53,12 +53,21 @@ properties: >>>>>>> - ti,am43xx-phy-gmii-sel >>>>>>> - ti,dm814-phy-gmii-sel >>>>>>> - ti,am654-phy-gmii-sel >>>>>>> + - ti,j7200-cpsw5g-phy-gmii-sel >>>>>> >>>>>> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming. >>>>> >>>>> In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While >>>>> CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added >>>>> with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for >>>>> CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name >>>>> of the compatible. >>>> >>>> Here we are talking about the PHY driver (phy-gmii-sel) and not the MAC (CPSW2G / CPSW5G) >>>> Does this PHY on J7200 always support QSGMII mode? if yes then embedding "cpsw5g" in compatible is wrong. >>> >>> The PHY on J7200 is part of the Add-On Ethernet card. It is possible to connect >>> RGMII, QSGMII and SGMII PHY. The CPSW5G MAC supports all these modes. With the >>> current patch, I am adding just QSGMII mode as an extra mode, but in a future >>> patch, I will be adding SGMII also as an extra mode. For this reason, CPSW5G is >>> being mentioned in the compatible name, to differentiate supported modes for >>> CPSW2G and CPSW5G. Also, the phy-gmii-sel driver actually configures CPSW MAC >>> registers and not the PHY. >> >> phy-gmii-sel configures CTRL MMR register right? How does it configure CPSW MAC register? >> >> Anyways, I just looked at the TRM and there are in fact separate phy-gmii-sel (ENET_CTRL) >> registers for CPSW2g and CPSW5g. So they warrant for separate compatibles as they are >> not identical. > > By CPSW MAC registers being configured, I meant that the configuration being > done is for the MAC and not for the PHY. As per the TRM, for CPSW2G, the > CTRLMMR_MCU_ENET_CTRL register is configured and for CPSW5G, the > CTRLMMR_ENETx_CTRL registers are configured, with x ranging from 1 to 4 > (corresponding to the 4 ports of CPSW5G). These registers configure the CPSW MAC > (CPSW2G/CPSW5G) and not the PHY. For this reason, I think that it would be > appropriate to use cpsw5g in the compatible name, to indicate which CTRLMMR > registers are being configured. Yes, I already agreed that separate compatible is fine :). > >>> >>>> >>>> You need to use a different compatible in CPSW driver and make sure CPSW2G doesn't initiate QSGMII mode. >>> >>> Yes, I will add a check there too by using a different compatible in the CPSW >>> driver, but shouldn't the phy-gmii-sel driver also have a check to ensure that >>> it doesn't try configuring QSGMII mode for CPSW2G? >> >> Yes, additional check in phy-gmii-sel driver is fine. >> >>> >>>> >>>>> >>>>>> >>>>>>> >>>>>>> reg: >>>>>>> maxItems: 1 >>>>>>> >>>>>>> '#phy-cells': true >>>>>>> >>>>>>> + ti,enet-ctrl-qsgmii: >>>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>>> + description: | >>>>>>> + Required only for QSGMII mode. Bitmask to select the port for >>>>>>> + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB >>>>>>> + ports automatically. Any of the 4 CPSW5G ports can act as the >>>>>>> + main port with the rest of them being the QSGMII_SUB ports. >>>>>>> + >>>>>> >>>>>> This is weird way of doing things. >>>>>> >>>>>> The Ethernet controller driver already knows which mode the port is >>>>>> supposed to operate. >>>>> >>>>> From the ethernet driver perspective, there is no difference between the QSGMII >>>>> or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel >>>>> driver configures CPSW MAC registers differently depending on the mode being >> >> You mean the ENET_CTRL register in CTRL_MMR space? > > Yes I am referring to the CTRLMMR_ENETx_CTRL registers as per the J7200 TRM, > corresponding to the CPSW5G MAC. > >> >>>>> QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to >>>>> identify the QSGMII main port and the rest are configured in CPSW MAC as >>>>> QSGMII-SUB ports. >>>>> >>>>>> >>>>>> e.g. >>>>>> +&cpsw0_port1 { >>>>>> + phy-handle = <&cpsw5g_phy0>; >>>>>> + phy-mode = "qsgmii"; >>>>>> + mac-address = [00 00 00 00 00 00]; >>>>>> + phys = <&cpsw0_phy_gmii_sel 1>; >>>>>> +}; >>>>>> + >>>>>> +&cpsw0_port2 { >>>>>> + phy-handle = <&cpsw5g_phy1>; >>>>>> + phy-mode = "qsgmii-sub"; >>>>>> + mac-address = [00 00 00 00 00 00]; >>>>>> + phys = <&cpsw0_phy_gmii_sel 2>; >>>>>> >>>>>> And it can convey the mode to the PHY driver via phy_ops->set_mode. >>>>>> So you should be depending on that instead of adding this new property. >>>>> >>>>> QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with >>>>> the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB >>>>> has to be introduced to the kernel. Additionally, all existing phy drivers will >>>>> have to be updated to recognize the new phy mode. >>>>> >>>>> Since the QSGMII-SUB mode is TI specific, it was decided that it would be better >>>>> to add a new property in TI specific files for identifying the QSGMII main port >>>>> and treating the rest as QSGMII-SUB ports. >>>> >>>> Who decides which port should be MAIN and which should be SUB? Can all ports be MAIN? >>>> Can all ports be SUB or there has to be at least one MAIN? >>> >>> All 4 ports in CPSW5G have the capability to be the MAIN port, with the only >>> restriction being that only one of them should be the MAIN port at a time. The >>> role of the CPSW5G ports is decided based on what PHY port each of the CPSW5G >>> ports connects to. >> >> OK, then instead of using bitmask and property being named "ti,enet-ctrl-qsgmii", why not >> just say "ti,qsgmii-main-port" = <main_port_number>; > > I plan to send patches for J721e device which has CPSW9G (8 external ports) MAC. > CPSW9G can work with two sets of QSGMII interfaces (4 ports + 4 ports). Thus, > using a bitmask for the QSGMII main port will help identify the QSGMII main port > across both sets of QSGMII interfaces. The bitmask in case of J721e CPSW9G will > consider the first 4 bits for the first interface's 4 ports and the next 4 bits > for the second interface's 4 ports. In this manner, it will be possible to > extend it for 8 port CPSW9G MAC as well, without having to add a new property > for the second QSGMII interface. > >> >> Also do some sanity check when getting that property. > > To ensure that multiple QSGMII ports are not declared as the main port, the > "ti,enet-ctrl-qsgmii" property has been declared as an enum: [1,2,4,8]. If a All I'm saying is that instead of bitmask please use port number to specify main port. You can use minimum/maximum to limit the values. Take care of limit checking per compatible and converting into bitmask in the driver. > different value other than the value in enum were to be used, then "make > dtbs_check" would raise an error. This will prevent configuring multiple QSGMII > ports as the main port at once. Also, in the phy-gmii-sel driver, a default > value of 1 is being assigned to the variable that will store the value > corresponding to the ti,enet-ctrl-qsgmii property from the device tree, thereby > treating the first CPSW5G port as the QSGMII main port by default. > > Thanks, > Siddharth. cheers, -roger ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 2022-06-03 12:36 ` Roger Quadros @ 2022-06-06 9:39 ` Siddharth Vadapalli 0 siblings, 0 replies; 11+ messages in thread From: Siddharth Vadapalli @ 2022-06-06 9:39 UTC (permalink / raw) To: Roger Quadros Cc: robh+dt, lee.jones, krzysztof.kozlowski+dt, vkoul, dan.carpenter, kishon, grygorii.strashko, devicetree, linux-kernel, linux-phy Hello Roger, On 03/06/22 18:06, Roger Quadros wrote: > Siddharth, > > On 03/06/2022 13:49, Siddharth Vadapalli wrote: >> Hello Roger, >> >> On 03/06/22 14:18, Roger Quadros wrote: >>> Hi Siddharth, >>> >>> On 01/06/2022 14:27, Siddharth Vadapalli wrote: >>>> Hello Roger, >>>> >>>> On 01/06/22 15:08, Roger Quadros wrote: >>>>> Siddharth, >>>>> >>>>> On 01/06/2022 09:01, Siddharth Vadapalli wrote: >>>>>> Hello Roger, >>>>>> >>>>>> On 31/05/22 17:15, Roger Quadros wrote: >>>>>>> Hi Siddharth, >>>>>>> >>>>>>> On 31/05/2022 14:12, 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 | 5 ++++ >>>>>>>> .../bindings/phy/ti,phy-gmii-sel.yaml | 24 ++++++++++++++++++- >>>>>>>> 2 files changed, 28 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 fa86691ebf16..e381ba62a513 100644 >>>>>>>> --- a/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>>>>>> +++ b/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml >>>>>>>> @@ -48,6 +48,11 @@ patternProperties: >>>>>>>> description: >>>>>>>> This is the SERDES lane control mux. >>>>>>>> >>>>>>>> + "phy@[0-9a-f]+$": >>>>>>>> + type: object >>>>>>>> + description: >>>>>>>> + This is the register to set phy mode through phy-gmii-sel driver. >>>>>>>> + >>>>>>> >>>>>>> Is this really required? The system controller has 100s of different such registers and it is not practical to mention about all. >>>>>> >>>>>> The property has to be mentioned in order to pass: make dtbs_check. >>>>>> >>>>>>> >>>>>>>> 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..7427758451e7 100644 >>>>>>>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml >>>>>>>> @@ -53,12 +53,21 @@ properties: >>>>>>>> - ti,am43xx-phy-gmii-sel >>>>>>>> - ti,dm814-phy-gmii-sel >>>>>>>> - ti,am654-phy-gmii-sel >>>>>>>> + - ti,j7200-cpsw5g-phy-gmii-sel >>>>>>> >>>>>>> Why not just "ti,j7200-phy-gmii-sel" so it is consistent naming. >>>>>> >>>>>> In TI's J7200 device, there are two CPSW MACs, namely CPSW2G and CPSW5G. While >>>>>> CPSW5G supports QSGMII mode, CPSW2G does not. Hence, the compatible being added >>>>>> with the extra mode (QSGMII) enabled is applicable only for CPSW5G and not for >>>>>> CPSW2G. Thus, to highlight this, the word "CPSW5G" has been included in the name >>>>>> of the compatible. >>>>> >>>>> Here we are talking about the PHY driver (phy-gmii-sel) and not the MAC (CPSW2G / CPSW5G) >>>>> Does this PHY on J7200 always support QSGMII mode? if yes then embedding "cpsw5g" in compatible is wrong. >>>> >>>> The PHY on J7200 is part of the Add-On Ethernet card. It is possible to connect >>>> RGMII, QSGMII and SGMII PHY. The CPSW5G MAC supports all these modes. With the >>>> current patch, I am adding just QSGMII mode as an extra mode, but in a future >>>> patch, I will be adding SGMII also as an extra mode. For this reason, CPSW5G is >>>> being mentioned in the compatible name, to differentiate supported modes for >>>> CPSW2G and CPSW5G. Also, the phy-gmii-sel driver actually configures CPSW MAC >>>> registers and not the PHY. >>> >>> phy-gmii-sel configures CTRL MMR register right? How does it configure CPSW MAC register? >>> >>> Anyways, I just looked at the TRM and there are in fact separate phy-gmii-sel (ENET_CTRL) >>> registers for CPSW2g and CPSW5g. So they warrant for separate compatibles as they are >>> not identical. >> >> By CPSW MAC registers being configured, I meant that the configuration being >> done is for the MAC and not for the PHY. As per the TRM, for CPSW2G, the >> CTRLMMR_MCU_ENET_CTRL register is configured and for CPSW5G, the >> CTRLMMR_ENETx_CTRL registers are configured, with x ranging from 1 to 4 >> (corresponding to the 4 ports of CPSW5G). These registers configure the CPSW MAC >> (CPSW2G/CPSW5G) and not the PHY. For this reason, I think that it would be >> appropriate to use cpsw5g in the compatible name, to indicate which CTRLMMR >> registers are being configured. > > Yes, I already agreed that separate compatible is fine :). > >> >>>> >>>>> >>>>> You need to use a different compatible in CPSW driver and make sure CPSW2G doesn't initiate QSGMII mode. >>>> >>>> Yes, I will add a check there too by using a different compatible in the CPSW >>>> driver, but shouldn't the phy-gmii-sel driver also have a check to ensure that >>>> it doesn't try configuring QSGMII mode for CPSW2G? >>> >>> Yes, additional check in phy-gmii-sel driver is fine. >>> >>>> >>>>> >>>>>> >>>>>>> >>>>>>>> >>>>>>>> reg: >>>>>>>> maxItems: 1 >>>>>>>> >>>>>>>> '#phy-cells': true >>>>>>>> >>>>>>>> + ti,enet-ctrl-qsgmii: >>>>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>>>> + description: | >>>>>>>> + Required only for QSGMII mode. Bitmask to select the port for >>>>>>>> + QSGMII main mode. Rest of the ports are selected as QSGMII_SUB >>>>>>>> + ports automatically. Any of the 4 CPSW5G ports can act as the >>>>>>>> + main port with the rest of them being the QSGMII_SUB ports. >>>>>>>> + >>>>>>> >>>>>>> This is weird way of doing things. >>>>>>> >>>>>>> The Ethernet controller driver already knows which mode the port is >>>>>>> supposed to operate. >>>>>> >>>>>> From the ethernet driver perspective, there is no difference between the QSGMII >>>>>> or QSGMII-SUB modes and both are treated the same. However, the phy-gmii-sel >>>>>> driver configures CPSW MAC registers differently depending on the mode being >>> >>> You mean the ENET_CTRL register in CTRL_MMR space? >> >> Yes I am referring to the CTRLMMR_ENETx_CTRL registers as per the J7200 TRM, >> corresponding to the CPSW5G MAC. >> >>> >>>>>> QSGMII or QSGMII-SUB. Hence, the ti,enet-ctrl-qsgmii property is used to >>>>>> identify the QSGMII main port and the rest are configured in CPSW MAC as >>>>>> QSGMII-SUB ports. >>>>>> >>>>>>> >>>>>>> e.g. >>>>>>> +&cpsw0_port1 { >>>>>>> + phy-handle = <&cpsw5g_phy0>; >>>>>>> + phy-mode = "qsgmii"; >>>>>>> + mac-address = [00 00 00 00 00 00]; >>>>>>> + phys = <&cpsw0_phy_gmii_sel 1>; >>>>>>> +}; >>>>>>> + >>>>>>> +&cpsw0_port2 { >>>>>>> + phy-handle = <&cpsw5g_phy1>; >>>>>>> + phy-mode = "qsgmii-sub"; >>>>>>> + mac-address = [00 00 00 00 00 00]; >>>>>>> + phys = <&cpsw0_phy_gmii_sel 2>; >>>>>>> >>>>>>> And it can convey the mode to the PHY driver via phy_ops->set_mode. >>>>>>> So you should be depending on that instead of adding this new property. >>>>>> >>>>>> QSGMII-SUB is not a standard mode in the Linux kernel. In order to proceed with >>>>>> the suggested implementation, a new phy mode named PHY_INTERFACE_MODE_QSGMII_SUB >>>>>> has to be introduced to the kernel. Additionally, all existing phy drivers will >>>>>> have to be updated to recognize the new phy mode. >>>>>> >>>>>> Since the QSGMII-SUB mode is TI specific, it was decided that it would be better >>>>>> to add a new property in TI specific files for identifying the QSGMII main port >>>>>> and treating the rest as QSGMII-SUB ports. >>>>> >>>>> Who decides which port should be MAIN and which should be SUB? Can all ports be MAIN? >>>>> Can all ports be SUB or there has to be at least one MAIN? >>>> >>>> All 4 ports in CPSW5G have the capability to be the MAIN port, with the only >>>> restriction being that only one of them should be the MAIN port at a time. The >>>> role of the CPSW5G ports is decided based on what PHY port each of the CPSW5G >>>> ports connects to. >>> >>> OK, then instead of using bitmask and property being named "ti,enet-ctrl-qsgmii", why not >>> just say "ti,qsgmii-main-port" = <main_port_number>; >> >> I plan to send patches for J721e device which has CPSW9G (8 external ports) MAC. >> CPSW9G can work with two sets of QSGMII interfaces (4 ports + 4 ports). Thus, >> using a bitmask for the QSGMII main port will help identify the QSGMII main port >> across both sets of QSGMII interfaces. The bitmask in case of J721e CPSW9G will >> consider the first 4 bits for the first interface's 4 ports and the next 4 bits >> for the second interface's 4 ports. In this manner, it will be possible to >> extend it for 8 port CPSW9G MAC as well, without having to add a new property >> for the second QSGMII interface. >> >>> >>> Also do some sanity check when getting that property. >> >> To ensure that multiple QSGMII ports are not declared as the main port, the >> "ti,enet-ctrl-qsgmii" property has been declared as an enum: [1,2,4,8]. If a > > All I'm saying is that instead of bitmask please use port number to specify main port. > You can use minimum/maximum to limit the values. > > Take care of limit checking per compatible and converting into bitmask in the driver. The current series of patches is for J7200 device which supports one QSGMII interface. However, I plan to post patches for another device (J721e) which has 8 external ports and therefore, can be configured as two sets of QSGMII interfaces. To identify the two main ports across the two QSGMII interfaces, a single port number will not be sufficient. Hence, a bitmask has been used, to avoid adding a new property for the second QSGMII interface's main port, when I post patches for J721e. Thanks, Siddharth. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200 2022-05-31 11:12 [PATCH 0/2] Add support for QSGMII mode Siddharth Vadapalli 2022-05-31 11:12 ` [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 Siddharth Vadapalli @ 2022-05-31 11:12 ` Siddharth Vadapalli 1 sibling, 0 replies; 11+ messages in thread From: Siddharth Vadapalli @ 2022-05-31 11:12 UTC (permalink / raw) To: robh+dt, lee.jones, krzysztof.kozlowski+dt, kishon, vkoul, dan.carpenter, grygorii.strashko, rogerq Cc: devicetree, linux-kernel, linux-phy, Siddharth 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,enet-ctrl-qsgmii", 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. Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- drivers/phy/ti/phy-gmii-sel.c | 39 ++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c index d0ab69750c6b..3def0909ba69 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_port; }; 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_port & 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); @@ -363,6 +394,8 @@ 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; + priv->qsgmii_main_port = PHY_GMII_PORT(1); + of_property_read_u32(node, "ti,enet-ctrl-qsgmii", &priv->qsgmii_main_port); priv->regmap = syscon_node_to_regmap(node->parent); if (IS_ERR(priv->regmap)) { -- 2.36.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-06-06 9:39 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-31 11:12 [PATCH 0/2] Add support for QSGMII mode Siddharth Vadapalli 2022-05-31 11:12 ` [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200 Siddharth Vadapalli 2022-05-31 11:45 ` Roger Quadros 2022-06-01 6:01 ` Siddharth Vadapalli 2022-06-01 9:38 ` Roger Quadros 2022-06-01 11:27 ` Siddharth Vadapalli 2022-06-03 8:48 ` Roger Quadros 2022-06-03 10:49 ` Siddharth Vadapalli 2022-06-03 12:36 ` Roger Quadros 2022-06-06 9:39 ` Siddharth Vadapalli 2022-05-31 11:12 ` [PATCH 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).