LinuxLists.cc - [PATCH 0/9] soundwire: intel: revisit SHIM programming

[permalink] [raw]

Subject: [PATCH 4/9] soundwire: intel: introduce helper for link synchronization

From: Pierre-Louis Bossart <[email protected]>

After arming the synchronization, the SYNCGO field controls the
hardware-based synchronization between links.

Move the programming and wait for clear of SYNCGO to dedicated helper.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 6a745602c9cc..0a4fc7f65743 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -512,6 +512,31 @@ static void intel_shim_sync_arm(struct sdw_intel *sdw)
mutex_unlock(sdw->link_res->shim_lock);
}

+static int intel_shim_sync_go_unlocked(struct sdw_intel *sdw)
+{
+ void __iomem *shim = sdw->link_res->shim;
+ u32 sync_reg;
+ int ret;
+
+ /* Read SYNC register */
+ sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
+
+ /*
+ * Set SyncGO bit to synchronously trigger a bank switch for
+ * all the masters. A write to SYNCGO bit clears CMDSYNC bit for all
+ * the Masters.
+ */
+ sync_reg |= SDW_SHIM_SYNC_SYNCGO;
+
+ ret = intel_clear_bit(shim, SDW_SHIM_SYNC, sync_reg,
+ SDW_SHIM_SYNC_SYNCGO);
+
+ if (ret < 0)
+ dev_err(sdw->cdns.dev, "SyncGO clear failed: %d\n", ret);
+
+ return ret;
+}
+
/*
* PDI routines
*/
@@ -763,15 +788,8 @@ static int intel_post_bank_switch(struct sdw_bus *bus)
ret = 0;
goto unlock;
}
- /*
- * Set SyncGO bit to synchronously trigger a bank switch for
- * all the masters. A write to SYNCGO bit clears CMDSYNC bit for all
- * the Masters.
- */
- sync_reg |= SDW_SHIM_SYNC_SYNCGO;

- ret = intel_clear_bit(shim, SDW_SHIM_SYNC, sync_reg,
- SDW_SHIM_SYNC_SYNCGO);
+ ret = intel_shim_sync_go_unlocked(sdw);
unlock:
mutex_unlock(sdw->link_res->shim_lock);

--
2.17.1

2020-06-24 05:31:21

[permalink] [raw]

Subject: [PATCH 5/9] soundwire: intel_init: add implementation of sdw_intel_enable_irq()

From: Pierre-Louis Bossart <[email protected]>

This function is required to enable all interrupts across all links.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel_init.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index f50a93130d12..d8f0c1472f1f 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -142,6 +142,30 @@ sdw_intel_scan_controller(struct sdw_intel_acpi_info *info)
return 0;
}

+#define HDA_DSP_REG_ADSPIC2 (0x10)
+#define HDA_DSP_REG_ADSPIS2 (0x14)
+#define HDA_DSP_REG_ADSPIC2_SNDW BIT(5)
+
+/**
+ * sdw_intel_enable_irq() - enable/disable Intel SoundWire IRQ
+ * @mmio_base: The mmio base of the control register
+ * @enable: true if enable
+ */
+void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable)
+{
+ u32 val;
+
+ val = readl(mmio_base + HDA_DSP_REG_ADSPIC2);
+
+ if (enable)
+ val |= HDA_DSP_REG_ADSPIC2_SNDW;
+ else
+ val &= ~HDA_DSP_REG_ADSPIC2_SNDW;
+
+ writel(val, mmio_base + HDA_DSP_REG_ADSPIC2);
+}
+EXPORT_SYMBOL(sdw_intel_enable_irq);
+
static struct sdw_intel_ctx
*sdw_intel_probe_controller(struct sdw_intel_res *res)
{
--
2.17.1

2020-06-24 05:32:41

[permalink] [raw]

Subject: [PATCH 1/9] soundwire: intel: reuse code for wait loops to set/clear bits

From: Pierre-Louis Bossart <[email protected]>

Refactor code and use same routines on set/clear

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.c | 45 +++++++++++++++++----------------------
1 file changed, 19 insertions(+), 26 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 7a65414e5714..8c7ae07c0fe1 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -123,40 +123,33 @@ static inline void intel_writew(void __iomem *base, int offset, u16 value)
writew(value, base + offset);
}

+static int intel_wait_bit(void __iomem *base, int offset, u32 mask, u32 target)
+{
+ int timeout = 10;
+ u32 reg_read;
+
+ do {
+ reg_read = readl(base + offset);
+ if ((reg_read & mask) == target)
+ return 0;
+
+ timeout--;
+ usleep_range(50, 100);
+ } while (timeout != 0);
+
+ return -EAGAIN;
+}
+
static int intel_clear_bit(void __iomem *base, int offset, u32 value, u32 mask)
{
- int timeout = 10;
- u32 reg_read;
-
writel(value, base + offset);
- do {
- reg_read = readl(base + offset);
- if (!(reg_read & mask))
- return 0;
-
- timeout--;
- udelay(50);
- } while (timeout != 0);
-
- return -EAGAIN;
+ return intel_wait_bit(base, offset, mask, 0);
}

static int intel_set_bit(void __iomem *base, int offset, u32 value, u32 mask)
{
- int timeout = 10;
- u32 reg_read;
-
writel(value, base + offset);
- do {
- reg_read = readl(base + offset);
- if (reg_read & mask)
- return 0;
-
- timeout--;
- udelay(50);
- } while (timeout != 0);
-
- return -EAGAIN;
+ return intel_wait_bit(base, offset, mask, mask);
}

/*
--
2.17.1

2020-06-24 05:32:41

[permalink] [raw]

Subject: [PATCH 9/9] Soundwire: intel_init: save Slave(s) _ADR info in sdw_intel_ctx

Save ACPI information in context so that we can match machine driver
with sdw _ADR matching tables.

Suggested-by: Guennadi Liakhovetski <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
Signed-off-by: Pierre-Louis Bossart <[email protected]>
---
drivers/soundwire/intel_init.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index eff4e385bb59..6502a5e82229 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -188,7 +188,11 @@ static struct sdw_intel_ctx
struct sdw_intel_link_res *link;
struct sdw_intel_ctx *ctx;
struct acpi_device *adev;
+ struct sdw_slave *slave;
+ struct list_head *node;
+ struct sdw_bus *bus;
u32 link_mask;
+ int num_slaves = 0;
int count;
int i;

@@ -265,6 +269,26 @@ static struct sdw_intel_ctx
link->cdns = platform_get_drvdata(pdev);

list_add_tail(&link->list, &ctx->link_list);
+ bus = &link->cdns->bus;
+ /* Calculate number of slaves */
+ list_for_each(node, &bus->slaves)
+ num_slaves++;
+ }
+
+ ctx->ids = devm_kcalloc(&adev->dev, num_slaves,
+ sizeof(*ctx->ids), GFP_KERNEL);
+ if (!ctx->ids)
+ goto err;
+
+ ctx->num_slaves = num_slaves;
+ i = 0;
+ list_for_each_entry(link, &ctx->link_list, list) {
+ bus = &link->cdns->bus;
+ list_for_each_entry(slave, &bus->slaves, node) {
+ ctx->ids[i].id = slave->id;
+ ctx->ids[i].link_id = bus->link_id;
+ i++;
+ }
}

return ctx;
--
2.17.1

2020-06-24 05:32:48

[permalink] [raw]

Subject: [PATCH 3/9] soundwire: intel: introduce a helper to arm link synchronization

From: Pierre-Louis Bossart <[email protected]>

Move code from pre_bank_switch to dedicated helper, will be used in
follow-up patches as recommended by programming flows.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 4792613e8e5a..6a745602c9cc 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -497,6 +497,21 @@ static int __maybe_unused intel_link_power_down(struct sdw_intel *sdw)
return 0;
}

+static void intel_shim_sync_arm(struct sdw_intel *sdw)
+{
+ void __iomem *shim = sdw->link_res->shim;
+ u32 sync_reg;
+
+ mutex_lock(sdw->link_res->shim_lock);
+
+ /* update SYNC register */
+ sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
+ sync_reg |= (SDW_SHIM_SYNC_CMDSYNC << sdw->instance);
+ intel_writel(shim, SDW_SHIM_SYNC, sync_reg);
+
+ mutex_unlock(sdw->link_res->shim_lock);
+}
+
/*
* PDI routines
*/
@@ -710,21 +725,12 @@ static int intel_pre_bank_switch(struct sdw_bus *bus)
{
struct sdw_cdns *cdns = bus_to_cdns(bus);
struct sdw_intel *sdw = cdns_to_intel(cdns);
- void __iomem *shim = sdw->link_res->shim;
- int sync_reg;

/* Write to register only for multi-link */
if (!bus->multi_link)
return 0;

- mutex_lock(sdw->link_res->shim_lock);
-
- /* Read SYNC register */
- sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
- sync_reg |= SDW_SHIM_SYNC_CMDSYNC << sdw->instance;
- intel_writel(shim, SDW_SHIM_SYNC, sync_reg);
-
- mutex_unlock(sdw->link_res->shim_lock);
+ intel_shim_sync_arm(sdw);

return 0;
}
--
2.17.1

2020-06-24 05:32:57

[permalink] [raw]

Subject: [PATCH 2/9] soundwire: intel: revisit SHIM programming sequences.

From: Pierre-Louis Bossart <[email protected]>

Somehow the existing code is not aligned with the steps described in
the documentation, refactor code and make sure the register
programming sequences are correct. Also add missing power-up,
power-down and wake capabilities (the last two are used in follow-up
patches but introduced here for consistency).

Some of the SHIM registers exposed fields that are link specific, and
in addition some of the power-related registers (SPA/CPA) take time to
be updated. Uncontrolled access leads to timeouts or errors. Add a
mutex, shared by all links, so that all accesses to such registers are
serialized, and follow a pattern of read-modify-write.

This includes making sure SHIM_SYNC is programmed only once, before
the first master is powered on. We use a 'shim_mask' field, shared
between all links and protected by a mutex, to deal with power-up and
power-down sequences.

Note that the SYNCPRD value is tied only to the XTAL value and not the
current bus frequency or the frame rate.

BugLink: https://github.com/thesofproject/linux/issues/1555
Signed-off-by: Rander Wang <[email protected]>
Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.c | 241 +++++++++++++++++++++++-----
drivers/soundwire/intel.h | 4 +
drivers/soundwire/intel_init.c | 4 +
include/linux/soundwire/sdw_intel.h | 2 +
4 files changed, 215 insertions(+), 36 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 8c7ae07c0fe1..4792613e8e5a 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -46,7 +46,8 @@
#define SDW_SHIM_LCTL_SPA BIT(0)
#define SDW_SHIM_LCTL_CPA BIT(8)

