[RFC,v3,5/7] dmaengine: Add Synopsys eDMA IP PCIe glue-logic

Commit Message

Gustavo Pimentel Jan. 11, 2019, 6:33 p.m. UTC

Synopsys eDMA IP is normally distributed along with Synopsys PCIe
EndPoint IP (depends of the use and licensing agreement).

This IP requires some basic configurations, such as:
 - eDMA registers BAR
 - eDMA registers offset
 - eDMA registers size
 - eDMA linked list memory BAR
 - eDMA linked list memory offset
 - eDMA linked list memory sze
 - eDMA data memory BAR
 - eDMA data memory offset
 - eDMA data memory size
 - eDMA version
 - eDMA mode
 - IRQs available for eDMA

As a working example, PCIe glue-logic will attach to a Synopsys PCIe
EndPoint IP prototype kit (Vendor ID = 0x16c3, Device ID = 0xedda),
which has built-in an eDMA IP with this default configuration:
 - eDMA registers BAR = 0
 - eDMA registers offset = 0x00001000 (4 Kbytes)
 - eDMA registers size = 0x00002000 (8 Kbytes)
 - eDMA linked list memory BAR = 2
 - eDMA linked list memory offset = 0x00000000 (0 Kbytes)
 - eDMA linked list memory size = 0x00800000 (8 Mbytes)
 - eDMA data memory BAR = 2
 - eDMA data memory offset = 0x00800000 (8 Mbytes)
 - eDMA data memory size = 0x03800000 (56 Mbytes)
 - eDMA version = 0
 - eDMA mode = EDMA_MODE_UNROLL
 - IRQs = 1

This driver can be compile as built-in or external module in kernel.

To enable this driver just select DW_EDMA_PCIE option in kernel
configuration, however it requires and selects automatically DW_EDMA
option too.

Changes:
RFC v1->RFC v2:
 - Replace comments // (C99 style) by /**/
 - Merge two pcim_iomap_regions() calls into just one call
 - Remove pci_try_set_mwi() call
 - Replace some dev_info() by dev_dbg() to reduce *noise*
 - Remove pci_name(pdev) call after being call dw_edma_remove()
 - Remove all power management support
 - Fix the headers of the .c and .h files according to the most recent
   convention
 - Fix errors and checks pointed out by checkpatch with --strict option
 - Replace patch small description tag from dma by dmaengine
RFC v2->RFC v3:
 - Fix printk variable of phys_addr_t type
 - Fix missing variable initialization (chan->configured)
 - Change linked list size to 512 Kbytes
 - Add data memory information
 - Add register size information
 - Add comments or improve existing ones
 - Add possibility to work with multiple IRQs feature
 - Replace MSI and MSI-X enable condition by pci_dev_msi_enabled()
 - Replace code to acquire MSI(-X) address and data by
   get_cached_msi_msg()

Signed-off-by: Gustavo Pimentel <[e-mail]>
Cc: Vinod Koul <[e-mail]>
Cc: Dan Williams <[e-mail]>
Cc: Eugeniy Paltsev <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Russell King <[e-mail]>
Cc: Niklas Cassel <[e-mail]>
Cc: Lorenzo Pieralisi <[e-mail]>
Cc: Joao Pinto <[e-mail]>
Cc: Jose Abreu <[e-mail]>
Cc: Luis Oliveira <[e-mail]>
Cc: Vitor Soares <[e-mail]>
Cc: Nelson Costa <[e-mail]>
Cc: Pedro Sousa <[e-mail]>
---
 drivers/dma/dw-edma/Kconfig        |   9 ++
 drivers/dma/dw-edma/Makefile       |   1 +
 drivers/dma/dw-edma/dw-edma-pcie.c | 254 +++++++++++++++++++++++++++++++++++++
 3 files changed, 264 insertions(+)
 create mode 100644 drivers/dma/dw-edma/dw-edma-pcie.c

Comments

Andy Shevchenko Jan. 11, 2019, 7:47 p.m. UTC | #1

On Fri, Jan 11, 2019 at 07:33:41PM +0100, Gustavo Pimentel wrote:
> Synopsys eDMA IP is normally distributed along with Synopsys PCIe
> EndPoint IP (depends of the use and licensing agreement).
> 
> This IP requires some basic configurations, such as:
>  - eDMA registers BAR
>  - eDMA registers offset
>  - eDMA registers size
>  - eDMA linked list memory BAR
>  - eDMA linked list memory offset
>  - eDMA linked list memory sze
>  - eDMA data memory BAR
>  - eDMA data memory offset
>  - eDMA data memory size
>  - eDMA version
>  - eDMA mode
>  - IRQs available for eDMA
> 
> As a working example, PCIe glue-logic will attach to a Synopsys PCIe
> EndPoint IP prototype kit (Vendor ID = 0x16c3, Device ID = 0xedda),
> which has built-in an eDMA IP with this default configuration:
>  - eDMA registers BAR = 0
>  - eDMA registers offset = 0x00001000 (4 Kbytes)
>  - eDMA registers size = 0x00002000 (8 Kbytes)
>  - eDMA linked list memory BAR = 2
>  - eDMA linked list memory offset = 0x00000000 (0 Kbytes)
>  - eDMA linked list memory size = 0x00800000 (8 Mbytes)
>  - eDMA data memory BAR = 2
>  - eDMA data memory offset = 0x00800000 (8 Mbytes)
>  - eDMA data memory size = 0x03800000 (56 Mbytes)
>  - eDMA version = 0
>  - eDMA mode = EDMA_MODE_UNROLL
>  - IRQs = 1
> 
> This driver can be compile as built-in or external module in kernel.
> 
> To enable this driver just select DW_EDMA_PCIE option in kernel
> configuration, however it requires and selects automatically DW_EDMA
> option too.
> 

