[flashrom] AMD - SP5100 - take SPI ownership (1/2)

[Stefan Tauner]

hello frederic,

thanks for your patches.
i have not looked at it in detail yet (i'll leave that to those who are
more familiar with the IMC problem), but i have spotted quite some
coding style issues. it would be great if you could fix those.
i'll explain what i noticed inline.

On Mon, 1 Aug 2011 10:45:24 +0200
frederic.temporelli at bull.net wrote:

> diff -ur ../flashrom-0.9.4-r1394/sb600spi.c ./sb600spi.c
> --- ../flashrom-0.9.4-r1394/sb600spi.c	2011-05-11 13:07:07.000000000 -0400
> +++ ./sb600spi.c	2011-07-29 22:45:48.693159918 -0400
> @@ -5,6 +5,8 @@
>   * Copyright (C) 2008 Joe Bao <Zheng.Bao at amd.com>
>   * Copyright (C) 2008 Advanced Micro Devices, Inc.
>   * Copyright (C) 2009, 2010 Carl-Daniel Hailfinger
> + * Copyright (C) 2011 Bull SAS
> + * (Written by Frederic Temporelli <frederic.temporelli at bull.net> for Bull SAS )
space before the closing parenthesis

>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -43,6 +45,63 @@
>  
>  static uint8_t *sb600_spibar = NULL;
>  
> +/* reference to LPC/SPI PCI device,
> + * requested for requesting/releasing
> + * device access, shared with IMC
> + */ 
> +struct pci_dev *lpc_isa_bridge = NULL;
> +
> +
> +/*
> + * avoid interaction from IMC while we are working with LPC/SPI
> + * this is done by requesting HostOwnLPC register (LPC_ISA_BRIDGE, write 1 in register 0x98)
line too long, please split (and some punctuation would increase readability then)

> + * then we have to read this register.
> + * Loop until we have the ownership
> + * see doc AMD 44413 - AMD SP5100 Register Reference Guide
> + */
> +static int resquest_lpc_ownership (void)
> +{
> +	uint8_t tmp;
> +	int i = 0;
> +
> +	if (lpc_isa_bridge == NULL)
> +		return 1;
> +
> +	pci_write_byte(lpc_isa_bridge, 0x98, 0x01);
> +
> +	while ( (tmp = pci_read_byte (lpc_isa_bridge, 0x98)) == 0 ){
we don't use inner space around clauses like here '( (' and '0 )' usually.
also there is a space missing between ')' and '{'

> +		pci_write_byte(lpc_isa_bridge, 0x98, 0x01);
> +		if (++i > 1024) {
> +			msg_perr("IMC did not release flash.\n");
> +			return 1;
> +		}
> +
> +	}
> +	msg_pspew("flash ownership after %i cycles.\n", i);
> +	i = 0;
please drop the line above completely

> + 	return 0;
> +}
> +
> +
one line break between functions please

> +/*
> + * release LPC/SPI ownership (IMC can access to these resources)
> + * this is done by releasing HostOwnLPC register
> + * (write 0 in register 0x98)
> + */
> +static int release_lpc_ownership (void)
> +{
> +
> +	if (lpc_isa_bridge == NULL)
> +		return 1;
> +

spaces instead of tabs in the following lines:
> +        pci_write_byte(lpc_isa_bridge, 0x98, 0x00);
> +
> +        msg_pspew("flash ownership is released.\n");
> +        return 0;
> +}
> +
> +
> +
one line break between functions please

>  static void reset_internal_fifo_pointer(void)
>  {
>  	mmio_writeb(mmio_readb(sb600_spibar + 2) | 0x10, sb600_spibar + 2);
> @@ -93,6 +152,8 @@
>  		      const unsigned char *writearr, unsigned char *readarr)
>  {
>  	int count;
spaces instead of tabs
> +        int result = 0;
> +
>  	/* First byte is cmd which can not being sent through FIFO. */
>  	unsigned char cmd = *writearr++;
>  	unsigned int readoffby1;
> @@ -103,16 +164,26 @@
>  	msg_pspew("%s, cmd=%x, writecnt=%x, readcnt=%x\n",
>  		  __func__, cmd, writecnt, readcnt);
>  
> +	if (resquest_lpc_ownership() != 0){
space missing between ')' and '{'

> +		result = SPI_PROGRAMMER_ERROR;
> +		msg_pinfo("can't take ownership of LPC/SPI");
> +		goto return_status;
> +	}
> + 
> +
>  	if (readcnt > 8) {
>  		msg_pinfo("%s, SB600 SPI controller can not receive %d bytes, "
>  		       "it is limited to 8 bytes\n", __func__, readcnt);
> -		return SPI_INVALID_LENGTH;
> +
> +		result = SPI_INVALID_LENGTH;
> +		goto return_status;
>  	}
>  
>  	if (writecnt > 8) {
>  		msg_pinfo("%s, SB600 SPI controller can not send %d bytes, "
>  		       "it is limited to 8 bytes\n", __func__, writecnt);
> -		return SPI_INVALID_LENGTH;
> +		result = SPI_INVALID_LENGTH;
> +		goto return_status;
>  	}
>  
>  	/* This is a workaround for a bug in SB600 and SB700. If we only send
> @@ -142,7 +213,10 @@
>  	 * the FIFO pointer to the first byte we want to send.
>  	 */
>  	if (reset_compare_internal_fifo_pointer(writecnt))
> -		return SPI_PROGRAMMER_ERROR;
> +	{
the '{' should be in the same line as the 'if'
> +		result = SPI_PROGRAMMER_ERROR;
> +		goto return_status;
> +	}
>  
>  	msg_pspew("Executing: \n");
>  	execute_command();
> @@ -159,7 +233,10 @@
>  	 * Usually, the chip will respond with 0x00 or 0xff.
>  	 */
>  	if (reset_compare_internal_fifo_pointer(writecnt + readcnt))
> -		return SPI_PROGRAMMER_ERROR;
> +	{
here too
> +		result = SPI_PROGRAMMER_ERROR;
> +		goto return_status;
> +	}
>  
>  	/* Skip the bytes we sent. */
>  	msg_pspew("Skipping: ");
> @@ -168,8 +245,11 @@
>  		msg_pspew("[%02x]", cmd);
>  	}
>  	msg_pspew("\n");
> -	if (compare_internal_fifo_pointer(writecnt))
> -		return SPI_PROGRAMMER_ERROR;
> +	if (compare_internal_fifo_pointer(writecnt)){
missing space

> +
> +		result = SPI_PROGRAMMER_ERROR;
> +		goto return_status;
> +	}
>  
>  	msg_pspew("Reading: ");
>  	for (count = 0; count < readcnt; count++, readarr++) {
> @@ -177,8 +257,11 @@
>  		msg_pspew("[%02x]", *readarr);
>  	}
>  	msg_pspew("\n");
> -	if (reset_compare_internal_fifo_pointer(readcnt + writecnt))
> -		return SPI_PROGRAMMER_ERROR;
> +	if (reset_compare_internal_fifo_pointer(readcnt + writecnt)) {
> +
> +		result = SPI_PROGRAMMER_ERROR;
> +		goto return_status;
> +	}
>  
>  	if (mmio_readb(sb600_spibar + 1) != readwrite) {
>  		msg_perr("Unexpected change in SB600 read/write count!\n");
> @@ -186,10 +269,12 @@
>  			 "causes random corruption.\nPlease stop all "
>  			 "applications and drivers and IPMI which access the "
>  			 "flash chip.\n");
> -		return SPI_PROGRAMMER_ERROR;
> +		result = SPI_PROGRAMMER_ERROR;
>  	}
>  
> -	return 0;
> +return_status:
> +	release_lpc_ownership();
> +	return result;
>  }
>  
>  static const struct spi_programmer spi_programmer_sb600 = {
> @@ -211,6 +296,9 @@
>  		"Reserved", "33", "22", "16.5"
>  	};
>  
> +
> + 	lpc_isa_bridge = dev;
> +
>  	/* Read SPI_BaseAddr */
>  	tmp = pci_read_long(dev, 0xa0);
>  	tmp &= 0xffffffe0;	/* remove bits 4-0 (reserved) */

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner