Dear Wolfgang Denk,
> In message <4e60da47.4070...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
