[PATCH 0/7] accel/qaic: Initial AIC200 support

[PATCH 0/7] accel/qaic: Initial AIC200 support

[Jeffrey Hugo]

Initial support to the driver to boot up AIC200. AIC200 uses BHIe
without BHI, which is something that the MHI bus has not supported until
now. While the MHI changes are listed first to facilitate cross-tree
merging, they are not needed until the last change in the series.

Also, AIC200 is a different product from AIC100 with MSI-X, different
BARs, and different MHI configuration so we finally need some
infrastructure in the driver to be able to handle product differences.
This is expected to evolve more over time.

Jeffrey Hugo (2):
  accel/qaic: Add config structs for supported cards
  accel/qaic: Add AIC200 support

Matthew Leung (2):
  bus: mhi: host: Refactor BHI/BHIe based firmware loading
  bus: mhi: host: Add a policy to enable image transfer via BHIe in PBL

Youssef Samir (3):
  accel/qaic: Allocate an exact number of MSIs
  accel/qaic: Add support for MSI-X
  accel/qaic: Mask out SR-IOV PCI resources

 drivers/accel/qaic/mhi_controller.c | 360 ++++++++++++++++++++++++++--
 drivers/accel/qaic/mhi_controller.h |   2 +-
 drivers/accel/qaic/qaic.h           |  14 +-
 drivers/accel/qaic/qaic_drv.c       |  97 +++++---
 drivers/accel/qaic/qaic_timesync.c  |   2 +-
 drivers/accel/qaic/sahara.c         |  39 ++-
 drivers/bus/mhi/host/boot.c         | 223 +++++++++++++----
 drivers/bus/mhi/host/init.c         |   2 +-
 drivers/bus/mhi/host/internal.h     |   8 +
 9 files changed, 641 insertions(+), 106 deletions(-)

-- 
2.34.1

[PATCH 1/7] bus: mhi: host: Refactor BHI/BHIe based firmware loading

[Jeffrey Hugo]

From: Matthew Leung <quic_mattleun@quicinc.com>

Refactor the firmware loading code to have distinct helper functions for
BHI and BHIe operations. This lays the foundation for separating the
firmware loading protocol from the firmware being loaded and the EE it
is loaded in.

Signed-off-by: Matthew Leung <quic_mattleun@quicinc.com>
Reviewed-by: Youssef Samir <quic_yabdulra@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 drivers/bus/mhi/host/boot.c | 155 +++++++++++++++++++++++++-----------
 1 file changed, 110 insertions(+), 45 deletions(-)

diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
index e8c92972f9df..e3f3c07166ad 100644
--- a/drivers/bus/mhi/host/boot.c
+++ b/drivers/bus/mhi/host/boot.c
@@ -177,6 +177,37 @@ int mhi_download_rddm_image(struct mhi_controller *mhi_cntrl, bool in_panic)
 }
 EXPORT_SYMBOL_GPL(mhi_download_rddm_image);
 
+static inline void mhi_fw_load_error_dump(struct mhi_controller *mhi_cntrl)
+{
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
+	rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
+	void __iomem *base = mhi_cntrl->bhi;
+	int ret;
+	u32 val;
+	int i;
+	struct {
+		char *name;
+		u32 offset;
+	} error_reg[] = {
+		{ "ERROR_CODE", BHI_ERRCODE },
+		{ "ERROR_DBG1", BHI_ERRDBG1 },
+		{ "ERROR_DBG2", BHI_ERRDBG2 },
+		{ "ERROR_DBG3", BHI_ERRDBG3 },
+		{ NULL },
+	};
+
+	read_lock_bh(pm_lock);
+	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
+		for (i = 0; error_reg[i].name; i++) {
+			ret = mhi_read_reg(mhi_cntrl, base, error_reg[i].offset, &val);
+			if (ret)
+				break;
+			dev_err(dev, "Reg: %s value: 0x%x\n", error_reg[i].name, val);
+		}
+	}
+	read_unlock_bh(pm_lock);
+}
+
 static int mhi_fw_load_bhie(struct mhi_controller *mhi_cntrl,
 			    const struct mhi_buf *mhi_buf)
 {
@@ -226,24 +257,13 @@ static int mhi_fw_load_bhie(struct mhi_controller *mhi_cntrl,
 }
 
 static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
-			   dma_addr_t dma_addr,
-			   size_t size)
+			    const struct mhi_buf *mhi_buf)
 {
-	u32 tx_status, val, session_id;
-	int i, ret;
-	void __iomem *base = mhi_cntrl->bhi;
-	rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
-	struct {
-		char *name;
-		u32 offset;
-	} error_reg[] = {
-		{ "ERROR_CODE", BHI_ERRCODE },
-		{ "ERROR_DBG1", BHI_ERRDBG1 },
-		{ "ERROR_DBG2", BHI_ERRDBG2 },
-		{ "ERROR_DBG3", BHI_ERRDBG3 },
-		{ NULL },
-	};
+	rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
+	void __iomem *base = mhi_cntrl->bhi;
+	u32 tx_status, session_id;
+	int ret;
 
 	read_lock_bh(pm_lock);
 	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
@@ -255,11 +275,9 @@ static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
 	dev_dbg(dev, "Starting image download via BHI. Session ID: %u\n",
 		session_id);
 	mhi_write_reg(mhi_cntrl, base, BHI_STATUS, 0);
-	mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_HIGH,
-		      upper_32_bits(dma_addr));
-	mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_LOW,
-		      lower_32_bits(dma_addr));
-	mhi_write_reg(mhi_cntrl, base, BHI_IMGSIZE, size);
+	mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_HIGH, upper_32_bits(mhi_buf->dma_addr));
+	mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_LOW, lower_32_bits(mhi_buf->dma_addr));
+	mhi_write_reg(mhi_cntrl, base, BHI_IMGSIZE, mhi_buf->len);
 	mhi_write_reg(mhi_cntrl, base, BHI_IMGTXDB, session_id);
 	read_unlock_bh(pm_lock);
 
@@ -274,18 +292,7 @@ static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
 
 	if (tx_status == BHI_STATUS_ERROR) {
 		dev_err(dev, "Image transfer failed\n");
-		read_lock_bh(pm_lock);
-		if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
-			for (i = 0; error_reg[i].name; i++) {
-				ret = mhi_read_reg(mhi_cntrl, base,
-						   error_reg[i].offset, &val);
-				if (ret)
-					break;
-				dev_err(dev, "Reg: %s value: 0x%x\n",
-					error_reg[i].name, val);
-			}
-		}
-		read_unlock_bh(pm_lock);
+		mhi_fw_load_error_dump(mhi_cntrl);
 		goto invalid_pm_state;
 	}
 
@@ -296,6 +303,16 @@ static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
 	return -EIO;
 }
 
+static void mhi_free_bhi_buffer(struct mhi_controller *mhi_cntrl,
+				struct image_info *image_info)
+{
+	struct mhi_buf *mhi_buf = image_info->mhi_buf;
+
+	dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len, mhi_buf->buf, mhi_buf->dma_addr);
+	kfree(image_info->mhi_buf);
+	kfree(image_info);
+}
+
 void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl,
 			 struct image_info *image_info)
 {
@@ -310,6 +327,47 @@ void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl,
 	kfree(image_info);
 }
 
+static int mhi_alloc_bhi_buffer(struct mhi_controller *mhi_cntrl,
+				struct image_info **image_info,
+				size_t alloc_size)
+{
+	struct image_info *img_info;
+	struct mhi_buf *mhi_buf;
+	int segments = 1;
+
+	img_info = kzalloc(sizeof(*img_info), GFP_KERNEL);
+	if (!img_info)
+		return -ENOMEM;
+
+	/* Allocate memory for entry */
+	img_info->mhi_buf = kcalloc(segments, sizeof(*img_info->mhi_buf),
+				    GFP_KERNEL);
+	if (!img_info->mhi_buf)
+		goto error_alloc_mhi_buf;
+
+	/* Allocate and populate vector table */
+	mhi_buf = img_info->mhi_buf;
+
+	mhi_buf->len = alloc_size;
+	mhi_buf->buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len,
+					  &mhi_buf->dma_addr, GFP_KERNEL);
+	if (!mhi_buf->buf)
+		goto error_alloc_segment;
+
+	img_info->bhi_vec = NULL;
+	img_info->entries = segments;
+	*image_info = img_info;
+
+	return 0;
+
+error_alloc_segment:
+	kfree(mhi_buf);
+error_alloc_mhi_buf:
+	kfree(img_info);
+
+	return -ENOMEM;
+}
+
 int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
 			 struct image_info **image_info,
 			 size_t alloc_size)
@@ -364,9 +422,18 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
 	return -ENOMEM;
 }
 
-static void mhi_firmware_copy(struct mhi_controller *mhi_cntrl,
-			      const u8 *buf, size_t remainder,
-			      struct image_info *img_info)
+static void mhi_firmware_copy_bhi(struct mhi_controller *mhi_cntrl,
+				  const u8 *buf, size_t size,
+				  struct image_info *img_info)
+{
+	struct mhi_buf *mhi_buf = img_info->mhi_buf;
+
+	memcpy(mhi_buf->buf, buf, size);
+}
+
+static void mhi_firmware_copy_bhie(struct mhi_controller *mhi_cntrl,
+				   const u8 *buf, size_t remainder,
+				   struct image_info *img_info)
 {
 	size_t to_cpy;
 	struct mhi_buf *mhi_buf = img_info->mhi_buf;
@@ -390,10 +457,9 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	const struct firmware *firmware = NULL;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 	enum mhi_pm_state new_state;
+	struct image_info *image;
 	const char *fw_name;
 	const u8 *fw_data;
-	void *buf;
-	dma_addr_t dma_addr;
 	size_t size, fw_sz;
 	int ret;
 
@@ -452,17 +518,16 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	fw_sz = firmware->size;
 
 skip_req_fw:
-	buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, size, &dma_addr,
-				 GFP_KERNEL);
-	if (!buf) {
+	ret = mhi_alloc_bhi_buffer(mhi_cntrl, &image, size);
+	if (ret) {
 		release_firmware(firmware);
 		goto error_fw_load;
 	}
+	mhi_firmware_copy_bhi(mhi_cntrl, fw_data, size, image);
 
 	/* Download image using BHI */
-	memcpy(buf, fw_data, size);
-	ret = mhi_fw_load_bhi(mhi_cntrl, dma_addr, size);
-	dma_free_coherent(mhi_cntrl->cntrl_dev, size, buf, dma_addr);
+	ret = mhi_fw_load_bhi(mhi_cntrl, image->mhi_buf);
+	mhi_free_bhi_buffer(mhi_cntrl, image);
 
 	/* Error or in EDL mode, we're done */
 	if (ret) {
@@ -493,7 +558,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 		}
 
 		/* Load the firmware into BHIE vec table */
-		mhi_firmware_copy(mhi_cntrl, fw_data, fw_sz, mhi_cntrl->fbc_image);
+		mhi_firmware_copy_bhie(mhi_cntrl, fw_data, fw_sz, mhi_cntrl->fbc_image);
 	}
 
 	release_firmware(firmware);
-- 
2.34.1

Re: [PATCH 1/7] bus: mhi: host: Refactor BHI/BHIe based firmware loading

[Jacek Lawrynowicz]

Hi,

On 12/13/2024 10:33 PM, Jeffrey Hugo wrote:
> From: Matthew Leung <quic_mattleun@quicinc.com>
> 
> Refactor the firmware loading code to have distinct helper functions for
> BHI and BHIe operations. This lays the foundation for separating the
> firmware loading protocol from the firmware being loaded and the EE it
> is loaded in.
> 
> Signed-off-by: Matthew Leung <quic_mattleun@quicinc.com>
> Reviewed-by: Youssef Samir <quic_yabdulra@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>  drivers/bus/mhi/host/boot.c | 155 +++++++++++++++++++++++++-----------
>  1 file changed, 110 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
> index e8c92972f9df..e3f3c07166ad 100644
> --- a/drivers/bus/mhi/host/boot.c
> +++ b/drivers/bus/mhi/host/boot.c
> @@ -177,6 +177,37 @@ int mhi_download_rddm_image(struct mhi_controller *mhi_cntrl, bool in_panic)
>  }
>  EXPORT_SYMBOL_GPL(mhi_download_rddm_image);
>  
> +static inline void mhi_fw_load_error_dump(struct mhi_controller *mhi_cntrl)
> +{
> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> +	rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
> +	void __iomem *base = mhi_cntrl->bhi;
> +	int ret;
> +	u32 val;
> +	int i;
> +	struct {
> +		char *name;
> +		u32 offset;
> +	} error_reg[] = {
> +		{ "ERROR_CODE", BHI_ERRCODE },
> +		{ "ERROR_DBG1", BHI_ERRDBG1 },
> +		{ "ERROR_DBG2", BHI_ERRDBG2 },
> +		{ "ERROR_DBG3", BHI_ERRDBG3 },
> +		{ NULL },
> +	};
> +
> +	read_lock_bh(pm_lock);
> +	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> +		for (i = 0; error_reg[i].name; i++) {
> +			ret = mhi_read_reg(mhi_cntrl, base, error_reg[i].offset, &val);
> +			if (ret)
> +				break;
> +			dev_err(dev, "Reg: %s value: 0x%x\n", error_reg[i].name, val);
> +		}
> +	}
> +	read_unlock_bh(pm_lock);
> +}
> +
>  static int mhi_fw_load_bhie(struct mhi_controller *mhi_cntrl,
>  			    const struct mhi_buf *mhi_buf)
>  {
> @@ -226,24 +257,13 @@ static int mhi_fw_load_bhie(struct mhi_controller *mhi_cntrl,
>  }
>  
>  static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
> -			   dma_addr_t dma_addr,
> -			   size_t size)
> +			    const struct mhi_buf *mhi_buf)
>  {
> -	u32 tx_status, val, session_id;
> -	int i, ret;
> -	void __iomem *base = mhi_cntrl->bhi;
> -	rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> -	struct {
> -		char *name;
> -		u32 offset;
> -	} error_reg[] = {
> -		{ "ERROR_CODE", BHI_ERRCODE },
> -		{ "ERROR_DBG1", BHI_ERRDBG1 },
> -		{ "ERROR_DBG2", BHI_ERRDBG2 },
> -		{ "ERROR_DBG3", BHI_ERRDBG3 },
> -		{ NULL },
> -	};
> +	rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
> +	void __iomem *base = mhi_cntrl->bhi;
> +	u32 tx_status, session_id;
> +	int ret;
>  
>  	read_lock_bh(pm_lock);
>  	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> @@ -255,11 +275,9 @@ static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
>  	dev_dbg(dev, "Starting image download via BHI. Session ID: %u\n",
>  		session_id);
>  	mhi_write_reg(mhi_cntrl, base, BHI_STATUS, 0);
> -	mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_HIGH,
> -		      upper_32_bits(dma_addr));
> -	mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_LOW,
> -		      lower_32_bits(dma_addr));
> -	mhi_write_reg(mhi_cntrl, base, BHI_IMGSIZE, size);
> +	mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_HIGH, upper_32_bits(mhi_buf->dma_addr));
> +	mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_LOW, lower_32_bits(mhi_buf->dma_addr));
> +	mhi_write_reg(mhi_cntrl, base, BHI_IMGSIZE, mhi_buf->len);
>  	mhi_write_reg(mhi_cntrl, base, BHI_IMGTXDB, session_id);
>  	read_unlock_bh(pm_lock);
>  
> @@ -274,18 +292,7 @@ static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
>  
>  	if (tx_status == BHI_STATUS_ERROR) {
>  		dev_err(dev, "Image transfer failed\n");
> -		read_lock_bh(pm_lock);
> -		if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> -			for (i = 0; error_reg[i].name; i++) {
> -				ret = mhi_read_reg(mhi_cntrl, base,
> -						   error_reg[i].offset, &val);
> -				if (ret)
> -					break;
> -				dev_err(dev, "Reg: %s value: 0x%x\n",
> -					error_reg[i].name, val);
> -			}
> -		}
> -		read_unlock_bh(pm_lock);
> +		mhi_fw_load_error_dump(mhi_cntrl);
>  		goto invalid_pm_state;
>  	}
>  
> @@ -296,6 +303,16 @@ static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
>  	return -EIO;
>  }
>  
> +static void mhi_free_bhi_buffer(struct mhi_controller *mhi_cntrl,
> +				struct image_info *image_info)
> +{
> +	struct mhi_buf *mhi_buf = image_info->mhi_buf;
> +
> +	dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len, mhi_buf->buf, mhi_buf->dma_addr);
> +	kfree(image_info->mhi_buf);
> +	kfree(image_info);
> +}
> +
>  void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl,
>  			 struct image_info *image_info)
>  {
> @@ -310,6 +327,47 @@ void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl,
>  	kfree(image_info);
>  }
>  
> +static int mhi_alloc_bhi_buffer(struct mhi_controller *mhi_cntrl,
> +				struct image_info **image_info,
> +				size_t alloc_size)
> +{
> +	struct image_info *img_info;
> +	struct mhi_buf *mhi_buf;
> +	int segments = 1;Are you planning for variable segment count in future?

> +	img_info = kzalloc(sizeof(*img_info), GFP_KERNEL);
> +	if (!img_info)
> +		return -ENOMEM;
> +
> +	/* Allocate memory for entry */
> +	img_info->mhi_buf = kcalloc(segments, sizeof(*img_info->mhi_buf),
> +				    GFP_KERNEL);
> +	if (!img_info->mhi_buf)
> +		goto error_alloc_mhi_buf;
> +
> +	/* Allocate and populate vector table */
> +	mhi_buf = img_info->mhi_buf;
> +
> +	mhi_buf->len = alloc_size;
> +	mhi_buf->buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len,
> +					  &mhi_buf->dma_addr, GFP_KERNEL);
> +	if (!mhi_buf->buf)
> +		goto error_alloc_segment;
> +
> +	img_info->bhi_vec = NULL;
> +	img_info->entries = segments;
> +	*image_info = img_info;
> +
> +	return 0;
> +
> +error_alloc_segment:
> +	kfree(mhi_buf);
> +error_alloc_mhi_buf:
> +	kfree(img_info);
> +
> +	return -ENOMEM;
> +}
> +
>  int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>  			 struct image_info **image_info,
>  			 size_t alloc_size)
> @@ -364,9 +422,18 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>  	return -ENOMEM;
>  }
>  
> -static void mhi_firmware_copy(struct mhi_controller *mhi_cntrl,
> -			      const u8 *buf, size_t remainder,
> -			      struct image_info *img_info)
> +static void mhi_firmware_copy_bhi(struct mhi_controller *mhi_cntrl,
> +				  const u8 *buf, size_t size,
> +				  struct image_info *img_info)
> +{
> +	struct mhi_buf *mhi_buf = img_info->mhi_buf;
> +
> +	memcpy(mhi_buf->buf, buf, size);
> +}
I'm not sure this function improves readablity.

