[PATCH 0/3] soundwire: clear bus clash/parity interrupt before the mask is enabled

[PATCH 0/3] soundwire: clear bus clash/parity interrupt before the mask is enabled

[Bard Liao]

The SoundWire specification allows a Slave device to report a bus clash
or parity error with the in-band interrupt mechanism.
Unfortunately, on some platforms, these errors are randomly reported and
don't seem to be valid.
This series suggests the addition of a Master level quirk to discard such
interrupts. The quirk should in theory have been added at the Slave level,
but since the problem was detected with different generations of Slave
devices it's hard to point to a specific IP. The problem might also be
board-dependent and hence dealing with a Master quirk is simpler.

Bard Liao (2):
  soundwire: bus: clear bus clash interrupt before the mask is enabled
  soundwire: intel: add SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH quirk

Pierre-Louis Bossart (1):
  soundwire: bus: clear parity interrupt before the mask is enabled

 drivers/soundwire/bus.c       | 19 +++++++++++++++++++
 drivers/soundwire/intel.c     |  3 +++
 include/linux/soundwire/sdw.h |  5 +++++
 3 files changed, 27 insertions(+)

-- 
2.17.1

[PATCH 1/3] soundwire: bus: clear bus clash interrupt before the mask is enabled

[Bard Liao]

The SoundWire specification allows a Slave device to report a bus clash
with the in-band interrupt mechanism when it detects a conflict while
driving a bitSlot it owns. This can be a symptom of an electrical conflict
or a programming error, and it's vital to detect reliably.

Unfortunately, on some platforms, bus clashes are randomly reported by
Slave devices after a bus reset, with an interrupt status set even before
the bus clash interrupt is enabled. These initial spurious interrupts are
not relevant and should optionally be filtered out, while leaving the
interrupt mechanism enabled to detect 'true' issues.

This patch suggests the addition of a Master level quirk to discard such
interrupts. The quirk should in theory have been added at the Slave level,
but since the problem was detected with different generations of Slave
devices it's hard to point to a specific IP. The problem might also be
board-dependent and hence dealing with a Master quirk is simpler.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c       | 10 ++++++++++
 include/linux/soundwire/sdw.h |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 6e1c988f3845..d394905936e4 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -1240,6 +1240,7 @@ static int sdw_slave_set_frequency(struct sdw_slave *slave)
 static int sdw_initialize_slave(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
+	int status;
 	int ret;
 	u8 val;
 
@@ -1247,6 +1248,15 @@ static int sdw_initialize_slave(struct sdw_slave *slave)
 	if (ret < 0)
 		return ret;
 
+	if (slave->bus->prop.quirks & SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH) {
+		/* Clear bus clash interrupt before enabling interrupt mask */
+		status = sdw_read_no_pm(slave, SDW_SCP_INT1);
+		if (status & SDW_SCP_INT1_BUS_CLASH) {
+			dev_warn(&slave->dev, "Bus clash detected before INT mask is enabled\n");
+			sdw_write_no_pm(slave, SDW_SCP_INT1, SDW_SCP_INT1_BUS_CLASH);
+		}
+	}
+
 	/*
 	 * Set SCP_INT1_MASK register, typically bus clash and
 	 * implementation-defined interrupt mask. The Parity detection
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index f0b01b728640..a2766c3b603d 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -405,6 +405,7 @@ struct sdw_slave_prop {
  * command
  * @mclk_freq: clock reference passed to SoundWire Master, in Hz.
  * @hw_disabled: if true, the Master is not functional, typically due to pin-mux
+ * @quirks: bitmask identifying optional behavior beyond the scope of the MIPI specification
  */
 struct sdw_master_prop {
 	u32 revision;
@@ -421,8 +422,11 @@ struct sdw_master_prop {
 	u32 err_threshold;
 	u32 mclk_freq;
 	bool hw_disabled;
+	u32 quirks;
 };
 
+#define SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH	BIT(0)
+
 int sdw_master_read_prop(struct sdw_bus *bus);
 int sdw_slave_read_prop(struct sdw_slave *slave);
 
-- 
2.17.1

[PATCH 2/3] soundwire: intel: add SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH quirk

[Bard Liao]

There is nothing we can do to handle the bus clash interrupt before
interrupt mask is enabled.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index a2d5cdaa9998..f7ba1a77a1df 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1286,6 +1286,8 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
 	if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE)
 		prop->hw_disabled = true;
 
+	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH;
+
 	return 0;
 }
 
-- 
2.17.1

[PATCH 3/3] soundwire: bus: clear parity interrupt before the mask is enabled

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

We recently added the ability to discard bus clash interrupts reported
on startup. These bus clash interrupts can happen randomly on some
platforms and don't seem to be valid. A master-level quirk helped
squelch those spurious errors.

Additional tests on a new platform with the Maxim 98373 amplifier
showed a rare case where the parity interrupt is also thrown on
startup, at the same time as bus clashes. This issue only seems to
happen infrequently and was only observed during suspend-resume stress
tests while audio is streaming. We could make the problem go away by
adding a Slave-level quirk, but there is no evidence that the issue is
actually a Slave problem: the parity is provided by the Master, which
could also set an invalid parity in corner cases.

This patch suggests an additional bus-level quirk for parity, which is
only applied when the codec device is not known to have an issue with
parity. The initial parity error will be ignored, but a trace will be
logged to help identify potential root causes (likely a combination of
issues on both master and slave sides influenced by board-specific
electrical parameters).

BugLink: https://github.com/thesofproject/linux/issues/2533
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/bus.c       | 9 +++++++++
 drivers/soundwire/intel.c     | 3 ++-
 include/linux/soundwire/sdw.h | 1 +
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index d394905936e4..57581fdb2ea9 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -1256,6 +1256,15 @@ static int sdw_initialize_slave(struct sdw_slave *slave)
 			sdw_write_no_pm(slave, SDW_SCP_INT1, SDW_SCP_INT1_BUS_CLASH);
 		}
 	}
+	if ((slave->bus->prop.quirks & SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY) &&
+	    !(slave->prop.quirks & SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY)) {
+		/* Clear parity interrupt before enabling interrupt mask */
+		status = sdw_read_no_pm(slave, SDW_SCP_INT1);
+		if (status & SDW_SCP_INT1_PARITY) {
+			dev_warn(&slave->dev, "PARITY error detected before INT mask is enabled\n");
+			sdw_write_no_pm(slave, SDW_SCP_INT1, SDW_SCP_INT1_PARITY);
+		}
+	}
 
 	/*
 	 * Set SCP_INT1_MASK register, typically bus clash and
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index f7ba1a77a1df..c1fdc85d0a74 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1286,7 +1286,8 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
 	if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE)
 		prop->hw_disabled = true;
 
-	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH;
+	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH |
+		SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY;
 
 	return 0;
 }
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index a2766c3b603d..30415354d419 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -426,6 +426,7 @@ struct sdw_master_prop {
 };
 
 #define SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH	BIT(0)
+#define SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY	BIT(1)
 
 int sdw_master_read_prop(struct sdw_bus *bus);
 int sdw_slave_read_prop(struct sdw_slave *slave);
-- 
2.17.1

Re: [PATCH 1/3] soundwire: bus: clear bus clash interrupt before the mask is enabled

[Vinod Koul]

On 26-01-21, 16:37, Bard Liao wrote:
> The SoundWire specification allows a Slave device to report a bus clash
> with the in-band interrupt mechanism when it detects a conflict while
> driving a bitSlot it owns. This can be a symptom of an electrical conflict
> or a programming error, and it's vital to detect reliably.
> 
> Unfortunately, on some platforms, bus clashes are randomly reported by
> Slave devices after a bus reset, with an interrupt status set even before
> the bus clash interrupt is enabled. These initial spurious interrupts are
> not relevant and should optionally be filtered out, while leaving the
> interrupt mechanism enabled to detect 'true' issues.
> 
> This patch suggests the addition of a Master level quirk to discard such
> interrupts. The quirk should in theory have been added at the Slave level,
> but since the problem was detected with different generations of Slave
> devices it's hard to point to a specific IP. The problem might also be
> board-dependent and hence dealing with a Master quirk is simpler.
> 
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/bus.c       | 10 ++++++++++
>  include/linux/soundwire/sdw.h |  4 ++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 6e1c988f3845..d394905936e4 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -1240,6 +1240,7 @@ static int sdw_slave_set_frequency(struct sdw_slave *slave)
>  static int sdw_initialize_slave(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> +	int status;
>  	int ret;
>  	u8 val;
>  
> @@ -1247,6 +1248,15 @@ static int sdw_initialize_slave(struct sdw_slave *slave)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (slave->bus->prop.quirks & SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH) {
> +		/* Clear bus clash interrupt before enabling interrupt mask */
> +		status = sdw_read_no_pm(slave, SDW_SCP_INT1);
> +		if (status & SDW_SCP_INT1_BUS_CLASH) {
> +			dev_warn(&slave->dev, "Bus clash detected before INT mask is enabled\n");
> +			sdw_write_no_pm(slave, SDW_SCP_INT1, SDW_SCP_INT1_BUS_CLASH);
> +		}
> +	}
> +
>  	/*
>  	 * Set SCP_INT1_MASK register, typically bus clash and
>  	 * implementation-defined interrupt mask. The Parity detection
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index f0b01b728640..a2766c3b603d 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -405,6 +405,7 @@ struct sdw_slave_prop {
>   * command
>   * @mclk_freq: clock reference passed to SoundWire Master, in Hz.
>   * @hw_disabled: if true, the Master is not functional, typically due to pin-mux
> + * @quirks: bitmask identifying optional behavior beyond the scope of the MIPI specification
>   */
>  struct sdw_master_prop {
>  	u32 revision;
> @@ -421,8 +422,11 @@ struct sdw_master_prop {
>  	u32 err_threshold;
>  	u32 mclk_freq;
>  	bool hw_disabled;
> +	u32 quirks;

Can we do u64 here please.. I dont know where we would end up.. but
would hate if we start running out of space ..


>  };
>  
> +#define SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH	BIT(0)
> +
>  int sdw_master_read_prop(struct sdw_bus *bus);
>  int sdw_slave_read_prop(struct sdw_slave *slave);
>  
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 1/3] soundwire: bus: clear bus clash interrupt before the mask is enabled