> Changes:
> RFC v1->RFC v2:

Changes go after '--- ' line.

>  - Replace comments // (C99 style) by /**/
>  - Merge two pcim_iomap_regions() calls into just one call
>  - Remove pci_try_set_mwi() call
>  - Replace some dev_info() by dev_dbg() to reduce *noise*
>  - Remove pci_name(pdev) call after being call dw_edma_remove()
>  - Remove all power management support
>  - Fix the headers of the .c and .h files according to the most recent
>    convention
>  - Fix errors and checks pointed out by checkpatch with --strict option
>  - Replace patch small description tag from dma by dmaengine
> RFC v2->RFC v3:
>  - Fix printk variable of phys_addr_t type
>  - Fix missing variable initialization (chan->configured)
>  - Change linked list size to 512 Kbytes
>  - Add data memory information
>  - Add register size information
>  - Add comments or improve existing ones
>  - Add possibility to work with multiple IRQs feature
>  - Replace MSI and MSI-X enable condition by pci_dev_msi_enabled()
>  - Replace code to acquire MSI(-X) address and data by
>    get_cached_msi_msg()

> +enum dw_edma_pcie_bar {
> +	BAR_0,
> +	BAR_1,
> +	BAR_2,
> +	BAR_3,
> +	BAR_4,
> +	BAR_5
> +};

pci-epf.h has this.
Why duplicate?


What else is being duplicated from PCI core?

> +static bool disable_msix;
> +module_param(disable_msix, bool, 0644);
> +MODULE_PARM_DESC(disable_msix, "Disable MSI-X interrupts");

Why?!
We are no allow new module parameters without very strong arguments.

> +
> +static int dw_edma_pcie_probe(struct pci_dev *pdev,
> +			      const struct pci_device_id *pid)
> +{
> +	const struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
> +	struct device *dev = &pdev->dev;
> +	struct dw_edma_chip *chip;
> +	struct dw_edma *dw;
> +	unsigned int irq_flags = PCI_IRQ_MSI;
> +	int err, nr_irqs, i;
> +

> +	if (!pdata) {
> +		dev_err(dev, "%s missing data structure\n", pci_name(pdev));
> +		return -EFAULT;
> +	}

Useless check.

> +
> +	/* Enable PCI device */
> +	err = pcim_enable_device(pdev);
> +	if (err) {
> +		dev_err(dev, "%s enabling device failed\n", pci_name(pdev));
> +		return err;
> +	}
> +
> +	/* Mapping PCI BAR regions */
> +	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar) |
> +				       BIT(pdata->ll_bar) |
> +				       BIT(pdata->dt_bar),
> +				 pci_name(pdev));
> +	if (err) {

> +		dev_err(dev, "%s eDMA BAR I/O remapping failed\n",
> +			pci_name(pdev));

Isn't it pci_err() ?
Same comment for the rest similar cases above and below.

> +		return err;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	nr_irqs = pci_alloc_irq_vectors(pdev, 1, pdata->irqs_cnt, irq_flags);
> +	if (nr_irqs < 1) {
> +		dev_err(dev, "%s failed to alloc IRQ vector (Number of IRQs=%u)\n",
> +			pci_name(pdev), nr_irqs);
> +		return -EPERM;
> +	}
> +
> +	/* Data structure initialization */
> +	chip->dw = dw;
> +	chip->dev = dev;
> +	chip->id = pdev->devfn;
> +	chip->irq = pdev->irq;
> +

> +	if (!pcim_iomap_table(pdev))
> +		return -EACCES;

Never happen condition. Thus useless.

> +	dev_info(dev, "DesignWare eDMA PCIe driver loaded completely\n");

Useless.

> +}
> +
> +static void dw_edma_pcie_remove(struct pci_dev *pdev)
> +{
> +	struct dw_edma_chip *chip = pci_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +	int err;
> +
> +	/* Stopping eDMA driver */
> +	err = dw_edma_remove(chip);
> +	if (err)
> +		dev_warn(dev, "can't remove device properly: %d\n", err);
> +
> +	/* Freeing IRQs */
> +	pci_free_irq_vectors(pdev);
> +
> +	dev_info(dev, "DesignWare eDMA PCIe driver unloaded completely\n");

Ditto.

> +}

> +MODULE_DEVICE_TABLE(pci, dw_edma_pcie_id_table);
> +
> +static struct pci_driver dw_edma_pcie_driver = {
> +	.name		= "dw-edma-pcie",
> +	.id_table	= dw_edma_pcie_id_table,
> +	.probe		= dw_edma_pcie_probe,
> +	.remove		= dw_edma_pcie_remove,

Power management?

> +};

Gustavo Pimentel Jan. 14, 2019, 11:38 a.m. UTC | #2

On 11/01/2019 19:47, Andy Shevchenko wrote:
> On Fri, Jan 11, 2019 at 07:33:41PM +0100, Gustavo Pimentel wrote:
>> Synopsys eDMA IP is normally distributed along with Synopsys PCIe
>> EndPoint IP (depends of the use and licensing agreement).
>>
>> This IP requires some basic configurations, such as:
>>  - eDMA registers BAR
>>  - eDMA registers offset
>>  - eDMA registers size
>>  - eDMA linked list memory BAR
>>  - eDMA linked list memory offset
>>  - eDMA linked list memory sze
>>  - eDMA data memory BAR
>>  - eDMA data memory offset
>>  - eDMA data memory size
>>  - eDMA version
>>  - eDMA mode
>>  - IRQs available for eDMA
>>
>> As a working example, PCIe glue-logic will attach to a Synopsys PCIe
>> EndPoint IP prototype kit (Vendor ID = 0x16c3, Device ID = 0xedda),
>> which has built-in an eDMA IP with this default configuration:
>>  - eDMA registers BAR = 0
>>  - eDMA registers offset = 0x00001000 (4 Kbytes)
>>  - eDMA registers size = 0x00002000 (8 Kbytes)
>>  - eDMA linked list memory BAR = 2
>>  - eDMA linked list memory offset = 0x00000000 (0 Kbytes)
>>  - eDMA linked list memory size = 0x00800000 (8 Mbytes)
>>  - eDMA data memory BAR = 2
>>  - eDMA data memory offset = 0x00800000 (8 Mbytes)
>>  - eDMA data memory size = 0x03800000 (56 Mbytes)
>>  - eDMA version = 0
>>  - eDMA mode = EDMA_MODE_UNROLL
>>  - IRQs = 1
>>
>> This driver can be compile as built-in or external module in kernel.
>>
>> To enable this driver just select DW_EDMA_PCIE option in kernel
>> configuration, however it requires and selects automatically DW_EDMA
>> option too.
>>
> 
>> Changes:
>> RFC v1->RFC v2:
> 
> Changes go after '--- ' line.

At the last Linux Plumbers Conference there were some subsystem maintainers who
asked that the track changes be included in the description as a way to not lose
the previous work done. That why I put it before the '---' line, but it's
indifferent to me, I can put it after the '---' line.

> 
>>  - Replace comments // (C99 style) by /**/
>>  - Merge two pcim_iomap_regions() calls into just one call
>>  - Remove pci_try_set_mwi() call
>>  - Replace some dev_info() by dev_dbg() to reduce *noise*
>>  - Remove pci_name(pdev) call after being call dw_edma_remove()
>>  - Remove all power management support
>>  - Fix the headers of the .c and .h files according to the most recent
>>    convention
>>  - Fix errors and checks pointed out by checkpatch with --strict option
>>  - Replace patch small description tag from dma by dmaengine
>> RFC v2->RFC v3:
>>  - Fix printk variable of phys_addr_t type
>>  - Fix missing variable initialization (chan->configured)
>>  - Change linked list size to 512 Kbytes
>>  - Add data memory information
>>  - Add register size information
>>  - Add comments or improve existing ones
>>  - Add possibility to work with multiple IRQs feature
>>  - Replace MSI and MSI-X enable condition by pci_dev_msi_enabled()
>>  - Replace code to acquire MSI(-X) address and data by
>>    get_cached_msi_msg()
> 
>> +enum dw_edma_pcie_bar {
>> +	BAR_0,
>> +	BAR_1,
>> +	BAR_2,
>> +	BAR_3,
>> +	BAR_4,
>> +	BAR_5
>> +};
> 
> pci-epf.h has this.
> Why duplicate?

I can use that header sure. Thanks.

> 
> 
> What else is being duplicated from PCI core?
> 
>> +static bool disable_msix;
>> +module_param(disable_msix, bool, 0644);
>> +MODULE_PARM_DESC(disable_msix, "Disable MSI-X interrupts");
> 
> Why?!
> We are no allow new module parameters without very strong arguments.

Since this is a reference driver and might be used to test customized HW
solutions, I added this parameter to allow the possibility to test the solution
forcing the MSI feature binding. This is required specially if who will test
this solution has a Root Complex with both features available (MSI and MSI-X),
because the Kernel will give always preference to MSI-X binding (assuming that
the EP has also both features available).

> 
>> +
>> +static int dw_edma_pcie_probe(struct pci_dev *pdev,
>> +			      const struct pci_device_id *pid)
>> +{
>> +	const struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
>> +	struct device *dev = &pdev->dev;
>> +	struct dw_edma_chip *chip;
>> +	struct dw_edma *dw;
>> +	unsigned int irq_flags = PCI_IRQ_MSI;
>> +	int err, nr_irqs, i;
>> +
> 
>> +	if (!pdata) {
>> +		dev_err(dev, "%s missing data structure\n", pci_name(pdev));
>> +		return -EFAULT;
>> +	}
> 
> Useless check.

Why? It's just a precaution, isn't it a good practice always to think of the
worst case?

> 
>> +
>> +	/* Enable PCI device */
>> +	err = pcim_enable_device(pdev);
>> +	if (err) {
>> +		dev_err(dev, "%s enabling device failed\n", pci_name(pdev));
>> +		return err;
>> +	}
>> +
>> +	/* Mapping PCI BAR regions */
>> +	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar) |
>> +				       BIT(pdata->ll_bar) |
>> +				       BIT(pdata->dt_bar),
>> +				 pci_name(pdev));
>> +	if (err) {
> 
>> +		dev_err(dev, "%s eDMA BAR I/O remapping failed\n",
>> +			pci_name(pdev));
> 
> Isn't it pci_err() ?
> Same comment for the rest similar cases above and below.

Ok, I'll replace all dev_* function in this file.
Thanks.

> 
>> +		return err;
>> +	}
>> +
>> +	pci_set_master(pdev);
>> +
>> +	nr_irqs = pci_alloc_irq_vectors(pdev, 1, pdata->irqs_cnt, irq_flags);
>> +	if (nr_irqs < 1) {
>> +		dev_err(dev, "%s failed to alloc IRQ vector (Number of IRQs=%u)\n",
>> +			pci_name(pdev), nr_irqs);
>> +		return -EPERM;
>> +	}
>> +
>> +	/* Data structure initialization */
>> +	chip->dw = dw;
>> +	chip->dev = dev;
>> +	chip->id = pdev->devfn;
>> +	chip->irq = pdev->irq;
>> +
> 
>> +	if (!pcim_iomap_table(pdev))
>> +		return -EACCES;
> 
> Never happen condition. Thus useless.

pcim_iomap_table() can return NULL in case of allocation failure. Besides that,
isn't it a good practice always to think of the worst case?

> 
>> +	dev_info(dev, "DesignWare eDMA PCIe driver loaded completely\n");
> 
> Useless.

It's helpful for bring up, I can pass it to dbg.

> 
>> +}
>> +
>> +static void dw_edma_pcie_remove(struct pci_dev *pdev)
>> +{
>> +	struct dw_edma_chip *chip = pci_get_drvdata(pdev);
>> +	struct device *dev = &pdev->dev;
>> +	int err;
>> +
>> +	/* Stopping eDMA driver */
>> +	err = dw_edma_remove(chip);
>> +	if (err)
>> +		dev_warn(dev, "can't remove device properly: %d\n", err);
>> +
>> +	/* Freeing IRQs */
>> +	pci_free_irq_vectors(pdev);
>> +
>> +	dev_info(dev, "DesignWare eDMA PCIe driver unloaded completely\n");
> 
> Ditto.

It's helpful for bring up, I can pass it to dbg.

> 
>> +}
> 
>> +MODULE_DEVICE_TABLE(pci, dw_edma_pcie_id_table);
>> +
>> +static struct pci_driver dw_edma_pcie_driver = {
>> +	.name		= "dw-edma-pcie",
>> +	.id_table	= dw_edma_pcie_id_table,
>> +	.probe		= dw_edma_pcie_probe,
>> +	.remove		= dw_edma_pcie_remove,
> 
> Power management?

I've removed the power management for now, since with my current setup I don't
have the necessary conditions to test it. I prefer not submitting that code for now.

> 
>> +};
> 

Thanks for the inputs Andy! They have been pretty good!

Regards,
Gustavo

Andy Shevchenko Jan. 15, 2019, 5:43 a.m. UTC | #3

On Mon, Jan 14, 2019 at 11:38:02AM +0000, Gustavo Pimentel wrote:
> On 11/01/2019 19:47, Andy Shevchenko wrote:
> > On Fri, Jan 11, 2019 at 07:33:41PM +0100, Gustavo Pimentel wrote:

> >> +static bool disable_msix;
> >> +module_param(disable_msix, bool, 0644);
> >> +MODULE_PARM_DESC(disable_msix, "Disable MSI-X interrupts");
> > 
> > Why?!
> > We are no allow new module parameters without very strong arguments.
> 
> Since this is a reference driver and might be used to test customized HW
> solutions, I added this parameter to allow the possibility to test the solution
> forcing the MSI feature binding. This is required specially if who will test
> this solution has a Root Complex with both features available (MSI and MSI-X),
> because the Kernel will give always preference to MSI-X binding (assuming that
> the EP has also both features available).

Yes, you may do it for testing purposes, but it doesn't fit the kernel standards.

> >> +	if (!pdata) {
> >> +		dev_err(dev, "%s missing data structure\n", pci_name(pdev));
> >> +		return -EFAULT;
> >> +	}
> > 
> > Useless check.
> 
> Why? It's just a precaution, isn't it a good practice always to think of the
> worst case?

You just can put an implicit requirement of pdata rather than doing this
useless check. I don't believe it would make sense to have NULL pdata for the
driver since it wouldn't be functional anyhow.

