[PATCH] firmware: convert acenic driver to request_firmware()

[PATCH] firmware: convert acenic driver to request_firmware()

[David Woodhouse]

Again with help from Jaswinder Singh.

Omitting the large part of the patch which actually moves the firmware
around, since you'll need to pull from the git tree to get the preceding
patches if you want to test it anyway.

We store the firmware in little-endian form now, and thus use
__raw_writel() to write it to the device, to avoid byteswapping by
writel(). I've revamped that loop in ace_copy() a little bit so it could
probably do with being tested.

I've dropped the information about SBSS and BSS sections of the firmware
-- we were clearing the whole of the device's memory in advance anyway,
so clearing the BSS sections for a _second_ time seems pointless. And
since the text,rodata,data sections were (almost) contiguous, we now
just load those as a single blob rather than keeping them separate.

Although it probably isn't necessary, we do preserve the ability to
change the load and start addresses from 0x4000, by putting them into a
header at the beginning of the firmware blob, along with the version
number.


diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index d040b7b..c555606 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -1921,6 +1921,7 @@ if NETDEV_1000
 config ACENIC
 	tristate "Alteon AceNIC/3Com 3C985/NetGear GA620 Gigabit support"
 	depends on PCI
+	select FW_LOADER
 	---help---
 	  Say Y here if you have an Alteon AceNIC, 3Com 3C985(B), NetGear
 	  GA620, SGI Gigabit or Farallon PN9000-SX PCI Gigabit Ethernet
@@ -1945,6 +1946,23 @@ config ACENIC_OMIT_TIGON_I
 
 	  The safe and default value for this is N.
 
+config ACENIC_TG1_FIRMWARE
+	bool "Include firmware for old Tigon I based AceNICs"
+	depends on ACENIC && !ACENIC_OMIT_TIGON_I
+	---help---
+	  This includes firmware for the original Alteon AceNIC and 3Com 3C985
+	  (non B version) in the kernel image.
+	  Say 'N' and let it get loaded from userspace on demand 
+
+config ACENIC_TG2_FIRMWARE
+	bool "Include firmware for Alteon AceNIC/3Com 3C985/NetGear GA620 Gigabit"
+	depends on ACENIC
+	---help---
+	  This includes firmware for later Alteon AceNIC, 3Com 3C985B,
+	  NetGear GA620, SGI Gigabit or Farallon PN9000-SX PCI Gigabit
+	  Ethernet adapter.
+	  Say 'N' and let it get loaded from userspace on demand 
+
 config DL2K
 	tristate "DL2000/TC902x-based Gigabit Ethernet support"
 	depends on PCI
diff --git a/drivers/net/acenic.c b/drivers/net/acenic.c
index 6c19265..1e42734 100644
--- a/drivers/net/acenic.c
+++ b/drivers/net/acenic.c
@@ -67,6 +67,7 @@
 #include <linux/mm.h>
 #include <linux/highmem.h>
 #include <linux/sockios.h>
+#include <linux/firmware.h>
 
 #if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 #include <linux/if_vlan.h>
@@ -187,8 +188,6 @@ MODULE_DEVICE_TABLE(pci, acenic_pci_tbl);
 #define MAX_RODATA_LEN	8*1024
 #define MAX_DATA_LEN	2*1024
 
-#include "acenic_firmware.h"
-
 #ifndef tigon2FwReleaseLocal
 #define tigon2FwReleaseLocal 0
 #endif
@@ -418,6 +417,8 @@ static int dis_pci_mem_inval[ACE_MAX_MOD_PARMS] = {1, 1, 1, 1, 1, 1, 1, 1};
 MODULE_AUTHOR("Jes Sorensen <jes@trained-monkey.org>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("AceNIC/3C985/GA620 Gigabit Ethernet driver");
+MODULE_FIRMWARE("acenic_tg1.bin");
+MODULE_FIRMWARE("acenic_tg2.bin");
 
 module_param_array_named(link, link_state, int, NULL, 0);
 module_param_array(trace, int, NULL, 0);
@@ -939,8 +940,8 @@ static int __devinit ace_init(struct net_device *dev)
 	case 4:
 	case 5:
 		printk(KERN_INFO "  Tigon I  (Rev. %i), Firmware: %i.%i.%i, ",
-		       tig_ver, tigonFwReleaseMajor, tigonFwReleaseMinor,
-		       tigonFwReleaseFix);
+		       tig_ver, ap->firmware_major, ap->firmware_minor,
+		       ap->firmware_fix);
 		writel(0, &regs->LocalCtrl);
 		ap->version = 1;
 		ap->tx_ring_entries = TIGON_I_TX_RING_ENTRIES;
@@ -948,8 +949,8 @@ static int __devinit ace_init(struct net_device *dev)
 #endif
 	case 6:
 		printk(KERN_INFO "  Tigon II (Rev. %i), Firmware: %i.%i.%i, ",
-		       tig_ver, tigon2FwReleaseMajor, tigon2FwReleaseMinor,
-		       tigon2FwReleaseFix);
+		       tig_ver, ap->firmware_major, ap->firmware_minor,
+		       ap->firmware_fix);
 		writel(readl(&regs->CpuBCtrl) | CPU_HALT, &regs->CpuBCtrl);
 		readl(&regs->CpuBCtrl);		/* PCI write posting */
 		/*
@@ -1201,7 +1202,9 @@ static int __devinit ace_init(struct net_device *dev)
 	memset(ap->info, 0, sizeof(struct ace_info));
 	memset(ap->skb, 0, sizeof(struct ace_skb));
 
-	ace_load_firmware(dev);
+	if (ace_load_firmware(dev))
+		goto init_error;
+
 	ap->fw_running = 0;
 
 	tmp_ptr = ap->info_dma;
@@ -1437,10 +1440,7 @@ static int __devinit ace_init(struct net_device *dev)
 	if (ap->version >= 2)
 		writel(tmp, &regs->TuneFastLink);
 
-	if (ACE_IS_TIGON_I(ap))
-		writel(tigonFwStartAddr, &regs->Pc);
-	if (ap->version == 2)
-		writel(tigon2FwStartAddr, &regs->Pc);
+	writel(ap->firmware_start, &regs->Pc);
 
 	writel(0, &regs->Mb0Lo);
 
@@ -2763,8 +2763,8 @@ static void ace_get_drvinfo(struct net_device *dev,
 
 	strlcpy(info->driver, "acenic", sizeof(info->driver));
 	snprintf(info->version, sizeof(info->version), "%i.%i.%i",
-		tigonFwReleaseMajor, tigonFwReleaseMinor,
-		tigonFwReleaseFix);
+		 ap->firmware_major, ap->firmware_minor,
+		 ap->firmware_fix);
 
 	if (ap->pdev)
 		strlcpy(info->bus_info, pci_name(ap->pdev),
@@ -2871,11 +2871,10 @@ static struct net_device_stats *ace_get_stats(struct net_device *dev)
 }
 

-static void __devinit ace_copy(struct ace_regs __iomem *regs, void *src,
-			    u32 dest, int size)
+static void __devinit ace_copy(struct ace_regs __iomem *regs, const __le32 *src,
+			       u32 dest, int size)
 {
 	void __iomem *tdest;
-	u32 *wsrc;
 	short tsize, i;
 
 	if (size <= 0)
@@ -2887,20 +2886,15 @@ static void __devinit ace_copy(struct ace_regs __iomem *regs, void *src,
 		tdest = (void __iomem *) &regs->Window +
 			(dest & (ACE_WINDOW_SIZE - 1));
 		writel(dest & ~(ACE_WINDOW_SIZE - 1), &regs->WinBase);
-		/*
-		 * This requires byte swapping on big endian, however
-		 * writel does that for us
-		 */
-		wsrc = src;
 		for (i = 0; i < (tsize / 4); i++) {
-			writel(wsrc[i], tdest + i*4);
+			/* Firmware is stored as little-endian */
+			__raw_writel(*src, tdest);
+			src++;
+			tdest += 4;
+			dest += 4;
+			size -= 4;
 		}
