Re: [PATCH 6/6] soundwire: qcom: add support to new interrupts

[Vinod Koul <vkoul@kernel.org>]

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