> >> +	/* Mapping PCI BAR regions */
> >> +	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar) |
> >> +				       BIT(pdata->ll_bar) |
> >> +				       BIT(pdata->dt_bar),
> >> +				 pci_name(pdev));
> >> +	if (err) {

> >> +		return err;
> >> +	}

> >> +	if (!pcim_iomap_table(pdev))
> >> +		return -EACCES;
> > 
> > Never happen condition. Thus useless.
> 
> pcim_iomap_table() can return NULL in case of allocation failure. Besides that,
> isn't it a good practice always to think of the worst case?

No, it can't in the conditions your have in the code. See above the lines I left.
If pcim_iomap_regions() successfully finished...

> >> +	dev_info(dev, "DesignWare eDMA PCIe driver loaded completely\n");
> > 
> > Useless.
> 
> It's helpful for bring up, I can pass it to dbg.

It just shows that someone didn't use existing tools and features. This message and similar are useless.
Hint: initcall_debug.

> >> +	dev_info(dev, "DesignWare eDMA PCIe driver unloaded completely\n");
> > 
> > Ditto.
> 
> It's helpful for bring up, I can pass it to dbg.

Ditto.

Gustavo Pimentel Jan. 15, 2019, 12:48 p.m. UTC | #4

On 15/01/2019 05:43, Andy Shevchenko wrote:
> On Mon, Jan 14, 2019 at 11:38:02AM +0000, Gustavo Pimentel wrote:
>> On 11/01/2019 19:47, Andy Shevchenko wrote:
>>> On Fri, Jan 11, 2019 at 07:33:41PM +0100, Gustavo Pimentel wrote:
> 
>>>> +static bool disable_msix;
>>>> +module_param(disable_msix, bool, 0644);
>>>> +MODULE_PARM_DESC(disable_msix, "Disable MSI-X interrupts");
>>>
>>> Why?!
>>> We are no allow new module parameters without very strong arguments.
>>
>> Since this is a reference driver and might be used to test customized HW
>> solutions, I added this parameter to allow the possibility to test the solution
>> forcing the MSI feature binding. This is required specially if who will test
>> this solution has a Root Complex with both features available (MSI and MSI-X),
>> because the Kernel will give always preference to MSI-X binding (assuming that
>> the EP has also both features available).
> 
> Yes, you may do it for testing purposes, but it doesn't fit the kernel standards.

Ok, but how should I proceed? May I leave it or substitute by another way to do
it? If so, how?
As I said, the intended is to be only used for this test case, on normal
operation the parameter it should be always false.

> 
>>>> +	if (!pdata) {
>>>> +		dev_err(dev, "%s missing data structure\n", pci_name(pdev));
>>>> +		return -EFAULT;
>>>> +	}
>>>
>>> Useless check.
>>
>> Why? It's just a precaution, isn't it a good practice always to think of the
>> worst case?
> 
> You just can put an implicit requirement of pdata rather than doing this

Ok, how can I do it? What I should add to the code to force that?

> useless check. I don't believe it would make sense to have NULL pdata for the
> driver since it wouldn't be functional anyhow.

Yes, you're right without pdata the driver can't do anything.
> 
>>>> +	/* Mapping PCI BAR regions */
>>>> +	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar) |
>>>> +				       BIT(pdata->ll_bar) |
>>>> +				       BIT(pdata->dt_bar),
>>>> +				 pci_name(pdev));
>>>> +	if (err) {
> 
>>>> +		return err;
>>>> +	}
> 
>>>> +	if (!pcim_iomap_table(pdev))
>>>> +		return -EACCES;
>>>
>>> Never happen condition. Thus useless.
>>
>> pcim_iomap_table() can return NULL in case of allocation failure. Besides that,
>> isn't it a good practice always to think of the worst case?
> 
> No, it can't in the conditions your have in the code. See above the lines I left.
> If pcim_iomap_regions() successfully finished...

Nice catch, I didn't saw that. I added that validation because of coverity
analysis. I'll add a comment here to suppress this coverity false positive error.

> 
>>>> +	dev_info(dev, "DesignWare eDMA PCIe driver loaded completely\n");
>>>
>>> Useless.
>>
>> It's helpful for bring up, I can pass it to dbg.
> 
> It just shows that someone didn't use existing tools and features. This message and similar are useless.
> Hint: initcall_debug.

I wasn't aware of it. Thanks for the hint :)

> 
>>>> +	dev_info(dev, "DesignWare eDMA PCIe driver unloaded completely\n");
>>>
>>> Ditto.
>>
>> It's helpful for bring up, I can pass it to dbg.
> 
> Ditto.
>

Andy Shevchenko Jan. 19, 2019, 3:45 p.m. UTC | #5

On Tue, Jan 15, 2019 at 12:48:34PM +0000, Gustavo Pimentel wrote:
> On 15/01/2019 05:43, Andy Shevchenko wrote:
> > On Mon, Jan 14, 2019 at 11:38:02AM +0000, Gustavo Pimentel wrote:
> >> On 11/01/2019 19:47, Andy Shevchenko wrote:
> >>> On Fri, Jan 11, 2019 at 07:33:41PM +0100, Gustavo Pimentel wrote:

