* Re: [PATCH] firmware: convert tg3 driver to request_firmware()
[not found] <1213799313.4794.20.camel@fc6.satnam>
@ 2008-06-18 14:54 ` Jeff Garzik
2008-06-18 15:16 ` David Woodhouse
0 siblings, 1 reply; 2+ messages in thread
From: Jeff Garzik @ 2008-06-18 14:54 UTC (permalink / raw)
To: Jaswinder Singh
Cc: kernelnewbies, kernel-janitors, dwmw2, David Miller, NetDev
Jaswinder Singh wrote:
> Firmware blob is big-endian starts with version numbers,
> followed by start address and length.
> length = end_address_of_bss - start_address_of_text.
> Remainder is the blob to be loaded contiguously from start address.
>
> Signed-off-by: Jaswinder Singh <jaswinder@infradead•org>
> --
> drivers/net/Kconfig | 8
> drivers/net/tg3.c | 782 ++++-----------------------------------
> drivers/net/tg3.h | 4
> firmware/Makefile | 2
> firmware/WHENCE | 19
> firmware/tigon/tg3.bin.ihex | 175 ++++++++
> firmware/tigon/tg3_tso.bin.ihex | 446 ++++++++++++++++++++++
> firmware/tigon/tg3_tso5.bin.ihex | 252 ++++++++++++
> 8 files changed, 992 insertions(+), 696 deletions(-)
> create mode 100644 firmware/tigon/tg3.bin.ihex
> create mode 100644 firmware/tigon/tg3_tso.bin.ihex
> create mode 100644 firmware/tigon/tg3_tso5.bin.ihex
Sigh, this has the same problems as the last patch.
* the Kconfig for the firmware should default to 'Y', to make it harder
for users to create a non-working driver.
* request_firmware() capability is clearly and obviously a separate
logical change, and should not be included in the same patch as firmware
separation.
It just makes the patch longer and more difficult to read and review
with the two separate changes mushed together. Having separate patches
also means it is easier to test and validate the loadable firmware approach.
* the firmware should live in the same dir as the driver, just like it
has for its entire lifetime until now. It's silly to create a driver
hierarchy in firmware/ that parallels the rest of the tree
* fedora-rawhide, debian-unstable, and similar targets must already be
prepped for firmware installs (both from the kernel and via external
firmware packages like ipw2200-firmware).
* all net driver patches should get copied to netdev@vger•kernel.org,
which is where networking changes are discussed
This patch should not create ANY operational differences for users.
Requiring additional steps such as "make firmware_install" in order for
the user to achieve the same working driver as they had in the last
kernel is an example of such a [script-breaking] operational difference.
We have to make it tough for users to screw this up. It's too easy to
create a non-working driver, with request_firmware()... as existing
distro support for firmwares has already proven in the field.
Jeff
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] firmware: convert tg3 driver to request_firmware()
2008-06-18 14:54 ` [PATCH] firmware: convert tg3 driver to request_firmware() Jeff Garzik
@ 2008-06-18 15:16 ` David Woodhouse
0 siblings, 0 replies; 2+ messages in thread
From: David Woodhouse @ 2008-06-18 15:16 UTC (permalink / raw)
To: Jeff Garzik
Cc: Jaswinder Singh, kernelnewbies, kernel-janitors, David Miller,
NetDev
On Wed, 2008-06-18 at 10:54 -0400, Jeff Garzik wrote:
> It just makes the patch longer and more difficult to read and review
> with the two separate changes mushed together. Having separate patches
> also means it is easier to test and validate the loadable firmware approach.
In this _particular_ case, because the big hex arrays were directly in
tg3.c and not a header file, it might be a little easier to review the
code changes for correctness if we post a cut-down patch which doesn't
actually _remove_ the hex arrays, but instead just leaves them present
but unused. That cut-down patch is below.
It still doesn't make sense to add a completely _new_ code path for
loading firmware by request_firmware() though; that would just have to
add new functions to parallel with each function touched by patch below,
and it would be much harder to read the patch and actually see what's
changing.
This is the patch we're seeking constructive review of:
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 07b3f77..a70147d 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -38,6 +38,7 @@
#include <linux/workqueue.h>
#include <linux/prefetch.h>
#include <linux/dma-mapping.h>
+#include <linux/firmware.h>
#include <net/checksum.h>
#include <net/ip.h>
@@ -139,6 +140,9 @@ MODULE_AUTHOR("David S. Miller (davem@redhat•com) and Jeff Garzik (jgarzik@pobox
MODULE_DESCRIPTION("Broadcom Tigon3 ethernet driver");
MODULE_LICENSE("GPL");
MODULE_VERSION(DRV_MODULE_VERSION);
+MODULE_FIRMWARE("tigon/tg3.bin");
+MODULE_FIRMWARE("tigon/tg3_tso.bin");
+MODULE_FIRMWARE("tigon/tg3_tso5.bin");
static int tg3_debug = -1; /* -1 == use TG3_DEF_MSG_ENABLE as value */
module_param(tg3_debug, int, 0);
@@ -5778,15 +5658,9 @@ static int tg3_halt_cpu(struct tg3 *tp, u32 offset)
}
struct fw_info {
- unsigned int text_base;
- unsigned int text_len;
- const u32 *text_data;
- unsigned int rodata_base;
- unsigned int rodata_len;
- const u32 *rodata_data;
- unsigned int data_base;
- unsigned int data_len;
- const u32 *data_data;
+ unsigned int fw_base;
+ unsigned int fw_len;
+ const __be32 *fw_data;
};
/* tp->lock is held. */
@@ -5823,24 +5697,11 @@ static int tg3_load_firmware_cpu(struct tg3 *tp, u32 cpu_base, u32 cpu_scratch_b
write_op(tp, cpu_scratch_base + i, 0);
tw32(cpu_base + CPU_STATE, 0xffffffff);
tw32(cpu_base + CPU_MODE, tr32(cpu_base+CPU_MODE)|CPU_MODE_HALT);
- for (i = 0; i < (info->text_len / sizeof(u32)); i++)
- write_op(tp, (cpu_scratch_base +
- (info->text_base & 0xffff) +
- (i * sizeof(u32))),
- (info->text_data ?
- info->text_data[i] : 0));
- for (i = 0; i < (info->rodata_len / sizeof(u32)); i++)
- write_op(tp, (cpu_scratch_base +
- (info->rodata_base & 0xffff) +
- (i * sizeof(u32))),
- (info->rodata_data ?
- info->rodata_data[i] : 0));
- for (i = 0; i < (info->data_len / sizeof(u32)); i++)
+ for (i = 0; i < (info->fw_len / sizeof(u32)); i++)
write_op(tp, (cpu_scratch_base +
- (info->data_base & 0xffff) +
+ (info->fw_base & 0xffff) +
(i * sizeof(u32))),
- (info->data_data ?
- info->data_data[i] : 0));
+ be32_to_cpu(info->fw_data[i]));
err = 0;
@@ -5852,17 +5713,20 @@ out:
static int tg3_load_5701_a0_firmware_fix(struct tg3 *tp)
{
struct fw_info info;
+ const __be32 *fw_data;
int err, i;
- info.text_base = TG3_FW_TEXT_ADDR;
- info.text_len = TG3_FW_TEXT_LEN;
- info.text_data = &tg3FwText[0];
- info.rodata_base = TG3_FW_RODATA_ADDR;
- info.rodata_len = TG3_FW_RODATA_LEN;
- info.rodata_data = &tg3FwRodata[0];
- info.data_base = TG3_FW_DATA_ADDR;
- info.data_len = TG3_FW_DATA_LEN;
- info.data_data = NULL;
+ fw_data = (void *)tp->fw->data;
+
+ /* Firmware blob starts with version numbers, followed by
+ start address and length. We are setting complete length.
+ length = end_address_of_bss - start_address_of_text.
+ Remainder is the blob to be loaded contiguously
+ from start address. */
+
+ info.fw_base = be32_to_cpu(fw_data[1]);
+ info.fw_len = tp->fw->size-12;
+ info.fw_data = &fw_data[3];
err = tg3_load_firmware_cpu(tp, RX_CPU_BASE,
RX_CPU_SCRATCH_BASE, RX_CPU_SCRATCH_SIZE,
@@ -5878,21 +5742,21 @@ static int tg3_load_5701_a0_firmware_fix(struct tg3 *tp)
/* Now startup only the RX cpu. */
tw32(RX_CPU_BASE + CPU_STATE, 0xffffffff);
- tw32_f(RX_CPU_BASE + CPU_PC, TG3_FW_TEXT_ADDR);
+ tw32_f(RX_CPU_BASE + CPU_PC, info.fw_base);
for (i = 0; i < 5; i++) {
- if (tr32(RX_CPU_BASE + CPU_PC) == TG3_FW_TEXT_ADDR)
+ if (tr32(RX_CPU_BASE + CPU_PC) == info.fw_base)
break;
tw32(RX_CPU_BASE + CPU_STATE, 0xffffffff);
tw32(RX_CPU_BASE + CPU_MODE, CPU_MODE_HALT);
- tw32_f(RX_CPU_BASE + CPU_PC, TG3_FW_TEXT_ADDR);
+ tw32_f(RX_CPU_BASE + CPU_PC, info.fw_base);
udelay(1000);
}
if (i >= 5) {
printk(KERN_ERR PFX "tg3_load_firmware fails for %s "
"to set RX CPU PC, is %08x should be %08x\n",
tp->dev->name, tr32(RX_CPU_BASE + CPU_PC),
- TG3_FW_TEXT_ADDR);
+ info.fw_base);
return -ENODEV;
}
tw32(RX_CPU_BASE + CPU_STATE, 0xffffffff);
@@ -5901,41 +5765,32 @@ static int tg3_load_5701_a0_firmware_fix(struct tg3 *tp)
/* tp->lock is held. */
static int tg3_load_tso_firmware(struct tg3 *tp)
{
struct fw_info info;
+ const __be32 *fw_data;
unsigned long cpu_base, cpu_scratch_base, cpu_scratch_size;
int err, i;
if (tp->tg3_flags2 & TG3_FLG2_HW_TSO)
return 0;
+ fw_data = (void *)tp->fw->data;
+
+ /* Firmware blob starts with version numbers, followed by
+ start address and length. We are setting complete length.
+ length = end_address_of_bss - start_address_of_text.
+ Remainder is the blob to be loaded contiguously
+ from start address. */
+
+ info.fw_base = be32_to_cpu(fw_data[1]);
+ cpu_scratch_size = tp->fw_len;
+ info.fw_len = tp->fw->size-12;
+ info.fw_data = &fw_data[3];
+
if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705) {
- info.text_base = TG3_TSO5_FW_TEXT_ADDR;
- info.text_len = TG3_TSO5_FW_TEXT_LEN;
- info.text_data = &tg3Tso5FwText[0];
- info.rodata_base = TG3_TSO5_FW_RODATA_ADDR;
- info.rodata_len = TG3_TSO5_FW_RODATA_LEN;
- info.rodata_data = &tg3Tso5FwRodata[0];
- info.data_base = TG3_TSO5_FW_DATA_ADDR;
- info.data_len = TG3_TSO5_FW_DATA_LEN;
- info.data_data = &tg3Tso5FwData[0];
cpu_base = RX_CPU_BASE;
cpu_scratch_base = NIC_SRAM_MBUF_POOL_BASE5705;
- cpu_scratch_size = (info.text_len +
- info.rodata_len +
- info.data_len +
- TG3_TSO5_FW_SBSS_LEN +
- TG3_TSO5_FW_BSS_LEN);
} else {
- info.text_base = TG3_TSO_FW_TEXT_ADDR;
- info.text_len = TG3_TSO_FW_TEXT_LEN;
- info.text_data = &tg3TsoFwText[0];
- info.rodata_base = TG3_TSO_FW_RODATA_ADDR;
- info.rodata_len = TG3_TSO_FW_RODATA_LEN;
- info.rodata_data = &tg3TsoFwRodata[0];
- info.data_base = TG3_TSO_FW_DATA_ADDR;
- info.data_len = TG3_TSO_FW_DATA_LEN;
- info.data_data = &tg3TsoFwData[0];
cpu_base = TX_CPU_BASE;
cpu_scratch_base = TX_CPU_SCRATCH_BASE;
cpu_scratch_size = TX_CPU_SCRATCH_SIZE;
@@ -6455,21 +5808,21 @@ static int tg3_load_tso_firmware(struct tg3 *tp)
/* Now startup the cpu. */
tw32(cpu_base + CPU_STATE, 0xffffffff);
- tw32_f(cpu_base + CPU_PC, info.text_base);
+ tw32_f(cpu_base + CPU_PC, info.fw_base);
for (i = 0; i < 5; i++) {
- if (tr32(cpu_base + CPU_PC) == info.text_base)
+ if (tr32(cpu_base + CPU_PC) == info.fw_base)
break;
tw32(cpu_base + CPU_STATE, 0xffffffff);
tw32(cpu_base + CPU_MODE, CPU_MODE_HALT);
- tw32_f(cpu_base + CPU_PC, info.text_base);
+ tw32_f(cpu_base + CPU_PC, info.fw_base);
udelay(1000);
}
if (i >= 5) {
printk(KERN_ERR PFX "tg3_load_tso_firmware fails for %s "
"to set CPU PC, is %08x should be %08x\n",
tp->dev->name, tr32(cpu_base + CPU_PC),
- info.text_base);
+ info.fw_base);
return -ENODEV;
}
tw32(cpu_base + CPU_STATE, 0xffffffff);
@@ -6731,11 +6084,7 @@ static int tg3_reset_hw(struct tg3 *tp, int reset_phy)
else if (tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE) {
int fw_len;
- fw_len = (TG3_TSO5_FW_TEXT_LEN +
- TG3_TSO5_FW_RODATA_LEN +
- TG3_TSO5_FW_DATA_LEN +
- TG3_TSO5_FW_SBSS_LEN +
- TG3_TSO5_FW_BSS_LEN);
+ fw_len = tp->fw_len;
fw_len = (fw_len + (0x80 - 1)) & ~(0x80 - 1);
tw32(BUFMGR_MB_POOL_ADDR,
NIC_SRAM_MBUF_POOL_BASE5705 + fw_len);
@@ -12706,6 +12055,8 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
struct net_device *dev;
struct tg3 *tp;
int err, pm_cap;
+ const __be32 *fw_data;
+ const char *fw_name = NULL;
char str[40];
u64 dma_mask, persist_dma_mask;
DECLARE_MAC_BUF(mac);
@@ -12878,8 +12229,16 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
tg3_init_bufmgr_config(tp);
+ if (tp->pci_chip_rev_id == CHIPREV_ID_5701_A0)
+ fw_name = "tigon/tg3.bin";
+
if (tp->tg3_flags2 & TG3_FLG2_HW_TSO) {
tp->tg3_flags2 |= TG3_FLG2_TSO_CAPABLE;
+
+ if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705)
+ fw_name = "tigon/tg3_tso5.bin";
+ else
+ fw_name = "tigon/tg3_tso.bin";
}
else if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5700 ||
GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5701 ||
@@ -12891,6 +12250,31 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
tp->tg3_flags2 |= TG3_FLG2_TSO_CAPABLE | TG3_FLG2_TSO_BUG;
}
+ if (fw_name) {
+ err = request_firmware(&tp->fw, fw_name, &tp->pdev->dev);
+ if (err) {
+ printk(KERN_ERR "tg3: Failed to load firmware \"%s\"\n",
+ fw_name);
+ goto err_out_apeunmap;
+ }
+
+ fw_data = (void *)tp->fw->data;
+
+ /* Firmware blob starts with version numbers, followed by
+ start address and length. We are setting complete length.
+ length = end_address_of_bss - start_address_of_text.
+ Remainder is the blob to be loaded contiguously
+ from start address. */
+
+ tp->fw_len = be32_to_cpu(fw_data[2]); /* includes bss */
+ if (tp->fw_len < (tp->fw->size-12)) {
+ printk(KERN_ERR "tg3: bogus length %d in \"%s\"\n",
+ tp->fw_len, fw_name);
+ err = -EINVAL;
+ goto err_out_apeunmap;
+ }
+ }
+
/* TSO is on by default on chips that support hardware TSO.
* Firmware TSO on older chips gives lower performance, so it
* is off by default, but can be enabled using ethtool.
@@ -13017,6 +12401,9 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
return 0;
err_out_apeunmap:
+ if (tp->fw)
+ release_firmware(tp->fw);
+
if (tp->aperegs) {
iounmap(tp->aperegs);
tp->aperegs = NULL;
@@ -13047,6 +12434,9 @@ static void __devexit tg3_remove_one(struct pci_dev *pdev)
if (dev) {
struct tg3 *tp = netdev_priv(dev);
+ if (tp->fw)
+ release_firmware(tp->fw);
+
flush_scheduled_work();
unregister_netdev(dev);
if (tp->aperegs) {
diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
index 0404f93..435ff9b 100644
--- a/drivers/net/tg3.h
+++ b/drivers/net/tg3.h
@@ -2611,6 +2611,10 @@ struct tg3 {
#define SST_25VF0X0_PAGE_SIZE 4098
struct ethtool_coalesce coal;
+
+ /* firmware info */
+ const struct firmware *fw;
+ u32 fw_len;
};
#endif /* !(_T3_H) */
--
dwmw2
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2008-06-18 15:17 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1213799313.4794.20.camel@fc6.satnam>
2008-06-18 14:54 ` [PATCH] firmware: convert tg3 driver to request_firmware() Jeff Garzik
2008-06-18 15:16 ` David Woodhouse
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox