[PATCH 0/3] Pwrap: Mediatek pwrap driver for MT6779

[PATCH 0/3] Pwrap: Mediatek pwrap driver for MT6779

[Argus Lin]

Here's version 1 of the patch series, include 3 patches:
1. Add compatible for MT6779 pwrap
2. Add pwrap driver for MT6779 SoCs. Keep PWRAP_HIPRIO_ARB_EN,
PWRAP_WDT_UNIT, and PWRAP_WDT_SRC_EN value if it has initialized.
When we enable interrupt flag, read current value then do logical
OR opersion with wrp->master->int_en_all.
3. Add MT6359 support for MT6779 SoCs.

Argus Lin (3):
  dt-bindings: pwrap: mediatek: add pwrap support for MT6779
  soc: mediatek: pwrap: add pwrap driver for MT6779 SoCs
  soc: mediatek: pwrap: add support for MT6359 PMIC

 .../devicetree/bindings/soc/mediatek/pwrap.txt     |   1 +
 drivers/soc/mediatek/mtk-pmic-wrap.c               | 157 +++++++++++++++++++--
 2 files changed, 150 insertions(+), 8 deletions(-)

--
1.8.1.1.dirty

[PATCH 1/3] dt-bindings: pwrap: mediatek: add pwrap support for MT6779

[Argus Lin]

Add binding document of pwrap for MT6779 SoCs.

Signed-off-by: Argus Lin <argus.lin@mediatek.com>
---
 Documentation/devicetree/bindings/soc/mediatek/pwrap.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt b/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
index 7a32404..ecac2bb 100644
--- a/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
+++ b/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
@@ -20,6 +20,7 @@ Required properties in pwrap device node.
 - compatible:
 	"mediatek,mt2701-pwrap" for MT2701/7623 SoCs
 	"mediatek,mt6765-pwrap" for MT6765 SoCs
+	"mediatek,mt6779-pwrap" for MT6779 SoCs
 	"mediatek,mt6797-pwrap" for MT6797 SoCs
 	"mediatek,mt7622-pwrap" for MT7622 SoCs
 	"mediatek,mt8135-pwrap" for MT8135 SoCs
--
1.8.1.1.dirty

[PATCH 2/3] soc: mediatek: pwrap: add pwrap driver for MT6779 SoCs

[Argus Lin]

MT6779 is a highly integrated SoCs, it uses MT6359 for power
management. This patch adds pwrap driver to access MT6359.
Pwrap of MT6779 support dynamic priority meichanism, sequence
monitor and starvation mechanism to make transaction more
reliable. WDT setting only need to init when it is zero,
otherwise keep current value. When setting interrupt enable
flag at pwrap_probe, read current setting then do logical OR
operation with wrp->master->int_en_all.

Signed-off-by: Argus Lin <argus.lin@mediatek.com>
---
 drivers/soc/mediatek/mtk-pmic-wrap.c | 85 ++++++++++++++++++++++++++++++++----
 1 file changed, 77 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
index c725315..fa8daf2 100644
--- a/drivers/soc/mediatek/mtk-pmic-wrap.c
+++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
@@ -497,6 +497,45 @@ enum pwrap_regs {
 	[PWRAP_DCM_DBC_PRD] =		0x1E0,
 };

+static int mt6779_regs[] = {
+	[PWRAP_MUX_SEL] =		0x0,
+	[PWRAP_WRAP_EN] =		0x4,
+	[PWRAP_DIO_EN] =		0x8,
+	[PWRAP_RDDMY] =			0x20,
+	[PWRAP_CSHEXT_WRITE] =		0x24,
+	[PWRAP_CSHEXT_READ] =		0x28,
+	[PWRAP_CSLEXT_WRITE] =		0x2C,
+	[PWRAP_CSLEXT_READ] =		0x30,
+	[PWRAP_EXT_CK_WRITE] =		0x34,
+	[PWRAP_STAUPD_CTRL] =		0x3C,
+	[PWRAP_STAUPD_GRPEN] =		0x40,
+	[PWRAP_EINT_STA0_ADR] =		0x44,
+	[PWRAP_HARB_HPRIO] =		0x68,
+	[PWRAP_HIPRIO_ARB_EN] =		0x6C,
+	[PWRAP_MAN_EN] =		0x7C,
+	[PWRAP_MAN_CMD] =		0x80,
+	[PWRAP_WACS0_EN] =		0x8C,
+	[PWRAP_WACS1_EN] =		0x94,
+	[PWRAP_WACS2_EN] =		0x9C,
+	[PWRAP_INIT_DONE0] =		0x90,
+	[PWRAP_INIT_DONE1] =		0x98,
+	[PWRAP_INIT_DONE2] =		0xA0,
+	[PWRAP_INT_EN] =		0xBC,
+	[PWRAP_INT_FLG_RAW] =		0xC0,
+	[PWRAP_INT_FLG] =		0xC4,
+	[PWRAP_INT_CLR] =		0xC8,
+	[PWRAP_INT1_EN] =		0xCC,
+	[PWRAP_INT1_FLG] =		0xD4,
+	[PWRAP_INT1_CLR] =		0xD8,
+	[PWRAP_TIMER_EN] =		0xF0,
+	[PWRAP_WDT_UNIT] =		0xF8,
+	[PWRAP_WDT_SRC_EN] =		0xFC,
+	[PWRAP_WDT_SRC_EN_1] =		0x100,
+	[PWRAP_WACS2_CMD] =		0xC20,
+	[PWRAP_WACS2_RDATA] =		0xC24,
+	[PWRAP_WACS2_VLDCLR] =		0xC28,
+};
+
 static int mt6797_regs[] = {
 	[PWRAP_MUX_SEL] =		0x0,
 	[PWRAP_WRAP_EN] =		0x4,
@@ -945,6 +984,7 @@ enum pmic_type {
 enum pwrap_type {
 	PWRAP_MT2701,
 	PWRAP_MT6765,
+	PWRAP_MT6779,
 	PWRAP_MT6797,
 	PWRAP_MT7622,
 	PWRAP_MT8135,
@@ -1377,6 +1417,7 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
 		break;
 	case PWRAP_MT2701:
 	case PWRAP_MT6765:
+	case PWRAP_MT6779:
 	case PWRAP_MT6797:
 	case PWRAP_MT8173:
 	case PWRAP_MT8516:
@@ -1468,8 +1509,10 @@ static int pwrap_init_security(struct pmic_wrapper *wrp)
 	pwrap_writel(wrp, 0x0, PWRAP_SIG_MODE);
 	pwrap_writel(wrp, wrp->slave->dew_regs[PWRAP_DEW_CRC_VAL],
 		     PWRAP_SIG_ADR);
-	pwrap_writel(wrp,
-		     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
+	if (pwrap_readl(wrp, PWRAP_HIPRIO_ARB_EN) == 0) {
+		pwrap_writel(wrp,
+			     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
+	}

 	return 0;
 }
@@ -1581,7 +1624,10 @@ static int pwrap_init(struct pmic_wrapper *wrp)

 	pwrap_writel(wrp, 1, PWRAP_WRAP_EN);

-	pwrap_writel(wrp, wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
+	if (pwrap_readl(wrp, PWRAP_HIPRIO_ARB_EN) == 0) {
+		pwrap_writel(wrp,
+			     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
+	}

 	pwrap_writel(wrp, 1, PWRAP_WACS2_EN);

@@ -1783,6 +1829,19 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
 	.init_soc_specific = NULL,
 };

+static const struct pmic_wrapper_type pwrap_mt6779 = {
+	.regs = mt6779_regs,
+	.type = PWRAP_MT6779,
+	.arb_en_all = 0,
+	.int_en_all = 0,
+	.int1_en_all = 0,
+	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
+	.wdt_src = 0,
+	.caps = 0,
+	.init_reg_clock = pwrap_common_init_reg_clock,
+	.init_soc_specific = NULL,
+};
+
 static const struct pmic_wrapper_type pwrap_mt6797 = {
 	.regs = mt6797_regs,
 	.type = PWRAP_MT6797,
@@ -1868,6 +1927,9 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
 		.compatible = "mediatek,mt6765-pwrap",
 		.data = &pwrap_mt6765,
 	}, {
+		.compatible = "mediatek,mt6779-pwrap",
+		.data = &pwrap_mt6779,
+	}, {
 		.compatible = "mediatek,mt6797-pwrap",
 		.data = &pwrap_mt6797,
 	}, {
@@ -1898,6 +1960,7 @@ static int pwrap_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	const struct of_device_id *of_slave_id = NULL;
 	struct resource *res;
+	u32 int_en;

 	if (np->child)
 		of_slave_id = of_match_node(of_slave_match_tbl, np->child);
@@ -1995,23 +2058,29 @@ static int pwrap_probe(struct platform_device *pdev)
 	}

 	/* Initialize watchdog, may not be done by the bootloader */
-	pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
+	if (pwrap_readl(wrp, PWRAP_WDT_UNIT) == 0)
+		pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
 	/*
 	 * Since STAUPD was not used on mt8173 platform,
 	 * so STAUPD of WDT_SRC which should be turned off
 	 */
-	pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
+	if (pwrap_readl(wrp, PWRAP_WDT_SRC_EN) == 0)
+		pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
 	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_WDT_SRC1))
 		pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN_1);

 	pwrap_writel(wrp, 0x1, PWRAP_TIMER_EN);
-	pwrap_writel(wrp, wrp->master->int_en_all, PWRAP_INT_EN);
+	int_en = pwrap_readl(wrp, PWRAP_INT_EN);
+	pwrap_writel(wrp, (int_en) | (wrp->master->int_en_all), PWRAP_INT_EN);
 	/*
 	 * We add INT1 interrupt to handle starvation and request exception
 	 * If we support it, we should enable it here.
 	 */
-	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
-		pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
+	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN)) {
+		int_en = pwrap_readl(wrp, PWRAP_INT1_EN);
+		pwrap_writel(wrp, (int_en) | wrp->master->int1_en_all,
+			     PWRAP_INT1_EN);
+	}

 	irq = platform_get_irq(pdev, 0);
 	ret = devm_request_irq(wrp->dev, irq, pwrap_interrupt,
--
1.8.1.1.dirty

[PATCH 3/3] soc: mediatek: pwrap: add support for MT6359 PMIC

[Argus Lin]

MT6359 is a new power management IC and it is used for
MT6779 SoCs. To define mt6359_regs for pmic register mapping
and pmic_mt6359 for accessing register.

Signed-off-by: Argus Lin <argus.lin@mediatek.com>
---
 drivers/soc/mediatek/mtk-pmic-wrap.c | 72 ++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
index fa8daf2..dd04318 100644
--- a/drivers/soc/mediatek/mtk-pmic-wrap.c
+++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
@@ -111,6 +111,29 @@ enum dew_regs {
 	PWRAP_RG_SPI_CON13,
 	PWRAP_SPISLV_KEY,

+	/* MT6359 only regs */
+	PWRAP_DEW_CRC_SWRST,
+	PWRAP_DEW_RG_EN_RECORD,
+	PWRAP_DEW_RECORD_CMD0,
+	PWRAP_DEW_RECORD_CMD1,
+	PWRAP_DEW_RECORD_CMD2,
+	PWRAP_DEW_RECORD_CMD3,
+	PWRAP_DEW_RECORD_CMD4,
+	PWRAP_DEW_RECORD_CMD5,
+	PWRAP_DEW_RECORD_WDATA0,
+	PWRAP_DEW_RECORD_WDATA1,
+	PWRAP_DEW_RECORD_WDATA2,
+	PWRAP_DEW_RECORD_WDATA3,
+	PWRAP_DEW_RECORD_WDATA4,
+	PWRAP_DEW_RECORD_WDATA5,
+	PWRAP_DEW_RG_ADDR_TARGET,
+	PWRAP_DEW_RG_ADDR_MASK,
+	PWRAP_DEW_RG_WDATA_TARGET,
+	PWRAP_DEW_RG_WDATA_MASK,
+	PWRAP_DEW_RG_SPI_RECORD_CLR,
+	PWRAP_DEW_RG_CMD_ALERT_CLR,
+	PWRAP_DEW_SPISLV_KEY,
+
 	/* MT6397 only regs */
 	PWRAP_DEW_EVENT_OUT_EN,
 	PWRAP_DEW_EVENT_SRC_EN,
@@ -197,6 +220,42 @@ enum dew_regs {
 	[PWRAP_SPISLV_KEY] =		0x044a,
 };

+static const u32 mt6359_regs[] = {
+	[PWRAP_DEW_RG_EN_RECORD] =	0x040a,
+	[PWRAP_DEW_DIO_EN] =		0x040c,
+	[PWRAP_DEW_READ_TEST] =		0x040e,
+	[PWRAP_DEW_WRITE_TEST] =	0x0410,
+	[PWRAP_DEW_CRC_SWRST] =		0x0412,
+	[PWRAP_DEW_CRC_EN] =		0x0414,
+	[PWRAP_DEW_CRC_VAL] =		0x0416,
+	[PWRAP_DEW_CIPHER_KEY_SEL] =	0x0418,
+	[PWRAP_DEW_CIPHER_IV_SEL] =	0x041a,
+	[PWRAP_DEW_CIPHER_EN] =		0x041c,
+	[PWRAP_DEW_CIPHER_RDY] =	0x041e,
+	[PWRAP_DEW_CIPHER_MODE] =	0x0420,
+	[PWRAP_DEW_CIPHER_SWRST] =	0x0422,
+	[PWRAP_DEW_RDDMY_NO] =		0x0424,
+	[PWRAP_DEW_RECORD_CMD0] =	0x0428,
+	[PWRAP_DEW_RECORD_CMD1] =	0x042a,
+	[PWRAP_DEW_RECORD_CMD2] =	0x042c,
+	[PWRAP_DEW_RECORD_CMD3] =	0x042e,
+	[PWRAP_DEW_RECORD_CMD4] =	0x0430,
+	[PWRAP_DEW_RECORD_CMD5] =	0x0432,
+	[PWRAP_DEW_RECORD_WDATA0] =	0x0434,
+	[PWRAP_DEW_RECORD_WDATA1] =	0x0436,
+	[PWRAP_DEW_RECORD_WDATA2] =	0x0438,
+	[PWRAP_DEW_RECORD_WDATA3] =	0x043a,
+	[PWRAP_DEW_RECORD_WDATA4] =	0x043c,
+	[PWRAP_DEW_RECORD_WDATA5] =	0x043e,
+	[PWRAP_DEW_RG_ADDR_TARGET] =	0x0440,
+	[PWRAP_DEW_RG_ADDR_MASK] =	0x0442,
+	[PWRAP_DEW_RG_WDATA_TARGET] =	0x0444,
+	[PWRAP_DEW_RG_WDATA_MASK] =	0x0446,
+	[PWRAP_DEW_RG_SPI_RECORD_CLR] =	0x0448,
+	[PWRAP_DEW_RG_CMD_ALERT_CLR] =	0x0448,
+	[PWRAP_DEW_SPISLV_KEY] =	0x044a,
+};
+
 static const u32 mt6397_regs[] = {
 	[PWRAP_DEW_BASE] =		0xbc00,
 	[PWRAP_DEW_EVENT_OUT_EN] =	0xbc00,
@@ -977,6 +1036,7 @@ enum pmic_type {
 	PMIC_MT6351,
 	PMIC_MT6357,
 	PMIC_MT6358,
+	PMIC_MT6359,
 	PMIC_MT6380,
 	PMIC_MT6397,
 };
@@ -1757,6 +1817,15 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
 	.pwrap_write = pwrap_write16,
 };

+static const struct pwrap_slv_type pmic_mt6359 = {
+	.dew_regs = mt6359_regs,
+	.type = PMIC_MT6359,
+	.regmap = &pwrap_regmap_config16,
+	.caps = PWRAP_SLV_CAP_DUALIO,
+	.pwrap_read = pwrap_read16,
+	.pwrap_write = pwrap_write16,
+};
+
 static const struct pwrap_slv_type pmic_mt6380 = {
 	.dew_regs = NULL,
 	.type = PMIC_MT6380,
@@ -1790,6 +1859,9 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
 		.compatible = "mediatek,mt6358",
 		.data = &pmic_mt6358,
 	}, {
+		.compatible = "mediatek,mt6359",
+		.data = &pmic_mt6359,
+	}, {
 		/* The MT6380 PMIC only implements a regulator, so we bind it
 		 * directly instead of using a MFD.
 		 */
--
1.8.1.1.dirty

Re: [PATCH 2/3] soc: mediatek: pwrap: add pwrap driver for MT6779 SoCs

[Matthias Brugger]

On 03/10/2019 09:48, Argus Lin wrote:
> MT6779 is a highly integrated SoCs, it uses MT6359 for power
> management. This patch adds pwrap driver to access MT6359.
> Pwrap of MT6779 support dynamic priority meichanism, sequence

mechanism

> monitor and starvation mechanism to make transaction more
> reliable. WDT setting only need to init when it is zero,
> otherwise keep current value. When setting interrupt enable

that's mt6779 specific?

> flag at pwrap_probe, read current setting then do logical OR
> operation with wrp->master->int_en_all.

same here, only specific to mt6779?

> 
> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 85 ++++++++++++++++++++++++++++++++----
>  1 file changed, 77 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index c725315..fa8daf2 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -497,6 +497,45 @@ enum pwrap_regs {
>  	[PWRAP_DCM_DBC_PRD] =		0x1E0,
>  };
> 
> +static int mt6779_regs[] = {
> +	[PWRAP_MUX_SEL] =		0x0,
> +	[PWRAP_WRAP_EN] =		0x4,
> +	[PWRAP_DIO_EN] =		0x8,
> +	[PWRAP_RDDMY] =			0x20,
> +	[PWRAP_CSHEXT_WRITE] =		0x24,
> +	[PWRAP_CSHEXT_READ] =		0x28,
> +	[PWRAP_CSLEXT_WRITE] =		0x2C,
> +	[PWRAP_CSLEXT_READ] =		0x30,
> +	[PWRAP_EXT_CK_WRITE] =		0x34,
> +	[PWRAP_STAUPD_CTRL] =		0x3C,
> +	[PWRAP_STAUPD_GRPEN] =		0x40,
> +	[PWRAP_EINT_STA0_ADR] =		0x44,
> +	[PWRAP_HARB_HPRIO] =		0x68,
> +	[PWRAP_HIPRIO_ARB_EN] =		0x6C,
> +	[PWRAP_MAN_EN] =		0x7C,
> +	[PWRAP_MAN_CMD] =		0x80,
> +	[PWRAP_WACS0_EN] =		0x8C,
> +	[PWRAP_WACS1_EN] =		0x94,
> +	[PWRAP_WACS2_EN] =		0x9C,
> +	[PWRAP_INIT_DONE0] =		0x90,
> +	[PWRAP_INIT_DONE1] =		0x98,
> +	[PWRAP_INIT_DONE2] =		0xA0,
> +	[PWRAP_INT_EN] =		0xBC,
> +	[PWRAP_INT_FLG_RAW] =		0xC0,
> +	[PWRAP_INT_FLG] =		0xC4,
> +	[PWRAP_INT_CLR] =		0xC8,
> +	[PWRAP_INT1_EN] =		0xCC,
> +	[PWRAP_INT1_FLG] =		0xD4,
> +	[PWRAP_INT1_CLR] =		0xD8,
> +	[PWRAP_TIMER_EN] =		0xF0,
> +	[PWRAP_WDT_UNIT] =		0xF8,
> +	[PWRAP_WDT_SRC_EN] =		0xFC,
> +	[PWRAP_WDT_SRC_EN_1] =		0x100,
> +	[PWRAP_WACS2_CMD] =		0xC20,
> +	[PWRAP_WACS2_RDATA] =		0xC24,
> +	[PWRAP_WACS2_VLDCLR] =		0xC28,
> +};
> +
>  static int mt6797_regs[] = {
>  	[PWRAP_MUX_SEL] =		0x0,
>  	[PWRAP_WRAP_EN] =		0x4,
> @@ -945,6 +984,7 @@ enum pmic_type {
>  enum pwrap_type {
>  	PWRAP_MT2701,
>  	PWRAP_MT6765,
> +	PWRAP_MT6779,
>  	PWRAP_MT6797,
>  	PWRAP_MT7622,
>  	PWRAP_MT8135,
> @@ -1377,6 +1417,7 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>  		break;
>  	case PWRAP_MT2701:
>  	case PWRAP_MT6765:
> +	case PWRAP_MT6779:
>  	case PWRAP_MT6797:
>  	case PWRAP_MT8173:
>  	case PWRAP_MT8516:
> @@ -1468,8 +1509,10 @@ static int pwrap_init_security(struct pmic_wrapper *wrp)
>  	pwrap_writel(wrp, 0x0, PWRAP_SIG_MODE);
>  	pwrap_writel(wrp, wrp->slave->dew_regs[PWRAP_DEW_CRC_VAL],
>  		     PWRAP_SIG_ADR);
> -	pwrap_writel(wrp,
> -		     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
> +	if (pwrap_readl(wrp, PWRAP_HIPRIO_ARB_EN) == 0) {

Did you make sure that this holds for all SoCs that are supported by the driver?
If so, why do we need this in mt6779 and didn't need that in the others?
Even more, mt6779 does not have the security capbaility, so why do you change
this code?

> +		pwrap_writel(wrp,
> +			     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
> +	}

I just realize that we write PWRAP_HIPRIO_ARB_EN twice if the slave supports
security. Do we really need that?

> 
>  	return 0;
>  }
> @@ -1581,7 +1624,10 @@ static int pwrap_init(struct pmic_wrapper *wrp)
> 
>  	pwrap_writel(wrp, 1, PWRAP_WRAP_EN);
> 
> -	pwrap_writel(wrp, wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
> +	if (pwrap_readl(wrp, PWRAP_HIPRIO_ARB_EN) == 0) {
> +		pwrap_writel(wrp,
> +			     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
> +	}
> 
>  	pwrap_writel(wrp, 1, PWRAP_WACS2_EN);
> 
> @@ -1783,6 +1829,19 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
>  	.init_soc_specific = NULL,
>  };
> 
> +static const struct pmic_wrapper_type pwrap_mt6779 = {
> +	.regs = mt6779_regs,
> +	.type = PWRAP_MT6779,
> +	.arb_en_all = 0,
> +	.int_en_all = 0,
> +	.int1_en_all = 0,
> +	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
> +	.wdt_src = 0,
> +	.caps = 0,
> +	.init_reg_clock = pwrap_common_init_reg_clock,
> +	.init_soc_specific = NULL,
> +};
> +
>  static const struct pmic_wrapper_type pwrap_mt6797 = {
>  	.regs = mt6797_regs,
>  	.type = PWRAP_MT6797,
> @@ -1868,6 +1927,9 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
>  		.compatible = "mediatek,mt6765-pwrap",
>  		.data = &pwrap_mt6765,
>  	}, {
> +		.compatible = "mediatek,mt6779-pwrap",
> +		.data = &pwrap_mt6779,
> +	}, {
>  		.compatible = "mediatek,mt6797-pwrap",
>  		.data = &pwrap_mt6797,
>  	}, {
> @@ -1898,6 +1960,7 @@ static int pwrap_probe(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	const struct of_device_id *of_slave_id = NULL;
>  	struct resource *res;
> +	u32 int_en;
> 
>  	if (np->child)
>  		of_slave_id = of_match_node(of_slave_match_tbl, np->child);
> @@ -1995,23 +2058,29 @@ static int pwrap_probe(struct platform_device *pdev)
>  	}
> 
>  	/* Initialize watchdog, may not be done by the bootloader */
> -	pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
> +	if (pwrap_readl(wrp, PWRAP_WDT_UNIT) == 0)

Same here, why do we need it in mt6779 and did you test if it does not break any
older SoC?

> +		pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
>  	/*
>  	 * Since STAUPD was not used on mt8173 platform,
>  	 * so STAUPD of WDT_SRC which should be turned off
>  	 */
> -	pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
> +	if (pwrap_readl(wrp, PWRAP_WDT_SRC_EN) == 0)
> +		pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
>  	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_WDT_SRC1))
>  		pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN_1);
> 
>  	pwrap_writel(wrp, 0x1, PWRAP_TIMER_EN);
> -	pwrap_writel(wrp, wrp->master->int_en_all, PWRAP_INT_EN);
> +	int_en = pwrap_readl(wrp, PWRAP_INT_EN);
> +	pwrap_writel(wrp, (int_en) | (wrp->master->int_en_all), PWRAP_INT_EN);

Looks ok to me, is it a bug fix, or only needed for mt6779?

>  	/*
>  	 * We add INT1 interrupt to handle starvation and request exception
>  	 * If we support it, we should enable it here.
>  	 */
> -	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
> -		pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN)) {
> +		int_en = pwrap_readl(wrp, PWRAP_INT1_EN);
> +		pwrap_writel(wrp, (int_en) | wrp->master->int1_en_all,
> +			     PWRAP_INT1_EN);
> +	}
> 
>  	irq = platform_get_irq(pdev, 0);
>  	ret = devm_request_irq(wrp->dev, irq, pwrap_interrupt,
> --
> 1.8.1.1.dirty
> 

Re: [PATCH 3/3] soc: mediatek: pwrap: add support for MT6359 PMIC

[Matthias Brugger]

On 03/10/2019 09:48, Argus Lin wrote:
> MT6359 is a new power management IC and it is used for
> MT6779 SoCs. To define mt6359_regs for pmic register mapping
> and pmic_mt6359 for accessing register.
> 
> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 72 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index fa8daf2..dd04318 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -111,6 +111,29 @@ enum dew_regs {
>  	PWRAP_RG_SPI_CON13,
>  	PWRAP_SPISLV_KEY,
> 
> +	/* MT6359 only regs */
> +	PWRAP_DEW_CRC_SWRST,
> +	PWRAP_DEW_RG_EN_RECORD,
> +	PWRAP_DEW_RECORD_CMD0,
> +	PWRAP_DEW_RECORD_CMD1,
> +	PWRAP_DEW_RECORD_CMD2,
> +	PWRAP_DEW_RECORD_CMD3,
> +	PWRAP_DEW_RECORD_CMD4,
> +	PWRAP_DEW_RECORD_CMD5,
> +	PWRAP_DEW_RECORD_WDATA0,
> +	PWRAP_DEW_RECORD_WDATA1,
> +	PWRAP_DEW_RECORD_WDATA2,
> +	PWRAP_DEW_RECORD_WDATA3,
> +	PWRAP_DEW_RECORD_WDATA4,
> +	PWRAP_DEW_RECORD_WDATA5,
> +	PWRAP_DEW_RG_ADDR_TARGET,
> +	PWRAP_DEW_RG_ADDR_MASK,
> +	PWRAP_DEW_RG_WDATA_TARGET,
> +	PWRAP_DEW_RG_WDATA_MASK,
> +	PWRAP_DEW_RG_SPI_RECORD_CLR,
> +	PWRAP_DEW_RG_CMD_ALERT_CLR,
> +	PWRAP_DEW_SPISLV_KEY,

That looks like PWRAP_SPISLV_KEY from MT6358.

Regards,
Matthias

> +
>  	/* MT6397 only regs */
>  	PWRAP_DEW_EVENT_OUT_EN,
>  	PWRAP_DEW_EVENT_SRC_EN,
> @@ -197,6 +220,42 @@ enum dew_regs {
>  	[PWRAP_SPISLV_KEY] =		0x044a,
>  };
> 
> +static const u32 mt6359_regs[] = {
> +	[PWRAP_DEW_RG_EN_RECORD] =	0x040a,
> +	[PWRAP_DEW_DIO_EN] =		0x040c,
> +	[PWRAP_DEW_READ_TEST] =		0x040e,
> +	[PWRAP_DEW_WRITE_TEST] =	0x0410,
> +	[PWRAP_DEW_CRC_SWRST] =		0x0412,
> +	[PWRAP_DEW_CRC_EN] =		0x0414,
> +	[PWRAP_DEW_CRC_VAL] =		0x0416,
> +	[PWRAP_DEW_CIPHER_KEY_SEL] =	0x0418,
> +	[PWRAP_DEW_CIPHER_IV_SEL] =	0x041a,
> +	[PWRAP_DEW_CIPHER_EN] =		0x041c,
> +	[PWRAP_DEW_CIPHER_RDY] =	0x041e,
> +	[PWRAP_DEW_CIPHER_MODE] =	0x0420,
> +	[PWRAP_DEW_CIPHER_SWRST] =	0x0422,
> +	[PWRAP_DEW_RDDMY_NO] =		0x0424,
> +	[PWRAP_DEW_RECORD_CMD0] =	0x0428,
> +	[PWRAP_DEW_RECORD_CMD1] =	0x042a,
> +	[PWRAP_DEW_RECORD_CMD2] =	0x042c,
> +	[PWRAP_DEW_RECORD_CMD3] =	0x042e,
> +	[PWRAP_DEW_RECORD_CMD4] =	0x0430,
> +	[PWRAP_DEW_RECORD_CMD5] =	0x0432,
> +	[PWRAP_DEW_RECORD_WDATA0] =	0x0434,
> +	[PWRAP_DEW_RECORD_WDATA1] =	0x0436,
> +	[PWRAP_DEW_RECORD_WDATA2] =	0x0438,
> +	[PWRAP_DEW_RECORD_WDATA3] =	0x043a,
> +	[PWRAP_DEW_RECORD_WDATA4] =	0x043c,
> +	[PWRAP_DEW_RECORD_WDATA5] =	0x043e,
> +	[PWRAP_DEW_RG_ADDR_TARGET] =	0x0440,
> +	[PWRAP_DEW_RG_ADDR_MASK] =	0x0442,
> +	[PWRAP_DEW_RG_WDATA_TARGET] =	0x0444,
> +	[PWRAP_DEW_RG_WDATA_MASK] =	0x0446,
> +	[PWRAP_DEW_RG_SPI_RECORD_CLR] =	0x0448,
> +	[PWRAP_DEW_RG_CMD_ALERT_CLR] =	0x0448,
> +	[PWRAP_DEW_SPISLV_KEY] =	0x044a,
> +};
> +
>  static const u32 mt6397_regs[] = {
>  	[PWRAP_DEW_BASE] =		0xbc00,
>  	[PWRAP_DEW_EVENT_OUT_EN] =	0xbc00,
> @@ -977,6 +1036,7 @@ enum pmic_type {
>  	PMIC_MT6351,
>  	PMIC_MT6357,
>  	PMIC_MT6358,
> +	PMIC_MT6359,
>  	PMIC_MT6380,
>  	PMIC_MT6397,
>  };
> @@ -1757,6 +1817,15 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
>  	.pwrap_write = pwrap_write16,
>  };
> 
> +static const struct pwrap_slv_type pmic_mt6359 = {
> +	.dew_regs = mt6359_regs,
> +	.type = PMIC_MT6359,
> +	.regmap = &pwrap_regmap_config16,
> +	.caps = PWRAP_SLV_CAP_DUALIO,
> +	.pwrap_read = pwrap_read16,
> +	.pwrap_write = pwrap_write16,
> +};
> +
>  static const struct pwrap_slv_type pmic_mt6380 = {
>  	.dew_regs = NULL,
>  	.type = PMIC_MT6380,
> @@ -1790,6 +1859,9 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
>  		.compatible = "mediatek,mt6358",
>  		.data = &pmic_mt6358,
>  	}, {
> +		.compatible = "mediatek,mt6359",
> +		.data = &pmic_mt6359,
> +	}, {
>  		/* The MT6380 PMIC only implements a regulator, so we bind it
>  		 * directly instead of using a MFD.
>  		 */
> --
> 1.8.1.1.dirty
> 

Re: [PATCH 3/3] soc: mediatek: pwrap: add support for MT6359 PMIC

[Argus Lin]

On Fri, 2019-10-04 at 01:27 +0200, Matthias Brugger wrote:
> 
> On 03/10/2019 09:48, Argus Lin wrote:
> > MT6359 is a new power management IC and it is used for
> > MT6779 SoCs. To define mt6359_regs for pmic register mapping
> > and pmic_mt6359 for accessing register.
> > 
> > Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-pmic-wrap.c | 72 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > index fa8daf2..dd04318 100644
> > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > @@ -111,6 +111,29 @@ enum dew_regs {
> >  	PWRAP_RG_SPI_CON13,
> >  	PWRAP_SPISLV_KEY,
> > 
> > +	/* MT6359 only regs */
> > +	PWRAP_DEW_CRC_SWRST,
> > +	PWRAP_DEW_RG_EN_RECORD,
> > +	PWRAP_DEW_RECORD_CMD0,
> > +	PWRAP_DEW_RECORD_CMD1,
> > +	PWRAP_DEW_RECORD_CMD2,
> > +	PWRAP_DEW_RECORD_CMD3,
> > +	PWRAP_DEW_RECORD_CMD4,
> > +	PWRAP_DEW_RECORD_CMD5,
> > +	PWRAP_DEW_RECORD_WDATA0,
> > +	PWRAP_DEW_RECORD_WDATA1,
> > +	PWRAP_DEW_RECORD_WDATA2,
> > +	PWRAP_DEW_RECORD_WDATA3,
> > +	PWRAP_DEW_RECORD_WDATA4,
> > +	PWRAP_DEW_RECORD_WDATA5,
> > +	PWRAP_DEW_RG_ADDR_TARGET,
> > +	PWRAP_DEW_RG_ADDR_MASK,
> > +	PWRAP_DEW_RG_WDATA_TARGET,
> > +	PWRAP_DEW_RG_WDATA_MASK,
> > +	PWRAP_DEW_RG_SPI_RECORD_CLR,
> > +	PWRAP_DEW_RG_CMD_ALERT_CLR,
> > +	PWRAP_DEW_SPISLV_KEY,
> 
> That looks like PWRAP_SPISLV_KEY from MT6358.
> 
> Regards,
> Matthias
> 
Yes, I think I can reuse the PWRAP_SPISLV_KEY of MT6358.

B.R.
Argus
> > +
> >  	/* MT6397 only regs */
> >  	PWRAP_DEW_EVENT_OUT_EN,
> >  	PWRAP_DEW_EVENT_SRC_EN,
> > @@ -197,6 +220,42 @@ enum dew_regs {
> >  	[PWRAP_SPISLV_KEY] =		0x044a,
> >  };
> > 
> > +static const u32 mt6359_regs[] = {
> > +	[PWRAP_DEW_RG_EN_RECORD] =	0x040a,
> > +	[PWRAP_DEW_DIO_EN] =		0x040c,
> > +	[PWRAP_DEW_READ_TEST] =		0x040e,
> > +	[PWRAP_DEW_WRITE_TEST] =	0x0410,
> > +	[PWRAP_DEW_CRC_SWRST] =		0x0412,
> > +	[PWRAP_DEW_CRC_EN] =		0x0414,
> > +	[PWRAP_DEW_CRC_VAL] =		0x0416,
> > +	[PWRAP_DEW_CIPHER_KEY_SEL] =	0x0418,
> > +	[PWRAP_DEW_CIPHER_IV_SEL] =	0x041a,
> > +	[PWRAP_DEW_CIPHER_EN] =		0x041c,
> > +	[PWRAP_DEW_CIPHER_RDY] =	0x041e,
> > +	[PWRAP_DEW_CIPHER_MODE] =	0x0420,
> > +	[PWRAP_DEW_CIPHER_SWRST] =	0x0422,
> > +	[PWRAP_DEW_RDDMY_NO] =		0x0424,
> > +	[PWRAP_DEW_RECORD_CMD0] =	0x0428,
> > +	[PWRAP_DEW_RECORD_CMD1] =	0x042a,
> > +	[PWRAP_DEW_RECORD_CMD2] =	0x042c,
> > +	[PWRAP_DEW_RECORD_CMD3] =	0x042e,
> > +	[PWRAP_DEW_RECORD_CMD4] =	0x0430,
> > +	[PWRAP_DEW_RECORD_CMD5] =	0x0432,
> > +	[PWRAP_DEW_RECORD_WDATA0] =	0x0434,
> > +	[PWRAP_DEW_RECORD_WDATA1] =	0x0436,
> > +	[PWRAP_DEW_RECORD_WDATA2] =	0x0438,
> > +	[PWRAP_DEW_RECORD_WDATA3] =	0x043a,
> > +	[PWRAP_DEW_RECORD_WDATA4] =	0x043c,
> > +	[PWRAP_DEW_RECORD_WDATA5] =	0x043e,
> > +	[PWRAP_DEW_RG_ADDR_TARGET] =	0x0440,
> > +	[PWRAP_DEW_RG_ADDR_MASK] =	0x0442,
> > +	[PWRAP_DEW_RG_WDATA_TARGET] =	0x0444,
> > +	[PWRAP_DEW_RG_WDATA_MASK] =	0x0446,
> > +	[PWRAP_DEW_RG_SPI_RECORD_CLR] =	0x0448,
> > +	[PWRAP_DEW_RG_CMD_ALERT_CLR] =	0x0448,
> > +	[PWRAP_DEW_SPISLV_KEY] =	0x044a,
> > +};
> > +
> >  static const u32 mt6397_regs[] = {
> >  	[PWRAP_DEW_BASE] =		0xbc00,
> >  	[PWRAP_DEW_EVENT_OUT_EN] =	0xbc00,
> > @@ -977,6 +1036,7 @@ enum pmic_type {
> >  	PMIC_MT6351,
> >  	PMIC_MT6357,
> >  	PMIC_MT6358,
> > +	PMIC_MT6359,
> >  	PMIC_MT6380,
> >  	PMIC_MT6397,
> >  };
> > @@ -1757,6 +1817,15 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
> >  	.pwrap_write = pwrap_write16,
> >  };
> > 
> > +static const struct pwrap_slv_type pmic_mt6359 = {
> > +	.dew_regs = mt6359_regs,
> > +	.type = PMIC_MT6359,
> > +	.regmap = &pwrap_regmap_config16,
> > +	.caps = PWRAP_SLV_CAP_DUALIO,
> > +	.pwrap_read = pwrap_read16,
> > +	.pwrap_write = pwrap_write16,
> > +};
> > +
> >  static const struct pwrap_slv_type pmic_mt6380 = {
> >  	.dew_regs = NULL,
> >  	.type = PMIC_MT6380,
> > @@ -1790,6 +1859,9 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
> >  		.compatible = "mediatek,mt6358",
> >  		.data = &pmic_mt6358,
> >  	}, {
> > +		.compatible = "mediatek,mt6359",
> > +		.data = &pmic_mt6359,
> > +	}, {
> >  		/* The MT6380 PMIC only implements a regulator, so we bind it
> >  		 * directly instead of using a MFD.
> >  		 */
> > --
> > 1.8.1.1.dirty
> > 

Re: [PATCH 2/3] soc: mediatek: pwrap: add pwrap driver for MT6779 SoCs

[Argus Lin]

On Fri, 2019-10-04 at 01:26 +0200, Matthias Brugger wrote:
> 
> On 03/10/2019 09:48, Argus Lin wrote:
> > MT6779 is a highly integrated SoCs, it uses MT6359 for power
> > management. This patch adds pwrap driver to access MT6359.
> > Pwrap of MT6779 support dynamic priority meichanism, sequence
> 
> mechanism
I will fix it.
> 
> > monitor and starvation mechanism to make transaction more
> > reliable. WDT setting only need to init when it is zero,
> > otherwise keep current value. When setting interrupt enable
> 
> that's mt6779 specific?
It is common code. The PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN default value
is zero. Different project can have different value, I think we can
check if it has been initialized.

Two methods execute pwrap_init at different product line.
1. at bootloader(Smart phone/Tablet/Auto)
PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN has been initialized at bootloader,
we don't need to initialize it at kernel again.
2. at kernel(Some specific Tablet)
PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN is zero, just initialize them at
kernel.

> 
> > flag at pwrap_probe, read current setting then do logical OR
> > operation with wrp->master->int_en_all.
> 
> same here, only specific to mt6779?
same reason like why check WDT_UNIT == 0. What we do in the past is to
overwrite pwrap_int_en use the same value at bootloader.
If pwrap_int_en has been initialized, it is better to read current
value, OR new value at kernel then write new one.
> 
> > 
> > Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-pmic-wrap.c | 85 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 77 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > index c725315..fa8daf2 100644
> > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > @@ -497,6 +497,45 @@ enum pwrap_regs {
> >  	[PWRAP_DCM_DBC_PRD] =		0x1E0,
> >  };
> > 
> > +static int mt6779_regs[] = {
> > +	[PWRAP_MUX_SEL] =		0x0,
> > +	[PWRAP_WRAP_EN] =		0x4,
> > +	[PWRAP_DIO_EN] =		0x8,
> > +	[PWRAP_RDDMY] =			0x20,
> > +	[PWRAP_CSHEXT_WRITE] =		0x24,
> > +	[PWRAP_CSHEXT_READ] =		0x28,
> > +	[PWRAP_CSLEXT_WRITE] =		0x2C,
> > +	[PWRAP_CSLEXT_READ] =		0x30,
> > +	[PWRAP_EXT_CK_WRITE] =		0x34,
> > +	[PWRAP_STAUPD_CTRL] =		0x3C,
> > +	[PWRAP_STAUPD_GRPEN] =		0x40,
> > +	[PWRAP_EINT_STA0_ADR] =		0x44,
> > +	[PWRAP_HARB_HPRIO] =		0x68,
> > +	[PWRAP_HIPRIO_ARB_EN] =		0x6C,
> > +	[PWRAP_MAN_EN] =		0x7C,
> > +	[PWRAP_MAN_CMD] =		0x80,
> > +	[PWRAP_WACS0_EN] =		0x8C,
> > +	[PWRAP_WACS1_EN] =		0x94,
> > +	[PWRAP_WACS2_EN] =		0x9C,
> > +	[PWRAP_INIT_DONE0] =		0x90,
> > +	[PWRAP_INIT_DONE1] =		0x98,
> > +	[PWRAP_INIT_DONE2] =		0xA0,
> > +	[PWRAP_INT_EN] =		0xBC,
> > +	[PWRAP_INT_FLG_RAW] =		0xC0,
> > +	[PWRAP_INT_FLG] =		0xC4,
> > +	[PWRAP_INT_CLR] =		0xC8,
> > +	[PWRAP_INT1_EN] =		0xCC,
> > +	[PWRAP_INT1_FLG] =		0xD4,
> > +	[PWRAP_INT1_CLR] =		0xD8,
> > +	[PWRAP_TIMER_EN] =		0xF0,
> > +	[PWRAP_WDT_UNIT] =		0xF8,
> > +	[PWRAP_WDT_SRC_EN] =		0xFC,
> > +	[PWRAP_WDT_SRC_EN_1] =		0x100,
> > +	[PWRAP_WACS2_CMD] =		0xC20,
> > +	[PWRAP_WACS2_RDATA] =		0xC24,
> > +	[PWRAP_WACS2_VLDCLR] =		0xC28,
> > +};
> > +
> >  static int mt6797_regs[] = {
> >  	[PWRAP_MUX_SEL] =		0x0,
> >  	[PWRAP_WRAP_EN] =		0x4,
> > @@ -945,6 +984,7 @@ enum pmic_type {
> >  enum pwrap_type {
> >  	PWRAP_MT2701,
> >  	PWRAP_MT6765,
> > +	PWRAP_MT6779,
> >  	PWRAP_MT6797,
> >  	PWRAP_MT7622,
> >  	PWRAP_MT8135,
> > @@ -1377,6 +1417,7 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
> >  		break;
> >  	case PWRAP_MT2701:
> >  	case PWRAP_MT6765:
> > +	case PWRAP_MT6779:
> >  	case PWRAP_MT6797:
> >  	case PWRAP_MT8173:
> >  	case PWRAP_MT8516:
> > @@ -1468,8 +1509,10 @@ static int pwrap_init_security(struct pmic_wrapper *wrp)
> >  	pwrap_writel(wrp, 0x0, PWRAP_SIG_MODE);
> >  	pwrap_writel(wrp, wrp->slave->dew_regs[PWRAP_DEW_CRC_VAL],
> >  		     PWRAP_SIG_ADR);
> > -	pwrap_writel(wrp,
> > -		     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
> > +	if (pwrap_readl(wrp, PWRAP_HIPRIO_ARB_EN) == 0) {
> 
> Did you make sure that this holds for all SoCs that are supported by the driver?
> If so, why do we need this in mt6779 and didn't need that in the others?
> Even more, mt6779 does not have the security capbaility, so why do you change
> this code?
revert it.
> > +		pwrap_writel(wrp,
> > +			     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
> > +	}
> 
> I just realize that we write PWRAP_HIPRIO_ARB_EN twice if the slave supports
> security. Do we really need that?
> 
revert it.
pwrap_init_security and pwrap_init do not called at MT6779. I will
revert this change.
> > 
> >  	return 0;
> >  }
> > @@ -1581,7 +1624,10 @@ static int pwrap_init(struct pmic_wrapper *wrp)
> > 
> >  	pwrap_writel(wrp, 1, PWRAP_WRAP_EN);
> > 
> > -	pwrap_writel(wrp, wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
> > +	if (pwrap_readl(wrp, PWRAP_HIPRIO_ARB_EN) == 0) {
> > +		pwrap_writel(wrp,
> > +			     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
> > +	}
> > 
> >  	pwrap_writel(wrp, 1, PWRAP_WACS2_EN);
> > 
> > @@ -1783,6 +1829,19 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
> >  	.init_soc_specific = NULL,
> >  };
> > 
> > +static const struct pmic_wrapper_type pwrap_mt6779 = {
> > +	.regs = mt6779_regs,
> > +	.type = PWRAP_MT6779,
> > +	.arb_en_all = 0,
> > +	.int_en_all = 0,
> > +	.int1_en_all = 0,
> > +	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
> > +	.wdt_src = 0,
> > +	.caps = 0,
> > +	.init_reg_clock = pwrap_common_init_reg_clock,
> > +	.init_soc_specific = NULL,
> > +};
> > +
> >  static const struct pmic_wrapper_type pwrap_mt6797 = {
> >  	.regs = mt6797_regs,
> >  	.type = PWRAP_MT6797,
> > @@ -1868,6 +1927,9 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
> >  		.compatible = "mediatek,mt6765-pwrap",
> >  		.data = &pwrap_mt6765,
> >  	}, {
> > +		.compatible = "mediatek,mt6779-pwrap",
> > +		.data = &pwrap_mt6779,
> > +	}, {
> >  		.compatible = "mediatek,mt6797-pwrap",
> >  		.data = &pwrap_mt6797,
> >  	}, {
> > @@ -1898,6 +1960,7 @@ static int pwrap_probe(struct platform_device *pdev)
> >  	struct device_node *np = pdev->dev.of_node;
> >  	const struct of_device_id *of_slave_id = NULL;
> >  	struct resource *res;
> > +	u32 int_en;
> > 
> >  	if (np->child)
> >  		of_slave_id = of_match_node(of_slave_match_tbl, np->child);
> > @@ -1995,23 +2058,29 @@ static int pwrap_probe(struct platform_device *pdev)
> >  	}
> > 
> >  	/* Initialize watchdog, may not be done by the bootloader */
> > -	pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
> > +	if (pwrap_readl(wrp, PWRAP_WDT_UNIT) == 0)
> 
> Same here, why do we need it in mt6779 and did you test if it does not break any
> older SoC?
It is common code. The PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN default value
is zero. Different project can have different value, I think we can
check if it has been initialized.

Two methods execute pwrap_init at different product line.
1. at bootloader(Smart phone/Tablet/Auto)
PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN has been initialized at bootloader,
we don't need to initialize it at kernel again.
2. at kernel(Some specific Tablet)
PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN is zero, just initialize them at
kernel.
> 
> > +		pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
> >  	/*
> >  	 * Since STAUPD was not used on mt8173 platform,
> >  	 * so STAUPD of WDT_SRC which should be turned off
> >  	 */
> > -	pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
> > +	if (pwrap_readl(wrp, PWRAP_WDT_SRC_EN) == 0)
> > +		pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
> >  	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_WDT_SRC1))
> >  		pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN_1);
> > 
> >  	pwrap_writel(wrp, 0x1, PWRAP_TIMER_EN);
> > -	pwrap_writel(wrp, wrp->master->int_en_all, PWRAP_INT_EN);
> > +	int_en = pwrap_readl(wrp, PWRAP_INT_EN);
> > +	pwrap_writel(wrp, (int_en) | (wrp->master->int_en_all), PWRAP_INT_EN);
> 
> Looks ok to me, is it a bug fix, or only needed for mt6779?
It is common code.
> 
> >  	/*
> >  	 * We add INT1 interrupt to handle starvation and request exception
> >  	 * If we support it, we should enable it here.
> >  	 */
> > -	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
> > -		pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
> > +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN)) {
> > +		int_en = pwrap_readl(wrp, PWRAP_INT1_EN);
> > +		pwrap_writel(wrp, (int_en) | wrp->master->int1_en_all,
> > +			     PWRAP_INT1_EN);
> > +	}
> > 
> >  	irq = platform_get_irq(pdev, 0);
> >  	ret = devm_request_irq(wrp->dev, irq, pwrap_interrupt,
> > --
> > 1.8.1.1.dirty
> > 

Re: [PATCH 1/3] dt-bindings: pwrap: mediatek: add pwrap support for MT6779

[Rob Herring]

On Thu, 3 Oct 2019 15:48:19 +0800, Argus Lin wrote:
> Add binding document of pwrap for MT6779 SoCs.
> 
> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> ---
>  Documentation/devicetree/bindings/soc/mediatek/pwrap.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/3] soc: mediatek: pwrap: add pwrap driver for MT6779 SoCs

[Argus Lin]

dear matthias:
sorry to disturb you.

Based on your opinion, I had updated my comment.

If you have any concern, please let me know.
thanks.

B.R.
Argus

On Mon, 2019-10-14 at 14:04 +0800, Argus Lin wrote:
> On Fri, 2019-10-04 at 01:26 +0200, Matthias Brugger wrote:
> > 
> > On 03/10/2019 09:48, Argus Lin wrote:
> > > MT6779 is a highly integrated SoCs, it uses MT6359 for power
> > > management. This patch adds pwrap driver to access MT6359.
> > > Pwrap of MT6779 support dynamic priority meichanism, sequence
> > 
> > mechanism
> I will fix it.
> > 
> > > monitor and starvation mechanism to make transaction more
> > > reliable. WDT setting only need to init when it is zero,
> > > otherwise keep current value. When setting interrupt enable
> > 
> > that's mt6779 specific?
> It is common code. The PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN default value
> is zero. Different project can have different value, I think we can
> check if it has been initialized.
> 
> Two methods execute pwrap_init at different product line.
> 1. at bootloader(Smart phone/Tablet/Auto)
> PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN has been initialized at bootloader,
> we don't need to initialize it at kernel again.
> 2. at kernel(Some specific Tablet)
> PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN is zero, just initialize them at
> kernel.
> 
> > 
> > > flag at pwrap_probe, read current setting then do logical OR
> > > operation with wrp->master->int_en_all.
> > 
> > same here, only specific to mt6779?
> same reason like why check WDT_UNIT == 0. What we do in the past is to
> overwrite pwrap_int_en use the same value at bootloader.
> If pwrap_int_en has been initialized, it is better to read current
> value, OR new value at kernel then write new one.
> > 
> > > 
> > > Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> > > ---
> > >  drivers/soc/mediatek/mtk-pmic-wrap.c | 85 ++++++++++++++++++++++++++++++++----
> > >  1 file changed, 77 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > > index c725315..fa8daf2 100644
> > > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> > > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > > @@ -497,6 +497,45 @@ enum pwrap_regs {
> > >  	[PWRAP_DCM_DBC_PRD] =		0x1E0,
> > >  };
> > > 
> > > +static int mt6779_regs[] = {
> > > +	[PWRAP_MUX_SEL] =		0x0,
> > > +	[PWRAP_WRAP_EN] =		0x4,
> > > +	[PWRAP_DIO_EN] =		0x8,
> > > +	[PWRAP_RDDMY] =			0x20,
> > > +	[PWRAP_CSHEXT_WRITE] =		0x24,
> > > +	[PWRAP_CSHEXT_READ] =		0x28,
> > > +	[PWRAP_CSLEXT_WRITE] =		0x2C,
> > > +	[PWRAP_CSLEXT_READ] =		0x30,
> > > +	[PWRAP_EXT_CK_WRITE] =		0x34,
> > > +	[PWRAP_STAUPD_CTRL] =		0x3C,
> > > +	[PWRAP_STAUPD_GRPEN] =		0x40,
> > > +	[PWRAP_EINT_STA0_ADR] =		0x44,
> > > +	[PWRAP_HARB_HPRIO] =		0x68,
> > > +	[PWRAP_HIPRIO_ARB_EN] =		0x6C,
> > > +	[PWRAP_MAN_EN] =		0x7C,
> > > +	[PWRAP_MAN_CMD] =		0x80,
> > > +	[PWRAP_WACS0_EN] =		0x8C,
> > > +	[PWRAP_WACS1_EN] =		0x94,
> > > +	[PWRAP_WACS2_EN] =		0x9C,
> > > +	[PWRAP_INIT_DONE0] =		0x90,
> > > +	[PWRAP_INIT_DONE1] =		0x98,
> > > +	[PWRAP_INIT_DONE2] =		0xA0,
> > > +	[PWRAP_INT_EN] =		0xBC,
> > > +	[PWRAP_INT_FLG_RAW] =		0xC0,
> > > +	[PWRAP_INT_FLG] =		0xC4,
> > > +	[PWRAP_INT_CLR] =		0xC8,
> > > +	[PWRAP_INT1_EN] =		0xCC,
> > > +	[PWRAP_INT1_FLG] =		0xD4,
> > > +	[PWRAP_INT1_CLR] =		0xD8,
> > > +	[PWRAP_TIMER_EN] =		0xF0,
> > > +	[PWRAP_WDT_UNIT] =		0xF8,
> > > +	[PWRAP_WDT_SRC_EN] =		0xFC,
> > > +	[PWRAP_WDT_SRC_EN_1] =		0x100,
> > > +	[PWRAP_WACS2_CMD] =		0xC20,
> > > +	[PWRAP_WACS2_RDATA] =		0xC24,
> > > +	[PWRAP_WACS2_VLDCLR] =		0xC28,
> > > +};
> > > +
> > >  static int mt6797_regs[] = {
> > >  	[PWRAP_MUX_SEL] =		0x0,
> > >  	[PWRAP_WRAP_EN] =		0x4,
> > > @@ -945,6 +984,7 @@ enum pmic_type {
> > >  enum pwrap_type {
> > >  	PWRAP_MT2701,
> > >  	PWRAP_MT6765,
> > > +	PWRAP_MT6779,
> > >  	PWRAP_MT6797,
> > >  	PWRAP_MT7622,
> > >  	PWRAP_MT8135,
> > > @@ -1377,6 +1417,7 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
> > >  		break;
> > >  	case PWRAP_MT2701:
> > >  	case PWRAP_MT6765:
> > > +	case PWRAP_MT6779:
> > >  	case PWRAP_MT6797:
> > >  	case PWRAP_MT8173:
> > >  	case PWRAP_MT8516:
> > > @@ -1468,8 +1509,10 @@ static int pwrap_init_security(struct pmic_wrapper *wrp)
> > >  	pwrap_writel(wrp, 0x0, PWRAP_SIG_MODE);
> > >  	pwrap_writel(wrp, wrp->slave->dew_regs[PWRAP_DEW_CRC_VAL],
> > >  		     PWRAP_SIG_ADR);
> > > -	pwrap_writel(wrp,
> > > -		     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
> > > +	if (pwrap_readl(wrp, PWRAP_HIPRIO_ARB_EN) == 0) {
> > 
> > Did you make sure that this holds for all SoCs that are supported by the driver?
> > If so, why do we need this in mt6779 and didn't need that in the others?
> > Even more, mt6779 does not have the security capbaility, so why do you change
> > this code?
> revert it.
> > > +		pwrap_writel(wrp,
> > > +			     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
> > > +	}
> > 
> > I just realize that we write PWRAP_HIPRIO_ARB_EN twice if the slave supports
> > security. Do we really need that?
> > 
> revert it.
> pwrap_init_security and pwrap_init do not called at MT6779. I will
> revert this change.
> > > 
> > >  	return 0;
> > >  }
> > > @@ -1581,7 +1624,10 @@ static int pwrap_init(struct pmic_wrapper *wrp)
> > > 
> > >  	pwrap_writel(wrp, 1, PWRAP_WRAP_EN);
> > > 
> > > -	pwrap_writel(wrp, wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
> > > +	if (pwrap_readl(wrp, PWRAP_HIPRIO_ARB_EN) == 0) {
> > > +		pwrap_writel(wrp,
> > > +			     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
> > > +	}
> > > 
> > >  	pwrap_writel(wrp, 1, PWRAP_WACS2_EN);
> > > 
> > > @@ -1783,6 +1829,19 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
> > >  	.init_soc_specific = NULL,
> > >  };
> > > 
> > > +static const struct pmic_wrapper_type pwrap_mt6779 = {
> > > +	.regs = mt6779_regs,
> > > +	.type = PWRAP_MT6779,
> > > +	.arb_en_all = 0,
> > > +	.int_en_all = 0,
> > > +	.int1_en_all = 0,
> > > +	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
> > > +	.wdt_src = 0,
> > > +	.caps = 0,
> > > +	.init_reg_clock = pwrap_common_init_reg_clock,
> > > +	.init_soc_specific = NULL,
> > > +};
> > > +
> > >  static const struct pmic_wrapper_type pwrap_mt6797 = {
> > >  	.regs = mt6797_regs,
> > >  	.type = PWRAP_MT6797,
> > > @@ -1868,6 +1927,9 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
> > >  		.compatible = "mediatek,mt6765-pwrap",
> > >  		.data = &pwrap_mt6765,
> > >  	}, {
> > > +		.compatible = "mediatek,mt6779-pwrap",
> > > +		.data = &pwrap_mt6779,
> > > +	}, {
> > >  		.compatible = "mediatek,mt6797-pwrap",
> > >  		.data = &pwrap_mt6797,
> > >  	}, {
> > > @@ -1898,6 +1960,7 @@ static int pwrap_probe(struct platform_device *pdev)
> > >  	struct device_node *np = pdev->dev.of_node;
> > >  	const struct of_device_id *of_slave_id = NULL;
> > >  	struct resource *res;
> > > +	u32 int_en;
> > > 
> > >  	if (np->child)
> > >  		of_slave_id = of_match_node(of_slave_match_tbl, np->child);
> > > @@ -1995,23 +2058,29 @@ static int pwrap_probe(struct platform_device *pdev)
> > >  	}
> > > 
> > >  	/* Initialize watchdog, may not be done by the bootloader */
> > > -	pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
> > > +	if (pwrap_readl(wrp, PWRAP_WDT_UNIT) == 0)
> > 
> > Same here, why do we need it in mt6779 and did you test if it does not break any
> > older SoC?
> It is common code. The PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN default value
> is zero. Different project can have different value, I think we can
> check if it has been initialized.
> 
> Two methods execute pwrap_init at different product line.
> 1. at bootloader(Smart phone/Tablet/Auto)
> PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN has been initialized at bootloader,
> we don't need to initialize it at kernel again.
> 2. at kernel(Some specific Tablet)
> PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN is zero, just initialize them at
> kernel.
> > 
> > > +		pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
> > >  	/*
> > >  	 * Since STAUPD was not used on mt8173 platform,
> > >  	 * so STAUPD of WDT_SRC which should be turned off
> > >  	 */
> > > -	pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
> > > +	if (pwrap_readl(wrp, PWRAP_WDT_SRC_EN) == 0)
> > > +		pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
> > >  	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_WDT_SRC1))
> > >  		pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN_1);
> > > 
> > >  	pwrap_writel(wrp, 0x1, PWRAP_TIMER_EN);
> > > -	pwrap_writel(wrp, wrp->master->int_en_all, PWRAP_INT_EN);
> > > +	int_en = pwrap_readl(wrp, PWRAP_INT_EN);
> > > +	pwrap_writel(wrp, (int_en) | (wrp->master->int_en_all), PWRAP_INT_EN);
> > 
> > Looks ok to me, is it a bug fix, or only needed for mt6779?
> It is common code.
> > 
> > >  	/*
> > >  	 * We add INT1 interrupt to handle starvation and request exception
> > >  	 * If we support it, we should enable it here.
> > >  	 */
> > > -	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
> > > -		pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
> > > +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN)) {
> > > +		int_en = pwrap_readl(wrp, PWRAP_INT1_EN);
> > > +		pwrap_writel(wrp, (int_en) | wrp->master->int1_en_all,
> > > +			     PWRAP_INT1_EN);
> > > +	}
> > > 
> > >  	irq = platform_get_irq(pdev, 0);
> > >  	ret = devm_request_irq(wrp->dev, irq, pwrap_interrupt,
> > > --
> > > 1.8.1.1.dirty
> > > 
> 

Re: [PATCH 3/3] soc: mediatek: pwrap: add support for MT6359 PMIC

[Argus Lin]

dear matthias:
sorry to disturb you.

Based on your opinion, I had updated my comment.

If you have any concern, please let me know.
thanks.

B.R.
Argus

On Mon, 2019-10-14 at 13:17 +0800, Argus Lin wrote:
> On Fri, 2019-10-04 at 01:27 +0200, Matthias Brugger wrote:
> > 
> > On 03/10/2019 09:48, Argus Lin wrote:
> > > MT6359 is a new power management IC and it is used for
> > > MT6779 SoCs. To define mt6359_regs for pmic register mapping
> > > and pmic_mt6359 for accessing register.
> > > 
> > > Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> > > ---
> > >  drivers/soc/mediatek/mtk-pmic-wrap.c | 72 ++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 72 insertions(+)
> > > 
> > > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > > index fa8daf2..dd04318 100644
> > > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> > > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > > @@ -111,6 +111,29 @@ enum dew_regs {
> > >  	PWRAP_RG_SPI_CON13,
> > >  	PWRAP_SPISLV_KEY,
> > > 
> > > +	/* MT6359 only regs */
> > > +	PWRAP_DEW_CRC_SWRST,
> > > +	PWRAP_DEW_RG_EN_RECORD,
> > > +	PWRAP_DEW_RECORD_CMD0,
> > > +	PWRAP_DEW_RECORD_CMD1,
> > > +	PWRAP_DEW_RECORD_CMD2,
> > > +	PWRAP_DEW_RECORD_CMD3,
> > > +	PWRAP_DEW_RECORD_CMD4,
> > > +	PWRAP_DEW_RECORD_CMD5,
> > > +	PWRAP_DEW_RECORD_WDATA0,
> > > +	PWRAP_DEW_RECORD_WDATA1,
> > > +	PWRAP_DEW_RECORD_WDATA2,
> > > +	PWRAP_DEW_RECORD_WDATA3,
> > > +	PWRAP_DEW_RECORD_WDATA4,
> > > +	PWRAP_DEW_RECORD_WDATA5,
> > > +	PWRAP_DEW_RG_ADDR_TARGET,
> > > +	PWRAP_DEW_RG_ADDR_MASK,
> > > +	PWRAP_DEW_RG_WDATA_TARGET,
> > > +	PWRAP_DEW_RG_WDATA_MASK,
> > > +	PWRAP_DEW_RG_SPI_RECORD_CLR,
> > > +	PWRAP_DEW_RG_CMD_ALERT_CLR,
> > > +	PWRAP_DEW_SPISLV_KEY,
> > 
> > That looks like PWRAP_SPISLV_KEY from MT6358.
> > 
> > Regards,
> > Matthias
> > 
> Yes, I think I can reuse the PWRAP_SPISLV_KEY of MT6358.
> 
> B.R.
> Argus
> > > +
> > >  	/* MT6397 only regs */
> > >  	PWRAP_DEW_EVENT_OUT_EN,
> > >  	PWRAP_DEW_EVENT_SRC_EN,
> > > @@ -197,6 +220,42 @@ enum dew_regs {
> > >  	[PWRAP_SPISLV_KEY] =		0x044a,
> > >  };
> > > 
> > > +static const u32 mt6359_regs[] = {
> > > +	[PWRAP_DEW_RG_EN_RECORD] =	0x040a,
> > > +	[PWRAP_DEW_DIO_EN] =		0x040c,
> > > +	[PWRAP_DEW_READ_TEST] =		0x040e,
> > > +	[PWRAP_DEW_WRITE_TEST] =	0x0410,
> > > +	[PWRAP_DEW_CRC_SWRST] =		0x0412,
> > > +	[PWRAP_DEW_CRC_EN] =		0x0414,
> > > +	[PWRAP_DEW_CRC_VAL] =		0x0416,
> > > +	[PWRAP_DEW_CIPHER_KEY_SEL] =	0x0418,
> > > +	[PWRAP_DEW_CIPHER_IV_SEL] =	0x041a,
> > > +	[PWRAP_DEW_CIPHER_EN] =		0x041c,
> > > +	[PWRAP_DEW_CIPHER_RDY] =	0x041e,
> > > +	[PWRAP_DEW_CIPHER_MODE] =	0x0420,
> > > +	[PWRAP_DEW_CIPHER_SWRST] =	0x0422,
> > > +	[PWRAP_DEW_RDDMY_NO] =		0x0424,
> > > +	[PWRAP_DEW_RECORD_CMD0] =	0x0428,
> > > +	[PWRAP_DEW_RECORD_CMD1] =	0x042a,
> > > +	[PWRAP_DEW_RECORD_CMD2] =	0x042c,
> > > +	[PWRAP_DEW_RECORD_CMD3] =	0x042e,
> > > +	[PWRAP_DEW_RECORD_CMD4] =	0x0430,
> > > +	[PWRAP_DEW_RECORD_CMD5] =	0x0432,
> > > +	[PWRAP_DEW_RECORD_WDATA0] =	0x0434,
> > > +	[PWRAP_DEW_RECORD_WDATA1] =	0x0436,
> > > +	[PWRAP_DEW_RECORD_WDATA2] =	0x0438,
> > > +	[PWRAP_DEW_RECORD_WDATA3] =	0x043a,
> > > +	[PWRAP_DEW_RECORD_WDATA4] =	0x043c,
> > > +	[PWRAP_DEW_RECORD_WDATA5] =	0x043e,
> > > +	[PWRAP_DEW_RG_ADDR_TARGET] =	0x0440,
> > > +	[PWRAP_DEW_RG_ADDR_MASK] =	0x0442,
> > > +	[PWRAP_DEW_RG_WDATA_TARGET] =	0x0444,
> > > +	[PWRAP_DEW_RG_WDATA_MASK] =	0x0446,
> > > +	[PWRAP_DEW_RG_SPI_RECORD_CLR] =	0x0448,
> > > +	[PWRAP_DEW_RG_CMD_ALERT_CLR] =	0x0448,
> > > +	[PWRAP_DEW_SPISLV_KEY] =	0x044a,
> > > +};
> > > +
> > >  static const u32 mt6397_regs[] = {
> > >  	[PWRAP_DEW_BASE] =		0xbc00,
> > >  	[PWRAP_DEW_EVENT_OUT_EN] =	0xbc00,
> > > @@ -977,6 +1036,7 @@ enum pmic_type {
> > >  	PMIC_MT6351,
> > >  	PMIC_MT6357,
> > >  	PMIC_MT6358,
> > > +	PMIC_MT6359,
> > >  	PMIC_MT6380,
> > >  	PMIC_MT6397,
> > >  };
> > > @@ -1757,6 +1817,15 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
> > >  	.pwrap_write = pwrap_write16,
> > >  };
> > > 
> > > +static const struct pwrap_slv_type pmic_mt6359 = {
> > > +	.dew_regs = mt6359_regs,
> > > +	.type = PMIC_MT6359,
> > > +	.regmap = &pwrap_regmap_config16,
> > > +	.caps = PWRAP_SLV_CAP_DUALIO,
> > > +	.pwrap_read = pwrap_read16,
> > > +	.pwrap_write = pwrap_write16,
> > > +};
> > > +
> > >  static const struct pwrap_slv_type pmic_mt6380 = {
> > >  	.dew_regs = NULL,
> > >  	.type = PMIC_MT6380,
> > > @@ -1790,6 +1859,9 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
> > >  		.compatible = "mediatek,mt6358",
> > >  		.data = &pmic_mt6358,
> > >  	}, {
> > > +		.compatible = "mediatek,mt6359",
> > > +		.data = &pmic_mt6359,
> > > +	}, {
> > >  		/* The MT6380 PMIC only implements a regulator, so we bind it
> > >  		 * directly instead of using a MFD.
> > >  		 */
> > > --
> > > 1.8.1.1.dirty
> > > 
> 

Re: [PATCH 2/3] soc: mediatek: pwrap: add pwrap driver for MT6779 SoCs

[Matthias Brugger]

On 14/10/2019 08:04, Argus Lin wrote:
> On Fri, 2019-10-04 at 01:26 +0200, Matthias Brugger wrote:
>>
>> On 03/10/2019 09:48, Argus Lin wrote:
>>> MT6779 is a highly integrated SoCs, it uses MT6359 for power
>>> management. This patch adds pwrap driver to access MT6359.
>>> Pwrap of MT6779 support dynamic priority meichanism, sequence
>>
>> mechanism
> I will fix it.
>>
>>> monitor and starvation mechanism to make transaction more
>>> reliable. WDT setting only need to init when it is zero,
>>> otherwise keep current value. When setting interrupt enable
>>
>> that's mt6779 specific?
> It is common code. The PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN default value
> is zero. Different project can have different value, I think we can
> check if it has been initialized.
> 
> Two methods execute pwrap_init at different product line.
> 1. at bootloader(Smart phone/Tablet/Auto)
> PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN has been initialized at bootloader,
> we don't need to initialize it at kernel again.
> 2. at kernel(Some specific Tablet)
> PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN is zero, just initialize them at
> kernel.
> 

Well normally what we do at kernel level, we just initialize the devices with
the needed value, even if it already was initialized by the bootloader. Does
this break anything if we initialize the device a second time? The reason behind
this is, that it makes the driver easier to read and independent from any
bootloader changes.

>>
>>> flag at pwrap_probe, read current setting then do logical OR
>>> operation with wrp->master->int_en_all.
>>
>> same here, only specific to mt6779?
> same reason like why check WDT_UNIT == 0. What we do in the past is to
> overwrite pwrap_int_en use the same value at bootloader.
> If pwrap_int_en has been initialized, it is better to read current
> value, OR new value at kernel then write new one.
>>
>>>
>>> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
>>> ---
>>>  drivers/soc/mediatek/mtk-pmic-wrap.c | 85 ++++++++++++++++++++++++++++++++----
>>>  1 file changed, 77 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
>>> index c725315..fa8daf2 100644
>>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
>>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
>>> @@ -497,6 +497,45 @@ enum pwrap_regs {
>>>  	[PWRAP_DCM_DBC_PRD] =		0x1E0,
>>>  };
>>>
>>> +static int mt6779_regs[] = {
>>> +	[PWRAP_MUX_SEL] =		0x0,
>>> +	[PWRAP_WRAP_EN] =		0x4,
>>> +	[PWRAP_DIO_EN] =		0x8,
>>> +	[PWRAP_RDDMY] =			0x20,
>>> +	[PWRAP_CSHEXT_WRITE] =		0x24,
>>> +	[PWRAP_CSHEXT_READ] =		0x28,
>>> +	[PWRAP_CSLEXT_WRITE] =		0x2C,
>>> +	[PWRAP_CSLEXT_READ] =		0x30,
>>> +	[PWRAP_EXT_CK_WRITE] =		0x34,
>>> +	[PWRAP_STAUPD_CTRL] =		0x3C,
>>> +	[PWRAP_STAUPD_GRPEN] =		0x40,
>>> +	[PWRAP_EINT_STA0_ADR] =		0x44,
>>> +	[PWRAP_HARB_HPRIO] =		0x68,
>>> +	[PWRAP_HIPRIO_ARB_EN] =		0x6C,
>>> +	[PWRAP_MAN_EN] =		0x7C,
>>> +	[PWRAP_MAN_CMD] =		0x80,
>>> +	[PWRAP_WACS0_EN] =		0x8C,
>>> +	[PWRAP_WACS1_EN] =		0x94,
>>> +	[PWRAP_WACS2_EN] =		0x9C,
>>> +	[PWRAP_INIT_DONE0] =		0x90,
>>> +	[PWRAP_INIT_DONE1] =		0x98,
>>> +	[PWRAP_INIT_DONE2] =		0xA0,
>>> +	[PWRAP_INT_EN] =		0xBC,
>>> +	[PWRAP_INT_FLG_RAW] =		0xC0,
>>> +	[PWRAP_INT_FLG] =		0xC4,
>>> +	[PWRAP_INT_CLR] =		0xC8,
>>> +	[PWRAP_INT1_EN] =		0xCC,
>>> +	[PWRAP_INT1_FLG] =		0xD4,
>>> +	[PWRAP_INT1_CLR] =		0xD8,
>>> +	[PWRAP_TIMER_EN] =		0xF0,
>>> +	[PWRAP_WDT_UNIT] =		0xF8,
>>> +	[PWRAP_WDT_SRC_EN] =		0xFC,
>>> +	[PWRAP_WDT_SRC_EN_1] =		0x100,
>>> +	[PWRAP_WACS2_CMD] =		0xC20,
>>> +	[PWRAP_WACS2_RDATA] =		0xC24,
>>> +	[PWRAP_WACS2_VLDCLR] =		0xC28,
>>> +};
>>> +
>>>  static int mt6797_regs[] = {
>>>  	[PWRAP_MUX_SEL] =		0x0,
>>>  	[PWRAP_WRAP_EN] =		0x4,
>>> @@ -945,6 +984,7 @@ enum pmic_type {
>>>  enum pwrap_type {
>>>  	PWRAP_MT2701,
>>>  	PWRAP_MT6765,
>>> +	PWRAP_MT6779,
>>>  	PWRAP_MT6797,
>>>  	PWRAP_MT7622,
>>>  	PWRAP_MT8135,
>>> @@ -1377,6 +1417,7 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>>>  		break;
>>>  	case PWRAP_MT2701:
>>>  	case PWRAP_MT6765:
>>> +	case PWRAP_MT6779:
>>>  	case PWRAP_MT6797:
>>>  	case PWRAP_MT8173:
>>>  	case PWRAP_MT8516:
>>> @@ -1468,8 +1509,10 @@ static int pwrap_init_security(struct pmic_wrapper *wrp)
>>>  	pwrap_writel(wrp, 0x0, PWRAP_SIG_MODE);
>>>  	pwrap_writel(wrp, wrp->slave->dew_regs[PWRAP_DEW_CRC_VAL],
>>>  		     PWRAP_SIG_ADR);
>>> -	pwrap_writel(wrp,
>>> -		     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
>>> +	if (pwrap_readl(wrp, PWRAP_HIPRIO_ARB_EN) == 0) {
>>
>> Did you make sure that this holds for all SoCs that are supported by the driver?
>> If so, why do we need this in mt6779 and didn't need that in the others?
>> Even more, mt6779 does not have the security capbaility, so why do you change
>> this code?
> revert it.
>>> +		pwrap_writel(wrp,
>>> +			     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
>>> +	}
>>
>> I just realize that we write PWRAP_HIPRIO_ARB_EN twice if the slave supports
>> security. Do we really need that?
>>
> revert it.
> pwrap_init_security and pwrap_init do not called at MT6779. I will
> revert this change.
>>>
>>>  	return 0;
>>>  }
>>> @@ -1581,7 +1624,10 @@ static int pwrap_init(struct pmic_wrapper *wrp)
>>>
>>>  	pwrap_writel(wrp, 1, PWRAP_WRAP_EN);
>>>
>>> -	pwrap_writel(wrp, wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
>>> +	if (pwrap_readl(wrp, PWRAP_HIPRIO_ARB_EN) == 0) {
>>> +		pwrap_writel(wrp,
>>> +			     wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN);
>>> +	}
>>>
>>>  	pwrap_writel(wrp, 1, PWRAP_WACS2_EN);
>>>
>>> @@ -1783,6 +1829,19 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
>>>  	.init_soc_specific = NULL,
>>>  };
>>>
>>> +static const struct pmic_wrapper_type pwrap_mt6779 = {
>>> +	.regs = mt6779_regs,
>>> +	.type = PWRAP_MT6779,
>>> +	.arb_en_all = 0,
>>> +	.int_en_all = 0,
>>> +	.int1_en_all = 0,
>>> +	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
>>> +	.wdt_src = 0,
>>> +	.caps = 0,
>>> +	.init_reg_clock = pwrap_common_init_reg_clock,
>>> +	.init_soc_specific = NULL,
>>> +};
>>> +
>>>  static const struct pmic_wrapper_type pwrap_mt6797 = {
>>>  	.regs = mt6797_regs,
>>>  	.type = PWRAP_MT6797,
>>> @@ -1868,6 +1927,9 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
>>>  		.compatible = "mediatek,mt6765-pwrap",
>>>  		.data = &pwrap_mt6765,
>>>  	}, {
>>> +		.compatible = "mediatek,mt6779-pwrap",
>>> +		.data = &pwrap_mt6779,
>>> +	}, {
>>>  		.compatible = "mediatek,mt6797-pwrap",
>>>  		.data = &pwrap_mt6797,
>>>  	}, {
>>> @@ -1898,6 +1960,7 @@ static int pwrap_probe(struct platform_device *pdev)
>>>  	struct device_node *np = pdev->dev.of_node;
>>>  	const struct of_device_id *of_slave_id = NULL;
>>>  	struct resource *res;
>>> +	u32 int_en;
>>>
>>>  	if (np->child)
>>>  		of_slave_id = of_match_node(of_slave_match_tbl, np->child);
>>> @@ -1995,23 +2058,29 @@ static int pwrap_probe(struct platform_device *pdev)
>>>  	}
>>>
>>>  	/* Initialize watchdog, may not be done by the bootloader */
>>> -	pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
>>> +	if (pwrap_readl(wrp, PWRAP_WDT_UNIT) == 0)
>>
>> Same here, why do we need it in mt6779 and did you test if it does not break any
>> older SoC?
> It is common code. The PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN default value
> is zero. Different project can have different value, I think we can
> check if it has been initialized.
> 
> Two methods execute pwrap_init at different product line.
> 1. at bootloader(Smart phone/Tablet/Auto)
> PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN has been initialized at bootloader,
> we don't need to initialize it at kernel again.

Same here, if it's just a "we don't need" and it does not break anything, then I
prefer to just initialize it again.

> 2. at kernel(Some specific Tablet)
> PWRAP_WDT_UNIT and PWRAP_WDT_SRC_EN is zero, just initialize them at
> kernel.
>>
>>> +		pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
>>>  	/*
>>>  	 * Since STAUPD was not used on mt8173 platform,
>>>  	 * so STAUPD of WDT_SRC which should be turned off
>>>  	 */
>>> -	pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
>>> +	if (pwrap_readl(wrp, PWRAP_WDT_SRC_EN) == 0)
>>> +		pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
>>>  	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_WDT_SRC1))
>>>  		pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN_1);
>>>
>>>  	pwrap_writel(wrp, 0x1, PWRAP_TIMER_EN);
>>> -	pwrap_writel(wrp, wrp->master->int_en_all, PWRAP_INT_EN);
>>> +	int_en = pwrap_readl(wrp, PWRAP_INT_EN);
>>> +	pwrap_writel(wrp, (int_en) | (wrp->master->int_en_all), PWRAP_INT_EN);
>>
>> Looks ok to me, is it a bug fix, or only needed for mt6779?
> It is common code.

Ok, I understand that's not a bug fix, but it makes the code more robust. As it
is independent from mt6779, please provide it as a separate patch.

Regards,
Matthias

>>
>>>  	/*
>>>  	 * We add INT1 interrupt to handle starvation and request exception
>>>  	 * If we support it, we should enable it here.
>>>  	 */
>>> -	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
>>> -		pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
>>> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN)) {
>>> +		int_en = pwrap_readl(wrp, PWRAP_INT1_EN);
>>> +		pwrap_writel(wrp, (int_en) | wrp->master->int1_en_all,
>>> +			     PWRAP_INT1_EN);
>>> +	}
>>>
>>>  	irq = platform_get_irq(pdev, 0);
>>>  	ret = devm_request_irq(wrp->dev, irq, pwrap_interrupt,
>>> --
>>> 1.8.1.1.dirty
>>>
> 
>