> >>>> +static bool disable_msix;
> >>>> +module_param(disable_msix, bool, 0644);
> >>>> +MODULE_PARM_DESC(disable_msix, "Disable MSI-X interrupts");
> >>>
> >>> Why?!
> >>> We are no allow new module parameters without very strong arguments.
> >>
> >> Since this is a reference driver and might be used to test customized HW
> >> solutions, I added this parameter to allow the possibility to test the solution
> >> forcing the MSI feature binding. This is required specially if who will test
> >> this solution has a Root Complex with both features available (MSI and MSI-X),
> >> because the Kernel will give always preference to MSI-X binding (assuming that
> >> the EP has also both features available).
> > 
> > Yes, you may do it for testing purposes, but it doesn't fit the kernel standards.
> 
> Ok, but how should I proceed? May I leave it or substitute by another way to do
> it? If so, how?
> As I said, the intended is to be only used for this test case, on normal
> operation the parameter it should be always false.

Keep out-of-tree patch for your needs.

> >>>> +	if (!pdata) {
> >>>> +		dev_err(dev, "%s missing data structure\n", pci_name(pdev));
> >>>> +		return -EFAULT;
> >>>> +	}
> >>>
> >>> Useless check.
> >>
> >> Why? It's just a precaution, isn't it a good practice always to think of the
> >> worst case?
> > 
> > You just can put an implicit requirement of pdata rather than doing this
> 
> Ok, how can I do it? What I should add to the code to force that?

Not adding, removing. That's what I put before.

> 
> > useless check. I don't believe it would make sense to have NULL pdata for the
> > driver since it wouldn't be functional anyhow.
> 
> Yes, you're right without pdata the driver can't do anything.

Gustavo Pimentel Jan. 21, 2019, 9:21 a.m. UTC | #6

On 19/01/2019 15:45, Andy Shevchenko wrote:
> On Tue, Jan 15, 2019 at 12:48:34PM +0000, Gustavo Pimentel wrote:
>> On 15/01/2019 05:43, Andy Shevchenko wrote:
>>> On Mon, Jan 14, 2019 at 11:38:02AM +0000, Gustavo Pimentel wrote:
>>>> On 11/01/2019 19:47, Andy Shevchenko wrote:
>>>>> On Fri, Jan 11, 2019 at 07:33:41PM +0100, Gustavo Pimentel wrote:
> 
>>>>>> +static bool disable_msix;
>>>>>> +module_param(disable_msix, bool, 0644);
>>>>>> +MODULE_PARM_DESC(disable_msix, "Disable MSI-X interrupts");
>>>>>
>>>>> Why?!
>>>>> We are no allow new module parameters without very strong arguments.
>>>>
>>>> Since this is a reference driver and might be used to test customized HW
>>>> solutions, I added this parameter to allow the possibility to test the solution
>>>> forcing the MSI feature binding. This is required specially if who will test
>>>> this solution has a Root Complex with both features available (MSI and MSI-X),
>>>> because the Kernel will give always preference to MSI-X binding (assuming that
>>>> the EP has also both features available).
>>>
>>> Yes, you may do it for testing purposes, but it doesn't fit the kernel standards.
>>
>> Ok, but how should I proceed? May I leave it or substitute by another way to do
>> it? If so, how?
>> As I said, the intended is to be only used for this test case, on normal
>> operation the parameter it should be always false.
> 
> Keep out-of-tree patch for your needs.

Ok.

> 
>>>>>> +	if (!pdata) {
>>>>>> +		dev_err(dev, "%s missing data structure\n", pci_name(pdev));
>>>>>> +		return -EFAULT;
>>>>>> +	}
>>>>>
>>>>> Useless check.
>>>>
>>>> Why? It's just a precaution, isn't it a good practice always to think of the
>>>> worst case?
>>>
>>> You just can put an implicit requirement of pdata rather than doing this
>>
>> Ok, how can I do it? What I should add to the code to force that?
> 
> Not adding, removing. That's what I put before.

I thought there was some API or code design to force that. Sorry my bad.

> 
>>
>>> useless check. I don't believe it would make sense to have NULL pdata for the
>>> driver since it wouldn't be functional anyhow.
>>
>> Yes, you're right without pdata the driver can't do anything.
>

Patch

diff --git a/drivers/dma/dw-edma/Kconfig b/drivers/dma/dw-edma/Kconfig
index 3016bed..c0838ce 100644
--- a/drivers/dma/dw-edma/Kconfig
+++ b/drivers/dma/dw-edma/Kconfig
@@ -7,3 +7,12 @@  config DW_EDMA
 	help
 	  Support the Synopsys DesignWare eDMA controller, normally
 	  implemented on endpoints SoCs.