-		dest += tsize;
-		src += tsize;
-		size -= tsize;
 	}
-
-	return;
 }
 

@@ -2939,8 +2933,13 @@ static void __devinit ace_clear(struct ace_regs __iomem *regs, u32 dest, int siz
  */
 static int __devinit ace_load_firmware(struct net_device *dev)
 {
+	const struct firmware *fw;
+	const char *fw_name = "acenic_tg2.bin";
 	struct ace_private *ap = netdev_priv(dev);
 	struct ace_regs __iomem *regs = ap->regs;
+	const __le32 *fw_data;
+	u32 load_addr;
+	int ret;
 
 	if (!(readl(&regs->CpuCtrl) & CPU_HALTED)) {
 		printk(KERN_ERR "%s: trying to download firmware while the "
@@ -2948,28 +2947,52 @@ static int __devinit ace_load_firmware(struct net_device *dev)
 		return -EFAULT;
 	}
 
+	if (ACE_IS_TIGON_I(ap))
+		fw_name = "acenic_tg1.bin";
+
+	ret = request_firmware(&fw, fw_name, &ap->pdev->dev);
+	if (ret) {
+		printk(KERN_ERR "%s: Failed to load firmware \"%s\"\n",
+		       ap->name, fw_name);
+		return ret;
+	}
+
+	fw_data = (void *)fw->data;
+
+	/* Firmware blob starts with version numbers, followed by
+	   load and start address. Remainder is the blob to be loaded
+	   contiguously from load address. We don't bother to represent
+	   the BSS/SBSS sections any more, since we were clearing the
+	   whole thing anyway. */
+	ap->firmware_major = fw->data[0];
+	ap->firmware_minor = fw->data[1];
+	ap->firmware_fix = fw->data[2];
+
+	ap->firmware_start = le32_to_cpu(fw_data[1]);
+	if (ap->firmware_start < 0x4000 || ap->firmware_start >= 0x80000) {
+		printk(KERN_ERR "%s: bogus load address %08x in \"%s\"\n",
+		       ap->name, ap->firmware_start, fw_name);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	load_addr = le32_to_cpu(fw_data[2]);
+	if (load_addr < 0x4000 || load_addr >= 0x80000) {
+		printk(KERN_ERR "%s: bogus load address %08x in \"%s\"\n",
+		       ap->name, load_addr, fw_name);
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/*
-	 * Do not try to clear more than 512KB or we end up seeing
-	 * funny things on NICs with only 512KB SRAM
+	 * Do not try to clear more than 512KiB or we end up seeing
+	 * funny things on NICs with only 512KiB SRAM
 	 */
 	ace_clear(regs, 0x2000, 0x80000-0x2000);
-	if (ACE_IS_TIGON_I(ap)) {
-		ace_copy(regs, tigonFwText, tigonFwTextAddr, tigonFwTextLen);
-		ace_copy(regs, tigonFwData, tigonFwDataAddr, tigonFwDataLen);
-		ace_copy(regs, tigonFwRodata, tigonFwRodataAddr,
-			 tigonFwRodataLen);
-		ace_clear(regs, tigonFwBssAddr, tigonFwBssLen);
-		ace_clear(regs, tigonFwSbssAddr, tigonFwSbssLen);
-	}else if (ap->version == 2) {
-		ace_clear(regs, tigon2FwBssAddr, tigon2FwBssLen);
-		ace_clear(regs, tigon2FwSbssAddr, tigon2FwSbssLen);
-		ace_copy(regs, tigon2FwText, tigon2FwTextAddr,tigon2FwTextLen);
-		ace_copy(regs, tigon2FwRodata, tigon2FwRodataAddr,
-			 tigon2FwRodataLen);
-		ace_copy(regs, tigon2FwData, tigon2FwDataAddr,tigon2FwDataLen);
-	}
-
-	return 0;
+	ace_copy(regs, &fw_data[3], load_addr, fw->size-12);
+ out:
+	release_firmware(fw);
+	return ret;
 }
 

diff --git a/drivers/net/acenic.h b/drivers/net/acenic.h
index 60ed183..cf7e80e 100644
--- a/drivers/net/acenic.h
+++ b/drivers/net/acenic.h
@@ -695,6 +695,10 @@ struct ace_private
 #endif
 	struct net_device_stats stats;
 	int			pci_using_dac;
+	u8			firmware_major;
+	u8			firmware_minor;
+	u8			firmware_fix;
+	u32			firmware_start;
 };
 


-- 
dwmw2

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[Jes Sorensen]

>>>>> "David" == David Woodhouse <dwmw2@infradead.org> writes:

David> Again with help from Jaswinder Singh.  Omitting the large part of
David> the patch which actually moves the firmware around, since you'll
David> need to pull from the git tree to get the preceding patches if
David> you want to test it anyway.

David> We store the firmware in little-endian form now, and thus use
David> __raw_writel() to write it to the device, to avoid byteswapping
David> by writel(). I've revamped that loop in ace_copy() a little bit
David> so it could probably do with being tested.

Mmmmm,

I am not particular bothered as to whether or not to put the firmware to
userspace or not, however has this patch been tested on big endian
systems? Given that the card is big endian, how is this going to affect
big endian systems?

Honestly, I think it's a bad idea to start messing with the byte order
of the data for something that happens once at boot time.

Cheers,
Jes

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[David Woodhouse]

On Mon, 2008-06-16 at 12:25 -0400, Jes Sorensen wrote:
> Mmmmm,
> 
> I am not particular bothered as to whether or not to put the firmware to
> userspace or not, however has this patch been tested on big endian
> systems? Given that the card is big endian, how is this going to affect
> big endian systems?

We don't have hardware, so haven't been able to test it on either
little-endian or big-endian machines, but we took care to ensure that it
should work on both.

> Honestly, I think it's a bad idea to start messing with the byte order
> of the data for something that happens once at boot time.

Currently, we have the firmware in the host's native endianness -- it's
in the driver in an array of uint32_t. And we just call writel() with
each word of it in turn.

If we provide the firmware via request_firmware(), we don't want to have
separate versions of the firmware files for big-endian and little-endian
hosts. It makes much more sense just to have _one_ version of each
binary file, and either call writel(le32_to_cpu()) for each word, or
writel(be32_to_cpu()).

We chose the former, because it can be simplified to __raw_writel().

Since the CPU on the device is big-endian, I would be amenable to an
argument that we should store the firmware in its native big-endian
form, and load it with writel(be32_to_cpu()) instead.

It doesn't matter much either way, really. 

-- 
dwmw2

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[Jeff Garzik]

David Woodhouse wrote:
> Again with help from Jaswinder Singh.
> 
> Omitting the large part of the patch which actually moves the firmware
> around, since you'll need to pull from the git tree to get the preceding
> patches if you want to test it anyway.
> 
> We store the firmware in little-endian form now, and thus use
> __raw_writel() to write it to the device, to avoid byteswapping by
> writel(). I've revamped that loop in ace_copy() a little bit so it could
> probably do with being tested.
> 
> I've dropped the information about SBSS and BSS sections of the firmware
> -- we were clearing the whole of the device's memory in advance anyway,
> so clearing the BSS sections for a _second_ time seems pointless. And
> since the text,rodata,data sections were (almost) contiguous, we now
> just load those as a single blob rather than keeping them separate.
> 
> Although it probably isn't necessary, we do preserve the ability to
> change the load and start addresses from 0x4000, by putting them into a
> header at the beginning of the firmware blob, along with the version
> number.

Mostly ok...

1) firmware separation should be a separate patch from 
request_firmware() support addition

2) [minor] firmware Kconfig entries should default to 'Y' during 
transition, though it's ok to remove those after transition is over

3) there are enough changes to warrant a requirement of a "driver still 
works" test before going in, IMO

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[David Woodhouse]

