mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Sascha Hauer <s.hauer@pengutronix.de>
To: Oleksij Rempel <linux@rempel-privat.de>
Cc: barebox@lists.infradead.org
Subject: Re: [RFC, PATCH v2 2/3] net: add ar231x-eth support
Date: Fri, 24 May 2013 09:09:16 +0200	[thread overview]
Message-ID: <20130524070916.GX32299@pengutronix.de> (raw)
In-Reply-To: <1369208989-14369-3-git-send-email-linux@rempel-privat.de>

On Wed, May 22, 2013 at 09:49:48AM +0200, Oleksij Rempel wrote:
> This driver should work with some Atheros WiSoCs:
> - ar2312, ar2313
> - ar2315, ar2316 ...
> 
> Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
> ---
>  drivers/net/Kconfig  |   7 +
>  drivers/net/Makefile |   1 +
>  drivers/net/ar231x.c | 429 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/ar231x.h | 219 ++++++++++++++++++++++++++
>  4 files changed, 656 insertions(+)
>  create mode 100644 drivers/net/ar231x.c
>  create mode 100644 drivers/net/ar231x.h
> 
> +
> +	/* FIXME: priv->{t,r}x_ring are virtual addresses,
> +	 * use virt-to-phys convertion */

We use 1:1 mappings, so I think this comment should be removed.

> +
> +static void ar231x_allocate_dma_descriptors(struct eth_device *edev)
> +{
> +	struct ar231x_eth_priv *priv = edev->priv;
> +	u16 ar231x_descr_size = sizeof(struct ar231x_descr);
> +	u16 i;
> +
> +	priv->tx_ring = xmalloc(ar231x_descr_size);

What alignment do you need here? This may or may not be safe.

> +
> +static int ar231x_eth_recv(struct eth_device *edev)
> +{
> +	struct ar231x_eth_priv *priv = edev->priv;
> +
> +	while (1) {
> +		struct ar231x_descr *rxdsc = priv->next_rxdsc;
> +		u32 status = rxdsc->status;
> +
> +		/* owned by DMA? */
> +		if (status & DMA_RX_OWN)
> +			break;
> +
> +		/* Pick only packets what we can handle:
> +		 * - only complete packet per buffer
> +		 *   (First and Last at same time)
> +		 * - drop multicast */
> +		if (!priv->kill_rx_ring &&
> +				((status & DMA_RX_MASK) == DMA_RX_FSLS)) {
> +			u16 length =
> +				((status >> DMA_RX_LEN_SHIFT) & 0x3fff)
> +				- CRC_LEN;
> +			net_receive((void *)rxdsc->buffer_ptr, length);
> +		}
> +		/* Clean descriptor. now it is owned by DMA. */
> +		priv->next_rxdsc = (struct ar231x_descr *)rxdsc->next_dsc_ptr;
> +		ar231x_flash_rxdsc(rxdsc);
> +	}

This loop looks wrong. You should only receive a single packet for each
call of this function.

> +
> +static int ar231x_eth_probe(struct device_d *dev)
> +{
> +	struct ar231x_eth_priv *priv;
> +	struct eth_device *edev;
> +	struct mii_bus *miibus;
> +	struct ar231x_eth_platform_data *pdata;
> +
> +	if (!dev->platform_data) {
> +		dev_err(dev, "no platform data\n");
> +		return -ENODEV;
> +	}
> +
> +	pdata = dev->platform_data;
> +
> +	priv = xzalloc(sizeof(struct ar231x_eth_priv));
> +	edev = &priv->edev;
> +	miibus = &priv->miibus;
> +	edev->priv = priv;
> +
> +	/* link all platform depended regs */
> +	priv->mac = pdata->mac;
> +
> +	priv->eth_regs = dev_request_mem_region(dev, 0);
> +	/* we have 0x100000 for eth, part of it are dma regs.
> +	 * So they are already requested */
> +	priv->dma_regs = (void *)(priv->eth_regs + 0x1000);
> +
> +	priv->phy_regs = dev_request_mem_region(dev, 1);
> +
> +	priv->cfg = pdata;
> +	edev->init = ar231x_eth_init;
> +	edev->open = ar231x_eth_open;
> +	edev->send = ar231x_eth_send;
> +	edev->recv = ar231x_eth_recv;
> +	edev->halt = ar231x_eth_halt;
> +	edev->get_ethaddr = ar231x_get_ethaddr;
> +	edev->set_ethaddr = ar231x_set_ethaddr;
> +
> +	priv->miibus.read = ar231x_miibus_read;
> +	priv->miibus.write = ar231x_miibus_write;
> +	priv->miibus.reset = ar231x_mdiibus_reset;
> +	priv->miibus.priv = priv;
> +	priv->miibus.parent = dev;
> +
> +	mdiobus_register(miibus);
> +	eth_register(edev);

Please check the return values of dev_request_mem_region,
mdiobus_register and eth_register. I'm fine with not properly cleaning
up afterwards, but we should be able to notice the user if something
goes wrong here. (Yeah, I know, many network driver don't do so)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

  reply	other threads:[~2013-05-24  7:09 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-05-22  7:49 [RFC, PATCH v2 0/3] work on atheros ar2313 Oleksij Rempel
2013-05-22  7:49 ` [RFC, PATCH v2 1/3] MIPS: add Atheros ar531x family support Oleksij Rempel
2013-05-23 13:35   ` Sascha Hauer
2013-05-23 13:43     ` Oleksij Rempel
2013-05-22  7:49 ` [RFC, PATCH v2 2/3] net: add ar231x-eth support Oleksij Rempel
2013-05-24  7:09   ` Sascha Hauer [this message]
2013-05-24  9:14     ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-25 15:57     ` Oleksij Rempel
2013-05-28  6:57       ` Sascha Hauer
2013-05-22  7:49 ` [RFC, PATCH v2 3/3] MIPS: ar231x: add netgear-wg102 Oleksij Rempel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130524070916.GX32299@pengutronix.de \
    --to=s.hauer@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=linux@rempel-privat.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox