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

[Wolfgang Denk]

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.

> 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.

How do you call code that exposes such behaviour?

I don't want to have this in mainline.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Don't hit a man when he's down - kick him; it's easier.