[Sound-open-firmware] [PATCH] SSP: refine padding bit setting on Broadwell
Pierre-Louis Bossart
pierre-louis.bossart at linux.intel.com
Fri Jun 1 15:54:23 CEST 2018
On 6/1/18 3:50 AM, Rander Wang wrote:
> merge commit 36f23c6cc3ed ("apl-ssp: fix padding bit issues in
> I2S/LEFT_J mode") and refine it on Broadwell.
>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> Signed-off-by: Rander Wang <rander.wang at linux.intel.com>
>
> ---
> test it on Broadwell
are the waveforms correct with this patch? What tests did you run?
I am also concerned that we are losing alignment with the Baytrail code,
and it becomes difficult to track the differences in register settings
between platforms.
> ---
> src/drivers/hsw-ssp.c | 99 ++++++++++++++++++++++++++++++++++++++++++---------
> src/include/sof/ssp.h | 11 +++---
> 2 files changed, 87 insertions(+), 23 deletions(-)
>
> diff --git a/src/drivers/hsw-ssp.c b/src/drivers/hsw-ssp.c
> index 7e5ae3d..b2fa99e 100644
> --- a/src/drivers/hsw-ssp.c
> +++ b/src/drivers/hsw-ssp.c
> @@ -77,11 +77,16 @@ static inline int ssp_set_config(struct dai *dai,
> uint32_t sscr1;
> uint32_t sscr2;
> uint32_t sspsp;
> + uint32_t sspsp2;
> uint32_t mdiv;
> uint32_t bdiv;
> uint32_t data_size;
> uint32_t start_delay;
> + uint32_t frame_end_padding;
> + uint32_t slot_end_padding;
> uint32_t frame_len = 0;
> + uint32_t bdiv_min;
> + uint32_t format;
> bool inverted_frame = false;
> int ret = 0;
>
> @@ -130,6 +135,9 @@ static inline int ssp_set_config(struct dai *dai,
> /* sspsp dynamic settings are SCMODE, SFRMP, DMYSTRT, SFRMWDTH */
> sspsp = SSPSP_ETDS; /* make sure SDO line is tri-stated when inactive */
>
> + /* sspsp2 no dynamic setting */
> + sspsp2 = 0x0;
> +
> ssp->config = *config;
> ssp->params = config->ssp;
>
> @@ -252,29 +260,83 @@ static inline int ssp_set_config(struct dai *dai,
> goto out;
> }
>
> - /* format */
> - switch (config->format & SOF_DAI_FMT_FORMAT_MASK) {
> - case SOF_DAI_FMT_I2S:
> -
> - start_delay = 1;
> -
> - /* set asserted frame length */
> - frame_len = config->ssp.tdm_slot_width;
> -
> - /* handle frame polarity, I2S default is falling/active low */
> - sspsp |= SSPSP_SFRMP(!inverted_frame);
> + bdiv_min = config->ssp.tdm_slots * config->ssp.sample_valid_bits;
> + if (bdiv < bdiv_min) {
> + trace_ssp_error("ecc");
> + ret = -EINVAL;
> + goto out;
> + }
>
> - break;
> + frame_end_padding = bdiv - bdiv_min;
> + if (frame_end_padding > SSPSP2_FEP_MASK) {
> + trace_ssp_error("ecd");
> + ret = -EINVAL;
> + goto out;
> + }
>
> + /* format */
> + format = config->format & SOF_DAI_FMT_FORMAT_MASK;
> + switch (format) {
> + case SOF_DAI_FMT_I2S:
> case SOF_DAI_FMT_LEFT_J:
>
> - start_delay = 0;
> + if (format == SOF_DAI_FMT_I2S) {
> + start_delay = 1;
this sort of code is not good for MISRA compliance.
You want one self-contained case at a time, and no additional tests in
the middle.
Can you please refactor in a follow-up patch?
> +
> + /*
> + * handle frame polarity, I2S default is falling/active low,
> + * non-inverted(inverted_frame=0) -- active low(SFRMP=0),
> + * inverted(inverted_frame=1) -- rising/active high(SFRMP=1),
> + * so, we should set SFRMP to inverted_frame.
> + */
> + sspsp |= SSPSP_SFRMP(inverted_frame);
> + sspsp |= SSPSP_FSRT;
> +
> + } else {
> + start_delay = 0;
> +
> + /*
> + * handle frame polarity, LEFT_J default is rising/active high,
> + * non-inverted(inverted_frame=0) -- active high(SFRMP=1),
> + * inverted(inverted_frame=1) -- falling/active low(SFRMP=0),
> + * so, we should set SFRMP to !inverted_frame.
> + */
> + sspsp |= SSPSP_SFRMP(!inverted_frame);
> + }
>
> - /* set asserted frame length */
> - frame_len = config->ssp.tdm_slot_width;
> + sscr0 |= SSCR0_FRDC(config->ssp.tdm_slots);
>
> - /* LEFT_J default is rising/active high, opposite of I2S */
> - sspsp |= SSPSP_SFRMP(inverted_frame);
> + if (bdiv % 2) {
> + trace_ssp_error("eca");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /* set asserted frame length to half frame length */
> + frame_len = bdiv / 2;
> +
> + /*
> + * for I2S/LEFT_J, the padding has to happen at the end
> + * of each slot
> + */
> + if (frame_end_padding % 2) {
> + trace_ssp_error("ece");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + slot_end_padding = frame_end_padding / 2;
> +
> + if (slot_end_padding > 15) {
> + /* can't handle padding over 15 bits */
> + trace_ssp_error("ecf");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + sspsp |= SSPSP_DMYSTOP(slot_end_padding & SSPSP_DMYSTOP_MASK);
> + slot_end_padding >>= SSPSP_DMYSTOP_BITS;
> + sspsp |= SSPSP_EDMYSTOP(slot_end_padding & SSPSP_EDMYSTOP_MASK);
>
> break;
> case SOF_DAI_FMT_DSP_A:
> @@ -288,6 +350,7 @@ static inline int ssp_set_config(struct dai *dai,
>
> /* handle frame polarity, DSP_A default is rising/active high */
> sspsp |= SSPSP_SFRMP(inverted_frame);
> + sspsp2 |= (frame_end_padding & SSPSP2_FEP_MASK);
>
> break;
> case SOF_DAI_FMT_DSP_B:
> @@ -301,6 +364,7 @@ static inline int ssp_set_config(struct dai *dai,
>
> /* handle frame polarity, DSP_A default is rising/active high */
> sspsp |= SSPSP_SFRMP(inverted_frame);
> + sspsp2 |= (frame_end_padding & SSPSP2_FEP_MASK);
>
> break;
> default:
> @@ -329,6 +393,7 @@ static inline int ssp_set_config(struct dai *dai,
> ssp_write(dai, SSPSP, sspsp);
> ssp_write(dai, SSTSA, config->ssp.tx_slots);
> ssp_write(dai, SSRSA, config->ssp.rx_slots);
> + ssp_write(dai, SSPSP2, sspsp2);
>
> ssp->state[DAI_DIR_PLAYBACK] = COMP_STATE_PREPARE;
> ssp->state[DAI_DIR_CAPTURE] = COMP_STATE_PREPARE;
> diff --git a/src/include/sof/ssp.h b/src/include/sof/ssp.h
> index d34b011..3a1f6ed 100644
> --- a/src/include/sof/ssp.h
> +++ b/src/include/sof/ssp.h
> @@ -55,12 +55,12 @@
> #define SSTSA 0x30
> #define SSRSA 0x34
> #define SSTSS 0x38
> +#define SSCR2 0x40
>
> #if defined CONFIG_BAYTRAIL ||\
> defined CONFIG_CHERRYTRAIL ||\
> defined CONFIG_BROADWELL ||\
> defined CONFIG_HASWELL
> -#define SSCR2 0x40
> #define SFIFOTT 0x6C
> #define SSCR3 0x70
> #define SSCR4 0x74
> @@ -159,14 +159,13 @@ extern const struct dai_ops ssp_ops;
> #define SSPSP_DMYSTOP_BITS 2
> #define SSPSP_DMYSTOP_MASK ((0x1 << SSPSP_DMYSTOP_BITS) - 1)
> #define SSPSP_FSRT (1 << 25)
> -
> -#if defined CONFIG_APOLLOLAKE || defined CONFIG_CANNONLAKE
> #define SSPSP_EDMYSTOP(x) ((x) << 26)
> #define SSPSP_EDMYSTOP_MASK 0x7
> +
> +#define SSPSP2 0x44
> #define SSPSP2_FEP_MASK 0xff
> -#define SSTSS 0x38
> -#define SSCR2 0x40
> -#define SSPSP2 0x44
> +
> +#if defined CONFIG_APOLLOLAKE || defined CONFIG_CANNONLAKE
> #define SSCR3 0x48
> #define SSIOC 0x4C
>
>
More information about the Sound-open-firmware
mailing list