Patchwork [RFC] National Semiconductor DP83815 NIC patch

login
register
about
Submitter Andrew Morgan
Date 2010-06-07 22:12:47
Message ID <4C0D6EDF.2060607@ziltro.com>
Download mbox | patch
Permalink /patch/1484/
State Accepted
Commit r1039
Headers show

Comments

Andrew Morgan - 2010-06-07 22:12:47
On 07/06/10 22:19, Carl-Daniel Hailfinger wrote:
> 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
>
>    

Here's the next patch, hopefully it is ok. I did try to think of more 
descriptive text for the comment, then got busy testing the code out 
instead.

I have removed one line:

max_rom_decode.parallel = 65536;

As it shouldn't have been there anyway. I don't believe this is a 
requirement as the NIC chip has more address lines than are required for 
64K and there are tracks on the PCB leading to the boot ROM socket for 
those address lines.

I had a quick try replacing OUTB with OUTL and that doesn't seem to 
work, even probe doesn't work then, lots of "probe_jedec_common: id1 
0xff, id2 0xff".

Patch attached.
Carl-Daniel Hailfinger - 2010-06-07 22:47:50
On 08.06.2010 00:12, Andrew Morgan wrote:
> On 07/06/10 22:19, Carl-Daniel Hailfinger wrote:
>> 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
>>
>>    
>
> Here's the next patch, hopefully it is ok. I did try to think of more
> descriptive text for the comment, then got busy testing the code out
> instead.
>
> I have removed one line:
>
> max_rom_decode.parallel = 65536;
>
> As it shouldn't have been there anyway. I don't believe this is a
> requirement as the NIC chip has more address lines than are required
> for 64K and there are tracks on the PCB leading to the boot ROM socket
> for those address lines.

Are you sure? AFAICS your code can't support more than 64 kB because it
truncates the address to 16 bits. Due to that, it should definitely set
max_rom_decode.parallel. You can try changing the address mask, and if
that give you good readbacks, you can still increase the size. However,
in the end every programmer with parallel flash has to set this limit to
make flashing safe for users.


> I had a quick try replacing OUTB with OUTL and that doesn't seem to
> work, even probe doesn't work then, lots of "probe_jedec_common: id1
> 0xff, id2 0xff".

Ah interesting, so it is probably just a MMIO access limitation.


> Patch attached.

Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
and committed in r1039.

I noticed you forgot the Signed-off-by statement in your latest patch,
but since there were only very small changes, I reused the Signed-off-by
statement in the commit log. It would be cool if you could respond to
your latest patch with a signoff, though (no need to resend the patch
itself).

TODO:
Please send a patch which sets max_rom_decode.parallel to a size which
makes sense (i.e. 65536 with the current code) and please add printing
of the programmer PCI devices to print.c and print_wiki.c.
It would be cool if you could add some info to the man page as well.
Just copy and paste from an existing programmer there.

Regards,
Carl-Daniel
Andrew Morgan - 2010-06-08 00:05:21
On 07/06/10 23:12, Andrew Morgan wrote:
> Patch attached.

Forgot to add:

Signed-off-by: Andrew Morgan <ziltro@ziltro.com>

Hoping to get another patch with further changes soon.

Patch

Index: flash.h
===================================================================
--- flash.h	(revision 1038)
+++ 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
@@ -339,7 +342,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);
@@ -491,6 +494,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 1038)
+++ 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 1038)
+++ 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,87 @@ 
+/*
+ * 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;
+
+	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 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.
+	 */
+	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 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.
+	 */
+	return INB(io_base_addr + BOOT_ROM_DATA);
+}
+
+#else
+#error PCI port I/O access is not supported on this architecture yet.
+#endif