Message-ID: <20250924-visiting-sasquatch-c58f782ff686@spud>
Date: Wed, 24 Sep 2025 20:43:15 +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: 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.

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

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

Powered by blists - more mailing lists