removing addr_needs_map in struct dma_mapping_ops

removing addr_needs_map in struct dma_mapping_ops

[FUJITA Tomonori]

I'm trying to convert POWERPC to use asm-generic/dma-mapping-common.h.

POWERPC needs addr_needs_map() in struct dma_mapping_ops for SWIOTLB
support but I want to avoid add addr_needs_map() in struct
dma_map_ops. IIRC, you guys think it as a temporary solution and
talked about defining something like struct dma_data. Then we could

struct dev_archdata {
       ...

       struct dma_data *ddata;
};

or

struct dev_archdata {
       ...

       struct dma_data ddata;
};


struct dma_data needs dma_direct_offset, iommu_table, dma_base, and
dma_window_size, anything else?


Is it acceptable?

Re: removing addr_needs_map in struct dma_mapping_ops

[Becky Bruce]

On Jul 9, 2009, at 8:47 PM, FUJITA Tomonori wrote:

> I'm trying to convert POWERPC to use asm-generic/dma-mapping-common.h.
>
> POWERPC needs addr_needs_map() in struct dma_mapping_ops for SWIOTLB
> support but I want to avoid add addr_needs_map() in struct
> dma_map_ops. IIRC, you guys think it as a temporary solution and

That is correct - I was planning to update when the iotlb folks had  
decided on a final scheme for this.  We don't like the additional  
indirection we're getting with the current implementation.

>
> talked about defining something like struct dma_data. Then we could
>
> struct dev_archdata {
>      ...
>
>      struct dma_data *ddata;
> };
>
> or
>
> struct dev_archdata {
>      ...
>
>      struct dma_data ddata;
> };
>
>
> struct dma_data needs dma_direct_offset, iommu_table, dma_base, and
> dma_window_size, anything else?

IIRC, what we had talked about was simpler - we talked about changing  
the current dev_archdata from this:

struct dev_archdata {
        struct device_node      *of_node;
        struct dma_mapping_ops  *dma_ops;
        void                    *dma_data;
};

to this:

struct dev_archdata {
	struct device_node *of_node;
	struct dma_mapping_ops *dma_ops;
	unsigned long long dma_data;
#ifdef CONFIG_SWIOTLB
	dma_addr_t max_direct_dma_addr;
#endif
};

Where max_direct_dma_addr is the address beyond which a specific  
device must use swiotlb, and dma_data is the offset like it is now  
(but wider on 32-bit systems than void *). I believe Ben had mentioned  
wanting to make the max_direct_dma_addr part conditional so we don't  
bloat archdata on platforms that don't ever bounce.

The change to the type of dma_data is actually in preparation for an  
optimization I have planned for 64-bit PCI devices (and which probably  
requires more discussion), so that doesn't need to happen now -  just  
leave it as a void *, and I can post a followup patch.

Let me know if I can help or do any testing - I've been meaning to  
look into switching to dma_map_ops for a while now but it hasn't  
managed to pop off my todo stack.

Cheers,
Becky

Re: removing addr_needs_map in struct dma_mapping_ops

[FUJITA Tomonori]

On Mon, 13 Jul 2009 16:50:43 -0500
Becky Bruce <beckyb@kernel.crashing.org> wrote:

> > talked about defining something like struct dma_data. Then we could
> >
> > struct dev_archdata {
> >      ...
> >
> >      struct dma_data *ddata;
> > };
> >
> > or
> >
> > struct dev_archdata {
> >      ...
> >
> >      struct dma_data ddata;
> > };
> >
> >
> > struct dma_data needs dma_direct_offset, iommu_table, dma_base, and
> > dma_window_size, anything else?
> 
> IIRC, what we had talked about was simpler - we talked about changing  
> the current dev_archdata from this:
> 
> struct dev_archdata {
>         struct device_node      *of_node;
>         struct dma_mapping_ops  *dma_ops;
>         void                    *dma_data;
> };
> 
> to this:
> 
> struct dev_archdata {
> 	struct device_node *of_node;
> 	struct dma_mapping_ops *dma_ops;
> 	unsigned long long dma_data;
> #ifdef CONFIG_SWIOTLB
> 	dma_addr_t max_direct_dma_addr;
> #endif
> };
> 
> Where max_direct_dma_addr is the address beyond which a specific  
> device must use swiotlb, and dma_data is the offset like it is now  
> (but wider on 32-bit systems than void *). I believe Ben had mentioned  
> wanting to make the max_direct_dma_addr part conditional so we don't  
> bloat archdata on platforms that don't ever bounce.

Only maximum address is enough? The minimum (dma_window_base_cur in
swiotlb_pci_addr_needs_map) is not necessary?