[Vinod Koul]

On 01-02-21, 15:58, Vinod Koul wrote:
> On 26-01-21, 16:37, Bard Liao wrote:

> >  struct sdw_master_prop {
> >  	u32 revision;
> > @@ -421,8 +422,11 @@ struct sdw_master_prop {
> >  	u32 err_threshold;
> >  	u32 mclk_freq;
> >  	bool hw_disabled;
> > +	u32 quirks;
> 
> Can we do u64 here please.. I dont know where we would end up.. but
> would hate if we start running out of space ..

Also, is the sdw_master_prop right place for a 'quirk' property. I think
we can use sdw_master_device or sdw_bus as this seems like a bus
quirk..?

--
~Vinod

Re: [PATCH 2/3] soundwire: intel: add SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH quirk

[Vinod Koul]

On 26-01-21, 16:37, Bard Liao wrote:
> There is nothing we can do to handle the bus clash interrupt before
> interrupt mask is enabled.
> 
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/intel.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index a2d5cdaa9998..f7ba1a77a1df 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1286,6 +1286,8 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
>  	if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE)
>  		prop->hw_disabled = true;
>  
> +	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH;

Should this not be last 'enabling' the quirk patch in series :)

-- 
~Vinod

Re: [PATCH 3/3] soundwire: bus: clear parity interrupt before the mask is enabled

[Vinod Koul]

