On Mon, Feb 24, 2014 at 8:00 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote: > Hi Magnus, > > I noticed during randconfig testing that you enabled DMABOUNCE for the > pci-rcar-gen2 driver as posted in this patch https://lkml.org/lkml/2014/2/5/30 Hi Arnd, The patch that you are referring to has been updated a few times, but I believe your comments are still valid for the series [PATCH v2 00/08] PCI: rcar: Recent driver patches from Ben Dooks and me (V2) > I didn't see the original post unfortunately, but I fear we have to > revert it and come up with a better solution, as your approach seems > wrong on a number of levels: > > * We really don't want any new users of DMABOUNCE on ARM but instead leave > it to the PXA/IXP/SA1100 based platforms using it today. > > * If your SoCs have an IOMMU, you should really use it, as that would > give you much better performance than bounce buffers anyway > > * If the hardware is so screwed up that you have to use bounce buffers, > use the SWIOTLB code that is reasonably modern. >From my point of view we need some kind of bounce buffer unless we have IOMMU support. I understand that an IOMMU would be much better than a software-based implementation. If it is possible to use an IOMMU with these devices remain to be seen. I didn't know about the SWIOTLB code, neither did I know that DMABOUNCE was supposed to be avoided. Now I do! I do realize that my following patches madly mix potential bus code and actual device support, however.. [PATCH v2 06/08] PCI: rcar: Add DMABOUNCE support [PATCH 07/08] PCI: rcar: Enable BOUNCE in case of HIGHMEM .. without my patches the driver does not handle CONFIG_BOUNCE and CONFIG_VMSPLIT_2G. > * The window base and size in the driver should not be hardcoded, as > this is likely not a device specific property but rather an artifact of > how it's connected. Use the standard "dma-ranges" property. I'm afraid that I may not understand your concern fully here. From my view the existing driver (without my patch) is having hard coded board-specific memory base and a hard coded size in it, and with my patches I try to rework that. Do you have any existing example code to recommend me? Regarding what the device can do and/or how it is connected - here is some hardware information: 1) 3 on-chip PCI bridges per SoC with USB EHCI/OHCI controllers on each of them (PCI bridge code is also shared between multiple SoCs and of course multiple boards). 2) The systems have 40-bit physical address space and the CPU can address this when LPAE is enabled. 3) System RAM is available in two banks - one bank in the lowest 32-bits (top 8 bits set to 0) and another bank in higher space. 4) The PCI bridge has a 32-bit base address for the windows with alignment requirement (needs to be evenly aligned based on size) 5) Each PCI bridge instance has two windows, but supported size differs. One window supports up to 2 GiB, another 256MiB. Without IOMMU available I came to the conclusion that I need both BOUNCE and DMABOUNCE to support the above hardware. > * You should not need your own dma_map_ops in the driver. What you > do there isn't really specific to your host but should live in some > place where it can be shared with other drivers. I think this boils down to the less-than-32-bit bus master capability and also the poor match to a DMA zone. Consider 24-bit ISA DMA address space vs 31-bit DMA space on a 32-bit system. I may be wrong, but a DMA zone in my mind is something suitable for the classic ISA DMA, not so much for modern complex systems with multiple devices that come with different bus master capabilities. That said, of course it makes sense to share code whenever possible. Can you explain a bit more what kind of code you would like to have broken out? > * The implementation of the dma_map_ops is wrong: at the very least, > you cannot use the dma_mask of the pci host when translating > calls from the device, since each pci device can have a different > dma mask that is set by the driver if it needs larger or smaller > than 32-bit DMA space. The current version of the driver (with or without my patch) seems to leave all dma mask handling left out. I understand that this is poor programming practise and I would like to see that fixed. As you noticed, I did not try to fix this issue in my patch. It may be worth noticing that the PCI devices hanging off the PCI bridge instances are all on-chip fixed to OHCI and EHCI and the drivers for these USB host controllers do not seem to try to set the dma mask as it is today. So we are talking about a fixed set of PCI devices here and not external PCI bus with general purpose PCI drivers. I'm not sure if that makes things much better though.. So yes, I'd like the PCI bridge driver to be fixed with proper dma mask handling. The question is just what that mask is supposed to represent on a system where we a) may have IOMMU (and if so we can address 40 bits), and if not we b) are limited to 31 bits but we also have a non-zero base address. I'm not sure how to represent that information with a single dma mask. Any suggestions? > * The block bounce code can't be enabled if CONFIG_BLOCK is disabled, > this is how I noticed your changes. Do you mean that the following patch is causing some kind of build error? [PATCH 07/08] PCI: rcar: Enable BOUNCE in case of HIGHMEM If possible, can you please let me know which patches that you want me to rework? > * The block layer assumes that it will bounce any highmem pages, > but that isn't necessarily what you want here: if you have 2GB > of lowmem space (CONFIG_VMSPLIT_2G) or more, you will also have > lowmem pages that need bouncing. Good to hear that you also came to the conclusion that the two cases need to be handled separately. =) The lowmem bounce case (CONFIG_VMSPLIT_2G) is implemented in: [PATCH v2 06/08] PCI: rcar: Add DMABOUNCE support The block layer bounce is also enabled in: [PATCH 07/08] PCI: rcar: Enable BOUNCE in case of HIGHMEM > * (completely unrelated, I noticed you set up a bogus I/O space > window. Don't do that, you will get get a panic if a device happens > to use it. Not providing an resource should work fine though) Right. To be honest, I'm not sure why the original author implemented this. Similar to the dma mask bits this is something that I would like to fix, but it has been out of scope for my patches so far. > On the bright side, your other changes in the same series all look > good. Thanks especially for sorting out the probe() function, I was > already wondering if we could change that the way you did. Thanks. Ideally I'd like to support bind and unbind on the bridge too (CARDBUS can hotplug PCI, so should we!), but I suppose that sorting out the bounce bits is more important. Also, the SWIOTLB code, are you aware of any existing ARM users? I also wonder if the code can work with multiple devices - the bits in lib/swiotlb.c looks like single instance with global system wide support only - perhaps it needs rework to support 3 PCI bridges? Thanks for your help, I appreciate your feedback. Cheers, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