-#define SDW_SHIM_SYNC_SYNCPRD_VAL 0x176F
+#define SDW_SHIM_SYNC_SYNCPRD_VAL_24 (24000 / SDW_CADENCE_GSYNC_KHZ - 1)
+#define SDW_SHIM_SYNC_SYNCPRD_VAL_38_4 (38400 / SDW_CADENCE_GSYNC_KHZ - 1)
#define SDW_SHIM_SYNC_SYNCPRD GENMASK(14, 0)
#define SDW_SHIM_SYNC_SYNCCPU BIT(15)
#define SDW_SHIM_SYNC_CMDSYNC_MASK GENMASK(19, 16)
@@ -272,8 +273,46 @@ static int intel_link_power_up(struct sdw_intel *sdw)
{
unsigned int link_id = sdw->instance;
void __iomem *shim = sdw->link_res->shim;
+ u32 *shim_mask = sdw->link_res->shim_mask;
+ struct sdw_bus *bus = &sdw->cdns.bus;
+ struct sdw_master_prop *prop = &bus->prop;
int spa_mask, cpa_mask;
- int link_control, ret;
+ int link_control;
+ int ret = 0;
+ u32 syncprd;
+ u32 sync_reg;
+
+ mutex_lock(sdw->link_res->shim_lock);
+
+ /*
+ * The hardware relies on an internal counter, typically 4kHz,
+ * to generate the SoundWire SSP - which defines a 'safe'
+ * synchronization point between commands and audio transport
+ * and allows for multi link synchronization. The SYNCPRD value
+ * is only dependent on the oscillator clock provided to
+ * the IP, so adjust based on _DSD properties reported in DSDT
+ * tables. The values reported are based on either 24MHz
+ * (CNL/CML) or 38.4 MHz (ICL/TGL+).
+ */
+ if (prop->mclk_freq % 6000000)
+ syncprd = SDW_SHIM_SYNC_SYNCPRD_VAL_38_4;
+ else
+ syncprd = SDW_SHIM_SYNC_SYNCPRD_VAL_24;
+
+ if (!*shim_mask) {
+ /* we first need to program the SyncPRD/CPU registers */
+ dev_dbg(sdw->cdns.dev,
+ "%s: first link up, programming SYNCPRD\n", __func__);
+
+ /* set SyncPRD period */
+ sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
+ sync_reg |= (syncprd <<
+ SDW_REG_SHIFT(SDW_SHIM_SYNC_SYNCPRD));
+
+ /* Set SyncCPU bit */
+ sync_reg |= SDW_SHIM_SYNC_SYNCCPU;
+ intel_writel(shim, SDW_SHIM_SYNC, sync_reg);
+ }

/* Link power up sequence */
link_control = intel_readl(shim, SDW_SHIM_LCTL);
@@ -282,70 +321,182 @@ static int intel_link_power_up(struct sdw_intel *sdw)
link_control |= spa_mask;

ret = intel_set_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
- if (ret < 0)
- return ret;
+ if (ret < 0) {
+ dev_err(sdw->cdns.dev, "Failed to power up link: %d\n", ret);
+ goto out;
+ }
+
+ if (!*shim_mask) {
+ /* SyncCPU will change once link is active */
+ ret = intel_wait_bit(shim, SDW_SHIM_SYNC,
+ SDW_SHIM_SYNC_SYNCCPU, 0);
+ if (ret < 0) {
+ dev_err(sdw->cdns.dev,
+ "Failed to set SHIM_SYNC: %d\n", ret);
+ goto out;
+ }
+ }
+
+ *shim_mask |= BIT(link_id);

sdw->cdns.link_up = true;
- return 0;
+out:
+ mutex_unlock(sdw->link_res->shim_lock);
+
+ return ret;
}

-static int intel_shim_init(struct sdw_intel *sdw)
+/* this needs to be called with shim_lock */
+static void intel_shim_glue_to_master_ip(struct sdw_intel *sdw)
{
void __iomem *shim = sdw->link_res->shim;
unsigned int link_id = sdw->instance;
- int sync_reg, ret;
- u16 ioctl = 0, act = 0;
-
- /* Initialize Shim */
- ioctl |= SDW_SHIM_IOCTL_BKE;
- intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
-
- ioctl |= SDW_SHIM_IOCTL_WPDD;
- intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
-
- ioctl |= SDW_SHIM_IOCTL_DO;
- intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
-
- ioctl |= SDW_SHIM_IOCTL_DOE;
- intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+ u16 ioctl;

/* Switch to MIP from Glue logic */
ioctl = intel_readw(shim, SDW_SHIM_IOCTL(link_id));

ioctl &= ~(SDW_SHIM_IOCTL_DOE);
intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+ usleep_range(10, 15);

ioctl &= ~(SDW_SHIM_IOCTL_DO);
intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+ usleep_range(10, 15);

ioctl |= (SDW_SHIM_IOCTL_MIF);
intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+ usleep_range(10, 15);

ioctl &= ~(SDW_SHIM_IOCTL_BKE);
ioctl &= ~(SDW_SHIM_IOCTL_COE);
+ intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+ usleep_range(10, 15);
+
+ /* at this point Master IP has full control of the I/Os */
+}
+
+/* this needs to be called with shim_lock */
+static void intel_shim_master_ip_to_glue(struct sdw_intel *sdw)
+{
+ unsigned int link_id = sdw->instance;
+ void __iomem *shim = sdw->link_res->shim;
+ u16 ioctl;
+
+ /* Glue logic */
+ ioctl = intel_readw(shim, SDW_SHIM_IOCTL(link_id));
+ ioctl |= SDW_SHIM_IOCTL_BKE;
+ ioctl |= SDW_SHIM_IOCTL_COE;
+ intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+ usleep_range(10, 15);

+ ioctl &= ~(SDW_SHIM_IOCTL_MIF);
intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+ usleep_range(10, 15);
+
+ /* at this point Integration Glue has full control of the I/Os */
+}
+
+static int intel_shim_init(struct sdw_intel *sdw, bool clock_stop)
+{
+ void __iomem *shim = sdw->link_res->shim;
+ unsigned int link_id = sdw->instance;
+ int ret = 0;
+ u16 ioctl = 0, act = 0;
+
+ mutex_lock(sdw->link_res->shim_lock);
+
+ /* Initialize Shim */
+ ioctl |= SDW_SHIM_IOCTL_BKE;
+ intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+ usleep_range(10, 15);
+
+ ioctl |= SDW_SHIM_IOCTL_WPDD;
+ intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+ usleep_range(10, 15);
+
+ ioctl |= SDW_SHIM_IOCTL_DO;
+ intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+ usleep_range(10, 15);
+
+ ioctl |= SDW_SHIM_IOCTL_DOE;
+ intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+ usleep_range(10, 15);
+
+ intel_shim_glue_to_master_ip(sdw);

act |= 0x1 << SDW_REG_SHIFT(SDW_SHIM_CTMCTL_DOAIS);
act |= SDW_SHIM_CTMCTL_DACTQE;
act |= SDW_SHIM_CTMCTL_DODS;
intel_writew(shim, SDW_SHIM_CTMCTL(link_id), act);
+ usleep_range(10, 15);

- /* Now set SyncPRD period */
- sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
- sync_reg |= (SDW_SHIM_SYNC_SYNCPRD_VAL <<
- SDW_REG_SHIFT(SDW_SHIM_SYNC_SYNCPRD));
-
- /* Set SyncCPU bit */
- sync_reg |= SDW_SHIM_SYNC_SYNCCPU;
- ret = intel_clear_bit(shim, SDW_SHIM_SYNC, sync_reg,
- SDW_SHIM_SYNC_SYNCCPU);
- if (ret < 0)
- dev_err(sdw->cdns.dev, "Failed to set sync period: %d\n", ret);
+ mutex_unlock(sdw->link_res->shim_lock);

return ret;
}

+static void __maybe_unused intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
+{
+ void __iomem *shim = sdw->link_res->shim;
+ unsigned int link_id = sdw->instance;
+ u16 wake_en, wake_sts;
+
+ mutex_lock(sdw->link_res->shim_lock);
+ wake_en = intel_readw(shim, SDW_SHIM_WAKEEN);
+
+ if (wake_enable) {
+ /* Enable the wakeup */
+ wake_en |= (SDW_SHIM_WAKEEN_ENABLE << link_id);
+ intel_writew(shim, SDW_SHIM_WAKEEN, wake_en);
+ } else {
+ /* Disable the wake up interrupt */
+ wake_en &= ~(SDW_SHIM_WAKEEN_ENABLE << link_id);
+ intel_writew(shim, SDW_SHIM_WAKEEN, wake_en);
+
+ /* Clear wake status */
+ wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS);
+ wake_sts |= (SDW_SHIM_WAKEEN_ENABLE << link_id);
+ intel_writew(shim, SDW_SHIM_WAKESTS_STATUS, wake_sts);
+ }
+ mutex_unlock(sdw->link_res->shim_lock);
+}
+
+static int __maybe_unused intel_link_power_down(struct sdw_intel *sdw)
+{
+ int link_control, spa_mask, cpa_mask;
+ unsigned int link_id = sdw->instance;
+ void __iomem *shim = sdw->link_res->shim;
+ u32 *shim_mask = sdw->link_res->shim_mask;
+ int ret = 0;
+
+ mutex_lock(sdw->link_res->shim_lock);
+
+ intel_shim_master_ip_to_glue(sdw);
+
+ /* Link power down sequence */
+ link_control = intel_readl(shim, SDW_SHIM_LCTL);
+ spa_mask = ~(SDW_SHIM_LCTL_SPA << link_id);
+ cpa_mask = (SDW_SHIM_LCTL_CPA << link_id);
+ link_control &= spa_mask;
+
+ ret = intel_clear_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
+
+ if (!(*shim_mask & BIT(link_id)))
+ dev_err(sdw->cdns.dev,
+ "%s: Unbalanced power-up/down calls\n", __func__);
+
+ *shim_mask &= ~BIT(link_id);
+
+ mutex_unlock(sdw->link_res->shim_lock);
+
+ if (ret < 0)
+ return ret;
+
+ sdw->cdns.link_up = false;
+ return 0;
+}
+
/*
* PDI routines
*/
@@ -566,11 +717,15 @@ static int intel_pre_bank_switch(struct sdw_bus *bus)
if (!bus->multi_link)
return 0;

