Re: [coreboot] coreboot & OpenSolaris

[Carl-Daniel Hailfinger]

Hi Joerg,

after the review I have some comments on the patch you sent. The
comments are inline in the patch, it would be great if you could resend
with the mentioned changes and also add a "Signed-off-by:" statement to
your submission. See
http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure for
details.

Thanks.

On 04.06.2008 16:42, Carl-Daniel Hailfinger wrote:
> Hi,
>
> [please CC Jörg on replies, I don't know whether he is already
> subscribed to the list.]
> Jörg Schilling and I have been working to get flashrom to compile again
> under Solaris. The credit belongs to Jörg.
>
> The patch below is a first hack to get everything running and not
> completely ready for merging. Adding platform-specific #include
> statements to each file is suboptimal, I'd prefer to have them all in
> flash.h. Once we have a mergeable patch, I'd like to get a review from
> the person who created Solaris support in the first place.
>
> Besides that, we may want to disable compilation explicitly for any
> untested arch. Especially the problem that there is no agreed-upon order
> of out[bwl] arguments is a disaster waiting to happen if someone tests a
> new Linux-incompatible platform.
>
> Regards,
> Carl-Daniel
>
> On 04.06.2008 16:22, Joerg Schilling wrote:
> > Carl-Daniel Hailfinger  wrote:
> > > Hast Du evtl. einen unfertigen Patch, den ich durchs Review jagen kann?
> > > Es wäre klasse, wenn wir das vor dem Release 1.0 mergen können.
> >
> > hier ist erstmal mein aktueller Stand:
> >
> > --- Makefile.orig	Di Jun  3 16:53:49 2008
> > +++ Makefile	Mi Jun  4 16:21:23 2008
> > @@ -15,6 +15,8 @@
> >  OS_ARCH	= $(shell uname)
> >  ifeq ($(OS_ARCH), SunOS)
> >  LDFLAGS = -lpci -lz
> > +CFLAGS += -I.
> > +LDFLAGS += -L/tmp/pciutils-2.2.10/lib/
> >  else
> >  LDFLAGS = -lpci -lz
> >  STRIP_ARGS = -s

Please skip the hunk for now since it is specific to your installation.
We can still merge something similar later.
> > @@ -52,7 +54,7 @@
> >    rm -f $(PROGRAM) .dependencies
> >
> > dep:
> > @$(CC) -MM *.c > .dependencies
> > $(CC) -M $(CFLAGS) $(OBJS:%.o=%.c) > .dependencies
> >
> > pciutils:
> >    @echo; echo -n "Checking for pciutils and zlib... "

I have trouble parsing the reason for the hunk above. AFAICS this is a
change for better debugging only.
> > @@ -60,7 +62,7 @@
> >    	   echo "struct pci_access *pacc;";	   \
> >    	   echo "int main(int argc, char **argv)"; \
> >    	   echo "{ pacc = pci_alloc(); return 0; }"; ) > .test.c )
> > @$(CC) $(CFLAGS) .test.c -o .test $(LDFLAGS) &>/dev/null &&	\
> > @$(CC) $(CFLAGS) .test.c -o .test $(LDFLAGS) >/dev/null 2>&1 &&	\
> > echo "found." || ( echo "not found."; echo;		\
> > echo "Please install pciutils-devel and zlib-devel.";	\
> > echo "See README for more information."; echo;		\
> > Index: flash.h
> >
> > --- flash.h	(revision 3360)
> > +++ flash.h	(working copy)
> > @@ -41,6 +41,14 @@
> >    #define INW(x) __extension__ ({ u_int tmp = (x); inw(tmp); })
> >    #define INL(x) __extension__ ({ u_int tmp = (x); inl(tmp); })
> >  #else
> > +#ifdef	__sun
> > #define OUTB(x, y) outb(y, x)
> > #define OUTW(x, y) outw(y, x)
> > #define OUTL(x, y) outl(y, x)
> > #define INB  inb
> > #define INW  inw
> > #define INL  inl
> >
> > +#else
> >    #define OUTB outb
> >    #define OUTW outw
> >    #define OUTL outl
> > @@ -48,6 +56,7 @@
> >    #define INW  inw
> >    #define INL  inl
> >  #endif
> > +#endif
> >
> > #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))

I'm contemplating whether we should use #elif here. It would surely make
the code more readable.
> > @@ -357,7 +366,7 @@
> >
> > /* udelay.c */
> >  void myusec_delay(int time);
> > -void myusec_calibrate_delay();
> > +void myusec_calibrate_delay(void);
> >
> > /* PCI handling for board/chipset_enable */
> >  struct pci_access *pacc;
> > @@ -406,12 +415,12 @@
> >  int probe_spi_rdid(struct flashchip *flash);
> >  int probe_spi_res(struct flashchip *flash);
> >  int spi_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr);
> > -void spi_write_enable();
> > -void spi_write_disable();
> > +void spi_write_enable(void);
> > +void spi_write_disable(void);
> >  int spi_chip_erase_c7(struct flashchip *flash);
> >  int spi_chip_write(struct flashchip *flash, uint8_t *buf);
> >  int spi_chip_read(struct flashchip *flash, uint8_t *buf);
> > -uint8_t spi_read_status_register();
> > +uint8_t spi_read_status_register(void);
> >  void spi_disable_blockprotect(void);
> >  void spi_byte_program(int address, uint8_t byte);
> >  void spi_page_program(int block, uint8_t *buf, uint8_t *bios);
> > @@ -450,6 +459,8 @@
> >    		     volatile uint8_t *dst);
> >  int probe_jedec(struct flashchip *flash);
> >  int erase_chip_jedec(struct flashchip *flash);
> > +int write_page_write_jedec(volatile uint8_t *bios, uint8_t *src,
> > 	   volatile uint8_t *dst, int page_size);
> >
> > int write_jedec(struct flashchip *flash, uint8_t *buf);
> >  int erase_sector_jedec(volatile uint8_t *bios, unsigned int page);
> >  int erase_block_jedec(volatile uint8_t *bios, unsigned int page);

