Received-SPF: pass (google.com: domain of linux-kernel+bounces-720-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <3de98516-9a28-4f58-8951-2a7752621fee@quicinc.com>
Date: Fri, 15 Dec 2023 18:03:33 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332
 platform
Content-Language: en-US
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, <agross@kernel.org>,
        <andersson@kernel.org>, <konrad.dybcio@linaro.org>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <andrew@lunn.ch>, <hkallweit1@gmail.com>, <linux@armlinux.org.uk>,
        <robert.marko@sartura.hr>
CC: <linux-arm-msm@vger.kernel.org>, <netdev@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <quic_srichara@quicinc.com>
References: <20231214090304.16884-1-quic_luoj@quicinc.com>
 <20231214090304.16884-6-quic_luoj@quicinc.com>
 <1e4c55c8-8ed6-4f2e-8328-8a173f09b62f@linaro.org>
 <3c8e33b4-6ebc-476f-a00f-15cc8a1ad9e4@quicinc.com>
 <b89abf8c-57f8-46a6-a071-b1591340fbdf@linaro.org>
From: Jie Luo <quic_luoj@quicinc.com>
In-Reply-To: <b89abf8c-57f8-46a6-a071-b1591340fbdf@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 12/15/2023 4:39 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 09:28, Jie Luo wrote:
>>
>>
>> On 12/15/2023 3:27 PM, Krzysztof Kozlowski wrote:
>>> On 14/12/2023 10:03, Luo Jie wrote:
>>>> Update the yaml file for the new DTS properties.
>>>>
>>>> 1. cmn-reference-clock for the CMN PLL source clock select.
>>>> 2. clock-frequency for MDIO clock frequency config.
>>>> 3. add uniphy AHB & SYS GCC clocks.
>>>> 4. add reset-gpios for MDIO bus level reset.
>>>>
>>>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>>>> ---
>>>>    .../bindings/net/qcom,ipq4019-mdio.yaml       | 143 +++++++++++++++++-
>>>>    1 file changed, 139 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>> index 3407e909e8a7..79f8513739e7 100644
>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>> @@ -20,6 +20,8 @@ properties:
>>>>              - enum:
>>>>                  - qcom,ipq6018-mdio
>>>>                  - qcom,ipq8074-mdio
>>>> +              - qcom,ipq9574-mdio
>>>> +              - qcom,ipq5332-mdio
> 
> Why do you add entries to the end of the list? In reversed order?

Thanks for pointing it out, i will move "- qcom,ipq5332-mdio" before
"- qcom,ipq6018-mdio".

> 
>>>>              - const: qcom,ipq4019-mdio
>>>>    
>>>>      "#address-cells":
>>>> @@ -30,19 +32,77 @@ properties:
>>>>    
>>>>      reg:
>>>>        minItems: 1
>>>> -    maxItems: 2
>>>> +    maxItems: 5
>>>>        description:
>>>> -      the first Address and length of the register set for the MDIO controller.
>>>> -      the second Address and length of the register for ethernet LDO, this second
>>>> -      address range is only required by the platform IPQ50xx.
>>>> +      the first Address and length of the register set for the MDIO controller,
>>>> +      the optional second, third and fourth address and length of the register
>>>> +      for ethernet LDO, these three address range are required by the platform
>>>> +      IPQ50xx/IPQ5332, the last address and length is for the CMN clock to
>>>> +      select the reference clock.
>>>> +
>>>> +  reg-names:
>>>> +    minItems: 1
>>>> +    maxItems: 5
>>>>    
>>>>      clocks:
>>>> +    minItems: 1
>>>>        items:
>>>>          - description: MDIO clock source frequency fixed to 100MHZ
>>>> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>>>> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>>>> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>>> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>>>    
>>>>      clock-names:
>>>> +    minItems: 1
>>>>        items:
>>>>          - const: gcc_mdio_ahb_clk
>>>> +      - const: uniphy0_ahb
>>>> +      - const: uniphy1_ahb
>>>> +      - const: uniphy0_sys
>>>> +      - const: uniphy1_sys
>>>> +
>>>> +  cmn-reference-clock:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>
>>> Nothing improved here
>>
>> With this change, the warning is not reported when i run
>> dt_binding_check, looks the new added property needs
>> the type ref to avoid the warning reported.
> 
> Nothing improved in the property name, nor its style, nor in the actual
> contents/values.

This property is for CMN PLL block reference clock configuration,
so i use this property name.

it will be appreciated if you can suggest a suitable name, thanks.

> 
> ...
> 
>>>> +  reset-gpios:
>>>> +    maxItems: 1
>>>> +
>>>> +  reset-assert-us:
>>>> +    maxItems: 1
>>>
>>> This does not look related to ipq5332.
>>
>> The reset gpio properties are needed on ipq5332, since qca8084 phy is
>> connected, which uses the MDIO bus level gpio reset.
> 
> I am talking about this property, not these properties.

ok.

> 
>>
>> Without declaring these gpio properties, the warning will be reported
>> by dt_binding_check.
> 
> How is it even possible to have warnings if there is no such node in
> DTS? We do not care about warnings in your downstream code.
> 
> 
> Best regards,
> Krzysztof
> 

If i do not declare the property "reset-assert-us" and 
"reset-deassert-us", the warning will be reported by "make 
dt_binding_check" since i
add a example in this file.