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