Re: [alsa-devel] snd soc spi read/write
Why are you replying off-list? You should implement 16x8 SPI reads in the core stuff I'd expect, then disable cache for the CODEC.
Sorry, I just forgot to CC. I wonder if rbtree compress type can support this kind of sparse register? And should the patch aginst 3.0 or 3.2?
On Fri, Aug 05, 2011 at 04:00:52PM +0800, Scott Jiang wrote:
Why are you replying off-list? You should implement 16x8 SPI reads in the core stuff I'd expect, then disable cache for the CODEC.
Sorry, I just forgot to CC. I wonder if rbtree compress type can support this kind of sparse register?
Yes, rbtree works fine with this. You'd still take a hit on the size of your register defaults though.
And should the patch aginst 3.0 or 3.2?
3.0 can't be patched, though you could try submitting a patch to the stable guys. Probably best to start off with 3.1 and see where we go from there.
2011/8/5 Mark Brown broonie@opensource.wolfsonmicro.com:
On Fri, Aug 05, 2011 at 04:00:52PM +0800, Scott Jiang wrote:
Why are you replying off-list? You should implement 16x8 SPI reads in the core stuff I'd expect, then disable cache for the CODEC.
Sorry, I just forgot to CC. I wonder if rbtree compress type can support this kind of sparse register?
Yes, rbtree works fine with this. You'd still take a hit on the size of your register defaults though.
And should the patch aginst 3.0 or 3.2?
3.0 can't be patched, though you could try submitting a patch to the stable guys. Probably best to start off with 3.1 and see where we go from there.
I got a problem here. As Takashi mentioned, our registers have a read/write bit in the upper 8bit. There are three methods: 1. pass different registers to snd_soc_read and snd_soc_write. But snd_kcontrol and snd_soc_dapm_widget can't work because I pass only one register. 2. deal with this bit in hw_read, but this will be deprecated by others whose chip doesn't have this bit. 3. I'd like to use SND_SOC_CUS type, but it has been removed since linux 3.0. I suggest we can reserve this type, considering SPI is a simple "de facto" standard.
On Tue, Aug 09, 2011 at 11:41:30AM +0800, Scott Jiang wrote:
There are three methods:
- pass different registers to snd_soc_read and snd_soc_write. But
snd_kcontrol and snd_soc_dapm_widget can't work because I pass only one register. 2. deal with this bit in hw_read, but this will be deprecated by others whose chip doesn't have this bit. 3. I'd like to use SND_SOC_CUS type, but it has been removed since linux 3.0. I suggest we can reserve this type, considering SPI is a simple "de facto" standard.
No, like I say we just need to teach regmap about this stuff. It's not that odd, it's just your hardware designers seem to want to consume extra bandwidth on the control bus for some reason AFAICT as it looks like all the registers are 0x8xx.
At Wed, 10 Aug 2011 01:04:48 +0900, Mark Brown wrote:
On Tue, Aug 09, 2011 at 11:41:30AM +0800, Scott Jiang wrote:
There are three methods:
- pass different registers to snd_soc_read and snd_soc_write. But
snd_kcontrol and snd_soc_dapm_widget can't work because I pass only one register. 2. deal with this bit in hw_read, but this will be deprecated by others whose chip doesn't have this bit. 3. I'd like to use SND_SOC_CUS type, but it has been removed since linux 3.0. I suggest we can reserve this type, considering SPI is a simple "de facto" standard.
No, like I say we just need to teach regmap about this stuff. It's not that odd, it's just your hardware designers seem to want to consume extra bandwidth on the control bus for some reason AFAICT as it looks like all the registers are 0x8xx.
Actually we should handle the register index only in the lower byte for these devices. For example, ad193x has raw registers ranged from 0 to 0x10 with 0x800 high byte, while cs4271 has from 0 to 0x08 with 0x2000 high byte. So the raw register fits with flat array cache well.
And, 16bit register value is needed only for SPI. For I2C, the upper byte is anyway dropped.
Takashi
On Wed, Aug 10, 2011 at 01:54:27PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
No, like I say we just need to teach regmap about this stuff. It's not that odd, it's just your hardware designers seem to want to consume extra bandwidth on the control bus for some reason AFAICT as it looks like all the registers are 0x8xx.
Actually we should handle the register index only in the lower byte for these devices. For example, ad193x has raw registers ranged from
That would fall within the definition of "teach the register map about this stuff"...
At Wed, 10 Aug 2011 23:55:50 +0900, Mark Brown wrote:
On Wed, Aug 10, 2011 at 01:54:27PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
No, like I say we just need to teach regmap about this stuff. It's not that odd, it's just your hardware designers seem to want to consume extra bandwidth on the control bus for some reason AFAICT as it looks like all the registers are 0x8xx.
Actually we should handle the register index only in the lower byte for these devices. For example, ad193x has raw registers ranged from
That would fall within the definition of "teach the register map about this stuff"...
Yeah, I expected that. But it also would include the changes of register definitions in the codec driver, right?
Takashi
On Wed, Aug 10, 2011 at 05:00:50PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
That would fall within the definition of "teach the register map about this stuff"...
Yeah, I expected that. But it also would include the changes of register definitions in the codec driver, right?
The idea is that the CODEC drivers will end up using regmap directly once it gets cache support migrated over to it. There's nothing ALSA specific about the cache support. Though right just not bothering to cache this device (it has readback support after all) is probably good enough.
It's a shame the bodge snuck through :/
At Thu, 11 Aug 2011 00:03:18 +0900, Mark Brown wrote:
On Wed, Aug 10, 2011 at 05:00:50PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
That would fall within the definition of "teach the register map about this stuff"...
Yeah, I expected that. But it also would include the changes of register definitions in the codec driver, right?
The idea is that the CODEC drivers will end up using regmap directly once it gets cache support migrated over to it. There's nothing ALSA specific about the cache support. Though right just not bothering to cache this device (it has readback support after all) is probably good enough.
OK, I like the idea, but it sounds a bit like a long way to go. I guess the cache-in-regmap won't be merged in 3.1 cycle?
Basically I don't care too much about this, but the fact we leave this being broken over two release cycles doesn't appear nice, especially when there is a quick-n-easy fix...
thanks,
Takashi
On Wed, Aug 10, 2011 at 05:15:21PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
The idea is that the CODEC drivers will end up using regmap directly once it gets cache support migrated over to it. There's nothing ALSA specific about the cache support. Though right just not bothering to cache this device (it has readback support after all) is probably good enough.
OK, I like the idea, but it sounds a bit like a long way to go. I guess the cache-in-regmap won't be merged in 3.1 cycle?
Well, regmap is only in 3.2.
Basically I don't care too much about this, but the fact we leave this being broken over two release cycles doesn't appear nice, especially when there is a quick-n-easy fix...
I don't see a problem with the idea of just making the registers volatile. There's no real need to cache the registers on a small SPI device with readback support, the caches mainly benefit I2C (which is much slower) and devices with no readback support with some other benefits for larger devices.
That ought to work for all versions of the framework and keeps the driver looking like a standard driver. There's existing stuff with the driver doing non-standard things like the fixed default setup which have been around for quite some time and I'd rather reduce that sort of stuff.
At Thu, 11 Aug 2011 00:34:58 +0900, Mark Brown wrote:
On Wed, Aug 10, 2011 at 05:15:21PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
The idea is that the CODEC drivers will end up using regmap directly once it gets cache support migrated over to it. There's nothing ALSA specific about the cache support. Though right just not bothering to cache this device (it has readback support after all) is probably good enough.
OK, I like the idea, but it sounds a bit like a long way to go. I guess the cache-in-regmap won't be merged in 3.1 cycle?
Well, regmap is only in 3.2.
Oh, it's merged in 3.1 :) Just unused by no one, yeah.
Basically I don't care too much about this, but the fact we leave this being broken over two release cycles doesn't appear nice, especially when there is a quick-n-easy fix...
I don't see a problem with the idea of just making the registers volatile. There's no real need to cache the registers on a small SPI device with readback support, the caches mainly benefit I2C (which is much slower) and devices with no readback support with some other benefits for larger devices.
Hey, I'm not against it at all. What I do care is to see a fix soonish.
It'd be a quick fix that shall be removed in near future by regmap after all, so we don't need to waste time by discussing too much about it.
So, go ahead as you like, and let's fix it quickly in a minimal way.
thanks,
Takashi
On Wed, Aug 10, 2011 at 06:02:33PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
OK, I like the idea, but it sounds a bit like a long way to go. I guess the cache-in-regmap won't be merged in 3.1 cycle?
Well, regmap is only in 3.2.
Oh, it's merged in 3.1 :) Just unused by no one, yeah.
I meant regmap ASoC support.
It'd be a quick fix that shall be removed in near future by regmap after all, so we don't need to waste time by discussing too much about it.
That's why I don't want to add stuff in the driver, I would like to avoid adding more things in the driver that need removing because I don't have confidence that we'll easily be able to back out the workaround later on if the driver's not really being looked after.
On 08/10/2011 05:34 PM, Mark Brown wrote:
On Wed, Aug 10, 2011 at 05:15:21PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
The idea is that the CODEC drivers will end up using regmap directly once it gets cache support migrated over to it. There's nothing ALSA specific about the cache support. Though right just not bothering to cache this device (it has readback support after all) is probably good enough.
OK, I like the idea, but it sounds a bit like a long way to go. I guess the cache-in-regmap won't be merged in 3.1 cycle?
Well, regmap is only in 3.2.
Basically I don't care too much about this, but the fact we leave this being broken over two release cycles doesn't appear nice, especially when there is a quick-n-easy fix...
I don't see a problem with the idea of just making the registers volatile. There's no real need to cache the registers on a small SPI device with readback support, the caches mainly benefit I2C (which is much slower) and devices with no readback support with some other benefits for larger devices.
The problem is that there is no read-back support out of the box. Readback requires setting the read bit in the registers address. Since this is not the upper-most bit, the default regmap spi read wont work.
And if we have to add our own read function we could as very well add our own write function which simply reinstates the old caching behavior.
In my opinion it would be nice if we could add a register cache base address, which specifies the offset at which the cache-able registers start. For example I have a codec driver in the queue where all non-volatile registers at 0x8000 and I don't really want to add 16k of zeros to the driver for the default register cache.
- Lars
On Wed, Aug 10, 2011 at 11:31:06PM +0200, Lars-Peter Clausen wrote:
The problem is that there is no read-back support out of the box. Readback requires setting the read bit in the registers address. Since this is not the upper-most bit, the default regmap spi read wont work.
Ah, so read and write aren't symmetric? That wasn't clear.
That's not complicated, though - like I'm sure I've said to you before it seems like we just need to make the read bit controllable by drivers. Some other devices need that too, and a shift of the address also (there's one 7 bit address device I saw recently which has the address in the top 7 bits of an 8 bit value).
Like I say I'm really not happy about adding further non-standard driver specifics to the Analog drivers, they're already not the best and they don't really seem to be getting much attention from anyone so I'm not confident anyone will come along and reverse any temporary bodges. I'd be reasonably happy with something temporary for 3.1 if we already have a fix in place for 3.2 but I don't have much confidence that anyone will work on that.
And if we have to add our own read function we could as very well add our own write function which simply reinstates the old caching behavior.
If the driver needs its own custom I/O it should do both read and write.
In my opinion it would be nice if we could add a register cache base address, which specifies the offset at which the cache-able registers start. For example I have a codec driver in the queue where all non-volatile registers at 0x8000 and I don't really want to add 16k of zeros to the driver for the default register cache.
I don't think it's worth adding a special case like that when fixing the more general issues for sparse registers also solves these problems - we still need to fix the sparse register maps anyway. Blocking registers together in rbtree (which we do already) means that if you've got one block of registers at a massive offset then the block with an offset just flops out of the code. If we also provide a more compact way of representing the defaults that devices with sparse registers can use then that problem will go away too.
On 08/11/2011 02:33 AM, Mark Brown wrote:
On Wed, Aug 10, 2011 at 11:31:06PM +0200, Lars-Peter Clausen wrote:
That's not complicated, though - like I'm sure I've said to you before it seems like we just need to make the read bit controllable by drivers. Some other devices need that too, and a shift of the address also (there's one 7 bit address device I saw recently which has the address in the top 7 bits of an 8 bit value).
Yes, I think I brought it up during the regmap review.
Like I say I'm really not happy about adding further non-standard driver specifics to the Analog drivers, they're already not the best and they don't really seem to be getting much attention from anyone so I'm not confident anyone will come along and reverse any temporary bodges. I'd be reasonably happy with something temporary for 3.1 if we already have a fix in place for 3.2 but I don't have much confidence that anyone will work on that.
So, just switch the ad193x driver to a rbcache.
And if we have to add our own read function we could as very well add our own write function which simply reinstates the old caching behavior.
If the driver needs its own custom I/O it should do both read and write.
We don't need reads if we cache writes. In the old ASoC IO code there wasn't even SPI read support.
In my opinion it would be nice if we could add a register cache base address, which specifies the offset at which the cache-able registers start. For example I have a codec driver in the queue where all non-volatile registers at 0x8000 and I don't really want to add 16k of zeros to the driver for the default register cache.
I don't think it's worth adding a special case like that when fixing the more general issues for sparse registers also solves these problems - we still need to fix the sparse register maps anyway. Blocking registers together in rbtree (which we do already) means that if you've got one block of registers at a massive offset then the block with an offset just flops out of the code.
Yes, I had a look at the rbcache the other day. The current code doesn't seem to be optimal. For example adjacent don't seem to be joined, so for example if I have 3 registers and write them in the order 0,2,1 I'll still end up with 2 blocks. But that's of course something that can be fixed. And I had almost been sold on it, if there wasn't the default register issue.
If we also provide a more compact way of representing the defaults that devices with sparse registers can use then that problem will go away too.
What do you have in mind for this? An array of pairs of register and value?
- Lars
On Thu, Aug 11, 2011 at 03:50:33AM +0200, Lars-Peter Clausen wrote:
On 08/11/2011 02:33 AM, Mark Brown wrote:
Like I say I'm really not happy about adding further non-standard driver specifics to the Analog drivers, they're already not the best and they don't really seem to be getting much attention from anyone so I'm not confident anyone will come along and reverse any temporary bodges. I'd be reasonably happy with something temporary for 3.1 if we already have a fix in place for 3.2 but I don't have much confidence that anyone will work on that.
So, just switch the ad193x driver to a rbcache.
That's one of the suggestions I made earlier, obviously it's painful for the register default table at present though.
And if we have to add our own read function we could as very well add our own write function which simply reinstates the old caching behavior.
If the driver needs its own custom I/O it should do both read and write.
We don't need reads if we cache writes. In the old ASoC IO code there wasn't even SPI read support.
Yeah, that's for the volatile case where we skip having a cache at all.
Yes, I had a look at the rbcache the other day. The current code doesn't seem to be optimal. For example adjacent don't seem to be joined, so for example if I have 3 registers and write them in the order 0,2,1 I'll still end up with 2 blocks. But that's of course something that can be fixed. And I had almost been sold on it, if there wasn't the default register issue.
None of the current ASoC code will coalesce register writes at all, and in the case where you're doing writes to registers that aren't actually adjacent it's going to be marginal if it's better to transmit the intervening register or transmit another register address. That only really makes a difference during cache sync anyway.
If we also provide a more compact way of representing the defaults that devices with sparse registers can use then that problem will go away too.
What do you have in mind for this? An array of pairs of register and value?
Yes, as I said in one of the earlier messages in this thread. It seems like a good combination of being writable/legible and compact.
On 08/11/2011 04:46 AM, Mark Brown wrote:
On Thu, Aug 11, 2011 at 03:50:33AM +0200, Lars-Peter Clausen wrote:
On 08/11/2011 02:33 AM, Mark Brown wrote:
[...]
Yes, I had a look at the rbcache the other day. The current code doesn't seem to be optimal. For example adjacent don't seem to be joined, so for example if I have 3 registers and write them in the order 0,2,1 I'll still end up with 2 blocks. But that's of course something that can be fixed. And I had almost been sold on it, if there wasn't the default register issue.
None of the current ASoC code will coalesce register writes at all, and in the case where you're doing writes to registers that aren't actually adjacent it's going to be marginal if it's better to transmit the intervening register or transmit another register address. That only really makes a difference during cache sync anyway.
I was think more in terms of in memory consumption and lookup time of the cache compared to a flat cache. If you have two blocks which have a gap of one register between them and that register gets inserted into the cache, ideally those two blocks would be merged, which doesn't seem to be the case currently. So instead of one rbnode with a block covering the whole register space you'll end up with a lot of smaller rbnodes.
If we also provide a more compact way of representing the defaults that devices with sparse registers can use then that problem will go away too.
What do you have in mind for this? An array of pairs of register and value?
Yes, as I said in one of the earlier messages in this thread. It seems like a good combination of being writable/legible and compact.
Hm, ok I'll give it a try. Though I'm not sure yet how to efficiently implement the default register lookup when syncing the cache.
- Lars
On Thu, Aug 11, 2011 at 05:09:14AM +0200, Lars-Peter Clausen wrote:
On 08/11/2011 04:46 AM, Mark Brown wrote:
None of the current ASoC code will coalesce register writes at all, and in the case where you're doing writes to registers that aren't actually adjacent it's going to be marginal if it's better to transmit the intervening register or transmit another register address. That only really makes a difference during cache sync anyway.
I was think more in terms of in memory consumption and lookup time of the cache compared to a flat cache. If you have two blocks which have a gap of one register between them and that register gets inserted into the cache, ideally those two blocks would be merged, which doesn't seem to be the case currently. So instead of one rbnode with a block covering the whole register space you'll end up with a lot of smaller rbnodes.
My guess is that it's probably not worth worrying about, especially for performance where you mostly just need to be better than physical I/O. For small register maps the memory overhead is similarly probably not worth worrying about, and obviously there's also LZO.
Yes, as I said in one of the earlier messages in this thread. It seems like a good combination of being writable/legible and compact.
Hm, ok I'll give it a try. Though I'm not sure yet how to efficiently implement the default register lookup when syncing the cache.
The caches can just unpack into their data, we need to take a copy anyway to allow the caches to be marked as __initdata and then the data will end up stored in a format that matches the method we're using to store the data.
Dimitris had done an initial version of the move of the cache over, though I didn't review it properly yet and he's on holiday now. I might repost it, there were a few issues but it's at least 90% of the way there IIRC from the time I had to look at it.
On 08/11/2011 07:32 AM, Mark Brown wrote:
On Thu, Aug 11, 2011 at 05:09:14AM +0200, Lars-Peter Clausen wrote:
On 08/11/2011 04:46 AM, Mark Brown wrote:
None of the current ASoC code will coalesce register writes at all, and in the case where you're doing writes to registers that aren't actually adjacent it's going to be marginal if it's better to transmit the intervening register or transmit another register address. That only really makes a difference during cache sync anyway.
I was think more in terms of in memory consumption and lookup time of the cache compared to a flat cache. If you have two blocks which have a gap of one register between them and that register gets inserted into the cache, ideally those two blocks would be merged, which doesn't seem to be the case currently. So instead of one rbnode with a block covering the whole register space you'll end up with a lot of smaller rbnodes.
My guess is that it's probably not worth worrying about, especially for performance where you mostly just need to be better than physical I/O. For small register maps the memory overhead is similarly probably not worth worrying about, and obviously there's also LZO.
I think I'll just test it with my codec and see how it turns out. If it there are dozens of small blocks, instead of a few larger blocks, I'll see if there is a way to easily improve the situation.
Yes, as I said in one of the earlier messages in this thread. It seems like a good combination of being writable/legible and compact.
Hm, ok I'll give it a try. Though I'm not sure yet how to efficiently implement the default register lookup when syncing the cache.
The caches can just unpack into their data, we need to take a copy anyway to allow the caches to be marked as __initdata and then the data will end up stored in a format that matches the method we're using to store the data.
Dimitris had done an initial version of the move of the cache over, though I didn't review it properly yet and he's on holiday now. I might repost it, there were a few issues but it's at least 90% of the way there IIRC from the time I had to look at it.
ok, that would be great, thanks.
- Lars
On Thu, Aug 11, 2011 at 02:32:53PM +0900, Mark Brown wrote:
On Thu, Aug 11, 2011 at 05:09:14AM +0200, Lars-Peter Clausen wrote:
On 08/11/2011 04:46 AM, Mark Brown wrote:
None of the current ASoC code will coalesce register writes at all, and in the case where you're doing writes to registers that aren't actually adjacent it's going to be marginal if it's better to transmit the intervening register or transmit another register address. That only really makes a difference during cache sync anyway.
I was think more in terms of in memory consumption and lookup time of the cache compared to a flat cache. If you have two blocks which have a gap of one register between them and that register gets inserted into the cache, ideally those two blocks would be merged, which doesn't seem to be the case currently. So instead of one rbnode with a block covering the whole register space you'll end up with a lot of smaller rbnodes.
Yes, that's true. I've got that in my TODO somewhere. It was not important enough during initial implementation.
Dimitris had done an initial version of the move of the cache over, though I didn't review it properly yet and he's on holiday now. I might repost it, there were a few issues but it's at least 90% of the way there IIRC from the time I had to look at it.
I'll be looking into this soon, there are a few issues to be resolved regarding the LZO code at the moment.
Thanks, Dimitris
participants (5)
-
Dimitris Papastamos -
Lars-Peter Clausen -
Mark Brown -
Scott Jiang -
Takashi Iwai