Re: MMAPPED disk package

["Anthony Tonizzo" <atonizzo at gmail dot com>]

> 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.

I did not even look at the IDE driver although in retrospect I should have.
The driver I started from was actually the /devs/disk/v85x which, with minor
changes, could be superseded by my package now. That package
implements the memory mapped interface to  ATA. IDE is just another
one of the ATA interfaces.

> 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.

These are masks specific to the mpc8xx, so the name seems alright
with me.

> 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.

I forgot to modify the CF_DISK_INSTANCE macro and strip any CF reference
from it. I am going to do it like its done in the IDE package. While a CardFlash
was indeed used for its testing, I strived to be as
general as possible in this implementation. As soon as I find a moment I
would like to connect a hard drive to the MBX and use this package
instead of the IDE one to access it. As far as I am concerned, the CVS
tree should really be changed into something like:

/devs/disk/ata/ide
/devs/disk/ata/mmapped

etc. since both my package and the IDE package are equipotent
implementations of part of the ATA spec. The package I submitted
can be used with CardFlash only because CardFlash memory
comes equipped with an ATA controller on board to make it look
like a hard drive.

> 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.

Will correct.

> 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 original file I used came with no copyright assignment however if
you try to download any example code for the MPC860 from the Frescale
site there is a EULA page whose terms of use you have to accept before
you download the code. I guess that is what you are looking for.

> 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).

I will go back and look at this. This is one of the things that was not
modified from the existing v85x package from Savin.

> 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 am not sure I understand how this would work. I will think about 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).

Will change.

Tony