Message-ID: <7e07388c.187f.199803c99cf.Coremail.hehuan1@eswincomputing.com>
Date: Thu, 25 Sep 2025 17:37:59 +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: 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-25 03:43:15 (星期四)
> 收件人: 何欢 <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: 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
> > 
> > > -----原始邮件-----
> > > 发件人: "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.
> 
> Okay cool.
> 
> > > 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.
> 
> I'll address this on the usb thread, thanks for the explanation there.
> 
> > > 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.

The writes to hsp_int_status and hsp_pwr_ctrl are just status indicators.
They assert that clocks and voltages are stable. There is no need to clear
these assertions when the driver is removed or clocks are disabled. This
is because the hardware cannot perform any relevant operations without the
driver loaded. Whether these registers are written or not has no impact on
hardware behavior when the clocks are off.

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

Our reset naming is based on our hardware's reset design. Although we do
not follow the typical 5-reset naming used by other vendors, the
functionality is equivalent. Each of our resets covers the corresponding
hardware domains. There is no one-to-one correspondence, but our
implementation includes all necessary reset functionality.

Powered by blists - more mailing lists