+
+config DW_EDMA_PCIE
+	tristate "Synopsys DesignWare eDMA PCIe driver"
+	depends on PCI && PCI_MSI
+	select DW_EDMA
+	help
+	  Provides a glue-logic between the Synopsys DesignWare
+	  eDMA controller and an endpoint PCIe device. This also serves
+	  as a reference design to whom desires to use this IP.
diff --git a/drivers/dma/dw-edma/Makefile b/drivers/dma/dw-edma/Makefile
index 0c53033..8d45c0d 100644
--- a/drivers/dma/dw-edma/Makefile
+++ b/drivers/dma/dw-edma/Makefile
@@ -4,3 +4,4 @@  obj-$(CONFIG_DW_EDMA)		+= dw-edma.o
 dw-edma-$(CONFIG_DEBUG_FS)	:= dw-edma-v0-debugfs.o
 dw-edma-objs			:= dw-edma-core.o \
 					dw-edma-v0-core.o $(dw-edma-y)
+obj-$(CONFIG_DW_EDMA_PCIE)	+= dw-edma-pcie.o
diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
new file mode 100644
index 0000000..b96b3c4
--- /dev/null
+++ b/drivers/dma/dw-edma/dw-edma-pcie.c
@@ -0,0 +1,254 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
+ * Synopsys DesignWare eDMA PCIe driver
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/device.h>
+#include <linux/dma/edma.h>
+#include <linux/msi.h>
+
+#include "dw-edma-core.h"
+
+enum dw_edma_pcie_bar {
+	BAR_0,
+	BAR_1,
+	BAR_2,
+	BAR_3,
+	BAR_4,
+	BAR_5
+};
+
+struct dw_edma_pcie_data {
+	/* eDMA registers location */
+	enum dw_edma_pcie_bar		rg_bar;
+	off_t				rg_off;
+	size_t				rg_sz;
+	/* eDMA memory linked list location */
+	enum dw_edma_pcie_bar		ll_bar;
+	off_t				ll_off;
+	size_t				ll_sz;
+	/* eDMA memory data location */
+	enum dw_edma_pcie_bar		dt_bar;
+	off_t				dt_off;
+	size_t				dt_sz;
+	/* Other */
+	u32				version;
+	enum dw_edma_mode		mode;
+	u8				irqs_cnt;
+};
+
+static const struct dw_edma_pcie_data snps_edda_data = {
+	/* eDMA registers location */
+	.rg_bar				= BAR_0,
+	.rg_off				= 0x00001000,	/*  4 Kbytes */
+	.rg_sz				= 0x00002000,	/*  8 Kbytes */
+	/* eDMA memory linked list location */
+	.ll_bar				= BAR_2,
+	.ll_off				= 0x00000000,	/*  0 Kbytes */
+	.ll_sz				= 0x00800000,	/*  8 Mbytes */
+	/* eDMA memory data location */
+	.dt_bar				= BAR_2,
+	.dt_off				= 0x00800000,	/*  8 Mbytes */
+	.dt_sz				= 0x03800000,	/* 56 Mbytes */
+	/* Other */
+	.version			= 0,
+	.mode				= EDMA_MODE_UNROLL,
+	.irqs_cnt			= 1,
+};
+
+static bool disable_msix;
+module_param(disable_msix, bool, 0644);
+MODULE_PARM_DESC(disable_msix, "Disable MSI-X interrupts");
+
+static int dw_edma_pcie_probe(struct pci_dev *pdev,
+			      const struct pci_device_id *pid)
+{
+	const struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
+	struct device *dev = &pdev->dev;
+	struct dw_edma_chip *chip;
+	struct dw_edma *dw;
+	unsigned int irq_flags = PCI_IRQ_MSI;
+	int err, nr_irqs, i;
+
+	if (!pdata) {
+		dev_err(dev, "%s missing data structure\n", pci_name(pdev));
+		return -EFAULT;
+	}
+
+	/* Enable PCI device */
+	err = pcim_enable_device(pdev);
+	if (err) {
+		dev_err(dev, "%s enabling device failed\n", pci_name(pdev));
+		return err;
+	}
+
+	/* Mapping PCI BAR regions */
+	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar) |
+				       BIT(pdata->ll_bar) |
+				       BIT(pdata->dt_bar),
+				 pci_name(pdev));
+	if (err) {
+		dev_err(dev, "%s eDMA BAR I/O remapping failed\n",
+			pci_name(pdev));
+		return err;
+	}
+
+	pci_set_master(pdev);
+
+	/* DMA configuration */
+	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
+	if (err) {
+		dev_err(dev, "%s DMA mask set failed\n", pci_name(pdev));
+		return err;
+	}
+
+	err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
+	if (err) {
+		dev_err(dev, "%s consistent DMA mask set failed\n",
+			pci_name(pdev));
+		return err;
+	}
+
+	/* Data structure allocation */
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
+	if (!dw)
+		return -ENOMEM;
+
+	/* IRQs allocation */
+	if (!disable_msix)
+		irq_flags |= PCI_IRQ_MSIX;
+
+	nr_irqs = pci_alloc_irq_vectors(pdev, 1, pdata->irqs_cnt, irq_flags);
+	if (nr_irqs < 1) {
+		dev_err(dev, "%s failed to alloc IRQ vector (Number of IRQs=%u)\n",
+			pci_name(pdev), nr_irqs);
+		return -EPERM;
+	}
+
+	/* Data structure initialization */
+	chip->dw = dw;
+	chip->dev = dev;
+	chip->id = pdev->devfn;
+	chip->irq = pdev->irq;
+
+	if (!pcim_iomap_table(pdev))
+		return -EACCES;
+
+	dw->rg_region.vaddr = (dma_addr_t)pcim_iomap_table(pdev)[pdata->rg_bar];
+	dw->rg_region.vaddr += pdata->rg_off;
+	dw->rg_region.paddr = pdev->resource[pdata->rg_bar].start;
+	dw->rg_region.paddr += pdata->rg_off;
+	dw->rg_region.sz = pdata->rg_sz;
+
+	dw->ll_region.vaddr = (dma_addr_t)pcim_iomap_table(pdev)[pdata->ll_bar];
+	dw->ll_region.vaddr += pdata->ll_off;
+	dw->ll_region.paddr = pdev->resource[pdata->ll_bar].start;
+	dw->ll_region.paddr += pdata->ll_off;
+	dw->ll_region.sz = pdata->ll_sz;
+
+	dw->dt_region.vaddr = (dma_addr_t)pcim_iomap_table(pdev)[pdata->dt_bar];
+	dw->dt_region.vaddr += pdata->dt_off;
+	dw->dt_region.paddr = pdev->resource[pdata->dt_bar].start;
+	dw->dt_region.paddr += pdata->dt_off;
+	dw->dt_region.sz = pdata->dt_sz;
+
+	dw->version = pdata->version;
+	dw->mode = pdata->mode;
+	dw->nr_irqs = nr_irqs;
+
+	/* Debug info */
+	dev_dbg(dev, "Version:\t%u\n", dw->version);
+
+	dev_dbg(dev, "Mode:\t%s\n",
+		dw->mode == EDMA_MODE_LEGACY ? "Legacy" : "Unroll");
+
+	dev_dbg(dev, "Registers:\tBAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%pa, p=%pa)\n",
+		pdata->rg_bar, pdata->rg_off, pdata->rg_sz,
+		&dw->rg_region.vaddr, &dw->rg_region.paddr);
+
+	dev_dbg(dev, "L. List:\tBAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%pa, p=%pa)\n",
+		pdata->ll_bar, pdata->ll_off, pdata->ll_sz,
+		&dw->ll_region.vaddr, &dw->ll_region.paddr);
+
+	dev_dbg(dev, "Data:\tBAR=%u, off=0x%.8lx, sz=0x%zx bytes, addr(v=%pa, p=%pa)\n",
+		pdata->dt_bar, pdata->dt_off, pdata->dt_sz,
+		&dw->dt_region.vaddr, &dw->dt_region.paddr);
+
+	dev_dbg(dev, "Nr. IRQs:\t%u\n", dw->nr_irqs);
+
+	/* Validating if PCI interrupts were enabled */
+	if (!pci_dev_msi_enabled(pdev)) {
+		dev_err(dev, "%s enable interrupt failed\n", pci_name(pdev));
+		return -EPERM;
+	}
+
+	/*
+	 * Acquiring PCI MSI(-X) configuration (address and data) for
+	 * setting it later on eDMA interrupt registers
+	 */
+	dw->msi = devm_kcalloc(dev, nr_irqs, sizeof(*dw->msi), GFP_KERNEL);
+	if (!dw->msi)
+		return -ENOMEM;
+
+	for (i = 0; i < nr_irqs; i++)
+		get_cached_msi_msg(pci_irq_vector(to_pci_dev(dev), i),
+				   &dw->msi[i]);
+
+	/* Starting eDMA driver */
+	err = dw_edma_probe(chip);
+	if (err) {
+		dev_err(dev, "%s eDMA probe failed\n", pci_name(pdev));
+		return err;
+	}
+
+	/* Saving data structure reference */
+	pci_set_drvdata(pdev, chip);
+
+	dev_info(dev, "DesignWare eDMA PCIe driver loaded completely\n");
+
+	return 0;
+}
+
+static void dw_edma_pcie_remove(struct pci_dev *pdev)
+{
+	struct dw_edma_chip *chip = pci_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	int err;
+
+	/* Stopping eDMA driver */
+	err = dw_edma_remove(chip);
+	if (err)
+		dev_warn(dev, "can't remove device properly: %d\n", err);
+
+	/* Freeing IRQs */
+	pci_free_irq_vectors(pdev);
+
+	dev_info(dev, "DesignWare eDMA PCIe driver unloaded completely\n");
+}
+
+static const struct pci_device_id dw_edma_pcie_id_table[] = {
+	{ PCI_DEVICE_DATA(SYNOPSYS, EDDA, &snps_edda_data) },
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, dw_edma_pcie_id_table);
+
+static struct pci_driver dw_edma_pcie_driver = {
+	.name		= "dw-edma-pcie",
+	.id_table	= dw_edma_pcie_id_table,
+	.probe		= dw_edma_pcie_probe,
+	.remove		= dw_edma_pcie_remove,
+};
+
+module_pci_driver(dw_edma_pcie_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Synopsys DesignWare eDMA PCIe driver");
+MODULE_AUTHOR("Gustavo Pimentel <[e-mail]>");