Message-ID: <e4d04ae7-c83f-79d4-7551-516e7f35c615@ti.com>
Date:   Thu, 9 Mar 2023 12:54:18 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
CC:     <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <linux@armlinux.org.uk>, <pabeni@redhat.com>, <robh+dt@kernel.org>,
        <nsekhar@ti.com>, <rogerq@kernel.org>, <netdev@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <srk@ti.com>,
        <s-vadapalli@ti.com>
Subject: Re: [PATCH net-next v2] dt-bindings: net: ti: k3-am654-cpsw-nuss:
 Document Serdes PHY
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <krzysztof.kozlowski+dt@linaro.org>
References: <20230308051835.276552-1-s-vadapalli@ti.com>
 <1ffed720-322c-fa73-1160-5fd73ce3c7c2@linaro.org>
 <7b6e8131-8e5b-88bc-69f7-b737c0c35bb6@ti.com>
 <dbbe3cd2-3329-d267-338b-8e513209ddcd@linaro.org>
 <882cdb42-3f80-048a-88a5-836c479a421f@ti.com>
 <624f5dc8-0807-e799-d66e-213aadabfc84@linaro.org>
From:   Siddharth Vadapalli <s-vadapalli@ti.com>
In-Reply-To: <624f5dc8-0807-e799-d66e-213aadabfc84@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Krzysztof,

On 09/03/23 11:51, Krzysztof Kozlowski wrote:
> On 09/03/2023 05:18, Siddharth Vadapalli wrote:
>> Hello Krzysztof,
>>
>> On 08/03/23 18:04, Krzysztof Kozlowski wrote:
>>> On 08/03/2023 09:38, Siddharth Vadapalli wrote:
>>>> Hello Krzysztof,
>>>>
>>>> On 08/03/23 14:04, Krzysztof Kozlowski wrote:
>>>>> On 08/03/2023 06:18, Siddharth Vadapalli wrote:
>>>>>> Update bindings to include Serdes PHY as an optional PHY, in addition to
>>>>>> the existing CPSW MAC's PHY. The CPSW MAC's PHY is required while the
>>>>>> Serdes PHY is optional. The Serdes PHY handle has to be provided only
>>>>>> when the Serdes is being configured in a Single-Link protocol. Using the
>>>>>> name "serdes-phy" to represent the Serdes PHY handle, the am65-cpsw-nuss
>>>>>> driver can obtain the Serdes PHY and request the Serdes to be
>>>>>> configured.
>>>>>>
>>>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>>> ---
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> This patch corresponds to the Serdes PHY bindings that were missed out in
>>>>>> the series at:
>>>>>> https://lore.kernel.org/r/20230104103432.1126403-1-s-vadapalli@ti.com/
>>>>>> This was pointed out at:
>>>>>> https://lore.kernel.org/r/CAMuHMdW5atq-FuLEL3htuE3t2uO86anLL3zeY7n1RqqMP_rH1g@mail.gmail.com/
>>>>>>
>>>>>> Changes from v1:
>>>>>> 1. Describe phys property with minItems, items and description.
>>>>>> 2. Use minItems and items in phy-names.
>>>>>> 3. Remove the description in phy-names.
>>>>>>
>>>>>> v1:
>>>>>> https://lore.kernel.org/r/20230306094750.159657-1-s-vadapalli@ti.com/
>>>>>>
>>>>>>  .../bindings/net/ti,k3-am654-cpsw-nuss.yaml        | 14 ++++++++++++--
>>>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
>>>>>> index 900063411a20..0fb48bb6a041 100644
>>>>>> --- a/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
>>>>>> @@ -126,8 +126,18 @@ properties:
>>>>>>              description: CPSW port number
>>>>>>  
>>>>>>            phys:
>>>>>> -            maxItems: 1
>>>>>> -            description: phandle on phy-gmii-sel PHY
>>>>>> +            minItems: 1
>>>>>> +            items:
>>>>>> +              - description: CPSW MAC's PHY.
>>>>>> +              - description: Serdes PHY. Serdes PHY is required only if
>>>>>> +                             the Serdes has to be configured in the
>>>>>> +                             Single-Link configuration.
>>>>>> +
>>>>>> +          phy-names:
>>>>>> +            minItems: 1
>>>>>> +            items:
>>>>>> +              - const: mac-phy
>>>>>> +              - const: serdes-phy
>>>>>
>>>>> Drop "phy" suffixes.
>>>>
>>>> The am65-cpsw driver fetches the Serdes PHY by looking for the string
>>>> "serdes-phy". Therefore, modifying the string will require changing the driver's
>>>> code as well. Please let me know if it is absolutely necessary to drop the phy
>>>> suffix. If so, I will post a new series with the changes involving dt-bindings
>>>> changes and the driver changes.
>>>
>>> Why the driver uses some properties before adding them to the binding?
>>
>> I missed adding the bindings for the Serdes PHY as a part of the series
>> mentioned in the section below the tearline of the patch. With this patch, I am
>> attempting to fix it.
>>
>>>
>>> And is it correct method of adding ABI? You add incorrect properties
>>> without documentation and then use this as an argument "driver already
>>> does it"?
>>
>> I apologize if my earlier comment appeared to justify the usage of "serdes-phy"
>> based on the driver already using it. I did not mean it in that sense. I simply
>> meant to ask if dropping "phy" suffixes was a suggestion or a rule. In that
>> context, I felt that if it was a suggestion, I would prefer retaining the names
>> with the "phy" suffixes, since the driver is already using it. Additionally, I
>> also mentioned in my earlier comment that if it is necessary to drop the "phy"
>> suffix, then I will do so and add another patch to change the string the driver
>> looks for as well.
>>
>> I shall take it that dropping "phy" suffixes is a rule/necessity. With this, I
>> will post the v3 series making this change, along with the patch to update the
>> string the driver looks for.
> 
> Drop the "phy" suffix.
> 
> It's a new binding. "phy" as suffix for "phy" is useless and for new
> bindings it should be dropped. If you submitted driver changes without
> bindings, which document the ABI, it's not good, but also not a reason
> for me for exceptions.

Thank you for clarifying. I will post the v3 series dropping the "phy" suffix
and also include the patch to change the name used in the driver to refer to the
Serdes PHY.

Regards,
Siddharth.