[U-Boot] (rather [LONG], sorry) Re: [PATCH 2/5] vf610: refactor DDRMC code

[Albert ARIBAUD]

Hi Stefan,

Le Mon, 13 Jul 2015 21:01:56 +0200, Stefan Agner
<stefan.agner at 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