diff mbox series

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

Message ID 20210126083746.3238-4-yung-chuan.liao@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series soundwire: clear bus clash/parity interrupt before the mask is enabled | expand

Commit Message

Bard Liao Jan. 26, 2021, 8:37 a.m. UTC
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(-)

Comments

Vinod Koul Feb. 1, 2021, 11:09 a.m. UTC | #1
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
Pierre-Louis Bossart Feb. 1, 2021, 4:29 p.m. UTC | #2
>>   	 * 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.
Vinod Koul Feb. 2, 2021, 4:44 a.m. UTC | #3
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.
diff mbox series

Patch

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);