[PATCH v2 0/3] soundwire: qcom: fix IP version v1.5.1 support

[PATCH v2 0/3] soundwire: qcom: fix IP version v1.5.1 support

[Srinivas Kandagatla]

While testing Qualcomm soundwire controller version 1.5.1, found two issue,
Firstly the frame shape information configured vs the bus parameters
are out of sync. secondly some ports on this ip version require
block packing mode support.

With this patchset I was able to test 2 WSA speakers!

Also I found a regression due to move to REG_FIELD, which patch 1 fixes it!

thanks,
srini

Changes since v1:
 - rebased on top of REG_FILED patch or soundwire-next branch
 - udated qcom_swrm_data to use u32 instead of int as suggested by VKoul

Srinivas Kandagatla (3):
  soundwire: qcom: clear BIT FIELDs before value set.
  soundwire: qcom: add support to block packing mode
  soundwire: qcom: get max rows and cols info from compatible

 drivers/soundwire/qcom.c | 76 ++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 18 deletions(-)

-- 
2.21.0

[PATCH v2 1/3] soundwire: qcom: clear BIT FIELDs before value set.

[Srinivas Kandagatla]

According to usage (bitfields.h) of REG_FIELDS,
Modify is:
  reg &= ~REG_FIELD_C;
  reg |= FIELD_PREP(REG_FIELD_C, c);

Patch ("soundwire: qcom : use FIELD_{GET|PREP}") seems to have
accidentally removed clearing bit field while modifying the register.

Fix this by adding back clear register mask before setting it up!

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/qcom.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index d7aabdaffee3..5d26361ab4f6 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -311,6 +311,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
 
 	/* Configure No pings */
 	ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR, &val);
+	val &= ~SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK;
 	val |= FIELD_PREP(SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK, SWRM_DEF_CMD_NO_PINGS);
 	ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
 
@@ -372,6 +373,9 @@ static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
 
 	ctrl->reg_read(ctrl, reg, &val);
 
+	val &= ~SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK;
+	val &= ~SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK;
+
 	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, SWRM_MAX_COL_VAL);
 	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, SWRM_MAX_ROW_VAL);
 
-- 
2.21.0

[PATCH v2 2/3] soundwire: qcom: add support to block packing mode

[Srinivas Kandagatla]

This patch adds support to block pack mode, which is required
on Qcom soundwire controllers v1.5.x on few ports!

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/qcom.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 5d26361ab4f6..76963a7bdaa3 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -54,6 +54,7 @@
 #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_BLOCK_CTRL3_BANK(n, m)	(0x1138 + 0x100 * (n - 1) + 0x40 * m)
 #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
@@ -82,6 +83,7 @@ struct qcom_swrm_port_config {
 	u8 si;
 	u8 off1;
 	u8 off2;
+	u8 bp_mode;
 };
 
 struct qcom_swrm_ctrl {
@@ -396,14 +398,22 @@ static int qcom_swrm_transport_params(struct sdw_bus *bus,
 {
 	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
 	u32 value;
+	int reg = SWRM_DP_PORT_CTRL_BANK((params->port_num), bank);
+	int ret;
 
 	value = params->offset1 << SWRM_DP_PORT_CTRL_OFFSET1_SHFT;
 	value |= params->offset2 << SWRM_DP_PORT_CTRL_OFFSET2_SHFT;
 	value |= params->sample_interval - 1;
 
-	return ctrl->reg_write(ctrl,
-			       SWRM_DP_PORT_CTRL_BANK((params->port_num), bank),
-			       value);
+	ret = ctrl->reg_write(ctrl, reg, value);
+
+	if (!ret && params->blk_pkg_mode) {
+		reg = SWRM_DP_BLOCK_CTRL3_BANK(params->port_num, bank);
+
+		ret = ctrl->reg_write(ctrl, reg, 1);
+	}
+
+	return ret;
 }
 
 static int qcom_swrm_port_enable(struct sdw_bus *bus,
@@ -451,6 +461,7 @@ static int qcom_swrm_compute_params(struct sdw_bus *bus)
 			p_rt->transport_params.sample_interval = pcfg->si + 1;
 			p_rt->transport_params.offset1 = pcfg->off1;
 			p_rt->transport_params.offset2 = pcfg->off2;
+			p_rt->transport_params.blk_pkg_mode = pcfg->bp_mode;
 		}
 
 		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
@@ -461,6 +472,7 @@ static int qcom_swrm_compute_params(struct sdw_bus *bus)
 					pcfg->si + 1;
 				p_rt->transport_params.offset1 = pcfg->off1;
 				p_rt->transport_params.offset2 = pcfg->off2;
+				p_rt->transport_params.blk_pkg_mode = pcfg->bp_mode;
 				i++;
 			}
 		}
@@ -707,6 +719,7 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
 	u8 off1[QCOM_SDW_MAX_PORTS];
 	u8 off2[QCOM_SDW_MAX_PORTS];
 	u8 si[QCOM_SDW_MAX_PORTS];
+	u8 bp_mode[QCOM_SDW_MAX_PORTS] = { 0, };
 	int i, ret, nports, val;
 
 	ctrl->reg_read(ctrl, SWRM_COMP_PARAMS, &val);
@@ -749,10 +762,13 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
 	if (ret)
 		return ret;
 
+	ret = of_property_read_u8_array(np, "qcom,ports-block-pack-mode",
+					bp_mode, nports);
 	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];
 	}
 
 	return 0;
-- 
2.21.0

[PATCH v2 3/3] soundwire: qcom: get max rows and cols info from compatible

[Srinivas Kandagatla]

currently the max rows and cols values are hardcoded. In reality
these values depend on the IP version. So get these based on
device tree compatible strings.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/qcom.c | 50 ++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 76963a7bdaa3..1dbf33684470 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -66,11 +66,6 @@
 #define SWRM_REG_VAL_PACK(data, dev, id, reg)	\
 			((reg) | ((id) << 16) | ((dev) << 20) | ((data) << 24))
 
-#define SWRM_MAX_ROW_VAL	0 /* Rows = 48 */
-#define SWRM_DEFAULT_ROWS	48
-#define SWRM_MIN_COL_VAL	0 /* Cols = 2 */
-#define SWRM_DEFAULT_COL	16
-#define SWRM_MAX_COL_VAL	7
 #define SWRM_SPECIAL_CMD_ID	0xF
 #define MAX_FREQ_NUM		1
 #define TIMEOUT_MS		(2 * HZ)
@@ -104,6 +99,8 @@ struct qcom_swrm_ctrl {
 	unsigned int version;
 	int num_din_ports;
 	int num_dout_ports;
+	int cols_index;
+	int rows_index;
 	unsigned long dout_port_mask;
 	unsigned long din_port_mask;
 	struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
@@ -113,6 +110,21 @@ struct qcom_swrm_ctrl {
 	int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val);
 };
 
+struct qcom_swrm_data {
+	u32 default_cols;
+	u32 default_rows;
+};
+
+static struct qcom_swrm_data swrm_v1_3_data = {
+	.default_rows = 48,
+	.default_cols = 16,
+};
+
+static struct qcom_swrm_data swrm_v1_5_data = {
+	.default_rows = 50,
+	.default_cols = 16,
+};
+
 #define to_qcom_sdw(b)	container_of(b, struct qcom_swrm_ctrl, bus)
 
 static int qcom_swrm_ahb_reg_read(struct qcom_swrm_ctrl *ctrl, int reg,
@@ -299,8 +311,10 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
 	u32 val;
 
 	/* Clear Rows and Cols */
-	val = FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, SWRM_MAX_ROW_VAL);
-	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, SWRM_MIN_COL_VAL);
+	val = FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK,
+			 ctrl->rows_index);
+	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK,
+			  ctrl->cols_index);
 
 	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
 
@@ -378,8 +392,10 @@ static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
 	val &= ~SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK;
 	val &= ~SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK;
 
