[U-Boot] [PATCH] ne2000: Fix broken build of three boards after CONFIG_NET_MULTI drop

[Bernhard Kaindl]

Am 16.10.2011 21:39, schrieb Mike Frysinger:
> On Sunday 16 October 2011 14:12:57 Bernhard Kaindl wrote:
>> ne2000 wasn't converted to CONFIG_NET_MULTI when the non-multi support was
>> dropped, so boards using it (qemu-mips, shmin, r7780mp) failed to compile
>> for multiple definition of eth_rx() and friends due to old ne2000_base.c.
>
> ah i wrote a patch for this but forgot to post it :/

Argh, I feared you or somebody else might have done it too.

>> - Tested using qemu-mips board,
>> - Tested the two renesas / sh boards r7780mp and shmin to compile again,
>>    and should work.
>
> but i couldn't test it, so this is even better

Be my guest (see doc/README.qemu_mips for everything you want to know):
ln -s u-boot.bin mips_bios.bin; qemu-system-mips -M mips -L . -nographic
qemu-mips # dhcp

>> --- a/board/qemu-mips/qemu-mips.c
>> +++ b/board/qemu-mips/qemu-mips.c
>>
>> +int board_eth_init(bd_t *bis)
>> +{
>> +	return ne2000_initialize();
>> +}
>> --- a/board/shmin/shmin.c
>> +++ b/board/shmin/shmin.c
>>
>> +int board_eth_init(bd_t *bis)
>> +{
>> +	return ne2000_initialize();
>> +}
>
> did you need to include netdev.h in this files for the new ne2000_initialize()
> prototype ?

Thanks, need to add an #include in shmin.c.

Lets really turn on -Werror to once for all put an end to such things.
Lazy maintainers can aways put a CFLAGS += Wno-error into their 
makefiles if they can't fix warnings in a given directory.
At the very least, -Werror-implicit-function-declaration should be used.
Both flags are supported by gcc-2.95.3 or older gcc.gnu.org says.

>> --- a/drivers/net/ne2000_base.c
>> +++ b/drivers/net/ne2000_base.c
>>
>> +int ne2000_initialize(void)
>> +{
>> +	struct eth_device *dev;
>> +
>> +	nic.base = (u8 *) CONFIG_DRIVER_NE2000_BASE;
>> +	nic.data = nic.base + DP_DATA;
>> +	nic.tx_buf1 = START_PG;
>> +	nic.tx_buf2 = START_PG2;
>> +	nic.rx_buf_start = RX_START;
>> +	nic.rx_buf_end = RX_END;
>
> this should be using dev->priv rather than a global "nic" data structure

Understand, but the 2/3 boards using it only have a single ne2k-based 
chip, and as no one actually uses the driver multi-card, I skipped
these larger changes to make it multi-card and use dev->priv everywhere.

>> +	dev = calloc(sizeof(*dev), 1);
>> +	pbuf = malloc(NE2000_RX_BUFFER_SIZE);
>> +	if (dev == NULL || pbuf == NULL)
>> +		return -1;
>
> if dev worked but pbuf failed, this leaks memory

If malloc fails here, either CONFIG_SYS_MALLOC_LEN is to small or malloc 
is broken. Three Ethernet drivers call hang() in this case, which is 
probably best in explaining to the developer who broke it.
I can replace that with "goto error" and give an error message on it
like the other recently merged drivers do in this init situation.

 > also, you should return 0 here not -1

The recently merged drivers (armada100 - heavvy reviewed and you and 
many others, the xilinx_axi_emac(acked by you) and many more return -1 
on malloc error.

>
>> +	if (!get_prom(dev->enetaddr, nic.base))
>> +		return -1;
>> +
>> +	dp83902a_init(dev);
>
> these should probably be in the eth->init step and not here

At first sight, I thought the same and did it the same but then you get

Net:   NE2000
Warning: failed to set MAC address

if you don't call eth_setenv_enetaddr("ethaddr", dev->enetaddr) with
the dev->enetaddr retrieved by get_prom(), so this has to be put here.

The error "Warning: failed to set MAC address" is in this case caused
by ethaddr not being set in env. See the comment to you NAK below.

There are limits to what we can put into eth->init, and if the board
or card has the original MAC address stored in HW (not U-Boot env) like 
the NE2000, the driver has to read the MAC from the HW and set ethaddr 
in env before registering the card, see below:

>> +	eth_setenv_enetaddr("ethaddr", dev->enetaddr);
>
> NAK: implement eth->write_hwaddr, and the driver should only use dev->enetaddr
> rather than touching the env

Wrong NAK: If ethaddr is not set in env, this "return -1" below triggers
and no call to eth->write_hwaddr can be reached (below the return -1):

net/eth.c:

int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
                    int eth_number)
{
         unsigned char env_enetaddr[6];
         int ret = 0;

         if (!eth_getenv_enetaddr_by_index(base_name, eth_number,
                                                       env_enetaddr))
                 return -1;

The NE2000 has the MAC in its PROM/EEPROM, which has to be used for it,
and the code above requires it to be set in the env as well or the error
shown in the previous comment occurs.

Read the call to eth_write_hwaddr() in net/eth.c and what it does.

>> +	/* For PCMCIA support: See doc/README.ne2000 on how to enable */
>> +#ifdef CONFIG_DRIVER_NE2000_CCR
>> +	{
>> +		vu_char *p = (vu_char *) CONFIG_DRIVER_NE2000_CCR;
>> +
>> +		PRINTK("CCR before is %x\n", *p);
>> +		*p = CONFIG_DRIVER_NE2000_VAL;
>> +		PRINTK("CCR after is %x\n", *p);
>> +	}
>> +#endif
>
> i think this should be in ne2k_init

It should not be ne2k_init but moved before calling get_prom() because 
it sets the PCMCIA card configuration register. No board still in 
mainline U-Boot uses an NE2000 in a PCMCIA slot, so this NE2000 in 
PCMCIA code could also be dropped in principle.

>> --- a/include/netdev.h
>> +++ b/include/netdev.h
>>
>> +int ne2000_initialize();
>
> needs to be "(void)"
Ack.

 > -mike

Summary:

 From this review, the needed changes to the patch to be done are:

- #include <netdev.h> in board/shmin/shmin.c to fix warning
- add "void" as argument to function prototype in include/netdev.h
- Move PCMCIA CCR init before get_prom() call or remove it (not used)

Bernhard