diff mbox series

[v3,15/25] bus: mhi: ep: Add support for processing MHI endpoint interrupts

Message ID 20220212182117.49438-16-manivannan.sadhasivam@linaro.org (mailing list archive)
State Superseded
Headers show
Series Add initial support for MHI endpoint stack | expand

Commit Message

Manivannan Sadhasivam Feb. 12, 2022, 6:21 p.m. UTC 
Add support for processing MHI endpoint interrupts such as control
interrupt, command interrupt and channel interrupt from the host.

The interrupts will be generated in the endpoint device whenever host
writes to the corresponding doorbell registers. The doorbell logic
is handled inside the hardware internally.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/ep/main.c | 113 +++++++++++++++++++++++++++++++++++++-
 include/linux/mhi_ep.h    |   2 +
 2 files changed, 113 insertions(+), 2 deletions(-)

Comments

Alex Elder Feb. 15, 2022, 10:39 p.m. UTC | #1 
On 2/12/22 12:21 PM, Manivannan Sadhasivam wrote:
> Add support for processing MHI endpoint interrupts such as control
> interrupt, command interrupt and channel interrupt from the host.
> 
> The interrupts will be generated in the endpoint device whenever host
> writes to the corresponding doorbell registers. The doorbell logic
> is handled inside the hardware internally.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Unless I'm mistaken, you have some bugs here.

Beyond that, I question whether you should be using workqueues
for handling all interrupts.  For now, it's fine, but there
might be room for improvement after this is accepted upstream
(using threaded interrupt handlers, for example).

					-Alex

> ---
>   drivers/bus/mhi/ep/main.c | 113 +++++++++++++++++++++++++++++++++++++-
>   include/linux/mhi_ep.h    |   2 +
>   2 files changed, 113 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> index ccb3c2795041..072b872e735b 100644
> --- a/drivers/bus/mhi/ep/main.c
> +++ b/drivers/bus/mhi/ep/main.c
> @@ -185,6 +185,56 @@ static void mhi_ep_ring_worker(struct work_struct *work)
>   	}
>   }
>   
> +static void mhi_ep_queue_channel_db(struct mhi_ep_cntrl *mhi_cntrl,
> +				    unsigned long ch_int, u32 ch_idx)
> +{
> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> +	struct mhi_ep_ring_item *item;
> +	struct mhi_ep_ring *ring;
> +	unsigned int i;

Why not u32 i?  And why is the ch_int argument unsigned long?  The value
passed in is a u32.

> +
> +	for_each_set_bit(i, &ch_int, 32) {
> +		/* Channel index varies for each register: 0, 32, 64, 96 */
> +		i += ch_idx;

This is a bug.  You should not be modifying the iterator
variable inside the loop.  Maybe do this instead:

	u32 ch_id = ch_idx + i;

	ring = &mhi_cntrl->mhi_chan[ch_id].ring

> +		ring = &mhi_cntrl->mhi_chan[i].ring;
> +

You are initializing all fields here so kmalloc() is fine
(rather than kzalloc()).  But if you ever add another field
to the mhi_ep_ring_item structure that's not guaranteed.
I think at least a comment here explaining why you're not
using kzalloc() would be helpful.

> +		item = kmalloc(sizeof(*item), GFP_ATOMIC);

Even an ATOMIC allocation can fail.  Check the return
pointer.

> +		item->ring = ring;
> +
> +		dev_dbg(dev, "Queuing doorbell interrupt for channel (%d)\n", i);

Use ch_id (or whatever you call it) here too.

> +		spin_lock(&mhi_cntrl->list_lock);
> +		list_add_tail(&item->node, &mhi_cntrl->ch_db_list);
> +		spin_unlock(&mhi_cntrl->list_lock);

Instead, create a list head on the stack and build up
this list without using the spinlock.  Then splice
everything you added into the ch_db_list at the end.

> +
> +		queue_work(mhi_cntrl->ring_wq, &mhi_cntrl->ring_work);

Maybe there's a small amount of latency saved by
doing this repeatedly, but you're queueing work
with the same work structure over and over again.

Instead, you could set a Boolean at the top:
	work = !!ch_int;

	for_each_set_bit() {
		. . .
	}

	if (work)
		queue_work(...);


> +	}
> +}
> +
> +/*
> + * Channel interrupt statuses are contained in 4 registers each of 32bit length.
> + * For checking all interrupts, we need to loop through each registers and then
> + * check for bits set.
> + */
> +static void mhi_ep_check_channel_interrupt(struct mhi_ep_cntrl *mhi_cntrl)
> +{
> +	u32 ch_int, ch_idx;
> +	int i;
> +
> +	mhi_ep_mmio_read_chdb_status_interrupts(mhi_cntrl);

You could have the above function could return a summary Boolean
value, which would indicate whether *any* channel interrupts
had occurred (skipping the below loop when we get just a control
or command doorbell interrupt).

> +
> +	for (i = 0; i < MHI_MASK_ROWS_CH_EV_DB; i++) {
> +		ch_idx = i * MHI_MASK_CH_EV_LEN;
> +
> +		/* Only process channel interrupt if the mask is enabled */
> +		ch_int = (mhi_cntrl->chdb[i].status & mhi_cntrl->chdb[i].mask);

Parentheses not needed.

> +		if (ch_int) {
> +			mhi_ep_queue_channel_db(mhi_cntrl, ch_int, ch_idx);
> +			mhi_ep_mmio_write(mhi_cntrl, MHI_CHDB_INT_CLEAR_A7_n(i),
> +							mhi_cntrl->chdb[i].status);
> +		}
> +	}
> +}
> +
>   static void mhi_ep_state_worker(struct work_struct *work)
>   {
>   	struct mhi_ep_cntrl *mhi_cntrl = container_of(work, struct mhi_ep_cntrl, state_work);
> @@ -222,6 +272,53 @@ static void mhi_ep_state_worker(struct work_struct *work)
>   	}
>   }
>   
> +static void mhi_ep_process_ctrl_interrupt(struct mhi_ep_cntrl *mhi_cntrl,
> +					 enum mhi_state state)
> +{
> +	struct mhi_ep_state_transition *item = kmalloc(sizeof(*item), GFP_ATOMIC);
> +

Do not assume ATOMIC allocations succeed.

I don't have any further comments on the rest.

> +	item->state = state;
> +	spin_lock(&mhi_cntrl->list_lock);
> +	list_add_tail(&item->node, &mhi_cntrl->st_transition_list);
> +	spin_unlock(&mhi_cntrl->list_lock);
> +
> +	queue_work(mhi_cntrl->state_wq, &mhi_cntrl->state_work);
> +}
> +

. . .
Manivannan Sadhasivam Feb. 22, 2022, 8:18 a.m. UTC | #2 
On Tue, Feb 15, 2022 at 04:39:30PM -0600, Alex Elder wrote:
> On 2/12/22 12:21 PM, Manivannan Sadhasivam wrote:
> > Add support for processing MHI endpoint interrupts such as control
> > interrupt, command interrupt and channel interrupt from the host.
> > 
> > The interrupts will be generated in the endpoint device whenever host
> > writes to the corresponding doorbell registers. The doorbell logic
> > is handled inside the hardware internally.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Unless I'm mistaken, you have some bugs here.
> 
> Beyond that, I question whether you should be using workqueues
> for handling all interrupts.  For now, it's fine, but there
> might be room for improvement after this is accepted upstream
> (using threaded interrupt handlers, for example).
> 

Only reason I didn't use bottom halves is that the memory for TRE buffers need
to be allocated each time, so essentially the caller should not sleep.

This is currently a limitation of iATU where there are only 8 windows for
mapping the host memory and each memory region size is also limited.

> 					-Alex
> 
> > ---
> >   drivers/bus/mhi/ep/main.c | 113 +++++++++++++++++++++++++++++++++++++-
> >   include/linux/mhi_ep.h    |   2 +
> >   2 files changed, 113 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> > index ccb3c2795041..072b872e735b 100644
> > --- a/drivers/bus/mhi/ep/main.c
> > +++ b/drivers/bus/mhi/ep/main.c
> > @@ -185,6 +185,56 @@ static void mhi_ep_ring_worker(struct work_struct *work)
> >   	}
> >   }
> > +static void mhi_ep_queue_channel_db(struct mhi_ep_cntrl *mhi_cntrl,
> > +				    unsigned long ch_int, u32 ch_idx)
> > +{
> > +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> > +	struct mhi_ep_ring_item *item;
> > +	struct mhi_ep_ring *ring;
> > +	unsigned int i;
> 
> Why not u32 i?  And why is the ch_int argument unsigned long?  The value
> passed in is a u32.
> 

for_each_set_bit() expects the 2nd argument to be of type "unsigned long".

Thanks,
Mani
Alex Elder Feb. 22, 2022, 2:08 p.m. UTC | #3 
On 2/22/22 2:18 AM, Manivannan Sadhasivam wrote:
> On Tue, Feb 15, 2022 at 04:39:30PM -0600, Alex Elder wrote:
>> On 2/12/22 12:21 PM, Manivannan Sadhasivam wrote:
>>> Add support for processing MHI endpoint interrupts such as control
>>> interrupt, command interrupt and channel interrupt from the host.
>>>
>>> The interrupts will be generated in the endpoint device whenever host
>>> writes to the corresponding doorbell registers. The doorbell logic
>>> is handled inside the hardware internally.
>>>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
>> Unless I'm mistaken, you have some bugs here.
>>
>> Beyond that, I question whether you should be using workqueues
>> for handling all interrupts.  For now, it's fine, but there
>> might be room for improvement after this is accepted upstream
>> (using threaded interrupt handlers, for example).
>>
> 
> Only reason I didn't use bottom halves is that the memory for TRE buffers need
> to be allocated each time, so essentially the caller should not sleep.

Threaded interrupt handlers can sleep.  If scheduled, they run
immediately after hard interrupt handlers.  For receive buffers,
yes, replacing a receive buffer just consumed would require an
allocation, but for transmit I think it might be possible to
avoid the need to do a memory allocation.  (Things to think
about at some future date.)

> This is currently a limitation of iATU where there are only 8 windows for
> mapping the host memory and each memory region size is also limited.

Those are hard limitations, and probably what constrains you the most.

					-Alex
> 
>> 					-Alex
>>
>>> ---
>>>    drivers/bus/mhi/ep/main.c | 113 +++++++++++++++++++++++++++++++++++++-
>>>    include/linux/mhi_ep.h    |   2 +
>>>    2 files changed, 113 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
>>> index ccb3c2795041..072b872e735b 100644
>>> --- a/drivers/bus/mhi/ep/main.c
>>> +++ b/drivers/bus/mhi/ep/main.c
>>> @@ -185,6 +185,56 @@ static void mhi_ep_ring_worker(struct work_struct *work)
>>>    	}
>>>    }
>>> +static void mhi_ep_queue_channel_db(struct mhi_ep_cntrl *mhi_cntrl,
>>> +				    unsigned long ch_int, u32 ch_idx)
>>> +{
>>> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>>> +	struct mhi_ep_ring_item *item;
>>> +	struct mhi_ep_ring *ring;
>>> +	unsigned int i;
>>
>> Why not u32 i?  And why is the ch_int argument unsigned long?  The value
>> passed in is a u32.
>>
> 
> for_each_set_bit() expects the 2nd argument to be of type "unsigned long".
> 
> Thanks,
> Mani
diff mbox series

Patch

diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
index ccb3c2795041..072b872e735b 100644
--- a/drivers/bus/mhi/ep/main.c
+++ b/drivers/bus/mhi/ep/main.c
@@ -185,6 +185,56 @@  static void mhi_ep_ring_worker(struct work_struct *work)
 	}
 }
 
