Re: [PATCH 4/10] Support for kernel+fs images

[Paul M. <le...@li...>]

On Thu, Jul 26, 2007 at 04:21:01PM +1000, David McCullough wrote:
> Jivin Paul Mundt lays it down ...
> > On Thu, Jul 26, 2007 at 03:27:13PM +1000, David McCullough wrote:
> > > Add support for copying our filesystem out of the way when images are
> > > constructed by concatenating the root filesystem to the end of a linux.bin
> > > which has the BSS removed (to save space).
> >
> > That's an interesting abuse :-)
> 
> It's a very old thing,  our images from the beginning of time have been
> like this.  If you don't like it,  no problems, you can get around it
> by making images with the BSS included in the linux.bin.
> 
Besides the watchdog cruft, I don't particularly have a problem with it.
I'll certainly accept patches for the functionality, although it will be
for 2.6.24.

> > CONFIG_SH_ROMBOOT is not added by any of your patches, likewise for
> > _mem_start and _rom_store. Presumably this should be sent as a follow up
> > patch if you want this functionality included.
> 
> My BAD,  the CONFIG_SH_ROMBOOT was contributed by someone else and since
> we don't use it,  I didn't need all the other bits to make it go.
> 
> I'll fix this if you think the "strange romfs thing" is acceptable in
> some shape or form.
> 
If there are sufficient users of this feature, then I don't have a
problem with merging it. If you aren't using romboot at all, then just
drop that from the update.

> > > +	/* copy backwards to avoid writing over ourselves */
> > > +	arch_service_watchdog();
> > > +	while (dp >= ((unsigned char *) (&_end[0]))) {
> > > +		*dp-- = *sp--;
> > > +		if ((((unsigned long) dp) & 0x7ffff) == 0)
> > > +			arch_service_watchdog();
> > > +	}
> > > +	arch_service_watchdog();
> > > +}
> > 
> > Can you not simply disable the watchdog in your loader and start it up
> > from the board code? Having to do dummy reads from the thing in this
> > convoluted manner is rather undesirable.
> 
> The watchdog is on from poweron, it cannot be disabled in any possible
> way.  You have at most 1 second (or less on some versions) to service it or
> it resets the hardware.
> 
Then perhaps we should just be abstracting out the romfs copy. ie, have
the generic copy_romfs() (possibly renamed to something more reasonable)
take care of extracting all of the various image details, and then kick
the addresses down to a platform-specific symbol to do the actual copy.
This too can be a weak symbol, where the default implementation simply
does the copy without the watchdog noise.

This way you can keep all of your platform-specific cruft isolated
without making a compromise on functionality.