On 26-01-21, 16:37, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> We recently added the ability to discard bus clash interrupts reported
> on startup. These bus clash interrupts can happen randomly on some
> platforms and don't seem to be valid. A master-level quirk helped
> squelch those spurious errors.
> 
> Additional tests on a new platform with the Maxim 98373 amplifier
> showed a rare case where the parity interrupt is also thrown on
> startup, at the same time as bus clashes. This issue only seems to
> happen infrequently and was only observed during suspend-resume stress
> tests while audio is streaming. We could make the problem go away by
> adding a Slave-level quirk, but there is no evidence that the issue is
> actually a Slave problem: the parity is provided by the Master, which
> could also set an invalid parity in corner cases.
> 
> This patch suggests an additional bus-level quirk for parity, which is
> only applied when the codec device is not known to have an issue with
> parity. The initial parity error will be ignored, but a trace will be
> logged to help identify potential root causes (likely a combination of
> issues on both master and slave sides influenced by board-specific
> electrical parameters).
> 
> BugLink: https://github.com/thesofproject/linux/issues/2533
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/bus.c       | 9 +++++++++
>  drivers/soundwire/intel.c     | 3 ++-
>  include/linux/soundwire/sdw.h | 1 +
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index d394905936e4..57581fdb2ea9 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -1256,6 +1256,15 @@ static int sdw_initialize_slave(struct sdw_slave *slave)
>  			sdw_write_no_pm(slave, SDW_SCP_INT1, SDW_SCP_INT1_BUS_CLASH);
>  		}
>  	}
> +	if ((slave->bus->prop.quirks & SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY) &&
> +	    !(slave->prop.quirks & SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY)) {
> +		/* Clear parity interrupt before enabling interrupt mask */
> +		status = sdw_read_no_pm(slave, SDW_SCP_INT1);
> +		if (status & SDW_SCP_INT1_PARITY) {
> +			dev_warn(&slave->dev, "PARITY error detected before INT mask is enabled\n");
> +			sdw_write_no_pm(slave, SDW_SCP_INT1, SDW_SCP_INT1_PARITY);
> +		}
> +	}
>  
>  	/*
>  	 * Set SCP_INT1_MASK register, typically bus clash and
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index f7ba1a77a1df..c1fdc85d0a74 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1286,7 +1286,8 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
>  	if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE)
>  		prop->hw_disabled = true;
>  
> -	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH;
> +	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH |
> +		SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY;

move this to intel patch please..

>  
>  	return 0;
>  }
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index a2766c3b603d..30415354d419 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -426,6 +426,7 @@ struct sdw_master_prop {
>  };
>  
>  #define SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH	BIT(0)
> +#define SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY	BIT(1)

Why not add this quirk in patch 1..?

Also add comments about each quirk, hopefully it wont be a big table

-- 
~Vinod

Re: [PATCH 1/3] soundwire: bus: clear bus clash interrupt before the mask is enabled

[Pierre-Louis Bossart]

On 2/1/21 4:38 AM, Vinod Koul wrote:
> On 01-02-21, 15:58, Vinod Koul wrote:
>> On 26-01-21, 16:37, Bard Liao wrote:
> 
>>>   struct sdw_master_prop {
>>>   	u32 revision;
>>> @@ -421,8 +422,11 @@ struct sdw_master_prop {
>>>   	u32 err_threshold;
>>>   	u32 mclk_freq;
>>>   	bool hw_disabled;
>>> +	u32 quirks;
>>
>> Can we do u64 here please.. I dont know where we would end up.. but
>> would hate if we start running out of space ..
No objection.

> Also, is the sdw_master_prop right place for a 'quirk' property. I think
> we can use sdw_master_device or sdw_bus as this seems like a bus
> quirk..?

It's already part of sdw_bus

struct sdw_bus {
	struct device *dev;
	struct sdw_master_device *md;
	unsigned int link_id;
	int id;
	struct list_head slaves;
	DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
	struct mutex bus_lock;
	struct mutex msg_lock;
	int (*compute_params)(struct sdw_bus *bus);
	const struct sdw_master_ops *ops;
	const struct sdw_master_port_ops *port_ops;
	struct sdw_bus_params params;
	struct sdw_master_prop prop;

The quirks could be set by a firmware property, and it seems logical to 
add them at the same place where we already have properties defined in 
firmware, no? That way all the standard, vendor-specific and quirks are 
read or added in the same place.

the sdw_master_device isn't a good place for quirks IMHO, it's a very 
shallow software-only layer without any existing ties to the hardware 
definition.

Re: [PATCH 2/3] soundwire: intel: add SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH quirk

[Pierre-Louis Bossart]

On 2/1/21 4:42 AM, Vinod Koul wrote:
> On 26-01-21, 16:37, Bard Liao wrote:
>> There is nothing we can do to handle the bus clash interrupt before
>> interrupt mask is enabled.
>>
>> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
>> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
>> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   drivers/soundwire/intel.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
>> index a2d5cdaa9998..f7ba1a77a1df 100644
>> --- a/drivers/soundwire/intel.c
>> +++ b/drivers/soundwire/intel.c
>> @@ -1286,6 +1286,8 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
>>   	if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE)
>>   		prop->hw_disabled = true;
>>   
>> +	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH;
> 
> Should this not be last 'enabling' the quirk patch in series :)

Sorry, I don't understand the comment. Do you mind clarifying Vinod?

Re: [PATCH 3/3] soundwire: bus: clear parity interrupt before the mask is enabled

[Pierre-Louis Bossart]

>>   	 * Set SCP_INT1_MASK register, typically bus clash and
>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
>> index f7ba1a77a1df..c1fdc85d0a74 100644
>> --- a/drivers/soundwire/intel.c
>> +++ b/drivers/soundwire/intel.c
>> @@ -1286,7 +1286,8 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
>>   	if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE)
>>   		prop->hw_disabled = true;
>>   
>> -	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH;
>> +	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH |
>> +		SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY;
> 
> move this to intel patch please..
> 
>>   
>>   	return 0;
>>   }
>> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
>> index a2766c3b603d..30415354d419 100644
>> --- a/include/linux/soundwire/sdw.h
>> +++ b/include/linux/soundwire/sdw.h
>> @@ -426,6 +426,7 @@ struct sdw_master_prop {
>>   };
>>   
>>   #define SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH	BIT(0)
>> +#define SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY	BIT(1)
> 
> Why not add this quirk in patch 1..?

There is an element of history here. We first found out about the bus 
clash on multiple devices and dealt with a specific bug number. Then we 
spend weeks on the parity issue on a new platform and ultimately showed 
we needed a similar work-around.

All these problems are not typical from a user perspective; they appear 
when loading/unloading modules in loops, at some point it seems some 
hardware devices don't always reset properly or there's something 
problematic in power delivery.

I don't think it's an issue if we refactor the code to add the quirks 
first, and add the intel.c patches later. We probably want 2 intel 
changes to keep the references to the bugs though and the detailed 
explanations.

> Also add comments about each quirk, hopefully it wont be a big table

Sounds fine.

Re: [PATCH 1/3] soundwire: bus: clear bus clash interrupt before the mask is enabled

[Vinod Koul]

On 01-02-21, 10:18, Pierre-Louis Bossart wrote:
> On 2/1/21 4:38 AM, Vinod Koul wrote:
> > On 01-02-21, 15:58, Vinod Koul wrote:
> > > On 26-01-21, 16:37, Bard Liao wrote:
> > 
> > > >   struct sdw_master_prop {
> > > >   	u32 revision;
> > > > @@ -421,8 +422,11 @@ struct sdw_master_prop {
> > > >   	u32 err_threshold;
> > > >   	u32 mclk_freq;
> > > >   	bool hw_disabled;
> > > > +	u32 quirks;
> > > 
> > > Can we do u64 here please.. I dont know where we would end up.. but
> > > would hate if we start running out of space ..
> No objection.
> 
> > Also, is the sdw_master_prop right place for a 'quirk' property. I think
> > we can use sdw_master_device or sdw_bus as this seems like a bus
> > quirk..?
> 
> It's already part of sdw_bus

Right, but the point is that the properties were mostly derived from
DiSco, so am bit skeptical about it adding it there..

> struct sdw_bus {
> 	struct device *dev;
> 	struct sdw_master_device *md;
> 	unsigned int link_id;
> 	int id;
> 	struct list_head slaves;
> 	DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
> 	struct mutex bus_lock;
> 	struct mutex msg_lock;
> 	int (*compute_params)(struct sdw_bus *bus);
> 	const struct sdw_master_ops *ops;
> 	const struct sdw_master_port_ops *port_ops;
> 	struct sdw_bus_params params;
> 	struct sdw_master_prop prop;
> 
> The quirks could be set by a firmware property, and it seems logical to add
> them at the same place where we already have properties defined in firmware,
> no? That way all the standard, vendor-specific and quirks are read or added
> in the same place.

Or they could be set by driver as well based on device id, compatible
and so on.. It maybe fw property as well, so limiting to property may not
be best idea IMO.

> the sdw_master_device isn't a good place for quirks IMHO, it's a very
> shallow software-only layer without any existing ties to the hardware
> definition.

This one I would agree.

-- 
~Vinod

Re: [PATCH 2/3] soundwire: intel: add SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH quirk

[Vinod Koul]

On 01-02-21, 10:20, Pierre-Louis Bossart wrote:
> 
> 
> On 2/1/21 4:42 AM, Vinod Koul wrote:
> > On 26-01-21, 16:37, Bard Liao wrote:
> > > There is nothing we can do to handle the bus clash interrupt before
> > > interrupt mask is enabled.
> > > 
> > > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > > Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> > > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > ---
> > >   drivers/soundwire/intel.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> > > index a2d5cdaa9998..f7ba1a77a1df 100644
> > > --- a/drivers/soundwire/intel.c
> > > +++ b/drivers/soundwire/intel.c
> > > @@ -1286,6 +1286,8 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
> > >   	if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE)
> > >   		prop->hw_disabled = true;
> > > +	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH;
> > 
> > Should this not be last 'enabling' the quirk patch in series :)
> 
> Sorry, I don't understand the comment. Do you mind clarifying Vinod?

Sure, I would like to series built as, first defining the quirk
along/followed by bus changes. Then the last patch should be intel
controller changes and setting the quirks (like above) in the last
patch.

Let me know if you would need further clarification

-- 
~Vinod

Re: [PATCH 3/3] soundwire: bus: clear parity interrupt before the mask is enabled

[Vinod Koul]

On 01-02-21, 10:29, Pierre-Louis Bossart wrote:
> 
> > >   	 * Set SCP_INT1_MASK register, typically bus clash and
> > > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> > > index f7ba1a77a1df..c1fdc85d0a74 100644
> > > --- a/drivers/soundwire/intel.c
> > > +++ b/drivers/soundwire/intel.c
> > > @@ -1286,7 +1286,8 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
> > >   	if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE)
> > >   		prop->hw_disabled = true;
> > > -	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH;
> > > +	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH |
> > > +		SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY;
> > 
> > move this to intel patch please..
> > 
> > >   	return 0;
> > >   }
> > > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> > > index a2766c3b603d..30415354d419 100644
> > > --- a/include/linux/soundwire/sdw.h
> > > +++ b/include/linux/soundwire/sdw.h
> > > @@ -426,6 +426,7 @@ struct sdw_master_prop {
> > >   };
> > >   #define SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH	BIT(0)
> > > +#define SDW_MASTER_QUIRKS_CLEAR_INITIAL_PARITY	BIT(1)
> > 
> > Why not add this quirk in patch 1..?
> 
> There is an element of history here. We first found out about the bus clash
> on multiple devices and dealt with a specific bug number. Then we spend
> weeks on the parity issue on a new platform and ultimately showed we needed
> a similar work-around.
> 
> All these problems are not typical from a user perspective; they appear when
> loading/unloading modules in loops, at some point it seems some hardware
> devices don't always reset properly or there's something problematic in
> power delivery.
> 
> I don't think it's an issue if we refactor the code to add the quirks first,
> and add the intel.c patches later. We probably want 2 intel changes to keep
> the references to the bugs though and the detailed explanations.

Yes I would like to see that. Explanations are always welcome including
development/debug notes.. Changelogs are very important documentation for
kernel, so relevant details are always good to add.
> 
> > Also add comments about each quirk, hopefully it wont be a big table
> 
> Sounds fine.

-- 
~Vinod

Re: [PATCH 1/3] soundwire: bus: clear bus clash interrupt before the mask is enabled

[Pierre-Louis Bossart]

On 2/1/21 10:39 PM, Vinod Koul wrote:
> On 01-02-21, 10:18, Pierre-Louis Bossart wrote:
>> On 2/1/21 4:38 AM, Vinod Koul wrote:
>>> On 01-02-21, 15:58, Vinod Koul wrote:
>>>> On 26-01-21, 16:37, Bard Liao wrote:
>>>
>>>>>    struct sdw_master_prop {
>>>>>    	u32 revision;
>>>>> @@ -421,8 +422,11 @@ struct sdw_master_prop {
>>>>>    	u32 err_threshold;
>>>>>    	u32 mclk_freq;
>>>>>    	bool hw_disabled;
>>>>> +	u32 quirks;
>>>>
>>>> Can we do u64 here please.. I dont know where we would end up.. but
>>>> would hate if we start running out of space ..
>> No objection.
>>
>>> Also, is the sdw_master_prop right place for a 'quirk' property. I think
>>> we can use sdw_master_device or sdw_bus as this seems like a bus
>>> quirk..?
>>
>> It's already part of sdw_bus
> 
> Right, but the point is that the properties were mostly derived from
> DiSco, so am bit skeptical about it adding it there..

Oh, I am planning to contribute such quirks as MIPI DisCo properties for 
the next revision of the document (along with a capability to disable a 
link). This was not intended to remain Linux- or Intel-specific.

>> struct sdw_bus {
>> 	struct device *dev;
>> 	struct sdw_master_device *md;
>> 	unsigned int link_id;
>> 	int id;
>> 	struct list_head slaves;
>> 	DECLARE_BITMAP(assigned, SDW_MAX_DEVICES);
>> 	struct mutex bus_lock;
>> 	struct mutex msg_lock;
>> 	int (*compute_params)(struct sdw_bus *bus);
>> 	const struct sdw_master_ops *ops;
>> 	const struct sdw_master_port_ops *port_ops;
>> 	struct sdw_bus_params params;
>> 	struct sdw_master_prop prop;
>>
>> The quirks could be set by a firmware property, and it seems logical to add
>> them at the same place where we already have properties defined in firmware,
>> no? That way all the standard, vendor-specific and quirks are read or added
>> in the same place.
> 
> Or they could be set by driver as well based on device id, compatible
> and so on.. It maybe fw property as well, so limiting to property may not
> be best idea IMO.

Not following, sorry. I didn't mean that only firmware can set them.

All of these sdw_master_prop can come from firmware or be hard-coded by 
a driver for a specific implementation. The point is that they need to 
be provided to the bus core so that it knows what to do.

If you look at DisCo today, we ignore the settings for the Slave 
(unfortunately all wrong in BIOS) so the Slave properties are hard-coded 
in drivers, but do use most of the firmware information for the Master, 
so it's really firmware and/or driver that can set these properties.

>> the sdw_master_device isn't a good place for quirks IMHO, it's a very
>> shallow software-only layer without any existing ties to the hardware
>> definition.
> 
> This one I would agree.
> 

Re: [PATCH 2/3] soundwire: intel: add SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH quirk

[Pierre-Louis Bossart]

>>>> +	prop->quirks = SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH;
>>>
>>> Should this not be last 'enabling' the quirk patch in series :)
>>
>> Sorry, I don't understand the comment. Do you mind clarifying Vinod?
> 
> Sure, I would like to series built as, first defining the quirk
> along/followed by bus changes. Then the last patch should be intel
> controller changes and setting the quirks (like above) in the last
> patch.
> 
> Let me know if you would need further clarification

Got it, thanks.

Re: [PATCH 1/3] soundwire: bus: clear bus clash interrupt before the mask is enabled

[Vinod Koul]

On 02-02-21, 10:52, Pierre-Louis Bossart wrote:
> 
> 
> On 2/1/21 10:39 PM, Vinod Koul wrote:
> > On 01-02-21, 10:18, Pierre-Louis Bossart wrote:
> > > On 2/1/21 4:38 AM, Vinod Koul wrote:
> > > > On 01-02-21, 15:58, Vinod Koul wrote:
> > > > > On 26-01-21, 16:37, Bard Liao wrote:
> > > > 
> > > > > >    struct sdw_master_prop {
> > > > > >    	u32 revision;
> > > > > > @@ -421,8 +422,11 @@ struct sdw_master_prop {
> > > > > >    	u32 err_threshold;
> > > > > >    	u32 mclk_freq;
> > > > > >    	bool hw_disabled;
> > > > > > +	u32 quirks;
> > > > > 
> > > > > Can we do u64 here please.. I dont know where we would end up.. but
> > > > > would hate if we start running out of space ..
> > > No objection.
> > > 
> > > > Also, is the sdw_master_prop right place for a 'quirk' property. I think
> > > > we can use sdw_master_device or sdw_bus as this seems like a bus
> > > > quirk..?
> > > 
> > > It's already part of sdw_bus
> > 
> > Right, but the point is that the properties were mostly derived from
> > DiSco, so am bit skeptical about it adding it there..
> 
> Oh, I am planning to contribute such quirks as MIPI DisCo properties for the
> next revision of the document (along with a capability to disable a link).
> This was not intended to remain Linux- or Intel-specific.

Okay lets keep it in properties then

-- 
~Vinod