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

[Pierre-Louis Bossart]

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.

>
>> +
>>   	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??

>>   	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