On Wed, 14 Apr 2021 14:00:23 +0200,
Hans Hu(SH-RD) wrote:
>
> Hi Takashi,
>
> ZHAOXIN HDAC controller has one design limitation when it works in non-snoop mode. This design limitation is: hardware can't guarantee that the write CORB cycle always completes first before the write CORBWP cycle. Here is the error scene:
> int snd_hdac_bus_send_cmd(struct hdac_bus *bus, unsigned int val)
> {
> ...
> bus->corb.buf[wp] = cpu_to_le32(val); // cycle_1: write value to CORB
> snd_hdac_chip_writew(bus, CORBWP, wp); // cycle_2: write wp to CORBWP
> ...
> }
> Normally, after cycle_2, DMA engine will start working and get data from CORB and sent it out. But if cycle_1 haven’t finished yet at this time(limitation occurs), which means the value haven’t been written into CORB, then DMA engine will get unexpected value, then error occurred. (feels like one kind of CORB underrun).
>
> If we add one read CORB cycle between cycle_1 and cycle_2, cycle_1 and cycle_2 operations will be synchronized, this limitation will be fixed. We have written a draft patch based on this situation and hope to be accepted, the patch example is as follows:
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 22af68b..c338db00 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -339,6 +339,7 @@ struct hdac_bus {
> bool sync_write:1; /* sync after verb write */
> bool use_posbuf:1; /* use position buffer */
> bool snoop:1; /* enable snooping */
> + bool no_snoop_corb_sync:1;
> bool align_bdle_4k:1; /* BDLE align 4K boundary */
> bool reverse_assign:1; /* assign devices in reverse order */
> bool corbrp_self_clear:1; /* CORBRP clears itself after reset */
> diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
> index 062da7a..6c90cdd 100644
> --- a/sound/hda/hdac_controller.c
> +++ b/sound/hda/hdac_controller.c
> @@ -167,6 +167,8 @@ int snd_hdac_bus_send_cmd(struct hdac_bus *bus, unsigned int val)
>
> bus->rirb.cmds[addr]++;
> bus->corb.buf[wp] = cpu_to_le32(val);
> + if (bus->no_snoop_corb_sync)
> + val = bus->corb.buf[wp];
> snd_hdac_chip_writew(bus, CORBWP, wp);
Just wondering whether using WRITE_ONCE() macro would be enough?
e.g.
WRITE_ONCE(bus->corb.buf[wp], cpu_to_le32(val));
Also, no_snoop_corb_sync is a bit confusing. What you do is rather
sync of the written value, so it can be taken as if other way round.
Maybe no_coherent_corb_write or such would be better understandable.
thanks,
Takashi
