| Message ID | 1373665694-7580-5-git-send-email-santosh.shilimkar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 12, 2013 at 05:48:13PM -0400, Santosh Shilimkar wrote: > Most of the kernel code assumes that max*pfn is maximum pfns because > the physical start of memory is expected to be PFN0. Since this > assumption is not true on ARM architectures, the meaning of max*pfn > is number of memory pages. This is done to keep drivers happy which > are making use of of these variable to calculate the dma bounce limit > using dma_mask. > > Now since we have a architecture override possibility for DMAable > maximum pfns, lets make meaning of max*pfns as maximum pnfs on ARM > as well. > > In the patch, the dma_to_pfn/pfn_to_dma() pair is hacked to take care of > the physical memory offset. It is done this way just to enable testing > since its understood that it can come in way of single zImage. As Santosh says, this is a hack - but we need to have a discussion about how to handle translations from PFN to bus addresses. Currently, the way we do that on ARM is mostly assume that physical addresses are the same as bus addresses, but that's not true everywhere, and certainly isn't true when you have a 32-bit DMA controller which can access physical memory, where the physical memory is above 4GB in physical space. We have certain platforms where the DMA address is already being programmed into a controller with less than 32-bits in its address register, and with a physical memory offset - and of course this case just works out of the box because the high bits are ignored by the device. What I'm basically saying is we've had this problem for a while, and we've lived with it by hoping and hacking, and adjusting max*pfn, but this is not long-term sustainable. We *need* to get away from the idea that DMA addresses are physical addresses and device DMA masks have some relationship to physical addresses. Consider for a moment: PCI address 0x00000000 ---> physical address 0xc0000000. You plug a card in which can't do 32-bit addressing (remember, there are such PCI cards in the past...). The driver sets the DMA mask to 0x0fffffff (or whatever). How does that relate to the PCI bus address? It's 0x00000000 to 0x0fffffff. How does that relate to the physical address space? 0xc0000000 to 0xcfffffff. This is why DMA masks can't be treated as some notional address limit. It just doesn't work when you have bus offsets. And the extreme case of that is LPAE with all system memory above the 4GB physical mark, with 32-bit DMA capable peripherals - which we're starting to see now. Ideally, I think we need some kind of per-bus DT property to describe the memory which can be accessed from the bus - to do it properly to cover the cases we've already seen, that would be an offset and a size. We then need some way for dma_to_pfn() and pfn_to_dma() to efficiently get at that information - bear in mind that they're hot paths when doing DMA mappings and the like. I doubt we want to be looking up the same property time and time again inside them.
On Fri, Jul 12, 2013 at 05:48:13PM -0400, Santosh Shilimkar wrote: > diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h > index 5b579b9..b2d5937 100644 > --- a/arch/arm/include/asm/dma-mapping.h > +++ b/arch/arm/include/asm/dma-mapping.h > @@ -47,12 +47,12 @@ static inline int dma_set_mask(struct device *dev, u64 mask) > #ifndef __arch_pfn_to_dma > static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn) > { > - return (dma_addr_t)__pfn_to_bus(pfn); > + return (dma_addr_t)__pfn_to_bus(pfn + PHYS_PFN_OFFSET); > } > > static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr) > { > - return __bus_to_pfn(addr); > + return __bus_to_pfn(addr) - PHYS_PFN_OFFSET; > } > > static inline void *dma_to_virt(struct device *dev, dma_addr_t addr) > @@ -64,15 +64,16 @@ static inline dma_addr_t virt_to_dma(struct device *dev, void *addr) > { > return (dma_addr_t)__virt_to_bus((unsigned long)(addr)); > } > + > #else > static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn) > { > - return __arch_pfn_to_dma(dev, pfn); > + return __arch_pfn_to_dma(dev, pfn + PHYS_PFN_OFFSET); > } > > static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr) > { > - return __arch_dma_to_pfn(dev, addr); > + return __arch_dma_to_pfn(dev, addr) - PHYS_PFN_OFFSET; > } > > static inline void *dma_to_virt(struct device *dev, dma_addr_t addr) > @@ -86,6 +87,13 @@ static inline dma_addr_t virt_to_dma(struct device *dev, void *addr) > } > #endif Note that I don't think the above are correct - the 'pfn' argument to the above functions already includes the PFN offset of physical memory - they're all physical_address >> PAGE_SHIFT values. > +/* The ARM override for dma_max_pfn() */ > +static inline unsigned long dma_max_pfn(struct device *dev) > +{ > + return dma_to_pfn(dev, *dev->dma_mask); > +} > +#define dma_max_pfn(dev) dma_max_pfn(dev) So I'd suggest this becomes: return PHYS_PFN_OFFSET + dma_to_pfn(dev, *dev->dma_mask);
On Wednesday 31 July 2013 06:56 AM, Russell King - ARM Linux wrote: > On Fri, Jul 12, 2013 at 05:48:13PM -0400, Santosh Shilimkar wrote: >> diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h >> index 5b579b9..b2d5937 100644 >> --- a/arch/arm/include/asm/dma-mapping.h >> +++ b/arch/arm/include/asm/dma-mapping.h >> @@ -47,12 +47,12 @@ static inline int dma_set_mask(struct device *dev, u64 mask) >> #ifndef __arch_pfn_to_dma >> static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn) >> { >> - return (dma_addr_t)__pfn_to_bus(pfn); >> + return (dma_addr_t)__pfn_to_bus(pfn + PHYS_PFN_OFFSET); >> } >> >> static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr) >> { >> - return __bus_to_pfn(addr); >> + return __bus_to_pfn(addr) - PHYS_PFN_OFFSET; >> } >> >> static inline void *dma_to_virt(struct device *dev, dma_addr_t addr) >> @@ -64,15 +64,16 @@ static inline dma_addr_t virt_to_dma(struct device *dev, void *addr) >> { >> return (dma_addr_t)__virt_to_bus((unsigned long)(addr)); >> } >> + >> #else >> static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn) >> { >> - return __arch_pfn_to_dma(dev, pfn); >> + return __arch_pfn_to_dma(dev, pfn + PHYS_PFN_OFFSET); >> } >> >> static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr) >> { >> - return __arch_dma_to_pfn(dev, addr); >> + return __arch_dma_to_pfn(dev, addr) - PHYS_PFN_OFFSET; >> } >> >> static inline void *dma_to_virt(struct device *dev, dma_addr_t addr) >> @@ -86,6 +87,13 @@ static inline dma_addr_t virt_to_dma(struct device *dev, void *addr) >> } >> #endif > > Note that I don't think the above are correct - the 'pfn' argument to the > above functions already includes the PFN offset of physical memory - > they're all physical_address >> PAGE_SHIFT values. > Right. Updated patch pushed into the patch system. (patch 7805/1) Regards, Santosh
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h index 5b579b9..b2d5937 100644 --- a/arch/arm/include/asm/dma-mapping.h +++ b/arch/arm/include/asm/dma-mapping.h @@ -47,12 +47,12 @@ static inline int dma_set_mask(struct device *dev, u64 mask) #ifndef __arch_pfn_to_dma static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn) { - return (dma_addr_t)__pfn_to_bus(pfn); + return (dma_addr_t)__pfn_to_bus(pfn + PHYS_PFN_OFFSET); } static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr) { - return __bus_to_pfn(addr); + return __bus_to_pfn(addr) - PHYS_PFN_OFFSET; } static inline void *dma_to_virt(struct device *dev, dma_addr_t addr) @@ -64,15 +64,16 @@ static inline dma_addr_t virt_to_dma(struct device *dev, void *addr) { return (dma_addr_t)__virt_to_bus((unsigned long)(addr)); } + #else static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn) { - return __arch_pfn_to_dma(dev, pfn); + return __arch_pfn_to_dma(dev, pfn + PHYS_PFN_OFFSET); } static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr) { - return __arch_dma_to_pfn(dev, addr); + return __arch_dma_to_pfn(dev, addr) - PHYS_PFN_OFFSET; } static inline void *dma_to_virt(struct device *dev, dma_addr_t addr) @@ -86,6 +87,13 @@ static inline dma_addr_t virt_to_dma(struct device *dev, void *addr) } #endif +/* The ARM override for dma_max_pfn() */ +static inline unsigned long dma_max_pfn(struct device *dev) +{ + return dma_to_pfn(dev, *dev->dma_mask); +} +#define dma_max_pfn(dev) dma_max_pfn(dev) + /* * DMA errors are defined by all-bits-set in the DMA address. */ diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c index 6833cbe..588a2c1 100644 --- a/arch/arm/mm/init.c +++ b/arch/arm/mm/init.c @@ -420,12 +420,10 @@ void __init bootmem_init(void) * This doesn't seem to be used by the Linux memory manager any * more, but is used by ll_rw_block. If we can get rid of it, we * also get rid of some of the stuff above as well. - * - * Note: max_low_pfn and max_pfn reflect the number of _pages_ in - * the system, not the maximum PFN. */ - max_low_pfn = max_low - PHYS_PFN_OFFSET; - max_pfn = max_high - PHYS_PFN_OFFSET; + min_low_pfn = min; + max_low_pfn = max_low; + max_pfn = max_high; } /* @@ -531,7 +529,7 @@ static inline void free_area_high(unsigned long pfn, unsigned long end) static void __init free_highpages(void) { #ifdef CONFIG_HIGHMEM - unsigned long max_low = max_low_pfn + PHYS_PFN_OFFSET; + unsigned long max_low = max_low_pfn; struct memblock_region *mem, *res; /* set highmem page free */
Most of the kernel code assumes that max*pfn is maximum pfns because the physical start of memory is expected to be PFN0. Since this assumption is not true on ARM architectures, the meaning of max*pfn is number of memory pages. This is done to keep drivers happy which are making use of of these variable to calculate the dma bounce limit using dma_mask. Now since we have a architecture override possibility for DMAable maximum pfns, lets make meaning of max*pfns as maximum pnfs on ARM as well. In the patch, the dma_to_pfn/pfn_to_dma() pair is hacked to take care of the physical memory offset. It is done this way just to enable testing since its understood that it can come in way of single zImage. Cc: Russell King <linux@arm.linux.org.uk> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Nicolas Pitre <nicolas.pitre@linaro.org> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> --- arch/arm/include/asm/dma-mapping.h | 16 ++++++++++++---- arch/arm/mm/init.c | 10 ++++------ 2 files changed, 16 insertions(+), 10 deletions(-)