Any reason for that write_page_write_jedec prototype?
> > Index: it87spi.c
> >
> > --- it87spi.c	(revision 3360)
> > +++ it87spi.c	(working copy)
> > @@ -26,6 +26,10 @@
> >  #include <pci/pci.h>
> >  #include <stdint.h>
> >  #include <string.h>
> > +/* for outb(),... */
> > +#if defined (__sun) && (defined(__i386) || defined(__amd64))
> > +#include <asm/sunddi.h>	/* XXX nonportable gcc/_KERNEL only, may disappear */
> > +#endif
> >  #include "flash.h"
> >  #include "spi.h"

Could you add the #include to flash.h instead?
> > Index: chipset_enable.c
> >
> > --- chipset_enable.c	(revision 3360)
> > +++ chipset_enable.c	(working copy)
> > @@ -33,6 +33,10 @@
> >  #include <sys/mman.h>
> >  #include <fcntl.h>
> >  #include <unistd.h>
> > +/* for outb(),... */
> > +#if defined (__sun) && (defined(__i386) || defined(__amd64))
> > +#include <asm/sunddi.h>	/* XXX nonportable gcc/_KERNEL only, may disappear */
> > +#endif
> >  #include "flash.h"
> >
> > static int enable_flash_ali_m1533(struct pci_dev *dev, const char *name)

Same here.
> > Index: flashrom.c
> >
> > --- flashrom.c	(revision 3360)
> > +++ flashrom.c	(working copy)
> > @@ -33,10 +33,9 @@
> >  #include <pci/pci.h>
> >  /* for iopl */
> >  #if defined (__sun) && (defined(__i386) || defined(__amd64))
> > -#include <strings.h>
> >  #include <sys/sysi86.h>
> >  #include <sys/psw.h>
> > -#include <asm/sunddi.h>
> > +#include <asm/sunddi.h>	/* XXX nonportable gcc/_KERNEL only, may disappear */
> >  #endif
> >  #include "flash.h"

Same here.
> > Index: board_enable.c
> >
> > --- board_enable.c	(revision 3360)
> > +++ board_enable.c	(working copy)
> > @@ -29,6 +29,10 @@
> >  #include <stdint.h>
> >  #include <string.h>
> >  #include <fcntl.h>
> > +/* for outb(),... */
> > +#if defined (__sun) && (defined(__i386) || defined(__amd64))
> > +#include <asm/sunddi.h>	/* XXX nonportable gcc/_KERNEL only, may disappear */
> > +#endif
> >  #include "flash.h"
> >
> > /*

And here.

Regards,
Carl-Daniel