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

From: Matthias Brugger
Date: Sun Nov 10 2019 - 13:47:39 EST

Next message: Marco Elver: "Re: KCSAN: data-race in __alloc_file / __alloc_file"
Previous message: Yu Zhao: "Re: [PATCH v4 2/2] mm: avoid slub allocation while holding list_lock"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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

Next message: Marco Elver: "Re: KCSAN: data-race in __alloc_file / __alloc_file"
Previous message: Yu Zhao: "Re: [PATCH v4 2/2] mm: avoid slub allocation while holding list_lock"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]