+ mutex_lock(sdw->link_res->shim_lock);
+
/* Read SYNC register */
sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
sync_reg |= SDW_SHIM_SYNC_CMDSYNC << sdw->instance;
intel_writel(shim, SDW_SHIM_SYNC, sync_reg);

+ mutex_unlock(sdw->link_res->shim_lock);
+
return 0;
}

@@ -585,6 +740,8 @@ static int intel_post_bank_switch(struct sdw_bus *bus)
if (!bus->multi_link)
return 0;

+ mutex_lock(sdw->link_res->shim_lock);
+
/* Read SYNC register */
sync_reg = intel_readl(shim, SDW_SHIM_SYNC);

@@ -596,9 +753,10 @@ static int intel_post_bank_switch(struct sdw_bus *bus)
*
* So, set the SYNCGO bit only if CMDSYNC bit is set for any Master.
*/
- if (!(sync_reg & SDW_SHIM_SYNC_CMDSYNC_MASK))
- return 0;
-
+ if (!(sync_reg & SDW_SHIM_SYNC_CMDSYNC_MASK)) {
+ ret = 0;
+ goto unlock;
+ }
/*
* Set SyncGO bit to synchronously trigger a bank switch for
* all the masters. A write to SYNCGO bit clears CMDSYNC bit for all
@@ -608,6 +766,9 @@ static int intel_post_bank_switch(struct sdw_bus *bus)

ret = intel_clear_bit(shim, SDW_SHIM_SYNC, sync_reg,
SDW_SHIM_SYNC_SYNCGO);
+unlock:
+ mutex_unlock(sdw->link_res->shim_lock);
+
if (ret < 0)
dev_err(sdw->cdns.dev, "Post bank switch failed: %d\n", ret);

@@ -1011,9 +1172,17 @@ static struct sdw_master_ops sdw_intel_ops = {

static int intel_init(struct sdw_intel *sdw)
{
+ bool clock_stop;
+
/* Initialize shim and controller */
intel_link_power_up(sdw);
- intel_shim_init(sdw);
+
+ clock_stop = sdw_cdns_is_clock_stop(&sdw->cdns);
+
+ intel_shim_init(sdw, clock_stop);
+
+ if (clock_stop)
+ return 0;

