From: Argus Lin <argus.lin@mediatek.com>
To: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
devicetree@vger.kernel.org, wsd_upstream@mediatek.com,
Chenglin Xu <chenglin.xu@mediatek.com>,
Catalin Marinas <catalin.marinas@arm.com>,
Sean Wang <sean.wang@mediatek.com>,
Will Deacon <will.deacon@arm.com>,
linux-kernel@vger.kernel.org, henryc.chen@mediatek.com,
flora.fu@mediatek.com, Rob Herring <robh+dt@kernel.org>,
Christophe Jaillet <christophe.jaillet@wanadoo.fr>,
linux-mediatek@lists.infradead.org,
Chen Zhong <chen.zhong@mediatek.com>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/3] soc: mediatek: pwrap: add pwrap driver for MT6779 SoCs
Date: Mon, 14 Oct 2019 14:04:25 +0800 [thread overview]
Message-ID: <1571033065.19600.23.camel@mtkswgap22> (raw)
In-Reply-To: <b2f881e2-959e-eccf-e62e-54c510608aaa@gmail.com>
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
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-10-14 6:04 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-03 7:48 [PATCH 0/3] Pwrap: Mediatek pwrap driver for MT6779 Argus Lin
2019-10-03 7:48 ` [PATCH 1/3] dt-bindings: pwrap: mediatek: add pwrap support " Argus Lin
2019-10-15 19:20 ` Rob Herring
2019-10-03 7:48 ` [PATCH 2/3] soc: mediatek: pwrap: add pwrap driver for MT6779 SoCs Argus Lin
2019-10-03 23:26 ` Matthias Brugger
2019-10-14 6:04 ` Argus Lin [this message]
2019-11-04 8:22 ` Argus Lin
2019-11-10 18:47 ` Matthias Brugger
2019-10-03 7:48 ` [PATCH 3/3] soc: mediatek: pwrap: add support for MT6359 PMIC Argus Lin
2019-10-03 23:27 ` Matthias Brugger
2019-10-14 5:17 ` Argus Lin
2019-11-04 8:23 ` Argus Lin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1571033065.19600.23.camel@mtkswgap22 \
--to=argus.lin@mediatek.com \
--cc=catalin.marinas@arm.com \
--cc=chen.zhong@mediatek.com \
--cc=chenglin.xu@mediatek.com \
--cc=christophe.jaillet@wanadoo.fr \
--cc=devicetree@vger.kernel.org \
--cc=flora.fu@mediatek.com \
--cc=henryc.chen@mediatek.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=mark.rutland@arm.com \
--cc=matthias.bgg@gmail.com \
--cc=robh+dt@kernel.org \
--cc=sean.wang@mediatek.com \
--cc=will.deacon@arm.com \
--cc=wsd_upstream@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).