linux-kernel - Re: [PATCH 2/3] soc:mediatek mt8189: Porting driver for spmi/pwrap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9b70908a-d664-4f2c-8fd1-3ca280fe7381@collabora.com>
Date: Fri, 1 Aug 2025 09:39:02 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: "niklaus.liu" <niklaus.liu@...iatek.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Stephen Boyd <sboyd@...nel.org>,
 Matthias Brugger <matthias.bgg@...il.com>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Flora Fu <flora.fu@...iatek.com>, Alexandre Mergnat <amergnat@...libre.com>,
 Hsin-Hsiung Wang <hsin-hsiung.wang@...iatek.com>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 Project_Global_Chrome_Upstream_Group@...iatek.com
Subject: Re: [PATCH 2/3] soc:mediatek mt8189: Porting driver for spmi/pwrap

Il 01/08/25 08:39, niklaus.liu ha scritto:
> Modify spmi/pwrap driver for mt8189
> 
> Signed-off-by: niklaus.liu <niklaus.liu@...iatek.com>
> ---
>   drivers/soc/mediatek/mtk-pmic-wrap.c | 27 +++++++++++++++++++++++++++
>   drivers/spmi/spmi-mtk-pmif.c         |  3 +++
>   2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 0bcd85826375..e3e8234e29a0 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -1087,6 +1087,17 @@ static const int mt8183_regs[] = {
>   	[PWRAP_WACS2_VLDCLR] =			0xC28,
>   };
>   
> +static int mt8189_regs[] = {
> +	[PWRAP_INIT_DONE2] =		0x0,
> +	[PWRAP_TIMER_EN] =		0x3e4,
> +	[PWRAP_INT_EN] =		0x450,
> +	[PWRAP_WACS2_CMD] =		0x880,
> +	[PWRAP_SWINF_2_WDATA_31_0] =	0x884,
> +	[PWRAP_SWINF_2_RDATA_31_0] =	0x894,
> +	[PWRAP_WACS2_VLDCLR] =		0x8a4,
> +	[PWRAP_WACS2_RDATA] =		0x8a8,
> +};

You can fully reuse mt8195_regs, as mt8189 has the same layout.

> +
>   static const int mt8195_regs[] = {
>   	[PWRAP_INIT_DONE2] =		0x0,
>   	[PWRAP_STAUPD_CTRL] =		0x4C,
> @@ -1324,6 +1335,7 @@ enum pwrap_type {
>   	PWRAP_MT8173,
>   	PWRAP_MT8183,
>   	PWRAP_MT8186,
> +	PWRAP_MT8189,
>   	PWRAP_MT8195,
>   	PWRAP_MT8365,
>   	PWRAP_MT8516,
> @@ -1854,6 +1866,7 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>   		break;
>   	case PWRAP_MT6873:
>   	case PWRAP_MT8183:
> +	case PWRAP_MT8189:
>   	case PWRAP_MT8195:
>   		break;
>   	}
> @@ -2393,6 +2406,19 @@ static const struct pmic_wrapper_type pwrap_mt8183 = {
>   	.init_soc_specific = pwrap_mt8183_init_soc_specific,
>   };
>   
> +static struct pmic_wrapper_type pwrap_mt8189 = {
> +	.regs = mt8189_regs,
> +	.type = PWRAP_MT8189,
> +	.arb_en_all = 0x777f,
> +	.int_en_all = 0x180000,
> +	.int1_en_all = 0,
> +	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
> +	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
> +	.caps = PWRAP_CAP_ARB,

Why are you avoiding to enable the INT1 interrupt on MT8189?

Is this working around a hardware bug, or did you simply forget to enable it?
I think you should really enable it, which means....

> +	.init_reg_clock = pwrap_common_init_reg_clock,
> +	.init_soc_specific = NULL,
> +};
> +
>   static const struct pmic_wrapper_type pwrap_mt8195 = {
>   	.regs = mt8195_regs,
>   	.type = PWRAP_MT8195,
> @@ -2456,6 +2482,7 @@ static const struct of_device_id of_pwrap_match_tbl[] = {
>   	{ .compatible = "mediatek,mt8173-pwrap", .data = &pwrap_mt8173 },
>   	{ .compatible = "mediatek,mt8183-pwrap", .data = &pwrap_mt8183 },
>   	{ .compatible = "mediatek,mt8186-pwrap", .data = &pwrap_mt8186 },
> +	{ .compatible = "mediatek,mt8189-pwrap", .data = &pwrap_mt8189 },

...means that you don't even need to add a new compatible in this list, because
the MT8195 compatible can be reused.

You only have to add the MT8189 compatible to the bindings, so that you are
allowed to specify in your devicetree node

compatible = "mediatek,mt8189-pwrap", "mediatek,mt8195-pwrap";

>   	{ .compatible = "mediatek,mt8195-pwrap", .data = &pwrap_mt8195 },
>   	{ .compatible = "mediatek,mt8365-pwrap", .data = &pwrap_mt8365 },
>   	{ .compatible = "mediatek,mt8516-pwrap", .data = &pwrap_mt8516 },
> diff --git a/drivers/spmi/spmi-mtk-pmif.c b/drivers/spmi/spmi-mtk-pmif.c
> index 160d36f7d238..00420568afef 100644
> --- a/drivers/spmi/spmi-mtk-pmif.c
> +++ b/drivers/spmi/spmi-mtk-pmif.c
> @@ -530,6 +530,9 @@ static const struct of_device_id mtk_spmi_match_table[] = {
>   	{
>   		.compatible = "mediatek,mt6873-spmi",
>   		.data = &mt6873_pmif_arb,
> +	}, {
> +		.compatible = "mediatek,mt8189-spmi",
> +		.data = &mt8195_pmif_arb,

This change is useless. Just add the compatible to the bindings so that you
can specify

compatible = "mediatek,mt8189-spmi", "mediatek,mt8195-spmi";

Regards,
Angelo