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

From: Argus Lin
Date: Mon Nov 04 2019 - 03:22:49 EST

Next message: Argus Lin: "Re: [PATCH 3/3] soc: mediatek: pwrap: add support for MT6359 PMIC"
Previous message: Peng Fan: "RE: [PATCH] clk: imx: imx8mn: add IMX8MN_CLK_SAI7_IPG clk"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@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: Argus Lin: "Re: [PATCH 3/3] soc: mediatek: pwrap: add support for MT6359 PMIC"
Previous message: Peng Fan: "RE: [PATCH] clk: imx: imx8mn: add IMX8MN_CLK_SAI7_IPG clk"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]