On 2/28/22 6:43 AM, Manivannan Sadhasivam wrote:
Add support for managing the Memory Mapped Input Output (MMIO) registers
of the MHI bus. All MHI operations are carried out using the MMIO registers
by both host and the endpoint device.
The MMIO registers reside inside the endpoint device memory (fixed
location based on the platform) and the address is passed by the MHI EP
controller driver during its registration.
I thought it might have been a mistake that MHI_MASK_ROWS_CH_EV_DB
was used when iterating over channels and events. Now I see it
represents the number of "rows" of 32-bit doorbell registers for
either events or channels.
I guess it might be reasonable to assume the number of event "rows"
is the same as the number of channel rows. But *maybe* consider
defining them separately, like:
MHI_MASK_ROWS_CH_DB
MHI_MASK_ROWS_EV_DB
I also have one more comment below.
Whether or not you implement one or both of these suggestions:
Reviewed-by: Alex Elder <elder@xxxxxxxxxx>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
---
drivers/bus/mhi/ep/Makefile | 2 +-
drivers/bus/mhi/ep/internal.h | 26 ++++
drivers/bus/mhi/ep/main.c | 6 +-
drivers/bus/mhi/ep/mmio.c | 272 ++++++++++++++++++++++++++++++++++
include/linux/mhi_ep.h | 18 +++
5 files changed, 322 insertions(+), 2 deletions(-)
create mode 100644 drivers/bus/mhi/ep/mmio.c
. . .
diff --git a/drivers/bus/mhi/ep/mmio.c b/drivers/bus/mhi/ep/mmio.c
new file mode 100644
index 000000000000..311c5d94c4d2
--- /dev/null
+++ b/drivers/bus/mhi/ep/mmio.c
@@ -0,0 +1,272 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Linaro Ltd.
+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/io.h>
+#include <linux/mhi_ep.h>
+
+#include "internal.h"
+
. . .
+bool mhi_ep_mmio_read_chdb_status_interrupts(struct mhi_ep_cntrl *mhi_cntrl)
+{
+ bool chdb = 0;
+ u32 i;
+
+ for (i = 0; i < MHI_MASK_ROWS_CH_EV_DB; i++) {
+ mhi_cntrl->chdb[i].status = mhi_ep_mmio_read(mhi_cntrl, MHI_CHDB_INT_STATUS_n(i));
+ chdb |= !!mhi_cntrl->chdb[i].status;
This is fine, but I think I'd prefer this to be:
if (mhi_cntrl->chdb[i].status)
chdb = true;
Because you're using a bitwise operator to set a Boolean value.
+ }
+
+ /* Return whether a channel doorbell interrupt occurred or not */
+ return chdb;
+}
+
. . .
