Re: [flashv2 merge] io/flash

Re: [flashv2 merge] io/flash

[Jonathan Larmour]

[Adding maintainers due to request for volunteers! Bart's mail came from
ecos-devel]
Bart Veer wrote:
> 
>>>>>> "Jifl" == Jonathan Larmour <jifl@eCosCentric.com> writes:
> 
>     <snip>
>     
>     Jifl> To which Bart validly replied:
>     Jifl> -=-=-=-=-=-=-
>     Jifl> I raised it with Andrew back in July. Unfortunately it is
>     Jifl> not currently possible. The currently defined constructor
>     Jifl> priorities are not sufficiently flexible to cope with
>     Jifl> dataflash, where the flash subsystem cannot be initialized
>     Jifl> until after the SPI bus. It would be necessary to reorganize
>     Jifl> the defined priorities, which is not something to be tackled
>     Jifl> lightly.
>     Jifl> -=-=-=-=-=-=-
> 
>     Jifl> Unfortunately discussion fizzled out. My bad.
> 
> I am not sure this is the right time to change the init priorities,

Only because it eventually affects the flash API, which means that this is
the right time.

> Now, I suspect that no solution is going to work for every bit of
> hardware that people can conceive off. As and when we run into
> problems we may have to add configury for setting certain init
> priorities, together with requires constraints in the platform HAL, to
> ensure that everything initializes in the correct order for a specific
> platform. However the following order should work for most systems:
> 
>   #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_BUS_CAN		(?)
>   #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_CONFIG
>   #define CYG_INIT_DEV_BLOCK_SECONDARY
>   #define CYG_INIT_DEV_CHAR		(all other devices)
>   #define CYG_INIT_IO_FS
[big snip]

I think this is all absolutely fine.

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

It's certainly plausible to want to read config data from an FS, IMHO. But
CYG_INIT_MEMALLOC seems unnecessarily early and would probably cause more
problems than it solves.

> Back to the original email and what should happen in the flash code,
> off the top of my head my preference would be along the following
> lines:
> 
> 1) assume that diag_printf() is available after CYG_INIT_HAL, although
> on a few platforms it may discard data until later in the init
> sequence.
> 
> 2) change the CHATTER() macro to cope with a NULL pf field.
> 
> 3) default all devices to pf=NULL, which should be happening anyway
> because the CYG_FLASH_DRIVER() macro does not initialize it.
> 
> 4) remove the pf argument from cyg_flash_init() completely, so that by
> default the flash subsystem is silent.
> 
> 5) add a cyg_flash_set_printf() function which installs a
> printf()-style function for a given flash device. Or possibly two
> functions, one for a given device, another for all devices. Allow this
> function to be called before cyg_flash_init().
> 
> 6) change RedBoot to install diag_printf() before cyg_flash_init().
> 
> I think this automatically gives us the desired default behaviour for
> both applications and RedBoot, and may end up removing a diag_printf()
> dependency for some applications.


I think that's great and clearly set out. I expect I'll be up to my
eyeballs on eCos 3.0 stuff and the other bits and pieces you know about.

Can one of the other maintainers or contributors perhaps step up and take
this forward please? I think that pinning down the flash driver API for the
3.0 release is vital.

> Or we could be more ambitious and get rid of cyg_flash_init()
> completely, replacing it with a CYG_INIT_DEV_FLASH constructor.

That's a comparatively small step.

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

Re: [flashv2 merge] io/flash

[Bart Veer]

>>>>> "Jifl" == Jonathan Larmour <jifl@eCosCentric.com> writes:

    <snip>
    
    >> Now, I suspect that no solution is going to work for every bit of
    >> hardware that people can conceive off. As and when we run into
    >> problems we may have to add configury for setting certain init
    >> priorities, together with requires constraints in the platform HAL, to
    >> ensure that everything initializes in the correct order for a specific
    >> platform. However the following order should work for most systems:
    >> 
    >> #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_BUS_CAN		(?)
    >> #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_CONFIG
    >> #define CYG_INIT_DEV_BLOCK_SECONDARY
    >> #define CYG_INIT_DEV_CHAR		(all other devices)
    >> #define CYG_INIT_IO_FS
    Jifl> [big snip]

    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.

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

On the other hand, some drivers for e.g. framebuffer devices may need
to mess about with the memory map and lower memtop. Although really
that kind of thing should be handled at the platform linker script
level, not at run-time.

I suspect that in the long term we'll be happier initializing memalloc
early on.

Bart

-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.

Re: [flashv2 merge] io/flash

[Jonathan Larmour]

Bart Veer wrote:
>>>>>> "Jifl" == Jonathan Larmour <jifl@eCosCentric.com> writes:
> 
>     <snip>
>     
>     >> Now, I suspect that no solution is going to work for every bit of
>     >> hardware that people can conceive off. As and when we run into
>     >> problems we may have to add configury for setting certain init
>     >> priorities, together with requires constraints in the platform HAL, to
>     >> ensure that everything initializes in the correct order for a specific
>     >> platform. However the following order should work for most systems:
>     >> 
>     >> #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_BUS_CAN		(?)
>     >> #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_CONFIG
>     >> #define CYG_INIT_DEV_BLOCK_SECONDARY
>     >> #define CYG_INIT_DEV_CHAR		(all other devices)
>     >> #define CYG_INIT_IO_FS
>     Jifl> [big snip]
> 
>     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.

Of course the purpose of having these numbers abstracted is that if we do
need to, it can be tweaked.

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

I thought you were saying the order would be CYG_INIT_MEMALLOC,
CYG_INIT_IO_FS and then the rest of the CYG_INIT_IO*. I don't have any
issue with MEMALLOC being first, but wouldn't have thought that
CYG_INIT_IO_FS being before CYG_INIT_IO* would work well.

Were you saying something differnt perhaps?

> On the other hand, some drivers for e.g. framebuffer devices may need
> to mess about with the memory map and lower memtop. Although really
> that kind of thing should be handled at the platform linker script
> level, not at run-time.
> 
> I suspect that in the long term we'll be happier initializing memalloc
> early on.

I agree, and didn't disagree :-).

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

Re: [flashv2 merge] io/flash

[Bart Veer]

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

Although the amount of code affected is fairly small, the potential
impact on existing ports is large. Any changes in this area would be
new and have not had extensive testing in eCosCentric's testfarm.

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

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.

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

Bart

-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.

Re: [flashv2 merge] io/flash

[Jonathan Larmour]

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