Message-ID: <674372d7.16fd.199751b489c.Coremail.hehuan1@eswincomputing.com>
Date: Tue, 23 Sep 2025 13:45:46 +0800 (GMT+08:00)
From: 何欢 <hehuan1@...incomputing.com>
To: "Conor Dooley" <conor@...nel.org>
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: 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@...nel.org>
> 发送时间:2025-09-13 03:10:04 (星期六)
> 收件人: hehuan1@...incomputing.com
> 抄送: 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
> 主题: 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?

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@eswincomputing.com/
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@...50000 {
    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@...0460000{
    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
> > 

Powered by blists - more mailing lists