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

[Thomas Gstädtner]

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
>
> 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) */
>
> 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
> 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)
> 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

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

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

[Stefan Tauner]

On Wed, 19 Oct 2011 23:20:32 +0200
Thomas Gstädtner  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.