Re: [PATCH v12 5/5] clk: en7523: add support for Airoha AN7583 clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] clk: en7523: AN7583 MDIO clocks lack rate ops in
  en7581_register_clocks()
- [Medium] clk: en7523: mismatched clk_ops callbacks in
  an7583_data.pcie_ops
--

commit 80220805154b9c38bb07ac1b3edf5679520c435d
Author: Christian Marangi <ansuelsmth@gmail.com>

clk: en7523: add support for Airoha AN7583 clock

This commit adds clock and reset controller support for the Airoha AN7583
SoC to the en7523 clock driver.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 7d302498c3d51..7523ec7e0d8aa 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c

[ ... ]

> @@ -369,6 +378,138 @@ static const struct en_clk_desc en7581_base_clks[] = {
>  	}
>  };
>  
> +static const struct en_clk_desc an7583_base_clks[] = {

[ ... ]

> +	}, {
> +		.id = AN7583_CLK_MDIO0,
> +		.name = "mdio0",
> +
> +		.base_reg = REG_CRYPTO_CLKSRC2,
> +
> +		.base_value = 25000000,
> +
> +		.div_bits = 4,
> +		.div_shift = 15,
> +		.div_step = 1,
> +		.div_offset = 1,
> +	}, {
> +		.id = AN7583_CLK_MDIO1,
> +		.name = "mdio1",
> +
> +		.base_reg = REG_CRYPTO_CLKSRC2,
> +
> +		.base_value = 25000000,
> +
> +		.div_bits = 4,
> +		.div_shift = 19,
> +		.div_step = 1,
> +		.div_offset = 1,
> +	}
> +};

[Severity: Medium]
Can AN7583_CLK_MDIO0 and AN7583_CLK_MDIO1 change their rate when registered
through en7581_register_clocks()?

en7581_register_clocks() registers every entry in base_clks with
devm_clk_hw_register_fixed_rate(), whose clk_fixed_rate_ops lacks
.determine_rate and .set_rate callbacks.  When airoha_mdio_probe() in
drivers/net/mdio/mdio-airoha.c calls clk_set_rate() to divide the 25 MHz
default clock down to 2.5 MHz, clk_set_rate() returns 0 without updating
the 4-bit divider in REG_CRYPTO_CLKSRC2, leaving the MDIO bus at 25 MHz.
Earlier series discussion noted that adjustable clock rate support is
planned for a separate series, but the in-tree airoha-mdio consumer already
relies on clk_set_rate().

[ ... ]

> @@ -1329,6 +1565,18 @@ static const struct en_clk_soc_data en7581_data = {
>  	.hw_init = en7581_clk_hw_init,
>  };
>  
> +static const struct en_clk_soc_data an7583_data = {
> +	.base_clks = an7583_base_clks,
> +	/* We increment num_clocks by 1 to account for additional PCIe clock */
> +	.num_clocks = ARRAY_SIZE(an7583_base_clks) + 1,
> +	.pcie_ops = {
> +		.is_enabled = en7581_pci_is_enabled,
> +		.prepare = en7581_pci_enable,
> +		.unprepare = en7581_pci_disable,
> +	},
> +	.hw_init = an7583_clk_hw_init,
> +};

[Severity: Medium]
Should pcie_ops pair .prepare and .unprepare with .is_prepared rather than
.is_enabled?

Because en7581_pci_enable() and en7581_pci_disable() are wired to .prepare
and .unprepare while .is_prepared, .disable, and .pci_unprepare are NULL,
clk_unprepare_unused_subtree() sees prepare_count == 0 with no .is_prepared
callback and skips .unprepare, while clk_disable_unused_subtree() sees
.is_enabled return true with no .disable callback.  If the PCIe clock is
enabled in REG_PCI_CONTROL at boot and no consumer claims EN7523_CLK_PCIE,
clk_disable_unused() never gates it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110000753.508-1-ansuelsmth@gmail.com?part=5