Re: [PATCH v1 06/12] drivers: soc: qcom: Add support for Qualcomm QDU device.

Kishore Batta <kishore.batta@xxxxxxxxxxxxxxxx> · Thu, 28 Aug 2025 18:18:08 +0530

On 8/26/2025 4:54 AM, Bjorn Andersson wrote:
On Mon, Aug 25, 2025 at 03:49:20PM +0530, Kishore Batta wrote:
Add support for the Qualcomm QDU device. The Qualcomm QDU driver acts
as a client driver to the Sahara protocol, including the QDU100-specific
image table and registering it during device initialization.
"including the QDU100-specific image table"? That's not "including" it's
the only thing it does.

The
registration of the image table is required to transfer the QDU100
specific firmware back to the device using Sahara protocol packets.
Compare above 5-6 lines with:

"The QDU device is flash-less and uses Sahara to load its firmware,
provide the table of firmware files."

The
MHI driver exposes a new channel name for the Qualcomm QDU100 device in
the pci_generic.c file, and the same channel support is added in the
Sahara driver.
Which MHI driver exposes a new channel name? Please rephrase this and
perhaps split it into a separate patch?

Signed-off-by: Kishore Batta <kishore.batta@xxxxxxxxxxxxxxxx>
---
  drivers/soc/qcom/Kconfig         | 14 ++++++
  drivers/soc/qcom/Makefile        |  1 +
  drivers/soc/qcom/qdu.c           | 85 ++++++++++++++++++++++++++++++++
  drivers/soc/qcom/sahara/sahara.c |  1 +
  4 files changed, 101 insertions(+)
  create mode 100644 drivers/soc/qcom/qdu.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 7ea4cff9a679..ffaaf6261c35 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -303,6 +303,20 @@ config QCOM_UBWC_CONFIG
  	  with coherent configuration data. This module functions as a single
  	  source of truth for that information.
  
+config QCOM_QDU
+	tristate "Qualcomm QDU driver"
+	select QCOM_SAHARA_PROTOCOL
+	help
+	  This is a client driver which registers the device specific operations
+	  with sahara protocol which is used to download firmware to Qualcomm
+	  distributed unit device.
+	  The Qualcomm QDU driver facilitates the registration of device
+	  specific operations with the sahara protocol, enabling firmware
+	  download to the QDU device.
This makes it sound like there's a bunch going on in this driver, is
more of this coming?

+
+	  To compile this driver as a module, choose M here: the module will be
+	  called qdu.
+
  source "drivers/soc/qcom/sahara/Kconfig"
  
  endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 99e490e3174e..8d036edf304a 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -41,3 +41,4 @@ obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)	+= qcom_ice.o
  obj-$(CONFIG_QCOM_PBS) +=	qcom-pbs.o
  obj-$(CONFIG_QCOM_UBWC_CONFIG) += ubwc_config.o
  obj-$(CONFIG_QCOM_SAHARA_PROTOCOL) += sahara/
+obj-$(CONFIG_QCOM_QDU) += qdu.o
This list was somewhat an approximation of being in alphabetical
order...

diff --git a/drivers/soc/qcom/qdu.c b/drivers/soc/qcom/qdu.c
new file mode 100644
index 000000000000..afd8011793fa
--- /dev/null
+++ b/drivers/soc/qcom/qdu.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
Look at other drivers in drivers/soc/qcom...they don't format this
comment like this.

+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/sahara_image_table_ops.h>
+
+static const char * const qdu100_image_table[] = {
+	[5] = "qcom/qdu100/uefi.elf",
+	[8] = "qcom/qdu100/qdsp6sw.mbn",
+	[16] = "qcom/qdu100/efs1.bin",
+	[17] = "qcom/qdu100/efs2.bin",
+	[20] = "qcom/qdu100/efs3.bin",
+	[23] = "qcom/qdu100/aop.mbn",
+	[25] = "qcom/qdu100/tz.mbn",
+	[29] = "qcom/qdu100/zeros_1sector.bin",
+	[33] = "qcom/qdu100/hypvm.mbn",
+	[34] = "qcom/qdu100/mdmddr.mbn",
+	[36] = "qcom/qdu100/multi_image_qti.mbn",
+	[37] = "qcom/qdu100/multi_image.mbn",
+	[38] = "qcom/qdu100/xbl_config.elf",
+	[39] = "qcom/qdu100/abl_userdebug.elf",
+	[40] = "qcom/qdu100/zeros_1sector.bin",
+	[41] = "qcom/qdu100/devcfg.mbn",
+	[42] = "qcom/qdu100/zeros_1sector.bin",
+	[43] = "qcom/qdu100/kernel_boot.elf",
+	[45] = "qcom/qdu100/tools_l.elf",
+	[46] = "qcom/qdu100/Quantum.elf",
+	[47] = "qcom/qdu100/quest.elf",
+	[48] = "qcom/qdu100/xbl_ramdump.elf",
+	[49] = "qcom/qdu100/shrm.elf",
+	[50] = "qcom/qdu100/cpucp.elf",
+	[51] = "qcom/qdu100/aop_devcfg.mbn",
+	[52] = "qcom/qdu100/fw_csm_gsi_3.0.elf",
+	[53] = "qcom/qdu100/qdsp6sw_dtbs.elf",
+	[54] = "qcom/qdu100/qupv3fw.elf",
+};
+
+static struct sahara_image_table_provider qdu100_provider = {
+	.image_table = qdu100_image_table,
+	.image_table_size = ARRAY_SIZE(qdu100_image_table),
+	.dev_name = "qcom-qdu100",
+	.fw_folder_name = "qdu100",
+	.list = LIST_HEAD_INIT(qdu100_provider.list)
+};
+
+static struct sahara_image_table_provider *qdu_providers[] = {
+	&qdu100_provider,
+};
+
+static int __init qdu_init(void)
So patch 2, 3, and 4 just laid the groundwork, so that you could add a
new "driver" with the only purpose of carrying an array of firmware
names and the extra code to register it with the Sahara driver.

Why don't you just put the qdu100_image_table[] array in sahara.c and
save us a whole bunch of changes?

Hi Bjorn,

I thought that embedding the QDU100 image table directly into sahara driver
would risk coupling it too tightly to a specific device, which goes against
the goal of keeping Sahara generic and protocol focused.

To address this, I have restructured the patch to introduce minimal MHI 
client driver(drivers/bus/mhi/qdu.c)
that solely registers the QDU100 image table using sahara registration 
APIs introduced earlier
in the series. This keeps sahara clean and device agnostic while still 
enabling firmware loading
for QDU and AIC devices.

The new driver is conditionally compiled using CONFIG_MHI_QDU and it 
follows the same pattern as AIC module
firmware registration mechanism.

Please let me know if this direction looks good to you, and I'll include 
the updated patch in the next version.


Thanks,

- Kishore.


+{
+	int ret;
+
+	for (int i = 0; i < ARRAY_SIZE(qdu_providers); i++) {
Future proof! Please don't.

Regards,
Bjorn

+		ret = sahara_register_image_table_provider(qdu_providers[i]);
+		if (ret) {
+			pr_err("qdu: Failed to register image table %d\n", ret);
+
+			/* Rollback previously registered providers */
+			while (--i >= 0)
+				sahara_unregister_image_table_provider(qdu_providers[i]);
+			return ret;
+		}
+	}
+	return 0;
+}
+module_init(qdu_init);
+
+static void __exit qdu_exit(void)
+{
+	int ret;
+
+	for (int i = 0; i < ARRAY_SIZE(qdu_providers); i++) {
+		ret = sahara_unregister_image_table_provider(qdu_providers[i]);
+		if (ret)
+			pr_err("qdu: Failed to unregister image table %d\n", ret);
+	}
+}
+module_exit(qdu_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Qualcomm distributed unit driver");
diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
index 5e17d71a2d34..b07f6bd0e19f 100644
--- a/drivers/soc/qcom/sahara/sahara.c
+++ b/drivers/soc/qcom/sahara/sahara.c
@@ -795,6 +795,7 @@ static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
  
  static const struct mhi_device_id sahara_mhi_match_table[] = {
  	{ .chan = "QAIC_SAHARA", },
+	{ .chan = "SAHARA", },
  	{},
  };
  
--
2.34.1