return sdw_cdns_init(&sdw->cdns);
}
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index 694117370ac3..d6bdd4d63e08 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -15,6 +15,8 @@
* @irq: Interrupt line
* @ops: Shim callback ops
* @dev: device implementing hw_params and free callbacks
+ * @shim_lock: mutex to handle access to shared SHIM registers
+ * @shim_mask: global pointer to check SHIM register initialization
*/
struct sdw_intel_link_res {
struct platform_device *pdev;
@@ -25,6 +27,8 @@ struct sdw_intel_link_res {
int irq;
const struct sdw_intel_ops *ops;
struct device *dev;
+ struct mutex *shim_lock; /* protect shared registers */
+ u32 *shim_mask;
};

struct sdw_intel {
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 3f2e884b4f6d..f50a93130d12 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -180,6 +180,7 @@ static struct sdw_intel_ctx
ctx->mmio_base = res->mmio_base;
ctx->link_mask = res->link_mask;
ctx->handle = res->handle;
+ mutex_init(&ctx->shim_lock);

link = ctx->links;
link_mask = ctx->link_mask;
@@ -201,6 +202,9 @@ static struct sdw_intel_ctx
link->ops = res->ops;
link->dev = res->dev;

+ link->shim_lock = &ctx->shim_lock;
+ link->shim_mask = &ctx->shim_mask;
+
memset(&pdevinfo, 0, sizeof(pdevinfo));

pdevinfo.parent = res->parent;
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h
index 979b41b5dcb4..120ffddc03d2 100644
--- a/include/linux/soundwire/sdw_intel.h
+++ b/include/linux/soundwire/sdw_intel.h
@@ -115,6 +115,7 @@ struct sdw_intel_slave_id {
* links
* @link_list: list to handle interrupts across all links
* @shim_lock: mutex to handle concurrent rmw access to shared SHIM registers.
+ * @shim_mask: flags to track initialization of SHIM shared registers
*/
struct sdw_intel_ctx {
int count;
@@ -126,6 +127,7 @@ struct sdw_intel_ctx {
struct sdw_intel_slave_id *ids;
struct list_head link_list;
struct mutex shim_lock; /* lock for access to shared SHIM registers */
+ u32 shim_mask;
};

/**
--
2.17.1

2020-06-24 05:33:35

[permalink] [raw]

Subject: [PATCH 6/9] soundwire: intel_init: use EXPORT_SYMBOL_NS

From: Pierre-Louis Bossart <[email protected]>

Make sure all symbols in this soundwire-intel-init module are exported
with a namespace.

The MODULE_IMPORT_NS will be used in Intel/SOF HDaudio modules to be
posted in a separate series.

Namespaces are only introduced for the Intel parts of the SoundWire
code at this time, in future patches we should also add namespaces for
Cadence parts and the SoundWire core.

Suggested-by: Greg KH <[email protected]>
Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel_init.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index d8f0c1472f1f..ad3175272e88 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -164,7 +164,7 @@ void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable)

writel(val, mmio_base + HDA_DSP_REG_ADSPIC2);
}
-EXPORT_SYMBOL(sdw_intel_enable_irq);
+EXPORT_SYMBOL_NS(sdw_intel_enable_irq, SOUNDWIRE_INTEL_INIT);

static struct sdw_intel_ctx
*sdw_intel_probe_controller(struct sdw_intel_res *res)
@@ -353,7 +353,7 @@ int sdw_intel_acpi_scan(acpi_handle *parent_handle,

return sdw_intel_scan_controller(info);
}
-EXPORT_SYMBOL(sdw_intel_acpi_scan);
+EXPORT_SYMBOL_NS(sdw_intel_acpi_scan, SOUNDWIRE_INTEL_INIT);

/**
* sdw_intel_probe() - SoundWire Intel probe routine
@@ -370,7 +370,7 @@ struct sdw_intel_ctx
{
return sdw_intel_probe_controller(res);
}
-EXPORT_SYMBOL(sdw_intel_probe);
+EXPORT_SYMBOL_NS(sdw_intel_probe, SOUNDWIRE_INTEL_INIT);

/**
* sdw_intel_startup() - SoundWire Intel startup
@@ -383,7 +383,7 @@ int sdw_intel_startup(struct sdw_intel_ctx *ctx)
{
return sdw_intel_startup_controller(ctx);
}
-EXPORT_SYMBOL(sdw_intel_startup);
+EXPORT_SYMBOL_NS(sdw_intel_startup, SOUNDWIRE_INTEL_INIT);
/**
* sdw_intel_exit() - SoundWire Intel exit
* @ctx: SoundWire context allocated in the probe
@@ -394,7 +394,7 @@ void sdw_intel_exit(struct sdw_intel_ctx *ctx)
{
sdw_intel_cleanup(ctx);
}
-EXPORT_SYMBOL(sdw_intel_exit);
+EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);

MODULE_LICENSE("Dual BSD/GPL");
MODULE_DESCRIPTION("Intel Soundwire Init Library");
--
2.17.1

2020-06-24 05:34:18

[permalink] [raw]

Subject: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads

The existing code uses one pair of interrupt handler/thread per link
but at the hardware level the interrupt is shared. This works fine for
legacy PCI interrupts, but leads to timeouts in MSI (Message-Signaled
Interrupt) mode, likely due to edges being lost.

This patch unifies interrupt handling for all links. The dedicated
handler is removed since we use a common one for all shared interrupt
sources, and the thread function takes care of dealing with interrupt
sources. This partition follows the model used for the SOF IPC on
HDaudio platforms, where similar timeout issues were noticed and doing
all the interrupt handling/clearing in the thread improved
reliability/stability.

Validation results with 4 links active in parallel show a night-and-day
improvement with no timeouts noticed even during stress tests. Latency
and quality of service are not affected by the change - mostly because
events on a SoundWire link are throttled by the bus frame rate
(typically 8..48kHz).

Signed-off-by: Bard Liao <[email protected]>
Signed-off-by: Pierre-Louis Bossart <[email protected]>
---
drivers/soundwire/cadence_master.c | 18 ++++++++++--------
drivers/soundwire/cadence_master.h | 4 ++++
drivers/soundwire/intel.c | 15 ---------------
drivers/soundwire/intel.h | 4 ++++
drivers/soundwire/intel_init.c | 19 +++++++++++++++++++
5 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index 613dbd415b91..24eafe0aa1c3 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -17,6 +17,7 @@
#include <linux/soundwire/sdw.h>
#include <sound/pcm_params.h>
#include <sound/soc.h>
+#include <linux/workqueue.h>
#include "bus.h"
#include "cadence_master.h"

@@ -790,7 +791,7 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id)
CDNS_MCP_INT_SLAVE_MASK, 0);

int_status &= ~CDNS_MCP_INT_SLAVE_MASK;
- ret = IRQ_WAKE_THREAD;
+ schedule_work(&cdns->work);
}

cdns_writel(cdns, CDNS_MCP_INTSTAT, int_status);
@@ -799,13 +800,15 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id)
EXPORT_SYMBOL(sdw_cdns_irq);

/**
- * sdw_cdns_thread() - Cadence irq thread handler
- * @irq: irq number
- * @dev_id: irq context
+ * To update slave status in a work since we will need to handle
+ * other interrupts eg. CDNS_MCP_INT_RX_WL during the update slave
+ * process.
+ * @work: cdns worker thread
*/
-irqreturn_t sdw_cdns_thread(int irq, void *dev_id)
+static void cdns_update_slave_status_work(struct work_struct *work)
{
- struct sdw_cdns *cdns = dev_id;
+ struct sdw_cdns *cdns =
+ container_of(work, struct sdw_cdns, work);
u32 slave0, slave1;

dev_dbg_ratelimited(cdns->dev, "Slave status change\n");
@@ -822,9 +825,7 @@ irqreturn_t sdw_cdns_thread(int irq, void *dev_id)
cdns_updatel(cdns, CDNS_MCP_INTMASK,
CDNS_MCP_INT_SLAVE_MASK, CDNS_MCP_INT_SLAVE_MASK);

- return IRQ_HANDLED;
}
-EXPORT_SYMBOL(sdw_cdns_thread);

/*
* init routines
@@ -1427,6 +1428,7 @@ int sdw_cdns_probe(struct sdw_cdns *cdns)
init_completion(&cdns->tx_complete);
cdns->bus.port_ops = &cdns_port_ops;

+ INIT_WORK(&cdns->work, cdns_update_slave_status_work);
return 0;
}
EXPORT_SYMBOL(sdw_cdns_probe);
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
index b410656f8194..7638858397df 100644
--- a/drivers/soundwire/cadence_master.h
+++ b/drivers/soundwire/cadence_master.h
@@ -129,6 +129,10 @@ struct sdw_cdns {

bool link_up;
unsigned int msg_count;
+
+ struct work_struct work;
+
+ struct list_head list;
};

#define bus_to_cdns(_bus) container_of(_bus, struct sdw_cdns, bus)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 0a4fc7f65743..06c553d94890 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1258,21 +1258,7 @@ static int intel_master_probe(struct platform_device *pdev)
"SoundWire master %d is disabled, will be ignored\n",
bus->link_id);

- /* Acquire IRQ */
- ret = request_threaded_irq(sdw->link_res->irq,
- sdw_cdns_irq, sdw_cdns_thread,
- IRQF_SHARED, KBUILD_MODNAME, cdns);
- if (ret < 0) {
- dev_err(dev, "unable to grab IRQ %d, disabling device\n",
- sdw->link_res->irq);
- goto err_init;
- }
-
return 0;
-
-err_init:
- sdw_bus_master_delete(bus);
- return ret;
}

int intel_master_startup(struct platform_device *pdev)
@@ -1344,7 +1330,6 @@ static int intel_master_remove(struct platform_device *pdev)
if (!bus->prop.hw_disabled) {
intel_debugfs_exit(sdw);
sdw_cdns_enable_interrupt(cdns, false);
- free_irq(sdw->link_res->irq, sdw);
snd_soc_unregister_component(dev);
}
sdw_bus_master_delete(bus);
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index d6bdd4d63e08..bf127c88eb51 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -17,6 +17,8 @@
* @dev: device implementing hw_params and free callbacks
* @shim_lock: mutex to handle access to shared SHIM registers
* @shim_mask: global pointer to check SHIM register initialization
+ * @cdns: Cadence master descriptor
+ * @list: used to walk-through all masters exposed by the same controller
*/
struct sdw_intel_link_res {
struct platform_device *pdev;
@@ -29,6 +31,8 @@ struct sdw_intel_link_res {
struct device *dev;
struct mutex *shim_lock; /* protect shared registers */
u32 *shim_mask;
+ struct sdw_cdns *cdns;
+ struct list_head list;
};

struct sdw_intel {
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index ad3175272e88..63b3beda443d 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -9,6 +9,7 @@

#include <linux/acpi.h>
#include <linux/export.h>
+#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/module.h>
#include <linux/platform_device.h>
@@ -166,6 +167,19 @@ void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable)
}
EXPORT_SYMBOL_NS(sdw_intel_enable_irq, SOUNDWIRE_INTEL_INIT);

+irqreturn_t sdw_intel_thread(int irq, void *dev_id)
+{
+ struct sdw_intel_ctx *ctx = dev_id;
+ struct sdw_intel_link_res *link;
+
+ list_for_each_entry(link, &ctx->link_list, list)
+ sdw_cdns_irq(irq, link->cdns);
+
+ sdw_intel_enable_irq(ctx->mmio_base, true);
+ return IRQ_HANDLED;
+}
+EXPORT_SYMBOL(sdw_intel_thread);
+
static struct sdw_intel_ctx
*sdw_intel_probe_controller(struct sdw_intel_res *res)
{
@@ -209,6 +223,8 @@ static struct sdw_intel_ctx
link = ctx->links;
link_mask = ctx->link_mask;

+ INIT_LIST_HEAD(&ctx->link_list);
+
/* Create SDW Master devices */
for (i = 0; i < count; i++, link++) {
if (!(link_mask & BIT(i))) {
@@ -246,6 +262,9 @@ static struct sdw_intel_ctx
goto err;
}
link->pdev = pdev;
+ link->cdns = platform_get_drvdata(pdev);
+
+ list_add_tail(&link->list, &ctx->link_list);
}

return ctx;
--
2.17.1

2020-06-24 05:34:22

[permalink] [raw]

Subject: [PATCH 8/9] soundwire: intel: add wake interrupt support

From: Rander Wang <[email protected]>

When system is suspended in clock stop mode on intel platforms, both
master and slave are in clock stop mode and soundwire bus is taken
over by a glue hardware. The bus message for jack event is processed
by this glue hardware, which will trigger an interrupt to resume audio
pci device. Then audio pci driver will resume soundwire master and slave,
transfer bus ownership to master, finally slave will report jack event
to master and codec driver is triggered to check jack status.

if a slave has been attached to a bus, the slave->dev_num_sticky
should be non-zero, so we can check this value to skip the
ghost devices defined in ACPI table but not populated in hardware.

Signed-off-by: Rander Wang <[email protected]>
Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/intel.c | 48 +++++++++++++++++++++++++++++++++-
drivers/soundwire/intel.h | 1 +
drivers/soundwire/intel_init.c | 22 ++++++++++++++++
3 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 06c553d94890..22d9fd3e34fa 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -13,6 +13,7 @@
#include <linux/io.h>
#include <linux/platform_device.h>
#include <sound/pcm_params.h>
+#include <linux/pm_runtime.h>
#include <sound/soc.h>
#include <linux/soundwire/sdw_registers.h>
#include <linux/soundwire/sdw.h>
@@ -436,7 +437,7 @@ static int intel_shim_init(struct sdw_intel *sdw, bool clock_stop)
return ret;
}

-static void __maybe_unused intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
+static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
{
void __iomem *shim = sdw->link_res->shim;
unsigned int link_id = sdw->instance;
@@ -1337,6 +1338,51 @@ static int intel_master_remove(struct platform_device *pdev)
return 0;
}

+int intel_master_process_wakeen_event(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct sdw_intel *sdw;
+ struct sdw_bus *bus;
+ struct sdw_slave *slave;
+ void __iomem *shim;
+ u16 wake_sts;
+
+ sdw = platform_get_drvdata(pdev);
+ bus = &sdw->cdns.bus;
+
+ if (bus->prop.hw_disabled) {
+ dev_dbg(dev,
+ "SoundWire master %d is disabled, ignoring\n",
+ bus->link_id);
+ return 0;
+ }
+
+ shim = sdw->link_res->shim;
+ wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS);
+
+ if (!(wake_sts & BIT(sdw->instance)))
+ return 0;
+
+ /* disable WAKEEN interrupt ASAP to prevent interrupt flood */
+ intel_shim_wake(sdw, false);
+
+ /*
+ * wake up master and slave so that slave can notify master
+ * the wakeen event and let codec driver check codec status
+ */
+ list_for_each_entry(slave, &bus->slaves, node) {
+ /*
+ * discard devices that are defined in ACPI tables but
+ * not physically present and devices that cannot
+ * generate wakes
+ */
+ if (slave->dev_num_sticky && slave->prop.wake_capable)
+ pm_request_resume(&slave->dev);
+ }
+
+ return 0;
+}
+
static struct platform_driver sdw_intel_drv = {
.probe = intel_master_probe,
.remove = intel_master_remove,
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index bf127c88eb51..4ea3d262d249 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -47,5 +47,6 @@ struct sdw_intel {
#define SDW_INTEL_QUIRK_MASK_BUS_DISABLE BIT(1)

int intel_master_startup(struct platform_device *pdev);
+int intel_master_process_wakeen_event(struct platform_device *pdev);

#endif /* __SDW_INTEL_LOCAL_H */
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 63b3beda443d..eff4e385bb59 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -415,5 +415,27 @@ void sdw_intel_exit(struct sdw_intel_ctx *ctx)
}
EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);

+void sdw_intel_process_wakeen_event(struct sdw_intel_ctx *ctx)
+{
+ struct sdw_intel_link_res *link;
+ u32 link_mask;
+ int i;
+
+ if (!ctx->links)
+ return;
+
+ link = ctx->links;
+ link_mask = ctx->link_mask;
+
+ /* Startup SDW Master devices */
+ for (i = 0; i < ctx->count; i++, link++) {
+ if (!(link_mask & BIT(i)))
+ continue;
+
+ intel_master_process_wakeen_event(link->pdev);
+ }
+}
+EXPORT_SYMBOL_NS(sdw_intel_process_wakeen_event, SOUNDWIRE_INTEL_INIT);
+
MODULE_LICENSE("Dual BSD/GPL");
MODULE_DESCRIPTION("Intel Soundwire Init Library");
--
2.17.1

2020-06-30 16:26:43

[permalink] [raw]

Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads

On 24-06-20, 01:35, Bard Liao wrote:
> The existing code uses one pair of interrupt handler/thread per link
> but at the hardware level the interrupt is shared. This works fine for
> legacy PCI interrupts, but leads to timeouts in MSI (Message-Signaled
> Interrupt) mode, likely due to edges being lost.
>
> This patch unifies interrupt handling for all links. The dedicated
> handler is removed since we use a common one for all shared interrupt
> sources, and the thread function takes care of dealing with interrupt
> sources. This partition follows the model used for the SOF IPC on
> HDaudio platforms, where similar timeout issues were noticed and doing
> all the interrupt handling/clearing in the thread improved
> reliability/stability.
>
> Validation results with 4 links active in parallel show a night-and-day
> improvement with no timeouts noticed even during stress tests. Latency
> and quality of service are not affected by the change - mostly because
> events on a SoundWire link are throttled by the bus frame rate
> (typically 8..48kHz).
>
> Signed-off-by: Bard Liao <[email protected]>
> Signed-off-by: Pierre-Louis Bossart <[email protected]>
> ---
> drivers/soundwire/cadence_master.c | 18 ++++++++++--------
> drivers/soundwire/cadence_master.h | 4 ++++
> drivers/soundwire/intel.c | 15 ---------------
> drivers/soundwire/intel.h | 4 ++++
> drivers/soundwire/intel_init.c | 19 +++++++++++++++++++
> 5 files changed, 37 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index 613dbd415b91..24eafe0aa1c3 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -17,6 +17,7 @@
> #include <linux/soundwire/sdw.h>
> #include <sound/pcm_params.h>
> #include <sound/soc.h>
> +#include <linux/workqueue.h>
> #include "bus.h"
> #include "cadence_master.h"
>
> @@ -790,7 +791,7 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id)
> CDNS_MCP_INT_SLAVE_MASK, 0);
>
> int_status &= ~CDNS_MCP_INT_SLAVE_MASK;
> - ret = IRQ_WAKE_THREAD;
> + schedule_work(&cdns->work);
> }
>
> cdns_writel(cdns, CDNS_MCP_INTSTAT, int_status);
> @@ -799,13 +800,15 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id)
> EXPORT_SYMBOL(sdw_cdns_irq);
>
> /**
> - * sdw_cdns_thread() - Cadence irq thread handler
> - * @irq: irq number
> - * @dev_id: irq context
> + * To update slave status in a work since we will need to handle
> + * other interrupts eg. CDNS_MCP_INT_RX_WL during the update slave
> + * process.
> + * @work: cdns worker thread
> */
> -irqreturn_t sdw_cdns_thread(int irq, void *dev_id)
> +static void cdns_update_slave_status_work(struct work_struct *work)
> {
> - struct sdw_cdns *cdns = dev_id;
> + struct sdw_cdns *cdns =
> + container_of(work, struct sdw_cdns, work);
> u32 slave0, slave1;
>
> dev_dbg_ratelimited(cdns->dev, "Slave status change\n");
> @@ -822,9 +825,7 @@ irqreturn_t sdw_cdns_thread(int irq, void *dev_id)
> cdns_updatel(cdns, CDNS_MCP_INTMASK,
> CDNS_MCP_INT_SLAVE_MASK, CDNS_MCP_INT_SLAVE_MASK);
>
> - return IRQ_HANDLED;
> }
> -EXPORT_SYMBOL(sdw_cdns_thread);
>
> /*
> * init routines
> @@ -1427,6 +1428,7 @@ int sdw_cdns_probe(struct sdw_cdns *cdns)
> init_completion(&cdns->tx_complete);
> cdns->bus.port_ops = &cdns_port_ops;
>
> + INIT_WORK(&cdns->work, cdns_update_slave_status_work);
> return 0;
> }
> EXPORT_SYMBOL(sdw_cdns_probe);
> diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
> index b410656f8194..7638858397df 100644
> --- a/drivers/soundwire/cadence_master.h
> +++ b/drivers/soundwire/cadence_master.h
> @@ -129,6 +129,10 @@ struct sdw_cdns {
>
> bool link_up;
> unsigned int msg_count;
> +
> + struct work_struct work;
> +
> + struct list_head list;
> };
>
> #define bus_to_cdns(_bus) container_of(_bus, struct sdw_cdns, bus)
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 0a4fc7f65743..06c553d94890 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1258,21 +1258,7 @@ static int intel_master_probe(struct platform_device *pdev)
> "SoundWire master %d is disabled, will be ignored\n",
> bus->link_id);
>
> - /* Acquire IRQ */
> - ret = request_threaded_irq(sdw->link_res->irq,
> - sdw_cdns_irq, sdw_cdns_thread,
> - IRQF_SHARED, KBUILD_MODNAME, cdns);
> - if (ret < 0) {
> - dev_err(dev, "unable to grab IRQ %d, disabling device\n",
> - sdw->link_res->irq);
> - goto err_init;
> - }
> -
> return 0;
> -
> -err_init:
> - sdw_bus_master_delete(bus);
> - return ret;
> }
>
> int intel_master_startup(struct platform_device *pdev)
> @@ -1344,7 +1330,6 @@ static int intel_master_remove(struct platform_device *pdev)
> if (!bus->prop.hw_disabled) {
> intel_debugfs_exit(sdw);
> sdw_cdns_enable_interrupt(cdns, false);
> - free_irq(sdw->link_res->irq, sdw);
> snd_soc_unregister_component(dev);
> }
> sdw_bus_master_delete(bus);
> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
> index d6bdd4d63e08..bf127c88eb51 100644
> --- a/drivers/soundwire/intel.h
> +++ b/drivers/soundwire/intel.h
> @@ -17,6 +17,8 @@
> * @dev: device implementing hw_params and free callbacks
> * @shim_lock: mutex to handle access to shared SHIM registers
> * @shim_mask: global pointer to check SHIM register initialization
> + * @cdns: Cadence master descriptor
> + * @list: used to walk-through all masters exposed by the same controller
> */
> struct sdw_intel_link_res {
> struct platform_device *pdev;
> @@ -29,6 +31,8 @@ struct sdw_intel_link_res {
> struct device *dev;
> struct mutex *shim_lock; /* protect shared registers */
> u32 *shim_mask;
> + struct sdw_cdns *cdns;
> + struct list_head list;
> };
>
> struct sdw_intel {
> diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
> index ad3175272e88..63b3beda443d 100644
> --- a/drivers/soundwire/intel_init.c
> +++ b/drivers/soundwire/intel_init.c
> @@ -9,6 +9,7 @@
>
> #include <linux/acpi.h>
> #include <linux/export.h>
> +#include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> @@ -166,6 +167,19 @@ void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable)
> }
> EXPORT_SYMBOL_NS(sdw_intel_enable_irq, SOUNDWIRE_INTEL_INIT);
>
> +irqreturn_t sdw_intel_thread(int irq, void *dev_id)
> +{
> + struct sdw_intel_ctx *ctx = dev_id;
> + struct sdw_intel_link_res *link;
> +
> + list_for_each_entry(link, &ctx->link_list, list)
> + sdw_cdns_irq(irq, link->cdns);
> +
> + sdw_intel_enable_irq(ctx->mmio_base, true);
> + return IRQ_HANDLED;
> +}
> +EXPORT_SYMBOL(sdw_intel_thread);

Who will call this API? Also don't see header for this..
Is this called from irq context or irq thread or something else?

Also no EXPORT_SYMBOL_NS() for this one?

--
~Vinod

2020-06-30 18:42:02

[permalink] [raw]

Subject: Re: [PATCH 8/9] soundwire: intel: add wake interrupt support

On 24-06-20, 01:35, Bard Liao wrote:
> From: Rander Wang <[email protected]>
>
> When system is suspended in clock stop mode on intel platforms, both
> master and slave are in clock stop mode and soundwire bus is taken
> over by a glue hardware. The bus message for jack event is processed
> by this glue hardware, which will trigger an interrupt to resume audio
> pci device. Then audio pci driver will resume soundwire master and slave,
> transfer bus ownership to master, finally slave will report jack event
> to master and codec driver is triggered to check jack status.
>
> if a slave has been attached to a bus, the slave->dev_num_sticky
> should be non-zero, so we can check this value to skip the
> ghost devices defined in ACPI table but not populated in hardware.
>
> Signed-off-by: Rander Wang <[email protected]>
> Signed-off-by: Pierre-Louis Bossart <[email protected]>
> Signed-off-by: Bard Liao <[email protected]>
> ---
> drivers/soundwire/intel.c | 48 +++++++++++++++++++++++++++++++++-
> drivers/soundwire/intel.h | 1 +
> drivers/soundwire/intel_init.c | 22 ++++++++++++++++
> 3 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 06c553d94890..22d9fd3e34fa 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -13,6 +13,7 @@
> #include <linux/io.h>
> #include <linux/platform_device.h>
> #include <sound/pcm_params.h>
> +#include <linux/pm_runtime.h>
> #include <sound/soc.h>
> #include <linux/soundwire/sdw_registers.h>
> #include <linux/soundwire/sdw.h>
> @@ -436,7 +437,7 @@ static int intel_shim_init(struct sdw_intel *sdw, bool clock_stop)
> return ret;
> }
>
> -static void __maybe_unused intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
> +static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)

