[TEST] soundwire: bus: clear bus clash interrupt after attached #2582

bardliao · 2020-11-25T07:58:24Z

We may get bus clash during the attach process, and the SDW_SCP_INT1_
BUS_CLASH bit will be set once we set the corresponding bit on the
SDW_SCP_INTMASK1 register. But it is harmless after codec is attached.
Clear it right after seting SDW_SCP_INTMASK1 register can reduce the
noise of harmless error.

Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com

Fixes #2578

bardliao · 2020-11-25T11:32:15Z

CI test 849 shows positive result.

RanderWang

LGTM

plbossart

Not sure about this @bardliao, see comments below.

Also not sure if this improves anything?

plbossart · 2020-11-30T14:57:54Z

drivers/soundwire/bus.c

+			       SDW_SCP_INT1_BUS_CLASH, SDW_SCP_INT1_BUS_CLASH);
+	if (ret < 0)
+		dev_err(&slave->dev,
+			"Clear SDW_SCP_INT1_BUS_CLASH failed:%d\n", ret);


How about doing the opposite, first clear the status and then enable the interrupt?

That would remove any memory of the initial bus clash, but allow for programming errors or persistent electrical errors to be detected.

I also think this should be an optional bus behavior, e.g. for Intel devices this behavior would be the default but we don't force all implementations to behave the same.

The commit message is also questionable, a bus clash is not a harmless error!

How about doing the opposite, first clear the status and then enable the interrupt?

That would remove any memory of the initial bus clash, but allow for programming errors or persistent electrical errors to be detected.

I tested it, but clearing before enabling doesn't help. I can still see bus clash on my side. On the other hand, clearing right after enabling fixes the clash issue on my side.

I also think this should be an optional bus behavior, e.g. for Intel devices this behavior would be the default but we don't force all implementations to behave the same.

The commit message is also questionable, a bus clash is not a harmless error!

Agree.
I will make it optional and revise the commit message.

How about doing the opposite, first clear the status and then enable the interrupt?
That would remove any memory of the initial bus clash, but allow for programming errors or persistent electrical errors to be detected.

I tested it, but clearing before enabling doesn't help. I can still see bus clash on my side. On the other hand, clearing right after enabling fixes the clash issue on my side.

Sorry, I tested it with a wrong way. I tested it with sdw_write_no_pm. i.e. without reading operation. That's why bus clash wasn't cleared. The test result is good if I tested it with the updated version.

clear register is not the key step, read register is. It is very trick. @bardliao do you plan to disclose this idea ?

clear register is not the key step, read register is. It is very trick. @bardliao do you plan to disclose this idea ?

mipi_SoundWire_specification_v1-2 session 11.1.2 "Interrupt Model" says successful read from DPN_IntStat register is a necessary process of clearing an interrupt.

RanderWang · 2020-12-01T09:20:17Z

drivers/soundwire/bus.c

+	status = sdw_read_no_pm(slave, SDW_SCP_INT1);
+	if (status & SDW_SCP_INT1_BUS_CLASH) {
+		dev_warn(&slave->dev, "Bus clash detected before enabling\n");
+		sdw_write_no_pm(slave, SDW_SCP_INT1, SDW_SCP_INT1_BUS_CLASH);


sdw_update_no_pm ?

sdw_update_no_pm ?

I got it . update can't work for it will compare clash bit. But how about other bits in SCP_INT1?

sdw_update_no_pm ?

I got it . update can't work for it will compare clash bit. But how about other bits in SCP_INT1?

We only want to clear the bus clash bit, so set 0 to all other bits.

plbossart

that's what I had in mind @bardliao, but it's a master quirk, not a slave one. see comments below.

plbossart · 2020-12-01T15:07:16Z

drivers/soundwire/intel.c

 	if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE)
 		prop->hw_disabled = true;

+	prop->quirks = SDW_SLAVE_QUIRKS_CLEAR_INITIAL_CLASH;


that should be SDW_MASTER_...

plbossart · 2020-12-01T15:10:02Z

include/linux/soundwire/sdw.h

 * 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 deltas from the MIPI specification


it's more 'bitmask identifying optional behavior beyond the scope of the MIPI specification'

plbossart · 2020-12-01T15:10:25Z

include/linux/soundwire/sdw.h

 };

 #define SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY	BIT(0)
+#define SDW_SLAVE_QUIRKS_CLEAR_INITIAL_CLASH	BIT(1)


it's s new set for SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH

plbossart · 2020-12-01T15:21:42Z

drivers/soundwire/bus.c

 static int sdw_initialize_slave(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
+	int status;


Commit message:

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.

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>

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>

xiulipan · 2020-12-02T06:57:36Z

SOFCI TEST

plbossart

LGTM, thanks @bardliao

bardliao requested review from RanderWang and plbossart November 25, 2020 07:58

bardliao requested review from jack-cy-yu, oder-chiou, ryans-lee and shumingfan as code owners November 25, 2020 07:58

RanderWang previously approved these changes Nov 26, 2020

View reviewed changes

bardliao dismissed RanderWang’s stale review via c1c8289 November 30, 2020 02:12

bardliao force-pushed the sdw-bus-clash-error branch from b0ff63f to c1c8289 Compare November 30, 2020 02:12

RanderWang previously approved these changes Nov 30, 2020

View reviewed changes

plbossart reviewed Nov 30, 2020

View reviewed changes

bardliao dismissed RanderWang’s stale review via b1ea2e0 December 1, 2020 09:09

bardliao force-pushed the sdw-bus-clash-error branch from c1c8289 to b1ea2e0 Compare December 1, 2020 09:09

RanderWang reviewed Dec 1, 2020

View reviewed changes

bardliao force-pushed the sdw-bus-clash-error branch 2 times, most recently from c483d14 to 90c26e9 Compare December 1, 2020 11:53

plbossart requested changes Dec 1, 2020

View reviewed changes

bardliao added 2 commits December 2, 2020 10:37

soundwire: intel: add SDW_MASTER_QUIRKS_CLEAR_INITIAL_CLASH quirk

7a2abd8

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>

bardliao force-pushed the sdw-bus-clash-error branch from 90c26e9 to 7a2abd8 Compare December 2, 2020 03:15

RanderWang approved these changes Dec 2, 2020

View reviewed changes

plbossart approved these changes Dec 2, 2020

View reviewed changes

plbossart merged commit 25012df into thesofproject:topic/sof-dev Dec 2, 2020

naveen-manohar mentioned this pull request Feb 3, 2022

IO transfer error timeout and system suspend-resume test fails #3401

Closed

[TEST] soundwire: bus: clear bus clash interrupt after attached #2582

[TEST] soundwire: bus: clear bus clash interrupt after attached #2582

Uh oh!

Conversation

bardliao commented Nov 25, 2020

Uh oh!

bardliao commented Nov 25, 2020

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiulipan commented Dec 2, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants