| Message ID | 20231212115151.20016-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/dt-meta-schema | fail | build log |
On Tue, 12 Dec 2023 19:51:50 +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> > --- > .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++- > 1 file changed, 153 insertions(+), 4 deletions(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml: cmn-reference-clock: missing type definition doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231212115151.20016-6-quic_luoj@quicinc.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Tue, Dec 12, 2023 at 07:51:50PM +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> > --- > .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++- > 1 file changed, 153 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..9546a6ad7841 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,71 @@ 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/IPQ9574, 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: gcc_uniphy0_ahb_clk > + - const: gcc_uniphy1_ahb_clk > + - const: gcc_uniphy0_sys_clk > + - const: gcc_uniphy1_sys_clk > + cmn-reference-clock: > + 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 Why is this not represented by an element of the clocks property? > + 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 frequecies above can be configured, other frequency > + will cause malfunction. If absent, the default hardware value is used. Likewise. Your commit message contains a bullet point list of what you are doing, but there's no explanation here for why custom properties are required to provide clock information. Thanks, Conor.
On Tue, Dec 12, 2023 at 07:51:50PM +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> > --- > .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++- > 1 file changed, 153 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..9546a6ad7841 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 A driver can function without knowing about all these new registers and clocks? If not, then it can't be compatible with "qcom,ipq4019-mdio". > > "#address-cells": > @@ -30,19 +32,71 @@ 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/IPQ9574, 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 These are all clock inputs to this h/w block and not some other clocks you want to manage? > > clock-names: > + minItems: 1 > items: > - const: gcc_mdio_ahb_clk > + - const: gcc_uniphy0_ahb_clk > + - const: gcc_uniphy1_ahb_clk > + - const: gcc_uniphy0_sys_clk > + - const: gcc_uniphy1_sys_clk "gcc" is presumably the name of the clock controller in QCom chips. Well, the clock source should not be part of the binding. The names should be local for what they are for. So drop 'gcc_'. And '_clk' is also redundant, so drop it too. Unfortunately you are stuck with the name of the 1st entry. > + > + cmn-reference-clock: > + 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 frequecies above can be configured, other frequency > + will cause malfunction. If absent, the default hardware value is used. > + > + reset-gpios: > + maxItems: 1 > + > + reset-assert-us: > + maxItems: 1 > + > + reset-deassert-us: > + maxItems: 1 > > required: > - compatible > @@ -61,6 +115,8 @@ allOf: > - qcom,ipq5018-mdio > - qcom,ipq6018-mdio > - qcom,ipq8074-mdio > + - qcom,ipq5332-mdio > + - qcom,ipq9574-mdio > then: > required: > - clocks > @@ -70,6 +126,40 @@ allOf: > clocks: false > clock-names: false > > + - if: > + properties: > + compatible: > + contains: > + enum: > + - qcom,ipq5332-mdio > + then: > + properties: > + clocks: > + minItems: 5 > + maxItems: 5 > + reg-names: > + items: > + - const: mdio > + - const: eth_ldo1 > + - const: eth_ldo2 > + - const: cmn_blk Perhaps cmn_blk should come 2nd, so all the variants have the same entry indices. Then you can move this to the top level and just say 'minItems: 4' here. > + > + - if: > + properties: > + compatible: > + contains: > + enum: > + - qcom,ipq9574-mdio > + then: > + properties: > + reg-names: > + items: > + - const: mdio > + - const: eth_ldo1 > + - const: eth_ldo2 > + - const: eth_ldo3 > + - const: cmn_blk And 'minItems: 5' here. The ipq9574 adds the CMN block, but none of the clocks? Weird. Rob
On 12/13/2023 12:06 AM, Conor Dooley wrote: > On Tue, Dec 12, 2023 at 07:51:50PM +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> >> --- >> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++- >> 1 file changed, 153 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..9546a6ad7841 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,71 @@ 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/IPQ9574, 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: gcc_uniphy0_ahb_clk >> + - const: gcc_uniphy1_ahb_clk >> + - const: gcc_uniphy0_sys_clk >> + - const: gcc_uniphy1_sys_clk > >> + cmn-reference-clock: >> + 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 > > Why is this not represented by an element of the clocks property? This property is for the reference clock source selection of CMN PLL, CMN PLL generates the different clock rates for the different Ethernet blocks, this CMN PLL configuration is not located in the GCC, so the clock framework can't be used, which is the general hardware register instead of RCG register for GCC. > >> + 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 frequecies above can be configured, other frequency >> + will cause malfunction. If absent, the default hardware value is used. > > Likewise. > > Your commit message contains a bullet point list of what you are doing, > but there's no explanation here for why custom properties are required > to provide clock information. > > Thanks, > Conor. Hi Conor, This property clock-frequency is optional to configure the MDIO working clock rate, and this is the MDIO general DT property, since the hardware default clock rate is 390625HZ, there is requirement for higher clock rate in the normal working case, i will update this information in the next patch set.
On 12/13/2023 4:06 AM, Rob Herring wrote: > On Tue, Dec 12, 2023 at 07:51:50PM +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> >> --- >> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++- >> 1 file changed, 153 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..9546a6ad7841 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 > > A driver can function without knowing about all these new registers and > clocks? If not, then it can't be compatible with "qcom,ipq4019-mdio". Yes, the driver can work without knowing the compatible string. the configuration is decided by the DT property defined or not. > >> >> "#address-cells": >> @@ -30,19 +32,71 @@ 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/IPQ9574, 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 > > These are all clock inputs to this h/w block and not some other clocks > you want to manage? Yes, for ipq5332, these 5 clocks are need to be managed, for the legacy platform such as ipq8074, only MDIO clock is needed. No other more clock needs to be managed for the current IPQ platforms. > >> >> clock-names: >> + minItems: 1 >> items: >> - const: gcc_mdio_ahb_clk >> + - const: gcc_uniphy0_ahb_clk >> + - const: gcc_uniphy1_ahb_clk >> + - const: gcc_uniphy0_sys_clk >> + - const: gcc_uniphy1_sys_clk > > "gcc" is presumably the name of the clock controller in QCom chips. > Well, the clock source should not be part of the binding. The names > should be local for what they are for. So drop 'gcc_'. And '_clk' is > also redundant, so drop it too. Unfortunately you are stuck with the > name of the 1st entry. Yes, gcc is the name of QCOM SOC clock controller. will remove the "gcc_" and "_clk" for the new added clocks. we should keep the existed DT gcc_mdio_ahb_clk unmodified, right? since it has been used in the current device tree. > >> + >> + cmn-reference-clock: >> + 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 frequecies above can be configured, other frequency >> + will cause malfunction. If absent, the default hardware value is used. >> + >> + reset-gpios: >> + maxItems: 1 >> + >> + reset-assert-us: >> + maxItems: 1 >> + >> + reset-deassert-us: >> + maxItems: 1 >> >> required: >> - compatible >> @@ -61,6 +115,8 @@ allOf: >> - qcom,ipq5018-mdio >> - qcom,ipq6018-mdio >> - qcom,ipq8074-mdio >> + - qcom,ipq5332-mdio >> + - qcom,ipq9574-mdio >> then: >> required: >> - clocks >> @@ -70,6 +126,40 @@ allOf: >> clocks: false >> clock-names: false >> >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - qcom,ipq5332-mdio >> + then: >> + properties: >> + clocks: >> + minItems: 5 >> + maxItems: 5 >> + reg-names: >> + items: >> + - const: mdio >> + - const: eth_ldo1 >> + - const: eth_ldo2 >> + - const: cmn_blk > > Perhaps cmn_blk should come 2nd, so all the variants have the same entry > indices. Then you can move this to the top level and just say 'minItems: > 4' here. Thanks Rob for the suggestion, i will update to move cmn_blk to the 2nd location. > > >> + >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - qcom,ipq9574-mdio >> + then: >> + properties: >> + reg-names: >> + items: >> + - const: mdio >> + - const: eth_ldo1 >> + - const: eth_ldo2 >> + - const: eth_ldo3 >> + - const: cmn_blk > > And 'minItems: 5' here. > > The ipq9574 adds the CMN block, but none of the clocks? Weird. > > Rob For ipq9574, only mdio clock is needed, the uniphy ahb and sys clock is not needed to configure. Yes, there is some Ethernet design delta between ipq9574 and ipq5332.
On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote: > > > On 12/13/2023 12:06 AM, Conor Dooley wrote: > > On Tue, Dec 12, 2023 at 07:51:50PM +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> > > > --- > > > .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++- > > > 1 file changed, 153 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..9546a6ad7841 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,71 @@ 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/IPQ9574, 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: gcc_uniphy0_ahb_clk > > > + - const: gcc_uniphy1_ahb_clk > > > + - const: gcc_uniphy0_sys_clk > > > + - const: gcc_uniphy1_sys_clk > > > > > + cmn-reference-clock: > > > + 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 > > > > Why is this not represented by an element of the clocks property? > > This property is for the reference clock source selection of CMN PLL, > CMN PLL generates the different clock rates for the different Ethernet > blocks, this CMN PLL configuration is not located in the GCC, so the > clock framework can't be used, which is the general hardware register > instead of RCG register for GCC. I don't see how the clock being provided by the "GCC" (whatever that is) or by some other clock controller or fixed clock makes a difference. Why can't the other clock provider be represented in the devicetree? > > > + 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 frequecies above can be configured, other frequency > > > + will cause malfunction. If absent, the default hardware value is used. > > > > Likewise. > > > > Your commit message contains a bullet point list of what you are doing, > > but there's no explanation here for why custom properties are required > > to provide clock information. > This property clock-frequency is optional to configure the MDIO working > clock rate, and this is the MDIO general DT property, since the hardware > default clock rate is 390625HZ, there is requirement for higher clock rate > in the normal working case, i will update this information in the > next patch set. I'm just realising that this particular one is not a custom property, the unusual `oneOf: - items: - enum:` structure here threw me. This can just be clock-frequency: enum: - 12500000 - 6250000 - 3125000 - 1562500 - 781250 - 390625 but you're missing a default, given your commit about the last element in that list being one. Thanks, Conor.
On 12/15/2023 1:12 AM, Conor Dooley wrote: > On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote: >> >> >> On 12/13/2023 12:06 AM, Conor Dooley wrote: >>> On Tue, Dec 12, 2023 at 07:51:50PM +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> >>>> --- >>>> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++- >>>> 1 file changed, 153 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..9546a6ad7841 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,71 @@ 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/IPQ9574, 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: gcc_uniphy0_ahb_clk >>>> + - const: gcc_uniphy1_ahb_clk >>>> + - const: gcc_uniphy0_sys_clk >>>> + - const: gcc_uniphy1_sys_clk >>> >>>> + cmn-reference-clock: >>>> + 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 >>> >>> Why is this not represented by an element of the clocks property? >> >> This property is for the reference clock source selection of CMN PLL, >> CMN PLL generates the different clock rates for the different Ethernet >> blocks, this CMN PLL configuration is not located in the GCC, so the >> clock framework can't be used, which is the general hardware register >> instead of RCG register for GCC. > > I don't see how the clock being provided by the "GCC" (whatever that is) > or by some other clock controller or fixed clock makes a difference. > Why can't the other clock provider be represented in the devicetree? > cmn-reference-clock is for selecting the reference clock source for the whole Ethernet block, which is just the standalone configure register. however the clock provider has the logical register distribution, such as for one clock tree, there is RCG, DIVIDER and branch registers in the qcom soc chip. The clock consumer defines the clock IDs of device tree to reference the clocks provided by the clock controller, and these clock IDs are provided by the header file of clock provider. like this, 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>; gcc is the device node of clock provider, and GCC_MDIO_AHB_CLK is the clock ID. >>>> + 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 frequecies above can be configured, other frequency >>>> + will cause malfunction. If absent, the default hardware value is used. >>> >>> Likewise. >>> >>> Your commit message contains a bullet point list of what you are doing, >>> but there's no explanation here for why custom properties are required >>> to provide clock information. > >> This property clock-frequency is optional to configure the MDIO working >> clock rate, and this is the MDIO general DT property, since the hardware >> default clock rate is 390625HZ, there is requirement for higher clock rate >> in the normal working case, i will update this information in the >> next patch set. > > I'm just realising that this particular one is not a custom property, > the unusual `oneOf: - items: - enum:` structure here threw me. This can > just be > clock-frequency: > enum: > - 12500000 > - 6250000 > - 3125000 > - 1562500 > - 781250 > - 390625 > > but you're missing a default, given your commit about the last element > in that list being one. > > Thanks, > Conor. Ok, will update this in the next patch set, thanks for the comments.
On 15/12/2023 07:46, Jie Luo wrote: > > > On 12/15/2023 1:12 AM, Conor Dooley wrote: >> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote: >>> >>> >>> On 12/13/2023 12:06 AM, Conor Dooley wrote: >>>> On Tue, Dec 12, 2023 at 07:51:50PM +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> >>>>> --- >>>>> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++- >>>>> 1 file changed, 153 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..9546a6ad7841 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,71 @@ 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/IPQ9574, 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: gcc_uniphy0_ahb_clk >>>>> + - const: gcc_uniphy1_ahb_clk >>>>> + - const: gcc_uniphy0_sys_clk >>>>> + - const: gcc_uniphy1_sys_clk >>>> >>>>> + cmn-reference-clock: >>>>> + 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 >>>> >>>> Why is this not represented by an element of the clocks property? >>> >>> This property is for the reference clock source selection of CMN PLL, >>> CMN PLL generates the different clock rates for the different Ethernet >>> blocks, this CMN PLL configuration is not located in the GCC, so the >>> clock framework can't be used, which is the general hardware register >>> instead of RCG register for GCC. >> >> I don't see how the clock being provided by the "GCC" (whatever that is) >> or by some other clock controller or fixed clock makes a difference. >> Why can't the other clock provider be represented in the devicetree? >> > > cmn-reference-clock is for selecting the reference clock source for the > whole Ethernet block, which is just the standalone configure register. Sure, you are aware though that all clocks are just configure registers? Which clocks are these mentioned in the property? From where do they come? Anyway, property is in existing form is not correct - this is not a generic property. > however the clock provider has the logical register distribution, such > as for one clock tree, there is RCG, DIVIDER and branch registers in > the qcom soc chip. > > The clock consumer defines the clock IDs of device tree to reference the > clocks provided by the clock controller, and these clock IDs are > provided by the header file of clock provider. > > like this, > 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>; > > gcc is the device node of clock provider, and GCC_MDIO_AHB_CLK is the > clock ID. Best regards, Krzysztof
On 12/15/2023 3:29 PM, Krzysztof Kozlowski wrote: > On 15/12/2023 07:46, Jie Luo wrote: >> >> >> On 12/15/2023 1:12 AM, Conor Dooley wrote: >>> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote: >>>> >>>> >>>> On 12/13/2023 12:06 AM, Conor Dooley wrote: >>>>> On Tue, Dec 12, 2023 at 07:51:50PM +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> >>>>>> --- >>>>>> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++- >>>>>> 1 file changed, 153 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..9546a6ad7841 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,71 @@ 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/IPQ9574, 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: gcc_uniphy0_ahb_clk >>>>>> + - const: gcc_uniphy1_ahb_clk >>>>>> + - const: gcc_uniphy0_sys_clk >>>>>> + - const: gcc_uniphy1_sys_clk >>>>> >>>>>> + cmn-reference-clock: >>>>>> + 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 >>>>> >>>>> Why is this not represented by an element of the clocks property? >>>> >>>> This property is for the reference clock source selection of CMN PLL, >>>> CMN PLL generates the different clock rates for the different Ethernet >>>> blocks, this CMN PLL configuration is not located in the GCC, so the >>>> clock framework can't be used, which is the general hardware register >>>> instead of RCG register for GCC. >>> >>> I don't see how the clock being provided by the "GCC" (whatever that is) >>> or by some other clock controller or fixed clock makes a difference. >>> Why can't the other clock provider be represented in the devicetree? >>> >> >> cmn-reference-clock is for selecting the reference clock source for the >> whole Ethernet block, which is just the standalone configure register. > > Sure, you are aware though that all clocks are just configure registers? > > Which clocks are these mentioned in the property? From where do they come? > > Anyway, property is in existing form is not correct - this is not a > generic property. > This property cmn-reference-clock is just the hardware register configuration, since the different IPQ platform needs to select the different reference clock source for the CMN PLL block that provides the various clock outputs to the all kinds of Ethernet devices, which is not from GCC provider. This is indeed not a generic property, which is the Ethernet function configs same as clock-frequency. > >> however the clock provider has the logical register distribution, such >> as for one clock tree, there is RCG, DIVIDER and branch registers in >> the qcom soc chip. >> >> The clock consumer defines the clock IDs of device tree to reference the >> clocks provided by the clock controller, and these clock IDs are >> provided by the header file of clock provider. >> >> like this, >> 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>; >> >> gcc is the device node of clock provider, and GCC_MDIO_AHB_CLK is the >> clock ID. > > > > Best regards, > Krzysztof >
On 15/12/2023 10:49, Jie Luo wrote: > > > On 12/15/2023 3:29 PM, Krzysztof Kozlowski wrote: >> On 15/12/2023 07:46, Jie Luo wrote: >>> >>> >>> On 12/15/2023 1:12 AM, Conor Dooley wrote: >>>> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote: >>>>> >>>>> >>>>> On 12/13/2023 12:06 AM, Conor Dooley wrote: >>>>>> On Tue, Dec 12, 2023 at 07:51:50PM +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> >>>>>>> --- >>>>>>> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++- >>>>>>> 1 file changed, 153 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..9546a6ad7841 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,71 @@ 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/IPQ9574, 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: gcc_uniphy0_ahb_clk >>>>>>> + - const: gcc_uniphy1_ahb_clk >>>>>>> + - const: gcc_uniphy0_sys_clk >>>>>>> + - const: gcc_uniphy1_sys_clk >>>>>> >>>>>>> + cmn-reference-clock: >>>>>>> + 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 >>>>>> >>>>>> Why is this not represented by an element of the clocks property? >>>>> >>>>> This property is for the reference clock source selection of CMN PLL, >>>>> CMN PLL generates the different clock rates for the different Ethernet >>>>> blocks, this CMN PLL configuration is not located in the GCC, so the >>>>> clock framework can't be used, which is the general hardware register >>>>> instead of RCG register for GCC. >>>> >>>> I don't see how the clock being provided by the "GCC" (whatever that is) >>>> or by some other clock controller or fixed clock makes a difference. >>>> Why can't the other clock provider be represented in the devicetree? >>>> >>> >>> cmn-reference-clock is for selecting the reference clock source for the >>> whole Ethernet block, which is just the standalone configure register. >> >> Sure, you are aware though that all clocks are just configure registers? >> >> Which clocks are these mentioned in the property? From where do they come? >> >> Anyway, property is in existing form is not correct - this is not a >> generic property. >> > > This property cmn-reference-clock is just the hardware register > configuration, since the different IPQ platform needs to select > the different reference clock source for the CMN PLL block that > provides the various clock outputs to the all kinds of Ethernet > devices, which is not from GCC provider. AGAIN: where do the clocks come from? Which device generates them? > > This is indeed not a generic property, which is the Ethernet > function configs same as clock-frequency. Then it should not be made as a generic property... Best regards, Krzysztof
On 12/15/2023 6:19 PM, Krzysztof Kozlowski wrote: > On 15/12/2023 10:49, Jie Luo wrote: >> >> >> On 12/15/2023 3:29 PM, Krzysztof Kozlowski wrote: >>> On 15/12/2023 07:46, Jie Luo wrote: >>>> >>>> >>>> On 12/15/2023 1:12 AM, Conor Dooley wrote: >>>>> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote: >>>>>> >>>>>> >>>>>> On 12/13/2023 12:06 AM, Conor Dooley wrote: >>>>>>> On Tue, Dec 12, 2023 at 07:51:50PM +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> >>>>>>>> --- >>>>>>>> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++- >>>>>>>> 1 file changed, 153 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..9546a6ad7841 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,71 @@ 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/IPQ9574, 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: gcc_uniphy0_ahb_clk >>>>>>>> + - const: gcc_uniphy1_ahb_clk >>>>>>>> + - const: gcc_uniphy0_sys_clk >>>>>>>> + - const: gcc_uniphy1_sys_clk >>>>>>> >>>>>>>> + cmn-reference-clock: >>>>>>>> + 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 >>>>>>> >>>>>>> Why is this not represented by an element of the clocks property? >>>>>> >>>>>> This property is for the reference clock source selection of CMN PLL, >>>>>> CMN PLL generates the different clock rates for the different Ethernet >>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the >>>>>> clock framework can't be used, which is the general hardware register >>>>>> instead of RCG register for GCC. >>>>> >>>>> I don't see how the clock being provided by the "GCC" (whatever that is) >>>>> or by some other clock controller or fixed clock makes a difference. >>>>> Why can't the other clock provider be represented in the devicetree? >>>>> >>>> >>>> cmn-reference-clock is for selecting the reference clock source for the >>>> whole Ethernet block, which is just the standalone configure register. >>> >>> Sure, you are aware though that all clocks are just configure registers? >>> >>> Which clocks are these mentioned in the property? From where do they come? >>> >>> Anyway, property is in existing form is not correct - this is not a >>> generic property. >>> >> >> This property cmn-reference-clock is just the hardware register >> configuration, since the different IPQ platform needs to select >> the different reference clock source for the CMN PLL block that >> provides the various clock outputs to the all kinds of Ethernet >> devices, which is not from GCC provider. > > AGAIN: where do the clocks come from? Which device generates them? Oh, OK, the reference clock is from wifi that provides 48MHZ to Ethernet block. > >> >> This is indeed not a generic property, which is the Ethernet >> function configs same as clock-frequency. > > > Then it should not be made as a generic property... sure, i saw your another comments, will prefix qcom_ on this property. > > > > Best regards, > Krzysztof >
On 15/12/2023 11:33, Jie Luo wrote: >>>>>>>>> + cmn-reference-clock: >>>>>>>>> + 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 >>>>>>>> >>>>>>>> Why is this not represented by an element of the clocks property? >>>>>>> >>>>>>> This property is for the reference clock source selection of CMN PLL, >>>>>>> CMN PLL generates the different clock rates for the different Ethernet >>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the >>>>>>> clock framework can't be used, which is the general hardware register >>>>>>> instead of RCG register for GCC. >>>>>> >>>>>> I don't see how the clock being provided by the "GCC" (whatever that is) >>>>>> or by some other clock controller or fixed clock makes a difference. >>>>>> Why can't the other clock provider be represented in the devicetree? >>>>>> >>>>> >>>>> cmn-reference-clock is for selecting the reference clock source for the >>>>> whole Ethernet block, which is just the standalone configure register. >>>> >>>> Sure, you are aware though that all clocks are just configure registers? >>>> >>>> Which clocks are these mentioned in the property? From where do they come? >>>> >>>> Anyway, property is in existing form is not correct - this is not a >>>> generic property. >>>> >>> >>> This property cmn-reference-clock is just the hardware register >>> configuration, since the different IPQ platform needs to select >>> the different reference clock source for the CMN PLL block that >>> provides the various clock outputs to the all kinds of Ethernet >>> devices, which is not from GCC provider. >> >> AGAIN: where do the clocks come from? Which device generates them? > > Oh, OK, the reference clock is from wifi that provides 48MHZ to > Ethernet block. Then WiFi should be providing you the clock and this device should be clock consumer, right? Best regards, Krzysztof
On 12/15/2023 6:37 PM, Krzysztof Kozlowski wrote: > On 15/12/2023 11:33, Jie Luo wrote: >>>>>>>>>> + cmn-reference-clock: >>>>>>>>>> + 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 >>>>>>>>> >>>>>>>>> Why is this not represented by an element of the clocks property? >>>>>>>> >>>>>>>> This property is for the reference clock source selection of CMN PLL, >>>>>>>> CMN PLL generates the different clock rates for the different Ethernet >>>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the >>>>>>>> clock framework can't be used, which is the general hardware register >>>>>>>> instead of RCG register for GCC. >>>>>>> >>>>>>> I don't see how the clock being provided by the "GCC" (whatever that is) >>>>>>> or by some other clock controller or fixed clock makes a difference. >>>>>>> Why can't the other clock provider be represented in the devicetree? >>>>>>> >>>>>> >>>>>> cmn-reference-clock is for selecting the reference clock source for the >>>>>> whole Ethernet block, which is just the standalone configure register. >>>>> >>>>> Sure, you are aware though that all clocks are just configure registers? >>>>> >>>>> Which clocks are these mentioned in the property? From where do they come? >>>>> >>>>> Anyway, property is in existing form is not correct - this is not a >>>>> generic property. >>>>> >>>> >>>> This property cmn-reference-clock is just the hardware register >>>> configuration, since the different IPQ platform needs to select >>>> the different reference clock source for the CMN PLL block that >>>> provides the various clock outputs to the all kinds of Ethernet >>>> devices, which is not from GCC provider. >>> >>> AGAIN: where do the clocks come from? Which device generates them? >> >> Oh, OK, the reference clock is from wifi that provides 48MHZ to >> Ethernet block. > > Then WiFi should be providing you the clock and this device should be > clock consumer, right? Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC for this 48MHZ clock output, it is the hardware PIN connection. > > > > Best regards, > Krzysztof >
> > > This is indeed not a generic property, which is the Ethernet > > > function configs same as clock-frequency. > > > > > > Then it should not be made as a generic property... > > sure, i saw your another comments, will prefix qcom_ on this property. iff the property stays, that would be a prefix of "qcom," not "qcom_". Cheers, Conor.
On 15/12/2023 11:40, Jie Luo wrote: > > > On 12/15/2023 6:37 PM, Krzysztof Kozlowski wrote: >> On 15/12/2023 11:33, Jie Luo wrote: >>>>>>>>>>> + cmn-reference-clock: >>>>>>>>>>> + 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 >>>>>>>>>> >>>>>>>>>> Why is this not represented by an element of the clocks property? >>>>>>>>> >>>>>>>>> This property is for the reference clock source selection of CMN PLL, >>>>>>>>> CMN PLL generates the different clock rates for the different Ethernet >>>>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the >>>>>>>>> clock framework can't be used, which is the general hardware register >>>>>>>>> instead of RCG register for GCC. >>>>>>>> >>>>>>>> I don't see how the clock being provided by the "GCC" (whatever that is) >>>>>>>> or by some other clock controller or fixed clock makes a difference. >>>>>>>> Why can't the other clock provider be represented in the devicetree? >>>>>>>> >>>>>>> >>>>>>> cmn-reference-clock is for selecting the reference clock source for the >>>>>>> whole Ethernet block, which is just the standalone configure register. >>>>>> >>>>>> Sure, you are aware though that all clocks are just configure registers? >>>>>> >>>>>> Which clocks are these mentioned in the property? From where do they come? >>>>>> >>>>>> Anyway, property is in existing form is not correct - this is not a >>>>>> generic property. >>>>>> >>>>> >>>>> This property cmn-reference-clock is just the hardware register >>>>> configuration, since the different IPQ platform needs to select >>>>> the different reference clock source for the CMN PLL block that >>>>> provides the various clock outputs to the all kinds of Ethernet >>>>> devices, which is not from GCC provider. >>>> >>>> AGAIN: where do the clocks come from? Which device generates them? >>> >>> Oh, OK, the reference clock is from wifi that provides 48MHZ to >>> Ethernet block. >> >> Then WiFi should be providing you the clock and this device should be >> clock consumer, right? > > Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC > for this 48MHZ clock output, it is the hardware PIN connection. All clocks are some hardware pin connections. Best regards, Krzysztof
On 12/15/2023 6:53 PM, Krzysztof Kozlowski wrote: > On 15/12/2023 11:40, Jie Luo wrote: >> >> >> On 12/15/2023 6:37 PM, Krzysztof Kozlowski wrote: >>> On 15/12/2023 11:33, Jie Luo wrote: >>>>>>>>>>>> + cmn-reference-clock: >>>>>>>>>>>> + 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 >>>>>>>>>>> >>>>>>>>>>> Why is this not represented by an element of the clocks property? >>>>>>>>>> >>>>>>>>>> This property is for the reference clock source selection of CMN PLL, >>>>>>>>>> CMN PLL generates the different clock rates for the different Ethernet >>>>>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the >>>>>>>>>> clock framework can't be used, which is the general hardware register >>>>>>>>>> instead of RCG register for GCC. >>>>>>>>> >>>>>>>>> I don't see how the clock being provided by the "GCC" (whatever that is) >>>>>>>>> or by some other clock controller or fixed clock makes a difference. >>>>>>>>> Why can't the other clock provider be represented in the devicetree? >>>>>>>>> >>>>>>>> >>>>>>>> cmn-reference-clock is for selecting the reference clock source for the >>>>>>>> whole Ethernet block, which is just the standalone configure register. >>>>>>> >>>>>>> Sure, you are aware though that all clocks are just configure registers? >>>>>>> >>>>>>> Which clocks are these mentioned in the property? From where do they come? >>>>>>> >>>>>>> Anyway, property is in existing form is not correct - this is not a >>>>>>> generic property. >>>>>>> >>>>>> >>>>>> This property cmn-reference-clock is just the hardware register >>>>>> configuration, since the different IPQ platform needs to select >>>>>> the different reference clock source for the CMN PLL block that >>>>>> provides the various clock outputs to the all kinds of Ethernet >>>>>> devices, which is not from GCC provider. >>>>> >>>>> AGAIN: where do the clocks come from? Which device generates them? >>>> >>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to >>>> Ethernet block. >>> >>> Then WiFi should be providing you the clock and this device should be >>> clock consumer, right? >> >> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC >> for this 48MHZ clock output, it is the hardware PIN connection. > > All clocks are some hardware pin connections. > > Best regards, > Krzysztof > Yes, all reference clocks here are from hardware pin connection.
On 12/15/2023 6:42 PM, Conor Dooley wrote: >>>> This is indeed not a generic property, which is the Ethernet >>>> function configs same as clock-frequency. >>> >>> >>> Then it should not be made as a generic property... >> >> sure, i saw your another comments, will prefix qcom_ on this property. > > iff the property stays, that would be a prefix of "qcom," not "qcom_". > > Cheers, > Conor. Ok, thanks.
On 15/12/2023 12:42, Jie Luo wrote: >>>>>>>> Which clocks are these mentioned in the property? From where do they come? >>>>>>>> >>>>>>>> Anyway, property is in existing form is not correct - this is not a >>>>>>>> generic property. >>>>>>>> >>>>>>> >>>>>>> This property cmn-reference-clock is just the hardware register >>>>>>> configuration, since the different IPQ platform needs to select >>>>>>> the different reference clock source for the CMN PLL block that >>>>>>> provides the various clock outputs to the all kinds of Ethernet >>>>>>> devices, which is not from GCC provider. >>>>>> >>>>>> AGAIN: where do the clocks come from? Which device generates them? >>>>> >>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to >>>>> Ethernet block. >>>> >>>> Then WiFi should be providing you the clock and this device should be >>>> clock consumer, right? >>> >>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC >>> for this 48MHZ clock output, it is the hardware PIN connection. >> >> All clocks are some hardware pin connections. >> >> Best regards, >> Krzysztof >> > > Yes, all reference clocks here are from hardware pin connection. You keep answering with short sentences without touching the root of the problem. I don't know exactly why, but I feel this discussion leads nowhere. After long discussion you finally admitted that clocks came from another device - Wifi. It took us like 6 emails? So last statement: if you have clock provider and clock consumer, you must represent it in the bindings or provide rationale why it should not or must not be represented in the bindings. So far I do not see any of such arguments. If you use arguments like: "My driver....": sorry, bindings are not about drivers "I don't have clock driver for WiFi": sorry, it does not matter if you can write one, right? Please reach internally your colleagues to solve these problems and make review process smoother. Best regards, Krzysztof
On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote: > On 15/12/2023 12:42, Jie Luo wrote: >>>>>>>>> Which clocks are these mentioned in the property? From where do they come? >>>>>>>>> >>>>>>>>> Anyway, property is in existing form is not correct - this is not a >>>>>>>>> generic property. >>>>>>>>> >>>>>>>> >>>>>>>> This property cmn-reference-clock is just the hardware register >>>>>>>> configuration, since the different IPQ platform needs to select >>>>>>>> the different reference clock source for the CMN PLL block that >>>>>>>> provides the various clock outputs to the all kinds of Ethernet >>>>>>>> devices, which is not from GCC provider. >>>>>>> >>>>>>> AGAIN: where do the clocks come from? Which device generates them? >>>>>> >>>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to >>>>>> Ethernet block. >>>>> >>>>> Then WiFi should be providing you the clock and this device should be >>>>> clock consumer, right? >>>> >>>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC >>>> for this 48MHZ clock output, it is the hardware PIN connection. >>> >>> All clocks are some hardware pin connections. >>> >>> Best regards, >>> Krzysztof >>> >> >> Yes, all reference clocks here are from hardware pin connection. > > You keep answering with short sentences without touching the root of the > problem. I don't know exactly why, but I feel this discussion leads > nowhere. After long discussion you finally admitted that clocks came > from another device - Wifi. It took us like 6 emails? > > So last statement: if you have clock provider and clock consumer, you > must represent it in the bindings or provide rationale why it should not > or must not be represented in the bindings. So far I do not see any of > such arguments. > > If you use arguments like: > "My driver....": sorry, bindings are not about drivers > "I don't have clock driver for WiFi": sorry, it does not matter if you > can write one, right? > > Please reach internally your colleagues to solve these problems and make > review process smoother. > > Best regards, > Krzysztof > > These reference clocks source do not need the hardware configuration, that is the reason why the clock provider is not needed, some reference clock source are even from external crystal. There is also no enable control for the reference clocks since it is inputted by the hardware PIN connection, i will update these description in the DT to make it more clear.
> > You keep answering with short sentences without touching the root of the > > problem. I don't know exactly why, but I feel this discussion leads > > nowhere. After long discussion you finally admitted that clocks came > > from another device - Wifi. It took us like 6 emails? > > > > So last statement: if you have clock provider and clock consumer, you > > must represent it in the bindings or provide rationale why it should not > > or must not be represented in the bindings. So far I do not see any of > > such arguments. > > > > If you use arguments like: > > "My driver....": sorry, bindings are not about drivers > > "I don't have clock driver for WiFi": sorry, it does not matter if you > > can write one, right? > > > > Please reach internally your colleagues to solve these problems and make > > review process smoother. Yes, i strongly agree with this. Its not our job as maintainers to educate big companies like Qualcomm how to write Linux drivers. There are more experienced driver writer within Qualcomm, you need to make contact with them, and get them to help you. Or you need to outsource the driver development to one of the companies which write mainline Linux drivers. Andrew
On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote: > > > On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote: > > On 15/12/2023 12:42, Jie Luo wrote: > > > > > > > > > > Which clocks are these mentioned in the property? From where do they come? > > > > > > > > > > > > > > > > > > > > Anyway, property is in existing form is not correct - this is not a > > > > > > > > > > generic property. > > > > > > > > > > > > > > > > > > > > > > > > > > > > This property cmn-reference-clock is just the hardware register > > > > > > > > > configuration, since the different IPQ platform needs to select > > > > > > > > > the different reference clock source for the CMN PLL block that > > > > > > > > > provides the various clock outputs to the all kinds of Ethernet > > > > > > > > > devices, which is not from GCC provider. > > > > > > > > > > > > > > > > AGAIN: where do the clocks come from? Which device generates them? > > > > > > > > > > > > > > Oh, OK, the reference clock is from wifi that provides 48MHZ to > > > > > > > Ethernet block. > > > > > > > > > > > > Then WiFi should be providing you the clock and this device should be > > > > > > clock consumer, right? > > > > > > > > > > Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC > > > > > for this 48MHZ clock output, it is the hardware PIN connection. > > > > > > > > All clocks are some hardware pin connections. > > > > > > > > Best regards, > > > > Krzysztof > > > > > > > > > > Yes, all reference clocks here are from hardware pin connection. > > > > You keep answering with short sentences without touching the root of the > > problem. I don't know exactly why, but I feel this discussion leads > > nowhere. After long discussion you finally admitted that clocks came > > from another device - Wifi. It took us like 6 emails? > > > > So last statement: if you have clock provider and clock consumer, you > > must represent it in the bindings or provide rationale why it should not > > or must not be represented in the bindings. So far I do not see any of > > such arguments. > > > > If you use arguments like: > > "My driver....": sorry, bindings are not about drivers > > "I don't have clock driver for WiFi": sorry, it does not matter if you > > can write one, right? > > > > Please reach internally your colleagues to solve these problems and make > > review process smoother. > These reference clocks source do not need the hardware configuration, > that is the reason why the clock provider is not needed, some reference > clock source are even from external crystal. I fail to understand how that makes this clock different to the clocks on any other platform. Clocks from external crystals are present in many many systems. See for example fixed-clock.yaml. > There is also no enable control for the reference clocks since it is > inputted by the hardware PIN connection, i will update these description > in the DT to make it more clear. Again, this does not justify having custom properties for this clock, as it is no different to other platforms. As far as I can tell, the only thing that a standard "clocks" property cannot convey here is the internal reference. I would suggest that since there is only one internal clock frequency, the absence of this particular clock in the "clocks" property can be used to determine that the reference is the internal one. Thanks, Conor.
On 12/15/2023 9:41 PM, Conor Dooley wrote: > On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote: >> >> >> On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote: >>> On 15/12/2023 12:42, Jie Luo wrote: >>>>>>>>>>> Which clocks are these mentioned in the property? From where do they come? >>>>>>>>>>> >>>>>>>>>>> Anyway, property is in existing form is not correct - this is not a >>>>>>>>>>> generic property. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> This property cmn-reference-clock is just the hardware register >>>>>>>>>> configuration, since the different IPQ platform needs to select >>>>>>>>>> the different reference clock source for the CMN PLL block that >>>>>>>>>> provides the various clock outputs to the all kinds of Ethernet >>>>>>>>>> devices, which is not from GCC provider. >>>>>>>>> >>>>>>>>> AGAIN: where do the clocks come from? Which device generates them? >>>>>>>> >>>>>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to >>>>>>>> Ethernet block. >>>>>>> >>>>>>> Then WiFi should be providing you the clock and this device should be >>>>>>> clock consumer, right? >>>>>> >>>>>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC >>>>>> for this 48MHZ clock output, it is the hardware PIN connection. >>>>> >>>>> All clocks are some hardware pin connections. >>>>> >>>>> Best regards, >>>>> Krzysztof >>>>> >>>> >>>> Yes, all reference clocks here are from hardware pin connection. >>> >>> You keep answering with short sentences without touching the root of the >>> problem. I don't know exactly why, but I feel this discussion leads >>> nowhere. After long discussion you finally admitted that clocks came >>> from another device - Wifi. It took us like 6 emails? >>> >>> So last statement: if you have clock provider and clock consumer, you >>> must represent it in the bindings or provide rationale why it should not >>> or must not be represented in the bindings. So far I do not see any of >>> such arguments. >>> >>> If you use arguments like: >>> "My driver....": sorry, bindings are not about drivers >>> "I don't have clock driver for WiFi": sorry, it does not matter if you >>> can write one, right? >>> >>> Please reach internally your colleagues to solve these problems and make >>> review process smoother. > >> These reference clocks source do not need the hardware configuration, >> that is the reason why the clock provider is not needed, some reference >> clock source are even from external crystal. > > I fail to understand how that makes this clock different to the clocks > on any other platform. Clocks from external crystals are present in many > many systems. See for example fixed-clock.yaml. Hi Conor, The reference clock rate has no meaning to the CMN PLL block, since the software can't control the behavior of CMN PLL, and various output clocks of CMN PLL block are fixed, adding this custom property is just for selecting the different reference clock source, since different IPQ platform needs to be configured the different reference clock source for the CMN PLL block. let's say if we register 48MHZ reference clock as the fix clock, we can't distinguish it is internal 48MHZ or external 48MHZ, for these two reference clock sources, there are different hardware configuration of CMN PLL block, and this reference clock selection is not applicable for the IPQ4019 platform. > >> There is also no enable control for the reference clocks since it is >> inputted by the hardware PIN connection, i will update these description >> in the DT to make it more clear. > > Again, this does not justify having custom properties for this clock, > as it is no different to other platforms. As far as I can tell, the only > thing that a standard "clocks" property cannot convey here is the > internal reference. I would suggest that since there is only one > internal clock frequency, the absence of this particular clock in the > "clocks" property can be used to determine that the reference is the > internal one. > > Thanks, > Conor. Yes, we can get the clock rate of the clocks property if we register these as the fix clock to distinguish the different clock source. Since the reference clock rate value has no matter with the CMN clock configuration, it is just the reference clock source selection, so i did not use the fix clock for this. Thanks for this suggestion, i will verify the fix clock register solution.
On 12/15/2023 9:39 PM, Andrew Lunn wrote: >>> You keep answering with short sentences without touching the root of the >>> problem. I don't know exactly why, but I feel this discussion leads >>> nowhere. After long discussion you finally admitted that clocks came >>> from another device - Wifi. It took us like 6 emails? >>> >>> So last statement: if you have clock provider and clock consumer, you >>> must represent it in the bindings or provide rationale why it should not >>> or must not be represented in the bindings. So far I do not see any of >>> such arguments. >>> >>> If you use arguments like: >>> "My driver....": sorry, bindings are not about drivers >>> "I don't have clock driver for WiFi": sorry, it does not matter if you >>> can write one, right? >>> >>> Please reach internally your colleagues to solve these problems and make >>> review process smoother. > > Yes, i strongly agree with this. Its not our job as maintainers to > educate big companies like Qualcomm how to write Linux drivers. There > are more experienced driver writer within Qualcomm, you need to make > contact with them, and get them to help you. Or you need to outsource > the driver development to one of the companies which write mainline > Linux drivers. > > Andrew Understand this, sorry for some easy mistakes happens here. i will sync with internal team before pushing the next patch set. Thanks a lot.
On Sat, Dec 16, 2023 at 09:16:49PM +0800, Jie Luo wrote: > > > On 12/15/2023 9:41 PM, Conor Dooley wrote: > > On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote: > > > > > > > > > On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote: > > > > On 15/12/2023 12:42, Jie Luo wrote: > > > > > > > > > > > > Which clocks are these mentioned in the property? From where do they come? > > > > > > > > > > > > > > > > > > > > > > > > Anyway, property is in existing form is not correct - this is not a > > > > > > > > > > > > generic property. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This property cmn-reference-clock is just the hardware register > > > > > > > > > > > configuration, since the different IPQ platform needs to select > > > > > > > > > > > the different reference clock source for the CMN PLL block that > > > > > > > > > > > provides the various clock outputs to the all kinds of Ethernet > > > > > > > > > > > devices, which is not from GCC provider. > > > > > > > > > > > > > > > > > > > > AGAIN: where do the clocks come from? Which device generates them? > > > > > > > > > > > > > > > > > > Oh, OK, the reference clock is from wifi that provides 48MHZ to > > > > > > > > > Ethernet block. > > > > > > > > > > > > > > > > Then WiFi should be providing you the clock and this device should be > > > > > > > > clock consumer, right? > > > > > > > > > > > > > > Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC > > > > > > > for this 48MHZ clock output, it is the hardware PIN connection. > > > > > > > > > > > > All clocks are some hardware pin connections. > > > > > > > > > > > > Best regards, > > > > > > Krzysztof > > > > > > > > > > > > > > > > Yes, all reference clocks here are from hardware pin connection. > > > > > > > > You keep answering with short sentences without touching the root of the > > > > problem. I don't know exactly why, but I feel this discussion leads > > > > nowhere. After long discussion you finally admitted that clocks came > > > > from another device - Wifi. It took us like 6 emails? > > > > > > > > So last statement: if you have clock provider and clock consumer, you > > > > must represent it in the bindings or provide rationale why it should not > > > > or must not be represented in the bindings. So far I do not see any of > > > > such arguments. > > > > > > > > If you use arguments like: > > > > "My driver....": sorry, bindings are not about drivers > > > > "I don't have clock driver for WiFi": sorry, it does not matter if you > > > > can write one, right? > > > > > > > > Please reach internally your colleagues to solve these problems and make > > > > review process smoother. > > > > > These reference clocks source do not need the hardware configuration, > > > that is the reason why the clock provider is not needed, some reference > > > clock source are even from external crystal. > > > > I fail to understand how that makes this clock different to the clocks > > on any other platform. Clocks from external crystals are present in many > > many systems. See for example fixed-clock.yaml. > > The reference clock rate has no meaning to the CMN PLL block, since the > software can't control the behavior of CMN PLL, and various output > clocks of CMN PLL block are fixed, adding this custom property is just > for selecting the different reference clock source, since different > IPQ platform needs to be configured the different reference clock source > for the CMN PLL block. Many, many other systems are in the same situation, where clocks are provided to a peripheral that has no control over the clock rate, but has to pick internal dividers or set bits in a config register depending on what clock rate is provided to it. That is not something special about this particular platform and other systems are able to use the clocks property for this purpose. > let's say if we register 48MHZ reference clock as the fix clock, we > can't distinguish it is internal 48MHZ or external 48MHZ, for these > two reference clock sources, there are different hardware configuration > of CMN PLL block That's easy, if the reference is external, it is provided by the clocks property. If it internal, then there will be no clocks property providing it. > and this reference clock selection is not applicable > for the IPQ4019 platform. Isn't this a patch for the IPQ4019? Why would it not be relevant? > > > There is also no enable control for the reference clocks since it is > > > inputted by the hardware PIN connection, i will update these description > > > in the DT to make it more clear. > > > > Again, this does not justify having custom properties for this clock, > > as it is no different to other platforms. As far as I can tell, the only > > thing that a standard "clocks" property cannot convey here is the > > internal reference. I would suggest that since there is only one > > internal clock frequency, the absence of this particular clock in the > > "clocks" property can be used to determine that the reference is the > > internal one. I'm surprised you didn't pick up on this, but there are actually _2_ internal references, which I have just noticed while double checking the binding patch. What is the impact of using the 48 MHz or 96 MHz internal reference? Thanks, Conor. > Yes, we can get the clock rate of the clocks property if we register > these as the fix clock to distinguish the different clock source. > > Since the reference clock rate value has no matter with the CMN clock > configuration, it is just the reference clock source selection, so > i did not use the fix clock for this. > > Thanks for this suggestion, i will verify the fix clock register solution.
On 12/16/2023 10:16 PM, Conor Dooley wrote: > On Sat, Dec 16, 2023 at 09:16:49PM +0800, Jie Luo wrote: >> >> >> On 12/15/2023 9:41 PM, Conor Dooley wrote: >>> On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote: >>>> >>>> >>>> On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote: >>>>> On 15/12/2023 12:42, Jie Luo wrote: >>>>>>>>>>>>> Which clocks are these mentioned in the property? From where do they come? >>>>>>>>>>>>> >>>>>>>>>>>>> Anyway, property is in existing form is not correct - this is not a >>>>>>>>>>>>> generic property. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> This property cmn-reference-clock is just the hardware register >>>>>>>>>>>> configuration, since the different IPQ platform needs to select >>>>>>>>>>>> the different reference clock source for the CMN PLL block that >>>>>>>>>>>> provides the various clock outputs to the all kinds of Ethernet >>>>>>>>>>>> devices, which is not from GCC provider. >>>>>>>>>>> >>>>>>>>>>> AGAIN: where do the clocks come from? Which device generates them? >>>>>>>>>> >>>>>>>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to >>>>>>>>>> Ethernet block. >>>>>>>>> >>>>>>>>> Then WiFi should be providing you the clock and this device should be >>>>>>>>> clock consumer, right? >>>>>>>> >>>>>>>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC >>>>>>>> for this 48MHZ clock output, it is the hardware PIN connection. >>>>>>> >>>>>>> All clocks are some hardware pin connections. >>>>>>> >>>>>>> Best regards, >>>>>>> Krzysztof >>>>>>> >>>>>> >>>>>> Yes, all reference clocks here are from hardware pin connection. >>>>> >>>>> You keep answering with short sentences without touching the root of the >>>>> problem. I don't know exactly why, but I feel this discussion leads >>>>> nowhere. After long discussion you finally admitted that clocks came >>>>> from another device - Wifi. It took us like 6 emails? >>>>> >>>>> So last statement: if you have clock provider and clock consumer, you >>>>> must represent it in the bindings or provide rationale why it should not >>>>> or must not be represented in the bindings. So far I do not see any of >>>>> such arguments. >>>>> >>>>> If you use arguments like: >>>>> "My driver....": sorry, bindings are not about drivers >>>>> "I don't have clock driver for WiFi": sorry, it does not matter if you >>>>> can write one, right? >>>>> >>>>> Please reach internally your colleagues to solve these problems and make >>>>> review process smoother. >>> >>>> These reference clocks source do not need the hardware configuration, >>>> that is the reason why the clock provider is not needed, some reference >>>> clock source are even from external crystal. >>> >>> I fail to understand how that makes this clock different to the clocks >>> on any other platform. Clocks from external crystals are present in many >>> many systems. See for example fixed-clock.yaml. >> >> The reference clock rate has no meaning to the CMN PLL block, since the >> software can't control the behavior of CMN PLL, and various output >> clocks of CMN PLL block are fixed, adding this custom property is just >> for selecting the different reference clock source, since different >> IPQ platform needs to be configured the different reference clock source >> for the CMN PLL block. > > Many, many other systems are in the same situation, where clocks are > provided to a peripheral that has no control over the clock rate, but > has to pick internal dividers or set bits in a config register depending > on what clock rate is provided to it. That is not something special > about this particular platform and other systems are able to use the > clocks property for this purpose. Sure, Thanks Conor for this information. i will try to replace this custom property with clocks property and verify the drive. > >> let's say if we register 48MHZ reference clock as the fix clock, we >> can't distinguish it is internal 48MHZ or external 48MHZ, for these >> two reference clock sources, there are different hardware configuration >> of CMN PLL block > > That's easy, if the reference is external, it is provided by the clocks > property. If it internal, then there will be no clocks property > providing it. Thanks for this detail. > >> and this reference clock selection is not applicable >> for the IPQ4019 platform. > > Isn't this a patch for the IPQ4019? Why would it not be relevant? IPQ4019 is the legacy chip, and the same MDIO bus driver is also extended to support the new IPQ platform, since the MDIO hardware is leveraged by the new IPQ platform. For the CMN PLL block, which is not existed on the legacy platform IPQ4019, but it does not matter, we can also distinguish it according to the CMN register base defined or not, the CMN reference clocks is configured only when the CMN register base defined in the reg property. > >>>> There is also no enable control for the reference clocks since it is >>>> inputted by the hardware PIN connection, i will update these description >>>> in the DT to make it more clear. >>> >>> Again, this does not justify having custom properties for this clock, >>> as it is no different to other platforms. As far as I can tell, the only >>> thing that a standard "clocks" property cannot convey here is the >>> internal reference. I would suggest that since there is only one >>> internal clock frequency, the absence of this particular clock in the >>> "clocks" property can be used to determine that the reference is the >>> internal on > > I'm surprised you didn't pick up on this, but there are actually _2_ > internal references, which I have just noticed while double checking the > binding patch. i noticed this, the reference clock source can be supported by clocks as you suggested here, it is really helpful. > > What is the impact of using the 48 MHz or 96 MHz internal reference? They works on the different IPQ platform, 96MHZ internal reference is used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is same as what you describe above, the different clock source rate is selected as the different register value, then the PLL can do the corresponding config to output the correct clock rate, the external clock source is also same if the clock rate is same, just the different hardware PIN is selected if the external reference source is configured. Thanks. > > Thanks, > Conor. > >> Yes, we can get the clock rate of the clocks property if we register >> these as the fix clock to distinguish the different clock source. >> >> Since the reference clock rate value has no matter with the CMN clock >> configuration, it is just the reference clock source selection, so >> i did not use the fix clock for this. >> >> Thanks for this suggestion, i will verify the fix clock register solution.
On Sat, Dec 16, 2023 at 11:37:08PM +0800, Jie Luo wrote: > On 12/16/2023 10:16 PM, Conor Dooley wrote: > > On Sat, Dec 16, 2023 at 09:16:49PM +0800, Jie Luo wrote: > > > On 12/15/2023 9:41 PM, Conor Dooley wrote: > > > > On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote: > > > > > On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote: > > > > > > On 15/12/2023 12:42, Jie Luo wrote: > > > > > There is also no enable control for the reference clocks since it is > > > > > inputted by the hardware PIN connection, i will update these description > > > > > in the DT to make it more clear. > > > > > > > > Again, this does not justify having custom properties for this clock, > > > > as it is no different to other platforms. As far as I can tell, the only > > > > thing that a standard "clocks" property cannot convey here is the > > > > internal reference. I would suggest that since there is only one > > > > internal clock frequency, the absence of this particular clock in the > > > > "clocks" property can be used to determine that the reference is the > > > > internal on > > > > I'm surprised you didn't pick up on this, but there are actually _2_ > > internal references, which I have just noticed while double checking the > > binding patch. > > i noticed this, the reference clock source can be supported by clocks as > you suggested here, it is really helpful. > > > What is the impact of using the 48 MHz or 96 MHz internal reference? > They works on the different IPQ platform, 96MHZ internal reference is > used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is > same as what you describe above, the different clock source rate is > selected as the different register value, then the PLL can do the > corresponding config to output the correct clock rate, the external > clock source is also same if the clock rate is same, just the different > hardware PIN is selected if the external reference source is configured. Ah, so there is only one internal reference frequency per device. Then my suggestion to use the presence of the clock in the clocks property should work, just the fallback to the internal reference is going to depend on the compatible. Thanks, Conor.
On 16/12/2023 16:37, Jie Luo wrote: >> >> I'm surprised you didn't pick up on this, but there are actually _2_ >> internal references, which I have just noticed while double checking the >> binding patch. > > i noticed this, the reference clock source can be supported by clocks as > you suggested here, it is really helpful. >> >> What is the impact of using the 48 MHz or 96 MHz internal reference? > They works on the different IPQ platform, 96MHZ internal reference is > used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is So the binding is just incorrect. Why do you even consider configuring 96 MHz internal reference on IPQ5332? Best regards, Krzysztof
On 12/19/2023 11:47 PM, Conor Dooley wrote: > On Sat, Dec 16, 2023 at 11:37:08PM +0800, Jie Luo wrote: >> On 12/16/2023 10:16 PM, Conor Dooley wrote: >>> On Sat, Dec 16, 2023 at 09:16:49PM +0800, Jie Luo wrote: >>>> On 12/15/2023 9:41 PM, Conor Dooley wrote: >>>>> On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote: >>>>>> On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote: >>>>>>> On 15/12/2023 12:42, Jie Luo wrote: > >>>>>> There is also no enable control for the reference clocks since it is >>>>>> inputted by the hardware PIN connection, i will update these description >>>>>> in the DT to make it more clear. >>>>> >>>>> Again, this does not justify having custom properties for this clock, >>>>> as it is no different to other platforms. As far as I can tell, the only >>>>> thing that a standard "clocks" property cannot convey here is the >>>>> internal reference. I would suggest that since there is only one >>>>> internal clock frequency, the absence of this particular clock in the >>>>> "clocks" property can be used to determine that the reference is the >>>>> internal on >>> >>> I'm surprised you didn't pick up on this, but there are actually _2_ >>> internal references, which I have just noticed while double checking the >>> binding patch. >> >> i noticed this, the reference clock source can be supported by clocks as >> you suggested here, it is really helpful. >> >>> What is the impact of using the 48 MHz or 96 MHz internal reference? >> They works on the different IPQ platform, 96MHZ internal reference is >> used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is >> same as what you describe above, the different clock source rate is >> selected as the different register value, then the PLL can do the >> corresponding config to output the correct clock rate, the external >> clock source is also same if the clock rate is same, just the different >> hardware PIN is selected if the external reference source is configured. > > > Ah, so there is only one internal reference frequency per device. Then > my suggestion to use the presence of the clock in the clocks property > should work, just the fallback to the internal reference is going to > depend on the compatible. > > Thanks, > Conor. The reference clock source is configurable, normally there is the fix reference clock configured per each IPQ platform, but we should keep the reference clock source configurable in case of the reference clock source switch needed in the future. you are right, the reference clock source can be distinguished by checking the clock rate and the compatible string.
On 12/20/2023 3:28 PM, Krzysztof Kozlowski wrote: > On 16/12/2023 16:37, Jie Luo wrote: >>> >>> I'm surprised you didn't pick up on this, but there are actually _2_ >>> internal references, which I have just noticed while double checking the >>> binding patch. >> >> i noticed this, the reference clock source can be supported by clocks as >> you suggested here, it is really helpful. >>> >>> What is the impact of using the 48 MHz or 96 MHz internal reference? >> They works on the different IPQ platform, 96MHZ internal reference is >> used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is > > So the binding is just incorrect. Why do you even consider configuring > 96 MHz internal reference on IPQ5332? > > > Best regards, > Krzysztof > Normally there is the fix reference clock source used per each IPQ platform, but we should keep it configurable, the CMN PLL block is same among the supported IPQ platforms, Maybe there is special case to switch the reference clock in the future. i will also update the dtbinding doc to limit the reference clock based on the compatible string.
diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml index 3407e909e8a7..9546a6ad7841 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,71 @@ 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/IPQ9574, 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: gcc_uniphy0_ahb_clk + - const: gcc_uniphy1_ahb_clk + - const: gcc_uniphy0_sys_clk + - const: gcc_uniphy1_sys_clk + + cmn-reference-clock: + 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 frequecies above can be configured, other frequency + will cause malfunction. If absent, the default hardware value is used. + + reset-gpios: + maxItems: 1 + + reset-assert-us: + maxItems: 1 + + reset-deassert-us: + maxItems: 1 required: - compatible @@ -61,6 +115,8 @@ allOf: - qcom,ipq5018-mdio - qcom,ipq6018-mdio - qcom,ipq8074-mdio + - qcom,ipq5332-mdio + - qcom,ipq9574-mdio then: required: - clocks @@ -70,6 +126,40 @@ allOf: clocks: false clock-names: false + - if: + properties: + compatible: + contains: + enum: + - qcom,ipq5332-mdio + then: + properties: + clocks: + minItems: 5 + maxItems: 5 + reg-names: + items: + - const: mdio + - const: eth_ldo1 + - const: eth_ldo2 + - const: cmn_blk + + - if: + properties: + compatible: + contains: + enum: + - qcom,ipq9574-mdio + then: + properties: + reg-names: + items: + - const: mdio + - const: eth_ldo1 + - const: eth_ldo2 + - const: eth_ldo3 + - const: cmn_blk + unevaluatedProperties: false examples: @@ -100,3 +190,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>, + <0x7A00610 0x4>, + <0x7A10610 0x4>, + <0x9B000 0x800>; + + reg-names = "mdio", + "eth_ldo1", + "eth_ldo2", + "cmn_blk"; + + 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", + "gcc_uniphy0_ahb_clk", + "gcc_uniphy1_ahb_clk", + "gcc_uniphy0_sys_clk", + "gcc_uniphy1_sys_clk"; + + 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 | 157 +++++++++++++++++- 1 file changed, 153 insertions(+), 4 deletions(-)