-	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, SWRM_MAX_COL_VAL);
-	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, SWRM_MAX_ROW_VAL);
+	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK,
+			  ctrl->cols_index);
+	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK,
+			  ctrl->rows_index);
 
 	return ctrl->reg_write(ctrl, reg, val);
 }
@@ -780,6 +796,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	struct sdw_master_prop *prop;
 	struct sdw_bus_params *params;
 	struct qcom_swrm_ctrl *ctrl;
+	const struct qcom_swrm_data *data;
 	int ret;
 	u32 val;
 
@@ -787,6 +804,9 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	if (!ctrl)
 		return -ENOMEM;
 
+	data = of_device_get_match_data(dev);
+	ctrl->rows_index = sdw_find_row_index(data->default_rows);
+	ctrl->cols_index = sdw_find_col_index(data->default_cols);
 #if IS_ENABLED(CONFIG_SLIMBUS)
 	if (dev->parent->bus == &slimbus_bus) {
 #else
@@ -836,8 +856,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	params = &ctrl->bus.params;
 	params->max_dr_freq = DEFAULT_CLK_FREQ;
 	params->curr_dr_freq = DEFAULT_CLK_FREQ;
-	params->col = SWRM_DEFAULT_COL;
-	params->row = SWRM_DEFAULT_ROWS;
+	params->col = data->default_cols;
+	params->row = data->default_rows;
 	ctrl->reg_read(ctrl, SWRM_MCP_STATUS, &val);
 	params->curr_bank = val & SWRM_MCP_STATUS_BANK_NUM_MASK;
 	params->next_bank = !params->curr_bank;
@@ -847,8 +867,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	prop->num_clk_gears = 0;
 	prop->num_clk_freq = MAX_FREQ_NUM;
 	prop->clk_freq = &qcom_swrm_freq_tbl[0];
-	prop->default_col = SWRM_DEFAULT_COL;
-	prop->default_row = SWRM_DEFAULT_ROWS;
+	prop->default_col = data->default_cols;
+	prop->default_row = data->default_rows;
 
 	ctrl->reg_read(ctrl, SWRM_COMP_HW_VERSION, &ctrl->version);
 
@@ -899,8 +919,8 @@ static int qcom_swrm_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id qcom_swrm_of_match[] = {
-	{ .compatible = "qcom,soundwire-v1.3.0", },
-	{ .compatible = "qcom,soundwire-v1.5.1", },
+	{ .compatible = "qcom,soundwire-v1.3.0", .data = &swrm_v1_3_data },
+	{ .compatible = "qcom,soundwire-v1.5.1", .data = &swrm_v1_5_data },
 	{/* sentinel */},
 };
 
-- 
2.21.0

Re: [PATCH v2 1/3] soundwire: qcom: clear BIT FIELDs before value set.

[Vinod Koul]

Hi Srini,

On 16-09-20, 10:21, Srinivas Kandagatla wrote:
> According to usage (bitfields.h) of REG_FIELDS,
> Modify is:
>   reg &= ~REG_FIELD_C;
>   reg |= FIELD_PREP(REG_FIELD_C, c);
> 
> Patch ("soundwire: qcom : use FIELD_{GET|PREP}") seems to have
> accidentally removed clearing bit field while modifying the register.
> 
> Fix this by adding back clear register mask before setting it up!
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/soundwire/qcom.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index d7aabdaffee3..5d26361ab4f6 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -311,6 +311,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>  
>  	/* Configure No pings */
>  	ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR, &val);
> +	val &= ~SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK;
>  	val |= FIELD_PREP(SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK, SWRM_DEF_CMD_NO_PINGS);

Should we rather use u32_replace_bits() here, I think the intention is
to replace bits.

>  	ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
>  
> @@ -372,6 +373,9 @@ static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
>  
>  	ctrl->reg_read(ctrl, reg, &val);
>  
> +	val &= ~SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK;
> +	val &= ~SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK;
> +
>  	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, SWRM_MAX_COL_VAL);
>  	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, SWRM_MAX_ROW_VAL);
>  
> -- 
> 2.21.0

-- 
~Vinod

Re: [PATCH v2 3/3] soundwire: qcom: get max rows and cols info from compatible

[Vinod Koul]

On 16-09-20, 10:21, Srinivas Kandagatla wrote:
> currently the max rows and cols values are hardcoded. In reality
> these values depend on the IP version. So get these based on
> device tree compatible strings.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/soundwire/qcom.c | 50 ++++++++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 76963a7bdaa3..1dbf33684470 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -66,11 +66,6 @@
>  #define SWRM_REG_VAL_PACK(data, dev, id, reg)	\
>  			((reg) | ((id) << 16) | ((dev) << 20) | ((data) << 24))
>  
> -#define SWRM_MAX_ROW_VAL	0 /* Rows = 48 */
> -#define SWRM_DEFAULT_ROWS	48
> -#define SWRM_MIN_COL_VAL	0 /* Cols = 2 */
> -#define SWRM_DEFAULT_COL	16
> -#define SWRM_MAX_COL_VAL	7
>  #define SWRM_SPECIAL_CMD_ID	0xF
>  #define MAX_FREQ_NUM		1
>  #define TIMEOUT_MS		(2 * HZ)
> @@ -104,6 +99,8 @@ struct qcom_swrm_ctrl {
>  	unsigned int version;
>  	int num_din_ports;
>  	int num_dout_ports;
> +	int cols_index;
> +	int rows_index;
>  	unsigned long dout_port_mask;
>  	unsigned long din_port_mask;
>  	struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
> @@ -113,6 +110,21 @@ struct qcom_swrm_ctrl {
>  	int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val);
>  };
>  
> +struct qcom_swrm_data {
> +	u32 default_cols;
> +	u32 default_rows;
> +};
> +
> +static struct qcom_swrm_data swrm_v1_3_data = {
> +	.default_rows = 48,
> +	.default_cols = 16,
> +};
> +
> +static struct qcom_swrm_data swrm_v1_5_data = {
> +	.default_rows = 50,
> +	.default_cols = 16,
> +};
> +
>  #define to_qcom_sdw(b)	container_of(b, struct qcom_swrm_ctrl, bus)
>  
>  static int qcom_swrm_ahb_reg_read(struct qcom_swrm_ctrl *ctrl, int reg,
> @@ -299,8 +311,10 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>  	u32 val;
>  
>  	/* Clear Rows and Cols */
> -	val = FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, SWRM_MAX_ROW_VAL);
> -	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, SWRM_MIN_COL_VAL);
> +	val = FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK,
> +			 ctrl->rows_index);
> +	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK,
> +			  ctrl->cols_index);

single line for these ?

>  	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
>  
> @@ -378,8 +392,10 @@ static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
>  	val &= ~SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK;
>  	val &= ~SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK;
>  
> -	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, SWRM_MAX_COL_VAL);
> -	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, SWRM_MAX_ROW_VAL);
> +	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK,
> +			  ctrl->cols_index);
> +	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK,
> +			  ctrl->rows_index);

here as well

>  
>  	return ctrl->reg_write(ctrl, reg, val);
>  }
> @@ -780,6 +796,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  	struct sdw_master_prop *prop;
>  	struct sdw_bus_params *params;
>  	struct qcom_swrm_ctrl *ctrl;
> +	const struct qcom_swrm_data *data;
>  	int ret;
>  	u32 val;
>  
> @@ -787,6 +804,9 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  	if (!ctrl)
>  		return -ENOMEM;
>  
> +	data = of_device_get_match_data(dev);

Wont it be good to check if data is valid, we do dereference it few line
below

