From fbbc73a20f38dcadf8a250bc761962588cd91f7e Mon Sep 17 00:00:00 2001 From: Simon Trimmer Date: Wed, 14 Sep 2022 17:02:44 +0100 Subject: soundwire: cadence: fix updating slave status when a bus has multiple peripherals The cadence IP explicitly reports slave status changes with bits for each possible change. The function cdns_update_slave_status() attempts to translate this into the current status of each of the slaves. However when there are multiple peripherals on a bus any slave that did not have a status change when the work function ran would not have it's status updated - the array is initialised to a value that equates to UNATTACHED and this can cause spurious reports that slaves had dropped off the bus. In the case where a slave has no status change or has multiple status changes the value from the last PING command is used. Signed-off-by: Simon Trimmer Signed-off-by: Richard Fitzgerald Reviewed-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20220914160248.1047627-2-rf@opensource.cirrus.com Signed-off-by: Vinod Koul --- drivers/soundwire/cadence_master.c | 57 +++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 32 deletions(-) (limited to 'drivers/soundwire/cadence_master.c') diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 4fbb19557f5e..245191d22ccd 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -782,6 +782,7 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns, enum sdw_slave_status status[SDW_MAX_DEVICES + 1]; bool is_slave = false; u32 mask; + u32 val; int i, set_status; memset(status, 0, sizeof(status)); @@ -789,41 +790,38 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns, for (i = 0; i <= SDW_MAX_DEVICES; i++) { mask = (slave_intstat >> (i * CDNS_MCP_SLAVE_STATUS_NUM)) & CDNS_MCP_SLAVE_STATUS_BITS; - if (!mask) - continue; - is_slave = true; set_status = 0; - if (mask & CDNS_MCP_SLAVE_INTSTAT_RESERVED) { - status[i] = SDW_SLAVE_RESERVED; - set_status++; - } + if (mask) { + is_slave = true; - if (mask & CDNS_MCP_SLAVE_INTSTAT_ATTACHED) { - status[i] = SDW_SLAVE_ATTACHED; - set_status++; - } - - if (mask & CDNS_MCP_SLAVE_INTSTAT_ALERT) { - status[i] = SDW_SLAVE_ALERT; - set_status++; - } + if (mask & CDNS_MCP_SLAVE_INTSTAT_RESERVED) { + status[i] = SDW_SLAVE_RESERVED; + set_status++; + } - if (mask & CDNS_MCP_SLAVE_INTSTAT_NPRESENT) { - status[i] = SDW_SLAVE_UNATTACHED; - set_status++; - } + if (mask & CDNS_MCP_SLAVE_INTSTAT_ATTACHED) { + status[i] = SDW_SLAVE_ATTACHED; + set_status++; + } - /* first check if Slave reported multiple status */ - if (set_status > 1) { - u32 val; + if (mask & CDNS_MCP_SLAVE_INTSTAT_ALERT) { + status[i] = SDW_SLAVE_ALERT; + set_status++; + } - dev_warn_ratelimited(cdns->dev, - "Slave %d reported multiple Status: %d\n", - i, mask); + if (mask & CDNS_MCP_SLAVE_INTSTAT_NPRESENT) { + status[i] = SDW_SLAVE_UNATTACHED; + set_status++; + } + } - /* check latest status extracted from PING commands */ + /* + * check that there was a single reported Slave status and when + * there is not use the latest status extracted from PING commands + */ + if (set_status != 1) { val = cdns_readl(cdns, CDNS_MCP_SLAVE_STAT); val >>= (i * 2); @@ -842,11 +840,6 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns, status[i] = SDW_SLAVE_RESERVED; break; } - - dev_warn_ratelimited(cdns->dev, - "Slave %d status updated to %d\n", - i, status[i]); - } } -- cgit v1.2.3 From 0c5e99c41504b74dcfa9f3643f55cacab5c1e41f Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Wed, 14 Sep 2022 17:02:47 +0100 Subject: soundwire: cadence: Fix lost ATTACHED interrupts when enumerating The correct way to handle interrupts is to clear the bits we are about to handle _before_ handling them. Thus if the condition then re-asserts during the handling we won't lose it. This patch changes cdns_update_slave_status_work() to do this. The previous code cleared the interrupts after handling them. The problem with this is that when handling enumeration of devices the ATTACH statuses can be accidentally cleared and so some or all of the devices never complete their enumeration. Thus we can have a situation like this: - one or more devices are reverting to ID #0 - accumulated status bits indicate some devices attached and some on ID #0. (Remember: status bits are sticky until they are handled) - Because of device on #0 sdw_handle_slave_status() programs the device ID and exits without handling the other status, expecting to get an ATTACHED from this reprogrammed device. - The device immediately starts reporting ATTACHED in PINGs, which will assert its CDNS_MCP_SLAVE_INTSTAT_ATTACHED bit. - cdns_update_slave_status_work() clears INTSTAT0/1. If the initial status had CDNS_MCP_SLAVE_INTSTAT_ATTACHED bit set it will be cleared. - The ATTACHED change for the device has now been lost. - cdns_update_slave_status_work() clears CDNS_MCP_INT_SLAVE_MASK so if the new ATTACHED state had set it, it will be cleared without ever having been handled. Unless there is some other state change from another device to cause a new interrupt, the ATTACHED state of the reprogrammed device will never cause an interrupt so its enumeration will not be completed. Signed-off-by: Richard Fitzgerald Reviewed-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20220914160248.1047627-5-rf@opensource.cirrus.com Signed-off-by: Vinod Koul --- drivers/soundwire/cadence_master.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) (limited to 'drivers/soundwire/cadence_master.c') diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 245191d22ccd..be9cd47f31ec 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -954,9 +954,22 @@ static void cdns_update_slave_status_work(struct work_struct *work) u32 device0_status; int retry_count = 0; + /* + * Clear main interrupt first so we don't lose any assertions + * that happen during this function. + */ + cdns_writel(cdns, CDNS_MCP_INTSTAT, CDNS_MCP_INT_SLAVE_MASK); + slave0 = cdns_readl(cdns, CDNS_MCP_SLAVE_INTSTAT0); slave1 = cdns_readl(cdns, CDNS_MCP_SLAVE_INTSTAT1); + /* + * Clear the bits before handling so we don't lose any + * bits that re-assert. + */ + cdns_writel(cdns, CDNS_MCP_SLAVE_INTSTAT0, slave0); + cdns_writel(cdns, CDNS_MCP_SLAVE_INTSTAT1, slave1); + /* combine the two status */ slave_intstat = ((u64)slave1 << 32) | slave0; @@ -964,8 +977,6 @@ static void cdns_update_slave_status_work(struct work_struct *work) update_status: cdns_update_slave_status(cdns, slave_intstat); - cdns_writel(cdns, CDNS_MCP_SLAVE_INTSTAT0, slave0); - cdns_writel(cdns, CDNS_MCP_SLAVE_INTSTAT1, slave1); /* * When there is more than one peripheral per link, it's @@ -982,6 +993,11 @@ update_status: * attention with PING commands. There is no need to check for * ALERTS since they are not allowed until a non-zero * device_number is assigned. + * + * Do not clear the INTSTAT0/1. While looping to enumerate devices on + * #0 there could be status changes on other devices - these must + * be kept in the INTSTAT so they can be handled when all #0 devices + * have been handled. */ device0_status = cdns_readl(cdns, CDNS_MCP_SLAVE_STAT); @@ -1001,8 +1017,7 @@ update_status: } } - /* clear and unmask Slave interrupt now */ - cdns_writel(cdns, CDNS_MCP_INTSTAT, CDNS_MCP_INT_SLAVE_MASK); + /* unmask Slave interrupt now */ cdns_updatel(cdns, CDNS_MCP_INTMASK, CDNS_MCP_INT_SLAVE_MASK, CDNS_MCP_INT_SLAVE_MASK); -- cgit v1.2.3 From ba05b39d265bdd16913f7684600d9d41e2796745 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Fri, 16 Sep 2022 11:35:05 +0100 Subject: soundwire: cadence: Don't overwrite msg->buf during write commands The buf passed in struct sdw_msg must only be written for a READ, in that case the RDATA part of the response is the data value of the register. For a write command there is no RDATA, and buf should be assumed to be const and unmodifable. The original caller should not expect its data buffer to be corrupted by an sdw_nwrite(). Signed-off-by: Richard Fitzgerald Reviewed-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20220916103505.1562210-1-rf@opensource.cirrus.com Signed-off-by: Vinod Koul --- drivers/soundwire/cadence_master.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'drivers/soundwire/cadence_master.c') diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index be9cd47f31ec..3ef472049980 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -544,9 +544,12 @@ cdns_fill_msg_resp(struct sdw_cdns *cdns, return SDW_CMD_IGNORED; } - /* fill response */ - for (i = 0; i < count; i++) - msg->buf[i + offset] = FIELD_GET(CDNS_MCP_RESP_RDATA, cdns->response_buf[i]); + if (msg->flags == SDW_MSG_FLAG_READ) { + /* fill response */ + for (i = 0; i < count; i++) + msg->buf[i + offset] = FIELD_GET(CDNS_MCP_RESP_RDATA, + cdns->response_buf[i]); + } return SDW_CMD_OK; } -- cgit v1.2.3 From 3ed96fb4a6d8426f766687ff02fc5fb5f91575fd Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Sat, 17 Sep 2022 13:35:17 +0100 Subject: soundwire: cadence: Write to correct address for each FIFO chunk _cdns_xfer_msg() must add the fragment offset to msg->addr to get the base target address of each FIFO chunk. Otherwise every chunk will be written to the first 32 register addresses. Signed-off-by: Richard Fitzgerald Reviewed-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20220917123517.229153-1-rf@opensource.cirrus.com Signed-off-by: Vinod Koul --- drivers/soundwire/cadence_master.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/soundwire/cadence_master.c') diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 3ef472049980..ca241bbeadd9 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -569,7 +569,7 @@ _cdns_xfer_msg(struct sdw_cdns *cdns, struct sdw_msg *msg, int cmd, } base = CDNS_MCP_CMD_BASE; - addr = msg->addr; + addr = msg->addr + offset; for (i = 0; i < count; i++) { data = FIELD_PREP(CDNS_MCP_CMD_DEV_ADDR, msg->dev_num); -- cgit v1.2.3 From 7f6bad4dfde0ec1d479fdcbbb62bccdbf3a93bb4 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Sat, 17 Sep 2022 16:48:21 +0100 Subject: soundwire: cadence: Fix error check in cdns_xfer_msg() _cdns_xfer_msg() returns an sdw_command_response value, not a negative error code. Signed-off-by: Richard Fitzgerald Reviewed-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20220917154822.690472-1-rf@opensource.cirrus.com Signed-off-by: Vinod Koul --- drivers/soundwire/cadence_master.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/soundwire/cadence_master.c') diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index ca241bbeadd9..3543a923ee6b 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -708,7 +708,7 @@ cdns_xfer_msg(struct sdw_bus *bus, struct sdw_msg *msg) for (i = 0; i < msg->len / CDNS_MCP_CMD_LEN; i++) { ret = _cdns_xfer_msg(cdns, msg, cmd, i * CDNS_MCP_CMD_LEN, CDNS_MCP_CMD_LEN, false); - if (ret < 0) + if (ret != SDW_CMD_OK) goto exit; } -- cgit v1.2.3 From bafb1eacfbd98c6cdbca7e1723ef933ad371cd51 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Sat, 17 Sep 2022 16:48:22 +0100 Subject: soundwire: cadence: Simplify error paths in cdns_xfer_msg() There's no need to goto an exit label to return from cdns_xfer_msg(). It doesn't do any cleanup, only a return statement. Replace the gotos with returns. Signed-off-by: Richard Fitzgerald Reviewed-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20220917154822.690472-2-rf@opensource.cirrus.com Signed-off-by: Vinod Koul --- drivers/soundwire/cadence_master.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) (limited to 'drivers/soundwire/cadence_master.c') diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 3543a923ee6b..30b8c628fdbd 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -709,17 +709,14 @@ cdns_xfer_msg(struct sdw_bus *bus, struct sdw_msg *msg) ret = _cdns_xfer_msg(cdns, msg, cmd, i * CDNS_MCP_CMD_LEN, CDNS_MCP_CMD_LEN, false); if (ret != SDW_CMD_OK) - goto exit; + return ret; } if (!(msg->len % CDNS_MCP_CMD_LEN)) - goto exit; - - ret = _cdns_xfer_msg(cdns, msg, cmd, i * CDNS_MCP_CMD_LEN, - msg->len % CDNS_MCP_CMD_LEN, false); + return SDW_CMD_OK; -exit: - return ret; + return _cdns_xfer_msg(cdns, msg, cmd, i * CDNS_MCP_CMD_LEN, + msg->len % CDNS_MCP_CMD_LEN, false); } EXPORT_SYMBOL(cdns_xfer_msg); -- cgit v1.2.3