Re: [PATCH v3 3/3] clk: qcom: add clock controller driver for qca8386/qca8084

[Konrad Dybcio]

On 10.08.2023 13:54, Luo Jie wrote:
> Add clock & reset controller driver for qca8386/qca8084.
> 
> Signed-off-by: Luo Jie <quic_luoj@xxxxxxxxxxx>
> ---

> +struct qcom_cc {
> +	struct qcom_reset_controller reset;
> +	struct clk_regmap **rclks;
> +	size_t num_rclks;
> +};
This all, including the probe func, is required because of the MDIO dance,
I assume?

Commonizing that would make more sense should more clocks like this appear
in the future.

[...]

> +static struct clk_branch nss_cc_switch_core_clk = {
> +	.halt_reg = 0x8,
> +	.clkr = {
> +		.enable_reg = 0x8,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(const struct clk_init_data) {
> +			.name = "nss_cc_switch_core_clk",
> +			.parent_hws = (const struct clk_hw *[]) {
> +				&nss_cc_switch_core_clk_src.clkr.hw,
> +			},
> +			.num_parents = 1,
> +			/* Can be disabled in PHY mode for power saving */
Well it clearly cannot be disabled if it has the CLK_IS_CRITICAL flag :D

What's the "PHY mode" you're talking about?

> +			.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
> +			.ops = &clk_branch2_mdio_ops,
> +		},
> +	},
> +};
I see a whole bunch of CRITICAL clocks.. please make sure these clocks
are actually necessary for Linux to know about (i.e. if we don't need
to call any operations on them, we might just skip registering them
with the driver).

Konrad