Re: [PATCH v2 4/4] clk: siflower: Add SF19A2890 peripheral clock and reset manager driver

[Philipp Zabel <p.zabel@pengutronix.de>]

On Mi, 2026-11-11 at 03:19 -0500, Joseph Telaak via B4 Relay wrote:
> From: Chuanhong Guo <gch981213@gmail.com>
> 
> Add a platform driver for the peripheral clock and reset manager (CRM)
> block found on Siflower SF19A2890 and compatible SoCs.
> 
> Each CRM block manages a group of APB peripherals through three registers:
> a reset register, a clock gate register, and a shared Bus Output Enable
> (BOE) register that must be asserted for any peripheral in the block to
> respond to bus transactions.
> 
> The driver registers a clock gate controller and a reset controller from
> a single DT node. A shared spinlock protects all read-modify-write
> accesses across both. BOE is managed automatically: asserted when the
> first clock gate is enabled and deasserted when the last one is disabled.
> 
> Some peripherals have more than one reset line, one for each clock
> domain of the same IP, like the separate UART and APB resets of a
> PL011. The driver exposes each such group as one reset, because the
> lines have to be asserted together: the GMAC's two lines are the two
> sides of an asynchronous AXI bridge, and asserting one alone locks up
> the bus.
> 
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> [Joseph Telaak:
>   - take it from Chuanhong's siflower-7.1 branch
>   - one compatible per block, with its gate bits, critical gates and
>     reset masks in tables in the driver instead of the
>     siflower,valid-gates, siflower,critical-gates,
>     siflower,reset-masks and siflower,num-resets properties
>   - parent gate N to input clock N by index, not by global name
>   - update the gates under the spinlock too
>   - drop the separator comments
>   - Kconfig dependencies and help, subject]
> Co-developed-by: Joseph Telaak <joseph@telaak.dev>
> Signed-off-by: Joseph Telaak <joseph@telaak.dev>
> ---
>  MAINTAINERS                                    |   1 +
>  drivers/clk/siflower/Kconfig                   |  11 +
>  drivers/clk/siflower/Makefile                  |   1 +
>  drivers/clk/siflower/clk-sf19a2890-periphcrm.c | 368 +++++++++++++++++++++++++
>  4 files changed, 381 insertions(+)
> 
[...]
> diff --git a/drivers/clk/siflower/clk-sf19a2890-periphcrm.c b/drivers/clk/siflower/clk-sf19a2890-periphcrm.c
> new file mode 100644
> index 000000000000..349e0d7ce0c3
> --- /dev/null
> +++ b/drivers/clk/siflower/clk-sf19a2890-periphcrm.c
> @@ -0,0 +1,368 @@
[...]
> +static inline struct sf19a2890_periphcrm *
> +rcdev_to_periphcrm(struct reset_controller_dev *rcdev)
> +{
> +	return container_of(rcdev, struct sf19a2890_periphcrm, rcdev);
> +}
> +
> +static int sf19a2890_periphcrm_reset_update(struct reset_controller_dev *rcdev,
> +					    unsigned long id, bool assert)
> +{
> +	struct sf19a2890_periphcrm *crm = rcdev_to_periphcrm(rcdev);
> +	unsigned long flags;
> +	u32 reg;
> +
> +	spin_lock_irqsave(&crm->lock, flags);

Could be simplified with guard(spinlock)(&crm->lock);

> +	reg = readl(crm->base + REG_RESET);
> +	if (assert)
> +		reg |= crm->reset_masks[id];
> +	else
> +		reg &= ~crm->reset_masks[id];
> +	writel(reg, crm->base + REG_RESET);
> +	spin_unlock_irqrestore(&crm->lock, flags);
> +	return 0;
> +}
> +
[...]
> +static const u32 sf19a2890_usb_resets[] = {
> +	[RST_USB] = GENMASK(2, 0),

Do these three bits control separate reset lines?
Does the hardware require them to be changed together?

> +	[RST_USB_PHY_POR] = BIT(3),
> +	[RST_USB_PHY] = BIT(4),
> +};
[...]
> +static const u32 sf19a2890_uart_resets[] = {
> +	[RST_UART0] = BIT(0) | BIT(4),
> +	[RST_UART1] = BIT(1) | BIT(5),
> +	[RST_UART2] = BIT(2) | BIT(6),

Same question as above.

regards
Philipp