> +static void mhi_firmware_copy_bhie(struct mhi_controller *mhi_cntrl,
> +				   const u8 *buf, size_t remainder,
> +				   struct image_info *img_info)
>  {
>  	size_t to_cpy;
>  	struct mhi_buf *mhi_buf = img_info->mhi_buf;
> @@ -390,10 +457,9 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	const struct firmware *firmware = NULL;
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  	enum mhi_pm_state new_state;
> +	struct image_info *image;
>  	const char *fw_name;
>  	const u8 *fw_data;
> -	void *buf;
> -	dma_addr_t dma_addr;
>  	size_t size, fw_sz;
>  	int ret;
>  
> @@ -452,17 +518,16 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	fw_sz = firmware->size;
>  
>  skip_req_fw:
> -	buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, size, &dma_addr,
> -				 GFP_KERNEL);
> -	if (!buf) {
> +	ret = mhi_alloc_bhi_buffer(mhi_cntrl, &image, size);
> +	if (ret) {
>  		release_firmware(firmware);
>  		goto error_fw_load;
>  	}
> +	mhi_firmware_copy_bhi(mhi_cntrl, fw_data, size, image);
>  
>  	/* Download image using BHI */
> -	memcpy(buf, fw_data, size);
> -	ret = mhi_fw_load_bhi(mhi_cntrl, dma_addr, size);
> -	dma_free_coherent(mhi_cntrl->cntrl_dev, size, buf, dma_addr);
> +	ret = mhi_fw_load_bhi(mhi_cntrl, image->mhi_buf);
> +	mhi_free_bhi_buffer(mhi_cntrl, image);
>  
>  	/* Error or in EDL mode, we're done */
>  	if (ret) {
> @@ -493,7 +558,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  		}
>  
>  		/* Load the firmware into BHIE vec table */
> -		mhi_firmware_copy(mhi_cntrl, fw_data, fw_sz, mhi_cntrl->fbc_image);
> +		mhi_firmware_copy_bhie(mhi_cntrl, fw_data, fw_sz, mhi_cntrl->fbc_image);
>  	}
>  
>  	release_firmware(firmware);

Regards,
Jacek

Re: [PATCH 1/7] bus: mhi: host: Refactor BHI/BHIe based firmware loading

[Manivannan Sadhasivam]

On Fri, Dec 13, 2024 at 02:33:34PM -0700, Jeffrey Hugo wrote:
> From: Matthew Leung <quic_mattleun@quicinc.com>
> 
> Refactor the firmware loading code to have distinct helper functions for
> BHI and BHIe operations. This lays the foundation for separating the
> firmware loading protocol from the firmware being loaded and the EE it
> is loaded in.
> 
> Signed-off-by: Matthew Leung <quic_mattleun@quicinc.com>
> Reviewed-by: Youssef Samir <quic_yabdulra@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>  drivers/bus/mhi/host/boot.c | 155 +++++++++++++++++++++++++-----------
>  1 file changed, 110 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
> index e8c92972f9df..e3f3c07166ad 100644
> --- a/drivers/bus/mhi/host/boot.c
> +++ b/drivers/bus/mhi/host/boot.c
> @@ -177,6 +177,37 @@ int mhi_download_rddm_image(struct mhi_controller *mhi_cntrl, bool in_panic)
>  }
>  EXPORT_SYMBOL_GPL(mhi_download_rddm_image);
>  
> +static inline void mhi_fw_load_error_dump(struct mhi_controller *mhi_cntrl)

No need to add 'inline' keyword in c files. You can trust the compiler.

> +{
> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> +	rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
> +	void __iomem *base = mhi_cntrl->bhi;
> +	int ret;
> +	u32 val;
> +	int i;

int ret, i?

> +	struct {
> +		char *name;
> +		u32 offset;
> +	} error_reg[] = {
> +		{ "ERROR_CODE", BHI_ERRCODE },
> +		{ "ERROR_DBG1", BHI_ERRDBG1 },
> +		{ "ERROR_DBG2", BHI_ERRDBG2 },
> +		{ "ERROR_DBG3", BHI_ERRDBG3 },
> +		{ NULL },
> +	};
> +
> +	read_lock_bh(pm_lock);
> +	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> +		for (i = 0; error_reg[i].name; i++) {
> +			ret = mhi_read_reg(mhi_cntrl, base, error_reg[i].offset, &val);
> +			if (ret)
> +				break;
> +			dev_err(dev, "Reg: %s value: 0x%x\n", error_reg[i].name, val);
> +		}
> +	}
> +	read_unlock_bh(pm_lock);
> +}
> +

[...]

> +static int mhi_alloc_bhi_buffer(struct mhi_controller *mhi_cntrl,
> +				struct image_info **image_info,
> +				size_t alloc_size)
> +{
> +	struct image_info *img_info;
> +	struct mhi_buf *mhi_buf;
> +	int segments = 1;
> +
> +	img_info = kzalloc(sizeof(*img_info), GFP_KERNEL);
> +	if (!img_info)
> +		return -ENOMEM;
> +
> +	/* Allocate memory for entry */
> +	img_info->mhi_buf = kcalloc(segments, sizeof(*img_info->mhi_buf),
> +				    GFP_KERNEL);

Why do you need kcalloc for only 1 segment?

> +	if (!img_info->mhi_buf)
> +		goto error_alloc_mhi_buf;
> +
> +	/* Allocate and populate vector table */
> +	mhi_buf = img_info->mhi_buf;
> +
> +	mhi_buf->len = alloc_size;
> +	mhi_buf->buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len,
> +					  &mhi_buf->dma_addr, GFP_KERNEL);
> +	if (!mhi_buf->buf)
> +		goto error_alloc_segment;
> +
> +	img_info->bhi_vec = NULL;
> +	img_info->entries = segments;
> +	*image_info = img_info;
> +
> +	return 0;
> +
> +error_alloc_segment:
> +	kfree(mhi_buf);
> +error_alloc_mhi_buf:
> +	kfree(img_info);
> +
> +	return -ENOMEM;
> +}
> +
>  int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>  			 struct image_info **image_info,
>  			 size_t alloc_size)
> @@ -364,9 +422,18 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>  	return -ENOMEM;
>  }
>  
> -static void mhi_firmware_copy(struct mhi_controller *mhi_cntrl,
> -			      const u8 *buf, size_t remainder,
> -			      struct image_info *img_info)
> +static void mhi_firmware_copy_bhi(struct mhi_controller *mhi_cntrl,
> +				  const u8 *buf, size_t size,
> +				  struct image_info *img_info)
> +{
> +	struct mhi_buf *mhi_buf = img_info->mhi_buf;
> +
> +	memcpy(mhi_buf->buf, buf, size);
> +}
> +
> +static void mhi_firmware_copy_bhie(struct mhi_controller *mhi_cntrl,
> +				   const u8 *buf, size_t remainder,
> +				   struct image_info *img_info)
>  {
>  	size_t to_cpy;
>  	struct mhi_buf *mhi_buf = img_info->mhi_buf;
> @@ -390,10 +457,9 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	const struct firmware *firmware = NULL;
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  	enum mhi_pm_state new_state;
> +	struct image_info *image;
>  	const char *fw_name;
>  	const u8 *fw_data;
> -	void *buf;
> -	dma_addr_t dma_addr;
>  	size_t size, fw_sz;
>  	int ret;
>  
> @@ -452,17 +518,16 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	fw_sz = firmware->size;
>  
>  skip_req_fw:
> -	buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, size, &dma_addr,
> -				 GFP_KERNEL);
> -	if (!buf) {
> +	ret = mhi_alloc_bhi_buffer(mhi_cntrl, &image, size);
> +	if (ret) {
>  		release_firmware(firmware);
>  		goto error_fw_load;
>  	}
> +	mhi_firmware_copy_bhi(mhi_cntrl, fw_data, size, image);

Why can't you directly use memcpy here? I know what you want to keep symmetry
with mhi_firmware_copy_bhie(), but it seems unnecessary to me.

Adding a comment like "Load the firmware into BHI vec table" is enough.

- Mani


-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/7] bus: mhi: host: Refactor BHI/BHIe based firmware loading

[Jeffrey Hugo]

On 1/7/2025 10:24 PM, Manivannan Sadhasivam wrote:
> On Fri, Dec 13, 2024 at 02:33:34PM -0700, Jeffrey Hugo wrote:
>> From: Matthew Leung <quic_mattleun@quicinc.com>
>>
>> Refactor the firmware loading code to have distinct helper functions for
>> BHI and BHIe operations. This lays the foundation for separating the
>> firmware loading protocol from the firmware being loaded and the EE it
>> is loaded in.
>>
>> Signed-off-by: Matthew Leung <quic_mattleun@quicinc.com>
>> Reviewed-by: Youssef Samir <quic_yabdulra@quicinc.com>
>> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>> ---
>>   drivers/bus/mhi/host/boot.c | 155 +++++++++++++++++++++++++-----------
>>   1 file changed, 110 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
>> index e8c92972f9df..e3f3c07166ad 100644
>> --- a/drivers/bus/mhi/host/boot.c
>> +++ b/drivers/bus/mhi/host/boot.c
>> @@ -177,6 +177,37 @@ int mhi_download_rddm_image(struct mhi_controller *mhi_cntrl, bool in_panic)
>>   }
>>   EXPORT_SYMBOL_GPL(mhi_download_rddm_image);
>>   
>> +static inline void mhi_fw_load_error_dump(struct mhi_controller *mhi_cntrl)
> 
> No need to add 'inline' keyword in c files. You can trust the compiler.

Done.

> 
>> +{
>> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>> +	rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
>> +	void __iomem *base = mhi_cntrl->bhi;
>> +	int ret;
>> +	u32 val;
>> +	int i;
> 
> int ret, i?

Done.

> 
>> +	struct {
>> +		char *name;
>> +		u32 offset;
>> +	} error_reg[] = {
>> +		{ "ERROR_CODE", BHI_ERRCODE },
>> +		{ "ERROR_DBG1", BHI_ERRDBG1 },
>> +		{ "ERROR_DBG2", BHI_ERRDBG2 },
>> +		{ "ERROR_DBG3", BHI_ERRDBG3 },
>> +		{ NULL },
>> +	};
>> +
>> +	read_lock_bh(pm_lock);
>> +	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
>> +		for (i = 0; error_reg[i].name; i++) {
>> +			ret = mhi_read_reg(mhi_cntrl, base, error_reg[i].offset, &val);
>> +			if (ret)
>> +				break;
>> +			dev_err(dev, "Reg: %s value: 0x%x\n", error_reg[i].name, val);
>> +		}
>> +	}
>> +	read_unlock_bh(pm_lock);
>> +}
>> +
> 
> [...]
> 
>> +static int mhi_alloc_bhi_buffer(struct mhi_controller *mhi_cntrl,
>> +				struct image_info **image_info,
>> +				size_t alloc_size)
>> +{
>> +	struct image_info *img_info;
>> +	struct mhi_buf *mhi_buf;
>> +	int segments = 1;
>> +
>> +	img_info = kzalloc(sizeof(*img_info), GFP_KERNEL);
>> +	if (!img_info)
>> +		return -ENOMEM;
>> +
>> +	/* Allocate memory for entry */
>> +	img_info->mhi_buf = kcalloc(segments, sizeof(*img_info->mhi_buf),
>> +				    GFP_KERNEL);
> 
> Why do you need kcalloc for only 1 segment?

Symmetry with mhi_alloc_bhie_table().  Will change.

> 
>> +	if (!img_info->mhi_buf)
>> +		goto error_alloc_mhi_buf;
>> +
>> +	/* Allocate and populate vector table */
>> +	mhi_buf = img_info->mhi_buf;
>> +
>> +	mhi_buf->len = alloc_size;
>> +	mhi_buf->buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len,
>> +					  &mhi_buf->dma_addr, GFP_KERNEL);
>> +	if (!mhi_buf->buf)
>> +		goto error_alloc_segment;
>> +
>> +	img_info->bhi_vec = NULL;
>> +	img_info->entries = segments;
>> +	*image_info = img_info;
>> +
>> +	return 0;
>> +
>> +error_alloc_segment:
>> +	kfree(mhi_buf);
>> +error_alloc_mhi_buf:
>> +	kfree(img_info);
>> +
>> +	return -ENOMEM;
>> +}
>> +
>>   int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>>   			 struct image_info **image_info,
>>   			 size_t alloc_size)
>> @@ -364,9 +422,18 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>>   	return -ENOMEM;
>>   }
>>   
>> -static void mhi_firmware_copy(struct mhi_controller *mhi_cntrl,
>> -			      const u8 *buf, size_t remainder,
>> -			      struct image_info *img_info)
>> +static void mhi_firmware_copy_bhi(struct mhi_controller *mhi_cntrl,
>> +				  const u8 *buf, size_t size,
>> +				  struct image_info *img_info)
>> +{
>> +	struct mhi_buf *mhi_buf = img_info->mhi_buf;
>> +
>> +	memcpy(mhi_buf->buf, buf, size);
>> +}
>> +
>> +static void mhi_firmware_copy_bhie(struct mhi_controller *mhi_cntrl,
>> +				   const u8 *buf, size_t remainder,
>> +				   struct image_info *img_info)
>>   {
>>   	size_t to_cpy;
>>   	struct mhi_buf *mhi_buf = img_info->mhi_buf;
>> @@ -390,10 +457,9 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>>   	const struct firmware *firmware = NULL;
>>   	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>>   	enum mhi_pm_state new_state;
>> +	struct image_info *image;
>>   	const char *fw_name;
>>   	const u8 *fw_data;
>> -	void *buf;
>> -	dma_addr_t dma_addr;
>>   	size_t size, fw_sz;
>>   	int ret;
>>   
>> @@ -452,17 +518,16 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>>   	fw_sz = firmware->size;
>>   
>>   skip_req_fw:
>> -	buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, size, &dma_addr,
>> -				 GFP_KERNEL);
>> -	if (!buf) {
>> +	ret = mhi_alloc_bhi_buffer(mhi_cntrl, &image, size);
>> +	if (ret) {
>>   		release_firmware(firmware);
>>   		goto error_fw_load;
>>   	}
>> +	mhi_firmware_copy_bhi(mhi_cntrl, fw_data, size, image);
> 
> Why can't you directly use memcpy here? I know what you want to keep symmetry
> with mhi_firmware_copy_bhie(), but it seems unnecessary to me.
> 
> Adding a comment like "Load the firmware into BHI vec table" is enough.

Just symmetry.  Jarek had the same comment.  Will inline.

[PATCH 2/7] bus: mhi: host: Add a policy to enable image transfer via BHIe in PBL

[Jeffrey Hugo]

From: Matthew Leung <quic_mattleun@quicinc.com>

Currently, mhi host only performs firmware transfer via BHI in PBL and
BHIe from SBL. To support BHIe transfer directly from PBL, a policy
needs to be added.

With this policy, BHIe will be used to transfer firmware in PBL if the
mhi controller has bhie regs, sets seg_len, and does not set
fbc_download. The intention is to transfer firmware using BHIe in PBL
without further BHIe transfers in SBL.

Signed-off-by: Matthew Leung <quic_mattleun@quicinc.com>
Reviewed-by: Youssef Samir <quic_yabdulra@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 drivers/bus/mhi/host/boot.c     | 80 +++++++++++++++++++++++++++------
 drivers/bus/mhi/host/init.c     |  2 +-
 drivers/bus/mhi/host/internal.h |  8 ++++
 3 files changed, 75 insertions(+), 15 deletions(-)

diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
index e3f3c07166ad..c9ecb6427209 100644
--- a/drivers/bus/mhi/host/boot.c
+++ b/drivers/bus/mhi/host/boot.c
@@ -452,12 +452,62 @@ static void mhi_firmware_copy_bhie(struct mhi_controller *mhi_cntrl,
 	}
 }
 
