* [PATCH 0/6] soundwire: qcom: various improvements
@ 2021-01-29 17:32 Srinivas Kandagatla
2021-01-29 17:32 ` [PATCH 1/6] soundwire: qcom: add support to missing transport params Srinivas Kandagatla
` (5 more replies)
0 siblings, 6 replies; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-01-29 17:32 UTC (permalink / raw)
To: vkoul
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel, Srinivas Kandagatla
During testing SoundWire controller on SM8250 MTP, we found
few issues like all the interrupts are not handled,
all transport parameters are not read from device tree.
Other major issue was register read/writes which was interrupt based
was an overhead and puts lot of limitation on context it can be used from.
So this patchset add various improvements to the existing driver
to address above issues.
Tested it on SM8250 MTP with 2x WSA881x speakers, HeadPhones on
WCD938x via lpass-rx-macro and Analog MICs via lpass-tx-macro.
Also tested on DragonBoard DB845c with 2xWSA881x speakers.
Srinivas Kandagatla (6):
soundwire: qcom: add support to missing transport params
soundwire: qcom: extract version field
soundwire: qcom: set continue execution flag for ignored commands
soundwire: qcom: start the clock during initialization
soundwire: qcom: update register read/write routine
soundwire: qcom: add support to new interrupts
drivers/soundwire/qcom.c | 471 ++++++++++++++++++++++++++++++---------
1 file changed, 366 insertions(+), 105 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/6] soundwire: qcom: add support to missing transport params
2021-01-29 17:32 [PATCH 0/6] soundwire: qcom: various improvements Srinivas Kandagatla
@ 2021-01-29 17:32 ` Srinivas Kandagatla
2021-01-29 19:20 ` Pierre-Louis Bossart
2021-02-01 14:13 ` Vinod Koul
2021-01-29 17:32 ` [PATCH 2/6] soundwire: qcom: extract version field Srinivas Kandagatla
` (4 subsequent siblings)
5 siblings, 2 replies; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-01-29 17:32 UTC (permalink / raw)
To: vkoul
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel, Srinivas Kandagatla
Some of the transport parameters derived from device tree
are not fully parsed by the driver.
This patch adds support to parse those missing parameters.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/soundwire/qcom.c | 107 +++++++++++++++++++++++++++++++++++++--
1 file changed, 103 insertions(+), 4 deletions(-)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 6d22df01f354..36e273795cbe 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -54,7 +54,13 @@
#define SWRM_MCP_SLV_STATUS 0x1090
#define SWRM_MCP_SLV_STATUS_MASK GENMASK(1, 0)
#define SWRM_DP_PORT_CTRL_BANK(n, m) (0x1124 + 0x100 * (n - 1) + 0x40 * m)
+#define SWRM_DP_PORT_CTRL_2_BANK(n, m) (0x1128 + 0x100 * (n - 1) + 0x40 * m)
+#define SWRM_DP_BLOCK_CTRL_1(n) (0x112C + 0x100 * (n - 1))
+#define SWRM_DP_BLOCK_CTRL2_BANK(n, m) (0x1130 + 0x100 * (n - 1) + 0x40 * m)
+#define SWRM_DP_PORT_HCTRL_BANK(n, m) (0x1134 + 0x100 * (n - 1) + 0x40 * m)
#define SWRM_DP_BLOCK_CTRL3_BANK(n, m) (0x1138 + 0x100 * (n - 1) + 0x40 * m)
+#define SWRM_DIN_DPn_PCM_PORT_CTRL(n) (0x1054 + 0x100 * (n - 1))
+
#define SWRM_DP_PORT_CTRL_EN_CHAN_SHFT 0x18
#define SWRM_DP_PORT_CTRL_OFFSET2_SHFT 0x10
#define SWRM_DP_PORT_CTRL_OFFSET1_SHFT 0x08
@@ -73,12 +79,20 @@
#define QCOM_SDW_MAX_PORTS 14
#define DEFAULT_CLK_FREQ 9600000
#define SWRM_MAX_DAIS 0xF
+#define SWR_INVALID_PARAM 0xFF
+#define SWR_HSTOP_MAX_VAL 0xF
+#define SWR_HSTART_MIN_VAL 0x0
struct qcom_swrm_port_config {
u8 si;
u8 off1;
u8 off2;
u8 bp_mode;
+ u8 hstart;
+ u8 hstop;
+ u8 word_length;
+ u8 bgp_count;
+ u8 lane_control;
};
struct qcom_swrm_ctrl {
@@ -396,7 +410,13 @@ static int qcom_swrm_port_params(struct sdw_bus *bus,
struct sdw_port_params *p_params,
unsigned int bank)
{
- /* TBD */
+ struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
+
+ if (p_params->bps != SWR_INVALID_PARAM)
+ return ctrl->reg_write(ctrl,
+ SWRM_DP_BLOCK_CTRL_1(p_params->num),
+ p_params->bps - 1);
+
return 0;
}
@@ -415,10 +435,32 @@ static int qcom_swrm_transport_params(struct sdw_bus *bus,
ret = ctrl->reg_write(ctrl, reg, value);
- if (!ret && params->blk_pkg_mode) {
- reg = SWRM_DP_BLOCK_CTRL3_BANK(params->port_num, bank);
+ if (params->lane_ctrl != SWR_INVALID_PARAM) {
+ reg = SWRM_DP_PORT_CTRL_2_BANK(params->port_num, bank);
+ value = params->lane_ctrl;
+ ret = ctrl->reg_write(ctrl, reg, value);
+ }
- ret = ctrl->reg_write(ctrl, reg, 1);
+ if (params->blk_grp_ctrl != SWR_INVALID_PARAM) {
+ reg = SWRM_DP_BLOCK_CTRL2_BANK(params->port_num, bank);
+ value = params->blk_grp_ctrl;
+ ret = ctrl->reg_write(ctrl, reg, value);
+ }
+
+ if (params->hstart != SWR_INVALID_PARAM
+ && params->hstop != SWR_INVALID_PARAM) {
+ reg = SWRM_DP_PORT_HCTRL_BANK(params->port_num, bank);
+ value = (params->hstop << 4) | params->hstart;
+ ret = ctrl->reg_write(ctrl, reg, value);
+ } else {
+ reg = SWRM_DP_PORT_HCTRL_BANK(params->port_num, bank);
+ value = (SWR_HSTOP_MAX_VAL << 4) | SWR_HSTART_MIN_VAL;
+ ret = ctrl->reg_write(ctrl, reg, value);
+ }
+
+ if (params->blk_pkg_mode != SWR_INVALID_PARAM) {
+ reg = SWRM_DP_BLOCK_CTRL3_BANK(params->port_num, bank);
+ ret = ctrl->reg_write(ctrl, reg, params->blk_pkg_mode);
}
return ret;
@@ -470,6 +512,17 @@ static int qcom_swrm_compute_params(struct sdw_bus *bus)
p_rt->transport_params.offset1 = pcfg->off1;
p_rt->transport_params.offset2 = pcfg->off2;
p_rt->transport_params.blk_pkg_mode = pcfg->bp_mode;
+ p_rt->transport_params.blk_grp_ctrl = pcfg->bgp_count;
+ p_rt->transport_params.hstart = pcfg->hstart;
+ p_rt->transport_params.hstop = pcfg->hstop;
+ p_rt->transport_params.lane_ctrl = pcfg->lane_control;
+ if (pcfg->word_length != SWR_INVALID_PARAM) {
+ sdw_fill_port_params(&p_rt->port_params,
+ p_rt->num, pcfg->word_length + 1,
+ SDW_PORT_FLOW_MODE_ISOCH,
+ SDW_PORT_DATA_MODE_NORMAL);
+ }
+
}
list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
@@ -481,6 +534,18 @@ static int qcom_swrm_compute_params(struct sdw_bus *bus)
p_rt->transport_params.offset1 = pcfg->off1;
p_rt->transport_params.offset2 = pcfg->off2;
p_rt->transport_params.blk_pkg_mode = pcfg->bp_mode;
+ p_rt->transport_params.blk_grp_ctrl = pcfg->bgp_count;
+
+ p_rt->transport_params.hstart = pcfg->hstart;
+ p_rt->transport_params.hstop = pcfg->hstop;
+ p_rt->transport_params.lane_ctrl = pcfg->lane_control;
+ if (pcfg->word_length != SWR_INVALID_PARAM) {
+ sdw_fill_port_params(&p_rt->port_params,
+ p_rt->num,
+ pcfg->word_length + 1,
+ SDW_PORT_FLOW_MODE_ISOCH,
+ SDW_PORT_DATA_MODE_NORMAL);
+ }
i++;
}
}
@@ -728,6 +793,11 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
u8 off2[QCOM_SDW_MAX_PORTS];
u8 si[QCOM_SDW_MAX_PORTS];
u8 bp_mode[QCOM_SDW_MAX_PORTS] = { 0, };
+ u8 hstart[QCOM_SDW_MAX_PORTS];
+ u8 hstop[QCOM_SDW_MAX_PORTS];
+ u8 word_length[QCOM_SDW_MAX_PORTS];
+ u8 bgp_count[QCOM_SDW_MAX_PORTS];
+ u8 lane_control[QCOM_SDW_MAX_PORTS];
int i, ret, nports, val;
ctrl->reg_read(ctrl, SWRM_COMP_PARAMS, &val);
@@ -772,11 +842,40 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
ret = of_property_read_u8_array(np, "qcom,ports-block-pack-mode",
bp_mode, nports);
+
+ ret = of_property_read_u8_array(np, "qcom,ports-hstart", hstart, nports);
+ if (ret)
+ memset(hstart, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
+
+ ret = of_property_read_u8_array(np, "qcom,ports-hstop", hstop, nports);
+ if (ret)
+ memset(hstop, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
+
+ ret = of_property_read_u8_array(np, "qcom,ports-word-length",
+ word_length, nports);
+ if (ret)
+ memset(word_length, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
+
+ ret = of_property_read_u8_array(np, "qcom,ports-block-group-count",
+ bgp_count, nports);
+ if (ret)
+ memset(bgp_count, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
+
+ ret = of_property_read_u8_array(np, "qcom,ports-lane-control",
+ lane_control, nports);
+ if (ret)
+ memset(lane_control, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
+
for (i = 0; i < nports; i++) {
ctrl->pconfig[i].si = si[i];
ctrl->pconfig[i].off1 = off1[i];
ctrl->pconfig[i].off2 = off2[i];
ctrl->pconfig[i].bp_mode = bp_mode[i];
+ ctrl->pconfig[i].hstart = hstart[i];
+ ctrl->pconfig[i].hstop = hstop[i];
+ ctrl->pconfig[i].word_length = word_length[i];
+ ctrl->pconfig[i].bgp_count = bgp_count[i];
+ ctrl->pconfig[i].lane_control = lane_control[i];
}
return 0;
--
2.21.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/6] soundwire: qcom: extract version field
2021-01-29 17:32 [PATCH 0/6] soundwire: qcom: various improvements Srinivas Kandagatla
2021-01-29 17:32 ` [PATCH 1/6] soundwire: qcom: add support to missing transport params Srinivas Kandagatla
@ 2021-01-29 17:32 ` Srinivas Kandagatla
2021-01-29 17:32 ` [PATCH 3/6] soundwire: qcom: set continue execution flag for ignored commands Srinivas Kandagatla
` (3 subsequent siblings)
5 siblings, 0 replies; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-01-29 17:32 UTC (permalink / raw)
To: vkoul
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel, Srinivas Kandagatla
Extract version field to major, minor and step, so that we can add
dynamic version checks to read/writes.
This will help for controller versions that need specific bits
to be programmed.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/soundwire/qcom.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 36e273795cbe..da6e0d4e9622 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -110,7 +110,9 @@ struct qcom_swrm_ctrl {
u8 wr_cmd_id;
u8 rd_cmd_id;
int irq;
- unsigned int version;
+ u8 version_major;
+ u8 version_minor;
+ u8 version_step;
int num_din_ports;
int num_dout_ports;
int cols_index;
@@ -961,7 +963,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
prop->default_col = data->default_cols;
prop->default_row = data->default_rows;
- ctrl->reg_read(ctrl, SWRM_COMP_HW_VERSION, &ctrl->version);
+ ctrl->reg_read(ctrl, SWRM_COMP_HW_VERSION, &val);
ret = devm_request_threaded_irq(dev, ctrl->irq, NULL,
qcom_swrm_irq_handler,
@@ -985,9 +987,11 @@ static int qcom_swrm_probe(struct platform_device *pdev)
if (ret)
goto err_master_add;
+ ctrl->version_major = (val >> 24) & 0xff;
+ ctrl->version_minor = (val >> 16) & 0xff;
+ ctrl->version_step = val & 0xffff;
dev_info(dev, "Qualcomm Soundwire controller v%x.%x.%x Registered\n",
- (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
- ctrl->version & 0xffff);
+ ctrl->version_major, ctrl->version_minor, ctrl->version_step);
return 0;
--
2.21.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 3/6] soundwire: qcom: set continue execution flag for ignored commands
2021-01-29 17:32 [PATCH 0/6] soundwire: qcom: various improvements Srinivas Kandagatla
2021-01-29 17:32 ` [PATCH 1/6] soundwire: qcom: add support to missing transport params Srinivas Kandagatla
2021-01-29 17:32 ` [PATCH 2/6] soundwire: qcom: extract version field Srinivas Kandagatla
@ 2021-01-29 17:32 ` Srinivas Kandagatla
2021-01-29 19:21 ` Pierre-Louis Bossart
2021-02-01 14:16 ` Vinod Koul
2021-01-29 17:32 ` [PATCH 4/6] soundwire: qcom: start the clock during initialization Srinivas Kandagatla
` (2 subsequent siblings)
5 siblings, 2 replies; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-01-29 17:32 UTC (permalink / raw)
To: vkoul
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel, Srinivas Kandagatla
version 1.5.1 and higher IPs of this controller required to set
continue execution on ingored command flag. This patch sets this flag.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/soundwire/qcom.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index da6e0d4e9622..3669bac11a32 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -40,6 +40,7 @@
#define SWRM_CMD_FIFO_CMD 0x308
#define SWRM_CMD_FIFO_STATUS 0x30C
#define SWRM_CMD_FIFO_CFG_ADDR 0x314
+#define SWRM_CONTINUE_EXEC_ON_CMD_IGNORE BIT(31)
#define SWRM_RD_WR_CMD_RETRIES 0x7
#define SWRM_CMD_FIFO_RD_FIFO_ADDR 0x318
#define SWRM_ENUMERATOR_CFG_ADDR 0x500
@@ -345,7 +346,16 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
/* Configure number of retries of a read/write cmd */
- ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, SWRM_RD_WR_CMD_RETRIES);
+ if (ctrl->version_major == 1 && ctrl->version_minor >= 5 &&
+ ctrl->version_step >= 1) {
+ /* Only for versions >= 1.5.1 */
+ ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR,
+ SWRM_RD_WR_CMD_RETRIES |
+ SWRM_CONTINUE_EXEC_ON_CMD_IGNORE);
+ } else {
+ ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR,
+ SWRM_RD_WR_CMD_RETRIES);
+ }
/* Set IRQ to PULSE */
ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
--
2.21.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 4/6] soundwire: qcom: start the clock during initialization
2021-01-29 17:32 [PATCH 0/6] soundwire: qcom: various improvements Srinivas Kandagatla
` (2 preceding siblings ...)
2021-01-29 17:32 ` [PATCH 3/6] soundwire: qcom: set continue execution flag for ignored commands Srinivas Kandagatla
@ 2021-01-29 17:32 ` Srinivas Kandagatla
2021-02-01 14:21 ` Vinod Koul
2021-01-29 17:32 ` [PATCH 5/6] soundwire: qcom: update register read/write routine Srinivas Kandagatla
2021-01-29 17:32 ` [PATCH 6/6] soundwire: qcom: add support to new interrupts Srinivas Kandagatla
5 siblings, 1 reply; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-01-29 17:32 UTC (permalink / raw)
To: vkoul
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel, Srinivas Kandagatla
Start the clock during initialization.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/soundwire/qcom.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 3669bac11a32..83df15d83935 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -47,6 +47,8 @@
#define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m) (0x101C + 0x40 * (m))
#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK GENMASK(2, 0)
#define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK GENMASK(7, 3)
+#define SWRM_MCP_BUS_CTRL 0x1044
+#define SWRM_MCP_BUS_CLK_START BIT(1)
#define SWRM_MCP_CFG_ADDR 0x1048
#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK GENMASK(21, 17)
#define SWRM_DEF_CMD_NO_PINGS 0x1f
@@ -345,6 +347,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
u32p_replace_bits(&val, SWRM_DEF_CMD_NO_PINGS, SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK);
ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
+ ctrl->reg_write(ctrl, SWRM_MCP_BUS_CTRL, SWRM_MCP_BUS_CLK_START);
/* Configure number of retries of a read/write cmd */
if (ctrl->version_major == 1 && ctrl->version_minor >= 5 &&
ctrl->version_step >= 1) {
--
2.21.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 5/6] soundwire: qcom: update register read/write routine
2021-01-29 17:32 [PATCH 0/6] soundwire: qcom: various improvements Srinivas Kandagatla
` (3 preceding siblings ...)
2021-01-29 17:32 ` [PATCH 4/6] soundwire: qcom: start the clock during initialization Srinivas Kandagatla
@ 2021-01-29 17:32 ` Srinivas Kandagatla
2021-01-29 19:33 ` Pierre-Louis Bossart
2021-02-01 14:26 ` Vinod Koul
2021-01-29 17:32 ` [PATCH 6/6] soundwire: qcom: add support to new interrupts Srinivas Kandagatla
5 siblings, 2 replies; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-01-29 17:32 UTC (permalink / raw)
To: vkoul
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel, Srinivas Kandagatla
In the existing code every soundwire register read and register write
are kinda blocked. Each of these are using a special command id that
generates interrupt after it successfully finishes. This is really
overhead, limiting and not really necessary unless we are doing
something special.
We can simply read/write the fifo that should also give exactly
what we need! This will also allow to read/write registers in
interrupt context, which was not possible with the special
command approach.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/soundwire/qcom.c | 148 +++++++++++++++++++++++++--------------
1 file changed, 96 insertions(+), 52 deletions(-)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 83df15d83935..d61b204dc284 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -78,13 +78,15 @@
#define SWRM_SPECIAL_CMD_ID 0xF
#define MAX_FREQ_NUM 1
#define TIMEOUT_MS (2 * HZ)
-#define QCOM_SWRM_MAX_RD_LEN 0xf
+#define QCOM_SWRM_MAX_RD_LEN 0x1
#define QCOM_SDW_MAX_PORTS 14
#define DEFAULT_CLK_FREQ 9600000
#define SWRM_MAX_DAIS 0xF
#define SWR_INVALID_PARAM 0xFF
#define SWR_HSTOP_MAX_VAL 0xF
#define SWR_HSTART_MIN_VAL 0x0
+#define SWR_BROADCAST_CMD_ID 0x0F
+#define MAX_FIFO_RD_FAIL_RETRY 3
struct qcom_swrm_port_config {
u8 si;
@@ -104,11 +106,13 @@ struct qcom_swrm_ctrl {
struct regmap *regmap;
void __iomem *mmio;
struct completion *comp;
+ struct completion broadcast;
struct work_struct slave_work;
/* read/write lock */
spinlock_t comp_lock;
/* Port alloc/free lock */
struct mutex port_lock;
+ struct mutex io_lock;
struct clk *hclk;
u8 wr_cmd_id;
u8 rd_cmd_id;
@@ -122,6 +126,8 @@ struct qcom_swrm_ctrl {
int rows_index;
unsigned long dout_port_mask;
unsigned long din_port_mask;
+ u8 rcmd_id;
+ u8 wcmd_id;
struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
enum sdw_slave_status status[SDW_MAX_DEVICES];
@@ -200,75 +206,111 @@ static int qcom_swrm_cpu_reg_write(struct qcom_swrm_ctrl *ctrl, int reg,
return SDW_CMD_OK;
}
-static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
- u8 dev_addr, u16 reg_addr)
+static u32 swrm_get_packed_reg_val(u8 *cmd_id, u8 cmd_data,
+ u8 dev_addr, u16 reg_addr)
{
- DECLARE_COMPLETION_ONSTACK(comp);
- unsigned long flags;
u32 val;
- int ret;
-
- spin_lock_irqsave(&ctrl->comp_lock, flags);
- ctrl->comp = ∁
- spin_unlock_irqrestore(&ctrl->comp_lock, flags);
- val = SWRM_REG_VAL_PACK(cmd_data, dev_addr,
- SWRM_SPECIAL_CMD_ID, reg_addr);
- ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
- if (ret)
- goto err;
-
- ret = wait_for_completion_timeout(ctrl->comp,
- msecs_to_jiffies(TIMEOUT_MS));
+ u8 id = *cmd_id;
- if (!ret)
- ret = SDW_CMD_IGNORED;
- else
- ret = SDW_CMD_OK;
-err:
- spin_lock_irqsave(&ctrl->comp_lock, flags);
- ctrl->comp = NULL;
- spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+ if (id != SWR_BROADCAST_CMD_ID) {
+ if (id < 14)
+ id += 1;
+ else
+ id = 0;
+ *cmd_id = id;
+ }
+ val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, id, reg_addr);
- return ret;
+ return val;
}
-static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
- u8 dev_addr, u16 reg_addr,
- u32 len, u8 *rval)
+
+static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *swrm, u8 cmd_data,
+ u8 dev_addr, u16 reg_addr)
{
- int i, ret;
+
u32 val;
- DECLARE_COMPLETION_ONSTACK(comp);
- unsigned long flags;
+ int ret = 0;
+ u8 cmd_id = 0x0;
+
+ mutex_lock(&swrm->io_lock);
+ if (dev_addr == SDW_BROADCAST_DEV_NUM) {
+ cmd_id = SWR_BROADCAST_CMD_ID;
+ val = swrm_get_packed_reg_val(&cmd_id, cmd_data,
+ dev_addr, reg_addr);
+ } else {
+ val = swrm_get_packed_reg_val(&swrm->wcmd_id, cmd_data,
+ dev_addr, reg_addr);
+ }
- spin_lock_irqsave(&ctrl->comp_lock, flags);
- ctrl->comp = ∁
- spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+ swrm->reg_write(swrm, SWRM_CMD_FIFO_WR_CMD, val);
- val = SWRM_REG_VAL_PACK(len, dev_addr, SWRM_SPECIAL_CMD_ID, reg_addr);
- ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
- if (ret)
- goto err;
+ /* version 1.3 or less */
+ if (swrm->version_major == 1 && swrm->version_minor <= 3)
+ usleep_range(150, 155);
- ret = wait_for_completion_timeout(ctrl->comp,
- msecs_to_jiffies(TIMEOUT_MS));
+ if (cmd_id == SWR_BROADCAST_CMD_ID) {
+ /*
+ * sleep for 10ms for MSM soundwire variant to allow broadcast
+ * command to complete.
+ */
+ ret = wait_for_completion_timeout(&swrm->broadcast, (2 * HZ/10));
+ if (!ret)
+ ret = SDW_CMD_IGNORED;
+ else
+ ret = SDW_CMD_OK;
- if (!ret) {
- ret = SDW_CMD_IGNORED;
- goto err;
} else {
ret = SDW_CMD_OK;
}
+ mutex_unlock(&swrm->io_lock);
+ return ret;
+}
- for (i = 0; i < len; i++) {
- ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR, &val);
- rval[i] = val & 0xFF;
+static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *swrm,
+ u8 dev_addr, u16 reg_addr,
+ u32 len, u8 *rval)
+{
+ u32 val;
+ u32 retry_attempt = 0;
+ u32 cmd_data;
+ int ret = SDW_CMD_OK;
+
+ mutex_lock(&swrm->io_lock);
+ val = swrm_get_packed_reg_val(&swrm->rcmd_id, len, dev_addr, reg_addr);
+
+ /* wait for FIFO RD to complete to avoid overflow */
+ usleep_range(100, 105);
+ swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
+ /* wait for FIFO RD CMD complete to avoid overflow */
+ usleep_range(250, 255);
+
+retry_read:
+
+ swrm->reg_read(swrm, SWRM_CMD_FIFO_RD_FIFO_ADDR, &cmd_data);
+ rval[0] = cmd_data & 0xFF;
+
+ if ((((cmd_data) & 0xF00) >> 8) != swrm->rcmd_id) {
+ if (retry_attempt < MAX_FIFO_RD_FAIL_RETRY) {
+ /* wait 500 us before retry on fifo read failure */
+ usleep_range(500, 505);
+ if (retry_attempt == (MAX_FIFO_RD_FAIL_RETRY - 1)) {
+ swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
+ swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
+ }
+ retry_attempt++;
+ goto retry_read;
+ } else {
+ dev_err(swrm->dev,
+ "failed to read fifo: reg: 0x%x, \
+ rcmd_id: 0x%x, dev_num: 0x%x, cmd_data: 0x%x\n",
+ reg_addr, swrm->rcmd_id,
+ dev_addr, cmd_data);
+ ret = SDW_CMD_IGNORED;
+ }
}
-err:
- spin_lock_irqsave(&ctrl->comp_lock, flags);
- ctrl->comp = NULL;
- spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+ mutex_unlock(&swrm->io_lock);
return ret;
}
@@ -949,6 +991,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
dev_set_drvdata(&pdev->dev, ctrl);
spin_lock_init(&ctrl->comp_lock);
mutex_init(&ctrl->port_lock);
+ mutex_init(&ctrl->io_lock);
+ init_completion(&ctrl->broadcast);
INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
ctrl->bus.ops = &qcom_swrm_ops;
--
2.21.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 6/6] soundwire: qcom: add support to new interrupts
2021-01-29 17:32 [PATCH 0/6] soundwire: qcom: various improvements Srinivas Kandagatla
` (4 preceding siblings ...)
2021-01-29 17:32 ` [PATCH 5/6] soundwire: qcom: update register read/write routine Srinivas Kandagatla
@ 2021-01-29 17:32 ` Srinivas Kandagatla
2021-01-29 19:38 ` Pierre-Louis Bossart
2021-02-01 14:31 ` Vinod Koul
5 siblings, 2 replies; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-01-29 17:32 UTC (permalink / raw)
To: vkoul
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel, Srinivas Kandagatla
Add support to new interrupts and update irq routine in a way
to deal with multiple pending interrupts with in a single interrupt!
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/soundwire/qcom.c | 191 ++++++++++++++++++++++++++++++---------
1 file changed, 146 insertions(+), 45 deletions(-)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index d61b204dc284..c699bd51d29a 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -28,10 +28,21 @@
#define SWRM_COMP_PARAMS_DIN_PORTS_MASK GENMASK(9, 5)
#define SWRM_INTERRUPT_STATUS 0x200
#define SWRM_INTERRUPT_STATUS_RMSK GENMASK(16, 0)
+#define SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ BIT(0)
#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED BIT(1)
#define SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS BIT(2)
+#define SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET BIT(3)
+#define SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW BIT(4)
+#define SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW BIT(5)
+#define SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW BIT(6)
#define SWRM_INTERRUPT_STATUS_CMD_ERROR BIT(7)
+#define SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION BIT(8)
+#define SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH BIT(9)
#define SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED BIT(10)
+#define SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED_V2 BIT(13)
+#define SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED_V2 BIT(14)
+#define SWRM_INTERRUPT_STATUS_EXT_CLK_STOP_WAKEUP BIT(16)
+#define SWRM_INTERRUPT_MAX 17
#define SWRM_INTERRUPT_MASK_ADDR 0x204
#define SWRM_INTERRUPT_CLEAR 0x208
#define SWRM_INTERRUPT_CPU_EN 0x210
@@ -105,11 +116,8 @@ struct qcom_swrm_ctrl {
struct device *dev;
struct regmap *regmap;
void __iomem *mmio;
- struct completion *comp;
struct completion broadcast;
struct work_struct slave_work;
- /* read/write lock */
- spinlock_t comp_lock;
/* Port alloc/free lock */
struct mutex port_lock;
struct mutex io_lock;
@@ -126,6 +134,7 @@ struct qcom_swrm_ctrl {
int rows_index;
unsigned long dout_port_mask;
unsigned long din_port_mask;
+ u32 intr_mask;
u8 rcmd_id;
u8 wcmd_id;
struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
@@ -315,6 +324,27 @@ static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *swrm,
return ret;
}
+static int qcom_swrm_get_alert_slave(struct qcom_swrm_ctrl *ctrl)
+{
+ u32 val;
+ int i;
+
+ ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val);
+
+ for (i = 0; i < SDW_MAX_DEVICES; i++) {
+ u32 s;
+
+ s = (val >> (i * 2));
+
+ if ((s & SWRM_MCP_SLV_STATUS_MASK) == SDW_SLAVE_ALERT) {
+ ctrl->status[i] = s;
+ return i;
+ }
+ }
+
+ return -EINVAL;
+}
+
static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
{
u32 val;
@@ -333,40 +363,122 @@ static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
{
- struct qcom_swrm_ctrl *ctrl = dev_id;
- u32 sts, value;
- unsigned long flags;
-
- ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS, &sts);
-
- if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
- ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
- dev_err_ratelimited(ctrl->dev,
- "CMD error, fifo status 0x%x\n",
- value);
- ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
- }
+ struct qcom_swrm_ctrl *swrm = dev_id;
+ u32 value, intr_sts, intr_sts_masked;
+ u32 i;
+ u8 devnum = 0;
+ int ret = IRQ_HANDLED;
+
+
+ swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, &intr_sts);
+ intr_sts_masked = intr_sts & swrm->intr_mask;
+
+handle_irq:
+ for (i = 0; i < SWRM_INTERRUPT_MAX; i++) {
+ value = intr_sts_masked & (1 << i);
+ if (!value)
+ continue;
+
+ switch (value) {
+ case SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ:
+ devnum = qcom_swrm_get_alert_slave(swrm);
+ if (devnum < 0) {
+ dev_err_ratelimited(swrm->dev,
+ "no slave alert found.spurious interrupt\n");
+ } else {
+ sdw_handle_slave_status(&swrm->bus, swrm->status);
+ }
- if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
- sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS)
- schedule_work(&ctrl->slave_work);
-
- /**
- * clear the interrupt before complete() is called, as complete can
- * schedule new read/writes which require interrupts, clearing the
- * interrupt would avoid missing interrupts in such cases.
- */
- ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
-
- if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED) {
- spin_lock_irqsave(&ctrl->comp_lock, flags);
- if (ctrl->comp)
- complete(ctrl->comp);
- spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+ break;
+ case SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED:
+ case SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS:
+ dev_err_ratelimited(swrm->dev, "%s: SWR new slave attached\n",
+ __func__);
+ qcom_swrm_get_device_status(swrm);
+ sdw_handle_slave_status(&swrm->bus, swrm->status);
+ break;
+ case SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET:
+ dev_err_ratelimited(swrm->dev,
+ "%s: SWR bus clsh detected\n",
+ __func__);
+ swrm->intr_mask &=
+ ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
+ swrm->reg_write(swrm,
+ SWRM_INTERRUPT_CPU_EN,
+ swrm->intr_mask);
+ break;
+ case SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW:
+ swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
+ dev_err_ratelimited(swrm->dev,
+ "%s: SWR read FIFO overflow fifo status 0x%x\n",
+ __func__, value);
+ break;
+ case SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW:
+ swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
+ dev_err_ratelimited(swrm->dev,
+ "%s: SWR read FIFO underflow fifo status 0x%x\n",
+ __func__, value);
+ break;
+ case SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW:
+ swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
+ dev_err(swrm->dev,
+ "%s: SWR write FIFO overflow fifo status %x\n",
+ __func__, value);
+ swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
+ break;
+ case SWRM_INTERRUPT_STATUS_CMD_ERROR:
+ swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
+ dev_err_ratelimited(swrm->dev,
+ "%s: SWR CMD error, fifo status 0x%x, flushing fifo\n",
+ __func__, value);
+ swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
+ break;
+ case SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION:
+ dev_err_ratelimited(swrm->dev,
+ "%s: SWR Port collision detected\n",
+ __func__);
+ swrm->intr_mask &= ~SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION;
+ swrm->reg_write(swrm,
+ SWRM_INTERRUPT_CPU_EN, swrm->intr_mask);
+ break;
+ case SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH:
+ dev_err_ratelimited(swrm->dev,
+ "%s: SWR read enable valid mismatch\n",
+ __func__);
+ swrm->intr_mask &=
+ ~SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH;
+ swrm->reg_write(swrm,
+ SWRM_INTERRUPT_CPU_EN, swrm->intr_mask);
+ break;
+ case SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED:
+ complete(&swrm->broadcast);
+ break;
+ case SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED_V2:
+ break;
+ case SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED_V2:
+ break;
+ case SWRM_INTERRUPT_STATUS_EXT_CLK_STOP_WAKEUP:
+ break;
+ default:
+ dev_err_ratelimited(swrm->dev,
+ "%s: SWR unknown interrupt value: %d\n",
+ __func__, value);
+ ret = IRQ_NONE;
+ break;
+ }
}
+ swrm->reg_write(swrm, SWRM_INTERRUPT_CLEAR, intr_sts);
+ swrm->reg_write(swrm, SWRM_INTERRUPT_CLEAR, 0x0);
+
+ swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, &intr_sts);
+ intr_sts_masked = intr_sts & swrm->intr_mask;
+
+ if (intr_sts_masked)
+ goto handle_irq;
- return IRQ_HANDLED;
+ return ret;
}
+
static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
{
u32 val;
@@ -380,6 +492,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
/* Disable Auto enumeration */
ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);
+ ctrl->intr_mask = SWRM_INTERRUPT_STATUS_RMSK;
/* Mask soundwire interrupts */
ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
SWRM_INTERRUPT_STATUS_RMSK);
@@ -615,16 +728,6 @@ static u32 qcom_swrm_freq_tbl[MAX_FREQ_NUM] = {
DEFAULT_CLK_FREQ,
};
-static void qcom_swrm_slave_wq(struct work_struct *work)
-{
- struct qcom_swrm_ctrl *ctrl =
- container_of(work, struct qcom_swrm_ctrl, slave_work);
-
- qcom_swrm_get_device_status(ctrl);
- sdw_handle_slave_status(&ctrl->bus, ctrl->status);
-}
-
-
static void qcom_swrm_stream_free_ports(struct qcom_swrm_ctrl *ctrl,
struct sdw_stream_runtime *stream)
{
@@ -989,11 +1092,9 @@ static int qcom_swrm_probe(struct platform_device *pdev)
ctrl->dev = dev;
dev_set_drvdata(&pdev->dev, ctrl);
- spin_lock_init(&ctrl->comp_lock);
mutex_init(&ctrl->port_lock);
mutex_init(&ctrl->io_lock);
init_completion(&ctrl->broadcast);
- INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
ctrl->bus.ops = &qcom_swrm_ops;
ctrl->bus.port_ops = &qcom_swrm_port_ops;
--
2.21.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 1/6] soundwire: qcom: add support to missing transport params
2021-01-29 17:32 ` [PATCH 1/6] soundwire: qcom: add support to missing transport params Srinivas Kandagatla
@ 2021-01-29 19:20 ` Pierre-Louis Bossart
2021-02-01 15:50 ` Srinivas Kandagatla
2021-02-01 14:13 ` Vinod Koul
1 sibling, 1 reply; 30+ messages in thread
From: Pierre-Louis Bossart @ 2021-01-29 19:20 UTC (permalink / raw)
To: Srinivas Kandagatla, vkoul
Cc: yung-chuan.liao, sanyog.r.kale, linux-kernel, alsa-devel
> struct qcom_swrm_port_config {
> u8 si;
> u8 off1;
> u8 off2;
> u8 bp_mode;
> + u8 hstart;
> + u8 hstop;
> + u8 word_length;
> + u8 bgp_count;
I couldn't figure out what 'bgp' was and had to search. Not sure how you
came up with this abbreviation of "qcom,ports-block-group-count". Adding
a comment wouldn't hurt.
> + u8 lane_control;
Are you able to use lane_control != 0 ? I thought we were missing stuff
at the bus.c level?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/6] soundwire: qcom: set continue execution flag for ignored commands
2021-01-29 17:32 ` [PATCH 3/6] soundwire: qcom: set continue execution flag for ignored commands Srinivas Kandagatla
@ 2021-01-29 19:21 ` Pierre-Louis Bossart
2021-02-01 14:16 ` Vinod Koul
1 sibling, 0 replies; 30+ messages in thread
From: Pierre-Louis Bossart @ 2021-01-29 19:21 UTC (permalink / raw)
To: Srinivas Kandagatla, vkoul
Cc: yung-chuan.liao, sanyog.r.kale, linux-kernel, alsa-devel
On 1/29/21 11:32 AM, Srinivas Kandagatla wrote:
> version 1.5.1 and higher IPs of this controller required to set
> continue execution on ingored command flag. This patch sets this flag.
typo: ignored.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 5/6] soundwire: qcom: update register read/write routine
2021-01-29 17:32 ` [PATCH 5/6] soundwire: qcom: update register read/write routine Srinivas Kandagatla
@ 2021-01-29 19:33 ` Pierre-Louis Bossart
2021-02-01 15:50 ` Srinivas Kandagatla
2021-02-01 14:26 ` Vinod Koul
1 sibling, 1 reply; 30+ messages in thread
From: Pierre-Louis Bossart @ 2021-01-29 19:33 UTC (permalink / raw)
To: Srinivas Kandagatla, vkoul
Cc: yung-chuan.liao, sanyog.r.kale, linux-kernel, alsa-devel
On 1/29/21 11:32 AM, Srinivas Kandagatla wrote:
> In the existing code every soundwire register read and register write
> are kinda blocked. Each of these are using a special command id that
what does 'kinda blocked' mean?
> generates interrupt after it successfully finishes. This is really
> overhead, limiting and not really necessary unless we are doing
> something special.
>
> We can simply read/write the fifo that should also give exactly
> what we need! This will also allow to read/write registers in
> interrupt context, which was not possible with the special
> command approach.
This is really unclear, sorry.
> + if (id != SWR_BROADCAST_CMD_ID) {
> + if (id < 14)
> + id += 1;
> + else
> + id = 0;
that is really odd. if id=13 (group2) then id becomes 14 (master
address). A comment is really needed here.
> + if (cmd_id == SWR_BROADCAST_CMD_ID) {
> + /*
> + * sleep for 10ms for MSM soundwire variant to allow broadcast
> + * command to complete.
that's also super-odd. There is nothing in SoundWire that makes any
difference between a regular and a broadcast command. they all complete
in the same time (a frame).
> + */
> + ret = wait_for_completion_timeout(&swrm->broadcast, (2 * HZ/10));
is this 10ms really or dependent on CONFIG_HZ?
> + if (!ret)
> + ret = SDW_CMD_IGNORED;
> + else
> + ret = SDW_CMD_OK;
no CMD_FAILED support?
> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *swrm,
> + u8 dev_addr, u16 reg_addr,
> + u32 len, u8 *rval)
> +{
> + u32 val;
> + u32 retry_attempt = 0;
> + u32 cmd_data;
> + int ret = SDW_CMD_OK;
> +
> + mutex_lock(&swrm->io_lock);
> + val = swrm_get_packed_reg_val(&swrm->rcmd_id, len, dev_addr, reg_addr);
> +
> + /* wait for FIFO RD to complete to avoid overflow */
> + usleep_range(100, 105);
> + swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
> + /* wait for FIFO RD CMD complete to avoid overflow */
> + usleep_range(250, 255);
> +
> +retry_read:
> +
> + swrm->reg_read(swrm, SWRM_CMD_FIFO_RD_FIFO_ADDR, &cmd_data);
> + rval[0] = cmd_data & 0xFF;
> +
> + if ((((cmd_data) & 0xF00) >> 8) != swrm->rcmd_id) {
> + if (retry_attempt < MAX_FIFO_RD_FAIL_RETRY) {
> + /* wait 500 us before retry on fifo read failure */
> + usleep_range(500, 505);
> + if (retry_attempt == (MAX_FIFO_RD_FAIL_RETRY - 1)) {
> + swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
> + swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
> + }
> + retry_attempt++;
> + goto retry_read;
> + } else {
> + dev_err(swrm->dev,
> + "failed to read fifo: reg: 0x%x, \
> + rcmd_id: 0x%x, dev_num: 0x%x, cmd_data: 0x%x\n",
> + reg_addr, swrm->rcmd_id,
> + dev_addr, cmd_data);
> + ret = SDW_CMD_IGNORED;
> + }
> }
the flow seems complicated with multiple tests and goto? Can this be
simplified?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] soundwire: qcom: add support to new interrupts
2021-01-29 17:32 ` [PATCH 6/6] soundwire: qcom: add support to new interrupts Srinivas Kandagatla
@ 2021-01-29 19:38 ` Pierre-Louis Bossart
2021-02-01 15:51 ` Srinivas Kandagatla
2021-02-01 14:31 ` Vinod Koul
1 sibling, 1 reply; 30+ messages in thread
From: Pierre-Louis Bossart @ 2021-01-29 19:38 UTC (permalink / raw)
To: Srinivas Kandagatla, vkoul
Cc: alsa-devel, linux-kernel, sanyog.r.kale, yung-chuan.liao
On 1/29/21 11:32 AM, Srinivas Kandagatla wrote:
> Add support to new interrupts and update irq routine in a way
> to deal with multiple pending interrupts with in a single interrupt!
I can't parse the wording after 'update irq routine'.
> + swrm->reg_write(swrm, SWRM_INTERRUPT_CLEAR, intr_sts);
> + swrm->reg_write(swrm, SWRM_INTERRUPT_CLEAR, 0x0);
what does this second write 0x0 do? Usually interrupts are W1C, and you
didn't have this before.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/6] soundwire: qcom: add support to missing transport params
2021-01-29 17:32 ` [PATCH 1/6] soundwire: qcom: add support to missing transport params Srinivas Kandagatla
2021-01-29 19:20 ` Pierre-Louis Bossart
@ 2021-02-01 14:13 ` Vinod Koul
2021-02-01 15:50 ` Srinivas Kandagatla
1 sibling, 1 reply; 30+ messages in thread
From: Vinod Koul @ 2021-02-01 14:13 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel
On 29-01-21, 17:32, Srinivas Kandagatla wrote:
> Some of the transport parameters derived from device tree
> are not fully parsed by the driver.
>
> This patch adds support to parse those missing parameters.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> drivers/soundwire/qcom.c | 107 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 103 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 6d22df01f354..36e273795cbe 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -54,7 +54,13 @@
> #define SWRM_MCP_SLV_STATUS 0x1090
> #define SWRM_MCP_SLV_STATUS_MASK GENMASK(1, 0)
> #define SWRM_DP_PORT_CTRL_BANK(n, m) (0x1124 + 0x100 * (n - 1) + 0x40 * m)
> +#define SWRM_DP_PORT_CTRL_2_BANK(n, m) (0x1128 + 0x100 * (n - 1) + 0x40 * m)
> +#define SWRM_DP_BLOCK_CTRL_1(n) (0x112C + 0x100 * (n - 1))
> +#define SWRM_DP_BLOCK_CTRL2_BANK(n, m) (0x1130 + 0x100 * (n - 1) + 0x40 * m)
> +#define SWRM_DP_PORT_HCTRL_BANK(n, m) (0x1134 + 0x100 * (n - 1) + 0x40 * m)
> #define SWRM_DP_BLOCK_CTRL3_BANK(n, m) (0x1138 + 0x100 * (n - 1) + 0x40 * m)
> +#define SWRM_DIN_DPn_PCM_PORT_CTRL(n) (0x1054 + 0x100 * (n - 1))
> +
> #define SWRM_DP_PORT_CTRL_EN_CHAN_SHFT 0x18
> #define SWRM_DP_PORT_CTRL_OFFSET2_SHFT 0x10
> #define SWRM_DP_PORT_CTRL_OFFSET1_SHFT 0x08
> @@ -73,12 +79,20 @@
> #define QCOM_SDW_MAX_PORTS 14
> #define DEFAULT_CLK_FREQ 9600000
> #define SWRM_MAX_DAIS 0xF
> +#define SWR_INVALID_PARAM 0xFF
> +#define SWR_HSTOP_MAX_VAL 0xF
> +#define SWR_HSTART_MIN_VAL 0x0
>
> struct qcom_swrm_port_config {
> u8 si;
> u8 off1;
> u8 off2;
> u8 bp_mode;
> + u8 hstart;
> + u8 hstop;
> + u8 word_length;
> + u8 bgp_count;
> + u8 lane_control;
> };
>
> struct qcom_swrm_ctrl {
> @@ -396,7 +410,13 @@ static int qcom_swrm_port_params(struct sdw_bus *bus,
> struct sdw_port_params *p_params,
> unsigned int bank)
> {
> - /* TBD */
> + struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +
> + if (p_params->bps != SWR_INVALID_PARAM)
> + return ctrl->reg_write(ctrl,
> + SWRM_DP_BLOCK_CTRL_1(p_params->num),
> + p_params->bps - 1);
> +
> return 0;
> }
>
> @@ -415,10 +435,32 @@ static int qcom_swrm_transport_params(struct sdw_bus *bus,
>
> ret = ctrl->reg_write(ctrl, reg, value);
>
> - if (!ret && params->blk_pkg_mode) {
> - reg = SWRM_DP_BLOCK_CTRL3_BANK(params->port_num, bank);
> + if (params->lane_ctrl != SWR_INVALID_PARAM) {
> + reg = SWRM_DP_PORT_CTRL_2_BANK(params->port_num, bank);
> + value = params->lane_ctrl;
> + ret = ctrl->reg_write(ctrl, reg, value);
> + }
>
> - ret = ctrl->reg_write(ctrl, reg, 1);
> + if (params->blk_grp_ctrl != SWR_INVALID_PARAM) {
> + reg = SWRM_DP_BLOCK_CTRL2_BANK(params->port_num, bank);
> + value = params->blk_grp_ctrl;
> + ret = ctrl->reg_write(ctrl, reg, value);
> + }
> +
> + if (params->hstart != SWR_INVALID_PARAM
> + && params->hstop != SWR_INVALID_PARAM) {
> + reg = SWRM_DP_PORT_HCTRL_BANK(params->port_num, bank);
> + value = (params->hstop << 4) | params->hstart;
> + ret = ctrl->reg_write(ctrl, reg, value);
> + } else {
> + reg = SWRM_DP_PORT_HCTRL_BANK(params->port_num, bank);
> + value = (SWR_HSTOP_MAX_VAL << 4) | SWR_HSTART_MIN_VAL;
> + ret = ctrl->reg_write(ctrl, reg, value);
> + }
> +
> + if (params->blk_pkg_mode != SWR_INVALID_PARAM) {
> + reg = SWRM_DP_BLOCK_CTRL3_BANK(params->port_num, bank);
> + ret = ctrl->reg_write(ctrl, reg, params->blk_pkg_mode);
> }
>
> return ret;
> @@ -470,6 +512,17 @@ static int qcom_swrm_compute_params(struct sdw_bus *bus)
> p_rt->transport_params.offset1 = pcfg->off1;
> p_rt->transport_params.offset2 = pcfg->off2;
> p_rt->transport_params.blk_pkg_mode = pcfg->bp_mode;
> + p_rt->transport_params.blk_grp_ctrl = pcfg->bgp_count;
> + p_rt->transport_params.hstart = pcfg->hstart;
> + p_rt->transport_params.hstop = pcfg->hstop;
> + p_rt->transport_params.lane_ctrl = pcfg->lane_control;
> + if (pcfg->word_length != SWR_INVALID_PARAM) {
> + sdw_fill_port_params(&p_rt->port_params,
> + p_rt->num, pcfg->word_length + 1,
> + SDW_PORT_FLOW_MODE_ISOCH,
> + SDW_PORT_DATA_MODE_NORMAL);
> + }
> +
> }
>
> list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> @@ -481,6 +534,18 @@ static int qcom_swrm_compute_params(struct sdw_bus *bus)
> p_rt->transport_params.offset1 = pcfg->off1;
> p_rt->transport_params.offset2 = pcfg->off2;
> p_rt->transport_params.blk_pkg_mode = pcfg->bp_mode;
> + p_rt->transport_params.blk_grp_ctrl = pcfg->bgp_count;
> +
> + p_rt->transport_params.hstart = pcfg->hstart;
> + p_rt->transport_params.hstop = pcfg->hstop;
> + p_rt->transport_params.lane_ctrl = pcfg->lane_control;
> + if (pcfg->word_length != SWR_INVALID_PARAM) {
> + sdw_fill_port_params(&p_rt->port_params,
> + p_rt->num,
> + pcfg->word_length + 1,
> + SDW_PORT_FLOW_MODE_ISOCH,
> + SDW_PORT_DATA_MODE_NORMAL);
> + }
> i++;
> }
> }
> @@ -728,6 +793,11 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
> u8 off2[QCOM_SDW_MAX_PORTS];
> u8 si[QCOM_SDW_MAX_PORTS];
> u8 bp_mode[QCOM_SDW_MAX_PORTS] = { 0, };
> + u8 hstart[QCOM_SDW_MAX_PORTS];
> + u8 hstop[QCOM_SDW_MAX_PORTS];
> + u8 word_length[QCOM_SDW_MAX_PORTS];
> + u8 bgp_count[QCOM_SDW_MAX_PORTS];
> + u8 lane_control[QCOM_SDW_MAX_PORTS];
> int i, ret, nports, val;
>
> ctrl->reg_read(ctrl, SWRM_COMP_PARAMS, &val);
> @@ -772,11 +842,40 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
>
> ret = of_property_read_u8_array(np, "qcom,ports-block-pack-mode",
> bp_mode, nports);
> +
> + ret = of_property_read_u8_array(np, "qcom,ports-hstart", hstart, nports);
> + if (ret)
> + memset(hstart, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
> +
> + ret = of_property_read_u8_array(np, "qcom,ports-hstop", hstop, nports);
> + if (ret)
> + memset(hstop, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
why not memset the whole area here and then populate it..?
> +
> + ret = of_property_read_u8_array(np, "qcom,ports-word-length",
> + word_length, nports);
> + if (ret)
> + memset(word_length, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
> +
> + ret = of_property_read_u8_array(np, "qcom,ports-block-group-count",
> + bgp_count, nports);
> + if (ret)
> + memset(bgp_count, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
> +
> + ret = of_property_read_u8_array(np, "qcom,ports-lane-control",
> + lane_control, nports);
> + if (ret)
> + memset(lane_control, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
> +
> for (i = 0; i < nports; i++) {
> ctrl->pconfig[i].si = si[i];
> ctrl->pconfig[i].off1 = off1[i];
> ctrl->pconfig[i].off2 = off2[i];
> ctrl->pconfig[i].bp_mode = bp_mode[i];
> + ctrl->pconfig[i].hstart = hstart[i];
> + ctrl->pconfig[i].hstop = hstop[i];
> + ctrl->pconfig[i].word_length = word_length[i];
> + ctrl->pconfig[i].bgp_count = bgp_count[i];
> + ctrl->pconfig[i].lane_control = lane_control[i];
--
~Vinod
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/6] soundwire: qcom: set continue execution flag for ignored commands
2021-01-29 17:32 ` [PATCH 3/6] soundwire: qcom: set continue execution flag for ignored commands Srinivas Kandagatla
2021-01-29 19:21 ` Pierre-Louis Bossart
@ 2021-02-01 14:16 ` Vinod Koul
2021-02-01 15:50 ` Srinivas Kandagatla
1 sibling, 1 reply; 30+ messages in thread
From: Vinod Koul @ 2021-02-01 14:16 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel
On 29-01-21, 17:32, Srinivas Kandagatla wrote:
> version 1.5.1 and higher IPs of this controller required to set
> continue execution on ingored command flag. This patch sets this flag.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> drivers/soundwire/qcom.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index da6e0d4e9622..3669bac11a32 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -40,6 +40,7 @@
> #define SWRM_CMD_FIFO_CMD 0x308
> #define SWRM_CMD_FIFO_STATUS 0x30C
> #define SWRM_CMD_FIFO_CFG_ADDR 0x314
> +#define SWRM_CONTINUE_EXEC_ON_CMD_IGNORE BIT(31)
> #define SWRM_RD_WR_CMD_RETRIES 0x7
> #define SWRM_CMD_FIFO_RD_FIFO_ADDR 0x318
> #define SWRM_ENUMERATOR_CFG_ADDR 0x500
> @@ -345,7 +346,16 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
> ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
>
> /* Configure number of retries of a read/write cmd */
> - ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, SWRM_RD_WR_CMD_RETRIES);
> + if (ctrl->version_major == 1 && ctrl->version_minor >= 5 &&
> + ctrl->version_step >= 1) {
why not use raw version value?
if (ctrl->raw > 0x10501 )
> + /* Only for versions >= 1.5.1 */
> + ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR,
> + SWRM_RD_WR_CMD_RETRIES |
> + SWRM_CONTINUE_EXEC_ON_CMD_IGNORE);
> + } else {
> + ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR,
> + SWRM_RD_WR_CMD_RETRIES);
> + }
>
> /* Set IRQ to PULSE */
> ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
> --
> 2.21.0
--
~Vinod
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/6] soundwire: qcom: start the clock during initialization
2021-01-29 17:32 ` [PATCH 4/6] soundwire: qcom: start the clock during initialization Srinivas Kandagatla
@ 2021-02-01 14:21 ` Vinod Koul
2021-02-01 15:50 ` Srinivas Kandagatla
0 siblings, 1 reply; 30+ messages in thread
From: Vinod Koul @ 2021-02-01 14:21 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel
On 29-01-21, 17:32, Srinivas Kandagatla wrote:
> Start the clock during initialization.
A detailed log please, which clock..? Also how do older controllers work
w/o this clk
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> drivers/soundwire/qcom.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 3669bac11a32..83df15d83935 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -47,6 +47,8 @@
> #define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m) (0x101C + 0x40 * (m))
> #define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK GENMASK(2, 0)
> #define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK GENMASK(7, 3)
> +#define SWRM_MCP_BUS_CTRL 0x1044
> +#define SWRM_MCP_BUS_CLK_START BIT(1)
> #define SWRM_MCP_CFG_ADDR 0x1048
> #define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK GENMASK(21, 17)
> #define SWRM_DEF_CMD_NO_PINGS 0x1f
> @@ -345,6 +347,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
> u32p_replace_bits(&val, SWRM_DEF_CMD_NO_PINGS, SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK);
> ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
>
> + ctrl->reg_write(ctrl, SWRM_MCP_BUS_CTRL, SWRM_MCP_BUS_CLK_START);
> /* Configure number of retries of a read/write cmd */
> if (ctrl->version_major == 1 && ctrl->version_minor >= 5 &&
> ctrl->version_step >= 1) {
> --
> 2.21.0
--
~Vinod
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 5/6] soundwire: qcom: update register read/write routine
2021-01-29 17:32 ` [PATCH 5/6] soundwire: qcom: update register read/write routine Srinivas Kandagatla
2021-01-29 19:33 ` Pierre-Louis Bossart
@ 2021-02-01 14:26 ` Vinod Koul
2021-02-01 15:50 ` Srinivas Kandagatla
1 sibling, 1 reply; 30+ messages in thread
From: Vinod Koul @ 2021-02-01 14:26 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel
On 29-01-21, 17:32, Srinivas Kandagatla wrote:
> In the existing code every soundwire register read and register write
> are kinda blocked. Each of these are using a special command id that
> generates interrupt after it successfully finishes. This is really
> overhead, limiting and not really necessary unless we are doing
> something special.
>
> We can simply read/write the fifo that should also give exactly
> what we need! This will also allow to read/write registers in
> interrupt context, which was not possible with the special
Okay but then why use a mutex ?
> command approach.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> drivers/soundwire/qcom.c | 148 +++++++++++++++++++++++++--------------
> 1 file changed, 96 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 83df15d83935..d61b204dc284 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -78,13 +78,15 @@
> #define SWRM_SPECIAL_CMD_ID 0xF
> #define MAX_FREQ_NUM 1
> #define TIMEOUT_MS (2 * HZ)
> -#define QCOM_SWRM_MAX_RD_LEN 0xf
> +#define QCOM_SWRM_MAX_RD_LEN 0x1
> #define QCOM_SDW_MAX_PORTS 14
> #define DEFAULT_CLK_FREQ 9600000
> #define SWRM_MAX_DAIS 0xF
> #define SWR_INVALID_PARAM 0xFF
> #define SWR_HSTOP_MAX_VAL 0xF
> #define SWR_HSTART_MIN_VAL 0x0
> +#define SWR_BROADCAST_CMD_ID 0x0F
> +#define MAX_FIFO_RD_FAIL_RETRY 3
>
> struct qcom_swrm_port_config {
> u8 si;
> @@ -104,11 +106,13 @@ struct qcom_swrm_ctrl {
> struct regmap *regmap;
> void __iomem *mmio;
> struct completion *comp;
> + struct completion broadcast;
> struct work_struct slave_work;
> /* read/write lock */
> spinlock_t comp_lock;
> /* Port alloc/free lock */
> struct mutex port_lock;
> + struct mutex io_lock;
> struct clk *hclk;
> u8 wr_cmd_id;
> u8 rd_cmd_id;
> @@ -122,6 +126,8 @@ struct qcom_swrm_ctrl {
> int rows_index;
> unsigned long dout_port_mask;
> unsigned long din_port_mask;
> + u8 rcmd_id;
> + u8 wcmd_id;
> struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
> struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
> enum sdw_slave_status status[SDW_MAX_DEVICES];
> @@ -200,75 +206,111 @@ static int qcom_swrm_cpu_reg_write(struct qcom_swrm_ctrl *ctrl, int reg,
> return SDW_CMD_OK;
> }
>
> -static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
> - u8 dev_addr, u16 reg_addr)
> +static u32 swrm_get_packed_reg_val(u8 *cmd_id, u8 cmd_data,
> + u8 dev_addr, u16 reg_addr)
> {
> - DECLARE_COMPLETION_ONSTACK(comp);
> - unsigned long flags;
> u32 val;
> - int ret;
> -
> - spin_lock_irqsave(&ctrl->comp_lock, flags);
> - ctrl->comp = ∁
> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
> - val = SWRM_REG_VAL_PACK(cmd_data, dev_addr,
> - SWRM_SPECIAL_CMD_ID, reg_addr);
> - ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
> - if (ret)
> - goto err;
> -
> - ret = wait_for_completion_timeout(ctrl->comp,
> - msecs_to_jiffies(TIMEOUT_MS));
> + u8 id = *cmd_id;
>
> - if (!ret)
> - ret = SDW_CMD_IGNORED;
> - else
> - ret = SDW_CMD_OK;
> -err:
> - spin_lock_irqsave(&ctrl->comp_lock, flags);
> - ctrl->comp = NULL;
> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
> + if (id != SWR_BROADCAST_CMD_ID) {
> + if (id < 14)
> + id += 1;
> + else
> + id = 0;
> + *cmd_id = id;
> + }
> + val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, id, reg_addr);
>
> - return ret;
> + return val;
> }
>
> -static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
> - u8 dev_addr, u16 reg_addr,
> - u32 len, u8 *rval)
> +
> +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *swrm, u8 cmd_data,
> + u8 dev_addr, u16 reg_addr)
> {
> - int i, ret;
> +
> u32 val;
> - DECLARE_COMPLETION_ONSTACK(comp);
> - unsigned long flags;
> + int ret = 0;
> + u8 cmd_id = 0x0;
> +
> + mutex_lock(&swrm->io_lock);
> + if (dev_addr == SDW_BROADCAST_DEV_NUM) {
> + cmd_id = SWR_BROADCAST_CMD_ID;
> + val = swrm_get_packed_reg_val(&cmd_id, cmd_data,
> + dev_addr, reg_addr);
> + } else {
> + val = swrm_get_packed_reg_val(&swrm->wcmd_id, cmd_data,
> + dev_addr, reg_addr);
> + }
>
> - spin_lock_irqsave(&ctrl->comp_lock, flags);
> - ctrl->comp = ∁
> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
> + swrm->reg_write(swrm, SWRM_CMD_FIFO_WR_CMD, val);
>
> - val = SWRM_REG_VAL_PACK(len, dev_addr, SWRM_SPECIAL_CMD_ID, reg_addr);
> - ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
> - if (ret)
> - goto err;
> + /* version 1.3 or less */
> + if (swrm->version_major == 1 && swrm->version_minor <= 3)
> + usleep_range(150, 155);
>
> - ret = wait_for_completion_timeout(ctrl->comp,
> - msecs_to_jiffies(TIMEOUT_MS));
> + if (cmd_id == SWR_BROADCAST_CMD_ID) {
> + /*
> + * sleep for 10ms for MSM soundwire variant to allow broadcast
> + * command to complete.
> + */
> + ret = wait_for_completion_timeout(&swrm->broadcast, (2 * HZ/10));
> + if (!ret)
> + ret = SDW_CMD_IGNORED;
> + else
> + ret = SDW_CMD_OK;
>
> - if (!ret) {
> - ret = SDW_CMD_IGNORED;
> - goto err;
> } else {
> ret = SDW_CMD_OK;
> }
> + mutex_unlock(&swrm->io_lock);
> + return ret;
> +}
>
> - for (i = 0; i < len; i++) {
> - ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR, &val);
> - rval[i] = val & 0xFF;
> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *swrm,
> + u8 dev_addr, u16 reg_addr,
> + u32 len, u8 *rval)
> +{
> + u32 val;
> + u32 retry_attempt = 0;
> + u32 cmd_data;
> + int ret = SDW_CMD_OK;
> +
> + mutex_lock(&swrm->io_lock);
> + val = swrm_get_packed_reg_val(&swrm->rcmd_id, len, dev_addr, reg_addr);
> +
> + /* wait for FIFO RD to complete to avoid overflow */
> + usleep_range(100, 105);
> + swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
> + /* wait for FIFO RD CMD complete to avoid overflow */
> + usleep_range(250, 255);
> +
> +retry_read:
do while{} ?
> +
> + swrm->reg_read(swrm, SWRM_CMD_FIFO_RD_FIFO_ADDR, &cmd_data);
> + rval[0] = cmd_data & 0xFF;
> +
> + if ((((cmd_data) & 0xF00) >> 8) != swrm->rcmd_id) {
> + if (retry_attempt < MAX_FIFO_RD_FAIL_RETRY) {
> + /* wait 500 us before retry on fifo read failure */
> + usleep_range(500, 505);
> + if (retry_attempt == (MAX_FIFO_RD_FAIL_RETRY - 1)) {
why not do this at the end if retry fails, that will make code look
neater
> + swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
> + swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
> + }
> + retry_attempt++;
> + goto retry_read;
> + } else {
> + dev_err(swrm->dev,
> + "failed to read fifo: reg: 0x%x, \
> + rcmd_id: 0x%x, dev_num: 0x%x, cmd_data: 0x%x\n",
> + reg_addr, swrm->rcmd_id,
> + dev_addr, cmd_data);
Do you want to log retry as err..?
> + ret = SDW_CMD_IGNORED;
> + }
> }
>
> -err:
> - spin_lock_irqsave(&ctrl->comp_lock, flags);
> - ctrl->comp = NULL;
> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
> + mutex_unlock(&swrm->io_lock);
>
> return ret;
> }
> @@ -949,6 +991,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
> dev_set_drvdata(&pdev->dev, ctrl);
> spin_lock_init(&ctrl->comp_lock);
> mutex_init(&ctrl->port_lock);
> + mutex_init(&ctrl->io_lock);
> + init_completion(&ctrl->broadcast);
> INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
>
> ctrl->bus.ops = &qcom_swrm_ops;
> --
> 2.21.0
--
~Vinod
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] soundwire: qcom: add support to new interrupts
2021-01-29 17:32 ` [PATCH 6/6] soundwire: qcom: add support to new interrupts Srinivas Kandagatla
2021-01-29 19:38 ` Pierre-Louis Bossart
@ 2021-02-01 14:31 ` Vinod Koul
2021-02-01 15:51 ` Srinivas Kandagatla
1 sibling, 1 reply; 30+ messages in thread
From: Vinod Koul @ 2021-02-01 14:31 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel
On 29-01-21, 17:32, Srinivas Kandagatla wrote:
> Add support to new interrupts and update irq routine in a way
> to deal with multiple pending interrupts with in a single interrupt!
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> drivers/soundwire/qcom.c | 191 ++++++++++++++++++++++++++++++---------
> 1 file changed, 146 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index d61b204dc284..c699bd51d29a 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -28,10 +28,21 @@
> #define SWRM_COMP_PARAMS_DIN_PORTS_MASK GENMASK(9, 5)
> #define SWRM_INTERRUPT_STATUS 0x200
> #define SWRM_INTERRUPT_STATUS_RMSK GENMASK(16, 0)
> +#define SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ BIT(0)
> #define SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED BIT(1)
> #define SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS BIT(2)
> +#define SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET BIT(3)
> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW BIT(4)
> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW BIT(5)
> +#define SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW BIT(6)
> #define SWRM_INTERRUPT_STATUS_CMD_ERROR BIT(7)
> +#define SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION BIT(8)
> +#define SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH BIT(9)
> #define SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED BIT(10)
> +#define SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED_V2 BIT(13)
> +#define SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED_V2 BIT(14)
> +#define SWRM_INTERRUPT_STATUS_EXT_CLK_STOP_WAKEUP BIT(16)
> +#define SWRM_INTERRUPT_MAX 17
> #define SWRM_INTERRUPT_MASK_ADDR 0x204
> #define SWRM_INTERRUPT_CLEAR 0x208
> #define SWRM_INTERRUPT_CPU_EN 0x210
> @@ -105,11 +116,8 @@ struct qcom_swrm_ctrl {
> struct device *dev;
> struct regmap *regmap;
> void __iomem *mmio;
> - struct completion *comp;
> struct completion broadcast;
> struct work_struct slave_work;
> - /* read/write lock */
> - spinlock_t comp_lock;
> /* Port alloc/free lock */
> struct mutex port_lock;
> struct mutex io_lock;
> @@ -126,6 +134,7 @@ struct qcom_swrm_ctrl {
> int rows_index;
> unsigned long dout_port_mask;
> unsigned long din_port_mask;
> + u32 intr_mask;
> u8 rcmd_id;
> u8 wcmd_id;
> struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
> @@ -315,6 +324,27 @@ static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *swrm,
> return ret;
> }
>
> +static int qcom_swrm_get_alert_slave(struct qcom_swrm_ctrl *ctrl)
> +{
> + u32 val;
> + int i;
> +
> + ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val);
> +
> + for (i = 0; i < SDW_MAX_DEVICES; i++) {
> + u32 s;
define at top of the function pls, also maybe better name status?
> +
> + s = (val >> (i * 2));
why * 2 ? Maybe add a comment for this logic
> +
> + if ((s & SWRM_MCP_SLV_STATUS_MASK) == SDW_SLAVE_ALERT) {
> + ctrl->status[i] = s;
> + return i;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
> {
> u32 val;
> @@ -333,40 +363,122 @@ static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
>
> static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
> {
> - struct qcom_swrm_ctrl *ctrl = dev_id;
> - u32 sts, value;
> - unsigned long flags;
> -
> - ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS, &sts);
> -
> - if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
> - ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
> - dev_err_ratelimited(ctrl->dev,
> - "CMD error, fifo status 0x%x\n",
> - value);
> - ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
> - }
> + struct qcom_swrm_ctrl *swrm = dev_id;
> + u32 value, intr_sts, intr_sts_masked;
> + u32 i;
> + u8 devnum = 0;
> + int ret = IRQ_HANDLED;
> +
> +
> + swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, &intr_sts);
> + intr_sts_masked = intr_sts & swrm->intr_mask;
> +
> +handle_irq:
maybe move this into a fn and avoid a goto for non err path?
> + for (i = 0; i < SWRM_INTERRUPT_MAX; i++) {
> + value = intr_sts_masked & (1 << i);
> + if (!value)
> + continue;
> +
> + switch (value) {
> + case SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ:
> + devnum = qcom_swrm_get_alert_slave(swrm);
> + if (devnum < 0) {
> + dev_err_ratelimited(swrm->dev,
> + "no slave alert found.spurious interrupt\n");
> + } else {
> + sdw_handle_slave_status(&swrm->bus, swrm->status);
> + }
>
> - if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
> - sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS)
> - schedule_work(&ctrl->slave_work);
> -
> - /**
> - * clear the interrupt before complete() is called, as complete can
> - * schedule new read/writes which require interrupts, clearing the
> - * interrupt would avoid missing interrupts in such cases.
> - */
> - ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
> -
> - if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED) {
> - spin_lock_irqsave(&ctrl->comp_lock, flags);
> - if (ctrl->comp)
> - complete(ctrl->comp);
> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
> + break;
> + case SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED:
> + case SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS:
> + dev_err_ratelimited(swrm->dev, "%s: SWR new slave attached\n",
> + __func__);
> + qcom_swrm_get_device_status(swrm);
> + sdw_handle_slave_status(&swrm->bus, swrm->status);
> + break;
> + case SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET:
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR bus clsh detected\n",
> + __func__);
> + swrm->intr_mask &=
> + ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
> + swrm->reg_write(swrm,
> + SWRM_INTERRUPT_CPU_EN,
> + swrm->intr_mask);
> + break;
> + case SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW:
> + swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR read FIFO overflow fifo status 0x%x\n",
> + __func__, value);
> + break;
> + case SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW:
> + swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR read FIFO underflow fifo status 0x%x\n",
> + __func__, value);
> + break;
> + case SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW:
> + swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
> + dev_err(swrm->dev,
> + "%s: SWR write FIFO overflow fifo status %x\n",
> + __func__, value);
> + swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
> + break;
> + case SWRM_INTERRUPT_STATUS_CMD_ERROR:
> + swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR CMD error, fifo status 0x%x, flushing fifo\n",
> + __func__, value);
> + swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
> + break;
> + case SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION:
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR Port collision detected\n",
> + __func__);
> + swrm->intr_mask &= ~SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION;
> + swrm->reg_write(swrm,
> + SWRM_INTERRUPT_CPU_EN, swrm->intr_mask);
> + break;
> + case SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH:
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR read enable valid mismatch\n",
> + __func__);
> + swrm->intr_mask &=
> + ~SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH;
> + swrm->reg_write(swrm,
> + SWRM_INTERRUPT_CPU_EN, swrm->intr_mask);
> + break;
> + case SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED:
> + complete(&swrm->broadcast);
> + break;
> + case SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED_V2:
> + break;
> + case SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED_V2:
> + break;
> + case SWRM_INTERRUPT_STATUS_EXT_CLK_STOP_WAKEUP:
> + break;
> + default:
> + dev_err_ratelimited(swrm->dev,
> + "%s: SWR unknown interrupt value: %d\n",
> + __func__, value);
> + ret = IRQ_NONE;
> + break;
> + }
> }
> + swrm->reg_write(swrm, SWRM_INTERRUPT_CLEAR, intr_sts);
> + swrm->reg_write(swrm, SWRM_INTERRUPT_CLEAR, 0x0);
> +
> + swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, &intr_sts);
> + intr_sts_masked = intr_sts & swrm->intr_mask;
> +
> + if (intr_sts_masked)
> + goto handle_irq;
>
> - return IRQ_HANDLED;
> + return ret;
> }
> +
> static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
> {
> u32 val;
> @@ -380,6 +492,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
> /* Disable Auto enumeration */
> ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);
>
> + ctrl->intr_mask = SWRM_INTERRUPT_STATUS_RMSK;
> /* Mask soundwire interrupts */
> ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
> SWRM_INTERRUPT_STATUS_RMSK);
> @@ -615,16 +728,6 @@ static u32 qcom_swrm_freq_tbl[MAX_FREQ_NUM] = {
> DEFAULT_CLK_FREQ,
> };
>
> -static void qcom_swrm_slave_wq(struct work_struct *work)
> -{
> - struct qcom_swrm_ctrl *ctrl =
> - container_of(work, struct qcom_swrm_ctrl, slave_work);
> -
> - qcom_swrm_get_device_status(ctrl);
> - sdw_handle_slave_status(&ctrl->bus, ctrl->status);
> -}
> -
> -
> static void qcom_swrm_stream_free_ports(struct qcom_swrm_ctrl *ctrl,
> struct sdw_stream_runtime *stream)
> {
> @@ -989,11 +1092,9 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>
> ctrl->dev = dev;
> dev_set_drvdata(&pdev->dev, ctrl);
> - spin_lock_init(&ctrl->comp_lock);
> mutex_init(&ctrl->port_lock);
> mutex_init(&ctrl->io_lock);
> init_completion(&ctrl->broadcast);
> - INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
>
> ctrl->bus.ops = &qcom_swrm_ops;
> ctrl->bus.port_ops = &qcom_swrm_port_ops;
> --
> 2.21.0
--
~Vinod
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/6] soundwire: qcom: add support to missing transport params
2021-01-29 19:20 ` Pierre-Louis Bossart
@ 2021-02-01 15:50 ` Srinivas Kandagatla
2021-02-01 16:33 ` Pierre-Louis Bossart
0 siblings, 1 reply; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-02-01 15:50 UTC (permalink / raw)
To: Pierre-Louis Bossart, vkoul
Cc: yung-chuan.liao, sanyog.r.kale, linux-kernel, alsa-devel
On 29/01/2021 19:20, Pierre-Louis Bossart wrote:
>
>> struct qcom_swrm_port_config {
>> u8 si;
>> u8 off1;
>> u8 off2;
>> u8 bp_mode;
>> + u8 hstart;
>> + u8 hstop;
>> + u8 word_length;
>> + u8 bgp_count;
>
> I couldn't figure out what 'bgp' was and had to search. Not sure how you
> came up with this abbreviation of "qcom,ports-block-group-count". Adding
> a comment wouldn't hurt.
I will rename this to blk_group_count which makes more sense!
>
>> + u8 lane_control;
>
> Are you able to use lane_control != 0 ? I thought we were missing stuff
> at the bus.c level?
Am not sure what is missing in bus.c but we do use lane_control for RX
slave on WCD938x codec. This uses datalane 1 for HPH and lane0 for
Compander/Class-H and other ports.
And it works!
--srini
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/6] soundwire: qcom: add support to missing transport params
2021-02-01 14:13 ` Vinod Koul
@ 2021-02-01 15:50 ` Srinivas Kandagatla
0 siblings, 0 replies; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-02-01 15:50 UTC (permalink / raw)
To: Vinod Koul
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel
On 01/02/2021 14:13, Vinod Koul wrote:
>> +
>> + ret = of_property_read_u8_array(np, "qcom,ports-hstop", hstop, nports);
>> + if (ret)
>> + memset(hstop, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
> why not memset the whole area here and then populate it..?
That is other way to do it!, I can do that in next spin!
--srini
>> +
>> + ret = of_property_read_u8_array(np, "qcom,ports-word-length",
>> + word_length, nports);
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/6] soundwire: qcom: set continue execution flag for ignored commands
2021-02-01 14:16 ` Vinod Koul
@ 2021-02-01 15:50 ` Srinivas Kandagatla
2021-02-02 4:46 ` Vinod Koul
0 siblings, 1 reply; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-02-01 15:50 UTC (permalink / raw)
To: Vinod Koul
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel
On 01/02/2021 14:16, Vinod Koul wrote:
>> /* Configure number of retries of a read/write cmd */
>> - ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, SWRM_RD_WR_CMD_RETRIES);
>> + if (ctrl->version_major == 1 && ctrl->version_minor >= 5 &&
>> + ctrl->version_step >= 1) {
> why not use raw version value?
>
> if (ctrl->raw > 0x10501 )
>
We can do that way as well, but Major Minor seems much clear to readers!
--srini
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/6] soundwire: qcom: start the clock during initialization
2021-02-01 14:21 ` Vinod Koul
@ 2021-02-01 15:50 ` Srinivas Kandagatla
2021-02-02 4:46 ` Vinod Koul
0 siblings, 1 reply; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-02-01 15:50 UTC (permalink / raw)
To: Vinod Koul
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel
On 01/02/2021 14:21, Vinod Koul wrote:
> On 29-01-21, 17:32, Srinivas Kandagatla wrote:
>> Start the clock during initialization.
>
> A detailed log please, which clock..? Also how do older controllers work
> w/o this clk
By default this is ON, however depending on that is not really reliable!
Explicitly enabling this will make things clear when we add clk pause
feature for this driver!
--srini
>
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> drivers/soundwire/qcom.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index 3669bac11a32..83df15d83935 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -47,6 +47,8 @@
>> #define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m) (0x101C + 0x40 * (m))
>> #define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK GENMASK(2, 0)
>> #define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK GENMASK(7, 3)
>> +#define SWRM_MCP_BUS_CTRL 0x1044
>> +#define SWRM_MCP_BUS_CLK_START BIT(1)
>> #define SWRM_MCP_CFG_ADDR 0x1048
>> #define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK GENMASK(21, 17)
>> #define SWRM_DEF_CMD_NO_PINGS 0x1f
>> @@ -345,6 +347,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>> u32p_replace_bits(&val, SWRM_DEF_CMD_NO_PINGS, SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK);
>> ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
>>
>> + ctrl->reg_write(ctrl, SWRM_MCP_BUS_CTRL, SWRM_MCP_BUS_CLK_START);
>> /* Configure number of retries of a read/write cmd */
>> if (ctrl->version_major == 1 && ctrl->version_minor >= 5 &&
>> ctrl->version_step >= 1) {
>> --
>> 2.21.0
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 5/6] soundwire: qcom: update register read/write routine
2021-01-29 19:33 ` Pierre-Louis Bossart
@ 2021-02-01 15:50 ` Srinivas Kandagatla
2021-02-01 16:42 ` Pierre-Louis Bossart
0 siblings, 1 reply; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-02-01 15:50 UTC (permalink / raw)
To: Pierre-Louis Bossart, vkoul
Cc: yung-chuan.liao, sanyog.r.kale, linux-kernel, alsa-devel
On 29/01/2021 19:33, Pierre-Louis Bossart wrote:
>
>
> On 1/29/21 11:32 AM, Srinivas Kandagatla wrote:
>> In the existing code every soundwire register read and register write
>> are kinda blocked. Each of these are using a special command id that
>
> what does 'kinda blocked' mean?
I meant read/writes are waiting for completion interrupt!
>
>> generates interrupt after it successfully finishes. This is really
>> overhead, limiting and not really necessary unless we are doing
>> something special.
>>
>> We can simply read/write the fifo that should also give exactly
>> what we need! This will also allow to read/write registers in
>> interrupt context, which was not possible with the special
>> command approach.
>
> This is really unclear, sorry.
If read/writes are waiting for an interrupt, it becomes difficult to
read or write to any registers from same interrupt handler!
>
>> + if (id != SWR_BROADCAST_CMD_ID) {
>> + if (id < 14)
>> + id += 1;
>> + else
>> + id = 0;
>
> that is really odd. if id=13 (group2) then id becomes 14 (master
> address). A comment is really needed here.
This is magic value for each fifo read or write, so that we can verify
that them by comparing with this magic value!
This has nothing to do with device number!
>
>> + if (cmd_id == SWR_BROADCAST_CMD_ID) {
>> + /*
>> + * sleep for 10ms for MSM soundwire variant to allow broadcast
>> + * command to complete.
>
> that's also super-odd. There is nothing in SoundWire that makes any
> difference between a regular and a broadcast command. they all complete
> in the same time (a frame).
>> + */
>> + ret = wait_for_completion_timeout(&swrm->broadcast, (2 *
>> HZ/10));
>
> is this 10ms really or dependent on CONFIG_HZ?
>
>> + if (!ret)
>> + ret = SDW_CMD_IGNORED;
>> + else
>> + ret = SDW_CMD_OK;
>
> no CMD_FAILED support?
Qcom controllers does not provide that information if the command is
ignored or failed by any means!
That was the behavior from the starting of this driver.
>
>> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *swrm,
>> + u8 dev_addr, u16 reg_addr,
>> + u32 len, u8 *rval)
>> +{
>> + u32 val;
>> + u32 retry_attempt = 0;
>> + u32 cmd_data;
>> + int ret = SDW_CMD_OK;
>> +
>> + mutex_lock(&swrm->io_lock);
>> + val = swrm_get_packed_reg_val(&swrm->rcmd_id, len, dev_addr,
>> reg_addr);
>> +
>> + /* wait for FIFO RD to complete to avoid overflow */
>> + usleep_range(100, 105);
>> + swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
>> + /* wait for FIFO RD CMD complete to avoid overflow */
>> + usleep_range(250, 255);
>> +
>> +retry_read:
>> +
>> + swrm->reg_read(swrm, SWRM_CMD_FIFO_RD_FIFO_ADDR, &cmd_data);
>> + rval[0] = cmd_data & 0xFF;
>> +
>> + if ((((cmd_data) & 0xF00) >> 8) != swrm->rcmd_id) {
>> + if (retry_attempt < MAX_FIFO_RD_FAIL_RETRY) {
>> + /* wait 500 us before retry on fifo read failure */
>> + usleep_range(500, 505);
>> + if (retry_attempt == (MAX_FIFO_RD_FAIL_RETRY - 1)) {
>> + swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
>> + swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
>> + }
>> + retry_attempt++;
>> + goto retry_read;
>> + } else {
>> + dev_err(swrm->dev,
>> + "failed to read fifo: reg: 0x%x, \
>> + rcmd_id: 0x%x, dev_num: 0x%x, cmd_data: 0x%x\n",
>> + reg_addr, swrm->rcmd_id,
>> + dev_addr, cmd_data);
>> + ret = SDW_CMD_IGNORED;
>> + }
>> }
>
> the flow seems complicated with multiple tests and goto? Can this be
> simplified?
I will try to simplify this in next version!
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 5/6] soundwire: qcom: update register read/write routine
2021-02-01 14:26 ` Vinod Koul
@ 2021-02-01 15:50 ` Srinivas Kandagatla
0 siblings, 0 replies; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-02-01 15:50 UTC (permalink / raw)
To: Vinod Koul
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel
On 01/02/2021 14:26, Vinod Koul wrote:
> On 29-01-21, 17:32, Srinivas Kandagatla wrote:
>> In the existing code every soundwire register read and register write
>> are kinda blocked. Each of these are using a special command id that
>> generates interrupt after it successfully finishes. This is really
>> overhead, limiting and not really necessary unless we are doing
>> something special.
>>
>> We can simply read/write the fifo that should also give exactly
>> what we need! This will also allow to read/write registers in
>> interrupt context, which was not possible with the special
>
> Okay but then why use a mutex ?
Read and writes can come from two places,
1> from SoundWire Core itself which takes care of locking
2> from interrupt handler in the driver itself, which is why we need an
additional locking!
Having said that I did have look at the current state of driver
interrupt handler, and it does not do any soundwire register read/writes
from interrupt handler so we can remove the lock for now!
>
>> command approach.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> drivers/soundwire/qcom.c | 148 +++++++++++++++++++++++++--------------
>> 1 file changed, 96 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index 83df15d83935..d61b204dc284 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -78,13 +78,15 @@
>> #define SWRM_SPECIAL_CMD_ID 0xF
>> #define MAX_FREQ_NUM 1
>> #define TIMEOUT_MS (2 * HZ)
>> -#define QCOM_SWRM_MAX_RD_LEN 0xf
>> +#define QCOM_SWRM_MAX_RD_LEN 0x1
>> #define QCOM_SDW_MAX_PORTS 14
>> #define DEFAULT_CLK_FREQ 9600000
>> #define SWRM_MAX_DAIS 0xF
>> #define SWR_INVALID_PARAM 0xFF
>> #define SWR_HSTOP_MAX_VAL 0xF
>> #define SWR_HSTART_MIN_VAL 0x0
>> +#define SWR_BROADCAST_CMD_ID 0x0F
>> +#define MAX_FIFO_RD_FAIL_RETRY 3
>>
>> struct qcom_swrm_port_config {
>> u8 si;
>> @@ -104,11 +106,13 @@ struct qcom_swrm_ctrl {
>> struct regmap *regmap;
>> void __iomem *mmio;
>> struct completion *comp;
>> + struct completion broadcast;
>> struct work_struct slave_work;
>> /* read/write lock */
>> spinlock_t comp_lock;
>> /* Port alloc/free lock */
>> struct mutex port_lock;
>> + struct mutex io_lock;
>> struct clk *hclk;
>> u8 wr_cmd_id;
>> u8 rd_cmd_id;
>> @@ -122,6 +126,8 @@ struct qcom_swrm_ctrl {
>> int rows_index;
>> unsigned long dout_port_mask;
>> unsigned long din_port_mask;
>> + u8 rcmd_id;
>> + u8 wcmd_id;
>> struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
>> struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
>> enum sdw_slave_status status[SDW_MAX_DEVICES];
>> @@ -200,75 +206,111 @@ static int qcom_swrm_cpu_reg_write(struct qcom_swrm_ctrl *ctrl, int reg,
>> return SDW_CMD_OK;
>> }
>>
>> -static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
>> - u8 dev_addr, u16 reg_addr)
>> +static u32 swrm_get_packed_reg_val(u8 *cmd_id, u8 cmd_data,
>> + u8 dev_addr, u16 reg_addr)
>> {
>> - DECLARE_COMPLETION_ONSTACK(comp);
>> - unsigned long flags;
>> u32 val;
>> - int ret;
>> -
>> - spin_lock_irqsave(&ctrl->comp_lock, flags);
>> - ctrl->comp = ∁
>> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
>> - val = SWRM_REG_VAL_PACK(cmd_data, dev_addr,
>> - SWRM_SPECIAL_CMD_ID, reg_addr);
>> - ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
>> - if (ret)
>> - goto err;
>> -
>> - ret = wait_for_completion_timeout(ctrl->comp,
>> - msecs_to_jiffies(TIMEOUT_MS));
>> + u8 id = *cmd_id;
>>
>> - if (!ret)
>> - ret = SDW_CMD_IGNORED;
>> - else
>> - ret = SDW_CMD_OK;
>> -err:
>> - spin_lock_irqsave(&ctrl->comp_lock, flags);
>> - ctrl->comp = NULL;
>> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
>> + if (id != SWR_BROADCAST_CMD_ID) {
>> + if (id < 14)
>> + id += 1;
>> + else
>> + id = 0;
>> + *cmd_id = id;
>> + }
>> + val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, id, reg_addr);
>>
>> - return ret;
>> + return val;
>> }
>>
>> -static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
>> - u8 dev_addr, u16 reg_addr,
>> - u32 len, u8 *rval)
>> +
>> +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *swrm, u8 cmd_data,
>> + u8 dev_addr, u16 reg_addr)
>> {
>> - int i, ret;
>> +
>> u32 val;
>> - DECLARE_COMPLETION_ONSTACK(comp);
>> - unsigned long flags;
>> + int ret = 0;
>> + u8 cmd_id = 0x0;
>> +
>> + mutex_lock(&swrm->io_lock);
>> + if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>> + cmd_id = SWR_BROADCAST_CMD_ID;
>> + val = swrm_get_packed_reg_val(&cmd_id, cmd_data,
>> + dev_addr, reg_addr);
>> + } else {
>> + val = swrm_get_packed_reg_val(&swrm->wcmd_id, cmd_data,
>> + dev_addr, reg_addr);
>> + }
>>
>> - spin_lock_irqsave(&ctrl->comp_lock, flags);
>> - ctrl->comp = ∁
>> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
>> + swrm->reg_write(swrm, SWRM_CMD_FIFO_WR_CMD, val);
>>
>> - val = SWRM_REG_VAL_PACK(len, dev_addr, SWRM_SPECIAL_CMD_ID, reg_addr);
>> - ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
>> - if (ret)
>> - goto err;
>> + /* version 1.3 or less */
>> + if (swrm->version_major == 1 && swrm->version_minor <= 3)
>> + usleep_range(150, 155);
>>
>> - ret = wait_for_completion_timeout(ctrl->comp,
>> - msecs_to_jif
--srini
fies(TIMEOUT_MS));
>> + if (cmd_id == SWR_BROADCAST_CMD_ID) {
>> + /*
>> + * sleep for 10ms for MSM soundwire variant to allow broadcast
>> + * command to complete.
>> + */
>> + ret = wait_for_completion_timeout(&swrm->broadcast, (2 * HZ/10));
>> + if (!ret)
>> + ret = SDW_CMD_IGNORED;
>> + else
>> + ret = SDW_CMD_OK;
>>
>> - if (!ret) {
>> - ret = SDW_CMD_IGNORED;
>> - goto err;
>> } else {
>> ret = SDW_CMD_OK;
>> }
>> + mutex_unlock(&swrm->io_lock);
>> + return ret;
>> +}
>>
>> - for (i = 0; i < len; i++) {
>> - ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR, &val);
>> - rval[i] = val & 0xFF;
>> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *swrm,
>> + u8 dev_addr, u16 reg_addr,
>> + u32 len, u8 *rval)
>> +{
>> + u32 val;
>> + u32 retry_attempt = 0;
>> + u32 cmd_data;
>> + int ret = SDW_CMD_OK;
>> +
>> + mutex_lock(&swrm->io_lock);
>> + val = swrm_get_packed_reg_val(&swrm->rcmd_id, len, dev_addr, reg_addr);
>> +
>> + /* wait for FIFO RD to complete to avoid overflow */
>> + usleep_range(100, 105);
>> + swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
>> + /* wait for FIFO RD CMD complete to avoid overflow */
>> + usleep_range(250, 255);
>> +
>> +retry_read:
>
> do while{} ?
Sure!
>
>> +
>> + swrm->reg_read(swrm, SWRM_CMD_FIFO_RD_FIFO_ADDR, &cmd_data);
>> + rval[0] = cmd_data & 0xFF;
>> +
>> + if ((((cmd_data) & 0xF00) >> 8) != swrm->rcmd_id) {
>> + if (retry_attempt < MAX_FIFO_RD_FAIL_RETRY) {
>> + /* wait 500 us before retry on fifo read failure */
>> + usleep_range(500, 505);
>> + if (retry_attempt == (MAX_FIFO_RD_FAIL_RETRY - 1)) {
>
> why not do this at the end if retry fails, that will make code look
> neater
I agree, will clean this one in next version!
>
>> + swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
>> + swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
>> + }
>> + retry_attempt++;
>> + goto retry_read;
>> + } else {
>> + dev_err(swrm->dev,
>> + "failed to read fifo: reg: 0x%x, \
>> + rcmd_id: 0x%x, dev_num: 0x%x, cmd_data: 0x%x\n",
>> + reg_addr, swrm->rcmd_id,
>> + dev_addr, cmd_data);
>
> Do you want to log retry as err..?
>
>> + ret = SDW_CMD_IGNORED;
>> + }
>> }
>>
>> -err:
>> - spin_lock_irqsave(&ctrl->comp_lock, flags);
>> - ctrl->comp = NULL;
>> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
>> + mutex_unlock(&swrm->io_lock);
>>
>> return ret;
>> }
>> @@ -949,6 +991,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>> dev_set_drvdata(&pdev->dev, ctrl);
>> spin_lock_init(&ctrl->comp_lock);
>> mutex_init(&ctrl->port_lock);
>> + mutex_init(&ctrl->io_lock);
>> + init_completion(&ctrl->broadcast);
>> INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
>>
>> ctrl->bus.ops = &qcom_swrm_ops;
>> --
>> 2.21.0
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] soundwire: qcom: add support to new interrupts
2021-02-01 14:31 ` Vinod Koul
@ 2021-02-01 15:51 ` Srinivas Kandagatla
0 siblings, 0 replies; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-02-01 15:51 UTC (permalink / raw)
To: Vinod Koul
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel
On 01/02/2021 14:31, Vinod Koul wrote:
> On 29-01-21, 17:32, Srinivas Kandagatla wrote:
>> Add support to new interrupts and update irq routine in a way
>> to deal with multiple pending interrupts with in a single interrupt!
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> drivers/soundwire/qcom.c | 191 ++++++++++++++++++++++++++++++---------
>> 1 file changed, 146 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index d61b204dc284..c699bd51d29a 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -28,10 +28,21 @@
>> #define SWRM_COMP_PARAMS_DIN_PORTS_MASK GENMASK(9, 5)
>> #define SWRM_INTERRUPT_STATUS 0x200
>> #define SWRM_INTERRUPT_STATUS_RMSK GENMASK(16, 0)
>> +#define SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ BIT(0)
>> #define SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED BIT(1)
>> #define SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS BIT(2)
>> +#define SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET BIT(3)
>> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW BIT(4)
>> +#define SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW BIT(5)
>> +#define SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW BIT(6)
>> #define SWRM_INTERRUPT_STATUS_CMD_ERROR BIT(7)
>> +#define SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION BIT(8)
>> +#define SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH BIT(9)
>> #define SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED BIT(10)
>> +#define SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED_V2 BIT(13)
>> +#define SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHED_V2 BIT(14)
>> +#define SWRM_INTERRUPT_STATUS_EXT_CLK_STOP_WAKEUP BIT(16)
>> +#define SWRM_INTERRUPT_MAX 17
>> #define SWRM_INTERRUPT_MASK_ADDR 0x204
>> #define SWRM_INTERRUPT_CLEAR 0x208
>> #define SWRM_INTERRUPT_CPU_EN 0x210
>> @@ -105,11 +116,8 @@ struct qcom_swrm_ctrl {
>> struct device *dev;
>> struct regmap *regmap;
>> void __iomem *mmio;
>> - struct completion *comp;
>> struct completion broadcast;
>> struct work_struct slave_work;
>> - /* read/write lock */
>> - spinlock_t comp_lock;
>> /* Port alloc/free lock */
>> struct mutex port_lock;
>> struct mutex io_lock;
>> @@ -126,6 +134,7 @@ struct qcom_swrm_ctrl {
>> int rows_index;
>> unsigned long dout_port_mask;
>> unsigned long din_port_mask;
>> + u32 intr_mask;
>> u8 rcmd_id;
>> u8 wcmd_id;
>> struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
>> @@ -315,6 +324,27 @@ static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *swrm,
>> return ret;
>> }
>>
>> +static int qcom_swrm_get_alert_slave(struct qcom_swrm_ctrl *ctrl)
>> +{
>> + u32 val;
>> + int i;
>> +
>> + ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val);
>> +
>> + for (i = 0; i < SDW_MAX_DEVICES; i++) {
>> + u32 s;
>
> define at top of the function pls, also maybe better name status?
Sure,
>
>> +
>> + s = (val >> (i * 2));
>
> why * 2 ? Maybe add a comment for this logic
This is status mask of 2 bits! Will clarify this in next version!
>
>> +
>> + if ((s & SWRM_MCP_SLV_STATUS_MASK) == SDW_SLAVE_ALERT) {
>> + ctrl->status[i] = s;
>> + return i;
>> + }
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
>> {
>> u32 val;
>> @@ -333,40 +363,122 @@ static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
>>
>> static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>> {
>> - struct qcom_swrm_ctrl *ctrl = dev_id;
>> - u32 sts, value;
>> - unsigned long flags;
>> -
>> - ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS, &sts);
>> -
>> - if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
>> - ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
>> - dev_err_ratelimited(ctrl->dev,
>> - "CMD error, fifo status 0x%x\n",
>> - value);
>> - ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
>> - }
>> + struct qcom_swrm_ctrl *swrm = dev_id;
>> + u32 value, intr_sts, intr_sts_masked;
>> + u32 i;
>> + u8 devnum = 0;
>> + int ret = IRQ_HANDLED;
>> +
>> +
>> + swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, &intr_sts);
>> + intr_sts_masked = intr_sts & swrm->intr_mask;
>> +
>> +handle_irq:
>
> maybe move this into a fn and avoid a goto for non err path?
>
Makes sense!
--srini
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] soundwire: qcom: add support to new interrupts
2021-01-29 19:38 ` Pierre-Louis Bossart
@ 2021-02-01 15:51 ` Srinivas Kandagatla
0 siblings, 0 replies; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-02-01 15:51 UTC (permalink / raw)
To: Pierre-Louis Bossart, vkoul
Cc: alsa-devel, linux-kernel, sanyog.r.kale, yung-chuan.liao
On 29/01/2021 19:38, Pierre-Louis Bossart wrote:
>
>
> On 1/29/21 11:32 AM, Srinivas Kandagatla wrote:
>> Add support to new interrupts and update irq routine in a way
>> to deal with multiple pending interrupts with in a single interrupt!
>
> I can't parse the wording after 'update irq routine'.
>
So, basically if we get an interrupt while processing previous one there
is a chance that we might miss the new interrupt!
This was a bug with existing code, which only showed up with recent
Headset Button Tests!
>
>> + swrm->reg_write(swrm, SWRM_INTERRUPT_CLEAR, intr_sts);
>> + swrm->reg_write(swrm, SWRM_INTERRUPT_CLEAR, 0x0);
>
> what does this second write 0x0 do? Usually interrupts are W1C, and you
> didn't have this before.
Digging it bit more seems to be an HW work around for one of the issues,
recommended by HW team. Will remove it from this patch and add it as
separate one with more details!
--srini
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/6] soundwire: qcom: add support to missing transport params
2021-02-01 15:50 ` Srinivas Kandagatla
@ 2021-02-01 16:33 ` Pierre-Louis Bossart
0 siblings, 0 replies; 30+ messages in thread
From: Pierre-Louis Bossart @ 2021-02-01 16:33 UTC (permalink / raw)
To: Srinivas Kandagatla, vkoul
Cc: yung-chuan.liao, sanyog.r.kale, linux-kernel, alsa-devel
>>> struct qcom_swrm_port_config {
>>> u8 si;
>>> u8 off1;
>>> u8 off2;
>>> u8 bp_mode;
>>> + u8 hstart;
>>> + u8 hstop;
>>> + u8 word_length;
>>> + u8 bgp_count;
>>
>> I couldn't figure out what 'bgp' was and had to search. Not sure how
>> you came up with this abbreviation of "qcom,ports-block-group-count".
>> Adding a comment wouldn't hurt.
>
> I will rename this to blk_group_count which makes more sense!
sounds good.
>>
>>> + u8 lane_control;
>>
>> Are you able to use lane_control != 0 ? I thought we were missing
>> stuff at the bus.c level?
> Am not sure what is missing in bus.c but we do use lane_control for RX
> slave on WCD938x codec. This uses datalane 1 for HPH and lane0 for
> Compander/Class-H and other ports.
>
> And it works!
Ah, good to know, thanks for the pointer.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 5/6] soundwire: qcom: update register read/write routine
2021-02-01 15:50 ` Srinivas Kandagatla
@ 2021-02-01 16:42 ` Pierre-Louis Bossart
2021-02-02 10:19 ` Srinivas Kandagatla
0 siblings, 1 reply; 30+ messages in thread
From: Pierre-Louis Bossart @ 2021-02-01 16:42 UTC (permalink / raw)
To: Srinivas Kandagatla, vkoul
Cc: yung-chuan.liao, sanyog.r.kale, linux-kernel, alsa-devel
On 2/1/21 9:50 AM, Srinivas Kandagatla wrote:
>
>
> On 29/01/2021 19:33, Pierre-Louis Bossart wrote:
>>
>>
>> On 1/29/21 11:32 AM, Srinivas Kandagatla wrote:
>>> In the existing code every soundwire register read and register write
>>> are kinda blocked. Each of these are using a special command id that
>>
>> what does 'kinda blocked' mean?
>
> I meant read/writes are waiting for completion interrupt!
>
>>
>>> generates interrupt after it successfully finishes. This is really
>>> overhead, limiting and not really necessary unless we are doing
>>> something special.
>>>
>>> We can simply read/write the fifo that should also give exactly
>>> what we need! This will also allow to read/write registers in
>>> interrupt context, which was not possible with the special
>>> command approach.
>>
>> This is really unclear, sorry.
>
> If read/writes are waiting for an interrupt, it becomes difficult to
> read or write to any registers from same interrupt handler!
Well, yes, you need to handle the complete() at a lower level than the
code that initiates the transactions otherwise you self-deadlock.
IIRC in the Intel initial code, the complete was in the handler and the
register IOs in the thread.
>
>
>>
>>> + if (id != SWR_BROADCAST_CMD_ID) {
>>> + if (id < 14)
>>> + id += 1;
>>> + else
>>> + id = 0;
>>
>> that is really odd. if id=13 (group2) then id becomes 14 (master
>> address). A comment is really needed here.
>
> This is magic value for each fifo read or write, so that we can verify
> that them by comparing with this magic value!
>
> This has nothing to do with device number!
You should probably add a comment here then, or use a #define instead of
the 14 which threw me off.
>
>>
>>> + if (cmd_id == SWR_BROADCAST_CMD_ID) {
>>> + /*
>>> + * sleep for 10ms for MSM soundwire variant to allow broadcast
>>> + * command to complete.
>>
>> that's also super-odd. There is nothing in SoundWire that makes any
>> difference between a regular and a broadcast command. they all
>> complete in the same time (a frame).
>>> + */
>>> + ret = wait_for_completion_timeout(&swrm->broadcast, (2 *
>>> HZ/10));
>>
>> is this 10ms really or dependent on CONFIG_HZ?
comment missed?
>>
>>> + if (!ret)
>>> + ret = SDW_CMD_IGNORED;
>>> + else
>>> + ret = SDW_CMD_OK;
>>
>> no CMD_FAILED support?
>
> Qcom controllers does not provide that information if the command is
> ignored or failed by any means!
>
> That was the behavior from the starting of this driver.
ah yes, now I remember this.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/6] soundwire: qcom: set continue execution flag for ignored commands
2021-02-01 15:50 ` Srinivas Kandagatla
@ 2021-02-02 4:46 ` Vinod Koul
0 siblings, 0 replies; 30+ messages in thread
From: Vinod Koul @ 2021-02-02 4:46 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel
On 01-02-21, 15:50, Srinivas Kandagatla wrote:
>
>
> On 01/02/2021 14:16, Vinod Koul wrote:
> > > /* Configure number of retries of a read/write cmd */
> > > - ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, SWRM_RD_WR_CMD_RETRIES);
> > > + if (ctrl->version_major == 1 && ctrl->version_minor >= 5 &&
> > > + ctrl->version_step >= 1) {
> > why not use raw version value?
> >
> > if (ctrl->raw > 0x10501 )
> >
> We can do that way as well, but Major Minor seems much clear to readers!
yes but comparison with numbers is always easiest and better :) We can
always add comment that check version 1.5.1 which will make it clear
--
~Vinod
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/6] soundwire: qcom: start the clock during initialization
2021-02-01 15:50 ` Srinivas Kandagatla
@ 2021-02-02 4:46 ` Vinod Koul
0 siblings, 0 replies; 30+ messages in thread
From: Vinod Koul @ 2021-02-02 4:46 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
linux-kernel, alsa-devel
On 01-02-21, 15:50, Srinivas Kandagatla wrote:
>
>
> On 01/02/2021 14:21, Vinod Koul wrote:
> > On 29-01-21, 17:32, Srinivas Kandagatla wrote:
> > > Start the clock during initialization.
> >
> > A detailed log please, which clock..? Also how do older controllers work
> > w/o this clk
>
> By default this is ON, however depending on that is not really reliable!
>
> Explicitly enabling this will make things clear when we add clk pause
> feature for this driver!
lets add this in the log please
--
~Vinod
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 5/6] soundwire: qcom: update register read/write routine
2021-02-01 16:42 ` Pierre-Louis Bossart
@ 2021-02-02 10:19 ` Srinivas Kandagatla
2021-02-02 16:58 ` Pierre-Louis Bossart
0 siblings, 1 reply; 30+ messages in thread
From: Srinivas Kandagatla @ 2021-02-02 10:19 UTC (permalink / raw)
To: Pierre-Louis Bossart, vkoul
Cc: yung-chuan.liao, sanyog.r.kale, linux-kernel, alsa-devel
On 01/02/2021 16:42, Pierre-Louis Bossart wrote:
>
>
> On 2/1/21 9:50 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 29/01/2021 19:33, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 1/29/21 11:32 AM, Srinivas Kandagatla wrote:
>>>> In the existing code every soundwire register read and register write
>>>> are kinda blocked. Each of these are using a special command id that
>>>
>>> what does 'kinda blocked' mean?
>>
>> I meant read/writes are waiting for completion interrupt!
>>
>>>
>>>> generates interrupt after it successfully finishes. This is really
>>>> overhead, limiting and not really necessary unless we are doing
>>>> something special.
>>>>
>>>> We can simply read/write the fifo that should also give exactly
>>>> what we need! This will also allow to read/write registers in
>>>> interrupt context, which was not possible with the special
>>>> command approach.
>>>
>>> This is really unclear, sorry.
>>
>> If read/writes are waiting for an interrupt, it becomes difficult to
>> read or write to any registers from same interrupt handler!
>
> Well, yes, you need to handle the complete() at a lower level than the
> code that initiates the transactions otherwise you self-deadlock.
>
> IIRC in the Intel initial code, the complete was in the handler and the
> register IOs in the thread.
>
Yes, we did the same in previous version of the code, however with this
patch reading/writing fifo directly without need of completion should
remove that need of another thread!
>>
>>
>>>
>>>> + if (id != SWR_BROADCAST_CMD_ID) {
>>>> + if (id < 14)
>>>> + id += 1;
>>>> + else
>>>> + id = 0;
>>>
>>> that is really odd. if id=13 (group2) then id becomes 14 (master
>>> address). A comment is really needed here.
>>
>> This is magic value for each fifo read or write, so that we can verify
>> that them by comparing with this magic value!
>>
>> This has nothing to do with device number!
>
> You should probably add a comment here then, or use a #define instead of
> the 14 which threw me off.
I agree!
>
>>
>>>
>>>> + if (cmd_id == SWR_BROADCAST_CMD_ID) {
>>>> + /*
>>>> + * sleep for 10ms for MSM soundwire variant to allow broadcast
>>>> + * command to complete.
>>>
>>> that's also super-odd. There is nothing in SoundWire that makes any
>>> difference between a regular and a broadcast command. they all
>>> complete in the same time (a frame).
>>>> + */
>>>> + ret = wait_for_completion_timeout(&swrm->broadcast, (2 *
>>>> HZ/10));
>>>
>>> is this 10ms really or dependent on CONFIG_HZ?
>
> comment missed?
Not intentionally :-)
I should probably to use msecs_to_jiffies here to keep it inline with
the comment!
--srini
>
>>>
>>>> + if (!ret)
>>>> + ret = SDW_CMD_IGNORED;
>>>> + else
>>>> + ret = SDW_CMD_OK;
>>>
>>> no CMD_FAILED support?
>>
>> Qcom controllers does not provide that information if the command is
>> ignored or failed by any means!
>>
>> That was the behavior from the starting of this driver.
>
> ah yes, now I remember this.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 5/6] soundwire: qcom: update register read/write routine
2021-02-02 10:19 ` Srinivas Kandagatla
@ 2021-02-02 16:58 ` Pierre-Louis Bossart
0 siblings, 0 replies; 30+ messages in thread
From: Pierre-Louis Bossart @ 2021-02-02 16:58 UTC (permalink / raw)
To: Srinivas Kandagatla, vkoul
Cc: yung-chuan.liao, sanyog.r.kale, linux-kernel, alsa-devel
>>>>> generates interrupt after it successfully finishes. This is really
>>>>> overhead, limiting and not really necessary unless we are doing
>>>>> something special.
>>>>>
>>>>> We can simply read/write the fifo that should also give exactly
>>>>> what we need! This will also allow to read/write registers in
>>>>> interrupt context, which was not possible with the special
>>>>> command approach.
>>>>
>>>> This is really unclear, sorry.
>>>
>>> If read/writes are waiting for an interrupt, it becomes difficult to
>>> read or write to any registers from same interrupt handler!
>>
>> Well, yes, you need to handle the complete() at a lower level than the
>> code that initiates the transactions otherwise you self-deadlock.
>>
>> IIRC in the Intel initial code, the complete was in the handler and
>> the register IOs in the thread.
>>
> Yes, we did the same in previous version of the code, however with this
> patch reading/writing fifo directly without need of completion should
> remove that need of another thread!
Right, but you'll also write-off some command/control efficiency by
either sleeping too much before checking the status, or sleeping too
little and reading status from a transaction that's not finished.
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2021-02-02 17:35 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-29 17:32 [PATCH 0/6] soundwire: qcom: various improvements Srinivas Kandagatla
2021-01-29 17:32 ` [PATCH 1/6] soundwire: qcom: add support to missing transport params Srinivas Kandagatla
2021-01-29 19:20 ` Pierre-Louis Bossart
2021-02-01 15:50 ` Srinivas Kandagatla
2021-02-01 16:33 ` Pierre-Louis Bossart
2021-02-01 14:13 ` Vinod Koul
2021-02-01 15:50 ` Srinivas Kandagatla
2021-01-29 17:32 ` [PATCH 2/6] soundwire: qcom: extract version field Srinivas Kandagatla
2021-01-29 17:32 ` [PATCH 3/6] soundwire: qcom: set continue execution flag for ignored commands Srinivas Kandagatla
2021-01-29 19:21 ` Pierre-Louis Bossart
2021-02-01 14:16 ` Vinod Koul
2021-02-01 15:50 ` Srinivas Kandagatla
2021-02-02 4:46 ` Vinod Koul
2021-01-29 17:32 ` [PATCH 4/6] soundwire: qcom: start the clock during initialization Srinivas Kandagatla
2021-02-01 14:21 ` Vinod Koul
2021-02-01 15:50 ` Srinivas Kandagatla
2021-02-02 4:46 ` Vinod Koul
2021-01-29 17:32 ` [PATCH 5/6] soundwire: qcom: update register read/write routine Srinivas Kandagatla
2021-01-29 19:33 ` Pierre-Louis Bossart
2021-02-01 15:50 ` Srinivas Kandagatla
2021-02-01 16:42 ` Pierre-Louis Bossart
2021-02-02 10:19 ` Srinivas Kandagatla
2021-02-02 16:58 ` Pierre-Louis Bossart
2021-02-01 14:26 ` Vinod Koul
2021-02-01 15:50 ` Srinivas Kandagatla
2021-01-29 17:32 ` [PATCH 6/6] soundwire: qcom: add support to new interrupts Srinivas Kandagatla
2021-01-29 19:38 ` Pierre-Louis Bossart
2021-02-01 15:51 ` Srinivas Kandagatla
2021-02-01 14:31 ` Vinod Koul
2021-02-01 15:51 ` Srinivas Kandagatla
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).