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

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

Hi all,

On 11/09/2013 01:12 PM, Eric Nelson 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

As I mentioned yesterday, I'd like to get some agreement about some
of the fundamentals of how we'll do multi-arch, since they have an
impact on some patches in flight, notably
	http://lists.denx.de/pipermail/u-boot/2013-November/thread.html#166154

As Edward's patch pointed out, the updates to consolidate the
pad names did not address the question of multiple declarations,
and in this document, I propose that (and how) we turn the
declarations into macros.

If that's agreed upon, I have a patch which does this without
any backward incompatibility (this required minor changes to mx6qarm2,
titanium, and mx6qsabreauto).

The other key change is that the DDR configuration files should
change to use macros instead of ".DATA" declarations directly
to allow use in run-time selection.

Again, this is simple enough to do in a backward-compatible way,
but I'd like to get agreement before submitting patches.

At the end of the document, I started to stray into policy and
know there may not be agreement about the "how" bits of this,
so I figured it was time to stop typing and get some feedback.

Please let me know your thoughts.

Regards,


Eric

Dear Eric Nelson,

In message <1384027962-11556-1-git-send-email-eric.nelson@boundarydevices.com> you wrote:
...
> +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.

I think the term "architectures" is greatly misleading here. An
"architecture" is for example ARm or MIPS or Power Architecture.

What you are talking about are mere CPU models.

Please adapt the Subject and commit message.

Thanks.

Best regards,

Wolfgang Denk

Dear Eric Nelson,

In message <527E99CB.7030804@boundarydevices.com> you wrote:
> 
> As I mentioned yesterday, I'd like to get some agreement about some
> of the fundamentals of how we'll do multi-arch, since they have an
> impact on some patches in flight, notably

We should be really precise when using terms like "architecture".
Multi-arch would actually mean that one U-Boot image was capable of
running on - say - both ARM and MIPS processors.

We are not in a shape to even be able to run on all boards of a single
processor family within the same architecture.  Even if others use
Big Words for their marketing documents, we should stick to plain
technical facts and use appropriate names.

Best regards,

Wolfgang Denk

Thanks Wolfgang,

On 11/09/2013 02:16 PM, Wolfgang Denk wrote:
> Dear Eric Nelson,
>
> In message <527E99CB.7030804@boundarydevices.com> you wrote:
>>
>> As I mentioned yesterday, I'd like to get some agreement about some
>> of the fundamentals of how we'll do multi-arch, since they have an
>> impact on some patches in flight, notably
>
> We should be really precise when using terms like "architecture".
> Multi-arch would actually mean that one U-Boot image was capable of
> running on - say - both ARM and MIPS processors.
>
> We are not in a shape to even be able to run on all boards of a single
> processor family within the same architecture.  Even if others use
> Big Words for their marketing documents, we should stick to plain
> technical facts and use appropriate names.
>

Points taken.

Thanks Wolfgang.

On 11/09/2013 02:14 PM, Wolfgang Denk wrote:
> Dear Eric Nelson,
>
> In message <1384027962-11556-1-git-send-email-eric.nelson@boundarydevices.com> you wrote:
> ...
>> +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.
>
> I think the term "architectures" is greatly misleading here. An
> "architecture" is for example ARm or MIPS or Power Architecture.
>
> What you are talking about are mere CPU models.
>
> Please adapt the Subject and commit message.
>

Done. I started typing in "multi-CPU", but that's also overloaded,
so I stumbled on BOMs, since that seems more to the point (that
a board can be assembled using different specific parts).

It still suffers from some awkwardness surrounding what to call
the division between the high-end parts and the low-end.

Choosing words is hard, and suggestions are welcome.

I starting to use the term "SKU", but I'm not sure how commonly
used that term has become.

Regards,


Eric

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.

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). 

