[alsa-devel] [PATCH] asoc tlv320aic33: skip usage of PLL in some cases
This patch makes the tlv320aic33 driver skip the initialisation of the PLL in case the sysclk is 256 * samplerate. Had to do some minor refactoring too - the check whether a sysclk set by set_sysclk() is valid is now done in set_hw_params().
Signed-off-by: Daniel Mack daniel@caiaq.de
On Thu, Apr 17, 2008 at 09:12:45PM +0200, Daniel Mack wrote:
This patch makes the tlv320aic33 driver skip the initialisation of the PLL in case the sysclk is 256 * samplerate. Had to do some minor refactoring too - the check whether a sysclk set by set_sysclk() is valid is now done in set_hw_params().
Sorry, there was a small typo in the patch I just submitted. Use this one instead.
Daniel
2008/4/17 Daniel Mack daniel@caiaq.org:
On Thu, Apr 17, 2008 at 09:12:45PM +0200, Daniel Mack wrote:
This patch makes the tlv320aic33 driver skip the initialisation of the PLL in case the sysclk is 256 * samplerate. Had to do some minor refactoring too - the check whether a sysclk set by set_sysclk() is valid is now done in set_hw_params().
Sorry, there was a small typo in the patch I just submitted. Use this one instead.
I had a quick look to your patch and AIC33 spec.
Is this the same than 256-clock transfer mode? Should you set the bit 3 in AIC3X_ASD_INTF_CTRLB in this case? Should you also still write the AIC3X_SAMPLE_RATE_SEL_REG?
Jarkko
Hi Jarkko,
On Fri, Apr 18, 2008 at 10:59:08AM +0300, Jarkko Nikula wrote:
I had a quick look to your patch and AIC33 spec.
Is this the same than 256-clock transfer mode?
No, the 256-clock mode is for output only, while in my setup the TLV is in slave mode. I attached this chip to the I2S output of an PXA270 which always outputs sample rate * 256 as system clock. In this very case, the PLL can be bypassed by selecting the left path described on page 27.
Should you set the bit 3 in AIC3X_ASD_INTF_CTRLB in this case? Should you also still write the AIC3X_SAMPLE_RATE_SEL_REG?
AIC3X_SAMPLE_RATE_SEL_REG defaults to 0 which is what I want in this case. Thus, I don't have to write it.
Daniel
On Fri, Apr 18, 2008 at 11:13 AM, Daniel Mack daniel@caiaq.org wrote:
Hi Jarkko,
No, the 256-clock mode is for output only, while in my setup the TLV is in slave mode. I attached this chip to the I2S output of an PXA270 which always outputs sample rate * 256 as system clock. In this very case, the PLL can be bypassed by selecting the left path described on page 27.
Ok, now I see. Probably you should refer it as, at least in comment, 128*Q instead of 256 eventhough the driver is not currently touching the Q value in AIC3X_PLL_PROGA_REG.
AIC3X_SAMPLE_RATE_SEL_REG defaults to 0 which is what I want in this case. Thus, I don't have to write it.
Are you sure this is a general case?
Jarkko
On Fri, Apr 18, 2008 at 11:58:49AM +0300, Jarkko Nikula wrote:
No, the 256-clock mode is for output only, while in my setup the TLV is in slave mode. I attached this chip to the I2S output of an PXA270 which always outputs sample rate * 256 as system clock. In this very case, the PLL can be bypassed by selecting the left path described on page 27.
Ok, now I see. Probably you should refer it as, at least in comment, 128*Q instead of 256 eventhough the driver is not currently touching the Q value in AIC3X_PLL_PROGA_REG.
Well, the constraint for that condition is that MCLK = 256*WCLK, and the reason why that works for the chip without PLL is that Q=2. I stated that a little better in the attached patch.
AIC3X_SAMPLE_RATE_SEL_REG defaults to 0 which is what I want in this case. Thus, I don't have to write it.
Are you sure this is a general case?
Well, the reset default is 0 (as stated on page 44) and set_hw_params() is the only location where this value is written. So if it's never written with a different value, it should always be set to its default, no?
Daniel
On Fri, Apr 18, 2008 at 11:38:21AM +0200, Daniel Mack wrote:
On Fri, Apr 18, 2008 at 11:58:49AM +0300, Jarkko Nikula wrote:
AIC3X_SAMPLE_RATE_SEL_REG defaults to 0 which is what I want in this case. Thus, I don't have to write it.
Are you sure this is a general case?
Well, the reset default is 0 (as stated on page 44) and set_hw_params() is the only location where this value is written. So if it's never written with a different value, it should always be set to its default, no?
Not all systems use a static SYSCLK rate - sometimes systems will select the rate based on their current usage. If the system has previously had to configure it then the register won't be at the default value any more.
On Fri, Apr 18, 2008 at 11:08:59AM +0100, Mark Brown wrote:
Well, the reset default is 0 (as stated on page 44) and set_hw_params() is the only location where this value is written. So if it's never written with a different value, it should always be set to its default, no?
Not all systems use a static SYSCLK rate - sometimes systems will select the rate based on their current usage. If the system has previously had to configure it then the register won't be at the default value any more.
Ok, I didn't consider that. New version of the patch attached which resets AIC3X_PLL_PROGA_REG and AIC3X_SAMPLE_RATE_SEL_REG to appropriate values in case the PLL setup is skipped.
Daniel
On Fri, Apr 18, 2008 at 12:38 PM, Daniel Mack daniel@caiaq.org wrote:
On Fri, Apr 18, 2008 at 11:58:49AM +0300, Jarkko Nikula wrote:
Ok, now I see. Probably you should refer it as, at least in comment,
128*Q
instead of 256 eventhough the driver is not currently touching the Q
value
in AIC3X_PLL_PROGA_REG.
Well, the constraint for that condition is that MCLK = 256*WCLK, and the reason why that works for the chip without PLL is that Q=2. I stated that a little better in the attached patch.
I would rather refer there just "Fsref = CLKDIV_IN / (128*Q)" as the condition where PLL can be disabled (or is mandatory where it must be disabled?).
Referring to data sheet page 27 is not good as it's correct only for certain version of AIC33 spec and driver supports other AIC3x chips as well :-)
Are you sure this is a general case?
Well, the reset default is 0 (as stated on page 44) and set_hw_params() is the only location where this value is written. So if it's never written with a different value, it should always be set to its default, no?
I mean even if the I2S interface Fs meets the MCLK/256 condition, the audio sample rate may be lower and it may be still required to set ADC and DAC rates.
Jarkko
On Fri, Apr 18, 2008 at 01:47:33PM +0300, Jarkko Nikula wrote:
Well, the constraint for that condition is that MCLK = 256*WCLK, and the reason why that works for the chip without PLL is that Q=2. I stated that a little better in the attached patch.
I would rather refer there just "Fsref = CLKDIV_IN / (128*Q)" as the condition where PLL can be disabled (or is mandatory where it must be disabled?).
Referring to data sheet page 27 is not good as it's correct only for certain version of AIC33 spec and driver supports other AIC3x chips as well :-)
Ok, I agree. I changed that to find an appropriate value for Q programmatically. Have a look at the attached patch, please. I hope i finally got it now ;)
Daniel
Subject: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases
Try to find an appropriate value for Q during clock setup in order to bypass the usage of the PLL in some cases. This saves some entries in the dividers table.
Signed-off-by: Daniel Mack daniel@caiaq.de
On Fri, Apr 18, 2008 at 2:41 PM, Daniel Mack daniel@caiaq.org wrote:
Ok, I agree. I changed that to find an appropriate value for Q programmatically. Have a look at the attached patch, please. I hope i finally got it now ;)
That sounds a good idea. Then more cases will be covered.
What I noticed that instead of params_rate, I think we should compare here the FSref of 44.1 and 48 kHz (how about dual-rate mode?) when defining can the PLL be bypassed
if (params_rate(params) == aic3x->sysclk / (128 * pll_q))
---
Probably you forgot to move bypass case in this version after writing the AIC3X_SAMPLE_RATE_SEL_REG?
Spec is also saying that when NDAC is 1.5, 2.5, ... 5.5, then odd values of Q are not allowed.
Not the easiest chip... probably worth to ask from TI what's the case e.g. when FSref is 2*48 kHz and ADC/DAC rate is 64 kHz (NDAC is 1.5).
And of course, over-designing the driver is not the purpose and probably some special cases can be now covered just with -EINVAL and let the user who needs them to send a patch :-)
Jarkko
On Fri, Apr 18, 2008 at 04:47:16PM +0300, Jarkko Nikula wrote:
Ok, I agree. I changed that to find an appropriate value for Q programmatically. Have a look at the attached patch, please. I hope i finally got it now ;)
That sounds a good idea. Then more cases will be covered.
What I noticed that instead of params_rate, I think we should compare here the FSref of 44.1 and 48 kHz (how about dual-rate mode?) when defining can the PLL be bypassed
if (params_rate(params) == aic3x->sysclk / (128 * pll_q))
Hmm? Why do you think so? I'm afraid I don't get your point here.
Probably you forgot to move bypass case in this version after writing the AIC3X_SAMPLE_RATE_SEL_REG?
No, actually not.
Spec is also saying that when NDAC is 1.5, 2.5, ... 5.5, then odd values of Q are not allowed.
NDAC and NADC are both hard-coded to 0 (divider of 1) at the moment. Support for more flexible settings could be done in another step. Also, the PLL setup could be done by calculation of values rather that by a lookup table.
I'd like to see this patch applied now as base for further refinements. Do you agree?
Daniel
On Fri, Apr 18, 2008 at 5:03 PM, Daniel Mack daniel@caiaq.org wrote:
On Fri, Apr 18, 2008 at 04:47:16PM +0300, Jarkko Nikula wrote:
if (params_rate(params) == aic3x->sysclk / (128 * pll_q))
Hmm? Why do you think so? I'm afraid I don't get your point here.
I'm might be completely wrong here since I've used this codec only in configuration where the interface rate or FSref equals to audio sampling rate , but as far as I've interpreted the spec correctly we can have e.g. the case where MCLK is 22.5792 MHz, FSref 44.1 kHz (i.e. PLL can be disabled) but audio sampling rate is 11.025 kHz (== params_rate(params)).
And I think here equation "Fsref = CLKDIV_IN / (128 × Q)" Fsref refers to 44.1 or 48 kHz not the audio sampling rate?
But I don't want to speculate or do my own interpretation here. I'll ask this from TI next week from my jarkko.nikula@nokia.com and can put you as a cc as well.
Or do the driver author have any better knowledge here?
Probably you forgot to move bypass case in this version after writing
the
AIC3X_SAMPLE_RATE_SEL_REG?
No, actually not.
Your latest patch has:
+ if (bypass_pll) + return 0; + /* codec sample rate select */ data = aic3x_divs[i].sr_reg; data |= (data << 4);
Spec is also saying that when NDAC is 1.5, 2.5, ... 5.5, then odd values
of
Q are not allowed.
NDAC and NADC are both hard-coded to 0 (divider of 1) at the moment. Support for more flexible settings could be done in another step. Also, the PLL setup could be done by calculation of values rather that by a lookup table.
Again, I'm might be wrong but I NDAC and NADC refers to register AIC3X_SAMPLE_RATE_SEL_REG which driver already takes into account properly.
I'd like to see this patch applied now as base for further refinements. Do you agree?
I cannot prevent it to be applied, that's the driver author or maintainers task to justify. I can only throw some discussion here if I see something to be unclear (to me).
And don't take my comments as a nitpicking. Definitely patch like this is worth to get in but we have to make sure that no possible bugs are introduced. -EINVAL is much better alternative for some new configurations.
Jarkko
On Fri, Apr 18, 2008 at 10:37:36PM +0300, Jarkko Nikula wrote:
I'm might be completely wrong here since I've used this codec only in configuration where the interface rate or FSref equals to audio sampling rate , but as far as I've interpreted the spec correctly we can have e.g. the case where MCLK is 22.5792 MHz, FSref 44.1 kHz (i.e. PLL can be disabled) but audio sampling rate is 11.025 kHz (== params_rate(params)).
Of course. Fsref is a setting for the filter to tell it which internal base-frequency it is operating on. But the setup of the sampling frequency has different meaning. Also, the clocking diagram (page 27 in the aic33 manual) doesn't even show that entity, or did I miss anything after staring at this for some days? ;)
But I don't want to speculate or do my own interpretation here. I'll ask this from TI next week from my jarkko.nikula@nokia.com and can put you as a cc as well.
Ok, thanks :)
Your latest patch has:
- if (bypass_pll)
return 0;
Yes, since all other setup can be skipped in case the PLL is bypassed. Did you miss the question here? ;)
Again, I'm might be wrong but I NDAC and NADC refers to register AIC3X_SAMPLE_RATE_SEL_REG which driver already takes into account properly.
Only in case the PLL is enabled. I agree that there could be some more setup around all the possible settings in this clock generation, but I'd my approach as a good first start.
And don't take my comments as a nitpicking. Definitely patch like this is worth to get in but we have to make sure that no possible bugs are introduced. -EINVAL is much better alternative for some new configurations.
No, I don't take it as nitpicking :) I agree that a driver for such a complex chip has to be modified with care to have any regression. But I don't see such as far, so I'd take this patch as a step towards a better solution. Unless someone comes up claiming that it's breaking anything.
Thanks and best rgeards, Daniel
participants (3)
-
Daniel Mack -
Jarkko Nikula -
Mark Brown