+static void mhi_ep_queue_channel_db(struct mhi_ep_cntrl *mhi_cntrl,
+				    unsigned long ch_int, u32 ch_idx)
+{
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
+	struct mhi_ep_ring_item *item;
+	struct mhi_ep_ring *ring;
+	unsigned int i;
+
+	for_each_set_bit(i, &ch_int, 32) {
+		/* Channel index varies for each register: 0, 32, 64, 96 */
+		i += ch_idx;
+		ring = &mhi_cntrl->mhi_chan[i].ring;
+
+		item = kmalloc(sizeof(*item), GFP_ATOMIC);
+		item->ring = ring;
+
+		dev_dbg(dev, "Queuing doorbell interrupt for channel (%d)\n", i);
+		spin_lock(&mhi_cntrl->list_lock);
+		list_add_tail(&item->node, &mhi_cntrl->ch_db_list);
+		spin_unlock(&mhi_cntrl->list_lock);
+
+		queue_work(mhi_cntrl->ring_wq, &mhi_cntrl->ring_work);
+	}
+}
+
+/*
+ * Channel interrupt statuses are contained in 4 registers each of 32bit length.
+ * For checking all interrupts, we need to loop through each registers and then
+ * check for bits set.
+ */
+static void mhi_ep_check_channel_interrupt(struct mhi_ep_cntrl *mhi_cntrl)
+{
+	u32 ch_int, ch_idx;
+	int i;
+
+	mhi_ep_mmio_read_chdb_status_interrupts(mhi_cntrl);
+
+	for (i = 0; i < MHI_MASK_ROWS_CH_EV_DB; i++) {
+		ch_idx = i * MHI_MASK_CH_EV_LEN;
+
+		/* Only process channel interrupt if the mask is enabled */
+		ch_int = (mhi_cntrl->chdb[i].status & mhi_cntrl->chdb[i].mask);
+		if (ch_int) {
+			mhi_ep_queue_channel_db(mhi_cntrl, ch_int, ch_idx);
+			mhi_ep_mmio_write(mhi_cntrl, MHI_CHDB_INT_CLEAR_A7_n(i),
+							mhi_cntrl->chdb[i].status);
+		}
+	}
+}
+
 static void mhi_ep_state_worker(struct work_struct *work)
 {
 	struct mhi_ep_cntrl *mhi_cntrl = container_of(work, struct mhi_ep_cntrl, state_work);
@@ -222,6 +272,53 @@  static void mhi_ep_state_worker(struct work_struct *work)
 	}
 }
 
