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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Tue, Jan 24, 2012 at 12:34:24AM +0100, Jaccon Bastiaansen wrote:
> Hello Sascha,
> 
> 2012/1/23 Sascha Hauer <s.hauer@pengutronix.de>:
> > On Thu, Jan 19, 2012 at 07:13:50PM +0100, Jaccon Bastiaansen wrote:
> >> Add CS89x0 networking support to the iMX21ADS board by using the
> >> platform driver support in the CS89x0 driver.
> >>
> >> Signed-off-by: Jaccon Bastiaansen <jaccon.bastiaansen@gmail.com>
> >> ---
> >>  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 |