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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4186 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]