> +	ctrl->rows_index = sdw_find_row_index(data->default_rows);
> +	ctrl->cols_index = sdw_find_col_index(data->default_cols);
>  #if IS_ENABLED(CONFIG_SLIMBUS)
>  	if (dev->parent->bus == &slimbus_bus) {
>  #else
> @@ -836,8 +856,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  	params = &ctrl->bus.params;
>  	params->max_dr_freq = DEFAULT_CLK_FREQ;
>  	params->curr_dr_freq = DEFAULT_CLK_FREQ;
> -	params->col = SWRM_DEFAULT_COL;
> -	params->row = SWRM_DEFAULT_ROWS;
> +	params->col = data->default_cols;
> +	params->row = data->default_rows;
>  	ctrl->reg_read(ctrl, SWRM_MCP_STATUS, &val);
>  	params->curr_bank = val & SWRM_MCP_STATUS_BANK_NUM_MASK;
>  	params->next_bank = !params->curr_bank;
> @@ -847,8 +867,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  	prop->num_clk_gears = 0;
>  	prop->num_clk_freq = MAX_FREQ_NUM;
>  	prop->clk_freq = &qcom_swrm_freq_tbl[0];
> -	prop->default_col = SWRM_DEFAULT_COL;
> -	prop->default_row = SWRM_DEFAULT_ROWS;
> +	prop->default_col = data->default_cols;
> +	prop->default_row = data->default_rows;
>  
>  	ctrl->reg_read(ctrl, SWRM_COMP_HW_VERSION, &ctrl->version);
>  
> @@ -899,8 +919,8 @@ static int qcom_swrm_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id qcom_swrm_of_match[] = {
> -	{ .compatible = "qcom,soundwire-v1.3.0", },
> -	{ .compatible = "qcom,soundwire-v1.5.1", },
> +	{ .compatible = "qcom,soundwire-v1.3.0", .data = &swrm_v1_3_data },
> +	{ .compatible = "qcom,soundwire-v1.5.1", .data = &swrm_v1_5_data },
>  	{/* sentinel */},
>  };
>  
> -- 
> 2.21.0

-- 
~Vinod

Re: [PATCH v2 3/3] soundwire: qcom: get max rows and cols info from compatible

[Srinivas Kandagatla]

On 16/09/2020 13:49, Vinod Koul wrote:
> On 16-09-20, 10:21, Srinivas Kandagatla wrote:
>> currently the max rows and cols values are hardcoded. In reality
>> these values depend on the IP version. So get these based on
>> device tree compatible strings.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/soundwire/qcom.c | 50 ++++++++++++++++++++++++++++------------
>>   1 file changed, 35 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index 76963a7bdaa3..1dbf33684470 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -66,11 +66,6 @@
>>   #define SWRM_REG_VAL_PACK(data, dev, id, reg)	\
>>   			((reg) | ((id) << 16) | ((dev) << 20) | ((data) << 24))
>>   
>> -#define SWRM_MAX_ROW_VAL	0 /* Rows = 48 */
>> -#define SWRM_DEFAULT_ROWS	48
>> -#define SWRM_MIN_COL_VAL	0 /* Cols = 2 */
>> -#define SWRM_DEFAULT_COL	16
>> -#define SWRM_MAX_COL_VAL	7
>>   #define SWRM_SPECIAL_CMD_ID	0xF
>>   #define MAX_FREQ_NUM		1
>>   #define TIMEOUT_MS		(2 * HZ)
>> @@ -104,6 +99,8 @@ struct qcom_swrm_ctrl {
>>   	unsigned int version;
>>   	int num_din_ports;
>>   	int num_dout_ports;
>> +	int cols_index;
>> +	int rows_index;
>>   	unsigned long dout_port_mask;
>>   	unsigned long din_port_mask;
>>   	struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
>> @@ -113,6 +110,21 @@ struct qcom_swrm_ctrl {
>>   	int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val);
>>   };
>>   
>> +struct qcom_swrm_data {
>> +	u32 default_cols;
>> +	u32 default_rows;
>> +};
>> +
>> +static struct qcom_swrm_data swrm_v1_3_data = {
>> +	.default_rows = 48,
>> +	.default_cols = 16,
>> +};
>> +
>> +static struct qcom_swrm_data swrm_v1_5_data = {
>> +	.default_rows = 50,
>> +	.default_cols = 16,
>> +};
>> +
>>   #define to_qcom_sdw(b)	container_of(b, struct qcom_swrm_ctrl, bus)
>>   
>>   static int qcom_swrm_ahb_reg_read(struct qcom_swrm_ctrl *ctrl, int reg,
>> @@ -299,8 +311,10 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>>   	u32 val;
>>   
>>   	/* Clear Rows and Cols */
>> -	val = FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, SWRM_MAX_ROW_VAL);
>> -	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, SWRM_MIN_COL_VAL);
>> +	val = FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK,
>> +			 ctrl->rows_index);
>> +	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK,
>> +			  ctrl->cols_index);
> 
> single line for these ?
My vimrc had this 80 line autowrap thingy which might have done this!!

