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

From: Matthias Brugger
Date: Thu Oct 03 2019 - 19:26:58 EST

Next message: Matthias Brugger: "Re: [PATCH 3/3] soc: mediatek: pwrap: add support for MT6359 PMIC"
Previous message: Stephen Boyd: "Re: [PATCH] base: soc: Handle custom soc information sysfs entries"
In reply to: Argus Lin: "[PATCH 2/3] soc: mediatek: pwrap: add pwrap driver for MT6779 SoCs"
Next in thread: Argus Lin: "[PATCH 3/3] soc: mediatek: pwrap: add support for MT6359 PMIC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxx>
> ---
> 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
>

Next message: Matthias Brugger: "Re: [PATCH 3/3] soc: mediatek: pwrap: add support for MT6359 PMIC"
Previous message: Stephen Boyd: "Re: [PATCH] base: soc: Handle custom soc information sysfs entries"
In reply to: Argus Lin: "[PATCH 2/3] soc: mediatek: pwrap: add pwrap driver for MT6779 SoCs"
Next in thread: Argus Lin: "[PATCH 3/3] soc: mediatek: pwrap: add support for MT6359 PMIC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]