Patchwork [RFC] National Semiconductor DP83815 NIC patch

login
register
about
Submitter Andrew Morgan
Date 2010-06-07 20:53:28
Message ID <4C0D5C48.5010707@ziltro.com>
Download mbox | patch
Permalink /patch/1483/
State Superseded
Headers show

Comments

Andrew Morgan - 2010-06-07 20:53:28
On 04/06/10 15:13, Carl-Daniel Hailfinger wrote:
> Oh, it looks really nice.
> In fact, we could commit this instantly except for two reasons:
> 1. It's untested, and completely untested drivers should default to no
> in the Makefile. Once we have the first successful test, we can change
> it to yes.
> 2. The INB/OUTB for data contradicts the datasheet (I know, the
> datasheet might very well be wrong), so a comment would be appreciated
> in those places, e.g. "The datasheet says this register can only be
> accessed with full 32 bit width, but that would make 8 bit writes
> impossible. Due to that, we assume the meaning was garbled in translation."
>
> Hm. It is entirely possible that the datasheet means the register access
> mode is 32 bit. The contents of the data register might only care about
> the lowest 8 bits. That would also mean your driver could work...
>
> Change those two things, and you get an ack from me and I'll commit.
>
> Regards,
> Carl-Daniel
>
>    
Thanks for looking at the previous patch! New patch attached...
This is now for DP83815/DP83816 (same PCI ID) and DP83820 (same flash 
access). I disabled compilation by default, and added the comment in 
both places.
I was hoping that now I have had a chance to solder a boot ROM socket to 
the card I could test the code and make it work. I have done some tests 
and it looks like this is very close, in fact it may well be that this 
code works but my soldering is no good! I've been getting some good data 
read & written, and some random data appearing. Probing seems to work 
fine. Erasing seems to work too, but read and/or write sometimes get 
random data. I believe a dry solder joint would explain this behavior.

Signed-off-by: Andrew Morgan<ziltro@ziltro.com>
Carl-Daniel Hailfinger - 2010-06-07 21:19:56
On 07.06.2010 22:53, Andrew Morgan wrote:
> On 04/06/10 15:13, Carl-Daniel Hailfinger wrote:
>> Oh, it looks really nice.
>> In fact, we could commit this instantly except for two reasons:
>> 1. It's untested, and completely untested drivers should default to no
>> in the Makefile. Once we have the first successful test, we can change
>> it to yes.
>> 2. The INB/OUTB for data contradicts the datasheet (I know, the
>> datasheet might very well be wrong), so a comment would be appreciated
>> in those places, e.g. "The datasheet says this register can only be
>> accessed with full 32 bit width, but that would make 8 bit writes
>> impossible. Due to that, we assume the meaning was garbled in
>> translation."
>>
>> Hm. It is entirely possible that the datasheet means the register access
>> mode is 32 bit. The contents of the data register might only care about
>> the lowest 8 bits. That would also mean your driver could work...
>>
>> Change those two things, and you get an ack from me and I'll commit.
>>
>> Regards,
>> Carl-Daniel
>>
>>    
> Thanks for looking at the previous patch! New patch attached...
> This is now for DP83815/DP83816 (same PCI ID) and DP83820 (same flash
> access). I disabled compilation by default, and added the comment in
> both places.
> I was hoping that now I have had a chance to solder a boot ROM socket
> to the card I could test the code and make it work. I have done some
> tests and it looks like this is very close, in fact it may well be
> that this code works but my soldering is no good! I've been getting
> some good data read & written, and some random data appearing. Probing
> seems to work fine. Erasing seems to work too, but read and/or write
> sometimes get random data. I believe a dry solder joint would explain
> this behavior.
>
> Signed-off-by: Andrew Morgan<ziltro@ziltro.com>
> +	/*
> +	The datasheet says this register can only be
> +	accessed with full 32 bit width, but that would make 8 bit writes
> +	impossible. Due to that, we assume the meaning was garbled in translation.
> +	*/
>   

Can you reformat this comment (and the other identical comment) a bit?
Please make sure to take care of the 80 column limit, start each line
with an asterisk and align the asterisks, similar to this:

	/*
	 * comment
	 * more comment
	 * even more comment
	 */


Ah yes, and while I know I suggested this wording, I'd like to rephrase
it a bit. Sorry.

"The datasheet requires 32 bit accesses to this register, but it seems
that requirement might only apply if the register is memory mapped. Bit
8-31 of this register are apparently don't care, and if this register is
I/O port mapped 8 bit accesses to the lowest byte of the register seem
to work fine. Due to that, we ignore the advice in the data sheet."

Note to you: It would be an interesting experiment to replace the OUTB
with an OUTL, and compare write/read reliability if the high 24 bits are
1 or 0. This is not needed before I commit, but hey... maybe it helps
you and the soldering is OK after all.

Sorry for the nitpicks in the second review round. I think the patch
looks really good, and plan to apply it later tonight.

Regards,
Carl-Daniel

Patch

Index: flash.h
===================================================================
--- flash.h	(revision 1033)
+++ flash.h	(working copy)
@@ -49,6 +49,9 @@ 
 	PROGRAMMER_NICREALTEK,
 	PROGRAMMER_NICREALTEK2,
 #endif	
+#if CONFIG_NICNATSEMI == 1
+	PROGRAMMER_NICNATSEMI,
+#endif	
 #if CONFIG_GFXNVIDIA == 1
 	PROGRAMMER_GFXNVIDIA,
 #endif
@@ -345,7 +348,7 @@ 
 /* print.c */
 char *flashbuses_to_text(enum chipbustype bustype);
 void print_supported(void);
-#if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT >= 1
+#if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT >= 1
 void print_supported_pcidevs(struct pcidev_status *devs);
 #endif
 void print_supported_wiki(void);
@@ -497,6 +500,14 @@ 
 extern struct pcidev_status nics_realteksmc1211[];
 #endif
 
+/* nicnatsemi.c */
+#if CONFIG_NICNATSEMI == 1
+int nicnatsemi_init(void);
+int nicnatsemi_shutdown(void);
+void nicnatsemi_chip_writeb(uint8_t val, chipaddr addr);
+uint8_t nicnatsemi_chip_readb(const chipaddr addr);
+extern struct pcidev_status nics_natsemi[];
+#endif
 
 /* satasii.c */
 #if CONFIG_SATASII == 1
Index: Makefile
===================================================================
--- Makefile	(revision 1033)
+++ Makefile	(working copy)
@@ -113,6 +113,9 @@ 
 # Always enable Realtek NICs for now.
 CONFIG_NICREALTEK ?= yes
 
+# Disable National Semiconductor NICs until support is complete and tested.
+CONFIG_NICNATSEMI ?= no
+
 # Always enable Bus Pirate SPI for now.
 CONFIG_BUSPIRATE_SPI ?= yes
 
@@ -192,6 +195,12 @@ 
 NEED_PCI := yes
 endif
 
+ifeq ($(CONFIG_NICNATSEMI), yes)
+FEATURE_CFLAGS += -D'CONFIG_NICNATSEMI=1'
+PROGRAMMER_OBJS += nicnatsemi.o
+NEED_PCI := yes
+endif
+
 ifeq ($(CONFIG_BUSPIRATE_SPI), yes)
 FEATURE_CFLAGS += -D'CONFIG_BUSPIRATE_SPI=1'
 PROGRAMMER_OBJS += buspirate_spi.o
Index: flashrom.c
===================================================================
--- flashrom.c	(revision 1033)
+++ flashrom.c	(working copy)
@@ -48,7 +48,7 @@ 
  * if more than one of them is selected. If only one is selected, it is clear
  * that the user wants that one to become the default.
  */
-#if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG > 1
+#if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG > 1
 #error Please enable either CONFIG_DUMMY or CONFIG_INTERNAL or disable support for all programmers except one.
 #endif
 enum programmer programmer =