why drop __maybe?

> {
> void __iomem *shim = sdw->link_res->shim;
> unsigned int link_id = sdw->instance;
> @@ -1337,6 +1338,51 @@ static int intel_master_remove(struct platform_device *pdev)
> return 0;
> }
>
> +int intel_master_process_wakeen_event(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct sdw_intel *sdw;
> + struct sdw_bus *bus;
> + struct sdw_slave *slave;
> + void __iomem *shim;
> + u16 wake_sts;
> +
> + sdw = platform_get_drvdata(pdev);
> + bus = &sdw->cdns.bus;
> +
> + if (bus->prop.hw_disabled) {
> + dev_dbg(dev,
> + "SoundWire master %d is disabled, ignoring\n",
> + bus->link_id);

single line pls

> + return 0;
> + }
> +
> + shim = sdw->link_res->shim;
> + wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS);
> +
> + if (!(wake_sts & BIT(sdw->instance)))
> + return 0;
> +
> + /* disable WAKEEN interrupt ASAP to prevent interrupt flood */
> + intel_shim_wake(sdw, false);

when & where is this enabled?

> +
> + /*
> + * wake up master and slave so that slave can notify master
> + * the wakeen event and let codec driver check codec status
> + */
> + list_for_each_entry(slave, &bus->slaves, node) {
> + /*
> + * discard devices that are defined in ACPI tables but
> + * not physically present and devices that cannot
> + * generate wakes
> + */
> + if (slave->dev_num_sticky && slave->prop.wake_capable)
> + pm_request_resume(&slave->dev);

Hmmm, shouldn't slave do this? would it not make sense to notify the
slave thru callback and then slave decides to resume or not..?

--
~Vinod

2020-06-30 18:55:34

[permalink] [raw]

Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads

>> +irqreturn_t sdw_intel_thread(int irq, void *dev_id)
>> +{
>> + struct sdw_intel_ctx *ctx = dev_id;
>> + struct sdw_intel_link_res *link;
>> +
>> + list_for_each_entry(link, &ctx->link_list, list)
>> + sdw_cdns_irq(irq, link->cdns);
>> +
>> + sdw_intel_enable_irq(ctx->mmio_base, true);
>> + return IRQ_HANDLED;
>> +}
>> +EXPORT_SYMBOL(sdw_intel_thread);
>
> Who will call this API? Also don't see header for this..

the header was merged 6 months ago:

6cd1d670bee6 soundwire: intel: update headers for interrupts

This function is called by the SOF driver:

static irqreturn_t hda_dsp_sdw_thread(int irq, void *context)
{
return sdw_intel_thread(irq, context);
}

the SOF driver implements a fallback when CONFIG_SOUNDWIRE is not defined.

static inline irqreturn_t hda_dsp_sdw_thread(int irq, void *context)
{
return IRQ_HANDLED;
}

> Is this called from irq context or irq thread or something else?

from IRQ thread, hence the name, see pointers above.

The key part is that we could only make the hardware work as intended by
using a single thread for all interrupt sources, and that patch is just
the generalization of what was implemented for HDaudio in mid-2019 after
months of lost interrupts and IPC errors. See below the code from
sound/soc/sof/intel/hda.c for interrupt handling.

static irqreturn_t hda_dsp_interrupt_handler(int irq, void *context)
{
struct snd_sof_dev *sdev = context;

/*
* Get global interrupt status. It includes all hardware interrupt
* sources in the Intel HD Audio controller.
*/
if (snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTSTS) &
SOF_HDA_INTSTS_GIS) {

/* disable GIE interrupt */
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
SOF_HDA_INTCTL,
SOF_HDA_INT_GLOBAL_EN,
0);

return IRQ_WAKE_THREAD;
}

