diff mbox

[2/2] ixp4xx_eth: Setup coherent_dma_mask

Message ID m3d2i1atmq.fsf@t19.piap.pl
State New
Headers show

Commit Message

Krzysztof Hałasa March 5, 2014, 9:21 a.m. UTC 
Simon Kågström <simon.kagstrom@netinsight.net> writes:

> --- a/drivers/net/ethernet/xscale/ixp4xx_eth.c
> +++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
> @@ -1426,6 +1426,10 @@ static int eth_init_one(struct platform_device *pdev)
>  	port->netdev = dev;
>  	port->id = pdev->id;
>  
> +	err = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
> +	if (err < 0)
> +		goto err_free;
> +

Both David and the DMA API say 32 bits must be the default. OTOH there
is other code like this in the kernel, guess IXP4xx is not alone with
such constrains.
Personally I think it should be done by the API, with ixp4xx core code
providing default 32-bit masks (FYI I've attached a 3.13 patch
I personally use).

I'm a bit tired of this and was waiting until Russell's DMA mask code is
merged (I'm busy doing other stuff ATM), perhaps there could be some way
out done incrementally now (sticking to the 32-bit default as a result).

What I don't like is specifying the "platform" DMA mask(s) for each
platform, as if the CPU built-in interface was anything-platform.
It's against my good programming rules, and it also increases the line
count unnecessarily which probably doesn't make Linus happy either.

Comments

Russell King - ARM Linux March 5, 2014, 9:43 a.m. UTC | #1 
On Wed, Mar 05, 2014 at 10:21:01AM +0100, Krzysztof Hałasa wrote:
> Simon Kågström <simon.kagstrom@netinsight.net> writes:
> 
> > --- a/drivers/net/ethernet/xscale/ixp4xx_eth.c
> > +++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
> > @@ -1426,6 +1426,10 @@ static int eth_init_one(struct platform_device *pdev)
> >  	port->netdev = dev;
> >  	port->id = pdev->id;
> >  
> > +	err = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
> > +	if (err < 0)
> > +		goto err_free;
> > +
> 
> Both David and the DMA API say 32 bits must be the default. OTOH there
> is other code like this in the kernel, guess IXP4xx is not alone with
> such constrains.

If you have drivers missing this call, that's part of the problem:

| For correct operation, you must interrogate the kernel in your device
| probe routine to see if the DMA controller on the machine can properly
| support the DMA addressing limitation your device has.  It is good
| style to do this even if your device holds the default setting,
| because this shows that you did think about these issues wrt. your
| device.
| 
| The query is performed via a call to dma_set_mask_and_coherent():
| 
|         int dma_set_mask_and_coherent(struct device *dev, u64 mask);
| 
| which will query the mask for both streaming and coherent APIs together.
| If you have some special requirements, then the following two separate
| queries can be used instead:
| 
|         The query for streaming mappings is performed via a call to
|         dma_set_mask():
| 
|                 int dma_set_mask(struct device *dev, u64 mask);
| 
|         The query for consistent allocations is performed via a call
|         to dma_set_coherent_mask():
| 
|                 int dma_set_coherent_mask(struct device *dev, u64 mask);
| 
| Here, dev is a pointer to the device struct of your device, and mask
| is a bit mask describing which bits of an address your device
| supports.  It returns zero if your card can perform DMA properly on
| the machine given the address mask you provided.  In general, the
| device struct of your device is embedded in the bus specific device
| struct of your device.  For example, a pointer to the device struct of
| your PCI device is pdev->dev (pdev is a pointer to the PCI device
| struct of your device).
| 
| If it returns non-zero, your device cannot perform DMA properly on
| this platform, and attempting to do so will result in undefined
| behavior.  You must either use a different mask, or not use DMA.
| 
| ...
Krzysztof Hałasa March 5, 2014, 9:55 a.m. UTC | #2 
Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

>> > +	err = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
>> > +	if (err < 0)
>> > +		goto err_free;
>> > +
>> 
>> Both David and the DMA API say 32 bits must be the default. OTOH there
>> is other code like this in the kernel, guess IXP4xx is not alone with
>> such constrains.
>
> If you have drivers missing this call, that's part of the problem:
>
> | For correct operation, you must interrogate the kernel in your device
> | probe routine to see if the DMA controller on the machine can properly
> | support the DMA addressing limitation your device has.

Well, we already know it can. Actually, the DMA controller is a part of
the CPU + RAM controller chip :-)

