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

[Wolfgang Denk]

Dear Michal Simek,

In message <4E60DA47.4070301 at monstr.eu> you wrote:
>
> >> 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?

You did not answer this - why not using CONFIG_440 instead of
CONFIG_PPC ?

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

Then maybe you should chose a better name for it, say m?dcr_xilinx()
or so.

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

I can only offer you a solution that seems acceptable to me.

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

The Processor Core User's Manual says for example:

	The DCR number (DCRN) is specified by the Device Control
	Register Immediate Prefix Register (DCRIPR)
	and the DCRF field of the mfdcr instruction.

To me, "number" translates much better into index than into address.

Also, the DCR number are incremented by 1 - if these were addresses in
the common sense, they could only point to 8 bit entities - but the
registers are actually 32 bit wide.


But again, I can only show you what I think could be an acceptable
approach.  If you don't like it, please feel free to develop a better
one.

In any case, I will not accept the current (V3) code.

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

Well, I just proposed one way - not elegant indeed, but I'd be willing
to swallow that.

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

I only complain about a few details of the implementation, and I even
lend you a helping hand by offering specific solutions.  Feel free to
take or to refuse it.  I got better things to do myself, either.


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
The ultimate barrier is one's viewpoint.
                        - Terry Pratchett, _The Dark Side of the Sun_