return IRQ_NONE;
}

static irqreturn_t hda_dsp_interrupt_thread(int irq, void *context)
{
struct snd_sof_dev *sdev = context;
struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata;

/* deal with streams and controller first */
if (hda_dsp_check_stream_irq(sdev))
hda_dsp_stream_threaded_handler(irq, sdev);

if (hda_dsp_check_ipc_irq(sdev))
sof_ops(sdev)->irq_thread(irq, sdev);

if (hda_dsp_check_sdw_irq(sdev))
hda_dsp_sdw_thread(irq, hdev->sdw);

if (hda_sdw_check_wakeen_irq(sdev))
hda_sdw_process_wakeen(sdev);

/* enable GIE interrupt */
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
SOF_HDA_INTCTL,
SOF_HDA_INT_GLOBAL_EN,
SOF_HDA_INT_GLOBAL_EN);

return IRQ_HANDLED;
}

> Also no EXPORT_SYMBOL_NS() for this one?

Good catch, it's a miss, all the exported functions should use a NS:

EXPORT_SYMBOL_NS(sdw_intel_enable_irq, SOUNDWIRE_INTEL_INIT);
EXPORT_SYMBOL(sdw_intel_thread);
EXPORT_SYMBOL_NS(sdw_intel_acpi_scan, SOUNDWIRE_INTEL_INIT);
EXPORT_SYMBOL_NS(sdw_intel_probe, SOUNDWIRE_INTEL_INIT);
EXPORT_SYMBOL_NS(sdw_intel_startup, SOUNDWIRE_INTEL_INIT);
EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);
EXPORT_SYMBOL_NS(sdw_intel_process_wakeen_event, SOUNDWIRE_INTEL_INIT);

2020-06-30 18:57:52

[permalink] [raw]

Subject: Re: [PATCH 8/9] soundwire: intel: add wake interrupt support

On 6/30/20 11:51 AM, Vinod Koul wrote:
> On 24-06-20, 01:35, Bard Liao wrote:
>> From: Rander Wang <[email protected]>
>>
>> When system is suspended in clock stop mode on intel platforms, both
>> master and slave are in clock stop mode and soundwire bus is taken
>> over by a glue hardware. The bus message for jack event is processed
>> by this glue hardware, which will trigger an interrupt to resume audio
>> pci device. Then audio pci driver will resume soundwire master and slave,
>> transfer bus ownership to master, finally slave will report jack event
>> to master and codec driver is triggered to check jack status.
>>
>> if a slave has been attached to a bus, the slave->dev_num_sticky
>> should be non-zero, so we can check this value to skip the
>> ghost devices defined in ACPI table but not populated in hardware.
>>
>> Signed-off-by: Rander Wang <[email protected]>
>> Signed-off-by: Pierre-Louis Bossart <[email protected]>
>> Signed-off-by: Bard Liao <[email protected]>
>> ---
>> drivers/soundwire/intel.c | 48 +++++++++++++++++++++++++++++++++-
>> drivers/soundwire/intel.h | 1 +
>> drivers/soundwire/intel_init.c | 22 ++++++++++++++++
>> 3 files changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
>> index 06c553d94890..22d9fd3e34fa 100644
>> --- a/drivers/soundwire/intel.c
>> +++ b/drivers/soundwire/intel.c
>> @@ -13,6 +13,7 @@
>> #include <linux/io.h>
>> #include <linux/platform_device.h>
>> #include <sound/pcm_params.h>
>> +#include <linux/pm_runtime.h>
>> #include <sound/soc.h>
>> #include <linux/soundwire/sdw_registers.h>
>> #include <linux/soundwire/sdw.h>
>> @@ -436,7 +437,7 @@ static int intel_shim_init(struct sdw_intel *sdw, bool clock_stop)
>> return ret;
>> }
>>
>> -static void __maybe_unused intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
>> +static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
>
> why drop __maybe?

the __maybe was used in previous patches to avoid throwing 'defined but
not used' errors.

In this patch this function is actually used so there's no longer a
reason to keep it.

>> {
>> void __iomem *shim = sdw->link_res->shim;
>> unsigned int link_id = sdw->instance;
>> @@ -1337,6 +1338,51 @@ static int intel_master_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +int intel_master_process_wakeen_event(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct sdw_intel *sdw;
>> + struct sdw_bus *bus;
>> + struct sdw_slave *slave;
>> + void __iomem *shim;
>> + u16 wake_sts;
>> +
>> + sdw = platform_get_drvdata(pdev);
>> + bus = &sdw->cdns.bus;
>> +
>> + if (bus->prop.hw_disabled) {
>> + dev_dbg(dev,
>> + "SoundWire master %d is disabled, ignoring\n",
>> + bus->link_id);
>
> single line pls

ok

>> + return 0;
>> + }
>> +
>> + shim = sdw->link_res->shim;
>> + wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS);
>> +
>> + if (!(wake_sts & BIT(sdw->instance)))
>> + return 0;
>> +
>> + /* disable WAKEEN interrupt ASAP to prevent interrupt flood */
>> + intel_shim_wake(sdw, false);
>
> when & where is this enabled?

in follow-up patches where the clock-stop mode is enabled.

} else if (clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET ||
!clock_stop_quirks) {

ret = sdw_cdns_clock_stop(cdns, true);
if (ret < 0) {
dev_err(dev, "cannot enable clock stop on suspend\n");
return ret;
}

ret = sdw_cdns_enable_interrupt(cdns, false);
if (ret < 0) {
dev_err(dev, "cannot disable interrupts on suspend\n");
return ret;
}

ret = intel_link_power_down(sdw);
if (ret) {
dev_err(dev, "Link power down failed: %d", ret);
return ret;
}

intel_shim_wake(sdw, true);

>> +
>> + /*
>> + * wake up master and slave so that slave can notify master
>> + * the wakeen event and let codec driver check codec status
>> + */
>> + list_for_each_entry(slave, &bus->slaves, node) {
>> + /*
>> + * discard devices that are defined in ACPI tables but
>> + * not physically present and devices that cannot
>> + * generate wakes
>> + */
>> + if (slave->dev_num_sticky && slave->prop.wake_capable)
>> + pm_request_resume(&slave->dev);
>
> Hmmm, shouldn't slave do this? would it not make sense to notify the
> slave thru callback and then slave decides to resume or not..?

In this mode, the bus is clock-stop mode, and events are detected with
level detector tied to PCI events. The master and slave devices are all
in pm_runtime suspended states. The codec cannot make any decisions on
its own since the bus is stopped, it needs to first resume, which
assumes that the master resumes first and the enumeration re-done before
it can access any of its registers.

By looping through the list of devices that can generate events, you
end-up first forcing the master to resume, and then each slave resumes
and can check who generated the event and what happened while suspended.
if the codec didn't generate the event it will go back to suspended mode
after the usual timeout.

We can add a callback but that callback would only be used for Intel
solutions, but internally it would only do a pm_request_resume() since
the codec cannot make any decisions before first resuming. In other
words, it would be an Intel-specific callback that is implemented using
generic resume operations. It's probably better to keep this in
Intel-specific code, no?

2020-07-01 05:43:39

[permalink] [raw]

Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads

On 30-06-20, 11:46, Pierre-Louis Bossart wrote:

> > Is this called from irq context or irq thread or something else?
>
> from IRQ thread, hence the name, see pointers above.
>
> The key part is that we could only make the hardware work as intended by
> using a single thread for all interrupt sources, and that patch is just the
> generalization of what was implemented for HDaudio in mid-2019 after months
> of lost interrupts and IPC errors. See below the code from
> sound/soc/sof/intel/hda.c for interrupt handling.

Sounds good. Now that you are already in irq thread, does it make sense
to spawn a worker thread for this and handle it there? Why not do in the
irq thread itself. Using a thread kind of defeats the whole point behind
concept of irq threads

--
~Vinod

2020-07-01 05:59:26

[permalink] [raw]

Subject: Re: [PATCH 8/9] soundwire: intel: add wake interrupt support

On 30-06-20, 12:18, Pierre-Louis Bossart wrote:
> > > + return 0;
> > > + }
> > > +
> > > + shim = sdw->link_res->shim;
> > > + wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS);
> > > +
> > > + if (!(wake_sts & BIT(sdw->instance)))
> > > + return 0;
> > > +
> > > + /* disable WAKEEN interrupt ASAP to prevent interrupt flood */
> > > + intel_shim_wake(sdw, false);
> >
> > when & where is this enabled?
>
> in follow-up patches where the clock-stop mode is enabled.

ok

