[U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

Mike Frysinger wrote:
>>>>> also, you should change the "hang()" to "return 0" in the init func.
>>>> Are you sure return 0 which should mean success. Anything different from
>>>> 0 seems to me relevant.
>>> as i said, the initialize function is not returning "success" or
>>> "failure". it is returning "# of devices registered".  if you cannot
>>> register any, you should return 0.  having the boot process fail because
>>> of network issues doesnt make much sense when u-boot can do quite a lot
>>> without the network. including updating itself via other means.
>> Interesting.
>> 1. you talked about hang() in initialize function(not dev->init) and for me
>> xilinx_axiemac_initialize Initialize function is called from 
>> board_eth_init which is called from eth_initialize(eth.c)
>>
>> There is this part of code
>> 	if (board_eth_init != __def_eth_init) {
>> 		if (board_eth_init(bis) < 0)
>> 			printf("Board Net Initialization Failed\n");
>>
>> If initialization failed the return value is < 0.
>> That's why hang() should be changed to return -1 and doesn't matter how
>> many device there are.
> 
> this detail isnt currently ironed out, so if you wanted to change the "hang()" 
> into "return -1" or "return 0", that is fine.

ok.

> 
>> If you write:
>>  >>> also, you should change the "hang()" to "return 0" in the init func.
>>
>> (hang is only in xilinx_axiemac_initialize) and should be changed which I
>> agree.
>>
>> If you propose any change which I should do, I expect that if you are focus
>> on blackfin that you have done that changes in all blackfin eth drivers.
>> For example in bfin_mac.c where hang is also used.
> 
> incorrect code in other drivers (including bfin_mac) is not justification for 
> adding incorrect code to new drivers.  bfin_mac.c's call to hang() is wrong 
> too in the current code base.

I am not arguing that I shouldn't fix this driver. I was/am still open to any suggestions which help 
me to improve microblaze code/drivers.
I am just saying that if you know that there is something wrong (even in blackfin part) and it is 
easy to fix it, then it will be good to fix it.

> 
>>>> I maintain emaclite driver and none tell me this that's why the process
>>>> is so slow. I believe if you release that documentation, which you are
>>>> talking about, then others will clean/test their drivers.
>>> the behavior i describe isnt a decision i made.  it was made by the
>>> previous net maintainer and agreed upon by others in the discussion.
>> Ok. If that decision was made than I expect that should be written
>> somewhere in doc. I know it is boring to write any documentation but I
>> expect that if any decision was made then is common that general code will
>> be changed.
> 
> obviously that is true, but docs only get written/updated when someone is so 
> inclined to do the work.  the existing doc only exists because i felt like 
> writing at the time.  ive never been the net maintainer and thus "obligated" 
> to write net documentation.  i just got tired of people doing it wrong and no 
> one knowing what should be going on.
> 
>> 1. hang() -> return -1
> 
> ok
> 
>> 2. driver initialize function (setup dev functions, driver name, priv, etc)
>> return -1 - if initialize failed
>> return 0 - on success
> 
> no, "return 1" when the device has successfully registered
> 
>> 3. dev->init
>> return -1 - if init failed
>> return 0 - on success
> 
> ok
> 
>> (here you are saying should be return # of devices)
> 
> no, i think you confused "initialize" with "init" in my feedback

ok. From my point of view is better do initialize only for one device
and not to registered all device in the driver. IMHO this functionality should be done by
board specific net function in board_eth_init.
Not sure if this covers all cases which can happen. But anyway if the driver initialize several 
drivers with the same type then you are losing flexibility.
I see at least one reason why will be better not to do it:
Initialize only one device which can speedup bootup time and maybe security reasons.

Number of registered device can be counted through eth_register function or similar
If you and others like to return number of devices I will return it that's no problem.
But I don't want to loose flexibility which is the reason why FPGAs are great.

To finish this discuss - here is what you think that it is correct.
ad 2)
return -1 - if initialize failed
return 0 - never return
return >0 - # of devices

Is it ok for you?

> 
>> 4. dev->recv
>> return -1 - failed
>> return 0 - packet not received
>> return >0 - success - packet lenght
> 
> "return 0" is still "success" in the sense that there is nothing to do, but 
> that nuance doesnt matter

agree.

> 
>> 5. dev->send
>> return -1 - failed
>> return 0 - success
>>
>> 6. dev->write_hwaddr
>> return -1 - failed
>> return 0 - success
> 
> ok

ok. great.
Michal


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian