[Sound-open-firmware] [PATCH] cnl: ssp: fix DSP_B setting in ssp
set ssp according to DSP_B spec
Signed-off-by: Rander Wang rander.wang@linux.intel.com --- src/drivers/apl-ssp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index a726f3c..c32bd35 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -333,10 +333,11 @@ static inline int ssp_set_config(struct dai *dai, sscr0 |= SSCR0_MOD | SSCR0_FRDC(config->num_slots);
/* set asserted frame length */ - frame_len = config->sample_container_bits; + frame_len = 1;
/* handle frame polarity, DSP_A default is rising/active high */ - sspsp |= SSPSP_SFRMP(inverted_frame); + sspsp |= SSPSP_SFRMP(!inverted_frame); + sspsp |= SSPSP_FSRT;
break; default:
On Wed, 2018-03-07 at 12:31 +0800, Rander Wang wrote:
set ssp according to DSP_B spec
Signed-off-by: Rander Wang rander.wang@linux.intel.com
src/drivers/apl-ssp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Applied.
Thanks
Liam
On 3/6/18 10:31 PM, Rander Wang wrote:
set ssp according to DSP_B spec
Signed-off-by: Rander Wang rander.wang@linux.intel.com
src/drivers/apl-ssp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index a726f3c..c32bd35 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -333,10 +333,11 @@ static inline int ssp_set_config(struct dai *dai, sscr0 |= SSCR0_MOD | SSCR0_FRDC(config->num_slots);
/* set asserted frame length */
frame_len = config->sample_container_bits;
frame_len = 1;
this needs to be done for DSP_B as well.
/* handle frame polarity, DSP_A default is rising/active high */
sspsp |= SSPSP_SFRMP(inverted_frame);
sspsp |= SSPSP_SFRMP(!inverted_frame);
I don't think this is correct. the documentation says with this bit asserted (value=1) the frame is active high which is exactly what we want.
sspsp |= SSPSP_FSRT;
why is this necessary? this disables a fix for dummy stop/padding.
break;default:
On Wed, 2018-03-07 at 08:01 -0600, Pierre-Louis Bossart wrote:
On 3/6/18 10:31 PM, Rander Wang wrote:
set ssp according to DSP_B spec
Signed-off-by: Rander Wang rander.wang@linux.intel.com
src/drivers/apl-ssp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index a726f3c..c32bd35 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -333,10 +333,11 @@ static inline int ssp_set_config(struct dai *dai, sscr0 |= SSCR0_MOD | SSCR0_FRDC(config-
num_slots);
/* set asserted frame length */
frame_len = config->sample_container_bits;
frame_len = 1;this needs to be done for DSP_B as well.
/* handle frame polarity, DSP_A default isrising/active high */
sspsp |= SSPSP_SFRMP(inverted_frame);
sspsp |= SSPSP_SFRMP(!inverted_frame);I don't think this is correct. the documentation says with this bit asserted (value=1) the frame is active high which is exactly what we want.
Rander, I'm assuming this was validated with a scope ?
Liam
hi Liam,
yes all the clk and data output are validated by scope.
And it is also validated by ZhiGang on APL. And the setting
is compared to the setting used by COE provided by YuXin.
Rander
On 3/7/2018 10:46 PM, Liam Girdwood wrote:
On Wed, 2018-03-07 at 08:01 -0600, Pierre-Louis Bossart wrote:
On 3/6/18 10:31 PM, Rander Wang wrote:
set ssp according to DSP_B spec
Signed-off-by: Rander Wang rander.wang@linux.intel.com
src/drivers/apl-ssp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index a726f3c..c32bd35 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -333,10 +333,11 @@ static inline int ssp_set_config(struct dai *dai, sscr0 |= SSCR0_MOD | SSCR0_FRDC(config-
num_slots);
/* set asserted frame length */
frame_len = config->sample_container_bits;
frame_len = 1;this needs to be done for DSP_B as well.
/* handle frame polarity, DSP_A default isrising/active high */
sspsp |= SSPSP_SFRMP(inverted_frame);
sspsp |= SSPSP_SFRMP(!inverted_frame);I don't think this is correct. the documentation says with this bit asserted (value=1) the frame is active high which is exactly what we want.
Rander, I'm assuming this was validated with a scope ?
Liam
On 3/7/18 7:28 PM, rander.wang wrote:
hi Liam,
yes all the clk and data output are validated by scope.
And it is also validated by ZhiGang on APL. And the setting
is compared to the setting used by COE provided by YuXin.
Can you share the waveforms please. With all due respect, I believe you are trying to match a setting and not follow ASoC descriptions.
If you are trying to match a setting, you should do so with changes in topology files, not force arbitrary conventions in the code.
Rander
On 3/7/2018 10:46 PM, Liam Girdwood wrote:
On Wed, 2018-03-07 at 08:01 -0600, Pierre-Louis Bossart wrote:
On 3/6/18 10:31 PM, Rander Wang wrote:
set ssp according to DSP_B spec
Signed-off-by: Rander Wang rander.wang@linux.intel.com
src/drivers/apl-ssp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index a726f3c..c32bd35 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -333,10 +333,11 @@ static inline int ssp_set_config(struct dai *dai, sscr0 |= SSCR0_MOD | SSCR0_FRDC(config-
num_slots);
/* set asserted frame length */ - frame_len = config->sample_container_bits; + frame_len = 1;
this needs to be done for DSP_B as well.
/* handle frame polarity, DSP_A default is rising/active high */ - sspsp |= SSPSP_SFRMP(inverted_frame); + sspsp |= SSPSP_SFRMP(!inverted_frame);
I don't think this is correct. the documentation says with this bit asserted (value=1) the frame is active high which is exactly what we want.
Rander, I'm assuming this was validated with a scope ?
Liam
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On 3/8/2018 10:14 AM, Pierre-Louis Bossart wrote:
On 3/7/18 7:28 PM, rander.wang wrote:
hi Liam,
yes all the clk and data output are validated by scope.
And it is also validated by ZhiGang on APL. And the setting
is compared to the setting used by COE provided by YuXin.
Can you share the waveforms please. With all due respect, I believe you are trying to match a setting and not follow ASoC descriptions.
If you are trying to match a setting, you should do so with changes in topology files, not force arbitrary conventions in the code.
i sent the email with pictures, but it is held:
The reason it is being held:
Message body is too big: 900413 bytes with a limit of 60 KB
And i have sent the pictures to you and Rajat.
maybe what i said is different from you.
what i mean is : data is sent when frame clk voltage keeping low, frame sync is at keeping high
I know we have a talk with Rajat about topology, but i have make it clear it is no need to modify topology, it is a error in FW
Rander
On 3/7/2018 10:46 PM, Liam Girdwood wrote:
On Wed, 2018-03-07 at 08:01 -0600, Pierre-Louis Bossart wrote:
On 3/6/18 10:31 PM, Rander Wang wrote:
set ssp according to DSP_B spec
Signed-off-by: Rander Wang rander.wang@linux.intel.com
src/drivers/apl-ssp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index a726f3c..c32bd35 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -333,10 +333,11 @@ static inline int ssp_set_config(struct dai *dai, sscr0 |= SSCR0_MOD | SSCR0_FRDC(config-
num_slots);
/* set asserted frame length */ - frame_len = config->sample_container_bits; + frame_len = 1;
this needs to be done for DSP_B as well.
/* handle frame polarity, DSP_A default is rising/active high */ - sspsp |= SSPSP_SFRMP(inverted_frame); + sspsp |= SSPSP_SFRMP(!inverted_frame);
I don't think this is correct. the documentation says with this bit asserted (value=1) the frame is active high which is exactly what we want.
Rander, I'm assuming this was validated with a scope ?
Liam
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Hi Pierre,
On 3/7/2018 10:01 PM, Pierre-Louis Bossart wrote:
On 3/6/18 10:31 PM, Rander Wang wrote:
set ssp according to DSP_B spec
Signed-off-by: Rander Wang rander.wang@linux.intel.com
src/drivers/apl-ssp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index a726f3c..c32bd35 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -333,10 +333,11 @@ static inline int ssp_set_config(struct dai *dai, sscr0 |= SSCR0_MOD | SSCR0_FRDC(config->num_slots); /* set asserted frame length */ - frame_len = config->sample_container_bits; + frame_len = 1;
this needs to be done for DSP_B as well.
you mean DSP_A ? yes, this patch is for DSP_B, next DSP_A will be validated
/* handle frame polarity, DSP_A default is rising/active high */ - sspsp |= SSPSP_SFRMP(inverted_frame); + sspsp |= SSPSP_SFRMP(!inverted_frame);
I don't think this is correct. the documentation says with this bit asserted (value=1) the frame is active high which is exactly what we want.
No, the frame active low is what we want according to the ASOC spec. Data is sent when active low, frame sync is at active high.
And without this modification, the frame sync clock is rotated 180 compared to spec. And it is validated by Zhigang on APL and
also it is set by COE
+ sspsp |= SSPSP_FSRT;
why is this necessary? this disables a fix for dummy stop/padding.
it is get from COE setting.
break; default:
On 3/7/18 7:43 PM, rander.wang wrote:
Hi Pierre,
On 3/7/2018 10:01 PM, Pierre-Louis Bossart wrote:
On 3/6/18 10:31 PM, Rander Wang wrote:
set ssp according to DSP_B spec
Signed-off-by: Rander Wang rander.wang@linux.intel.com
src/drivers/apl-ssp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index a726f3c..c32bd35 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -333,10 +333,11 @@ static inline int ssp_set_config(struct dai *dai, sscr0 |= SSCR0_MOD | SSCR0_FRDC(config->num_slots); /* set asserted frame length */ - frame_len = config->sample_container_bits; + frame_len = 1;
this needs to be done for DSP_B as well.
you mean DSP_A ? yes, this patch is for DSP_B, next DSP_A will be validated
yes
/* handle frame polarity, DSP_A default is rising/active high */ - sspsp |= SSPSP_SFRMP(inverted_frame); + sspsp |= SSPSP_SFRMP(!inverted_frame);
I don't think this is correct. the documentation says with this bit asserted (value=1) the frame is active high which is exactly what we want.
No, the frame active low is what we want according to the ASOC spec. Data is sent when active low, frame sync is at active high.
Look at the diagrams I shared offline, this is not what you are describing. We want the frame sync to be indicated by a low->high transition (rising edge) immediately following a falling edge of the bit clock.
And without this modification, the frame sync clock is rotated 180 compared to spec. And it is validated by Zhigang on APL and
also it is set by COE
+ sspsp |= SSPSP_FSRT;
why is this necessary? this disables a fix for dummy stop/padding.
it is get from COE setting.
that's not an explanation, sorry.
break; default:
participants (4)
-
Liam Girdwood -
Pierre-Louis Bossart -
Rander Wang -
rander.wang