Re: [PATCH v7 09/23] media: qcom: camss: csid: Scale the clocks with the CSI-2 link rate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] media: qcom: camss: csid: sibling phy_rx branch clocks drop
  shared RCG rate
--

commit 08b369e2e076c38176db85510f92710a98c49c6c
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

media: qcom: camss: csid: Scale the clocks with the CSI-2 link rate

This commit scales the CSID core and CSIPHY receive clocks across all
SoC generations based on the CSI-2 link frequency and votes for minimum
clock rates using clk_set_rate_range() and clk_drop_range().

> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 51f20907ffde6..714cea6883a78 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c

[ ... ]

> @@ -532,72 +533,197 @@ const struct csid_format_info *csid_get_fmt_entry(const struct csid_format_info
>  	return &formats[0];
>  }
>  

[ ... ]

> +		/*
> +		 * Vote for the rate as a minimum, as the clock may be shared
> +		 * with another CSID which needs it to run faster.
> +		 */
> +		ret = clk_set_rate_range(clock->clk, clock->freq[j], ULONG_MAX);
> +		if (ret < 0) {
> +			dev_err(dev, "clk set rate range failed: %d\n", ret);
> +			return ret;
> +		}
>  	}
>  
>  	return 0;
>  }
>  
> +/*
> + * Drop the minimum rates the CSID votes for and take its clocks back to their
> + * lowest level, or to the lowest level the other users of a shared clock need.
> + */
> +static void csid_release_clock_rates(struct csid_device *csid)
> +{
> +	int i;
> +
> +	for (i = 0; i < csid->nclocks; i++) {
> +		struct camss_clock *clock = &csid->clock[i];
> +		long rate;
> +
> +		if (!clock->nfreqs || !csid_match_scaled_clock_name(clock))
> +			continue;
> +
> +		clk_drop_range(clock->clk);
> +
> +		/* Round first, so that the rate respects the other votes */
> +		rate = clk_round_rate(clock->clk, clock->freq[0]);
> +		if (rate > 0)
> +			clk_set_rate(clock->clk, rate);
> +	}
> +}

[Severity: High]
Can voting on clock->clk with clk_set_rate_range() fail to protect the
shared receive RCG when two CSIDs use different child branch clocks?

On SoCs such as SM8550, SM8750, Kaanapali, SA8775P, and SM6150, full and
lite CSIDs (or individual CSID instances) acquire separate phy_rx branch
clocks (for example "csiphy_rx" / cam_cc_csid_csiphy_rx_clk and
"vfe_lite_cphy_rx" / cam_cc_ife_lite_cphy_rx_clk) that set
CLK_SET_RATE_PARENT and share the single parent RCG
cam_cc_cphy_rx_clk_src:

drivers/media/platform/qcom/camss/camss.c:csid_res_8550[] = {
    ...
	/* CSID0 (full) */
	.clock = { "csid", "csiphy_rx" },
	.clock_rate = { { 400000000, 480000000 },
			{ 400000000, 480000000 } },
    ...
	/* CSID_LITE0 */
	.clock = { "vfe_lite_csid", "vfe_lite_cphy_rx" },
	.clock_rate = { { 400000000, 480000000 },
			{ 400000000, 480000000 } },
    ...
};

Because clk_set_rate_range() records the minimum rate on the child
branch clock's clk_core rather than on the shared parent RCG's clk_core,
rate calculations on one branch clock only inspect the rate boundaries of
that branch and its parent RCG, ignoring minimum rate votes held on
sibling branch clocks.  If CSID0 is streaming at 480 MHz and CSID_LITE0
either starts a slower stream or stops and runs
csid_release_clock_rates() to set clock->freq[0] (400 MHz), the clock
framework lowers cam_cc_cphy_rx_clk_src to 400 MHz and underclocks CSID0
mid-stream.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-add-support-for-camss-on-sm8750-v7-0-8ff2a11835f6@oss.qualcomm.com?part=9