Re: [PATCH] clk: qcom: camcc-sdm845/670: Raise CSIPHY timer clock limit

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The newly added 288 MHz CSIPHY timer clock entry erroneously uses a fixed 808 MHz shared PLL with a divider of 2, configuring the hardware for 404 MHz while falsely tracking it as 288 MHz, or otherwise forcing an invalid shared PLL rate.
- [High] Adding a dummy 400 MHz entry to the camss clock_rate tables bypasses the link frequency safety margin validation, allowing unsupported frequencies to be silently clamped.
--

--- Patch [1]: [PATCH] clk: qcom: camcc-sdm845/670: Raise CSIPHY timer clock limit ---
commit 376f8b331f3abeb701471cc1ebbcaaf5bbe8d122
Author: David Heidelberg <[email protected]>

clk: qcom: camcc-sdm845/670: Raise CSIPHY timer clock limit

This commit adds a 288 MHz entry to the shared camcc CSIPHY timer frequency
table and adds a 400 MHz entry to the sdm845 and sdm670 CSIPHY timer
clock_rate tables. This aims to allow the Samsung S5K2X7SP camera to run at a
1.15 GHz link frequency.

> diff --git a/drivers/clk/qcom/camcc-sdm845.c b/drivers/clk/qcom/camcc-sdm845.c
> index 534dc56fc13ca..77bb783b4ef0a 100644
> --- a/drivers/clk/qcom/camcc-sdm845.c
> +++ b/drivers/clk/qcom/camcc-sdm845.c
> @@ -255,6 +255,7 @@ static const struct freq_tbl ftbl_cam_cc_csi0phytimer_clk_src[] = {
>  	F(19200000, P_BI_TCXO, 1, 0, 0),
>  	F(240000000, P_CAM_CC_PLL2_OUT_EVEN, 2, 0, 0),
>  	F(269333333, P_CAM_CC_PLL1_OUT_EVEN, 3, 0, 0),
> +	F(288000000, P_CAM_CC_PLL1_OUT_EVEN, 2, 0, 0),

[Severity: High]
Does this configuration force an invalid rate on the shared PLL1, or
mathematically result in an incorrect output frequency?

When requesting 288 MHz in ftbl_cam_cc_csi0phytimer_clk_src, this table uses
P_CAM_CC_PLL1_OUT_EVEN with a divider of 2. If the parent PLL1 is statically
probed at 808 MHz, a divider of 2 produces 404 MHz instead of 288 MHz, which
could break CSI protocol timing.

Alternatively, since the clock rate request propagates to the parent PLL, does
this cause the framework to request 576 MHz (288 MHz * 2) from PLL1?

Reconfiguring a shared PLL would disrupt other consumers like BPS, IFE, and
IPE that rely on the original parent rate.

>  	{ }
>  };
>  
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 2123f6388e3d7..e1bf828cf7af2 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1075,7 +1075,7 @@ static const struct camss_subdev_resources csiphy_res_670[] = {
>  		.clock_rate = { { 0 },
>  				{ 0 },
>  				{ 0 },
> -				{ 19200000, 240000000, 269333333 } },
> +				{ 19200000, 240000000, 269333333, 400000000 } },

[Severity: High]
Could the addition of this 400 MHz entry bypass the link frequency safety
margin validation in the camss driver?

By adding a dummy 400 MHz entry here in csiphy_res_670 (and similarly in
csiphy_res_845) when the clock controller only provides up to 288 MHz, it
appears any sensor configuration requiring between 288 MHz and 380 MHz will
erroneously pass the margin check.

When the hardware then requests 400 MHz, will clk_round_rate() silently
clamp it down to 288 MHz?

This might cause new sensor configurations requiring a frequency in this gap
to fail at runtime with obscure hardware pipeline failures instead of being
cleanly rejected.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1