[alsa-devel] [PATCH 0/6] soundwire: cadence: better logs and error handling
This is a stand-alone set of patches to improve error handling and provide better information to the platform integrator.
As suggested by Vinod, these patches are shared first - with the risk that they are separated from the actual steps where they are needed.
For reference, the complete set of 100+ patches required for SoundWire on Intel platforms is available here:
https://github.com/thesofproject/linux/pull/1692
Pierre-Louis Bossart (4): soundwire: cadence_master: filter out bad interrupts soundwire: cadence_master: log more useful information during timeouts soundwire: cadence_master: handle multiple status reports per Slave soundwire: bus: check first if Slaves become UNATTACHED
Rander Wang (2): soundwire: cadence_master: clear interrupt status before enabling interrupt soundwire: cadence_master: remove config update for interrupt setting
drivers/soundwire/bus.c | 18 +++++++++ drivers/soundwire/cadence_master.c | 60 +++++++++++++++++++++++++----- 2 files changed, 68 insertions(+), 10 deletions(-)
base-commit: b637124800a157c4df3699d1137d8533394f7678
If somehow we read the interrupt status while the IP is not powered the result is probably undefined or 0xffffffff. We do know that some of the bits are reserved and read as zero, so use as a filter to discard invalid configurations.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index fed21e2b2277..a0ec21b64d42 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -74,6 +74,7 @@ MODULE_PARM_DESC(cdns_mcp_int_mask, "Cadence MCP IntMask"); #define CDNS_MCP_INTMASK 0x48
#define CDNS_MCP_INT_IRQ BIT(31) +#define CDNS_MCP_INT_RESERVED1 GENMASK(30, 17) #define CDNS_MCP_INT_WAKEUP BIT(16) #define CDNS_MCP_INT_SLAVE_RSVD BIT(15) #define CDNS_MCP_INT_SLAVE_ALERT BIT(14) @@ -85,10 +86,12 @@ MODULE_PARM_DESC(cdns_mcp_int_mask, "Cadence MCP IntMask"); #define CDNS_MCP_INT_DATA_CLASH BIT(9) #define CDNS_MCP_INT_PARITY BIT(8) #define CDNS_MCP_INT_CMD_ERR BIT(7) +#define CDNS_MCP_INT_RESERVED2 GENMASK(6, 4) #define CDNS_MCP_INT_RX_NE BIT(3) #define CDNS_MCP_INT_RX_WL BIT(2) #define CDNS_MCP_INT_TXE BIT(1) #define CDNS_MCP_INT_TXF BIT(0) +#define CDNS_MCP_INT_RESERVED (CDNS_MCP_INT_RESERVED1 | CDNS_MCP_INT_RESERVED2)
#define CDNS_MCP_INTSET 0x4C
@@ -705,6 +708,10 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id)
int_status = cdns_readl(cdns, CDNS_MCP_INTSTAT);
+ /* check for reserved values read as zero */ + if (int_status & CDNS_MCP_INT_RESERVED) + return IRQ_NONE; + if (!(int_status & CDNS_MCP_INT_IRQ)) return IRQ_NONE;
From: Rander Wang rander.wang@intel.com
make sure all interrupts status are cleared before enabling interrupt so that there is no unexpected interrupt triggered.
Signed-off-by: Rander Wang rander.wang@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index a0ec21b64d42..847b8f5f0a32 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -856,6 +856,16 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state) mask = interrupt_mask;
update_masks: + /* clear slave interrupt status before enabling interrupt */ + if (state) { + u32 slave_state; + + slave_state = cdns_readl(cdns, CDNS_MCP_SLAVE_INTSTAT0); + cdns_writel(cdns, CDNS_MCP_SLAVE_INTSTAT0, slave_state); + slave_state = cdns_readl(cdns, CDNS_MCP_SLAVE_INTSTAT1); + cdns_writel(cdns, CDNS_MCP_SLAVE_INTSTAT1, slave_state); + } + cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0, slave_intmask0); cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1, slave_intmask1); cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
Add the type of command, device number, register offset and length to reverse engineer what caused the issue.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- 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 847b8f5f0a32..6ef2f759310d 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -447,7 +447,8 @@ _cdns_xfer_msg(struct sdw_cdns *cdns, struct sdw_msg *msg, int cmd, time = wait_for_completion_timeout(&cdns->tx_complete, msecs_to_jiffies(CDNS_TX_TIMEOUT)); if (!time) { - dev_err(cdns->dev, "IO transfer timed out\n"); + 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; return SDW_CMD_TIMEOUT; }
From: Rander Wang rander.wang@intel.com
Config only needs to be updated when setting MCP_Config, MCP_Control and MCP_CmdCtrl to make these register setting effective. When updating config in master, master will communicate with slave to update status. Communication will be failed when masters and slaves are in clock stop state, and this unnecessary config update makes interrupt setting failed.
Tested on Comet Lake with soundwire enabled
Signed-off-by: Rander Wang rander.wang@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 6ef2f759310d..19b4862e8cce 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -820,7 +820,7 @@ int sdw_cdns_exit_reset(struct sdw_cdns *cdns) EXPORT_SYMBOL(sdw_cdns_exit_reset);
/** - * sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config + * sdw_cdns_enable_interrupt() - Enable SDW interrupts * @cdns: Cadence instance */ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state) @@ -871,8 +871,7 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state) cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1, slave_intmask1); cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
- /* commit changes */ - return cdns_update_config(cdns); + return 0; } EXPORT_SYMBOL(sdw_cdns_enable_interrupt);
When a Slave reports multiple status in the sticky bits, find the latest configuration from the mirror of the PING frame status and update the status directly.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 35 +++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 19b4862e8cce..b0de7d752bdb 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -676,13 +676,36 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns,
/* first check if Slave reported multiple status */ if (set_status > 1) { + u32 val; + dev_warn_ratelimited(cdns->dev, - "Slave reported multiple Status: %d\n", - mask); - /* - * TODO: we need to reread the status here by - * issuing a PING cmd - */ + "Slave %d reported multiple Status: %d\n", + i, mask); + + /* check latest status extracted from PING commands */ + val = cdns_readl(cdns, CDNS_MCP_SLAVE_STAT); + val >>= (i * 2); + + switch (val & 0x3) { + case 0: + status[i] = SDW_SLAVE_UNATTACHED; + break; + case 1: + status[i] = SDW_SLAVE_ATTACHED; + break; + case 2: + status[i] = SDW_SLAVE_ALERT; + break; + case 3: + default: + status[i] = SDW_SLAVE_RESERVED; + break; + } + + dev_warn_ratelimited(cdns->dev, + "Slave %d status updated to %d\n", + i, status[i]); + } }
Before checking for the presence of Device0, we first need to clean-up the internal state of Slaves that are no longer attached.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/bus.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index be5d437058ed..66fd09e36b73 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -979,6 +979,24 @@ int sdw_handle_slave_status(struct sdw_bus *bus, struct sdw_slave *slave; int i, ret = 0;
+ /* first check if any Slaves fell off the bus */ + for (i = 1; i <= SDW_MAX_DEVICES; i++) { + mutex_lock(&bus->bus_lock); + if (test_bit(i, bus->assigned) == false) { + mutex_unlock(&bus->bus_lock); + continue; + } + mutex_unlock(&bus->bus_lock); + + slave = sdw_get_slave(bus, i); + if (!slave) + continue; + + if (status[i] == SDW_SLAVE_UNATTACHED && + slave->status != SDW_SLAVE_UNATTACHED) + sdw_modify_slave_status(slave, SDW_SLAVE_UNATTACHED); + } + if (status[0] == SDW_SLAVE_ATTACHED) { dev_dbg(bus->dev, "Slave attached, programming device number\n"); ret = sdw_program_device_num(bus);
On 10-01-20, 15:57, Pierre-Louis Bossart wrote:
This is a stand-alone set of patches to improve error handling and provide better information to the platform integrator.
As suggested by Vinod, these patches are shared first - with the risk that they are separated from the actual steps where they are needed.
For reference, the complete set of 100+ patches required for SoundWire on Intel platforms is available here:
Applied all, thanks
participants (2)
-
Pierre-Louis Bossart -
Vinod Koul