> > > + * wake up master and slave so that slave can notify master
> > > + * the wakeen event and let codec driver check codec status
> > > + */
> > > + list_for_each_entry(slave, &bus->slaves, node) {
> > > + /*
> > > + * discard devices that are defined in ACPI tables but
> > > + * not physically present and devices that cannot
> > > + * generate wakes
> > > + */
> > > + if (slave->dev_num_sticky && slave->prop.wake_capable)
> > > + pm_request_resume(&slave->dev);
> >
> > Hmmm, shouldn't slave do this? would it not make sense to notify the
> > slave thru callback and then slave decides to resume or not..?
>
> In this mode, the bus is clock-stop mode, and events are detected with level
> detector tied to PCI events. The master and slave devices are all in
> pm_runtime suspended states. The codec cannot make any decisions on its own
> since the bus is stopped, it needs to first resume, which assumes that the
> master resumes first and the enumeration re-done before it can access any of
> its registers.
>
> By looping through the list of devices that can generate events, you end-up
> first forcing the master to resume, and then each slave resumes and can
> check who generated the event and what happened while suspended. if the
> codec didn't generate the event it will go back to suspended mode after the
> usual timeout.
>
> We can add a callback but that callback would only be used for Intel
> solutions, but internally it would only do a pm_request_resume() since the
> codec cannot make any decisions before first resuming. In other words, it
> would be an Intel-specific callback that is implemented using generic resume
> operations. It's probably better to keep this in Intel-specific code, no?

I do not like the idea that a device would be woken up, that kind of
defeats the whole idea behind the runtime pm. Waking up a device to
check the events is a generic sdw concept, I don't see that as Intel
specific one.

I would like to see a generic callback for the devices and let devices
do the resume part, that is standard operating principle when we deal
with suspended devices. If the device thinks they need to resume, they
will do the runtime resume, check the status and sleep if not
applicable. Since we have set the parents correctly, any resume
operation for slaves would wake master up as well...

I do not see a need for intel driver to resume slave devices here, or
did I miss something?

--
~Vinod

2020-07-01 16:54:24

[permalink] [raw]

Subject: Re: [PATCH 8/9] soundwire: intel: add wake interrupt support

>>>> + * wake up master and slave so that slave can notify master
>>>> + * the wakeen event and let codec driver check codec status
>>>> + */
>>>> + list_for_each_entry(slave, &bus->slaves, node) {
>>>> + /*
>>>> + * discard devices that are defined in ACPI tables but
>>>> + * not physically present and devices that cannot
>>>> + * generate wakes
>>>> + */
>>>> + if (slave->dev_num_sticky && slave->prop.wake_capable)
>>>> + pm_request_resume(&slave->dev);
>>>
>>> Hmmm, shouldn't slave do this? would it not make sense to notify the
>>> slave thru callback and then slave decides to resume or not..?
>>
>> In this mode, the bus is clock-stop mode, and events are detected with level
>> detector tied to PCI events. The master and slave devices are all in
>> pm_runtime suspended states. The codec cannot make any decisions on its own
>> since the bus is stopped, it needs to first resume, which assumes that the
>> master resumes first and the enumeration re-done before it can access any of
>> its registers.
>>
>> By looping through the list of devices that can generate events, you end-up
>> first forcing the master to resume, and then each slave resumes and can
>> check who generated the event and what happened while suspended. if the
>> codec didn't generate the event it will go back to suspended mode after the
>> usual timeout.
>>
>> We can add a callback but that callback would only be used for Intel
>> solutions, but internally it would only do a pm_request_resume() since the
>> codec cannot make any decisions before first resuming. In other words, it
>> would be an Intel-specific callback that is implemented using generic resume
>> operations. It's probably better to keep this in Intel-specific code, no?
>
> I do not like the idea that a device would be woken up, that kind of
> defeats the whole idea behind the runtime pm. Waking up a device to
> check the events is a generic sdw concept, I don't see that as Intel
> specific one.

In this case, this in an Intel-specific mode that's beyond what MIPI
defined. This is not the traditional in-band SoundWire wake defined in
the SoundWire specification. The bus is completely down, the master IP
is powered-off and all context lost. There is still the ability for a
Slave device to throw a wake as defined by MIPI in clock-stop-mode1, but
this is handled with a separate level detector and the wake detection is
handled by the PCI subsystem. On a wake, the master IP needs to be
powered-up, the entire bus needs to be restarted and the Slave devices
re-enumerated.

We trigger that sequence with a loop that calls pm_request_resume() for
all Slave devices that are present and exposed in their properties that
they are wake-capable. As you rightly said in your comments, this will
result a nice wake-up handled by the pm_runtime framework in the right
sequence (DSP first, then SoundWire master then Slave devices).

You will see in follow-up patches that the master IP can be configured
in different clock stop modes, depending on the needs (power/latency
compromise typically). When the more traditional SoundWire wake is used,
we do not use this sequence at all.

> I would like to see a generic callback for the devices and let devices
> do the resume part, that is standard operating principle when we deal
> with suspended devices. If the device thinks they need to resume, they
> will do the runtime resume, check the status and sleep if not
> applicable. Since we have set the parents correctly, any resume
> operation for slaves would wake master up as well...
>
> I do not see a need for intel driver to resume slave devices here, or
> did I miss something?

Yes, the part "If the device thinks they need to resume" is not quite
right. The Slave device cannot access its registers before fully
resuming, which in this case means a re-enumeration, so cannot "think"
or make a decision on its own. That's the reason why we force them to
resume, since it's the first step they would need to do anyways, and
then if they don't have anything to do they go back to idle after a
timeout. I invite you to see the suspend/resume sequences in codec
drivers where you will see regmap access moves to cache-only on suspend
and full access restored on resume, along with a hardware sync.

I see your point and I really think we are talking about a similar
sequence, but we simplified your idea since there's no possibility of
making a decision before Slave devices resume.

The only optimization we did here is that we only resume Slave devices
than can generate a wake, and filter out the 'ghost' devices that are
described in ACPI tables but don't physically exist.

Does this help?

2020-07-02 07:38:18

by Liao, Bard

[permalink] [raw]

Subject: RE: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads

> -----Original Message-----
> From: Vinod Koul <[email protected]>
> Sent: Wednesday, July 1, 2020 1:42 PM
> To: Pierre-Louis Bossart <[email protected]>
> Cc: Bard Liao <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Lin,
> Mengdong <[email protected]>; Blauciak, Slawomir
> <[email protected]>; Kale, Sanyog R <[email protected]>;
> [email protected]; Liao, Bard <[email protected]>
> Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt
> handlers/threads
>
> On 30-06-20, 11:46, Pierre-Louis Bossart wrote:
>
> > > Is this called from irq context or irq thread or something else?
> >
> > from IRQ thread, hence the name, see pointers above.
> >
> > The key part is that we could only make the hardware work as intended by
> > using a single thread for all interrupt sources, and that patch is just the
> > generalization of what was implemented for HDaudio in mid-2019 after
> months
> > of lost interrupts and IPC errors. See below the code from
> > sound/soc/sof/intel/hda.c for interrupt handling.
>
> Sounds good. Now that you are already in irq thread, does it make sense
> to spawn a worker thread for this and handle it there? Why not do in the
> irq thread itself. Using a thread kind of defeats the whole point behind
> concept of irq threads

Not sure If you are talking about cdns_update_slave_status_work().
The reason we need to spawn a worker thread in sdw_cdns_irq() is
that we will do sdw transfer which will generate an interrupt when
a slave interrupt is triggered. And the handler will not be invoked if the
previous handler is not return yet.
Please see the scenario below for better explanation.
1. Slave interrupt arrives
2.1 Try to read Slave register and waiting for the transfer response
2.2 Get the transfer response interrupt and finish the sdw transfer.
3. Finish the Slave interrupt handling.

Interrupts are triggered in step 1 and 2.2, but step 2.2's handler will not be
invoked if step 1's handler is not return yet.
What we do is to spawn a worker thread to do step 2 and return from step 1.
So the handler can be invoked when the transfer response interrupt arrives.

>
> --
> ~Vinod

2020-07-02 15:03:41

[permalink] [raw]

Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads

On 7/2/20 2:35 AM, Liao, Bard wrote:
>> -----Original Message-----
>> From: Vinod Koul <[email protected]>
>> Sent: Wednesday, July 1, 2020 1:42 PM
>> To: Pierre-Louis Bossart <[email protected]>
>> Cc: Bard Liao <[email protected]>; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected]; Lin,
>> Mengdong <[email protected]>; Blauciak, Slawomir
>> <[email protected]>; Kale, Sanyog R <[email protected]>;
>> [email protected]; Liao, Bard <[email protected]>
>> Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt
>> handlers/threads
>>
>> On 30-06-20, 11:46, Pierre-Louis Bossart wrote:
>>
>>>> Is this called from irq context or irq thread or something else?
>>>
>>> from IRQ thread, hence the name, see pointers above.
>>>
>>> The key part is that we could only make the hardware work as intended by
>>> using a single thread for all interrupt sources, and that patch is just the
>>> generalization of what was implemented for HDaudio in mid-2019 after
>> months
>>> of lost interrupts and IPC errors. See below the code from
>>> sound/soc/sof/intel/hda.c for interrupt handling.
>>
>> Sounds good. Now that you are already in irq thread, does it make sense
>> to spawn a worker thread for this and handle it there? Why not do in the
>> irq thread itself. Using a thread kind of defeats the whole point behind
>> concept of irq threads
>
> Not sure If you are talking about cdns_update_slave_status_work().
> The reason we need to spawn a worker thread in sdw_cdns_irq() is
> that we will do sdw transfer which will generate an interrupt when
> a slave interrupt is triggered. And the handler will not be invoked if the
> previous handler is not return yet.
> Please see the scenario below for better explanation.
> 1. Slave interrupt arrives
> 2.1 Try to read Slave register and waiting for the transfer response
> 2.2 Get the transfer response interrupt and finish the sdw transfer.
> 3. Finish the Slave interrupt handling.
>
> Interrupts are triggered in step 1 and 2.2, but step 2.2's handler will not be
> invoked if step 1's handler is not return yet.
> What we do is to spawn a worker thread to do step 2 and return from step 1.
> So the handler can be invoked when the transfer response interrupt arrives.

To build on Bard's correct answer, the irq thread only takes care of
'immediate' actions, such as command completion, parity or bus clash
errors. The rest of the work can be split in
a) changes to device state, usually for attachment and enumeration. This
is rather slow and will entail regmap syncs.
b) device interrupts - typically only for jack detection which is also
rather slow.

Since this irq thread function is actually part of the entire HDaudio
controller interrupt handling, we have to defer the work for cases a)
and b) and re-enable the HDaudio interrupts at the end of the irq thread
function - see the code I shared earlier.

In addition, both a) and b) will result in transactions over the bus,
which will trigger interrupts to signal the command completions. In
other words, because of the asynchronous nature of the transactions, we
need a two-level implementation. If you look at the previous solution it
was the same, the commands were issued in the irq thread and the command
completion was handled in the handler, since we had to make the handler
minimal with a global GIE interrupt disable we kept the same hierarchy
to deal with commands but move it up one level.

You could argue that maybe a worker thread is not optimal and could be
replaced by something better/faster. Since the jack detection is
typically handled with a worker thread in all ASoC codec drivers, we
didn't feel the need to optimize further. We did not see any performance
impact with this change.

Does this answer to your concern?

2020-07-15 05:55:48

[permalink] [raw]

Subject: Re: [PATCH 8/9] soundwire: intel: add wake interrupt support

