| Message ID | 1489990547-1510-3-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng: > Without providing the proper reg_defaults, the regmap registers first > read out may be always 0 if enabling cache, which results in the > following issue we met. > e.g. During driver probe in imx6_pm_domain_power_on(): > regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val); > The PGC_PUPSCR register val is always 0 but it's actually 0xf01 in HW. > > Since GPC registers are tightly related to CPU bring up and may be > changed in bootloader, we don't want to provide defaults. > And the cache really does not save too much for GPC module. > > Therefore, simply disable cache to fix the issue and make life easy. While I agree that not using the cache may be the right thing to do here, bypassing the cache by not providing the volatile function is only half the fix. Please also set the cache type to REGCACHE_NONE, to make it clear that the cache isn't used. Regards, Lucas > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Shawn Guo <shawnguo@kernel.org> > Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver") > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > --- > drivers/soc/imx/gpc.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c > index c9bfdfd..7e6a672 100644 > --- a/drivers/soc/imx/gpc.c > +++ b/drivers/soc/imx/gpc.c > @@ -289,22 +289,12 @@ static bool imx_gpc_readable_reg(struct device *dev, unsigned int reg) > return (reg % 4 == 0) && (reg <= 0x2ac); > } > > -static bool imx_gpc_volatile_reg(struct device *dev, unsigned int reg) > -{ > - if (reg == GPC_CNTR) > - return true; > - > - return false; > -} > - > static const struct regmap_config imx_gpc_regmap_config = { > - .cache_type = REGCACHE_FLAT, > .reg_bits = 32, > .val_bits = 32, > .reg_stride = 4, > > .readable_reg = imx_gpc_readable_reg, > - .volatile_reg = imx_gpc_volatile_reg, > > .max_register = 0x2ac, > };
Am Donnerstag, den 23.03.2017, 02:53 +0800 schrieb Dong Aisheng: > Hi Lucas, > > Thanks for the review. > > On Mon, Mar 20, 2017 at 10:38:21AM +0100, Lucas Stach wrote: > > Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng: > > > Without providing the proper reg_defaults, the regmap registers first > > > read out may be always 0 if enabling cache, which results in the > > > following issue we met. > > > e.g. During driver probe in imx6_pm_domain_power_on(): > > > regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val); > > > The PGC_PUPSCR register val is always 0 but it's actually 0xf01 in HW. > > > > > > Since GPC registers are tightly related to CPU bring up and may be > > > changed in bootloader, we don't want to provide defaults. > > > And the cache really does not save too much for GPC module. > > > > > > Therefore, simply disable cache to fix the issue and make life easy. > > > > While I agree that not using the cache may be the right thing to do > > here, bypassing the cache by not providing the volatile function is only > > half the fix. > > > > Removing volatile function actually is only due to it becomes meaningless > after disable cache, not used to bypass or fix the cache issue. > > > Please also set the cache type to REGCACHE_NONE, to make it clear that > > the cache isn't used. > > > > I believe all guys normally may know the regmap cache is disabled by default > if without specifying cache_type, this way is also commonly used in kernel. > So i just simply remove it. > Another bonus is it fully hides the cache bits from drive user. > No one needs to know it if not use. > > Anyway, if you insist on it, i could explicitly claim the cache type to > REGCACHE_NONE in V2. Um, no thanks. I claim lack of coffee for my first review. This patch is Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > > > > > Cc: Lucas Stach <l.stach@pengutronix.de> > > > Cc: Shawn Guo <shawnguo@kernel.org> > > > Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver") > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > > --- > > > drivers/soc/imx/gpc.c | 10 ---------- > > > 1 file changed, 10 deletions(-) > > > > > > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c > > > index c9bfdfd..7e6a672 100644 > > > --- a/drivers/soc/imx/gpc.c > > > +++ b/drivers/soc/imx/gpc.c > > > @@ -289,22 +289,12 @@ static bool imx_gpc_readable_reg(struct device *dev, unsigned int reg) > > > return (reg % 4 == 0) && (reg <= 0x2ac); > > > } > > > > > > -static bool imx_gpc_volatile_reg(struct device *dev, unsigned int reg) > > > -{ > > > - if (reg == GPC_CNTR) > > > - return true; > > > - > > > - return false; > > > -} > > > - > > > static const struct regmap_config imx_gpc_regmap_config = { > > > - .cache_type = REGCACHE_FLAT, > > > .reg_bits = 32, > > > .val_bits = 32, > > > .reg_stride = 4, > > > > > > .readable_reg = imx_gpc_readable_reg, > > > - .volatile_reg = imx_gpc_volatile_reg, > > > > > > .max_register = 0x2ac, > > > }; > > > >
Hi Lucas, Thanks for the review. On Mon, Mar 20, 2017 at 10:38:21AM +0100, Lucas Stach wrote: > Am Montag, den 20.03.2017, 14:15 +0800 schrieb Dong Aisheng: > > Without providing the proper reg_defaults, the regmap registers first > > read out may be always 0 if enabling cache, which results in the > > following issue we met. > > e.g. During driver probe in imx6_pm_domain_power_on(): > > regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val); > > The PGC_PUPSCR register val is always 0 but it's actually 0xf01 in HW. > > > > Since GPC registers are tightly related to CPU bring up and may be > > changed in bootloader, we don't want to provide defaults. > > And the cache really does not save too much for GPC module. > > > > Therefore, simply disable cache to fix the issue and make life easy. > > While I agree that not using the cache may be the right thing to do > here, bypassing the cache by not providing the volatile function is only > half the fix. > Removing volatile function actually is only due to it becomes meaningless after disable cache, not used to bypass or fix the cache issue. > Please also set the cache type to REGCACHE_NONE, to make it clear that > the cache isn't used. > I believe all guys normally may know the regmap cache is disabled by default if without specifying cache_type, this way is also commonly used in kernel. So i just simply remove it. Another bonus is it fully hides the cache bits from drive user. No one needs to know it if not use. Anyway, if you insist on it, i could explicitly claim the cache type to REGCACHE_NONE in V2. Thanks Regards Dong Aisheng > Regards, > Lucas > > > Cc: Lucas Stach <l.stach@pengutronix.de> > > Cc: Shawn Guo <shawnguo@kernel.org> > > Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver") > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > --- > > drivers/soc/imx/gpc.c | 10 ---------- > > 1 file changed, 10 deletions(-) > > > > diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c > > index c9bfdfd..7e6a672 100644 > > --- a/drivers/soc/imx/gpc.c > > +++ b/drivers/soc/imx/gpc.c > > @@ -289,22 +289,12 @@ static bool imx_gpc_readable_reg(struct device *dev, unsigned int reg) > > return (reg % 4 == 0) && (reg <= 0x2ac); > > } > > > > -static bool imx_gpc_volatile_reg(struct device *dev, unsigned int reg) > > -{ > > - if (reg == GPC_CNTR) > > - return true; > > - > > - return false; > > -} > > - > > static const struct regmap_config imx_gpc_regmap_config = { > > - .cache_type = REGCACHE_FLAT, > > .reg_bits = 32, > > .val_bits = 32, > > .reg_stride = 4, > > > > .readable_reg = imx_gpc_readable_reg, > > - .volatile_reg = imx_gpc_volatile_reg, > > > > .max_register = 0x2ac, > > }; > >
diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c index c9bfdfd..7e6a672 100644 --- a/drivers/soc/imx/gpc.c +++ b/drivers/soc/imx/gpc.c @@ -289,22 +289,12 @@ static bool imx_gpc_readable_reg(struct device *dev, unsigned int reg) return (reg % 4 == 0) && (reg <= 0x2ac); } -static bool imx_gpc_volatile_reg(struct device *dev, unsigned int reg) -{ - if (reg == GPC_CNTR) - return true; - - return false; -} - static const struct regmap_config imx_gpc_regmap_config = { - .cache_type = REGCACHE_FLAT, .reg_bits = 32, .val_bits = 32, .reg_stride = 4, .readable_reg = imx_gpc_readable_reg, - .volatile_reg = imx_gpc_volatile_reg, .max_register = 0x2ac, };
Without providing the proper reg_defaults, the regmap registers first read out may be always 0 if enabling cache, which results in the following issue we met. e.g. During driver probe in imx6_pm_domain_power_on(): regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val); The PGC_PUPSCR register val is always 0 but it's actually 0xf01 in HW. Since GPC registers are tightly related to CPU bring up and may be changed in bootloader, we don't want to provide defaults. And the cache really does not save too much for GPC module. Therefore, simply disable cache to fix the issue and make life easy. Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Shawn Guo <shawnguo@kernel.org> Fixes: 721cabf6c660 ("soc: imx: move PGC handling to a new GPC driver") Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- drivers/soc/imx/gpc.c | 10 ---------- 1 file changed, 10 deletions(-)