LL TEMAC V6 refactored

U-Boot Superseded Stephan Linz 3 participants 9 messages Nov 27, 2011 – Nov 29, 2011

Subsystems (2)

Network drivers Arm/Zynq architecture

lore

Share & tools

lore

Stephan Linz

author

2011-11-27 18:43 UTC #0 lore raw

@Michal: please, can you test this code on Xilinx PPC4xx platforms and acknowledge the results? I have no access to a ml405 and ml507.

Note: To use the driver on Microblaze, you will need additional platform patches by Michal Simek:
- http://patchwork.ozlabs.org/patch/112535/
- http://patchwork.ozlabs.org/patch/112534/
- http://patchwork.ozlabs.org/patch/112532/

I will collect all related patches on a public bundle:
- http://patchwork.ozlabs.org/bundle/rexut/net-ll_temac/

br,
Stephan

Stephan Linz

author

2011-11-27 18:43 UTC #1 lore raw

PATCH v6 1/1 net: ll_temac: Add LL TEMAC driver to u-boot

Xilinx LocalLink Tri-Mode Ether MAC driver can be
used by Xilinx Microblaze or Xilinx ppc405/440 in
SDMA and FIFO mode. DCR or XPS bus can be used.

The driver uses and requires MII and PHYLIB.

Signed-off-by Stephan Linz <[email protected]>

---
v6: Code cleanup with ./tools/checkpatch.pl

v5: Remove more endless loops
Remove useless parenthesis in pointer operations Move phyaddr predefinition on top
Remove supernumerary newlines
Remove parenthesis around numbers (globally)

v4: Separate fifo and sdma code from driver core
Split sdma code into separate DCR and XPS bus access code Add extensive register struct definitions and enumerations Add new callbacks into fifo an sdma code
Prepare CDMAC buffer handling to be unique for every instance Separate Xilinx specific indirect DCR access, so we can move to arch Remove useless 'emac' parameter from indirect access helper functions Correct MDIO clock setup.
Remove endless loops
Common code beautifying

v3: Use helper functions for fifo mode
Use helper functions for indirect accesses
Code cleanup
Add comments for MAGIC values
Simplify code in fifo mode

v2: Remove helper function for access to temac
Remove SDMA/FIFO/DCR macros and configure it in board Setup mac by write_hwaddr

---
 drivers/net/Makefile               |    2 +
 drivers/net/xilinx_ll_temac.c      |  395 ++++++++++++++++++++++++++++++++++
 drivers/net/xilinx_ll_temac.h      |  306 ++++++++++++++++++++++++++
 drivers/net/xilinx_ll_temac_fifo.c |   93 ++++++++
 drivers/net/xilinx_ll_temac_fifo.h |  115 ++++++++++
 drivers/net/xilinx_ll_temac_sdma.c |  415 ++++++++++++++++++++++++++++++++++++
 drivers/net/xilinx_ll_temac_sdma.h |  277 ++++++++++++++++++++++++
 include/netdev.h                   |    2 +
 8 files changed, 1605 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/xilinx_ll_temac.c
 create mode 100644 drivers/net/xilinx_ll_temac.h
 create mode 100644 drivers/net/xilinx_ll_temac_fifo.c
 create mode 100644 drivers/net/xilinx_ll_temac_fifo.h
 create mode 100644 drivers/net/xilinx_ll_temac_sdma.c
 create mode 100644 drivers/net/xilinx_ll_temac_sdma.h

drivers/net/Makefile +2 -0

index d3df82e..14d71a7 100644
@@ -76,6 +76,8 @@ COBJS-$(CONFIG_ULI526X) += uli526x.o
 COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o
 COBJS-$(CONFIG_XILINX_AXIEMAC) += xilinx_axi_emac.o
 COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
+COBJS-$(CONFIG_XILINX_LL_TEMAC) += xilinx_ll_temac.o \
+		xilinx_ll_temac_fifo.o xilinx_ll_temac_sdma.o
 
 COBJS	:= $(sort $(COBJS-y))
 SRCS	:= $(COBJS:.o=.c)

drivers/net/xilinx_ll_temac.c new file +395 -0

395 lines not on this page · raw message

drivers/net/xilinx_ll_temac.h new file +306 -0

306 lines not on this page · raw message

drivers/net/xilinx_ll_temac_fifo.c new file +93 -0

93 lines not on this page · raw message

drivers/net/xilinx_ll_temac_fifo.h new file +115 -0

115 lines not on this page · raw message

drivers/net/xilinx_ll_temac_sdma.c new file +415 -0

415 lines not on this page · raw message

drivers/net/xilinx_ll_temac_sdma.h new file +277 -0

277 lines not on this page · raw message

include/netdev.h +2 -0

index 04d9f75..061c1f1 100644
@@ -100,6 +100,8 @@ int xilinx_axiemac_initialize(bd_t *bis, unsigned long base_addr,
 							unsigned long dma_addr);
 int xilinx_emaclite_initialize(bd_t *bis, unsigned long base_addr,
 							int txpp, int rxpp);
