[PATCH v2 0/3] soundwire: cadence: Fix oversized FIFO size define
As determined by experimentation and asking a hardware person, the FIFO in the Cadence IP is actually only 8 entries long, not 32. This is fixed in patch #1.
As a bonus, patches #2 and #3 fix two other things I noticed while debugging this.
Changes since v1: - Rewrite commit message of patch #1 - Only reduce response_buf to 34 (32 + 2) - Trim RX_FIFO_AVAIL to length of response_buf instead of expected FIFO size
Richard Fitzgerald (3): soundwire: cadence: Don't overflow the command FIFOs soundwire: cadence: Remove wasted space in response_buf soundwire: cadence: Drain the RX FIFO after an IO timeout
drivers/soundwire/cadence_master.c | 46 +++++++++++++++++++----------- drivers/soundwire/cadence_master.h | 13 ++++++++- 2 files changed, 41 insertions(+), 18 deletions(-)
The command FIFOs in the Cadence IP can be configured during design up to 32 entries, and the code in cadence_master.c was assuming the full 32-entry FIFO. But all current Intel implementations use an 8-entry FIFO.
Up to now the longest message used was 6 entries so this wasn't causing any problem. But future Cirrus Logic codecs have downloadable firmware or tuning blobs. It is more efficient for the codec driver to issue long transfers that can take advantage of any queuing in the Soundwire controller and avoid the overhead of repeatedly writing the page registers.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Fixes: 2f52a5177caa ("soundwire: cdns: Add cadence library") --- Changes since v1: Commit description rewritten. No code change. --- drivers/soundwire/cadence_master.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index a1de363eba3f..27699f341f2c 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -127,7 +127,8 @@ MODULE_PARM_DESC(cdns_mcp_int_mask, "Cadence MCP IntMask");
#define CDNS_MCP_CMD_BASE 0x80 #define CDNS_MCP_RESP_BASE 0x80 -#define CDNS_MCP_CMD_LEN 0x20 +/* FIFO can hold 8 commands */ +#define CDNS_MCP_CMD_LEN 8 #define CDNS_MCP_CMD_WORD_LEN 0x4
#define CDNS_MCP_CMD_SSP_TAG BIT(31)
The response_buf was declared much larger (128 entries) than the number of responses that could ever be written into it. The Cadence IP is configurable up to a maximum of 32 entries, and the datasheet says that RX_FIFO_AVAIL can be 2 larger than this. So allow up to 34 responses.
Also add checking in cdns_read_response() to prevent overflowing reponse_buf if RX_FIFO_AVAIL contains an unexpectedly large number.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- Changes since v1: - Make response_buf large enough for the maximum IP config of 32 plus the mysterious extra 2. - If RX_FIFO_AVAIL is too big trim it to the size of response_buf instead of CDNS_MCP_CMD_LEN. --- drivers/soundwire/cadence_master.c | 7 +++++++ drivers/soundwire/cadence_master.h | 13 ++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 27699f341f2c..a6635f7f350e 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -774,8 +774,15 @@ static void cdns_read_response(struct sdw_cdns *cdns) u32 num_resp, cmd_base; int i;
+ /* RX_FIFO_AVAIL can be 2 entries more than the FIFO size */ + BUILD_BUG_ON(ARRAY_SIZE(cdns->response_buf) < CDNS_MCP_CMD_LEN + 2); + num_resp = cdns_readl(cdns, CDNS_MCP_FIFOSTAT); num_resp &= CDNS_MCP_RX_FIFO_AVAIL; + if (num_resp > ARRAY_SIZE(cdns->response_buf)) { + dev_warn(cdns->dev, "RX AVAIL %d too long\n", num_resp); + num_resp = ARRAY_SIZE(cdns->response_buf); + }
cmd_base = CDNS_MCP_CMD_BASE;
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 0434d70d4b1f..e0a64b28c6b9 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -8,6 +8,12 @@ #define SDW_CADENCE_GSYNC_KHZ 4 /* 4 kHz */ #define SDW_CADENCE_GSYNC_HZ (SDW_CADENCE_GSYNC_KHZ * 1000)
+/* + * The Cadence IP supports up to 32 entries in the FIFO, though implementations + * can configure the IP to have a smaller FIFO. + */ +#define CDNS_MCP_IP_MAX_CMD_LEN 32 + /** * struct sdw_cdns_pdi: PDI (Physical Data Interface) instance * @@ -117,7 +123,12 @@ struct sdw_cdns { struct sdw_bus bus; unsigned int instance;
- u32 response_buf[0x80]; + /* + * The datasheet says the RX FIFO AVAIL can be 2 entries more + * than the FIFO capacity, so allow for this. + */ + u32 response_buf[CDNS_MCP_IP_MAX_CMD_LEN + 2]; + struct completion tx_complete; struct sdw_defer *defer;
If wait_for_completion_timeout() times-out in _cdns_xfer_msg() it is possible that something could have been written to the RX FIFO. In this case, we should drain the RX FIFO so that anything in it doesn't carry over and mess up the next transfer.
Obviously, if we got to this state something went wrong, and we don't really know the state of everything. The cleanup in this situation cannot be bullet-proof but we should attempt to avoid breaking future transaction, if only to reduce the amount of error noise when debugging the failure from a kernel log.
Note that this patch only implements the draining for blocking (non-deferred) transfers. The deferred API doesn't have any proper handling of error conditions and would need some re-design before implementing cleanup. That is a task for a separate patch...
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- Changes since v1: The change is "the same" but the function that is moved up the file has different content because of changes in patch #2 --- drivers/soundwire/cadence_master.c | 50 ++++++++++++++++-------------- 1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index a6635f7f350e..521387322145 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -555,6 +555,29 @@ cdns_fill_msg_resp(struct sdw_cdns *cdns, return SDW_CMD_OK; }
+static void cdns_read_response(struct sdw_cdns *cdns) +{ + u32 num_resp, cmd_base; + int i; + + /* RX_FIFO_AVAIL can be 2 entries more than the FIFO size */ + BUILD_BUG_ON(ARRAY_SIZE(cdns->response_buf) < CDNS_MCP_CMD_LEN + 2); + + num_resp = cdns_readl(cdns, CDNS_MCP_FIFOSTAT); + num_resp &= CDNS_MCP_RX_FIFO_AVAIL; + if (num_resp > ARRAY_SIZE(cdns->response_buf)) { + dev_warn(cdns->dev, "RX AVAIL %d too long\n", num_resp); + num_resp = ARRAY_SIZE(cdns->response_buf); + } + + cmd_base = CDNS_MCP_CMD_BASE; + + for (i = 0; i < num_resp; i++) { + cdns->response_buf[i] = cdns_readl(cdns, cmd_base); + cmd_base += CDNS_MCP_CMD_WORD_LEN; + } +} + static enum sdw_command_response _cdns_xfer_msg(struct sdw_cdns *cdns, struct sdw_msg *msg, int cmd, int offset, int count, bool defer) @@ -596,6 +619,10 @@ _cdns_xfer_msg(struct sdw_cdns *cdns, struct sdw_msg *msg, int cmd, dev_err(cdns->dev, "IO transfer timed out, cmd %d device %d addr %x len %d\n", cmd, msg->dev_num, msg->addr, msg->len); msg->len = 0; + + /* Drain anything in the RX_FIFO */ + cdns_read_response(cdns); + return SDW_CMD_TIMEOUT; }
@@ -769,29 +796,6 @@ EXPORT_SYMBOL(cdns_read_ping_status); * IRQ handling */
-static void cdns_read_response(struct sdw_cdns *cdns) -{ - u32 num_resp, cmd_base; - int i; - - /* RX_FIFO_AVAIL can be 2 entries more than the FIFO size */ - BUILD_BUG_ON(ARRAY_SIZE(cdns->response_buf) < CDNS_MCP_CMD_LEN + 2); - - num_resp = cdns_readl(cdns, CDNS_MCP_FIFOSTAT); - num_resp &= CDNS_MCP_RX_FIFO_AVAIL; - if (num_resp > ARRAY_SIZE(cdns->response_buf)) { - dev_warn(cdns->dev, "RX AVAIL %d too long\n", num_resp); - num_resp = ARRAY_SIZE(cdns->response_buf); - } - - cmd_base = CDNS_MCP_CMD_BASE; - - for (i = 0; i < num_resp; i++) { - cdns->response_buf[i] = cdns_readl(cdns, cmd_base); - cmd_base += CDNS_MCP_CMD_WORD_LEN; - } -} - static int cdns_update_slave_status(struct sdw_cdns *cdns, u64 slave_intstat) {
On 12/2/22 10:18, Richard Fitzgerald wrote:
As determined by experimentation and asking a hardware person, the FIFO in the Cadence IP is actually only 8 entries long, not 32. This is fixed in patch #1.
As a bonus, patches #2 and #3 fix two other things I noticed while debugging this.
Changes since v1:
- Rewrite commit message of patch #1
- Only reduce response_buf to 34 (32 + 2)
- Trim RX_FIFO_AVAIL to length of response_buf instead of expected FIFO size
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Richard Fitzgerald (3): soundwire: cadence: Don't overflow the command FIFOs soundwire: cadence: Remove wasted space in response_buf soundwire: cadence: Drain the RX FIFO after an IO timeout
drivers/soundwire/cadence_master.c | 46 +++++++++++++++++++----------- drivers/soundwire/cadence_master.h | 13 ++++++++- 2 files changed, 41 insertions(+), 18 deletions(-)
On 02-12-22, 16:18, Richard Fitzgerald wrote:
As determined by experimentation and asking a hardware person, the FIFO in the Cadence IP is actually only 8 entries long, not 32. This is fixed in patch #1.
As a bonus, patches #2 and #3 fix two other things I noticed while debugging this.
Applied, thanks
participants (3)
-
Pierre-Louis Bossart -
Richard Fitzgerald -
Vinod Koul