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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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