> The change to the type of dma_data is actually in preparation for an  
> optimization I have planned for 64-bit PCI devices (and which probably  
> requires more discussion), so that doesn't need to happen now -  just  
> leave it as a void *, and I can post a followup patch.
> 
> Let me know if I can help or do any testing - I've been meaning to  
> look into switching to dma_map_ops for a while now but it hasn't  
> managed to pop off my todo stack.

Ok, how about this? I'm not familiar with POWERPC so I might
misunderstand something.


diff --git a/arch/powerpc/include/asm/device.h b/arch/powerpc/include/asm/device.h
index 7d2277c..0086f8d 100644
--- a/arch/powerpc/include/asm/device.h
+++ b/arch/powerpc/include/asm/device.h
@@ -16,6 +16,9 @@ struct dev_archdata {
 	/* DMA operations on that device */
 	struct dma_mapping_ops	*dma_ops;
 	void			*dma_data;
+#ifdef CONFIG_SWIOTLB
+	dma_addr_t		max_direct_dma_addr;
+#endif
 };
 
 static inline void dev_archdata_set_node(struct dev_archdata *ad,
diff --git a/arch/powerpc/include/asm/swiotlb.h b/arch/powerpc/include/asm/swiotlb.h
index 30891d6..b23a4f1 100644
--- a/arch/powerpc/include/asm/swiotlb.h
+++ b/arch/powerpc/include/asm/swiotlb.h
@@ -24,4 +24,6 @@ static inline void dma_mark_clean(void *addr, size_t size) {}
 extern unsigned int ppc_swiotlb_enable;
 int __init swiotlb_setup_bus_notifier(void);
 
+extern void pci_dma_dev_setup_swiotlb(struct pci_dev *pdev);
+
 #endif /* __ASM_SWIOTLB_H */
diff --git a/arch/powerpc/kernel/dma-swiotlb.c b/arch/powerpc/kernel/dma-swiotlb.c
index 68ccf11..e21359e 100644
--- a/arch/powerpc/kernel/dma-swiotlb.c
+++ b/arch/powerpc/kernel/dma-swiotlb.c
@@ -56,39 +56,16 @@ swiotlb_arch_address_needs_mapping(struct device *hwdev, dma_addr_t addr,
 				   size_t size)
 {
 	struct dma_mapping_ops *dma_ops = get_dma_ops(hwdev);
+	struct dev_archdata *sd = &hwdev->archdata;
 
 	BUG_ON(!dma_ops);
-	return dma_ops->addr_needs_map(hwdev, addr, size);
-}
 
-/*
- * Determine if an address is reachable by a pci device, or if we must bounce.
- */
-static int
-swiotlb_pci_addr_needs_map(struct device *hwdev, dma_addr_t addr, size_t size)
-{
-	u64 mask = dma_get_mask(hwdev);
-	dma_addr_t max;
-	struct pci_controller *hose;
-	struct pci_dev *pdev = to_pci_dev(hwdev);
-
-	hose = pci_bus_to_host(pdev->bus);
-	max = hose->dma_window_base_cur + hose->dma_window_size;
-
-	/* check that we're within mapped pci window space */
-	if ((addr + size > max) | (addr < hose->dma_window_base_cur))
+	if (sd->max_direct_dma_addr && addr + size > sd->max_direct_dma_addr)
 		return 1;
 
-	return !is_buffer_dma_capable(mask, addr, size);
-}
-
-static int
-swiotlb_addr_needs_map(struct device *hwdev, dma_addr_t addr, size_t size)
-{
 	return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size);
 }
 
