Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
Hi > > > On Thu, Apr 18, 2019 at 09:37:06AM +, S.j. Wang wrote: > > > > > And this is according to IMX6DQRM: > > > > > Limited support for the case when output sampling rates is > > > > > between 8kHz and 30kHz. The limitation is the supported ratio > > > > > (Fsin/Fsout) range as between 1/24 to 8 > > > > > > > > > > This should cover your 8.125 condition already, even if having > > > > > an outrate range between [8KHz, 30KHz] check, since an outrate > > > > > above 30KHz will not have an inrate bigger than 8.125 times of > > > > > it, given the maximum input rate is 192KHz. > > > > > > > > > > So I think that we can just drop that 8.125 condition from your > > > > > change and there's no need to error out any more. > > > > > > > > > No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported. > > > > This is not covered by > > > > > > > > if ((outrate > 8000 && outrate < 3) && > > > > (outrate/inrate > 24 || inrate/outrate > 8)) { > > > > > > Good catch. The range should be [8KHz, 30KHz] vs. (8KHz, 32KHz) in > > > the code. Then I think the fix should be at both lines: > > > > > > - if ((outrate > 8000 && outrate < 3) && > > > - (outrate/inrate > 24 || inrate/outrate > 8)) { > > > + if ((outrate >= 8000 && outrate =< 3) && > > > + (outrate > 24 * inrate || inrate > 8 * outrate)) { > > > > > > Overall, I think we should fix this instead of adding an extra one, > > > since it is very likely saying the same thing. > > > > Actually if outrate < 8kHz, there will be issue too. > > Here is the thing, the RM doesn't explicitly state that ASRC can support a > lower output sample rate than 8KHz. And I actually had a concern when > reviewing your PATCH-2, as the table of supported output sample rate no > longer matches RM. > > If you've verified a lower output sample rate working solid with the > process_option function, that means our driver can go beyond the > limitation mentioned in the RM, then I believe [8KHz, 32KHz] should be > updated too -- that says we can do: > - if ((outrate > 8000 && outrate < 3) && > - (outrate/inrate > 24 || inrate/outrate > 8)) { > + if ((outrate >= 5512 && outrate =< 3) && > + (outrate > 24 * inrate || inrate > 8 * outrate)) { > > Actually "ourate > 24 * inrate" is kind of pointless for range [5KHz, 32KHz] > but we can keep it since it matches RM. Ok, will send v4. Best regards Wang shengjiu
Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
On Thu, Apr 18, 2019 at 09:37:06AM +, S.j. Wang wrote: > > > > And this is according to IMX6DQRM: > > > > Limited support for the case when output sampling rates is > > > > between 8kHz and 30kHz. The limitation is the supported ratio > > > > (Fsin/Fsout) range as between 1/24 to 8 > > > > > > > > This should cover your 8.125 condition already, even if having an > > > > outrate range between [8KHz, 30KHz] check, since an outrate above > > > > 30KHz will not have an inrate bigger than 8.125 times of it, given > > > > the maximum input rate is 192KHz. > > > > > > > > So I think that we can just drop that 8.125 condition from your > > > > change and there's no need to error out any more. > > > > > > > No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported. > > > This is not covered by > > > > > > if ((outrate > 8000 && outrate < 3) && > > > (outrate/inrate > 24 || inrate/outrate > 8)) { > > > > Good catch. The range should be [8KHz, 30KHz] vs. (8KHz, 32KHz) in the > > code. Then I think the fix should be at both lines: > > > > - if ((outrate > 8000 && outrate < 3) && > > - (outrate/inrate > 24 || inrate/outrate > 8)) { > > + if ((outrate >= 8000 && outrate =< 3) && > > + (outrate > 24 * inrate || inrate > 8 * outrate)) { > > > > Overall, I think we should fix this instead of adding an extra one, since > > it is > > very likely saying the same thing. > > Actually if outrate < 8kHz, there will be issue too. Here is the thing, the RM doesn't explicitly state that ASRC can support a lower output sample rate than 8KHz. And I actually had a concern when reviewing your PATCH-2, as the table of supported output sample rate no longer matches RM. If you've verified a lower output sample rate working solid with the process_option function, that means our driver can go beyond the limitation mentioned in the RM, then I believe [8KHz, 32KHz] should be updated too -- that says we can do: - if ((outrate > 8000 && outrate < 3) && - (outrate/inrate > 24 || inrate/outrate > 8)) { + if ((outrate >= 5512 && outrate =< 3) && + (outrate > 24 * inrate || inrate > 8 * outrate)) { Actually "ourate > 24 * inrate" is kind of pointless for range [5KHz, 32KHz] but we can keep it since it matches RM.
Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
Hi > > On Thu, Apr 18, 2019 at 08:50:48AM +, S.j. Wang wrote: > > > And this is according to IMX6DQRM: > > > Limited support for the case when output sampling rates is > > > between 8kHz and 30kHz. The limitation is the supported ratio > > > (Fsin/Fsout) range as between 1/24 to 8 > > > > > > This should cover your 8.125 condition already, even if having an > > > outrate range between [8KHz, 30KHz] check, since an outrate above > > > 30KHz will not have an inrate bigger than 8.125 times of it, given > > > the maximum input rate is 192KHz. > > > > > > So I think that we can just drop that 8.125 condition from your > > > change and there's no need to error out any more. > > > > > No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported. > > This is not covered by > > > > if ((outrate > 8000 && outrate < 3) && > > (outrate/inrate > 24 || inrate/outrate > 8)) { > > Good catch. The range should be [8KHz, 30KHz] vs. (8KHz, 32KHz) in the > code. Then I think the fix should be at both lines: > > - if ((outrate > 8000 && outrate < 3) && > - (outrate/inrate > 24 || inrate/outrate > 8)) { > + if ((outrate >= 8000 && outrate =< 3) && > + (outrate > 24 * inrate || inrate > 8 * outrate)) { > > Overall, I think we should fix this instead of adding an extra one, since it > is > very likely saying the same thing. Actually if outrate < 8kHz, there will be issue too. Best regards Wang shengjiu
Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
On Thu, Apr 18, 2019 at 08:50:48AM +, S.j. Wang wrote: > > And this is according to IMX6DQRM: > > Limited support for the case when output sampling rates is > > between 8kHz and 30kHz. The limitation is the supported ratio > > (Fsin/Fsout) range as between 1/24 to 8 > > > > This should cover your 8.125 condition already, even if having an outrate > > range between [8KHz, 30KHz] check, since an outrate above 30KHz will not > > have an inrate bigger than 8.125 times of it, given the maximum input rate > > is 192KHz. > > > > So I think that we can just drop that 8.125 condition from your change and > > there's no need to error out any more. > > > No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported. > This is not covered by > > if ((outrate > 8000 && outrate < 3) && > (outrate/inrate > 24 || inrate/outrate > 8)) { Good catch. The range should be [8KHz, 30KHz] vs. (8KHz, 32KHz) in the code. Then I think the fix should be at both lines: - if ((outrate > 8000 && outrate < 3) && - (outrate/inrate > 24 || inrate/outrate > 8)) { + if ((outrate >= 8000 && outrate =< 3) && + (outrate > 24 * inrate || inrate > 8 * outrate)) { Overall, I think we should fix this instead of adding an extra one, since it is very likely saying the same thing.
Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
Hi > > > On Thu, Apr 18, 2019 at 02:37:03AM +, S.j. Wang wrote: > > > Here: > > > > + /* Does not support cases: Tsout > 8.125 * Tsin */ > > > > + if (inrate * 8 > 65 * outrate) > > Though it might not matter any more (see my last comments), it should be > "inrate > 8.125 * outrate" in the comments. > > > > > + return -EINVAL; > > > And here: > > > > + ret = fsl_asrc_sel_proc(inrate, outrate, _proc, _proc); > > > > + if (ret) { > > > > + pair_err("No supported pre-processing options\n"); > > > > + return ret; > > > > + } > > > > > > Instead of a general message, I was thinking of a more specific one > > > by telling users that the ratio between the two rates isn't > > > supported -- something similar to what I suggested previously: > > > > > > pair_err("Does not support %d (input) > 8.125 * %d (output)\n", > > > outrate, inrate); > > > > > > In fsl_asrc_sel_proc, we can't call the pair_err for there is no > > struct fsl_asrc_pair *pair in the argument. Do you think we need to > > add this argument? > > I's thinking of adding it to the top of fsl_asrc_config_pair() as a part of > inrate-outrate-validation, however, I found that actually we already have a > similar check in the early routine: > if ((outrate > 8000 && outrate < 3) && > (outrate/inrate > 24 || inrate/outrate > 8)) { > pair_err("exceed supported ratio range [1/24, 8] for \ > inrate/outrate: %d/%d\n", inrate, outrate); > return -EINVAL; > } > > And this is according to IMX6DQRM: > Limited support for the case when output sampling rates is > between 8kHz and 30kHz. The limitation is the supported ratio > (Fsin/Fsout) range as between 1/24 to 8 > > This should cover your 8.125 condition already, even if having an outrate > range between [8KHz, 30KHz] check, since an outrate above 30KHz will not > have an inrate bigger than 8.125 times of it, given the maximum input rate > is 192KHz. > > So I think that we can just drop that 8.125 condition from your change and > there's no need to error out any more. > No, if outrate=8kHz, inrate > 88.2kHz, these cases are not supported. This is not covered by if ((outrate > 8000 && outrate < 3) && (outrate/inrate > 24 || inrate/outrate > 8)) { > However, we do need a patch to fix a potential rounding issue: > - (outrate/inrate > 24 || inrate/outrate > 8)) { > + (outrate > 24 * inrate || inrate > 8 * outrate)) { > > Should fix the missing whitespace also. And it will be needed to send to > stable kernel too. Will you help submit a change? > > Thanks
Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
On Thu, Apr 18, 2019 at 02:37:03AM +, S.j. Wang wrote: > > Here: > > > + /* Does not support cases: Tsout > 8.125 * Tsin */ > > > + if (inrate * 8 > 65 * outrate) Though it might not matter any more (see my last comments), it should be "inrate > 8.125 * outrate" in the comments. > > > + return -EINVAL; > > And here: > > > + ret = fsl_asrc_sel_proc(inrate, outrate, _proc, _proc); > > > + if (ret) { > > > + pair_err("No supported pre-processing options\n"); > > > + return ret; > > > + } > > > > Instead of a general message, I was thinking of a more specific one by > > telling users that the ratio between the two rates isn't supported -- > > something similar to what I suggested previously: > > > > pair_err("Does not support %d (input) > 8.125 * %d (output)\n", > > outrate, inrate); > > > In fsl_asrc_sel_proc, we can't call the pair_err for there is no > struct fsl_asrc_pair *pair in the argument. Do you think we need to > add this argument? I's thinking of adding it to the top of fsl_asrc_config_pair() as a part of inrate-outrate-validation, however, I found that actually we already have a similar check in the early routine: if ((outrate > 8000 && outrate < 3) && (outrate/inrate > 24 || inrate/outrate > 8)) { pair_err("exceed supported ratio range [1/24, 8] for \ inrate/outrate: %d/%d\n", inrate, outrate); return -EINVAL; } And this is according to IMX6DQRM: Limited support for the case when output sampling rates is between 8kHz and 30kHz. The limitation is the supported ratio (Fsin/Fsout) range as between 1/24 to 8 This should cover your 8.125 condition already, even if having an outrate range between [8KHz, 30KHz] check, since an outrate above 30KHz will not have an inrate bigger than 8.125 times of it, given the maximum input rate is 192KHz. So I think that we can just drop that 8.125 condition from your change and there's no need to error out any more. However, we do need a patch to fix a potential rounding issue: - (outrate/inrate > 24 || inrate/outrate > 8)) { + (outrate > 24 * inrate || inrate > 8 * outrate)) { Should fix the missing whitespace also. And it will be needed to send to stable kernel too. Will you help submit a change? Thanks
Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
Hi > > Hi Shengjiu, > > This looks better. Just a couple of more small comments inline. > > On Wed, Apr 17, 2019 at 09:06:18AM +, S.j. Wang wrote: > > > +static int fsl_asrc_sel_proc(int inrate, int outrate, int *pre_proc, > > + int *post_proc) > > Just a nit: it looks better by grouping them two-two. > > static int fsl_asrc_sel_proc(int inrate, int outrate, > int *pre_proc, int *post_proc) > > > + /* Condition for selection of post-processing */ > > + post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) || > > + (inrate > 56000 && outrate < 56000); > > Could align the indentation: > post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) || > (inrate > 56000 && outrate < 56000); > > Here: > > + /* Does not support cases: Tsout > 8.125 * Tsin */ > > + if (inrate * 8 > 65 * outrate) > > + return -EINVAL; > And here: > > + ret = fsl_asrc_sel_proc(inrate, outrate, _proc, _proc); > > + if (ret) { > > + pair_err("No supported pre-processing options\n"); > > + return ret; > > + } > > Instead of a general message, I was thinking of a more specific one by > telling users that the ratio between the two rates isn't supported -- > something similar to what I suggested previously: > > pair_err("Does not support %d (input) > 8.125 * %d (output)\n", > outrate, inrate); > In fsl_asrc_sel_proc, we can't call the pair_err for there is no struct fsl_asrc_pair *pair in the argument. Do you think we need to add this argument? > Thanks > Nicolin
Re: [PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
Hi Shengjiu, This looks better. Just a couple of more small comments inline. On Wed, Apr 17, 2019 at 09:06:18AM +, S.j. Wang wrote: > +static int fsl_asrc_sel_proc(int inrate, int outrate, int *pre_proc, > + int *post_proc) Just a nit: it looks better by grouping them two-two. static int fsl_asrc_sel_proc(int inrate, int outrate, int *pre_proc, int *post_proc) > + /* Condition for selection of post-processing */ > + post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) || > + (inrate > 56000 && outrate < 56000); Could align the indentation: post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) || (inrate > 56000 && outrate < 56000); Here: > + /* Does not support cases: Tsout > 8.125 * Tsin */ > + if (inrate * 8 > 65 * outrate) > + return -EINVAL; And here: > + ret = fsl_asrc_sel_proc(inrate, outrate, _proc, _proc); > + if (ret) { > + pair_err("No supported pre-processing options\n"); > + return ret; > + } Instead of a general message, I was thinking of a more specific one by telling users that the ratio between the two rates isn't supported -- something similar to what I suggested previously: pair_err("Does not support %d (input) > 8.125 * %d (output)\n", outrate, inrate); Thanks Nicolin
[PATCH V3 1/2] ASoC: fsl_asrc: replace the process_option table with function
When we want to support more sample rate, for example 12kHz/24kHz we need update the process_option table, if we want to support more sample rate next time, the table need to be updated again. which is not flexible. We got a function fsl_asrc_sel_proc to replace the table, which can give the pre-processing and post-processing options according to the sample rate. Signed-off-by: Shengjiu Wang --- sound/soc/fsl/fsl_asrc.c | 78 +++- 1 file changed, 58 insertions(+), 20 deletions(-) diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index 0b937924d2e4..d34d539d01f2 100644 --- a/sound/soc/fsl/fsl_asrc.c +++ b/sound/soc/fsl/fsl_asrc.c @@ -26,24 +26,6 @@ #define pair_dbg(fmt, ...) \ dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index, ##__VA_ARGS__) -/* Sample rates are aligned with that defined in pcm.h file */ -static const u8 process_option[][12][2] = { - /* 8kHz 11.025kHz 16kHz 22.05kHz 32kHz 44.1kHz 48kHz 64kHz 88.2kHz 96kHz 176kHz 192kHz */ - {{0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0},}, /* 5512Hz */ - {{0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0},}, /* 8kHz */ - {{0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0},}, /* 11025Hz */ - {{1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0},}, /* 16kHz */ - {{1, 2}, {1, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0}, {0, 0}, {0, 0},}, /* 22050Hz */ - {{1, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0}, {0, 0},}, /* 32kHz */ - {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0},}, /* 44.1kHz */ - {{2, 2}, {2, 2}, {2, 1}, {2, 1}, {0, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0}, {0, 0},}, /* 48kHz */ - {{2, 2}, {2, 2}, {2, 2}, {2, 1}, {1, 2}, {0, 2}, {0, 2}, {0, 1}, {0, 1}, {0, 1}, {0, 1}, {0, 0},}, /* 64kHz */ - {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1, 1}, {1, 1}, {1, 1}, {1, 1},}, /* 88.2kHz */ - {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {1, 2}, {1, 2}, {1, 2}, {1, 1}, {1, 1}, {1, 1}, {1, 1}, {1, 1},}, /* 96kHz */ - {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2, 1}, {2, 1}, {2, 1}, {2, 1},}, /* 176kHz */ - {{2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 2}, {2, 1}, {2, 1}, {2, 1}, {2, 1}, {2, 1},}, /* 192kHz */ -}; - /* Corresponding to process_option */ static int supported_input_rate[] = { 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, @@ -80,6 +62,54 @@ static unsigned char *clk_map[2]; /** + * Select the pre-processing and post-processing options + * + * inrate: input sample rate + * outrate: output sample rate + * pre_proc: return value for pre-processing option + * post_proc: return value for post-processing option + */ +static int fsl_asrc_sel_proc(int inrate, int outrate, int *pre_proc, +int *post_proc) +{ + bool post_proc_cond2; + bool post_proc_cond0; + + /* Does not support cases: Tsout > 8.125 * Tsin */ + if (inrate * 8 > 65 * outrate) + return -EINVAL; + + /* Otherwise, select pre_proc between [0, 2] */ + if (inrate * 8 > 33 * outrate) + *pre_proc = 2; + else if (inrate * 8 > 15 * outrate) { + if (inrate > 152000) + *pre_proc = 2; + else + *pre_proc = 1; + } else if (inrate < 76000) + *pre_proc = 0; + else if (inrate > 152000) + *pre_proc = 2; + else + *pre_proc = 1; + + /* Condition for selection of post-processing */ + post_proc_cond2 = (inrate * 15 > outrate * 16 && outrate < 56000) || + (inrate > 56000 && outrate < 56000); + post_proc_cond0 = inrate * 23 < outrate * 8; + + if (post_proc_cond2) + *post_proc = 2; + else if (post_proc_cond0) + *post_proc = 0; + else + *post_proc = 1; + + return 0; +} + +/** * Request ASRC pair * * It assigns pair by the order of A->C->B because allocation of pair B, @@ -239,8 +269,10 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair) u32 inrate, outrate, indiv, outdiv; u32 clk_index[2], div[2]; int in, out, channels; + int pre_proc, post_proc; struct clk *clk; bool ideal; + int ret; if (!config) { pair_err("invalid pair config\n"); @@ -289,6 +321,12 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair) return -EINVAL; } + ret =