[Sound-open-firmware] [PATCH] apl-ssp: fix capture double speed issue

[Liam Girdwood]

On Wed, 2018-03-14 at 11:37 -0500, Pierre-Louis Bossart wrote:
> 
> On 03/14/2018 11:04 AM, Liam Girdwood wrote:
> > On Wed, 2018-03-14 at 23:12 +0800, Keyon Jie wrote:
> > > Previously TFT and RFT are set to 0s which means we are using
> > > threshold level 1, where interrupt asserts for every valid bits
> > > of each slot(e.g. for s16_le capture, interrupts assert for
> > > each 16 bits), but DMAC will treat it as notification of one
> > > full sample(e.g. 32 bits for s16_le stereo) and read those all
> > > bits, this will lead to capture with double speed at format
> > > s16_le stereo.
> > > 
> > > Here add handle for FIFO trigger Threshold level, set them to
> > > bytes for each full sample(active_slots * valid_bits / 8), and
> > > the issue is fixed.
> > > 
> > > Signed-off-by: Keyon Jie <yang.jie at linux.intel.com>
> > > ---
> > >   src/drivers/apl-ssp.c | 25 +++++++++++++++++++++++++
> > >   1 file changed, 25 insertions(+)
> > > 
> > > diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c
> > > index 1ee107c..ecf70fb 100644
> > > --- a/src/drivers/apl-ssp.c
> > > +++ b/src/drivers/apl-ssp.c
> > > @@ -43,6 +43,19 @@
> > >   #define trace_ssp_error(__e)	trace_error(TRACE_CLASS_SSP, __e)
> > >   #define tracev_ssp(__e)	tracev_event(TRACE_CLASS_SSP, __e)
> > >   
> > > +/* FIXME: move this to a helper and optimize */
> > > +static int hweight_32(uint32_t mask)
> > 
> > count_set_bits32() and can you move to lib/lib.c probably separate patch
> > too.
> 
> Came from ssp.c, something I never optimized...
> > 
> > > +{
> > > +	int i;
> > > +	int count = 0;
> > > +
> > > +	for (i = 0; i < 32; i++) {
> > > +		count += mask & 1;
> > > +		mask >>= 1;
> > > +	}
> > > +	return count;
> > > +}
> > > +
> > >   /* save SSP context prior to entering D3 */
> > >   static int ssp_context_store(struct dai *dai)
> > >   {
> > > @@ -96,6 +109,9 @@ static inline int ssp_set_config(struct dai *dai,
> > >   	uint32_t dummy_stop;
> > >   	uint32_t frame_len = 0;
> > >   	uint32_t bdiv_min;
> > > +	uint32_t active_tx_slots = 2;
> > > +	uint32_t active_rx_slots = 2;
> > 
> > why do we use 2 as default value, for stereo. ?
> > 
> > This value should always be set based on channel mask so I would set to 0
> > here
> > and check that it's not 0 later on (return error and produce trace if 0).
> 
> this was an 'optimization' based on the fact that for I2S/LEFT_J we can 
> keep this as stereo.
> If we don't set those then we have to manually set this to two in the 
> switch case.
> 

Some of the data used comes from topology so we would need to make sure that
topology tx/rx slot mask would be non zero as they are used to multiply this
value;. 

> > 
> > > +
> > >   	bool inverted_frame = false;
> > >   	int ret = 0;
> > >   
> > > @@ -353,6 +369,9 @@ static inline int ssp_set_config(struct dai *dai,
> > >   		sspsp |= SSPSP_SFRMP(!inverted_frame);
> > >   		sspsp |= SSPSP_FSRT;
> > >   
> > > +		active_tx_slots = hweight_32(config->tx_slot_mask);
> > > +		active_rx_slots = hweight_32(config->rx_slot_mask);
> > > +
> > >   		break;
> > >   	case SOF_DAI_FMT_DSP_B:
> > >   
> > > @@ -371,6 +390,9 @@ static inline int ssp_set_config(struct dai *dai,
> > >   		 */
> > >   		sspsp |= SSPSP_SFRMP(!inverted_frame);
> > >   
> > > +		active_tx_slots = hweight_32(config->tx_slot_mask);
> > > +		active_rx_slots = hweight_32(config->rx_slot_mask);
> > > +
> > >   		break;
> > >   	default:
> > >   		trace_ssp_error("eca");
> > > @@ -408,6 +430,9 @@ static inline int ssp_set_config(struct dai *dai,
> > >   	/* bypass divider for MCLK */
> > >   	mdivr = 0x00000fff;
> > >   
> > > +	sscr3 |= SSCR3_TX(active_tx_slots * config->sample_valid_bits /
> > > 8) |
> > > +		 SSCR3_RX(active_rx_slots * config->sample_valid_bits /
> > > 8);
> > > +
> 
> For Baytrail, we had:
> 
>      /* FIXME:
>       * watermarks - (RFT + 1) should equal DMA SRC_MSIZE
>       */
>      sfifott = (SFIFOTT_TX(2*active_tx_slots) |
>             SFIFOTT_RX(2*active_rx_slots));
> 
> Don't we need to align RFT/TFT to the DMA_SRC_MSIZE as well? e.g. for 24 
> bits can we actually have a threshold that would be a multiple of 3??

Not sure, iirc one of the watermark registers is deprecated.

Liam

> 
> > >   	trace_ssp("coe");
> > >   	ssp_write(dai, SSCR0, sscr0);
> > >   	ssp_write(dai, SSCR1, sscr1);
> > 
> > _______________________________________________
> > Sound-open-firmware mailing list
> > Sound-open-firmware at alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
> 
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware