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: Tue, 28 May 2013 08:57:20 +0200	[thread overview]
Message-ID: <20130528065720.GU32299@pengutronix.de> (raw)
In-Reply-To: <51A0DF4E.8020009@rempel-privat.de>

On Sat, May 25, 2013 at 05:57:02PM +0200, Oleksij Rempel wrote:
> Am 24.05.2013 09:09, schrieb Sascha Hauer:
> >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.
> 
> This part should be fixed after Anthonys "MIPS: add initial cache support"
> 
> >>+
> >>+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.
> 
> Currently there is no other choice.
> 
> >>+
> >>+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.
> 
> This loop is needed to filter broadcast packtes. If remove this loop
> and reduce rx buffer, i will get really bad packet losses.

What I meant is that you should only pull a single packet out of the
queue each time you enter ar231x_eth_recv. You shouldn't have to reduce
the rx buffer for this.

Anyway, thinking about it it should be fine to call net_receive on all
currently available buffers like you did above. Just keep it like it is
now.

So with the request_mem_region fix this driver should be fine.

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-28  6:57 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
2013-05-24  9:14     ` Jean-Christophe PLAGNIOL-VILLARD
2013-05-25 15:57     ` Oleksij Rempel
2013-05-28  6:57       ` Sascha Hauer [this message]
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=20130528065720.GU32299@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