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: <aeaef3fd-3347-1843-4038-fb6b6d52d266@linaro.org>
Date:   Sat, 27 May 2023 18:48:29 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v3 12/17] media: venus: firmware: Correct IS_V6() checks
Content-Language: en-US
To:     Dikshita Agarwal <quic_dikshita@quicinc.com>,
        Stanimir Varbanov <stanimir.k.varbanov@gmail.com>,
        Vikash Garodia <quic_vgarodia@quicinc.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Dikshita Agarwal <dikshita@qti.qualcomm.com>,
        Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
        Mansur Alisha Shaik <mansur@codeaurora.org>,
        Jonathan Marek <jonathan@marek.ca>,
        Hans Verkuil <hans.verkuil@cisco.com>
Cc:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        Stanimir Varbanov <stanimir.varbanov@linaro.org>,
        linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Marijn Suijten <marijn.suijten@somainline.org>
References: <20230228-topic-venus-v3-0-6092ae43b58f@linaro.org>
 <20230228-topic-venus-v3-12-6092ae43b58f@linaro.org>
 <76f0d91e-eff0-3044-fd99-9371a9ce0cb3@quicinc.com>
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <76f0d91e-eff0-3044-fd99-9371a9ce0cb3@quicinc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 26.05.2023 09:03, Dikshita Agarwal wrote:
> 
> 
> On 5/18/2023 2:44 AM, Konrad Dybcio wrote:
>> Most of these checks should have checked for TZ presence (or well,
>> absence), as we shouldn't really be doing things that the black box
>> does for us on non-CrOS platforms.
>>
>> The IS_V6() check in venus_shutdown_no_tz() should have checked
>> whether the core version is IRIS2_1 (so, SC7280). Correct that.
>>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/firmware.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
>> index 572b649c56f3..ceb917f2e0d4 100644
>> --- a/drivers/media/platform/qcom/venus/firmware.c
>> +++ b/drivers/media/platform/qcom/venus/firmware.c
>> @@ -29,7 +29,11 @@ static void venus_reset_cpu(struct venus_core *core)
>>  	u32 fw_size = core->fw.mapped_mem_size;
>>  	void __iomem *wrapper_base;
>>  
>> -	if (IS_V6(core))
>> +	/*
>> +	 * When there's no Qualcomm TZ (like on Chromebooks), the OS is
>> +	 * responsible for bringing up the hardware instead.
>> +	 */
>> +	if (!core->use_tz)
>>  		wrapper_base = core->wrapper_tz_base;
>>  	else
>>  		wrapper_base = core->wrapper_base;
> this is invoked only for platforms not using TZ.
> The version checks are kept to differentiate between different TZ base offset.
> wrapper base offset for V6 (IRIS2_1) is calculated as
> 	wrapper_base = core->wrapper_tz_base
> while for others (non V6) wrapper base is calculated as
> 	wrapper_base = core->wrapper_base;
OK I see, this patch is bad indeed..

The IS_V6 should be IRIS2_1 as you mentioned, I'll do that instead.

We should however think about whether assuming every SC7180/SC7280
doesn't use TZ (reminder: not all SC7[12]80 are Chromebooks, there are
quite a lot of WoA laptops with WP firmware that has PAS), but that's
a problem that we may discuss separately.

Konrad
> 
> so this change in not correct.
> V6 check can be replaced with VPU version(IRIS2_1) check.
> 
>> @@ -41,7 +45,7 @@ static void venus_reset_cpu(struct venus_core *core)
>>  	writel(fw_size, wrapper_base + WRAPPER_NONPIX_START_ADDR);
>>  	writel(fw_size, wrapper_base + WRAPPER_NONPIX_END_ADDR);
>>  
>> -	if (IS_V6(core)) {
>> +	if (!core->use_tz) {
>>  		/* Bring XTSS out of reset */
>>  		writel(0, wrapper_base + WRAPPER_TZ_XTSS_SW_RESET);
>>  	} else {
>> @@ -67,7 +71,7 @@ int venus_set_hw_state(struct venus_core *core, bool resume)
>>  	if (resume) {
>>  		venus_reset_cpu(core);
>>  	} else {
>> -		if (IS_V6(core))
>> +		if (!core->use_tz)
>>  			writel(WRAPPER_XTSS_SW_RESET_BIT,
>>  			       core->wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
>>  		else
> 
> this part of the code will only be executed for non TZ platform.
> for TZ based platforms it will return few instructions earlier in the same API.
> Again, version checks are kept to differentiate between different TZ base
> offset. V6 check can be replaced with VPU version(IRIS2_1) check.
> 
> Thanks,
> Dikshita
>> @@ -179,7 +183,7 @@ static int venus_shutdown_no_tz(struct venus_core *core)
>>  	void __iomem *wrapper_base = core->wrapper_base;
>>  	void __iomem *wrapper_tz_base = core->wrapper_tz_base;
>>  
>> -	if (IS_V6(core)) {
>> +	if (IS_IRIS2_1(core)) {
>>  		/* Assert the reset to XTSS */
>>  		reg = readl(wrapper_tz_base + WRAPPER_TZ_XTSS_SW_RESET);
>>  		reg |= WRAPPER_XTSS_SW_RESET_BIT;
>>
> 
> 
> 
> 
>