* [PATCH 0/5] soundwire: only clear valid interrupts
@ 2020-11-24 1:33 Bard Liao
2020-11-24 1:33 ` [PATCH 1/5] soundwire: bus: add comments to explain interrupt loop filter Bard Liao
` (5 more replies)
0 siblings, 6 replies; 7+ messages in thread
From: Bard Liao @ 2020-11-24 1:33 UTC (permalink / raw)
To: alsa-devel, vkoul
Cc: vinod.koul, linux-kernel, gregkh, jank, srinivas.kandagatla,
rander.wang, ranjani.sridharan, hui.wang, pierre-louis.bossart,
sanyog.r.kale, mengdong.lin, bard.liao
We wrote 1 to the handled interrupts bits along with 0 to all other bits
to the SoundWire DPx interrupt register. However, DP0 has reserved fields
and the read-only SDCA_CASCADE bit. DPN also has reserved fields. We should
not try to write values in these fields.
Besides, we deal with pending interrupts in a loop but we didn't reset the
slave_notify status.
Pierre-Louis Bossart (5):
soundwire: bus: add comments to explain interrupt loop filter
soundwire: bus: reset slave_notify status at each loop
soundwire: registers: add definitions for clearable interrupt fields
soundwire: bus: only clear valid DP0 interrupts
soundwire: bus: only clear valid DPN interrupts
drivers/soundwire/bus.c | 27 +++++++++++++++++--------
include/linux/soundwire/sdw_registers.h | 11 ++++++++++
2 files changed, 30 insertions(+), 8 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/5] soundwire: bus: add comments to explain interrupt loop filter
2020-11-24 1:33 [PATCH 0/5] soundwire: only clear valid interrupts Bard Liao
@ 2020-11-24 1:33 ` Bard Liao
2020-11-24 1:33 ` [PATCH 2/5] soundwire: bus: reset slave_notify status at each loop Bard Liao
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Bard Liao @ 2020-11-24 1:33 UTC (permalink / raw)
To: alsa-devel, vkoul
Cc: vinod.koul, linux-kernel, gregkh, jank, srinivas.kandagatla,
rander.wang, ranjani.sridharan, hui.wang, pierre-louis.bossart,
sanyog.r.kale, mengdong.lin, bard.liao
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The interrupt handling in SoundWire requires software to re-read the
interrupt status after clearing an interrupt. In case the interrupt is
still outstanding, the code in bus.c will loop a number of times,
however that loop is limited to the interrupts detected in the first
read. This strategy helps meet SoundWire requirements without
remaining forever in an interrupt handler.
Add a couple of comments to document this design.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
drivers/soundwire/bus.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index ffe4600fd95b..45131b9f5080 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -1334,6 +1334,7 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status)
"SDW_DP0_INT read failed:%d\n", status2);
return status2;
}
+ /* filter to limit loop to interrupts identified in the first status read */
status &= status2;
count++;
@@ -1404,6 +1405,7 @@ static int sdw_handle_port_interrupt(struct sdw_slave *slave,
"SDW_DPN_INT read failed:%d\n", status2);
return status2;
}
+ /* filter to limit loop to interrupts identified in the first status read */
status &= status2;
count++;
@@ -1589,7 +1591,10 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
sdca_cascade = ret & SDW_DP0_SDCA_CASCADE;
}
- /* Make sure no interrupts are pending */
+ /*
+ * Make sure no interrupts are pending, but filter to limit loop
+ * to interrupts identified in the first status read
+ */
buf &= _buf;
buf2[0] &= _buf2[0];
buf2[1] &= _buf2[1];
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/5] soundwire: bus: reset slave_notify status at each loop
2020-11-24 1:33 [PATCH 0/5] soundwire: only clear valid interrupts Bard Liao
2020-11-24 1:33 ` [PATCH 1/5] soundwire: bus: add comments to explain interrupt loop filter Bard Liao
@ 2020-11-24 1:33 ` Bard Liao
2020-11-24 1:33 ` [PATCH 3/5] soundwire: registers: add definitions for clearable interrupt fields Bard Liao
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Bard Liao @ 2020-11-24 1:33 UTC (permalink / raw)
To: alsa-devel, vkoul
Cc: vinod.koul, linux-kernel, gregkh, jank, srinivas.kandagatla,
rander.wang, ranjani.sridharan, hui.wang, pierre-louis.bossart,
sanyog.r.kale, mengdong.lin, bard.liao
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The code loops multiple times to deal with pending interrupts, but we
never reset the slave_notify status.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
drivers/soundwire/bus.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 45131b9f5080..d6e646521819 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -1425,7 +1425,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
u8 clear = 0, bit, port_status[15] = {0};
int port_num, stat, ret, count = 0;
unsigned long port;
- bool slave_notify = false;
+ bool slave_notify;
u8 sdca_cascade = 0;
u8 buf, buf2[2], _buf, _buf2[2];
bool parity_check;
@@ -1467,6 +1467,8 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
}
do {
+ slave_notify = false;
+
/*
* Check parity, bus clash and Slave (impl defined)
* interrupt
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/5] soundwire: registers: add definitions for clearable interrupt fields
2020-11-24 1:33 [PATCH 0/5] soundwire: only clear valid interrupts Bard Liao
2020-11-24 1:33 ` [PATCH 1/5] soundwire: bus: add comments to explain interrupt loop filter Bard Liao
2020-11-24 1:33 ` [PATCH 2/5] soundwire: bus: reset slave_notify status at each loop Bard Liao
@ 2020-11-24 1:33 ` Bard Liao
2020-11-24 1:33 ` [PATCH 4/5] soundwire: bus: only clear valid DP0 interrupts Bard Liao
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Bard Liao @ 2020-11-24 1:33 UTC (permalink / raw)
To: alsa-devel, vkoul
Cc: vinod.koul, linux-kernel, gregkh, jank, srinivas.kandagatla,
rander.wang, ranjani.sridharan, hui.wang, pierre-louis.bossart,
sanyog.r.kale, mengdong.lin, bard.liao
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
DP0 has reserved fields and the read-only SDCA_CASCADE bit. We should
not try to write values in these fields, so add a formal definition
for clearable interrupts to be used in DP0 interrupt handling.
DPN also has reserved fields so add definitions for clearable
interrupts as well.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
include/linux/soundwire/sdw_registers.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h
index f420e8059779..0cb1a22685b8 100644
--- a/include/linux/soundwire/sdw_registers.h
+++ b/include/linux/soundwire/sdw_registers.h
@@ -41,6 +41,12 @@
#define SDW_DP0_INT_IMPDEF1 BIT(5)
#define SDW_DP0_INT_IMPDEF2 BIT(6)
#define SDW_DP0_INT_IMPDEF3 BIT(7)
+#define SDW_DP0_INTERRUPTS (SDW_DP0_INT_TEST_FAIL | \
+ SDW_DP0_INT_PORT_READY | \
+ SDW_DP0_INT_BRA_FAILURE | \
+ SDW_DP0_INT_IMPDEF1 | \
+ SDW_DP0_INT_IMPDEF2 | \
+ SDW_DP0_INT_IMPDEF3)
#define SDW_DP0_PORTCTRL_DATAMODE GENMASK(3, 2)
#define SDW_DP0_PORTCTRL_NXTINVBANK BIT(4)
@@ -241,6 +247,11 @@
#define SDW_DPN_INT_IMPDEF1 BIT(5)
#define SDW_DPN_INT_IMPDEF2 BIT(6)
#define SDW_DPN_INT_IMPDEF3 BIT(7)
+#define SDW_DPN_INTERRUPTS (SDW_DPN_INT_TEST_FAIL | \
+ SDW_DPN_INT_PORT_READY | \
+ SDW_DPN_INT_IMPDEF1 | \
+ SDW_DPN_INT_IMPDEF2 | \
+ SDW_DPN_INT_IMPDEF3)
#define SDW_DPN_PORTCTRL_FLOWMODE GENMASK(1, 0)
#define SDW_DPN_PORTCTRL_DATAMODE GENMASK(3, 2)
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 4/5] soundwire: bus: only clear valid DP0 interrupts
2020-11-24 1:33 [PATCH 0/5] soundwire: only clear valid interrupts Bard Liao
` (2 preceding siblings ...)
2020-11-24 1:33 ` [PATCH 3/5] soundwire: registers: add definitions for clearable interrupt fields Bard Liao
@ 2020-11-24 1:33 ` Bard Liao
2020-11-24 1:33 ` [PATCH 5/5] soundwire: bus: only clear valid DPN interrupts Bard Liao
2020-11-25 5:02 ` [PATCH 0/5] soundwire: only clear valid interrupts Vinod Koul
5 siblings, 0 replies; 7+ messages in thread
From: Bard Liao @ 2020-11-24 1:33 UTC (permalink / raw)
To: alsa-devel, vkoul
Cc: vinod.koul, linux-kernel, gregkh, jank, srinivas.kandagatla,
rander.wang, ranjani.sridharan, hui.wang, pierre-louis.bossart,
sanyog.r.kale, mengdong.lin, bard.liao
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We should only access the fields that are relevant for DP0, and never
write to reserved or read-only SDCA_CASCADE fields.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
drivers/soundwire/bus.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index d6e646521819..f66a804f9b74 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -1280,7 +1280,7 @@ static int sdw_initialize_slave(struct sdw_slave *slave)
static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status)
{
- u8 clear = 0, impl_int_mask;
+ u8 clear, impl_int_mask;
int status, status2, ret, count = 0;
status = sdw_read(slave, SDW_DP0_INT);
@@ -1291,6 +1291,8 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status)
}
do {
+ clear = status & ~SDW_DP0_INTERRUPTS;
+
if (status & SDW_DP0_INT_TEST_FAIL) {
dev_err(&slave->dev, "Test fail for port 0\n");
clear |= SDW_DP0_INT_TEST_FAIL;
@@ -1319,7 +1321,7 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status)
*slave_status = clear;
}
- /* clear the interrupt */
+ /* clear the interrupts but don't touch reserved and SDCA_CASCADE fields */
ret = sdw_write(slave, SDW_DP0_INT, clear);
if (ret < 0) {
dev_err(slave->bus->dev,
@@ -1340,7 +1342,7 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status)
count++;
/* we can get alerts while processing so keep retrying */
- } while (status != 0 && count < SDW_READ_INTR_CLEAR_RETRY);
+ } while ((status & SDW_DP0_INTERRUPTS) && (count < SDW_READ_INTR_CLEAR_RETRY));
if (count == SDW_READ_INTR_CLEAR_RETRY)
dev_warn(slave->bus->dev, "Reached MAX_RETRY on DP0 read\n");
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 5/5] soundwire: bus: only clear valid DPN interrupts
2020-11-24 1:33 [PATCH 0/5] soundwire: only clear valid interrupts Bard Liao
` (3 preceding siblings ...)
2020-11-24 1:33 ` [PATCH 4/5] soundwire: bus: only clear valid DP0 interrupts Bard Liao
@ 2020-11-24 1:33 ` Bard Liao
2020-11-25 5:02 ` [PATCH 0/5] soundwire: only clear valid interrupts Vinod Koul
5 siblings, 0 replies; 7+ messages in thread
From: Bard Liao @ 2020-11-24 1:33 UTC (permalink / raw)
To: alsa-devel, vkoul
Cc: vinod.koul, linux-kernel, gregkh, jank, srinivas.kandagatla,
rander.wang, ranjani.sridharan, hui.wang, pierre-louis.bossart,
sanyog.r.kale, mengdong.lin, bard.liao
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Mirror the changes made for DP0 and don't modify reserved fields.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
drivers/soundwire/bus.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index f66a804f9b74..d1e8c3a54976 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -1353,7 +1353,7 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status)
static int sdw_handle_port_interrupt(struct sdw_slave *slave,
int port, u8 *slave_status)
{
- u8 clear = 0, impl_int_mask;
+ u8 clear, impl_int_mask;
int status, status2, ret, count = 0;
u32 addr;
@@ -1370,6 +1370,8 @@ static int sdw_handle_port_interrupt(struct sdw_slave *slave,
}
do {
+ clear = status & ~SDW_DPN_INTERRUPTS;
+
if (status & SDW_DPN_INT_TEST_FAIL) {
dev_err(&slave->dev, "Test fail for port:%d\n", port);
clear |= SDW_DPN_INT_TEST_FAIL;
@@ -1392,7 +1394,7 @@ static int sdw_handle_port_interrupt(struct sdw_slave *slave,
*slave_status = clear;
}
- /* clear the interrupt */
+ /* clear the interrupt but don't touch reserved fields */
ret = sdw_write(slave, addr, clear);
if (ret < 0) {
dev_err(slave->bus->dev,
@@ -1413,7 +1415,7 @@ static int sdw_handle_port_interrupt(struct sdw_slave *slave,
count++;
/* we can get alerts while processing so keep retrying */
- } while (status != 0 && count < SDW_READ_INTR_CLEAR_RETRY);
+ } while ((status & SDW_DPN_INTERRUPTS) && (count < SDW_READ_INTR_CLEAR_RETRY));
if (count == SDW_READ_INTR_CLEAR_RETRY)
dev_warn(slave->bus->dev, "Reached MAX_RETRY on port read");
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 0/5] soundwire: only clear valid interrupts
2020-11-24 1:33 [PATCH 0/5] soundwire: only clear valid interrupts Bard Liao
` (4 preceding siblings ...)
2020-11-24 1:33 ` [PATCH 5/5] soundwire: bus: only clear valid DPN interrupts Bard Liao
@ 2020-11-25 5:02 ` Vinod Koul
5 siblings, 0 replies; 7+ messages in thread
From: Vinod Koul @ 2020-11-25 5:02 UTC (permalink / raw)
To: Bard Liao
Cc: alsa-devel, linux-kernel, gregkh, jank, srinivas.kandagatla,
rander.wang, ranjani.sridharan, hui.wang, pierre-louis.bossart,
sanyog.r.kale, mengdong.lin, bard.liao
On 24-11-20, 09:33, Bard Liao wrote:
> We wrote 1 to the handled interrupts bits along with 0 to all other bits
> to the SoundWire DPx interrupt register. However, DP0 has reserved fields
> and the read-only SDCA_CASCADE bit. DPN also has reserved fields. We should
> not try to write values in these fields.
> Besides, we deal with pending interrupts in a loop but we didn't reset the
> slave_notify status.
Applied, thanks
--
~Vinod
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-25 5:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-24 1:33 [PATCH 0/5] soundwire: only clear valid interrupts Bard Liao
2020-11-24 1:33 ` [PATCH 1/5] soundwire: bus: add comments to explain interrupt loop filter Bard Liao
2020-11-24 1:33 ` [PATCH 2/5] soundwire: bus: reset slave_notify status at each loop Bard Liao
2020-11-24 1:33 ` [PATCH 3/5] soundwire: registers: add definitions for clearable interrupt fields Bard Liao
2020-11-24 1:33 ` [PATCH 4/5] soundwire: bus: only clear valid DP0 interrupts Bard Liao
2020-11-24 1:33 ` [PATCH 5/5] soundwire: bus: only clear valid DPN interrupts Bard Liao
2020-11-25 5:02 ` [PATCH 0/5] soundwire: only clear valid interrupts Vinod Koul
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).