| Message ID | 20210126083746.3238-4-yung-chuan.liao@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | soundwire: clear bus clash/parity interrupt before the mask is enabled | expand |
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
>> * 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.
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 --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);