[PATCH v2 02/11] arm: pxa27x: support ICP DAS LP-8x4x

Sergei Ianovich
2013-12-08T07:05:35Z

On Sun, 2013-12-08 at 03:21 +0100, Arnd Bergmann wrote:
> On Friday 06 December 2013, Sergei Ianovich wrote:
> You have some rather unusual options in here. I'd suggest you go through
> the reduced defconfig file and remove all options that look like they
> are unnecessary for your system.
>
> It's probably based on some distro config that enables lots of modules
> you don't actually want.

I tried to future-proof the config, by enabling all partition tables and
USB disks and modems that could be plugged into the device.

I'll remove those. However, I would like to retain config options
related to low latency and small kernel size. Keeping them will
hopefully allow being notified about changes affecting the system.

Will this fly?

> > +#define LP8X4X_FPGA_PHYS 0x17000000
> > +#define LP8X4X_FPGA_VIRT ((void *) 0xf1000000)
> > +#define LP8X4X_P2V(x) IOMEM((x) - LP8X4X_FPGA_PHYS + LP8X4X_FPGA_VIRT)
> > +#define LP8X4X_V2P(x) ((x) - LP8X4X_FPGA_VIRT + LP8X4X_FPGA_PHYS)
>
> I think you misunderstood my comment, the point was that you should
> move the IOMEM() to the LP8X4X_FPGA_VIRT definition, like
>
> #define LP8X4X_FPGA_VIRT ((void __iomem *) 0xf1000000)
> #define LP8X4X_P2V(x) (x) - LP8X4X_FPGA_PHYS + LP8X4X_FPGA_VIRT)
>
> which would result in the correct type because of pointer arithmetics.
>
> > +/* board level registers in the FPGA */
> > +
> > +#define LP8X4X_EOI LP8X4X_P2V(0x17009006)
> > +#define LP8X4X_INSINT LP8X4X_P2V(0x17009008)
> > +#define LP8X4X_ENSYSINT LP8X4X_P2V(0x1700900A)
> > +#define LP8X4X_PRIMINT LP8X4X_P2V(0x1700900C)
> > +#define LP8X4X_SECOINT LP8X4X_P2V(0x1700900E)
> > +#define LP8X4X_ENRISEINT LP8X4X_P2V(0x17009010)
> > +#define LP8X4X_CLRRISEINT LP8X4X_P2V(0x17009012)
> > +#define LP8X4X_ENHILVINT LP8X4X_P2V(0x17009014)
> > +#define LP8X4X_CLRHILVINT LP8X4X_P2V(0x17009016)
> > +#define LP8X4X_ENFALLINT LP8X4X_P2V(0x17009018)
> > +#define LP8X4X_CLRFALLINT LP8X4X_P2V(0x1700901a)
>
> Thinking about this again, it's actually better if you just get rid of
> LP8X4X_P2V() entirely and redefine these to
>
> #define LP8X4X_EOI 0x0006
> #define LP8X4X_INSINT 0x0008
> ...
>
> and change the users to do e.g.
>
> readl(LP8X4X_FPGA_VIRT + LP8X4X_INSINT);

I am trying to boot the system with device tree. If I manage, I'll move
this code into drivers/irqchip/ as a platform device. Otherwise, I'll
make this change.

> This is closer to the normal way of adding the offset to an __iomem
> pointer returned from ioremap.
>
> > + platform_device_register_resndata(NULL, "pxa2xx-flash", 0,
> > + &lp8x4x_flash_resources[0], 1,
> > + &lp8x4x_flash_data[0], sizeof(lp8x4x_flash_data[0]));
> > + platform_device_register_resndata(NULL, "pxa2xx-flash", 1,
> > + &lp8x4x_flash_resources[1], 1,
> > + &lp8x4x_flash_data[1], sizeof(lp8x4x_flash_data[1]));
> > + platform_device_register_simple("dm9000", 0,
> > + &lp8x4x_dm9000_resources[0], 3);
> > + platform_device_register_simple("dm9000", 1,
> > + &lp8x4x_dm9000_resources[3], 3);
>
> This looks much better than the first version, a slight improvement IMHO would
> be to split the resource arrays into one symbol per device to turn this into
>
> platform_device_register_resndata(NULL, "pxa2xx-flash", 0,
> &lp8x4x_flash_resources0, 1,
> &lp8x4x_flash_data0, sizeof(lp8x4x_flash_data0));
> platform_device_register_resndata(NULL, "pxa2xx-flash", 1,
> &lp8x4x_flash_resources1, 1,
> &lp8x4x_flash_data1, sizeof(lp8x4x_flash_data1));
>
> It's not important though if you have a strong preference for the way you
> did this, it just seems unusual.

I'll make this change, if device tree doesn't boot.

Thanks for reviewing.