[PATCH 0/7] SoundWire: intel: fix SHIM programming sequences

[PATCH 0/7] SoundWire: intel: fix SHIM programming sequences

[Pierre-Louis Bossart]

The low-level register programming sequences contributed in
71bb8a1b059ecd ('soundwire: intel: Add Intel Master driver') do not
follow the internal documentation and recommended flows. It's anyone's
guess how the code might have worked. Fix and add all missing helpers
for clock-stop and hardware-based synchronization.

This patchset needs to be applied on top of "[PATCH 00/16] SoundWire:
cadence: add clock stop and fix programming sequences"

Reviewers might object that the code is provided without some required
initializations for mutexes and shim masks, they will be added as part
of the transition to sdw_master_device - still stuck as of 3/11.

Pierre-Louis Bossart (6):
  soundwire: intel: add helpers for link power down and shim wake
  soundwire: intel: reuse code for wait loops to set/clear bits
  soundwire: intel: add mutex to prevent concurrent access to SHIM
    registers
  soundwire: intel: add definitions for shim_mask
  soundwire: intel: introduce a helper to arm link synchronization
  soundwire: intel: introduce helper for link synchronization

Rander Wang (1):
  soundwire: intel: follow documentation sequences for SHIM registers

 drivers/soundwire/intel.c           | 342 ++++++++++++++++++++++------
 drivers/soundwire/intel.h           |   4 +
 include/linux/soundwire/sdw_intel.h |   2 +
 3 files changed, 277 insertions(+), 71 deletions(-)

-- 
2.20.1

[PATCH 1/7] soundwire: intel: add helpers for link power down and shim wake

[Pierre-Louis Bossart]

These routines are required for power management

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 53 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 3c83e76c6bf9..28a8563c4e0f 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -364,6 +364,59 @@ static int intel_shim_init(struct sdw_intel *sdw)
 	return ret;
 }
 
+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;
+	u16 wake_en, wake_sts;
+
+	if (wake_enable) {
+		/* Enable the wakeup */
+		intel_writew(shim, SDW_SHIM_WAKEEN,
+			     (SDW_SHIM_WAKEEN_ENABLE << link_id));
+	} else {
+		/* Disable the wake up interrupt */
+		wake_en = intel_readw(shim, SDW_SHIM_WAKEEN);
+		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);
+	}
+}
+
+static int intel_link_power_down(struct sdw_intel *sdw)
+{
+	int link_control, spa_mask, cpa_mask, ret;
+	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);
+
+	ioctl &= ~(SDW_SHIM_IOCTL_MIF);
+	intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+
+	/* 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 (ret < 0)
+		return ret;
+
+	sdw->cdns.link_up = false;
+	return 0;
+}
+
 /*
  * PDI routines
  */
-- 
2.20.1

[PATCH 2/7] soundwire: intel: reuse code for wait loops to set/clear bits

[Pierre-Louis Bossart]

Refactor code and use same routines on set/clear

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 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 28a8563c4e0f..1a3b828b03a1 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -134,40 +134,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--;
+		udelay(50);
+	} 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.20.1

[PATCH 3/7] soundwire: intel: add mutex to prevent concurrent access to SHIM registers

[Pierre-Louis Bossart]

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.

The mutex initialization is done at the higher layer since the same
mutex is used for all links.

GitHub issue: https://github.com/thesofproject/linux/issues/1555
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 37 +++++++++++++++++++++++++++++++------
 drivers/soundwire/intel.h |  2 ++
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 1a3b828b03a1..3c271a8044b8 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -286,6 +286,8 @@ static int intel_link_power_up(struct sdw_intel *sdw)
 	int spa_mask, cpa_mask;
 	int link_control, ret;
 
+	mutex_lock(sdw->link_res->shim_lock);
+
 	/* Link power up sequence */
 	link_control = intel_readl(shim, SDW_SHIM_LCTL);
 	spa_mask = (SDW_SHIM_LCTL_SPA << link_id);
@@ -293,6 +295,8 @@ 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);
+	mutex_unlock(sdw->link_res->shim_lock);
+
 	if (ret < 0)
 		return ret;
 
@@ -307,6 +311,8 @@ static int intel_shim_init(struct sdw_intel *sdw)
 	int sync_reg, ret;
 	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);
@@ -351,6 +357,8 @@ static int intel_shim_init(struct sdw_intel *sdw)
 	sync_reg |= SDW_SHIM_SYNC_SYNCCPU;
 	ret = intel_clear_bit(shim, SDW_SHIM_SYNC, sync_reg,
 			      SDW_SHIM_SYNC_SYNCCPU);
+	mutex_unlock(sdw->link_res->shim_lock);
+
 	if (ret < 0)
 		dev_err(sdw->cdns.dev, "Failed to set sync period: %d\n", ret);
 
@@ -363,13 +371,15 @@ static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
 	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 */
-		intel_writew(shim, SDW_SHIM_WAKEEN,
-			     (SDW_SHIM_WAKEEN_ENABLE << link_id));
+		wake_en |= (SDW_SHIM_WAKEEN_ENABLE << link_id);
+		intel_writew(shim, SDW_SHIM_WAKEEN, wake_en);
 	} else {
 		/* Disable the wake up interrupt */
-		wake_en = intel_readw(shim, SDW_SHIM_WAKEEN);
 		wake_en &= ~(SDW_SHIM_WAKEEN_ENABLE << link_id);
 		intel_writew(shim, SDW_SHIM_WAKEEN, wake_en);
 
@@ -378,6 +388,7 @@ static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
 		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 intel_link_power_down(struct sdw_intel *sdw)
@@ -387,6 +398,8 @@ static int intel_link_power_down(struct sdw_intel *sdw)
 	void __iomem *shim = sdw->link_res->shim;
 	u16 ioctl;
 
+	mutex_lock(sdw->link_res->shim_lock);
+
 	/* Glue logic */
 	ioctl = intel_readw(shim, SDW_SHIM_IOCTL(link_id));
 	ioctl |= SDW_SHIM_IOCTL_BKE;
@@ -403,6 +416,8 @@ static int intel_link_power_down(struct sdw_intel *sdw)
 	link_control &=  spa_mask;
 
 	ret = intel_clear_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
+	mutex_unlock(sdw->link_res->shim_lock);
+
 	if (ret < 0)
 		return ret;
 
@@ -630,11 +645,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;
 }
 
@@ -649,6 +668,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);
 
@@ -660,9 +681,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
@@ -672,6 +694,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);
 
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index 38b7c125fb10..568c84a80d79 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -15,6 +15,7 @@
  * @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
  */
 struct sdw_intel_link_res {
 	struct platform_device *pdev;
@@ -25,6 +26,7 @@ struct sdw_intel_link_res {
 	int irq;
 	const struct sdw_intel_ops *ops;
 	struct device *dev;
+	struct mutex *shim_lock; /* protect shared registers */
 };
 
 #endif /* __SDW_INTEL_LOCAL_H */
-- 
2.20.1

[PATCH 4/7] soundwire: intel: add definitions for shim_mask

[Pierre-Louis Bossart]

We want to make sure SHIM register fields such as SYNCPRD are only
programmed once. Since we don't have a controller-level driver, we
need master-level drivers to collaborate: the registers will only be
programmed when the first link is powered-up.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.h           | 2 ++
 include/linux/soundwire/sdw_intel.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index 568c84a80d79..cfc83120b8f9 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -16,6 +16,7 @@
  * @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;
@@ -27,6 +28,7 @@ struct sdw_intel_link_res {
 	const struct sdw_intel_ops *ops;
 	struct device *dev;
 	struct mutex *shim_lock; /* protect shared registers */
+	u32 *shim_mask;
 };
 
 #endif /* __SDW_INTEL_LOCAL_H */
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.20.1

[PATCH 5/7] soundwire: intel: follow documentation sequences for SHIM registers

[Pierre-Louis Bossart]

From: Rander Wang <rander.wang@intel.com>

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.

This includes making sure SHIM_SYNC is programmed only once, before
the first link is powered on.

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

Signed-off-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 186 ++++++++++++++++++++++++++++----------
 1 file changed, 139 insertions(+), 47 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 3c271a8044b8..9c6514fe1284 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)
@@ -283,11 +284,48 @@ 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);
 	spa_mask = (SDW_SHIM_LCTL_SPA << link_id);
@@ -295,73 +333,118 @@ 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);
-	mutex_unlock(sdw->link_res->shim_lock);
+	if (ret < 0) {
+		dev_err(sdw->cdns.dev, "Failed to power up link: %d\n", ret);
+		goto out;
+	}
 
-	if (ret < 0)
-		return ret;
+	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;
-
-	mutex_lock(sdw->link_res->shim_lock);
-
-	/* 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);
 	mutex_unlock(sdw->link_res->shim_lock);
 
-	if (ret < 0)
-		dev_err(sdw->cdns.dev, "Failed to set sync period: %d\n", ret);
-
 	return ret;
 }
 
@@ -393,21 +476,15 @@ static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
 
 static int intel_link_power_down(struct sdw_intel *sdw)
 {
-	int link_control, spa_mask, cpa_mask, ret;
+	int link_control, spa_mask, cpa_mask;
 	unsigned int link_id = sdw->instance;
 	void __iomem *shim = sdw->link_res->shim;
-	u16 ioctl;
+	u32 *shim_mask = sdw->link_res->shim_mask;
+	int ret = 0;
 
 	mutex_lock(sdw->link_res->shim_lock);
 
-	/* 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);
-
-	ioctl &= ~(SDW_SHIM_IOCTL_MIF);
-	intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+	intel_shim_master_ip_to_glue(sdw);
 
 	/* Link power down sequence */
 	link_control = intel_readl(shim, SDW_SHIM_LCTL);
@@ -416,6 +493,13 @@ static int intel_link_power_down(struct sdw_intel *sdw)
 	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)
@@ -1144,9 +1228,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);
 }
-- 
2.20.1

[PATCH 6/7] soundwire: intel: introduce a helper to arm link synchronization

[Pierre-Louis Bossart]

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 <pierre-louis.bossart@linux.intel.com>
---
 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 9c6514fe1284..93d157d04eb9 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -509,6 +509,21 @@ static int 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
  */
@@ -722,21 +737,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.20.1

[PATCH 7/7] soundwire: intel: introduce helper for link synchronization

[Pierre-Louis Bossart]

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 <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 47 ++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 93d157d04eb9..e2dac2f0b343 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -524,6 +524,44 @@ 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;
+}
+
+static int intel_shim_sync_go(struct sdw_intel *sdw)
+{
+	int ret;
+
+	mutex_lock(sdw->link_res->shim_lock);
+
+	ret = intel_shim_sync_go_unlocked(sdw);
+
+	mutex_unlock(sdw->link_res->shim_lock);
+
+	return ret;
+}
+
 /*
  * PDI routines
  */
@@ -775,15 +813,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.20.1

Re: [PATCH 2/7] soundwire: intel: reuse code for wait loops to set/clear bits

[Vinod Koul]

On 11-03-20, 17:10, Pierre-Louis Bossart wrote:
> Refactor code and use same routines on set/clear
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  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 28a8563c4e0f..1a3b828b03a1 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -134,40 +134,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--;
> +		udelay(50);

This should use udelay_range, but this can be different patch as this is
code move, so okay

> +	} 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.20.1

-- 
~Vinod

Re: [PATCH 3/7] soundwire: intel: add mutex to prevent concurrent access to SHIM registers

[Vinod Koul]

