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

[Dikshita Agarwal]

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;

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;
>