| Message ID | 1374315339-31469-2-git-send-email-jogo@openwrt.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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.
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.
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), }, }, };
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(+)