+static void mhi_ep_process_ctrl_interrupt(struct mhi_ep_cntrl *mhi_cntrl,
+					 enum mhi_state state)
+{
+	struct mhi_ep_state_transition *item = kmalloc(sizeof(*item), GFP_ATOMIC);
+
+	item->state = state;
+	spin_lock(&mhi_cntrl->list_lock);
+	list_add_tail(&item->node, &mhi_cntrl->st_transition_list);
+	spin_unlock(&mhi_cntrl->list_lock);
+
+	queue_work(mhi_cntrl->state_wq, &mhi_cntrl->state_work);
+}
+
+/*
+ * Interrupt handler that services interrupts raised by the host writing to
+ * MHICTRL and Command ring doorbell (CRDB) registers for state change and
+ * channel interrupts.
+ */
+static irqreturn_t mhi_ep_irq(int irq, void *data)
+{
+	struct mhi_ep_cntrl *mhi_cntrl = data;
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
+	enum mhi_state state;
+	u32 int_value;
+
+	/* Acknowledge the interrupts */
+	int_value = mhi_ep_mmio_read(mhi_cntrl, MHI_CTRL_INT_STATUS_A7);
+	mhi_ep_mmio_write(mhi_cntrl, MHI_CTRL_INT_CLEAR_A7, int_value);
+
+	/* Check for ctrl interrupt */
+	if (FIELD_GET(MHI_CTRL_INT_STATUS_A7_MSK, int_value)) {
+		dev_dbg(dev, "Processing ctrl interrupt\n");
+		mhi_ep_process_ctrl_interrupt(mhi_cntrl, state);
+	}
+
+	/* Check for command doorbell interrupt */
+	if (FIELD_GET(MHI_CTRL_INT_STATUS_CRDB_MSK, int_value)) {
+		dev_dbg(dev, "Processing command doorbell interrupt\n");
+		queue_work(mhi_cntrl->ring_wq, &mhi_cntrl->ring_work);
+	}
+
+	/* Check for channel interrupts */
+	mhi_ep_check_channel_interrupt(mhi_cntrl);
+
+	return IRQ_HANDLED;
+}
+
 static void mhi_ep_release_device(struct device *dev)
 {
 	struct mhi_ep_device *mhi_dev = to_mhi_ep_device(dev);
@@ -409,7 +506,7 @@  int mhi_ep_register_controller(struct mhi_ep_cntrl *mhi_cntrl,
 	struct mhi_ep_device *mhi_dev;
 	int ret;
 
-	if (!mhi_cntrl || !mhi_cntrl->cntrl_dev || !mhi_cntrl->mmio)
+	if (!mhi_cntrl || !mhi_cntrl->cntrl_dev || !mhi_cntrl->mmio || !mhi_cntrl->irq)
 		return -EINVAL;
 
 	ret = parse_ch_cfg(mhi_cntrl, config);
@@ -454,12 +551,20 @@  int mhi_ep_register_controller(struct mhi_ep_cntrl *mhi_cntrl,
 		goto err_destroy_state_wq;
 	}
 
+	irq_set_status_flags(mhi_cntrl->irq, IRQ_NOAUTOEN);
+	ret = request_irq(mhi_cntrl->irq, mhi_ep_irq, IRQF_TRIGGER_HIGH,
+			  "doorbell_irq", mhi_cntrl);
+	if (ret) {
+		dev_err(mhi_cntrl->cntrl_dev, "Failed to request Doorbell IRQ\n");
+		goto err_ida_free;
+	}
+
 	/* Allocate the controller device */
 	mhi_dev = mhi_ep_alloc_device(mhi_cntrl, MHI_DEVICE_CONTROLLER);
 	if (IS_ERR(mhi_dev)) {
 		dev_err(mhi_cntrl->cntrl_dev, "Failed to allocate controller device\n");
 		ret = PTR_ERR(mhi_dev);
-		goto err_ida_free;
+		goto err_free_irq;
 	}
 
 	dev_set_name(&mhi_dev->dev, "mhi_ep%d", mhi_cntrl->index);
@@ -477,6 +582,8 @@  int mhi_ep_register_controller(struct mhi_ep_cntrl *mhi_cntrl,
 
 err_put_dev:
 	put_device(&mhi_dev->dev);
+err_free_irq:
+	free_irq(mhi_cntrl->irq, mhi_cntrl);
 err_ida_free:
 	ida_free(&mhi_ep_cntrl_ida, mhi_cntrl->index);
 err_destroy_state_wq:
@@ -499,6 +606,8 @@  void mhi_ep_unregister_controller(struct mhi_ep_cntrl *mhi_cntrl)
 	destroy_workqueue(mhi_cntrl->state_wq);
 	destroy_workqueue(mhi_cntrl->ring_wq);
 
+	free_irq(mhi_cntrl->irq, mhi_cntrl);
+
 	kfree(mhi_cntrl->mhi_cmd);
 	kfree(mhi_cntrl->mhi_chan);
 
diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h
index 72ce30cbe87e..a207058a4991 100644
--- a/include/linux/mhi_ep.h
+++ b/include/linux/mhi_ep.h
@@ -90,6 +90,7 @@  struct mhi_ep_db_info {
  * @chdb_offset: Channel doorbell offset set by the host
  * @erdb_offset: Event ring doorbell offset set by the host
  * @index: MHI Endpoint controller index
+ * @irq: IRQ used by the endpoint controller
  */
 struct mhi_ep_cntrl {
 	struct device *cntrl_dev;
@@ -142,6 +143,7 @@  struct mhi_ep_cntrl {
 	u32 chdb_offset;
 	u32 erdb_offset;
 	int index;
+	int irq;
 };
 
 /**