Re: [PATCH 1/3] soundwire: cadence: fix updating slave status when a bus has multiple peripherals

On 8/25/22 14:22, Richard Fitzgerald wrote:

From: Simon Trimmer simont@opensource.cirrus.com

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 simont@opensource.cirrus.com Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com

Goodness, this is one bug fix.

We've been chasing such issues for a while with 2 amps on the same link

https://github.com/thesofproject/linux/issues/3638 https://github.com/thesofproject/linux/issues/3325

and also came up with the same conclusion that there were false reports of UNATTACHED, and the conclusion was also to use the PING status directly.

see https://github.com/thesofproject/linux/pull/3786

I think this patch is much better than what I suggested, in that it fixes the root cause for the false report instead of double-checking if a device is truly UNATTACHED.

Nice work!

Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

---

drivers/soundwire/cadence_master.c | 63 +++++++++++++----------------- 1 file changed, 28 insertions(+), 35 deletions(-)

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_RESERVED) {
  

• 		status[i] = SDW_SLAVE_RESERVED;
  

• 		set_status++;
  

• 	}
  

• 	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_NPRESENT) {
  

• 		status[i] = SDW_SLAVE_UNATTACHED;
  

• 		set_status++;
  

• 	}
  

  }

• 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_NPRESENT) {
  

• 	status[i] = SDW_SLAVE_UNATTACHED;
  

• 	set_status++;
  

• }
  

• /* first check if Slave reported multiple status */
  

• if (set_status > 1) {
  

• 	u32 val;
  

• 	dev_warn_ratelimited(cdns->dev,
  

• 			     "Slave %d reported multiple Status: %d\n",
  

• 			     i, mask);
  

• 	/* 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]);
  

• } }