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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7cb32560-b7af-4395-bb58-04dc4bbfa420@linaro.org>
Date: Thu, 17 Jul 2025 13:59:19 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
 Wenmeng Liu <quic_wenmliu@...cinc.com>, Robert Foss <rfoss@...nel.org>,
 Todor Tomov <todor.too@...il.com>, Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 3/3] media: qcom: camss: tpg: Add TPG support for
 SA8775P

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@...cinc.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

+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