> 
>>   	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
>>   
>> @@ -378,8 +392,10 @@ static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
>>   	val &= ~SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK;
>>   	val &= ~SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK;
>>   
>> -	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, SWRM_MAX_COL_VAL);
>> -	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, SWRM_MAX_ROW_VAL);
>> +	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK,
>> +			  ctrl->cols_index);
>> +	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK,
>> +			  ctrl->rows_index);
> 
> here as well
> 
>>   
>>   	return ctrl->reg_write(ctrl, reg, val);
>>   }
>> @@ -780,6 +796,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>>   	struct sdw_master_prop *prop;
>>   	struct sdw_bus_params *params;
>>   	struct qcom_swrm_ctrl *ctrl;
>> +	const struct qcom_swrm_data *data;
>>   	int ret;
>>   	u32 val;
>>   
>> @@ -787,6 +804,9 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>>   	if (!ctrl)
>>   		return -ENOMEM;
>>   
>> +	data = of_device_get_match_data(dev);
> 
> Wont it be good to check if data is valid, we do dereference it few line
> below
We Should not hit that case as we will never reach here without matching 
compatible!
If you insist, I can add a check but data will be always be valid at the 
time of check!

> 
>> +	ctrl->rows_index = sdw_find_row_index(data->default_rows);
>> +	ctrl->cols_index = sdw_find_col_index(data->default_cols);
>>   #if IS_ENABLED(CONFIG_SLIMBUS)
>>   	if (dev->parent->bus == &slimbus_bus) {
>>   #else
>> @@ -836,8 +856,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>>   	params = &ctrl->bus.params;
>>   	params->max_dr_freq = DEFAULT_CLK_FREQ;
>>   	params->curr_dr_freq = DEFAULT_CLK_FREQ;
>> -	params->col = SWRM_DEFAULT_COL;
>> -	params->row = SWRM_DEFAULT_ROWS;
>> +	params->col = data->default_cols;
>> +	params->row = data->default_rows;
>>   	ctrl->reg_read(ctrl, SWRM_MCP_STATUS, &val);
>>   	params->curr_bank = val & SWRM_MCP_STATUS_BANK_NUM_MASK;
>>   	params->next_bank = !params->curr_bank;
>> @@ -847,8 +867,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>>   	prop->num_clk_gears = 0;
>>   	prop->num_clk_freq = MAX_FREQ_NUM;
>>   	prop->clk_freq = &qcom_swrm_freq_tbl[0];
>> -	prop->default_col = SWRM_DEFAULT_COL;
>> -	prop->default_row = SWRM_DEFAULT_ROWS;
>> +	prop->default_col = data->default_cols;
>> +	prop->default_row = data->default_rows;
>>   
>>   	ctrl->reg_read(ctrl, SWRM_COMP_HW_VERSION, &ctrl->version);
>>   
>> @@ -899,8 +919,8 @@ static int qcom_swrm_remove(struct platform_device *pdev)
>>   }
>>   
>>   static const struct of_device_id qcom_swrm_of_match[] = {
>> -	{ .compatible = "qcom,soundwire-v1.3.0", },
>> -	{ .compatible = "qcom,soundwire-v1.5.1", },
>> +	{ .compatible = "qcom,soundwire-v1.3.0", .data = &swrm_v1_3_data },
>> +	{ .compatible = "qcom,soundwire-v1.5.1", .data = &swrm_v1_5_data },
>>   	{/* sentinel */},
>>   };
>>   
>> -- 
>> 2.21.0
> 

Re: [PATCH v2 1/3] soundwire: qcom: clear BIT FIELDs before value set.

[Pierre-Louis Bossart]

>> According to usage (bitfields.h) of REG_FIELDS,
>> Modify is:
>>    reg &= ~REG_FIELD_C;
>>    reg |= FIELD_PREP(REG_FIELD_C, c);


if this is indeed the case, all the code in cadence_master.c is also 
broken, e.g:

	dpn_config = cdns_readl(cdns, dpn_config_off);

	dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_WL, (p_params->bps - 1));
	dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_PORT_FLOW, p_params->flow_mode);
	dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_PORT_DAT, p_params->data_mode);


Gah.

Re: [PATCH v2 1/3] soundwire: qcom: clear BIT FIELDs before value set.

[Vinod Koul]

On 16-09-20, 08:18, Pierre-Louis Bossart wrote:
> 
> > > According to usage (bitfields.h) of REG_FIELDS,
> > > Modify is:
> > >    reg &= ~REG_FIELD_C;
> > >    reg |= FIELD_PREP(REG_FIELD_C, c);
> 
> 
> if this is indeed the case, all the code in cadence_master.c is also broken,
> e.g:
> 
> 	dpn_config = cdns_readl(cdns, dpn_config_off);
> 
> 	dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_WL, (p_params->bps - 1));
> 	dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_PORT_FLOW, p_params->flow_mode);
> 	dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_PORT_DAT, p_params->data_mode);

This should be replaced with u32_replace_bits(), i am sending the fix
> 
> 
> Gah.

should have caught this :(

-- 
~Vinod

Re: [PATCH v2 1/3] soundwire: qcom: clear BIT FIELDs before value set.

[Pierre-Louis Bossart]

On 9/16/20 9:29 AM, Vinod Koul wrote:
> On 16-09-20, 08:18, Pierre-Louis Bossart wrote:
>>
>>>> According to usage (bitfields.h) of REG_FIELDS,
>>>> Modify is:
>>>>     reg &= ~REG_FIELD_C;
>>>>     reg |= FIELD_PREP(REG_FIELD_C, c);
>>
>>
>> if this is indeed the case, all the code in cadence_master.c is also broken,
>> e.g:
>>
>> 	dpn_config = cdns_readl(cdns, dpn_config_off);
>>
>> 	dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_WL, (p_params->bps - 1));
>> 	dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_PORT_FLOW, p_params->flow_mode);
>> 	dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_PORT_DAT, p_params->data_mode);
> 
> This should be replaced with u32_replace_bits(), i am sending the fix

wondering if we should replace all uses of FIELD_PREP with either 
u32_insert_bits() or u32_encode_bits() then?

Re: [PATCH v2 1/3] soundwire: qcom: clear BIT FIELDs before value set.

[Vinod Koul]

On 16-09-20, 09:36, Pierre-Louis Bossart wrote:
> 
> 
> On 9/16/20 9:29 AM, Vinod Koul wrote:
> > On 16-09-20, 08:18, Pierre-Louis Bossart wrote:
> > > 
> > > > > According to usage (bitfields.h) of REG_FIELDS,
> > > > > Modify is:
> > > > >     reg &= ~REG_FIELD_C;
> > > > >     reg |= FIELD_PREP(REG_FIELD_C, c);
> > > 
> > > 
> > > if this is indeed the case, all the code in cadence_master.c is also broken,
> > > e.g:
> > > 
> > > 	dpn_config = cdns_readl(cdns, dpn_config_off);
> > > 
> > > 	dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_WL, (p_params->bps - 1));
> > > 	dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_PORT_FLOW, p_params->flow_mode);
> > > 	dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_PORT_DAT, p_params->data_mode);
> > 
> > This should be replaced with u32_replace_bits(), i am sending the fix
> 
> wondering if we should replace all uses of FIELD_PREP with either
> u32_insert_bits() or u32_encode_bits() then?

That might be overkill as in the rest of the cases we have

        foo = FIELD_PREP();
        foo |= FIELD_PREP();

so the first one would set the bitfield and clear the rest

Thanks
-- 
~Vinod