| Message ID | 604078485ca9ab04cea5bc531425d6035b27bc88.1505980364.git.sean.wang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/21/2017 10:26 AM, sean.wang@mediatek.com wrote: > From: Sean Wang <sean.wang@mediatek.com> > > pwrap initialization is highly associated with the base SoC, so > update here for allowing pwrap_init without slave program which would be > used to those PMICs without extra encryption on bus such as MT6380. > > Signed-off-by: Chenglin Xu <chenglin.xu@mediatek.com> > Signed-off-by: Chen Zhong <chen.zhong@mediatek.com> > Signed-off-by: Sean Wang <sean.wang@mediatek.com> > --- > drivers/soc/mediatek/mtk-pmic-wrap.c | 91 +++++++++++++++++++++--------------- > 1 file changed, 54 insertions(+), 37 deletions(-) > > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c > index 27d7ccc..9c6d855 100644 > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c > @@ -531,6 +531,7 @@ struct pmic_wrapper_type { > u32 spi_w; > u32 wdt_src; > int has_bridge:1; > + int slv_program:1; > int (*init_reg_clock)(struct pmic_wrapper *wrp); > int (*init_soc_specific)(struct pmic_wrapper *wrp); > }; > @@ -999,9 +1000,12 @@ static int pwrap_init(struct pmic_wrapper *wrp) > } > > /* Reset SPI slave */ > - ret = pwrap_reset_spislave(wrp); > - if (ret) > - return ret; > + > + if (wrp->master->slv_program) { > + ret = pwrap_reset_spislave(wrp); > + if (ret) > + return ret; > + } > > pwrap_writel(wrp, 1, PWRAP_WRAP_EN); > > @@ -1013,45 +1017,52 @@ static int pwrap_init(struct pmic_wrapper *wrp) > if (ret) > return ret; > > - /* Setup serial input delay */ > - ret = pwrap_init_sidly(wrp); > - if (ret) > - return ret; > + if (wrp->master->slv_program) { This if branch is really long and complex enough to put it into function apart. Thanks, Matthias PD please take into account the comments I made on v3 of the series.
On Tue, 2017-10-10 at 20:00 +0200, Matthias Brugger wrote: > > On 09/21/2017 10:26 AM, sean.wang@mediatek.com wrote: > > From: Sean Wang <sean.wang@mediatek.com> > > > > pwrap initialization is highly associated with the base SoC, so > > update here for allowing pwrap_init without slave program which would be > > used to those PMICs without extra encryption on bus such as MT6380. > > > > Signed-off-by: Chenglin Xu <chenglin.xu@mediatek.com> > > Signed-off-by: Chen Zhong <chen.zhong@mediatek.com> > > Signed-off-by: Sean Wang <sean.wang@mediatek.com> > > --- > > drivers/soc/mediatek/mtk-pmic-wrap.c | 91 +++++++++++++++++++++--------------- > > 1 file changed, 54 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c > > index 27d7ccc..9c6d855 100644 > > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c > > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c > > @@ -531,6 +531,7 @@ struct pmic_wrapper_type { > > u32 spi_w; > > u32 wdt_src; > > int has_bridge:1; > > + int slv_program:1; > > int (*init_reg_clock)(struct pmic_wrapper *wrp); > > int (*init_soc_specific)(struct pmic_wrapper *wrp); > > }; > > @@ -999,9 +1000,12 @@ static int pwrap_init(struct pmic_wrapper *wrp) > > } > > > > /* Reset SPI slave */ > > - ret = pwrap_reset_spislave(wrp); > > - if (ret) > > - return ret; > > + > > + if (wrp->master->slv_program) { > > + ret = pwrap_reset_spislave(wrp); > > + if (ret) > > + return ret; > > + } > > > > pwrap_writel(wrp, 1, PWRAP_WRAP_EN); > > > > @@ -1013,45 +1017,52 @@ static int pwrap_init(struct pmic_wrapper *wrp) > > if (ret) > > return ret; > > > > - /* Setup serial input delay */ > > - ret = pwrap_init_sidly(wrp); > > - if (ret) > > - return ret; > > + if (wrp->master->slv_program) { > > This if branch is really long and complex enough to put it into function apart. > > Thanks, > Matthias > > PD please take into account the comments I made on v3 of the series. > I'll try to breakdown the long logic into the short one and use a flag indicating the slave capability decides whether the functions is required being enabled for the slave instead of slv_program which is less meaningful. In this way, pmic_init will be more extensible when more different SoCs and target slaves with various flavors into the driver. And also take into accounts those suggestions you made in v3 in the next version. Sean
On 10/13/2017 11:41 AM, Sean Wang wrote: > On Tue, 2017-10-10 at 20:00 +0200, Matthias Brugger wrote: >> >> On 09/21/2017 10:26 AM, sean.wang@mediatek.com wrote: >>> From: Sean Wang <sean.wang@mediatek.com> >>> >>> pwrap initialization is highly associated with the base SoC, so >>> update here for allowing pwrap_init without slave program which would be >>> used to those PMICs without extra encryption on bus such as MT6380. >>> >>> Signed-off-by: Chenglin Xu <chenglin.xu@mediatek.com> >>> Signed-off-by: Chen Zhong <chen.zhong@mediatek.com> >>> Signed-off-by: Sean Wang <sean.wang@mediatek.com> >>> --- >>> drivers/soc/mediatek/mtk-pmic-wrap.c | 91 +++++++++++++++++++++--------------- >>> 1 file changed, 54 insertions(+), 37 deletions(-) >>> >>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c >>> index 27d7ccc..9c6d855 100644 >>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c >>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c >>> @@ -531,6 +531,7 @@ struct pmic_wrapper_type { >>> u32 spi_w; >>> u32 wdt_src; >>> int has_bridge:1; >>> + int slv_program:1; >>> int (*init_reg_clock)(struct pmic_wrapper *wrp); >>> int (*init_soc_specific)(struct pmic_wrapper *wrp); >>> }; >>> @@ -999,9 +1000,12 @@ static int pwrap_init(struct pmic_wrapper *wrp) >>> } >>> >>> /* Reset SPI slave */ >>> - ret = pwrap_reset_spislave(wrp); >>> - if (ret) >>> - return ret; >>> + >>> + if (wrp->master->slv_program) { >>> + ret = pwrap_reset_spislave(wrp); >>> + if (ret) >>> + return ret; >>> + } >>> >>> pwrap_writel(wrp, 1, PWRAP_WRAP_EN); >>> >>> @@ -1013,45 +1017,52 @@ static int pwrap_init(struct pmic_wrapper *wrp) >>> if (ret) >>> return ret; >>> >>> - /* Setup serial input delay */ >>> - ret = pwrap_init_sidly(wrp); >>> - if (ret) >>> - return ret; >>> + if (wrp->master->slv_program) { >> >> This if branch is really long and complex enough to put it into function apart. >> >> Thanks, >> Matthias >> >> PD please take into account the comments I made on v3 of the series. >> > > I'll try to breakdown the long logic into the short one and use a flag > indicating the slave capability decides whether the functions is > required being enabled for the slave instead of slv_program which is > less meaningful. In this way, pmic_init will be more extensible when > more different SoCs and target slaves with various flavors into the > driver. And also take into accounts those suggestions you made in v3 in > the next version. > > Sean > I totally agree, but I wanted to underline that right now the if branch under "if (wrp->master->slv_program)" is around 30 lines, so I think it would be a good candidate to put it into it's own function. For example: pwrap_init_encryption() As from what I understand from the commit log, slv_program in the end enables encryption of the communication, right? Regards, Matthias
On Fri, 2017-10-13 at 16:07 +0200, Matthias Brugger wrote: > > On 10/13/2017 11:41 AM, Sean Wang wrote: > > On Tue, 2017-10-10 at 20:00 +0200, Matthias Brugger wrote: > >> > >> On 09/21/2017 10:26 AM, sean.wang@mediatek.com wrote: > >>> From: Sean Wang <sean.wang@mediatek.com> > >>> > >>> pwrap initialization is highly associated with the base SoC, so > >>> update here for allowing pwrap_init without slave program which would be > >>> used to those PMICs without extra encryption on bus such as MT6380. > >>> > >>> Signed-off-by: Chenglin Xu <chenglin.xu@mediatek.com> > >>> Signed-off-by: Chen Zhong <chen.zhong@mediatek.com> > >>> Signed-off-by: Sean Wang <sean.wang@mediatek.com> > >>> --- > >>> drivers/soc/mediatek/mtk-pmic-wrap.c | 91 +++++++++++++++++++++--------------- > >>> 1 file changed, 54 insertions(+), 37 deletions(-) > >>> > >>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> index 27d7ccc..9c6d855 100644 > >>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> @@ -531,6 +531,7 @@ struct pmic_wrapper_type { > >>> u32 spi_w; > >>> u32 wdt_src; > >>> int has_bridge:1; > >>> + int slv_program:1; > >>> int (*init_reg_clock)(struct pmic_wrapper *wrp); > >>> int (*init_soc_specific)(struct pmic_wrapper *wrp); > >>> }; > >>> @@ -999,9 +1000,12 @@ static int pwrap_init(struct pmic_wrapper *wrp) > >>> } > >>> > >>> /* Reset SPI slave */ > >>> - ret = pwrap_reset_spislave(wrp); > >>> - if (ret) > >>> - return ret; > >>> + > >>> + if (wrp->master->slv_program) { > >>> + ret = pwrap_reset_spislave(wrp); > >>> + if (ret) > >>> + return ret; > >>> + } > >>> > >>> pwrap_writel(wrp, 1, PWRAP_WRAP_EN); > >>> > >>> @@ -1013,45 +1017,52 @@ static int pwrap_init(struct pmic_wrapper *wrp) > >>> if (ret) > >>> return ret; > >>> > >>> - /* Setup serial input delay */ > >>> - ret = pwrap_init_sidly(wrp); > >>> - if (ret) > >>> - return ret; > >>> + if (wrp->master->slv_program) { > >> > >> This if branch is really long and complex enough to put it into function apart. > >> > >> Thanks, > >> Matthias > >> > >> PD please take into account the comments I made on v3 of the series. > >> > > > > I'll try to breakdown the long logic into the short one and use a flag > > indicating the slave capability decides whether the functions is > > required being enabled for the slave instead of slv_program which is > > less meaningful. In this way, pmic_init will be more extensible when > > more different SoCs and target slaves with various flavors into the > > driver. And also take into accounts those suggestions you made in v3 in > > the next version. > > > > Sean > > > > I totally agree, but I wanted to underline that right now the if branch under > "if (wrp->master->slv_program)" is around 30 lines, so I think it would be a > good candidate to put it into it's own function. For example: > pwrap_init_encryption() understood. I'll try to turn the code block into a function. > As from what I understand from the commit log, slv_program in the end enables > encryption of the communication, right? > yes. slv_program in the end will enable the security feature over the bus such as the encryption and the signature for the data integrity when the pwrap slave can support it. > Regards, > Matthias
diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c index 27d7ccc..9c6d855 100644 --- a/drivers/soc/mediatek/mtk-pmic-wrap.c +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c @@ -531,6 +531,7 @@ struct pmic_wrapper_type { u32 spi_w; u32 wdt_src; int has_bridge:1; + int slv_program:1; int (*init_reg_clock)(struct pmic_wrapper *wrp); int (*init_soc_specific)(struct pmic_wrapper *wrp); }; @@ -999,9 +1000,12 @@ static int pwrap_init(struct pmic_wrapper *wrp) } /* Reset SPI slave */ - ret = pwrap_reset_spislave(wrp); - if (ret) - return ret; + + if (wrp->master->slv_program) { + ret = pwrap_reset_spislave(wrp); + if (ret) + return ret; + } pwrap_writel(wrp, 1, PWRAP_WRAP_EN); @@ -1013,45 +1017,52 @@ static int pwrap_init(struct pmic_wrapper *wrp) if (ret) return ret; - /* Setup serial input delay */ - ret = pwrap_init_sidly(wrp); - if (ret) - return ret; + if (wrp->master->slv_program) { + /* Setup serial input delay */ + ret = pwrap_init_sidly(wrp); + if (ret) + return ret; - /* Enable dual IO mode */ - pwrap_write(wrp, wrp->slave->dew_regs[PWRAP_DEW_DIO_EN], 1); + /* Enable dual IO mode */ + pwrap_write(wrp, wrp->slave->dew_regs[PWRAP_DEW_DIO_EN], 1); - /* Check IDLE & INIT_DONE in advance */ - ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_idle_and_sync_idle); - if (ret) { - dev_err(wrp->dev, "%s fail, ret=%d\n", __func__, ret); - return ret; - } + /* Check IDLE & INIT_DONE in advance */ + ret = pwrap_wait_for_state(wrp, + pwrap_is_fsm_idle_and_sync_idle); + if (ret) { + dev_err(wrp->dev, "%s fail, ret=%d\n", __func__, ret); + return ret; + } - pwrap_writel(wrp, 1, PWRAP_DIO_EN); + pwrap_writel(wrp, 1, PWRAP_DIO_EN); - /* Read Test */ - pwrap_read(wrp, wrp->slave->dew_regs[PWRAP_DEW_READ_TEST], &rdata); - if (rdata != PWRAP_DEW_READ_TEST_VAL) { - dev_err(wrp->dev, "Read test failed after switch to DIO mode: 0x%04x != 0x%04x\n", + /* Read Test */ + pwrap_read(wrp, + wrp->slave->dew_regs[PWRAP_DEW_READ_TEST], &rdata); + if (rdata != PWRAP_DEW_READ_TEST_VAL) { + dev_err(wrp->dev, + "Read failed on DIO mode: 0x%04x!=0x%04x\n", PWRAP_DEW_READ_TEST_VAL, rdata); - return -EFAULT; - } - - /* Enable encryption */ - ret = pwrap_init_cipher(wrp); - if (ret) - return ret; + return -EFAULT; + } - /* Signature checking - using CRC */ - if (pwrap_write(wrp, wrp->slave->dew_regs[PWRAP_DEW_CRC_EN], 0x1)) - return -EFAULT; + /* Enable encryption */ + ret = pwrap_init_cipher(wrp); + if (ret) + return ret; - pwrap_writel(wrp, 0x1, PWRAP_CRC_EN); - pwrap_writel(wrp, 0x0, PWRAP_SIG_MODE); - pwrap_writel(wrp, wrp->slave->dew_regs[PWRAP_DEW_CRC_VAL], - PWRAP_SIG_ADR); - pwrap_writel(wrp, wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN); + /* Signature checking - using CRC */ + if (pwrap_write(wrp, + wrp->slave->dew_regs[PWRAP_DEW_CRC_EN], 0x1)) + return -EFAULT; + + pwrap_writel(wrp, 0x1, PWRAP_CRC_EN); + pwrap_writel(wrp, 0x0, PWRAP_SIG_MODE); + pwrap_writel(wrp, wrp->slave->dew_regs[PWRAP_DEW_CRC_VAL], + PWRAP_SIG_ADR); + pwrap_writel(wrp, + wrp->master->arb_en_all, PWRAP_HIPRIO_ARB_EN); + } if (wrp->master->type == PWRAP_MT8135) pwrap_writel(wrp, 0x7, PWRAP_RRARB_EN); @@ -1059,8 +1070,11 @@ static int pwrap_init(struct pmic_wrapper *wrp) pwrap_writel(wrp, 0x1, PWRAP_WACS0_EN); pwrap_writel(wrp, 0x1, PWRAP_WACS1_EN); pwrap_writel(wrp, 0x1, PWRAP_WACS2_EN); - pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD); - pwrap_writel(wrp, 0xff, PWRAP_STAUPD_GRPEN); + + if (wrp->master->slv_program) { + pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD); + pwrap_writel(wrp, 0xff, PWRAP_STAUPD_GRPEN); + } if (wrp->master->init_soc_specific) { ret = wrp->master->init_soc_specific(wrp); @@ -1146,6 +1160,7 @@ static const struct pmic_wrapper_type pwrap_mt2701 = { .spi_w = PWRAP_MAN_CMD_SPI_WRITE_NEW, .wdt_src = PWRAP_WDT_SRC_MASK_ALL, .has_bridge = 0, + .slv_program = 1, .init_reg_clock = pwrap_mt2701_init_reg_clock, .init_soc_specific = pwrap_mt2701_init_soc_specific, }; @@ -1158,6 +1173,7 @@ static const struct pmic_wrapper_type pwrap_mt8135 = { .spi_w = PWRAP_MAN_CMD_SPI_WRITE, .wdt_src = PWRAP_WDT_SRC_MASK_ALL, .has_bridge = 1, + .slv_program = 1, .init_reg_clock = pwrap_mt8135_init_reg_clock, .init_soc_specific = pwrap_mt8135_init_soc_specific, }; @@ -1170,6 +1186,7 @@ static const struct pmic_wrapper_type pwrap_mt8173 = { .spi_w = PWRAP_MAN_CMD_SPI_WRITE, .wdt_src = PWRAP_WDT_SRC_MASK_NO_STAUPD, .has_bridge = 0, + .slv_program = 1, .init_reg_clock = pwrap_mt8173_init_reg_clock, .init_soc_specific = pwrap_mt8173_init_soc_specific, };