On 2/18/22 2:07 AM, Manivannan Sadhasivam wrote:
On Tue, Feb 15, 2022 at 02:03:13PM -0600, Alex Elder wrote:
On 2/12/22 12:21 PM, Manivannan Sadhasivam wrote:
Add support for managing the MHI ring. The MHI ring is a circular queue
of data structures used to pass the information between host and the
endpoint.
MHI support 3 types of rings:
1. Transfer ring
2. Event ring
3. Command ring
All rings reside inside the host memory and the MHI EP device maps it to
the device memory using blocks like PCIe iATU. The mapping is handled in
the MHI EP controller driver itself.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
Great explanation. One more thing to add, is that the command
and transfer rings are directed from the host to the MHI EP device,
while the event rings are directed from the EP device toward the
host.
That's correct, will add.
I notice that you've improved a few things I had notes about,
and I don't recall suggesting them. I'm very happy about that.
I have a few more comments here, some worth thinking about
at least.
-Alex
---
drivers/bus/mhi/ep/Makefile | 2 +-
drivers/bus/mhi/ep/internal.h | 33 +++++
drivers/bus/mhi/ep/main.c | 59 +++++++-
drivers/bus/mhi/ep/ring.c | 267 ++++++++++++++++++++++++++++++++++
include/linux/mhi_ep.h | 11 ++
5 files changed, 370 insertions(+), 2 deletions(-)
create mode 100644 drivers/bus/mhi/ep/ring.c
diff --git a/drivers/bus/mhi/ep/Makefile b/drivers/bus/mhi/ep/Makefile
index a1555ae287ad..7ba0e04801eb 100644
--- a/drivers/bus/mhi/ep/Makefile
+++ b/drivers/bus/mhi/ep/Makefile
. . .
diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
index 950b5bcabe18..2c8045766292 100644
--- a/drivers/bus/mhi/ep/main.c
+++ b/drivers/bus/mhi/ep/main.c
@@ -18,6 +18,48 @@
static DEFINE_IDA(mhi_ep_cntrl_ida);
The following function handles command or channel interrupt work.
Both
What I meant was to suggest a comment that stated that it
is used for both of those. Not really a bit deal though.
+static void mhi_ep_ring_worker(struct work_struct *work)
+{
+ struct mhi_ep_cntrl *mhi_cntrl = container_of(work,
+ struct mhi_ep_cntrl, ring_work);
+ struct device *dev = &mhi_cntrl->mhi_dev->dev;
+ struct mhi_ep_ring_item *itr, *tmp;
+ struct mhi_ep_ring *ring;
+ struct mhi_ep_chan *chan;
+ unsigned long flags;
+ LIST_HEAD(head);
+ int ret;
+
+ /* Process the command ring first */
+ ret = mhi_ep_process_ring(&mhi_cntrl->mhi_cmd->ring);
+ if (ret) {
At the moment I'm not sure where this work gets scheduled.
But what if there is no command to process? It looks
like you go update the cached pointer no matter what
to see if there's anything new. But it seems like you
ought to be able to do this when interrupted for a
command rather than all the time.
No, ring cache is not getting updated all the time. If you look into
process_ring(), first the write pointer is read from MMIO and there is a
check to see if there are elements in the ring or not. Only if that
check passes, the ring cache will get updated.
Since the same work item is used for both cmd and transfer rings, this
check is necessary. The other option would be to use different work items
for command and transfer rings. This is something I want to try once
this initial version gets merged.
OK. I accept your explanation (even though I confess I did not
go back and look at the code again...).
Thanks Mani.
-Alex
Thanks,
Mani
