| Message ID | 20241213213340.2551697-7-quic_jhugo@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | accel/qaic: Initial AIC200 support | expand |
On 12/13/24 13:33, Jeffrey Hugo wrote: > As the number of cards supported by the driver grows, their > configurations will differ. The driver needs to become more dynamic > to support these configurations. Currently, each card may differ in > the exposed BARs, the regions they map to, and the family. > > Create config structs for each card, and let the driver configure the > qaic_device struct based on the configurations passed to the driver. > > Co-developed-by: Youssef Samir <quic_yabdulra@quicinc.com> > Signed-off-by: Youssef Samir <quic_yabdulra@quicinc.com> > Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com> > --- > drivers/accel/qaic/qaic.h | 13 +++-- > drivers/accel/qaic/qaic_drv.c | 76 ++++++++++++++++++++---------- > drivers/accel/qaic/qaic_timesync.c | 2 +- > 3 files changed, 61 insertions(+), 30 deletions(-) > > diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h > index 02561b6cecc6..cf97fd9a7e70 100644 > --- a/drivers/accel/qaic/qaic.h > +++ b/drivers/accel/qaic/qaic.h > @@ -32,6 +32,11 @@ > #define to_accel_kdev(qddev) (to_drm(qddev)->accel->kdev) /* Return Linux device of accel node */ > #define to_qaic_device(dev) (to_qaic_drm_device((dev))->qdev) > > +enum aic_families { > + FAMILY_AIC100, > + FAMILY_MAX, > +}; > + > enum __packed dev_states { > /* Device is offline or will be very soon */ > QAIC_OFFLINE, > @@ -113,10 +118,10 @@ struct qaic_device { > struct pci_dev *pdev; > /* Req. ID of request that will be queued next in MHI control device */ > u32 next_seq_num; > - /* Base address of bar 0 */ > - void __iomem *bar_0; > - /* Base address of bar 2 */ > - void __iomem *bar_2; > + /* Base address of the MHI bar */ > + void __iomem *bar_mhi; > + /* Base address of the DBCs bar */ > + void __iomem *bar_dbc; > /* Controller structure for MHI devices */ > struct mhi_controller *mhi_cntrl; > /* MHI control channel device */ > diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c > index 00fa07aebacd..4e63e475b389 100644 > --- a/drivers/accel/qaic/qaic_drv.c > +++ b/drivers/accel/qaic/qaic_drv.c > @@ -34,13 +34,38 @@ > > MODULE_IMPORT_NS("DMA_BUF"); > > -#define PCI_DEV_AIC080 0xa080 > -#define PCI_DEV_AIC100 0xa100 > +#define PCI_DEVICE_ID_QCOM_AIC080 0xa080 > +#define PCI_DEVICE_ID_QCOM_AIC100 0xa100 > #define QAIC_NAME "qaic" > #define QAIC_DESC "Qualcomm Cloud AI Accelerators" > #define CNTL_MAJOR 5 > #define CNTL_MINOR 0 > > +struct qaic_device_config { > + /* Indicates the AIC family the device belongs to */ > + int family; > + /* A bitmask representing the available BARs */ > + int bar_mask; > + /* An index value used to identify the MHI controller BAR */ > + unsigned int mhi_bar_idx; > + /* An index value used to identify the DBCs BAR */ > + unsigned int dbc_bar_idx; > +}; > + > +static const struct qaic_device_config aic080_config = { > + .family = FAMILY_AIC100, > + .bar_mask = BIT(0) | BIT(2) | BIT(4), > + .mhi_bar_idx = 0, > + .dbc_bar_idx = 2, > +}; > + > +static const struct qaic_device_config aic100_config = { > + .family = FAMILY_AIC100, > + .bar_mask = BIT(0) | BIT(2) | BIT(4), > + .mhi_bar_idx = 0, > + .dbc_bar_idx = 2, > +}; > + > bool datapath_polling; > module_param(datapath_polling, bool, 0400); > MODULE_PARM_DESC(datapath_polling, "Operate the datapath in polling mode"); > @@ -352,7 +377,8 @@ void qaic_dev_reset_clean_local_state(struct qaic_device *qdev) > release_dbc(qdev, i); > } > > -static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_device_id *id) > +static struct qaic_device *create_qdev(struct pci_dev *pdev, > + const struct qaic_device_config *config) > { > struct device *dev = &pdev->dev; > struct qaic_drm_device *qddev; > @@ -365,12 +391,10 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de > return NULL; > > qdev->dev_state = QAIC_OFFLINE; > - if (id->device == PCI_DEV_AIC080 || id->device == PCI_DEV_AIC100) { > - qdev->num_dbc = 16; > - qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, sizeof(*qdev->dbc), GFP_KERNEL); > - if (!qdev->dbc) > - return NULL; > - } > + qdev->num_dbc = 16; Is it better to put num_dbc in qaic_device_config? Thanks, Lizhi > + qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, sizeof(*qdev->dbc), GFP_KERNEL); > + if (!qdev->dbc) > + return NULL; > > qddev = devm_drm_dev_alloc(&pdev->dev, &qaic_accel_driver, struct qaic_drm_device, drm); > if (IS_ERR(qddev)) > @@ -426,7 +450,8 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de > return qdev; > } > > -static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev) > +static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev, > + const struct qaic_device_config *config) > { > int bars; > int ret; > @@ -434,9 +459,9 @@ static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev) > bars = pci_select_bars(pdev, IORESOURCE_MEM) & 0x3f; > > /* make sure the device has the expected BARs */ > - if (bars != (BIT(0) | BIT(2) | BIT(4))) { > - pci_dbg(pdev, "%s: expected BARs 0, 2, and 4 not found in device. Found 0x%x\n", > - __func__, bars); > + if (bars != config->bar_mask) { > + pci_dbg(pdev, "%s: expected BARs %#x not found in device. Found %#x\n", > + __func__, config->bar_mask, bars); > return -EINVAL; > } > > @@ -449,13 +474,13 @@ static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev) > return ret; > dma_set_max_seg_size(&pdev->dev, UINT_MAX); > > - qdev->bar_0 = devm_ioremap_resource(&pdev->dev, &pdev->resource[0]); > - if (IS_ERR(qdev->bar_0)) > - return PTR_ERR(qdev->bar_0); > + qdev->bar_mhi = devm_ioremap_resource(&pdev->dev, &pdev->resource[config->mhi_bar_idx]); > + if (IS_ERR(qdev->bar_mhi)) > + return PTR_ERR(qdev->bar_mhi); > > - qdev->bar_2 = devm_ioremap_resource(&pdev->dev, &pdev->resource[2]); > - if (IS_ERR(qdev->bar_2)) > - return PTR_ERR(qdev->bar_2); > + qdev->bar_dbc = devm_ioremap_resource(&pdev->dev, &pdev->resource[config->dbc_bar_idx]); > + if (IS_ERR(qdev->bar_dbc)) > + return PTR_ERR(qdev->bar_dbc); > > /* Managed release since we use pcim_enable_device above */ > pci_set_master(pdev); > @@ -517,21 +542,22 @@ static int init_msi(struct qaic_device *qdev, struct pci_dev *pdev) > > static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > + struct qaic_device_config *config = (struct qaic_device_config *)id->driver_data; > struct qaic_device *qdev; > int mhi_irq; > int ret; > int i; > > - qdev = create_qdev(pdev, id); > + qdev = create_qdev(pdev, config); > if (!qdev) > return -ENOMEM; > > - ret = init_pci(qdev, pdev); > + ret = init_pci(qdev, pdev, config); > if (ret) > return ret; > > for (i = 0; i < qdev->num_dbc; ++i) > - qdev->dbc[i].dbc_base = qdev->bar_2 + QAIC_DBC_OFF(i); > + qdev->dbc[i].dbc_base = qdev->bar_dbc + QAIC_DBC_OFF(i); > > mhi_irq = init_msi(qdev, pdev); > if (mhi_irq < 0) > @@ -541,7 +567,7 @@ static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (ret) > return ret; > > - qdev->mhi_cntrl = qaic_mhi_register_controller(pdev, qdev->bar_0, mhi_irq, > + qdev->mhi_cntrl = qaic_mhi_register_controller(pdev, qdev->bar_mhi, mhi_irq, > qdev->single_msi); > if (IS_ERR(qdev->mhi_cntrl)) { > ret = PTR_ERR(qdev->mhi_cntrl); > @@ -609,8 +635,8 @@ static struct mhi_driver qaic_mhi_driver = { > }; > > static const struct pci_device_id qaic_ids[] = { > - { PCI_DEVICE(PCI_VENDOR_ID_QCOM, PCI_DEV_AIC080), }, > - { PCI_DEVICE(PCI_VENDOR_ID_QCOM, PCI_DEV_AIC100), }, > + { PCI_DEVICE_DATA(QCOM, AIC080, (kernel_ulong_t)&aic080_config), }, > + { PCI_DEVICE_DATA(QCOM, AIC100, (kernel_ulong_t)&aic100_config), }, > { } > }; > MODULE_DEVICE_TABLE(pci, qaic_ids); > diff --git a/drivers/accel/qaic/qaic_timesync.c b/drivers/accel/qaic/qaic_timesync.c > index 301f4462d51b..2473c66309d4 100644 > --- a/drivers/accel/qaic/qaic_timesync.c > +++ b/drivers/accel/qaic/qaic_timesync.c > @@ -201,7 +201,7 @@ static int qaic_timesync_probe(struct mhi_device *mhi_dev, const struct mhi_devi > goto free_sync_msg; > > /* Qtimer register pointer */ > - mqtsdev->qtimer_addr = qdev->bar_0 + QTIMER_REG_OFFSET; > + mqtsdev->qtimer_addr = qdev->bar_mhi + QTIMER_REG_OFFSET; > timer_setup(timer, qaic_timesync_timer, 0); > timer->expires = jiffies + msecs_to_jiffies(timesync_delay_ms); > add_timer(timer);
On 12/13/2024 5:35 PM, Lizhi Hou wrote: > > On 12/13/24 13:33, Jeffrey Hugo wrote: >> -static struct qaic_device *create_qdev(struct pci_dev *pdev, const >> struct pci_device_id *id) >> +static struct qaic_device *create_qdev(struct pci_dev *pdev, >> + const struct qaic_device_config *config) >> { >> struct device *dev = &pdev->dev; >> struct qaic_drm_device *qddev; >> @@ -365,12 +391,10 @@ static struct qaic_device *create_qdev(struct >> pci_dev *pdev, const struct pci_de >> return NULL; >> qdev->dev_state = QAIC_OFFLINE; >> - if (id->device == PCI_DEV_AIC080 || id->device == PCI_DEV_AIC100) { >> - qdev->num_dbc = 16; >> - qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, >> sizeof(*qdev->dbc), GFP_KERNEL); >> - if (!qdev->dbc) >> - return NULL; >> - } >> + qdev->num_dbc = 16; > > Is it better to put num_dbc in qaic_device_config? I think there is no clear "right answer". All known devices use 16. There may be a future device which has a different value, at which point I think this needs to be in qaic_device_config. For this patch, I was conservative and only included items in qaic_device_config which do vary between the known devices. I'll think in this a bit more. > > Thanks, > > Lizhi >
On 12/20/24 09:15, Jeffrey Hugo wrote: > On 12/13/2024 5:35 PM, Lizhi Hou wrote: >> >> On 12/13/24 13:33, Jeffrey Hugo wrote: >>> -static struct qaic_device *create_qdev(struct pci_dev *pdev, const >>> struct pci_device_id *id) >>> +static struct qaic_device *create_qdev(struct pci_dev *pdev, >>> + const struct qaic_device_config *config) >>> { >>> struct device *dev = &pdev->dev; >>> struct qaic_drm_device *qddev; >>> @@ -365,12 +391,10 @@ static struct qaic_device *create_qdev(struct >>> pci_dev *pdev, const struct pci_de >>> return NULL; >>> qdev->dev_state = QAIC_OFFLINE; >>> - if (id->device == PCI_DEV_AIC080 || id->device == >>> PCI_DEV_AIC100) { >>> - qdev->num_dbc = 16; >>> - qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, >>> sizeof(*qdev->dbc), GFP_KERNEL); >>> - if (!qdev->dbc) >>> - return NULL; >>> - } >>> + qdev->num_dbc = 16; >> >> Is it better to put num_dbc in qaic_device_config? > > I think there is no clear "right answer". All known devices use 16. > There may be a future device which has a different value, at which > point I think this needs to be in qaic_device_config. For this patch, > I was conservative and only included items in qaic_device_config which > do vary between the known devices. > > I'll think in this a bit more. Reviewed-by: Lizhi Hou <lizhi.hou@amd.com> > >> >> Thanks, >> >> Lizhi >>
diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h index 02561b6cecc6..cf97fd9a7e70 100644 --- a/drivers/accel/qaic/qaic.h +++ b/drivers/accel/qaic/qaic.h @@ -32,6 +32,11 @@ #define to_accel_kdev(qddev) (to_drm(qddev)->accel->kdev) /* Return Linux device of accel node */ #define to_qaic_device(dev) (to_qaic_drm_device((dev))->qdev) +enum aic_families { + FAMILY_AIC100, + FAMILY_MAX, +}; + enum __packed dev_states { /* Device is offline or will be very soon */ QAIC_OFFLINE, @@ -113,10 +118,10 @@ struct qaic_device { struct pci_dev *pdev; /* Req. ID of request that will be queued next in MHI control device */ u32 next_seq_num; - /* Base address of bar 0 */ - void __iomem *bar_0; - /* Base address of bar 2 */ - void __iomem *bar_2; + /* Base address of the MHI bar */ + void __iomem *bar_mhi; + /* Base address of the DBCs bar */ + void __iomem *bar_dbc; /* Controller structure for MHI devices */ struct mhi_controller *mhi_cntrl; /* MHI control channel device */ diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c index 00fa07aebacd..4e63e475b389 100644 --- a/drivers/accel/qaic/qaic_drv.c +++ b/drivers/accel/qaic/qaic_drv.c @@ -34,13 +34,38 @@ MODULE_IMPORT_NS("DMA_BUF"); -#define PCI_DEV_AIC080 0xa080 -#define PCI_DEV_AIC100 0xa100 +#define PCI_DEVICE_ID_QCOM_AIC080 0xa080 +#define PCI_DEVICE_ID_QCOM_AIC100 0xa100 #define QAIC_NAME "qaic" #define QAIC_DESC "Qualcomm Cloud AI Accelerators" #define CNTL_MAJOR 5 #define CNTL_MINOR 0 +struct qaic_device_config { + /* Indicates the AIC family the device belongs to */ + int family; + /* A bitmask representing the available BARs */ + int bar_mask; + /* An index value used to identify the MHI controller BAR */ + unsigned int mhi_bar_idx; + /* An index value used to identify the DBCs BAR */ + unsigned int dbc_bar_idx; +}; + +static const struct qaic_device_config aic080_config = { + .family = FAMILY_AIC100, + .bar_mask = BIT(0) | BIT(2) | BIT(4), + .mhi_bar_idx = 0, + .dbc_bar_idx = 2, +}; + +static const struct qaic_device_config aic100_config = { + .family = FAMILY_AIC100, + .bar_mask = BIT(0) | BIT(2) | BIT(4), + .mhi_bar_idx = 0, + .dbc_bar_idx = 2, +}; + bool datapath_polling; module_param(datapath_polling, bool, 0400); MODULE_PARM_DESC(datapath_polling, "Operate the datapath in polling mode"); @@ -352,7 +377,8 @@ void qaic_dev_reset_clean_local_state(struct qaic_device *qdev) release_dbc(qdev, i); } -static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_device_id *id) +static struct qaic_device *create_qdev(struct pci_dev *pdev, + const struct qaic_device_config *config) { struct device *dev = &pdev->dev; struct qaic_drm_device *qddev; @@ -365,12 +391,10 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de return NULL; qdev->dev_state = QAIC_OFFLINE; - if (id->device == PCI_DEV_AIC080 || id->device == PCI_DEV_AIC100) { - qdev->num_dbc = 16; - qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, sizeof(*qdev->dbc), GFP_KERNEL); - if (!qdev->dbc) - return NULL; - } + qdev->num_dbc = 16; + qdev->dbc = devm_kcalloc(dev, qdev->num_dbc, sizeof(*qdev->dbc), GFP_KERNEL); + if (!qdev->dbc) + return NULL; qddev = devm_drm_dev_alloc(&pdev->dev, &qaic_accel_driver, struct qaic_drm_device, drm); if (IS_ERR(qddev)) @@ -426,7 +450,8 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de return qdev; } -static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev) +static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev, + const struct qaic_device_config *config) { int bars; int ret; @@ -434,9 +459,9 @@ static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev) bars = pci_select_bars(pdev, IORESOURCE_MEM) & 0x3f; /* make sure the device has the expected BARs */ - if (bars != (BIT(0) | BIT(2) | BIT(4))) { - pci_dbg(pdev, "%s: expected BARs 0, 2, and 4 not found in device. Found 0x%x\n", - __func__, bars); + if (bars != config->bar_mask) { + pci_dbg(pdev, "%s: expected BARs %#x not found in device. Found %#x\n", + __func__, config->bar_mask, bars); return -EINVAL; } @@ -449,13 +474,13 @@ static int init_pci(struct qaic_device *qdev, struct pci_dev *pdev) return ret; dma_set_max_seg_size(&pdev->dev, UINT_MAX); - qdev->bar_0 = devm_ioremap_resource(&pdev->dev, &pdev->resource[0]); - if (IS_ERR(qdev->bar_0)) - return PTR_ERR(qdev->bar_0); + qdev->bar_mhi = devm_ioremap_resource(&pdev->dev, &pdev->resource[config->mhi_bar_idx]); + if (IS_ERR(qdev->bar_mhi)) + return PTR_ERR(qdev->bar_mhi); - qdev->bar_2 = devm_ioremap_resource(&pdev->dev, &pdev->resource[2]); - if (IS_ERR(qdev->bar_2)) - return PTR_ERR(qdev->bar_2); + qdev->bar_dbc = devm_ioremap_resource(&pdev->dev, &pdev->resource[config->dbc_bar_idx]); + if (IS_ERR(qdev->bar_dbc)) + return PTR_ERR(qdev->bar_dbc); /* Managed release since we use pcim_enable_device above */ pci_set_master(pdev); @@ -517,21 +542,22 @@ static int init_msi(struct qaic_device *qdev, struct pci_dev *pdev) static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { + struct qaic_device_config *config = (struct qaic_device_config *)id->driver_data; struct qaic_device *qdev; int mhi_irq; int ret; int i; - qdev = create_qdev(pdev, id); + qdev = create_qdev(pdev, config); if (!qdev) return -ENOMEM; - ret = init_pci(qdev, pdev); + ret = init_pci(qdev, pdev, config); if (ret) return ret; for (i = 0; i < qdev->num_dbc; ++i) - qdev->dbc[i].dbc_base = qdev->bar_2 + QAIC_DBC_OFF(i); + qdev->dbc[i].dbc_base = qdev->bar_dbc + QAIC_DBC_OFF(i); mhi_irq = init_msi(qdev, pdev); if (mhi_irq < 0) @@ -541,7 +567,7 @@ static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (ret) return ret; - qdev->mhi_cntrl = qaic_mhi_register_controller(pdev, qdev->bar_0, mhi_irq, + qdev->mhi_cntrl = qaic_mhi_register_controller(pdev, qdev->bar_mhi, mhi_irq, qdev->single_msi); if (IS_ERR(qdev->mhi_cntrl)) { ret = PTR_ERR(qdev->mhi_cntrl); @@ -609,8 +635,8 @@ static struct mhi_driver qaic_mhi_driver = { }; static const struct pci_device_id qaic_ids[] = { - { PCI_DEVICE(PCI_VENDOR_ID_QCOM, PCI_DEV_AIC080), }, - { PCI_DEVICE(PCI_VENDOR_ID_QCOM, PCI_DEV_AIC100), }, + { PCI_DEVICE_DATA(QCOM, AIC080, (kernel_ulong_t)&aic080_config), }, + { PCI_DEVICE_DATA(QCOM, AIC100, (kernel_ulong_t)&aic100_config), }, { } }; MODULE_DEVICE_TABLE(pci, qaic_ids); diff --git a/drivers/accel/qaic/qaic_timesync.c b/drivers/accel/qaic/qaic_timesync.c index 301f4462d51b..2473c66309d4 100644 --- a/drivers/accel/qaic/qaic_timesync.c +++ b/drivers/accel/qaic/qaic_timesync.c @@ -201,7 +201,7 @@ static int qaic_timesync_probe(struct mhi_device *mhi_dev, const struct mhi_devi goto free_sync_msg; /* Qtimer register pointer */ - mqtsdev->qtimer_addr = qdev->bar_0 + QTIMER_REG_OFFSET; + mqtsdev->qtimer_addr = qdev->bar_mhi + QTIMER_REG_OFFSET; timer_setup(timer, qaic_timesync_timer, 0); timer->expires = jiffies + msecs_to_jiffies(timesync_delay_ms); add_timer(timer);