| Message ID | m3d2i1atmq.fsf@t19.piap.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. | | ...
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.
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
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 --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 +}