Re: [coreboot] [PATCH] flashrom: clean up SPI probing, bus handling

[Carl-Daniel Hailfinger]

On 29.06.2008 01:43, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > On 29.06.2008 01:04, Stefan Reinauer wrote:
> > > See patch. It's a first attempt, but I think we should get this in in
> > > small enough step by step improvements.
> > >
> > > First attempt to clean up SPI probing and create a common
> > > construct: the flash bus.
> > >
> > > At some point the flash bus will be part of struct flashchip.
> > >
> > > Signed-off-by: Stefan Reinauer
> >
> > Nice, but I'd like a small design change:
> > Keep a list of available buses: flashbuses[]
> > Call all functions with an additional parameter: flashbus
>
> Yes, something along the line. Alongside with this I want to clean up
> use of struct flashchip
> for flashes[] but I would really like to get the current change in the
> tree yet.

I'm not a fan of committing code which will be changed shortly
afterwards, but...
> > Per-bus probing is a lot easier with that change.
> >
> > The cleanups unrelated to flash bus support are
> > Acked-by: Carl-Daniel Hailfinger
> >
> > For the rest, please consider my design change suggestions.
>
> Thanks.. please allow me to come up with something that integrates
> your suggestions after we get the current state in the tree.
>
> I am thinking of splitting up struct flashchip into the description
> part and the instance part.
>
> struct flashchip {
>        const char *vendor;
>        const char *name;
>        /* With 32bit manufacture_id and model_id we can cover IDs up to
>         * (including) the 4th bank of JEDEC JEP106W Standard
> Manufacturer's
>         * Identification code.
>         */
>        uint32_t manufacture_id;
>        uint32_t model_id;
>    int total_size;
>    int page_size;
>
>    /* Indicate if flashrom has been tested with this flash chip
>
> and if
>         * everything worked correctly.
>         */
>        uint32_t tested;
>    int (*probe) (struct flashchip *flash);
>    int (*erase) (struct flashchip *flash);
>    int (*write) (struct flashchip *flash, uint8_t *buf);
>    int (*read) (struct flashchip *flash, uint8_t *buf);

Maybe change struct flashchip to struct flashchip_instance for the 4
functions above?
>    uint32_t compatible_busses;
>
> }
>
> //  now this is what flashes is going to be:
> struct flashchip_instance {
>        struct flashchip *flash;
>    flashbus_t bus;
>      /* Some flash devices have an additional register space. */
>    volatile uint8_t *virtual_memory;
>    volatile uint8_t *virtual_registers;
>
>   unsigned long physical_memory;

What does that struct member mean?
> };

Overall, I like your new design.

Since we don't know yet for sure what's the problem with Pietro's hang,
I'd like to keep the code churn in SPI low until he has given us more info.

Regards,
Carl-Daniel
> > > Index: flash.h
> > >
> > > --- flash.h    (revision 3393)
> > > +++ flash.h    (working copy)
> > > @@ -370,10 +370,18 @@
> > >  /* chipset_enable.c */
> > >  int chipset_flash_enable(void);
> > >  void print_supported_chipsets(void);
> > > -extern int ich7_detected;
> > > -extern int ich9_detected;
> > > -extern void *ich_spibar;
> > >
> > > +typedef enum {
> >
> > BUS_TYPE_FWH
> > BUS_TYPE_PARALLEL
> > > BUS_TYPE_LPC,
> > > BUS_TYPE_ICH7_SPI,
> > > BUS_TYPE_ICH9_SPI,
> > > BUS_TYPE_IT87XX_SPI,
> > > BUS_TYPE_VIA_SPI
> > >
> > > +} flashbus_t;
> > >
> > > +extern flashbus_t flashbus;
> >
> > See my initial comment:
> >
> > extern flashbus_t flashbuses[];
> > > +flashbus_t flashbus = BUS_TYPE_LPC;
> >
> > flashbus_t flashbuses[MAX_BUSES] = {BUS_TYPE_LPC, };
> > int flashbus_count=1;
> >
> > (and matching changes below)