SoundWire: disable Realtek spurious parity errors by plbossart · Pull Request #1984 · thesofproject/linux

plbossart · 2020-04-06T15:46:15Z

split from PR #1980 and refocused on parity errors reported by Realtek devices even when the PARITY mask is not set.

plbossart · 2020-04-07T14:17:10Z

@shumingfan @bardliao @RanderWang the point of this PR is to show that there is a problem with the initialization of the codec, even when we explicitly disable the PARITY interrupt we still get a parity interrupt...

RanderWang · 2020-04-08T06:35:29Z

@shumingfan @bardliao @RanderWang the point of this PR is to show that there is a problem with the initialization of the codec, even when we explicitly disable the PARITY interrupt we still get a parity interrupt...

@plbossart This PR makes effect on tgl & cml u & h. Parity error disappears with it. @bardliao what is your status ?

bardliao · 2020-04-08T07:22:25Z

@shumingfan @bardliao @RanderWang the point of this PR is to show that there is a problem with the initialization of the codec, even when we explicitly disable the PARITY interrupt we still get a parity interrupt...

@plbossart This PR makes effect on tgl & cml u & h. Parity error disappears with it. @bardliao what is your status ?

I can still see Parity error after the PR is applied. But the frequency is lower than before. I.e. Parity error appears in almost every boot before the PR is applied, but it is about 20% that I can see Parity error after the PR is applied. Besides, I can only see Parity error reported by rt711 with the PR. Parity error is reported by rt1308 and/or rt711 and/or rt715 randomly without this PR.

plbossart · 2020-04-08T14:56:46Z

@shumingfan @bardliao @RanderWang the point of this PR is to show that there is a problem with the initialization of the codec, even when we explicitly disable the PARITY interrupt we still get a parity interrupt...

@plbossart This PR makes effect on tgl & cml u & h. Parity error disappears with it. @bardliao what is your status ?

I can still see Parity error after the PR is applied. But the frequency is lower than before. I.e. Parity error appears in almost every boot before the PR is applied, but it is about 20% that I can see Parity error after the PR is applied. Besides, I can only see Parity error reported by rt711 with the PR. Parity error is reported by rt1308 and/or rt711 and/or rt715 randomly without this PR.

@bardliao Indeed I managed to make the parity error go away completely by re-reading the INT1_MASK register after updating it. So there's something odd here. I had one theory that we should not be changing the interrupt masks while we are dealing with an interrupt, but pushing this setting to a work-queue didn't seem to help.

RanderWang · 2020-04-09T06:03:11Z

@plbossart According to spec, Parity error can only be detected when Master is not a bus owner. When master is a bus owner than parity is calculated based on sampled bus state so detection of the error is not possible. In normal usage, the master owns the bus and the generation of SoundWire Control Word. Is the interrupt unreliable and why we enable it always ?

Parity Error Interrupt Request  1- Parity Error Detected  0- No Parity Error Parity error can be 
generated only when monitor is bus owner. This bit is set when parity calculated by master is 
different than parity set by monitor

RanderWang · 2020-04-09T07:21:24Z

@plbossart sorry, the parity error is caused by slave, not master. The SCP_INTStat1 is already 1 (parity error) after slave number is assigned and before enabling INT1_PARITY, so parity error will generated immediately when enabling parity interrupt.

plbossart · 2020-04-09T10:51:43Z

@plbossart sorry, the parity error is caused by slave, not master. The SCP_INTStat1 is already 1 (parity error) after slave number is assigned and before enabling INT1_PARITY, so parity error will generated immediately when enabling parity interrupt.

That's a problem. when a Slave become attached, it cannot raise an interrupt before the master explicitly programs the relevant masks. See the spec, after a Hard Reset (Severe Reset in 1.2) all masks are cleared.

plbossart · 2020-04-15T22:59:27Z

@bardliao can you look into this with our Realtek friends?

bardliao · 2020-04-16T00:35:59Z

@bardliao can you look into this with our Realtek friends?

Sure, I will co-work with @shumingfan

bardliao · 2020-04-16T08:21:33Z

Indeed I managed to make the parity error go away completely by re-reading the INT1_MASK register after updating it.

@plbossart re-reading INT1_MASK doesn't work on my side but clearing SDW_SCP_INT1 before setting INT1_MASK does. Discussed with @shumingfan, we think it is possible that the parity error happens in the synchronization stage.

@plbossart sorry, the parity error is caused by slave, not master. The SCP_INTStat1 is already 1 (parity error) after slave number is assigned and before enabling INT1_PARITY, so parity error will generated immediately when enabling parity interrupt.

That's a problem. when a Slave become attached, it cannot raise an interrupt before the master explicitly programs the relevant masks. See the spec, after a Hard Reset (Severe Reset in 1.2) all masks are cleared.