But I guess with this new wording it's something the drivers can use.
Simon Kagstrom March 5, 2014, 10:07 a.m. UTC | #3 
On Wed, 5 Mar 2014 10:55:02 +0100
Krzysztof Hałasa <khalasa@piap.pl> wrote:

> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> > If you have drivers missing this call, that's part of the problem:
> >
> > | For correct operation, you must interrogate the kernel in your device
> > | probe routine to see if the DMA controller on the machine can properly
> > | support the DMA addressing limitation your device has.
> 
> Well, we already know it can. Actually, the DMA controller is a part of
> the CPU + RAM controller chip :-)
> 
> But I guess with this new wording it's something the drivers can use.

With that wording, I would say these two patches should be applicable:
The first corrects dma_set_coherent_mask() to actually setup the
coherent_dma_mask (and make it applicable outside of the PCI domain)
and the second sets it up in the driver, as per

> > | support the DMA addressing limitation your device has.  It is good
> > | style to do this even if your device holds the default setting,
> > | because this shows that you did think about these issues wrt. your
> > | device.

Without these patches (or Krzysztofs patch), the ixp4xx platform is
broken for many common configurations.

// Simon
Russell King - ARM Linux March 5, 2014, 10:12 a.m. UTC | #4 
On Wed, Mar 05, 2014 at 10:55:02AM +0100, Krzysztof Hałasa wrote:
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> 
> >> > +	err = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
> >> > +	if (err < 0)
> >> > +		goto err_free;
> >> > +
> >> 
> >> Both David and the DMA API say 32 bits must be the default. OTOH there
> >> is other code like this in the kernel, guess IXP4xx is not alone with
> >> such constrains.
> >
> > If you have drivers missing this call, that's part of the problem:
> >
> > | For correct operation, you must interrogate the kernel in your device
> > | probe routine to see if the DMA controller on the machine can properly
> > | support the DMA addressing limitation your device has.
> 
> Well, we already know it can. Actually, the DMA controller is a part of
> the CPU + RAM controller chip :-)
> 
> But I guess with this new wording it's something the drivers can use.

What I quoted is not "new wording" apart from the addition of the interface
which sets both masks.  This has been documented as a requirement since
before there was a dma_* API (the pci_dma_* API predated that, and also
had this requirement.)

It's something that has long been ignored on ARM because "oh we can do it
in the platform code" - not anymore, not with DT.  This short-cut has
finally bitten, and we now must conform.

So, in the interests of everything working as it should, it's something
that needs to be fixed irrespective of anything else: all our drivers
which perform DMA should make the appropriate DMA mask calls.
diff mbox

Patch

diff --git a/arch/arm/mach-ixp4xx/common-pci.c b/arch/arm/mach-ixp4xx/common-pci.c
index 6d6bde3..cefb80b 100644
--- a/arch/arm/mach-ixp4xx/common-pci.c
+++ b/arch/arm/mach-ixp4xx/common-pci.c
@@ -316,32 +316,6 @@  static int abort_handler(unsigned long addr, unsigned int fsr, struct pt_regs *r
 }
 
 
-static int ixp4xx_needs_bounce(struct device *dev, dma_addr_t dma_addr, size_t size)
-{
-	return (dma_addr + size) >= SZ_64M;
-}
-
-/*
- * Setup DMA mask to 64MB on PCI devices. Ignore all other devices.
- */
-static int ixp4xx_pci_platform_notify(struct device *dev)
-{
-	if(dev->bus == &pci_bus_type) {
-		*dev->dma_mask =  SZ_64M - 1;
-		dev->coherent_dma_mask = SZ_64M - 1;
-		dmabounce_register_dev(dev, 2048, 4096, ixp4xx_needs_bounce);
-	}
-	return 0;
-}
-
-static int ixp4xx_pci_platform_notify_remove(struct device *dev)
-{
-	if(dev->bus == &pci_bus_type) {
-		dmabounce_unregister_dev(dev);
-	}
-	return 0;
-}
-
 void __init ixp4xx_pci_preinit(void)
 {
 	unsigned long cpuid = read_cpuid_id();
@@ -475,20 +449,8 @@  int ixp4xx_setup(int nr, struct pci_sys_data *sys)
 	pci_add_resource_offset(&sys->resources, &res[0], sys->io_offset);
 	pci_add_resource_offset(&sys->resources, &res[1], sys->mem_offset);
 
-	platform_notify = ixp4xx_pci_platform_notify;
-	platform_notify_remove = ixp4xx_pci_platform_notify_remove;
-
 	return 1;
 }
 
