Re: Re: [PATCH v2 1/2] dt-bindings: mmc: sdhci-of-dwcmshc: Add Eswin EIC7700

[何欢]

Dear Conor,
Thank you for your valuable and professional suggestions.
Please find our explanations embedded below your comments in the
original email.

Best regards,

He Huan
Eswin Computing

> -----原始邮件-----
> 发件人: "Conor Dooley" <conor@xxxxxxxxxx>
> 发送时间:2025-09-13 03:10:04 (星期六)
> 收件人: hehuan1@xxxxxxxxxxxxxxxxxx
> 抄送: ulf.hansson@xxxxxxxxxx, robh@xxxxxxxxxx, krzk+dt@xxxxxxxxxx, conor+dt@xxxxxxxxxx, jszhang@xxxxxxxxxx, adrian.hunter@xxxxxxxxx, p.zabel@xxxxxxxxxxxxxx, linux-mmc@xxxxxxxxxxxxxxx, devicetree@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, ningyu@xxxxxxxxxxxxxxxxxx, linmin@xxxxxxxxxxxxxxxxxx, pinkesh.vaghela@xxxxxxxxxxxxxx, xuxiang@xxxxxxxxxxxxxxxxxx, luyulin@xxxxxxxxxxxxxxxxxx, dongxuyang@xxxxxxxxxxxxxxxxxx, zhangsenchuan@xxxxxxxxxxxxxxxxxx, weishangjuan@xxxxxxxxxxxxxxxxxx, lizhi2@xxxxxxxxxxxxxxxxxx, caohang@xxxxxxxxxxxxxxxxxx
> 主题: 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@xxxxxxxxxxxxxxxxxx wrote:
> > From: Huan He <hehuan1@xxxxxxxxxxxxxxxxxx>
> > 
> > EIC7700 use Synopsys dwcmshc IP for SD/eMMC controllers.
> > Add Eswin EIC7700 support in sdhci-of-dwcmshc.yaml.
> > 
> > Signed-off-by: Huan He <hehuan1@xxxxxxxxxxxxxxxxxx>
> > ---
> >  .../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?

Thank you for the feedback. I will remove it in the next version.

> 
> > +
> > +  '#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.

Thank you for the feedback. You are absolutely right; We've taken your
advice. In v3 of the patchset, we have completely removed the 
eswin,syscrg-csr property. The device tree binding now relies solely
on the standard clocks and clock-names properties to acquire the
necessary clock.

> 
> 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?

Thank you for raising this. Your concerns regarding the hsp-sp-csr
clocks are valid.
The functionality and purpose of the HSP (hsp-sp-csr) were explained
in our previous patch series for the USB module. The relevant
discussion can be found here:
https://lore.kernel.org/linux-usb/17731a13.1cce.19974dfc64d.Coremail.caohang@xxxxxxxxxxxxxxxxxx/
Please let us know this explanation has addressed your doubts. We're
happy to provide further details if needed.

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

The writes to hsp_int_status and hsp_pwr_ctrl are not enabling clocks
or power rails.They are stability assertions.
Assert clock stability: Write a value to the hsp_int_status register.
This signals to the eMMC controller that platform clocks (axi master
bus clock, internal core base clock, timer clock) are enabled and
stable.
Assert voltage stability: Write a value to hsp_pwr_ctrl. This signals
that VDD is stable and permits transition to high-speed modes (e.g.,
UHS-I).

> 
> 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.

The full dts is as follows: 
sdhci_emmc: mmc@50450000 {
    compatible = "eswin,eic7700-dwcmshc";
    reg = <0x0 0x50450000 0x0 0x10000>;
    clocks = <&clock 264>, <&clock 546>;
    clock-names = "core", "bus";
    assigned-clocks = <&clock 264>;
    assigned-clock-rates = <200000000>;
    resets = <&reset 75>, <&reset 72>, <&reset 88>, <&reset 92>;
    reset-names = "txrx", "phy", "bus", "axi";
    interrupt-parent = <&plic>;
    interrupts = <79>;
    bus-width = <8>;
    non-removable;
    mmc-hs400-1_8v;
    max-frequency = <200000000>;
    #size-cells = <2>;
    no-sdio;
    no-sd;
    eswin,hsp-sp-csr = <&hsp_sp_csr 0x508 0x50c>;
    eswin,drive-impedance-ohms = <50>;
};

sdio: mmc@0x50460000{
    compatible = "eswin,eic7700-dwcmshc";
    reg = <0x0 0x50460000 0x0 0x10000>;
    clocks = <&clock 265>, <&clock 546>;
    clock-names ="core","bus";
    resets = <&reset 76>, <&reset 73>, <&reset 87>, <&reset 91>;
    reset-names = "txrx","phy", "bus", "axi";
    interrupt-parent = <&plic>;
    interrupts = <81>;
    clock-frequency = <208000000>;
    max-frequency = <208000000>;
    #address-cells = <1>;
    #size-cells = <0>;
    bus-width = <4>;
    no-sdio;
    no-mmc;
    eswin,hsp-sp-csr = <&hsp_sp_csr 0x608 0x60c>;
    eswin,drive-impedance-ohms = <33>;
};

> 
> > +
> > +  drive-impedance-ohm:
> 
> How come this one has no eswin prefix? Also, the unit is "Ohms", not
> "Ohm".

In version 3, we renamed the property from drive-impedance-ohm to
eswin,drive-impedance-ohms.

> 
> 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.

We have simplified the names as follows:
reset-names:
  items:
    - const: axi
    - const: phy
    - const: bus
    - const: txrx
Regarding the functionality of these resets:
prst and arst: correspond to the resets for the bus and AXI domains.
txrx: is used for the reset of the internal transmit and receive clock
domains.
phy: is used for the reset of the internal PHY.
This will be corrected in the next patch. Is this correct?

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