Re: [PATCH 3/3] soc: mediatek: pwrap: add MT6589 PWARP and MT6320 PMIC support

On 7/16/26 19:17, Akari Tsuyukusa wrote:
> Add support for MediaTek MT6589 PWRAP and MT6320 PMIC which are used
> together.
> 
> Since the PWRAP Bridge hardware on MT6589 is identical to the one on
> MT8135, refactor the existing register definitions:
> - Rename the MT8135-specific "PWRAP_MT8135_BRIDGE_" prefix to a generic
>    "PWRAP_BRIDGE_" prefix to reflect that they are shared across SoCs.
> - Add the full set of PWRAP Bridge registers to match the hardware
>    specification, rather than defining only a subset of them.
> 
> Signed-off-by: Akari Tsuyukusa <[email protected]>
> ---
>   drivers/soc/mediatek/mtk-pmic-wrap.c | 292 +++++++++++++++++++++------
>   1 file changed, 235 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 0bcd85826375..3a5b007d60f5 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -17,15 +17,31 @@
>   #define PWRAP_POLL_DELAY_US	10
>   #define PWRAP_POLL_TIMEOUT_US	10000
>   
> -#define PWRAP_MT8135_BRIDGE_IORD_ARB_EN		0x4
> -#define PWRAP_MT8135_BRIDGE_WACS3_EN		0x10
> -#define PWRAP_MT8135_BRIDGE_INIT_DONE3		0x14
> -#define PWRAP_MT8135_BRIDGE_WACS4_EN		0x24
> -#define PWRAP_MT8135_BRIDGE_INIT_DONE4		0x28
> -#define PWRAP_MT8135_BRIDGE_INT_EN		0x38
> -#define PWRAP_MT8135_BRIDGE_TIMER_EN		0x48
> -#define PWRAP_MT8135_BRIDGE_WDT_UNIT		0x50
> -#define PWRAP_MT8135_BRIDGE_WDT_SRC_EN		0x54

Yeah I understand why you're renaming those, but choose:

a. You avoid renaming and add the rest as PWRAP_MT8135_XXXX; or
b. You make two commits, one for the rename (and explaining why) and one
    for introducing MT6589 support.

Your choice.