Yes, the SDW_SCP_INT1 is already 1, but it is only a flag say hey there is a party error happens and I will raise an interrupt when you set the mask. In fact, it didn't raise an interrupt before we set the mask. Another issue is that we have a share irq, and we will always check the interrupt status. So, we can still see parity error log when the irq is triggered by other interrupts with the parity error bit is set.

In my opinion, clear SDW_SCP_INT1 before setting INT1_MASK could be an valid solution as we don't care about the interrupts happens before INT1_MASK is set.

plbossart · 2020-04-16T13:39:51Z

@bardliao how do you explain that we still get the Parity error then the PARITY mask is not set, as in this PR?

bardliao · 2020-04-17T01:51:13Z

@bardliao how do you explain that we still get the Parity error then the PARITY mask is not set, as in this PR?

We handle all Slave interrupts in sdw_handle_slave_alerts(). So when sdw_handle_slave_alerts() is triggered by Slave impl defined interrupt, it will also show Parity error if SDW_SCP_INT1_PARITY bit is set. In other words, you won't see Parity error if there is no other Slave interrupts happens.

plbossart · 2020-04-17T14:51:30Z

@bardliao how do you explain that we still get the Parity error then the PARITY mask is not set, as in this PR?

We handle all Slave interrupts in sdw_handle_slave_alerts(). So when sdw_handle_slave_alerts() is triggered by Slave impl defined interrupt, it will also show Parity error if SDW_SCP_INT1_PARITY bit is set. In other words, you won't see Parity error if there is no other Slave interrupts happens.

ok, so now that makes sense, thanks @bardliao. I was confused by the 'mask' definition. It does not enable a specific interrupt, it gates the propagation of the interrupt to the PING frame status.

So I guess what we need to do is check if the parity mask is set and discard the interrupt if it isn't, that way the initial condition will not show up.

plbossart · 2020-04-18T00:39:52Z

@shumingfan @bardliao can you please take a look.
I tried to re-enable the parity detection at the end of the io_init function, but was still getting errors, so I will let you chime in on when this can be safely re-enabled.

Also I still have spurious messages now related to jack calibration - when I don't have a headphone/headset plugged-in.
[ 26.384259] rt711_calibration, calibration time-out!

Can this be demoted to a debug message?

lyakh

a very minor comment, looks good to me otherwise.

kv2019i

Looks good. One commit msg typo and a few minor style issues, but nothing really to gate sending upstream (but good to fix if you refresh the series for other reasons).

plbossart · 2020-07-07T15:33:40Z

updated branch to fix all comments from @lyakh and @kv2019i

Note for maintainers: this needs to be submitted upstream after the max98373 codec support is merged.

Add a slave-level property and program the SCP_INT1_MASK as desired by the codec driver. Since there is no DisCo property this has to be an implementation-specific firmware property or hard-coded in the driver. The only functionality change is that implementation-defined interrupts are no longer set for amplifiers - those interrupts are typically for jack detection or acoustic event detection/hotwording. Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Unlike the traditional usage, in the SoundWire specification the interrupt masks only gate the propagation of an interrupt condition to the PING frame status. They do not gate the changes of the INT_STAT registers, which will happen regardless of the mask settings. See Figure 116 of the SoundWire 1.2 specification for an in-depth description of the interrupt model. When the bus driver reads the SCP_INT1_STAT register, it will retrieve all the interrupt status, including for the mask fields that were not explicitly set. For example, even if the PARITY mask is not set, the PARITY error status will be reported if an implementation-defined interrupt for jack detection is enabled and occurs. Filtering undesired interrupt reports and handling has to be implemented in software. This patch enables this filtering for the INT1_IMPL_DEF, PARITY and BUS_CLASH interrupt sources. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Some Slaves report incorrect information in their interrupt status registers after a master/bus reset, track the initial interrupt handling so that quirks can be introduced to filter out incorrect information while keeping interrupts enabled in steady state. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

If a Slave device reports with a quirk that its initial parity check may be incorrect, filter it but keep the parity checks active in steady state. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The parity calculation is not reset on a Severe Reset, which leads to misleading/harmless errors reported on startup. The addition of a quirk helps filter out such errors while leaving the error checks on in steady-state. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Provide prototype and export symbol to enable tests. The bus lock is handled externally to avoid conflicts e.g. between kernel-generated traffic and test traffic. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The Cadence IP can inject errors, let's make use of this capability to test Slave parity error checks. See e.g. example log where both the master and slave detect the parity error injected on a dummy read command. cd /sys/kernel/debug/soundwire/master-1/intel-sdw/ echo 1 > cdns-parity-error-injection [ 44.756249] intel-master sdw-master-1: Parity error [ 44.756313] intel-master sdw-master-1: Msg NACK received [ 44.756366] intel-master sdw-master-1: Msg NACKed for Slave 15 [ 44.756375] intel-master sdw-master-1: trf on Slave 15 failed:-5 [ 44.756382] intel-master sdw-master-1: parity error injection, read: -5 [ 44.756649] rt1308 sdw:1:25d:1308:0: Parity error detected The code makes sure the Master device is resumed, hence the clock restarted, before sending a parity error. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

