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

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

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.