From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthias Brugger Subject: Re: [PATCH V2 1/4] soc: mediatek: PMIC wrap: DEW base addr may vary Date: Thu, 21 Jan 2016 18:23:24 +0100 Message-ID: <56A1140C.7080507@gmail.com> References: <1452441884-25882-1-git-send-email-blogic@openwrt.org> <1452572924.31465.13.camel@mtksdaap41> <56A0C4B4.8090807@gmail.com> <56A0D7AA.3010301@openwrt.org> <56A10718.1010801@gmail.com> <56A10C7A.5080809@openwrt.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56A10C7A.5080809-p3rKhJxN3npAfugRpC6u6w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+glpam-linux-mediatek=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: John Crispin , Henry Chen Cc: "linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" List-Id: linux-mediatek@lists.infradead.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 >>>>>> --- >>>>>> 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