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

[Bryan O'Donoghue]

On 17/07/2025 13:54, Konrad Dybcio wrote:
> On 7/17/25 5:20 AM, Wenmeng Liu wrote:
> > Add support for TPG found on SA8775P.
> >
> > Signed-off-by: Wenmeng Liu <quic_wenmliu@xxxxxxxxxxx>
> > ---
>
> [...]
>
> > +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

+1

the _relaxed() writes make me distinctly unrelaxed() and the magic 
numbers should be spelled out as bitfields with clear names. within reason.

For example 0xA5 is an obvious output pattern of alternating 1s and 0s.

---
bod