Re: [flashv2 merge] io/flash

[Jonathan Larmour <jifl at eCosCentric dot com>]

Bart Veer wrote:
>>>>>> "Jifl" == Jonathan Larmour <jifl@eCosCentric.com> writes:
> 
>     Jifl> I think this is all absolutely fine.
> 
>     >> Actually, it is wrong in at least one place. I remember one
>     >> board where the CAN interface was hanging off an SPI bus
>     >> instead of being on-chip. That implies CAN should be treated as
>     >> a network device, not a bus, and should init at
>     >> CYG_INIT_DEV_CHAR.
>     >> 
>     >> Everybody, please think carefully about any funny boards you
>     >> have come across over the years, and whether or not the above
>     >> order makes sense.
> 
>     Jifl> Of course the purpose of having these numbers abstracted is
>     Jifl> that if we do need to, it can be tweaked.
> 
> Yes and no. Tweaking the numbers at any time in the future runs the
> risk of breaking existing ports. If we are going to make the change
> now then I hope we can get the order as close to perfect as possible.

Oh sure, I just mean that it's not the end of the world.

>     >> >> Finally the file I/O subsystem. Possibly this should happen
>     >> >> earlier, between DEV_BLOCK_PRIMARY and CONFIG, so that an
>     >> >> implementation of the config data module can be layered on top
>     >> >> of file I/O. Or possibly CYG_INIT_IO_FS should happen
>     >> >> immediately after CYG_INIT_MEMALLOC, with the proviso that file
>     >> >> I/O operations for devices may fail until later in the init
>     >> >> sequence.
> 
>     Jifl> It's certainly plausible to want to read config data from an
>     Jifl> FS, IMHO. But CYG_INIT_MEMALLOC seems unnecessarily early
>     Jifl> and would probably cause more problems than it solves.
> 
>     >> I am not sure I agree with that. MEMALLOC should only involve main
>     >> memory, with no need to worry about whether or not any of the I/O
>     >> subsystem is available yet. Initializing MEMALLOC early means that
>     >> device drivers could perform run-time detection and dynamically
>     >> allocate any buffers required, instead of statically allocating for
>     >> the worst case.
> 
>     Jifl> I thought you were saying the order would be
>     Jifl> CYG_INIT_MEMALLOC, CYG_INIT_IO_FS and then the rest of the
>     Jifl> CYG_INIT_IO*. I don't have any issue with MEMALLOC being
>     Jifl> first, but wouldn't have thought that CYG_INIT_IO_FS being
>     Jifl> before CYG_INIT_IO* would work well.
> 
>     Jifl> Were you saying something differnt perhaps?
> 
> Sorry, misunderstanding. OK, MEMALLOC happens before any I/O.
> 
> As to CYG_INIT_IO_FS, the question is really what that is for. The
> main use right now is in io/fileio to initialize the various mutex
> locks, although there are some other uses in that package.
> Initializing mutexes and the file descriptor table early on should be
> harmless, but I am not familiar enough with the internals of the file
> I/O package to understand all the implications.

The constructor in the fileio package (at CYG_INIT_IO_FS) calls
cyg_mtab_init() which causes various filesystems in the mtab to be mounted
at startup time. Although this is perhaps a bit dubious anyway, considering
interrupts are off and the underlying device may be interrupt-driven. (Of
course we can just say "don't do that then" :-), and make them mount it in
their application startup).

> Allowing the config code to mount a file system on top of a primary
> block device and then open and read a file obviously makes sense. It
> is not obvious that it makes sense to allow open() and read() before
> any primary block devices are initialized, but on the other hand it
> avoids problems if we ever add something ahead of primary block
> devices.

I doubt in most cases it's possible to mount a file system without
accessing the device. Trying to make it "lazy" by waiting for the first
open() or read() doesn't sound good to me - at the very least you would
want to check for a valid file system.

Anyway, I think, as you were suggesting before, that CYG_INIT_IO_FS should
be between DEV_BLOCK_PRIMARY and CONFIG. Even though DEV_BLOCK_PRIMARY and
DEV_BLOCK_SECONDARY are adjacent, it may be useful in future to have the
distinction. The finer-grained the better arguably.

So the list would now be:
  #define CYG_INIT_BUS_PRIMARY
  #define CYG_INIT_BUS_PCI		(alias for PRIMARY)
  #define CYG_INIT_BUS_SECONDARY
  #define CYG_INIT_BUS_USBHOST		(alias for secondary)
  #define CYG_INIT_BUS_TERTIARY
  #define CYG_INIT_BUS_SPI		(alias for tertiary)
  #define CYG_INIT_BUS_I2C		(ditto)
  #define CYG_INIT_DEV_WATCHDOG
  #define CYG_INIT_DEV_WALLCLOCK
  #define CYG_INIT_DEV_BLOCK_PRIMARY
  #define CYG_INIT_DEV_FLASH		(alias for BLOCK_PRIMARY)
  #define CYG_INIT_DEV_BLOCK_SECONDARY
  #define CYG_INIT_CONFIG
  #define CYG_INIT_BUS_CAN		(?)
  #define CYG_INIT_DEV_CHAR		(all other devices)
  #define CYG_INIT_IO_FS

> There are also uses of CYG_INIT_IO_FS in the vnc_server and httpd
> packages. Those should probably get initialized a lot later.

Agreed.

So any volunteers?

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine