Re: [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
[e-mail] 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
> Copyright (C) 2008 Advanced Micro Devices, Inc.
> Copyright (C) 2009, 2010 Carl-Daniel Hailfinger
> Copyright (C) 2011 Bull SAS
> (Written by Frederic Temporelli  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) */

AMD - SP5100 - take SPI ownership (1/2)

[Paul Menzel]

Dear Frederic,

Am Montag, den 01.08.2011, 11:32 +0200 schrieb Stefan Tauner:
> thanks for your patches.

indeed a big thank you.
> i have not looked at it in detail yet (i'll leave that to those who are
> more familiar with the IMC problem),

Same for me.
> but i have spotted quite some coding style issues. it would be great if you could fix those.

[…]

Please also add your Signed-off-by line as required by the development
guidelines [1].

[…]

Thanks,

Paul

[1] http://flashrom.org/Development_Guidelines

AMD - SP5100 - take SPI ownership (1/2)

Hello,

Here's  a patch for using SPI from AMD SouthBridge (SB700, SP5100, ...)
without  having issue with Integrated MicroControler  (IMC) .
This issue has been reported by Carl-Daniel Hailfinger in ChangeSet 1173
http://flashrom.org/trac/flashrom/changeset/1173
AMD is now providing details about SP5100 register in document 44413:
http://support.amd.com/us/Embedded_TechDocs/44413.pdf
In this document (rev 3.02), we can see that a register is in charge of 
managing access to LPC (p 271 and 283)
With this patch, we take LPC ownership before each set of commands to SPI.
Ownership is released when command is done.
So, there's no more SPI corrupted command due to the IMC. 
Note: this patch doesn't remove the write protection.
A second patch will be in charge of removing this write protection.

Signed-off-by: Frederic Temporelli 
---
The first patch attached to my previous mail has been corrected
according to Stefan and Paul recommandations. Many thanks

-----Stefan Tauner  a écrit : -----
A : [e-mail]
De : Stefan Tauner 
Date : 01/08/2011 12:02
Cc : [e-mail]
Objet : Re: [flashrom] AMD - SP5100 - take SPI ownership (1/2)

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
[e-mail] 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 
>   * Copyright (C) 2008 Advanced Micro Devices, Inc.
>   * Copyright (C) 2009, 2010 Carl-Daniel Hailfinger
> Copyright (C) 2011 Bull SAS
> (Written by Frederic Temporelli  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) */