[PATCH v2 0/5] soundwire: intel/cadence: better initialization

[PATCH v2 0/5] soundwire: intel/cadence: better initialization

[Pierre-Louis Bossart]

V2 of the original series 'soundwire: inits and PM additions for 5.4',
with PM additions removed since more tests on hardware are required.

Changes since v1: addressed feedback from Vinod Koul
clarified init changes impact Intel and Cadence sides
remove unnecessary intermediate variable
disable interrupts when exit_reset fails, updated error handling
returned -EINVAL on debugfs invalid parameter

Pierre-Louis Bossart (5):
  soundwire: intel/cadence: fix startup sequence
  soundwire: cadence_master: add hw_reset capability in debugfs
  soundwire: intel: add helper for initialization
  soundwire: intel/cadence: add flag for interrupt enable
  soundwire: cadence_master: make clock stop exit configurable on init

 drivers/soundwire/cadence_master.c | 131 +++++++++++++++++++++--------
 drivers/soundwire/cadence_master.h |   5 +-
 drivers/soundwire/intel.c          |  38 ++++++---
 3 files changed, 126 insertions(+), 48 deletions(-)

-- 
2.20.1

[PATCH v2 1/5] soundwire: intel/cadence: fix startup sequence

[Pierre-Louis Bossart]

Multiple changes squashed in single patch to avoid tick-tock effect
and avoid breaking compilation/bisect

1. 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.

2. Move interrupt enable after interrupt handler registration

3. 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.

4. flush command FIFOs

Better error handling will be provided after interrupt disable is
provided in follow-up patches.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/cadence_master.c | 80 +++++++++++++++++++++---------
 drivers/soundwire/cadence_master.h |  1 +
 drivers/soundwire/intel.c          | 14 +++++-
 3 files changed, 69 insertions(+), 26 deletions(-)

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index 502ed4ec8f07..e3d06330d125 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,38 @@ 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)
+{
+	/* 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 */
+	return cdns_update_config(cdns);
+}
+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 +831,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 +1006,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 +1032,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 bcc5077b2814..57e599919479 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1044,8 +1044,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);
@@ -1063,6 +1061,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;
+	}
+
 	/* Register DAIs */
 	ret = intel_register_dai(sdw);
 	if (ret) {
-- 
2.20.1

[PATCH v2 2/5] soundwire: cadence_master: add hw_reset capability in debugfs

[Pierre-Louis Bossart]

Provide debugfs capability to kick link and devices into hard-reset
(as defined by MIPI). This capability is really useful when some
devices are no longer responsive and/or to check the software handling
of resynchronization.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/cadence_master.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index e3d06330d125..5f900cf2acb9 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -340,6 +340,23 @@ static int cdns_reg_show(struct seq_file *s, void *data)
 }
 DEFINE_SHOW_ATTRIBUTE(cdns_reg);
 
