| Message ID | 20230228-topic-venus-v3-12-6092ae43b58f@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Venus QoL / maintainability fixes | expand |
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. > @@ -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; >
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; >> > > > > >
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; >> > > > > >
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; @@ -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 @@ -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;