Message-ID: <20250912-pork-oaf-3480d3d0ef67@spud>
Date: Fri, 12 Sep 2025 20:10:04 +0100
From: Conor Dooley <conor@...nel.org>
To: hehuan1@...incomputing.com
Cc: ulf.hansson@...aro.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, jszhang@...nel.org, adrian.hunter@...el.com,
	p.zabel@...gutronix.de, linux-mmc@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	ningyu@...incomputing.com, linmin@...incomputing.com,
	pinkesh.vaghela@...fochips.com, xuxiang@...incomputing.com,
	luyulin@...incomputing.com, dongxuyang@...incomputing.com,
	zhangsenchuan@...incomputing.com, weishangjuan@...incomputing.com,
	lizhi2@...incomputing.com, caohang@...incomputing.com
Subject: Re: [PATCH v2 1/2] dt-bindings: mmc: sdhci-of-dwcmshc: Add Eswin
 EIC7700

On Fri, Sep 12, 2025 at 05:37:13PM +0800, hehuan1@...incomputing.com wrote:
> From: Huan He <hehuan1@...incomputing.com>
> 
> EIC7700 use Synopsys dwcmshc IP for SD/eMMC controllers.
> Add Eswin EIC7700 support in sdhci-of-dwcmshc.yaml.
> 
> Signed-off-by: Huan He <hehuan1@...incomputing.com>
> ---
>  .../bindings/mmc/snps,dwcmshc-sdhci.yaml      | 81 +++++++++++++++++--
>  1 file changed, 75 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
> index f882219a0a26..e0f34bc28e0c 100644
> --- a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
> @@ -30,6 +30,7 @@ properties:
>            - sophgo,sg2002-dwcmshc
>            - sophgo,sg2042-dwcmshc
>            - thead,th1520-dwcmshc
> +          - eswin,eic7700-dwcmshc
>  
>    reg:
>      maxItems: 1
> @@ -52,17 +53,51 @@ properties:
>      maxItems: 5
>  
>    reset-names:
> -    items:
> -      - const: core
> -      - const: bus
> -      - const: axi
> -      - const: block
> -      - const: timer
> +    maxItems: 5
>  
>    rockchip,txclk-tapnum:
>      description: Specify the number of delay for tx sampling.
>      $ref: /schemas/types.yaml#/definitions/uint8
>  
> +  clock-output-names:
> +    maxItems: 1
> +    description:
> +      The name of the clock output representing the card clock,
> +      consumed by the PHY.

You have one clock, why do you need this?

> +
> +  '#clock-cells':
> +    enum: [0]

const: 0

> +    description:
> +      Specifies how many cells are used when referencing the
> +      exported clock from another node. This property indicates
> +      that the clock output has no extra parameters and represents
> +      the card clock.

This description is not needed.

> +
> +  eswin,hsp-sp-csr:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - description: Phandle to HSP(High-Speed Peripheral) device
> +      - description: Offset of the stability status register for
> +                     internal clock
> +      - description: Offset of the stability register for host
> +                     regulator voltage.
> +    description: |
> +      High-Speed Peripheral device needed to configure internal
> +      clocks, and the power.
> +
> +  eswin,syscrg-csr:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - description: Phandle to system CRG(System Clock and Reset
> +                     Generator) device
> +      - description: Offset of core clock control register
> +    description: |
> +      System Clock and Reset Generator device needed to configure
> +      core clock.

This reeks of improper clock tree description. Why can you not just
request the rate that you need via the common clk framework? Likewise
for reset. You already have a clocks property that has to include the
core clock, so I don't see why you need another property to get around
it.

As a result, I'm also suspicious of your hsp-sp-csr, but these at least
appear to be internal clocks if your description is to be believed.
I'd like you to explain exactly what those clocks do and what the "HSP"
actually is. What other peripherals use it?

Also, your driver turns on this hsp clock but never turns it off. Same
for the power.

I want to see the full dts for what you're doing here before I approve
this, there's too much here that looks wrong.

> +
> +  drive-impedance-ohm:

How come this one has no eswin prefix? Also, the unit is "Ohms", not
"Ohm".

Additionally, any eswin properties should be restricted to eswin devices
only.

> +    description: Specifies the drive impedance in Ohm.
> +    enum: [33, 40, 50, 66, 100]
> +
>  required:
>    - compatible
>    - reg
> @@ -110,6 +145,40 @@ allOf:
>              - const: block
>              - const: timer
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: eswin,eic7700-dwcmshc
> +    then:
> +      properties:
> +        resets:
> +          minItems: 4
> +          maxItems: 4
> +        reset-names:
> +          items:
> +            - const: arstn
> +            - const: phy_rst
> +            - const: prstn
> +            - const: txrx_rst

How come you're so drastically different to the other devices?
Also, putting "_rst" in a reset name is pointless. These are all resets
after all by nature.

Cheers,
Conor.

> +      required:
> +        - clock-output-names
> +        - '#clock-cells'
> +        - eswin,hsp-sp-csr
> +        - eswin,syscrg-csr
> +        - drive-impedance-ohm
> +    else:
> +      properties:
> +        resets:
> +          maxItems: 5
> +        reset-names:
> +          items:
> +            - const: core
> +            - const: bus
> +            - const: axi
> +            - const: block
> +            - const: timer
> +
>    - if:
>        properties:
>          compatible:
> -- 
> 2.25.1
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists