Patchwork [1/2] arm: ixp4xx: set cohorent_dma_mask for ethernet platform devices

login
register
mail settings
Submitter Jonas Gorski
Date July 20, 2013, 10:15 a.m.
Message ID <1374315339-31469-2-git-send-email-jogo@openwrt.org>
Download mbox | patch
Permalink /patch/260434/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Jonas Gorski - July 20, 2013, 10:15 a.m.
ARM requires the cohorent_dma_mask set, so set it for the platform
devices so that the ethernet driver has access to it.

Signed-off-by: Jonas Gorski <jogo@openwrt.org>
---
 arch/arm/mach-ixp4xx/fsg-setup.c     | 2 ++
 arch/arm/mach-ixp4xx/goramo_mlr.c    | 2 ++
 arch/arm/mach-ixp4xx/ixdp425-setup.c | 3 +++
 arch/arm/mach-ixp4xx/nas100d-setup.c | 1 +
 arch/arm/mach-ixp4xx/nslu2-setup.c   | 1 +
 arch/arm/mach-ixp4xx/omixp-setup.c   | 3 +++
 arch/arm/mach-ixp4xx/vulcan-setup.c  | 2 ++
 7 files changed, 14 insertions(+)
Krzysztof Halasa - July 21, 2013, 2:45 p.m.
Jonas Gorski <jogo@openwrt.org> writes:

> ARM requires the cohorent_dma_mask set, so set it for the platform
> devices so that the ethernet driver has access to it.

I recognize the need to fix this issue and I appreciate your efforts,
but... I think this patch tries to make the driver functional again at
all costs and this a very bad idea. The IXP4xx Ethernet MACs are not
normal platform devices, they are in fact built-in CPU resources. The
platform device structs are only used to set parameters. What the patch
does is unneeded and IMHO harmful code duplication. It makes completely
no sense to set DMA masks in code for individual platforms as it's not
something platforms can decide, or *even should know of*. It's simply
a CPU attribute, a value that is shared by all IXP4xx CPUs and thus all
platforms and systems using it.

This is against the "line of code" count rules (or "rules").

Also the dev->dev.parent is IMHO a bad idea. The queue numbers and MAC
addresses are in no way "parents" of Ethernet controllers, and even if
they somehow were, I find it rather hard to believe they can have DMA
masks.

I think the previous patch (which sets the masks in one place, in
Ethernet driver code) was better, though not perfect.

My fault is I haven't fixed it yet. Will try to invent something.
Ben Hutchings - July 21, 2013, 4:40 p.m.
On Sun, 2013-07-21 at 16:45 +0200, Krzysztof Halasa wrote:
> Jonas Gorski <jogo@openwrt.org> writes:
> 
> > ARM requires the cohorent_dma_mask set, so set it for the platform
> > devices so that the ethernet driver has access to it.
> 
> I recognize the need to fix this issue and I appreciate your efforts,
> but... I think this patch tries to make the driver functional again at
> all costs and this a very bad idea. The IXP4xx Ethernet MACs are not
> normal platform devices, they are in fact built-in CPU resources.

Sounds like a normal platform device to me...

> The
> platform device structs are only used to set parameters. What the patch
> does is unneeded and IMHO harmful code duplication. It makes completely
> no sense to set DMA masks in code for individual platforms as it's not
> something platforms can decide, or *even should know of*. It's simply
> a CPU attribute, a value that is shared by all IXP4xx CPUs and thus all
> platforms and systems using it.
> 
> This is against the "line of code" count rules (or "rules").
> 
> Also the dev->dev.parent is IMHO a bad idea. The queue numbers and MAC
> addresses are in no way "parents" of Ethernet controllers,

Well, those are the platform data.

> and even if
> they somehow were, I find it rather hard to believe they can have DMA
> masks.

I think the problem is that the platform device and the platform data
have not been properly separated.  The machine-specific setup functions
should be passing the eth_plat_info and MAC ID into a common ixp4xx
function which would then create the platform device with the correct
DMA mask etc.

Ben.

> I think the previous patch (which sets the masks in one place, in
> Ethernet driver code) was better, though not perfect.
> 
> My fault is I haven't fixed it yet. Will try to invent something.
Krzysztof Halasa - July 22, 2013, 2:49 p.m.
Ben Hutchings <bhutchings@solarflare.com> writes:

> I think the problem is that the platform device and the platform data
> have not been properly separated.  The machine-specific setup functions
> should be passing the eth_plat_info and MAC ID into a common ixp4xx
> function which would then create the platform device with the correct
> DMA mask etc.

That's also my idea. Will look at this.

Patch

diff --git a/arch/arm/mach-ixp4xx/fsg-setup.c b/arch/arm/mach-ixp4xx/fsg-setup.c
index 429966b7..c120e78 100644
--- a/arch/arm/mach-ixp4xx/fsg-setup.c
+++ b/arch/arm/mach-ixp4xx/fsg-setup.c
@@ -142,12 +142,14 @@  static struct platform_device fsg_eth[] = {
 		.id			= IXP4XX_ETH_NPEB,
 		.dev = {
 			.platform_data	= fsg_plat_eth,
+			.coherent_dma_mask = DMA_BIT_MASK(32),
 		},
 	}, {
 		.name			= "ixp4xx_eth",
 		.id			= IXP4XX_ETH_NPEC,
 		.dev = {
 			.platform_data	= fsg_plat_eth + 1,
+			.coherent_dma_mask = DMA_BIT_MASK(32),
 		},
 	}
 };
diff --git a/arch/arm/mach-ixp4xx/goramo_mlr.c b/arch/arm/mach-ixp4xx/goramo_mlr.c
index e54ff49..0d1b101 100644
--- a/arch/arm/mach-ixp4xx/goramo_mlr.c
+++ b/arch/arm/mach-ixp4xx/goramo_mlr.c
@@ -294,10 +294,12 @@  static struct platform_device device_eth_tab[] = {
 		.name			= "ixp4xx_eth",
 		.id			= IXP4XX_ETH_NPEB,
 		.dev.platform_data	= eth_plat,
+		.dev.coherent_dma_mask	= DMA_BIT_MASK(32),
 	}, {
 		.name			= "ixp4xx_eth",
 		.id			= IXP4XX_ETH_NPEC,
 		.dev.platform_data	= eth_plat + 1,
+		.dev.coherent_dma_mask	= DMA_BIT_MASK(32),
 	}
 };
 
diff --git a/arch/arm/mach-ixp4xx/ixdp425-setup.c b/arch/arm/mach-ixp4xx/ixdp425-setup.c
index 22d688b..f608629 100644
--- a/arch/arm/mach-ixp4xx/ixdp425-setup.c
+++ b/arch/arm/mach-ixp4xx/ixdp425-setup.c
@@ -20,6 +20,7 @@ 
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/delay.h>
+#include <linux/dma-mapping.h>
 #include <asm/types.h>
 #include <asm/setup.h>
 #include <asm/memory.h>
@@ -195,10 +196,12 @@  static struct platform_device ixdp425_eth[] = {
 		.name			= "ixp4xx_eth",
 		.id			= IXP4XX_ETH_NPEB,
 		.dev.platform_data	= ixdp425_plat_eth,
+		.dev.coherent_dma_mask	= DMA_BIT_MASK(32),
 	}, {
 		.name			= "ixp4xx_eth",
 		.id			= IXP4XX_ETH_NPEC,
 		.dev.platform_data	= ixdp425_plat_eth + 1,
+		.dev.coherent_dma_mask	= DMA_BIT_MASK(32),
 	}
 };
 
diff --git a/arch/arm/mach-ixp4xx/nas100d-setup.c b/arch/arm/mach-ixp4xx/nas100d-setup.c
index ed667ce..59654f3 100644
--- a/arch/arm/mach-ixp4xx/nas100d-setup.c
+++ b/arch/arm/mach-ixp4xx/nas100d-setup.c
@@ -170,6 +170,7 @@  static struct platform_device nas100d_eth[] = {
 		.name			= "ixp4xx_eth",
 		.id			= IXP4XX_ETH_NPEB,
 		.dev.platform_data	= nas100d_plat_eth,
+		.dev.coherent_dma_mask	= DMA_BIT_MASK(32),
 	}
 };
 
diff --git a/arch/arm/mach-ixp4xx/nslu2-setup.c b/arch/arm/mach-ixp4xx/nslu2-setup.c
index 7e55236..ff078d2 100644
--- a/arch/arm/mach-ixp4xx/nslu2-setup.c
+++ b/arch/arm/mach-ixp4xx/nslu2-setup.c
@@ -182,6 +182,7 @@  static struct platform_device nslu2_eth[] = {
 		.name			= "ixp4xx_eth",
 		.id			= IXP4XX_ETH_NPEB,
 		.dev.platform_data	= nslu2_plat_eth,
+		.dev.coherent_dma_mask	= DMA_BIT_MASK(32),
 	}
 };
 
diff --git a/arch/arm/mach-ixp4xx/omixp-setup.c b/arch/arm/mach-ixp4xx/omixp-setup.c
index 75ef03d..b79cd51 100644
--- a/arch/arm/mach-ixp4xx/omixp-setup.c
+++ b/arch/arm/mach-ixp4xx/omixp-setup.c
@@ -17,6 +17,7 @@ 
 #include <linux/serial_8250.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
+#include <linux/dma-mapping.h>
 #ifdef CONFIG_LEDS_CLASS
 #include <linux/leds.h>
 #endif
@@ -190,10 +191,12 @@  static struct platform_device ixdp425_eth[] = {
 		.name			= "ixp4xx_eth",
 		.id			= IXP4XX_ETH_NPEB,
 		.dev.platform_data	= ixdp425_plat_eth,
+		.dev.coherent_dma_mask	= DMA_BIT_MASK(32),
 	}, {
 		.name			= "ixp4xx_eth",
 		.id			= IXP4XX_ETH_NPEC,
 		.dev.platform_data	= ixdp425_plat_eth + 1,
+		.dev.coherent_dma_mask	= DMA_BIT_MASK(32),
 	},
 };
 
diff --git a/arch/arm/mach-ixp4xx/vulcan-setup.c b/arch/arm/mach-ixp4xx/vulcan-setup.c
index d599e35..162d71f 100644
--- a/arch/arm/mach-ixp4xx/vulcan-setup.c
+++ b/arch/arm/mach-ixp4xx/vulcan-setup.c
@@ -139,6 +139,7 @@  static struct platform_device vulcan_eth[] = {
 		.id			= IXP4XX_ETH_NPEB,
 		.dev = {
 			.platform_data	= &vulcan_plat_eth[0],
+			.coherent_dma_mask = DMA_BIT_MASK(32),
 		},
 	},
 	[1] = {
@@ -146,6 +147,7 @@  static struct platform_device vulcan_eth[] = {
 		.id			= IXP4XX_ETH_NPEC,
 		.dev = {
 			.platform_data	= &vulcan_plat_eth[1],
+			.coherent_dma_mask = DMA_BIT_MASK(32),
 		},
 	},
 };