| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <DA1JMS.JREL9T5RHOOO2@gmail.com>
Date: Wed, 06 Nov 2024 14:30:13 +0300
From: Yassine Oudjana <yassine.oudjana@...il.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Sen Chu <sen.chu@...iatek.com>, Sean Wang
<sean.wang@...iatek.com>, Macpaul Lin <macpaul.lin@...iatek.com>, Lee Jones
<lee@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>, Liam Girdwood
<lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, jason-ch chen
<Jason-ch.Chen@...iatek.com>, Chen Zhong <chen.zhong@...iatek.com>, Flora Fu
<flora.fu@...iatek.com>, Alexandre Mergnat <amergnat@...libre.com>,
Yassine Oudjana <y.oudjana@...tonmail.com>, linux-input@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH 3/6] soc: mediatek: pwrap: Add support for MT6735 and
MT6328 SoC/PMIC pair
On Tue, Oct 22 2024 at 11:44:46 +02:00:00, AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.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 <angelogioacchino.delregno@...labora.com> wrote:
>>> Il 18/10/24 10:10, Yassine Oudjana ha scritto:
>>>> From: Yassine Oudjana <y.oudjana@...tonmail.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@...tonmail.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- pmic-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,
>>>> - /* MT6323 only regs */
>>>> + /* MT6323 and MT6328 only regs */
>>>> PWRAP_DEW_CIPHER_EN,
>>>> PWRAP_DEW_RDDMY_NO,
>>>> @@ -121,8 +122,10 @@ enum dew_regs {
>>>> PWRAP_RG_SPI_CON13,
>>>> PWRAP_SPISLV_KEY,
>>>> - /* 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,
>>>> };
>>>> +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,
>>>> - /* 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 yet 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 accessed again. This is the only place I could find
>> where they are accessed:
>> https://gitlab.com/Tooniis/linux-samsung-grandpplte/-/blob/master/drivers/misc/
>> mediatek/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.
Powered by blists - more mailing lists