+static int cdns_hw_reset(void *data, u64 value)
+{
+	struct sdw_cdns *cdns = data;
+	int ret;
+
+	if (value != 1)
+		return -EINVAL;
+
+	ret = sdw_cdns_exit_reset(cdns);
+
+	dev_dbg(cdns->dev, "link hw_reset done: %d\n", ret);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(cdns_hw_reset_fops, NULL, cdns_hw_reset, "%llu\n");
+
 /**
  * sdw_cdns_debugfs_init() - Cadence debugfs init
  * @cdns: Cadence instance
@@ -348,6 +365,9 @@ DEFINE_SHOW_ATTRIBUTE(cdns_reg);
 void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
 {
 	debugfs_create_file("cdns-registers", 0400, root, cdns, &cdns_reg_fops);
+
+	debugfs_create_file("cdns-hw-reset", 0200, root, cdns,
+			    &cdns_hw_reset_fops);
 }
 EXPORT_SYMBOL_GPL(sdw_cdns_debugfs_init);
 
-- 
2.20.1

Re: [PATCH v2 2/5] soundwire: cadence_master: add hw_reset capability in debugfs

[Vinod Koul]

On 16-09-19, 14:09, Pierre-Louis Bossart wrote:
> Provide debugfs capability to kick link and devices into hard-reset
> (as defined by MIPI). This capability is really useful when some
> devices are no longer responsive and/or to check the software handling
> of resynchronization.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/cadence_master.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index e3d06330d125..5f900cf2acb9 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -340,6 +340,23 @@ static int cdns_reg_show(struct seq_file *s, void *data)
>  }
>  DEFINE_SHOW_ATTRIBUTE(cdns_reg);
>  
> +static int cdns_hw_reset(void *data, u64 value)
> +{
> +	struct sdw_cdns *cdns = data;
> +	int ret;
> +
> +	if (value != 1)
> +		return -EINVAL;
> +
> +	ret = sdw_cdns_exit_reset(cdns);

So we are performing reset of the device behind the kernel, so I think
it makes sense to mark the kernel as tainted.

> +
> +	dev_dbg(cdns->dev, "link hw_reset done: %d\n", ret);
> +
> +	return ret;

We may want to get rid of the debug and do:
        return sdw_cdns_exit_reset();


> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(cdns_hw_reset_fops, NULL, cdns_hw_reset, "%llu\n");
> +
>  /**
>   * sdw_cdns_debugfs_init() - Cadence debugfs init
>   * @cdns: Cadence instance
> @@ -348,6 +365,9 @@ DEFINE_SHOW_ATTRIBUTE(cdns_reg);
>  void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
>  {
>  	debugfs_create_file("cdns-registers", 0400, root, cdns, &cdns_reg_fops);
> +
> +	debugfs_create_file("cdns-hw-reset", 0200, root, cdns,
> +			    &cdns_hw_reset_fops);
>  }
>  EXPORT_SYMBOL_GPL(sdw_cdns_debugfs_init);
>  
> -- 
> 2.20.1

-- 
~Vinod

Re: [alsa-devel] [PATCH v2 2/5] soundwire: cadence_master: add hw_reset capability in debugfs

[Pierre-Louis Bossart]

On 10/20/19 11:04 PM, Vinod Koul wrote:
> On 16-09-19, 14:09, Pierre-Louis Bossart wrote:
>> Provide debugfs capability to kick link and devices into hard-reset
>> (as defined by MIPI). This capability is really useful when some
>> devices are no longer responsive and/or to check the software handling
>> of resynchronization.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   drivers/soundwire/cadence_master.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
>> index e3d06330d125..5f900cf2acb9 100644
>> --- a/drivers/soundwire/cadence_master.c
>> +++ b/drivers/soundwire/cadence_master.c
>> @@ -340,6 +340,23 @@ static int cdns_reg_show(struct seq_file *s, void *data)
>>   }
>>   DEFINE_SHOW_ATTRIBUTE(cdns_reg);
>>   
>> +static int cdns_hw_reset(void *data, u64 value)
>> +{
>> +	struct sdw_cdns *cdns = data;
>> +	int ret;
>> +
>> +	if (value != 1)
>> +		return -EINVAL;
>> +
>> +	ret = sdw_cdns_exit_reset(cdns);
> 
> So we are performing reset of the device behind the kernel, so I think
> it makes sense to mark the kernel as tainted.

This is a bit ironic. This reset is the only way to prove that the 
enumeration is done right and that all the subsystem fully recovers, and 
yet we'd mark the kernel as 'untrustworthy' and all bug reports would be 
ignored.

I don't mind doing this but we'd have to agree on a code. The only one I 
see relevant is "taint requested by userspace application", which is not 
exactly super useful.

> 
>> +
>> +	dev_dbg(cdns->dev, "link hw_reset done: %d\n", ret);
>> +
>> +	return ret;
> 
> We may want to get rid of the debug and do:
>          return sdw_cdns_exit_reset();

this debug line is useful, it marks the start of the reset sequence and 
that's valuable information. Remove it and you've lost the ability to 
analyze the dmesg logs. It's even more useful if we mark the kernel as 
tainted as you suggested above.

> 
>> +}
>> +
>> +DEFINE_DEBUGFS_ATTRIBUTE(cdns_hw_reset_fops, NULL, cdns_hw_reset, "%llu\n");
>> +
>>   /**
>>    * sdw_cdns_debugfs_init() - Cadence debugfs init
>>    * @cdns: Cadence instance
>> @@ -348,6 +365,9 @@ DEFINE_SHOW_ATTRIBUTE(cdns_reg);
>>   void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
>>   {
>>   	debugfs_create_file("cdns-registers", 0400, root, cdns, &cdns_reg_fops);
>> +
>> +	debugfs_create_file("cdns-hw-reset", 0200, root, cdns,
>> +			    &cdns_hw_reset_fops);
>>   }
>>   EXPORT_SYMBOL_GPL(sdw_cdns_debugfs_init);
>>   
>> -- 
>> 2.20.1
> 

Re: [alsa-devel] [PATCH v2 2/5] soundwire: cadence_master: add hw_reset capability in debugfs

[Vinod Koul]

On 21-10-19, 05:20, Pierre-Louis Bossart wrote:
> On 10/20/19 11:04 PM, Vinod Koul wrote:
> > On 16-09-19, 14:09, Pierre-Louis Bossart wrote:
> > > Provide debugfs capability to kick link and devices into hard-reset
> > > (as defined by MIPI). This capability is really useful when some
> > > devices are no longer responsive and/or to check the software handling
> > > of resynchronization.
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > ---
> > >   drivers/soundwire/cadence_master.c | 20 ++++++++++++++++++++
> > >   1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> > > index e3d06330d125..5f900cf2acb9 100644
> > > --- a/drivers/soundwire/cadence_master.c
> > > +++ b/drivers/soundwire/cadence_master.c
> > > @@ -340,6 +340,23 @@ static int cdns_reg_show(struct seq_file *s, void *data)
> > >   }
> > >   DEFINE_SHOW_ATTRIBUTE(cdns_reg);
> > > +static int cdns_hw_reset(void *data, u64 value)
> > > +{
> > > +	struct sdw_cdns *cdns = data;
> > > +	int ret;
> > > +
> > > +	if (value != 1)
> > > +		return -EINVAL;
> > > +
> > > +	ret = sdw_cdns_exit_reset(cdns);
> > 
> > So we are performing reset of the device behind the kernel, so I think
> > it makes sense to mark the kernel as tainted.
> 
> This is a bit ironic. This reset is the only way to prove that the
> enumeration is done right and that all the subsystem fully recovers, and yet
> we'd mark the kernel as 'untrustworthy' and all bug reports would be
> ignored.

Nope you dont expect this would be done on a production system and for
you own testing you can choose not to ignore the reports!

> I don't mind doing this but we'd have to agree on a code. The only one I see
> relevant is "taint requested by userspace application", which is not exactly
> super useful.

But it does tell you that userspace did something so watch out!

> > > +	dev_dbg(cdns->dev, "link hw_reset done: %d\n", ret);
> > > +
> > > +	return ret;
> > 
> > We may want to get rid of the debug and do:
> >          return sdw_cdns_exit_reset();
> 
> this debug line is useful, it marks the start of the reset sequence and
> that's valuable information. Remove it and you've lost the ability to
> analyze the dmesg logs. It's even more useful if we mark the kernel as
> tainted as you suggested above.

ok

> 
> > 
> > > +}
> > > +
> > > +DEFINE_DEBUGFS_ATTRIBUTE(cdns_hw_reset_fops, NULL, cdns_hw_reset, "%llu\n");
> > > +
> > >   /**
> > >    * sdw_cdns_debugfs_init() - Cadence debugfs init
> > >    * @cdns: Cadence instance
> > > @@ -348,6 +365,9 @@ DEFINE_SHOW_ATTRIBUTE(cdns_reg);
> > >   void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
> > >   {
> > >   	debugfs_create_file("cdns-registers", 0400, root, cdns, &cdns_reg_fops);
> > > +
> > > +	debugfs_create_file("cdns-hw-reset", 0200, root, cdns,
> > > +			    &cdns_hw_reset_fops);
> > >   }
> > >   EXPORT_SYMBOL_GPL(sdw_cdns_debugfs_init);
> > > -- 
> > > 2.20.1
> > 

-- 
~Vinod

[PATCH v2 3/5] soundwire: intel: add helper for initialization

[Pierre-Louis Bossart]

Move code to helper for reuse in power management routines

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 57e599919479..cdb3243e8823 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -994,6 +994,15 @@ static struct sdw_master_ops sdw_intel_ops = {
 	.post_bank_switch = intel_post_bank_switch,
 };
 
+static int intel_init(struct sdw_intel *sdw)
+{
+	/* Initialize shim and controller */
+	intel_link_power_up(sdw);
+	intel_shim_init(sdw);
+
+	return sdw_cdns_init(&sdw->cdns);
+}
+
 /*
  * probe and init
  */
