Re: [PATCH v3 1/7] AMSO1100 Low Level Driver.

[Arjan van de Ven <arjan@infradead.org>]

On Tue, 2006-06-20 at 15:30 -0500, Steve Wise wrote:

> +/*
> + * Allocate TX ring elements and chain them together.
> + * One-to-one association of adapter descriptors with ring elements.
> + */
> +static int c2_tx_ring_alloc(struct c2_ring *tx_ring, void *vaddr,
> +			    dma_addr_t base, void __iomem * mmio_txp_ring)
> +{
> +	struct c2_tx_desc *tx_desc;
> +	struct c2_txp_desc __iomem *txp_desc;
> +	struct c2_element *elem;
> +	int i;
> +
> +	tx_ring->start = kmalloc(sizeof(*elem) * tx_ring->count, GFP_KERNEL);

I would think this needs a dma_alloc_coherent() rather than a kmalloc...


> +
> +/* Free all buffers in RX ring, assumes receiver stopped */
> +static void c2_rx_clean(struct c2_port *c2_port)
> +{
> +	struct c2_dev *c2dev = c2_port->c2dev;
> +	struct c2_ring *rx_ring = &c2_port->rx_ring;
> +	struct c2_element *elem;
> +	struct c2_rx_desc *rx_desc;
> +
> +	elem = rx_ring->start;
> +	do {
> +		rx_desc = elem->ht_desc;
> +		rx_desc->len = 0;
> +
> +		__raw_writew(0, elem->hw_desc + C2_RXP_STATUS);
> +		__raw_writew(0, elem->hw_desc + C2_RXP_COUNT);
> +		__raw_writew(0, elem->hw_desc + C2_RXP_LEN);

you seem to be a fan of the __raw_write() functions... any reason why?
__raw_ is not a magic "go faster" prefix....

Also on a related note, have you checked the driver for the needed PCI
posting flushes?

> +
> +	/* Disable IRQs by clearing the interrupt mask */
> +	writel(1, c2dev->regs + C2_IDIS);
> +	writel(0, c2dev->regs + C2_NIMR0);

like here...
> +
> +	elem = tx_ring->to_use;
> +	elem->skb = skb;
> +	elem->mapaddr = mapaddr;
> +	elem->maplen = maplen;
> +
> +	/* Tell HW to xmit */
> +	__raw_writeq(cpu_to_be64(mapaddr), elem->hw_desc + C2_TXP_ADDR);
> +	__raw_writew(cpu_to_be16(maplen), elem->hw_desc + C2_TXP_LEN);
> +	__raw_writew(cpu_to_be16(TXP_HTXD_READY), elem->hw_desc + C2_TXP_FLAGS);

or here

> +static int c2_change_mtu(struct net_device *netdev, int new_mtu)
> +{
> +	int ret = 0;
> +
> +	if (new_mtu < ETH_ZLEN || new_mtu > ETH_JUMBO_MTU)
> +		return -EINVAL;
> +
> +	netdev->mtu = new_mtu;
> +
> +	if (netif_running(netdev)) {
> +		c2_down(netdev);
> +
> +		c2_up(netdev);
> +	}

this looks odd...