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: <6d16e2af-b3f6-93bd-4517-eca9128ba57e@quicinc.com>
Date:   Fri, 26 May 2023 13:15:09 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; 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>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Stanimir Varbanov <stanimir.k.varbanov@gmail.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:   Vikash Garodia <quic_vgarodia@quicinc.com>
In-Reply-To: <76f0d91e-eff0-3044-fd99-9371a9ce0cb3@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 5/26/2023 12:33 PM, 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;
> 
> so this change in not correct.
> V6 check can be replaced with VPU version(IRIS2_1) check.

This patch is causing boot failure for sc7180. Dropping the patch could boot the
target. Addressing the comments should fix the issue.

Thanks,
Vikash
>> @@ -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;
>>
> 
> 
> 
> 
>