@@ -59,6 +59,9 @@ 
 	PROGRAMMER_NICREALTEK
 	PROGRAMMER_NICREALTEK2
 #endif
+#if CONFIG_NICNATSEMI == 1
+	PROGRAMMER_NICNATSEMI
+#endif
 #if CONFIG_GFXNVIDIA == 1
 	PROGRAMMER_GFXNVIDIA
 #endif
@@ -199,6 +202,24 @@ 
 	},
 #endif
 
+#if CONFIG_NICNATSEMI == 1
+	{
+		.name                   = "nicnatsemi",
+		.init                   = nicnatsemi_init,
+		.shutdown               = nicnatsemi_shutdown,
+		.map_flash_region       = fallback_map,
+		.unmap_flash_region     = fallback_unmap,
+		.chip_readb             = nicnatsemi_chip_readb,
+		.chip_readw             = fallback_chip_readw,
+		.chip_readl             = fallback_chip_readl,
+		.chip_readn             = fallback_chip_readn,
+		.chip_writeb            = nicnatsemi_chip_writeb,
+		.chip_writew            = fallback_chip_writew,
+		.chip_writel            = fallback_chip_writel,
+		.chip_writen            = fallback_chip_writen,
+		.delay                  = internal_delay,
+	},
+#endif
 
 #if CONFIG_GFXNVIDIA == 1
 	{
Index: nicnatsemi.c
===================================================================
--- nicnatsemi.c	(revision 0)
+++ nicnatsemi.c	(revision 0)
@@ -0,0 +1,82 @@ 
+/*
+ * This file is part of the flashrom project.
+ *
+ * Copyright (C) 2010 Andrew Morgan <ziltro@ziltro.com>
+ *
+ * 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
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
+ */
+
+#if defined(__i386__) || defined(__x86_64__)
+
+#include <stdlib.h>
+#include "flash.h"
+
+#define PCI_VENDOR_ID_NATSEMI	0x100b
+
+#define BOOT_ROM_ADDR		0x50
+#define BOOT_ROM_DATA		0x54
+
+struct pcidev_status nics_natsemi[] = {
+	{0x100b, 0x0020, NT, "National Semiconductor", "DP83815/DP83816"},
+	{0x100b, 0x0022, NT, "National Semiconductor", "DP83820"},
+	{},
+};
+
+int nicnatsemi_init(void)
+{
+	get_io_perms();
+
+	io_base_addr = pcidev_init(PCI_VENDOR_ID_NATSEMI, PCI_BASE_ADDRESS_0,
+				   nics_natsemi, programmer_param);
+
+	buses_supported = CHIP_BUSTYPE_PARALLEL;
+	max_rom_decode.parallel = 65536;
+
+	return 0;
+}
+
+int nicnatsemi_shutdown(void)
+{
+	free(programmer_param);
+	pci_cleanup(pacc);
+	release_io_perms();
+	return 0;
+}
+
+void nicnatsemi_chip_writeb(uint8_t val, chipaddr addr)
+{
+	OUTL((uint32_t)addr & 0x0000FFFF, io_base_addr + BOOT_ROM_ADDR);
+	/*
+	The datasheet says this register can only be
+	accessed with full 32 bit width, but that would make 8 bit writes
+	impossible. Due to that, we assume the meaning was garbled in translation.
+	*/
+	OUTB(val, io_base_addr + BOOT_ROM_DATA);
+}
+
+uint8_t nicnatsemi_chip_readb(const chipaddr addr)
+{
+	OUTL(((uint32_t)addr & 0x0000FFFF), io_base_addr + BOOT_ROM_ADDR);
+	/*
+	The datasheet says this register can only be
+	accessed with full 32 bit width, but that would make 8 bit writes
+	impossible. Due to that, we assume the meaning was garbled in translation.
+	*/
+	return INB(io_base_addr + BOOT_ROM_DATA);
+}
+
+#else
+#error PCI port I/O access is not supported on this architecture yet.
+#endif