Re: [PATCH V2 1/4] soc: mediatek: PMIC wrap: DEW base addr may vary

[John Crispin <blogic-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>]

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.

	John