Re: Fixes to MTD flash driver on AMD Alchemy db1100 board

[Ulrich Eckhardt <eckhardt@satorlaser.com>]

Josh Green wrote:
> Hello, I found a couple compile problems with the
> drivers/mtd/maps/db1x00-flash.c MTD driver.  I'm using linux-mips CVS
> from a few weeks back, corresponding to 2.6.11rc2.  I noticed some
> recent CVS traffic in regards to this driver, but I didn't see them in
> cvsweb on the linux-mips site.  My apologies if this is something that
> has already been reported.  

Even if reported, it hasn't been fixed - I have similar problems.

> - Cast return value of ioremap to (void __iomem *) to get rid of warning
> concerning conversion of integer to pointer

This one needs further inspection, I'd say. Grepping through the sources, I 
see that some platforms define ioremap to return 'void*' and some use 'void 
__iomem*'. The same class inconsistencies exist for iounmap(), I think the 
right thing is 'iounmap( void __iomem*)'.

I found a comment that the value returned from ioremap() is not to be used as 
a virtual address, so it can't be used directly anyway, so a  qualifier like 
volatile is not even necessary. The qualifier becomes necessary inside the 
functions that perform the actual IO like readb(), but everything outside 
should not even attempt to look at this pointer.
Yes, on MIPS it can be used as virtual address AFAICT, some (broken?) code 
might even do so. If that code then relies on the volatile qualifier it will 
break...

I went ahead and changed the functions in asm-mips/io.h, and my Au1100 board 
still seems to work as before. Several other architectures need these fixes, 
too, and several cases where the returnvalue of ioremap() or the parameter to 
iounmap() is cast falsely/unnecessarily are also present. 

Hacking on above stuff, I came across another thing that escapes me: inside 
functions like writes*() and reads*(), the the buffer to write to or read is 
taken as 'void*', then to be cast to 'volatile void*'. In the case of 
writes*(), IMHO the pointer should be const. In neither cases does it make 
sense to me to add the volatile to the pointer. What is the reason for this?

Disclaimer: I'm far from being a kernel expert, so if I'm talking crap 
somebody please enlighten me. I just looked at the code and saw what to me 
looked inconsistent.

> - Setup DB1X00_BOTH_BANKS, DB1X00_BOOT_ONLY, and DB1X00_USER_ONLY
> defines in db1x00.h (used pb1550.h as an example) 

I copied over the code from the db1x00.h of Linux 2.4 to achieve the same. 
However, I didn't put this in any header because its use is limited to just 
one file, AFAICT.
 Hmm, I just looked a bit further, and now I wonder why there are so many 
drivers for Au1xx0 based boards. pb1550-flash.c and db1550-flash.c are almost 
similar, including the comment about the file they are based on (look at it, 
seems like search&replace gone amok).
 I haven't looked too far into it, but I really wonder if not at least 
db1x00-flash.c and db1550-flash.c could be merged with pb1xxx-flash.c and 
pb1550-flash.c, if not even all four could be merged into a single driver. 
Point is that the main difference seem to be the memory layout of the flash, 
all the rest seems generic enough.

> I can see the partitions in /dev/mtd now, but I have not thoroughly
> tested it yet to see if there are any other problems.

Can you tell me how you created /dev/mtd? My version (Debian/x86) of MAKEDEV 
doesn't know these. Also, could you tell me how you configured your kernel? I 
have never seen an MTD working, so I don't even know if what I'm doing is 
supposed to work. :(

Uli