Re: [Qemu-devel] [PATCH 4/7] m25p80: add a m25p80_set_rom_storage() routine

[mar.krzeminski]

W dniu 04.07.2016 o 20:12, Cédric Le Goater pisze:
> On 07/04/2016 07:57 PM, mar.krzeminski wrote:
> > W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze:
> > > Some SPI controllers, such as the Aspeed AST2400, have a mode in which
> > > accesses to the flash content are no different than doing MMIOs. The
> > > controller generates all the necessary commands to load (or store)
> > > data in memory.
> > >
> > > To emulate such a behavior, we need to have access to the underlying
> > > storage of the flash object. The purpose of the routine is to set the
> > > storage of a preinitialized SPI flash object, which can then be used
> > > by the object itself but also by a SPI controller to handled the
> > > MMIOs.
> > Hi,
> >
> > I am not sure if this approach is correct. I can not find any datasheet
> > to this SPI controller, but as you mentioned in first paragraph, controller
> > generates all commands (probably ones are somehow configurable).
> yes. see this patch :
>
>         http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg08229.html
>
>         /* CEx Control Register */
>         #define R_CTRL0           (0x10 / 4)
>         #define   CTRL_CMD_SHIFT           16
>         #define   CTRL_CMD_MASK            0xff
>
>
> > In this series you hack this behaviour and you do direct access to file.
> It is true it is not very respectful of the m25p80 interface.
>
> > IMHO you should emulate sending such commands in SPI controller
> > model.
> I will give it a try. I don't think the alternative is a complex
> change anyhow.
Register logic + few commands, then write is almost copy-paste.
Shouldn’t be complicated, but you have real HW modelling.

Thanks,
Marcin
> Thanks,
>
> C.
>
> > Thanks,
> > Marcin
> >
> > > This is sufficient to support read only accesses. For writes, we would
> > > certainly need to externalize flash_write8() routine.
> > >
> > > Signed-off-by: Cédric Le Goater <address@hidden>
> > > ---
> > >    hw/block/m25p80.c        | 14 +++++++++++++-
> > >    include/hw/block/flash.h |  2 ++
> > >    2 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > index aa964280beab..fec0fd4c2228 100644
> > > --- a/hw/block/m25p80.c
> > > +++ b/hw/block/m25p80.c
> > > @@ -29,6 +29,7 @@
> > >    #include "qemu/bitops.h"
> > >    #include "qemu/log.h"
> > >    #include "qapi/error.h"
> > > +#include "hw/block/flash.h"
> > >      #ifndef M25P80_ERR_DEBUG
> > >    #define M25P80_ERR_DEBUG 0
> > > @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error **errp)
> > >          if (s->blk) {
> > >            DB_PRINT_L(0, "Binding to IF_MTD drive\n");
> > > -        s->storage = blk_blockalign(s->blk, s->size);
> > > +
> > > +        /* using an external storage. see m25p80_create_rom() */
> > > +        if (!s->storage) {
> > > +            s->storage = blk_blockalign(s->blk, s->size);
> > > +        }
> > >              if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
> > >                error_setg(errp, "failed to read the initial flash content");
> > > @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void)
> > >    }
> > >      type_init(m25p80_register_types)
> > > +
> > > +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom)
> > > +{
> > > +    Flash *s = M25P80(dev);
> > > +
> > > +    s->storage = memory_region_get_ram_ptr(rom);
> > > +}
> > > diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
> > > index 50ccbbcf1352..9d780f4ae0c9 100644
> > > --- a/include/hw/block/flash.h
> > > +++ b/include/hw/block/flash.h
> > > @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
> > >    void ecc_reset(ECCState *s);
> > >    extern VMStateDescription vmstate_ecc_state;
> > >    +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom);
> > > +
> > >    #endif