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

[Lizhi Hou <lizhi.hou@amd.com>]

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