Re: [PATCH 7/7] clk: stm32mp25: add ck_ker_usb2phy1_stp clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] clk: stm32mp25: missing .disable_unused in clk_stm32_usb2phy1_stp_ops
- [Medium] clk: stm32mp25: stale or zero cached rate for ck_ker_usb2phy1_stp
--

commit 34669aa594d0d50f9cff33a984e8708660cbe4e8
Author: Gabriel Fernandez <gabriel.fernandez@foss.st.com>

clk: stm32mp25: add ck_ker_usb2phy1_stp clock

This commit adds the ck_ker_usb2phy1_stp gate clock for USB Host low power
support on STM32MP25, parenting it to HSE_KER_ON and deriving its rate from
ck_ker_usb2phy1.

> diff --git a/drivers/clk/stm32/clk-stm32mp25.c b/drivers/clk/stm32/clk-stm32mp25.c
> index a6a1293370eb5..649af99680f46 100644
> --- a/drivers/clk/stm32/clk-stm32mp25.c
> +++ b/drivers/clk/stm32/clk-stm32mp25.c

[ ... ]

> @@ -1635,6 +1637,42 @@ static struct clk_stm32_composite ck_ker_usb2phy1 = {
>  					    &clk_stm32_composite_ops, 0),
>  };
>  
> +static int clk_stm32_usb2phy1_stp_enable(struct clk_hw *hw)
> +{
> +	return clk_stm32_gate_ops.enable(hw);
> +}
> +
> +static void clk_stm32_usb2phy1_stp_disable(struct clk_hw *hw)
> +{
> +	clk_stm32_gate_ops.disable(hw);
> +}
> +
> +static int clk_stm32_usb2phy1_stp_is_enabled(struct clk_hw *hw)
> +{
> +	return clk_stm32_gate_ops.is_enabled(hw);
> +}
> +
> +static unsigned long clk_stm32_usb2phy1_stp_recalc_rate(struct clk_hw *hw,
> +							unsigned long parent_rate)
> +{
> +	/* The rate of ck_ker_usb2phy1_stp is the same as ck_ker_usb2phy1 */
> +	return clk_hw_get_rate(&ck_ker_usb2phy1.hw);
> +}
> +
> +static const struct clk_ops clk_stm32_usb2phy1_stp_ops = {
> +	.enable		= clk_stm32_usb2phy1_stp_enable,
> +	.disable	= clk_stm32_usb2phy1_stp_disable,
> +	.is_enabled	= clk_stm32_usb2phy1_stp_is_enabled,
> +	.recalc_rate	= clk_stm32_usb2phy1_stp_recalc_rate,
> +};

[Severity: Medium]
Should clk_stm32_usb2phy1_stp_ops also wire up .disable_unused to
clk_stm32_gate_ops.disable_unused?

In drivers/clk/stm32/clk-stm32-core.c, clk_stm32_gate_ops provides a dedicated
.disable_unused callback:

const struct clk_ops clk_stm32_gate_ops = {
	.enable		= clk_stm32_gate_enable,
	.disable	= clk_stm32_gate_disable,
	.is_enabled	= clk_stm32_gate_is_enabled,
	.disable_unused	= clk_stm32_gate_disable_unused,
};

Because clk_stm32_usb2phy1_stp_ops omits .disable_unused,
clk_disable_unused_subtree() in drivers/clk/clk.c falls back to calling
core->ops->disable(core->hw) when the clock is enabled in hardware at boot
(bit 4 of RCC_USB2PHY1CFGR set) but unused by any consumer:

	if (clk_core_is_enabled(core)) {
		trace_clk_disable(core);
		if (core->ops->disable_unused)
			core->ops->disable_unused(core->hw);
		else if (core->ops->disable)
			core->ops->disable(core->hw);
		trace_clk_disable_complete(core);
	}

That calls clk_stm32_usb2phy1_stp_disable() -> clk_stm32_gate_ops.disable() ->
stm32_gate_endisable() with data->gate_cpt[GATE_USB2PHY1STP] == 0:

	} else {
		if (--data->gate_cpt[gate_id] > 0)
			return;

		if (gate->set_clr != 0)
			writel(BIT(gate->bit_idx), addr + gate->set_clr);
		else
			writel(readl(addr) & ~BIT(gate->bit_idx), addr);
	}

Wouldn't this underflow the u16 reference counter
data->gate_cpt[GATE_USB2PHY1STP] from 0 to 0xffff, causing
stm32_gate_endisable() to return early without clearing bit 4 in
RCC_USB2PHY1CFGR and preventing later clk_enable() / clk_disable() cycles from
disabling the gate?

> +
> +/* Parent clock of ck_ker_usb2phy1_stp is HSE_KER_ON to ensure it remains active during low power */
> +static struct clk_stm32_gate ck_ker_usb2phy1_stp = {
> +	.gate_id = GATE_USB2PHY1STP,
> +	.hw.init = CLK_HW_INIT_INDEX("ck_ker_usb2phy1_stp", HSE_KER_ON,
> +				     &clk_stm32_usb2phy1_stp_ops, 0),
> +};

[Severity: Medium]
Can clk_get_rate() on ck_ker_usb2phy1_stp return a stale or zero cached rate
here?

In drivers/clk/clk.c, clk_core_get_rate_recalc() only invokes .recalc_rate on
clk_get_rate() when CLK_GET_RATE_NOCACHE is set:

static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
{
	if (core && (core->flags & CLK_GET_RATE_NOCACHE))
		__clk_recalc_rates(core, false, 0);

	return clk_core_get_rate_nolock(core);
}

Because ck_ker_usb2phy1_stp is parented to HSE_KER_ON rather than
ck_ker_usb2phy1 and its flags are 0, any rate or parent change on
ck_ker_usb2phy1 (such as via assigned-clock-parents / assigned-clock-rates in
of_clk_set_defaults() or runtime clk_set_rate() / clk_set_parent()) only
recalculates ck_ker_usb2phy1's subtree and leaves ck_ker_usb2phy1_stp's cached
core->rate stale.

In addition, when stm32mp25_rcc_clocks_probe() runs before the SCMI clock
provider is registered, stm32_rcc_clock_init() registers ck_ker_usb2phy1 and
then ck_ker_usb2phy1_stp as orphans via hlist_add_head(&core->child_node,
&clk_orphan_list) in __clk_core_init(). Because hlist_add_head() places
ck_ker_usb2phy1_stp ahead of ck_ker_usb2phy1 on clk_orphan_list,
clk_core_reparent_orphans_nolock() reparents and recalculates
ck_ker_usb2phy1_stp while ck_ker_usb2phy1 is still an orphan with rate 0,
caching 0 for ck_ker_usb2phy1_stp.

Finally, on device trees such as arch/arm64/boot/dts/st/stm32mp251.dtsi and
stm32mp231.dtsi where the optional CK_SCMI_HSE_KER_ON clock is not listed in
the rcc node's clocks property, ck_ker_usb2phy1_stp remains an orphan
(core->parent == NULL), causing clk_core_get_rate_nolock() to always return 0.

Should ck_ker_usb2phy1_stp set CLK_GET_RATE_NOCACHE, and how should the case
where HSE_KER_ON is absent from the device tree be handled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-ups_mp25_clk_update-v1-0-e906e7a38cb3@foss.st.com?part=7