[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 <c-d.hailfinger.devel.2006 at gmx.net> 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