On 17.07.2010 00:18, Michael Karcher wrote:
> Am Freitag, den 16.07.2010, 23:54 +0200 schrieb Carl-Daniel Hailfinger:
>
>> -extern enum bitbang_spi_master bitbang_spi_master;
>>
> [...]
>
>> -int bitbang_spi_init(void);
>> +int bitbang_spi_init(enum bitbang_spi_master master);
>>
> Eliminating global variables... great.
>
I also made it static now.
>> -/* Length of half a clock period in usecs */
>> -int bitbang_spi_half_period = 0;
>> +/* Length of half a clock period in usecs. Default to 1 (500 kHz). */
>> +int bitbang_spi_half_period = 1;
>>
> Why so slow? SPI was designed to be fast (if you want a slow system, use
> I2C instead ;) ). It might be quite sensible on programmers at the
> parallel port having some cable length and no driver chip at the flash
> piece (maybe that applies to SPIPGM), but that is specific to this
> programmer. nVidia on-board stuff should run slow enough without any
> programmer_delay calls.
>
Not sure. I figured that 1 usec would be a sane default, and could be
reduced to zero by individual programmers depending on their knowledge
about timing.
>> +/* Note that CS# is active low, so val=0 means the chip is active. */
>> void bitbang_spi_set_cs(int val)
>>
> I don't like the name spi_set_cs then, something like spi_set_negcs? To
> bad "/" and "#" characters are forbidden in identifiers...
>
Nobody besides SPI bitbanging driver writers will ever see that, and I
bet they all are happy about not having to care.
>> bitbang_spi_set_cs(1);
>> bitbang_spi_set_sck(0);
>> + bitbang_spi_set_mosi(0);
>>
> Initializeing all bits might make sense, looks OK.
>
Paranoid, I know.
>> buses_supported = CHIP_BUSTYPE_SPI;
>>
> Why do you take away LPC/FWH/PARALLEL here? Think nForce SPI, which
> could cooperate with nForce LPC just like on Intel chipsets.
>
You have a point. The bitbanging core now does not mess with
buses_supported anymore.
>> /* Never shrink. realloc() calls are expensive. */
>> if (bufsize > oldbufsize) {
>> - bufout = realloc(bufout, bufsize);
>> - if (!bufout) {
>> + tmp = realloc(bufout, bufsize);
>> + if (!tmp) {
>> msg_perr("Out of memory!\n");
>> + if (bufout)
>> + free(bufout);
>> + bufout = NULL;
>> if (bufin)
>> free(bufin);
>> + bufin = NULL;
>> + oldbufsize = 0;
>> exit(1);
>>
> We can (somehow) handle Intel chipsets rejecting instructions or not
> being able to execute some instruction without exit()ing. Why do we need
> to call exit here?
>
Failed memory allocation pretty much everywhere leads to exit(1).
Changed to return an error code instead, and just keep the old buffer.
That is saner anyway.
> Uncommented parts OK.
>
Thanks for the review.
Change the SPI bitbanging core to fix a few subtle bugs (which had no
effect so far) and to make integration of the RayeR SPIPGM and Nvidia
MCP6x/MCP7x SPI patches easier.
Handle out-of-memory gracefully instead of exiting the application.
A big thank you to Johannes Sjölund for testing the code as part of the
Nvidia MCP6x/MCP7x SPI bitbanging patch.
This is the common part of the RayeR/Nvidia SPI patch. Tested on a dozen
machines, works fine.
Signed-off-by: Carl-Daniel Hailfinger <[email protected]>
Index: flashrom-bitbang_spi_fixup/flash.h
===================================================================
--- flashrom-bitbang_spi_fixup/flash.h (Revision 1083)
+++ flashrom-bitbang_spi_fixup/flash.h (Arbeitskopie)
@@ -133,8 +133,6 @@
extern const int bitbang_spi_master_count;
-extern enum bitbang_spi_master bitbang_spi_master;
-
struct bitbang_spi_master_entry {
void (*set_cs) (int val);
void (*set_sck) (int val);
@@ -535,7 +533,7 @@
/* bitbang_spi.c */
extern int bitbang_spi_half_period;
extern const struct bitbang_spi_master_entry bitbang_spi_master_table[];
-int bitbang_spi_init(void);
+int bitbang_spi_init(enum bitbang_spi_master master);
int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt,
const unsigned char *writearr, unsigned char *readarr);
int bitbang_spi_read(struct flashchip *flash, uint8_t *buf, int start, int
len);
int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf, int start,
int len);
Index: flashrom-bitbang_spi_fixup/hwaccess.h
===================================================================
--- flashrom-bitbang_spi_fixup/hwaccess.h (Revision 1083)
+++ flashrom-bitbang_spi_fixup/hwaccess.h (Arbeitskopie)
@@ -176,6 +176,10 @@
#define __DARWIN__
#endif
+/* Clarification about OUTB/OUTW/OUTL argument order:
+ * OUT[BWL](val, port)
+ */
+
#if defined(__FreeBSD__) || defined(__DragonFly__)
#include <machine/cpufunc.h>
#define off64_t off_t
Index: flashrom-bitbang_spi_fixup/bitbang_spi.c
===================================================================
--- flashrom-bitbang_spi_fixup/bitbang_spi.c (Revision 1083)
+++ flashrom-bitbang_spi_fixup/bitbang_spi.c (Arbeitskopie)
@@ -26,10 +26,10 @@
#include "chipdrivers.h"
#include "spi.h"
-/* Length of half a clock period in usecs */
-int bitbang_spi_half_period = 0;
+/* Length of half a clock period in usecs. Default to 1 (500 kHz). */
+int bitbang_spi_half_period = 1;
-enum bitbang_spi_master bitbang_spi_master = BITBANG_SPI_INVALID;
+static enum bitbang_spi_master bitbang_spi_master = BITBANG_SPI_INVALID;
const struct bitbang_spi_master_entry bitbang_spi_master_table[] = {
{}, /* This entry corresponds to BITBANG_SPI_INVALID. */
@@ -37,6 +37,7 @@
const int bitbang_spi_master_count = ARRAY_SIZE(bitbang_spi_master_table);
+/* Note that CS# is active low, so val=0 means the chip is active. */
void bitbang_spi_set_cs(int val)
{
bitbang_spi_master_table[bitbang_spi_master].set_cs(val);
@@ -57,11 +58,18 @@
return bitbang_spi_master_table[bitbang_spi_master].get_miso();
}
-int bitbang_spi_init(void)
+int bitbang_spi_init(enum bitbang_spi_master master)
{
+ bitbang_spi_master = master;
+
+ if (bitbang_spi_master == BITBANG_SPI_INVALID) {
+ msg_perr("Invalid bitbang SPI master. \n"
+ "Please report a bug at [email protected]\n");
+ return 1;
+ }
bitbang_spi_set_cs(1);
bitbang_spi_set_sck(0);
- buses_supported = CHIP_BUSTYPE_SPI;
+ bitbang_spi_set_mosi(0);
return 0;
}
@@ -87,6 +95,7 @@
{
static unsigned char *bufout = NULL;
static unsigned char *bufin = NULL;
+ unsigned char *tmp;
static int oldbufsize = 0;
int bufsize;
int i;
@@ -98,20 +107,35 @@
bufsize = max(writecnt + readcnt, 260);
/* Never shrink. realloc() calls are expensive. */
if (bufsize > oldbufsize) {
- bufout = realloc(bufout, bufsize);
- if (!bufout) {
+ tmp = realloc(bufout, bufsize);
+ if (!tmp) {
msg_perr("Out of memory!\n");
- if (bufin)
- free(bufin);
- exit(1);
- }
- bufin = realloc(bufout, bufsize);
- if (!bufin) {
+ /* Old buffer is untouched, so we can just return. */
+ return SPI_GENERIC_ERROR;
+ } else
+ bufout = tmp;
+
+ tmp = realloc(bufin, bufsize);
+ if (!tmp) {
msg_perr("Out of memory!\n");
- if (bufout)
- free(bufout);
- exit(1);
- }
+ /* Now we have a problem. bufout has the new size, but
+ * bufin still has the old size. Try to shrink bufout
+ * so both buffers have the same old size.
+ */
+ tmp = realloc(bufout, oldbufsize);
+ if (!tmp) {
+ /* Not fatal, we're just using a few bytes more
+ * memory than necessary.
+ */
+ msg_perr("Shrinking allocation failed!\n");
+ } else {
+ bufout = tmp;
+ }
+ /* Old buffers are untouched, so we can just return. */
+ return SPI_GENERIC_ERROR;
+ } else
+ bufin = tmp;
+
oldbufsize = bufsize;
}
@@ -135,8 +159,13 @@
int bitbang_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len)
{
- /* Maximum read length is unlimited, use 64k bytes. */
- return spi_read_chunked(flash, buf, start, len, 64 * 1024);
+ /* Maximum read length is unlimited in theory.
+ * The current implementation can handle reads of up to 65536 bytes.
+ * Please note that you need two buffers of 2n+4 bytes each for a read
+ * of n bytes, resulting in a total memory requirement of 4n+8 bytes.
+ * To conserve memory, read in chunks of 256 bytes.
+ */
+ return spi_read_chunked(flash, buf, start, len, 256);
}
int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf, int start,
int len)
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
