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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250925-await-rotting-fe5d368bc2fd@spud>
Date: Thu, 25 Sep 2025 20:32:35 +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: Re: [PATCH v2 1/2] dt-bindings: mmc: sdhci-of-dwcmshc: Add
 Eswin EIC7700

On Thu, Sep 25, 2025 at 05:37:59PM +0800, 何欢 wrote:
> Dear Conor,
> Thank you for your valuable and professional suggestions.
> Please find our explanations embedded below your comments in the
> original email.

FWIW, there's no need to thank me on every mail!
> > > > 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.

Okay, cool. That sounds fine to me.

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

Okay. It's just unusual that you have something significantly different
to what the other 10+ devices have. It's fine to be different if your
hardware genuinely is not the same.

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