> +The register addresses and values are consistent between the first
> +two processors in the list above (i.MX6DQ processors) and the second
> +two (i.MX6DLS for Dual-Lite/Solo).
> +
> +The i.MX6SL (Solo-Lite) processor is not pin compatible, so this 
> +document does not describe support for that variant.
> +
> +Because of the pin-compatibility, a number of manufacturers produce 
> +identical boards with BOM options for two or more of the processors.
> +
> +Similarly, identical boards are offered in a number of different
> +memory layouts, whether by partially populating the DRAM sockets
> +or by populating them with different densities of DDR.
> +
> +By following the conventions described in this document, a board
> +can support each of these options in a single boot image, and
> +decrease the overhead for managing images.
> +
> +Note that adding multi-arch support will add to the size of the
> +bootable image and slow the boot process slightly. If size and
> +speed are critical, a configuration-specific build can be produced
> +that removes this overhead.
> +
> +2. BOOT FLOW
> +------------
> +The boot process for i.MX6 processors begins with execution of a first 
> +level loader in the processor's internal ROM. This loader samples
> +boot pins and on-chip fuses to determine the source of the secondary
> +boot image.
> +
> +The boot image itself consists of a header (the DCD) which describes 
> +the load address and payload (the U-Boot firmware). It also contains
> +a set of register/value pairs used to initialize the CPU prior
> +to execution of U-Boot.
> +
> +The boot image is produced in a final stage of the build process
> +by the imximage tool by processing a configuration (.cfg) file.
> +
> +In a single-architecture, single memory-layout image, the DCD
> +can include DDR memory initialization values and the load address
> +may be DDR directly.
> +
> +In order to support multiple processors, the DCD must contain
> +a load address for the i.MX6's internal RAM (OCRAM) because the 
> +DDR memory speed (at least) will be dependent on the processor 
> +variant. Thankfully, the DCD items needed to load this binary
> +are consistent between all of the processors.
> +
> +For this reason, support for SPL (secondary program loader) is 
> +a requirement in order to support multiple architectures in the 
> +same image. The SPL image will determine the processor variant
> +and memory configuration, configure the IOMUX controller and
> +DDR appropriately, then load either a full version of U-Boot 
> +or an O/S.
> +
> +3. DDR configuration
> +--------------------
> +
> +The DDR configuration data for single architecture boards is defined
> +within .cfg files in the various board directories.
> +
> +As of this writing, most boards use the structure defined in 
> +board/boundary/nitrogen6x/ that separates the pieces of DCD
> +data according to function, with this general form:
> +
> +	#include "ddr-setup.cfg"
> +	#include "1066mhz_4x128mx16.cfg"
> +	#include "clocks.cfg"
> +
> +Note that only the second of these is specific to the CPU
> +variant or memory-layout, and the multi-arch equivalent
> +can simply omit that for later initialization.
> +
> +	#include "ddr-setup.cfg"
> +	#include "clocks.cfg"
> +
> +In order to support the use of the memory configuration
> +files by both the SPL code and the imximage tool, the
> +memory configuration files (1066mhz_4x128mx16.cfg, et cetera)
> +have been converted to use the DCD_REG macro.
> +
> +In other words, this declaration in 1066mhz_4x128mx16.cfg
> +
> +	DCD_REG(MX6_MMDC_P0_MDCFG0, 0x555A7974)
> +
> +will be turned into this by the preprocessor when
> +used by imximage:
> +	
> +	DATA 4, MX6_MMDC_P0_MDCFG0, 0x555A7974
> +
> +and this when used to generate memory configuration tables
> +used by the SPL:
> +	{MX6_MMDC_P0_MDCFG0, 0x555A7974},
> +
> +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.

> +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?

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

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).


> +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)
> +
> +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.

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.

> +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).  

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

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

Thanks and regards,

//Tapani

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

On 11/11/2013 08:12 AM, Eric Nelson wrote:
> Thanks Tapani,
>
> On 11/11/2013 05:03 AM, Tapani wrote:
>>
 >> <snip>
>>
>> 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.
>
Whoops.

I should have said "logic and declarations"...

Hi all,

On 11/11/2013 11:42 AM, Eric Nelson wrote:
> On 11/11/2013 08:12 AM, Eric Nelson wrote:
>> On 11/11/2013 05:03 AM, Tapani wrote:
>>> On Sat,  9 Nov 2013 13:12:42 -0700
>>> Eric Nelson <eric.nelson@boundarydevices.com> wrote:
...
>
> The following is a diff with some updates, and I've attached a complete
> updated version.
>
> Can you tell me whether this bit is better, worse, similar?
>
 > ...
 >
> +Or since the arrays are guaranteed to be the same size, the somewhat
> +simpler form:
>       imx_iomux_v3_setup_multiple_pads(
> -        MX6REF(nitrogen_pads),
> -        ARRAY_SIZE(MX6REF(nitrogen_pads))
> +        is_cpu_type(MXC_CPU_MX6Q)
> +            ? mx6q_nitrogen_pads
> +            : mx6dl_nitrogen_pads,
> +        ARRAY_SIZE(mx6q_nitrogen_pads)
>       );
>
> -N.B. This doesn't work, since ARRAY_SIZE can't be passed this kind of
> -reference...
>

