This is the mail archive of the
ecos-patches@sourceware.org
mailing list for the eCos project.
Re: MMAPPED disk package
Hi Anthony and Andrew,
Andrew Lunn wrote:
On Sun, Nov 19, 2006 at 12:38:56PM -0800, Anthony Tonizzo wrote:
Here is a package for a generic memory mapped ATA
disk.
There are comments in the code which say part of it comes from Savin
Zlobec. Could you point me to this code. I just want to check we have
the necessary copyright paperwork.
I was assuming that's because it started from the IDE driver, by Savin. But
I guess that needs checking. So the only thing that matters is if Savin
added new code, other than what was already in CVS.
How CF specific is this code? Would it be better to call the package
CF_DISK or CF_ATA?
I was thinking that. CF_DISK would seem marginally better to me just
because (although strictly just as correct) "ATA" might imply the more
common ATA command set, and CF cards are meant to support a "TrueIDE" mode
which pretty much maps to that command set (and if implemented, would just
use the normal IDE driver). But the CF support here is the CF-specific ATA
standard stuff. I've no strong feeling either away, other than "mmapped"
seems wrong.
This would affect the CDL options as well as the directory and file names.
Im not sure mask.h is the correct name for that file. I think there is
a standard name for register definitions, but i cannot remember it
right now.
I would normally put things like this in files called
var_io.h/proc_io.h/plf_io.h. In this case, I see mpc8xx already has a
var_regs.h file, but this new file is a bit namespace polluting, and so it
might be better to keep it separate after all. Plus it was pulled in from
elsewhere as a standalone file anyway. I suggest mpc8xx_regs.h.
I have some more comments, sorry!
I think these defines need to be given CYG_ prefixes, to control the
namespace better:
cyg_bool PCMCIAInit(cyg_int32);
#define DISK_HW_INIT(a) PCMCIAInit((cyg_int32)a);
#define HW_DISK_MEMORY_BASE 0xFA000000
Prefixing each with CYG_DEVS_DISKS_CF_ would be better.
Likewise other defines in plf_io.h.
Minor issue, but mmapped_disk.h has this at the top:
CYGONCE_HAL_DEV_DISK_MMAPPED_H but isn't a HAL file. Similarly in the CDL
CYGHWR_HAL_DEVS_DISKS_MMAPPED_IWIDTH wants "_HAL" removed.
More important is that that file says "Originally a file from
Motorola/Frescale." in which case we need to confirm the license and
redistribution terms of that file.
The initialisation doesn't seem quite right. It seems to me that the
CF_DISK_INSTANCE macro should be defined in an exported header file, and
then the relevant platform HAL should instantiate devices using that macro.
That way it can choose the appropriate number of instances for the
hardware. The CYGVAR_DEVS_DISK_MMAPED_DISK0 CDL would thus be removed, and
dev_disk_mmapped.c no longer forced into libextras.a (instead the above
instantiating HAL file would be instead).
This also gives the possibility, in the HAL, of disabling a CF interface by
CDL. This could be important, e.g. on powerpc MBX, so that you can add
CYGPKG_IO_DISK to your config, but not get potentially unnecessary CF
support. You might be wanting support for SPI dataflash, not CF so there
needs to be a way to disable it.
I think some of them the 16-bit versus 8-bit routines could be merged and
thus simplified, and instead the choice being done by an #ifdef around a
typedef, and a one-off #ifdef in the function body to choose
CF_REG16_DUPLICATE_EVEN_DATA versus CF_REG8_DATA_8BIT.
There's several magic numbers in cf_disk_init that deserve to be made
#defines (I'm noticing 0x200/0x202/0x204/0x848A at least).
Other than that it looks like all good stuff, thanks Anthony!
Jifl
--
eCosCentric http://www.eCosCentric.com/ The eCos and RedBoot experts
------["The best things in life aren't things."]------ Opinions==mine