Hi Andy,
Sorry for the delay in response. I will do what you suggest in your email. I do have one response to one of your comments below.
On Thu, Jul 2, 2020 at 4:43 AM Andy Shevchenko andriy.shevchenko@linux.intePHYSl.com wrote:
On Wed, Jul 01, 2020 at 05:21:38PM -0400, Jim Quinlan wrote:
The new field 'dma_range_map' in struct device is used to facilitate the use of single or multiple offsets between mapping regions of cpu addrs and dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only capable of holding a single uniform offset and had no region bounds checking.
The function of_dma_get_range() has been modified so that it takes a single argument -- the device node -- and returns a map, NULL, or an error code. The map is an array that holds the information regarding the DMA regions. Each range entry contains the address offset, the cpu_start address, the dma_start address, and the size of the region.
of_dma_configure() is the typical manner to set range offsets but there are a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel driver code. These cases now invoke the function dma_attach_offset_range(dev, cpu_addr, dma_addr, size).
...
if (dev && dev->dma_range_map)pfn -= (unsigned long)PFN_DOWN(dma_offset_from_phys_addr(dev, PFN_PHYS(pfn)));Instead of casting use PHYS_PFN() and it will be consistent with latter in the same line.
if (dev && dev->dma_range_map)pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, addr));Ditto.
...
dev_err(dev, "set dma_offset%08llx%s\n", KEYSTONE_HIGH_PHYS_START- KEYSTONE_LOW_PHYS_START, ret ? " failed" : "");Please, avoid such indentation. Better split it to the three lines, argument per line (expect dev which will go on the first one).
This applies to all similar places.
...
unsigned long pfn = (dma_handle >> PAGE_SHIFT);PHYS_PFN() / PFN_DOWN() ?
if (!WARN_ON(!dev) && dev->dma_range_map)pfn += (unsigned long)PFN_DOWN(dma_offset_from_dma_addr(dev, dma_handle));PHYS_PFN() ?
...
r = kcalloc(num_ranges + 1, sizeof(struct bus_dma_region), GFP_KERNEL);sizeof(*r) ?
if (!r)return ERR_PTR(-ENOMEM);...
ret = IS_ERR(map) ? PTR_ERR(map) : 0;PTR_ERR_OR_ZERO()
...
/* We want the offset map to be device-managed, so alloc & copy */dev->dma_range_map = devm_kcalloc(dev, num_ranges + 1, sizeof(*r),GFP_KERNEL);The question is how many times per device lifetime this can be called?
Someone else questioned this. There are two cases that come to mind: our overnight test which load/unloads the RC driver over and over, and a customer that does an unbind/bind of the RC driver when their EP is hung and wants to restart. Both cases are atypical and are weak arguments to justify the second copy. I will drop the copy.
Thanks, Jim Quinlan Broadcom STB
...
if (!dev->dma_range_map)return -ENOMEM;memcpy((void *)dev->dma_range_map, map, sizeof(*r) * num_ranges + 1);If it's continuous, perhaps kmemdup() ?
...
rc = IS_ERR(map) ? PTR_ERR(map) : 0;PTR_ERR_OR_ZERO()
...
= dma_offset_from_phys_addr(dev, PFN_PHYS(mem->pfn_base));return (dma_addr_t)PFN_PHYS(mem->pfn_base) - dma_offset;Looking at this more, I think you need to introduce in the same header (pfn.h) something like:
#define PFN_DMA_ADDR() #define DMA_ADDR_PFN()-- With Best Regards, Andy Shevchenko