From: Matthias Brugger <matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: John Crispin <blogic-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>,
Henry Chen <henryc.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Cc: "linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org"
<linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
Subject: Re: [PATCH V2 1/4] soc: mediatek: PMIC wrap: DEW base addr may vary
Date: Thu, 21 Jan 2016 18:23:24 +0100 [thread overview]
Message-ID: <56A1140C.7080507@gmail.com> (raw)
In-Reply-To: <56A10C7A.5080809-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
On 21/01/16 17:51, John Crispin wrote:
>
>
> On 21/01/2016 17:28, Matthias Brugger wrote:
>>
>>
>> On 21/01/16 14:05, John Crispin wrote:
>>>
>>>
>>> On 21/01/2016 12:44, Matthias Brugger wrote:
>>>>
>>>>
>>>> On 12/01/16 05:28, Henry Chen wrote:
>>>>> On Mon, 2016-01-11 at 00:04 +0800, John Crispin wrote:
>>>>>> The MT6323 and possible other PMICs have a different DEW base addr.
>>>>>> Change
>>>>>> the driver so that it handles the DEW registeres in the same manner
>>>>>> as the
>>>>>> rest of the registers.
>>>>>>
>>>>>> Signed-off-by: John Crispin <blogic-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
>>>>>> ---
>>>>>> drivers/soc/mediatek/mtk-pmic-wrap.c | 166
>>>>>> +++++++++++++++++++++++++---------
>>>>>> 1 file changed, 122 insertions(+), 44 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c
>>>>>> b/drivers/soc/mediatek/mtk-pmic-wrap.c
>>>>>> index 105597a..f3fccea 100644
>>>>>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
>>>>>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
>>>>>> @@ -61,32 +61,85 @@
>>>>>> #define PWRAP_MAN_CMD_OP_OUTQ (0xa << 8)
>>>>>>
>>>>>> /* macro for slave device wrapper registers */
>>>>>> -#define PWRAP_DEW_BASE 0xbc00
>>>>>> -#define PWRAP_DEW_EVENT_OUT_EN (PWRAP_DEW_BASE + 0x0)
>>>>>> -#define PWRAP_DEW_DIO_EN (PWRAP_DEW_BASE + 0x2)
>>>>>> -#define PWRAP_DEW_EVENT_SRC_EN (PWRAP_DEW_BASE + 0x4)
>>>>>> -#define PWRAP_DEW_EVENT_SRC (PWRAP_DEW_BASE + 0x6)
>>>>>> -#define PWRAP_DEW_EVENT_FLAG (PWRAP_DEW_BASE + 0x8)
>>>>>> -#define PWRAP_DEW_READ_TEST (PWRAP_DEW_BASE + 0xa)
>>>>>> -#define PWRAP_DEW_WRITE_TEST (PWRAP_DEW_BASE + 0xc)
>>>>>> -#define PWRAP_DEW_CRC_EN (PWRAP_DEW_BASE + 0xe)
>>>>>> -#define PWRAP_DEW_CRC_VAL (PWRAP_DEW_BASE + 0x10)
>>>>>> -#define PWRAP_DEW_MON_GRP_SEL (PWRAP_DEW_BASE + 0x12)
>>>>>> -#define PWRAP_DEW_MON_FLAG_SEL (PWRAP_DEW_BASE + 0x14)
>>>>>> -#define PWRAP_DEW_EVENT_TEST (PWRAP_DEW_BASE + 0x16)
>>>>>> -#define PWRAP_DEW_CIPHER_KEY_SEL (PWRAP_DEW_BASE + 0x18)
>>>>>> -#define PWRAP_DEW_CIPHER_IV_SEL (PWRAP_DEW_BASE +
>>>>>> 0x1a)
>>>>>> -#define PWRAP_DEW_CIPHER_LOAD (PWRAP_DEW_BASE + 0x1c)
>>>>>> -#define PWRAP_DEW_CIPHER_START (PWRAP_DEW_BASE + 0x1e)
>>>>>> -#define PWRAP_DEW_CIPHER_RDY (PWRAP_DEW_BASE + 0x20)
>>>>>> -#define PWRAP_DEW_CIPHER_MODE (PWRAP_DEW_BASE + 0x22)
>>>>>> -#define PWRAP_DEW_CIPHER_SWRST (PWRAP_DEW_BASE + 0x24)
>>>>>> -#define PWRAP_MT8173_DEW_CIPHER_IV0 (PWRAP_DEW_BASE + 0x26)
>>>>>> -#define PWRAP_MT8173_DEW_CIPHER_IV1 (PWRAP_DEW_BASE + 0x28)
>>>>>> -#define PWRAP_MT8173_DEW_CIPHER_IV2 (PWRAP_DEW_BASE + 0x2a)
>>>>>> -#define PWRAP_MT8173_DEW_CIPHER_IV3 (PWRAP_DEW_BASE + 0x2c)
>>>>>> -#define PWRAP_MT8173_DEW_CIPHER_IV4 (PWRAP_DEW_BASE + 0x2e)
>>>>>> -#define PWRAP_MT8173_DEW_CIPHER_IV5 (PWRAP_DEW_BASE + 0x30)
>>>>>> +enum pwrap_dew_regs {
>>>>>> + PWRAP_DEW_EVENT_OUT_EN,
>>>>>> + PWRAP_DEW_DIO_EN,
>>>>>> + PWRAP_DEW_EVENT_SRC_EN,
>>>>>> + PWRAP_DEW_EVENT_SRC,
>>>>>> + PWRAP_DEW_EVENT_FLAG,
>>>>>> + PWRAP_DEW_READ_TEST,
>>>>>> + PWRAP_DEW_WRITE_TEST,
>>>>>> + PWRAP_DEW_CRC_EN,
>>>>>> + PWRAP_DEW_CRC_VAL,
>>>>>> + PWRAP_DEW_MON_GRP_SEL,
>>>>>> + PWRAP_DEW_MON_FLAG_SEL,
>>>>>> + PWRAP_DEW_EVENT_TEST,
>>>>>> + PWRAP_DEW_CIPHER_KEY_SEL,
>>>>>> + PWRAP_DEW_CIPHER_IV_SEL,
>>>>>> + PWRAP_DEW_CIPHER_LOAD,
>>>>>> + PWRAP_DEW_CIPHER_START,
>>>>>> + PWRAP_DEW_CIPHER_RDY,
>>>>>> + PWRAP_DEW_CIPHER_MODE,
>>>>>> + PWRAP_DEW_CIPHER_SWRST,
>>>>>> +
>>>>>> + /* MT8173 only regs */
>>>>>> + PWRAP_DEW_CIPHER_IV0,
>>>>>> + PWRAP_DEW_CIPHER_IV1,
>>>>>> + PWRAP_DEW_CIPHER_IV2,
>>>>>> + PWRAP_DEW_CIPHER_IV3,
>>>>>> + PWRAP_DEW_CIPHER_IV4,
>>>>>> + PWRAP_DEW_CIPHER_IV5,
>>>>>> +};
>>>>>> +
>>>>>> +static int mt8135_dew_regs[] = {
>>>>>> + [PWRAP_DEW_EVENT_OUT_EN] = 0x0,
>>>>>> + [PWRAP_DEW_DIO_EN] = 0x2,
>>>>>> + [PWRAP_DEW_EVENT_SRC_EN] = 0x4,
>>>>>> + [PWRAP_DEW_EVENT_SRC] = 0x6,
>>>>>> + [PWRAP_DEW_EVENT_FLAG] = 0x8,
>>>>>> + [PWRAP_DEW_READ_TEST] = 0xa,
>>>>>> + [PWRAP_DEW_WRITE_TEST] = 0xc,
>>>>>> + [PWRAP_DEW_CRC_EN] = 0xe,
>>>>>> + [PWRAP_DEW_CRC_VAL] = 0x10,
>>>>>> + [PWRAP_DEW_MON_GRP_SEL] = 0x12,
>>>>>> + [PWRAP_DEW_MON_FLAG_SEL] = 0x14,
>>>>>> + [PWRAP_DEW_EVENT_TEST] = 0x16,
>>>>>> + [PWRAP_DEW_CIPHER_KEY_SEL] = 0x18,
>>>>>> + [PWRAP_DEW_CIPHER_IV_SEL] = 0x1a,
>>>>>> + [PWRAP_DEW_CIPHER_LOAD] = 0x1c,
>>>>>> + [PWRAP_DEW_CIPHER_START] = 0x1e,
>>>>>> + [PWRAP_DEW_CIPHER_RDY] = 0x20,
>>>>>> + [PWRAP_DEW_CIPHER_MODE] = 0x22,
>>>>>> + [PWRAP_DEW_CIPHER_SWRST] = 0x24,
>>>>>> +};
>>>>>> +
>>>>>> +static int mt8173_dew_regs[] = {
>>>>>> + [PWRAP_DEW_EVENT_OUT_EN] = 0x0,
>>>>>> + [PWRAP_DEW_DIO_EN] = 0x2,
>>>>>> + [PWRAP_DEW_EVENT_SRC_EN] = 0x4,
>>>>>> + [PWRAP_DEW_EVENT_SRC] = 0x6,
>>>>>> + [PWRAP_DEW_EVENT_FLAG] = 0x8,
>>>>>> + [PWRAP_DEW_READ_TEST] = 0xa,
>>>>>> + [PWRAP_DEW_WRITE_TEST] = 0xc,
>>>>>> + [PWRAP_DEW_CRC_EN] = 0xe,
>>>>>> + [PWRAP_DEW_CRC_VAL] = 0x10,
>>>>>> + [PWRAP_DEW_MON_GRP_SEL] = 0x12,
>>>>>> + [PWRAP_DEW_MON_FLAG_SEL] = 0x14,
>>>>>> + [PWRAP_DEW_EVENT_TEST] = 0x16,
>>>>>> + [PWRAP_DEW_CIPHER_KEY_SEL] = 0x18,
>>>>>> + [PWRAP_DEW_CIPHER_IV_SEL] = 0x1a,
>>>>>> + [PWRAP_DEW_CIPHER_LOAD] = 0x1c,
>>>>>> + [PWRAP_DEW_CIPHER_START] = 0x1e,
>>>>>> + [PWRAP_DEW_CIPHER_RDY] = 0x20,
>>>>>> + [PWRAP_DEW_CIPHER_MODE] = 0x22,
>>>>>> + [PWRAP_DEW_CIPHER_SWRST] = 0x24,
>>>>>> + [PWRAP_DEW_CIPHER_IV0] = 0x26,
>>>>>> + [PWRAP_DEW_CIPHER_IV1] = 0x28,
>>>>>> + [PWRAP_DEW_CIPHER_IV2] = 0x2a,
>>>>>> + [PWRAP_DEW_CIPHER_IV3] = 0x2c,
>>>>>> + [PWRAP_DEW_CIPHER_IV4] = 0x2e,
>>>>>> + [PWRAP_DEW_CIPHER_IV5] = 0x30,
>>>>>> +};
>>>>>>
>>>>>> enum pwrap_regs {
>>>>>> PWRAP_MUX_SEL,
>>>>>> @@ -347,18 +400,24 @@ enum pwrap_type {
>>>>>>
>>>>>> struct pmic_wrapper_type {
>>>>>> int *regs;
>>>>>> + int *dew_regs;
>>>>>> + u32 dew_base;
>>>>>> enum pwrap_type type;
>>>>>> u32 arb_en_all;
>>>>>> };
>>>>>>
>>>>>> static struct pmic_wrapper_type pwrap_mt8135 = {
>>>>>> .regs = mt8135_regs,
>>>>>> + .dew_regs = mt8135_dew_regs,
>>>>>> + .dew_base = 0xbc00,
>>>>>> .type = PWRAP_MT8135,
>>>>>> .arb_en_all = 0x1ff,
>>>>>> };
>>>>>>
>>>>>> static struct pmic_wrapper_type pwrap_mt8173 = {
>>>>>> .regs = mt8173_regs,
>>>>>> + .dew_regs = mt8173_dew_regs,
>>>>>> + .dew_base = 0xbc00,
>>>>>> .type = PWRAP_MT8173,
>>>>>> .arb_en_all = 0x3f,
>>>>>> };
>>>>>> @@ -368,6 +427,8 @@ struct pmic_wrapper {
>>>>>> void __iomem *base;
>>>>>> struct regmap *regmap;
>>>>>> int *regs;
>>>>>> + int *dew_regs;
>>>>>> + u32 dew_base;
>>>>>> enum pwrap_type type;
>>>>>> u32 arb_en_all;
>>>>>> struct clk *clk_spi;
>>>>>> @@ -475,6 +536,18 @@ static int pwrap_read(struct pmic_wrapper *wrp,
>>>>>> u32 adr, u32 *rdata)
>>>>>> return 0;
>>>>>> }
>>>>>
>>>>> Hi John,
>>>>>
>>>>> Because the DEW was the address of PMIC not the address of AP. I think
>>>>> that dew_regs/dew_base was much better to define out of pmic_wrapper,
>>>>> maybe create another structure for it.
>>>>>
>>>>> struct pwrap_slv_type {
>>>>> const u32 *dew_regs;
>>>>> enum pmic_type type;
>>>>> };
>>>>>
>>>>> and define for different PMIC, something likes
>>>>>
>>>>> static const struct pwrap_slv_type pmic_mt6397 = {
>>>>> .dew_regs = mt6397_regs,
>>>>> .type = PMIC_MT6397,
>>>>> };
>>>>>
>>>>> static const struct pwrap_slv_type pmic_mt6323 = {
>>>>> .dew_regs = mt6323_regs,
>>>>> .type = PMIC_MT6323,
>>>>> };
>>>>>
>>>>
>>>> I would like to go one step further and actually get the DEW values
>>>> depending on the PMIC in the device tree, which is the child of the
>>>> pmic-wrapper.
>>>>
>>>> Regards,
>>>> Matthias
>>>
>>>
>>> not sure if that is a good idea. the code path and dew register usage
>>> depends on the slave type. putting the dew registers into the DT will
>>> only have the effect that we will have 1 array less in the driver but in
>>> turn will have code to load that exact array back. also it wont be a
>>> simple array that we store in the DT. but we need to match register
>>> names to register offsets.
>>>
>>> although i agree that putting static data into the DT tends to be a good
>>> thing i believe in this case it is not really sane.
>>>
>>
>> Actually I wasn't thinking of this. My idea (poor mans solution) would
>> be to identify the pmic dts node and use the values dependent on this,
>> rather then on the SoC version.
>
> i have that bit already in my series based on code i got from mtk.
>
> however even cooler might be to just read the CID register and make an
> educated decision based on that.
>
> i'll try to cleanup the patches tomorrow and then post them.
>
>>
>> The premium class solution would be to have the registers definded in
>> the pmic driver and get them accessed through the pmic-wrapper driver.
>
> indeed that would be an option.
>
> another thing that we might want to replace the pwrap_is_mtxyz()
> functions and use switch (pwrap->type) { case PWRAP_MTXYZ: ... }
> constructs instead.
>
Yeah, that would be good. Would you mind doing this?
If yes, it would be great if you base your work on the v4.5-next/soc [1]
branch.
Cheers,
Matthias
[1] https://github.com/mbgg/linux-mediatek/tree/v4.5-next/soc
next prev parent reply other threads:[~2016-01-21 17:23 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-10 16:04 [PATCH V2 1/4] soc: mediatek: PMIC wrap: DEW base addr may vary John Crispin
[not found] ` <1452441884-25882-1-git-send-email-blogic-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
2016-01-10 16:04 ` [PATCH V2 2/4] soc: mediatek: PMIC wrap: INT_EN mask " John Crispin
2016-01-10 16:04 ` [PATCH V2 3/4] soc: mediatek: PMIC wrap the SPI_W bit in PWRAP_MAN_CMD " John Crispin
2016-01-10 16:04 ` [PATCH V2 4/4] soc: mediatek: PMIC wrap: add support for MT7623/6323 John Crispin
2016-01-12 4:28 ` [PATCH V2 1/4] soc: mediatek: PMIC wrap: DEW base addr may vary Henry Chen
2016-01-21 11:44 ` Matthias Brugger
[not found] ` <56A0C4B4.8090807-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-01-21 13:05 ` John Crispin
[not found] ` <56A0D7AA.3010301-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
2016-01-21 16:28 ` Matthias Brugger
[not found] ` <56A10718.1010801-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-01-21 16:51 ` John Crispin
[not found] ` <56A10C7A.5080809-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
2016-01-21 17:23 ` Matthias Brugger [this message]
2016-01-22 5:04 ` Henry Chen
2016-01-22 9:03 ` John Crispin
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=56A1140C.7080507@gmail.com \
--to=matthias.bgg-re5jqeeqqe8avxtiumwx3w@public.gmane.org \
--cc=blogic-p3rKhJxN3npAfugRpC6u6w@public.gmane.org \
--cc=henryc.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org \
--cc=linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
/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).