Re: [PATCH 3/6] soc: mediatek: pwrap: Add support for MT6735 and MT6328 SoC/PMIC pair

[Yassine Oudjana <yassine.oudjana@gmail.com>]

On Tue, Oct 22 2024 at 11:44:46 +02:00:00, AngeloGioacchino Del Regno 
<angelogioacchino.delregno@collabora.com> wrote:
> Il 21/10/24 16:48, Yassine Oudjana ha scritto:
>> 
>> On Mon, Oct 21 2024 at 15:25:00 +02:00:00, AngeloGioacchino Del 
>> Regno \x7f<angelogioacchino.delregno@collabora.com> wrote:
>>> Il 18/10/24 10:10, Yassine Oudjana ha scritto:
>>>> From: Yassine Oudjana <y.oudjana@protonmail.com>
>>>> 
>>>> Add register definitions and configuration for the MT6735 SoC and 
>>>> the
>>>> MT6328 PMIC which are commonly paired and communicate through the 
>>>> PMIC
>>>> wrapper.
>>>> 
>>>> Note that the PMIC wrapper on MT6735M has a slightly different 
>>>> register
>>>> map and is therefore NOT compatible with MT6735.
>>>> 
>>>> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>>>> ---
>>>>   drivers/soc/mediatek/mtk-pmic-wrap.c | 251 
>>>> ++++++++++++++++++++++++++-
>>>>   1 file changed, 248 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c 
>>>> b/drivers/soc/mediatek/mtk- \x7f\x7f\x7fpmic-wrap.c
>>>> index 9fdc0ef792026..b9e8dd2a5999d 100644
>>>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
>>>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
>>>> @@ -3,6 +3,7 @@
>>>>    * Copyright (c) 2014 MediaTek Inc.
>>>>    * Author: Flora Fu, MediaTek
>>>>    */
>>>> +
>>>>   #include <linux/clk.h>
>>>>   #include <linux/interrupt.h>
>>>>   #include <linux/io.h>
>>>> @@ -100,7 +101,7 @@ enum dew_regs {
>>>>       PWRAP_DEW_CIPHER_MODE,
>>>>       PWRAP_DEW_CIPHER_SWRST,
>>>>   \x7f-    /* MT6323 only regs */
>>>> +    /* MT6323 and MT6328 only regs */
>>>>       PWRAP_DEW_CIPHER_EN,
>>>>       PWRAP_DEW_RDDMY_NO,
>>>>   \x7f@@ -121,8 +122,10 @@ enum dew_regs {
>>>>       PWRAP_RG_SPI_CON13,
>>>>       PWRAP_SPISLV_KEY,
>>>>   \x7f-    /* MT6359 only regs */
>>>> +    /* MT6359 and MT6328 only regs */
>>>>       PWRAP_DEW_CRC_SWRST,
>>>> +
>>>> +    /* MT6359 only regs */
>>>>       PWRAP_DEW_RG_EN_RECORD,
>>>>       PWRAP_DEW_RECORD_CMD0,
>>>>       PWRAP_DEW_RECORD_CMD1,
>>>> @@ -171,6 +174,23 @@ static const u32 mt6323_regs[] = {
>>>>       [PWRAP_DEW_RDDMY_NO] =        0x01a4,
>>>>   };
>>>>   \x7f+static const u32 mt6328_regs[] = {
>>>> +    [PWRAP_DEW_DIO_EN] =        0x02d4,
>>>> +    [PWRAP_DEW_READ_TEST] =        0x02d6,
>>>> +    [PWRAP_DEW_WRITE_TEST] =    0x02d8,
>>>> +    [PWRAP_DEW_CRC_SWRST] =        0x02da,
>>>> +    [PWRAP_DEW_CRC_EN] =        0x02dc,
>>>> +    [PWRAP_DEW_CRC_VAL] =        0x02de,
>>>> +    [PWRAP_DEW_MON_GRP_SEL] =    0x02e0,
>>>> +    [PWRAP_DEW_CIPHER_KEY_SEL] =    0x02e2,
>>>> +    [PWRAP_DEW_CIPHER_IV_SEL] =    0x02e4,
>>>> +    [PWRAP_DEW_CIPHER_EN] =        0x02e6,
>>>> +    [PWRAP_DEW_CIPHER_RDY] =    0x02e8,
>>>> +    [PWRAP_DEW_CIPHER_MODE] =    0x02ea,
>>>> +    [PWRAP_DEW_CIPHER_SWRST] =    0x02ec,
>>>> +    [PWRAP_DEW_RDDMY_NO] =        0x02ee,
>>>> +};
>>>> +
>>>>   static const u32 mt6331_regs[] = {
>>>>       [PWRAP_DEW_DIO_EN] =        0x018c,
>>>>       [PWRAP_DEW_READ_TEST] =        0x018e,
>>>> @@ -394,7 +414,7 @@ enum pwrap_regs {
>>>>       PWRAP_ADC_RDATA_ADDR1,
>>>>       PWRAP_ADC_RDATA_ADDR2,
>>>>   \x7f-    /* MT7622 only regs */
>>>> +    /* MT7622 and MT6735 only regs */
>>>>       PWRAP_STA,
>>>>       PWRAP_CLR,
>>>>       PWRAP_DVFS_ADR8,
>>>> @@ -417,6 +437,8 @@ enum pwrap_regs {
>>>>       PWRAP_ADC_RDATA_ADDR,
>>>>       PWRAP_GPS_STA,
>>>>       PWRAP_SW_RST,
>>>> +
>>>> +    /* MT7622 only regs */
>>>>       PWRAP_DVFS_STEP_CTRL0,
>>>>       PWRAP_DVFS_STEP_CTRL1,
>>>>       PWRAP_DVFS_STEP_CTRL2,
>>>> @@ -481,6 +503,50 @@ enum pwrap_regs {
>>>>       /* MT8516 only regs */
>>>>       PWRAP_OP_TYPE,
>>>>       PWRAP_MSB_FIRST,
>>>> +
>>>> +    /* MT6735 only regs */
>>>> +    PWRAP_WACS3_EN,
>>>> +    PWRAP_INIT_DONE3,
>>>> +    PWRAP_WACS3_CMD,
>>>> +    PWRAP_WACS3_RDATA,
>>>> +    PWRAP_WACS3_VLDCLR,
>>> 
>>> Are you sure that you need the PWRAP_MD_ADC_xxxx registers in here?
>>> 
>>> Since MD is relatively big on its own, I'm not sure how to proceed 
>>> here.. it may
>>> make sense to split the MD part to a different array, or it may 
>>> not... I do need
>>> to understand what's going on.
>>> 
>>> Can you please point me at some reference code to look at, so that 
>>> I can understand
>>> the situation a bit?
>>> 
>>> Besides, I'm noticing that the "MD_ADC_RDATA_ADDR_R(x)" are 
>>> sequential registers
>>> and that's on more than just MT6735: instead of having 32 more 
>>> entries, it might
>>> make more sense to set only the first and declare the number (or 
>>> size) of regs...
>>> 
>>> Something like:
>>> 
>>> enum pwrap_regs {
>>>     .....
>>>     PWRAP_MD_ADC_RDATA_ADDR_LATEST,
>>>     PWRAP_MD_ADC_RDATA_ADDR_WP,
>>>     PWRAP_MD_ADC_RDATA_ADDR_R0,
>>>     PWRAP_MD_ADC_STA0,
>>>     PWRAP_MD_ADC_STA1,
>>>     PWRAP_MD_ADC_STA2
>>> };
>>> 
>>> static const struct pmic_wrapper_type pwrap_mt6735 = {
>>>     .regs = mt6735_regs,
>>>     .num_md_addr = 32,
>>>     [other stuff]
>>> };
>>> 
>>> ...but again, please, if you can point me at an implementation that 
>>> actually
>>> uses the R(x) registers, that'd be better ... so that we can choose 
>>> the best
>>> option to add those in there.
>>> 
>>> Everything else is great: good job :-)
>>> 
>>> Cheers,
>>> Angelo
>> 
>> I just defined all the registers I could find. We aren't using them 
>> for anything \x7fyet so it's also fine to keep them out for now.
>> 
>> It seems that in the downstream kernel they are only initialized 
>> once and never \x7faccessed again. This is the only place I could find 
>> where they are accessed:
>> https://gitlab.com/Tooniis/linux-samsung-grandpplte/-/blob/master/drivers/misc/ 
>> \x7fmediatek/pmic_wrap/mt6735/pwrap_hal.c#L1254-1290
>> 
>> 
> 
> Thanks for pointing me at that reference.
> 
> Yeah, looks like they're getting statically initialized to some 
> value, and then
> nothing else... I wonder if the values that they're writing are just 
> the register
> defaults, or if they're actually overriding anything for real...
> 
> ...that's because, in case those are the defaults, we may get them 
> set by just
> resetting the MD, catching two birds with one stone.
> That'd be easy to check, anyway, as you can just read them out and 
> see if the
> values are these...

The values aren't defaults. The registers all default to 0 and these 
values are PMIC registers.

Anyway, I'll just remove the registers since I won't need them any time 
soon.