+static enum mhi_fw_load_type mhi_fw_load_type_get(const struct mhi_controller *mhi_cntrl)
+{
+	enum mhi_fw_load_type ret = MHI_FW_LOAD_UNKNOWN;
+
+	if (mhi_cntrl->fbc_download) {
+		if (mhi_cntrl->bhie && mhi_cntrl->seg_len)
+			ret = MHI_FW_LOAD_FBC;
+	} else {
+		if (mhi_cntrl->bhie && mhi_cntrl->seg_len)
+			ret = MHI_FW_LOAD_BHIE;
+		else
+			ret = MHI_FW_LOAD_BHI;
+	}
+	return ret;
+}
+
+static int mhi_send_image_bhi(struct mhi_controller *mhi_cntrl, const u8 *fw_data, size_t size)
+{
+	struct image_info *image;
+	int ret;
+
+	ret = mhi_alloc_bhi_buffer(mhi_cntrl, &image, size);
+	if (ret)
+		return ret;
+
+	mhi_firmware_copy_bhi(mhi_cntrl, fw_data, size, image);
+
+	ret = mhi_fw_load_bhi(mhi_cntrl, &image->mhi_buf[image->entries - 1]);
+	mhi_free_bhi_buffer(mhi_cntrl, image);
+
+	return ret;
+}
+
+static int mhi_send_image_bhie(struct mhi_controller *mhi_cntrl, const u8 *fw_data, size_t size)
+{
+	struct image_info *image;
+	int ret;
+
+	ret = mhi_alloc_bhie_table(mhi_cntrl, &image, size);
+	if (ret)
+		return ret;
+
+	mhi_firmware_copy_bhie(mhi_cntrl, fw_data, size, image);
+
+	ret = mhi_fw_load_bhie(mhi_cntrl, &image->mhi_buf[image->entries - 1]);
+	mhi_free_bhie_table(mhi_cntrl, image);
+
+	return ret;
+}
+
 void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 {
 	const struct firmware *firmware = NULL;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
+	enum mhi_fw_load_type fw_load_type;
 	enum mhi_pm_state new_state;
-	struct image_info *image;
 	const char *fw_name;
 	const u8 *fw_data;
 	size_t size, fw_sz;
@@ -481,6 +531,12 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	fw_name = (mhi_cntrl->ee == MHI_EE_EDL) ?
 		mhi_cntrl->edl_image : mhi_cntrl->fw_image;
 
+	fw_load_type = mhi_fw_load_type_get(mhi_cntrl);
+	if (fw_load_type == MHI_FW_LOAD_UNKNOWN) {
+		dev_err(dev, "Cannot load FW as load type is UNKNOWN\n");
+		return;
+	}
+
 	/* check if the driver has already provided the firmware data */
 	if (!fw_name && mhi_cntrl->fbc_download &&
 	    mhi_cntrl->fw_data && mhi_cntrl->fw_sz) {
@@ -518,20 +574,16 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	fw_sz = firmware->size;
 
 skip_req_fw:
-	ret = mhi_alloc_bhi_buffer(mhi_cntrl, &image, size);
-	if (ret) {
-		release_firmware(firmware);
-		goto error_fw_load;
-	}
-	mhi_firmware_copy_bhi(mhi_cntrl, fw_data, size, image);
-
-	/* Download image using BHI */
-	ret = mhi_fw_load_bhi(mhi_cntrl, image->mhi_buf);
-	mhi_free_bhi_buffer(mhi_cntrl, image);
+	if (fw_load_type == MHI_FW_LOAD_BHIE)
+		ret = mhi_send_image_bhie(mhi_cntrl, fw_data, size);
+	else
+		ret = mhi_send_image_bhi(mhi_cntrl, fw_data, size);
 
 	/* Error or in EDL mode, we're done */
 	if (ret) {
-		dev_err(dev, "MHI did not load image over BHI, ret: %d\n", ret);
+		dev_err(dev, "MHI did not load image over BHI%s, ret: %d\n",
+			fw_load_type == MHI_FW_LOAD_BHIE ? "e" : "",
+			ret);
 		release_firmware(firmware);
 		goto error_fw_load;
 	}
@@ -550,7 +602,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	 * If we're doing fbc, populate vector tables while
 	 * device transitioning into MHI READY state
 	 */
-	if (mhi_cntrl->fbc_download) {
+	if (fw_load_type == MHI_FW_LOAD_FBC) {
 		ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->fbc_image, fw_sz);
 		if (ret) {
 			release_firmware(firmware);
@@ -575,7 +627,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	return;
 
 error_ready_state:
-	if (mhi_cntrl->fbc_download) {
+	if (fw_load_type == MHI_FW_LOAD_FBC) {
 		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image);
 		mhi_cntrl->fbc_image = NULL;
 	}
diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index a9b1f8beee7b..13e7a55f54ff 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -1144,7 +1144,7 @@ int mhi_prepare_for_power_up(struct mhi_controller *mhi_cntrl)
 	}
 	mhi_cntrl->bhi = mhi_cntrl->regs + bhi_off;
 
-	if (mhi_cntrl->fbc_download || mhi_cntrl->rddm_size) {
+	if (mhi_cntrl->fbc_download || mhi_cntrl->rddm_size || mhi_cntrl->seg_len) {
 		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs, BHIEOFF,
 				   &bhie_off);
 		if (ret) {
diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
index 3134f111be35..afcf536083bc 100644
--- a/drivers/bus/mhi/host/internal.h
+++ b/drivers/bus/mhi/host/internal.h
@@ -29,6 +29,14 @@ struct bhi_vec_entry {
 	u64 size;
 };
 
+enum mhi_fw_load_type {
+	MHI_FW_LOAD_UNKNOWN,
+	MHI_FW_LOAD_BHI,	/* BHI only in PBL */
+	MHI_FW_LOAD_BHIE,	/* BHIe only in PBL */
+	MHI_FW_LOAD_FBC,	/* BHI in PBL followed by BHIe in SBL */
+	MHI_FW_LOAD_MAX,
+};
+
 enum mhi_ch_state_type {
 	MHI_CH_STATE_TYPE_RESET,
 	MHI_CH_STATE_TYPE_STOP,
-- 
2.34.1

Re: [PATCH 2/7] bus: mhi: host: Add a policy to enable image transfer via BHIe in PBL

[Jacek Lawrynowicz]

Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>

On 12/13/2024 10:33 PM, Jeffrey Hugo wrote:
> From: Matthew Leung <quic_mattleun@quicinc.com>
> 
> Currently, mhi host only performs firmware transfer via BHI in PBL and
> BHIe from SBL. To support BHIe transfer directly from PBL, a policy
> needs to be added.
> 
> With this policy, BHIe will be used to transfer firmware in PBL if the
> mhi controller has bhie regs, sets seg_len, and does not set
> fbc_download. The intention is to transfer firmware using BHIe in PBL
> without further BHIe transfers in SBL.
> 
> Signed-off-by: Matthew Leung <quic_mattleun@quicinc.com>
> Reviewed-by: Youssef Samir <quic_yabdulra@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>  drivers/bus/mhi/host/boot.c     | 80 +++++++++++++++++++++++++++------
>  drivers/bus/mhi/host/init.c     |  2 +-
>  drivers/bus/mhi/host/internal.h |  8 ++++
>  3 files changed, 75 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
> index e3f3c07166ad..c9ecb6427209 100644
> --- a/drivers/bus/mhi/host/boot.c
> +++ b/drivers/bus/mhi/host/boot.c
> @@ -452,12 +452,62 @@ static void mhi_firmware_copy_bhie(struct mhi_controller *mhi_cntrl,
>  	}
>  }
>  
> +static enum mhi_fw_load_type mhi_fw_load_type_get(const struct mhi_controller *mhi_cntrl)
> +{
> +	enum mhi_fw_load_type ret = MHI_FW_LOAD_UNKNOWN;
> +
> +	if (mhi_cntrl->fbc_download) {
> +		if (mhi_cntrl->bhie && mhi_cntrl->seg_len)
> +			ret = MHI_FW_LOAD_FBC;
> +	} else {
> +		if (mhi_cntrl->bhie && mhi_cntrl->seg_len)
> +			ret = MHI_FW_LOAD_BHIE;
> +		else
> +			ret = MHI_FW_LOAD_BHI;
> +	}
> +	return ret;
> +}
> +
> +static int mhi_send_image_bhi(struct mhi_controller *mhi_cntrl, const u8 *fw_data, size_t size)
> +{
> +	struct image_info *image;
> +	int ret;
> +
> +	ret = mhi_alloc_bhi_buffer(mhi_cntrl, &image, size);
> +	if (ret)
> +		return ret;
> +
> +	mhi_firmware_copy_bhi(mhi_cntrl, fw_data, size, image);
> +
> +	ret = mhi_fw_load_bhi(mhi_cntrl, &image->mhi_buf[image->entries - 1]);
> +	mhi_free_bhi_buffer(mhi_cntrl, image);
> +
> +	return ret;
> +}
> +
> +static int mhi_send_image_bhie(struct mhi_controller *mhi_cntrl, const u8 *fw_data, size_t size)
> +{
> +	struct image_info *image;
> +	int ret;
> +
> +	ret = mhi_alloc_bhie_table(mhi_cntrl, &image, size);
> +	if (ret)
> +		return ret;
> +
> +	mhi_firmware_copy_bhie(mhi_cntrl, fw_data, size, image);
> +
> +	ret = mhi_fw_load_bhie(mhi_cntrl, &image->mhi_buf[image->entries - 1]);
> +	mhi_free_bhie_table(mhi_cntrl, image);
> +
> +	return ret;
> +}
> +
>  void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  {
>  	const struct firmware *firmware = NULL;
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> +	enum mhi_fw_load_type fw_load_type;
>  	enum mhi_pm_state new_state;
> -	struct image_info *image;
>  	const char *fw_name;
>  	const u8 *fw_data;
>  	size_t size, fw_sz;
> @@ -481,6 +531,12 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	fw_name = (mhi_cntrl->ee == MHI_EE_EDL) ?
>  		mhi_cntrl->edl_image : mhi_cntrl->fw_image;
>  
> +	fw_load_type = mhi_fw_load_type_get(mhi_cntrl);
> +	if (fw_load_type == MHI_FW_LOAD_UNKNOWN) {
> +		dev_err(dev, "Cannot load FW as load type is UNKNOWN\n");
> +		return;
> +	}
> +
>  	/* check if the driver has already provided the firmware data */
>  	if (!fw_name && mhi_cntrl->fbc_download &&
>  	    mhi_cntrl->fw_data && mhi_cntrl->fw_sz) {
> @@ -518,20 +574,16 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	fw_sz = firmware->size;
>  
>  skip_req_fw:
> -	ret = mhi_alloc_bhi_buffer(mhi_cntrl, &image, size);
> -	if (ret) {
> -		release_firmware(firmware);
> -		goto error_fw_load;
> -	}
> -	mhi_firmware_copy_bhi(mhi_cntrl, fw_data, size, image);
> -
> -	/* Download image using BHI */
> -	ret = mhi_fw_load_bhi(mhi_cntrl, image->mhi_buf);
> -	mhi_free_bhi_buffer(mhi_cntrl, image);
> +	if (fw_load_type == MHI_FW_LOAD_BHIE)
> +		ret = mhi_send_image_bhie(mhi_cntrl, fw_data, size);
> +	else
> +		ret = mhi_send_image_bhi(mhi_cntrl, fw_data, size);
>  
>  	/* Error or in EDL mode, we're done */
>  	if (ret) {
> -		dev_err(dev, "MHI did not load image over BHI, ret: %d\n", ret);
> +		dev_err(dev, "MHI did not load image over BHI%s, ret: %d\n",
> +			fw_load_type == MHI_FW_LOAD_BHIE ? "e" : "",
> +			ret);
>  		release_firmware(firmware);
>  		goto error_fw_load;
>  	}
> @@ -550,7 +602,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	 * If we're doing fbc, populate vector tables while
>  	 * device transitioning into MHI READY state
>  	 */
> -	if (mhi_cntrl->fbc_download) {
> +	if (fw_load_type == MHI_FW_LOAD_FBC) {
>  		ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->fbc_image, fw_sz);
>  		if (ret) {
>  			release_firmware(firmware);
> @@ -575,7 +627,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	return;
>  
>  error_ready_state:
> -	if (mhi_cntrl->fbc_download) {
> +	if (fw_load_type == MHI_FW_LOAD_FBC) {
>  		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image);
>  		mhi_cntrl->fbc_image = NULL;
>  	}
> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index a9b1f8beee7b..13e7a55f54ff 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -1144,7 +1144,7 @@ int mhi_prepare_for_power_up(struct mhi_controller *mhi_cntrl)
>  	}
>  	mhi_cntrl->bhi = mhi_cntrl->regs + bhi_off;
>  
> -	if (mhi_cntrl->fbc_download || mhi_cntrl->rddm_size) {
> +	if (mhi_cntrl->fbc_download || mhi_cntrl->rddm_size || mhi_cntrl->seg_len) {
>  		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs, BHIEOFF,
>  				   &bhie_off);
>  		if (ret) {
> diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
> index 3134f111be35..afcf536083bc 100644
> --- a/drivers/bus/mhi/host/internal.h
> +++ b/drivers/bus/mhi/host/internal.h
> @@ -29,6 +29,14 @@ struct bhi_vec_entry {
>  	u64 size;
>  };
>  
> +enum mhi_fw_load_type {
> +	MHI_FW_LOAD_UNKNOWN,
> +	MHI_FW_LOAD_BHI,	/* BHI only in PBL */
> +	MHI_FW_LOAD_BHIE,	/* BHIe only in PBL */
> +	MHI_FW_LOAD_FBC,	/* BHI in PBL followed by BHIe in SBL */
> +	MHI_FW_LOAD_MAX,
> +};
> +
>  enum mhi_ch_state_type {
>  	MHI_CH_STATE_TYPE_RESET,
>  	MHI_CH_STATE_TYPE_STOP,

Re: [PATCH 2/7] bus: mhi: host: Add a policy to enable image transfer via BHIe in PBL

[Manivannan Sadhasivam]

On Fri, Dec 13, 2024 at 02:33:35PM -0700, Jeffrey Hugo wrote:
> From: Matthew Leung <quic_mattleun@quicinc.com>
> 
> Currently, mhi host only performs firmware transfer via BHI in PBL and

s/mhi/MHI here and below.

> BHIe from SBL. To support BHIe transfer directly from PBL, a policy
> needs to be added.
> 
> With this policy, BHIe will be used to transfer firmware in PBL if the
> mhi controller has bhie regs, sets seg_len, and does not set

s/bhie/BHIe

> fbc_download. The intention is to transfer firmware using BHIe in PBL
> without further BHIe transfers in SBL.
> 
> Signed-off-by: Matthew Leung <quic_mattleun@quicinc.com>
> Reviewed-by: Youssef Samir <quic_yabdulra@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>  drivers/bus/mhi/host/boot.c     | 80 +++++++++++++++++++++++++++------
>  drivers/bus/mhi/host/init.c     |  2 +-
>  drivers/bus/mhi/host/internal.h |  8 ++++
>  3 files changed, 75 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
> index e3f3c07166ad..c9ecb6427209 100644
> --- a/drivers/bus/mhi/host/boot.c
> +++ b/drivers/bus/mhi/host/boot.c
> @@ -452,12 +452,62 @@ static void mhi_firmware_copy_bhie(struct mhi_controller *mhi_cntrl,
>  	}
>  }
>  
> +static enum mhi_fw_load_type mhi_fw_load_type_get(const struct mhi_controller *mhi_cntrl)
> +{
> +	enum mhi_fw_load_type ret = MHI_FW_LOAD_UNKNOWN;

You can directly return the enum without a local variable.

> +
> +	if (mhi_cntrl->fbc_download) {
> +		if (mhi_cntrl->bhie && mhi_cntrl->seg_len)

I don't think this condition can fail. If 'mhi_cntrl->bhie' is NULL,
mhi_prepare_for_power_up() will fail. So I think MHI_FW_LOAD_UNKNOWN is not
needed.

Also, all the validation should be performed early, not while loading fw.

> +			ret = MHI_FW_LOAD_FBC;
> +	} else {
> +		if (mhi_cntrl->bhie && mhi_cntrl->seg_len)
> +			ret = MHI_FW_LOAD_BHIE;
> +		else
> +			ret = MHI_FW_LOAD_BHI;
> +	}
> +	return ret;
> +}
> +
> +static int mhi_send_image_bhi(struct mhi_controller *mhi_cntrl, const u8 *fw_data, size_t size)

mhi_load_image_bhi?

> +{
> +	struct image_info *image;
> +	int ret;
> +
> +	ret = mhi_alloc_bhi_buffer(mhi_cntrl, &image, size);
> +	if (ret)
> +		return ret;
> +
> +	mhi_firmware_copy_bhi(mhi_cntrl, fw_data, size, image);
> +
> +	ret = mhi_fw_load_bhi(mhi_cntrl, &image->mhi_buf[image->entries - 1]);
> +	mhi_free_bhi_buffer(mhi_cntrl, image);
> +
> +	return ret;
> +}
> +
> +static int mhi_send_image_bhie(struct mhi_controller *mhi_cntrl, const u8 *fw_data, size_t size)

mhi_load_image_bhie?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 2/7] bus: mhi: host: Add a policy to enable image transfer via BHIe in PBL

[Jeffrey Hugo]

On 1/7/2025 10:42 PM, Manivannan Sadhasivam wrote:
> On Fri, Dec 13, 2024 at 02:33:35PM -0700, Jeffrey Hugo wrote:
>> From: Matthew Leung <quic_mattleun@quicinc.com>
>>
>> Currently, mhi host only performs firmware transfer via BHI in PBL and
> 
> s/mhi/MHI here and below.

Done

> 
>> BHIe from SBL. To support BHIe transfer directly from PBL, a policy
>> needs to be added.
>>
>> With this policy, BHIe will be used to transfer firmware in PBL if the
>> mhi controller has bhie regs, sets seg_len, and does not set
> 
> s/bhie/BHIe

Done

> 
>> fbc_download. The intention is to transfer firmware using BHIe in PBL
>> without further BHIe transfers in SBL.
>>
>> Signed-off-by: Matthew Leung <quic_mattleun@quicinc.com>
>> Reviewed-by: Youssef Samir <quic_yabdulra@quicinc.com>
>> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>> ---
>>   drivers/bus/mhi/host/boot.c     | 80 +++++++++++++++++++++++++++------
>>   drivers/bus/mhi/host/init.c     |  2 +-
>>   drivers/bus/mhi/host/internal.h |  8 ++++
>>   3 files changed, 75 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
>> index e3f3c07166ad..c9ecb6427209 100644
>> --- a/drivers/bus/mhi/host/boot.c
>> +++ b/drivers/bus/mhi/host/boot.c
>> @@ -452,12 +452,62 @@ static void mhi_firmware_copy_bhie(struct mhi_controller *mhi_cntrl,
>>   	}
>>   }
>>   
>> +static enum mhi_fw_load_type mhi_fw_load_type_get(const struct mhi_controller *mhi_cntrl)
>> +{
>> +	enum mhi_fw_load_type ret = MHI_FW_LOAD_UNKNOWN;
> 
> You can directly return the enum without a local variable.

Done

> 
>> +
>> +	if (mhi_cntrl->fbc_download) {
>> +		if (mhi_cntrl->bhie && mhi_cntrl->seg_len)
> 
> I don't think this condition can fail. If 'mhi_cntrl->bhie' is NULL,
> mhi_prepare_for_power_up() will fail. So I think MHI_FW_LOAD_UNKNOWN is not
> needed.
> 
> Also, all the validation should be performed early, not while loading fw.

You are right.  That will not fail.  I've simplified this, relying on 
the existing validation.

> 
>> +			ret = MHI_FW_LOAD_FBC;
>> +	} else {
>> +		if (mhi_cntrl->bhie && mhi_cntrl->seg_len)
>> +			ret = MHI_FW_LOAD_BHIE;
>> +		else
>> +			ret = MHI_FW_LOAD_BHI;
>> +	}
>> +	return ret;
>> +}
>> +
>> +static int mhi_send_image_bhi(struct mhi_controller *mhi_cntrl, const u8 *fw_data, size_t size)
> 
> mhi_load_image_bhi?

Done

> 
>> +{
>> +	struct image_info *image;
>> +	int ret;
>> +
>> +	ret = mhi_alloc_bhi_buffer(mhi_cntrl, &image, size);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mhi_firmware_copy_bhi(mhi_cntrl, fw_data, size, image);
>> +
>> +	ret = mhi_fw_load_bhi(mhi_cntrl, &image->mhi_buf[image->entries - 1]);
>> +	mhi_free_bhi_buffer(mhi_cntrl, image);
>> +
>> +	return ret;
>> +}
>> +
>> +static int mhi_send_image_bhie(struct mhi_controller *mhi_cntrl, const u8 *fw_data, size_t size)
> 
> mhi_load_image_bhie?

Done

[PATCH 3/7] accel/qaic: Allocate an exact number of MSIs

[Jeffrey Hugo]

From: Youssef Samir <quic_yabdulra@quicinc.com>

Devices use 1 MSI vector for the MHI controller and as many vectors as
the DMA bridge channels on the device. During the probing of the
device, the driver allocates 32 MSI vectors, which is usually more
than what is needed for AIC100 devices, which is wasting resources.

Allocate only the needed number of MSI vectors per device.

Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
Reviewed-by: Troy Hanson <quic_thanson@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 drivers/accel/qaic/qaic_drv.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index 81819b9ef8d4..6e9bed17b3f1 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -465,12 +465,13 @@ static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev)
 
 static int init_msi(struct qaic_device *qdev, struct pci_dev *pdev)
 {
+	int irq_count = qdev->num_dbc + 1;
 	int mhi_irq;
 	int ret;
 	int i;
 
 	/* Managed release since we use pcim_enable_device */
-	ret = pci_alloc_irq_vectors(pdev, 32, 32, PCI_IRQ_MSI);
+	ret = pci_alloc_irq_vectors(pdev, irq_count, irq_count, PCI_IRQ_MSI);
 	if (ret == -ENOSPC) {
 		ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
 		if (ret < 0)
@@ -485,7 +486,8 @@ static int init_msi(struct qaic_device *qdev, struct pci_dev *pdev)
 		 * interrupted, it shouldn't race with itself.
 		 */
 		qdev->single_msi = true;
-		pci_info(pdev, "Allocating 32 MSIs failed, operating in 1 MSI mode. Performance may be impacted.\n");
+		pci_info(pdev, "Allocating %d MSIs failed, operating in 1 MSI mode. Performance may be impacted.\n",
+			 irq_count);
 	} else if (ret < 0) {
 		return ret;
 	}
-- 
2.34.1

Re: [PATCH 3/7] accel/qaic: Allocate an exact number of MSIs

[Lizhi Hou]

On 12/13/24 13:33, Jeffrey Hugo wrote:
> From: Youssef Samir <quic_yabdulra@quicinc.com>
>
> Devices use 1 MSI vector for the MHI controller and as many vectors as
> the DMA bridge channels on the device. During the probing of the
> device, the driver allocates 32 MSI vectors, which is usually more
> than what is needed for AIC100 devices, which is wasting resources.
>
> Allocate only the needed number of MSI vectors per device.
>
> Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
> Reviewed-by: Troy Hanson <quic_thanson@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>   drivers/accel/qaic/qaic_drv.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index 81819b9ef8d4..6e9bed17b3f1 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -465,12 +465,13 @@ static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev)
>   
>   static int init_msi(struct qaic_device *qdev, struct pci_dev *pdev)
>   {
> +	int irq_count = qdev->num_dbc + 1;
>   	int mhi_irq;
>   	int ret;
>   	int i;
>   
>   	/* Managed release since we use pcim_enable_device */
> -	ret = pci_alloc_irq_vectors(pdev, 32, 32, PCI_IRQ_MSI);
> +	ret = pci_alloc_irq_vectors(pdev, irq_count, irq_count, PCI_IRQ_MSI);
>   	if (ret == -ENOSPC) {
>   		ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
>   		if (ret < 0)
> @@ -485,7 +486,8 @@ static int init_msi(struct qaic_device *qdev, struct pci_dev *pdev)
>   		 * interrupted, it shouldn't race with itself.
>   		 */
>   		qdev->single_msi = true;
> -		pci_info(pdev, "Allocating 32 MSIs failed, operating in 1 MSI mode. Performance may be impacted.\n");
> +		pci_info(pdev, "Allocating %d MSIs failed, operating in 1 MSI mode. Performance may be impacted.\n",
> +			 irq_count);
>   	} else if (ret < 0) {
>   		return ret;
>   	}
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>

[PATCH 4/7] accel/qaic: Add support for MSI-X

[Jeffrey Hugo]

From: Youssef Samir <quic_yabdulra@quicinc.com>

AIC200 device will support MSI-X while AIC100 devices will keep using
MSI. pci_alloc_irq_vectors() will try to allocate MSI-X vectors if it
is supported by the target device, otherwise, it will fallback to MSI.

Add support for MSI-X vectors allocation for AIC200 devices.

Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 drivers/accel/qaic/qaic_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index 6e9bed17b3f1..ce0428f6cb82 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -471,9 +471,9 @@ static int init_msi(struct qaic_device *qdev, struct pci_dev *pdev)
 	int i;
 
 	/* Managed release since we use pcim_enable_device */
-	ret = pci_alloc_irq_vectors(pdev, irq_count, irq_count, PCI_IRQ_MSI);
+	ret = pci_alloc_irq_vectors(pdev, irq_count, irq_count, PCI_IRQ_MSI | PCI_IRQ_MSIX);
 	if (ret == -ENOSPC) {
-		ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
+		ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI | PCI_IRQ_MSIX);
 		if (ret < 0)
 			return ret;
 
-- 
2.34.1

Re: [PATCH 4/7] accel/qaic: Add support for MSI-X

[Lizhi Hou]

On 12/13/24 13:33, Jeffrey Hugo wrote:
> From: Youssef Samir <quic_yabdulra@quicinc.com>
>
> AIC200 device will support MSI-X while AIC100 devices will keep using
> MSI. pci_alloc_irq_vectors() will try to allocate MSI-X vectors if it
> is supported by the target device, otherwise, it will fallback to MSI.
>
> Add support for MSI-X vectors allocation for AIC200 devices.
>
> Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>   drivers/accel/qaic/qaic_drv.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index 6e9bed17b3f1..ce0428f6cb82 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -471,9 +471,9 @@ static int init_msi(struct qaic_device *qdev, struct pci_dev *pdev)
>   	int i;
>   
>   	/* Managed release since we use pcim_enable_device */
> -	ret = pci_alloc_irq_vectors(pdev, irq_count, irq_count, PCI_IRQ_MSI);
> +	ret = pci_alloc_irq_vectors(pdev, irq_count, irq_count, PCI_IRQ_MSI | PCI_IRQ_MSIX);
>   	if (ret == -ENOSPC) {
> -		ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> +		ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI | PCI_IRQ_MSIX);
>   		if (ret < 0)
>   			return ret;
>   
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>

[PATCH 5/7] accel/qaic: Mask out SR-IOV PCI resources

[Jeffrey Hugo]

From: Youssef Samir <quic_yabdulra@quicinc.com>

During the initialization of the qaic device, pci_select_bars() is
used to fetch a bitmask of the BARs exposed by the device. On devices
that have Virtual Functions capabilities, the bitmask includes SR-IOV
BARs.

Use a mask to filter out SR-IOV BARs if they exist.

Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 drivers/accel/qaic/qaic_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index ce0428f6cb82..00fa07aebacd 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -431,7 +431,7 @@ static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev)
 	int bars;
 	int ret;
 
-	bars = pci_select_bars(pdev, IORESOURCE_MEM);
+	bars = pci_select_bars(pdev, IORESOURCE_MEM) & 0x3f;
 
 	/* make sure the device has the expected BARs */
 	if (bars != (BIT(0) | BIT(2) | BIT(4))) {
-- 
2.34.1

Re: [PATCH 5/7] accel/qaic: Mask out SR-IOV PCI resources

[Lizhi Hou]

On 12/13/24 13:33, Jeffrey Hugo wrote:
> From: Youssef Samir <quic_yabdulra@quicinc.com>
>
> During the initialization of the qaic device, pci_select_bars() is
> used to fetch a bitmask of the BARs exposed by the device. On devices
> that have Virtual Functions capabilities, the bitmask includes SR-IOV
> BARs.
>
> Use a mask to filter out SR-IOV BARs if they exist.
>
> Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>   drivers/accel/qaic/qaic_drv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index ce0428f6cb82..00fa07aebacd 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -431,7 +431,7 @@ static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev)
>   	int bars;
>   	int ret;
>   
> -	bars = pci_select_bars(pdev, IORESOURCE_MEM);
> +	bars = pci_select_bars(pdev, IORESOURCE_MEM) & 0x3f;
>   
>   	/* make sure the device has the expected BARs */
>   	if (bars != (BIT(0) | BIT(2) | BIT(4))) {
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>

[PATCH 6/7] accel/qaic: Add config structs for supported cards

[Jeffrey Hugo]

As the number of cards supported by the driver grows, their
configurations will differ. The driver needs to become more dynamic
to support these configurations. Currently, each card may differ in
the exposed BARs, the regions they map to, and the family.

Create config structs for each card, and let the driver configure the
qaic_device struct based on the configurations passed to the driver.

Co-developed-by: Youssef Samir <quic_yabdulra@quicinc.com>
Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 drivers/accel/qaic/qaic.h          | 13 +++--
 drivers/accel/qaic/qaic_drv.c      | 76 ++++++++++++++++++++----------
 drivers/accel/qaic/qaic_timesync.c |  2 +-
 3 files changed, 61 insertions(+), 30 deletions(-)

diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
index 02561b6cecc6..cf97fd9a7e70 100644
--- a/drivers/accel/qaic/qaic.h
+++ b/drivers/accel/qaic/qaic.h
@@ -32,6 +32,11 @@
 #define to_accel_kdev(qddev) (to_drm(qddev)->accel->kdev) /* Return Linux device of accel node */
 #define to_qaic_device(dev) (to_qaic_drm_device((dev))->qdev)
 
+enum aic_families {
+	FAMILY_AIC100,
+	FAMILY_MAX,
+};
+
 enum __packed dev_states {
 	/* Device is offline or will be very soon */
 	QAIC_OFFLINE,
@@ -113,10 +118,10 @@ struct qaic_device {
 	struct pci_dev		*pdev;
 	/* Req. ID of request that will be queued next in MHI control device */
 	u32			next_seq_num;
-	/* Base address of bar 0 */
-	void __iomem		*bar_0;
-	/* Base address of bar 2 */
-	void __iomem		*bar_2;
+	/* Base address of the MHI bar */
+	void __iomem		*bar_mhi;
+	/* Base address of the DBCs bar */
+	void __iomem		*bar_dbc;
 	/* Controller structure for MHI devices */
 	struct mhi_controller	*mhi_cntrl;
 	/* MHI control channel device */
diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index 00fa07aebacd..4e63e475b389 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -34,13 +34,38 @@
 
 MODULE_IMPORT_NS("DMA_BUF");
 
-#define PCI_DEV_AIC080			0xa080
-#define PCI_DEV_AIC100			0xa100
+#define PCI_DEVICE_ID_QCOM_AIC080	0xa080
+#define PCI_DEVICE_ID_QCOM_AIC100	0xa100
 #define QAIC_NAME			"qaic"
 #define QAIC_DESC			"Qualcomm Cloud AI Accelerators"
 #define CNTL_MAJOR			5
 #define CNTL_MINOR			0
 
+struct qaic_device_config {
+	/* Indicates the AIC family the device belongs to */
+	int family;
+	/* A bitmask representing the available BARs */
+	int bar_mask;
+	/* An index value used to identify the MHI controller BAR */
+	unsigned int mhi_bar_idx;
+	/* An index value used to identify the DBCs BAR */
+	unsigned int dbc_bar_idx;
+};
+
+static const struct qaic_device_config aic080_config = {
+	.family = FAMILY_AIC100,
+	.bar_mask = BIT(0) | BIT(2) | BIT(4),
+	.mhi_bar_idx = 0,
+	.dbc_bar_idx = 2,
+};
+
+static const struct qaic_device_config aic100_config = {
+	.family = FAMILY_AIC100,
+	.bar_mask = BIT(0) | BIT(2) | BIT(4),
+	.mhi_bar_idx = 0,
+	.dbc_bar_idx = 2,
+};
+
 bool datapath_polling;
 module_param(datapath_polling, bool, 0400);
 MODULE_PARM_DESC(datapath_polling, "Operate the datapath in polling mode");
@@ -352,7 +377,8 @@ void qaic_dev_reset_clean_local_state(struct qaic_device *qdev)
 		release_dbc(qdev, i);
 }
 
-static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_device_id *id)
+static struct qaic_device *create_qdev(struct pci_dev *pdev,
+				       const struct qaic_device_config *config)
 {
 	struct device *dev = &pdev->dev;
 	struct qaic_drm_device *qddev;
@@ -365,12 +391,10 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de
 		return NULL;
 
 	qdev->dev_state = QAIC_OFFLINE;
-	if (id->device == PCI_DEV_AIC080 || id->device == PCI_DEV_AIC100) {
-		qdev->num_dbc = 16;
-		qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, sizeof(*qdev->dbc), GFP_KERNEL);
-		if (!qdev->dbc)
-			return NULL;
-	}
+	qdev->num_dbc = 16;
+	qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, sizeof(*qdev->dbc), GFP_KERNEL);
+	if (!qdev->dbc)
+		return NULL;
 
 	qddev = devm_drm_dev_alloc(&pdev->dev, &qaic_accel_driver, struct qaic_drm_device, drm);
 	if (IS_ERR(qddev))
@@ -426,7 +450,8 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de
 	return qdev;
 }
 
-static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev)
+static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev,
+		    const struct qaic_device_config *config)
 {
 	int bars;
 	int ret;
@@ -434,9 +459,9 @@ static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev)
 	bars = pci_select_bars(pdev, IORESOURCE_MEM) & 0x3f;
 
 	/* make sure the device has the expected BARs */
-	if (bars != (BIT(0) | BIT(2) | BIT(4))) {
-		pci_dbg(pdev, "%s: expected BARs 0, 2, and 4 not found in device. Found 0x%x\n",
-			__func__, bars);
+	if (bars != config->bar_mask) {
+		pci_dbg(pdev, "%s: expected BARs %#x not found in device. Found %#x\n",
+			__func__, config->bar_mask, bars);
 		return -EINVAL;
 	}
 
@@ -449,13 +474,13 @@ static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev)
 		return ret;
 	dma_set_max_seg_size(&pdev->dev, UINT_MAX);
 
-	qdev->bar_0 = devm_ioremap_resource(&pdev->dev, &pdev->resource[0]);
-	if (IS_ERR(qdev->bar_0))
-		return PTR_ERR(qdev->bar_0);
+	qdev->bar_mhi = devm_ioremap_resource(&pdev->dev, &pdev->resource[config->mhi_bar_idx]);
+	if (IS_ERR(qdev->bar_mhi))
+		return PTR_ERR(qdev->bar_mhi);
 
-	qdev->bar_2 = devm_ioremap_resource(&pdev->dev, &pdev->resource[2]);
-	if (IS_ERR(qdev->bar_2))
-		return PTR_ERR(qdev->bar_2);
+	qdev->bar_dbc = devm_ioremap_resource(&pdev->dev, &pdev->resource[config->dbc_bar_idx]);
+	if (IS_ERR(qdev->bar_dbc))
+		return PTR_ERR(qdev->bar_dbc);
 
 	/* Managed release since we use pcim_enable_device above */
 	pci_set_master(pdev);
@@ -517,21 +542,22 @@ static int init_msi(struct qaic_device *qdev, struct pci_dev *pdev)
 
 static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	struct qaic_device_config *config = (struct qaic_device_config *)id->driver_data;
 	struct qaic_device *qdev;
 	int mhi_irq;
 	int ret;
 	int i;
 
-	qdev = create_qdev(pdev, id);
+	qdev = create_qdev(pdev, config);
 	if (!qdev)
 		return -ENOMEM;
 
-	ret = init_pci(qdev, pdev);
+	ret = init_pci(qdev, pdev, config);
 	if (ret)
 		return ret;
 
 	for (i = 0; i < qdev->num_dbc; ++i)
-		qdev->dbc[i].dbc_base = qdev->bar_2 + QAIC_DBC_OFF(i);
+		qdev->dbc[i].dbc_base = qdev->bar_dbc + QAIC_DBC_OFF(i);
 
 	mhi_irq = init_msi(qdev, pdev);
 	if (mhi_irq < 0)
@@ -541,7 +567,7 @@ static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (ret)
 		return ret;
 
-	qdev->mhi_cntrl = qaic_mhi_register_controller(pdev, qdev->bar_0, mhi_irq,
+	qdev->mhi_cntrl = qaic_mhi_register_controller(pdev, qdev->bar_mhi, mhi_irq,
 						       qdev->single_msi);
 	if (IS_ERR(qdev->mhi_cntrl)) {
 		ret = PTR_ERR(qdev->mhi_cntrl);
@@ -609,8 +635,8 @@ static struct mhi_driver qaic_mhi_driver = {
 };
 
 static const struct pci_device_id qaic_ids[] = {
-	{ PCI_DEVICE(PCI_VENDOR_ID_QCOM, PCI_DEV_AIC080), },
-	{ PCI_DEVICE(PCI_VENDOR_ID_QCOM, PCI_DEV_AIC100), },
+	{ PCI_DEVICE_DATA(QCOM, AIC080, (kernel_ulong_t)&aic080_config), },
+	{ PCI_DEVICE_DATA(QCOM, AIC100, (kernel_ulong_t)&aic100_config), },
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, qaic_ids);
diff --git a/drivers/accel/qaic/qaic_timesync.c b/drivers/accel/qaic/qaic_timesync.c
index 301f4462d51b..2473c66309d4 100644
--- a/drivers/accel/qaic/qaic_timesync.c
+++ b/drivers/accel/qaic/qaic_timesync.c
@@ -201,7 +201,7 @@ static int qaic_timesync_probe(struct mhi_device *mhi_dev, const struct mhi_devi
 		goto free_sync_msg;
 
 	/* Qtimer register pointer */
-	mqtsdev->qtimer_addr = qdev->bar_0 + QTIMER_REG_OFFSET;
+	mqtsdev->qtimer_addr = qdev->bar_mhi + QTIMER_REG_OFFSET;
 	timer_setup(timer, qaic_timesync_timer, 0);
 	timer->expires = jiffies + msecs_to_jiffies(timesync_delay_ms);
 	add_timer(timer);
-- 
2.34.1

Re: [PATCH 6/7] accel/qaic: Add config structs for supported cards

[Lizhi Hou]

On 12/13/24 13:33, Jeffrey Hugo wrote:
> As the number of cards supported by the driver grows, their
> configurations will differ. The driver needs to become more dynamic
> to support these configurations. Currently, each card may differ in
> the exposed BARs, the regions they map to, and the family.
>
> Create config structs for each card, and let the driver configure the
> qaic_device struct based on the configurations passed to the driver.
>
> Co-developed-by: Youssef Samir <quic_yabdulra@quicinc.com>
> Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>   drivers/accel/qaic/qaic.h          | 13 +++--
>   drivers/accel/qaic/qaic_drv.c      | 76 ++++++++++++++++++++----------
>   drivers/accel/qaic/qaic_timesync.c |  2 +-
>   3 files changed, 61 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
> index 02561b6cecc6..cf97fd9a7e70 100644
> --- a/drivers/accel/qaic/qaic.h
> +++ b/drivers/accel/qaic/qaic.h
> @@ -32,6 +32,11 @@
>   #define to_accel_kdev(qddev) (to_drm(qddev)->accel->kdev) /* Return Linux device of accel node */
>   #define to_qaic_device(dev) (to_qaic_drm_device((dev))->qdev)
>   
> +enum aic_families {
> +	FAMILY_AIC100,
> +	FAMILY_MAX,
> +};
> +
>   enum __packed dev_states {
>   	/* Device is offline or will be very soon */
>   	QAIC_OFFLINE,
> @@ -113,10 +118,10 @@ struct qaic_device {
>   	struct pci_dev		*pdev;
>   	/* Req. ID of request that will be queued next in MHI control device */
>   	u32			next_seq_num;
> -	/* Base address of bar 0 */
> -	void __iomem		*bar_0;
> -	/* Base address of bar 2 */
> -	void __iomem		*bar_2;
> +	/* Base address of the MHI bar */
> +	void __iomem		*bar_mhi;
> +	/* Base address of the DBCs bar */
> +	void __iomem		*bar_dbc;
>   	/* Controller structure for MHI devices */
>   	struct mhi_controller	*mhi_cntrl;
>   	/* MHI control channel device */
> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index 00fa07aebacd..4e63e475b389 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -34,13 +34,38 @@
>   
>   MODULE_IMPORT_NS("DMA_BUF");
>   
> -#define PCI_DEV_AIC080			0xa080
> -#define PCI_DEV_AIC100			0xa100
> +#define PCI_DEVICE_ID_QCOM_AIC080	0xa080
> +#define PCI_DEVICE_ID_QCOM_AIC100	0xa100
>   #define QAIC_NAME			"qaic"
>   #define QAIC_DESC			"Qualcomm Cloud AI Accelerators"
>   #define CNTL_MAJOR			5
>   #define CNTL_MINOR			0
>   
> +struct qaic_device_config {
> +	/* Indicates the AIC family the device belongs to */
> +	int family;
> +	/* A bitmask representing the available BARs */
> +	int bar_mask;
> +	/* An index value used to identify the MHI controller BAR */
> +	unsigned int mhi_bar_idx;
> +	/* An index value used to identify the DBCs BAR */
> +	unsigned int dbc_bar_idx;
> +};
> +
> +static const struct qaic_device_config aic080_config = {
> +	.family = FAMILY_AIC100,
> +	.bar_mask = BIT(0) | BIT(2) | BIT(4),
> +	.mhi_bar_idx = 0,
> +	.dbc_bar_idx = 2,
> +};
> +
> +static const struct qaic_device_config aic100_config = {
> +	.family = FAMILY_AIC100,
> +	.bar_mask = BIT(0) | BIT(2) | BIT(4),
> +	.mhi_bar_idx = 0,
> +	.dbc_bar_idx = 2,
> +};
> +
>   bool datapath_polling;
>   module_param(datapath_polling, bool, 0400);
>   MODULE_PARM_DESC(datapath_polling, "Operate the datapath in polling mode");
> @@ -352,7 +377,8 @@ void qaic_dev_reset_clean_local_state(struct qaic_device *qdev)
>   		release_dbc(qdev, i);
>   }
>   
> -static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_device_id *id)
> +static struct qaic_device *create_qdev(struct pci_dev *pdev,
> +				       const struct qaic_device_config *config)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct qaic_drm_device *qddev;
> @@ -365,12 +391,10 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de
>   		return NULL;
>   
>   	qdev->dev_state = QAIC_OFFLINE;
> -	if (id->device == PCI_DEV_AIC080 || id->device == PCI_DEV_AIC100) {
> -		qdev->num_dbc = 16;
> -		qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, sizeof(*qdev->dbc), GFP_KERNEL);
> -		if (!qdev->dbc)
> -			return NULL;
> -	}
> +	qdev->num_dbc = 16;

Is it better to put num_dbc in qaic_device_config?


Thanks,

Lizhi

> +	qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, sizeof(*qdev->dbc), GFP_KERNEL);
> +	if (!qdev->dbc)
> +		return NULL;
>   
>   	qddev = devm_drm_dev_alloc(&pdev->dev, &qaic_accel_driver, struct qaic_drm_device, drm);
>   	if (IS_ERR(qddev))
> @@ -426,7 +450,8 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de
>   	return qdev;
>   }
>   
> -static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev)
> +static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev,
> +		    const struct qaic_device_config *config)
>   {
>   	int bars;
>   	int ret;
> @@ -434,9 +459,9 @@ static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev)
>   	bars = pci_select_bars(pdev, IORESOURCE_MEM) & 0x3f;
>   
>   	/* make sure the device has the expected BARs */
> -	if (bars != (BIT(0) | BIT(2) | BIT(4))) {
> -		pci_dbg(pdev, "%s: expected BARs 0, 2, and 4 not found in device. Found 0x%x\n",
> -			__func__, bars);
> +	if (bars != config->bar_mask) {
> +		pci_dbg(pdev, "%s: expected BARs %#x not found in device. Found %#x\n",
> +			__func__, config->bar_mask, bars);
>   		return -EINVAL;
>   	}
>   
> @@ -449,13 +474,13 @@ static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev)
>   		return ret;
>   	dma_set_max_seg_size(&pdev->dev, UINT_MAX);
>   
> -	qdev->bar_0 = devm_ioremap_resource(&pdev->dev, &pdev->resource[0]);
> -	if (IS_ERR(qdev->bar_0))
> -		return PTR_ERR(qdev->bar_0);
> +	qdev->bar_mhi = devm_ioremap_resource(&pdev->dev, &pdev->resource[config->mhi_bar_idx]);
> +	if (IS_ERR(qdev->bar_mhi))
> +		return PTR_ERR(qdev->bar_mhi);
>   
> -	qdev->bar_2 = devm_ioremap_resource(&pdev->dev, &pdev->resource[2]);
> -	if (IS_ERR(qdev->bar_2))
> -		return PTR_ERR(qdev->bar_2);
> +	qdev->bar_dbc = devm_ioremap_resource(&pdev->dev, &pdev->resource[config->dbc_bar_idx]);
> +	if (IS_ERR(qdev->bar_dbc))
> +		return PTR_ERR(qdev->bar_dbc);
>   
>   	/* Managed release since we use pcim_enable_device above */
>   	pci_set_master(pdev);
> @@ -517,21 +542,22 @@ static int init_msi(struct qaic_device *qdev, struct pci_dev *pdev)
>   
>   static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   {
> +	struct qaic_device_config *config = (struct qaic_device_config *)id->driver_data;
>   	struct qaic_device *qdev;
>   	int mhi_irq;
>   	int ret;
>   	int i;
>   
> -	qdev = create_qdev(pdev, id);
> +	qdev = create_qdev(pdev, config);
>   	if (!qdev)
>   		return -ENOMEM;
>   
> -	ret = init_pci(qdev, pdev);
> +	ret = init_pci(qdev, pdev, config);
>   	if (ret)
>   		return ret;
>   
>   	for (i = 0; i < qdev->num_dbc; ++i)
> -		qdev->dbc[i].dbc_base = qdev->bar_2 + QAIC_DBC_OFF(i);
> +		qdev->dbc[i].dbc_base = qdev->bar_dbc + QAIC_DBC_OFF(i);
>   
>   	mhi_irq = init_msi(qdev, pdev);
>   	if (mhi_irq < 0)
> @@ -541,7 +567,7 @@ static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   	if (ret)
>   		return ret;
>   
> -	qdev->mhi_cntrl = qaic_mhi_register_controller(pdev, qdev->bar_0, mhi_irq,
> +	qdev->mhi_cntrl = qaic_mhi_register_controller(pdev, qdev->bar_mhi, mhi_irq,
>   						       qdev->single_msi);
>   	if (IS_ERR(qdev->mhi_cntrl)) {
>   		ret = PTR_ERR(qdev->mhi_cntrl);
> @@ -609,8 +635,8 @@ static struct mhi_driver qaic_mhi_driver = {
>   };
>   
>   static const struct pci_device_id qaic_ids[] = {
> -	{ PCI_DEVICE(PCI_VENDOR_ID_QCOM, PCI_DEV_AIC080), },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_QCOM, PCI_DEV_AIC100), },
> +	{ PCI_DEVICE_DATA(QCOM, AIC080, (kernel_ulong_t)&aic080_config), },
> +	{ PCI_DEVICE_DATA(QCOM, AIC100, (kernel_ulong_t)&aic100_config), },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(pci, qaic_ids);
> diff --git a/drivers/accel/qaic/qaic_timesync.c b/drivers/accel/qaic/qaic_timesync.c
> index 301f4462d51b..2473c66309d4 100644
> --- a/drivers/accel/qaic/qaic_timesync.c
> +++ b/drivers/accel/qaic/qaic_timesync.c
> @@ -201,7 +201,7 @@ static int qaic_timesync_probe(struct mhi_device *mhi_dev, const struct mhi_devi
>   		goto free_sync_msg;
>   
>   	/* Qtimer register pointer */
> -	mqtsdev->qtimer_addr = qdev->bar_0 + QTIMER_REG_OFFSET;
> +	mqtsdev->qtimer_addr = qdev->bar_mhi + QTIMER_REG_OFFSET;
>   	timer_setup(timer, qaic_timesync_timer, 0);
>   	timer->expires = jiffies + msecs_to_jiffies(timesync_delay_ms);
>   	add_timer(timer);

Re: [PATCH 6/7] accel/qaic: Add config structs for supported cards

[Jeffrey Hugo]

On 12/13/2024 5:35 PM, Lizhi Hou wrote:
> 
> On 12/13/24 13:33, Jeffrey Hugo wrote:
>> -static struct qaic_device *create_qdev(struct pci_dev *pdev, const 
>> struct pci_device_id *id)
>> +static struct qaic_device *create_qdev(struct pci_dev *pdev,
>> +                       const struct qaic_device_config *config)
>>   {
>>       struct device *dev = &pdev->dev;
>>       struct qaic_drm_device *qddev;
>> @@ -365,12 +391,10 @@ static struct qaic_device *create_qdev(struct 
>> pci_dev *pdev, const struct pci_de
>>           return NULL;
>>       qdev->dev_state = QAIC_OFFLINE;
>> -    if (id->device == PCI_DEV_AIC080 || id->device == PCI_DEV_AIC100) {
>> -        qdev->num_dbc = 16;
>> -        qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, 
>> sizeof(*qdev->dbc), GFP_KERNEL);
>> -        if (!qdev->dbc)
>> -            return NULL;
>> -    }
>> +    qdev->num_dbc = 16;
> 
> Is it better to put num_dbc in qaic_device_config?

I think there is no clear "right answer".  All known devices use 16. 
There may be a future device which has a different value, at which point 
I think this needs to be in qaic_device_config.  For this patch, I was 
conservative and only included items in qaic_device_config which do vary 
between the known devices.

I'll think in this a bit more.

> 
> Thanks,
> 
> Lizhi
> 

Re: [PATCH 6/7] accel/qaic: Add config structs for supported cards

[Lizhi Hou]

On 12/20/24 09:15, Jeffrey Hugo wrote:
> On 12/13/2024 5:35 PM, Lizhi Hou wrote:
>>
>> On 12/13/24 13:33, Jeffrey Hugo wrote:
>>> -static struct qaic_device *create_qdev(struct pci_dev *pdev, const 
>>> struct pci_device_id *id)
>>> +static struct qaic_device *create_qdev(struct pci_dev *pdev,
>>> +                       const struct qaic_device_config *config)
>>>   {
>>>       struct device *dev = &pdev->dev;
>>>       struct qaic_drm_device *qddev;
>>> @@ -365,12 +391,10 @@ static struct qaic_device *create_qdev(struct 
>>> pci_dev *pdev, const struct pci_de
>>>           return NULL;
>>>       qdev->dev_state = QAIC_OFFLINE;
>>> -    if (id->device == PCI_DEV_AIC080 || id->device == 
>>> PCI_DEV_AIC100) {
>>> -        qdev->num_dbc = 16;
>>> -        qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, 
>>> sizeof(*qdev->dbc), GFP_KERNEL);
>>> -        if (!qdev->dbc)
>>> -            return NULL;
>>> -    }
>>> +    qdev->num_dbc = 16;
>>
>> Is it better to put num_dbc in qaic_device_config?
>
> I think there is no clear "right answer".  All known devices use 16. 
> There may be a future device which has a different value, at which 
> point I think this needs to be in qaic_device_config.  For this patch, 
> I was conservative and only included items in qaic_device_config which 
> do vary between the known devices.
>
> I'll think in this a bit more.
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>
>
>>
>> Thanks,
>>
>> Lizhi
>>

[PATCH 7/7] accel/qaic: Add AIC200 support

[Jeffrey Hugo]

Add basic support for the new AIC200 product. The PCIe Device ID is
0xa110. With this, we can turn on the lights for AIC200 by leveraging
much of the existing driver.

Co-developed-by: Youssef Samir <quic_yabdulra@quicinc.com>
Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 drivers/accel/qaic/mhi_controller.c | 360 ++++++++++++++++++++++++++--
 drivers/accel/qaic/mhi_controller.h |   2 +-
 drivers/accel/qaic/qaic.h           |   1 +
 drivers/accel/qaic/qaic_drv.c       |  11 +-
 drivers/accel/qaic/sahara.c         |  39 ++-
 5 files changed, 395 insertions(+), 18 deletions(-)

diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
index 8ab82e78dd94..d68df2f6a7e6 100644
--- a/drivers/accel/qaic/mhi_controller.c
+++ b/drivers/accel/qaic/mhi_controller.c
@@ -20,6 +20,11 @@ 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 const char *fw_image_paths[FAMILY_MAX] = {
+	[FAMILY_AIC100] = "qcom/aic100/sbl.bin",
+	[FAMILY_AIC200] = "qcom/aic200/sbl.bin",
+};
+
 static const struct mhi_channel_config aic100_channels[] = {
 	{
 		.name = "QAIC_LOOPBACK",
@@ -439,6 +444,297 @@ static const struct mhi_channel_config aic100_channels[] = {
 	},
 };
 
+static const struct mhi_channel_config aic200_channels[] = {
+	{
+		.name = "QAIC_LOOPBACK",
+		.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,
+	},
+	{
+		.name = "QAIC_LOOPBACK",
+		.num = 1,
+		.num_elements = 32,
+		.local_elements = 0,
+		.event_ring = 0,
+		.dir = DMA_FROM_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,
+	},
+	{
+		.name = "QAIC_SAHARA",
+		.num = 2,
+		.num_elements = 32,
+		.local_elements = 0,
+		.event_ring = 0,
+		.dir = DMA_TO_DEVICE,
+		.ee_mask = MHI_CH_EE_SBL,
+		.pollcfg = 0,
+		.doorbell = MHI_DB_BRST_DISABLE,
+		.lpm_notify = false,
+		.offload_channel = false,
+		.doorbell_mode_switch = false,
+		.auto_queue = false,
+		.wake_capable = false,
+	},
+	{
+		.name = "QAIC_SAHARA",
+		.num = 3,
+		.num_elements = 32,
+		.local_elements = 0,
+		.event_ring = 0,
+		.dir = DMA_FROM_DEVICE,
+		.ee_mask = MHI_CH_EE_SBL,
+		.pollcfg = 0,
+		.doorbell = MHI_DB_BRST_DISABLE,
+		.lpm_notify = false,
+		.offload_channel = false,
+		.doorbell_mode_switch = false,
+		.auto_queue = false,
+		.wake_capable = false,
+	},
+	{
+		.name = "QAIC_SSR",
+		.num = 6,
+		.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,
+	},
+	{
+		.name = "QAIC_SSR",
+		.num = 7,
+		.num_elements = 32,
+		.local_elements = 0,
+		.event_ring = 0,
+		.dir = DMA_FROM_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,
+	},
+	{
+		.name = "QAIC_CONTROL",
+		.num = 10,
+		.num_elements = 128,
+		.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,
+	},
+	{
+		.name = "QAIC_CONTROL",
+		.num = 11,
+		.num_elements = 128,
+		.local_elements = 0,
+		.event_ring = 0,
+		.dir = DMA_FROM_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,
+	},
+	{
+		.name = "QAIC_LOGGING",
+		.num = 12,
+		.num_elements = 32,
+		.local_elements = 0,
+		.event_ring = 0,
+		.dir = DMA_TO_DEVICE,
+		.ee_mask = MHI_CH_EE_SBL,
+		.pollcfg = 0,
+		.doorbell = MHI_DB_BRST_DISABLE,
+		.lpm_notify = false,
+		.offload_channel = false,
+		.doorbell_mode_switch = false,
+		.auto_queue = false,
+		.wake_capable = false,
+	},
+	{
+		.name = "QAIC_LOGGING",
+		.num = 13,
+		.num_elements = 32,
+		.local_elements = 0,
+		.event_ring = 0,
+		.dir = DMA_FROM_DEVICE,
+		.ee_mask = MHI_CH_EE_SBL,
+		.pollcfg = 0,
+		.doorbell = MHI_DB_BRST_DISABLE,
+		.lpm_notify = false,
+		.offload_channel = false,
+		.doorbell_mode_switch = false,
+		.auto_queue = false,
+		.wake_capable = false,
+	},
+	{
+		.name = "QAIC_STATUS",
+		.num = 14,
+		.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,
+	},
+	{
+		.name = "QAIC_STATUS",
+		.num = 15,
+		.num_elements = 32,
+		.local_elements = 0,
+		.event_ring = 0,
+		.dir = DMA_FROM_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,
+	},
+	{
+		.name = "QAIC_TELEMETRY",
+		.num = 16,
+		.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,
+	},
+	{
+		.name = "QAIC_TELEMETRY",
+		.num = 17,
+		.num_elements = 32,
+		.local_elements = 0,
+		.event_ring = 0,
+		.dir = DMA_FROM_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,
+	},
+	{
+		.name = "QAIC_TIMESYNC_PERIODIC",
+		.num = 22,
+		.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,
+	},
+	{
+		.name = "QAIC_TIMESYNC_PERIODIC",
+		.num = 23,
+		.num_elements = 32,
+		.local_elements = 0,
+		.event_ring = 0,
+		.dir = DMA_FROM_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,
+	},
+	{
+		.name = "IPCR",
+		.num = 24,
+		.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,
+	},
+	{
+		.name = "IPCR",
+		.num = 25,
+		.num_elements = 32,
+		.local_elements = 0,
+		.event_ring = 0,
+		.dir = DMA_FROM_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 = true,
+		.wake_capable = false,
+	},
+};
+
 static struct mhi_event_config aic100_events[] = {
 	{
 		.num_elements = 32,
@@ -454,16 +750,44 @@ static struct mhi_event_config aic100_events[] = {
 	},
 };
 
-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 struct mhi_event_config aic200_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,
+	},
+};
+
+static struct mhi_controller_config mhi_cntrl_configs[] = {
+	[FAMILY_AIC100] = {
+		.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,
+	},
+	[FAMILY_AIC200] = {
+		.max_channels = 128,
+		.timeout_ms = 0, /* controlled by mhi_timeout */
+		.buf_len = 0,
+		.num_channels = ARRAY_SIZE(aic200_channels),
+		.ch_cfg = aic200_channels,
+		.num_events = ARRAY_SIZE(aic200_events),
+		.event_cfg = aic200_events,
+		.use_bounce_buf = false,
+		.m2_no_db = false,
+	},
 };
 
 static int mhi_read_reg(struct mhi_controller *mhi_cntrl, void __iomem *addr, u32 *out)
@@ -545,8 +869,9 @@ static int mhi_reset_and_async_power_up(struct mhi_controller *mhi_cntrl)
 }
 
 struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, void __iomem *mhi_bar,
-						    int mhi_irq, bool shared_msi)
+						    int mhi_irq, bool shared_msi, int family)
 {
+	struct mhi_controller_config mhi_config = mhi_cntrl_configs[family];
 	struct mhi_controller *mhi_cntrl;
 	int ret;
 
@@ -573,6 +898,13 @@ struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi
 	mhi_cntrl->nr_irqs = 1;
 	mhi_cntrl->irq = devm_kmalloc(&pci_dev->dev, sizeof(*mhi_cntrl->irq), GFP_KERNEL);
 
+	if (family == FAMILY_AIC200) {
+		mhi_cntrl->name = "AIC200";
+		mhi_cntrl->seg_len = SZ_512K;
+	} else {
+		mhi_cntrl->name = "AIC100";
+	}
+
 	if (!mhi_cntrl->irq)
 		return ERR_PTR(-ENOMEM);
 
@@ -581,11 +913,11 @@ struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi
 	if (shared_msi) /* MSI shared with data path, no IRQF_NO_SUSPEND */
 		mhi_cntrl->irq_flags = IRQF_SHARED;
 
-	mhi_cntrl->fw_image = "qcom/aic100/sbl.bin";
+	mhi_cntrl->fw_image = fw_image_paths[family];
 
 	/* use latest configured timeout */
-	aic100_config.timeout_ms = mhi_timeout_ms;
-	ret = mhi_register_controller(mhi_cntrl, &aic100_config);
+	mhi_config.timeout_ms = mhi_timeout_ms;
+	ret = mhi_register_controller(mhi_cntrl, &mhi_config);
 	if (ret) {
 		pci_err(pci_dev, "mhi_register_controller failed %d\n", ret);
 		return ERR_PTR(ret);
diff --git a/drivers/accel/qaic/mhi_controller.h b/drivers/accel/qaic/mhi_controller.h
index 500e7f4af2af..8939f6ae185e 100644
--- a/drivers/accel/qaic/mhi_controller.h
+++ b/drivers/accel/qaic/mhi_controller.h
@@ -8,7 +8,7 @@
 #define MHICONTROLLERQAIC_H_
 
 struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, void __iomem *mhi_bar,
-						    int mhi_irq, bool shared_msi);
+						    int mhi_irq, bool shared_msi, int family);
 void qaic_mhi_free_controller(struct mhi_controller *mhi_cntrl, bool link_up);
 void qaic_mhi_start_reset(struct mhi_controller *mhi_cntrl);
 void qaic_mhi_reset_done(struct mhi_controller *mhi_cntrl);
diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
index cf97fd9a7e70..0dbb8e32e4b9 100644
--- a/drivers/accel/qaic/qaic.h
+++ b/drivers/accel/qaic/qaic.h
@@ -34,6 +34,7 @@
 
 enum aic_families {
 	FAMILY_AIC100,
+	FAMILY_AIC200,
 	FAMILY_MAX,
 };
 
diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index 4e63e475b389..3b415e2c9431 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -36,6 +36,7 @@ MODULE_IMPORT_NS("DMA_BUF");
 
 #define PCI_DEVICE_ID_QCOM_AIC080	0xa080
 #define PCI_DEVICE_ID_QCOM_AIC100	0xa100
+#define PCI_DEVICE_ID_QCOM_AIC200	0xa110
 #define QAIC_NAME			"qaic"
 #define QAIC_DESC			"Qualcomm Cloud AI Accelerators"
 #define CNTL_MAJOR			5
@@ -66,6 +67,13 @@ static const struct qaic_device_config aic100_config = {
 	.dbc_bar_idx = 2,
 };
 
+static const struct qaic_device_config aic200_config = {
+	.family = FAMILY_AIC200,
+	.bar_mask = BIT(0) | BIT(1) | BIT(2) | BIT(4),
+	.mhi_bar_idx = 1,
+	.dbc_bar_idx = 2,
+};
+
 bool datapath_polling;
 module_param(datapath_polling, bool, 0400);
 MODULE_PARM_DESC(datapath_polling, "Operate the datapath in polling mode");
@@ -568,7 +576,7 @@ static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return ret;
 
 	qdev->mhi_cntrl = qaic_mhi_register_controller(pdev, qdev->bar_mhi, mhi_irq,
-						       qdev->single_msi);
+						       qdev->single_msi, config->family);
 	if (IS_ERR(qdev->mhi_cntrl)) {
 		ret = PTR_ERR(qdev->mhi_cntrl);
 		qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
@@ -637,6 +645,7 @@ static struct mhi_driver qaic_mhi_driver = {
 static const struct pci_device_id qaic_ids[] = {
 	{ PCI_DEVICE_DATA(QCOM, AIC080, (kernel_ulong_t)&aic080_config), },
 	{ PCI_DEVICE_DATA(QCOM, AIC100, (kernel_ulong_t)&aic100_config), },
+	{ PCI_DEVICE_DATA(QCOM, AIC200, (kernel_ulong_t)&aic200_config), },
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, qaic_ids);
diff --git a/drivers/accel/qaic/sahara.c b/drivers/accel/qaic/sahara.c
index 09c8b055aa81..3ebcc1f7ff58 100644
--- a/drivers/accel/qaic/sahara.c
+++ b/drivers/accel/qaic/sahara.c
@@ -188,6 +188,34 @@ static const char * const aic100_image_table[] = {
 	[10] = "qcom/aic100/fw10.bin",
 };
 
+static const char * const aic200_image_table[] = {
+	[5]  = "qcom/aic200/uefi.elf",
+	[12] = "qcom/aic200/aic200-nsp.bin",
+	[23] = "qcom/aic200/aop.mbn",
+	[32] = "qcom/aic200/tz.mbn",
+	[33] = "qcom/aic200/hypvm.mbn",
+	[39] = "qcom/aic200/aic200_abl.elf",
+	[40] = "qcom/aic200/apdp.mbn",
+	[41] = "qcom/aic200/devcfg.mbn",
+	[42] = "qcom/aic200/sec.elf",
+	[43] = "qcom/aic200/aic200-hlos.elf",
+	[49] = "qcom/aic200/shrm.elf",
+	[50] = "qcom/aic200/cpucp.elf",
+	[51] = "qcom/aic200/aop_devcfg.mbn",
+	[57] = "qcom/aic200/cpucp_dtbs.elf",
+	[62] = "qcom/aic200/uefi_dtbs.elf",
+	[63] = "qcom/aic200/xbl_ac_config.mbn",
+	[64] = "qcom/aic200/tz_ac_config.mbn",
+	[65] = "qcom/aic200/hyp_ac_config.mbn",
+	[66] = "qcom/aic200/pdp.elf",
+	[67] = "qcom/aic200/pdp_cdb.elf",
+	[68] = "qcom/aic200/sdi.mbn",
+	[69] = "qcom/aic200/dcd.mbn",
+	[73] = "qcom/aic200/gearvm.mbn",
+	[74] = "qcom/aic200/sti.bin",
+	[75] = "qcom/aic200/pvs.bin",
+};
+
 static int sahara_find_image(struct sahara_context *context, u32 image_id)
 {
 	int ret;
@@ -748,8 +776,15 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
 	context->mhi_dev = mhi_dev;
 	INIT_WORK(&context->fw_work, sahara_processing);
 	INIT_WORK(&context->dump_work, sahara_dump_processing);
-	context->image_table = aic100_image_table;
-	context->table_size = ARRAY_SIZE(aic100_image_table);
+
+	if (!strcmp(mhi_dev->mhi_cntrl->name, "AIC200")) {
+		context->image_table = aic200_image_table;
+		context->table_size = ARRAY_SIZE(aic200_image_table);
+	} else {
+		context->image_table = aic100_image_table;
+		context->table_size = ARRAY_SIZE(aic100_image_table);
+	}
+
 	context->active_image_id = SAHARA_IMAGE_ID_NONE;
 	dev_set_drvdata(&mhi_dev->dev, context);
 
-- 
2.34.1

Re: [PATCH 7/7] accel/qaic: Add AIC200 support

[Lizhi Hou]

On 12/13/24 13:33, Jeffrey Hugo wrote:
> Add basic support for the new AIC200 product. The PCIe Device ID is
> 0xa110. With this, we can turn on the lights for AIC200 by leveraging
> much of the existing driver.
>
> Co-developed-by: Youssef Samir <quic_yabdulra@quicinc.com>
> Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>   drivers/accel/qaic/mhi_controller.c | 360 ++++++++++++++++++++++++++--
>   drivers/accel/qaic/mhi_controller.h |   2 +-
>   drivers/accel/qaic/qaic.h           |   1 +
>   drivers/accel/qaic/qaic_drv.c       |  11 +-
>   drivers/accel/qaic/sahara.c         |  39 ++-
>   5 files changed, 395 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
> index 8ab82e78dd94..d68df2f6a7e6 100644
> --- a/drivers/accel/qaic/mhi_controller.c
> +++ b/drivers/accel/qaic/mhi_controller.c
> @@ -20,6 +20,11 @@ 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 const char *fw_image_paths[FAMILY_MAX] = {
> +	[FAMILY_AIC100] = "qcom/aic100/sbl.bin",
> +	[FAMILY_AIC200] = "qcom/aic200/sbl.bin",
> +};
> +
>   static const struct mhi_channel_config aic100_channels[] = {
>   	{
>   		.name = "QAIC_LOOPBACK",
> @@ -439,6 +444,297 @@ static const struct mhi_channel_config aic100_channels[] = {
>   	},
>   };
>   
> +static const struct mhi_channel_config aic200_channels[] = {
> +	{
> +		.name = "QAIC_LOOPBACK",
> +		.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,
> +	},
> +	{
> +		.name = "QAIC_LOOPBACK",
> +		.num = 1,
> +		.num_elements = 32,
> +		.local_elements = 0,
> +		.event_ring = 0,
> +		.dir = DMA_FROM_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,
> +	},
> +	{
> +		.name = "QAIC_SAHARA",
> +		.num = 2,
> +		.num_elements = 32,
> +		.local_elements = 0,
> +		.event_ring = 0,
> +		.dir = DMA_TO_DEVICE,
> +		.ee_mask = MHI_CH_EE_SBL,
> +		.pollcfg = 0,
> +		.doorbell = MHI_DB_BRST_DISABLE,
> +		.lpm_notify = false,
> +		.offload_channel = false,
> +		.doorbell_mode_switch = false,
> +		.auto_queue = false,
> +		.wake_capable = false,
> +	},
> +	{
> +		.name = "QAIC_SAHARA",
> +		.num = 3,
> +		.num_elements = 32,
> +		.local_elements = 0,
> +		.event_ring = 0,
> +		.dir = DMA_FROM_DEVICE,
> +		.ee_mask = MHI_CH_EE_SBL,
> +		.pollcfg = 0,
> +		.doorbell = MHI_DB_BRST_DISABLE,
> +		.lpm_notify = false,
> +		.offload_channel = false,
> +		.doorbell_mode_switch = false,
> +		.auto_queue = false,
> +		.wake_capable = false,
> +	},
> +	{
> +		.name = "QAIC_SSR",
> +		.num = 6,
> +		.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,
> +	},
> +	{
> +		.name = "QAIC_SSR",
> +		.num = 7,
> +		.num_elements = 32,
> +		.local_elements = 0,
> +		.event_ring = 0,
> +		.dir = DMA_FROM_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,
> +	},
> +	{
> +		.name = "QAIC_CONTROL",
> +		.num = 10,
> +		.num_elements = 128,
> +		.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,
> +	},
> +	{
> +		.name = "QAIC_CONTROL",
> +		.num = 11,
> +		.num_elements = 128,
> +		.local_elements = 0,
> +		.event_ring = 0,
> +		.dir = DMA_FROM_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,
> +	},
> +	{
> +		.name = "QAIC_LOGGING",
> +		.num = 12,
> +		.num_elements = 32,
> +		.local_elements = 0,
> +		.event_ring = 0,
> +		.dir = DMA_TO_DEVICE,
> +		.ee_mask = MHI_CH_EE_SBL,
> +		.pollcfg = 0,
> +		.doorbell = MHI_DB_BRST_DISABLE,
> +		.lpm_notify = false,
> +		.offload_channel = false,
> +		.doorbell_mode_switch = false,
> +		.auto_queue = false,
> +		.wake_capable = false,
> +	},
> +	{
> +		.name = "QAIC_LOGGING",
> +		.num = 13,
> +		.num_elements = 32,
> +		.local_elements = 0,
> +		.event_ring = 0,
> +		.dir = DMA_FROM_DEVICE,
> +		.ee_mask = MHI_CH_EE_SBL,
> +		.pollcfg = 0,
> +		.doorbell = MHI_DB_BRST_DISABLE,
> +		.lpm_notify = false,
> +		.offload_channel = false,
> +		.doorbell_mode_switch = false,
> +		.auto_queue = false,
> +		.wake_capable = false,
> +	},
> +	{
> +		.name = "QAIC_STATUS",
> +		.num = 14,
> +		.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,
> +	},
> +	{
> +		.name = "QAIC_STATUS",
> +		.num = 15,
> +		.num_elements = 32,
> +		.local_elements = 0,
> +		.event_ring = 0,
> +		.dir = DMA_FROM_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,
> +	},
> +	{
> +		.name = "QAIC_TELEMETRY",
> +		.num = 16,
> +		.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,
> +	},
> +	{
> +		.name = "QAIC_TELEMETRY",
> +		.num = 17,
> +		.num_elements = 32,
> +		.local_elements = 0,
> +		.event_ring = 0,
> +		.dir = DMA_FROM_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,
> +	},
> +	{
> +		.name = "QAIC_TIMESYNC_PERIODIC",
> +		.num = 22,
> +		.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,
> +	},
> +	{
> +		.name = "QAIC_TIMESYNC_PERIODIC",
> +		.num = 23,
> +		.num_elements = 32,
> +		.local_elements = 0,
> +		.event_ring = 0,
> +		.dir = DMA_FROM_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,
> +	},
> +	{
> +		.name = "IPCR",
> +		.num = 24,
> +		.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,
> +	},
> +	{
> +		.name = "IPCR",
> +		.num = 25,
> +		.num_elements = 32,
> +		.local_elements = 0,
> +		.event_ring = 0,
> +		.dir = DMA_FROM_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 = true,
> +		.wake_capable = false,
> +	},
> +};
> +
>   static struct mhi_event_config aic100_events[] = {
>   	{
>   		.num_elements = 32,
> @@ -454,16 +750,44 @@ static struct mhi_event_config aic100_events[] = {
>   	},
>   };
>   
> -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 struct mhi_event_config aic200_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,
> +	},
> +};
> +
> +static struct mhi_controller_config mhi_cntrl_configs[] = {
> +	[FAMILY_AIC100] = {
> +		.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,
> +	},
> +	[FAMILY_AIC200] = {
> +		.max_channels = 128,
> +		.timeout_ms = 0, /* controlled by mhi_timeout */
> +		.buf_len = 0,
> +		.num_channels = ARRAY_SIZE(aic200_channels),
> +		.ch_cfg = aic200_channels,
> +		.num_events = ARRAY_SIZE(aic200_events),
> +		.event_cfg = aic200_events,
> +		.use_bounce_buf = false,
> +		.m2_no_db = false,
> +	},
>   };
>   
>   static int mhi_read_reg(struct mhi_controller *mhi_cntrl, void __iomem *addr, u32 *out)
> @@ -545,8 +869,9 @@ static int mhi_reset_and_async_power_up(struct mhi_controller *mhi_cntrl)
>   }
>   
>   struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, void __iomem *mhi_bar,
> -						    int mhi_irq, bool shared_msi)
> +						    int mhi_irq, bool shared_msi, int family)
>   {
> +	struct mhi_controller_config mhi_config = mhi_cntrl_configs[family];
>   	struct mhi_controller *mhi_cntrl;
>   	int ret;
>   
> @@ -573,6 +898,13 @@ struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi
>   	mhi_cntrl->nr_irqs = 1;
>   	mhi_cntrl->irq = devm_kmalloc(&pci_dev->dev, sizeof(*mhi_cntrl->irq), GFP_KERNEL);
>   
> +	if (family == FAMILY_AIC200) {
> +		mhi_cntrl->name = "AIC200";
> +		mhi_cntrl->seg_len = SZ_512K;
> +	} else {
> +		mhi_cntrl->name = "AIC100";
> +	}
> +

Only AIC200 needs to set 'seg_len'? Maybe these hard coded settings can 
also be in qaic_device_config?

It might be better to move after the following check at least.

>   	if (!mhi_cntrl->irq)
>   		return ERR_PTR(-ENOMEM);
>   
> @@ -581,11 +913,11 @@ struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi
>   	if (shared_msi) /* MSI shared with data path, no IRQF_NO_SUSPEND */
>   		mhi_cntrl->irq_flags = IRQF_SHARED;
>   
> -	mhi_cntrl->fw_image = "qcom/aic100/sbl.bin";
> +	mhi_cntrl->fw_image = fw_image_paths[family];
Maybe fw_image path in qaic_device_config?
>   
>   	/* use latest configured timeout */
> -	aic100_config.timeout_ms = mhi_timeout_ms;
> -	ret = mhi_register_controller(mhi_cntrl, &aic100_config);
> +	mhi_config.timeout_ms = mhi_timeout_ms;
> +	ret = mhi_register_controller(mhi_cntrl, &mhi_config);
>   	if (ret) {
>   		pci_err(pci_dev, "mhi_register_controller failed %d\n", ret);
>   		return ERR_PTR(ret);
> diff --git a/drivers/accel/qaic/mhi_controller.h b/drivers/accel/qaic/mhi_controller.h
> index 500e7f4af2af..8939f6ae185e 100644
> --- a/drivers/accel/qaic/mhi_controller.h
> +++ b/drivers/accel/qaic/mhi_controller.h
> @@ -8,7 +8,7 @@
>   #define MHICONTROLLERQAIC_H_
>   
>   struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, void __iomem *mhi_bar,
> -						    int mhi_irq, bool shared_msi);
> +						    int mhi_irq, bool shared_msi, int family);
>   void qaic_mhi_free_controller(struct mhi_controller *mhi_cntrl, bool link_up);
>   void qaic_mhi_start_reset(struct mhi_controller *mhi_cntrl);
>   void qaic_mhi_reset_done(struct mhi_controller *mhi_cntrl);
> diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
> index cf97fd9a7e70..0dbb8e32e4b9 100644
> --- a/drivers/accel/qaic/qaic.h
> +++ b/drivers/accel/qaic/qaic.h
> @@ -34,6 +34,7 @@
>   
>   enum aic_families {
>   	FAMILY_AIC100,
> +	FAMILY_AIC200,
>   	FAMILY_MAX,
>   };
>   
> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index 4e63e475b389..3b415e2c9431 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -36,6 +36,7 @@ MODULE_IMPORT_NS("DMA_BUF");
>   
>   #define PCI_DEVICE_ID_QCOM_AIC080	0xa080
>   #define PCI_DEVICE_ID_QCOM_AIC100	0xa100
> +#define PCI_DEVICE_ID_QCOM_AIC200	0xa110
>   #define QAIC_NAME			"qaic"
>   #define QAIC_DESC			"Qualcomm Cloud AI Accelerators"
>   #define CNTL_MAJOR			5
> @@ -66,6 +67,13 @@ static const struct qaic_device_config aic100_config = {
>   	.dbc_bar_idx = 2,
>   };
>   
> +static const struct qaic_device_config aic200_config = {
> +	.family = FAMILY_AIC200,
> +	.bar_mask = BIT(0) | BIT(1) | BIT(2) | BIT(4),

Will this pass the BAR mask check in init_pci()?

Thanks,

Lizhi

> +	.mhi_bar_idx = 1,
> +	.dbc_bar_idx = 2,
> +};
> +
>   bool datapath_polling;
>   module_param(datapath_polling, bool, 0400);
>   MODULE_PARM_DESC(datapath_polling, "Operate the datapath in polling mode");
> @@ -568,7 +576,7 @@ static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   		return ret;
>   
>   	qdev->mhi_cntrl = qaic_mhi_register_controller(pdev, qdev->bar_mhi, mhi_irq,
> -						       qdev->single_msi);
> +						       qdev->single_msi, config->family);
>   	if (IS_ERR(qdev->mhi_cntrl)) {
>   		ret = PTR_ERR(qdev->mhi_cntrl);
>   		qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
> @@ -637,6 +645,7 @@ static struct mhi_driver qaic_mhi_driver = {
>   static const struct pci_device_id qaic_ids[] = {
>   	{ PCI_DEVICE_DATA(QCOM, AIC080, (kernel_ulong_t)&aic080_config), },
>   	{ PCI_DEVICE_DATA(QCOM, AIC100, (kernel_ulong_t)&aic100_config), },
> +	{ PCI_DEVICE_DATA(QCOM, AIC200, (kernel_ulong_t)&aic200_config), },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(pci, qaic_ids);
> diff --git a/drivers/accel/qaic/sahara.c b/drivers/accel/qaic/sahara.c
> index 09c8b055aa81..3ebcc1f7ff58 100644
> --- a/drivers/accel/qaic/sahara.c
> +++ b/drivers/accel/qaic/sahara.c
> @@ -188,6 +188,34 @@ static const char * const aic100_image_table[] = {
>   	[10] = "qcom/aic100/fw10.bin",
>   };
>   
> +static const char * const aic200_image_table[] = {
> +	[5]  = "qcom/aic200/uefi.elf",
> +	[12] = "qcom/aic200/aic200-nsp.bin",
> +	[23] = "qcom/aic200/aop.mbn",
> +	[32] = "qcom/aic200/tz.mbn",
> +	[33] = "qcom/aic200/hypvm.mbn",
> +	[39] = "qcom/aic200/aic200_abl.elf",
> +	[40] = "qcom/aic200/apdp.mbn",
> +	[41] = "qcom/aic200/devcfg.mbn",
> +	[42] = "qcom/aic200/sec.elf",
> +	[43] = "qcom/aic200/aic200-hlos.elf",
> +	[49] = "qcom/aic200/shrm.elf",
> +	[50] = "qcom/aic200/cpucp.elf",
> +	[51] = "qcom/aic200/aop_devcfg.mbn",
> +	[57] = "qcom/aic200/cpucp_dtbs.elf",
> +	[62] = "qcom/aic200/uefi_dtbs.elf",
> +	[63] = "qcom/aic200/xbl_ac_config.mbn",
> +	[64] = "qcom/aic200/tz_ac_config.mbn",
> +	[65] = "qcom/aic200/hyp_ac_config.mbn",
> +	[66] = "qcom/aic200/pdp.elf",
> +	[67] = "qcom/aic200/pdp_cdb.elf",
> +	[68] = "qcom/aic200/sdi.mbn",
> +	[69] = "qcom/aic200/dcd.mbn",
> +	[73] = "qcom/aic200/gearvm.mbn",
> +	[74] = "qcom/aic200/sti.bin",
> +	[75] = "qcom/aic200/pvs.bin",
> +};
> +
>   static int sahara_find_image(struct sahara_context *context, u32 image_id)
>   {
>   	int ret;
> @@ -748,8 +776,15 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
>   	context->mhi_dev = mhi_dev;
>   	INIT_WORK(&context->fw_work, sahara_processing);
>   	INIT_WORK(&context->dump_work, sahara_dump_processing);
> -	context->image_table = aic100_image_table;
> -	context->table_size = ARRAY_SIZE(aic100_image_table);
> +
> +	if (!strcmp(mhi_dev->mhi_cntrl->name, "AIC200")) {
> +		context->image_table = aic200_image_table;
> +		context->table_size = ARRAY_SIZE(aic200_image_table);
> +	} else {
> +		context->image_table = aic100_image_table;
> +		context->table_size = ARRAY_SIZE(aic100_image_table);
> +	}
> +
>   	context->active_image_id = SAHARA_IMAGE_ID_NONE;
>   	dev_set_drvdata(&mhi_dev->dev, context);
>   

Re: [PATCH 7/7] accel/qaic: Add AIC200 support

[Jeffrey Hugo]

On 12/13/2024 5:49 PM, Lizhi Hou wrote:
> 
> On 12/13/24 13:33, Jeffrey Hugo wrote:
>> @@ -573,6 +898,13 @@ struct mhi_controller 
>> *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi
>>       mhi_cntrl->nr_irqs = 1;
>>       mhi_cntrl->irq = devm_kmalloc(&pci_dev->dev, 
>> sizeof(*mhi_cntrl->irq), GFP_KERNEL);
>> +    if (family == FAMILY_AIC200) {
>> +        mhi_cntrl->name = "AIC200";
>> +        mhi_cntrl->seg_len = SZ_512K;
>> +    } else {
>> +        mhi_cntrl->name = "AIC100";
>> +    }
>> +
> 
> Only AIC200 needs to set 'seg_len'? Maybe these hard coded settings can 
> also be in qaic_device_config?

Yes, seg_len is related to the BHIe loading, which is new from AIC100 to 
AIC200.

For the moment, I think I'd like to keep the MHI details "encapsulated" 
within this portion of the driver.  With the continuing development of 
AIC200, I'm expecting a bit of volitility here which I hope doesn't leak 
into the rest of the driver.

> It might be better to move after the following check at least.

I agree.

> 
>>       if (!mhi_cntrl->irq)
>>           return ERR_PTR(-ENOMEM);
>> @@ -581,11 +913,11 @@ struct mhi_controller 
>> *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi
>>       if (shared_msi) /* MSI shared with data path, no IRQF_NO_SUSPEND */
>>           mhi_cntrl->irq_flags = IRQF_SHARED;
>> -    mhi_cntrl->fw_image = "qcom/aic100/sbl.bin";
>> +    mhi_cntrl->fw_image = fw_image_paths[family];
> Maybe fw_image path in qaic_device_config?
>>       /* use latest configured timeout */
>> -    aic100_config.timeout_ms = mhi_timeout_ms;
>> -    ret = mhi_register_controller(mhi_cntrl, &aic100_config);
>> +    mhi_config.timeout_ms = mhi_timeout_ms;
>> +    ret = mhi_register_controller(mhi_cntrl, &mhi_config);
>>       if (ret) {
>>           pci_err(pci_dev, "mhi_register_controller failed %d\n", ret);
>>           return ERR_PTR(ret);
>> diff --git a/drivers/accel/qaic/mhi_controller.h 
>> b/drivers/accel/qaic/mhi_controller.h
>> index 500e7f4af2af..8939f6ae185e 100644
>> --- a/drivers/accel/qaic/mhi_controller.h
>> +++ b/drivers/accel/qaic/mhi_controller.h
>> @@ -8,7 +8,7 @@
>>   #define MHICONTROLLERQAIC_H_
>>   struct mhi_controller *qaic_mhi_register_controller(struct pci_dev 
>> *pci_dev, void __iomem *mhi_bar,
>> -                            int mhi_irq, bool shared_msi);
>> +                            int mhi_irq, bool shared_msi, int family);
>>   void qaic_mhi_free_controller(struct mhi_controller *mhi_cntrl, bool 
>> link_up);
>>   void qaic_mhi_start_reset(struct mhi_controller *mhi_cntrl);
>>   void qaic_mhi_reset_done(struct mhi_controller *mhi_cntrl);
>> diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
>> index cf97fd9a7e70..0dbb8e32e4b9 100644
>> --- a/drivers/accel/qaic/qaic.h
>> +++ b/drivers/accel/qaic/qaic.h
>> @@ -34,6 +34,7 @@
>>   enum aic_families {
>>       FAMILY_AIC100,
>> +    FAMILY_AIC200,
>>       FAMILY_MAX,
>>   };
>> diff --git a/drivers/accel/qaic/qaic_drv.c 
>> b/drivers/accel/qaic/qaic_drv.c
>> index 4e63e475b389..3b415e2c9431 100644
>> --- a/drivers/accel/qaic/qaic_drv.c
>> +++ b/drivers/accel/qaic/qaic_drv.c
>> @@ -36,6 +36,7 @@ MODULE_IMPORT_NS("DMA_BUF");
>>   #define PCI_DEVICE_ID_QCOM_AIC080    0xa080
>>   #define PCI_DEVICE_ID_QCOM_AIC100    0xa100
>> +#define PCI_DEVICE_ID_QCOM_AIC200    0xa110
>>   #define QAIC_NAME            "qaic"
>>   #define QAIC_DESC            "Qualcomm Cloud AI Accelerators"
>>   #define CNTL_MAJOR            5
>> @@ -66,6 +67,13 @@ static const struct qaic_device_config 
>> aic100_config = {
>>       .dbc_bar_idx = 2,
>>   };
>> +static const struct qaic_device_config aic200_config = {
>> +    .family = FAMILY_AIC200,
>> +    .bar_mask = BIT(0) | BIT(1) | BIT(2) | BIT(4),
> 
> Will this pass the BAR mask check in init_pci()?

Yes, BITs 0, 1, 2, 4 would be 0x17 and that value is & with 0x3f 
(masking off upper bits).  The result would be 0x17.

> 
> Thanks,
> 
> Lizhi
> 

Re: [PATCH 7/7] accel/qaic: Add AIC200 support

[Lizhi Hou]

On 12/20/24 09:26, Jeffrey Hugo wrote:
> On 12/13/2024 5:49 PM, Lizhi Hou wrote:
>>
>> On 12/13/24 13:33, Jeffrey Hugo wrote:
>>> @@ -573,6 +898,13 @@ struct mhi_controller 
>>> *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi
>>>       mhi_cntrl->nr_irqs = 1;
>>>       mhi_cntrl->irq = devm_kmalloc(&pci_dev->dev, 
>>> sizeof(*mhi_cntrl->irq), GFP_KERNEL);
>>> +    if (family == FAMILY_AIC200) {
>>> +        mhi_cntrl->name = "AIC200";
>>> +        mhi_cntrl->seg_len = SZ_512K;
>>> +    } else {
>>> +        mhi_cntrl->name = "AIC100";
>>> +    }
>>> +
>>
>> Only AIC200 needs to set 'seg_len'? Maybe these hard coded settings 
>> can also be in qaic_device_config?
>
> Yes, seg_len is related to the BHIe loading, which is new from AIC100 
> to AIC200.
>
> For the moment, I think I'd like to keep the MHI details 
> "encapsulated" within this portion of the driver.  With the continuing 
> development of AIC200, I'm expecting a bit of volitility here which I 
> hope doesn't leak into the rest of the driver.
>
>> It might be better to move after the following check at least.
>
> I agree.
>
>>
>>>       if (!mhi_cntrl->irq)
>>>           return ERR_PTR(-ENOMEM);
>>> @@ -581,11 +913,11 @@ struct mhi_controller 
>>> *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi
>>>       if (shared_msi) /* MSI shared with data path, no 
>>> IRQF_NO_SUSPEND */
>>>           mhi_cntrl->irq_flags = IRQF_SHARED;
>>> -    mhi_cntrl->fw_image = "qcom/aic100/sbl.bin";
>>> +    mhi_cntrl->fw_image = fw_image_paths[family];
>> Maybe fw_image path in qaic_device_config?
>>>       /* use latest configured timeout */
>>> -    aic100_config.timeout_ms = mhi_timeout_ms;
>>> -    ret = mhi_register_controller(mhi_cntrl, &aic100_config);
>>> +    mhi_config.timeout_ms = mhi_timeout_ms;
>>> +    ret = mhi_register_controller(mhi_cntrl, &mhi_config);
>>>       if (ret) {
>>>           pci_err(pci_dev, "mhi_register_controller failed %d\n", ret);
>>>           return ERR_PTR(ret);
>>> diff --git a/drivers/accel/qaic/mhi_controller.h 
>>> b/drivers/accel/qaic/mhi_controller.h
>>> index 500e7f4af2af..8939f6ae185e 100644
>>> --- a/drivers/accel/qaic/mhi_controller.h
>>> +++ b/drivers/accel/qaic/mhi_controller.h
>>> @@ -8,7 +8,7 @@
>>>   #define MHICONTROLLERQAIC_H_
>>>   struct mhi_controller *qaic_mhi_register_controller(struct pci_dev 
>>> *pci_dev, void __iomem *mhi_bar,
>>> -                            int mhi_irq, bool shared_msi);
>>> +                            int mhi_irq, bool shared_msi, int family);
>>>   void qaic_mhi_free_controller(struct mhi_controller *mhi_cntrl, 
>>> bool link_up);
>>>   void qaic_mhi_start_reset(struct mhi_controller *mhi_cntrl);
>>>   void qaic_mhi_reset_done(struct mhi_controller *mhi_cntrl);
>>> diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
>>> index cf97fd9a7e70..0dbb8e32e4b9 100644
>>> --- a/drivers/accel/qaic/qaic.h
>>> +++ b/drivers/accel/qaic/qaic.h
>>> @@ -34,6 +34,7 @@
>>>   enum aic_families {
>>>       FAMILY_AIC100,
>>> +    FAMILY_AIC200,
>>>       FAMILY_MAX,
>>>   };
>>> diff --git a/drivers/accel/qaic/qaic_drv.c 
>>> b/drivers/accel/qaic/qaic_drv.c
>>> index 4e63e475b389..3b415e2c9431 100644
>>> --- a/drivers/accel/qaic/qaic_drv.c
>>> +++ b/drivers/accel/qaic/qaic_drv.c
>>> @@ -36,6 +36,7 @@ MODULE_IMPORT_NS("DMA_BUF");
>>>   #define PCI_DEVICE_ID_QCOM_AIC080    0xa080
>>>   #define PCI_DEVICE_ID_QCOM_AIC100    0xa100
>>> +#define PCI_DEVICE_ID_QCOM_AIC200    0xa110
>>>   #define QAIC_NAME            "qaic"
>>>   #define QAIC_DESC            "Qualcomm Cloud AI Accelerators"
>>>   #define CNTL_MAJOR            5
>>> @@ -66,6 +67,13 @@ static const struct qaic_device_config 
>>> aic100_config = {
>>>       .dbc_bar_idx = 2,
>>>   };
>>> +static const struct qaic_device_config aic200_config = {
>>> +    .family = FAMILY_AIC200,
>>> +    .bar_mask = BIT(0) | BIT(1) | BIT(2) | BIT(4),
>>
>> Will this pass the BAR mask check in init_pci()?
>
> Yes, BITs 0, 1, 2, 4 would be 0x17 and that value is & with 0x3f 
> (masking off upper bits).  The result would be 0x17.

It seems BIT(1) is not expected in init_pci?

     if (bars != (BIT(0) | BIT(2) | BIT(4))) {


Lizhi

>
>>
>> Thanks,
>>
>> Lizhi
>>

Re: [PATCH 7/7] accel/qaic: Add AIC200 support

[Jeffrey Hugo]

On 12/20/2024 10:33 AM, Lizhi Hou wrote:
> 
> On 12/20/24 09:26, Jeffrey Hugo wrote:
>> On 12/13/2024 5:49 PM, Lizhi Hou wrote:
>>>
>>> On 12/13/24 13:33, Jeffrey Hugo wrote:
>>>> +static const struct qaic_device_config aic200_config = {
>>>> +    .family = FAMILY_AIC200,
>>>> +    .bar_mask = BIT(0) | BIT(1) | BIT(2) | BIT(4),
>>>
>>> Will this pass the BAR mask check in init_pci()?
>>
>> Yes, BITs 0, 1, 2, 4 would be 0x17 and that value is & with 0x3f 
>> (masking off upper bits).  The result would be 0x17.
> 
> It seems BIT(1) is not expected in init_pci?
> 
>      if (bars != (BIT(0) | BIT(2) | BIT(4))) {

I think you are only referencing patch 5, when you should also reference 
patch 6.  This check gets modified in patch 6 -

-	if (bars != (BIT(0) | BIT(2) | BIT(4))) {
-		pci_dbg(pdev, "%s: expected BARs 0, 2, and 4 not found in device. 
Found 0x%x\n",
-			__func__, bars);
+	if (bars != config->bar_mask) {
+		pci_dbg(pdev, "%s: expected BARs %#x not found in device. Found %#x\n",
+			__func__, config->bar_mask, bars);
  		return -EINVAL;
  	}


Do you still see an issue?

Re: [PATCH 7/7] accel/qaic: Add AIC200 support

[Lizhi Hou]

On 12/20/24 09:50, Jeffrey Hugo wrote:
> On 12/20/2024 10:33 AM, Lizhi Hou wrote:
>>
>> On 12/20/24 09:26, Jeffrey Hugo wrote:
>>> On 12/13/2024 5:49 PM, Lizhi Hou wrote:
>>>>
>>>> On 12/13/24 13:33, Jeffrey Hugo wrote:
>>>>> +static const struct qaic_device_config aic200_config = {
>>>>> +    .family = FAMILY_AIC200,
>>>>> +    .bar_mask = BIT(0) | BIT(1) | BIT(2) | BIT(4),
>>>>
>>>> Will this pass the BAR mask check in init_pci()?
>>>
>>> Yes, BITs 0, 1, 2, 4 would be 0x17 and that value is & with 0x3f 
>>> (masking off upper bits).  The result would be 0x17.
>>
>> It seems BIT(1) is not expected in init_pci?
>>
>>      if (bars != (BIT(0) | BIT(2) | BIT(4))) {
>
> I think you are only referencing patch 5, when you should also 
> reference patch 6.  This check gets modified in patch 6 -
>
> -    if (bars != (BIT(0) | BIT(2) | BIT(4))) {
> -        pci_dbg(pdev, "%s: expected BARs 0, 2, and 4 not found in 
> device. Found 0x%x\n",
> -            __func__, bars);
> +    if (bars != config->bar_mask) {
> +        pci_dbg(pdev, "%s: expected BARs %#x not found in device. 
> Found %#x\n",
> +            __func__, config->bar_mask, bars);
>          return -EINVAL;
>      }
>
>
> Do you still see an issue?
No. :)
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>

Re: [PATCH 7/7] accel/qaic: Add AIC200 support

[kernel test robot]

Hi Jeffrey,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.13-rc4 next-20241220]
[cannot apply to mani-mhi/mhi-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jeffrey-Hugo/bus-mhi-host-Refactor-BHI-BHIe-based-firmware-loading/20241214-053540
base:   linus/master
patch link:    https://lore.kernel.org/r/20241213213340.2551697-8-quic_jhugo%40quicinc.com
patch subject: [PATCH 7/7] accel/qaic: Add AIC200 support
config: i386-randconfig-007-20241227 (https://download.01.org/0day-ci/archive/20241228/202412280859.rkIldm5t-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241228/202412280859.rkIldm5t-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412280859.rkIldm5t-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/accel/qaic/sahara.c:5:
   In file included from include/linux/devcoredump.h:12:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:2223:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/accel/qaic/sahara.c:781:24: error: assigning to 'const char **' from 'const char *const[76]' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
     781 |                 context->image_table = aic200_image_table;
         |                                      ^ ~~~~~~~~~~~~~~~~~~
   1 warning and 1 error generated.


vim +781 drivers/accel/qaic/sahara.c

   744	
   745	static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_id *id)
   746	{
   747		struct sahara_context *context;
   748		int ret;
   749		int i;
   750	
   751		context = devm_kzalloc(&mhi_dev->dev, sizeof(*context), GFP_KERNEL);
   752		if (!context)
   753			return -ENOMEM;
   754	
   755		context->rx = devm_kzalloc(&mhi_dev->dev, SAHARA_PACKET_MAX_SIZE, GFP_KERNEL);
   756		if (!context->rx)
   757			return -ENOMEM;
   758	
   759		/*
   760		 * AIC100 defines SAHARA_TRANSFER_MAX_SIZE as the largest value it
   761		 * will request for READ_DATA. This is larger than
   762		 * SAHARA_PACKET_MAX_SIZE, and we need 9x SAHARA_PACKET_MAX_SIZE to
   763		 * cover SAHARA_TRANSFER_MAX_SIZE. When the remote side issues a
   764		 * READ_DATA, it requires a transfer of the exact size requested. We
   765		 * can use MHI_CHAIN to link multiple buffers into a single transfer
   766		 * but the remote side will not consume the buffers until it sees an
   767		 * EOT, thus we need to allocate enough buffers to put in the tx fifo
   768		 * to cover an entire READ_DATA request of the max size.
   769		 */
   770		for (i = 0; i < SAHARA_NUM_TX_BUF; ++i) {
   771			context->tx[i] = devm_kzalloc(&mhi_dev->dev, SAHARA_PACKET_MAX_SIZE, GFP_KERNEL);
   772			if (!context->tx[i])
   773				return -ENOMEM;
   774		}
   775	
   776		context->mhi_dev = mhi_dev;
   777		INIT_WORK(&context->fw_work, sahara_processing);
   778		INIT_WORK(&context->dump_work, sahara_dump_processing);
   779	
   780		if (!strcmp(mhi_dev->mhi_cntrl->name, "AIC200")) {
 > 781			context->image_table = aic200_image_table;
   782			context->table_size = ARRAY_SIZE(aic200_image_table);
   783		} else {
   784			context->image_table = aic100_image_table;
   785			context->table_size = ARRAY_SIZE(aic100_image_table);
   786		}
   787	
   788		context->active_image_id = SAHARA_IMAGE_ID_NONE;
   789		dev_set_drvdata(&mhi_dev->dev, context);
   790	
   791		ret = mhi_prepare_for_transfer(mhi_dev);
   792		if (ret)
   793			return ret;
   794	
   795		ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, context->rx, SAHARA_PACKET_MAX_SIZE, MHI_EOT);
   796		if (ret) {
   797			mhi_unprepare_from_transfer(mhi_dev);
   798			return ret;
   799		}
   800	
   801		return 0;
   802	}
   803	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki