Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ef61906d-98f2-2806-9ad7-2a99f7928bb1@nexus-software.ie>
Date:   Fri, 4 Aug 2023 21:50:55 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH 3/6] media: venus: hfi_venus: Support only updating
 certain bits with presets
Content-Language: en-US
To:     Konrad Dybcio <konrad.dybcio@linaro.org>,
        Stanimir Varbanov <stanimir.k.varbanov@gmail.com>,
        Vikash Garodia <quic_vgarodia@quicinc.com>,
        Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>
Cc:     Marijn Suijten <marijn.suijten@somainline.org>,
        Konrad Dybcio <konradybcio@kernel.org>,
        linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230731-topic-8280_venus-v1-0-8c8bbe1983a5@linaro.org>
 <20230731-topic-8280_venus-v1-3-8c8bbe1983a5@linaro.org>
From:   Bryan O'Donoghue <pure.logic@nexus-software.ie>
In-Reply-To: <20230731-topic-8280_venus-v1-3-8c8bbe1983a5@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 04/08/2023 21:09, Konrad Dybcio wrote:
> On some platforms (like SM8350) we're expected to only touch certain bits
> (such as 0 and 4 corresponding to mask 0x11). Add support for doing so.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/media/platform/qcom/venus/core.h      |  1 +
>   drivers/media/platform/qcom/venus/hfi_venus.c | 15 ++++++++++++---
>   2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index d996abd339e1..2999c24392f5 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -38,6 +38,7 @@ struct freq_tbl {
>   struct reg_val {
>   	u32 reg;
>   	u32 value;
> +	u32 mask;
>   };
>   
>   struct bw_tbl {
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 19fc6575a489..d4bad66f7293 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -349,10 +349,19 @@ static void venus_set_registers(struct venus_hfi_device *hdev)
>   	const struct venus_resources *res = hdev->core->res;
>   	const struct reg_val *tbl = res->reg_tbl;
>   	unsigned int count = res->reg_tbl_size;
> -	unsigned int i;
> +	unsigned int i, val;

u32 val;

> +
> +	for (i = 0; i < count; i++) {
> +		val = tbl[i].value;

struct reg_val looks like this

struct reg_val {
         u32 reg;
         u32 value;
};

val should be declared a u32

> -	for (i = 0; i < count; i++)
> -		writel(tbl[i].value, hdev->core->base + tbl[i].reg);
> +		/* In some cases, we only want to update certain bits */

I'll trust this is a legitimate and true statement.

> +		if (tbl[i].mask) {
> +			val = readl(hdev->core->base + tbl[i].reg);
> +			val = (val & ~tbl[i].mask) | (tbl[i].value & tbl[i].mask);

feels like something regmap_update_bits() already does though, I prefer 
this because there's less code in it.

> +		}
> +
> +		writel(val, hdev->core->base + tbl[i].reg);
> +	}

With the val declaration fix

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>