As mentioned in my original e-mail, I wanted to get some feedback
about the most important questions:

1. Whether to turn declarations in mx6q_pins.h/mx6dl_pins.h into macros
2. Whether to double-include the same in mx6-pins.h
3. Whether to define baseline pads (the 90% case) in a header and
double-include it, and
4. Whether to macro-fy the memory layout files like 
1066mhz_4x128mx16.cfg so they can be used by imximage and gcc.

I also started veering off into policy questions that need the
answers to the bits above first.

I did draft some additional notes about the straggling, un-addressed
10% of the pads though, and listed the details below.

Regards,


Eric

---------------------------------------------------------------------
The remaining 10% of references can't be resolved in such a
simple way, and require some additional run-time logic.

In general, these are places where the pad configuration
changes at run-time, and there are two prominent instances
in the code base: ethernet pads and I2C pads.

In the ethernet case, two sets of pad settings are used
to first defines the pads as GPIOs during reset of a PHY,
and the second configures the pads for use as RGMII pins.

It is possible to use the multiple #include approach for
these as well, but there are only six pads for the second
configuration, and duplicating them is not very onerous,
so it is recommended that the initial settings be placed
into "boardname-pads.h" along with the others, and that
the second simply be duplicated:

	static iomux_v3_cfg_t const mx6q_enet2_pads[] = {
		MX6Q_PAD_RGMII_RXC__ENET_RGMII_RXC...
		MX6Q_PAD_RGMII_RD0__ENET_RGMII_RD0...
		...
         };
	static iomux_v3_cfg_t const mx6dl_enet2_pads[] = {
		MX6DL_PAD_RGMII_RXC__ENET_RGMII_RXC...
		MX6DL_PAD_RGMII_RD0__ENET_RGMII_RD0...
		...
         };

The eth_init() routine can then make a choice at run-time between
the two.