On Mon, 2008-06-16 at 16:34 -0400, Jeff Garzik wrote:
> Mostly ok...
> 
> 1) firmware separation should be a separate patch from 
> request_firmware() support addition

It's kind of hard to do that. Taking this patch as an example -- we've
had to make minor changes to the firmware loading w.r.t. endianness,
etc.

If we really wanted to do it in two steps of '1. add request_firmware()'
followed by '2. remove old static firmware', then the whole process
would just be a lot more cumbersome than we need.

In this case, the first patch would probably add a completely new
version of ace_load_firmware() and ace_copy(), while the second patch
would remove the old versions of each function. To make sense of and
review that lot, you'd actually want to run 'combinediff' on them anyway
so you end up with what I've posted here. Only then is it really obvious
what's happening.

I do understand the desire to keep patches simple and do things in
stages -- but in this case (and the other request_firmware() patches
I've done), it seems like separating those two steps would actually be
counter-productive.

AFAICT it doesn't make it any easier to test the new code path, either.
It's still only really testable after you've applied both patches,
surely?

> 2) [minor] firmware Kconfig entries should default to 'Y' during 
> transition, though it's ok to remove those after transition is over

I don't think it's good to have a 'default y' on options for which the
help text says "Say 'N' and let udev load it as $DEITY intended".

As a compromise, perhaps we could do a 'CONFIG_INCLUDE_ALL_FIRMWARE' and
make them all default to whatever that option is set to? I'm really
unconvinced of the need for that, though -- running 'make
firmware_install' is all that's necessary, and is the preferred option. 

> 3) there are enough changes to warrant a requirement of a "driver still 
> works" test before going in, IMO

Most definitely. As I said, I've been very careful to ensure that the
converted firmware is correct, in conjunction with the driver changes
(like the endianness considerations in this case). But that is no
substitute for testing.

-- 
dwmw2

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[Jeff Garzik]

David Woodhouse wrote:
> On Mon, 2008-06-16 at 16:34 -0400, Jeff Garzik wrote:
>> Mostly ok...
>>
>> 1) firmware separation should be a separate patch from 
>> request_firmware() support addition
> 
> It's kind of hard to do that. Taking this patch as an example -- we've
> had to make minor changes to the firmware loading w.r.t. endianness,
> etc.
> 
> If we really wanted to do it in two steps of '1. add request_firmware()'
> followed by '2. remove old static firmware', then the whole process
> would just be a lot more cumbersome than we need.
> 
> In this case, the first patch would probably add a completely new
> version of ace_load_firmware() and ace_copy(), while the second patch
> would remove the old versions of each function. To make sense of and
> review that lot, you'd actually want to run 'combinediff' on them anyway
> so you end up with what I've posted here. Only then is it really obvious
> what's happening.
> 
> I do understand the desire to keep patches simple and do things in
> stages -- but in this case (and the other request_firmware() patches
> I've done), it seems like separating those two steps would actually be
> counter-productive.
> 
> AFAICT it doesn't make it any easier to test the new code path, either.
> It's still only really testable after you've applied both patches,
> surely?

An intermediate request_firmware() step is something that can go into an 
upstream kernel _immediately_, without even needing firmware/ install 
infrastructure.

That, in turn, enables the ability to test a driver with externally 
loaded firmwares, while still being able to fall back to a known 
working, guaranteed present built-in firmware.

IOW, the first step for each driver should be:

+	request_firmware()
+	if (success)
+		use externally-loaded firmware
+	else
		use built-in firmware

and that's it.  Nothing else [in the first patch].

Thus you minimize change -- both source code upheaval as well as 
operational behavior upheaval -- while adding the capability for people 
to immediately begin testing external firmware loading... in parallel 
with further firmware/ development you yourself are doing.

While I'm on the subject, I think the most user-friendly, 
developer-friendly, and time-friendly ordering is

0) prepare kernel-firmware package with extracted firmwares from kernel 
tree, and have it available in rawhide, cooker, opensuse-devel, etc.

1) what I said above: request_firmware() for every driver

At this point, users have udev-loadable firmware in hand and a driver 
capable of loading said firmware, and can immediately start operating in 
the desired environment.

2) start peeling built-in firmwares out of the C source



What I think you don't get is that moving the firmware out of the driver 
is the _easy_ part that comes _last_.

The core problem with request_firmware() loading external firmwares is 
usability:  the user damn well needs to have their firmware available, 
otherwise their hardware doesn't work.

That means the pressure is on you to have all the elements in place with 
all the major distros _before_ you do the easy part.

Otherwise the user experience is going to suck, the last thing we all 
want...




> 
>> 2) [minor] firmware Kconfig entries should default to 'Y' during 
>> transition, though it's ok to remove those after transition is over
> 
> I don't think it's good to have a 'default y' on options for which the
> help text says "Say 'N' and let udev load it as $DEITY intended".
> 
> As a compromise, perhaps we could do a 'CONFIG_INCLUDE_ALL_FIRMWARE' and
> make them all default to whatever that option is set to? I'm really
> unconvinced of the need for that, though -- running 'make
> firmware_install' is all that's necessary, and is the preferred option. 

I'm mainly thinking of user and developer experience here.

I just want to make it damn near impossible to accidentally create a 
non-working configuration, after your patches are applied.

This isn't like other driver Kconfig options, where the driver will 
continue to work even if you auto-set everything to 'N'.  If the user 
doesn't have multiple pieces of the puzzle _already in place_, it is all 
too easy to create a non-working driver.

The truth of the matter is, the user really does want to set that 
Kconfig option, during the initial time period after your patches are 
applied.

Once things settled down and infrastructure is in place, the 'default y' 
could go away.


>> 3) there are enough changes to warrant a requirement of a "driver still 
>> works" test before going in, IMO
> 
> Most definitely. As I said, I've been very careful to ensure that the
> converted firmware is correct, in conjunction with the driver changes
> (like the endianness considerations in this case). But that is no
> substitute for testing.

Cool :)

	Jeff

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[David Woodhouse]

On Mon, 2008-06-16 at 18:11 -0400, Jeff Garzik wrote:
> An intermediate request_firmware() step is something that can go into an 
> upstream kernel _immediately_, without even needing firmware/ install 
> infrastructure.

Not before 2.6.26, it can't -- it's much too late for that. And I'm
planning to ask Linus to take the 'make firmware_install' infrastructure
as soon as the merge window opens for 2.6.27. So there's certainly no
need to separate them on _those_ grounds.

The parts which live in the Fedora kernel specfile to spit out the
kernel-firmware subpackage are also ready to get merged as soon as the
infrastructure is upstream.

> That, in turn, enables the ability to test a driver with externally 
> loaded firmwares, while still being able to fall back to a known 
> working, guaranteed present built-in firmware.
> 
> IOW, the first step for each driver should be:
> 
> +	request_firmware()
> +	if (success)
> +		use externally-loaded firmware
> +	else
> 		use built-in firmware
> 
> and that's it.  Nothing else [in the first patch].

