[U-Boot] [PATCH RFC] imx: add multi-architecture README

[Eric Nelson <eric.nelson@boundarydevices.com>]

Thanks Tapani,

On 11/11/2013 05:03 AM, Tapani wrote:
>
> Eric,
>
> this documentation is a very good initiative. In overall I agree with what you
> have sketched, and it in many ways what we have demonstrated working in practice.
>
Yeah. You've already gone through two patch submissions and Troy went
through the same over a year ago with our first submission of Nitrogen6.

It's clear that we need to nail this down and I hope a README will be
easier to work with and a bit lighter than patches.

> There are a few question marks I have around your suggestion. Mainly around how
> the pinmuxing is suggested to be done.
>
> See the comments inline.
>
> On Sat,  9 Nov 2013 13:12:42 -0700
> Eric Nelson <eric.nelson@boundarydevices.com> wrote:
>
>> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
>> ---
>>   doc/README.imx6-multi-arch | 254 +++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 254 insertions(+)
>>   create mode 100644 doc/README.imx6-multi-arch
>>
>> diff --git a/doc/README.imx6-multi-arch b/doc/README.imx6-multi-arch
>> new file mode 100644
>> index 0000000..a31718c
>> --- /dev/null
>> +++ b/doc/README.imx6-multi-arch
>> @@ -0,0 +1,254 @@
>> +Supporting multiple architectures on Freescale i.MX6
>> +
>> +This file describes how to support multiple CPU architectures
>> +(i.MX6DQ and i.MX6DLS) in a single U-Boot image.
>> +
>> +Because memory configuration differs between architectures,
>> +auto-configuration of DDR is also covered.
>> +
>> +1. BACKGROUND
>> +-------------
>> +The Freescale i.MX6 processor family contains four processors which are pin
>> +compatible. Refer to http://freescale.com/imx6series for details and reference
>> +manuals, but the highlights from a U-Boot perspective are as follows:
>> +
>> +i.MX6Q	- Quad core, 64-bit DDR1066, 256K OCRAM
>> +i.MX6D	- Dual core, 64-bit DDR1066, 256K OCRAM
>> +i.MX6DL	- Dual core, 64-bit DDR800, 128K OCRAM
>> +i.MX6S	- Single core, 32-bit DDR800, 128K OCRAM
>> +
>> +These processors are also largely register-compatible, but not completely.
>> +In particular, the IOMUX registers for common functions are in different
>> +locations and have different selector values.
>> +
>
> Let's not assume that list supported CPUs is complete yet (and you haven't).
>
>>
 >> <snip>
 >>
>> +3. IOMUX declarations
>> +---------------------
>> +
>> +The declarations inside the header files
>> +	arch/arm/include/asm/arch-mx6/mx6q_pins.h
>> +and
>> +	arch/arm/include/asm/arch-mx6/mx6dl_pins.h
>> +
>> +are used to configure the pads usage for a particular
>> +board design.
>> +
>> +As mentioned earlier, the register addresses and values
>> +are different between the 6DQ and 6DLS processor sets,
>> +and these differences are expressed in two header files:
>> +
>
> The wording is a little imprecise here :-)
>
> Change the formulation to "can be different" (unless you know for sure
> they can never bee the same, even for the upcoming imx6 models).
> The header files contain the padconfigs rather than differences.
>
Okay.

The two sets of registers are different, but there may be
some which are exactly the same.

