Having no cache defaults is not a mistake, especially for devices
which do have some kind of strapping values which can't be provided
statically. For this case a warning is the wrong log level, the initial
cache values do need to be read form hardware. Reduce level to info.
Signed-off-by: Alexander Stein <[email protected]>
---
drivers/base/regmap/regcache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 362e043e26d8..89b92eda8b02 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -54,7 +54,7 @@ static int regcache_hw_init(struct regmap *map)
if (!map->reg_defaults_raw) {
bool cache_bypass = map->cache_bypass;
- dev_warn(map->dev, "No cache defaults, reading back from HW\n");
+ dev_info(map->dev, "No cache defaults, reading back from HW\n");
/* Bypass the cache access till data read from HW */
map->cache_bypass = true;
--
2.25.1
On Wed, Sep 21, 2022 at 09:10:54AM +0200, Alexander Stein wrote:
> Having no cache defaults is not a mistake, especially for devices
> which do have some kind of strapping values which can't be provided
> statically. For this case a warning is the wrong log level, the initial
> cache values do need to be read form hardware. Reduce level to info.
> if (!map->reg_defaults_raw) {
> bool cache_bypass = map->cache_bypass;
> - dev_warn(map->dev, "No cache defaults, reading back from HW\n");
> + dev_info(map->dev, "No cache defaults, reading back from HW\n");
This is a warning which is only generated in the case where we have a
non-sparse cache where we won't read from the hardware in the csae of
missing values and will instead just substitute zero as a default value.
It's warning that we might end up changing values for the device
unexpectedly in the case of a read/modify/write cycle that doesn't
actually check what the device currently has set.
In this case if you can't use a rbtree cache I would recommend
bootstrapping by initially reading without a cache first to get the
default values, then replacing the regmap with one that uses the
defaults you just read. You'll need some care over suspend/resume
though.
Hello Mark,
Am Freitag, 25. November 2022, 20:30:52 CET schrieb Mark Brown:
> * PGP Signed by an unknown key
>
> On Wed, Sep 21, 2022 at 09:10:54AM +0200, Alexander Stein wrote:
> > Having no cache defaults is not a mistake, especially for devices
> > which do have some kind of strapping values which can't be provided
> > statically. For this case a warning is the wrong log level, the initial
> > cache values do need to be read form hardware. Reduce level to info.
> >
> > if (!map->reg_defaults_raw) {
> >
> > bool cache_bypass = map->cache_bypass;
> >
> > - dev_warn(map->dev, "No cache defaults, reading back from
HW\n");
> > + dev_info(map->dev, "No cache defaults, reading back from
HW\n");
>
> This is a warning which is only generated in the case where we have a
> non-sparse cache where we won't read from the hardware in the csae of
> missing values and will instead just substitute zero as a default value.
> It's warning that we might end up changing values for the device
> unexpectedly in the case of a read/modify/write cycle that doesn't
> actually check what the device currently has set.
>
> In this case if you can't use a rbtree cache I would recommend
> bootstrapping by initially reading without a cache first to get the
> default values, then replacing the regmap with one that uses the
> defaults you just read. You'll need some care over suspend/resume
> though.
The driver I'm working on clk-renesas-pcie.c only has 8 registers, so there is
no benefit to use a different cache than REGCACHE_FLAT.
But I currently fail to see why this is only for non-sparse caches. This is
printed even before the cache_ops init() is called.
Before the call to regcache_hw_init() there is this comment:
> /* Some devices such as PMICs don't have cache defaults,
> * we cope with this by reading back the HW registers and
> * crafting the cache defaults by hand.
> */
This is exactly what I want to do: Read back HW registers and create a cache.
Actually regcache_hw_init() is exactly doing what you are suggesting:
1. reading HW registers into map->reg_defaults_raw
2.
a) copy them into map->reg_defaults
b) read HW register individually if 1. failed
So thinking more about this, this warning should be removed altogether IMHO.
Best regards,
Alexander
On Mon, Nov 28, 2022 at 03:58:40PM +0100, Alexander Stein wrote:
> Am Freitag, 25. November 2022, 20:30:52 CET schrieb Mark Brown:
> > In this case if you can't use a rbtree cache I would recommend
> > bootstrapping by initially reading without a cache first to get the
> > default values, then replacing the regmap with one that uses the
> > defaults you just read. You'll need some care over suspend/resume
> > though.
> The driver I'm working on clk-renesas-pcie.c only has 8 registers, so there is
> no benefit to use a different cache than REGCACHE_FLAT.
There's a benefit when it comes to bootstrapping (though also a slight
performance cost, and non-atomicity).
> But I currently fail to see why this is only for non-sparse caches. This is
> printed even before the cache_ops init() is called.
We only go into hw_init() in the case where the map has specified
num_reg_defaults_raw, as you say you could trigger that for any cache
type but if you're using a sparse cache you're either going to provide
no defaults or provide defaults in the standard, sparse format rather
than the raw format so in practice it won't trigger.
> Before the call to regcache_hw_init() there is this comment:
> > /* Some devices such as PMICs don't have cache defaults,
> > * we cope with this by reading back the HW registers and
> > * crafting the cache defaults by hand.
> > */
> This is exactly what I want to do: Read back HW registers and create a cache.
> Actually regcache_hw_init() is exactly doing what you are suggesting:
> 1. reading HW registers into map->reg_defaults_raw
> 2.
> a) copy them into map->reg_defaults
> b) read HW register individually if 1. failed
The other thing with doing a readback to fill the cache is that it's a
potentially slow operation (depending on how big the cache is and the
bus in use) and also a potentially destructive one for things like clear
on read registers. If regmaps are doing this deliberately (which does
seem valid) we probably want a clearer mechanism for specifying it - I
think an explicit flag would do the trick.