Re: [PATCH 3/4] media: qcom: camss: Add support for SM8150

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: qcom: camss: vfe_lite0/1 clock rates ignored in vfe_set_clock_rates()
- [Medium] media: qcom: camss: vdda-phy and vdda-pll assigned to CSID instead of CSIPHY
--

commit aa7ed9591fa067f32f7175a9e60c6b9597ae38fa
Author: Andrew Gigena <mail@andrewgigena.dev>

media: qcom: camss: Add support for SM8150

This commit adds CAMSS platform support for the Qualcomm SM8150 SoC,
including four CSIPHY, four CSID, two VFE, and two VFE-Lite instances. It
adds CAMSS_8150 to the shared CSIPHY and VFE paths and defines the per-block
clocks, regulators, power domains, interrupts, and interconnects.

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index c54185c93a8a9..7035f5b333ff3 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1940,6 +1940,254 @@ static const struct resources_icc icc_res_sm6350[] = {
>  	},
>  };
>  
> +static const struct camss_subdev_resources csiphy_res_sm8150[] = {
> +	/* CSIPHY0 */
> +	{
> +		.regulators = {},
> +		.clock = { "csiphy0", "csiphy0_timer" },

[ ... ]

> +static const struct camss_subdev_resources csid_res_sm8150[] = {
> +	/* CSID0 */
> +	{
> +		.regulators = {
> +			{ .supply = "vdda-phy", .init_load_uA = 0 },
> +			{ .supply = "vdda-pll", .init_load_uA = 0 }
> +		},

[Severity: Medium]
Should the vdda-phy and vdda-pll supplies be attached to csiphy_res_sm8150
instead of csid_res_sm8150 (similar to how commit 0567d0f1cc62 moved them
from CSID to CSIPHY for SM8250)?

When streaming starts via video_prepare_streaming() -> v4l2_pipeline_pm_get()
-> pipeline_pm_power(), media_graph_walk_next() pops upstream entities first,
invoking csiphy_set_power_legacy() on CSIPHY before csid_set_power() on CSID:

drivers/media/platform/qcom/camss/camss-csiphy.c:csiphy_set_power_legacy() {
	...
		ret = regulator_bulk_enable(csiphy->num_supplies,
					    csiphy->supplies);
		if (ret < 0) {
			pm_runtime_put_sync(dev);
			return ret;
		}

		ret = camss_enable_clocks(csiphy->nclocks, csiphy->clock, dev);
		if (ret < 0) {
			regulator_bulk_disable(csiphy->num_supplies,
					       csiphy->supplies);
			pm_runtime_put_sync(dev);
			return ret;
		}

		enable_irq(csiphy->irq);

		csiphy_reset(csiphy);

		csiphy->res->hw_ops->hw_version_read(csiphy, dev);
	...
}

With csiphy_res_sm8150 leaving .regulators empty and csid_res_sm8150 holding
vdda-phy and vdda-pll, won't csiphy_set_power_legacy() enable the CSIPHY
clocks and access CSIPHY registers in csiphy_reset() and hw_version_read()
before the PHY regulators are enabled, and also power on the PHY regulators
unnecessarily when CSID is used standalone with its test pattern generator?

> +		.clock = { "vfe0_csid", "vfe0_cphy_rx", "vfe0", "vfe0_axi" },

[ ... ]

> +	/* CSID_LITE0 */
> +	{
> +		.regulators = {
> +			{ .supply = "vdda-phy", .init_load_uA = 0 },
> +			{ .supply = "vdda-pll", .init_load_uA = 0 }
> +		},
> +		.clock = { "vfe_lite0_csid", "vfe_lite0_cphy_rx", "vfe_lite0" },
> +		.clock_rate = { { 400000000, 480000000, 600000000 },
> +				{ 0 },
> +				{ 0 }, },

[ ... ]

> +static const struct camss_subdev_resources vfe_res_sm8150[] = {

[ ... ]

> +	/* VFE_LITE_0 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "gcc_axi_sf", "cpas_ahb", "camnoc_axi", "vfe_lite0" },
> +		.clock_rate = { { 0 },
> +				{ 0 },
> +				{ 19200000, 80000000},
> +				{ 19200000, 150000000, 266666667, 320000000, 400000000, 480000000 },
> +				{ 320000000, 400000000, 480000000, 600000000 }, },
> +		.reg = { "vfe_lite0" },
> +		.interrupt = { "vfe_lite0" },
> +		.vfe = {
> +			.is_lite = true,
> +			.line_num = 4,
> +			.hw_ops = &vfe_ops_170,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},
> +	/* VFE_LITE_1 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "gcc_axi_sf", "cpas_ahb", "camnoc_axi", "vfe_lite1" },
> +		.clock_rate = { { 0 },
> +				{ 0 },
> +				{ 19200000, 80000000},
> +				{ 19200000, 150000000, 266666667, 320000000, 400000000, 480000000 },
> +				{ 320000000, 400000000, 480000000, 600000000 }, },

[Severity: High]
Will the clock rates for vfe_lite0 and vfe_lite1 (as well as cpas_ahb) ever
get configured on SM8150?

During probe, camss_init_subdevices() initializes each entry in
vfe_res_sm8150 with vfe->id set to the array index, so VFE_LITE_0 has
vfe->id == 2 and VFE_LITE_1 has vfe->id == 3.

When powering on a VFE instance, vfe_get() calls vfe_set_clock_rates() (or
vfe_check_clock_rates()), which only processes clocks matching
vfe_match_clock_names():

drivers/media/platform/qcom/camss/camss-vfe.c:vfe_match_clock_names() {
	char vfe_name[7]; /* vfeXXX\0 */
	char vfe_lite_name[12]; /* vfe_liteXXX\0 */

	snprintf(vfe_name, sizeof(vfe_name), "vfe%d", vfe->id);
	snprintf(vfe_lite_name, sizeof(vfe_lite_name), "vfe_lite%d", vfe->id);

	return (!strcmp(clock->name, vfe_name) ||
		!strcmp(clock->name, vfe_lite_name) ||
		!strcmp(clock->name, "vfe_lite") ||
		!strcmp(clock->name, "camnoc_axi") ||
		!strcmp(clock->name, "camnoc_rt_axi"));
}

For VFE_LITE_0 and VFE_LITE_1, vfe_lite_name evaluates to "vfe_lite2" and
"vfe_lite3", which do not match "vfe_lite0" or "vfe_lite1" (nor does
vfe_match_clock_names() match "cpas_ahb"). Since csid_res_sm8150 also sets
the clock_rate entry for "vfe_lite0" and "vfe_lite1" to 0, neither
csid_set_clock_rates() nor vfe_set_clock_rates() will set their rates,
leaving the VFE-Lite clocks at their 19.2 MHz boot frequency when streaming.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-sm8150-v7-4-rc1-camss-v1-0-7a9534a9a22f@andrewgigena.dev?part=3