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

[Roger Quadros]

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

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