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 <[email protected]>
> ---
> .../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.
>
> 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?
> + 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?
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.
> +
> + 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.
...
> +
> + 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.
Best regards,
Krzysztof
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 <[email protected]>
>> ---
>> .../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
>
On 17/11/2023 11:36, Jie Luo wrote:
>>> 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.
OTHER variants. Not this one.
>
>>
>>> + 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
No clue what's this, but no, I do not believe you tested it at all. It's
not about yamllint. It's was not tested. Look at errors reported on
mailing list.
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
Best regards,
Krzysztof
On 11/17/2023 6:40 PM, Krzysztof Kozlowski wrote:
> On 17/11/2023 11:36, Jie Luo wrote:
>>>> 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.
>
> OTHER variants. Not this one.
The change here is for the clock number added for the ipq5332 platform,
the other platforms still use only legacy MDIO clock.
so i add minItems and maxItems, i will check other .yaml files for the
reference.
>
>>
>>>
>>>> + 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
>
> No clue what's this, but no, I do not believe you tested it at all. It's
> not about yamllint. It's was not tested. Look at errors reported on
> mailing list.
>
>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>
Hi Krzysztof,
Here is the output when i run the dt check with this patch applied in my
workspace, md64 is the alias for compiling the arm64 platform.
md64 dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
warning: python package 'yamllint' not installed, skipping
CHKDT Documentation/devicetree/bindings/processed-schema.json
SCHEMA Documentation/devicetree/bindings/processed-schema.json
DTEX
Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.example.dts
DTC_CHK
Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.example.dtb
>
> Best regards,
> Krzysztof
>
On 17/11/2023 12:20, Jie Luo wrote:
>
>
> On 11/17/2023 6:40 PM, Krzysztof Kozlowski wrote:
>> On 17/11/2023 11:36, Jie Luo wrote:
>>>>> 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.
>>
>> OTHER variants. Not this one.
>
> The change here is for the clock number added for the ipq5332 platform,
> the other platforms still use only legacy MDIO clock.
Then your patch is wrong as I said. You now affect other variants. I
don't quite get your responses. Style of them suggests that you
disagree, but you are not providing any relevant argument.
>
> so i add minItems and maxItems, i will check other .yaml files for the
> reference.
>
>>
>>>
>>>>
>>>>> + 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
>>
>> No clue what's this, but no, I do not believe you tested it at all. It's
>> not about yamllint. It's was not tested. Look at errors reported on
>> mailing list.
>>
>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>
>
> Hi Krzysztof,
> Here is the output when i run the dt check with this patch applied in my
> workspace, md64 is the alias for compiling the arm64 platform.
We still do not know your base and dtschema version.
Best regards,
Krzysztof
On 11/17/2023 8:43 PM, Krzysztof Kozlowski wrote:
> On 17/11/2023 12:20, Jie Luo wrote:
>>
>>
>> On 11/17/2023 6:40 PM, Krzysztof Kozlowski wrote:
>>> On 17/11/2023 11:36, Jie Luo wrote:
>>>>>> 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.
>>>
>>> OTHER variants. Not this one.
>>
>> The change here is for the clock number added for the ipq5332 platform,
>> the other platforms still use only legacy MDIO clock.
>
> Then your patch is wrong as I said. You now affect other variants. I
> don't quite get your responses. Style of them suggests that you
> disagree, but you are not providing any relevant argument.
The clock arguments are provided in the later part as below. i will also
provide more detail clock names for the new added clocks for the ipq5332
platform in description.
- if:
properties:
compatible:
contains:
enum:
- qcom,ipq5332-mdio
then:
properties:
clocks:
items:
- description: MDIO clock source frequency fixed to 100MHZ
- description: UNIPHY0 AHB clock source frequency fixed to
100MHZ
- description: UNIPHY0 SYS clock source frequency fixed to
24MHZ
- description: UNIPHY1 AHB clock source frequency fixed to
100MHZ
- description: UNIPHY1 SYS clock source frequency fixed to
24MHZ
clock-names:
items:
- const: gcc_mdio_ahb_clk
- const: gcc_uniphy0_ahb_clk
- const: gcc_uniphy0_sys_clk
- const: gcc_uniphy1_ahb_clk
- const: gcc_uniphy1_sys_clk
>
>>
>> so i add minItems and maxItems, i will check other .yaml files for the
>> reference.
>>
>>>
>>>>
>>>>>
>>>>>> + 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
>>>
>>> No clue what's this, but no, I do not believe you tested it at all. It's
>>> not about yamllint. It's was not tested. Look at errors reported on
>>> mailing list.
>>>
>>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>
>>
>> Hi Krzysztof,
>> Here is the output when i run the dt check with this patch applied in my
>> workspace, md64 is the alias for compiling the arm64 platform.
>
> We still do not know your base and dtschema version.
The code base is the commit id:
5ba73bec5e7b0494da7fdca3e003d8b97fa932cd
<Add linux-next specific files for 20231114>
The dtschema version is as below.
dt-doc-validate --version
2023.9
>
>
> Best regards,
> Krzysztof
>
> The clock arguments are provided in the later part as below. i will also
> provide more detail clock names for the new added clocks for the ipq5332
> platform in description.
>
> - if:
>
> properties:
>
> compatible:
>
> contains:
>
> enum:
>
> - qcom,ipq5332-mdio
>
> then:
>
> properties:
>
> clocks:
>
> items:
>
> - description: MDIO clock source frequency fixed to 100MHZ
>
> - description: UNIPHY0 AHB clock source frequency fixed to
> 100MHZ
> - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
> - description: UNIPHY1 AHB clock source frequency fixed to
> 100MHZ
> - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
As i said before, the frequency of the clocks does not matter
here. That appears to be the drivers problem. I assume every board
design, with any sort of PHY, needs the same clock configuration?
Andrew
On 11/18/2023 11:36 PM, Andrew Lunn wrote:
>> The clock arguments are provided in the later part as below. i will also
>> provide more detail clock names for the new added clocks for the ipq5332
>> platform in description.
>>
>> - if:
>>
>> properties:
>>
>> compatible:
>>
>> contains:
>>
>> enum:
>>
>> - qcom,ipq5332-mdio
>>
>> then:
>>
>> properties:
>>
>> clocks:
>>
>> items:
>>
>> - description: MDIO clock source frequency fixed to 100MHZ
>>
>> - description: UNIPHY0 AHB clock source frequency fixed to
>> 100MHZ
>> - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>> - description: UNIPHY1 AHB clock source frequency fixed to
>> 100MHZ
>> - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>
> As i said before, the frequency of the clocks does not matter
> here. That appears to be the drivers problem. I assume every board
> design, with any sort of PHY, needs the same clock configuration?
>
> Andrew
Yes, Andrew, no matter what kind of PHY is connected, these clocks are
fix clocks, the clock rates are same as mentioned above, which are the
SOC clock configurations.