kv2019i · 2020-07-09T15:05:35Z

@plbossart @bardliao This seems ready to go, but I didn't merge yet. You two probably know better whether we need more review from SDW codec folks on this or not.

plbossart · 2020-07-09T15:08:29Z

@plbossart @bardliao This seems ready to go, but I didn't merge yet. You two probably know better whether we need more review from SDW codec folks on this or not.

Let's wait for @bardliao to review and merge next week.

bardliao · 2020-07-13T08:38:55Z

BDW_WSB_RT286 pr-device-test fails at several PRs and I think it is not caused by this PR. So merge it now.

plbossart requested review from RanderWang, bardliao, dbaluta, juimonen, keyonjie, kv2019i, lgirdwood, libinyang, lyakh and ranj063 as code owners April 6, 2020 15:46

plbossart mentioned this pull request Apr 6, 2020

Reduce dmesg verbosity #1980

Merged

bardliao reviewed Apr 7, 2020

View reviewed changes

Comment thread drivers/soundwire/bus.c

plbossart changed the title ~~SoundWire: Realtek spurious parity errors~~ [TEST] SoundWire: Realtek spurious parity errors Apr 7, 2020

plbossart force-pushed the sdw/rt1308-parity-errors branch from a15b339 to e260b29 Compare April 18, 2020 00:36

plbossart changed the title ~~[TEST] SoundWire: Realtek spurious parity errors~~ SoundWire: disable Realtek spurious parity errors Apr 18, 2020

plbossart requested a review from bardliao April 18, 2020 00:37

plbossart force-pushed the sdw/rt1308-parity-errors branch from c39cca9 to 1c44b22 Compare July 2, 2020 19:57

bardliao reviewed Jul 3, 2020

View reviewed changes

Comment thread drivers/soundwire/bus.c

lyakh requested changes Jul 6, 2020

View reviewed changes

plbossart force-pushed the sdw/rt1308-parity-errors branch from 1c44b22 to 83ecdf5 Compare July 7, 2020 00:10

plbossart requested review from bardliao and lyakh July 7, 2020 00:10

lyakh previously approved these changes Jul 7, 2020

View reviewed changes

Comment thread drivers/soundwire/cadence_master.c

kv2019i previously approved these changes Jul 7, 2020

View reviewed changes

Comment thread drivers/soundwire/bus.c

Comment thread drivers/soundwire/bus.c

Comment thread drivers/soundwire/bus.c

plbossart dismissed stale reviews from kv2019i and lyakh via fe35d95 July 7, 2020 15:32

plbossart force-pushed the sdw/rt1308-parity-errors branch from 83ecdf5 to fe35d95 Compare July 7, 2020 15:32

plbossart requested a review from naveen-manohar as a code owner July 7, 2020 15:32

plbossart requested review from kv2019i and lyakh July 7, 2020 15:33

plbossart added 7 commits July 7, 2020 13:35

soundwire: bus: use quirk to filter out invalid parity errors

4f77e87

If a Slave device reports with a quirk that its initial parity check may be incorrect, filter it but keep the parity checks active in steady state. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart force-pushed the sdw/rt1308-parity-errors branch from fe35d95 to d85b1c9 Compare July 7, 2020 18:36

lyakh approved these changes Jul 8, 2020

View reviewed changes

kv2019i approved these changes Jul 9, 2020

View reviewed changes

bardliao approved these changes Jul 13, 2020

View reviewed changes

bardliao merged commit 898ba76 into thesofproject:topic/sof-dev Jul 13, 2020

Conversation

plbossart commented Apr 6, 2020

Uh oh!

Uh oh!

plbossart commented Apr 7, 2020

Uh oh!

RanderWang commented Apr 8, 2020

Uh oh!

bardliao commented Apr 8, 2020

Uh oh!

plbossart commented Apr 8, 2020

Uh oh!

RanderWang commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Apr 9, 2020

Uh oh!

plbossart commented Apr 9, 2020

Uh oh!

plbossart commented Apr 15, 2020

Uh oh!

bardliao commented Apr 16, 2020

Uh oh!

bardliao commented Apr 16, 2020

Uh oh!

plbossart commented Apr 16, 2020

Uh oh!

bardliao commented Apr 17, 2020

Uh oh!

plbossart commented Apr 17, 2020

Uh oh!

plbossart commented Apr 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart commented Jul 7, 2020

Uh oh!

kv2019i commented Jul 9, 2020

Uh oh!

plbossart commented Jul 9, 2020

Uh oh!

bardliao commented Jul 13, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RanderWang commented Apr 9, 2020 •

edited

Loading