From mboxrd@z Thu Jan  1 00:00:00 1970
From: Rob Herring <robherring2@gmail.com>
Date: Fri, 02 Dec 2011 16:02:00 -0600
Subject: [U-Boot] [PATCH 1/2] net: add Calxeda xgmac driver
In-Reply-To: <201112021630.34130.vapier@gentoo.org>
References: <1322857309-2662-1-git-send-email-robherring2@gmail.com>
	<1322857309-2662-2-git-send-email-robherring2@gmail.com>
	<201112021630.34130.vapier@gentoo.org>
Message-ID: <4ED94AD8.7020505@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Mike,

Thanks for your quick review.

On 12/02/2011 03:30 PM, Mike Frysinger wrote:
> On Friday 02 December 2011 15:21:48 Rob Herring wrote:
>> --- /dev/null
>> +++ b/drivers/net/calxedaxgmac.c
>>
>> +	writel(value, dev->iobase + XGMAC_CORE_CONFIG);
> 
> you should declare a C struct that represents the hardware's register layout, 
> and then use that rather than iobase+register_offset
> 

Is that a suggestion or u-boot mandate? Because the Linux version of the
driver does it the current way already, it's certainly done both ways in
u-boot drivers already and personally I really don't like structs for
register offsets.

...

>> +	macaddr[1] = readl(dev->iobase + XGMAC_CORE_MACADDR0HI);
>> +	macaddr[0] = readl(dev->iobase + XGMAC_CORE_MACADDR0LO);
>> +	memcpy(dev->enetaddr, macaddr, 6);
> 
> does the initial mac regs really start off with useful info ?

Yes. It contains the only value that will work.

>> +	sprintf(enetvar, id ? "eth%daddr" : "ethaddr", id);
>> +	eth_setenv_enetaddr(enetvar, dev->enetaddr);
> 
> NAK: delete this

PXE boot needs the MAC address to generate filenames and gets it from
the env. See format_mac_pxe function in common/cmd_pxe.c. Should that be
done differently? The user setting a MAC address on our platform won't
work, so using the env setting as an override is not valid.

Rob