From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Richard Fitzgerald <rf@opensource.cirrus.com>,
vkoul@kernel.org, yung-chuan.liao@linux.intel.com,
sanyog.r.kale@intel.com
Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
patches@opensource.cirrus.com,
Simon Trimmer <simont@opensource.cirrus.com>
Subject: Re: [PATCH 1/3] soundwire: cadence: fix updating slave status when a bus has multiple peripherals
Date: Thu, 25 Aug 2022 14:57:16 +0200 [thread overview]
Message-ID: <11eedecf-d119-da5d-a0cd-0db270b74825@linux.intel.com> (raw)
In-Reply-To: <20220825122241.273090-2-rf@opensource.cirrus.com>
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]);
> -
> }
> }
>
next prev parent reply other threads:[~2022-08-25 14:37 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-25 12:22 [PATCH 0/3] soundwire: Fixes for spurious and missing UNATTACH Richard Fitzgerald
2022-08-25 12:22 ` [PATCH 1/3] soundwire: cadence: fix updating slave status when a bus has multiple peripherals Richard Fitzgerald
2022-08-25 12:57 ` Pierre-Louis Bossart [this message]
2022-08-25 12:22 ` [PATCH 2/3] soundwire: bus: Don't lose unattach notifications Richard Fitzgerald
2022-08-25 12:39 ` Pierre-Louis Bossart
2022-08-25 12:22 ` [PATCH 3/3] soundwire: bus: Fix lost UNATTACH when re-enumerating Richard Fitzgerald
2022-08-25 14:24 ` Pierre-Louis Bossart
2022-08-25 15:25 ` Richard Fitzgerald
2022-08-26 8:06 ` Pierre-Louis Bossart
2022-08-29 9:50 ` Richard Fitzgerald
2022-08-26 10:38 ` Richard Fitzgerald
2022-08-30 9:00 ` Richard Fitzgerald
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=11eedecf-d119-da5d-a0cd-0db270b74825@linux.intel.com \
--to=pierre-louis.bossart@linux.intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=linux-kernel@vger.kernel.org \
--cc=patches@opensource.cirrus.com \
--cc=rf@opensource.cirrus.com \
--cc=sanyog.r.kale@intel.com \
--cc=simont@opensource.cirrus.com \
--cc=vkoul@kernel.org \
--cc=yung-chuan.liao@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).