[U-Boot] [PATCH] net: convert altera_tse to driver model and phylib

[Marek Vasut <marex@denx.de>]

On Monday, October 19, 2015 at 04:36:21 AM, Thomas Chou wrote:

Hi!

> Convert altera_tse to driver model and phylib.
> 
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>  configs/nios2-generic_defconfig             |   2 +
>  doc/device-tree-bindings/net/altera_tse.txt | 112 ++++
>  drivers/net/Kconfig                         |   9 +
>  drivers/net/altera_tse.c                    | 938
> ++++++++++------------------ drivers/net/altera_tse.h                    |
> 283 +++------
>  include/configs/nios2-generic.h             |   8 +
>  6 files changed, 536 insertions(+), 816 deletions(-)
>  create mode 100644 doc/device-tree-bindings/net/altera_tse.txt
> 
> diff --git a/configs/nios2-generic_defconfig

[...]

>  /* sgdma debug - print descriptor */
>  static void alt_sgdma_print_desc(volatile struct alt_sgdma_descriptor
> *desc) {
>  	debug("SGDMA DEBUG :\n");
> -	debug("desc->source : 0x%x \n", (unsigned int)desc->source);
> -	debug("desc->destination : 0x%x \n", (unsigned int)desc->destination);
> -	debug("desc->next : 0x%x \n", (unsigned int)desc->next);
> -	debug("desc->source_pad : 0x%x \n", (unsigned int)desc->source_pad);
> -	debug("desc->destination_pad : 0x%x \n",
> -	      (unsigned int)desc->destination_pad);
> -	debug("desc->next_pad : 0x%x \n", (unsigned int)desc->next_pad);
> -	debug("desc->bytes_to_transfer : 0x%x \n",
> -	      (unsigned int)desc->bytes_to_transfer);
> -	debug("desc->actual_bytes_transferred : 0x%x \n",
> -	      (unsigned int)desc->actual_bytes_transferred);
> -	debug("desc->descriptor_status : 0x%x \n",
> -	      (unsigned int)desc->descriptor_status);
> -	debug("desc->descriptor_control : 0x%x \n",
> -	      (unsigned int)desc->descriptor_control);
> +	debug("desc->source : 0x%x\n", (unsigned int)desc->source);
> +	debug("desc->destination : 0x%x\n", (unsigned int)desc->destination);
> +	debug("desc->next : 0x%x\n", (unsigned int)desc->next);
> +	debug("desc->source_pad : 0x%x\n", desc->source_pad);
> +	debug("desc->destination_pad : 0x%x\n", desc->destination_pad);
> +	debug("desc->next_pad : 0x%x\n", desc->next_pad);
> +	debug("desc->bytes_to_transfer : 0x%x\n", desc->bytes_to_transfer);
> +	debug("desc->actual_bytes_transferred : 0x%x\n",
> +	      desc->actual_bytes_transferred);
> +	debug("desc->descriptor_status : 0x%x\n", desc->descriptor_status);
> +	debug("desc->descriptor_control : 0x%x\n", desc->descriptor_control);
>  }

Looks like a cleanup part of the patch, not a conversion, so you might want
to split this out. Btw. while at it, please drop those (unsigned int) casts.