> +/* MT6589 and MT8135 specific PWARP Bridge registers */
> +#define PWRAP_BRIDGE_IARB_INIT		0x0
> +#define PWRAP_BRIDGE_IORD_ARB_EN	0x4
> +#define PWRAP_BRIDGE_IARB_STA0		0x8
> +#define PWRAP_BRIDGE_IARB_STA1		0xc
> +#define PWRAP_BRIDGE_WACS3_EN		0x10
> +#define PWRAP_BRIDGE_INIT_DONE3		0x14
> +#define PWRAP_BRIDGE_WACS3_CMD		0x18
> +#define PWRAP_BRIDGE_WACS3_RDATA	0x1c
> +#define PWRAP_BRIDGE_WACS3_VLDCLR	0x20
> +#define PWRAP_BRIDGE_WACS4_EN		0x24
> +#define PWRAP_BRIDGE_INIT_DONE4		0x28
> +#define PWRAP_BRIDGE_WACS4_CMD		0x2c
> +#define PWRAP_BRIDGE_WACS4_RDATA	0x30
> +#define PWRAP_BRIDGE_WACS4_VLDCLR	0x34
> +#define PWRAP_BRIDGE_INT_EN		0x38
> +#define PWRAP_BRIDGE_INT_FLG_RAW	0x3c
> +#define PWRAP_BRIDGE_INT_FLG		0x40
> +#define PWRAP_BRIDGE_INT_CLR		0x44
> +#define PWRAP_BRIDGE_TIMER_EN		0x48
> +#define PWRAP_BRIDGE_TIMER_STA		0x4c
> +#define PWRAP_BRIDGE_WDT_UNIT		0x50
> +#define PWRAP_BRIDGE_WDT_SRC_EN		0x54
> +#define PWRAP_BRIDGE_WDT_FLG		0x58
> +#define PWRAP_BRIDGE_DEBUG_INT_SEL	0x5c
>   
>   /* macro for wrapper status */
>   #define PWRAP_GET_WACS_RDATA(x)		(((x) >> 0) & 0x0000ffff)
> @@ -100,6 +116,24 @@ enum dew_regs {
>   	PWRAP_DEW_CIPHER_MODE,
>   	PWRAP_DEW_CIPHER_SWRST,
>   
> +	/* MT6320 only regs */
> +	PWRAP_DEW_CIPHER_IV0,
> +	PWRAP_DEW_CIPHER_IV1,
> +	PWRAP_DEW_CIPHER_IV2,
> +	PWRAP_DEW_CIPHER_IV3,
> +	PWRAP_DEW_CIPHER_IV4,
> +	PWRAP_DEW_CIPHER_IV5,
> +
> +	/* MT6320 and MT6397 only regs */
> +	PWRAP_DEW_EVENT_OUT_EN,
> +	PWRAP_DEW_EVENT_SRC_EN,
> +	PWRAP_DEW_EVENT_SRC,
> +	PWRAP_DEW_EVENT_FLAG,
> +	PWRAP_DEW_MON_FLAG_SEL,
> +	PWRAP_DEW_EVENT_TEST,
> +	PWRAP_DEW_CIPHER_LOAD,
> +	PWRAP_DEW_CIPHER_START,
> +
>   	/* MT6323 only regs */
>   	PWRAP_DEW_CIPHER_EN,
>   	PWRAP_DEW_RDDMY_NO,
> @@ -142,16 +176,34 @@ enum dew_regs {
>   	PWRAP_DEW_RG_WDATA_MASK,
>   	PWRAP_DEW_RG_SPI_RECORD_CLR,
>   	PWRAP_DEW_RG_CMD_ALERT_CLR,
> +};
>   
> -	/* MT6397 only regs */
> -	PWRAP_DEW_EVENT_OUT_EN,
> -	PWRAP_DEW_EVENT_SRC_EN,
> -	PWRAP_DEW_EVENT_SRC,
> -	PWRAP_DEW_EVENT_FLAG,
> -	PWRAP_DEW_MON_FLAG_SEL,
> -	PWRAP_DEW_EVENT_TEST,
> -	PWRAP_DEW_CIPHER_LOAD,
> -	PWRAP_DEW_CIPHER_START,
> +static const u32 mt6320_regs[] = {
> +	[PWRAP_DEW_EVENT_OUT_EN] =	0x0000,
> +	[PWRAP_DEW_DIO_EN] =		0x0002,
> +	[PWRAP_DEW_EVENT_SRC_EN] =	0x0004,
> +	[PWRAP_DEW_EVENT_SRC] =		0x0006,
> +	[PWRAP_DEW_EVENT_FLAG] =	0x0008,
> +	[PWRAP_DEW_READ_TEST] =		0x000a,
> +	[PWRAP_DEW_WRITE_TEST] =	0x000c,
> +	[PWRAP_DEW_CRC_EN] =		0x000e,
> +	[PWRAP_DEW_CRC_VAL] =		0x0010,
> +	[PWRAP_DEW_MON_GRP_SEL] =	0x0012,
> +	[PWRAP_DEW_MON_FLAG_SEL] =	0x0014,
> +	[PWRAP_DEW_EVENT_TEST] =	0x0016,
> +	[PWRAP_DEW_CIPHER_KEY_SEL] =	0x0018,
> +	[PWRAP_DEW_CIPHER_IV_SEL] =	0x001a,
> +	[PWRAP_DEW_CIPHER_LOAD] =	0x001c,
> +	[PWRAP_DEW_CIPHER_START] =	0x001e,
> +	[PWRAP_DEW_CIPHER_RDY] =	0x0020,
> +	[PWRAP_DEW_CIPHER_MODE] =	0x0022,
> +	[PWRAP_DEW_CIPHER_SWRST] =	0x0024,
> +	[PWRAP_DEW_CIPHER_IV0] =	0x0026,
> +	[PWRAP_DEW_CIPHER_IV1] =	0x0028,
> +	[PWRAP_DEW_CIPHER_IV2] =	0x002a,
> +	[PWRAP_DEW_CIPHER_IV3] =	0x002c,
> +	[PWRAP_DEW_CIPHER_IV4] =	0x002e,
> +	[PWRAP_DEW_CIPHER_IV5] =	0x0030,
>   };
>   
>   static const u32 mt6323_regs[] = {
> @@ -386,6 +438,22 @@ enum pwrap_regs {
>   	PWRAP_EINT_STA1_ADR,
>   	PWRAP_SWINF_2_WDATA_31_0,
>   	PWRAP_SWINF_2_RDATA_31_0,
> +	PWRAP_DVFS_ADR0,
> +	PWRAP_DVFS_WDATA0,
> +	PWRAP_DVFS_ADR1,
> +	PWRAP_DVFS_WDATA1,
> +	PWRAP_DVFS_ADR2,
> +	PWRAP_DVFS_WDATA2,
> +	PWRAP_DVFS_ADR3,
> +	PWRAP_DVFS_WDATA3,
> +	PWRAP_DVFS_ADR4,
> +	PWRAP_DVFS_WDATA4,
> +	PWRAP_DVFS_ADR5,
> +	PWRAP_DVFS_WDATA5,
> +	PWRAP_DVFS_ADR6,
> +	PWRAP_DVFS_WDATA6,
> +	PWRAP_DVFS_ADR7,
> +	PWRAP_DVFS_WDATA7,
>   
>   	/* MT2701 only regs */
>   	PWRAP_ADC_CMD_ADDR,
> @@ -394,6 +462,24 @@ enum pwrap_regs {
>   	PWRAP_ADC_RDATA_ADDR1,
>   	PWRAP_ADC_RDATA_ADDR2,
>   
> +	/* MT6589 only regs */
> +	PWRAP_CIPHER_IV0,
> +	PWRAP_CIPHER_IV1,
> +	PWRAP_CIPHER_IV2,
> +
> +	/* MT6589 and MT8135 only regs */
> +	PWRAP_CSHEXT,
> +	PWRAP_EVENT_IN_EN,
> +	PWRAP_EVENT_DST_EN,
> +	PWRAP_RRARB_INIT,
> +	PWRAP_RRARB_EN,
> +	PWRAP_RRARB_STA0,
> +	PWRAP_RRARB_STA1,
> +	PWRAP_EVENT_STA,
> +	PWRAP_EVENT_STACLR,
> +	PWRAP_CIPHER_LOAD,
> +	PWRAP_CIPHER_START,
> +
>   	/* MT7622 only regs */
>   	PWRAP_STA,
>   	PWRAP_CLR,
> @@ -422,38 +508,9 @@ enum pwrap_regs {
>   	PWRAP_DVFS_STEP_CTRL2,
>   	PWRAP_SPI2_CTRL,
>   
> -	/* MT8135 only regs */
> -	PWRAP_CSHEXT,
> -	PWRAP_EVENT_IN_EN,
> -	PWRAP_EVENT_DST_EN,
> -	PWRAP_RRARB_INIT,
> -	PWRAP_RRARB_EN,
> -	PWRAP_RRARB_STA0,
> -	PWRAP_RRARB_STA1,
> -	PWRAP_EVENT_STA,
> -	PWRAP_EVENT_STACLR,
> -	PWRAP_CIPHER_LOAD,
> -	PWRAP_CIPHER_START,
> -

Why are you reordering stuff around? I don't understand.

If you have a reason, make sure it's a good one, and make a different commit
just for the reordering, because otherwise the diff for this one gets a bit
hard to read.


Cheers,
Angelo

>   	/* MT8173 only regs */
>   	PWRAP_RDDMY,
>   	PWRAP_SI_CK_CON,
> -	PWRAP_DVFS_ADR0,
> -	PWRAP_DVFS_WDATA0,
> -	PWRAP_DVFS_ADR1,
> -	PWRAP_DVFS_WDATA1,
> -	PWRAP_DVFS_ADR2,
> -	PWRAP_DVFS_WDATA2,
> -	PWRAP_DVFS_ADR3,
> -	PWRAP_DVFS_WDATA3,
> -	PWRAP_DVFS_ADR4,
> -	PWRAP_DVFS_WDATA4,
> -	PWRAP_DVFS_ADR5,
> -	PWRAP_DVFS_WDATA5,
> -	PWRAP_DVFS_ADR6,
> -	PWRAP_DVFS_WDATA6,
> -	PWRAP_DVFS_ADR7,
> -	PWRAP_DVFS_WDATA7,
>   	PWRAP_SPMINF_STA,
>   	PWRAP_CIPHER_EN,