Re: [REGRESSION][PATCH 2/3] venus: firmware: Correct non-pix start and end addresses

[Matthias Kaehlcke]

On Thu, Jan 26, 2023 at 07:19:13PM +0000, Matthias Kaehlcke wrote:
> Hi Stanimir,
> 
> On Wed, Oct 05, 2022 at 11:37:29AM +0300, Stanimir Varbanov wrote:
> > The default values for those registers are zero.
> > 
> > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>
> > ---
> >  drivers/media/platform/qcom/venus/firmware.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> > index 3851cedc3329..71e43611d1cf 100644
> > --- a/drivers/media/platform/qcom/venus/firmware.c
> > +++ b/drivers/media/platform/qcom/venus/firmware.c
> > @@ -38,8 +38,8 @@ static void venus_reset_cpu(struct venus_core *core)
> >  	writel(fw_size, wrapper_base + WRAPPER_FW_END_ADDR);
> >  	writel(0, wrapper_base + WRAPPER_CPA_START_ADDR);
> >  	writel(fw_size, wrapper_base + WRAPPER_CPA_END_ADDR);
> > -	writel(fw_size, wrapper_base + WRAPPER_NONPIX_START_ADDR);
> > -	writel(fw_size, wrapper_base + WRAPPER_NONPIX_END_ADDR);
> > +	writel(0, wrapper_base + WRAPPER_NONPIX_START_ADDR);
> > +	writel(0, wrapper_base + WRAPPER_NONPIX_END_ADDR);
> >  
> >  	if (IS_V6(core)) {
> >  		/* Bring XTSS out of reset */
> 
> I found that this commit prevents the AOSS from entering sleep mode during
> system suspend at least on sc7180 and sc7280. AOSS not entering sleep mode
> leads to a (apparently significant) increase in S3 power consumption, on
> trogdor and herobrine it prevents the system from staying suspended, because
> the embedded controller detect the condition and wakes the sytem up again.
> 
> Testing is slightly involved, since unfortunately this is not the only issue
> in v6.2-rcN that impacts AOSS sleep.
> 
> To reach AOSS sleep you also have to revert this commit:
> 
> 3a39049f88e4 soc: qcom: rpmhpd: Use highest corner until sync_state
> 
> And apply something like the diff below (or enable the bwmon driver).
> 
> On a trogdor device you will see something like this when AOSS doesn't
> enter sleep mode during system suspend:
> 
>   [   32.882869] EC detected sleep transition timeout. Total sleep transitions: 0
>   [   32.882886] WARNING: CPU: 7 PID: 5682 at drivers/platform/chrome/cros_ec.c:146 cros_ec_sleep_event+0x100/0x10c
>   [   32.900393] Modules linked in: uinput veth uvcvideo videobuf2_vmalloc venus_enc venus_dec videobuf2_dma_contig videobuf2_memops onboard_usb_hub cros_ec_typec typec hci_uart btqca xt_MASQUERADE venus_core v4l2_mem2mem videobuf2_v4l2 videobuf2_common qcom_q6v5_mss qcom_pil_v
>   [   32.940015] CPU: 7 PID: 5682 Comm: cat Tainted: G        W          6.1.0-rc2+ #295 d14276115b3f6b03fc99220174e5d7724847cbd6
>   [   32.951525] Hardware name: Google Villager (rev1+) with LTE (DT)
>   [   32.957695] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>   [   32.964848] pc : cros_ec_sleep_event+0x100/0x10c
>   [   32.969596] lr : cros_ec_sleep_event+0x100/0x10c
> 
> I'm also happy to help with testing if you have a candidate fix.

This change introduced an important regression at least for sc7180 and sc7280,
and probably other QC SoCs, I wonder if it should be reverted unless there is
an obvious better fix.

> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 0adf13399e64..c1f6952764c5 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -3488,7 +3488,7 @@ IPCC_MPROC_SIGNAL_GLINK_QMP
>                 };
> 
>                 pmu@9091000 {
> 		-                       compatible = "qcom,sc7280-llcc-bwmon";
> 		+                       // compatible = "qcom,sc7280-llcc-bwmon";
> 		                        reg = <0 0x9091000 0 0x1000>;
> 
>                         interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>;
> 			@@ -3528,7 +3528,7 @@ opp-7 {
> 			                };
> 
>                 pmu@90b6400 {
> 		-                       compatible = "qcom,sc7280-cpu-bwmon", "qcom,msm8998-bwmon";
> 		+                       // compatible = "qcom,sc7280-cpu-bwmon", "qcom,msm8998-bwmon";
> 		                        reg = <0 0x090b6400 0 0x600>;
> 
>                         interrupts = <GIC_SPI 581 IRQ_TYPE_LEVEL_HIGH>;