Re: [PATCH 5/8] media: iris: Move vpu register defines to common header file

Dmitry Baryshkov
2025-10-16T18:58:45Z

On Thu, 16 Oct 2025 at 21:37, Vikash Garodia
<vikash.garodia@oss.qualcomm.com> wrote:
>
>
> On 10/16/2025 7:17 PM, Dmitry Baryshkov wrote:
> >> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_register_defines.h b/drivers/media/platform/qcom/iris/iris_vpu_register_defines.h
> >> index fe8a39e5e5a3fc68dc3a706ffdba07a5558163cf..6474f561c8dc29d1975bb44792595d86f16b6cff 100644
> >> --- a/drivers/media/platform/qcom/iris/iris_vpu_register_defines.h
> >> +++ b/drivers/media/platform/qcom/iris/iris_vpu_register_defines.h
> >> @@ -9,9 +9,38 @@
> >> #define VCODEC_BASE_OFFS 0x00000000
> >> #define CPU_BASE_OFFS 0x000A0000
> >> #define WRAPPER_BASE_OFFS 0x000B0000
> >> +#define AON_BASE_OFFS 0x000E0000
> >> +#define WRAPPER_TZ_BASE_OFFS 0x000C0000
> >> +#define AON_MVP_NOC_RESET 0x0001F000
> >>
> >> #define CPU_CS_BASE_OFFS (CPU_BASE_OFFS)
> >>
> >> #define WRAPPER_CORE_POWER_STATUS (WRAPPER_BASE_OFFS + 0x80)
> >> +#define WRAPPER_CORE_CLOCK_CONFIG (WRAPPER_BASE_OFFS + 0x88)
> >> +#define AON_WRAPPER_MVP_NOC_LPI_CONTROL (AON_BASE_OFFS)
> >> +#define WRAPPER_DEBUG_BRIDGE_LPI_CONTROL (WRAPPER_BASE_OFFS + 0x54)
> >> +#define WRAPPER_DEBUG_BRIDGE_LPI_STATUS (WRAPPER_BASE_OFFS + 0x58)
> >> +#define WRAPPER_IRIS_CPU_NOC_LPI_CONTROL (WRAPPER_BASE_OFFS + 0x5C)
> >> +#define WRAPPER_IRIS_CPU_NOC_LPI_STATUS (WRAPPER_BASE_OFFS + 0x60)
> >> +#define WRAPPER_TZ_CTL_AXI_CLOCK_CONFIG (WRAPPER_TZ_BASE_OFFS + 0x14)
> >> +#define CPU_CS_AHB_BRIDGE_SYNC_RESET (CPU_CS_BASE_OFFS + 0x160)
> >> +#define CPU_CS_X2RPMH (CPU_CS_BASE_OFFS + 0x168)
> >> +#define AON_WRAPPER_MVP_NOC_RESET_REQ (AON_MVP_NOC_RESET + 0x000)
> >> +#define AON_WRAPPER_MVP_NOC_RESET_ACK (AON_MVP_NOC_RESET + 0x004)
> >> +#define VCODEC_SS_IDLE_STATUSN (VCODEC_BASE_OFFS + 0x70)
> >> +#define AON_WRAPPER_MVP_NOC_LPI_STATUS (AON_BASE_OFFS + 0x4)
> >> +#define WRAPPER_TZ_QNS4PDXFIFO_RESET (WRAPPER_TZ_BASE_OFFS + 0x18)
> > Registers here got totally unsorted (they were in the original source
> > file). Seeing this makes me sad.
> >
>
> Sure, i will be improving this part in v2.
>
> >> +
> >> +#define CORE_BRIDGE_SW_RESET BIT(0)
> >> +#define CORE_BRIDGE_HW_RESET_DISABLE BIT(1)
> >> +#define MSK_SIGNAL_FROM_TENSILICA BIT(0)
> >> +#define MSK_CORE_POWER_ON BIT(1)
> >> +#define CTL_AXI_CLK_HALT BIT(0)
> >> +#define CTL_CLK_HALT BIT(1)
> >> +#define REQ_POWER_DOWN_PREP BIT(0)
> >> +#define RESET_HIGH BIT(0)
> >> +#define NOC_LPI_STATUS_DONE BIT(0) /* Indicates the NOC handshake is complete */
> >> +#define NOC_LPI_STATUS_DENY BIT(1) /* Indicates the NOC handshake is denied */
> >> +#define NOC_LPI_STATUS_ACTIVE BIT(2) /* Indicates the NOC is active */
> > Ugh. This mixed all the bits, loosing connection between the register
> > and the corresponding bits. I'm going to pick up this patch into the
> > sc7280 series and I will improve it there, keeping the link between
> > registers and bit fields.
> >
>
> Ok, not updating this part in the next revision of my series. Do you mean
> something like
>
> #define CORE_BRIDGE_SW_RESET_BIT0 BIT(0)
> #define CORE_BRIDGE_HW_RESET_DISABLE_BIT1 BIT(1)

No, just keeping those BIT() definition next to the corresponding
register #define.

--
With best wishes
Dmitry