On 9/4/19 2:11 AM, Vinod Koul wrote:
On 13-08-19, 16:32, Pierre-Louis Bossart wrote:
Multiple changes squashed in single patch to avoid tick-tock effect.
- Per the hardware documentation, all changes to MCP_CONFIG,
MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL need to be validated with a self-clearing write to MCP_CONFIG_UPDATE. Add a helper and do the update when the CONFIG is changed.
Move interrupt enable after interrupt handler registration
Add a new helper to start the hardware bus reset with maximum duration
to make sure the Slave(s) correctly detect the reset pattern and to ensure electrical conflicts can be resolved.
- flush command FIFOs
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/cadence_master.c | 84 +++++++++++++++++++++--------- drivers/soundwire/cadence_master.h | 1 + drivers/soundwire/intel.c | 14 ++++- 3 files changed, 73 insertions(+), 26 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 502ed4ec8f07..046622e4b264 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -231,6 +231,22 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int offset, u32 value) return -EAGAIN; }
+/*
- 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) +{
- int ret;
- ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE,
CDNS_MCP_CONFIG_UPDATE_BIT);- if (ret < 0)
dev_err(cdns->dev, "Config update timedout\n");- return ret;
+}
- /*
*/
- debugfs
@@ -752,7 +768,42 @@ EXPORT_SYMBOL(sdw_cdns_thread); /*
- init routines
*/ -static int _cdns_enable_interrupt(struct sdw_cdns *cdns)
+/**
- sdw_cdns_exit_reset() - Program reset parameters and start bus operations
- @cdns: Cadence instance
- */
+int sdw_cdns_exit_reset(struct sdw_cdns *cdns) +{
- int ret;
- /* program maximum length reset to be safe */
- cdns_updatel(cdns, CDNS_MCP_CONTROL,
CDNS_MCP_CONTROL_RST_DELAY,CDNS_MCP_CONTROL_RST_DELAY);- /* use hardware generated reset */
- cdns_updatel(cdns, CDNS_MCP_CONTROL,
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 */
- ret = cdns_update_config(cdns);
- return ret;
return cdns_update_config() ?
ok
+} +EXPORT_SYMBOL(sdw_cdns_exit_reset);
+/**
- sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config
- @cdns: Cadence instance
- */
+int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns) { u32 mask;
@@ -784,24 +835,8 @@ static int _cdns_enable_interrupt(struct sdw_cdns *cdns)
cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
- return 0;
-}
-/**
- sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config
- @cdns: Cadence instance
- */
-int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns) -{
- int ret;
- _cdns_enable_interrupt(cdns);
- ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE,
CDNS_MCP_CONFIG_UPDATE_BIT);- if (ret < 0)
dev_err(cdns->dev, "Config update timedout\n");- return ret;
- /* commit changes */
- return cdns_update_config(cdns); } EXPORT_SYMBOL(sdw_cdns_enable_interrupt);
@@ -975,6 +1010,10 @@ 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);
- /* flush command FIFOs */
- cdns_updatel(cdns, CDNS_MCP_CONTROL, CDNS_MCP_CONTROL_CMD_RST,
CDNS_MCP_CONTROL_CMD_RST);- /* Set cmd accept mode */ cdns_updatel(cdns, CDNS_MCP_CONTROL, CDNS_MCP_CONTROL_CMD_ACCEPT, CDNS_MCP_CONTROL_CMD_ACCEPT);
@@ -997,13 +1036,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns) /* Set cmd mode for Tx and Rx cmds */ val &= ~CDNS_MCP_CONFIG_CMD;
/* Set operation to normal */
val &= ~CDNS_MCP_CONFIG_OP;
val |= CDNS_MCP_CONFIG_OP_NORMAL;
cdns_writel(cdns, CDNS_MCP_CONFIG, val);
return 0;
- /* commit changes */
- return cdns_update_config(cdns); } EXPORT_SYMBOL(sdw_cdns_init);
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 0b72b7094735..1a67728c5000 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -161,6 +161,7 @@ irqreturn_t sdw_cdns_thread(int irq, void *dev_id); 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); int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns);
#ifdef CONFIG_DEBUG_FS diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 13c54eac0cc3..5f14c6acce80 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c
As I have said in the past it doesnt help having a patch touching two components. The patch is titled cadence!
no, it's called 'fix startup sequence for Intel/Cadence' and it's intentional to modify the two parts in a single patch. I tried to split this but it's very difficult due to the dependencies. If you look at all the initial comments from early July people were lost when I did these changes in steps. I try to keep patches local and self-contained, but in this case it's nearly impossible or makes the review very hard for no good reason.
@@ -1043,8 +1043,6 @@ static int intel_probe(struct platform_device *pdev) if (ret) goto err_init;
- ret = sdw_cdns_enable_interrupt(&sdw->cdns);
- /* Read the PDI config and initialize cadence PDI */ intel_pdi_init(sdw, &config); ret = sdw_cdns_pdi_init(&sdw->cdns, config);
@@ -1062,6 +1060,18 @@ static int intel_probe(struct platform_device *pdev) goto err_init; }
- ret = sdw_cdns_enable_interrupt(&sdw->cdns);
- if (ret < 0) {
dev_err(sdw->cdns.dev, "cannot enable interrupts\n");goto err_init;- }
- ret = sdw_cdns_exit_reset(&sdw->cdns);
- if (ret < 0) {
dev_err(sdw->cdns.dev, "unable to exit bus reset sequence\n");goto err_init;Don't you want to disable interrupts at least... before you return error? err_init does bus cleanup and not controller one
yes good point, let me look at this.