+int xilinx_ll_temac_initialize(bd_t *bis, unsigned long base_addr,
+						int mode, unsigned long ctrl);
 
 /* Boards with PCI network controllers can call this from their board_eth_init()
  * function to initialize whatever's on board.

Signature · 1 line

1.7.0.4

Wolfgang Denk

2011-11-27 19:09 UTC #2 lore raw

PATCH v6 1/1 net: ll_temac: Add LL TEMAC driver to u-boot

Dear Stephan Linz,

Quoted 7 lines · In message <[email protected]> you wrote:

Xilinx LocalLink Tri-Mode Ether MAC driver can be
used by Xilinx Microblaze or Xilinx ppc405/440 in
SDMA and FIFO mode. DCR or XPS bus can be used.
The driver uses and requires MII and PHYLIB.
Signed-off-by Stephan Linz <[email protected]>

...

drivers/net/xilinx_ll_temac.c line 70

Quoted 5 lines

drivers/net/xilinx_ll_temac.c new file +5 -0

+static inline void xps_ll_temac_check_status(struct temac_reg *regs, u32 mask)
+{
+	unsigned timeout = 2000;
+	while (timeout && (!(in_be32(&regs->rdy) & mask)))
+		timeout--;

This has been asked before: what exactly is the limit for the timeout here? 2000 loops though that loop is not exactly a known value. Please add some udelay() here (which is also good for triggering the watchdog, should you have one running).

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (!timeout)
+		printf("%s: Timeout\n", __func__);

Why is this function void when you are know that you might have to handle error situations (like a timeout)?

drivers/net/xilinx_ll_temac.c line 93

Quoted 13 lines

drivers/net/xilinx_ll_temac.c new file +13 -0

+static void xps_ll_temac_hostif_set(struct eth_device *dev, u8 phy_addr,
+					u8 reg_addr, u16 phy_data)
+{
+	struct temac_reg *regs = (struct temac_reg *)dev->iobase;
+
+	out_be32(&regs->lsw, (phy_data & LSW_REGDAT_MASK));
+	out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMWD);
+	out_be32(&regs->lsw,
+		((phy_addr << LSW_PHYAD_POS) & LSW_PHYAD_MASK) |
+		(reg_addr & LSW_REGAD_MASK));
+	out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMAI);
+	xps_ll_temac_check_status(regs, RSE_MIIM_WR);
+}

So what happens here if we have a timeout in xps_ll_temac_check_status()? SHould such an error not be handled?

Ditto for all following functions.

Quoted 1 line

+ * Undirect hostif read to ll_temac.

"Undirect"?? Or "Indirect" ?

"read to"?? Or "read from" ?

Please fix globally.

drivers/net/xilinx_ll_temac.c line 149

Quoted 5 lines

drivers/net/xilinx_ll_temac.c new file +5 -0

+#ifdef DEBUG
+static inline void read_phy_reg(struct eth_device *dev, u8 phy_addr)
+{
+	int j, result;
+	debug("phy%d ", phy_addr);

Please always have one blank line between declarations and code. Please fix globally.

drivers/net/xilinx_ll_temac.c line 238

Quoted 3 lines

drivers/net/xilinx_ll_temac.c new file +3 -0

+	if (ll_temac->ctrlreset)
+		if (ll_temac->ctrlreset(dev))
+			return -1;

Braces needed for multiline statements. Please fix globally.

Quoted 6 lines

drivers/net/xilinx_ll_temac.c new file +6 -0

+/* halt device */
+static void ll_temac_halt(struct eth_device *dev)
+{
+#ifdef ETH_HALTING
+	struct ll_temac *ll_temac = dev->priv;
+

drivers/net/xilinx_ll_temac.c line 273

Quoted 10 lines

drivers/net/xilinx_ll_temac.c new file +10 -0

+	/* Disable Receiver */
+	xps_ll_temac_indirect_set(dev, TEMAC_RCW0, 0);
+
+	/* Disable Transmitter */
+	xps_ll_temac_indirect_set(dev, TEMAC_TC, 0);
+
+	if (ll_temac->ctrlhalt)
+		ll_temac->ctrlhalt(dev);
+#endif
+}

ETH_HALTING is permanently undef'ed, so all this is dead code. Please remove.

drivers/net/xilinx_ll_temac.c line 324

Quoted 5 lines

drivers/net/xilinx_ll_temac.c new file +5 -0

+static int ll_temac_bus_reset(struct mii_dev *bus)
+{
+	debug("Just bus reset\n");
+	return 0;
+}

Can we remove this function? It does not appear to perform any useful operation?

Quoted 1 line

+ * FIXME: This part should going up to arch/powerpc -- but where?

s/going/go/ ?

Quoted 1 line

+/* Check for TX and RX channel errrors. */

s/rrr/rr/ ?

drivers/net/xilinx_ll_temac_sdma.c line 96

Quoted 3 lines

drivers/net/xilinx_ll_temac_sdma.c new file +3 -0
+int ll_temac_halt_sdma(struct eth_device *dev)
+{
+	unsigned timeout = 2000;

See comments above.

drivers/net/xilinx_ll_temac_sdma.c line 117

Quoted 4 lines

drivers/net/xilinx_ll_temac_sdma.c new file +4 -0
+	if (!timeout) {
+		printf("%s: Timeout\n", __func__);
+		return 1;
+	}

Interesting - here you return an error code, above you don't :-(

...

Quoted 5 lines

drivers/net/xilinx_ll_temac_sdma.c new file +5 -0

+
+int ll_temac_send_sdma(struct eth_device *dev,
+				volatile void *buffer, int length)
+{
+	unsigned timeout = 2000;

...

drivers/net/xilinx_ll_temac_sdma.c line 230

Quoted 4 lines

drivers/net/xilinx_ll_temac_sdma.c new file +4 -0
+	if (!timeout)
+		printf("%s: Timeout\n", __func__);
+
+	return 0;

Same issues again. Please fix timout and error handling globally.

Quoted 104 lines

drivers/net/xilinx_ll_temac_sdma.c new file +104 -0

+
+#if defined(CONFIG_XILINX_440) || defined(CONFIG_XILINX_405)
+/* Check for TX and RX channel errrors. */
+static inline int ll_temac_dmac_error(struct eth_device *dev)
+{
+	int err;
+	struct ll_temac *ll_temac = dev->priv;
+	unsigned dmac_ctrl = ll_temac->ctrladdr;
+
+	err = mifdcr_xilinx(dmac_ctrl + TX_CHNL_STS) & CHNL_STS_ERROR;
+	err |= mifdcr_xilinx(dmac_ctrl + RX_CHNL_STS) & CHNL_STS_ERROR;
+	return err;
+}
+
+void ll_temac_init_dmac(struct eth_device *dev)
+{
+	struct ll_temac *ll_temac = dev->priv;
+	unsigned dmac_ctrl = ll_temac->ctrladdr;
+	struct cdmac_bd *rx_dp = ll_temac->rx_dp;
+	struct cdmac_bd *tx_dp = ll_temac->tx_dp;
+
+	memset(tx_dp, 0, sizeof(*tx_dp));
+	memset(rx_dp, 0, sizeof(*rx_dp));
+
+	/* from LL TEMAC (Rx) */
+	rx_dp->phys_buf_p = ll_temac->rx_bp;
+
+	rx_dp->next_p = rx_dp;
+	rx_dp->buf_len = PKTSIZE_ALIGN;
+	flush_cache((u32)rx_dp, sizeof(*tx_dp));
+	flush_cache((u32)rx_dp->phys_buf_p, PKTSIZE_ALIGN);
+
+	/* setup first fd */
+	mitdcr_xilinx(dmac_ctrl + RX_CURDESC_PTR, (u32)rx_dp);
+	mitdcr_xilinx(dmac_ctrl + RX_TAILDESC_PTR, (u32)rx_dp);
+	mitdcr_xilinx(dmac_ctrl + RX_NXTDESC_PTR, (u32)rx_dp);
+
+	/* to LL TEMAC (Tx) */
+	tx_dp->phys_buf_p = ll_temac->tx_bp;
+	tx_dp->next_p = tx_dp;
+
+	flush_cache((u32)tx_dp, sizeof(*tx_dp));
+	mitdcr_xilinx(dmac_ctrl + TX_CURDESC_PTR, (u32)rx_dp);
+}
+
+int ll_temac_halt_dmac(struct eth_device *dev)
+{
+	unsigned timeout = 2000;
+	struct ll_temac *ll_temac = dev->priv;
+	unsigned dmac_ctrl = ll_temac->ctrladdr;
+
+	/*
+	 * Soft reset the DMA
+	 *
+	 * Quote from MPMC documentation: Writing a 1 to this field
+	 * forces the DMA engine to shutdown and reset itself. After
+	 * setting this bit, software must poll it until the bit is
+	 * cleared by the DMA. This indicates that the reset process
+	 * is done and the pipeline has been flushed.
+	 */
+	mitdcr_xilinx(dmac_ctrl + DMA_CONTROL_REG, DMA_CONTROL_RESET);
+	while (timeout && (mifdcr_xilinx(dmac_ctrl + DMA_CONTROL_REG)
+					& DMA_CONTROL_RESET))
+		timeout--;
+
+	if (!timeout) {
+		printf("%s: Timeout\n", __func__);
+		return 1;
+	}
+
+	return 0;
+}
+
+int ll_temac_reset_dmac(struct eth_device *dev)
+{
+	u32 r;
+	struct ll_temac *ll_temac = dev->priv;
+	unsigned dmac_ctrl = ll_temac->ctrladdr;
+
+	/* Soft reset the DMA.  */
+	if (ll_temac_halt_dmac(dev))
+		return 1;
+
+	/* Now clear the interrupts.  */
+	r = mifdcr_xilinx(dmac_ctrl + TX_CHNL_CTRL);
+	r &= ~CHNL_CTRL_IRQ_MASK;
+	mitdcr_xilinx(dmac_ctrl + TX_CHNL_CTRL, r);
+
+	r = mifdcr_xilinx(dmac_ctrl + RX_CHNL_CTRL);
+	r &= ~CHNL_CTRL_IRQ_MASK;
+	mitdcr_xilinx(dmac_ctrl + RX_CHNL_CTRL, r);
+
+	/* Now ACK pending IRQs.  */
+	mitdcr_xilinx(dmac_ctrl + TX_IRQ_REG, IRQ_REG_IRQ_MASK);
+	mitdcr_xilinx(dmac_ctrl + RX_IRQ_REG, IRQ_REG_IRQ_MASK);
+
+	/* Set tail-ptr mode, disable errors for both channels.  */
+	mitdcr_xilinx(dmac_ctrl + DMA_CONTROL_REG,
+			/* Enable use of tail pointer register */
+			DMA_CONTROL_TPE |
+			/* Disable error when 2 or 4 bit coalesce cnt overfl */
+			DMA_CONTROL_RXOCEID |
+			/* Disable error when 2 or 4 bit coalesce cnt overfl */
+			DMA_CONTROL_TXOCEID);

drivers/net/xilinx_ll_temac_sdma.c line 345

Quoted 10 lines

drivers/net/xilinx_ll_temac_sdma.c new file +10 -0

+
+	return 0;
+}
+
+int ll_temac_recv_dmac(struct eth_device *dev)
+{
+	int length;
+	struct ll_temac *ll_temac = dev->priv;
+	unsigned dmac_ctrl = ll_temac->ctrladdr;
+	struct cdmac_bd *rx_dp = ll_temac->rx_dp;

Umm.... this code is more or less exactly the same as for ll_temac_recv_sdma(). Can we please avoid this duplication of code?

Ditto for the other duplicated functions.

Best regards,

Wolfgang Denk

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Q: What do you get when you cross an ethernet with an income statement?
A: A local area networth.

Stephan Linz

author

2011-11-27 20:06 UTC #3 lore raw

PATCH v6 1/1 net: ll_temac: Add LL TEMAC driver to u-boot

Am Sonntag, den 27.11.2011, 20:09 +0100 schrieb Wolfgang Denk:

Quoted 12 lines

Dear Stephan Linz,
Quoted 7 lines · In message <[email protected]> you wrote:
Xilinx LocalLink Tri-Mode Ether MAC driver can be
used by Xilinx Microblaze or Xilinx ppc405/440 in
SDMA and FIFO mode. DCR or XPS bus can be used.
The driver uses and requires MII and PHYLIB.
Signed-off-by Stephan Linz <[email protected]>
...

Quoted 10 lines

drivers/net/xilinx_ll_temac.c line 70
Quoted 5 lines
drivers/net/xilinx_ll_temac.c new file +5 -0
+static inline void xps_ll_temac_check_status(struct temac_reg *regs, u32 mask)
+{
+	unsigned timeout = 2000;
+	while (timeout && (!(in_be32(&regs->rdy) & mask)))
+		timeout--;
This has been asked before: what exactly is the limit for the timeout here? 2000 loops though that loop is not exactly a known value. Please add some udelay() here (which is also good for triggering the watchdog, should you have one running).

Hi Wolfgang,

I'll try to make the timeout handling more precise. Your argument with the watchdog is an interesting issue I forget completely. Thanks for the hint.

Quoted 8 lines

Review
Quoted 4 lines
(quoted diff) +2 -0
+	if (!timeout)
+		printf("%s: Timeout\n", __func__);
Why is this function void when you are know that you might have to handle error situations (like a timeout)?

Quoted 16 lines

drivers/net/xilinx_ll_temac.c line 93

Quoted 13 lines

drivers/net/xilinx_ll_temac.c new file +13 -0

+static void xps_ll_temac_hostif_set(struct eth_device *dev, u8 phy_addr,
+					u8 reg_addr, u16 phy_data)
+{
+	struct temac_reg *regs = (struct temac_reg *)dev->iobase;
+
+	out_be32(&regs->lsw, (phy_data & LSW_REGDAT_MASK));
+	out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMWD);
+	out_be32(&regs->lsw,
+		((phy_addr << LSW_PHYAD_POS) & LSW_PHYAD_MASK) |
+		(reg_addr & LSW_REGAD_MASK));
+	out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMAI);
+	xps_ll_temac_check_status(regs, RSE_MIIM_WR);
+}

So what happens here if we have a timeout in xps_ll_temac_check_status()? SHould such an error not be handled?

Yes we should. I'll fix it.

Quoted 5 lines

Quoted 1 line
+ * Undirect hostif read to ll_temac.
"Undirect"?? Or "Indirect" ?
"read to"?? Or "read from" ?
Please fix globally.

yep.

Quoted 9 lines

drivers/net/xilinx_ll_temac.c line 149
Quoted 6 lines
drivers/net/xilinx_ll_temac.c new file +5 -0
+#ifdef DEBUG
+static inline void read_phy_reg(struct eth_device *dev, u8 phy_addr)
+{
+	int j, result;
+	debug("phy%d ", phy_addr);
Please always have one blank line between declarations and code. Please fix globally.

yep.

Quoted 5 lines

drivers/net/xilinx_ll_temac.c line 238
Quoted 3 lines
drivers/net/xilinx_ll_temac.c new file +3 -0
+	if (ll_temac->ctrlreset)
+		if (ll_temac->ctrlreset(dev))
+			return -1;
Braces needed for multiline statements. Please fix globally.

yep.

Quoted 12 lines

drivers/net/xilinx_ll_temac.c new file +11 -0

+/* halt device */
+static void ll_temac_halt(struct eth_device *dev)
+{
+#ifdef ETH_HALTING
+	struct ll_temac *ll_temac = dev->priv;
+
+	/* Disable Receiver */
+	xps_ll_temac_indirect_set(dev, TEMAC_RCW0, 0);
+
+	/* Disable Transmitter */
+	xps_ll_temac_indirect_set(dev, TEMAC_TC, 0);

Quoted 8 lines

drivers/net/xilinx_ll_temac.c line 273
Quoted 5 lines
drivers/net/xilinx_ll_temac.c new file +5 -0
+
+	if (ll_temac->ctrlhalt)
+		ll_temac->ctrlhalt(dev);
+#endif
+}
ETH_HALTING is permanently undef'ed, so all this is dead code. Please remove.

@Michal: Is there any platform which can not halt the TEMAC? I have no problem with this code but I left ETH_HALTING undefined from original driver code. I would like to remove all the ETH_HALTING statements and hold this code.

Quoted 9 lines

drivers/net/xilinx_ll_temac.c line 324
Quoted 6 lines
drivers/net/xilinx_ll_temac.c new file +5 -0
+static int ll_temac_bus_reset(struct mii_dev *bus)
+{
+	debug("Just bus reset\n");
+	return 0;
+}
Can we remove this function? It does not appear to perform any useful operation?

@Michal: Have you any objections?

@all: What is the meaning of mii_dev bus reset? What is expacted from the perspective of the MII driver code?

Quoted 5 lines

Quoted 2 lines
+ * FIXME: This part should going up to arch/powerpc -- but where?
s/going/go/ ?
s/rrr/rr/ ?

yep.

Quoted 7 lines

drivers/net/xilinx_ll_temac_sdma.c line 96
Quoted 4 lines
drivers/net/xilinx_ll_temac_sdma.c new file +3 -0
+int ll_temac_halt_sdma(struct eth_device *dev)
+{
+	unsigned timeout = 2000;
See comments above.

Quoted 6 lines

drivers/net/xilinx_ll_temac_sdma.c line 117
Quoted 4 lines
drivers/net/xilinx_ll_temac_sdma.c new file +4 -0
+	if (!timeout) {
+		printf("%s: Timeout\n", __func__);
+		return 1;
+	}
Interesting - here you return an error code, above you don't :-(

Grr, ache on my head. I need one more look to the timeouts (see above).

Quoted 13 lines

Same issues again. Please fix timout and error handling globally.
drivers/net/xilinx_ll_temac_sdma.c
Quoted 7 lines
drivers/net/xilinx_ll_temac_sdma.c new file +7 -0
+
+#if defined(CONFIG_XILINX_440) || defined(CONFIG_XILINX_405)
+/* Check for TX and RX channel errrors. */
+static inline int ll_temac_dmac_error(struct eth_device *dev)
+{
+	int err;
+--snip--
Umm.... this code is more or less exactly the same as for ll_temac_recv_sdma(). Can we please avoid this duplication of code?

Quoted 1 line

Ditto for the other duplicated functions.

You are completely right, but for more than one week I have not found any practicable way to avoid duplicate code here. The only really good approach would be to use private function pointers in struct ll_temac for all the in_*/out_* calls we need in sdma code and refactor the DCR code to use its special in_*/out_* functions with the same prototype. This special 'DCR' function can map into indirect DCR access. But unfortunately in_be32() and out_be32() for Microblaze are not real functions. They are CPP defines :-(

I will review arch/microblaze/include/asm/io.h together with Michal. Until then it would be nice if we could keep this code as it is. I will fix the dublicate code if we have in/out functions -- no defines.

Signature · 8 lines

Viele Gr??e,
Stephan Linz
______________________________________________________________________________
MB-Ref: http://www.li-pro.de/xilinx_mb:mbref:start
OpenDCC: http://www.li-pro.net/opendcc.phtml
PC/M: http://www.li-pro.net/pcm.phtml
Sourceforge: http://sourceforge.net/users/slz
Gitorious: https://gitorious.org/~slz

Wolfgang Denk

2011-11-27 22:29 UTC #4 lore raw

PATCH v6 1/1 net: ll_temac: Add LL TEMAC driver to u-boot

Dear Stephan Linz,

Quoted 8 lines · In message <1322424370.3870.370.camel@keto> you wrote:

Quoted 3 lines
ETH_HALTING is permanently undef'ed, so all this is dead code. Please remove.
@Michal: Is there any platform which can not halt the TEMAC? I have no problem with this code but I left ETH_HALTING undefined from original driver code. I would like to remove all the ETH_HALTING statements and hold this code.

I have no idea. But there is no code in mainline that uses ETH_HALTING, and your file #undef's it.

Quoted 3 lines

Quoted 1 line
Interesting - here you return an error code, above you don't :-(
Grr, ache on my head. I need one more look to the timeouts (see above).

ache != ash. I'm nut sure which you prefer, though ;-)

Quoted 10 lines

Quoted 4 lines
Umm.... this code is more or less exactly the same as for ll_temac_recv_sdma(). Can we please avoid this duplication of code?
Ditto for the other duplicated functions.
You are completely right, but for more than one week I have not found any practicable way to avoid duplicate code here. The only really good approach would be to use private function pointers in struct ll_temac for all the in_*/out_* calls we need in sdma code and refactor the DCR code to use its special in_*/out_* functions with the same prototype.

diff'ing the functions it appears there are only very few function pointers that would be needed.

Quoted 3 lines

This special 'DCR' function can map into indirect DCR access. But unfortunately in_be32() and out_be32() for Microblaze are not real functions. They are CPP defines :-(

Change it? Turning them into inline functions should be not too difficult.

Quoted 3 lines

I will review arch/microblaze/include/asm/io.h together with Michal. Until then it would be nice if we could keep this code as it is. I will fix the dublicate code if we have in/out functions -- no defines.

Hm... past experience has shown that the stragey of adding bad code first and then cleaning it up has never worked really well, nor fast. Insisting on clean patches from the beginning is probably more painful, but almost always faster, and more reliable.

Best regards,

Wolfgang Denk

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The first 90% of a project takes 90% of the time, the last 10%  takes
the other 90% of the time.

Stephan Linz

author

2011-11-29 19:55 UTC #5 lore raw

PATCH v6 1/1 net: ll_temac: Add LL TEMAC driver to u-boot

Am Sonntag, den 27.11.2011, 23:29 +0100 schrieb Wolfgang Denk:

Quoted 2 lines

Dear Stephan Linz,

--snip--

Quoted 7 lines

Quoted 4 lines
This special 'DCR' function can map into indirect DCR access. But unfortunately in_be32() and out_be32() for Microblaze are not real functions. They are CPP defines :-(
Change it? Turning them into inline functions should be not too difficult.

Hello Wolfgang,
Hello Michal,

I have sync arch/microblaze/include/asm/io.h with Linux kernel code. Now I ask me whether there is any option to test all the io helper functions in a simple way? Do you know one?

Best regards,
Stephan

Quoted 14 lines

Quoted 4 lines
I will review arch/microblaze/include/asm/io.h together with Michal. Until then it would be nice if we could keep this code as it is. I will fix the dublicate code if we have in/out functions -- no defines.
Hm... past experience has shown that the stragey of adding bad code first and then cleaning it up has never worked really well, nor fast. Insisting on clean patches from the beginning is probably more painful, but almost always faster, and more reliable.
Best regards,
Wolfgang Denk

Signature · 8 lines

Viele Gr??e,
Stephan Linz
______________________________________________________________________________
MB-Ref: http://www.li-pro.de/xilinx_mb:mbref:start
OpenDCC: http://www.li-pro.net/opendcc.phtml
PC/M: http://www.li-pro.net/pcm.phtml
Sourceforge: http://sourceforge.net/users/slz
Gitorious: https://gitorious.org/~slz

Michal Simek

2011-11-28 11:06 UTC #6 lore raw

PATCH v6 1/1 net: ll_temac: Add LL TEMAC driver to u-boot

In reply to Stephan Linz

Quoted 111 lines · Stephan Linz

Am Sonntag, den 27.11.2011, 20:09 +0100 schrieb Wolfgang Denk:
Quoted 12 lines
Dear Stephan Linz,
Quoted 7 lines · In message <[email protected]> you wrote:
Xilinx LocalLink Tri-Mode Ether MAC driver can be
used by Xilinx Microblaze or Xilinx ppc405/440 in
SDMA and FIFO mode. DCR or XPS bus can be used.
The driver uses and requires MII and PHYLIB.
Signed-off-by Stephan Linz <[email protected]>
...
Quoted 9 lines
drivers/net/xilinx_ll_temac.c line 70
Quoted 5 lines
drivers/net/xilinx_ll_temac.c new file +5 -0
+static inline void xps_ll_temac_check_status(struct temac_reg *regs, u32 mask)
+{
+	unsigned timeout = 2000;
+	while (timeout && (!(in_be32(&regs->rdy) & mask)))
+		timeout--;
This has been asked before: what exactly is the limit for the timeout here? 2000 loops though that loop is not exactly a known value. Please add some udelay() here (which is also good for triggering the watchdog, should you have one running).
Hi Wolfgang,
I'll try to make the timeout handling more precise. Your argument with the watchdog is an interesting issue I forget completely. Thanks for the hint.
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+	if (!timeout)
+		printf("%s: Timeout\n", __func__);
Why is this function void when you are know that you might have to handle error situations (like a timeout)?
Quoted 15 lines
drivers/net/xilinx_ll_temac.c line 93
Quoted 13 lines
drivers/net/xilinx_ll_temac.c new file +13 -0
+static void xps_ll_temac_hostif_set(struct eth_device *dev, u8 phy_addr,
+					u8 reg_addr, u16 phy_data)
+{
+	struct temac_reg *regs = (struct temac_reg *)dev->iobase;
+
+	out_be32(&regs->lsw, (phy_data & LSW_REGDAT_MASK));
+	out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMWD);
+	out_be32(&regs->lsw,
+		((phy_addr << LSW_PHYAD_POS) & LSW_PHYAD_MASK) |
+		(reg_addr & LSW_REGAD_MASK));
+	out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMAI);
+	xps_ll_temac_check_status(regs, RSE_MIIM_WR);
+}
So what happens here if we have a timeout in xps_ll_temac_check_status()? SHould such an error not be handled?
Yes we should. I'll fix it.
Quoted 5 lines
Quoted 1 line
+ * Undirect hostif read to ll_temac.
"Undirect"?? Or "Indirect" ?
"read to"?? Or "read from" ?
Please fix globally.
yep.
Quoted 7 lines
drivers/net/xilinx_ll_temac.c line 149
Quoted 5 lines
drivers/net/xilinx_ll_temac.c new file +5 -0
+#ifdef DEBUG
+static inline void read_phy_reg(struct eth_device *dev, u8 phy_addr)
+{
+	int j, result;
+	debug("phy%d ", phy_addr);
Please always have one blank line between declarations and code. Please fix globally.
yep.
Quoted 4 lines
drivers/net/xilinx_ll_temac.c line 238
Quoted 3 lines
drivers/net/xilinx_ll_temac.c new file +3 -0
+	if (ll_temac->ctrlreset)
+		if (ll_temac->ctrlreset(dev))
+			return -1;
Braces needed for multiline statements. Please fix globally.
yep.
Quoted 10 lines
Quoted 10 lines
drivers/net/xilinx_ll_temac.c new file +10 -0
+/* halt device */
+static void ll_temac_halt(struct eth_device *dev)
+{
+#ifdef ETH_HALTING
+	struct ll_temac *ll_temac = dev->priv;
+
+	/* Disable Receiver */
+	xps_ll_temac_indirect_set(dev, TEMAC_RCW0, 0);
+
+	/* Disable Transmitter */
Quoted 8 lines
drivers/net/xilinx_ll_temac.c line 273
Quoted 6 lines
drivers/net/xilinx_ll_temac.c new file +6 -0
+	xps_ll_temac_indirect_set(dev, TEMAC_TC, 0);
+
+	if (ll_temac->ctrlhalt)
+		ll_temac->ctrlhalt(dev);
+#endif
+}
ETH_HALTING is permanently undef'ed, so all this is dead code. Please remove.
@Michal: Is there any platform which can not halt the TEMAC? I have no problem with this code but I left ETH_HALTING undefined from original driver code. I would like to remove all the ETH_HALTING statements and hold this code.
Quoted 7 lines
drivers/net/xilinx_ll_temac.c line 324
Quoted 5 lines
drivers/net/xilinx_ll_temac.c new file +5 -0
+static int ll_temac_bus_reset(struct mii_dev *bus)
+{
+	debug("Just bus reset\n");
+	return 0;
+}
Can we remove this function? It does not appear to perform any useful operation?

Quoted 4 lines

@Michal: Have you any objections?
@all: What is the meaning of mii_dev bus reset? What is expacted from the perspective of the MII driver code?

It was necessary before this patch.

phylib: reset mii bus only if reset handler is registered
sha1: e3a77218a256edbe201112a39beeed8adcabae3f

Currently you can simple remove.

Michal

Signature · 4 lines

Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Michal Simek

2011-11-28 11:11 UTC #7 lore raw

PATCH v6 1/1 net: ll_temac: Add LL TEMAC driver to u-boot

In reply to Stephan Linz

Quoted 98 lines · Stephan Linz

Am Sonntag, den 27.11.2011, 20:09 +0100 schrieb Wolfgang Denk:
Quoted 12 lines
Dear Stephan Linz,
Quoted 7 lines · In message <[email protected]> you wrote:
Xilinx LocalLink Tri-Mode Ether MAC driver can be
used by Xilinx Microblaze or Xilinx ppc405/440 in
SDMA and FIFO mode. DCR or XPS bus can be used.
The driver uses and requires MII and PHYLIB.
Signed-off-by Stephan Linz <[email protected]>
...
Quoted 9 lines
drivers/net/xilinx_ll_temac.c line 70
Quoted 5 lines
drivers/net/xilinx_ll_temac.c new file +5 -0
+static inline void xps_ll_temac_check_status(struct temac_reg *regs, u32 mask)
+{
+	unsigned timeout = 2000;
+	while (timeout && (!(in_be32(&regs->rdy) & mask)))
+		timeout--;
This has been asked before: what exactly is the limit for the timeout here? 2000 loops though that loop is not exactly a known value. Please add some udelay() here (which is also good for triggering the watchdog, should you have one running).
Hi Wolfgang,
I'll try to make the timeout handling more precise. Your argument with the watchdog is an interesting issue I forget completely. Thanks for the hint.
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+	if (!timeout)
+		printf("%s: Timeout\n", __func__);
Why is this function void when you are know that you might have to handle error situations (like a timeout)?
Quoted 15 lines
drivers/net/xilinx_ll_temac.c line 93
Quoted 13 lines
drivers/net/xilinx_ll_temac.c new file +13 -0
+static void xps_ll_temac_hostif_set(struct eth_device *dev, u8 phy_addr,
+					u8 reg_addr, u16 phy_data)
+{
+	struct temac_reg *regs = (struct temac_reg *)dev->iobase;
+
+	out_be32(&regs->lsw, (phy_data & LSW_REGDAT_MASK));
+	out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMWD);
+	out_be32(&regs->lsw,
+		((phy_addr << LSW_PHYAD_POS) & LSW_PHYAD_MASK) |
+		(reg_addr & LSW_REGAD_MASK));
+	out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMAI);
+	xps_ll_temac_check_status(regs, RSE_MIIM_WR);
+}
So what happens here if we have a timeout in xps_ll_temac_check_status()? SHould such an error not be handled?
Yes we should. I'll fix it.
Quoted 5 lines
Quoted 1 line
+ * Undirect hostif read to ll_temac.
"Undirect"?? Or "Indirect" ?
"read to"?? Or "read from" ?
Please fix globally.
yep.
Quoted 7 lines
drivers/net/xilinx_ll_temac.c line 149
Quoted 5 lines
drivers/net/xilinx_ll_temac.c new file +5 -0
+#ifdef DEBUG
+static inline void read_phy_reg(struct eth_device *dev, u8 phy_addr)
+{
+	int j, result;
+	debug("phy%d ", phy_addr);
Please always have one blank line between declarations and code. Please fix globally.
yep.
Quoted 4 lines
drivers/net/xilinx_ll_temac.c line 238
Quoted 3 lines
drivers/net/xilinx_ll_temac.c new file +3 -0
+	if (ll_temac->ctrlreset)
+		if (ll_temac->ctrlreset(dev))
+			return -1;
Braces needed for multiline statements. Please fix globally.
yep.
Quoted 10 lines
Quoted 10 lines
drivers/net/xilinx_ll_temac.c new file +10 -0
+/* halt device */
+static void ll_temac_halt(struct eth_device *dev)
+{
+#ifdef ETH_HALTING
+	struct ll_temac *ll_temac = dev->priv;
+
+	/* Disable Receiver */
+	xps_ll_temac_indirect_set(dev, TEMAC_RCW0, 0);
+
+	/* Disable Transmitter */
Quoted 8 lines
drivers/net/xilinx_ll_temac.c line 273
Quoted 6 lines
drivers/net/xilinx_ll_temac.c new file +6 -0
+	xps_ll_temac_indirect_set(dev, TEMAC_TC, 0);
+
+	if (ll_temac->ctrlhalt)
+		ll_temac->ctrlhalt(dev);
+#endif
+}
ETH_HALTING is permanently undef'ed, so all this is dead code. Please remove.

Quoted 4 lines

@Michal: Is there any platform which can not halt the TEMAC? I have no problem with this code but I left ETH_HALTING undefined from original driver code. I would like to remove all the ETH_HALTING statements and hold this code.

I am not aware about any problem with halting. I had it there for debugging purpose. You can simple remove ETH_HALTING statemets.

Michal

Signature · 4 lines

Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Michal Simek

2011-11-28 11:35 UTC #8 lore raw

In reply to Stephan Linz

Quoted 12 lines · Stephan Linz

@Michal: please, can you test this code on Xilinx PPC4xx platforms and acknowledge the results? I have no access to a ml405 and ml507.
Note: To use the driver on Microblaze, you will need additional platform patches by Michal Simek:
- http://patchwork.ozlabs.org/patch/112535/
- http://patchwork.ozlabs.org/patch/112534/
- http://patchwork.ozlabs.org/patch/112532/
I will collect all related patches on a public bundle:
- http://patchwork.ozlabs.org/bundle/rexut/net-ll_temac/

I will test it on ppc board when none has any objection about the code.

Thanks,
Michal

Signature · 4 lines

Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian