| Message ID | 20231214090304.16884-6-quic_luoj@quicinc.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | support ipq5332 platform | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Thu, Dec 14, 2023 at 05:03:04PM +0800, 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> Can you please wait until discussion has finished on a patchset before sending another version? I had not yet got a chance to look at the reply you sent to my comments on v2. Thanks, Conor. > --- > .../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 > - 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 > + oneOf: > + - items: > + - enum: > + - 0 # CMN PLL reference internal 48MHZ > + - 1 # CMN PLL reference external 25MHZ > + - 2 # CMN PLL reference external 31250KHZ > + - 3 # CMN PLL reference external 40MHZ > + - 4 # CMN PLL reference external 48MHZ > + - 5 # CMN PLL reference external 50MHZ > + - 6 # CMN PLL reference internal 96MHZ > + > + clock-frequency: > + oneOf: > + - items: > + - enum: > + - 12500000 > + - 6250000 > + - 3125000 > + - 1562500 > + - 781250 > + - 390625 > + description: > + The MDIO bus clock that must be output by the MDIO bus hardware, > + only the listed frequencies above can be supported, other frequency > + will cause malfunction. If absent, the default hardware value 0xff > + is used, which means the default MDIO clock frequency 390625HZ, The > + MDIO clock frequency is MDIO_SYS_CLK/(MDIO_CLK_DIV + 1), the SoC > + MDIO_SYS_CLK is fixed to 100MHZ, the MDIO_CLK_DIV is from MDIO control > + register, there is higher clock frequency requirement on the normal > + working case where the MDIO slave devices support high clock frequency. > + > + reset-gpios: > + maxItems: 1 > + > + reset-assert-us: > + maxItems: 1 > + > + reset-deassert-us: > + maxItems: 1 > > required: > - compatible > @@ -61,6 +121,8 @@ allOf: > - qcom,ipq5018-mdio > - qcom,ipq6018-mdio > - qcom,ipq8074-mdio > + - qcom,ipq5332-mdio > + - qcom,ipq9574-mdio > then: > required: > - clocks > @@ -70,6 +132,20 @@ allOf: > clocks: false > clock-names: false > > + - if: > + properties: > + compatible: > + contains: > + enum: > + - qcom,ipq5332-mdio > + then: > + properties: > + clocks: > + minItems: 5 > + maxItems: 5 > + reg-names: > + minItems: 4 > + > unevaluatedProperties: false > > examples: > @@ -100,3 +176,62 @@ examples: > reg = <4>; > }; > }; > + > + - | > + #include <dt-bindings/clock/qcom,ipq5332-gcc.h> > + #include <dt-bindings/gpio/gpio.h> > + > + mdio@90000 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "qcom,ipq5332-mdio", > + "qcom,ipq4019-mdio"; > + cmn-reference-clock = <0>; > + clock-frequency = <6250000>; > + > + reset-gpios = <&tlmm 51 GPIO_ACTIVE_LOW>; > + reset-assert-us = <100000>; > + reset-deassert-us = <100000>; > + > + reg = <0x90000 0x64>, > + <0x9B000 0x800>, > + <0x7A00610 0x4>, > + <0x7A10610 0x4>; > + > + reg-names = "mdio", > + "cmn_blk", > + "eth_ldo1", > + "eth_ldo2"; > + > + clocks = <&gcc GCC_MDIO_AHB_CLK>, > + <&gcc GCC_UNIPHY0_AHB_CLK>, > + <&gcc GCC_UNIPHY1_AHB_CLK>, > + <&gcc GCC_UNIPHY0_SYS_CLK>, > + <&gcc GCC_UNIPHY1_SYS_CLK>; > + > + clock-names = "gcc_mdio_ahb_clk", > + "uniphy0_ahb", > + "uniphy1_ahb", > + "uniphy0_sys", > + "uniphy1_sys"; > + > + qca8kphy0: ethernet-phy@1 { > + compatible = "ethernet-phy-id004d.d180"; > + reg = <1>; > + }; > + > + qca8kphy1: ethernet-phy@2 { > + compatible = "ethernet-phy-id004d.d180"; > + reg = <2>; > + }; > + > + qca8kphy2: ethernet-phy@3 { > + compatible = "ethernet-phy-id004d.d180"; > + reg = <3>; > + }; > + > + qca8kphy3: ethernet-phy@4 { > + compatible = "ethernet-phy-id004d.d180"; > + reg = <4>; > + }; > + }; > -- > 2.42.0 >
On 12/14/2023 11:58 PM, Conor Dooley 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> > Can you please wait until discussion has finished on a patchset before > sending another version? I had not yet got a chance to look at the > reply you sent to my comments on v2. > > Thanks, > Conor. got it, will keep this in mind, thanks for the suggestion.
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 > - 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 > + oneOf: > + - items: So it is enum or not? > + - enum: > + - 0 # CMN PLL reference internal 48MHZ > + - 1 # CMN PLL reference external 25MHZ > + - 2 # CMN PLL reference external 31250KHZ > + - 3 # CMN PLL reference external 40MHZ > + - 4 # CMN PLL reference external 48MHZ > + - 5 # CMN PLL reference external 50MHZ > + - 6 # CMN PLL reference internal 96MHZ > + > + clock-frequency: > + oneOf: > + - items: Same questions > + - enum: > + - 12500000 > + - 6250000 > + - 3125000 > + - 1562500 > + - 781250 > + - 390625 > + description: > + The MDIO bus clock that must be output by the MDIO bus hardware, > + only the listed frequencies above can be supported, other frequency > + will cause malfunction. If absent, the default hardware value 0xff > + is used, which means the default MDIO clock frequency 390625HZ, The > + MDIO clock frequency is MDIO_SYS_CLK/(MDIO_CLK_DIV + 1), the SoC > + MDIO_SYS_CLK is fixed to 100MHZ, the MDIO_CLK_DIV is from MDIO control > + register, there is higher clock frequency requirement on the normal > + working case where the MDIO slave devices support high clock frequency. > + > + reset-gpios: > + maxItems: 1 > + > + reset-assert-us: > + maxItems: 1 This does not look related to ipq5332. > + > + reset-deassert-us: > + maxItems: 1 Neither this. > > required: > - compatible > @@ -61,6 +121,8 @@ allOf: > - qcom,ipq5018-mdio > - qcom,ipq6018-mdio > - qcom,ipq8074-mdio > + - qcom,ipq5332-mdio > + - qcom,ipq9574-mdio > then: > required: > - clocks > @@ -70,6 +132,20 @@ allOf: > clocks: false > clock-names: false > > + - if: > + properties: > + compatible: > + contains: > + enum: > + - qcom,ipq5332-mdio > + then: > + properties: > + clocks: > + minItems: 5 > + maxItems: 5 > + reg-names: > + minItems: 4 > + > unevaluatedProperties: false > > examples: > @@ -100,3 +176,62 @@ examples: > reg = <4>; > }; > }; > + > + - | > + #include <dt-bindings/clock/qcom,ipq5332-gcc.h> > + #include <dt-bindings/gpio/gpio.h> > + > + mdio@90000 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "qcom,ipq5332-mdio", > + "qcom,ipq4019-mdio"; > + cmn-reference-clock = <0>; > + clock-frequency = <6250000>; > + > + reset-gpios = <&tlmm 51 GPIO_ACTIVE_LOW>; > + reset-assert-us = <100000>; > + reset-deassert-us = <100000>; > + > + reg = <0x90000 0x64>, > + <0x9B000 0x800>, > + <0x7A00610 0x4>, > + <0x7A10610 0x4>; Lowercase hex, wrong order of properties. Align it with coding style. Best regards, Krzysztof
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 >> - 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. > >> + oneOf: >> + - items: > > So it is enum or not? it's enum, i will remove the "oneOf: - items:" in the next patch set. > >> + - enum: >> + - 0 # CMN PLL reference internal 48MHZ >> + - 1 # CMN PLL reference external 25MHZ >> + - 2 # CMN PLL reference external 31250KHZ >> + - 3 # CMN PLL reference external 40MHZ >> + - 4 # CMN PLL reference external 48MHZ >> + - 5 # CMN PLL reference external 50MHZ >> + - 6 # CMN PLL reference internal 96MHZ >> + >> + clock-frequency: >> + oneOf: >> + - items: > > Same questions it's enum. > >> + - enum: >> + - 12500000 >> + - 6250000 >> + - 3125000 >> + - 1562500 >> + - 781250 >> + - 390625 >> + description: >> + The MDIO bus clock that must be output by the MDIO bus hardware, >> + only the listed frequencies above can be supported, other frequency >> + will cause malfunction. If absent, the default hardware value 0xff >> + is used, which means the default MDIO clock frequency 390625HZ, The >> + MDIO clock frequency is MDIO_SYS_CLK/(MDIO_CLK_DIV + 1), the SoC >> + MDIO_SYS_CLK is fixed to 100MHZ, the MDIO_CLK_DIV is from MDIO control >> + register, there is higher clock frequency requirement on the normal >> + working case where the MDIO slave devices support high clock frequency. >> + >> + 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. Without declaring these gpio properties, the warning will be reported by dt_binding_check. > >> + >> + reset-deassert-us: >> + maxItems: 1 > > Neither this. same as above. > >> >> required: >> - compatible >> @@ -61,6 +121,8 @@ allOf: >> - qcom,ipq5018-mdio >> - qcom,ipq6018-mdio >> - qcom,ipq8074-mdio >> + - qcom,ipq5332-mdio >> + - qcom,ipq9574-mdio >> then: >> required: >> - clocks >> @@ -70,6 +132,20 @@ allOf: >> clocks: false >> clock-names: false >> >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - qcom,ipq5332-mdio >> + then: >> + properties: >> + clocks: >> + minItems: 5 >> + maxItems: 5 >> + reg-names: >> + minItems: 4 >> + >> unevaluatedProperties: false >> >> examples: >> @@ -100,3 +176,62 @@ examples: >> reg = <4>; >> }; >> }; >> + >> + - | >> + #include <dt-bindings/clock/qcom,ipq5332-gcc.h> >> + #include <dt-bindings/gpio/gpio.h> >> + >> + mdio@90000 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "qcom,ipq5332-mdio", >> + "qcom,ipq4019-mdio"; >> + cmn-reference-clock = <0>; >> + clock-frequency = <6250000>; >> + >> + reset-gpios = <&tlmm 51 GPIO_ACTIVE_LOW>; >> + reset-assert-us = <100000>; >> + reset-deassert-us = <100000>; >> + >> + reg = <0x90000 0x64>, >> + <0x9B000 0x800>, >> + <0x7A00610 0x4>, >> + <0x7A10610 0x4>; > > Lowercase hex, wrong order of properties. Align it with coding style. will correct it in the next patch set, thanks. > > > > Best regards, > Krzysztof >
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? >>> - 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. ... >>> + 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. > > 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
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.
On 15/12/2023 11:03, Jie Luo wrote: >>>>> + 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. See example-schema about naming. Read writing-bindings. You need vendor prefix for custom properties. > >> >> ... >> >>>>> + 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. This argument does not make sense, sorry. Obviously if property is not allowed, it should be removed. Provide rationale, in terms of hardware, why this property must be added and why it cannot be deduced from the compatible. Best regards, Krzysztof
On 12/15/2023 6:21 PM, Krzysztof Kozlowski wrote: > On 15/12/2023 11:03, Jie Luo wrote: >>>>>> + 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. > > See example-schema about naming. Read writing-bindings. You need vendor > prefix for custom properties. Ok, 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. > > This argument does not make sense, sorry. Obviously if property is not > allowed, it should be removed. > > Provide rationale, in terms of hardware, why this property must be added > and why it cannot be deduced from the compatible. > > Best regards, > Krzysztof > So i can remove "reset-assert-us" and "reset-deassert-us" from the added example to avoid the dt check warning? even these two properties are needed to be defined in the device tree to make this driver working correctly.
On 15/12/2023 13:03, Jie Luo wrote: >>> 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. >> >> This argument does not make sense, sorry. Obviously if property is not >> allowed, it should be removed. >> >> Provide rationale, in terms of hardware, why this property must be added >> and why it cannot be deduced from the compatible. >> >> Best regards, >> Krzysztof >> > > So i can remove "reset-assert-us" and "reset-deassert-us" from the added > example to avoid the dt check warning? even these two properties are > needed to be defined in the device tree to make this driver working > correctly. Sorry, that does not answer my question at all. First, "Driver" is not hardware. My second question was simply ignored. In the v2 thread you as well respond with some short, unrelated sentences not answering to the real questions. It's a waste of my time. Please reach internally in Qualcomm for guidance how to upstream patches and how to write bindings. Best regards, Krzysztof
On 12/15/2023 8:14 PM, Krzysztof Kozlowski wrote: > On 15/12/2023 13:03, Jie Luo wrote: >>>> 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. >>> >>> This argument does not make sense, sorry. Obviously if property is not >>> allowed, it should be removed. >>> >>> Provide rationale, in terms of hardware, why this property must be added >>> and why it cannot be deduced from the compatible. >>> >>> Best regards, >>> Krzysztof >>> >> >> So i can remove "reset-assert-us" and "reset-deassert-us" from the added >> example to avoid the dt check warning? even these two properties are >> needed to be defined in the device tree to make this driver working >> correctly. > > Sorry, that does not answer my question at all. First, "Driver" is not > hardware. My second question was simply ignored. In the v2 thread you as > well respond with some short, unrelated sentences not answering to the > real questions. It's a waste of my time. Please reach internally in > Qualcomm for guidance how to upstream patches and how to write bindings. > > Best regards, > Krzysztof > These properties "reset-assert-us" and "reset-deassert-us" are the general properties from mdio.yaml, which are used when the MDIO bus driver is registered by the MDIO framework. The general DT property already supports to do the correct config, then compatible string is not needed to be checked for doing the configs. i will check the binding examples to avoid this kind of problems.
> These properties "reset-assert-us" and "reset-deassert-us" are the > general properties from mdio.yaml, which are used when the MDIO > bus driver is registered by the MDIO framework. > The general DT property already supports to do the correct config, > then compatible string is not needed to be checked for doing the > configs. Given the complexity of your device, i doubt you can make it work without using a compatible containing the ID register values. That will get your driver loaded, and the probe method called which can then deal with all the clocks and resets in whatever way it wants. Andrew
On 12/15/2023 9:34 PM, Andrew Lunn wrote: >> These properties "reset-assert-us" and "reset-deassert-us" are the >> general properties from mdio.yaml, which are used when the MDIO >> bus driver is registered by the MDIO framework. >> The general DT property already supports to do the correct config, >> then compatible string is not needed to be checked for doing the >> configs. > > Given the complexity of your device, i doubt you can make it work > without using a compatible containing the ID register values. That > will get your driver loaded, and the probe method called which can > then deal with all the clocks and resets in whatever way it wants. > > Andrew > I misunderstood Krzysztof's suggestion in the previous message, i thought the reset properties configuration is checked according to the compatible string in the drive code. Yes, these properties can be deduced from the compatible string in the DT doc, i will update this in the next patch set. Thanks for the comments and suggestion.
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 - 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 + oneOf: + - items: + - enum: + - 0 # CMN PLL reference internal 48MHZ + - 1 # CMN PLL reference external 25MHZ + - 2 # CMN PLL reference external 31250KHZ + - 3 # CMN PLL reference external 40MHZ + - 4 # CMN PLL reference external 48MHZ + - 5 # CMN PLL reference external 50MHZ + - 6 # CMN PLL reference internal 96MHZ + + clock-frequency: + oneOf: + - items: + - enum: + - 12500000 + - 6250000 + - 3125000 + - 1562500 + - 781250 + - 390625 + description: + The MDIO bus clock that must be output by the MDIO bus hardware, + only the listed frequencies above can be supported, other frequency + will cause malfunction. If absent, the default hardware value 0xff + is used, which means the default MDIO clock frequency 390625HZ, The + MDIO clock frequency is MDIO_SYS_CLK/(MDIO_CLK_DIV + 1), the SoC + MDIO_SYS_CLK is fixed to 100MHZ, the MDIO_CLK_DIV is from MDIO control + register, there is higher clock frequency requirement on the normal + working case where the MDIO slave devices support high clock frequency. + + reset-gpios: + maxItems: 1 + + reset-assert-us: + maxItems: 1 + + reset-deassert-us: + maxItems: 1 required: - compatible @@ -61,6 +121,8 @@ allOf: - qcom,ipq5018-mdio - qcom,ipq6018-mdio - qcom,ipq8074-mdio + - qcom,ipq5332-mdio + - qcom,ipq9574-mdio then: required: - clocks @@ -70,6 +132,20 @@ allOf: clocks: false clock-names: false + - if: + properties: + compatible: + contains: + enum: + - qcom,ipq5332-mdio + then: + properties: + clocks: + minItems: 5 + maxItems: 5 + reg-names: + minItems: 4 + unevaluatedProperties: false examples: @@ -100,3 +176,62 @@ examples: reg = <4>; }; }; + + - | + #include <dt-bindings/clock/qcom,ipq5332-gcc.h> + #include <dt-bindings/gpio/gpio.h> + + mdio@90000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "qcom,ipq5332-mdio", + "qcom,ipq4019-mdio"; + cmn-reference-clock = <0>; + clock-frequency = <6250000>; + + reset-gpios = <&tlmm 51 GPIO_ACTIVE_LOW>; + reset-assert-us = <100000>; + reset-deassert-us = <100000>; + + reg = <0x90000 0x64>, + <0x9B000 0x800>, + <0x7A00610 0x4>, + <0x7A10610 0x4>; + + reg-names = "mdio", + "cmn_blk", + "eth_ldo1", + "eth_ldo2"; + + clocks = <&gcc GCC_MDIO_AHB_CLK>, + <&gcc GCC_UNIPHY0_AHB_CLK>, + <&gcc GCC_UNIPHY1_AHB_CLK>, + <&gcc GCC_UNIPHY0_SYS_CLK>, + <&gcc GCC_UNIPHY1_SYS_CLK>; + + clock-names = "gcc_mdio_ahb_clk", + "uniphy0_ahb", + "uniphy1_ahb", + "uniphy0_sys", + "uniphy1_sys"; + + qca8kphy0: ethernet-phy@1 { + compatible = "ethernet-phy-id004d.d180"; + reg = <1>; + }; + + qca8kphy1: ethernet-phy@2 { + compatible = "ethernet-phy-id004d.d180"; + reg = <2>; + }; + + qca8kphy2: ethernet-phy@3 { + compatible = "ethernet-phy-id004d.d180"; + reg = <3>; + }; + + qca8kphy3: ethernet-phy@4 { + compatible = "ethernet-phy-id004d.d180"; + reg = <4>; + }; + };
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(-)