| Message ID | 20210429073050.21039-17-peng.fan@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | soc: imx: gpcv2: support i.MX8MM | expand |
Am Donnerstag, dem 29.04.2021 um 15:30 +0800 schrieb Peng Fan (OSS): > From: Peng Fan <peng.fan@nxp.com> > > i.MX8MM has blk ctl module, the handshake can only finish after setting > blk ctl. The blk ctl driver will set the bus clk bit and the handshake > will finish there. we just add a delay and suppose the handshake will > finish after that. Instead of this patch, just drop patch 05/16 from this series. I think we should leave a comment in the code on why we aren't waiting for the handshake ack, as this is non-obvious from looking at the driver code. Basically add a polished version of the commit log from this patch into the driver code. Regards, Lucas > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/soc/imx/gpcv2.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c > index 49dd137f6b94..04564017bfe9 100644 > --- a/drivers/soc/imx/gpcv2.c > +++ b/drivers/soc/imx/gpcv2.c > @@ -251,14 +251,6 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd) > regmap_update_bits(domain->regmap, GPC_PU_PWRHSK, > domain->bits.hskreq, domain->bits.hskreq); > > > > > - ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PWRHSK, > - reg_val, > - (reg_val & domain->bits.hskack), > - 0, USEC_PER_MSEC); > - if (ret) { > - dev_err(domain->dev, "failed to power up ADB400\n"); > - goto out_clk_disable; > - } > } > > > > > /* Disable reset clocks for all devices in the domain */
On 2021/4/29 22:34, Lucas Stach wrote: > Am Donnerstag, dem 29.04.2021 um 15:30 +0800 schrieb Peng Fan (OSS): >> From: Peng Fan <peng.fan@nxp.com> >> >> i.MX8MM has blk ctl module, the handshake can only finish after setting >> blk ctl. The blk ctl driver will set the bus clk bit and the handshake >> will finish there. we just add a delay and suppose the handshake will >> finish after that. > > Instead of this patch, just drop patch 05/16 from this series. I think > we should leave a comment in the code on why we aren't waiting for the > handshake ack, as this is non-obvious from looking at the driver code. > > Basically add a polished version of the commit log from this patch into > the driver code. I see. Will do it in V2. Thanks, Peng. > > Regards, > Lucas > >> Signed-off-by: Peng Fan <peng.fan@nxp.com> >> --- >> drivers/soc/imx/gpcv2.c | 8 -------- >> 1 file changed, 8 deletions(-) >> >> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c >> index 49dd137f6b94..04564017bfe9 100644 >> --- a/drivers/soc/imx/gpcv2.c >> +++ b/drivers/soc/imx/gpcv2.c >> @@ -251,14 +251,6 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd) >> regmap_update_bits(domain->regmap, GPC_PU_PWRHSK, >> domain->bits.hskreq, domain->bits.hskreq); >> >> >> >> >> - ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PWRHSK, >> - reg_val, >> - (reg_val & domain->bits.hskack), >> - 0, USEC_PER_MSEC); >> - if (ret) { >> - dev_err(domain->dev, "failed to power up ADB400\n"); >> - goto out_clk_disable; >> - } >> } >> >> >> >> >> /* Disable reset clocks for all devices in the domain */ > >
On 2021/4/29 22:34, Lucas Stach wrote: > Am Donnerstag, dem 29.04.2021 um 15:30 +0800 schrieb Peng Fan (OSS): >> From: Peng Fan <peng.fan@nxp.com> >> >> i.MX8MM has blk ctl module, the handshake can only finish after setting >> blk ctl. The blk ctl driver will set the bus clk bit and the handshake >> will finish there. we just add a delay and suppose the handshake will >> finish after that. > > Instead of this patch, just drop patch 05/16 from this series. I think > we should leave a comment in the code on why we aren't waiting for the > handshake ack, as this is non-obvious from looking at the driver code. > > Basically add a polished version of the commit log from this patch into > the driver code. After thinking more, for power down, we need wait handshake. For power up, we could ignore handshake, because BLK-CTL setting bus clk en will auto trigger handshake. For power down, the bus clk en already set, and we need wait, otherwise we need add delay there. So I would only drop the power up waiting and add some comments there. Thanks, Peng. > > Regards, > Lucas > >> Signed-off-by: Peng Fan <peng.fan@nxp.com> >> --- >> drivers/soc/imx/gpcv2.c | 8 -------- >> 1 file changed, 8 deletions(-) >> >> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c >> index 49dd137f6b94..04564017bfe9 100644 >> --- a/drivers/soc/imx/gpcv2.c >> +++ b/drivers/soc/imx/gpcv2.c >> @@ -251,14 +251,6 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd) >> regmap_update_bits(domain->regmap, GPC_PU_PWRHSK, >> domain->bits.hskreq, domain->bits.hskreq); >> >> >> >> >> - ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PWRHSK, >> - reg_val, >> - (reg_val & domain->bits.hskack), >> - 0, USEC_PER_MSEC); >> - if (ret) { >> - dev_err(domain->dev, "failed to power up ADB400\n"); >> - goto out_clk_disable; >> - } >> } >> >> >> >> >> /* Disable reset clocks for all devices in the domain */ > >
Am Freitag, dem 30.04.2021 um 15:14 +0800 schrieb Peng Fan (OSS): > > On 2021/4/29 22:34, Lucas Stach wrote: > > Am Donnerstag, dem 29.04.2021 um 15:30 +0800 schrieb Peng Fan (OSS): > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > i.MX8MM has blk ctl module, the handshake can only finish after setting > > > blk ctl. The blk ctl driver will set the bus clk bit and the handshake > > > will finish there. we just add a delay and suppose the handshake will > > > finish after that. > > > > Instead of this patch, just drop patch 05/16 from this series. I think > > we should leave a comment in the code on why we aren't waiting for the > > handshake ack, as this is non-obvious from looking at the driver code. > > > > Basically add a polished version of the commit log from this patch into > > the driver code. > > After thinking more, for power down, we need wait handshake. For power > up, we could ignore handshake, because BLK-CTL setting bus clk en > will auto trigger handshake. > > For power down, the bus clk en already set, and we need wait, otherwise > we need add delay there. > > So I would only drop the power up waiting and add some comments there. Ah, right. This way makes a lot of sense. Regards, Lucas
diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c index 49dd137f6b94..04564017bfe9 100644 --- a/drivers/soc/imx/gpcv2.c +++ b/drivers/soc/imx/gpcv2.c @@ -251,14 +251,6 @@ static int imx_pgc_power_up(struct generic_pm_domain *genpd) regmap_update_bits(domain->regmap, GPC_PU_PWRHSK, domain->bits.hskreq, domain->bits.hskreq); - ret = regmap_read_poll_timeout(domain->regmap, GPC_PU_PWRHSK, - reg_val, - (reg_val & domain->bits.hskack), - 0, USEC_PER_MSEC); - if (ret) { - dev_err(domain->dev, "failed to power up ADB400\n"); - goto out_clk_disable; - } } /* Disable reset clocks for all devices in the domain */