Re: [PATCH RFC v1 2/2] ASoC: meson: aiu: Move AIU_I2S_MISC hold setting to aiu-fifo-i2s

[Jerome Brunet <jbrunet@baylibre.com>]

On Mon 06 Dec 2021 at 18:28, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Jerome,
>
> On Mon, Dec 6, 2021 at 11:02 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>>
>>
>> On Sun 05 Dec 2021 at 19:08, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>>
>> > The out-of-tree vendor driver uses the following approach to set the
>> > AIU_I2S_MISC register:
>> > 1) write AIU_MEM_I2S_START_PTR and AIU_MEM_I2S_RD_PTR
>> > 2) configure AIU_I2S_MUTE_SWAP[15:0]
>> > 3) write AIU_MEM_I2S_END_PTR
>> > 4) set AIU_I2S_MISC[2] to 1 (documented as: "put I2S interface in hold
>> >    mode")
>> > 5) set AIU_I2S_MISC[4] to 1 (depending on the driver revision it always
>> >    stays at 1 while for older drivers this bit is unset in step 4)
>> > 6) set AIU_I2S_MISC[2] to 0
>> > 7) write AIU_MEM_I2S_MASKS
>> > 8) toggle AIU_MEM_I2S_CONTROL[0]
>> > 9) toggle AIU_MEM_I2S_BUF_CNTL[0]
>> >
>> > Additional testing shows that when AIU_I2S_MISC[2] is set to 1 then no
>> > interrupts are generated anymore. The way this bit is managed by the
>> > vendor driver as well as not getting any interrupts can mean that it's
>> > related to the FIFO and not the encoder.
>>
>> Not necessarily. If the encoder stops pulling data, the FIFO will going
>> over the DDR. Since it generates an IRQ after reading N bytes, IRQ would
>> stop too. AFAIK, if the pipeline is stalled, the IRQ stops anyway
> ah, right. so I think you're right: it can be either way
>
>> ... but this is not really important
> I'll remove that section from the description in v2
>
>> >
>> > Move setting the AIU_I2S_MISC[2] bit to aiu_fifo_i2s_hw_params() so it
>> > closer resembles the flow in the vendor kernel. While here also
>> > configure AIU_I2S_MISC[4] (documented as: "force each audio data to
>> > left or right according to the bit attached with the audio data")
>> > similar to how the vendor driver does this.
>>
>> I understand the part of HOLD, not about FORCE_LR.
>> Is it necessary to fix the problem ? Have you tested without this change
>> ?
> On my Le Potato board (GXL / S905X) FORCE_LR is either enabled by the
> bootloader or being enabled is the default value.
> All versions of the vendor driver are also setting it in some way,

+1
Would you mind adding a comment in the code saying just that - so we
know why it's there ?

> including the latest one that I have access to [0].
> I prefer to keep this explicit write in for two reasons:
> - we're not hit by surprise if some SoC/bootloaders don't set this bit
> by default
> - the code in the mainline does not skip anything that the vendor
> driver does

You can bet I've skipped a fair share of what the vendor driver does, on
purpose.
>
> To specifically answer your question: I have not tried whether this
> bit needs to be set and if unsetting it manually breaks anything.
>
>> > This fixes the infamous and
>> > long-standing "machine gun noise" issue (a buffer underrun issue).
>>
>> Well done ! It took us a while to nail it, Thanks a lot !!
> Thanks - this took a while to figure out but it's great to finally
> have it solved!
>
>
> Best regards,
> Martin
>
>
> [0] https://github.com/khadas/linux/blob/khadas-vims-4.9.y/sound/soc/amlogic/meson/audio_hw.c#L194:L202

With this, feel free to repost without the RFC tag and with my

Acked-by: Jerome Brunet <jbrunet@baylibre.com>