@@ -1036,11 +1045,8 @@ static int intel_probe(struct platform_device *pdev)
 		return 0;
 	}
 
-	/* Initialize shim and controller */
-	intel_link_power_up(sdw);
-	intel_shim_init(sdw);
-
-	ret = sdw_cdns_init(&sdw->cdns);
+	/* Initialize shim, controller and Cadence IP */
+	ret = intel_init(sdw);
 	if (ret)
 		goto err_init;
 
-- 
2.20.1

[PATCH v2 4/5] soundwire: intel/cadence: add flag for interrupt enable

[Pierre-Louis Bossart]

Prepare for future PM support and fix error handling by disabling
interrupts as needed.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/cadence_master.c | 18 ++++++++++++------
 drivers/soundwire/cadence_master.h |  2 +-
 drivers/soundwire/intel.c          | 12 +++++++-----
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index 5f900cf2acb9..a71df99ca18f 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -819,14 +819,17 @@ 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)
+int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
 {
-	u32 mask;
+	u32 slave_intmask0 = 0;
+	u32 slave_intmask1 = 0;
+	u32 mask = 0;
+
+	if (!state)
+		goto update_masks;
 
-	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
-		    CDNS_MCP_SLAVE_INTMASK0_MASK);
-	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
-		    CDNS_MCP_SLAVE_INTMASK1_MASK);
+	slave_intmask0 = CDNS_MCP_SLAVE_INTMASK0_MASK;
+	slave_intmask1 = CDNS_MCP_SLAVE_INTMASK1_MASK;
 
 	/* enable detection of all slave state changes */
 	mask = CDNS_MCP_INT_SLAVE_MASK;
@@ -849,6 +852,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
 	if (interrupt_mask) /* parameter override */
 		mask = interrupt_mask;
 
