Re: [PATCH v1 04/12] drivers: accel: Register Qualcomm AIC specific image tables with Sahara.

[Kishore Batta <kishore.batta@oss.qualcomm.com>]

On 8/26/2025 3:08 AM, Bjorn Andersson wrote:
> On Mon, Aug 25, 2025 at 03:49:18PM +0530, Kishore Batta wrote:
>> Register Qualcomm AIC-specific image tables with the Sahara protocol.
>> The Sahara protocol provides a method for client drivers to register
>> device-specific image tables, which is mandatory for firmware transfer.
>> During QAIC device initialization, the QAIC driver must register the
>> image table information with the Sahara protocol for firmware transfer
>> to occur. Once the device is probed, it sends the required Sahara packets
>> to the host. Based on the connected device, Sahara selects the appropriate
>> image table and sends the firmware image data back to the device.
> This does describe things that is happening. But it doesn't describe the
> purpose of this patch.
ACK. In v2, i have rewritten it properly.
>> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
>> ---
>>   drivers/accel/qaic/mhi_controller.c | 57 +++++++++++++++++++++++++++--
>>   drivers/accel/qaic/mhi_controller.h |  2 +
>>   drivers/accel/qaic/qaic_drv.c       |  7 ++++
>>   drivers/accel/qaic/sahara.c         | 17 +++++----
>>   drivers/accel/qaic/sahara.h         |  6 ---
>>   5 files changed, 73 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
>> index 5cc7994f4809..16c346e0e3b5 100644
>> --- a/drivers/accel/qaic/mhi_controller.c
>> +++ b/drivers/accel/qaic/mhi_controller.c
>> @@ -13,6 +13,7 @@
>>   
>>   #include "mhi_controller.h"
>>   #include "qaic.h"
>> +#include "sahara_image_table_ops.h"
>>   
>>   #define MAX_RESET_TIME_SEC 25
>>   
>> @@ -801,8 +802,6 @@ const char * const aic100_image_table[] = {
>>   	[10] = "qcom/aic100/fw10.bin",
>>   };
>>   
>> -const size_t aic100_image_table_size = ARRAY_SIZE(aic100_image_table);
>> -
>>   const char * const aic200_image_table[] = {
>>   	[5]  = "qcom/aic200/uefi.elf",
>>   	[12] = "qcom/aic200/aic200-nsp.bin",
>> @@ -831,7 +830,59 @@ const char * const aic200_image_table[] = {
>>   	[75] = "qcom/aic200/pvs.bin",
>>   };
>>   
>> -const size_t aic200_image_table_size = ARRAY_SIZE(aic200_image_table);
>> +static struct sahara_image_table_provider aic100_provider = {
>> +	.image_table = aic100_image_table,
>> +	.image_table_size = ARRAY_SIZE(aic100_image_table),
>> +	.dev_name = "AIC100",
>> +	.fw_folder_name = "aic100",
>> +	.list = LIST_HEAD_INIT(aic100_provider.list)
>> +};
>> +
>> +static struct sahara_image_table_provider aic200_provider = {
>> +	.image_table = aic200_image_table,
>> +	.image_table_size = ARRAY_SIZE(aic200_image_table),
>> +	.dev_name = "AIC200",
>> +	.fw_folder_name = "aic200",
>> +	.list = LIST_HEAD_INIT(aic200_provider.list)
>> +};
>> +
>> +static struct sahara_image_table_provider *aic_providers[] = {
>> +	&aic100_provider,
>> +	&aic200_provider,
>> +};
>> +
>> +int qaic_sahara_register_image_tables(void)
>> +{
>> +	int ret;
>> +
>> +	for (int i = 0; i < ARRAY_SIZE(aic_providers); i++) {
>> +		ret = sahara_register_image_table_provider(aic_providers[i]);
>> +		if (ret) {
>> +			pr_err("qaic: Failed to register image table %d\n",
>> +			       ret);
>> +
>> +			/* Rollback previously registered providers */
>> +			while (--i >= 0)
>> +				sahara_unregister_image_table_provider(aic_providers[i]);
>> +
>> +			return ret;
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>> +void qaic_sahara_unregister_image_tables(void)
>> +{
>> +	int ret;
>> +
>> +	for (int i = 0; i < ARRAY_SIZE(aic_providers); i++) {
>> +		ret = sahara_unregister_image_table_provider(aic_providers[i]);
>> +		if (ret)
>> +			pr_err("qaic: Failed to unregister image table %d\n",
>> +			       ret);
>> +	}
>> +}
>> +
>>   
>>   static int mhi_read_reg(struct mhi_controller *mhi_cntrl, void __iomem *addr, u32 *out)
>>   {
>> diff --git a/drivers/accel/qaic/mhi_controller.h b/drivers/accel/qaic/mhi_controller.h
>> index 8939f6ae185e..90c0f07cbdf6 100644
>> --- a/drivers/accel/qaic/mhi_controller.h
>> +++ b/drivers/accel/qaic/mhi_controller.h
>> @@ -12,5 +12,7 @@ struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi
>>   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);
>> +int qaic_sahara_register_image_tables(void);
>> +void qaic_sahara_unregister_image_tables(void);
>>   
>>   #endif /* MHICONTROLLERQAIC_H_ */
>> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
>> index e31bcb0ecfc9..5c4fab328003 100644
>> --- a/drivers/accel/qaic/qaic_drv.c
>> +++ b/drivers/accel/qaic/qaic_drv.c
>> @@ -688,6 +688,12 @@ static int __init qaic_init(void)
>>   		goto free_mhi;
>>   	}
>>   
>> +	ret = qaic_sahara_register_image_tables();
> Now that you're doing this on a per-device basis (but actually per
> driver), could this somehow be done from qaic_mhi_register_controller()
> instead. So we don't run this code unless you actually have a QAIC
> attached?
I have removed registration mechanism in v2.
>> +	if (ret) {
>> +		pr_debug("qaic: Image table registration failed %d\n", ret);
> That's not a debug print...which is also the reason why you pr_err()
> inside the function. I.e. this is at best spamming the log.
Removed in v2.
>> +		goto free_mhi;
>> +	}
>> +
> Regards,
> Bjorn
>
>>   	ret = qaic_timesync_init();
>>   	if (ret)
>>   		pr_debug("qaic: qaic_timesync_init failed %d\n", ret);
>> @@ -727,6 +733,7 @@ static void __exit qaic_exit(void)
>>   	 * reinitializing the link_up state after the cleanup is done.
>>   	 */
>>   	link_up = true;
>> +	qaic_sahara_unregister_image_tables();
>>   	qaic_ras_unregister();
>>   	qaic_bootlog_unregister();
>>   	qaic_timesync_deinit();
>> diff --git a/drivers/accel/qaic/sahara.c b/drivers/accel/qaic/sahara.c
>> index cf8f8b585223..7eae329396be 100644
>> --- a/drivers/accel/qaic/sahara.c
>> +++ b/drivers/accel/qaic/sahara.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/workqueue.h>
>>   
>>   #include "sahara.h"
>> +#include "sahara_image_table_ops.h"
>>   
>>   #define SAHARA_HELLO_CMD		0x1  /* Min protocol version 1.0 */
>>   #define SAHARA_HELLO_RESP_CMD		0x2  /* Min protocol version 1.0 */
>> @@ -738,13 +739,15 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
>>   	INIT_WORK(&context->fw_work, sahara_processing);
>>   	INIT_WORK(&context->dump_work, sahara_dump_processing);
>>   
>> -	if (!strcmp(mhi_dev->mhi_cntrl->name, "AIC200")) {
>> -		context->image_table = aic200_image_table;
>> -		context->table_size = aic200_image_table_size;
>> -	} else {
>> -		context->image_table = aic100_image_table;
>> -		context->table_size = aic100_image_table_size;
>> -	}
>> +	/* Get the image table for a given device name */
>> +	context->image_table = sahara_get_image_table(mhi_dev->mhi_cntrl->name);
>> +	if (!context->image_table)
>> +		return -EINVAL;
>> +
>> +	/* Get the image table size for a given device name */
>> +	context->table_size = sahara_get_image_table_size(mhi_dev->mhi_cntrl->name);
>> +	if (!context->table_size)
>> +		return -EINVAL;
>>   
>>   	context->active_image_id = SAHARA_IMAGE_ID_NONE;
>>   	dev_set_drvdata(&mhi_dev->dev, context);
>> diff --git a/drivers/accel/qaic/sahara.h b/drivers/accel/qaic/sahara.h
>> index d7fd447ca85b..dde8c736d29e 100644
>> --- a/drivers/accel/qaic/sahara.h
>> +++ b/drivers/accel/qaic/sahara.h
>> @@ -8,10 +8,4 @@
>>   int sahara_register(void);
>>   void sahara_unregister(void);
>>   
>> -extern const char * const aic200_image_table[];
>> -extern const size_t aic200_image_table_size;
>> -
>> -extern const char * const aic100_image_table[];
>> -extern const size_t aic100_image_table_size;
>> -
>>   #endif /* __SAHARA_H__ */
>> -- 
>> 2.34.1
>>