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

[Michal Simek]

Dear Wolfgang Denk,

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

Probably the best is CONFIG_XILINX_440.

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

np.

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

seems?


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

DCRN is number but for my case I use 0 for register "address" and 1 for value.
To be accurate - 0 is indirect dcr address reg, and 1 is indirect dcr access reg.
(Have changed that magic value to macros according technical documentation).


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

And agree that DCR number is incremented by 1.

On www.xilinx.com/support/documentation/user_guides/ug200.pdf page 261
is written that it is address that's why I use address.

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

sure.

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

You mean that array of structs, right?

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

Sure.

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