Re: [PATCH 4/4 v4] mmc, ARM: Add zboot from eSD support for SuperH

[Simon Horman <horms@verge.net.au>]

Hi Magnus,

On Wed, Mar 16, 2011 at 09:37:50AM +0900, Magnus Damm wrote:
> Hi Simon,
> 
> Thanks for your work on this!

[ snip ]

> 
> On Wed, Mar 16, 2011 at 7:27 AM, Simon Horman <horms@verge.net.au> wrote:
> > This allows a ROM-able zImage to be written to eSD and for SuperH Mobile
> > ARM to boot directly from the SDHI hardware block.
> >
> > This is achieved by the MaskROM loading the first portion of the image into
> > MERAM and then jumping to it.  This portion contains loader code which
> > copies the entire image to SDRAM and jumps to it. From there the zImage
> > boot code proceeds as normal, uncompressing the image into its final
> > location and then jumping to it.
> >
> > Cc: Paul Mundt <lethal@linux-sh.org>
> > Cc: Magnus Damm <magnus.damm@gmail.com>
> > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >
> > ---
> >
> > This patch is based on the for-next branch of
> > Russell King's linux-2.6-arm tree
> >
> > v2
> > * Consistently use __raw_readw(). As pointed out by Paul Mundt
> >
> > v3
> > * Remove mmcif_update_progress2(), it was for debugging during development
> > * Move CPU specific code into mach/sdhi.h
> >  As requested by Magnus Damm
> > * Use linux/mmc/tmio.h now that it exists
> > * Remove SDHI_EXT_SWAP, it is unused
> > * Replace use of MMCIF_PROGRESS_* with MMC_PROGRESS_*
> > * Don't include linux/mmc/sh_mmcif.h
> >  + Replace use of MMCIF_CE_RESP_CMD12 with RESP_CMD12
> >  + Include linux/io.h
> >
> > v4
> > * Move definition of SDHI_BASE into CPU-specific code.
> >  Thanks to Magnus Damm.
> > ---
> 
> > +asmlinkage void mmc_loader(unsigned short *buf, unsigned long len)
> > +{
> > +       int high_capacity;
> > +
> > +       mmc_init_progress();
> > +
> > +       mmc_update_progress(MMC_PROGRESS_ENTER);
> > +       sdhi_boot_enter();
> > +
> > +       /* setup SDHI hardware */
> > +       mmc_update_progress(MMC_PROGRESS_INIT);
> > +       high_capacity = sdhi_boot_init(SDHI_BASE);
> > +       if (high_capacity < 0)
> > +               goto err;
> > +
> > +       mmc_update_progress(MMC_PROGRESS_LOAD);
> > +       /* load kernel */
> > +       if (sdhi_boot_do_read(SDHI_BASE, high_capacity,
> > +                             0, /* Kernel is at block 1 */
> > +                             (len + TMIO_BBS - 1) / TMIO_BBS, buf))
> > +               goto err;
> > +
> > +       sdhi_boot_cleanup();
> > +
> > +       mmc_update_progress(MMC_PROGRESS_DONE);
> > +
> > +       return;
> > +err:
> > +       __raw_writel(__raw_readl(PORTR031_000DR) | 1, PORTR031_000DR);
> > +       for(;;);
> > +
> > +}
> 
> Sorry for not catching this earlier, but this __raw_writel() to
> PORTR031_000DR is cpu specific as well. So please move that to the CPU
> specific code.

I will just remove that code, its was useful for development
but I didn't mean to include it in my patch submission.

> Not sure if it makes sense at this point, but perhaps it's a good idea
> to move the mmc_loader() function into the CPU specific portion. As
> you know, the CPU itself has multiple SDHI hardware blocks, and
> because of that we want the common SDHI loader to be written to
> support any SDHI hardware block instance.

Wouldn't that mean moving all of
arch/arm/boot/compressed/sdhi-shmobile.c into CPU specific code?
That could easily be achived by just guarding its compilation with
CONFIG_ARCH_SH7372 (as mmcif-sh7372.c already is) and perhaps
renaming the file to sdhi-sh7372.c. We could probably move
arch/arm/mach-shmobile/include/mach/sdhi-sh7372.h back into
sdhi-shmobile.c.

> Right now the SDHI_BASE variable is limiting the shared SDHI loader
> code to a fixed hardware block instance. That's fine because we only
> boot from a single SDHI hardware block instance on sh7372, but future
> processors most likely support selecting boot SDHI hardware block
> instance.

So mmc_loader() would need to take SDHI_BASE as an argument?
That sounds like a fairly small amount of refactoring.

How do you envisage that the hardware block would be selected?
At compile time through Kconfig? If so the current #define mechanism
might be sufficient.

> Apart from those minor bits I think the code is getting into a really
> good shape!

Thanks