Re: [PATCH 1/2] drm/imagination: Improve firmware power off for layout_mars config

[Matt Coster <Matt.Coster@imgtec.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7761 bytes --]

Hi Brajesh,

Just a couple minor comments but this otherwise looks good to me.

On 05/03/2026 11:06, Brajesh Gupta wrote:
> In layout_mars HW config, Firmware MCU moved from SideKick to new Mars
> domain so Firmware takes care of powering down Sidekick/Jones and SLC.

I don't know which is correct, but I know we should probably be
consistent with the capitlisation of SideKick/Sidekick.

> Skip checks for those from kernel and check idle bits for Firmware MCU
> and system arbiter excluding SOCIF.
> 
> Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
> ---
>  drivers/gpu/drm/imagination/pvr_fw_startstop.c | 86 +++++++++++++++++---------
>  1 file changed, 58 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_fw_startstop.c b/drivers/gpu/drm/imagination/pvr_fw_startstop.c
> index dcbb9903e791..ce089f51f06a 100644
> --- a/drivers/gpu/drm/imagination/pvr_fw_startstop.c
> +++ b/drivers/gpu/drm/imagination/pvr_fw_startstop.c
> @@ -208,19 +208,31 @@ pvr_fw_stop(struct pvr_device *pvr_dev)
>                                        ~(ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN |
>                                          ROGUE_CR_SIDEKICK_IDLE_SOCIF_EN |
>                                          ROGUE_CR_SIDEKICK_IDLE_HOSTIF_EN);
> -       bool skip_garten_idle = false;
> +       u64 layout_mars_value = 0;
> +       bool layout_mars = false;
>         u32 reg_value;
>         int err;
> 
> +       if (PVR_FEATURE_VALUE(pvr_dev, layout_mars, &layout_mars_value) == 0)
> +               layout_mars = layout_mars_value > 0;
> +
>         /*
> -        * Wait for Sidekick/Jones to signal IDLE except for the Garten Wrapper.
> -        * For cores with the LAYOUT_MARS feature, SIDEKICK would have been
> +        * For cores with the LAYOUT_MARS feature, SIDEKICK and SLC would have been
>          * powered down by the FW.
>          */
> -       err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE, sidekick_idle_mask,
> -                               sidekick_idle_mask, POLL_TIMEOUT_USEC);
> -       if (err)
> -               return err;
> +       if (!layout_mars) {
> +               /* Wait for Sidekick/Jones to signal IDLE except for the Garten Wrapper. */
> +               err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE, sidekick_idle_mask,
> +                                       sidekick_idle_mask, POLL_TIMEOUT_USEC);
> +               if (err)
> +                       return err;
> +
> +               /* Wait for SLC to signal IDLE. */
> +               err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE, ROGUE_CR_SLC_IDLE_MASKFULL,
> +                                        ROGUE_CR_SLC_IDLE_MASKFULL, POLL_TIMEOUT_USEC);
> +               if (err)
> +                       return err;
> +       }
> 
>         /* Unset MTS DM association with threads. */
>         pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_INTCTX_THREAD0_DM_ASSOC,
> @@ -229,6 +241,7 @@ pvr_fw_stop(struct pvr_device *pvr_dev)
>         pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_BGCTX_THREAD0_DM_ASSOC,
>                        ROGUE_CR_MTS_BGCTX_THREAD0_DM_ASSOC_MASKFULL &
>                        ROGUE_CR_MTS_BGCTX_THREAD0_DM_ASSOC_DM_ASSOC_CLRMSK);
> +

I think this might be a remnant from the second patch, can you drop it?

>         pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC,
>                        ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC_MASKFULL &
>                        ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC_DM_ASSOC_CLRMSK);
> @@ -270,25 +283,23 @@ pvr_fw_stop(struct pvr_device *pvr_dev)
>                 return err;
> 
>         /*
> -        * Wait for SLC to signal IDLE.
> -        * For cores with the LAYOUT_MARS feature, SLC would have been powered
> -        * down by the FW.
> +        * For cores with the LAYOUT_MARS feature, SIDEKICK and SLC would have been
> +        * powered down by the FW.
>          */
> -       err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE,
> -                               ROGUE_CR_SLC_IDLE_MASKFULL,
> -                               ROGUE_CR_SLC_IDLE_MASKFULL, POLL_TIMEOUT_USEC);
> -       if (err)
> -               return err;
> +       if (!layout_mars) {
> +               /* Wait for SLC to signal IDLE. */
> +               err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE,
> +                                       ROGUE_CR_SLC_IDLE_MASKFULL,
> +                                       ROGUE_CR_SLC_IDLE_MASKFULL, POLL_TIMEOUT_USEC);
> +               if (err)
> +                       return err;
> 
> -       /*
> -        * Wait for Sidekick/Jones to signal IDLE except for the Garten Wrapper.
> -        * For cores with the LAYOUT_MARS feature, SIDEKICK would have been powered
> -        * down by the FW.
> -        */
> -       err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE, sidekick_idle_mask,
> -                               sidekick_idle_mask, POLL_TIMEOUT_USEC);
> -       if (err)
> -               return err;
> +               /* Wait for Sidekick/Jones to signal IDLE except for the Garten Wrapper. */
> +               err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE, sidekick_idle_mask,
> +                                       sidekick_idle_mask, POLL_TIMEOUT_USEC);
> +               if (err)
> +                       return err;
> +       }
> 
>         if (pvr_dev->fw_dev.processor_type == PVR_FW_PROCESSOR_TYPE_META) {
>                 err = pvr_meta_cr_read32(pvr_dev, META_CR_TxVECINT_BHALT, &reg_value);
> @@ -300,11 +311,30 @@ pvr_fw_stop(struct pvr_device *pvr_dev)
>                  * Wrapper if there is no debugger attached (TxVECINT_BHALT =
>                  * 0x0).
>                  */
> -               if (reg_value)
> -                       skip_garten_idle = true;
> -       }
> +               if (!reg_value) {
> +                       err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE,
> +                                               ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN,
> +                                               ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN,
> +                                               POLL_TIMEOUT_USEC);
> +                       if (err)
> +                               return err;

This duplicates the polling of CR_SIDEKICK_IDLE. Can you move it down
outside this laddered if/elseif block, and make it conditional on a new
variable poll_sidekick_idle_at_end or similar?

Cheers,
Matt

> +               }
> +       } else if (layout_mars) {
> +               /*
> +                * As FW core has been moved from SIDEKICK to the new MARS domain, checking
> +                * idle bits for CPU & System Arbiter excluding SOCIF which will never be
> +                * idle if Host polling on this register
> +                */
> +               err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_MARS_IDLE,
> +                                       ROGUE_CR_MARS_IDLE_CPU_EN |
> +                                       ROGUE_CR_MARS_IDLE_MH_SYSARB0_EN,
> +                                       ROGUE_CR_MARS_IDLE_CPU_EN |
> +                                       ROGUE_CR_MARS_IDLE_MH_SYSARB0_EN,
> +                                       POLL_TIMEOUT_USEC);
> 
> -       if (!skip_garten_idle) {
> +               if (err)
> +                       return err;
> +       } else {
>                 err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE,
>                                         ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN,
>                                         ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN,
> 
> --
> 2.43.0
> 


-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]