lkml.org 
[lkml]   [2004]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Gigabit Ethernet support for forcedeth
Jeff Garzik wrote:

> Manfred Spraul wrote:
>
>> -#define FORCEDETH_VERSION "0.25"
>> +#define FORCEDETH_VERSION "0.28"
>
>
> does this mean that .26 and .27 will be submitted as separate patches?
>
I think they were internal patches from Carl-Daniel. I think one of them
was for wol support, the other one a bugfix.

> what is the goal for a 1.0.0 version?
>
At least
- wol support
- ethtool support

> These days I try to encourage moving to a 1.0 version once things seem
> to be working and stable.
>
Ok.

>> /* limited to 1 packet until we understand NV_TX_LASTPACKET */
>> -#define TX_LIMIT_STOP 10
>> -#define TX_LIMIT_START 5
>> +#define TX_LIMIT_STOP 63
>> +#define TX_LIMIT_START 62
>
>
> what's with the "limited to 1 packet" comment?
>
It's probably stale, I must check my docs. I'll remove it after some
more testing.

>> #define RX_NIC_BUFSIZE (DEFAULT_MTU + 64)
>
>
> definition of DEFAULT_MTU is a bit silly ... why not just use
> ETH_DATA_LEN?
>
Ok, I'll fix that.

>> - u16 tx_flags;
>> + u32 tx_flags;
>> };
>
>
> has this driver been tested with the case where ethtool is used to
> force the media to a specific speed (such as 100mbit full duplex)?
>
The driver doesn't support ethtool, so this was not tested.
I have tested it with a cross-over cable to a natsemi nic and then
forced link speeds on the natsemi nic with ethtool.

>>
>> - prd = &np->tx_ring[i];
>> + Flags = cpu_to_le32(np->tx_ring[i].FlagLen);
>
>
> shouldn't this be le32_to_cpu() ?
>
Ups. You are right. I'll fix it and send you an updated patch.

>> + dprintk(KERN_INFO "%s: reconfiguration for multicast lists.\n",
>> + dev->name);
>> nv_start_rx(dev);
>> spin_unlock_irq(&np->lock);
>
>
> some of these dprintk() would be more appropriate as verbose printks
> enabled and disabled using netif_msg_xxx bitmaps.
>
I agree - dprintk on is too verbose and dprintk off is too quiet. I'll
fix it in another patch.

>> - /* 4) continue setup */
>> + /* 4) give hw rings */
>> + writel((u32) np->ring_addr, base + NvRegRxRingPhysAddr);
>> + writel((u32) (np->ring_addr + RX_RING*sizeof(struct ring_desc)),
>> base + NvRegTxRingPhysAddr);
>> + writel( ((RX_RING-1) << NVREG_RINGSZ_RXSHIFT) + ((TX_RING-1) <<
>> NVREG_RINGSZ_TXSHIFT),
>> + base + NvRegRingSizes);
>
>
> your driver needs to call pci_set_dma_mask() and
> pci_set_consistent_dma_mask().
>
I'll add that.

>> + np->tx_flags = NV_TX2_LASTPACKET|NV_TX2_VALID;
>> + if (id->driver_data & DEV_NEED_LASTPACKET1)
>> + np->tx_flags |= NV_TX2_LASTPACKET1;
>> + }
>
>
> would it be faster to simply ensure that np->tx_flags is fixed-endian?
>
Not really: the only use is
ring_flags = cpu_to_le32( (skb->len-1) | tx_flags);
Pre-swapping it would change that to
ring_flags = cpu_to_le32(skb->len-1) | np->tx_flags;

>> + if (i == 32) {
>> + /* PHY in isolate mode? No phy attached and user wants to
>> + * test loopback? Very odd, but can be correct.
>> + */
>> + printk(KERN_INFO "%s: open: Could not find a valid PHY.\n",
>> + pci_name(pci_dev));
>> + }
>
>
> I recommend trying phy 0 after phy 31, _iff_ the scan found nothing.
>
A phy with id 0 is in isolate mode. It must be reconfigured before it
can be used, and this is not yet handled.

>>
>> MODULE_PARM(max_interrupt_work, "i");
>> MODULE_PARM_DESC(max_interrupt_work, "forcedeth maximum events
>> handled per interrupt");
>> - +
>> MODULE_AUTHOR("Manfred Spraul <manfred@colorfullife.com>");
>> MODULE_DESCRIPTION("Reverse Engineered nForce ethernet driver");
>> MODULE_LICENSE("GPL");
>
>
> module_param() use would be nice :)
>
Ok.

--
Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:04    [W:0.079 / U:0.476 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site