Re: [PATCH 2/2] clk: qcom: Add SM7250 display clock controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: qcom: dispcc-sm7250: missing CLK_SET_RATE_PARENT on byte0_intf_clk
--

commit ab220a77d171c816d3d394b53d2f36fed1ff7979
Author: Ishu Brar <patches@brar.si>

clk: qcom: Add SM7250 display clock controller driver

This commit adds the display clock controller driver for the Qualcomm SM7250
SoC, providing MDSS clocks, the MDSS GDSC, and the core reset.

> diff --git a/drivers/clk/qcom/dispcc-sm7250.c b/drivers/clk/qcom/dispcc-sm7250.c
> new file mode 100644
> index 0000000000000..f114a9f5407ef
> --- /dev/null
> +++ b/drivers/clk/qcom/dispcc-sm7250.c

[ ... ]

> +static struct clk_regmap_div dispcc_mdss_byte0_div_clk_src = {
> +	.reg = 0x2128,
> +	.shift = 0,
> +	.width = 2,
> +	.clkr = {
> +		.hw.init = &(const struct clk_init_data) {
> +			.name = "dispcc_mdss_byte0_div_clk_src",
> +			.parent_hws = (const struct clk_hw*[]) {
> +				&dispcc_mdss_byte0_clk_src.clkr.hw,
> +			},
> +			.num_parents = 1,
> +			.ops = &clk_regmap_div_ops,
> +		},
> +	},
> +};
> +
> +static struct clk_branch dispcc_mdss_byte0_intf_clk = {
> +	.halt_reg = 0x202c,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x202c,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(const struct clk_init_data) {
> +			.name = "dispcc_mdss_byte0_intf_clk",
> +			.parent_hws = (const struct clk_hw*[]) {
> +				&dispcc_mdss_byte0_div_clk_src.clkr.hw,
> +			},
> +			.num_parents = 1,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};

[Severity: Medium]
Should dispcc_mdss_byte0_intf_clk in drivers/clk/qcom/dispcc-sm7250.c set
.flags = CLK_SET_RATE_PARENT, matching dispcc_mdss_byte1_intf_clk right below
it?

During DSI0 power-on, msm_dsi_host_power_on() -> dsi_link_clk_set_rate_6g()
in drivers/gpu/drm/msm/dsi/dsi_host.c configures the byte interface clock:

	if (msm_host->byte_intf_clk) {
		ret = clk_set_rate(msm_host->byte_intf_clk, msm_host->byte_intf_clk_rate);
		if (ret) {
			pr_err("%s: Failed to set rate byte intf clk, %d\n",
			       __func__, ret);
			return ret;
		}
	}

Because dispcc_mdss_byte0_intf_clk uses clk_branch2_ops and omits
CLK_SET_RATE_PARENT, clk_core_round_rate_nolock() and clk_calc_new_rates() in
drivers/clk/clk.c treat it as a pass-through clock without an adjustable
parent and return 0 without forwarding the rate change to
dispcc_mdss_byte0_div_clk_src (register 0x2128), leaving the DSI0 byte
interface divider unconfigured when a non-default divider is needed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-dispcc-sm7250-v1-0-07bdefb99801@brar.si?part=2