Dear Wolfgang Denk,
In message 4E60DA47.4070301@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