On 11-03-20, 17:10, Pierre-Louis Bossart wrote:
> 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.
> 
> The mutex initialization is done at the higher layer since the same
> mutex is used for all links.
> 
> GitHub issue: https://github.com/thesofproject/linux/issues/1555
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/intel.c | 37 +++++++++++++++++++++++++++++++------
>  drivers/soundwire/intel.h |  2 ++
>  2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 1a3b828b03a1..3c271a8044b8 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -286,6 +286,8 @@ static int intel_link_power_up(struct sdw_intel *sdw)
>  	int spa_mask, cpa_mask;
>  	int link_control, ret;
>  
> +	mutex_lock(sdw->link_res->shim_lock);
> +
>  	/* Link power up sequence */
>  	link_control = intel_readl(shim, SDW_SHIM_LCTL);
>  	spa_mask = (SDW_SHIM_LCTL_SPA << link_id);
> @@ -293,6 +295,8 @@ 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);
> +	mutex_unlock(sdw->link_res->shim_lock);
> +
>  	if (ret < 0)
>  		return ret;
>  
> @@ -307,6 +311,8 @@ static int intel_shim_init(struct sdw_intel *sdw)
>  	int sync_reg, ret;
>  	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);
> @@ -351,6 +357,8 @@ static int intel_shim_init(struct sdw_intel *sdw)
>  	sync_reg |= SDW_SHIM_SYNC_SYNCCPU;
>  	ret = intel_clear_bit(shim, SDW_SHIM_SYNC, sync_reg,
>  			      SDW_SHIM_SYNC_SYNCCPU);
> +	mutex_unlock(sdw->link_res->shim_lock);
> +
>  	if (ret < 0)
>  		dev_err(sdw->cdns.dev, "Failed to set sync period: %d\n", ret);
>  
> @@ -363,13 +371,15 @@ static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
>  	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 */
> -		intel_writew(shim, SDW_SHIM_WAKEEN,
> -			     (SDW_SHIM_WAKEEN_ENABLE << link_id));
> +		wake_en |= (SDW_SHIM_WAKEEN_ENABLE << link_id);
> +		intel_writew(shim, SDW_SHIM_WAKEEN, wake_en);
>  	} else {
>  		/* Disable the wake up interrupt */
> -		wake_en = intel_readw(shim, SDW_SHIM_WAKEEN);
>  		wake_en &= ~(SDW_SHIM_WAKEEN_ENABLE << link_id);
>  		intel_writew(shim, SDW_SHIM_WAKEEN, wake_en);
>  
> @@ -378,6 +388,7 @@ static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
>  		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 intel_link_power_down(struct sdw_intel *sdw)
> @@ -387,6 +398,8 @@ static int intel_link_power_down(struct sdw_intel *sdw)
>  	void __iomem *shim = sdw->link_res->shim;
>  	u16 ioctl;
>  
> +	mutex_lock(sdw->link_res->shim_lock);
> +
>  	/* Glue logic */
>  	ioctl = intel_readw(shim, SDW_SHIM_IOCTL(link_id));
>  	ioctl |= SDW_SHIM_IOCTL_BKE;
> @@ -403,6 +416,8 @@ static int intel_link_power_down(struct sdw_intel *sdw)
>  	link_control &=  spa_mask;
>  
>  	ret = intel_clear_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
> +	mutex_unlock(sdw->link_res->shim_lock);
> +
>  	if (ret < 0)
>  		return ret;
>  
> @@ -630,11 +645,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;
>  }
>  
> @@ -649,6 +668,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);
>  
> @@ -660,9 +681,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
> @@ -672,6 +694,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);
>  
> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
> index 38b7c125fb10..568c84a80d79 100644
> --- a/drivers/soundwire/intel.h
> +++ b/drivers/soundwire/intel.h
> @@ -15,6 +15,7 @@
>   * @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
>   */
>  struct sdw_intel_link_res {
>  	struct platform_device *pdev;
> @@ -25,6 +26,7 @@ struct sdw_intel_link_res {
>  	int irq;
>  	const struct sdw_intel_ops *ops;
>  	struct device *dev;
> +	struct mutex *shim_lock; /* protect shared registers */

Where is this mutex initialized? Did you test this...

-- 
~Vinod

Re: [PATCH 4/7] soundwire: intel: add definitions for shim_mask

[Vinod Koul]

On 11-03-20, 17:10, Pierre-Louis Bossart wrote:
> We want to make sure SHIM register fields such as SYNCPRD are only
> programmed once. Since we don't have a controller-level driver, we
> need master-level drivers to collaborate: the registers will only be
> programmed when the first link is powered-up.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/intel.h           | 2 ++
>  include/linux/soundwire/sdw_intel.h | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
> index 568c84a80d79..cfc83120b8f9 100644
> --- a/drivers/soundwire/intel.h
> +++ b/drivers/soundwire/intel.h
> @@ -16,6 +16,7 @@
>   * @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;
> @@ -27,6 +28,7 @@ struct sdw_intel_link_res {
>  	const struct sdw_intel_ops *ops;
>  	struct device *dev;
>  	struct mutex *shim_lock; /* protect shared registers */
> +	u32 *shim_mask;

You have a pointer, okay where is it initialized

>  };
>  
>  #endif /* __SDW_INTEL_LOCAL_H */
> 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;

And a integer, question: why do you need pointer and integer, why not
use only one..?

-- 
~Vinod

Re: [PATCH 5/7] soundwire: intel: follow documentation sequences for SHIM registers

[Vinod Koul]

On 11-03-20, 17:10, Pierre-Louis Bossart wrote:
> From: Rander Wang <rander.wang@intel.com>
> 
> Somehow the existing code is not aligned with the steps described in
> the documentation, refactor code and make sure the register

Is the documentation available public space so that we can correct

> programming sequences are correct.
> 
> This includes making sure SHIM_SYNC is programmed only once, before
> the first link is powered on.
> 
> Note that the SYNCPRD value is tied only to the XTAL value and not the
> current bus frequency or the frame rate.
> 
> Signed-off-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/intel.c | 186 ++++++++++++++++++++++++++++----------
>  1 file changed, 139 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 3c271a8044b8..9c6514fe1284 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)
> @@ -283,11 +284,48 @@ 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;

this is a local pointer, so the one defined previously is not used.

> +	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+).

