[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Mark Brown [mailto:broonie at opensource.wolfsonmicro.com]
> Sent: Tuesday, August 02, 2011 4:39 PM
> To: Dong Aisheng-B29396
> Cc: alsa-devel at alsa-project.org; linux-arm-kernel at lists.infradead.org;
> lrg at ti.com; s.hauer at pengutronix.de; w.sang at pengutronix.de
> Subject: Re: [PATCH 1/1] ASoC: core: cache index fix
> 
> On Tue, Aug 02, 2011 at 08:03:23AM +0000, Dong Aisheng-B29396 wrote:
> 
> 
> > > ...hw_read() shouldn't need to know about this stuff
> > Here the reg_cache_size is the maximum cache index in the register
> cache array.
> > Therefore, the original code using reg to compare with reg_cache_size
> > is not correct when the reg_cache_step is not 1.
> > e.g. the reg to read is 0x30 and reg_cache_size maybe only 0x16.
> > So we use idx to check if it exceeds the cache size.
> 
> I see what you're doing, but like I say this is just legacy code that
> shouldn't be peering at the cache size any more and layering additional
> stuff in is just going to make the situation worse.
> 
> > BTW, do you mean the soc_io layer does not need to know the details of
> > idx&reg Conversion?
> 
> Yes.
> 
> > Do I need to implement a help function like reg_is_cachable(reg) to be
> used here?
> 
> No, I think we should hide these decisions completely within the cache
> code.

Yes, but the issue is that hw_read will check if reg is in cache array
And checking like " if (reg >= codec->driver->reg_cache_size ||" is wrong
when the step is not 1 that will cause registers with their index are greater
than cache index will not be able to fetch data from cache.

If we high this in cache code, do you mean hide it in snd_soc_cache_read?
If that, the hw_read may fail and it looks not as we expected.

@@ -62,8 +65,11 @@ static unsigned int hw_read(struct snd_soc_codec *codec, unsigned int reg)  {
 	int ret;
 	unsigned int val;
+	unsigned int idx;
+
+	idx = snd_soc_cache_reg_to_idx(codec, reg);
 
-	if (reg >= codec->driver->reg_cache_size ||
+	if (idx >= codec->driver->reg_cache_size ||
 	    snd_soc_codec_volatile_register(codec, reg) ||
 	    codec->cache_bypass) {
 		if (codec->cache_only)


> > The cache block in each rbnode is still using cache index to fetch
> > value which is similar like flat cache.
> > (see snd_soc_rbtree_get_register & snd_soc_rbtree_set_register).
> > Additionally, the snd_soc_rbtree_cache_init using cache index to do
> > init by default. However, the rbtree_cache_read/write still use reg to
> index.
> 
> This doesn't mean it's a good idea to add extra complexity here!  
I agree.

> A
> register map with step size greater than one should never have more than
> one register in a block anyway with the current code so the step size
> should never be perceptible.
The question is that rbtree uses reg index to index as follows:
        if (rbnode) {
                snd_soc_rbtree_get_base_top_reg(rbnode, &base_reg, &top_reg);
                if (reg >= base_reg && reg <= top_reg) {
                        reg_tmp = reg - base_reg;
                        *value = snd_soc_rbtree_get_register(rbnode, reg_tmp);
                        return 0;
                }
        }
So the block may be reg0, reg2, reg4.....
And the block is flat, the value is get/set by rbnode->block[reg_tmp]:
I understand right, there may be hole in the block, right?

> > The main problem is that the default cache array is indexed by cache
> > index like cache[idx] while all io function using reg to fetch data.
> > Many places in cache core miss used cache index and reg index.
> 
> > The purpose of this patch is to use both of them in correct places.
> > The simple rule is converted to cache index to fetch data from cache
> > when do Register io.
> 
> We need to deal with this sanely, dancing around with step sizes in every
> place where we access registers is just not going to be maintainable.
> Essentially no modern hardware uses step sizes other than one so they'll
> get very little testing, especially with the advanced cache mechanisms
> which aren't really useful on older CODECs, and the additional complexity
> really hurts legibility.
Yes, I agree that we should not introduce too much additional complexity.
The better case may be that only change the rbtree init code to get correct
Register value for the registers. Then the rbtree_cache_read/write can index
the register correctly.
(I also think the rbtree cache should not know step)

Then the only complexity is checking reg index for flat cache read/write
But that is really needed for different cache step of flat cache.

> It occurs to me that what we want to do here may be to make a new
> register cache type for step sizes then hide all the complexity for these
> devices in there.  That keeps everything together and ensures that newer
> devices don't pay a complexity cost.
If we add new cache type, does it have any big difference with FLAT?
Maybe if we can fix the cache step issue, the FLAT cache can be reuse.

> For the register default tables it's probably sensible to just pad them
> out with the zero registers; it'll cost a little space but not too much.
Yes, it's the most easy way now.