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

[Michal Simek]

Dear Wolfgang Denk,

Wolfgang Denk wrote:
> Dear Michal Simek,
> 
> In message <4E609682.8030701 at monstr.eu> you wrote:
>>>>>>>> +static void sdma_out_be32(struct ll_priv *priv, u32 offset, u32 val)
>>>>>>>> +{
>>>>>>>> +	if (priv->mode & DCR_BIT)
>>>>>>>> +		mtdcr_local(priv->ctrl + offset, val);
>>>>>>>> +	else
>>>>>>>> +		out_be32((u32 *)(priv->ctrl + offset * 4), val);
>>>>>>>> +}
>>> ...
> ...
>>>> The first reg for DMA2 accessed trough DCR is at 0xB0, the second at 0xB1, etc..
>>> This is indeed a good example, as it shows how terribly broken your
>>> code is.
>>>
>>> See function sdma_out_be32() above.  It is suppose to write a 32 bit
>>> value ("u32 val") as a 32 bit entity in big endian mode ("_be32") to
>>> some device register - but the register addresses are (1) not aligned
>>> to 32 bit boundaries and (2) not even 32 bits apart.
>> I think you misunderstand what there is written.
> 
> Maybe.  Maybe even I want to misunderstand it.  The problem is that
> the code does not prevent such misunderstanding.
> 
> There are many shortcomings of that code.

I think that this is the reason why we have review process, don't we?

Especially this driver is 2-3 years old and it is used by many our customers.
It is only my effort to clear xilinx drivers/platforms.

As I see there is still ugly board/xilinx/common folder and ancient enet driver and i2c
driver.

> 
>> DCR is defined just for PPC right now because none wanted to do it for Microblaze.
> 
> 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.

> 
> How do you call code that exposes such behaviour?

If I look at drivers/net folder there are a lot of examples like that.
None expect that altera_tse/bfin_mac/tsec will be possible to use with all systems.
Maybe you expect that they can be use on the same architecture but this is Xilinx FPGA.
It is up to you how you want to compose your system.

IRC tsec is used just for Freescale PPC and this ll_temac driver is just used for xilinx microblaze
and xilinx ppc.

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.

I saw that there are some drivers in arch/<cpu>/ folders but this is not that case because
can be possible to use it for microblaze and xilinx ppc.

This ll_temac driver is just another net IP like emaclite is. Emaclite driver is possible to use
on Microblaze and xilinx ppc systems and in near future with arm on Xilinx zynq platform.


> I don't want to have this in mainline.

If this is your decision, I won't send any updated version.

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