Re: [PATCH 2/8] media: qcom: camss: Add SM8150 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] media: qcom: camss: vfe_lite0 and vfe_lite1 clock rates never
  set on SM8150
- [Low] media: qcom: camss: lane_regs_sm8150 duplicates
  lane_regs_sc8280xp
--

commit c1d0bee4e04b8b076b3e10ab488e0e81fc0e07f5
Author: Gaurav Suman <gauravsuman007@gmail.com>

media: qcom: camss: Add SM8150 support

This commit adds SM8150 (Titan 175) support to the Qualcomm CAMSS driver,
including CSIPHY v1.1 lane settings and CSIPHY, CSID, VFE, and interconnect
resource definitions.

> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> index dac8d2ecf7995..3d8992d5dbc7b 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> @@ -144,6 +144,81 @@ csiphy_lane_regs lane_regs_sa8775p[] = {
>  	{0x0664, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
>  };
>  
> +/* SM8150 (Titan 175): CSIPHY v1.1, 2-phase lane settings */
> +static const struct
> +csiphy_lane_regs lane_regs_sm8150[] = {
> +	{0x0004, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	{0x002C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	{0x0034, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},

[ ... ]

> @@ -1221,6 +1297,10 @@ static int csiphy_init(struct csiphy_device *csiphy)
>  		regs->lane_regs = &lane_regs_sdm845[0];
>  		regs->lane_array_size = ARRAY_SIZE(lane_regs_sdm845);
>  		break;
> +	case CAMSS_8150:
> +		regs->lane_regs = &lane_regs_sm8150[0];
> +		regs->lane_array_size = ARRAY_SIZE(lane_regs_sm8150);
> +		break;
>  	case CAMSS_2290:
>  	case CAMSS_6150:
>  		regs->lane_regs = &lane_regs_qcm2290[0];

[Severity: Low]
This isn't a bug, but lane_regs_sm8150 is identical to the existing
lane_regs_sc8280xp table in this file.  Could CAMSS_8150 share the
CAMSS_8280XP case in csiphy_init() instead of duplicating the 70-entry
table?

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index c54185c93a8a9..bda61bf1f5739 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c

[ ... ]

> +	/* VFE_LITE0 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "camnoc_axi", "cpas_ahb", "slow_ahb_src",
> +			   "vfe_lite0" },
> +		.clock_rate = { { 0 },
> +				{ 400000000 },
> +				{ 0 },
> +				{ 0 },
> +				{ 320000000, 400000000, 480000000, 600000000 } },
> +		.reg = { "vfe_lite0" },
> +		.interrupt = { "vfe_lite0" },
> +		.vfe = {
> +			.line_num = 4,
> +			.is_lite = true,
> +			.hw_ops = &vfe_ops_170,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},
> +	/* VFE_LITE1 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "camnoc_axi", "cpas_ahb", "slow_ahb_src",
> +			   "vfe_lite1" },
> +		.clock_rate = { { 0 },
> +				{ 400000000 },
> +				{ 0 },
> +				{ 0 },
> +				{ 320000000, 400000000, 480000000, 600000000 } },
> +		.reg = { "vfe_lite1" },
> +		.interrupt = { "vfe_lite1" },
> +		.vfe = {
> +			.line_num = 4,
> +			.is_lite = true,
> +			.hw_ops = &vfe_ops_170,
> +			.formats_rdi = &vfe_formats_rdi_845,
> +			.formats_pix = &vfe_formats_pix_845
> +		}
> +	},

[Severity: High]
Does vfe_set_clock_rates() skip setting the clock rates for vfe_lite0 and
vfe_lite1?

VFE_LITE0 and VFE_LITE1 sit at indices 2 and 3 of vfe_res_sm8150, so
camss_init_subdevices() -> msm_vfe_subdev_init() assigns vfe->id = 2 and
vfe->id = 3.  When vfe_set_clock_rates() checks each clock,
vfe_match_clock_names() formats the lite clock name from vfe->id:

drivers/media/platform/qcom/camss/camss-vfe.c:vfe_match_clock_names() {
	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->id 2 and 3, vfe_lite_name is "vfe_lite2" and "vfe_lite3", which
does not match "vfe_lite0" or "vfe_lite1".  Because csid_res_sm8150 does
not list these clocks either, clk_set_rate() is never called on vfe_lite0
or vfe_lite1, leaving both VFE Lite blocks at their 19.2 MHz boot rate when
streaming through VFE_LITE0 or VFE_LITE1.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-sm8150-camera-v1-0-3d2e7b407516@gmail.com?part=2