[PATCH 4/4] POWERPC: Merge 32 and 64-bit dma code

[Becky Bruce]

On Sep 8, 2008, at 5:03 PM, Christoph Hellwig wrote:

>> -	if (unlikely(dev == NULL || dev->archdata.dma_ops == NULL))
>> +
>> +	if (unlikely(dev == NULL) || dev->archdata.dma_ops == NULL) {
>> +#ifdef CONFIG_PPC64
>> 		return NULL;
>> +#else
>> +		/* Use default on 32-bit if dma_ops is not set up */
>> +		return &dma_direct_ops;
>> +#endif
>> +	}
>> +
>
> This is okay for the transition, but I think long-term it should
> be setup for all busses.

Agreed!

>
>
>> }
>>
>> @@ -132,7 +154,14 @@ static inline dma_addr_t  
>> dma_map_single_attrs(struct device *dev,
>> 	struct dma_mapping_ops *dma_ops = get_dma_ops(dev);
>>
>> 	BUG_ON(!dma_ops);
>> -	return dma_ops->map_single(dev, cpu_addr, size, direction, attrs);
>> +
>> +	if (dma_ops->map_single)
>> +		return dma_ops->map_single(dev, cpu_addr, size, direction,
>> +					   attrs);
>> +
>> +	return dma_ops->map_page(dev, virt_to_page(cpu_addr),
>> +				 (unsigned long)cpu_addr % PAGE_SIZE, size,
>> +				 direction, attrs);
>
> Why would a dma ops implementation not provide map_single/ 
> unmap_single?

.... because you asked me to have just map/unmap_page in your review  
of an earlier rev of this patch series in May? :)  I don't actually  
expect you to remember this, because it was a long time ago, but  
here's the relevant chunk of the conversation:

On May 23, 2008, at 4:51 AM, Christoph Hellwig wrote:

> On Wed, Apr 30, 2008 at 06:36:43PM -0500, Becky Bruce wrote:
>> In addition, the dma_map/unmap_page functions are added to dma_ops on
>> HIGHMEM-enabled configs because we can't just fall back on map/ 
>> unmap_single
>> when HIGHMEM is enabled.  Adding these to dma_ops makes it cleaner to
>> substitute different functionality once we have iommu/swiotlb  
>> support.
>
> Maybe I'm missing something but we should only have the page ones.
> virt_to_page is cheap and we'll most likely need the phys address  
> which
> we get from the page anyway.


So I did that for the dma_direct_* ops, but discovered that doing it  
for the iommu case (which I can't test and don't fully understand)  
would be a bit more complicated.  The above code (in conjunction with  
the same code for map_page) allows you to have map/unmap_page, map/ 
unmap_single, or both.  I'm happy to change it again, though.  We  
could just do the above in map_page/unmap_page, calling map/ 
unmap_single if there is no map/unmap_page, and provide both a  
dma_direct_map/unmap_single and a dma_direct_map/unmap_page for the  
dma_direct* ops.

Thanks for taking the time to review - I appreciate it!
-Becky