Sorry this looks quite bad to read, we have 80 chars, so please use
like below:

	/*
         * 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+).
	 */

-- 
~Vinod

Re: [PATCH 3/7] soundwire: intel: add mutex to prevent concurrent access to SHIM registers

[Pierre-Louis Bossart]

>> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
>> index 38b7c125fb10..568c84a80d79 100644
>> --- a/drivers/soundwire/intel.h
>> +++ b/drivers/soundwire/intel.h
>> @@ -15,6 +15,7 @@
>>    * @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
>>    */
>>   struct sdw_intel_link_res {
>>   	struct platform_device *pdev;
>> @@ -25,6 +26,7 @@ struct sdw_intel_link_res {
>>   	int irq;
>>   	const struct sdw_intel_ops *ops;
>>   	struct device *dev;
>> +	struct mutex *shim_lock; /* protect shared registers */
> 
> Where is this mutex initialized? Did you test this...

Dude, we've been testing the heck out of SoundWire.

If you want to see the actual initialization it's in the intel_init.c code:

https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L231

And this was clearly stated in the cover letter:

"Reviewers might object that the code is provided without some required
initializations for mutexes and shim masks, they will be added as part
of the transition to sdw_master_device - still stuck as of 3/11."

Re: [PATCH 4/7] soundwire: intel: add definitions for shim_mask

[Pierre-Louis Bossart]

>> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
>> index 568c84a80d79..cfc83120b8f9 100644
>> --- a/drivers/soundwire/intel.h
>> +++ b/drivers/soundwire/intel.h
>> @@ -16,6 +16,7 @@
>>    * @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;
>> @@ -27,6 +28,7 @@ struct sdw_intel_link_res {
>>   	const struct sdw_intel_ops *ops;
>>   	struct device *dev;
>>   	struct mutex *shim_lock; /* protect shared registers */
>> +	u32 *shim_mask;
> 
> You have a pointer, okay where is it initialized

same answer as shim_lock, it's initialized at the higher level

https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L252


>>   };
>>   
>>   #endif /* __SDW_INTEL_LOCAL_H */
>> 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;
> 
> And a integer, question: why do you need pointer and integer, why not
> use only one..?

You may have missed that the structures are different.

the sdw_intel_ctx is global for all links, so that the shim_mask integer 
value.
the sdw_intel_link_res is link-specific and use a pointer to the same 
shim_mask value, protected by shim_lock.

Re: [PATCH 5/7] soundwire: intel: follow documentation sequences for SHIM registers

[Pierre-Louis Bossart]

On 3/20/20 8:51 AM, Vinod Koul wrote:
> On 11-03-20, 17:10, Pierre-Louis Bossart wrote:
>> From: Rander Wang <rander.wang@intel.com>
>>
>> Somehow the existing code is not aligned with the steps described in
>> the documentation, refactor code and make sure the register
> 
> Is the documentation available public space so that we can correct

No, so you'll have to trust us blindly on this one.


>> @@ -283,11 +284,48 @@ 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;
> 
> this is a local pointer, so the one defined previously is not used.

No idea what you are saying, it's the same address so changes to 
*shim_mask will be the same as in *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+).
> 
> Sorry this looks quite bad to read, we have 80 chars, so please use
> like below:
> 
> 	/*
>           * 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+).
> 	 */

Are we really going to have an emacs vs vi discussion here?

Re: [PATCH 3/7] soundwire: intel: add mutex to prevent concurrent access to SHIM registers

[Vinod Koul]

On 20-03-20, 09:07, Pierre-Louis Bossart wrote:
> 
> > > diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
> > > index 38b7c125fb10..568c84a80d79 100644
> > > --- a/drivers/soundwire/intel.h
> > > +++ b/drivers/soundwire/intel.h
> > > @@ -15,6 +15,7 @@
> > >    * @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
> > >    */
> > >   struct sdw_intel_link_res {
> > >   	struct platform_device *pdev;
> > > @@ -25,6 +26,7 @@ struct sdw_intel_link_res {
> > >   	int irq;
> > >   	const struct sdw_intel_ops *ops;
> > >   	struct device *dev;
> > > +	struct mutex *shim_lock; /* protect shared registers */
> > 
> > Where is this mutex initialized? Did you test this...
> 
> Dude, we've been testing the heck out of SoundWire.
> 
> If you want to see the actual initialization it's in the intel_init.c code:
> 
> https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L231

Which doesn't make much sense. A patch should do complete thing. I don't
see a reason why you cannot pull this single line into this patch.

It belongs here, not anywhere else.

-- 
~Vinod

Re: [PATCH 4/7] soundwire: intel: add definitions for shim_mask

[Vinod Koul]

On 20-03-20, 09:13, Pierre-Louis Bossart wrote:
> 
> > > diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
> > > index 568c84a80d79..cfc83120b8f9 100644
> > > --- a/drivers/soundwire/intel.h
> > > +++ b/drivers/soundwire/intel.h
> > > @@ -16,6 +16,7 @@
> > >    * @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;
> > > @@ -27,6 +28,7 @@ struct sdw_intel_link_res {
> > >   	const struct sdw_intel_ops *ops;
> > >   	struct device *dev;
> > >   	struct mutex *shim_lock; /* protect shared registers */
> > > +	u32 *shim_mask;
> > 
> > You have a pointer, okay where is it initialized
> 
> same answer as shim_lock, it's initialized at the higher level
> 
> https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L252

Why can't it be done here, what stops you?

You really need to keep initialzation and usage in same patch :(

-- 
~Vinod

Re: [PATCH 5/7] soundwire: intel: follow documentation sequences for SHIM registers

[Vinod Koul]

On 20-03-20, 09:20, Pierre-Louis Bossart wrote:

> > > @@ -283,11 +284,48 @@ 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;
> > 
> > this is a local pointer, so the one defined previously is not used.
> 
> No idea what you are saying, it's the same address so changes to *shim_mask
> will be the same as in *sdw->link_res->shim_mask.

There seems to be too many shim_masks, in global structs, then pointer
and then local ones. It is really confusing...

> > > +	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+).
> > 
> > Sorry this looks quite bad to read, we have 80 chars, so please use
> > like below:
> > 
> > 	/*
> >           * 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+).
> > 	 */
> 
> Are we really going to have an emacs vs vi discussion here?

What has that got to do with editor to use, nothing imo.

All I am asking is to use 80 chars here and make it look decent to
read. And not truncate at 60 ish chars which seems above

-- 
~Vinod