<font face="Default Sans Serif,Verdana,Arial,Helvetica,sans-serif" size="2"><font size="2" face="Default Sans Serif,Verdana,Arial,Helvetica,sans-serif"><font size="2" face="Default Sans Serif,Verdana,Arial,Helvetica,sans-serif">Hello,<br><br><br>Here's  a patch for using SPI from AMD SouthBridge (SB700, SP5100, ...)<br>without  having issue with Integrated MicroControler  (IMC) .<br>This issue has been reported by Carl-Daniel Hailfinger in ChangeSet 1173<br><font size="2" face="Default Sans Serif,Verdana,Arial,Helvetica,sans-serif"><font size="2" face="Default Sans Serif,Verdana,Arial,Helvetica,sans-serif"><a target="_blank" class="moz-txt-link-freetext" href="http://flashrom.org/trac/flashrom/changeset/1173">http://flashrom.org/trac/flashrom/changeset/1173</a></font></font><br>AMD is now providing details about SP5100 register in document 44413:<br><a target="_blank" class="moz-txt-link-freetext" href="http://support.amd.com/us/Embedded_TechDocs/44413.pdf">http://support.amd.com/us/Embedded_TechDocs/44413.pdf</a><br>In this document (rev 3.02), we can see that a register is in charge of <br>managing access to LPC (p 271 and 283)<br>With this patch, we take LPC ownership before each set of commands to SPI.<br>Ownership is released when command is done.<br>So, there's no more SPI corrupted command due to the IMC. <br>Note: this patch doesn't remove the write protection.<br>A second patch will be in charge of removing this write protection.<br><br>Signed-off-by: Frederic Temporelli <a class="moz-txt-link-rfc2396E" href="mailto:frederic.temporelli@bull.net"><frederic.temporelli@bull.net></a><br>---<br>The first patch attached to my previous mail has been corrected<br>according to Stefan and Paul recommandations. Many thanks<br><br>--<br>Fred<br></font></font><br><font color="#990099">-----Stefan Tauner <a class="moz-txt-link-rfc2396E" href="mailto:stefan.tauner@student.tuwien.ac.at"><stefan.tauner@student.tuwien.ac.at></a> a écrit : -----</font><div><blockquote style="padding-right:0px;padding-left:5px;margin-left:5px;border-left:solid black 2px;margin-right:0px">A : <a class="moz-txt-link-abbreviated" href="mailto:frederic.temporelli@bull.net">frederic.temporelli@bull.net</a><br>De : Stefan Tauner <a class="moz-txt-link-rfc2396E" href="mailto:stefan.tauner@student.tuwien.ac.at"><stefan.tauner@student.tuwien.ac.at></a><br>Date : 01/08/2011 12:02<br>Cc : <a class="moz-txt-link-abbreviated" href="mailto:flashrom@flashrom.org">flashrom@flashrom.org</a><br>Objet : Re: [flashrom] AMD - SP5100 - take SPI ownership (1/2)<br><br><font size="2" face="Courier New,Courier,monospace">hello frederic,<br><br>thanks for your patches.<br>i have not looked at it in detail yet (i'll leave that to those who are<br>more familiar with the IMC problem), but i have spotted quite some<br>coding style issues. it would be great if you could fix those.<br>i'll explain what i noticed inline.<br><br>On Mon, 1 Aug 2011 10:45:24 +0200<br><a class="moz-txt-link-abbreviated" href="mailto:frederic.temporelli@bull.net">frederic.temporelli@bull.net</a> wrote:<br><br>> diff -ur ../flashrom-0.9.4-r1394/sb600spi.c ./sb600spi.c<br>> --- ../flashrom-0.9.4-r1394/sb600spi.c  2011-05-11 13:07:07.000000000 -0400<br>> +++ ./sb600spi.c      2011-07-29 22:45:48.693159918 -0400<br>> @@ -5,6 +5,8 @@<br>>   * Copyright (C) 2008 Joe Bao <a class="moz-txt-link-rfc2396E" href="mailto:Zheng.Bao@amd.com"><Zheng.Bao@amd.com></a><br>>   * Copyright (C) 2008 Advanced Micro Devices, Inc.<br>>   * Copyright (C) 2009, 2010 Carl-Daniel Hailfinger<br>> + * Copyright (C) 2011 Bull SAS<br>> + * (Written by Frederic Temporelli <a class="moz-txt-link-rfc2396E" href="mailto:frederic.temporelli@bull.net"><frederic.temporelli@bull.net></a> for Bull SAS )<br>space before the closing parenthesis<br><br>>   *<br>>   * This program is free software; you can redistribute it and/or modify<br>>   * it under the terms of the GNU General Public License as published by<br>> @@ -43,6 +45,63 @@<br>>  <br>>  static uint8_t *sb600_spibar = NULL;<br>>  <br>> +/* reference to LPC/SPI PCI device,<br>> + * requested for requesting/releasing<br>> + * device access, shared with IMC<br>> + */ <br>> +struct pci_dev *lpc_isa_bridge = NULL;<br>> +<br>> +<br>> +/*<br>> + * avoid interaction from IMC while we are working with LPC/SPI<br>> + * this is done by requesting HostOwnLPC register (LPC_ISA_BRIDGE, write 1 in register 0x98)<br>line too long, please split (and some punctuation would increase readability then)<br><br>> + * then we have to read this register.<br>> + * Loop until we have the ownership<br>> + * see doc AMD 44413 - AMD SP5100 Register Reference Guide<br>> + */<br>> +static int resquest_lpc_ownership (void)<br>> +{<br>> +    uint8_t tmp;<br>> +    int i = 0;<br>> +<br>> +      if (lpc_isa_bridge == NULL)<br>> +             return 1;<br>> +<br>> +       pci_write_byte(lpc_isa_bridge, 0x98, 0x01);<br>> +<br>> +     while ( (tmp = pci_read_byte (lpc_isa_bridge, 0x98)) == 0 ){<br>we don't use inner space around clauses like here '( (' and '0 )' usually.<br>also there is a space missing between ')' and '{'<br><br>> +           pci_write_byte(lpc_isa_bridge, 0x98, 0x01);<br>> +             if (++i > 1024) {<br>> +                    msg_perr("IMC did not release flash.\n");<br>> +                     return 1;<br>> +               }<br>> +<br>> +       }<br>> +       msg_pspew("flash ownership after %i cycles.\n", i);<br>> +   i = 0;<br>please drop the line above completely<br><br>> +         return 0;<br>> +}<br>> +<br>> +<br>one line break between functions please<br><br>> +/*<br>> + * release LPC/SPI ownership (IMC can access to these resources)<br>> + * this is done by releasing HostOwnLPC register<br>> + * (write 0 in register 0x98)<br>> + */<br>> +static int release_lpc_ownership (void)<br>> +{<br>> +<br>> + if (lpc_isa_bridge == NULL)<br>> +             return 1;<br>> +<br><br>spaces instead of tabs in the following lines:<br>> +        pci_write_byte(lpc_isa_bridge, 0x98, 0x00);<br>> +<br>> +        msg_pspew("flash ownership is released.\n");<br>> +        return 0;<br>> +}<br>> +<br>> +<br>> +<br>one line break between functions please<br><br>>  static void reset_internal_fifo_pointer(void)<br>>  {<br>>        mmio_writeb(mmio_readb(sb600_spibar + 2) | 0x10, sb600_spibar + 2);<br>> @@ -93,6 +152,8 @@<br>>                       const unsigned char *writearr, unsigned char *readarr)<br>>  {<br>>    int count;<br>spaces instead of tabs<br>> +        int result = 0;<br>> +<br>>        /* First byte is cmd which can not being sent through FIFO. */<br>>       unsigned char cmd = *writearr++;<br>>     unsigned int readoffby1;<br>> @@ -103,16 +164,26 @@<br>>         msg_pspew("%s, cmd=%x, writecnt=%x, readcnt=%x\n",<br>>                   __func__, cmd, writecnt, readcnt);<br>>  <br>> +  if (resquest_lpc_ownership() != 0){<br>space missing between ')' and '{'<br><br>> +                result = SPI_PROGRAMMER_ERROR;<br>> +          msg_pinfo("can't take ownership of LPC/SPI");<br>> +         goto return_status;<br>> +     }<br>> + <br>> +<br>>   if (readcnt > 8) {<br>>                msg_pinfo("%s, SB600 SPI controller can not receive %d bytes, "<br>>                   "it is limited to 8 bytes\n", __func__, readcnt);<br>> -               return SPI_INVALID_LENGTH;<br>> +<br>> +              result = SPI_INVALID_LENGTH;<br>> +            goto return_status;<br>>          }<br>>  <br>>       if (writecnt > 8) {<br>>               msg_pinfo("%s, SB600 SPI controller can not send %d bytes, "<br>>                      "it is limited to 8 bytes\n", __func__, writecnt);<br>> -              return SPI_INVALID_LENGTH;<br>> +              result = SPI_INVALID_LENGTH;<br>> +            goto return_status;<br>>          }<br>>  <br>>       /* This is a workaround for a bug in SB600 and SB700. If we only send<br>> @@ -142,7 +213,10 @@<br>>      * the FIFO pointer to the first byte we want to send.<br>>        */<br>>          if (reset_compare_internal_fifo_pointer(writecnt))<br>> -              return SPI_PROGRAMMER_ERROR;<br>> +    {<br>the '{' should be in the same line as the 'if'<br>> +               result = SPI_PROGRAMMER_ERROR;<br>> +          goto return_status;<br>> +     }<br>>  <br>>       msg_pspew("Executing: \n");<br>>        execute_command();<br>> @@ -159,7 +233,10 @@<br>>         * Usually, the chip will respond with 0x00 or 0xff.<br>>          */<br>>          if (reset_compare_internal_fifo_pointer(writecnt + readcnt))<br>> -            return SPI_PROGRAMMER_ERROR;<br>> +    {<br>here too<br>> +             result = SPI_PROGRAMMER_ERROR;<br>> +          goto return_status;<br>> +     }<br>>  <br>>       /* Skip the bytes we sent. */<br>>        msg_pspew("Skipping: ");<br>> @@ -168,8 +245,11 @@<br>>                msg_pspew("[%02x]", cmd);<br>>          }<br>>    msg_pspew("\n");<br>> -      if (compare_internal_fifo_pointer(writecnt))<br>> -            return SPI_PROGRAMMER_ERROR;<br>> +    if (compare_internal_fifo_pointer(writecnt)){<br>missing space<br><br>> +<br>> +          result = SPI_PROGRAMMER_ERROR;<br>> +          goto return_status;<br>> +     }<br>>  <br>>       msg_pspew("Reading: ");<br>>    for (count = 0; count < readcnt; count++, readarr++) {<br>> @@ -177,8 +257,11 @@<br>>                 msg_pspew("[%02x]", *readarr);<br>>     }<br>>    msg_pspew("\n");<br>> -      if (reset_compare_internal_fifo_pointer(readcnt + writecnt))<br>> -            return SPI_PROGRAMMER_ERROR;<br>> +    if (reset_compare_internal_fifo_pointer(readcnt + writecnt)) {<br>> +<br>> +          result = SPI_PROGRAMMER_ERROR;<br>> +          goto return_status;<br>> +     }<br>>  <br>>       if (mmio_readb(sb600_spibar + 1) != readwrite) {<br>>             msg_perr("Unexpected change in SB600 read/write count!\n");<br>> @@ -186,10 +269,12 @@<br>>                     "causes random corruption.\nPlease stop all "<br>>                      "applications and drivers and IPMI which access the "<br>>                      "flash chip.\n");<br>> -            return SPI_PROGRAMMER_ERROR;<br>> +            result = SPI_PROGRAMMER_ERROR;<br>>       }<br>>  <br>> -  return 0;<br>> +return_status:<br>> + release_lpc_ownership();<br>> +        return result;<br>>  }<br>>  <br>>  static const struct spi_programmer spi_programmer_sb600 = {<br>> @@ -211,6 +296,9 @@<br>>                  "Reserved", "33", "22", "16.5"<br>>       };<br>>  <br>> +<br>> +         lpc_isa_bridge = dev;<br>> +<br>>        /* Read SPI_BaseAddr */<br>>      tmp = pci_read_long(dev, 0xa0);<br>>      tmp &= 0xffffffe0;  /* remove bits 4-0 (reserved) */<br><br>-- <br>Kind regards/Mit freundlichen Grüßen, Stefan Tauner<br></font></blockquote></div><div></div></font>