>> +For i.MX6Q and i.MX6D:
>> +	arch/arm/include/asm/arch-mx6/mx6q_pins.h
>> +
>> +and for i.MX6DL and i.MX6S:
>> +	arch/arm/include/asm/arch-mx6/mx6dls_pins.h
>> +
>> +For example, the SD3_DAT2 pad is used for SD card data
>> +on all currently supported i.MX6 boards.
>> +
>> +On i.MX6DQ, this is selected by writing a zero to the
>> +mux register at address 0x020E02C8. On i.MX6DLS, the
>> +address is 0x020E031C.
>> +
>> +The header files mx6q_pins.h and mx6dls_pins consolidate
>> +the settings through a macro providing a common name
>> +of SD3_DAT2__USDHC3_DAT2:
>> +
>> +	MX6_PAD_DECL(SD3_DAT2__USDHC3_DAT2,...)
>> +
>> +By using the MX6_PAD_DECL macro, this can be expanded
>> +in one of three ways, depending on the declarations of
>> +CONFIG_MX6x by a board file. Valid options are:
>> +
>> +	MX6Q	- single architecture for i.MX6DQ
>> +	MX6DL	- single architecture for i.MX6DL/S
>> +	MX6QDL	- multi-architecture
>> +
>> +In the first two cases, the MX6_PAD_DECL macro will
>> +be expanded into a declararation with the MX6_PAD_
>> +prefix:
>> +	MX6_PAD_name = IOMUX_PAD(...)
>> +
>> +In the last case, the MX6_PAD_DECL macro will be
>> +expanded into two sets of declarations, with the
>> +prefix MX6Q_PAD_ for the i.MX6DQ pads and the
>> +prefix MX6DL_PAD_ for the i.MX6DLS pads.
>> +
>> +This is accomplished by the header file mx6-pins.h:
>> +
>> +	#ifdef CONFIG_MX6QDL
>> +	enum {
>> +		#define MX6_PAD_DECL ...
>> +		#include "mx6q_pins.h"
>> +		
>> +		#define MX6_PAD_DECL ...
>> +		#include "mx6dl_pins.h"
>> +	};
>> +	#elif defined(CONFIG_MX6Q)
>> +	enum {
>> +		#define MX6_PAD_DECL ...
>> +		#include "mx6q_pins.h"
>> +	};
>> +	#elif defined(CONFIG_MX6DL)
>> +	enum {
>> +		#define MX6_PAD_DECL ...
>> +		#include "mx6dl_pins.h"
>> +	};
>> +	#endif
>> +
>
> Opinion: This is a terrible macro kludge to begin with. However, I'm afraid there
> is no solutions completely free macro hacks, but maybe we can have less of them?
>
Agreed, but I don't see a way around it.

If we want to declare each pad once, we either need to always name them
differently (i.e. MX6Q_PAD_padname and MX6DL_PAD_padname) or use a macro
to translate the names.