+update_masks:
+	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);
 
 	/* commit changes */
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
index 1a67728c5000..302351808098 100644
--- a/drivers/soundwire/cadence_master.h
+++ b/drivers/soundwire/cadence_master.h
@@ -162,7 +162,7 @@ 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);
+int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
 
 #ifdef CONFIG_DEBUG_FS
 void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index cdb3243e8823..08530c136c5f 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1036,7 +1036,7 @@ static int intel_probe(struct platform_device *pdev)
 	ret = sdw_add_bus_master(&sdw->cdns.bus);
 	if (ret) {
 		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
-		goto err_master_reg;
+		return ret;
 	}
 
 	if (sdw->cdns.bus.prop.hw_disabled) {
@@ -1067,7 +1067,7 @@ static int intel_probe(struct platform_device *pdev)
 		goto err_init;
 	}
 
-	ret = sdw_cdns_enable_interrupt(&sdw->cdns);
+	ret = sdw_cdns_enable_interrupt(&sdw->cdns, true);
 	if (ret < 0) {
 		dev_err(sdw->cdns.dev, "cannot enable interrupts\n");
 		goto err_init;
@@ -1076,7 +1076,7 @@ static int intel_probe(struct platform_device *pdev)
 	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;
+		goto err_interrupt;
 	}
 
 	/* Register DAIs */
@@ -1084,18 +1084,19 @@ static int intel_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(sdw->cdns.dev, "DAI registration failed: %d\n", ret);
 		snd_soc_unregister_component(sdw->cdns.dev);
-		goto err_dai;
+		goto err_interrupt;
 	}
 
 	intel_debugfs_init(sdw);
 
 	return 0;
 
+err_interrupt:
+	sdw_cdns_enable_interrupt(&sdw->cdns, false);
 err_dai:
 	free_irq(sdw->res->irq, sdw);
 err_init:
 	sdw_delete_bus_master(&sdw->cdns.bus);
-err_master_reg:
 	return ret;
 }
 
@@ -1107,6 +1108,7 @@ static int intel_remove(struct platform_device *pdev)
 
 	if (!sdw->cdns.bus.prop.hw_disabled) {
 		intel_debugfs_exit(sdw);
+		sdw_cdns_enable_interrupt(&sdw->cdns, false);
 		free_irq(sdw->res->irq, sdw);
 		snd_soc_unregister_component(sdw->cdns.dev);
 	}
-- 
2.20.1

Re: [PATCH v2 4/5] soundwire: intel/cadence: add flag for interrupt enable

[Vinod Koul]

On 16-09-19, 14:09, Pierre-Louis Bossart wrote:
> Prepare for future PM support and fix error handling by disabling
> interrupts as needed.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/cadence_master.c | 18 ++++++++++++------
>  drivers/soundwire/cadence_master.h |  2 +-
>  drivers/soundwire/intel.c          | 12 +++++++-----
>  3 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index 5f900cf2acb9..a71df99ca18f 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -819,14 +819,17 @@ 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)
> +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
>  {
> -	u32 mask;
> +	u32 slave_intmask0 = 0;
> +	u32 slave_intmask1 = 0;
> +	u32 mask = 0;
> +
> +	if (!state)
> +		goto update_masks;
>  
> -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
> -		    CDNS_MCP_SLAVE_INTMASK0_MASK);
> -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
> -		    CDNS_MCP_SLAVE_INTMASK1_MASK);
> +	slave_intmask0 = CDNS_MCP_SLAVE_INTMASK0_MASK;
> +	slave_intmask1 = CDNS_MCP_SLAVE_INTMASK1_MASK;
>  
>  	/* enable detection of all slave state changes */
>  	mask = CDNS_MCP_INT_SLAVE_MASK;
> @@ -849,6 +852,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
>  	if (interrupt_mask) /* parameter override */
>  		mask = interrupt_mask;
>  
> +update_masks:
> +	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);
>  
>  	/* commit changes */
> diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
> index 1a67728c5000..302351808098 100644
> --- a/drivers/soundwire/cadence_master.h
> +++ b/drivers/soundwire/cadence_master.h
> @@ -162,7 +162,7 @@ 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);
> +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
>  
>  #ifdef CONFIG_DEBUG_FS
>  void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index cdb3243e8823..08530c136c5f 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1036,7 +1036,7 @@ static int intel_probe(struct platform_device *pdev)
>  	ret = sdw_add_bus_master(&sdw->cdns.bus);
>  	if (ret) {
>  		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
> -		goto err_master_reg;
> +		return ret;

I am not sure I like this line change, before this IIRC the function and
single place of return, so changing this doesn't seem to improve
anything here..?

>  	}
>  
>  	if (sdw->cdns.bus.prop.hw_disabled) {
> @@ -1067,7 +1067,7 @@ static int intel_probe(struct platform_device *pdev)
>  		goto err_init;
>  	}
>  
> -	ret = sdw_cdns_enable_interrupt(&sdw->cdns);
> +	ret = sdw_cdns_enable_interrupt(&sdw->cdns, true);
>  	if (ret < 0) {
>  		dev_err(sdw->cdns.dev, "cannot enable interrupts\n");
>  		goto err_init;
> @@ -1076,7 +1076,7 @@ static int intel_probe(struct platform_device *pdev)
>  	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;
> +		goto err_interrupt;
>  	}
>  
>  	/* Register DAIs */
> @@ -1084,18 +1084,19 @@ static int intel_probe(struct platform_device *pdev)
>  	if (ret) {
>  		dev_err(sdw->cdns.dev, "DAI registration failed: %d\n", ret);
>  		snd_soc_unregister_component(sdw->cdns.dev);
> -		goto err_dai;
> +		goto err_interrupt;
>  	}
>  
>  	intel_debugfs_init(sdw);
>  
>  	return 0;
>  
> +err_interrupt:
> +	sdw_cdns_enable_interrupt(&sdw->cdns, false);
>  err_dai:

