[alsa-devel] Remove clkdiv and pll setters from pxa dais
Hi,
As discussed in the simple-card thread the other day, the .set_clkdiv and .set_pll callbacks for DAIs are considered legacy. In order to support PXA platforms with DT, we need get rid of those for the DAIs of PXA hardware.
On the good side, pxa-ssp is the only problematic one of the PXA dais, the ac97, i2s don't have any special clocking knbos, and the mmp-sspa should be straight forward to fix.
I looked into pxa-ssp and it seems the conversion to calculate the audio clock dividers automatically seems doable.
There are currently 4 platforms that use this cpu dai in mainline that also use snd_soc_dai_set_pll() or snd_soc_dai_set_clkdiv():
* Zylonite * Brownstone * Magician * Raumfeld
I can test modifications on the latter one, but for the others, I need to understand what the machine drivers are doing, and there are some bits I don't grok.
For instance, code for Zylonite does this:
/* Add 1 to the width for the leading clock cycle */ pll_out = rate * (width + 1) * 8;
The commit which introduced these lines dates back to 2009 and was done by you, Mark. Can you remember what the reason for this was? I've never seen sample frames with 17 bits :) This is a setup that we can't generically describe through .hw_params() or .dai_fmt() in the cpu dai, correct?
In the magician code, you'll find this:
case 22050: acps = 5622000; switch (width) { case 16: /* 702750 Hz ~= 22050 Hz * 32 (-0.41%) */ acds = PXA_SSP_CLK_AUDIO_DIV_2; break; default: /* 32 */ /* 1405500 Hz ~= 22050 Hz * 64 (-0.41%) */ acds = PXA_SSP_CLK_AUDIO_DIV_1; } break; case 44100: acps = 5622000; switch (width) { case 16: /* 1405500 Hz ~= 44100 Hz * 32 (-0.41%) */ acds = PXA_SSP_CLK_AUDIO_DIV_2; break; default: /* 32 */ /* 2811000 Hz ~= 44100 Hz * 64 (-0.41%) */ acds = PXA_SSP_CLK_AUDIO_DIV_1; } break;
So for both 22050 and 44100, the base frequency and all dividers are the same, which can't be right. I assume these rates have never been used. I'll ignore this and implement the table in the datasheet which should do the right thing. Philipp?
What we need, however, is a way to describe whether the dai is mclk master or slave. Would we add a DT propery for that?
Thanks, Daniel
On Wed, May 30, 2018 at 10:35:25AM +0200, Daniel Mack wrote:
For instance, code for Zylonite does this:
/* Add 1 to the width for the leading clock cycle */ pll_out = rate * (width + 1) * 8;
The commit which introduced these lines dates back to 2009 and was done by you, Mark. Can you remember what the reason for this was? I've never seen sample frames with 17 bits :) This is a setup that we can't generically describe through .hw_params() or .dai_fmt() in the cpu dai, correct?
I think it's copied from somewhere else probably, it looks like there's a bit of code motion going on. I bet it was just for I2S, keeping the extra clock cycle for the initial rising edge in the frame but not actually required.
So for both 22050 and 44100, the base frequency and all dividers are the same, which can't be right. I assume these rates have never been used. I'll ignore this and implement the table in the datasheet which should do the right thing. Philipp?
That looks buggy, yeah. I doubt anyone ever used 22.05kHz.
What we need, however, is a way to describe whether the dai is mclk master or slave. Would we add a DT propery for that?
That might be sensible, though the MCLK isn't really part of the DAI.
On Wednesday, May 30, 2018 11:24 AM, Mark Brown wrote:
On Wed, May 30, 2018 at 10:35:25AM +0200, Daniel Mack wrote:
For instance, code for Zylonite does this:
/* Add 1 to the width for the leading clock cycle */ pll_out = rate * (width + 1) * 8;The commit which introduced these lines dates back to 2009 and was done by you, Mark. Can you remember what the reason for this was? I've never seen sample frames with 17 bits :) This is a setup that we can't generically describe through .hw_params() or .dai_fmt() in the cpu dai, correct?
I think it's copied from somewhere else probably, it looks like there's a bit of code motion going on. I bet it was just for I2S, keeping the extra clock cycle for the initial rising edge in the frame but not actually required.
Hmm, okay. You happen to still have access to that board for regression tests?
So for both 22050 and 44100, the base frequency and all dividers are the same, which can't be right. I assume these rates have never been used. I'll ignore this and implement the table in the datasheet which should do the right thing. Philipp?
That looks buggy, yeah. I doubt anyone ever used 22.05kHz.
AFAIU, it's rather the 44.1 rate that looks wrong. But okay, we'll see.
What we need, however, is a way to describe whether the dai is mclk master or slave. Would we add a DT propery for that?
That might be sensible, though the MCLK isn't really part of the DAI.
Well, the DAI may well be the producer of the MCLK. If there's only a master/slave mode to decide, we could set that property on the machine side as well, in the subnode of simple-card for instance (similar to 'bitclock-master' and 'frame-master'), but then the question is which callback to use for propagating that master/slave setting to the cpu dai. We will, however, need such a callback anyway for non-DT boards, as those won't go away anytime soon. Could we squeeze that into the SND_SOC_DAIFMT_ mask? Not sure which options would be acceptable.
Thanks, Daniel
On Wed, May 30, 2018 at 11:47:15AM +0200, Daniel Mack wrote:
On Wednesday, May 30, 2018 11:24 AM, Mark Brown wrote:
I think it's copied from somewhere else probably, it looks like there's a bit of code motion going on. I bet it was just for I2S, keeping the extra clock cycle for the initial rising edge in the frame but not actually required.
Hmm, okay. You happen to still have access to that board for regression tests?
No, I doubt anyone does - it was the PXA32x reference platform. The ones I'm aware of unfortunately got thrown out.
So for both 22050 and 44100, the base frequency and all dividers are the same, which can't be right. I assume these rates have never been used. I'll ignore this and implement the table in the datasheet which should do the right thing. Philipp?
That looks buggy, yeah. I doubt anyone ever used 22.05kHz.
AFAIU, it's rather the 44.1 rate that looks wrong. But okay, we'll see.
That's definitely been tested... perhaps the parent rate is set incorrectly?
What we need, however, is a way to describe whether the dai is mclk master or slave. Would we add a DT propery for that?
That might be sensible, though the MCLK isn't really part of the DAI.
Well, the DAI may well be the producer of the MCLK. If there's only a master/slave mode to decide, we could set that property on the machine side
It might be the same physical block, but the MCLK could also come from somewhere completely different and not even need to be in sync with the rest of the clocks.
as well, in the subnode of simple-card for instance (similar to 'bitclock-master' and 'frame-master'), but then the question is which callback to use for propagating that master/slave setting to the cpu dai. We will, however, need such a callback anyway for non-DT boards, as those won't go away anytime soon. Could we squeeze that into the SND_SOC_DAIFMT_ mask? Not sure which options would be acceptable.
DAIFMT feels like a push, like I say we'll need to handle clocks that aren't aligned with the DAI as well. Possibly either the existing set_sysclk() operation or some new operation.
Mark Brown broonie@kernel.org writes:
On Wed, May 30, 2018 at 11:47:15AM +0200, Daniel Mack wrote:
On Wednesday, May 30, 2018 11:24 AM, Mark Brown wrote:
I think it's copied from somewhere else probably, it looks like there's a bit of code motion going on. I bet it was just for I2S, keeping the extra clock cycle for the initial rising edge in the frame but not actually required.
Hmm, okay. You happen to still have access to that board for regression tests?
No, I doubt anyone does - it was the PXA32x reference platform. The ones I'm aware of unfortunately got thrown out.
I have a zylonite based on a pxa310 in my non-regression bench, if that can help. That's one of the targets I use to validate patches against the PXA tree.
I'll let you come up with a patch. Meanwhile I'll try to figure out if I can make the audio alive again.
Cheers.
On Wed, May 30, 2018 at 09:15:37PM +0200, Robert Jarzmik wrote:
Mark Brown broonie@kernel.org writes:
No, I doubt anyone does - it was the PXA32x reference platform. The ones I'm aware of unfortunately got thrown out.
I have a zylonite based on a pxa310 in my non-regression bench, if that can help. That's one of the targets I use to validate patches against the PXA tree.
Ah, that's excellent!
I'll let you come up with a patch. Meanwhile I'll try to figure out if I can make the audio alive again.
Probably worth taking a look at the big AC'97 refactoring that happened a while ago I guess...
Mark Brown broonie@kernel.org writes:
I'll let you come up with a patch. Meanwhile I'll try to figure out if I can make the audio alive again.
Probably worth taking a look at the big AC'97 refactoring that happened a while ago I guess...
Ah that was already the case, I had tested on the zylonite platform, up to the point where the asound zylonite card was correctly probed.
That reminds me that the last patch of the AC97 serie was not merged (the PXA specific one), so I'll repost it : - original https://patchwork.kernel.org/patch/9951919/ - repost will add a NULL pointer check + rebase, in its v8 version
Cheers.
participants (3)
-
Daniel Mack -
Mark Brown -
Robert Jarzmik