Re: [PATCH 1/2] alchemy: add au1000-eth platform device
From: Sergei Shtylyov <hidden>
Date: 2009-08-24 18:01:21
Also in:
linux-mips
Hello. Florian Fainelli wrote:
quoted
quoted
This patch adds the board code to register a per-board au1000-eth platform device to be used wit the au1000-eth platform driver in a subsequent patch. Note that the au1000-eth driver knows about the default driver settings such that we do not need to pass any platform_data informations in most cases except db1x00.
quoted
Sigh, NAK... Please don't register the SoC device per board, do it in alchemy/common/platfrom.c and find a way to pass the board specific platform data from the board file there instead -- something like arch/arm/mach-davinci/usb.c does.
Ok, like I promised, this was the per-board device registration. Do you prefer something like this:
I certainly do, but still not in this incarnation... :-)
quoted hunk ↗ jump to hunk
-- From fd75b7c7fa3c05c21122c43e43260d2785475a79 Mon Sep 17 00:00:00 2001 From: Florian Fainelli <florian@openwrt.org> Date: Tue, 18 Aug 2009 17:53:21 +0200 Subject: [PATCH] alchemy: add au1000-eth platform device (v2) This patch makes the board code register the au1000-eth platform device. The au1000-eth platform data can be overriden with the au1xxx_override_eth0_cfg function like it has to be done for the Bosporus board. Changes from v1: - remove per-board platform.c file - add an override function to pass custom eth0 platform_data PHY settings Signed-off-by: Florian Fainelli <florian@openwrt.org> ---diff --git a/arch/mips/alchemy/common/platform.c b/arch/mips/alchemy/common/platform.c index 117f99f..559294a 100644 --- a/arch/mips/alchemy/common/platform.c +++ b/arch/mips/alchemy/common/platform.c@@ -19,6 +19,7 @@ #include <asm/mach-au1x00/au1xxx.h> #include <asm/mach-au1x00/au1xxx_dbdma.h> #include <asm/mach-au1x00/au1100_mmc.h> +#include <asm/mach-au1x00/au1xxx_eth.h> #define PORT(_base, _irq) \ { \@@ -331,6 +332,76 @@ static struct platform_device pbdb_smbus_device = { }; #endif +/* Macro to help defining the Ethernet MAC resources */ +#define MAC_RES(_base, _enable, _irq) \ + { \ + .start = CPHYSADDR(_base), \ + .end = CPHYSADDR(_base + 0xffff), \ + .flags = IORESOURCE_MEM, \ + }, \ + { \ + .start = CPHYSADDR(_enable), \ + .end = CPHYSADDR(_enable + 0x3), \ + .flags = IORESOURCE_MEM, \ + }, \ + { \ + .start = _irq, \ + .end = _irq, \ + .flags = IORESOURCE_IRQ \ + } + +static struct resource au1xxx_eth0_resources[] = { +#if defined(CONFIG_SOC_AU1000) + MAC_RES(AU1000_ETH0_BASE, AU1000_MAC0_ENABLE, AU1000_MAC0_DMA_INT), +#elif defined(CONFIG_SOC_AU1100) + MAC_RES(AU1100_ETH0_BASE, AU1100_MAC0_ENABLE, AU1100_MAC0_DMA_INT), +#elif defined(CONFIG_SOC_AU1550) + MAC_RES(AU1550_ETH0_BASE, AU1550_MAC0_ENABLE, AU1550_MAC0_DMA_INT), +#elif defined(CONFIG_SOC_AU1500) + MAC_RES(AU1500_ETH0_BASE, AU1500_MAC0_ENABLE, AU1500_MAC0_DMA_INT), +#endif +}; + +static struct resource au1xxx_eth1_resources[] = { +#if defined(CONFIG_SOC_AU1000) + MAC_RES(AU1000_ETH1_BASE, AU1000_MAC1_ENABLE, AU1000_MAC1_DMA_INT), +#elif defined(CONFIG_SOC_AU1550) + MAC_RES(AU1550_ETH1_BASE, AU1550_MAC1_ENABLE, AU1550_MAC1_DMA_INT), +#elif defined(CONFIG_SOC_AU1500) + MAC_RES(AU1500_ETH1_BASE, AU1500_MAC1_ENABLE, AU1500_MAC1_DMA_INT), +#endif +}; + +static struct au1000_eth_platform_data au1xxx_eth0_platform_data = { + .phy1_search_mac0 = 1, +};
I'm not sure that the default platfrom data is really a great idea...
+#ifndef CONFIG_SOC_AU1100
+static struct platform_device au1xxx_eth1_device = {
+ .name = "au1000-eth",
+ .id = 1,
+ .num_resources = ARRAY_SIZE(au1xxx_eth1_resources),
+ .resource = au1xxx_eth1_resources,
And where's the platfrom data for the second Ethernet?
+};
+#endif
+
+void __init au1xxx_override_eth0_cfg(struct au1000_eth_platform_data *eth_data)
+{
+ if (!eth_data)
+ return;
+
+ memcpy(&au1xxx_eth0_platform_data, eth_data,
+ sizeof(struct au1000_eth_platform_data));
Why not just set the pointer in au1xxx_eth0_device. And really, why not
make the function more generic, with a prototype like:
void __init au1xxx_override_eth_cfg(unsigned port, struct
au1000_eth_platform_data *eth_data);
quoted hunk ↗ jump to hunk
+} + static struct platform_device *au1xxx_platform_devices[] __initdata = { &au1xx0_uart_device, &au1xxx_usb_ohci_device,@@ -351,17 +422,25 @@ static struct platform_device *au1xxx_platform_devices[] __initdata = { #ifdef SMBUS_PSC_BASE &pbdb_smbus_device, #endif + &au1xxx_eth0_device, }; static int __init au1xxx_platform_init(void) { unsigned int uartclk = get_au1x00_uart_baud_base() * 16; - int i; + int i, ni; /* Fill up uartclk. */ for (i = 0; au1x00_uart_data[i].flags; i++) au1x00_uart_data[i].uartclk = uartclk; + /* Register second MAC if enabled in pinfunc */ +#ifndef CONFIG_SOC_AU1100 + ni = (int)((au_readl(SYS_PINFUNC) & (u32)(SYS_PF_NI2)) >> 4); + if (!(ni + 1))
Why so complex, and how can (ni + 1) ever be 0?! :-/
Doesn't that field when 0 mean the pins configured for MAC1 and when 1
-- for GPIO? Why not just:
if (!(au_readl(SYS_PINFUNC) & SYS_PF_NI2))
+ platform_device_register(&au1xxx_eth1_device); +#endif +
WBR, Sergei