net: ll_temac: Add LL TEMAC driver to u-boot

Add axi_ethernet driver for little-endian Microblaze.

RX/TX BDs and rxframe buffer are shared among all axi_ethernet MACs. Only one MAC can work in one time.

Signed-off-by Michal Simek <[email protected]>

+/* Mask used to verify certain PHY features (or register contents)
+ * in the register above:

+ *  0x1000: 10Mbps full duplex support
+ *  0x0800: 10Mbps half duplex support
+ *  0x0008: Auto-negotiation support

+ */

+static int IsRxReady(struct eth_device *dev)

Dear Michal Simek,

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

Add axi_ethernet driver for little-endian Microblaze.

RX/TX BDs and rxframe buffer are shared among all axi_ethernet MACs. Only one MAC can work in one time.

Signed-off-by Michal Simek <[email protected]>

Quoted 6 lines

+/* Mask used to verify certain PHY features (or register contents)
+ * in the register above:

+ *  0x1000: 10Mbps full duplex support
+ *  0x0800: 10Mbps half duplex support
+ *  0x0008: Auto-negotiation support

+ */

Incorrect multiline comment style. Please fix globally.

drivers/net/xilinx_axi_emac.c line 173

Quoted 7 lines

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

+	u32 timeout = 200;
+	/* Wait till MDIO interface is ready to accept a new transaction. */
+	while (timeout && (!(in_be32(&regs->mdio_mcr)
+						& XAE_MDIO_MCR_READY_MASK)))
+		timeout--;

Multiline statement - braches needed.

Also, add some udelay() or similar to have a defined length of the timeout. Please fix globally.

drivers/net/xilinx_axi_emac.c line 312

Quoted 10 lines

drivers/net/xilinx_axi_emac.c new file +8 -0

