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

From: Argus Lin
Date: Mon Oct 14 2019 - 02:04:42 EST

Next message: Alexey Budankov: "[PATCH v1] perf/core: fix restoring of Intel LBR call stack on a context switch"
Previous message: Wu Hao: "[PATCH v7 2/3] fpga: dfl: fme: add thermal management support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Alexey Budankov: "[PATCH v1] perf/core: fix restoring of Intel LBR call stack on a context switch"
Previous message: Wu Hao: "[PATCH v7 2/3] fpga: dfl: fme: add thermal management support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]