The I2C case is a bit different, in that the I2C driver will switch
between the I2C and GPIO modes. GPIO mode is used during recovery
from I2C bus, and the mux and pad settings are defined in
the i2c_pads_info structure:

	struct i2c_pads_info i2c_pad_info0 = {
		.scl = {
			.i2c_mode = MX6_PAD_EIM_D21__I2C1_SCL | PC,
			.gpio_mode = MX6_PAD_EIM_D21__GPIO_3_21 | PC,
		},
		.sda = {
			...

I'm torn about this one. There are a total of 12 pad references (3 I2C
structures with 4 pads per structure) in some board files, and I'm
not sure whether the entire structures should be simply declared twice
or if some other macro configuration is appropriate.

This is common enough (exists in every i.MX6 board) that we should
all be doing it the same way.

Hello all,

On 11/11/2013 12:18 PM, Eric Nelson wrote:
>
 > <snip>
>
> As mentioned in my original e-mail, I wanted to get some feedback
> about the most important questions:
>
> 1. Whether to turn declarations in mx6q_pins.h/mx6dl_pins.h into macros
> 2. Whether to double-include the same in mx6-pins.h
> 3. Whether to define baseline pads (the 90% case) in a header and
> double-include it, and
> 4. Whether to macro-fy the memory layout files like
> 1066mhz_4x128mx16.cfg so they can be used by imximage and gcc.
>
> I also started veering off into policy questions that need the
> answers to the bits above first.
>
> I did draft some additional notes about the straggling, un-addressed
> 10% of the pads though, and listed the details below.
>
> <snip>
 >
> The I2C case is a bit different, in that the I2C driver will switch
> between the I2C and GPIO modes. GPIO mode is used during recovery
> from I2C bus, and the mux and pad settings are defined in
> the i2c_pads_info structure:
>
>      struct i2c_pads_info i2c_pad_info0 = {
>          .scl = {
>              .i2c_mode = MX6_PAD_EIM_D21__I2C1_SCL | PC,
>              .gpio_mode = MX6_PAD_EIM_D21__GPIO_3_21 | PC,
>          },
>          .sda = {
>              ...
>
> I'm torn about this one. There are a total of 12 pad references (3 I2C
> structures with 4 pads per structure) in some board files, and I'm
> not sure whether the entire structures should be simply declared twice
> or if some other macro configuration is appropriate.
>
> This is common enough (exists in every i.MX6 board) that we should
> all be doing it the same way.
>

If you've followed along with all of this, there's another alternative
to consider, which is the approach we've used in our Linux 3.0.35
kernels.

If you look at pads-mx6_nitrogen6x.h in our 3.0.35 kernel tree:
	http://bit.ly/1eECBxm

You'll see something different from what I proposed for
"boardname-pads.h".

Instead of simply containing a series of macro calls, this file
contains complete declarations with the names of each variable
wrapped with the macro MX6NAME():

	static iomux_v3_cfg_t MX6NAME(csi0_sensor_pads)[] = {

MX6NAME is defined based on whether the "FOR_DL_SOLO" macro
is defined.

	#ifdef FOR_DL_SOLO
	#define MX6(a) MX6DL_##a
	#define MX6PAD(a) MX6DL_PAD_##a
	#define MX6NAME(a) mx6dl_solo_##a
	#else
	#define MX6(a) MX6Q_##a
	#define MX6PAD(a) MX6Q_PAD_##a
	#define MX6NAME(a) mx6q_##a
	#endif


And the board file includes the pad header twice:
	http://bit.ly/1dYWpbU

This approach requires a bit more effort to understand, but almost
completely isolates the body of the code from the question of
whether the build is for a single processor variant or supports
multiple variants.

It also addresses the question of what to do with the i2c
and ethernet pad declarations above.

Using this approach, they move into "boardname-pads.h".

Regards,


Eric

Thank you Eric,

this is a comment to your comments. See inline.

< long snip >

> >> +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
> >> +

Actually, it occurred to me yesterday that this kludge maybe solves the drawback 
of our method. Having this we can use our way to setup the pinmuxing without 
having duplicate MX6Q/MX6DL defines at all!

More about that below.

> >
> > 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).
> 

And we have the same, precisely because we named them with suffices :-)

> 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.
>

Maybe you misunderstood me. We both know there are many more iMX6 CPUs 
coming next year (and they are under NDA; but the public Freescale roadmap 
confirms a imx6-next line, so we can safely say that stuff is coming).

My point was that the approach we take to make multi-variant code now
should be scalable to add additional variants in the future. Assuming the
new variants are compatible enough, but maybe requiring their own padconfs.

So we have families of cpus like (just an example): { iMX6Q, iMX6D }, 
{ iMX6S, iMX6DL }, { iMX6SL }, { iMX6x1, iMX6x2 }, { iMX6x3, iMX6x4 }, ...
We should be able to support multi-variant over as many families of cpus
as possible, without having another macro-fu nightmare when we need to
support another new CPU group.

My worry was that your approach could grow in complexity when a hypothetical
iMX6X cpu is to be supported.

> > Short sighted thinking caused the need for this mess to begin with, just trying
> > to not do the same mistake again.
> >
< snip >

> >
> >> +4. IOMUX usage in board files
> >> +-----------------------------
> >> +

< snip >

> >
> > 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?
> 

Ok, for the third time :-) But this time combined with some of your suggestions.

In mx6_pins.h we can do the 20 lines of macro-fu you suggested, to create
the enums for MX6Q_PAD_x + MX6DL_PAD_x or MX6_PAD_x, depending on CPUs to 
support. Or use separate includes with duplicate padconf definitions.

Anyway assume MX6Q_PAD_x and MX6DL_PAD_x definitions are in scope.


Now somewhere (board file? mx6_pins.h?) we need one helper macro:

