[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
>