Re: [PATCH v1 02/12] drivers: accel : Move AIC specific image tables to mhi_controller.c file

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

On 8/26/2025 2:38 AM, Bjorn Andersson wrote:
> On Mon, Aug 25, 2025 at 03:49:16PM +0530, Kishore Batta wrote:
>
> "git log --oneline -- drivers/accel/qaic/sahara.c" says that subject
> prefix should be "accel/qaic: "
>
>> Move the AIC-specific image tables from the Sahara driver to the AIC
>> specific controller file. This change prevents the Sahara driver from
>> being tagged to a specific Qualcomm device, making it easier to add
>> support for new devices with their own image tables.
> I don't have any concerns with moving the firmware mapping out of the
> sahara driver, but the implementation already supports two different
> devices...so it's not "tagged to a specific device".
>
> Also, while at it, please start your commit message with a problem
> statement and finish it with the technical description of the change
> you're doing.
Apologies for the delayed response. It took some time to sort out the 
issue with kernel_boot.elf image. Acknowledged. In v2, I'm no longer 
claiming the driver is tagged to a single device. Instead, the 
motivation is that the implementation lived under QAIC even though its 
an MHI protocol driver used by multiple devices. The driver will now be 
moved to drivers/bus/mhi and structured as a reusable MHI protocol 
driver. In v2, all commit messages are rewritten to begin with the 
problem being solved and then describe the technical approach/changes.
>> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
>> ---
>>   drivers/accel/qaic/mhi_controller.c | 43 +++++++++++++++++++++++++++++
>>   drivers/accel/qaic/sahara.c         | 43 ++---------------------------
>>   drivers/accel/qaic/sahara.h         |  7 +++++
>>   3 files changed, 52 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
>> index 13a14c6c6168..5cc7994f4809 100644
>> --- a/drivers/accel/qaic/mhi_controller.c
>> +++ b/drivers/accel/qaic/mhi_controller.c
>> @@ -790,6 +790,49 @@ static struct mhi_controller_config mhi_cntrl_configs[] = {
>>   	},
>>   };
>>   
>> +const char * const aic100_image_table[] = {
>> +	[1]  = "qcom/aic100/fw1.bin",
>> +	[2]  = "qcom/aic100/fw2.bin",
>> +	[4]  = "qcom/aic100/fw4.bin",
>> +	[5]  = "qcom/aic100/fw5.bin",
>> +	[6]  = "qcom/aic100/fw6.bin",
>> +	[8]  = "qcom/aic100/fw8.bin",
>> +	[9]  = "qcom/aic100/fw9.bin",
>> +	[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",
>> +	[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",
>> +};
>> +
>> +const size_t aic200_image_table_size = ARRAY_SIZE(aic200_image_table);
>> +
>>   static int mhi_read_reg(struct mhi_controller *mhi_cntrl, void __iomem *addr, u32 *out)
>>   {
>>   	u32 tmp;
>> diff --git a/drivers/accel/qaic/sahara.c b/drivers/accel/qaic/sahara.c
>> index 3ebcc1f7ff58..cf8f8b585223 100644
>> --- a/drivers/accel/qaic/sahara.c
>> +++ b/drivers/accel/qaic/sahara.c
>> @@ -177,45 +177,6 @@ struct sahara_context {
>>   	bool				is_mem_dump_mode;
>>   };
>>   
>> -static const char * const aic100_image_table[] = {
>> -	[1]  = "qcom/aic100/fw1.bin",
>> -	[2]  = "qcom/aic100/fw2.bin",
>> -	[4]  = "qcom/aic100/fw4.bin",
>> -	[5]  = "qcom/aic100/fw5.bin",
>> -	[6]  = "qcom/aic100/fw6.bin",
>> -	[8]  = "qcom/aic100/fw8.bin",
>> -	[9]  = "qcom/aic100/fw9.bin",
>> -	[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;
>> @@ -779,10 +740,10 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
>>   
>>   	if (!strcmp(mhi_dev->mhi_cntrl->name, "AIC200")) {
>>   		context->image_table = aic200_image_table;
>> -		context->table_size = ARRAY_SIZE(aic200_image_table);
>> +		context->table_size = aic200_image_table_size;
>>   	} else {
>>   		context->image_table = aic100_image_table;
>> -		context->table_size = ARRAY_SIZE(aic100_image_table);
>> +		context->table_size = aic100_image_table_size;
>>   	}
>>   
>>   	context->active_image_id = SAHARA_IMAGE_ID_NONE;
>> diff --git a/drivers/accel/qaic/sahara.h b/drivers/accel/qaic/sahara.h
>> index 640208acc0d1..d7fd447ca85b 100644
>> --- a/drivers/accel/qaic/sahara.h
>> +++ b/drivers/accel/qaic/sahara.h
>> @@ -7,4 +7,11 @@
>>   
>>   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;
> Making sahara.c reference these arrays through extern declarations is
> pretty ugly, and in patch 4 you're forgetting to add the "static"
> keyword to the image_tables...
>
> How about introducing patch 3 first, with special handling for AIC[12]00
> and then squash this with what is now patch 4 to make the move in one
> go?
Fixed in v2 by removing extern based cross file linkage entirely. 
Firmware image tables are now defined in the Sahara driver and selected 
via a probe-time variant table, so there are no externs and all tables 
remain static.
>
> Still need a statement in one of these commit messages to why you don't
> just add qdu_image_table[] to sahara.c...
>
> Regards,
> Bjorn
The QDU100 image table is now part of the Sahara driver and selected via 
the SAHARA channel match using the probe time variant mechanism. This 
avoids a separate QDU client driver and any registration API.
>> +
>>   #endif /* __SAHARA_H__ */
>> -- 
>> 2.34.1
>>