[U-Boot] [PATCH 05/14] powerpc/qoirq: Add support for FMan ethernet in Independent mode

[Timur Tabi <timur@freescale.com>]

Mingkai Hu wrote:
> +LIB	:= $(obj)libfm.a

Libraries are now .o files, not .a files.  Take a look at the other Makefiles to
see how they've recently changed.

> +u32 fm_muram_alloc(struct fm_muram *mem, u32 size, u32 align)
> +{
> +	u32 ret;
> +	u32 align_mask, off;
> +	u32 save;
> +
> +	align_mask = align - 1;
> +	save = mem->alloc;
> +
> +	if ((off = (save & align_mask)) != 0)
> +		mem->alloc += (align - off);
> +	if ((off = size & align_mask) != 0)
> +		size += (align - off);
> +	if ((mem->alloc + size) >= mem->top) {
> +		mem->alloc = save;
> +		printf("%s: run out of ram.\n", __func__);
> +	}

What exactly happens here?  If you run out of memory, you return a pointer
anyway?  Shouldn't you return NULL here or something?

> +	/* Loop through each microcode. */
> +	for (i = 0; i < firmware->count; i++) {
> +		const struct qe_microcode *ucode = &firmware->microcode[i];
> +
> +		/* Upload a microcode if it's present */
> +		if (ucode->code_offset) {
> +			printf("Fman: Uploading microcode version %u.%u.%u.\n",
> +			       ucode->major, ucode->minor, ucode->revision);
> +			fm->ucode = (void *) CONFIG_SYS_FMAN_FW_ADDR +
> +				ucode->code_offset;

Just FYI, this code will only work on systems that have NOR flash mapped to I/O
space.  If the customer boots from NAND and doesn't have any NOR flash on his
board, then CONFIG_SYS_FMAN_FW_ADDR will be invalid.  In addition, I don't see
any code that passes the value of CONFIG_SYS_FMAN_FW_ADDR to the kernel via
"fman_ucode".

> +	/* setup TxBD */
> +	txbd->buf_ptr_hi = 0;
> +	txbd->buf_ptr_lo = (u32)buf;
> +	txbd->len = len;
> +	__asm__ __volatile__ ("sync");

Why do we sometimes do "asm("sync");" and sometimes we do "__asm__ __volatile__
("sync");"?  And why do we have hard-coded asm anyway?  There is a sync()
function that does this already.


> +	txbd->status = TxBD_READY | TxBD_LAST;
> +	__asm__ __volatile__ ("sync");
> +
> +	/* update TxQD, let RISC to send the packet */
> +	offset_in = muram_readw(&pram->txqd.offset_in);
> +	offset_in += SIZEOFBD;
> +	if (offset_in >= muram_readw(&pram->txqd.bd_ring_size))
> +		offset_in = 0;
> +	muram_writew(&pram->txqd.offset_in, offset_in);
> +	__asm__ __volatile__ ("sync");
> +
> +	/* wait for buffer to be transmitted */
> +	for (i = 0; txbd->status & TxBD_READY; i++) {
> +		udelay(1000);
> +		if (i > 0x10000) {
> +			printf("%s: Tx error\n", dev->name);
> +			return 0;
> +		}
> +	}

If you're going to do a timeout, do it the same way everyone else does.  Start
with the timeout value, and decrement the variable.  Something like this:

	timeout = 0x10000; (why a hex number anyway?)
	while (!(txbd->status & TxBD_READY) && --timeout)
		udelay(1000);

> +static void
> +__def_board_ft_fman_fixup_port(void *blob, char * prop, phys_addr_t pa,
> +				enum fm_port port, int offset)
> +{
> +	return ;

Delete this "return ;".  It doesn't do anything.

-- 
Timur Tabi
Linux kernel developer at Freescale