If it were that simple, I would agree with you. In practice, it hasn't
turned out to be that simple. I really do think it would be
counter-productive for both review and testing purposes. 

For review because, as I said, we'd end up providing new copies of
certain functions in the first patch (or conditional noise in them), and
then ripping out the old code path in the second -- the patch which it
would make sense to _read_ and review would be the combined one.

For testing because there's quite a large chance that if something goes
wrong with the request_firmware() path, it'd fall back to the other path
and people would say "OK, that works" when it doesn't. And we both know
that kind of thing will still happen even if we have a bloody great
warning printk in the fallback case.

I suppose what we _could_ do is make your 'use built-in firmware' path
use that firmware in the _same_ form as we have it provided by
request_firmware(). That way, we really are testing the same code path.
Would that meet your approval?

If you look closely, you'll see that that is exactly what I've done
already. But without the complexity in the driver -- the driver just
calls request_firmware(), and the firmware is either provided from
within the kernel image, or from userspace.

> While I'm on the subject, I think the most user-friendly, 
> developer-friendly, and time-friendly ordering is
> 
> 0) prepare kernel-firmware package with extracted firmwares from kernel 
> tree, and have it available in rawhide, cooker, opensuse-devel, etc.
> 
> 1) what I said above: request_firmware() for every driver
> 
> At this point, users have udev-loadable firmware in hand and a driver 
> capable of loading said firmware, and can immediately start operating in 
> the desired environment.
> 
> 2) start peeling built-in firmwares out of the C source

Why would you insist that we support request_firmware() in _every_
driver, before we _depend_ on it in any? That doesn't seem to make a lot
of sense. Especially since we already _have_ a whole bunch of drivers
which _depend_ on request_firmware().

Would you suggest that we shouldn't have drivers like myri10ge depending
on request_firmware(), just because the tg3 driver still doesn't have
that option?

Each driver stands alone, and can just be converted in one step.

> What I think you don't get is that moving the firmware out of the driver 
> is the _easy_ part that comes _last_.
> 
> The core problem with request_firmware() loading external firmwares is 
> usability:  the user damn well needs to have their firmware available, 
> otherwise their hardware doesn't work.
> 
> That means the pressure is on you to have all the elements in place with 
> all the major distros _before_ you do the easy part.

Firstly, distributions already _have_ all the elements in place for
loading firmware, and even for including it in the initrd as necessary.
It's not as if we're making them cope with something new and exciting.
So there should be no particular concern on that front. All they need to
do is put the firmware into /lib/firmware.

Secondly, we are still allowing people to build firmware into the kernel
-- we're not _forcing_ them to use udev or initrds at all. So it really
isn't hard for them to cope with the transition. at all.

> Otherwise the user experience is going to suck, the last thing we all 
> want...

I've given a lot of thought to how the transition affects userspace,
have implemented the Fedora packaging side of it and spoken to some of
the people who would take the brunt of the users' displeasure if it goes
wrong. And I didn't reach the same conclusions you did.

> The truth of the matter is, the user really does want to set that 
> Kconfig option, during the initial time period after your patches are 
> applied.

They shouldn't need to do that. "make firmware_install" works right now,
and the rest of the infrastructure is _already_ in place and actively
used by a lot of other drivers.

> Once things settled down and infrastructure is in place, the 'default y' 
> could go away.

For distributions it doesn't matter; it'll get dealt with properly.
Fedora is already ready; other distributions shouldn't find it hard to
do the same when they update their kernel to 2.6.27.

For individuals, they either need to run 'make firmware_install' or
build the firmware in. Preferably the former, which is why we've put
"Say 'N'" in the help texts for these options.

Perhaps we could put a big reminder in the final output for the
modules_install or vmlinux targets, saying:
	You need to run 'make firmware_install'

I don't think it's a particularly valid concern, though.

> >> 3) there are enough changes to warrant a requirement of a "driver still 
> >> works" test before going in, IMO
> > 
> > Most definitely. As I said, I've been very careful to ensure that the
> > converted firmware is correct, in conjunction with the driver changes
> > (like the endianness considerations in this case). But that is no
> > substitute for testing.
> 
> Cool :)

I'm sorry, perhaps I misspoke. I don't actually plan to hold off on
merging patches until I've got positive testing results for each and
every one, although obviously I'd like to. It's just not practical,
though. For some things, like Ambassador ATM cards, I'm not even sure
anyone still _has_ the hardware. And the changes are, for the most part,
Obviously Correct™ janitorial-style stuff.

But I am actively trying to get people to test these patches on real
hardware, and would be very _happy_ if they were tested before they go
in. That's one of the reasons they're in linux-next already.

And because they _don't_ automatically fall back to the old code paths,
I know that a success report in linux-next is a success report for the
changes we've made.

-- 
dwmw2

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[Jes Sorensen]

>>>>> "David" == David Woodhouse <dwmw2@infradead.org> writes:

David> On Mon, 2008-06-16 at 12:25 -0400, Jes Sorensen wrote:
David> We don't have hardware, so haven't been able to test it on either
David> little-endian or big-endian machines, but we took care to ensure
David> that it should work on both.

Hmmm, I have the cards, but no BE boxes to plug them into - stop by and
I'll give you one :-)

David> Since the CPU on the device is big-endian, I would be amenable to
David> an argument that we should store the firmware in its native
David> big-endian form, and load it with writel(be32_to_cpu()) instead.

David> It doesn't matter much either way, really.

I'm not really biased, the card isn't in production anymore and there's
fewer and fewer of them out there. As long as it works it's fine with
me, but I guess I have a slight preference for staying as close to the
original format as possible.

Cheers,
Jes

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[David Woodhouse]

On Tue, 2008-06-17 at 12:50 -0400, Jes Sorensen wrote:
> >>>>> "David" == David Woodhouse <dwmw2@infradead.org> writes:
> 
> David> On Mon, 2008-06-16 at 12:25 -0400, Jes Sorensen wrote:
> David> We don't have hardware, so haven't been able to test it on either
> David> little-endian or big-endian machines, but we took care to ensure
> David> that it should work on both.
> 
> Hmmm, I have the cards, but no BE boxes to plug them into - stop by and
> I'll give you one :-)

Should have suggested that last week when I was unemployed :)

> David> Since the CPU on the device is big-endian, I would be amenable to
> David> an argument that we should store the firmware in its native
> David> big-endian form, and load it with writel(be32_to_cpu()) instead.
> 
> David> It doesn't matter much either way, really.
> 
> I'm not really biased, the card isn't in production anymore and there's
> fewer and fewer of them out there. As long as it works it's fine with
> me, but I guess I have a slight preference for staying as close to the
> original format as possible.

OK, we'll switch to BE, to be closer to the original form of the
firmware.

-- 
dwmw2

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[David Woodhouse]

On Tue, 2008-06-17 at 11:40 +0100, David Woodhouse wrote:
> 
> > IOW, the first step for each driver should be:
> > 
> > +     request_firmware()
> > +     if (success)
> > +             use externally-loaded firmware
> > +     else
> >               use built-in firmware
> > 
> > and that's it.  Nothing else [in the first patch].
> 
> If it were that simple, I would agree with you. In practice, it hasn't
> turned out to be that simple. I really do think it would be
> counter-productive for both review and testing purposes. 
> 
> For review because, as I said, we'd end up providing new copies of
> certain functions in the first patch (or conditional noise in them), and
> then ripping out the old code path in the second -- the patch which it
> would make sense to _read_ and review would be the combined one.
> 
> For testing because there's quite a large chance that if something goes
> wrong with the request_firmware() path, it'd fall back to the other path
> and people would say "OK, that works" when it doesn't. And we both know
> that kind of thing will still happen even if we have a bloody great
> warning printk in the fallback case.
> 
> I suppose what we _could_ do is make your 'use built-in firmware' path
> use that firmware in the _same_ form as we have it provided by
> request_firmware(). That way, we really are testing the same code path.
> Would that meet your approval?
> 
> If you look closely, you'll see that that is exactly what I've done
> already. But without the complexity in the driver -- the driver just
> calls request_firmware(), and the firmware is either provided from
> within the kernel image, or from userspace.

