Re: [PATCH v3 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200

[Roger Quadros]

Siddharth,

On 30/08/2022 07:31, Siddharth Vadapalli wrote:
> Hello Roger,
> 
> On 29/08/22 18:13, Roger Quadros wrote:
>> Siddharth,
>>
>> On 29/08/2022 07:53, Siddharth Vadapalli wrote:
>>> Hello Roger,
>>>
>>> On 25/08/22 13:11, Roger Quadros wrote:
>>>> Hi Siddharth,
>>>>
>>>> On 22/08/2022 09:56, Siddharth Vadapalli wrote:
>>>>> 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.
>>>>
>>>> Can you please describe here why you are using "ti,qsgmii-main-ports" instead
>>>> of "ti,qsgmii-main-port" as there can be only one main port per PHY instance?
>>>
>>> Thank you for reviewing the patch. I am using "ports" instead of "port"
>>> because I plan to add support for CPSW9G on TI's J721e device in the
>>> future patches. CPSW9G (8 external ports) supports up to two QSGMII main
>>> ports. For CPSW9G, by specifying the two main ports in the device tree,
>>> it is possible to configure the CTRLMMR_ENETx_CTRL register for each of
>>> the 8 ports, with the two QSGMII main ports being configured as main
>>> ports in the CTRLMMR_ENETx_CTRL register and the rest of them being
>>> configured as sub ports. Since I will be using the same property
>>> "ti,qsgmii-main-ports" for CPSW9G as well, the property will be an array
>>> of 2 values for CPSW9G. Therefore, I am using "ports" instead of "port".
>>> Please let me know if this is fine.
>>>
>>
>> OK. Please mention this in commit message.
> 
> I will mention that the property ti,qsgmii-main-ports is used to
> configure the CTRLMMR_ENETx_CTRL register and that it is possible
> depending on the device for there to be more than one main port which is
> why the property is an array of values.
> 
> Would it be sufficient to mention the above in the commit message?
> Please let me know.

This is fine. Thanks.

cheers,
-roger