[PATCH 00/16] SoundWire: cadence: add clock stop and fix programming sequences
To make progress with SoundWire support, this patchset provides the missing support for clock stop modes, and revisits all Cadence Master register settings. The current code is for some reason not aligned with internal documentation and hardware recommended flows, specifically for multi-link operation.
Pierre-Louis Bossart (12): soundwire: cadence: s/update_config/config_update soundwire: cadence: handle error cases with CONFIG_UPDATE soundwire: cadence: mask Slave interrupt before stopping clock soundwire: cadence: merge routines to clear/set bits soundwire: cadence: move clock/SSP related inits to dedicated function soundwire: cadence: make SSP interval programmable soundwire: cadence: reorder MCP_CONFIG settings soundwire: cadence: enable NORMAL operation in cdns_init() soundwire: cadence: remove PREQ_DELAY assignment soundwire: cadence: remove automatic command retries soundwire: cadence: commit changes in the exit_reset() sequence soundwire: cadence: multi-link support
Rander Wang (4): soundwire: cadence: simplifiy cdns_init() soundwire: cadence: add interface to check clock status soundwire: cadence: add clock_stop/restart routines soundwire: cadence: fix a io timeout issue in S3 test
drivers/soundwire/cadence_master.c | 297 ++++++++++++++++++++++++----- drivers/soundwire/cadence_master.h | 9 +- drivers/soundwire/intel.c | 2 +- 3 files changed, 261 insertions(+), 47 deletions(-)
base-commit: 5de79ba865d7770c3bdde7c266ed425832764aac
Somehow we inverted the two, align with register definition to avoid further confusion.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 9bec270d0fa4..a1a889d1d7dc 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -235,7 +235,7 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value) * all changes to the MCP_CONFIG, MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL * need to be confirmed with a write to MCP_CONFIG_UPDATE */ -static int cdns_update_config(struct sdw_cdns *cdns) +static int cdns_config_update(struct sdw_cdns *cdns) { int ret;
@@ -838,7 +838,7 @@ int sdw_cdns_exit_reset(struct sdw_cdns *cdns) CDNS_MCP_CONFIG_OP_NORMAL);
/* commit changes */ - return cdns_update_config(cdns); + return cdns_config_update(cdns); } EXPORT_SYMBOL(sdw_cdns_exit_reset);
@@ -1084,7 +1084,7 @@ int sdw_cdns_init(struct sdw_cdns *cdns, bool clock_stop_exit) cdns_writel(cdns, CDNS_MCP_CONFIG, val);
/* commit changes */ - return cdns_update_config(cdns); + return cdns_config_update(cdns); } EXPORT_SYMBOL(sdw_cdns_init);
From: Rander Wang rander.wang@intel.com
There is no need for the clock_stop_exit argument with the latest implementation
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 | 12 +----------- drivers/soundwire/cadence_master.h | 2 +- drivers/soundwire/intel.c | 2 +- 3 files changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index a1a889d1d7dc..941809ea00a8 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1018,22 +1018,12 @@ static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols) * sdw_cdns_init() - Cadence initialization * @cdns: Cadence instance */ -int sdw_cdns_init(struct sdw_cdns *cdns, bool clock_stop_exit) +int sdw_cdns_init(struct sdw_cdns *cdns) { struct sdw_bus *bus = &cdns->bus; struct sdw_master_prop *prop = &bus->prop; u32 val; int divider; - int ret; - - if (clock_stop_exit) { - ret = cdns_clear_bit(cdns, CDNS_MCP_CONTROL, - CDNS_MCP_CONTROL_CLK_STOP_CLR); - if (ret < 0) { - dev_err(cdns->dev, "Couldn't exit from clock stop\n"); - return ret; - } - }
/* Set clock divider */ divider = (prop->mclk_freq / prop->max_clk_freq) - 1; diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 2de1b2493ffc..44e802bba702 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -138,7 +138,7 @@ extern struct sdw_master_ops sdw_cdns_master_ops; irqreturn_t sdw_cdns_irq(int irq, void *dev_id); irqreturn_t sdw_cdns_thread(int irq, void *dev_id);
-int sdw_cdns_init(struct sdw_cdns *cdns, bool clock_stop_exit); +int sdw_cdns_init(struct sdw_cdns *cdns); int sdw_cdns_pdi_init(struct sdw_cdns *cdns, struct sdw_cdns_stream_config config); int sdw_cdns_exit_reset(struct sdw_cdns *cdns); diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index a327669c757b..3c83e76c6bf9 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1077,7 +1077,7 @@ static int intel_init(struct sdw_intel *sdw) intel_link_power_up(sdw); intel_shim_init(sdw);
- return sdw_cdns_init(&sdw->cdns, false); + return sdw_cdns_init(&sdw->cdns); }
/*
From: Rander Wang rander.wang@intel.com
If master is in clock stop state, driver can't modify registers in master except the registers for clock stop setting.
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 | 19 +++++++++++++++++++ drivers/soundwire/cadence_master.h | 2 ++ 2 files changed, 21 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 941809ea00a8..71cba2585151 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1207,6 +1207,25 @@ static const struct sdw_master_port_ops cdns_port_ops = { .dpn_port_enable_ch = cdns_port_enable, };
+/** + * sdw_cdns_is_clock_stop: Check clock status + * + * @cdns: Cadence instance + */ +bool sdw_cdns_is_clock_stop(struct sdw_cdns *cdns) +{ + u32 status; + + status = cdns_readl(cdns, CDNS_MCP_STAT) & CDNS_MCP_STAT_CLK_STOP; + if (status) { + dev_dbg(cdns->dev, "Clock is stopped\n"); + return true; + } + + return false; +} +EXPORT_SYMBOL(sdw_cdns_is_clock_stop); + /** * sdw_cdns_probe() - Cadence probe routine * @cdns: Cadence instance diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 44e802bba702..691faa386889 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -144,6 +144,8 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns, int sdw_cdns_exit_reset(struct sdw_cdns *cdns); int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
+bool sdw_cdns_is_clock_stop(struct sdw_cdns *cdns); + #ifdef CONFIG_DEBUG_FS void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root); #endif
On 11-03-20, 13:41, Pierre-Louis Bossart wrote:
From: Rander Wang rander.wang@intel.com
If master is in clock stop state, driver can't modify registers in master except the registers for clock stop setting.
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 | 19 +++++++++++++++++++ drivers/soundwire/cadence_master.h | 2 ++ 2 files changed, 21 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 941809ea00a8..71cba2585151 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1207,6 +1207,25 @@ static const struct sdw_master_port_ops cdns_port_ops = { .dpn_port_enable_ch = cdns_port_enable, };
+/**
- sdw_cdns_is_clock_stop: Check clock status
- @cdns: Cadence instance
- */
+bool sdw_cdns_is_clock_stop(struct sdw_cdns *cdns) +{
- u32 status;
- status = cdns_readl(cdns, CDNS_MCP_STAT) & CDNS_MCP_STAT_CLK_STOP;
- if (status) {
dev_dbg(cdns->dev, "Clock is stopped\n");return true;- }
This can be further optimized to:
return !!(cdns_readl(cdns, CDNS_MCP_STAT) & CDNS_MCP_STAT_CLK_STOP);
+/**
- sdw_cdns_is_clock_stop: Check clock status
- @cdns: Cadence instance
- */
+bool sdw_cdns_is_clock_stop(struct sdw_cdns *cdns) +{
- u32 status;
- status = cdns_readl(cdns, CDNS_MCP_STAT) & CDNS_MCP_STAT_CLK_STOP;
- if (status) {
dev_dbg(cdns->dev, "Clock is stopped\n");return true;- }
This can be further optimized to:
return !!(cdns_readl(cdns, CDNS_MCP_STAT) & CDNS_MCP_STAT_CLK_STOP);
The logs are very useful for debug.
On 13-03-20, 11:31, Pierre-Louis Bossart wrote:
+/**
- sdw_cdns_is_clock_stop: Check clock status
- @cdns: Cadence instance
- */
+bool sdw_cdns_is_clock_stop(struct sdw_cdns *cdns) +{
- u32 status;
- status = cdns_readl(cdns, CDNS_MCP_STAT) & CDNS_MCP_STAT_CLK_STOP;
- if (status) {
dev_dbg(cdns->dev, "Clock is stopped\n");return true;- }
This can be further optimized to:
return !!(cdns_readl(cdns, CDNS_MCP_STAT) & CDNS_MCP_STAT_CLK_STOP);The logs are very useful for debug.
You have this log also in caller function.
config_update() may time out or cannot be use in ClockStopMode
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 71cba2585151..4089c271305a 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -239,6 +239,11 @@ static int cdns_config_update(struct sdw_cdns *cdns) { int ret;
+ if (sdw_cdns_is_clock_stop(cdns)) { + dev_err(cdns->dev, "Cannot program MCP_CONFIG_UPDATE in ClockStopMode\n"); + return -EINVAL; + } + ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE, CDNS_MCP_CONFIG_UPDATE_BIT); if (ret < 0)
On 11-03-20, 13:41, Pierre-Louis Bossart wrote:
config_update() may time out or cannot be use in ClockStopMode
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/cadence_master.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 71cba2585151..4089c271305a 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -239,6 +239,11 @@ static int cdns_config_update(struct sdw_cdns *cdns) { int ret;
- if (sdw_cdns_is_clock_stop(cdns)) {
dev_err(cdns->dev, "Cannot program MCP_CONFIG_UPDATE in ClockStopMode\n");
This looks fine but duplicates the log, so maybe you can remove from here or preceding patch... Or use single line as I suggested in that patch and keep this as is.
On 3/13/20 7:08 AM, Vinod Koul wrote:
On 11-03-20, 13:41, Pierre-Louis Bossart wrote:
config_update() may time out or cannot be use in ClockStopMode
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/cadence_master.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 71cba2585151..4089c271305a 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -239,6 +239,11 @@ static int cdns_config_update(struct sdw_cdns *cdns) { int ret;
- if (sdw_cdns_is_clock_stop(cdns)) {
dev_err(cdns->dev, "Cannot program MCP_CONFIG_UPDATE in ClockStopMode\n");This looks fine but duplicates the log, so maybe you can remove from here or preceding patch... Or use single line as I suggested in that patch and keep this as is.
It doesn't duplicate it. In Patch1 it's a dev_dbg log that's useful for integration, and that function is used in other places (see the soundwire:intel series).
In this patch this is a clear error leading to a master hang.
From: Rander Wang rander.wang@intel.com
Add support for clock stop and restart, with two configuration parameters:
1) when entering the ClockStop mode, Slave-initiated wakes can be prevented.
2) When exiting the ClockStop mode, the caller can request a Bus Reset (either if all Slaves were configured in ClockStopMode1 or the Master IP lost context and enumeration is required)
The code handles the case where no Slaves are present by configuring the IP to treat COMMAND_IGNORED as success.
The exit_reset part can be dealt with in the caller, along with the required syncArm/syncGo sequence in multi-link mode.
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 | 161 ++++++++++++++++++++++++++++- drivers/soundwire/cadence_master.h | 2 + 2 files changed, 162 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 4089c271305a..1ec537b2789f 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -225,12 +225,30 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value) return 0;
timeout--; - udelay(50); + usleep_range(50, 100); } while (timeout != 0);
return -EAGAIN; }
+static int cdns_set_wait(struct sdw_cdns *cdns, int offset, u32 mask, u32 value) +{ + int timeout = 10; + u32 reg_read; + + /* Wait for bit to be set */ + do { + reg_read = readl(cdns->registers + offset); + if ((reg_read & mask) == value) + return 0; + + timeout--; + usleep_range(50, 100); + } while (timeout != 0); + + return -ETIMEDOUT; +} + /* * all changes to the MCP_CONFIG, MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL * need to be confirmed with a write to MCP_CONFIG_UPDATE @@ -1231,6 +1249,147 @@ bool sdw_cdns_is_clock_stop(struct sdw_cdns *cdns) } EXPORT_SYMBOL(sdw_cdns_is_clock_stop);
+/** + * sdw_cdns_clock_stop: Cadence clock stop configuration routine + * + * @cdns: Cadence instance + * @block_wake: prevent wakes if required by the platform + */ +int sdw_cdns_clock_stop(struct sdw_cdns *cdns, bool block_wake) +{ + bool slave_present = false; + struct sdw_slave *slave; + u32 status; + int ret; + + /* Check suspend status */ + status = cdns_readl(cdns, CDNS_MCP_STAT); + if (status & CDNS_MCP_STAT_CLK_STOP) { + dev_dbg(cdns->dev, "Clock is already stopped\n"); + return 1; + } + + /* + * For specific platforms, it is required to be able to put + * master into a state in which it ignores wake-up trials + * in clock stop state + */ + if (block_wake) + cdns_updatel(cdns, CDNS_MCP_CONTROL, + CDNS_MCP_CONTROL_BLOCK_WAKEUP, + CDNS_MCP_CONTROL_BLOCK_WAKEUP); + + list_for_each_entry(slave, &cdns->bus.slaves, node) { + if (slave->status == SDW_SLAVE_ATTACHED || + slave->status == SDW_SLAVE_ALERT) { + slave_present = true; + break; + } + } + + /* + * This CMD_ACCEPT should be used when there are no devices + * attached on the link when entering clock stop mode. If this is + * not set and there is a broadcast write then the command ignored + * will be treated as a failure + */ + if (!slave_present) + cdns_updatel(cdns, CDNS_MCP_CONTROL, + CDNS_MCP_CONTROL_CMD_ACCEPT, + CDNS_MCP_CONTROL_CMD_ACCEPT); + else + cdns_updatel(cdns, CDNS_MCP_CONTROL, + CDNS_MCP_CONTROL_CMD_ACCEPT, 0); + + /* commit changes */ + ret = cdns_config_update(cdns); + if (ret < 0) { + dev_err(cdns->dev, "%s: config_update failed\n", __func__); + return ret; + } + + /* Prepare slaves for clock stop */ + ret = sdw_bus_prep_clk_stop(&cdns->bus); + if (ret < 0) { + dev_err(cdns->dev, "prepare clock stop failed %d", ret); + return ret; + } + + /* + * Enter clock stop mode and only report errors if there are + * Slave devices present (ALERT or ATTACHED) + */ + ret = sdw_bus_clk_stop(&cdns->bus); + if (ret < 0 && slave_present && ret != -ENODATA) { + dev_err(cdns->dev, "bus clock stop failed %d", ret); + return ret; + } + + ret = cdns_set_wait(cdns, CDNS_MCP_STAT, + CDNS_MCP_STAT_CLK_STOP, + CDNS_MCP_STAT_CLK_STOP); + if (ret < 0) + dev_err(cdns->dev, "Clock stop failed %d\n", ret); + + return ret; +} +EXPORT_SYMBOL(sdw_cdns_clock_stop); + +/** + * sdw_cdns_clock_restart: Cadence PM clock restart configuration routine + * + * @cdns: Cadence instance + * @bus_reset: context may be lost while in low power modes and the bus + * may require a Severe Reset and re-enumeration after a wake. + */ +int sdw_cdns_clock_restart(struct sdw_cdns *cdns, bool bus_reset) +{ + int ret; + + ret = cdns_clear_bit(cdns, CDNS_MCP_CONTROL, + CDNS_MCP_CONTROL_CLK_STOP_CLR); + if (ret < 0) { + dev_err(cdns->dev, "Couldn't exit from clock stop\n"); + return ret; + } + + ret = cdns_set_wait(cdns, CDNS_MCP_STAT, CDNS_MCP_STAT_CLK_STOP, 0); + if (ret < 0) { + dev_err(cdns->dev, "clock stop exit failed %d\n", ret); + return ret; + } + + cdns_updatel(cdns, CDNS_MCP_CONTROL, + CDNS_MCP_CONTROL_BLOCK_WAKEUP, 0); + + /* + * clear CMD_ACCEPT so that the command ignored + * will be treated as a failure during a broadcast write + */ + cdns_updatel(cdns, CDNS_MCP_CONTROL, CDNS_MCP_CONTROL_CMD_ACCEPT, 0); + + if (!bus_reset) { + + /* enable bus operations with clock and data */ + cdns_updatel(cdns, CDNS_MCP_CONFIG, + CDNS_MCP_CONFIG_OP, + CDNS_MCP_CONFIG_OP_NORMAL); + + ret = cdns_config_update(cdns); + if (ret < 0) { + dev_err(cdns->dev, "%s: config_update failed\n", __func__); + return ret; + } + + ret = sdw_bus_exit_clk_stop(&cdns->bus); + if (ret < 0) + dev_err(cdns->dev, "bus failed to exit clock stop %d\n", ret); + } + + return ret; +} +EXPORT_SYMBOL(sdw_cdns_clock_restart); + /** * sdw_cdns_probe() - Cadence probe routine * @cdns: Cadence instance diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 691faa386889..e8fa5c7e09f4 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -145,6 +145,8 @@ int sdw_cdns_exit_reset(struct sdw_cdns *cdns); int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
bool sdw_cdns_is_clock_stop(struct sdw_cdns *cdns); +int sdw_cdns_clock_stop(struct sdw_cdns *cdns, bool block_wake); +int sdw_cdns_clock_restart(struct sdw_cdns *cdns, bool bus_reset);
#ifdef CONFIG_DEBUG_FS void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
On 11-03-20, 13:41, Pierre-Louis Bossart wrote:
@@ -225,12 +225,30 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value) return 0;
timeout--;
udelay(50);
usleep_range(50, 100);
this seems okay change, but unrelated to this patch
+int sdw_cdns_clock_stop(struct sdw_cdns *cdns, bool block_wake) +{
- bool slave_present = false;
- struct sdw_slave *slave;
- u32 status;
- int ret;
- /* Check suspend status */
- status = cdns_readl(cdns, CDNS_MCP_STAT);
- if (status & CDNS_MCP_STAT_CLK_STOP) {
dev_dbg(cdns->dev, "Clock is already stopped\n");return 1;
return of 1..? Does that indicate success/fail..?
On 3/13/20 7:21 AM, Vinod Koul wrote:
On 11-03-20, 13:41, Pierre-Louis Bossart wrote:
@@ -225,12 +225,30 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value) return 0;
timeout--;
udelay(50);
usleep_range(50, 100);this seems okay change, but unrelated to this patch
ok. It doesn't really matter anyway, this function is removed in Patch 8
+int sdw_cdns_clock_stop(struct sdw_cdns *cdns, bool block_wake) +{
- bool slave_present = false;
- struct sdw_slave *slave;
- u32 status;
- int ret;
- /* Check suspend status */
- status = cdns_readl(cdns, CDNS_MCP_STAT);
- if (status & CDNS_MCP_STAT_CLK_STOP) {
dev_dbg(cdns->dev, "Clock is already stopped\n");return 1;return of 1..? Does that indicate success/fail..?
success. I guess it could be moved as 0.
ret = sdw_cdns_clock_stop(cdns, true); if (ret < 0) { dev_err(dev, "cannot enable clock stop on suspend\n"); return ret; }
On 13-03-20, 12:07, Pierre-Louis Bossart wrote:
On 3/13/20 7:21 AM, Vinod Koul wrote:
On 11-03-20, 13:41, Pierre-Louis Bossart wrote:
@@ -225,12 +225,30 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value) return 0; timeout--;
udelay(50);
usleep_range(50, 100);this seems okay change, but unrelated to this patch
ok. It doesn't really matter anyway, this function is removed in Patch 8
Ok pls drop from here.
+int sdw_cdns_clock_stop(struct sdw_cdns *cdns, bool block_wake) +{
- bool slave_present = false;
- struct sdw_slave *slave;
- u32 status;
- int ret;
- /* Check suspend status */
- status = cdns_readl(cdns, CDNS_MCP_STAT);
- if (status & CDNS_MCP_STAT_CLK_STOP) {
dev_dbg(cdns->dev, "Clock is already stopped\n");return 1;return of 1..? Does that indicate success/fail..?
success. I guess it could be moved as 0.
That would be better. We use 0 for success everywhere and -ve error codes.
From: Rander Wang rander.wang@intel.com
After system resumes from S3, io timeout occurs when setting one unused master on Comet Lake platform. In this case, the master is reset to default state, and FIFOLEVEL is reset to default value, but msg_count used for tracing FIFOLEVEL is still with old value, so FIFOLEVEL will not be set if a new msg FIFO usage is equal to the old msg_count.
This patch updates msg_count to default value of FIFOLEVEL when resetting master.
Tested on Comet Lake platform.
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 | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 1ec537b2789f..02f18ce6b7e7 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1068,6 +1068,9 @@ int sdw_cdns_init(struct sdw_cdns *cdns) cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL); cdns_writel(cdns, CDNS_MCP_SSP_CTRL1, CDNS_DEFAULT_SSP_INTERVAL);
+ /* reset msg_count to default value of FIFOLEVEL */ + cdns->msg_count = cdns_readl(cdns, CDNS_MCP_FIFOLEVEL); + /* flush command FIFOs */ cdns_updatel(cdns, CDNS_MCP_CONTROL, CDNS_MCP_CONTROL_CMD_RST, CDNS_MCP_CONTROL_CMD_RST);
Intel QA reported a very rare case, possibly hardware-dependent, where a Slave can become UNATTACHED during a clock stop sequence, which leads to timeouts and failed suspend sequences.
This patch suppresses the handling of all Slave events while this transition happens. The two cases that matter are:
a) alerts: if the Slave wants to signal an alert condition, it can do so using the in-band wake, so there's almost no impact with this patch.
b) sync loss or imp-def reset: in those cases, bringing back the Slave to functional state requires a complete re-enumeration. It's better to just ignore this case and restart cleanly, rather than attempt a 'clean' suspend.
Validation results show the timeouts no longer visible with this patch.
GitHub issue: https://github.com/thesofproject/linux/issues/1678 Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 02f18ce6b7e7..a4ba57f44c1f 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -865,6 +865,24 @@ int sdw_cdns_exit_reset(struct sdw_cdns *cdns) } EXPORT_SYMBOL(sdw_cdns_exit_reset);
+/** + * sdw_cdns_enable_slave_interrupt() - Enable SDW slave interrupts + * @cdns: Cadence instance + * @state: boolean for true/false + */ +static void cdns_enable_slave_interrupts(struct sdw_cdns *cdns, bool state) +{ + u32 mask; + + mask = cdns_readl(cdns, CDNS_MCP_INTMASK); + if (state) + mask |= CDNS_MCP_INT_SLAVE_MASK; + else + mask &= ~CDNS_MCP_INT_SLAVE_MASK; + + cdns_writel(cdns, CDNS_MCP_INTMASK, mask); +} + /** * sdw_cdns_enable_interrupt() - Enable SDW interrupts * @cdns: Cadence instance @@ -1272,6 +1290,13 @@ int sdw_cdns_clock_stop(struct sdw_cdns *cdns, bool block_wake) return 1; }
+ /* + * Before entering clock stop we mask the Slave + * interrupts. This helps avoid having to deal with e.g. a + * Slave becoming UNATTACHED while the clock is being stopped + */ + cdns_enable_slave_interrupts(cdns, false); + /* * For specific platforms, it is required to be able to put * master into a state in which it ignores wake-up trials @@ -1349,6 +1374,9 @@ int sdw_cdns_clock_restart(struct sdw_cdns *cdns, bool bus_reset) { int ret;
+ /* unmask Slave interrupts that were masked when stopping the clock */ + cdns_enable_slave_interrupts(cdns, true); + ret = cdns_clear_bit(cdns, CDNS_MCP_CONTROL, CDNS_MCP_CONTROL_CLK_STOP_CLR); if (ret < 0) {
On 11-03-20, 13:41, Pierre-Louis Bossart wrote:
Intel QA reported a very rare case, possibly hardware-dependent, where a Slave can become UNATTACHED during a clock stop sequence, which leads to timeouts and failed suspend sequences.
This patch suppresses the handling of all Slave events while this transition happens. The two cases that matter are:
a) alerts: if the Slave wants to signal an alert condition, it can do so using the in-band wake, so there's almost no impact with this patch.
b) sync loss or imp-def reset: in those cases, bringing back the Slave to functional state requires a complete re-enumeration. It's better to just ignore this case and restart cleanly, rather than attempt a 'clean' suspend.
Validation results show the timeouts no longer visible with this patch.
GitHub issue: https://github.com/thesofproject/linux/issues/1678 Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/cadence_master.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 02f18ce6b7e7..a4ba57f44c1f 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -865,6 +865,24 @@ int sdw_cdns_exit_reset(struct sdw_cdns *cdns) } EXPORT_SYMBOL(sdw_cdns_exit_reset);
+/**
- sdw_cdns_enable_slave_interrupt() - Enable SDW slave interrupts
- @cdns: Cadence instance
- @state: boolean for true/false
- */
+static void cdns_enable_slave_interrupts(struct sdw_cdns *cdns, bool state)
Do you want to rename this as cdns_configure_slave_interrupts, with argument as enable/disable... ?
+/**
- sdw_cdns_enable_slave_interrupt() - Enable SDW slave interrupts
- @cdns: Cadence instance
- @state: boolean for true/false
- */
+static void cdns_enable_slave_interrupts(struct sdw_cdns *cdns, bool state)
Do you want to rename this as cdns_configure_slave_interrupts, with argument as enable/disable... ?
this follows the convention we already have with:
int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
but it just deals with slave interrupts only.
Use a single loop to wait for hardware to set/clear fields.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index a4ba57f44c1f..22ff66d38a4a 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -211,26 +211,6 @@ static inline void cdns_updatel(struct sdw_cdns *cdns, cdns_writel(cdns, offset, tmp); }
-static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value) -{ - int timeout = 10; - u32 reg_read; - - writel(value, cdns->registers + offset); - - /* Wait for bit to be self cleared */ - do { - reg_read = readl(cdns->registers + offset); - if ((reg_read & value) == 0) - return 0; - - timeout--; - usleep_range(50, 100); - } while (timeout != 0); - - return -EAGAIN; -} - static int cdns_set_wait(struct sdw_cdns *cdns, int offset, u32 mask, u32 value) { int timeout = 10; @@ -249,6 +229,14 @@ static int cdns_set_wait(struct sdw_cdns *cdns, int offset, u32 mask, u32 value) return -ETIMEDOUT; }
+static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value) +{ + writel(value, cdns->registers + offset); + + /* Wait for bit to be self cleared */ + return cdns_set_wait(cdns, offset, value, 0); +} + /* * all changes to the MCP_CONFIG, MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL * need to be confirmed with a write to MCP_CONFIG_UPDATE
This helps isolate code and align with recommended programming flows
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 22ff66d38a4a..91fe89a246ee 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1043,11 +1043,7 @@ static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols) return val; }
-/** - * sdw_cdns_init() - Cadence initialization - * @cdns: Cadence instance - */ -int sdw_cdns_init(struct sdw_cdns *cdns) +static void cdns_init_clock_ctrl(struct sdw_cdns *cdns) { struct sdw_bus *bus = &cdns->bus; struct sdw_master_prop *prop = &bus->prop; @@ -1073,6 +1069,18 @@ int sdw_cdns_init(struct sdw_cdns *cdns) /* Set SSP interval to default value */ cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL); cdns_writel(cdns, CDNS_MCP_SSP_CTRL1, CDNS_DEFAULT_SSP_INTERVAL); +} + +/** + * sdw_cdns_init() - Cadence initialization + * @cdns: Cadence instance + */ +int sdw_cdns_init(struct sdw_cdns *cdns) +{ + u32 val; + int ret; + + cdns_init_clock_ctrl(cdns);
/* reset msg_count to default value of FIFOLEVEL */ cdns->msg_count = cdns_readl(cdns, CDNS_MCP_FIFOLEVEL);
In multi-master mode, the IP will only accept SSP intervals with integer relationships between the frame rate and the gsync frequency.
E.g for a 48kHz frame rate and 4 kHz gsync signal, the SSP interval can only be 1, 2, 3, 4, 6, 12.
To simplify we only allow one SSP per gsync interval.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 7 ++++--- drivers/soundwire/cadence_master.h | 3 +++ 2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 91fe89a246ee..4a7e7329ff0d 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -183,7 +183,6 @@ MODULE_PARM_DESC(cdns_mcp_int_mask, "Cadence MCP IntMask"); #define CDNS_PDI_CONFIG_PORT GENMASK(4, 0)
/* Driver defaults */ -#define CDNS_DEFAULT_SSP_INTERVAL 0x18 #define CDNS_TX_TIMEOUT 2000
#define CDNS_SCP_RX_FIFOLEVEL 0x2 @@ -1048,6 +1047,7 @@ static void cdns_init_clock_ctrl(struct sdw_cdns *cdns) struct sdw_bus *bus = &cdns->bus; struct sdw_master_prop *prop = &bus->prop; u32 val; + u32 ssp_interval; int divider;
/* Set clock divider */ @@ -1067,8 +1067,9 @@ static void cdns_init_clock_ctrl(struct sdw_cdns *cdns) cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, val);
/* Set SSP interval to default value */ - cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL); - cdns_writel(cdns, CDNS_MCP_SSP_CTRL1, CDNS_DEFAULT_SSP_INTERVAL); + ssp_interval = prop->default_frame_rate / SDW_CADENCE_GSYNC_HZ; + cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, ssp_interval); + cdns_writel(cdns, CDNS_MCP_SSP_CTRL1, ssp_interval); }
/** diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index e8fa5c7e09f4..b410656f8194 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -5,6 +5,9 @@ #ifndef __SDW_CADENCE_H #define __SDW_CADENCE_H
+#define SDW_CADENCE_GSYNC_KHZ 4 /* 4 kHz */ +#define SDW_CADENCE_GSYNC_HZ (SDW_CADENCE_GSYNC_KHZ * 1000) + /** * struct sdw_cdns_pdi: PDI (Physical Data Interface) instance *
Follow hardware programming flows and add placeholder comment for multi-master mode.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 4a7e7329ff0d..dd8c9b051037 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1097,21 +1097,23 @@ int sdw_cdns_init(struct sdw_cdns *cdns) /* Configure mcp config */ val = cdns_readl(cdns, CDNS_MCP_CONFIG);
- /* Set Max cmd retry to 15 */ - val |= CDNS_MCP_CONFIG_MCMD_RETRY; - - /* Set frame delay between PREQ and ping frame to 15 frames */ - val |= 0xF << SDW_REG_SHIFT(CDNS_MCP_CONFIG_MPREQ_DELAY); - - /* Disable auto bus release */ - val &= ~CDNS_MCP_CONFIG_BUS_REL; - - /* Disable sniffer mode */ - val &= ~CDNS_MCP_CONFIG_SNIFFER; - /* Set cmd mode for Tx and Rx cmds */ val &= ~CDNS_MCP_CONFIG_CMD;
+ /* Disable sniffer mode */ + val &= ~CDNS_MCP_CONFIG_SNIFFER; + + /* Disable auto bus release */ + val &= ~CDNS_MCP_CONFIG_BUS_REL; + + /* Multi-master support to be added here */ + + /* Set frame delay between PREQ and ping frame to 15 frames */ + val |= 0xF << SDW_REG_SHIFT(CDNS_MCP_CONFIG_MPREQ_DELAY); + + /* Set Max cmd retry to 15 */ + val |= CDNS_MCP_CONFIG_MCMD_RETRY; + cdns_writel(cdns, CDNS_MCP_CONFIG, val);
/* commit changes */
Follow recommended programming sequences, this needs to be enabled before the reset sequence.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index dd8c9b051037..7757c51b200d 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -842,11 +842,6 @@ int sdw_cdns_exit_reset(struct sdw_cdns *cdns) CDNS_MCP_CONTROL_HW_RST, CDNS_MCP_CONTROL_HW_RST);
- /* enable bus operations with clock and data */ - cdns_updatel(cdns, CDNS_MCP_CONFIG, - CDNS_MCP_CONFIG_OP, - CDNS_MCP_CONFIG_OP_NORMAL); - /* commit changes */ return cdns_config_update(cdns); } @@ -1097,6 +1092,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns) /* Configure mcp config */ val = cdns_readl(cdns, CDNS_MCP_CONFIG);
+ /* enable bus operations with clock and data */ + val &= ~CDNS_MCP_CONFIG_OP; + val |= CDNS_MCP_CONFIG_OP_NORMAL; + /* Set cmd mode for Tx and Rx cmds */ val &= ~CDNS_MCP_CONFIG_CMD;
The hardware default is 0x1F, and the existing code does an OR with 0xF. This is a no-op, remove.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 7757c51b200d..a428c7935589 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1107,8 +1107,7 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
/* Multi-master support to be added here */
- /* Set frame delay between PREQ and ping frame to 15 frames */ - val |= 0xF << SDW_REG_SHIFT(CDNS_MCP_CONFIG_MPREQ_DELAY); + /* leave frame delay to hardware default of 0x1F */
/* Set Max cmd retry to 15 */ val |= CDNS_MCP_CONFIG_MCMD_RETRY;
This is a good idea on paper, but it's not recommended at all when operating in multi-master mode. It's also not recommended when doing bank switches, since the retransmission would happen at the next SSP, and the command protocol is stuck in the mean time.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index a428c7935589..acafa6222171 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1109,8 +1109,7 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
/* leave frame delay to hardware default of 0x1F */
- /* Set Max cmd retry to 15 */ - val |= CDNS_MCP_CONFIG_MCMD_RETRY; + /* leave command retry to hardware default of 0 */
cdns_writel(cdns, CDNS_MCP_CONFIG, val);
Follow recommended flows, the BUS_RESET must be programmed before the UPDATE_CONFIG.
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 acafa6222171..112f4ebcf1be 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1074,7 +1074,6 @@ static void cdns_init_clock_ctrl(struct sdw_cdns *cdns) int sdw_cdns_init(struct sdw_cdns *cdns) { u32 val; - int ret;
cdns_init_clock_ctrl(cdns);
@@ -1113,8 +1112,8 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
cdns_writel(cdns, CDNS_MCP_CONFIG, val);
- /* commit changes */ - return cdns_config_update(cdns); + /* changes will be committed later */ + return 0; } EXPORT_SYMBOL(sdw_cdns_init);
Enable multi-link (aka multi-master configuration). In this configuration, updates and commands with the 'ssp_sync' tag will be deferred and controlled by the gsync hardware signal.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 112f4ebcf1be..5b07ce195100 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -843,7 +843,13 @@ int sdw_cdns_exit_reset(struct sdw_cdns *cdns) CDNS_MCP_CONTROL_HW_RST);
/* commit changes */ - return cdns_config_update(cdns); + cdns_updatel(cdns, CDNS_MCP_CONFIG_UPDATE, + CDNS_MCP_CONFIG_UPDATE_BIT, + CDNS_MCP_CONFIG_UPDATE_BIT); + + /* don't wait here */ + return 0; + } EXPORT_SYMBOL(sdw_cdns_exit_reset);
@@ -1104,7 +1110,9 @@ int sdw_cdns_init(struct sdw_cdns *cdns) /* Disable auto bus release */ val &= ~CDNS_MCP_CONFIG_BUS_REL;
- /* Multi-master support to be added here */ + if (cdns->bus.multi_link) + /* Set Multi-master mode to take gsync into account */ + val |= CDNS_MCP_CONFIG_MMASTER;
/* leave frame delay to hardware default of 0x1F */
participants (2)
-
Pierre-Louis Bossart -
Vinod Koul