Isn't this unused now?

>  	free_irq(sdw->res->irq, sdw);
>  err_init:
>  	sdw_delete_bus_master(&sdw->cdns.bus);
> -err_master_reg:
>  	return ret;
>  }
>  
> @@ -1107,6 +1108,7 @@ static int intel_remove(struct platform_device *pdev)
>  
>  	if (!sdw->cdns.bus.prop.hw_disabled) {
>  		intel_debugfs_exit(sdw);
> +		sdw_cdns_enable_interrupt(&sdw->cdns, false);
>  		free_irq(sdw->res->irq, sdw);
>  		snd_soc_unregister_component(sdw->cdns.dev);
>  	}
> -- 
> 2.20.1

-- 
~Vinod

Re: [alsa-devel] [PATCH v2 4/5] soundwire: intel/cadence: add flag for interrupt enable

[Pierre-Louis Bossart]

On 10/20/19 11:14 PM, Vinod Koul wrote:
> On 16-09-19, 14:09, Pierre-Louis Bossart wrote:
>> Prepare for future PM support and fix error handling by disabling
>> interrupts as needed.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   drivers/soundwire/cadence_master.c | 18 ++++++++++++------
>>   drivers/soundwire/cadence_master.h |  2 +-
>>   drivers/soundwire/intel.c          | 12 +++++++-----
>>   3 files changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
>> index 5f900cf2acb9..a71df99ca18f 100644
>> --- a/drivers/soundwire/cadence_master.c
>> +++ b/drivers/soundwire/cadence_master.c
>> @@ -819,14 +819,17 @@ 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)
>> +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
>>   {
>> -	u32 mask;
>> +	u32 slave_intmask0 = 0;
>> +	u32 slave_intmask1 = 0;
>> +	u32 mask = 0;
>> +
>> +	if (!state)
>> +		goto update_masks;
>>   
>> -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
>> -		    CDNS_MCP_SLAVE_INTMASK0_MASK);
>> -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
>> -		    CDNS_MCP_SLAVE_INTMASK1_MASK);
>> +	slave_intmask0 = CDNS_MCP_SLAVE_INTMASK0_MASK;
>> +	slave_intmask1 = CDNS_MCP_SLAVE_INTMASK1_MASK;
>>   
>>   	/* enable detection of all slave state changes */
>>   	mask = CDNS_MCP_INT_SLAVE_MASK;
>> @@ -849,6 +852,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
>>   	if (interrupt_mask) /* parameter override */
>>   		mask = interrupt_mask;
>>   
>> +update_masks:
>> +	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);
>>   
>>   	/* commit changes */
>> diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
>> index 1a67728c5000..302351808098 100644
>> --- a/drivers/soundwire/cadence_master.h
>> +++ b/drivers/soundwire/cadence_master.h
>> @@ -162,7 +162,7 @@ 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);
>> +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
>>   
>>   #ifdef CONFIG_DEBUG_FS
>>   void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
>> index cdb3243e8823..08530c136c5f 100644
>> --- a/drivers/soundwire/intel.c
>> +++ b/drivers/soundwire/intel.c
>> @@ -1036,7 +1036,7 @@ static int intel_probe(struct platform_device *pdev)
>>   	ret = sdw_add_bus_master(&sdw->cdns.bus);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
>> -		goto err_master_reg;
>> +		return ret;
> 
> I am not sure I like this line change, before this IIRC the function and
> single place of return, so changing this doesn't seem to improve
> anything here..?

Doing a goto to do a return is not very nice either.

I can change this, but it doesn't really matter: this entire code will 
be removed anyways to get rid of platform_devices and the probe itself 
will be split in two.