I believe you didn't respond to this, but just spouted the same
complaint at another patch which was posted for review, without even
seeming to have _looked_ at the part of that patch which really needed
reviewing.

This has needed doing for a long time, and I've finally got off my
posterior and started working on it. If you wanted it done precisely
your way, you could always have done it yourself. As it is, we have a
minor disagreement about some of the details, but it still needs doing.
And since it still seems to be me doing it and not you, I'm still doing
it the way I believe is best.

We have a bunch of other drivers to fix up to use request_firmware(),
other than the network drivers. If you like, we can move on to those and
let you do drivers/net your way, splitting each patch into two stages.
I think it's pointless and counter-productive to do it that way, but if
it's your own time you're wasting on generating that extra churn, then I
suppose it's not _so_ counter-productive that I would argue that we
should reject your patches. People can always run 'combinediff' on your
two patches to get a single, reviewable patch which would match the one
we would have sent, after all. And the harm to the testing process is
your concern, once you're providing the patches :)

OK?

-- 
dwmw2

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[Jeff Garzik]

David Woodhouse wrote:
> I believe you didn't respond to this, but just spouted the same
> complaint at another patch which was posted for review, without even
> seeming to have _looked_ at the part of that patch which really needed
> reviewing.
> 
> This has needed doing for a long time, and I've finally got off my
> posterior and started working on it. If you wanted it done precisely
> your way, you could always have done it yourself. As it is, we have a
> minor disagreement about some of the details, but it still needs doing.
> And since it still seems to be me doing it and not you, I'm still doing
> it the way I believe is best.


So, if you get an unhappy review, the reviewer must do your work for 
you?  That doesn't work for university students, and that doesn't fly here.

We are in the part of the Linux kernel coding process called 'taking 
feedback'.

The onus is on YOU to make your work friendly to both users and developers.

You can do whatever you please, but if its not friendly to Linux users 
(our customers) or Linux developers, it's mostly dead in the water. 
Those are my concerns, which you repeatedly fail to address.

For Pete's sake, you haven't even been able to admit the obvious:  these 
patches have a major, major chance of producing a non-working driver for 
users.

Your patch and your goal mean nothing if Linux users wind up having to 
jump through a dozen new hoops, just to ensure that a driver working 
yesterday continues to work today.  That's just plain bad engineering, 
no matter how noble the goal.

	Jeff

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[David Woodhouse]

On Tue, 2008-06-17 at 12:50 -0400, Jes Sorensen wrote:
> I'm not really biased, the card isn't in production anymore and
> there's fewer and fewer of them out there. As long as it works it's
> fine with me, but I guess I have a slight preference for staying as
> close to the original format as possible.

OK, here's the relevant part again, adjusted to handle the firmware in
its original big-endian form. As before, this is Obviously Correct™ on
both big-endian and little-endian hosts, but hasn't been tested.

diff --git a/drivers/net/acenic.c b/drivers/net/acenic.c
index 6c19265..167aa59 100644
--- a/drivers/net/acenic.c
+++ b/drivers/net/acenic.c
@@ -67,6 +67,7 @@
 #include <linux/mm.h>
 #include <linux/highmem.h>
 #include <linux/sockios.h>
+#include <linux/firmware.h>
 
 #if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 #include <linux/if_vlan.h>
@@ -187,8 +188,6 @@ MODULE_DEVICE_TABLE(pci, acenic_pci_tbl);
 #define MAX_RODATA_LEN	8*1024
 #define MAX_DATA_LEN	2*1024
 
-#include "acenic_firmware.h"
-
 #ifndef tigon2FwReleaseLocal
 #define tigon2FwReleaseLocal 0
 #endif
@@ -418,6 +417,10 @@ static int dis_pci_mem_inval[ACE_MAX_MOD_PARMS] = {1, 1, 1, 1, 1, 1, 1, 1};
 MODULE_AUTHOR("Jes Sorensen <jes@trained-monkey.org>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("AceNIC/3C985/GA620 Gigabit Ethernet driver");
+#ifndef CONFIG_ACENIC_OMIT_TIGON_I
+MODULE_FIRMWARE("acenic/tg1.bin");
+#endif
+MODULE_FIRMWARE("acenic/tg2.bin");
 
 module_param_array_named(link, link_state, int, NULL, 0);
 module_param_array(trace, int, NULL, 0);
@@ -939,8 +942,8 @@ static int __devinit ace_init(struct net_device *dev)
 	case 4:
 	case 5:
 		printk(KERN_INFO "  Tigon I  (Rev. %i), Firmware: %i.%i.%i, ",
-		       tig_ver, tigonFwReleaseMajor, tigonFwReleaseMinor,
-		       tigonFwReleaseFix);
+		       tig_ver, ap->firmware_major, ap->firmware_minor,
+		       ap->firmware_fix);
 		writel(0, &regs->LocalCtrl);
 		ap->version = 1;
 		ap->tx_ring_entries = TIGON_I_TX_RING_ENTRIES;
@@ -948,8 +951,8 @@ static int __devinit ace_init(struct net_device *dev)
 #endif
 	case 6:
 		printk(KERN_INFO "  Tigon II (Rev. %i), Firmware: %i.%i.%i, ",
-		       tig_ver, tigon2FwReleaseMajor, tigon2FwReleaseMinor,
-		       tigon2FwReleaseFix);
+		       tig_ver, ap->firmware_major, ap->firmware_minor,
+		       ap->firmware_fix);
 		writel(readl(&regs->CpuBCtrl) | CPU_HALT, &regs->CpuBCtrl);
 		readl(&regs->CpuBCtrl);		/* PCI write posting */
 		/*
@@ -1201,7 +1204,9 @@ static int __devinit ace_init(struct net_device *dev)
 	memset(ap->info, 0, sizeof(struct ace_info));
 	memset(ap->skb, 0, sizeof(struct ace_skb));
 
-	ace_load_firmware(dev);
+	if (ace_load_firmware(dev))
+		goto init_error;
+
 	ap->fw_running = 0;
 
 	tmp_ptr = ap->info_dma;
@@ -1437,10 +1442,7 @@ static int __devinit ace_init(struct net_device *dev)
 	if (ap->version >= 2)
 		writel(tmp, &regs->TuneFastLink);
 
-	if (ACE_IS_TIGON_I(ap))
-		writel(tigonFwStartAddr, &regs->Pc);
-	if (ap->version == 2)
-		writel(tigon2FwStartAddr, &regs->Pc);
+	writel(ap->firmware_start, &regs->Pc);
 
 	writel(0, &regs->Mb0Lo);
 
@@ -2763,8 +2765,8 @@ static void ace_get_drvinfo(struct net_device *dev,
 
 	strlcpy(info->driver, "acenic", sizeof(info->driver));
 	snprintf(info->version, sizeof(info->version), "%i.%i.%i",
-		tigonFwReleaseMajor, tigonFwReleaseMinor,
-		tigonFwReleaseFix);
+		 ap->firmware_major, ap->firmware_minor,
+		 ap->firmware_fix);
 
 	if (ap->pdev)
 		strlcpy(info->bus_info, pci_name(ap->pdev),
@@ -2871,11 +2873,10 @@ static struct net_device_stats *ace_get_stats(struct net_device *dev)
 }
 
 
-static void __devinit ace_copy(struct ace_regs __iomem *regs, void *src,
-			    u32 dest, int size)
+static void __devinit ace_copy(struct ace_regs __iomem *regs, const __be32 *src,
+			       u32 dest, int size)
 {
 	void __iomem *tdest;
-	u32 *wsrc;
 	short tsize, i;
 
 	if (size <= 0)
@@ -2887,20 +2888,15 @@ static void __devinit ace_copy(struct ace_regs __iomem *regs, void *src,
 		tdest = (void __iomem *) &regs->Window +
 			(dest & (ACE_WINDOW_SIZE - 1));
 		writel(dest & ~(ACE_WINDOW_SIZE - 1), &regs->WinBase);
-		/*
-		 * This requires byte swapping on big endian, however
-		 * writel does that for us
-		 */
-		wsrc = src;
 		for (i = 0; i < (tsize / 4); i++) {
-			writel(wsrc[i], tdest + i*4);
+			/* Firmware is big-endian */
+			writel(be32_to_cpup(src), tdest);
+			src++;
+			tdest += 4;
+			dest += 4;
+			size -= 4;
 		}
