Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <76e081ba-9d5a-41df-9c1b-d782e5656973@quicinc.com>
Date:   Fri, 17 Nov 2023 18:36:18 +0800
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 9/9] 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: <20231115032515.4249-1-quic_luoj@quicinc.com>
 <20231115032515.4249-10-quic_luoj@quicinc.com>
 <834cbb58-3a88-4ba6-8db6-10440a4d0893@linaro.org>
From:   Jie Luo <quic_luoj@quicinc.com>
In-Reply-To: <834cbb58-3a88-4ba6-8db6-10440a4d0893@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 11/16/2023 7:56 PM, Krzysztof Kozlowski wrote:
> On 15/11/2023 04:25, Luo Jie wrote:
>> On platform IPQ5332, the MDIO address of qca8084 can be programed
>> when the device tree property "fixup" defined, the clock sequence
>> needs to be completed before the PHY probeable.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 138 +++++++++++++++++-
>>   1 file changed, 130 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> index 3407e909e8a7..7ff92be14ee1 100644
>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> @@ -15,11 +15,13 @@ properties:
>>         - enum:
>>             - qcom,ipq4019-mdio
>>             - qcom,ipq5018-mdio
>> +          - qcom,ipq5332-mdio
>>   
>>         - items:
>>             - enum:
>>                 - qcom,ipq6018-mdio
>>                 - qcom,ipq8074-mdio
>> +              - qcom,ipq9574-mdio
>>             - const: qcom,ipq4019-mdio
>>   
>>     "#address-cells":
>> @@ -30,19 +32,47 @@ 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
> 
> You must describe the items and constrain them per each variant.

Ok, will describe these items one by one in the next patch set.

> 
>>   
>>     clocks:
>> -    items:
>> -      - description: MDIO clock source frequency fixed to 100MHZ
>> +    minItems: 1
>> +    maxItems: 5
>> +    description:
> 
> Doesn't this make all other variants with incorrect constraints?

There are 5 clock items, the first one is the legacy MDIO clock, the
other clocks are new added for ipq5332 platform, will describe it more
clearly in the next patch set.

> 
>> +      MDIO system clock frequency fixed to 100MHZ, and the GCC uniphy
>> +      clocks enabled for resetting ethernet PHY.
>>   
>>     clock-names:
>> -    items:
>> -      - const: gcc_mdio_ahb_clk
>> +    minItems: 1
>> +    maxItems: 5
>> +
>> +  phy-reset-gpio:
> 
> No, for multiple reasons. It's gpios first of all. Where do you see such
> property? Where is the existing definition?

will remove this property, and update to use the exited PHY GPIO reset.

> 
> Then it is "reset-gpios" if this is MDIO. Why do you put phy properties
> in MDIO?
> 
>> +    minItems: 1
>> +    maxItems: 3
>> +    description:
>> +      GPIO used to reset the PHY, each GPIO is for resetting the connected
>> +      ethernet PHY device.
>> +
>> +  phyaddr-fixup:
>> +    description: Register address for programing MDIO address of PHY devices
> 
> You did not test code which you sent.

Hi Krzysztof,
This patch is passed with the following command in my workspace.
i will upgrade and install yamllint to make sure there is no
warning reported anymore.

make dt_bg_check 
DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml

> 
>> +
>> +  pcsaddr-fixup:
>> +    description: Register address for programing MDIO address of PCS devices
>> +
>> +  mdio-clk-fixup:
>> +    description: The initialization clocks to be configured
>> +
>> +  fixup:
>> +    description: The MDIO address of PHY/PCS device to be programed
> 
> Please do not send untested code.
> 

Ok, will complete the full test before uploading the patch.

> 
> ...
> 
>> +
>> +      qca8kphy2: ethernet-phy@3 {
>> +        reg = <3>;
>> +        fixup;
>> +      };
>> +
>> +      qca8kphy3: ethernet-phy@4 {
>> +        reg = <4>;
>> +        fixup;
>> +      };
>> +
>> +      qca8kpcs0: pcsphy0@5 {
>> +        compatible = "qcom,qca8k_pcs";
>> +        reg = <5>;
>> +        fixup;
>> +        };
> 
> Fix indentation.

Will Fix it in the next patch set, thanks.

> 
> Best regards,
> Krzysztof
>