Re: [PATCH 1/2] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J7200

[Siddharth Vadapalli]

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@xxxxxx>
>> ---
>>  .../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.