-
 /*
  * At the moment, all platforms that use this code only require
  * swiotlb to be used if we're operating on HIGHMEM.  Since
@@ -104,7 +81,6 @@ struct dma_mapping_ops swiotlb_dma_ops = {
 	.dma_supported = swiotlb_dma_supported,
 	.map_page = swiotlb_map_page,
 	.unmap_page = swiotlb_unmap_page,
-	.addr_needs_map = swiotlb_addr_needs_map,
 	.sync_single_range_for_cpu = swiotlb_sync_single_range_for_cpu,
 	.sync_single_range_for_device = swiotlb_sync_single_range_for_device,
 	.sync_sg_for_cpu = swiotlb_sync_sg_for_cpu,
@@ -119,13 +95,23 @@ struct dma_mapping_ops swiotlb_pci_dma_ops = {
 	.dma_supported = swiotlb_dma_supported,
 	.map_page = swiotlb_map_page,
 	.unmap_page = swiotlb_unmap_page,
-	.addr_needs_map = swiotlb_pci_addr_needs_map,
 	.sync_single_range_for_cpu = swiotlb_sync_single_range_for_cpu,
 	.sync_single_range_for_device = swiotlb_sync_single_range_for_device,
 	.sync_sg_for_cpu = swiotlb_sync_sg_for_cpu,
 	.sync_sg_for_device = swiotlb_sync_sg_for_device
 };
 
+void pci_dma_dev_setup_swiotlb(struct pci_dev *pdev)
+{
+	struct pci_controller *hose;
+	struct dev_archdata *sd;
+
+	hose = pci_bus_to_host(pdev->bus);
+	sd = &pdev->dev.archdata;
+	sd->max_direct_dma_addr =
+		hose->dma_window_base_cur + hose->dma_window_size;
+}
+
 static int ppc_swiotlb_bus_notify(struct notifier_block *nb,
 				  unsigned long action, void *data)
 {
diff --git a/arch/powerpc/platforms/85xx/mpc8536_ds.c b/arch/powerpc/platforms/85xx/mpc8536_ds.c
index 055ff41..401751b 100644
--- a/arch/powerpc/platforms/85xx/mpc8536_ds.c
+++ b/arch/powerpc/platforms/85xx/mpc8536_ds.c
@@ -136,6 +136,7 @@ define_machine(mpc8536_ds) {
 	.init_IRQ		= mpc8536_ds_pic_init,
 #ifdef CONFIG_PCI
 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
+	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
 #endif
 	.get_irq		= mpic_get_irq,
 	.restart		= fsl_rstcr_restart,
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
index 849c0ac..1ba8e38 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
@@ -277,6 +277,7 @@ define_machine(mpc8544_ds) {
 	.init_IRQ		= mpc85xx_ds_pic_init,
 #ifdef CONFIG_PCI
 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
+	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
 #endif
 	.get_irq		= mpic_get_irq,
 	.restart		= fsl_rstcr_restart,
@@ -291,6 +292,7 @@ define_machine(mpc8572_ds) {
 	.init_IRQ		= mpc85xx_ds_pic_init,
 #ifdef CONFIG_PCI
 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
+	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
 #endif
 	.get_irq		= mpic_get_irq,
 	.restart		= fsl_rstcr_restart,
@@ -305,6 +307,7 @@ define_machine(p2020_ds) {
 	.init_IRQ		= mpc85xx_ds_pic_init,
 #ifdef CONFIG_PCI
 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
+	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
 #endif
 	.get_irq		= mpic_get_irq,
 	.restart		= fsl_rstcr_restart,
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
index 60ed9c0..165a2de 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
@@ -356,6 +356,7 @@ define_machine(mpc8568_mds) {
 	.progress	= udbg_progress,
 #ifdef CONFIG_PCI
 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
+	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
 #endif
 };
 
@@ -377,5 +378,6 @@ define_machine(mpc8569_mds) {
 	.progress	= udbg_progress,
 #ifdef CONFIG_PCI
 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
+	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
 #endif
 };
diff --git a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c b/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
index 6632702..d1878f3 100644
--- a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
+++ b/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
@@ -187,5 +187,6 @@ define_machine(mpc86xx_hpcn) {
 	.progress		= udbg_progress,
 #ifdef CONFIG_PCI
 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
+	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
 #endif
 };

Re: removing addr_needs_map in struct dma_mapping_ops

[Becky Bruce]

On Jul 13, 2009, at 7:49 PM, FUJITA Tomonori wrote:

> On Mon, 13 Jul 2009 16:50:43 -0500
> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>
>>> talked about defining something like struct dma_data. Then we could
>>>
>>> struct dev_archdata {
>>>     ...
>>>
>>>     struct dma_data *ddata;
>>> };
>>>
>>> or
>>>
>>> struct dev_archdata {
>>>     ...
>>>
>>>     struct dma_data ddata;
>>> };
>>>
>>>
>>> struct dma_data needs dma_direct_offset, iommu_table, dma_base, and
>>> dma_window_size, anything else?
>>
>> IIRC, what we had talked about was simpler - we talked about changing
>> the current dev_archdata from this:
>>
>> struct dev_archdata {
>>        struct device_node      *of_node;
>>        struct dma_mapping_ops  *dma_ops;
>>        void                    *dma_data;
>> };
>>
>> to this:
>>
>> struct dev_archdata {
>> 	struct device_node *of_node;
>> 	struct dma_mapping_ops *dma_ops;
>> 	unsigned long long dma_data;
>> #ifdef CONFIG_SWIOTLB
>> 	dma_addr_t max_direct_dma_addr;
>> #endif
>> };
>>
>> Where max_direct_dma_addr is the address beyond which a specific
>> device must use swiotlb, and dma_data is the offset like it is now
>> (but wider on 32-bit systems than void *). I believe Ben had  
>> mentioned
>> wanting to make the max_direct_dma_addr part conditional so we don't
>> bloat archdata on platforms that don't ever bounce.
>
> Only maximum address is enough? The minimum (dma_window_base_cur in
> swiotlb_pci_addr_needs_map) is not necessary?
>
>
>> The change to the type of dma_data is actually in preparation for an
>> optimization I have planned for 64-bit PCI devices (and which  
>> probably
>> requires more discussion), so that doesn't need to happen now -  just
>> leave it as a void *, and I can post a followup patch.
>>
>> Let me know if I can help or do any testing - I've been meaning to
>> look into switching to dma_map_ops for a while now but it hasn't
>> managed to pop off my todo stack.
>
> Ok, how about this? I'm not familiar with POWERPC so I might
> misunderstand something.

This is close, but it misses the setup for non-pci devices. We have a  
bus notifier that we use to set up archdata for those devices -   
ppc_swiotlb_bus_notify() in arch/powerpc/kernel/dma-swiotlb.c.  It  
won't cause breakage to not have this set up, because those will fall  
through to the dma_capable(), but I think we should initialize it  
anyway (who knows what it will end up used for later....).

>
>
>
> diff --git a/arch/powerpc/include/asm/device.h b/arch/powerpc/ 
> include/asm/device.h
> index 7d2277c..0086f8d 100644
> --- a/arch/powerpc/include/asm/device.h
> +++ b/arch/powerpc/include/asm/device.h
> @@ -16,6 +16,9 @@ struct dev_archdata {
> 	/* DMA operations on that device */
> 	struct dma_mapping_ops	*dma_ops;
> 	void			*dma_data;
> +#ifdef CONFIG_SWIOTLB
> +	dma_addr_t		max_direct_dma_addr;
> +#endif
> };
>
> static inline void dev_archdata_set_node(struct dev_archdata *ad,
> diff --git a/arch/powerpc/include/asm/swiotlb.h b/arch/powerpc/ 
> include/asm/swiotlb.h
> index 30891d6..b23a4f1 100644
> --- a/arch/powerpc/include/asm/swiotlb.h
> +++ b/arch/powerpc/include/asm/swiotlb.h
> @@ -24,4 +24,6 @@ static inline void dma_mark_clean(void *addr,  
> size_t size) {}
> extern unsigned int ppc_swiotlb_enable;
> int __init swiotlb_setup_bus_notifier(void);
>
> +extern void pci_dma_dev_setup_swiotlb(struct pci_dev *pdev);
> +
> #endif /* __ASM_SWIOTLB_H */
> diff --git a/arch/powerpc/kernel/dma-swiotlb.c b/arch/powerpc/kernel/ 
> dma-swiotlb.c
> index 68ccf11..e21359e 100644
> --- a/arch/powerpc/kernel/dma-swiotlb.c
> +++ b/arch/powerpc/kernel/dma-swiotlb.c
> @@ -56,39 +56,16 @@ swiotlb_arch_address_needs_mapping(struct device  
> *hwdev, dma_addr_t addr,
> 				   size_t size)
> {
> 	struct dma_mapping_ops *dma_ops = get_dma_ops(hwdev);
> +	struct dev_archdata *sd = &hwdev->archdata;
>
> 	BUG_ON(!dma_ops);
> -	return dma_ops->addr_needs_map(hwdev, addr, size);
> -}

You can get rid of the dma_ops stuff here.... it's no longer needed.

>
>
> -/*
> - * Determine if an address is reachable by a pci device, or if we  
> must bounce.
> - */
> -static int
> -swiotlb_pci_addr_needs_map(struct device *hwdev, dma_addr_t addr,  
> size_t size)
> -{
> -	u64 mask = dma_get_mask(hwdev);
> -	dma_addr_t max;
> -	struct pci_controller *hose;
> -	struct pci_dev *pdev = to_pci_dev(hwdev);
> -
> -	hose = pci_bus_to_host(pdev->bus);
> -	max = hose->dma_window_base_cur + hose->dma_window_size;
> -
> -	/* check that we're within mapped pci window space */
> -	if ((addr + size > max) | (addr < hose->dma_window_base_cur))
> +	if (sd->max_direct_dma_addr && addr + size > sd- 
> >max_direct_dma_addr)
> 		return 1;
>
> -	return !is_buffer_dma_capable(mask, addr, size);
> -}
> -
> -static int
> -swiotlb_addr_needs_map(struct device *hwdev, dma_addr_t addr,  
> size_t size)
> -{
> 	return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size);
> }
>
> -
> /*
>  * At the moment, all platforms that use this code only require
>  * swiotlb to be used if we're operating on HIGHMEM.  Since
> @@ -104,7 +81,6 @@ struct dma_mapping_ops swiotlb_dma_ops = {
> 	.dma_supported = swiotlb_dma_supported,
> 	.map_page = swiotlb_map_page,
> 	.unmap_page = swiotlb_unmap_page,
> -	.addr_needs_map = swiotlb_addr_needs_map,
> 	.sync_single_range_for_cpu = swiotlb_sync_single_range_for_cpu,
> 	.sync_single_range_for_device = swiotlb_sync_single_range_for_device,
> 	.sync_sg_for_cpu = swiotlb_sync_sg_for_cpu,
> @@ -119,13 +95,23 @@ struct dma_mapping_ops swiotlb_pci_dma_ops = {
> 	.dma_supported = swiotlb_dma_supported,
> 	.map_page = swiotlb_map_page,
> 	.unmap_page = swiotlb_unmap_page,
> -	.addr_needs_map = swiotlb_pci_addr_needs_map,
> 	.sync_single_range_for_cpu = swiotlb_sync_single_range_for_cpu,
> 	.sync_single_range_for_device = swiotlb_sync_single_range_for_device,
> 	.sync_sg_for_cpu = swiotlb_sync_sg_for_cpu,
> 	.sync_sg_for_device = swiotlb_sync_sg_for_device
> };
>
> +void pci_dma_dev_setup_swiotlb(struct pci_dev *pdev)
> +{
> +	struct pci_controller *hose;
> +	struct dev_archdata *sd;
> +
> +	hose = pci_bus_to_host(pdev->bus);
> +	sd = &pdev->dev.archdata;
> +	sd->max_direct_dma_addr =
> +		hose->dma_window_base_cur + hose->dma_window_size;
> +}
> +
> static int ppc_swiotlb_bus_notify(struct notifier_block *nb,
> 				  unsigned long action, void *data)
> {
> diff --git a/arch/powerpc/platforms/85xx/mpc8536_ds.c b/arch/powerpc/ 
> platforms/85xx/mpc8536_ds.c
> index 055ff41..401751b 100644
> --- a/arch/powerpc/platforms/85xx/mpc8536_ds.c
> +++ b/arch/powerpc/platforms/85xx/mpc8536_ds.c
> @@ -136,6 +136,7 @@ define_machine(mpc8536_ds) {
> 	.init_IRQ		= mpc8536_ds_pic_init,
> #ifdef CONFIG_PCI
> 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> #endif
> 	.get_irq		= mpic_get_irq,
> 	.restart		= fsl_rstcr_restart,
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/ 
> platforms/85xx/mpc85xx_ds.c
> index 849c0ac..1ba8e38 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> @@ -277,6 +277,7 @@ define_machine(mpc8544_ds) {
> 	.init_IRQ		= mpc85xx_ds_pic_init,
> #ifdef CONFIG_PCI
> 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> #endif
> 	.get_irq		= mpic_get_irq,
> 	.restart		= fsl_rstcr_restart,
> @@ -291,6 +292,7 @@ define_machine(mpc8572_ds) {
> 	.init_IRQ		= mpc85xx_ds_pic_init,
> #ifdef CONFIG_PCI
> 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> #endif
> 	.get_irq		= mpic_get_irq,
> 	.restart		= fsl_rstcr_restart,
> @@ -305,6 +307,7 @@ define_machine(p2020_ds) {
> 	.init_IRQ		= mpc85xx_ds_pic_init,
> #ifdef CONFIG_PCI
> 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> #endif
> 	.get_irq		= mpic_get_irq,
> 	.restart		= fsl_rstcr_restart,
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/ 
> powerpc/platforms/85xx/mpc85xx_mds.c
> index 60ed9c0..165a2de 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
> @@ -356,6 +356,7 @@ define_machine(mpc8568_mds) {
> 	.progress	= udbg_progress,
> #ifdef CONFIG_PCI
> 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> #endif
> };
>
> @@ -377,5 +378,6 @@ define_machine(mpc8569_mds) {
> 	.progress	= udbg_progress,
> #ifdef CONFIG_PCI
> 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> #endif
> };
> diff --git a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c b/arch/ 
> powerpc/platforms/86xx/mpc86xx_hpcn.c
> index 6632702..d1878f3 100644
> --- a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
> +++ b/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
> @@ -187,5 +187,6 @@ define_machine(mpc86xx_hpcn) {
> 	.progress		= udbg_progress,
> #ifdef CONFIG_PCI
> 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> #endif
> };

Instead of initializing this here (which has problems if ! 
CONFIG_SWIOTLB), place this in the xxxxx_xxxx_setup_arch function in  
the same files, which already have an #ifdef CONFIG_SWIOTLB in which  
this can be embedded.

I'm about to be off-list for a few days but will be happy to help when  
I'm back next week.

Thanks!
Becky


>
> --
> To unsubscribe from this list: send the line "unsubscribe linux- 
> kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: removing addr_needs_map in struct dma_mapping_ops

[FUJITA Tomonori]

On Wed, 15 Jul 2009 18:59:03 -0500
Becky Bruce <beckyb@kernel.crashing.org> wrote:

> 
> On Jul 13, 2009, at 7:49 PM, FUJITA Tomonori wrote:
> 
> > On Mon, 13 Jul 2009 16:50:43 -0500
> > Becky Bruce <beckyb@kernel.crashing.org> wrote:
> >
> >>> talked about defining something like struct dma_data. Then we could
> >>>
> >>> struct dev_archdata {
> >>>     ...
> >>>
> >>>     struct dma_data *ddata;
> >>> };
> >>>
> >>> or
> >>>
> >>> struct dev_archdata {
> >>>     ...
> >>>
> >>>     struct dma_data ddata;
> >>> };
> >>>
> >>>
> >>> struct dma_data needs dma_direct_offset, iommu_table, dma_base, and
> >>> dma_window_size, anything else?
> >>
> >> IIRC, what we had talked about was simpler - we talked about changing
> >> the current dev_archdata from this:
> >>
> >> struct dev_archdata {
> >>        struct device_node      *of_node;
> >>        struct dma_mapping_ops  *dma_ops;
> >>        void                    *dma_data;
> >> };
> >>
> >> to this:
> >>
> >> struct dev_archdata {
> >> 	struct device_node *of_node;
> >> 	struct dma_mapping_ops *dma_ops;
> >> 	unsigned long long dma_data;
> >> #ifdef CONFIG_SWIOTLB
> >> 	dma_addr_t max_direct_dma_addr;
> >> #endif
> >> };
> >>
> >> Where max_direct_dma_addr is the address beyond which a specific
> >> device must use swiotlb, and dma_data is the offset like it is now
> >> (but wider on 32-bit systems than void *). I believe Ben had  
> >> mentioned
> >> wanting to make the max_direct_dma_addr part conditional so we don't
> >> bloat archdata on platforms that don't ever bounce.
> >
> > Only maximum address is enough? The minimum (dma_window_base_cur in
> > swiotlb_pci_addr_needs_map) is not necessary?
> >
> >
> >> The change to the type of dma_data is actually in preparation for an
> >> optimization I have planned for 64-bit PCI devices (and which  
> >> probably
> >> requires more discussion), so that doesn't need to happen now -  just
> >> leave it as a void *, and I can post a followup patch.
> >>
> >> Let me know if I can help or do any testing - I've been meaning to
> >> look into switching to dma_map_ops for a while now but it hasn't
> >> managed to pop off my todo stack.
> >
> > Ok, how about this? I'm not familiar with POWERPC so I might
> > misunderstand something.
> 
> This is close, but it misses the setup for non-pci devices. We have a  
> bus notifier that we use to set up archdata for those devices -   
> ppc_swiotlb_bus_notify() in arch/powerpc/kernel/dma-swiotlb.c.  It  
> won't cause breakage to not have this set up, because those will fall  
> through to the dma_capable(), but I think we should initialize it  
> anyway (who knows what it will end up used for later....).

You mean that you like to initialize max_direct_dma_addr to zero
explicitly in ppc_swiotlb_bus_notify()?


> > diff --git a/arch/powerpc/include/asm/device.h b/arch/powerpc/ 
> > include/asm/device.h
> > index 7d2277c..0086f8d 100644
> > --- a/arch/powerpc/include/asm/device.h
> > +++ b/arch/powerpc/include/asm/device.h
> > @@ -16,6 +16,9 @@ struct dev_archdata {
> > 	/* DMA operations on that device */
> > 	struct dma_mapping_ops	*dma_ops;
> > 	void			*dma_data;
> > +#ifdef CONFIG_SWIOTLB
> > +	dma_addr_t		max_direct_dma_addr;
> > +#endif
> > };
> >
> > static inline void dev_archdata_set_node(struct dev_archdata *ad,
> > diff --git a/arch/powerpc/include/asm/swiotlb.h b/arch/powerpc/ 
> > include/asm/swiotlb.h
> > index 30891d6..b23a4f1 100644
> > --- a/arch/powerpc/include/asm/swiotlb.h
> > +++ b/arch/powerpc/include/asm/swiotlb.h
> > @@ -24,4 +24,6 @@ static inline void dma_mark_clean(void *addr,  
> > size_t size) {}
> > extern unsigned int ppc_swiotlb_enable;
> > int __init swiotlb_setup_bus_notifier(void);
> >
> > +extern void pci_dma_dev_setup_swiotlb(struct pci_dev *pdev);
> > +
> > #endif /* __ASM_SWIOTLB_H */
> > diff --git a/arch/powerpc/kernel/dma-swiotlb.c b/arch/powerpc/kernel/ 
> > dma-swiotlb.c
> > index 68ccf11..e21359e 100644
> > --- a/arch/powerpc/kernel/dma-swiotlb.c
> > +++ b/arch/powerpc/kernel/dma-swiotlb.c
> > @@ -56,39 +56,16 @@ swiotlb_arch_address_needs_mapping(struct device  
> > *hwdev, dma_addr_t addr,
> > 				   size_t size)
> > {
> > 	struct dma_mapping_ops *dma_ops = get_dma_ops(hwdev);
> > +	struct dev_archdata *sd = &hwdev->archdata;
> >
> > 	BUG_ON(!dma_ops);
> > -	return dma_ops->addr_needs_map(hwdev, addr, size);
> > -}
> 
> You can get rid of the dma_ops stuff here.... it's no longer needed.

Yeah, I'll do later in this patchset that converts POWERPC to use
dma_map_ops structure; this is the first patch of the patchset.


> > - * Determine if an address is reachable by a pci device, or if we  
> > must bounce.
> > - */
> > -static int
> > -swiotlb_pci_addr_needs_map(struct device *hwdev, dma_addr_t addr,  
> > size_t size)
> > -{
> > -	u64 mask = dma_get_mask(hwdev);
> > -	dma_addr_t max;
> > -	struct pci_controller *hose;
> > -	struct pci_dev *pdev = to_pci_dev(hwdev);
> > -
> > -	hose = pci_bus_to_host(pdev->bus);
> > -	max = hose->dma_window_base_cur + hose->dma_window_size;
> > -
> > -	/* check that we're within mapped pci window space */
> > -	if ((addr + size > max) | (addr < hose->dma_window_base_cur))
> > +	if (sd->max_direct_dma_addr && addr + size > sd- 
> > >max_direct_dma_addr)
> > 		return 1;
> >
> > -	return !is_buffer_dma_capable(mask, addr, size);
> > -}
> > -
> > -static int
> > -swiotlb_addr_needs_map(struct device *hwdev, dma_addr_t addr,  
> > size_t size)
> > -{
> > 	return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size);
> > }
> >
> > -
> > /*
> >  * At the moment, all platforms that use this code only require
> >  * swiotlb to be used if we're operating on HIGHMEM.  Since
> > @@ -104,7 +81,6 @@ struct dma_mapping_ops swiotlb_dma_ops = {
> > 	.dma_supported = swiotlb_dma_supported,
> > 	.map_page = swiotlb_map_page,
> > 	.unmap_page = swiotlb_unmap_page,
> > -	.addr_needs_map = swiotlb_addr_needs_map,
> > 	.sync_single_range_for_cpu = swiotlb_sync_single_range_for_cpu,
> > 	.sync_single_range_for_device = swiotlb_sync_single_range_for_device,
> > 	.sync_sg_for_cpu = swiotlb_sync_sg_for_cpu,
> > @@ -119,13 +95,23 @@ struct dma_mapping_ops swiotlb_pci_dma_ops = {
> > 	.dma_supported = swiotlb_dma_supported,
> > 	.map_page = swiotlb_map_page,
> > 	.unmap_page = swiotlb_unmap_page,
> > -	.addr_needs_map = swiotlb_pci_addr_needs_map,
> > 	.sync_single_range_for_cpu = swiotlb_sync_single_range_for_cpu,
> > 	.sync_single_range_for_device = swiotlb_sync_single_range_for_device,
> > 	.sync_sg_for_cpu = swiotlb_sync_sg_for_cpu,
> > 	.sync_sg_for_device = swiotlb_sync_sg_for_device
> > };
> >
> > +void pci_dma_dev_setup_swiotlb(struct pci_dev *pdev)
> > +{
> > +	struct pci_controller *hose;
> > +	struct dev_archdata *sd;
> > +
> > +	hose = pci_bus_to_host(pdev->bus);
> > +	sd = &pdev->dev.archdata;
> > +	sd->max_direct_dma_addr =
> > +		hose->dma_window_base_cur + hose->dma_window_size;
> > +}
> > +
> > static int ppc_swiotlb_bus_notify(struct notifier_block *nb,
> > 				  unsigned long action, void *data)
> > {
> > diff --git a/arch/powerpc/platforms/85xx/mpc8536_ds.c b/arch/powerpc/ 
> > platforms/85xx/mpc8536_ds.c
> > index 055ff41..401751b 100644
> > --- a/arch/powerpc/platforms/85xx/mpc8536_ds.c
> > +++ b/arch/powerpc/platforms/85xx/mpc8536_ds.c
> > @@ -136,6 +136,7 @@ define_machine(mpc8536_ds) {
> > 	.init_IRQ		= mpc8536_ds_pic_init,
> > #ifdef CONFIG_PCI
> > 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> > +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> > #endif
> > 	.get_irq		= mpic_get_irq,
> > 	.restart		= fsl_rstcr_restart,
> > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/ 
> > platforms/85xx/mpc85xx_ds.c
> > index 849c0ac..1ba8e38 100644
> > --- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> > +++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> > @@ -277,6 +277,7 @@ define_machine(mpc8544_ds) {
> > 	.init_IRQ		= mpc85xx_ds_pic_init,
> > #ifdef CONFIG_PCI
> > 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> > +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> > #endif
> > 	.get_irq		= mpic_get_irq,
> > 	.restart		= fsl_rstcr_restart,
> > @@ -291,6 +292,7 @@ define_machine(mpc8572_ds) {
> > 	.init_IRQ		= mpc85xx_ds_pic_init,
> > #ifdef CONFIG_PCI
> > 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> > +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> > #endif
> > 	.get_irq		= mpic_get_irq,
> > 	.restart		= fsl_rstcr_restart,
> > @@ -305,6 +307,7 @@ define_machine(p2020_ds) {
> > 	.init_IRQ		= mpc85xx_ds_pic_init,
> > #ifdef CONFIG_PCI
> > 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> > +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> > #endif
> > 	.get_irq		= mpic_get_irq,
> > 	.restart		= fsl_rstcr_restart,
> > diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/ 
> > powerpc/platforms/85xx/mpc85xx_mds.c
> > index 60ed9c0..165a2de 100644
> > --- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c
> > +++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
> > @@ -356,6 +356,7 @@ define_machine(mpc8568_mds) {
> > 	.progress	= udbg_progress,
> > #ifdef CONFIG_PCI
> > 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> > +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> > #endif
> > };
> >
> > @@ -377,5 +378,6 @@ define_machine(mpc8569_mds) {
> > 	.progress	= udbg_progress,
> > #ifdef CONFIG_PCI
> > 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> > +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> > #endif
> > };
> > diff --git a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c b/arch/ 
> > powerpc/platforms/86xx/mpc86xx_hpcn.c
> > index 6632702..d1878f3 100644
> > --- a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
> > +++ b/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
> > @@ -187,5 +187,6 @@ define_machine(mpc86xx_hpcn) {
> > 	.progress		= udbg_progress,
> > #ifdef CONFIG_PCI
> > 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
> > +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
> > #endif
> > };
> 
> Instead of initializing this here (which has problems if ! 
> CONFIG_SWIOTLB),

Oops, I overlooked it.


> place this in the xxxxx_xxxx_setup_arch function in  
> the same files, which already have an #ifdef CONFIG_SWIOTLB in which  
> this can be embedded.

But the xxxxx_xxxx_setup_arch function doesn't access to each device
so we need to add something like for_each_pci_dev()? You prefer that?


> I'm about to be off-list for a few days but will be happy to help when  
> I'm back next week.

Thanks!

Re: removing addr_needs_map in struct dma_mapping_ops

[Kumar Gala]

On Jul 23, 2009, at 2:09 AM, FUJITA Tomonori wrote:

>>> diff --git a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c b/arch/
>>> powerpc/platforms/86xx/mpc86xx_hpcn.c
>>> index 6632702..d1878f3 100644
>>> --- a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
>>> +++ b/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
>>> @@ -187,5 +187,6 @@ define_machine(mpc86xx_hpcn) {
>>> 	.progress		= udbg_progress,
>>> #ifdef CONFIG_PCI
>>> 	.pcibios_fixup_bus	= fsl_pcibios_fixup_bus,
>>> +	.pci_dma_dev_setup	= pci_dma_dev_setup_swiotlb,
>>> #endif
>>> };
>>
>> Instead of initializing this here (which has problems if !
>> CONFIG_SWIOTLB),
>
> Oops, I overlooked it.
>
>
>> place this in the xxxxx_xxxx_setup_arch function in
>> the same files, which already have an #ifdef CONFIG_SWIOTLB in which
>> this can be embedded.
>
> But the xxxxx_xxxx_setup_arch function doesn't access to each device
> so we need to add something like for_each_pci_dev()? You prefer that?

No.. I think what we want is:

#ifdef CONFIG_SWIOTLB
         if (lmb_end_of_DRAM() > max) {
                 ppc_swiotlb_enable = 1;
                 set_pci_dma_ops(&swiotlb_pci_dma_ops);
		ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb;
         }
#endif

Buts its been a few days since Becky & I chatted about this and I feel  
like I'm forgetting something.

- k