[v2,5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

  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(-)
  

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.