Re: [PATCH v3 12/17] media: venus: firmware: Correct IS_V6() checks

[Konrad Dybcio]

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@xxxxxxxxxx>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
>> ---
>>  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;
>>
> 
> 
> 
> 
>