[U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot

[Michal Simek]

Dear Wolfgang Denk,

> In message <4E60B220.6010309 at monstr.eu> you wrote:
>> As I see there is still ugly board/xilinx/common folder and ancient enet driver and i2c
>> driver.
> 
> Indeed, and improvementrs are more than welcome.
> 
>>> Actually even this is incorrect - AFAIK Device Control Registers (DCR)
>>> exist not on all PPC systems, but only on 4xx (and even there only on
>>> some models).  So your code works on a few systems, silently does not
>>> do anything on others, and crashes on yet others with an illegal
>>> instruction.
>> That driver is not definitely for all ppc systems. That IP is available just for
>> Xilinx FPGA where can be possible to use it with Microblaze and xilinx ppc440 (maybe ppc405).
>> That DCR handling, which is implemented in this driver, fits to xilinx ppc440 implementation.
>> Which means that none can add this IP to any other PPC system out of Xilinx FPGA.
> 
> So why not use something like CONFIG_440 in this test, and add an
> #error for anything else?
> 
> Do we actually need this m{f,t}dcr_local() then?

DCR handling is specific for Xilinx ppc440 which means that not all PPC440 can use it.
As you see m{f,t}dcr_local setup handling for it that's why it is neeeded.


> 
>> Sorry I can't see any problem to have driver for specific platform or even to have one driver
>> which supports two completely different platform.
> 
> My issue is that this code silently breaks or crashes when certain
> (undocumented) conditions are not met.  Preventing this seems not to
> bee too difficult: add a comment, make it depend on the right CONFIG_
> settings, and bail out with a clear error message when conditions are
> not met.

Driver is protected by CONFIG_XILINX_LL_TEMAC option which means that
any platform is not silently breaks. You can use it with Microblaze and PPC
and configuration is done (xparameters.h and config.mk files) by u-boot BSP
in connection to Xilinx EDK which check if DCR can be used or not.

> 
> As for the other part of the problem - you try to mix two different
> cases: one where you refer to an index, and one where you refer to an
> address. 

In technical sense it is still address not index. It is different addressing mode.

I have done it because it is much better than a lot of ifdefs. It is more than that
because ppc has dcr up to 4 DMAs but memory controller supports up to 8 DMAs
that's why I think that it is better to support both modes and decide by configuration.

  This obviously doesn't mix well.  If there is no better way
> of doing this, I'd still prefer deriving the index from the offset in
> a struct than deriving the address from an offset - the intention is
> to enable the compiler to perform type checking, which is impossible
> with a typeless base+offset address.

I understand the reasons for that but I can't see any elegant way how to do so.

>>> I don't want to have this in mainline.
>> If this is your decision, I won't send any updated version.
> 
> Attempted extortion?

My god why do you think that it is extortion?
If you don't want to add it to mainline because you think that this driver
is bad/broken/anything, I can do nothing with it and make no sense for me to invest my time to it.
And I am not going to disturb others with it.

Regards,
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