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

From: Roger Quadros <rogerq@kernel.org>
To: Siddharth Vadapalli <s-vadapalli@ti.com>
Cc: robh+dt@kernel.org, lee.jones@linaro.org,
	krzysztof.kozlowski@linaro.org,
	krzysztof.kozlowski+dt@linaro.org, kishon@ti.com,
	vkoul@kernel.org, dan.carpenter@oracle.com,
	grygorii.strashko@ti.com, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-phy@lists.infradead.org
Subject: Re: [PATCH v3 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL in J7200
Date: Tue, 30 Aug 2022 14:13:20 +0300	[thread overview]
Message-ID: <7dff961e-908d-48de-fff1-8b6029d28eed@kernel.org> (raw)
In-Reply-To: <533a4186-62b1-ce7b-e097-9fb04be8f018@ti.com>

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy