Hi Stefan,
Le Mon, 13 Jul 2015 21:01:56 +0200, Stefan Agner stefan.agner@toradex.com a écrit :
Hi Albert, Hi Stefano,
On 10.07.2015 10:09, Stefano Babic wrote:
Hi Albert, Stefan,
On 19/06/2015 19:33, Albert ARIBAUD wrote:
I could probably factor back out the JEDEC settings, but there are still differences in the lists of registers to write between the existing vf610twr/colibri_vf and the new pcm052, especially the PHY regs but elsewhere too, and there are some writes in the driver that the PCM052 does not have.
As I wanted to leave the existing boards strictly unaffected, and as I did not want to start sprinkling '#if defined(some-board)' over the driver code, I went for a fully board-controlled design so that no board could possibly be affected by any future change to the driver.
How about a mix? I could keep the JEDEC and lvl pointers in the DDR controller init call arguments and append "per-boards" CR and PHY arrays. The driver would do the JEDEC writes (thus keeping JEDEC DDR3 additions simple), the LVL writes if not NULL, then the "per-board" CR writes if not NULL, then the current common PHY writes, then the "per-board" PHY writes if not null.
This matches IMHO what we have already tried to do with most of Frescale's i.MXes, putting general code and setting in the arch/cpu/ (or in the imx_common for MX5 and MX6), but letting the board code to write the board specific part.
Some mix seems to me a goog compromise between flexibility and common code.
As far as I understood Alberts proposition it would mean that we write some registers directly from board code right? I'm not sure if this works out for all registers, since some might have JEDEC and Board specific fields in one register...?
Well, it would not be /from/ board code as boards would actually pass descriptions and the writing would occur in the driver within the general write sequence, but I see your point about some register values possibly needing to be constructed from both the jedec struct) /and/ some other source -- see below.
Also, we can hardly avoid that each board will require regiser writes which will be somewhat unique to it. The question is not whether we can avoid that, it is how we should specify that and where we should put that.
Looking throug some CR registers lead me to belive that most settings are exact the same or already covered by the JEDEC struct... The few remaning ones could be part of a renamed ddrmc_lvl_info struct (e.g. ddrmc_cr_info).
I do understand the benefit of passing JEDEC parameters rather that register values, because this allows "working in the problem domain", i.e. using JEDEC values straight from datasheets, and having thdriver to the "heavy lifting", i.e. converting this set of JEDEC values into a consistent set of interrelated register values.
I might understand it for PHY settings when you might know that several PHY registers need values consistent with one another (more below).
Generally, though, I don't think there is added values in turning register values into a struct rather than an (offset, value) pair list which is IMO more versatile.
Take the lvl struct for example: it embodies a 'leveling' abstraction which does not exist in the IP per se, and does not exist outside the IP as JEDEC parameters do; it' artificial. And while it does provide some consistency, it does not even hide all hardware implementation details, so you still need to go see the IP's register definition if you want to fill that struct (as opposed to only having to read the DDR3 datasheet to fill the JEDEC structure). And if you need to go read the register structure, working with (offset, value) pairs makes a simpler driver.
Last point: the driver is supposed to work with various incarnations of the IP, not all of which may have the same set of known, and unknown, registers. I stumbled uplon this problem with the current driver code: it writes some (non-JEDEC) CR registers which my board does not write as far as I can tell, and it does not do some writes my board does.
I could have gone through the driver's 'extra' writes and tested whether they could be done on my board without any ill effect. I could also hve gone through my board's extra writes and seen whether they could be removed without any any ill effect. In fact, I *had* to do some of each, because some of the driver's writes (in the lvl struct notably, but elsewhere too) caused issues.
The problem was, I could not test what effect my changes would have on the other boards that use the driver -- it's not even a question of time: I just don't have these other boards which happen to use the driver that I'm modifying (and I expect that's a pretty common case).
Or I could make my changes so that existing boards using the driver kept writing the registers as they do in the current code and that my board write the registers as it needs to, and keep only the common part in the driver.
This way, I could ensure that existing code would retain the exact behavior it currently expects from the IP it runs on while in the same time getting the exact behavior I expected on my board.
The same for the PHY settings (ddrmc_phy_info). This would lead to a common place where all registers gets written, while having the flexibility to use board specific data... No back and forth between the board file and the common code.
Here too, there is a problem: current code does not write all the registers my code wants written. If I added writes to the driver unconditionally, I would risk breaking existing boards by adding writes to them that they never intended to do. If I added it conditionally, I would add artificial complexity to the driver, basically injecting board-specifics in it. That's why I initially went the "driver provides the tools, board provides the values" route.
Now I do understand that one might want to avoid duplication, which is why I would agree with keeping the existing sequence in the driver and only defining board-specific sequences in the board code.
I did not an exact survey how much of the registers are actually different, but I feel that the difference is not that big that moving the whole initialization to the board files is worth the effort...
OTOH, if the difference is not that big, then keeping it in the board is not that painful either. :)
Seriously, though: There's zero reason we should have board dependency compiled into drivers, since we can always write the driver API so that board-specifics are passed by argument to driver functions, and having a board-agnostic driver is a Good Thing (tm).
Now, what part is board-specific and how it is passed, that's quite an open point.
I personally think that the way we should abstract things, and whether we should abstract them at all, depends on the benefit from an overall SW architecture standpoint.
For instance, I have come to think the JEDEC abstraction is good, because it can be be reused by other (future /or/ existing) drivers, and ecause the JEDEC definition for a given DDR3 chip can be hared across all boards that use it (and then, all can benefit from 'bugfixes', if any, in the chip's JEDEC definition).
The only thing I'm not 100% sure is, as you mention it, whether the IP's JEDEC-related are *purely* JEDEC or whether they also depend in part on the board's HW design (track lengths, buffers, whatever). But even if they do, then we can always pass the existing, pure, JEDEC info in the JEDEC struct, /plus/ HW related info in another argument, and have the driver do the mix. The separation would allow us to keep sharing the pure JEDEC info in all boards that use the same DDR3 chip.
OTOH, structs like the lvl struct, with the driver interpreting them into register writes, seems not to benefit the SW architecture: you can't generalize that to other drivers. You can't share lvl struct values across boards. The lvl struct's only benefit is to ensure that a small group of writes will be consistent, something that the developer will quite probably have checked from the spec anyway; it is prone to introducing restrictions that will inevitably be hit one day, and then we will have to maintain the struct *and* the writing code, and run a risk of affecting existing boards each time we do it.
The (offset, value) sequence is indeed very low-level and does indeed make the board hardware-dependent, but the board already *is*, anyway -- anyone looking at the board's DDR3 init certainly should have read the DDR3 controller (and chip) HW specs -- and the maintenance cost in the is low: the driver won't need any once it knows how to run the sequence properly, existing boards which use it will only need maintenance in case of bugs, and new boards will only need to get their new sequence right without fear of affecting existing code.
-- Stefan
This would keep common parts (JEDEC and minimal settings) in the driver while allowing board their own specific settings -- even overriding the driver settings, since the per-board writes would come last before CR000 is rewritten.
Would that be ok ?
--
Best regards, Stefano Babic
Cordialement, Albert ARIBAUD 3ADEV