diff mbox series

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

Message ID 20220825122241.273090-2-rf@opensource.cirrus.com (mailing list archive)
State Superseded
Headers show
Series soundwire: Fixes for spurious and missing UNATTACH | expand

Commit Message

Richard Fitzgerald Aug. 25, 2022, 12:22 p.m. UTC 
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>
---
 drivers/soundwire/cadence_master.c | 63 +++++++++++++-----------------
 1 file changed, 28 insertions(+), 35 deletions(-)

Comments

Pierre-Louis Bossart Aug. 25, 2022, 12:57 p.m. UTC | #1 
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]);
> -
>  		}
>  	}
>
diff mbox series

Patch

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]);
-
 		}
 	}