Message-ID: <20250924-shimmer-sphinx-1a12caeab401@spud>
Date: Wed, 24 Sep 2025 20:55:22 +0100
From: Conor Dooley <conor@...nel.org>
To: Hang Cao <caohang@...incomputing.com>
Cc: gregkh@...uxfoundation.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, Thinh.Nguyen@...opsys.com,
	p.zabel@...gutronix.de, linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org, devicetree@...r.kernel.org,
	ningyu@...incomputing.com, linmin@...incomputing.com,
	pinkesh.vaghela@...fochips.com,
	Senchuan Zhang <zhangsenchuan@...incomputing.com>
Subject: Re: Re: [PATCH v3 1/2] dt-bindings: usb: Add ESWIN EIC7700 USB
 controller

On Tue, Sep 23, 2025 at 12:40:46PM +0800, Hang Cao wrote:
> > > From: Hang Cao <caohang@...incomputing.com>
> > > +  eswin,hsp-sp-csr:
> > > +    description:
> > > +      HSP CSR is to control and get status of different high-speed peripherals
> > > +      (such as Ethernet, USB, SATA, etc.) via register, which can close
> > > +      module's clock,reset module independently and tune board-level's
> > > +      parameters of PHY, etc.
> > > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > +    items:
> > > +      - items:
> > > +          - description: phandle to HSP Register Controller hsp_sp_csr node.
> > > +          - description: usb bus register offset.
> > > +          - description: axi low power register offset.
> > > +          - description: vbus frequency register offset.
> > > +          - description: mpll register offset.
> > 
> > As I mentioned on the shdci binding patch, I'm not happy with the
> > justification for this phandle. What exactly is the clock that this
> > controls and why does it not have a dedicated clock-controller driver
> > and reset-controller driver?
> > 
> In the current design framework, the clock can be divided into two parts: 
> 1. The top-clock, which is used to manage and control the clocks of various subsystems (such as HSP, GPU, NPU, etc.); 
> 2. The subsystem clocks managed independently by each subsystem.
> The top-clock is a standard clock design(featuring gate, divider, and mux functions) that has been registered in the 
> common clock framework,with a dedicated clock controller driver.
> 
> The subsystem clocks managed by subsystems are controlled and configured through the CSR (Control and Status Register) 
> of each respective subsystem. For example, the HSP subsystem uses the eswin,hsp-sp-csr. Additionally, this CSR is
>  responsible for managing startup functions, performing independent reset of specific modules, and adjusting 
> PHY parameters to achieve board-level tuning (for USB/SATA interfaces, etc.).

Unlike the use of the HSP in the sdhci driver, where it appears to be
setting bits that indicate stability (according to your colleague) what
you say here (and what is done in the driver on the reset side in
particular) seems like something that should be handled by a dedicated
driver. "independent reset of specific modules" is the domain of
reset-controller drivers. What are the other modules for which the HSP
has resets? Does it have clocks for other modules too?

> The top-clock manages the global clocks of subsystems. Taking the HSP subsystem as an example, the top-clock
>  configures the hsp_aclk_ctrl and hsp_cfg_ctrl of HSP subsystem only.

> In contrast, the subsystem clocks are managed via their own CSRs. For instance, the USB ref clock used in the USB module of 
> the HSP subsystem can only be configured through the hsp-csr, and cannot be set via the top-clock controller driver.
> As for the reset function, it is not integrated into a dedicated controller driver either, for reasons similar to those of the 
> clock management mentioned above.

That just sounds to me like the hsp-csr needs to become both a
reset-controller and a clock-controller! It's not unusual to have more
than one clock-controller in an device, the top-clock being a
clock-controller does not mean that the HSP also cannot be one.


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

Powered by blists - more mailing lists