Re: [PATCH v2 2/9] media: camss: csiphy: add CSIPHY 2.4.0 lane settings for SM8750

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] media: camss: csiphy: SKEW_CAL value mismatch in lane_regs_sm8750
--

commit d9b4ac81070d2e3ae2fc4404b18f6fc6946dfb72
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

media: camss: csiphy: add CSIPHY 2.4.0 lane settings for SM8750

This commit adds the CSIPHY 2.4.0 two-phase lane register table
lane_regs_sm8750[] for SM8750, marks CAMSS_8750 as a generation-2 PHY in
csiphy_is_gen2(), and configures the lane table and 0x138 common status
offset in csiphy_init().

>     media: camss: csiphy: add CSIPHY 2.4.0 lane settings for SM8750
>     
>     SM8750 uses the CSIPHY 2.4.0 CSI PHY.  The block keeps the register layout
>     already handled by csiphy_ops_3ph_1_0 (four data lanes at a 0x400 stride,
>     clock lane at 0xc00, common block at +0x1000), so the generation-2 lane
>     programming sequence applies unchanged, but the per-lane values differ from
>     the 2.3.0 block used on SM8550/SM8650:
>     
>       - the lane register 0x0000 default is 0x8c instead of 0x8e,
>       - every lane gains the four SKEW_CAL registers
>         (0x0094/0x005c/0x0060/0x0064),
>       - the common status block sits at 0x138 instead of 0xb0.
>     
>     Add lane_regs_sm8750[] with those values, report CAMSS_8750 as a
>     generation-2 PHY, and select the new table together with the 0x138
>     common status offset in csiphy_init().
>     
>     The register map is the one the CSIPHY 2.4.0 vendor driver uses for its
>     two-phase variant (csiphy_2ph_v2_4_0_reg, 103 entries): address, mask and
>     parameter type are identical for all 103 entries, and the data byte is
>     identical for 99.  The four remaining entries are the SKEW_CAL data bytes
>     of the four lanes (0x005c and its per-lane copies), which are 0x00 here,
>     i.e. the vendor value.  This SoC wires the PHY in two-phase mode; the
>     three-phase variant of the same PHY (csiphy_3ph_v2_4_0_reg, 72 entries)
>     uses a different address layout, so its table cannot be reused.  The
>     0x138 common status offset is where the vendor parameter block of this
>     generation points its status register window (0x1000 + 0x138).
>     
>     These settings are the ones the CSIPHY runs with on a Lenovo TB322FC
>     (SM8750), where both the rear s5kjns and the front gc08a8 sensor stream
>     at full resolution and 30 fps.  Enabling the SKEW_CAL data byte of the
>     four lanes (0x005c = 0x54) instead of leaving it at the vendor value was
>     also measured on that device: 90 frames with each setting, no error
>     reported, 30.26 fps and 53.81 vs 53.78 frame time deviation, i.e. no
>     measurable difference, so the vendor value is kept.
>     
>     Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
> 
> 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..9cf22ff17baef 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
> @@ -804,6 +804,123 @@ csiphy_lane_regs lane_regs_sm8650[] = {
>  	{0x0c10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
>  };
>  
> +/* GEN2 2.4.0 2PH 4 lane DPHY mode */
> +static const struct
> +csiphy_lane_regs lane_regs_sm8750[] = {
> +	/* LN 0 */

[ ... ]

> +	{0x0094, 0xD7, 0x00, CSIPHY_SKEW_CAL},
> +	{0x005C, 0x54, 0x00, CSIPHY_SKEW_CAL},
> +	{0x0060, 0xFD, 0x00, CSIPHY_SKEW_CAL},
> +	{0x0064, 0x7F, 0x00, CSIPHY_SKEW_CAL},
> +
> +	/* LN 2 */

[ ... ]

> +	{0x0494, 0xD7, 0x00, CSIPHY_SKEW_CAL},
> +	{0x045C, 0x54, 0x00, CSIPHY_SKEW_CAL},
> +	{0x0460, 0xFD, 0x00, CSIPHY_SKEW_CAL},
> +	{0x0464, 0x7F, 0x00, CSIPHY_SKEW_CAL},
> +
> +	/* LN 4 */

[ ... ]

> +	{0x0894, 0xD7, 0x00, CSIPHY_SKEW_CAL},
> +	{0x085C, 0x54, 0x00, CSIPHY_SKEW_CAL},
> +	{0x0860, 0xFD, 0x00, CSIPHY_SKEW_CAL},
> +	{0x0864, 0x7F, 0x00, CSIPHY_SKEW_CAL},
> +
> +	/* LN 6 */

[ ... ]

> +	{0x0C94, 0xD7, 0x00, CSIPHY_SKEW_CAL},
> +	{0x0C5C, 0x54, 0x00, CSIPHY_SKEW_CAL},
> +	{0x0C60, 0xFD, 0x00, CSIPHY_SKEW_CAL},
> +	{0x0C64, 0x7F, 0x00, CSIPHY_SKEW_CAL},

[Severity: Medium]
Should the reg_data values for 0x005C, 0x045C, 0x085C, and 0x0C5C in
lane_regs_sm8750[] in drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
be 0x00 instead of 0x54 in this commit?

The commit message states that the SKEW_CAL data bytes of the four lanes
(0x005c and its per-lane copies) are 0x00 to match the vendor value, and that
the vendor value is kept after testing 0x54. However, lane_regs_sm8750[] sets
reg_data to 0x54 for all four entries here, and a later commit in the series,
68a5dddeaab25 ("media: camss: csid: add the 980 generation"), changes these
four entries in lane_regs_sm8750[] from 0x54 to 0x00.

Also, when csiphy_init() selects lane_regs_sm8750[] for CAMSS_8750 and stream
startup runs csiphy_lanes_enable() -> csiphy_gen2_config_lanes(), entries
marked with CSIPHY_SKEW_CAL are currently skipped without writing to hardware:

drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c:csiphy_gen2_config_lanes() {
	for (i = 0; i < array_size; i++, r++) {
		switch (r->csiphy_param_type) {
		...
		case CSIPHY_SKEW_CAL:
			/* TODO: support application of skew from dt flag */
			continue;
		...
		}
		writel_relaxed(val, csiphy->base + r->reg_addr);
}

Because CSIPHY_SKEW_CAL entries hit continue before writel_relaxed(), changing
0x005c between 0x00 and 0x54 in lane_regs_sm8750[] is currently a no-op at
runtime, and leaving 0x54 in the table would program the non-vendor value once
CSIPHY_SKEW_CAL handling is implemented.

Additionally, the commit message mentions "four data lanes at a 0x400 stride,
clock lane at 0xc00", whereas in lane_regs_sm8750[] the four data lanes (LN 0,
LN 2, LN 4, LN 6) are at 0x0000, 0x0400, 0x0800, and 0x0c00, and the clock
lane (LN CLK) is at 0x0e00. Should the commit message and table values in this
patch be updated to match?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028093003.722745-1-xiaqinghuan@mail.ustc.edu.cn?part=2