Re: [PATCH V4 2/4] CS89x0 : add CS89x0 platform device to the iMX21ADS board

On Tue, Jan 24, 2012 at 12:34:24AM +0100, Jaccon Bastiaansen wrote:

Hello Sascha,

2012/1/23 Sascha Hauer [off-list ref]:

quoted

On Thu, Jan 19, 2012 at 07:13:50PM +0100, Jaccon Bastiaansen wrote:

quoted

Add CS89x0 networking support to the iMX21ADS board by using the
platform driver support in the CS89x0 driver.

Signed-off-by: Jaccon Bastiaansen <redacted>
---
 arch/arm/configs/imx_v4_v5_defconfig |    2 ++
 arch/arm/mach-imx/mach-mx21ads.c     |   31 ++++++++++++++++++++++++++++---
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/arch/arm/configs/imx_v4_v5_defconfig b/arch/arm/configs/imx_v4_v5_defconfig
index 11a4192..fc0106f 100644
--- a/arch/arm/configs/imx_v4_v5_defconfig
+++ b/arch/arm/configs/imx_v4_v5_defconfig

@@ -81,6 +81,8 @@ CONFIG_NET_ETHERNET=y

 CONFIG_SMC91X=y
 CONFIG_DM9000=y
 CONFIG_SMC911X=y
+CONFIG_CS89x0=y
+CONFIG_CS89x0_PLATFORM=y
 # CONFIG_NETDEV_1000 is not set
 # CONFIG_NETDEV_10000 is not set
 # CONFIG_INPUT_MOUSEDEV is not set

diff --git a/arch/arm/mach-imx/mach-mx21ads.c b/arch/arm/mach-imx/mach-mx21ads.c
index 25f8402..4fd37fc 100644
--- a/arch/arm/mach-imx/mach-mx21ads.c
+++ b/arch/arm/mach-imx/mach-mx21ads.c

@@ -37,8 +37,8 @@

 #define MX21ADS_REG_ADDR(offset)    (void __force __iomem *) \
              (MX21ADS_MMIO_BASE_ADDR + (offset))

+#define MX21ADS_CS8900A_MMIO_SIZE   0x200000
 #define MX21ADS_CS8900A_IRQ         IRQ_GPIOE(11)
-#define MX21ADS_CS8900A_IOBASE_REG  MX21ADS_REG_ADDR(0x000000)
 #define MX21ADS_ST16C255_IOBASE_REG MX21ADS_REG_ADDR(0x200000)
 #define MX21ADS_VERSION_REG         MX21ADS_REG_ADDR(0x400000)
 #define MX21ADS_IO_REG              MX21ADS_REG_ADDR(0x800000)

@@ -159,6 +159,26 @@ static struct platform_device mx21ads_nor_mtd_device = {

      .resource = &mx21ads_flash_resource,
 };

+static const struct resource mx21ads_cs8900_resources[] __initconst = {
+     {
+             .start = MX21_CS1_BASE_ADDR,
+             .end = MX21_CS1_BASE_ADDR +  MX21ADS_CS8900A_MMIO_SIZE - 1,
+             .flags = IORESOURCE_MEM,
+     },
+     {
+             .start = MX21ADS_CS8900A_IRQ,
+             .end = MX21ADS_CS8900A_IRQ,
+             .flags = IORESOURCE_IRQ,
+     }
+};
+
+static const struct platform_device_info mx21ads_cs8900_devinfo __initconst = {
+     .name = "cs89x0",
+     .id = 0,
+     .res = mx21ads_cs8900_resources,
+     .num_res = ARRAY_SIZE(mx21ads_cs8900_resources),
+};
+
 static const struct imxuart_platform_data uart_pdata_rts __initconst = {
      .flags = IMXUART_HAVE_RTSCTS,
 };

@@ -254,14 +274,17 @@ mx21ads_nand_board_info __initconst = {

 static struct map_desc mx21ads_io_desc[] __initdata = {
      /*
       * Memory-mapped I/O on MX21ADS Base board:
-      *   - CS8900A Ethernet controller
       *   - ST16C2552CJ UART
       *   - CPU and Base board version
       *   - Base board I/O register
+      *
+      * Note that the CS8900 uses the first MX21ADS_CS8900A_MMIO_SIZE
+      * bytes of the CS1 memory range.
       */
      {
              .virtual = MX21ADS_MMIO_BASE_ADDR,
-             .pfn = __phys_to_pfn(MX21_CS1_BASE_ADDR),
+             .pfn = __phys_to_pfn(MX21_CS1_BASE_ADDR +
+                                     MX21ADS_CS8900A_MMIO_SIZE),

Since you now use ioremap the cs89x0 driver this mapping is not used
for the cs89x0 anymore, so it's strange to have references to it
here.
You still need the mapping though, but only for MX21ADS_IO_REG.

The ST16C255 UART is the first peripheral in the CS1 memory range for
which this fixed mapping is now required. I could create another
constant (for example: MX21ADS_ST16C255_OFFSET) to indicate the offset
in the CS1 memory range where the mapping should start. But since that
would create another constant with the same value as
MX21ADS_CS8900A_MMIO_SIZE, I decided to use MX21ADS_CS8900A_MMIO_SIZE.

Do you prefer to a constant with another name here?

Ok, looking again at it I understand what you are doing here and it
seems wrong. Instead of mapping MX21_CS1_BASE_ADDR you now map
MX21_CS1_BASE_ADDR + MX21ADS_CS8900A_MMIO_SIZE. This means that the
offset to MX21ADS_IO_REG is no longer 0x800000 but
0x800000 - MX21ADS_CS8900A_MMIO_SIZE.
Please just keep the mapping like it was before, it won't hurt.
BTW the 8250 serial driver also ioremaps the memory if you add
UPF_IOREMAP to the platform data.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |