Re: [PATCH v3 6/7] phy: qcom-mipi-csi2: Add SM8150 CSI2 DPHY support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] phy: qcom-mipi-csi2: wrong SM8150 lane polarity offsets in
  lanes_enable()
--

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

phy: qcom-mipi-csi2: Add SM8150 CSI2 DPHY support

This commit adds SM8150 platform configuration and lane initialization tables
to the Qualcomm MIPI CSI2 DPHY driver, along with support for a fixed timer
clock rate and single power domain attachment.

> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> index 966d79c98f9b2..b8b6589cee35d 100644
> --- a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c

[ ... ]

> @@ -352,6 +419,36 @@ static const struct mipi_csi2phy_hw_ops phy_qcom_mipi_csi2_ops_3ph_1_0 = {
>  	.lanes_disable = phy_qcom_mipi_csi2_lanes_disable,
>  };
>  
> +static const char * const sm8150_clks[] = {
> +	"core",
> +	"timer"
> +};
> +
> +static const char * const sm8150_supplies[] = {
> +	"vdda-phy",
> +	"vdda-pll"
> +};
> +
> +static struct mipi_csi2_genpd sm8150_genpds[] = {
> +	{ .name = "top", .scaled = false },
> +};
> +
> +const struct mipi_csi2phy_soc_cfg mipi_csi2_dphy_sm8150 = {
> +	.ops = &phy_qcom_mipi_csi2_ops_3ph_1_0,
> +	.reg_info = {
> +		.init_seq = lane_regs_sm8150,
> +		.lane_array_size = ARRAY_SIZE(lane_regs_sm8150),
> +		.common_regs_offset = 0x800,
> +	},

[Severity: Medium]
Does phy_qcom_mipi_csi2_lanes_enable() write lane polarity inversion to the
wrong register offsets on SM8150?

In lane_regs_sm8150[], the physical lane register blocks use a 0x100-byte
stride (data lanes at 0x0000, 0x0200, 0x0400, 0x0600 and the clock lane at
0x0700) with common registers starting at 0x0800.  However,
phy_qcom_mipi_csi2_lanes_enable() programs CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n(),
which hardcodes X1E80100's 0x200-byte per-lane stride:

drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c:
    #define CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n(n)		((0x200 * (n)) + 0x24)
    ...
    phy_qcom_mipi_csi2_lanes_enable() {
        ...
	/* Lane configuration for polarity @ CSIPHY-base + CTRL9 */
	for (i = 0; i < cfg->num_data_lanes; i++) {
		if (lane_cfg->data[i].pol) {
			u8 pos = lane_cfg->data[i].pos;

			writel(BIT(2), csi2phy->base + CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n(pos * 2));
		}
	}

	if (lane_cfg->clk.pol)
		writel(BIT(2), csi2phy->base + CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n(lane_cfg->clk.pos));

When a board configures non-zero lane-polarities on SM8150, data lanes 1..3
and the clock lane write to 0x0424, 0x0824, 0x0c24, and 0x0e24 instead of
0x0224, 0x0424, 0x0624, and 0x0724, leaving those lanes uninverted and
overwriting CSI_COMMON_CTRL9 at 0x0824 when data lane 2 is inverted.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-sm8150-v7.4-rc1-camss-v3-0-0103e8a0bbcd@andrewgigena.dev?part=6