> 
>>   	}
>>   
>>   	if (sdw->cdns.bus.prop.hw_disabled) {
>> @@ -1067,7 +1067,7 @@ static int intel_probe(struct platform_device *pdev)
>>   		goto err_init;
>>   	}
>>   
>> -	ret = sdw_cdns_enable_interrupt(&sdw->cdns);
>> +	ret = sdw_cdns_enable_interrupt(&sdw->cdns, true);
>>   	if (ret < 0) {
>>   		dev_err(sdw->cdns.dev, "cannot enable interrupts\n");
>>   		goto err_init;
>> @@ -1076,7 +1076,7 @@ static int intel_probe(struct platform_device *pdev)
>>   	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;
>> +		goto err_interrupt;
>>   	}
>>   
>>   	/* Register DAIs */
>> @@ -1084,18 +1084,19 @@ static int intel_probe(struct platform_device *pdev)
>>   	if (ret) {
>>   		dev_err(sdw->cdns.dev, "DAI registration failed: %d\n", ret);
>>   		snd_soc_unregister_component(sdw->cdns.dev);
>> -		goto err_dai;
>> +		goto err_interrupt;
>>   	}
>>   
>>   	intel_debugfs_init(sdw);
>>   
>>   	return 0;
>>   
>> +err_interrupt:
>> +	sdw_cdns_enable_interrupt(&sdw->cdns, false);
>>   err_dai:
> 
> Isn't this unused now?
> 
>>   	free_irq(sdw->res->irq, sdw);
>>   err_init:
>>   	sdw_delete_bus_master(&sdw->cdns.bus);
>> -err_master_reg:
>>   	return ret;
>>   }
>>   
>> @@ -1107,6 +1108,7 @@ static int intel_remove(struct platform_device *pdev)
>>   
>>   	if (!sdw->cdns.bus.prop.hw_disabled) {
>>   		intel_debugfs_exit(sdw);
>> +		sdw_cdns_enable_interrupt(&sdw->cdns, false);
>>   		free_irq(sdw->res->irq, sdw);
>>   		snd_soc_unregister_component(sdw->cdns.dev);
>>   	}
>> -- 
>> 2.20.1
> 

Re: [alsa-devel] [PATCH v2 4/5] soundwire: intel/cadence: add flag for interrupt enable

[Vinod Koul]

On 21-10-19, 05:26, Pierre-Louis Bossart wrote:
> On 10/20/19 11:14 PM, Vinod Koul wrote:
> > On 16-09-19, 14:09, Pierre-Louis Bossart wrote:
> > > Prepare for future PM support and fix error handling by disabling
> > > interrupts as needed.
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > ---
> > >   drivers/soundwire/cadence_master.c | 18 ++++++++++++------
> > >   drivers/soundwire/cadence_master.h |  2 +-
> > >   drivers/soundwire/intel.c          | 12 +++++++-----
> > >   3 files changed, 20 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> > > index 5f900cf2acb9..a71df99ca18f 100644
> > > --- a/drivers/soundwire/cadence_master.c
> > > +++ b/drivers/soundwire/cadence_master.c
> > > @@ -819,14 +819,17 @@ 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)
> > > +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
> > >   {
> > > -	u32 mask;
> > > +	u32 slave_intmask0 = 0;
> > > +	u32 slave_intmask1 = 0;
> > > +	u32 mask = 0;
> > > +
> > > +	if (!state)
> > > +		goto update_masks;
> > > -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
> > > -		    CDNS_MCP_SLAVE_INTMASK0_MASK);
> > > -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
> > > -		    CDNS_MCP_SLAVE_INTMASK1_MASK);
> > > +	slave_intmask0 = CDNS_MCP_SLAVE_INTMASK0_MASK;
> > > +	slave_intmask1 = CDNS_MCP_SLAVE_INTMASK1_MASK;
> > >   	/* enable detection of all slave state changes */
> > >   	mask = CDNS_MCP_INT_SLAVE_MASK;
> > > @@ -849,6 +852,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
> > >   	if (interrupt_mask) /* parameter override */
> > >   		mask = interrupt_mask;
> > > +update_masks:
> > > +	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);
> > >   	/* commit changes */
> > > diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
> > > index 1a67728c5000..302351808098 100644
> > > --- a/drivers/soundwire/cadence_master.h
> > > +++ b/drivers/soundwire/cadence_master.h
> > > @@ -162,7 +162,7 @@ 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);
> > > +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
> > >   #ifdef CONFIG_DEBUG_FS
> > >   void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
> > > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> > > index cdb3243e8823..08530c136c5f 100644
> > > --- a/drivers/soundwire/intel.c
> > > +++ b/drivers/soundwire/intel.c
> > > @@ -1036,7 +1036,7 @@ static int intel_probe(struct platform_device *pdev)
> > >   	ret = sdw_add_bus_master(&sdw->cdns.bus);
> > >   	if (ret) {
> > >   		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
> > > -		goto err_master_reg;
> > > +		return ret;
> > 
> > I am not sure I like this line change, before this IIRC the function and
> > single place of return, so changing this doesn't seem to improve
> > anything here..?
> 
> Doing a goto to do a return is not very nice either.

Hrmm, isn't that what you are doing few lines below. The point here is
that this line of change here doesnt change anything, doesnt improve
anything so why change :)