#define IMX6_SET_PAD(p) \
	if ( is_mx6q ) \
		mxc_iomux_v3_setup_pad(MX6Q_##p); \
	else \
		mxc_iomux_v3_setup_pad(MX6DL_##p)


Using this macro a pad can be set in code, and no need for tables!
(The compiler will do a good job on that, don't worry about the resulting code)

For instance, a board file can now initialize UART1 with:

static __init void edm_cf_imx6_init_uart(void) {
        IMX6_SET_PAD( PAD_CSI0_DAT10__UART1_TXD );
        IMX6_SET_PAD( PAD_CSI0_DAT11__UART1_RXD );
        IMX6_SET_PAD( PAD_EIM_D19__UART1_CTS );
        IMX6_SET_PAD( PAD_EIM_D20__UART1_RTS );

	imx6_add_uart(0, NULL);
}

An example of this architecture is in our kernel board file:
https://github.com/TechNexion/linux/blob/imx-3.0.35-4.1.0/arch/arm/mach-mx6/board-edm_cf_imx6.c

Using this method the boardfile part contains less macro hacks, is clearer, 
no tables, and results in shorter C code than your suggestion. (Ok, the 
clearer part could be an opinion.)

Actually, I am not sure this is mutually exclusive with your suggestion.
(Since we suspect you like your board file as much as we like ours, maybe 
it is better to not mandate how board files should do their pinmuxing?).


Anyway, let's work together on this, so we can avoid the maintenance mess the 
iMX6 products otherwise could become.


regards,

//Tapani

Hi Tapani,

On 11/12/2013 04:05 AM, Tapani wrote:
>
> Thank you Eric,
>
< snip >

>>
>> 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.
>>
>
> Maybe you misunderstood me. We both know there are many more iMX6 CPUs
> coming next year (and they are under NDA; but the public Freescale roadmap
> confirms a imx6-next line, so we can safely say that stuff is coming).
>
> My point was that the approach we take to make multi-variant code now
> should be scalable to add additional variants in the future. Assuming the
> new variants are compatible enough, but maybe requiring their own padconfs.
>
> So we have families of cpus like (just an example): { iMX6Q, iMX6D },
> { iMX6S, iMX6DL }, { iMX6SL }, { iMX6x1, iMX6x2 }, { iMX6x3, iMX6x4 }, ...
> We should be able to support multi-variant over as many families of cpus
> as possible, without having another macro-fu nightmare when we need to
> support another new CPU group.
>
> My worry was that your approach could grow in complexity when a hypothetical
> iMX6X cpu is to be supported.
>

I think this scales linearly with the number of CPU variants.

I'm also not sure that any of the new processors will be pin-compatible,
which is really the only time this effort helps.

< snip >

>>>
>>> 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.t
>>> 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?
>>
>
> Ok, for the third time :-) But this time combined with some of your suggestions.
>
> In mx6_pins.h we can do the 20 lines of macro-fu you suggested, to create
> the enums for MX6Q_PAD_x + MX6DL_PAD_x or MX6_PAD_x, depending on CPUs to
> support. Or use separate includes with duplicate padconf definitions.
>
> Anyway assume MX6Q_PAD_x and MX6DL_PAD_x definitions are in scope.
>
>
> Now somewhere (board file? mx6_pins.h?) we need one helper macro:
>
> #define IMX6_SET_PAD(p) \
> 	if ( is_mx6q ) \
> 		mxc_iomux_v3_setup_pad(MX6Q_##p); \
> 	else \
> 		mxc_iomux_v3_setup_pad(MX6DL_##p)
>
>
> Using this macro a pad can be set in code, and no need for tables!
> (The compiler will do a good job on that, don't worry about the resulting code)
>
> For instance, a board file can now initialize UART1 with:
>
> static __init void edm_cf_imx6_init_uart(void) {
>          IMX6_SET_PAD( PAD_CSI0_DAT10__UART1_TXD );
>          IMX6_SET_PAD( PAD_CSI0_DAT11__UART1_RXD );
>          IMX6_SET_PAD( PAD_EIM_D19__UART1_CTS );
>          IMX6_SET_PAD( PAD_EIM_D20__UART1_RTS );
>
> 	imx6_add_uart(0, NULL);
> }
>

I didn't catch that you were using a global (local) variable
for is_mx6q so the compiler can optimize this, but I would still
prefer an array and a single call to imx_iomux_v3_setup_multiple_pads().

I would also suggest re-visiting the pad setting in each block
(uart, SD, i2c, etc) and consider a single block of pad setup,
regardless of whether it's done with a table or individual calls.

> An example of this architecture is in our kernel board file:
> https://github.com/TechNexion/linux/blob/imx-3.0.35-4.1.0/arch/arm/mach-mx6/board-edm_cf_imx6.c
>
> Using this method the boardfile part contains less macro hacks, is clearer,
> no tables, and results in shorter C code than your suggestion. (Ok, the
> clearer part could be an opinion.)
>
> Actually, I am not sure this is mutually exclusive with your suggestion.
> (Since we suspect you like your board file as much as we like ours, maybe
> it is better to not mandate how board files should do their pinmuxing?).
>

I actually don't like our board file so much, but don't want to thrash
it without a clear(er) direction.

>
> Anyway, let's work together on this, so we can avoid the maintenance mess the
> iMX6 products otherwise could become.
>

Sounds good.

I'm hoping to get some feedback from at least Fabio and Stefano on this.

They've been notably silent, but I'm sure it's because they're busy...

Regards,


Eric