I think we're all in agreement that each use of a pad reference should
be in a generic form (i.e. shouldn't specify MX6Q_ or MX6DL_) when the
pad is independent of the model.

We have this (single-name for a pad setting regardless of CPU variant)
now for single-model builds (with no macro-fu) precisely because we
named the pad declarations the same (without the Q or DL suffix).

If we try to switch to separate names (MX6Q_PAD/MX6DL_PAD), we have an
immediate need to refactor all board files to replace the simple
macro names to something else.

The ~20 lines of code above provide a means of backward-compatibility.

As I side benefit, I like being able to use word-selection to grab the
entire generic pad reference such as SD2_DAT1__USHDC2_DAT1
from this declaration:

	MX6_PAD_DECL(SD2_DAT1__USDHC2_DAT1,...)

and paste it into the spot where it's used:
	MX6_PAD_DEF(SD2_DAT1__USDHC2_DAT1)

(As opposed to having to hand-edit to remove the MX6Q_PAD_ prefix
from one of the declarations)

> Technical point: Could you clarify how this approach scales? There are still
> new imx6 models to be released (imx6-next).
>

It's always tough to tell until we hash through the reference manuals.

> Short sighted thinking caused the need for this mess to begin with, just trying
> to not do the same mistake again.
>
> I think it is better to have one set of macro definitions for DL, Q, x1, x2,...
> and leave the current definitions untouched for compatibility with current boards.
> The price of duplicate definitions is less than extensive macro-messing. (Opinion,
> again).

Please, no!

Going through these pads many times to try and make them consistent
between MX6Q_ and MX6DL_ was time consuming, and we've had enough
cases of small but important bits not being propagated everywhere.

I'd rather see us tackle the re-factoring needed to rename the pads
than duplicate them all.

>
>> +4. IOMUX usage in board files
>> +-----------------------------
>> +
>> +The machinery described above is sufficient to allow a set of
>> +pad registers to be defined for a specific architecture:
>> +
>> +	static iomux_v3_cfg_t const mypads[] = {
>> +		MX6_PAD_x,
>> +		...
>> +	};
>> +
>> +or multiple architectures:
>> +	static iomux_v3_cfg_t const mx6q_pads[] = {
>> +		MX6Q_PAD_x,
>> +		...
>> +	};
>> +
>> +	static iomux_v3_cfg_t const mx6dl_pads[] = {
>> +		MX6DL_PAD_x,
>> +		...
>> +	};
>> +
>> +In practice, 90% of the references to pads are in these
>> +types of static arrays, and mostly separated by usage
>> +(ethernet pads in a different array from UART pads).
>> +
>> +Going forward, it is recommended that these be consolidated
>> +instead by architecture, such that all pads that apply to
>> +both i.MX6DQ and i.MX6DLS architectures are defined in
>> +"boardname-pads.h" with macros of this form:
>> +	MX6_PAD_DEF(PAD_REFERENCE)
>> +

Re-reading this, I suggest that MX6_PAD_REF (reference) instead of
MX6_PAD_DEF (definition) is a better name.

>> +And that this file be included twice when being used in a
>> +multi-architecture build.
>> +
>> +e.g.
>> +	static iomux_v3_cfg_t const mx6q_nitrogen_pads[] = {
>> +		#define MX6_PAD_DEF(PAD_DEF) MX6Q_PAD_#PAD_DEF,
>> +		#include "nitrogen6x-pads.h"
>> +	};
>> +	static iomux_v3_cfg_t const mx6dl_nitrogen_pads[] = {
>> +		#define MX6_PAD_DEF(PAD_DEF) MX6DL_PAD_#PAD_DEF,
>> +		#include "nitrogen6x-pads.h"
>> +	}};
>> +
>> +Doing this allows the bulk of the pads to be defined in a
>> +single place.
>> +
>
> Opinion: Maybe call these files something else than header files, since
> they are rather some kind of .pad files.
>

Works for me.

> Also I do not like this overuse of the C preprocessor, the mess created
> easily gets unreadable. Maybe it already is...? :-)
 >
> Even with this description, it is not immediately obvious how it works.
>

I think I jumped ahead too much. I'm going to re-work this section
a bit and I'll forward the results under separate cover.

My hope is that by reading this file, the reader will come away with
a clear picture of what to do.

>> +For pads that are specific to i.MX6DQ or i.MX6DLS, it is
>> +recommended that they be defined directly in the board file.
>> +
>> +Finally, the convenience macro MX6REF(x) allows run-time
>> +selection of a variable based on the CPU type on which
>> +the reference is made:
>> +
>> +	imx_iomux_v3_setup_multiple_pads(
>> +		MX6REF(nitrogen_pads),
>> +		ARRAY_SIZE(MX6REF(nitrogen_pads))
>> +	);
>> +
>> +N.B. This doesn't work, since ARRAY_SIZE can't be passed this
>> +kind of reference...
>> +
>
> We have suggested an alternative solution, but somehow nobody seem
> to notice. We avoid almost all the preprocessor messing, and have the
> definitions only once. And it would scale for even more cpu types.
> Admittedly, the drawback is duplicate padconf macro definitions
> (or having to convert the existing boards padconfigs).
>
Somehow I missed it. What I recall involved duplicating code and data.

Can you explain?

> In my opinion (and experience) the duplicate definitions is worth it.
>

Again, I'd prefer a big-bang refactoring to duplicate declarations.

> Regardless, how we end up doing this -- this documentation is an
> excellent initiative and discussion point.
>

I'm glad to hear that.

We all want this to get straightened out so we can push forward for
what our customers are after.

Regards,


Eric