-int dma_set_coherent_mask(struct device *dev, u64 mask)
-{
-	if (mask >= SZ_64M - 1)
-		return 0;
-
-	return -EIO;
-}
-
 EXPORT_SYMBOL(ixp4xx_pci_read);
 EXPORT_SYMBOL(ixp4xx_pci_write);
-EXPORT_SYMBOL(dma_set_coherent_mask);
diff --git a/arch/arm/mach-ixp4xx/common.c b/arch/arm/mach-ixp4xx/common.c
index a7906eb..0b6d146 100644
--- a/arch/arm/mach-ixp4xx/common.c
+++ b/arch/arm/mach-ixp4xx/common.c
@@ -30,8 +30,8 @@ 
 #include <linux/export.h>
 #include <linux/gpio.h>
 #include <linux/cpu.h>
+#include <linux/pci.h>
 #include <linux/sched_clock.h>
-
 #include <mach/udc.h>
 #include <mach/hardware.h>
 #include <mach/io.h>
@@ -40,7 +40,6 @@ 
 #include <asm/page.h>
 #include <asm/irq.h>
 #include <asm/system_misc.h>
-
 #include <asm/mach/map.h>
 #include <asm/mach/irq.h>
 #include <asm/mach/time.h>
@@ -578,6 +577,56 @@  void ixp4xx_restart(enum reboot_mode mode, const char *cmd)
 	}
 }
 
+#ifdef CONFIG_PCI
+static int ixp4xx_needs_bounce(struct device *dev, dma_addr_t dma_addr, size_t size)
+{
+	return (dma_addr + size) >= SZ_64M;
+}
+
+static int ixp4xx_platform_notify_remove(struct device *dev)
+{
+	if (dev->bus == &pci_bus_type)
+		dmabounce_unregister_dev(dev);
+
+	return 0;
+}
+#endif
+
+/*
+ * Setup DMA mask to 64MB on PCI devices and 4 GB on all other things.
+ */
+static int ixp4xx_platform_notify(struct device *dev)
+{
+	dev->dma_mask = &dev->coherent_dma_mask;
+
+#ifdef CONFIG_PCI
+	if (dev_is_pci(dev)) {
+		dev->coherent_dma_mask = DMA_BIT_MASK(28); /* 64 MB */
+		dmabounce_register_dev(dev, 2048, 4096, ixp4xx_needs_bounce);
+		return 0;
+	}
+#endif
+
+	dev->coherent_dma_mask = DMA_BIT_MASK(32);
+	return 0;
+}
+
+int dma_set_coherent_mask(struct device *dev, u64 mask)
+{
+#ifdef CONFIG_PCI
+	if (dev_is_pci(dev))
+		mask &= DMA_BIT_MASK(28); /* 64 MB */
+#endif
+
+	if ((mask & DMA_BIT_MASK(28)) == DMA_BIT_MASK(28)) {
+		dev->coherent_dma_mask = mask;
+		return 0;
+	}
+
+	return -EIO;		/* device wanted sub-64MB mask */
+}
+EXPORT_SYMBOL(dma_set_coherent_mask);
+
 #ifdef CONFIG_IXP4XX_INDIRECT_PCI
 /*
  * In the case of using indirect PCI, we simply return the actual PCI
@@ -600,12 +649,16 @@  static void ixp4xx_iounmap(void __iomem *addr)
 	if (!is_pci_memory((__force u32)addr))
 		__iounmap(addr);
 }
+#endif
 
 void __init ixp4xx_init_early(void)
 {
+	platform_notify = ixp4xx_platform_notify;
+#ifdef CONFIG_PCI
+	platform_notify_remove = ixp4xx_platform_notify_remove;
+#endif
+#ifdef CONFIG_IXP4XX_INDIRECT_PCI
 	arch_ioremap_caller = ixp4xx_ioremap_caller;
 	arch_iounmap = ixp4xx_iounmap;
-}
-#else
-void __init ixp4xx_init_early(void) {}
 #endif
+}