Re: [PATCH v2 3/3] media: qcom: camss: tpg: Add TPG support for SA8775P

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 7/17/25 5:20 AM, Wenmeng Liu wrote:
> Add support for TPG found on SA8775P.
> 
> Signed-off-by: Wenmeng Liu <quic_wenmliu@quicinc.com>
> ---

[...]

> +static int tpg_stream_on(struct tpg_device *tpg)
> +{
> +	struct tpg_testgen_config *tg = &tpg->testgen;
> +	struct v4l2_mbus_framefmt *input_format;
> +	const struct tpg_format_info *format;
> +	u8 lane_cnt = tpg->res->lane_cnt;
> +	u8 i;
> +	u8 dt_cnt = 0;
> +	u32 val;
> +
> +	/* Loop through all enabled VCs and configure stream for each */
> +	for (i = 0; i < tpg->res->vc_cnt; i++) {
> +		input_format = &tpg->fmt[MSM_TPG_PAD_SRC + i];
> +		format = tpg_get_fmt_entry(tpg->res->formats->formats,
> +					   tpg->res->formats->nformats,
> +					   input_format->code);
> +
> +		val = (input_format->height & 0xffff) << TPG_VC_m_DT_n_CFG_0_FRAME_HEIGHT;
> +		val |= (input_format->width & 0xffff) << TPG_VC_m_DT_n_CFG_0_FRAME_WIDTH;
> +		writel_relaxed(val, tpg->base + TPG_VC_m_DT_n_CFG_0(i, dt_cnt));
> +
> +		val = format->data_type << TPG_VC_m_DT_n_CFG_1_DATA_TYPE;
> +		writel_relaxed(val, tpg->base + TPG_VC_m_DT_n_CFG_1(i, dt_cnt));
> +
> +		val = (tg->mode - 1) << TPG_VC_m_DT_n_CFG_2_PAYLOAD_MODE;
> +		val |= 0xBE << TPG_VC_m_DT_n_CFG_2_USER_SPECIFIED_PAYLOAD;
> +		val |= format->encode_format << TPG_VC_m_DT_n_CFG_2_ENCODE_FORMAT;
> +		writel_relaxed(val, tpg->base + TPG_VC_m_DT_n_CFG_2(i, dt_cnt));
> +
> +		writel_relaxed(0xA00, tpg->base + TPG_VC_n_COLOR_BARS_CFG(i));
> +
> +		writel_relaxed(0x4701, tpg->base + TPG_VC_n_HBI_CFG(i));
> +		writel_relaxed(0x438, tpg->base + TPG_VC_n_VBI_CFG(i));

Please provide context for the magic numbers> +
> +		writel_relaxed(0x12345678, tpg->base + TPG_VC_n_LSFR_SEED(i));
> +
> +		/* configure one DT, infinite frames */
> +		val = i << TPG_VC_n_CFG0_VC_NUM;
> +		val |= 0 << TPG_VC_n_CFG0_NUM_FRAMES;
> +		writel_relaxed(val, tpg->base + TPG_VC_n_CFG0(i));
> +	}
> +
> +	writel_relaxed(1, tpg->base + TPG_TOP_IRQ_MASK);
> +
> +	val = 1 << TPG_CTRL_TEST_EN;
> +	val |= 0 << TPG_CTRL_PHY_SEL;
> +	val |= (lane_cnt - 1) << TPG_CTRL_NUM_ACTIVE_LANES;
> +	val |= 0 << TPG_CTRL_VC_DT_PATTERN_ID;
> +	val |= (tpg->res->vc_cnt - 1) << TPG_CTRL_NUM_ACTIVE_VC;
> +	writel_relaxed(val, tpg->base + TPG_CTRL);

You want the last writel here (and in _off()) to *not* be relaxed,
so that all the prior accesses would have been sent off to the hw

[...]

> +static u32 tpg_hw_version(struct tpg_device *tpg)
> +{
> +	u32 hw_version;
> +	u32 hw_gen;
> +	u32 hw_rev;
> +	u32 hw_step;
> +
> +	hw_version = readl_relaxed(tpg->base + TPG_HW_VERSION);
> +	hw_gen = (hw_version >> HW_VERSION_GENERATION) & 0xF;
> +	hw_rev = (hw_version >> HW_VERSION_REVISION) & 0xFFF;
> +	hw_step = (hw_version >> HW_VERSION_STEPPING) & 0xFFFF;

FIELD_GET()

> +	dev_dbg(tpg->camss->dev, "tpg HW Version = %u.%u.%u\n",
> +		hw_gen, hw_rev, hw_step);

dev_dbg_once()

[...]

> +static int tpg_reset(struct tpg_device *tpg)
> +{
> +	writel_relaxed(0, tpg->base + TPG_CTRL);
> +	writel_relaxed(0, tpg->base + TPG_TOP_IRQ_MASK);
> +	writel_relaxed(1, tpg->base + TPG_TOP_IRQ_CLEAR);
> +	writel_relaxed(1, tpg->base + TPG_IRQ_CMD);
> +	writel_relaxed(1, tpg->base + TPG_CLEAR);

similar comment as before

Konrad