+/* STOP DMA transfers */
+static void axiemac_halt(struct eth_device *dev)
+{
+	struct axidma_priv *priv = dev->priv;
+
+	/* Stop the hardware */
+	priv->dmatx->control &= ~XAXIDMA_CR_RUNSTOP_MASK;
+	priv->dmarx->control &= ~XAXIDMA_CR_RUNSTOP_MASK;

Please ALWAYS use I/O accessors to access device registers. Please fix globally.

Quoted 2 lines

+static int IsRxReady(struct eth_device *dev)

As requested before: Please get rid of these CamelCaps identifiers!!!

drivers/net/xilinx_axi_emac.c line 568

Quoted 9 lines

drivers/net/xilinx_axi_emac.c new file +7 -0

+static int axiemac_miiphy_read(const char *devname, uchar addr,
+							uchar reg, ushort *val)
+{
+	struct eth_device *dev = eth_get_dev();
+	*val = phyread(dev, addr, reg);
+	debug("axiemac: Read MII 0x%x, 0x%x, 0x%x\n", addr, reg, *val);
+	return 0;

Please separate declarations and code by a single blank line. Please fix globally.

Why does this function return int when you uncondsitionally always return 0 only?

drivers/net/xilinx_axi_emac.c line 578

Quoted 11 lines

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

+}
+
+static int axiemac_miiphy_write(const char *devname, uchar addr,
+							uchar reg, ushort val)
+{
+	struct eth_device *dev = eth_get_dev();
+	debug("axiemac: Write MII 0x%x, 0x%x, 0x%x\n", addr, reg, val);
+	phywrite(dev, addr, reg, val);
+	return 0;
+}

Ditto.

drivers/net/xilinx_axi_emac.c line 584

Quoted 5 lines

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

+static int axiemac_bus_reset(struct mii_dev *bus)
+{
+	debug("axiemac: Bus reset\n");
+	return 0;
+}

Ditto.

LL Temac driver can be used by microblaze, xilinx ppc405/440 in sdma and fifo mode. DCR or XPS bus can be used.

The driver uses and requires PHYLIB.

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

+static void sdma_out_be32(struct ll_priv *priv, u32 offset, u32 val)
+{
+	if (priv->mode & DCR_BIT)
+		mtdcr_local(priv->ctrl + offset, val);
+	else

Dear Michal Simek,

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

LL Temac driver can be used by microblaze, xilinx ppc405/440 in sdma and fifo mode. DCR or XPS bus can be used.

The driver uses and requires PHYLIB.

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

+static void sdma_out_be32(struct ll_priv *priv, u32 offset, u32 val)
+{
+	if (priv->mode & DCR_BIT)
+		mtdcr_local(priv->ctrl + offset, val);
+	else

drivers/net/xilinx_ll_temac.c line 188

Quoted 10 lines

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

+		out_be32((u32 *)(priv->ctrl + offset * 4), val);
+}
+
+static u32 sdma_in_be32(struct ll_priv *priv, u32 offset)
+{
+	if (priv->mode & DCR_BIT)
+		return mfdcr_local(priv->ctrl + offset);
+
+	return in_be32((u32 *)(priv->ctrl + offset * 4));
+}

Can we please get rid of these functions? As mentioned many, many times before, we discourage all use of "base address plus offset" to access any device registers etc.

These functions here re-introduce such accesses, and this is something I will not accept.

drivers/net/xilinx_ll_temac.c line 339

Quoted 4 lines

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

+	/* Soft reset the DMA.  */
+	sdma_out_be32(priv, DMA_CONTROL_REG, DMA_CONTROL_RESET);
+	while (sdma_in_be32(priv, DMA_CONTROL_REG) & DMA_CONTROL_RESET)
+		;

I think this has been pointed out before: we do not accept such potentially endless loops. Please fix this (and integrate theother previously made review remarks). Thanks.

drivers/net/xilinx_ll_temac.c line 409

Quoted 3 lines

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

+	do {
+		flush_cache((u32)&tx_bd, sizeof(tx_bd));
+	} while (!(tx_bd.stat & BDSTAT_COMPLETED_MASK));

Ditto. Please fix globally.

drivers/net/xilinx_ll_temac.c line 432

Quoted 5 lines

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

+	/* Read out the packet info and start the DMA
+	   onto the second buffer to enable the ethernet rx
+	   path to run in parallel with sw processing
+	   packets.  */

Incorrect multiline comment style; please fix globally.

Quoted 21 lines

Quoted 6 lines

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

+static void sdma_out_be32(struct ll_priv *priv, u32 offset, u32 val)
+{
+	if (priv->mode & DCR_BIT)
+		mtdcr_local(priv->ctrl + offset, val);
+	else

drivers/net/xilinx_ll_temac.c line 188

Quoted 10 lines

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

+		out_be32((u32 *)(priv->ctrl + offset * 4), val);
+}
+
+static u32 sdma_in_be32(struct ll_priv *priv, u32 offset)
+{
+	if (priv->mode & DCR_BIT)
+		return mfdcr_local(priv->ctrl + offset);
+
+	return in_be32((u32 *)(priv->ctrl + offset * 4));
+}

Can we please get rid of these functions? As mentioned many, many times before, we discourage all use of "base address plus offset" to access any device registers etc.

Quoted 2 lines

These functions here re-introduce such accesses, and this is something I will not accept.

Ok. How to do it?

For bus access it is necessary to use 4B offsets for DCR just 1B
and one system can contains two MACs where the first use 4B offset and the second 1B.

Dear Michal Simek,

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

Quoted 21 lines

Quoted 5 lines

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

+static void sdma_out_be32(struct ll_priv *priv, u32 offset, u32 val)
+{
+	if (priv->mode & DCR_BIT)
+		mtdcr_local(priv->ctrl + offset, val);
+	else

drivers/net/xilinx_ll_temac.c line 188

Quoted 10 lines

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

+		out_be32((u32 *)(priv->ctrl + offset * 4), val);
+}
+
+static u32 sdma_in_be32(struct ll_priv *priv, u32 offset)
+{
+	if (priv->mode & DCR_BIT)
+		return mfdcr_local(priv->ctrl + offset);
+
+	return in_be32((u32 *)(priv->ctrl + offset * 4));
+}

Can we please get rid of these functions? As mentioned many, many times before, we discourage all use of "base address plus offset" to access any device registers etc.

These functions here re-introduce such accesses, and this is something I will not accept.

Quoted 5 lines

Ok. How to do it?

For bus access it is necessary to use 4B offsets for DCR just 1B
and one system can contains two MACs where the first use 4B offset and the second 1B.

I don't think your description here matches the code above. With "1 byte offsets" you would be doing all unaligned bus accesses.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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

+static void sdma_out_be32(struct ll_priv *priv, u32 offset, u32 val)
+{
+	if (priv->mode & DCR_BIT)
+		mtdcr_local(priv->ctrl + offset, val);
+	else
+		out_be32((u32 *)(priv->ctrl + offset * 4), val);
+}

On PPC system with DCR is special connection between memory controller through DCR bus. Handling is done with mfdcr_local and mtdcr_local functions.

DMA : Sdma address ranges (www.xilinx.com/support/documentation/user_guides/ug200.pdf page 261 and 299)

0   : 0x80-0x90
1   : 0x98-0xA8
2   : 0xB0-0xC0
3   : 0xC8-0xD8

The first reg for DMA2 accessed trough DCR is at 0xB0, the second at 0xB1, etc..

Dear Michal Simek,

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

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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

+static void sdma_out_be32(struct ll_priv *priv, u32 offset, u32 val)
+{
+	if (priv->mode & DCR_BIT)
+		mtdcr_local(priv->ctrl + offset, val);
+	else
+		out_be32((u32 *)(priv->ctrl + offset * 4), val);
+}

...

Quoted 10 lines

On PPC system with DCR is special connection between memory controller through DCR bus. Handling is done with mfdcr_local and mtdcr_local functions.

DMA : Sdma address ranges (www.xilinx.com/support/documentation/user_guides/ug200.pdf page 261 and 299)

0   : 0x80-0x90
1   : 0x98-0xA8
2   : 0xB0-0xC0
3   : 0xC8-0xD8

The first reg for DMA2 accessed trough DCR is at 0xB0, the second at 0xB1, etc..

This is indeed a good example, as it shows how terribly broken your code is.

See function sdma_out_be32() above. It is suppose to write a 32 bit value ("u32 val") as a 32 bit entity in big endian mode ("_be32") to some device register - but the register addresses are (1) not aligned to 32 bit boundaries and (2) not even 32 bits apart.

Quoted 9 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

(quoted diff) +7 -0

+static void sdma_out_be32(struct ll_priv *priv, u32 offset, u32 val)
+{
+	if (priv->mode & DCR_BIT)
+		mtdcr_local(priv->ctrl + offset, val);
+	else
+		out_be32((u32 *)(priv->ctrl + offset * 4), val);
+}

...

Quoted 9 lines

Quoted 1 line

The first reg for DMA2 accessed trough DCR is at 0xB0, the second at 0xB1, etc..

This is indeed a good example, as it shows how terribly broken your code is.

See function sdma_out_be32() above. It is suppose to write a 32 bit value ("u32 val") as a 32 bit entity in big endian mode ("_be32") to some device register - but the register addresses are (1) not aligned to 32 bit boundaries and (2) not even 32 bits apart.

I think you misunderstand what there is written.

DCR is defined just for PPC right now because none wanted to do it for Microblaze.

Dear Michal Simek,

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

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

(quoted diff) +7 -0

+static void sdma_out_be32(struct ll_priv *priv, u32 offset, u32 val)
+{
+	if (priv->mode & DCR_BIT)
+		mtdcr_local(priv->ctrl + offset, val);
+	else
+		out_be32((u32 *)(priv->ctrl + offset * 4), val);
+}

...

...

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

The first reg for DMA2 accessed trough DCR is at 0xB0, the second at 0xB1, etc..

This is indeed a good example, as it shows how terribly broken your code is.

See function sdma_out_be32() above. It is suppose to write a 32 bit value ("u32 val") as a 32 bit entity in big endian mode ("_be32") to some device register - but the register addresses are (1) not aligned to 32 bit boundaries and (2) not even 32 bits apart.

Quoted 1 line

I think you misunderstand what there is written.

Maybe. Maybe even I want to misunderstand it. The problem is that the code does not prevent such misunderstanding.

There are many shortcomings of that code.

Quoted 2 lines

DCR is defined just for PPC right now because none wanted to do it for Microblaze.

Actually even this is incorrect - AFAIK Device Control Registers (DCR) exist not on all PPC systems, but only on 4xx (and even there only on some models). So your code works on a few systems, silently does not do anything on others, and crashes on yet others with an illegal instruction.

How do you call code that exposes such behaviour?

I don't want to have this in mainline.

As I see there is still ugly board/xilinx/common folder and ancient enet driver and i2c driver.

Quoted 5 lines

Actually even this is incorrect - AFAIK Device Control Registers (DCR) exist not on all PPC systems, but only on 4xx (and even there only on some models). So your code works on a few systems, silently does not do anything on others, and crashes on yet others with an illegal instruction.

That driver is not definitely for all ppc systems. That IP is available just for Xilinx FPGA where can be possible to use it with Microblaze and xilinx ppc440 (maybe ppc405). That DCR handling, which is implemented in this driver, fits to xilinx ppc440 implementation. Which means that none can add this IP to any other PPC system out of Xilinx FPGA.

Sorry I can't see any problem to have driver for specific platform or even to have one driver which supports two completely different platform.

Quoted 1 line

I don't want to have this in mainline.

If this is your decision, I won't send any updated version.

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

As I see there is still ugly board/xilinx/common folder and ancient enet driver and i2c driver.

Indeed, and improvementrs are more than welcome.

Quoted 9 lines

Quoted 5 lines

Actually even this is incorrect - AFAIK Device Control Registers (DCR) exist not on all PPC systems, but only on 4xx (and even there only on some models). So your code works on a few systems, silently does not do anything on others, and crashes on yet others with an illegal instruction.

That driver is not definitely for all ppc systems. That IP is available just for Xilinx FPGA where can be possible to use it with Microblaze and xilinx ppc440 (maybe ppc405). That DCR handling, which is implemented in this driver, fits to xilinx ppc440 implementation. Which means that none can add this IP to any other PPC system out of Xilinx FPGA.

So why not use something like CONFIG_440 in this test, and add an #error for anything else?

Do we actually need this m{f,t}dcr_local() then?

Quoted 3 lines

Sorry I can't see any problem to have driver for specific platform or even to have one driver which supports two completely different platform.

My issue is that this code silently breaks or crashes when certain (undocumented) conditions are not met. Preventing this seems not to bee too difficult: add a comment, make it depend on the right CONFIG_ settings, and bail out with a clear error message when conditions are not met.

As for the other part of the problem - you try to mix two different
cases: one where you refer to an index, and one where you refer to an address.

of doing this, I'd still prefer deriving the index from the offset in a struct than deriving the address from an offset - the intention is to enable the compiler to perform type checking, which is impossible with a typeless base+offset address.

Quoted 2 lines

Quoted 1 line

I don't want to have this in mainline.

If this is your decision, I won't send any updated version.

Attempted extortion?

Quoted 8 lines

Quoted 5 lines

That driver is not definitely for all ppc systems. That IP is available just for Xilinx FPGA where can be possible to use it with Microblaze and xilinx ppc440 (maybe ppc405). That DCR handling, which is implemented in this driver, fits to xilinx ppc440 implementation. Which means that none can add this IP to any other PPC system out of Xilinx FPGA.

So why not use something like CONFIG_440 in this test, and add an #error for anything else?

Quoted 1 line

Do we actually need this m{f,t}dcr_local() then?

DCR handling is specific for Xilinx ppc440 which means that not all PPC440 can use it. As you see m{f,t}dcr_local setup handling for it that's why it is neeeded.

Quoted 5 lines

My issue is that this code silently breaks or crashes when certain (undocumented) conditions are not met. Preventing this seems not to bee too difficult: add a comment, make it depend on the right CONFIG_ settings, and bail out with a clear error message when conditions are not met.

Driver is protected by CONFIG_XILINX_LL_TEMAC option which means that any platform is not silently breaks. You can use it with Microblaze and PPC and configuration is done (xparameters.h and config.mk files) by u-boot BSP in connection to Xilinx EDK which check if DCR can be used or not.

Quoted 3 lines

As for the other part of the problem - you try to mix two different
cases: one where you refer to an index, and one where you refer to an address.

In technical sense it is still address not index. It is different addressing mode.

This obviously doesn't mix well. If there is no better way

Quoted 4 lines

of doing this, I'd still prefer deriving the index from the offset in a struct than deriving the address from an offset - the intention is to enable the compiler to perform type checking, which is impossible with a typeless base+offset address.

I understand the reasons for that but I can't see any elegant way how to do so.

If you don't want to add it to mainline because you think that this driver
is bad/broken/anything, I can do nothing with it and make no sense for me to invest my time to it.

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

Quoted 6 lines

Quoted 4 lines

That driver is not definitely for all ppc systems. That IP is available just for Xilinx FPGA where can be possible to use it with Microblaze and xilinx ppc440 (maybe ppc405). That DCR handling, which is implemented in this driver, fits to xilinx ppc440 implementation. Which means that none can add this IP to any other PPC system out of Xilinx FPGA.

So why not use something like CONFIG_440 in this test, and add an #error for anything else?

You did not answer this - why not using CONFIG_440 instead of CONFIG_PPC ?

Quoted 4 lines

Quoted 2 lines

Do we actually need this m{f,t}dcr_local() then?

DCR handling is specific for Xilinx ppc440 which means that not all PPC440 can use it. As you see m{f,t}dcr_local setup handling for it that's why it is neeeded.

Then maybe you should chose a better name for it, say m?dcr_xilinx() or so.

Quoted 6 lines

Quoted 6 lines

My issue is that this code silently breaks or crashes when certain (undocumented) conditions are not met. Preventing this seems not to bee too difficult: add a comment, make it depend on the right CONFIG_ settings, and bail out with a clear error message when conditions are not met.

Quoted 4 lines

Driver is protected by CONFIG_XILINX_LL_TEMAC option which means that any platform is not silently breaks. You can use it with Microblaze and PPC and configuration is done (xparameters.h and config.mk files) by u-boot BSP in connection to Xilinx EDK which check if DCR can be used or not.

I can only offer you a solution that seems acceptable to me.

Quoted 4 lines

Quoted 3 lines

As for the other part of the problem - you try to mix two different
cases: one where you refer to an index, and one where you refer to an address.

In technical sense it is still address not index. It is different addressing mode.

The Processor Core User's Manual says for example:

The DCR number (DCRN) is specified by the Device Control Register Immediate Prefix Register (DCRIPR) and the DCRF field of the mfdcr instruction.

To me, "number" translates much better into index than into address.

Also, the DCR number are incremented by 1 - if these were addresses in the common sense, they could only point to 8 bit entities - but the registers are actually 32 bit wide.

But again, I can only show you what I think could be an acceptable approach. If you don't like it, please feel free to develop a better one.

In any case, I will not accept the current (V3) code.

Quoted 7 lines

This obviously doesn't mix well. If there is no better way

Quoted 4 lines

of doing this, I'd still prefer deriving the index from the offset in a struct than deriving the address from an offset - the intention is to enable the compiler to perform type checking, which is impossible with a typeless base+offset address.

I understand the reasons for that but I can't see any elegant way how to do so.

Well, I just proposed one way - not elegant indeed, but I'd be willing to swallow that.

Quoted 3 lines

If you don't want to add it to mainline because you think that this driver
is bad/broken/anything, I can do nothing with it and make no sense for me to invest my time to it.

I only complain about a few details of the implementation, and I even lend you a helping hand by offering specific solutions. Feel free to take or to refuse it. I got better things to do myself, either.

Quoted 2 lines

I can only offer you a solution that seems acceptable to me.

seems?

Quoted 9 lines

Quoted 6 lines

This obviously doesn't mix well. If there is no better way

Quoted 4 lines

of doing this, I'd still prefer deriving the index from the offset in a struct than deriving the address from an offset - the intention is to enable the compiler to perform type checking, which is impossible with a typeless base+offset address.

I understand the reasons for that but I can't see any elegant way how to do so.

Well, I just proposed one way - not elegant indeed, but I'd be willing to swallow that.

You mean that array of structs, right?