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

[Jeffrey Hugo <quic_jhugo@xxxxxxxxxxx>]

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