Re: [PATCH v2 5/7] soundwire: qcom: prepare for handling different register layouts

[Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>]

On 03/04/2023 14:25, Krzysztof Kozlowski wrote:
> Currently the driver supports Qualcomm Soundwire controller versions
> from v1.3 till v1.7 which mostly have same register layout.  With
> coming Qualcomm Soundwire v2.0, several registers were moved and
> changed, thus a different register layout will have to be supported.
>
> Prepare for this by:
> 1. Renaming few register defines to indicate v1.3 (earliest supported)
>     version,
> 2. Add a simple table for mapping register to its offset,
> 3. Change the code to use the mapping table.
>
> Since only few registers differ, this solution seems easier then
> switching to regmap fields.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
>
> ---

Reviewed-by: Srinivas Kandagagatla <srinivas.kandagatla@xxxxxxxxxx>
Tested-by: Srinivas Kandagagatla <srinivas.kandagatla@xxxxxxxxxx>

tested on RB5 and SM8450 HDK.

--srini
> Changes since v1:
> 1. Fix lang typo in subject.
> ---
>   drivers/soundwire/qcom.c | 130 +++++++++++++++++++++++++++++----------
>   1 file changed, 97 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 00522de47b6f..b6666ffe37ae 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -41,7 +41,7 @@
>   #define SWRM_COMP_PARAMS_DOUT_PORTS_MASK			GENMASK(4, 0)
>   #define SWRM_COMP_PARAMS_DIN_PORTS_MASK				GENMASK(9, 5)
>   #define SWRM_COMP_MASTER_ID					0x104
> -#define SWRM_INTERRUPT_STATUS					0x200
> +#define SWRM_V1_3_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)
> @@ -58,20 +58,20 @@
>   #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
> -#define SWRM_CMD_FIFO_WR_CMD					0x300
> -#define SWRM_CMD_FIFO_RD_CMD					0x304
> +#define SWRM_V1_3_INTERRUPT_MASK_ADDR				0x204
> +#define SWRM_V1_3_INTERRUPT_CLEAR				0x208
> +#define SWRM_V1_3_INTERRUPT_CPU_EN				0x210
> +#define SWRM_V1_3_CMD_FIFO_WR_CMD				0x300
> +#define SWRM_V1_3_CMD_FIFO_RD_CMD				0x304
>   #define SWRM_CMD_FIFO_CMD					0x308
>   #define SWRM_CMD_FIFO_FLUSH					0x1
> -#define SWRM_CMD_FIFO_STATUS					0x30C
> +#define SWRM_V1_3_CMD_FIFO_STATUS				0x30C
>   #define SWRM_RD_CMD_FIFO_CNT_MASK				GENMASK(20, 16)
>   #define SWRM_WR_CMD_FIFO_CNT_MASK				GENMASK(12, 8)
>   #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_V1_3_CMD_FIFO_RD_FIFO_ADDR				0x318
>   #define SWRM_RD_FIFO_CMD_ID_MASK				GENMASK(11, 8)
>   #define SWRM_ENUMERATOR_CFG_ADDR				0x500
>   #define SWRM_ENUMERATOR_SLAVE_DEV_ID_1(m)		(0x530 + 0x8 * (m))
> @@ -97,7 +97,7 @@
>   #define SWRM_DP_BLOCK_CTRL3_BANK(n, m)	(0x1138 + 0x100 * (n - 1) + 0x40 * m)
>   #define SWRM_DP_SAMPLECTRL2_BANK(n, m)	(0x113C + 0x100 * (n - 1) + 0x40 * m)
>   #define SWRM_DIN_DPn_PCM_PORT_CTRL(n)	(0x1054 + 0x100 * (n - 1))
> -#define SWR_MSTR_MAX_REG_ADDR		(0x1740)
> +#define SWR_V1_3_MSTR_MAX_REG_ADDR				0x1740
>   
>   #define SWRM_DP_PORT_CTRL_EN_CHAN_SHFT				0x18
>   #define SWRM_DP_PORT_CTRL_OFFSET2_SHFT				0x10
> @@ -143,10 +143,28 @@ struct qcom_swrm_port_config {
>   	u8 lane_control;
>   };
>   
> +/*
> + * Internal IDs for different register layouts.  Only few registers differ per
> + * each variant, so the list of IDs below does not include all of registers.
> + */
> +enum {
> +	SWRM_REG_FRAME_GEN_ENABLED,
> +	SWRM_REG_INTERRUPT_STATUS,
> +	SWRM_REG_INTERRUPT_MASK_ADDR,
> +	SWRM_REG_INTERRUPT_CLEAR,
> +	SWRM_REG_INTERRUPT_CPU_EN,
> +	SWRM_REG_CMD_FIFO_WR_CMD,
> +	SWRM_REG_CMD_FIFO_RD_CMD,
> +	SWRM_REG_CMD_FIFO_STATUS,
> +	SWRM_REG_CMD_FIFO_RD_FIFO_ADDR,
> +};
> +
>   struct qcom_swrm_ctrl {
>   	struct sdw_bus bus;
>   	struct device *dev;
>   	struct regmap *regmap;
> +	u32 max_reg;
> +	const unsigned int *reg_layout;
>   	void __iomem *mmio;
>   	struct reset_control *audio_cgcr;
>   #ifdef CONFIG_DEBUG_FS
> @@ -187,22 +205,42 @@ struct qcom_swrm_data {
>   	u32 default_cols;
>   	u32 default_rows;
>   	bool sw_clk_gate_required;
> +	u32 max_reg;
> +	const unsigned int *reg_layout;
> +};
> +
> +static const unsigned int swrm_v1_3_reg_layout[] = {
> +	[SWRM_REG_FRAME_GEN_ENABLED] = SWRM_COMP_STATUS,
> +	[SWRM_REG_INTERRUPT_STATUS] = SWRM_V1_3_INTERRUPT_STATUS,
> +	[SWRM_REG_INTERRUPT_MASK_ADDR] = SWRM_V1_3_INTERRUPT_MASK_ADDR,
> +	[SWRM_REG_INTERRUPT_CLEAR] = SWRM_V1_3_INTERRUPT_CLEAR,
> +	[SWRM_REG_INTERRUPT_CPU_EN] = SWRM_V1_3_INTERRUPT_CPU_EN,
> +	[SWRM_REG_CMD_FIFO_WR_CMD] = SWRM_V1_3_CMD_FIFO_WR_CMD,
> +	[SWRM_REG_CMD_FIFO_RD_CMD] = SWRM_V1_3_CMD_FIFO_RD_CMD,
> +	[SWRM_REG_CMD_FIFO_STATUS] = SWRM_V1_3_CMD_FIFO_STATUS,
> +	[SWRM_REG_CMD_FIFO_RD_FIFO_ADDR] = SWRM_V1_3_CMD_FIFO_RD_FIFO_ADDR,
>   };
>   
>   static const struct qcom_swrm_data swrm_v1_3_data = {
>   	.default_rows = 48,
>   	.default_cols = 16,
> +	.max_reg = SWR_V1_3_MSTR_MAX_REG_ADDR,
> +	.reg_layout = swrm_v1_3_reg_layout,
>   };
>   
>   static const struct qcom_swrm_data swrm_v1_5_data = {
>   	.default_rows = 50,
>   	.default_cols = 16,
> +	.max_reg = SWR_V1_3_MSTR_MAX_REG_ADDR,
> +	.reg_layout = swrm_v1_3_reg_layout,
>   };
>   
>   static const struct qcom_swrm_data swrm_v1_6_data = {
>   	.default_rows = 50,
>   	.default_cols = 16,
>   	.sw_clk_gate_required = true,
> +	.max_reg = SWR_V1_3_MSTR_MAX_REG_ADDR,
> +	.reg_layout = swrm_v1_3_reg_layout,
>   };
>   
>   #define to_qcom_sdw(b)	container_of(b, struct qcom_swrm_ctrl, bus)
> @@ -286,7 +324,8 @@ static int swrm_wait_for_rd_fifo_avail(struct qcom_swrm_ctrl *ctrl)
>   
>   	do {
>   		/* Check for fifo underflow during read */
> -		ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
> +		ctrl->reg_read(ctrl, ctrl->reg_layout[SWRM_REG_CMD_FIFO_STATUS],
> +			       &value);
>   		fifo_outstanding_data = FIELD_GET(SWRM_RD_CMD_FIFO_CNT_MASK, value);
>   
>   		/* Check if read data is available in read fifo */
> @@ -311,7 +350,8 @@ static int swrm_wait_for_wr_fifo_avail(struct qcom_swrm_ctrl *ctrl)
>   
>   	do {
>   		/* Check for fifo overflow during write */
> -		ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
> +		ctrl->reg_read(ctrl, ctrl->reg_layout[SWRM_REG_CMD_FIFO_STATUS],
> +			       &value);
>   		fifo_outstanding_cmds = FIELD_GET(SWRM_WR_CMD_FIFO_CNT_MASK, value);
>   
>   		/* Check for space in write fifo before writing */
> @@ -353,7 +393,7 @@ static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
>   		reinit_completion(&ctrl->broadcast);
>   
>   	/* Its assumed that write is okay as we do not get any status back */
> -	ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
> +	ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_CMD_FIFO_WR_CMD], val);
>   
>   	if (ctrl->version <= SWRM_VERSION_1_3_0)
>   		usleep_range(150, 155);
> @@ -392,7 +432,7 @@ static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
>   
>   	/* wait for FIFO RD to complete to avoid overflow */
>   	usleep_range(100, 105);
> -	ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
> +	ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_CMD_FIFO_RD_CMD], val);
>   	/* wait for FIFO RD CMD complete to avoid overflow */
>   	usleep_range(250, 255);
>   
> @@ -400,7 +440,8 @@ static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
>   		return SDW_CMD_FAIL_OTHER;
>   
>   	do {
> -		ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR, &cmd_data);
> +		ctrl->reg_read(ctrl, ctrl->reg_layout[SWRM_REG_CMD_FIFO_RD_FIFO_ADDR],
> +			       &cmd_data);
>   		rval[0] = cmd_data & 0xFF;
>   		cmd_id = FIELD_GET(SWRM_RD_FIFO_CMD_ID_MASK, cmd_data);
>   
> @@ -410,7 +451,9 @@ static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
>   				usleep_range(500, 505);
>   				ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD,
>   						SWRM_CMD_FIFO_FLUSH);
> -				ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
> +				ctrl->reg_write(ctrl,
> +						ctrl->reg_layout[SWRM_REG_CMD_FIFO_RD_CMD],
> +						val);
>   			}
>   			retry_attempt++;
>   		} else {
> @@ -564,7 +607,8 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>   	int ret = IRQ_HANDLED;
>   	clk_prepare_enable(ctrl->hclk);
>   
> -	ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS, &intr_sts);
> +	ctrl->reg_read(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_STATUS],
> +		       &intr_sts);
>   	intr_sts_masked = intr_sts & ctrl->intr_mask;
>   
>   	do {
> @@ -602,29 +646,39 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>   						"%s: SWR bus clsh detected\n",
>   						__func__);
>   				ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
> -				ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN, ctrl->intr_mask);
> +				ctrl->reg_write(ctrl,
> +						ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
> +						ctrl->intr_mask);
>   				break;
>   			case SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW:
> -				ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
> +				ctrl->reg_read(ctrl,
> +					       ctrl->reg_layout[SWRM_REG_CMD_FIFO_STATUS],
> +					       &value);
>   				dev_err_ratelimited(ctrl->dev,
>   					"%s: SWR read FIFO overflow fifo status 0x%x\n",
>   					__func__, value);
>   				break;
>   			case SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOW:
> -				ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
> +				ctrl->reg_read(ctrl,
> +					       ctrl->reg_layout[SWRM_REG_CMD_FIFO_STATUS],
> +					       &value);
>   				dev_err_ratelimited(ctrl->dev,
>   					"%s: SWR read FIFO underflow fifo status 0x%x\n",
>   					__func__, value);
>   				break;
>   			case SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW:
> -				ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
> +				ctrl->reg_read(ctrl,
> +					       ctrl->reg_layout[SWRM_REG_CMD_FIFO_STATUS],
> +					       &value);
>   				dev_err(ctrl->dev,
>   					"%s: SWR write FIFO overflow fifo status %x\n",
>   					__func__, value);
>   				ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
>   				break;
>   			case SWRM_INTERRUPT_STATUS_CMD_ERROR:
> -				ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
> +				ctrl->reg_read(ctrl,
> +					       ctrl->reg_layout[SWRM_REG_CMD_FIFO_STATUS],
> +					       &value);
>   				dev_err_ratelimited(ctrl->dev,
>   					"%s: SWR CMD error, fifo status 0x%x, flushing fifo\n",
>   					__func__, value);
> @@ -636,7 +690,8 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>   						__func__);
>   				ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION;
>   				ctrl->reg_write(ctrl,
> -					SWRM_INTERRUPT_CPU_EN, ctrl->intr_mask);
> +						ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
> +						ctrl->intr_mask);
>   				break;
>   			case SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH:
>   				dev_err_ratelimited(ctrl->dev,
> @@ -645,7 +700,8 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>   				ctrl->intr_mask &=
>   					~SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH;
>   				ctrl->reg_write(ctrl,
> -					SWRM_INTERRUPT_CPU_EN, ctrl->intr_mask);
> +						ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
> +						ctrl->intr_mask);
>   				break;
>   			case SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED:
>   				complete(&ctrl->broadcast);
> @@ -664,8 +720,10 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>   				break;
>   			}
>   		}
> -		ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, intr_sts);
> -		ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS, &intr_sts);
> +		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CLEAR],
> +				intr_sts);
> +		ctrl->reg_read(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_STATUS],
> +			       &intr_sts);
>   		intr_sts_masked = intr_sts & ctrl->intr_mask;
>   	} while (intr_sts_masked);
>   
> @@ -690,7 +748,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>   
>   	ctrl->intr_mask = SWRM_INTERRUPT_STATUS_RMSK;
>   	/* Mask soundwire interrupts */
> -	ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
> +	ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_MASK_ADDR],
>   			SWRM_INTERRUPT_STATUS_RMSK);
>   
>   	/* Configure No pings */
> @@ -723,7 +781,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>   
>   	/* enable CPU IRQs */
>   	if (ctrl->mmio) {
> -		ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN,
> +		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
>   				SWRM_INTERRUPT_STATUS_RMSK);
>   	}
>   	ctrl->slave_status = 0;
> @@ -1312,7 +1370,7 @@ static int swrm_reg_show(struct seq_file *s_file, void *data)
>   		return ret;
>   	}
>   
> -	for (reg = 0; reg <= SWR_MSTR_MAX_REG_ADDR; reg += 4) {
> +	for (reg = 0; reg <= ctrl->max_reg; reg += 4) {
>   		ctrl->reg_read(ctrl, reg, &reg_val);
>   		seq_printf(s_file, "0x%.3x: 0x%.2x\n", reg, reg_val);
>   	}
> @@ -1340,6 +1398,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	data = of_device_get_match_data(dev);
> +	ctrl->max_reg = data->max_reg;
> +	ctrl->reg_layout = data->reg_layout;
>   	ctrl->rows_index = sdw_find_row_index(data->default_rows);
>   	ctrl->cols_index = sdw_find_col_index(data->default_cols);
>   #if IS_REACHABLE(CONFIG_SLIMBUS)
> @@ -1556,12 +1616,14 @@ static int __maybe_unused swrm_runtime_resume(struct device *dev)
>   		} else {
>   			ctrl->reg_write(ctrl, SWRM_MCP_BUS_CTRL, SWRM_MCP_BUS_CLK_START);
>   		}
> -		ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR,
> +		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CLEAR],
>   			SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET);
>   
>   		ctrl->intr_mask |= SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
> -		ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR, ctrl->intr_mask);
> -		ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN, ctrl->intr_mask);
> +		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_MASK_ADDR],
> +				ctrl->intr_mask);
> +		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
> +				ctrl->intr_mask);
>   
>   		usleep_range(100, 105);
>   		if (!swrm_wait_for_frame_gen_enabled(ctrl))
> @@ -1583,8 +1645,10 @@ static int __maybe_unused swrm_runtime_suspend(struct device *dev)
>   	if (!ctrl->clock_stop_not_supported) {
>   		/* Mask bus clash interrupt */
>   		ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
> -		ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR, ctrl->intr_mask);
> -		ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN, ctrl->intr_mask);
> +		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_MASK_ADDR],
> +				ctrl->intr_mask);
> +		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
> +				ctrl->intr_mask);
>   		/* Prepare slaves for clock stop */
>   		ret = sdw_bus_prep_clk_stop(&ctrl->bus);
>   		if (ret < 0 && ret != -ENODATA) {