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

login
register
about
Submitter Frederic Temporelli
Date 2011-08-01 17:21:19
Message ID <OF26291724.0748616D-ONC12578DF.005F55BA-C12578DF.005F5603@bull.net>
Download mbox | patch
Permalink /patch/3326/
State Superseded
Headers show

Comments

Frederic Temporelli - 2011-08-01 17:21:19
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 <frederic.temporelli@bull.net>
---
The first patch attached to my previous mail has been corrected
according to Stefan and Paul recommandations. Many thanks

--
Fred

-----Stefan Tauner <stefan.tauner@student.tuwien.ac.at> a écrit : -----
A : frederic.temporelli@bull.net
De : Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
Date : 01/08/2011 12:02
Cc : flashrom@flashrom.org
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
frederic.temporelli@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@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@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) */
Thomas Gstädtner - 2011-10-19 21:20:32
On -10.01.-28163 20:59, Frederic Temporelli wrote:
> 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 <frederic.temporelli@bull.net>
> 
> ---
> The first patch attached to my previous mail has been corrected
> according to Stefan and Paul recommandations. Many thanks
> 
> --
> Fred
> 
> -----Stefan Tauner <stefan.tauner@student.tuwien.ac.at> a écrit : -----
> A : frederic.temporelli@bull.net
> De : Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
> Date : 01/08/2011 12:02
> Cc : flashrom@flashrom.org
> 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
> frederic.temporelli@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@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@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) */
> 
> 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-08-01 21:17:40.573417039 -0400
> @@ -5,6 +5,8 @@
>   * Copyright (C) 2008 Joe Bao <Zheng.Bao@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@bull.net> for Bull SAS)
>   *
>   * 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,60 @@
>  
>  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).
> + * Then we have to read this register,
> + * and 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) {
> +		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);
> + 	return 0;
> +}
> +
> +/*
> + * 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;
> +
> +	pci_write_byte(lpc_isa_bridge, 0x98, 0x00);
> +
> +	msg_pspew("flash ownership is released.\n");
> +	return 0;
> +}
> +
>  static void reset_internal_fifo_pointer(void)
>  {
>  	mmio_writeb(mmio_readb(sb600_spibar + 2) | 0x10, sb600_spibar + 2);
> @@ -93,6 +149,8 @@
>  		      const unsigned char *writearr, unsigned char *readarr)
>  {
>  	int count;
> +	int result = 0;
> +
>  	/* First byte is cmd which can not being sent through FIFO. */
>  	unsigned char cmd = *writearr++;
>  	unsigned int readoffby1;
> @@ -103,16 +161,26 @@
>  	msg_pspew("%s, cmd=%x, writecnt=%x, readcnt=%x\n",
>  		  __func__, cmd, writecnt, readcnt);
>  
> +	if (resquest_lpc_ownership() != 0) {
> +		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
> @@ -141,8 +209,10 @@
>  	 * We should send the data by sequence, which means we need to reset
>  	 * the FIFO pointer to the first byte we want to send.
>  	 */
> -	if (reset_compare_internal_fifo_pointer(writecnt))
> -		return SPI_PROGRAMMER_ERROR;
> +	if (reset_compare_internal_fifo_pointer(writecnt)) {
> +		result = SPI_PROGRAMMER_ERROR;
> +		goto return_status;
> +	}
>  
>  	msg_pspew("Executing: \n");
>  	execute_command();
> @@ -158,8 +228,10 @@
>  	 * the opcode, the FIFO already stores the response from the chip.
>  	 * Usually, the chip will respond with 0x00 or 0xff.
>  	 */
> -	if (reset_compare_internal_fifo_pointer(writecnt + readcnt))
> -		return SPI_PROGRAMMER_ERROR;
> +	if (reset_compare_internal_fifo_pointer(writecnt + readcnt)) {
> +		result = SPI_PROGRAMMER_ERROR;
> +		goto return_status;
> +	}
>  
>  	/* Skip the bytes we sent. */
>  	msg_pspew("Skipping: ");
> @@ -168,8 +240,10 @@
>  		msg_pspew("[%02x]", cmd);
>  	}
>  	msg_pspew("\n");
> -	if (compare_internal_fifo_pointer(writecnt))
> -		return SPI_PROGRAMMER_ERROR;
> +	if (compare_internal_fifo_pointer(writecnt)) {
> +		result = SPI_PROGRAMMER_ERROR;
> +		goto return_status;
> +	}
>  
>  	msg_pspew("Reading: ");
>  	for (count = 0; count < readcnt; count++, readarr++) {
> @@ -177,8 +251,10 @@
>  		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 +262,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 +289,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) */

Acked-by: Thomas Gstaedtner <thomas@gstaedtner.net>

I tested this patch (along with 2/2 of course) on live hardware (>10
reads, >10 erase/writes) and it works fine for me.
Tested on: Supermicro H8SCM-F-O, AMD SP5100

Please note, that I did no proper code review, I only tested, so let me
know if the "acked-by" sign-off should not be used here.
It did not seem like you used the Tested-by sign-off.

thomasg
Stefan Tauner - 2011-10-20 13:20:53
On Wed, 19 Oct 2011 23:20:32 +0200
Thomas Gstädtner <thomas@gstaedtner.net> wrote:

> Please note, that I did no proper code review, I only tested, so let me
> know if the "acked-by" sign-off should not be used here.
> It did not seem like you used the Tested-by sign-off.

hey there and thanks for testing!

we do use that tag from time to time if it fits. i think it does so in
this case, because an acked-by is not appropriate without a code
review (and some past history within the community - not sure if this
is true, i think i have seen your name earlier...), but you should still
get credit for your effort.
Rudolf Marek - 2013-03-25 07:47:17
Hi all,

I was asked by Idwer to look into this. Since the patch was code reviewed 
already I have just some comments:

1) does it work if IMC is disabled?

If not, we should first detect if IMC firmware is active.

2) it writes to reserved bits 7:1

This should be easy fix

3) is there a way to release it back after writes are done?


Alternative way would be to send "go to IDLE/ram command to firmware.

Thanks
Rudolf
roxfan - 2013-03-25 09:04:33
Hello Rudolf,

  Recently we had someone on IRC with another AMD board. I had a look
  at its bios and wrote some pseudocode for its board enable function.
  Maybe it will be useful for this or other situations.

  P.S. the function names are mostly my own and there may be some small
  mistakes in the code.

// Supermicro H8SGL board enable pseudocode
// PrepareForFlashProgramming is at F000:05FC in the unpacked SLAB (module 1b)
// for the BIOS H8SGL2.831

h8sgl_board_enable:
  flash_write_enable()
  enable_rom_map()
  oem_enable()
  imc_gpio9_high()

imc_present:
  return pci[0:14.3][0x40] & 0x80; // IntegratedImcPresent

testenable_on:
  io[0xCD6] = 0x53; // TESTENABLE?
  io[0xCD7] |= 8;

testenable_off:
  io[0xCD6] = 0x53; // TESTENABLE?
  io[0xCD7] &= ~8;

// 8587
hpet_bar_unhide:
  pci[0:14.0][0x43] &= ~8; // HPETBarHid = 0

// 8599
hpet_bar_hide:
  pci[0:14.0][0x43] |= 8;  // HPETBarHid = 1

// 868A
imc_read_byte(idx):
  io[0x3E] = idx
  return io[0x3F]

// 8697
imc_write_byte(idx, val):
  io[0x3E] = idx
  io[0x3F] = val

// 86A4
imc_wait_reply:
  while 1:
    fixed_delay(66)
    if imc_read_byte(0x82) == 0xFA:
      break

// 8630
imc_send_command(id, val):
  if imc_present():
    imc_write_byte(0x82, 0)
    imc_write_byte(0x83, id>>8)
    imc_write_byte(0x84, id&0xFF)
    imc_write_byte(0x80, val)
    fixed_delay(4000)
    imc_wait_reply()

// 85D5
imc_disable:
  hpet_bar_unhide()
  if (pci[0:14.0][0x20] & 2) == 0:
    pci[0:14.0][0x20] |= 2
    if cmos[0x7D] & 8:
      imc_send_command(0x00B4, 0x96)
  hpet_bar_hide()

// 1388
disable_imc_if_present:
  if imc_present():
    testenable_on()
    imc_disable()

// 12FD
flash_write_enable:
  disable_imc_if_present()
  pci[0:14.0][0x79] |= 1; // PM_Addr_Enable = 1
  io[0xC6F] |= 0x40;      // Flash Rom Program Enable?

// 13DD
enable_rom_map:
  val = pci[0:14.0][0x41]
  val &= ~0x12
  val |= 0x12
  pci[0:14.0][0x41] = val
  
  pci[0:14.3][0xA3] |= 0x10; // Rom Range 2 Port Enable
  pci[0:14.3][0x6C]  = 0xA0; // LPC ROM Address Range 2