> -/* This is a generic routine that the SGDMA mode-specific routines
> +/*
> + * This is a generic routine that the SGDMA mode-specific routines
>   * call to populate a descriptor.
>   * arg1	    :pointer to first SGDMA descriptor.
>   * arg2	    :pointer to next  SGDMA descriptor.
> @@ -108,7 +107,6 @@ static void alt_sgdma_construct_descriptor_burst(
>  	 * pointing at this descriptor, it will not run (via the "owned by
>  	 * hardware" bit) until all other descriptor has been set up.
>  	 */
> -
>  	desc->descriptor_control =
>  	    ((ALT_SGDMA_DESCRIPTOR_CONTROL_OWNED_BY_HW_MSK) |
>  	     (generate_eop ?
> @@ -121,18 +119,19 @@ static void alt_sgdma_construct_descriptor_burst(
>  		    );
>  }
> 
> -static int alt_sgdma_do_sync_transfer(volatile struct alt_sgdma_registers
> *dev, -			       volatile struct alt_sgdma_descriptor 
*desc)
> +static int alt_sgdma_do_sync_transfer(
> +	volatile struct alt_sgdma_registers *regs,
> +	volatile struct alt_sgdma_descriptor *desc)

The volatiles are almost certainly wrong, so they should go.

Oh wait, is this driver NOT using any I/O accessors ? :-O

>  {
>  	unsigned int status;
>  	int counter = 0;
> 
>  	/* Wait for any pending transfers to complete */
>  	alt_sgdma_print_desc(desc);
> -	status = dev->status;
> +	status = regs->status;
> 
>  	counter = 0;
> -	while (dev->status & ALT_SGDMA_STATUS_BUSY_MSK) {
> +	while (regs->status & ALT_SGDMA_STATUS_BUSY_MSK) {
>  		if (counter++ > ALT_TSE_SGDMA_BUSY_WATCHDOG_CNTR)
>  			break;
>  	}
> @@ -144,12 +143,12 @@ static int alt_sgdma_do_sync_transfer(volatile struct
> alt_sgdma_registers *dev, * Clear any (previous) status register
> information
>  	 * that might occlude our error checking later.
>  	 */
> -	dev->status = 0xFF;
> +	regs->status = 0xFF;
> 
>  	/* Point the controller at the descriptor */
> -	dev->next_descriptor_pointer = (unsigned int)desc & 0x1FFFFFFF;
> +	regs->next_descriptor_pointer = (unsigned int)desc & 0x1FFFFFFF;

This looks pretty wrong, with the cast and all.

>  	debug("next desc in sgdma 0x%x\n",
> -	      (unsigned int)dev->next_descriptor_pointer);
> +	      (unsigned int)regs->next_descriptor_pointer);
> 
>  	/*
>  	 * Set up SGDMA controller to:
> @@ -157,30 +156,31 @@ static int alt_sgdma_do_sync_transfer(volatile struct
> alt_sgdma_registers *dev, * - Run once a valid descriptor is written to
> controller
>  	 * - Stop on an error with any particular descriptor
>  	 */
> -	dev->control = (ALT_SGDMA_CONTROL_RUN_MSK |
> +	regs->control = (ALT_SGDMA_CONTROL_RUN_MSK |
>  			ALT_SGDMA_CONTROL_STOP_DMA_ER_MSK);
> 
>  	/* Wait for the descriptor (chain) to complete */
> -	status = dev->status;
> +	status = regs->status;
>  	debug("wait for sgdma....");
> -	while (dev->status & ALT_SGDMA_STATUS_BUSY_MSK)
> +	while (regs->status & ALT_SGDMA_STATUS_BUSY_MSK)
>  		;

Eventually, this unbounded wait should be nuked and replaced with a proper
wait with timeout. In case you're up to it, that'd be real nice.

>  	debug("done\n");
> 
>  	/* Clear Run */
> -	dev->control = (dev->control & (~ALT_SGDMA_CONTROL_RUN_MSK));
> +	regs->control = (regs->control & (~ALT_SGDMA_CONTROL_RUN_MSK));
> 
>  	/* Get & clear status register contents */
> -	status = dev->status;
> -	dev->status = 0xFF;
> +	status = regs->status;
> +	regs->status = 0xFF;
> 
>  	/* we really should check if the transfer completes properly */
>  	debug("tx sgdma status = 0x%x", status);
>  	return 0;
>  }

[...]

> -static int tse_eth_rx(struct eth_device *dev)
> +static void tse_eth_rx_setup(struct altera_tse_priv *priv)
> +{
> +	uchar *rx_buf = net_rx_packets[priv->rx_cur & 1];
> +	volatile struct alt_sgdma_descriptor *rx_desc = priv->rx_desc;
> +
> +	invalidate_dcache_range((unsigned long)rx_buf,
> +				(unsigned long)rx_buf + PKTSIZE_ALIGN);
> +	alt_sgdma_construct_descriptor_burst(
> +		&rx_desc[0],
> +		&rx_desc[1],
> +		(unsigned int)0x0,	/* read addr */
> +		(unsigned int *)rx_buf,
> +		0x0,	/* length or EOP */
> +		0x0,	/* gen eop */
> +		0x0,	/* read fixed */
> +		0x0,	/* write fixed or sop */
> +		0x0,	/* read burst */
> +		0x0,	/* write burst */
> +		0x0	/* channel */

You might want to pass the args as a structure instead of having a function
with billion arguments. But that's just a suggestion for improvement.

> +		);
> +
> +	/* setup the sgdma */
> +	alt_sgdma_do_async_transfer(priv->sgdma_rx, &rx_desc[0]);
> +}

[...]

> +static int altera_tse_probe(struct udevice *dev)
> +{
> +	struct eth_pdata *pdata = dev_get_platdata(dev);
> +	struct altera_tse_priv *priv = dev_get_priv(dev);
> +	const void *blob = gd->fdt_blob;
> +	int node = dev->of_offset;
> +	const char *list, *end;
> +	const fdt32_t *cell;
> +	void *base, *desc_mem = NULL;
> +	unsigned long addr, size;
> +	int len, idx;
> +	int ret;
> +
> +	/* decode regs, assume address-cells and size-cells are both one */
> +	list = fdt_getprop(blob, node, "reg-names", &len);
> +	if (!list)
> +		return -ENOENT;
> +	end = list + len;
> +	cell = fdt_getprop(blob, node, "reg", &len);
> +	if (!cell)
> +		return -ENOENT;
> +	idx = 0;
> +	while (list < end) {
> +		addr = fdt_translate_address((void *)blob,
> +					     node, cell + idx);
> +		size = fdt_addr_to_cpu(cell[idx + 1]);
> +		base = ioremap(addr, size);
> +		len = strlen(list);
> +		if (strcmp(list, "control_port") == 0)
> +			priv->mac_dev = base;

fdtdec_get_addr() won't do here?

> +		else if (strcmp(list, "rx_csr") == 0)
> +			priv->sgdma_rx = base;
> +		else if (strcmp(list, "tx_csr") == 0)
> +			priv->sgdma_tx = base;
> +		else if (strcmp(list, "s1") == 0)
> +			desc_mem = base;
> +		idx += 2;
> +		list += (len + 1);
>  	}

[...]