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

Conor Dooley
2023-12-12T16:06:45Z

[-- Attachment #1: Type: text/plain, Size: 3877 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]