|
| Applied to 'sky2' branch of netdev-2.6.git. Let me know when the RX
| hangs are fixed, and we can push upstream.
Okay. Most all of your comments are straightforward
and fixes will be in the next version, but some need
a little more discussion...
|
| > +static void gm_phy_write(struct sky2_hw *hw, unsigned port, u16 reg, u16
val)
| > +{
| > + int i;
| > +
| > + gma_write16(hw, port, GM_SMI_DATA, val);
| > + gma_write16(hw, port, GM_SMI_CTRL,
| > + GM_SMI_CT_PHY_AD(PHY_ADDR_MARV) | GM_SMI_CT_REG_AD(reg));
| > +
| > + for (i = 0; i < PHY_RETRIES; i++) {
| > + udelay(1);
|
| PCI posting bug... do we care?
We always end up doing a read of the control register
so how is that a problem?
| > + sky2_write8(hw, SK_REG(port, GMAC_IRQ_MSK), 0);
| > + /* disable PHY IRQs */
| > + gm_phy_write(hw, port, PHY_MARV_INT_MASK, 0);
| > + gma_write16(hw, port, GM_MC_ADDR_H1, 0); /* clear MC hash */
| > + gma_write16(hw, port, GM_MC_ADDR_H2, 0);
| > + gma_write16(hw, port, GM_MC_ADDR_H3, 0);
| > + gma_write16(hw, port, GM_MC_ADDR_H4, 0);
|
| can this be done with two 32-bit writes? or writeq?
No that part of the chip seems to only really take 16 bit
accesses, and the multicast address parts are not contiguous.
| > +
| > + /* set LED Function Control register */
| > + gm_phy_write(hw, port, PHY_MARV_PHY_CTRL,
| > + (PHY_M_LEDC_LOS_CTRL(1) | /* LINK/ACT */
| > + PHY_M_LEDC_INIT_CTRL(7) | /* 10 Mbps */
| > + PHY_M_LEDC_STA1_CTRL(7) | /* 100 Mbps */
| > + PHY_M_LEDC_STA0_CTRL(7))); /* 1000
Mbps */
|
| are you enabling the -possibility- of turning on the LEDs, or actually
| turning on all these LEDs?
Not sure really, and don't have that flavor of chip to tell.
That piece copied from sk98lin.
| > +static inline void sky2_rx_add(struct sky2_port *sky2, dma_addr_t map, u16
len)
| > +{
| > + struct sky2_rx_le *le;
| > +
| > + if (sizeof(map) > sizeof(u32)) {
| > + le = sky2_next_rx(sky2);
| > + le->rx.addr = cpu_to_le32((u64) map >> 32);
|
| use (map >> 16) >> 16
Does it matter? because the map will be a 64 bit value
if we ever get to that part of the code anyway. On 32bit
address systems this code disappears.
| > +
| > + paddr = pci_map_single(sky2->hw->pdev, re->skb->data,
| > + rx_buf_size, PCI_DMA_FROMDEVICE);
| > +
| > + pci_unmap_len_set(re, maplen, rx_buf_size);
| > + pci_unmap_addr_set(re, mapaddr, paddr);
|
| pci_unmap???
This is book keeping for iommu systems. see other drivers.
| > +
| > +/*
| > + * Worst case number of list elements is 36
| > + * TSO + CHKSUM + ADDR64 + BUFFER + (ADDR+BUFFER)*MAXFRAGS
| > + */
| > +#define MAX_SKB_TX_LE (4 + 2*MAX_SKB_FRAGS)
| > +
| > +static inline int sky2_xmit_avail(const struct sky2_port *sky2)
| > +{
| > + return (sky2->tx_cons > sky2->tx_prod ? 0 : TX_RING_SIZE)
| > + + sky2->tx_cons - sky2->tx_prod - 1;
| > +}
|
| this seems terribly convoluted and fragile. can't you simplify things?
| Look at some other drivers for examples.
It is how e1000 does it, but will be reworking it to be
more like tg3 with variable ring size.
| > +
| > + /* Handle TCP checksum offload */
| > + ctrl = 0;
| > + if (skb->ip_summed == CHECKSUM_HW) {
| > + ptrdiff_t hdr = skb->h.raw - skb->data;
| > +
| > + ctrl = CALSUM | WR_SUM | INIT_SUM | LOCK_SUM;
| > + if (skb->nh.iph->protocol == IPPROTO_UDP)
| > + ctrl |= UDPTCP;
|
| is this bit set only for UDP?
It appears so from sk98lin driver, still needs testing.
| > + goto err_out_free_hw;
| > + }
| > +
| > + err = request_irq(pdev->irq, sky2_intr, SA_SHIRQ, DRV_NAME, hw);
| > + if (err) {
| > + printk(KERN_ERR PFX "%s: cannot assign irq %d\n",
| > + pci_name(pdev), pdev->irq);
| > + goto err_out_iounmap;
| > + }
|
| don't do this in probe, do it in dev->open(). reading your interrupt
| handling code you are OBVIOUSLY not ready to handle interrupts at this
| point.
Since this driver has to support dual port boards, it
gets wierd. I would rather just change to mask of irq's in HW
then get it at probe.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
