linux-kernel - Re: [PATCH v4 4/6] media: qcom: camss: Add sc8280xp resource details

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <3e0958a9-4d1e-4d1b-a914-5da154caa11f@linaro.org>
Date:   Thu, 9 Nov 2023 14:44:44 +0100
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        hverkuil-cisco@...all.nl, laurent.pinchart@...asonboard.com,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Robert Foss <rfoss@...nel.org>,
        Todor Tomov <todor.too@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, vincent.knecht@...loo.org,
        matti.lehtimaki@...il.com, quic_grosikop@...cinc.com
Cc:     linux-arm-msm@...r.kernel.org, linux-media@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 4/6] media: qcom: camss: Add sc8280xp resource details



On 11/9/23 12:30, Bryan O'Donoghue wrote:
> This commit describes the hardware layout for the sc8280xp for the
> following hardware blocks:
> 
> - 4 x VFE, 4 RDI per VFE
> - 4 x VFE Lite, 4 RDI per VFE
> - 4 x CSID
> - 4 x CSID Lite
> - 4 x CSI PHY
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
> ---
[...]

> +static const struct camss_subdev_resources csid_res_sc8280xp[] = {
> +	/* CSID0 */
> +	{
> +		.regulators = { "vdda-phy", "vdda-pll" },
> +		.clock = { "vfe0_csid", "vfe0_cphy_rx", "vfe0", "vfe0_axi" },
> +		.clock_rate = { { 400000000, 400000000, 480000000, 600000000, 600000000, 600000000 },
(why is it 400, 400, 480, 600, 600, 600 and not 400, 480, 600?)

> +				{ 0 },
> +				{ 0 },
> +				{ 0 } },
There's a funny bug..

camss-csiphy.c and camss-vfe.c (sounds like room for commonization):

while (res->clock_rate[i][clock->nfreqs])
	clock->nfreqs++;

this works fine in this case, because the last frequency is followed
by a zero, so overflowing the 2nd dimension of the array into the last+1
member (meaning the first member of the following entry in the 1st dimension)
stops this loop

however

[...]

> +static const struct camss_subdev_resources vfe_res_sc8280xp[] = {
> +	/* IFE0 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "gcc_axi_sf", "cpas_ahb", "camnoc_axi", "vfe0", "vfe0_axi" },
> +		.clock_rate = { { 0 },
> +				{ 0 },
> +				{ 19200000, 80000000, 80000000, 80000000, 80000000},
> +				{ 19200000, 150000000, 266666667, 320000000, 400000000, 480000000 },
> +				{ 400000000, 558000000, 637000000, 760000000 },
> +				{ 0 }, },
Not the case here!

I'd suggest moving this to something like:

struct res_clk_data {
	const char * const names;
	const u64 * const rates; (or unsigned long / unsigned long long / uint?
				  there was some capping for arm32)
	const u8 num_clks;
}

OR even better

separate out clocks that just need to be on/off ("intf/interface clocks" sounds
like a good name for these) from ones that require scaling, use clk_bulk apis
for the former and OPP for the latter to make sure the correct performance state
is requested on the RPMhPDs

Konrad