-		dest += tsize;
-		src += tsize;
-		size -= tsize;
 	}
-
-	return;
 }
 
 
@@ -2939,8 +2935,13 @@ static void __devinit ace_clear(struct ace_regs __iomem *regs, u32 dest, int siz
  */
 static int __devinit ace_load_firmware(struct net_device *dev)
 {
+	const struct firmware *fw;
+	const char *fw_name = "acenic/tg2.bin";
 	struct ace_private *ap = netdev_priv(dev);
 	struct ace_regs __iomem *regs = ap->regs;
+	const __be32 *fw_data;
+	u32 load_addr;
+	int ret;
 
 	if (!(readl(&regs->CpuCtrl) & CPU_HALTED)) {
 		printk(KERN_ERR "%s: trying to download firmware while the "
@@ -2948,28 +2949,52 @@ static int __devinit ace_load_firmware(struct net_device *dev)
 		return -EFAULT;
 	}
 
+	if (ACE_IS_TIGON_I(ap))
+		fw_name = "acenic/tg1.bin";
+
+	ret = request_firmware(&fw, fw_name, &ap->pdev->dev);
+	if (ret) {
+		printk(KERN_ERR "%s: Failed to load firmware \"%s\"\n",
+		       ap->name, fw_name);
+		return ret;
+	}
+
+	fw_data = (void *)fw->data;
+
+	/* Firmware blob starts with version numbers, followed by
+	   load and start address. Remainder is the blob to be loaded
+	   contiguously from load address. We don't bother to represent
+	   the BSS/SBSS sections any more, since we were clearing the
+	   whole thing anyway. */
+	ap->firmware_major = fw->data[0];
+	ap->firmware_minor = fw->data[1];
+	ap->firmware_fix = fw->data[2];
+
+	ap->firmware_start = be32_to_cpu(fw_data[1]);
+	if (ap->firmware_start < 0x4000 || ap->firmware_start >= 0x80000) {
+		printk(KERN_ERR "%s: bogus load address %08x in \"%s\"\n",
+		       ap->name, ap->firmware_start, fw_name);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	load_addr = be32_to_cpu(fw_data[2]);
+	if (load_addr < 0x4000 || load_addr >= 0x80000) {
+		printk(KERN_ERR "%s: bogus load address %08x in \"%s\"\n",
+		       ap->name, load_addr, fw_name);
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/*
-	 * Do not try to clear more than 512KB or we end up seeing
-	 * funny things on NICs with only 512KB SRAM
+	 * Do not try to clear more than 512KiB or we end up seeing
+	 * funny things on NICs with only 512KiB SRAM
 	 */
 	ace_clear(regs, 0x2000, 0x80000-0x2000);
-	if (ACE_IS_TIGON_I(ap)) {
-		ace_copy(regs, tigonFwText, tigonFwTextAddr, tigonFwTextLen);
-		ace_copy(regs, tigonFwData, tigonFwDataAddr, tigonFwDataLen);
-		ace_copy(regs, tigonFwRodata, tigonFwRodataAddr,
-			 tigonFwRodataLen);
-		ace_clear(regs, tigonFwBssAddr, tigonFwBssLen);
-		ace_clear(regs, tigonFwSbssAddr, tigonFwSbssLen);
-	}else if (ap->version == 2) {
-		ace_clear(regs, tigon2FwBssAddr, tigon2FwBssLen);
-		ace_clear(regs, tigon2FwSbssAddr, tigon2FwSbssLen);
-		ace_copy(regs, tigon2FwText, tigon2FwTextAddr,tigon2FwTextLen);
-		ace_copy(regs, tigon2FwRodata, tigon2FwRodataAddr,
-			 tigon2FwRodataLen);
-		ace_copy(regs, tigon2FwData, tigon2FwDataAddr,tigon2FwDataLen);
-	}
-
-	return 0;
+	ace_copy(regs, &fw_data[3], load_addr, fw->size-12);
+ out:
+	release_firmware(fw);
+	return ret;
 }
 
 
diff --git a/drivers/net/acenic.h b/drivers/net/acenic.h
index 60ed183..cf7e80e 100644
--- a/drivers/net/acenic.h
+++ b/drivers/net/acenic.h
@@ -695,6 +695,10 @@ struct ace_private
 #endif
 	struct net_device_stats stats;
 	int			pci_using_dac;
+	u8			firmware_major;
+	u8			firmware_minor;
+	u8			firmware_fix;
+	u32			firmware_start;
 };
 
 

-- 
dwmw2

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[David Woodhouse]

On Wed, 2008-06-18 at 12:26 -0400, Jeff Garzik wrote:
> So, if you get an unhappy review, the reviewer must do your work for 
> you?  That doesn't work for university students, and that doesn't fly here.
> 
> We are in the part of the Linux kernel coding process called 'taking 
> feedback'.

I've taken your feedback.
I've considered it.
I've observed that it matches some of the thoughts I originally had.
And I've explained why I think you're wrong.

> The onus is on YOU to make your work friendly to both users and developers.

Indeed so. And by doing it the way we're doing it, I believe I _am_
making it friendly to both users and developers. In the patch I just
sent, you can see exactly what's changing. Now, just for fun, let's try
splitting it into two pointlessly separate patches as you seem to want.

First we add the 'new' code path...

--- drivers/net/acenic.c	2008-03-21 19:35:32.000000000 +0000
+++ drivers/net/acenic.c	2008-06-18 17:40:10.000000000 +0100
@@ -67,6 +67,7 @@
 #include <linux/mm.h>
 #include <linux/highmem.h>
 #include <linux/sockios.h>
+#include <linux/firmware.h>
 
 #if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 #include <linux/if_vlan.h>
@@ -187,8 +188,6 @@ MODULE_DEVICE_TABLE(pci, acenic_pci_tbl)
 #define MAX_RODATA_LEN	8*1024
 #define MAX_DATA_LEN	2*1024
 
-#include "acenic_firmware.h"
-
 #ifndef tigon2FwReleaseLocal
 #define tigon2FwReleaseLocal 0
 #endif
@@ -418,6 +417,10 @@ static int dis_pci_mem_inval[ACE_MAX_MOD
 MODULE_AUTHOR("Jes Sorensen <jes@trained-monkey.org>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("AceNIC/3C985/GA620 Gigabit Ethernet driver");
+#ifndef CONFIG_ACENIC_OMIT_TIGON_I
+MODULE_FIRMWARE("acenic/tg1.bin");
+#endif
+MODULE_FIRMWARE("acenic/tg2.bin");
 
 module_param_array_named(link, link_state, int, NULL, 0);
 module_param_array(trace, int, NULL, 0);
@@ -939,8 +942,8 @@ static int __devinit ace_init(struct net
 	case 4:
 	case 5:
 		printk(KERN_INFO "  Tigon I  (Rev. %i), Firmware: %i.%i.%i, ",
-		       tig_ver, tigonFwReleaseMajor, tigonFwReleaseMinor,
-		       tigonFwReleaseFix);
+		       tig_ver, ap->firmware_major, ap->firmware_minor,
+		       ap->firmware_fix);
 		writel(0, &regs->LocalCtrl);
 		ap->version = 1;
 		ap->tx_ring_entries = TIGON_I_TX_RING_ENTRIES;
@@ -948,8 +951,8 @@ static int __devinit ace_init(struct net
 #endif
 	case 6:
 		printk(KERN_INFO "  Tigon II (Rev. %i), Firmware: %i.%i.%i, ",
-		       tig_ver, tigon2FwReleaseMajor, tigon2FwReleaseMinor,
-		       tigon2FwReleaseFix);
+		       tig_ver, ap->firmware_major, ap->firmware_minor,
+		       ap->firmware_fix);
 		writel(readl(&regs->CpuBCtrl) | CPU_HALT, &regs->CpuBCtrl);
 		readl(&regs->CpuBCtrl);		/* PCI write posting */
 		/*
@@ -1201,7 +1204,9 @@ static int __devinit ace_init(struct net
 	memset(ap->info, 0, sizeof(struct ace_info));
 	memset(ap->skb, 0, sizeof(struct ace_skb));
 
-	ace_load_firmware(dev);
+	if (ace_load_firmware(dev))
+		goto init_error;
+
 	ap->fw_running = 0;
 
 	tmp_ptr = ap->info_dma;
@@ -1437,10 +1442,7 @@ static int __devinit ace_init(struct net
 	if (ap->version >= 2)
 		writel(tmp, &regs->TuneFastLink);
 
-	if (ACE_IS_TIGON_I(ap))
-		writel(tigonFwStartAddr, &regs->Pc);
-	if (ap->version == 2)
-		writel(tigon2FwStartAddr, &regs->Pc);
+	writel(ap->firmware_start, &regs->Pc);
 
 	writel(0, &regs->Mb0Lo);
 
@@ -2763,8 +2765,8 @@ static void ace_get_drvinfo(struct net_d
 
 	strlcpy(info->driver, "acenic", sizeof(info->driver));
 	snprintf(info->version, sizeof(info->version), "%i.%i.%i",
-		tigonFwReleaseMajor, tigonFwReleaseMinor,
-		tigonFwReleaseFix);
+		 ap->firmware_major, ap->firmware_minor,
+		 ap->firmware_fix);
 
 	if (ap->pdev)
 		strlcpy(info->bus_info, pci_name(ap->pdev),
@@ -2904,6 +2906,33 @@ static void __devinit ace_copy(struct ac
 }
 
 
+static void __devinit ace_copy_be(struct ace_regs __iomem *regs, const __be32 *src,
+			       u32 dest, int size)
+{
+	void __iomem *tdest;
+	short tsize, i;
+
+	if (size <= 0)
+		return;
+
+	while (size > 0) {
+		tsize = min_t(u32, ((~dest & (ACE_WINDOW_SIZE - 1)) + 1),
+			    min_t(u32, size, ACE_WINDOW_SIZE));
+		tdest = (void __iomem *) &regs->Window +
+			(dest & (ACE_WINDOW_SIZE - 1));
+		writel(dest & ~(ACE_WINDOW_SIZE - 1), &regs->WinBase);
+		for (i = 0; i < (tsize / 4); i++) {
+			/* Firmware is big-endian */
+			writel(be32_to_cpup(src), tdest);
+			src++;
+			tdest += 4;
+			dest += 4;
+			size -= 4;
+		}
+	}
+}
+
+
 static void __devinit ace_clear(struct ace_regs __iomem *regs, u32 dest, int size)
 {
 	void __iomem *tdest;
@@ -2930,30 +2959,27 @@ static void __devinit ace_clear(struct a
 	return;
 }
 
-
 /*
  * Download the firmware into the SRAM on the NIC
  *
  * This operation requires the NIC to be halted and is performed with
  * interrupts disabled and with the spinlock hold.
  */
-static int __devinit ace_load_firmware(struct net_device *dev)
+static int __devinit ace_load_static_firmware(struct net_device *dev)
 {
 	struct ace_private *ap = netdev_priv(dev);
 	struct ace_regs __iomem *regs = ap->regs;
 
-	if (!(readl(&regs->CpuCtrl) & CPU_HALTED)) {
-		printk(KERN_ERR "%s: trying to download firmware while the "
-		       "CPU is running!\n", ap->name);
-		return -EFAULT;
-	}
-
 	/*
 	 * Do not try to clear more than 512KB or we end up seeing
 	 * funny things on NICs with only 512KB SRAM
 	 */
 	ace_clear(regs, 0x2000, 0x80000-0x2000);
 	if (ACE_IS_TIGON_I(ap)) {
+		ap->firmware_major = tigonFwReleaseMajor;
+		ap->firmware_minor = tigonFwReleaseMinor;
+		ap->firmware_fix = tigonFwReleaseFix;
+		ap->firmware_start = tigonFwStartAddr;
 		ace_copy(regs, tigonFwText, tigonFwTextAddr, tigonFwTextLen);
 		ace_copy(regs, tigonFwData, tigonFwDataAddr, tigonFwDataLen);
 		ace_copy(regs, tigonFwRodata, tigonFwRodataAddr,
@@ -2961,6 +2987,10 @@ static int __devinit ace_load_firmware(s
 		ace_clear(regs, tigonFwBssAddr, tigonFwBssLen);
 		ace_clear(regs, tigonFwSbssAddr, tigonFwSbssLen);
 	}else if (ap->version == 2) {
+		ap->firmware_major = tigon2FwReleaseMajor;
+		ap->firmware_minor = tigon2FwReleaseMinor;
+		ap->firmware_fix = tigon2FwReleaseFix;
+		ap->firmware_start = tigon2FwStartAddr;
 		ace_clear(regs, tigon2FwBssAddr, tigon2FwBssLen);
 		ace_clear(regs, tigon2FwSbssAddr, tigon2FwSbssLen);
 		ace_copy(regs, tigon2FwText, tigon2FwTextAddr,tigon2FwTextLen);
@@ -2972,6 +3002,70 @@ static int __devinit ace_load_firmware(s
 	return 0;
 }
 
+static int __devinit ace_load_firmware(struct net_device *dev)
+{
+	const struct firmware *fw;
+	const char *fw_name = "acenic/tg2.bin";
+	struct ace_private *ap = netdev_priv(dev);
+	struct ace_regs __iomem *regs = ap->regs;
+	const __be32 *fw_data;
+	u32 load_addr;
+	int ret;
+
+	if (!(readl(&regs->CpuCtrl) & CPU_HALTED)) {
+		printk(KERN_ERR "%s: trying to download firmware while the "
+		       "CPU is running!\n", ap->name);
+		return -EFAULT;
+	}
+
+	if (ACE_IS_TIGON_I(ap))
+		fw_name = "acenic/tg1.bin";
+
+	ret = request_firmware(&fw, fw_name, &ap->pdev->dev);
+	if (ret) {
+		printk(KERN_ERR "%s: Failed to load firmware \"%s\"\n",
+		       ap->name, fw_name);
+		return ace_load_static_firmware(dev);
+	}
+
+	fw_data = (void *)fw->data;
+
+	/* Firmware blob starts with version numbers, followed by
+	   load and start address. Remainder is the blob to be loaded
+	   contiguously from load address. We don't bother to represent
+	   the BSS/SBSS sections any more, since we were clearing the
+	   whole thing anyway. */
+	ap->firmware_major = fw->data[0];
+	ap->firmware_minor = fw->data[1];
+	ap->firmware_fix = fw->data[2];
+
+	ap->firmware_start = be32_to_cpu(fw_data[1]);
+	if (ap->firmware_start < 0x4000 || ap->firmware_start >= 0x80000) {
+		printk(KERN_ERR "%s: bogus load address %08x in \"%s\"\n",
+		       ap->name, ap->firmware_start, fw_name);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	load_addr = be32_to_cpu(fw_data[2]);
+	if (load_addr < 0x4000 || load_addr >= 0x80000) {
+		printk(KERN_ERR "%s: bogus load address %08x in \"%s\"\n",
+		       ap->name, load_addr, fw_name);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/*
+	 * Do not try to clear more than 512KiB or we end up seeing
+	 * funny things on NICs with only 512KiB SRAM
+	 */
+	ace_clear(regs, 0x2000, 0x80000-0x2000);
+	ace_copy_be(regs, &fw_data[3], load_addr, fw->size-12);
+ out:
+	release_firmware(fw);
+	return ret;
+}
+
 
 /*
  * The eeprom on the AceNIC is an Atmel i2c EEPROM.


-- 
dwmw2

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[David Woodhouse]

On Wed, 2008-06-18 at 17:44 +0100, David Woodhouse wrote:
> In the patch I just
> sent, you can see exactly what's changing. Now, just for fun, let's
> try
> splitting it into two pointlessly separate patches as you seem to
> want.
> 
> First we add the 'new' code path...

... and then we remove the old...

For this pair of patches to be sanely reviewable, you need to combine
them into one and see what _actually_ changed. And that's why I've
posted it like that.

--- drivers/net/acenic.c	2008-06-18 17:40:10.000000000 +0100
+++ drivers/net/acenic.c	2008-06-18 17:17:51.000000000 +0100
@@ -2873,40 +2873,7 @@ static struct net_device_stats *ace_get_
 }
 
 
-static void __devinit ace_copy(struct ace_regs __iomem *regs, void *src,
-			    u32 dest, int size)
-{
-	void __iomem *tdest;
-	u32 *wsrc;
-	short tsize, i;
-
-	if (size <= 0)
-		return;
-
-	while (size > 0) {
-		tsize = min_t(u32, ((~dest & (ACE_WINDOW_SIZE - 1)) + 1),
-			    min_t(u32, size, ACE_WINDOW_SIZE));
-		tdest = (void __iomem *) &regs->Window +
-			(dest & (ACE_WINDOW_SIZE - 1));
-		writel(dest & ~(ACE_WINDOW_SIZE - 1), &regs->WinBase);
-		/*
-		 * This requires byte swapping on big endian, however
-		 * writel does that for us
-		 */
-		wsrc = src;
-		for (i = 0; i < (tsize / 4); i++) {
-			writel(wsrc[i], tdest + i*4);
-		}
-		dest += tsize;
-		src += tsize;
-		size -= tsize;
-	}
-
-	return;
-}
-
-
-static void __devinit ace_copy_be(struct ace_regs __iomem *regs, const __be32 *src,
+static void __devinit ace_copy(struct ace_regs __iomem *regs, const __be32 *src,
 			       u32 dest, int size)
 {
 	void __iomem *tdest;
@@ -2959,49 +2926,13 @@ static void __devinit ace_clear(struct a
 	return;
 }
 
+
 /*
  * Download the firmware into the SRAM on the NIC
  *
  * This operation requires the NIC to be halted and is performed with
  * interrupts disabled and with the spinlock hold.
  */
-static int __devinit ace_load_static_firmware(struct net_device *dev)
-{
-	struct ace_private *ap = netdev_priv(dev);
-	struct ace_regs __iomem *regs = ap->regs;
-
-	/*
-	 * Do not try to clear more than 512KB or we end up seeing
-	 * funny things on NICs with only 512KB SRAM
-	 */
-	ace_clear(regs, 0x2000, 0x80000-0x2000);
-	if (ACE_IS_TIGON_I(ap)) {
-		ap->firmware_major = tigonFwReleaseMajor;
-		ap->firmware_minor = tigonFwReleaseMinor;
-		ap->firmware_fix = tigonFwReleaseFix;
-		ap->firmware_start = tigonFwStartAddr;
-		ace_copy(regs, tigonFwText, tigonFwTextAddr, tigonFwTextLen);
-		ace_copy(regs, tigonFwData, tigonFwDataAddr, tigonFwDataLen);
-		ace_copy(regs, tigonFwRodata, tigonFwRodataAddr,
-			 tigonFwRodataLen);
-		ace_clear(regs, tigonFwBssAddr, tigonFwBssLen);
-		ace_clear(regs, tigonFwSbssAddr, tigonFwSbssLen);
-	}else if (ap->version == 2) {
-		ap->firmware_major = tigon2FwReleaseMajor;
-		ap->firmware_minor = tigon2FwReleaseMinor;
-		ap->firmware_fix = tigon2FwReleaseFix;
-		ap->firmware_start = tigon2FwStartAddr;
-		ace_clear(regs, tigon2FwBssAddr, tigon2FwBssLen);
-		ace_clear(regs, tigon2FwSbssAddr, tigon2FwSbssLen);
-		ace_copy(regs, tigon2FwText, tigon2FwTextAddr,tigon2FwTextLen);
-		ace_copy(regs, tigon2FwRodata, tigon2FwRodataAddr,
-			 tigon2FwRodataLen);
-		ace_copy(regs, tigon2FwData, tigon2FwDataAddr,tigon2FwDataLen);
-	}
-
-	return 0;
-}
-
 static int __devinit ace_load_firmware(struct net_device *dev)
 {
 	const struct firmware *fw;
@@ -3025,7 +2956,7 @@ static int __devinit ace_load_firmware(s
 	if (ret) {
 		printk(KERN_ERR "%s: Failed to load firmware \"%s\"\n",
 		       ap->name, fw_name);
-		return ace_load_static_firmware(dev);
+		return ret;
 	}
 
 	fw_data = (void *)fw->data;
@@ -3060,7 +2991,7 @@ static int __devinit ace_load_firmware(s
 	 * funny things on NICs with only 512KiB SRAM
 	 */
 	ace_clear(regs, 0x2000, 0x80000-0x2000);
-	ace_copy_be(regs, &fw_data[3], load_addr, fw->size-12);
+	ace_copy(regs, &fw_data[3], load_addr, fw->size-12);
  out:
 	release_firmware(fw);
 	return ret;

-- 
dwmw2

Re: [PATCH] firmware: convert acenic driver to request_firmware()

[Jes Sorensen]

>>>>> "David" == David Woodhouse <dwmw2@infradead.org> writes:

David> On Wed, 2008-06-18 at 17:44 +0100, David Woodhouse wrote:
>> In the patch I just sent, you can see exactly what's changing. Now,
>> just for fun, let's try splitting it into two pointlessly separate
>> patches as you seem to want.
>> 
>> First we add the 'new' code path...

David> ... and then we remove the old...

I'm good with either of these two models - single or dual patch
revision. You can throw in an Acked-by: Jes Sorensen <jes@sgi.com> if
you like.


Cheers,
Jes