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

[Lizhi Hou <lizhi.hou@xxxxxxx>]

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