Re: [PATCH 6/7] accel/qaic: Add config structs for supported cards

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

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