On 01-07-20, 10:25, Pierre-Louis Bossart wrote:
>
> > > > > + * wake up master and slave so that slave can notify master
> > > > > + * the wakeen event and let codec driver check codec status
> > > > > + */
> > > > > + list_for_each_entry(slave, &bus->slaves, node) {
> > > > > + /*
> > > > > + * discard devices that are defined in ACPI tables but
> > > > > + * not physically present and devices that cannot
> > > > > + * generate wakes
> > > > > + */
> > > > > + if (slave->dev_num_sticky && slave->prop.wake_capable)
> > > > > + pm_request_resume(&slave->dev);
> > > >
> > > > Hmmm, shouldn't slave do this? would it not make sense to notify the
> > > > slave thru callback and then slave decides to resume or not..?
> > >
> > > In this mode, the bus is clock-stop mode, and events are detected with level
> > > detector tied to PCI events. The master and slave devices are all in
> > > pm_runtime suspended states. The codec cannot make any decisions on its own
> > > since the bus is stopped, it needs to first resume, which assumes that the
> > > master resumes first and the enumeration re-done before it can access any of
> > > its registers.
> > >
> > > By looping through the list of devices that can generate events, you end-up
> > > first forcing the master to resume, and then each slave resumes and can
> > > check who generated the event and what happened while suspended. if the
> > > codec didn't generate the event it will go back to suspended mode after the
> > > usual timeout.
> > >
> > > We can add a callback but that callback would only be used for Intel
> > > solutions, but internally it would only do a pm_request_resume() since the
> > > codec cannot make any decisions before first resuming. In other words, it
> > > would be an Intel-specific callback that is implemented using generic resume
> > > operations. It's probably better to keep this in Intel-specific code, no?
> >
> > I do not like the idea that a device would be woken up, that kind of
> > defeats the whole idea behind the runtime pm. Waking up a device to
> > check the events is a generic sdw concept, I don't see that as Intel
> > specific one.
>
> In this case, this in an Intel-specific mode that's beyond what MIPI
> defined. This is not the traditional in-band SoundWire wake defined in the
> SoundWire specification. The bus is completely down, the master IP is
> powered-off and all context lost. There is still the ability for a Slave
> device to throw a wake as defined by MIPI in clock-stop-mode1, but this is
> handled with a separate level detector and the wake detection is handled by
> the PCI subsystem. On a wake, the master IP needs to be powered-up, the
> entire bus needs to be restarted and the Slave devices re-enumerated.

Right and I would expect that Slave device would do runtime_get_sync()
first thing in the callback. That would ensure that the Master is
powered up, Slave is powered up, all enumeration is complete. This is
more standard way to deal with this, we expect devices to be so we
low powered or off so cannot do read/write unless we resume.

> We trigger that sequence with a loop that calls pm_request_resume() for all
> Slave devices that are present and exposed in their properties that they are
> wake-capable. As you rightly said in your comments, this will result a nice
> wake-up handled by the pm_runtime framework in the right sequence (DSP
> first, then SoundWire master then Slave devices).
>
> You will see in follow-up patches that the master IP can be configured in
> different clock stop modes, depending on the needs (power/latency compromise
> typically). When the more traditional SoundWire wake is used, we do not use
> this sequence at all.

The point which is not clear to me if why do we need a specific
sequence. Above sequence should work for you, if not I would like to
understand why.

> > I would like to see a generic callback for the devices and let devices
> > do the resume part, that is standard operating principle when we deal
> > with suspended devices. If the device thinks they need to resume, they
> > will do the runtime resume, check the status and sleep if not
> > applicable. Since we have set the parents correctly, any resume
> > operation for slaves would wake master up as well...
> >
> > I do not see a need for intel driver to resume slave devices here, or
> > did I miss something?
>
> Yes, the part "If the device thinks they need to resume" is not quite right.
> The Slave device cannot access its registers before fully resuming, which in
> this case means a re-enumeration, so cannot "think" or make a decision on
> its own. That's the reason why we force them to resume, since it's the first
> step they would need to do anyways, and then if they don't have anything to
> do they go back to idle after a timeout. I invite you to see the
> suspend/resume sequences in codec drivers where you will see regmap access
> moves to cache-only on suspend and full access restored on resume, along
> with a hardware sync.
>
> I see your point and I really think we are talking about a similar sequence,
> but we simplified your idea since there's no possibility of making a
> decision before Slave devices resume.

If we are worried about Slave device remembering then we should save
state in device context. But i think that slave should *always* perform
runtime resume as a first step in the callback before they would do any
bus/device ops. After all, the callback is wake from low power state

> The only optimization we did here is that we only resume Slave devices than
> can generate a wake, and filter out the 'ghost' devices that are described
> in ACPI tables but don't physically exist.
>
> Does this help?
>

--
~Vinod

2020-07-15 05:56:00

[permalink] [raw]

Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads

On 02-07-20, 10:01, Pierre-Louis Bossart wrote:

> > > Sounds good. Now that you are already in irq thread, does it make sense
> > > to spawn a worker thread for this and handle it there? Why not do in the
> > > irq thread itself. Using a thread kind of defeats the whole point behind
> > > concept of irq threads
> >
> > Not sure If you are talking about cdns_update_slave_status_work().
> > The reason we need to spawn a worker thread in sdw_cdns_irq() is
> > that we will do sdw transfer which will generate an interrupt when
> > a slave interrupt is triggered. And the handler will not be invoked if the
> > previous handler is not return yet.
> > Please see the scenario below for better explanation.
> > 1. Slave interrupt arrives
> > 2.1 Try to read Slave register and waiting for the transfer response
> > 2.2 Get the transfer response interrupt and finish the sdw transfer.
> > 3. Finish the Slave interrupt handling.
> >
> > Interrupts are triggered in step 1 and 2.2, but step 2.2's handler will not be
> > invoked if step 1's handler is not return yet.
> > What we do is to spawn a worker thread to do step 2 and return from step 1.
> > So the handler can be invoked when the transfer response interrupt arrives.
>
> To build on Bard's correct answer, the irq thread only takes care of
> 'immediate' actions, such as command completion, parity or bus clash errors.
> The rest of the work can be split in
> a) changes to device state, usually for attachment and enumeration. This is
> rather slow and will entail regmap syncs.
> b) device interrupts - typically only for jack detection which is also
> rather slow.
>
> Since this irq thread function is actually part of the entire HDaudio
> controller interrupt handling, we have to defer the work for cases a) and b)
> and re-enable the HDaudio interrupts at the end of the irq thread function -
> see the code I shared earlier.
>
> In addition, both a) and b) will result in transactions over the bus, which
> will trigger interrupts to signal the command completions. In other words,
> because of the asynchronous nature of the transactions, we need a two-level
> implementation. If you look at the previous solution it was the same, the
> commands were issued in the irq thread and the command completion was
> handled in the handler, since we had to make the handler minimal with a
> global GIE interrupt disable we kept the same hierarchy to deal with
> commands but move it up one level.
>
> You could argue that maybe a worker thread is not optimal and could be
> replaced by something better/faster. Since the jack detection is typically
> handled with a worker thread in all ASoC codec drivers, we didn't feel the
> need to optimize further. We did not see any performance impact with this
> change.
>
> Does this answer to your concern?

The point is that we are already in irq_thread which is designed to
handle any bottom half processing and can be given priority, spawning of
worker threads for another bottom half seems unnecessary to me and would
increase the latency for you.

I would have handled everything in irq_thread and returned, after all we
are in bottom half :)

Is there a reason for worker thread here, if so it is not clear to me
atm.

Thanks
--
~Vinod

2020-07-15 15:09:11

[permalink] [raw]

Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads

On 7/14/20 11:54 PM, Vinod Koul wrote:
> On 02-07-20, 10:01, Pierre-Louis Bossart wrote:
>
>>>> Sounds good. Now that you are already in irq thread, does it make sense
>>>> to spawn a worker thread for this and handle it there? Why not do in the
>>>> irq thread itself. Using a thread kind of defeats the whole point behind
>>>> concept of irq threads
>>>
>>> Not sure If you are talking about cdns_update_slave_status_work().
>>> The reason we need to spawn a worker thread in sdw_cdns_irq() is
>>> that we will do sdw transfer which will generate an interrupt when
>>> a slave interrupt is triggered. And the handler will not be invoked if the
>>> previous handler is not return yet.
>>> Please see the scenario below for better explanation.
>>> 1. Slave interrupt arrives
>>> 2.1 Try to read Slave register and waiting for the transfer response
>>> 2.2 Get the transfer response interrupt and finish the sdw transfer.
>>> 3. Finish the Slave interrupt handling.
>>>
>>> Interrupts are triggered in step 1 and 2.2, but step 2.2's handler will not be
>>> invoked if step 1's handler is not return yet.
>>> What we do is to spawn a worker thread to do step 2 and return from step 1.
>>> So the handler can be invoked when the transfer response interrupt arrives.
>>
>> To build on Bard's correct answer, the irq thread only takes care of
>> 'immediate' actions, such as command completion, parity or bus clash errors.
>> The rest of the work can be split in
>> a) changes to device state, usually for attachment and enumeration. This is
>> rather slow and will entail regmap syncs.
>> b) device interrupts - typically only for jack detection which is also
>> rather slow.
>>
>> Since this irq thread function is actually part of the entire HDaudio
>> controller interrupt handling, we have to defer the work for cases a) and b)
>> and re-enable the HDaudio interrupts at the end of the irq thread function -
>> see the code I shared earlier.
>>
>> In addition, both a) and b) will result in transactions over the bus, which
>> will trigger interrupts to signal the command completions. In other words,
>> because of the asynchronous nature of the transactions, we need a two-level
>> implementation. If you look at the previous solution it was the same, the
>> commands were issued in the irq thread and the command completion was
>> handled in the handler, since we had to make the handler minimal with a
>> global GIE interrupt disable we kept the same hierarchy to deal with
>> commands but move it up one level.
>>
>> You could argue that maybe a worker thread is not optimal and could be
>> replaced by something better/faster. Since the jack detection is typically
>> handled with a worker thread in all ASoC codec drivers, we didn't feel the
>> need to optimize further. We did not see any performance impact with this
>> change.
>>
>> Does this answer to your concern?
>
> The point is that we are already in irq_thread which is designed to
> handle any bottom half processing and can be given priority, spawning of
> worker threads for another bottom half seems unnecessary to me and would
> increase the latency for you.
>
> I would have handled everything in irq_thread and returned, after all we
> are in bottom half :)
>
> Is there a reason for worker thread here, if so it is not clear to me
> atm.

I think we explained it at length: the irq thread deals with command
completion so the command initiation required for enumeration and
imp-def interrupt needs to be issued in *another* thread.

You cannot have in the same thread a wait_for_completion() and
complete(), it'd be a by-design deadlock.

Maybe a comparison would help.

previous design for N masters
N+2 Handlers + threads (one IPC, one stream, N SoundWire)
each SoundWire handler takes care of command completion and wakes a
thread for enumeration and imp-def interrupt.

New design
Single handler for ALL interrupt sources
The handler masks the global interrupt and wakes a thread that deals
with all interrupt sources, one after the other. The SoundWire thread
function for each Master will take case of command completion and
schedules a workqueue for enumeration and imp-def interrupt. The irq
thread then unmask the global interrupt and returns IRQ_HANDLED.

2020-07-15 15:09:35