On Mon 06 Dec 2021 at 18:28, Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx> wrote: > Hi Jerome, > > On Mon, Dec 6, 2021 at 11:02 AM Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote: >> >> >> On Sun 05 Dec 2021 at 19:08, Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