> I can change this, but it doesn't really matter: this entire code will be
> removed anyways to get rid of platform_devices and the probe itself will be
> split in two.
> 
> > 
> > >   	}
> > >   	if (sdw->cdns.bus.prop.hw_disabled) {
> > > @@ -1067,7 +1067,7 @@ static int intel_probe(struct platform_device *pdev)
> > >   		goto err_init;
> > >   	}
> > > -	ret = sdw_cdns_enable_interrupt(&sdw->cdns);
> > > +	ret = sdw_cdns_enable_interrupt(&sdw->cdns, true);
> > >   	if (ret < 0) {
> > >   		dev_err(sdw->cdns.dev, "cannot enable interrupts\n");
> > >   		goto err_init;
> > > @@ -1076,7 +1076,7 @@ static int intel_probe(struct platform_device *pdev)
> > >   	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;
> > > +		goto err_interrupt;
> > >   	}
> > >   	/* Register DAIs */
> > > @@ -1084,18 +1084,19 @@ static int intel_probe(struct platform_device *pdev)
> > >   	if (ret) {
> > >   		dev_err(sdw->cdns.dev, "DAI registration failed: %d\n", ret);
> > >   		snd_soc_unregister_component(sdw->cdns.dev);
> > > -		goto err_dai;
> > > +		goto err_interrupt;
> > >   	}
> > >   	intel_debugfs_init(sdw);
> > >   	return 0;
> > > +err_interrupt:
> > > +	sdw_cdns_enable_interrupt(&sdw->cdns, false);
> > >   err_dai:
> > 
> > Isn't this unused now?

??? you missed this.

-- 
~Vinod

Re: [alsa-devel] [PATCH v2 4/5] soundwire: intel/cadence: add flag for interrupt enable

[Pierre-Louis Bossart]

On 10/21/19 11:55 PM, Vinod Koul wrote:
> On 21-10-19, 05:26, Pierre-Louis Bossart wrote:
>> On 10/20/19 11:14 PM, Vinod Koul wrote:
>>> On 16-09-19, 14:09, Pierre-Louis Bossart wrote:
>>>> Prepare for future PM support and fix error handling by disabling
>>>> interrupts as needed.
>>>>
>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>> ---
>>>>    drivers/soundwire/cadence_master.c | 18 ++++++++++++------
>>>>    drivers/soundwire/cadence_master.h |  2 +-
>>>>    drivers/soundwire/intel.c          | 12 +++++++-----
>>>>    3 files changed, 20 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
>>>> index 5f900cf2acb9..a71df99ca18f 100644
>>>> --- a/drivers/soundwire/cadence_master.c
>>>> +++ b/drivers/soundwire/cadence_master.c
>>>> @@ -819,14 +819,17 @@ 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)
>>>> +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
>>>>    {
>>>> -	u32 mask;
>>>> +	u32 slave_intmask0 = 0;
>>>> +	u32 slave_intmask1 = 0;
>>>> +	u32 mask = 0;
>>>> +
>>>> +	if (!state)
>>>> +		goto update_masks;
>>>> -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
>>>> -		    CDNS_MCP_SLAVE_INTMASK0_MASK);
>>>> -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
>>>> -		    CDNS_MCP_SLAVE_INTMASK1_MASK);
>>>> +	slave_intmask0 = CDNS_MCP_SLAVE_INTMASK0_MASK;
>>>> +	slave_intmask1 = CDNS_MCP_SLAVE_INTMASK1_MASK;
>>>>    	/* enable detection of all slave state changes */
>>>>    	mask = CDNS_MCP_INT_SLAVE_MASK;
>>>> @@ -849,6 +852,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
>>>>    	if (interrupt_mask) /* parameter override */
>>>>    		mask = interrupt_mask;
>>>> +update_masks:
>>>> +	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);
>>>>    	/* commit changes */
>>>> diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
>>>> index 1a67728c5000..302351808098 100644
>>>> --- a/drivers/soundwire/cadence_master.h
>>>> +++ b/drivers/soundwire/cadence_master.h
>>>> @@ -162,7 +162,7 @@ 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);
>>>> +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
>>>>    #ifdef CONFIG_DEBUG_FS
>>>>    void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
>>>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
>>>> index cdb3243e8823..08530c136c5f 100644
>>>> --- a/drivers/soundwire/intel.c
>>>> +++ b/drivers/soundwire/intel.c
>>>> @@ -1036,7 +1036,7 @@ static int intel_probe(struct platform_device *pdev)
>>>>    	ret = sdw_add_bus_master(&sdw->cdns.bus);
>>>>    	if (ret) {
>>>>    		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
>>>> -		goto err_master_reg;
>>>> +		return ret;
>>>
>>> I am not sure I like this line change, before this IIRC the function and
>>> single place of return, so changing this doesn't seem to improve
>>> anything here..?
>>
>> Doing a goto to do a return is not very nice either.
> 
> Hrmm, isn't that what you are doing few lines below. The point here is
> that this line of change here doesnt change anything, doesnt improve
> anything so why change :)

I knew there was a reason.. the existing code on your soundwire/next 
branch mixes goto and return, so I chose to use a return for the first 
case as well.

	ret = sdw_add_bus_master(&sdw->cdns.bus);
	if (ret) {
		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
		goto err_master_reg; >>> changed to return ret;
	}

	if (sdw->cdns.bus.prop.hw_disabled) {
		dev_info(&pdev->dev, "SoundWire master %d is disabled, ignoring\n",
			 sdw->cdns.bus.link_id);
		return 0;
	}



> 
>> I can change this, but it doesn't really matter: this entire code will be
>> removed anyways to get rid of platform_devices and the probe itself will be
>> split in two.
>>
>>>
>>>>    	}
>>>>    	if (sdw->cdns.bus.prop.hw_disabled) {
>>>> @@ -1067,7 +1067,7 @@ static int intel_probe(struct platform_device *pdev)
>>>>    		goto err_init;
>>>>    	}
>>>> -	ret = sdw_cdns_enable_interrupt(&sdw->cdns);
>>>> +	ret = sdw_cdns_enable_interrupt(&sdw->cdns, true);
>>>>    	if (ret < 0) {
>>>>    		dev_err(sdw->cdns.dev, "cannot enable interrupts\n");
>>>>    		goto err_init;
>>>> @@ -1076,7 +1076,7 @@ static int intel_probe(struct platform_device *pdev)
>>>>    	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;
>>>> +		goto err_interrupt;
>>>>    	}
>>>>    	/* Register DAIs */
>>>> @@ -1084,18 +1084,19 @@ static int intel_probe(struct platform_device *pdev)
>>>>    	if (ret) {
>>>>    		dev_err(sdw->cdns.dev, "DAI registration failed: %d\n", ret);
>>>>    		snd_soc_unregister_component(sdw->cdns.dev);
>>>> -		goto err_dai;
>>>> +		goto err_interrupt;
>>>>    	}
>>>>    	intel_debugfs_init(sdw);
>>>>    	return 0;
>>>> +err_interrupt:
>>>> +	sdw_cdns_enable_interrupt(&sdw->cdns, false);
>>>>    err_dai:
>>>
>>> Isn't this unused now?
> 
> ??? you missed this.

fixed.

[PATCH v2 5/5] soundwire: cadence_master: make clock stop exit configurable on init

[Pierre-Louis Bossart]

The use of clock stop is not a requirement, the IP can e.g. be
completely power gated and not detect any wakes while in s2idle/deep
sleep.

For now clock-stop is not supported anyways so the control parameter
is always false. This will be revisited when we add clock stop.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/cadence_master.c | 15 ++++++++-------
 drivers/soundwire/cadence_master.h |  2 +-
 drivers/soundwire/intel.c          |  2 +-
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index a71df99ca18f..06d83ad88d76 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -996,7 +996,7 @@ 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)
+int sdw_cdns_init(struct sdw_cdns *cdns, bool clock_stop_exit)
 {
 	struct sdw_bus *bus = &cdns->bus;
 	struct sdw_master_prop *prop = &bus->prop;
@@ -1004,12 +1004,13 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
 	int divider;
 	int ret;
 
-	/* Exit clock stop */
-	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;
+	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 */
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
index 302351808098..9d7a48ac6fc4 100644
--- a/drivers/soundwire/cadence_master.h
+++ b/drivers/soundwire/cadence_master.h
@@ -158,7 +158,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);
+int sdw_cdns_init(struct sdw_cdns *cdns, bool clock_stop_exit);
 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 08530c136c5f..2443b9b32b46 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1000,7 +1000,7 @@ static int intel_init(struct sdw_intel *sdw)
 	intel_link_power_up(sdw);
 	intel_shim_init(sdw);
 
-	return sdw_cdns_init(&sdw->cdns);
+	return sdw_cdns_init(&sdw->cdns, false);
 }
 
 /*
-- 
2.20.1

Re: [alsa-devel] [PATCH v2 0/5] soundwire: intel/cadence: better initialization

[Pierre-Louis Bossart]

On 9/16/19 2:09 PM, Pierre-Louis Bossart wrote:
> V2 of the original series 'soundwire: inits and PM additions for 5.4',
> with PM additions removed since more tests on hardware are required.

Vinod, if you are back at your desk, those patches are almost a month 
old. thanks!

> 
> Changes since v1: addressed feedback from Vinod Koul
> clarified init changes impact Intel and Cadence sides
> remove unnecessary intermediate variable
> disable interrupts when exit_reset fails, updated error handling
> returned -EINVAL on debugfs invalid parameter
> 
> Pierre-Louis Bossart (5):
>    soundwire: intel/cadence: fix startup sequence
>    soundwire: cadence_master: add hw_reset capability in debugfs
>    soundwire: intel: add helper for initialization
>    soundwire: intel/cadence: add flag for interrupt enable
>    soundwire: cadence_master: make clock stop exit configurable on init
> 
>   drivers/soundwire/cadence_master.c | 131 +++++++++++++++++++++--------
>   drivers/soundwire/cadence_master.h |   5 +-
>   drivers/soundwire/intel.c          |  38 ++++++---
>   3 files changed, 126 insertions(+), 48 deletions(-)
>