// 1353
oem_enable:
  if imc_present():
    hpet_bar_unhide()
    pci[0:14.0][0x20] &= ~1;
    hpet_bar_hide()
    testenable_off()

// 94F1
imc_gpio9_high:
  pci[0:14.3][0xC9] &= ~2; // IMC_Gpio_OeB[9]=0 (enable)
  pci[0:14.3][0xC7] |=  2; // IMC_Gpio_Out[9]=1 (high)

Monday, March 25, 2013, 8:47:17 AM, you wrote:

RM> Hi all,

RM> I was asked by Idwer to look into this. Since the patch was code reviewed
RM> already I have just some comments:

RM> 1) does it work if IMC is disabled?

RM> If not, we should first detect if IMC firmware is active.

RM> 2) it writes to reserved bits 7:1

RM> This should be easy fix

RM> 3) is there a way to release it back after writes are done?


RM> Alternative way would be to send "go to IDLE/ram command to firmware.

RM> Thanks
RM> Rudolf

RM> _______________________________________________
RM> flashrom mailing list
RM> flashrom@flashrom.org
RM> http://www.flashrom.org/mailman/listinfo/flashrom
Rudolf Marek - 2013-03-25 20:58:55
Hi all,

OK, so again it just shuts down IMC. I will try to come up with some patch.

Thanks
Rudolf

Patch

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-08-01 21:17:40.573417039 -0400
@@ -5,6 +5,8 @@ 
  * Copyright (C) 2008 Joe Bao <Zheng.Bao@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@bull.net> for Bull SAS)
  *
  * 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,60 @@ 
 
 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).
+ * Then we have to read this register,
+ * and 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) {
+		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);
+ 	return 0;
+}
+
+/*
+ * 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;
+
+	pci_write_byte(lpc_isa_bridge, 0x98, 0x00);
+
+	msg_pspew("flash ownership is released.\n");
+	return 0;
+}
+
 static void reset_internal_fifo_pointer(void)
 {
 	mmio_writeb(mmio_readb(sb600_spibar + 2) | 0x10, sb600_spibar + 2);
@@ -93,6 +149,8 @@ 
 		      const unsigned char *writearr, unsigned char *readarr)
 {
 	int count;
+	int result = 0;
+
 	/* First byte is cmd which can not being sent through FIFO. */
 	unsigned char cmd = *writearr++;
 	unsigned int readoffby1;
@@ -103,16 +161,26 @@ 
 	msg_pspew("%s, cmd=%x, writecnt=%x, readcnt=%x\n",
 		  __func__, cmd, writecnt, readcnt);
 
+	if (resquest_lpc_ownership() != 0) {
+		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
@@ -141,8 +209,10 @@ 
 	 * We should send the data by sequence, which means we need to reset
 	 * the FIFO pointer to the first byte we want to send.
 	 */
-	if (reset_compare_internal_fifo_pointer(writecnt))
-		return SPI_PROGRAMMER_ERROR;
+	if (reset_compare_internal_fifo_pointer(writecnt)) {
+		result = SPI_PROGRAMMER_ERROR;
+		goto return_status;
+	}
 
 	msg_pspew("Executing: \n");
 	execute_command();
@@ -158,8 +228,10 @@ 
 	 * the opcode, the FIFO already stores the response from the chip.
 	 * Usually, the chip will respond with 0x00 or 0xff.
 	 */
-	if (reset_compare_internal_fifo_pointer(writecnt + readcnt))
-		return SPI_PROGRAMMER_ERROR;
+	if (reset_compare_internal_fifo_pointer(writecnt + readcnt)) {
+		result = SPI_PROGRAMMER_ERROR;
+		goto return_status;
+	}
 
 	/* Skip the bytes we sent. */
 	msg_pspew("Skipping: ");
@@ -168,8 +240,10 @@ 
 		msg_pspew("[%02x]", cmd);
 	}
 	msg_pspew("\n");
-	if (compare_internal_fifo_pointer(writecnt))
-		return SPI_PROGRAMMER_ERROR;
+	if (compare_internal_fifo_pointer(writecnt)) {
+		result = SPI_PROGRAMMER_ERROR;
+		goto return_status;
+	}
 
 	msg_pspew("Reading: ");
 	for (count = 0; count < readcnt; count++, readarr++) {
@@ -177,8 +251,10 @@ 
 		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 +262,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 +289,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) */