Re: [PATCH RFC 10/13] dmaengine: sdxi: Add PCI driver support

[Nathan Lynch <nathan.lynch@amd.com>]

Mario Limonciello <mario.limonciello@amd.com> writes:
> On 9/5/2025 1:48 PM, Nathan Lynch via B4 Relay wrote:
>> +static int sdxi_pci_map(struct sdxi_dev *sdxi)
>> +{
>> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
>> +	int bars, ret;
>> +
>> +	bars = 1 << MMIO_CTL_REGS_BAR | 1 << MMIO_DOORBELL_BAR;
>> +	ret = pcim_iomap_regions(pdev, bars, SDXI_DRV_NAME);
>> +	if (ret) {
>> +		sdxi_err(sdxi, "pcim_iomap_regions failed (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	sdxi->dbs_bar = pci_resource_start(pdev, MMIO_DOORBELL_BAR);
>> +
>> +	/* FIXME: pcim_iomap_table may return NULL, and it's deprecated. */
>> +	sdxi->ctrl_regs = pcim_iomap_table(pdev)[MMIO_CTL_REGS_BAR];
>> +	sdxi->dbs = pcim_iomap_table(pdev)[MMIO_DOORBELL_BAR];
>> +	if (!sdxi->ctrl_regs || !sdxi->dbs) {
>> +		sdxi_err(sdxi, "pcim_iomap_table failed\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void sdxi_pci_unmap(struct sdxi_dev *sdxi)
>> +{
>> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
>> +
>> +	pcim_iounmap(pdev, sdxi->ctrl_regs);
>> +	pcim_iounmap(pdev, sdxi->dbs);
>> +}
>> +
>> +static int sdxi_pci_init(struct sdxi_dev *sdxi)
>> +{
>> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
>> +	struct device *dev = &pdev->dev;
>> +	int dma_bits = 64;
>> +	int ret;
>> +
>> +	ret = pcim_enable_device(pdev);
>> +	if (ret) {
>> +		sdxi_err(sdxi, "pcim_enbale_device failed\n");
>> +		return ret;
>> +	}
>> +
>> +	pci_set_master(pdev);
>
> Does pci_set_master() need to come before dma_set_mask_and_coherent() 
> and sdxi_pci_map()?

Yes, I think so. I'll audit this code for conformance to the
initialization sequence described in Documentation/pci/pci.rst.

>
> If so; there should be an error handling path that does
> pci_clear_master().

Right.

>
>> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(dma_bits));
>> +	if (ret) {
>> +		sdxi_err(sdxi, "failed to set DMA mask & coherent bits\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = sdxi_pci_map(sdxi);
>> +	if (ret) {
>> +		sdxi_err(sdxi, "failed to map device IO resources\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void sdxi_pci_exit(struct sdxi_dev *sdxi)
>> +{
>> +	sdxi_pci_unmap(sdxi);
>
> I think you need a pci_clear_master() here.

OK, will fix.