On Fri, Mar 24, 2023 at 09:26:49AM -0600, Jeffrey Hugo wrote:
On 3/24/2023 4:26 AM, Manivannan Sadhasivam wrote:
On Mon, Mar 20, 2023 at 09:11:09AM -0600, Jeffrey Hugo wrote:
An AIC100 device contains a MHI interface with a number of different
channels for controlling different aspects of the device. The MHI
controller works with the MHI bus to enable and drive that interface.
AIC100 uses the BHI protocol in PBL to load SBL. The MHI controller
expects the SBL to be located at /lib/firmware/qcom/aic100/sbl.bin and
expects the MHI bus to manage the process of loading and sending SBL to
the device.
Signed-off-by: Jeffrey Hugo <quic_jhugo@xxxxxxxxxxx>
Reviewed-by: Carl Vanderlip <quic_carlv@xxxxxxxxxxx>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@xxxxxxxxxxx>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@xxxxxxxxxxxxxxx>
---
drivers/accel/qaic/mhi_controller.c | 563 ++++++++++++++++++++++++++++++++++++
drivers/accel/qaic/mhi_controller.h | 16 +
2 files changed, 579 insertions(+)
create mode 100644 drivers/accel/qaic/mhi_controller.c
create mode 100644 drivers/accel/qaic/mhi_controller.h
diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
new file mode 100644
index 0000000..777dfbe
--- /dev/null
+++ b/drivers/accel/qaic/mhi_controller.c
@@ -0,0 +1,563 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/* Copyright (c) 2019-2021, The Linux Foundation. All rights reserved. */
+/* Copyright (c) 2021-2023 Qualcomm Innovation Center, Inc. All rights reserved. */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/memblock.h>
+#include <linux/mhi.h>
+#include <linux/moduleparam.h>
+#include <linux/pci.h>
+#include <linux/sizes.h>
+
+#include "mhi_controller.h"
+#include "qaic.h"
+
+#define MAX_RESET_TIME_SEC 25
+
+static unsigned int mhi_timeout_ms = 2000; /* 2 sec default */
+module_param(mhi_timeout_ms, uint, 0600);
+MODULE_PARM_DESC(mhi_timeout_ms, "MHI controller timeout value");
+
+static struct mhi_channel_config aic100_channels[] = {
+ {
+ .name = "QAIC_LOOPBACK",
Why do you need QAIC_ prefix for channel names?
To avoid existing and anticipated conflicts.
As you are aware, the channel name becomes critical for the bus device and
is the key that the consumer driver will probe on.
Sadly, that is rife for conflicts. You can only have one driver for a
particular MHI device (channel). Multiple drivers can register for it, but
only the first one will bind to the device. This creates a race condition.
Whoever is able to register with the bus first, owns all instances of that
device. That also means that particular driver on the bus also needs to be
able to handle all instances of that device.
The WWAN subsystem already claims DIAG. You and I both know from the WWAN
subsystem creation experience, the Net folks don't want a common framework
that can service multiple types of devices. QAIC devices are not WWAN
devices, and were an argument for having a WWAN specific thing. So, I can't
leverage WWAN, and frankly I shouldn't because my device is not a WWAN
device. The WWAN userspace shouldn't try to use ACCEL/QAIC devices (one of
the reasons for having ACCEL instead of DRM). Therefore DIAG devices are
WWAN exclusive, and I need to have a different device. "DIAG2" seems like a
poor name. If the QAIC DIAG device is going to be QAIC specific, having
QAIC in the name to isolate and identify it seems like the best option.
I anticipate similar conflicts with
SAHARA/QDSS/DEBUG/TIMESYNC/LOGGING/LOOPBACK. All of these are "common" with
other existing MHI devices.
Hmm, this is something I anticipated to happen... :/
I anticipate future conflicts with STATUS/RAS/TELEMETRY/CONTROL/SSR. These
are rather generic channel names. It seems likely that a future WWAN device
or other MHI device would want a channel with the same name as one of these.
I'd like to leave that open as a possibility by not exclusivly claiming the
sole use to one of these names.
Arguably this is an internal implementation detail with how the MHI bus
operates and could be fixed at first look. However I don't think that is
the case because it looks like the WWAN subsystem is exposing these names to
userspace, which creates a uAPI that cannot be broken. Therefore I think we
are rather quite stuck with this situation and what I have proposed with
this patch is the best thing I've come up with to address the problem. If
you have an alternate suggestion, I'm willing to discuss with you.
I think what you have is the best for now. Only downside of this approach is
the code duplication among the client drivers but I think we compromised this
during the WWAN framework discussion.
+ .num = 0,
+ .num_elements = 32,
+ .local_elements = 0,
+ .event_ring = 0,
+ .dir = DMA_TO_DEVICE,
+ .ee_mask = MHI_CH_EE_AMSS,
+ .pollcfg = 0,
+ .doorbell = MHI_DB_BRST_DISABLE,
+ .lpm_notify = false,
+ .offload_channel = false,
+ .doorbell_mode_switch = false,
+ .auto_queue = false,
+ .wake_capable = false,
+ },
[...]
+static struct mhi_event_config aic100_events[] = {
+ {
+ .num_elements = 32,
+ .irq_moderation_ms = 0,
+ .irq = 0,
+ .channel = U32_MAX,
+ .priority = 1,
+ .mode = MHI_DB_BRST_DISABLE,
+ .data_type = MHI_ER_CTRL,
+ .hardware_event = false,
+ .client_managed = false,
+ .offload_channel = false,
+ },
+};
+
It'd be nice to use macros for defining the channels and events as done in the
pci_generic driver.
I think the pci_generic driver has a usecase for using a macro in that it is
servicing multiple devices, with different configuration. Right now, we
only have the one device with the one config. I suspect that will change in
the future, but I don't have concrete information at the time to inform a
proper design.
I feel this should be left until such time the multi-device scenario becomes
realized.
Ok, this sounds reasonable to me.
+static struct mhi_controller_config aic100_config = {
+ .max_channels = 128,
+ .timeout_ms = 0, /* controlled by mhi_timeout */
+ .buf_len = 0,
+ .num_channels = ARRAY_SIZE(aic100_channels),
+ .ch_cfg = aic100_channels,
+ .num_events = ARRAY_SIZE(aic100_events),
+ .event_cfg = aic100_events,
+ .use_bounce_buf = false,
+ .m2_no_db = false,
+};
+
+static int mhi_read_reg(struct mhi_controller *mhi_cntl, void __iomem *addr, u32 *out)
+{
+ u32 tmp = readl_relaxed(addr);
+
+ if (tmp == U32_MAX)
+ return -EIO;
+
+ *out = tmp;
+
+ return 0;
+}
+
+static void mhi_write_reg(struct mhi_controller *mhi_cntl, void __iomem *addr, u32 val)
+{
+ writel_relaxed(val, addr);
+}
+
+static int mhi_runtime_get(struct mhi_controller *mhi_cntl)
+{
+ return 0;
+}
+
+static void mhi_runtime_put(struct mhi_controller *mhi_cntl)
+{
+}
+
+static void mhi_status_cb(struct mhi_controller *mhi_cntl, enum mhi_callback reason)
+{
+ struct qaic_device *qdev = pci_get_drvdata(to_pci_dev(mhi_cntl->cntrl_dev));
+
+ /* this event occurs in atomic context */
+ if (reason == MHI_CB_FATAL_ERROR)
+ pci_err(qdev->pdev, "Fatal error received from device. Attempting to recover\n");
Why no dev_err()?
pci_err is more specific than dev_err. It is built upon dev_err. pci_err
seems to be preferred for pci devices, and also matches uses elsewhere in
the driver.
Ok.
+ /* this event occurs in non-atomic context */
+ if (reason == MHI_CB_SYS_ERROR)
+ qaic_dev_reset_clean_local_state(qdev, true);
+}
+
+static int mhi_reset_and_async_power_up(struct mhi_controller *mhi_cntl)
+{
+ char time_sec = 1;
u8?
Eh. Ok I guess. I usually reserve the size specific types for things where
that size is required, such as sending data over a network.
+ int current_ee;
+ int ret;
+
+ /* Reset the device to bring the device in PBL EE */
+ mhi_soc_reset(mhi_cntl);
+
+ /*
+ * Keep checking the execution environment(EE) after every 1 second
+ * interval.
+ */
+ do {
+ msleep(1000);
+ current_ee = mhi_get_exec_env(mhi_cntl);
+ } while (current_ee != MHI_EE_PBL && time_sec++ <= MAX_RESET_TIME_SEC);
+
+ /* If the device is in PBL EE retry power up */
+ if (current_ee == MHI_EE_PBL)
+ ret = mhi_async_power_up(mhi_cntl);
+ else
+ ret = -EIO;
+
+ return ret;
+}
+
+struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, void __iomem *mhi_bar,
+ int mhi_irq)
+{
+ struct mhi_controller *mhi_cntl;
Cosmetic change: We use "mhi_cntrl" in other controller drivers. So it is
better to follow the same pattern here also.
If you insist. "cntl" is the more common abbreviation. The MHI bus is the
first place I recall seeing "cntrl".
For some reason, all MHI controller drivers have picked up this notation. So
I'd like to keep it same.
