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

On Tue, Sep 23, 2025 at 01:45:46PM +0800, 何欢 wrote:

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

quoted

-----原始邮件-----
发件人: "Conor Dooley" [off-list ref]
发送时间:2025-09-13 03:10:04 (星期六)
收件人: hehuan1@eswincomputing.com
抄送: ulf.hansson@linaro.org, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, jszhang@kernel.org, adrian.hunter@intel.com, p.zabel@pengutronix.de, linux-mmc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com, xuxiang@eswincomputing.com, luyulin@eswincomputing.com, dongxuyang@eswincomputing.com, zhangsenchuan@eswincomputing.com, weishangjuan@eswincomputing.com, lizhi2@eswincomputing.com, caohang@eswincomputing.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@eswincomputing.com wrote:

quoted

From: Huan He <redacted>

EIC7700 use Synopsys dwcmshc IP for SD/eMMC controllers.
Add Eswin EIC7700 support in sdhci-of-dwcmshc.yaml.

Signed-off-by: Huan He <redacted>
---
 .../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.

quoted

quoted

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

const: 0

quoted

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

quoted

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

Okay cool.

quoted

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/ (local)
Please let us know this explanation has addressed your doubts. We're
happy to provide further details if needed.

I'll address this on the usb thread, thanks for the explanation there.

quoted

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.

Do you still need to "remove" the assertions if the driver is removed,
and the clocks get disabled? Or is that not a concern, because the
hardware can't do anything relevant without the driver loaded? If it;s
not a concern, then that seems okay to me.

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

quoted

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.

Okay, that doesn't look too bad, with the updates you've made to remove
the sysrg-csr property.

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

quoted

quoted

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

quoted

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

quoted

+    description: Specifies the drive impedance in Ohm.
+    enum: [33, 40, 50, 66, 100]
+
 required:
   - compatible
   - regsdhci_eic7700_dt_parse_clk_phases

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

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?

I don't know if it is correct or not, but it looks better than before.
Can you explain why you aren't using the "normal" 5 resets that other
devices do?