[U-Boot] SPL boot on iMX6

[U-Boot] SPL boot on iMX6

[Tapani Utriainen]

Hello all,

we would like to have some comments how to architecture some patches we would 
like to submit.

Background:

We have got SPL boot working, circumventing the need to have separate u-boot
binaries depending on iMX6 CPU type and memory configuration. This allows us
to have one (say) bootable SD card for all versions of our new SoMs.

Formatting the patches presents us with some questions:

1. Padconfigs. For some reason the existing padconfiguration macros are set
compile time depending on target cpu variant. Hence the need to add new 
macros (or smth) so the binary can configure the pads for many cpu variants.
This would cause duplicate and redundant sets of pad configuration macros 
for the imx6. Is there any alternative to this, more than rewriting code to 
comply with cpu specific padconfigs?

2. Is there a minimum set of features that should be supported by new boards?
(Thinking of features like fdt, fat or network boot).
It seems that most imx6 based boards have some standard features enabled by
default, but some of those we haven't tested on our new board.

Any comments regarding this are welcome,

//Tapani

[U-Boot] SPL boot on iMX6

[Stefano Babic]

Hi Tapani,

On 26/08/2013 09:17, Tapani Utriainen wrote:
> 
> Hello all,
> 
> we would like to have some comments how to architecture some patches we would 
> like to submit.

Nice

> 
> Background:
> 
> We have got SPL boot working, circumventing the need to have separate u-boot
> binaries depending on iMX6 CPU type and memory configuration. This allows us
> to have one (say) bootable SD card for all versions of our new SoMs.

Agree on this - we have already discussed in the past about this point,
and the way to get a single image for the different i.MX6 SOCs is to add
SPL support, moving part of the initialization stuff away from the DCD
table managed directly from the bootrom.

> 
> Formatting the patches presents us with some questions:
> 
> 1. Padconfigs. For some reason the existing padconfiguration macros are set
> compile time depending on target cpu variant. Hence the need to add new 
> macros (or smth) so the binary can configure the pads for many cpu variants.
> This would cause duplicate and redundant sets of pad configuration macros 
> for the imx6. Is there any alternative to this, more than rewriting code to 
> comply with cpu specific padconfigs?

Macros wee added exactly in the time they needed, and maybe a global
look was missing.

However, can you provide much more detail about this ? Which macros, in
which files ?

> 
> 2. Is there a minimum set of features that should be supported by new boards?
> (Thinking of features like fdt, fat or network boot).

No, there is not - the board maintainer is responsible to add the
features the board can. You can add support for a restricted number of
feature and later add a patch to full support the board.

> It seems that most imx6 based boards have some standard features enabled by
> default, but some of those we haven't tested on our new board.

You should dropped them, and add them again once they are tested and if
you really want to have them.

Best regards,
Stefano


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] SPL boot on iMX6

[Tapani]

On Mon, 26 Aug 2013 09:42:30 +0200
Stefano Babic <sbabic@denx.de> wrote:

> Hi Tapani,
> 
> > 
> > 1. Padconfigs. For some reason the existing padconfiguration macros are set
> > compile time depending on target cpu variant. Hence the need to add new 
> > macros (or smth) so the binary can configure the pads for many cpu variants.
> > This would cause duplicate and redundant sets of pad configuration macros 
> > for the imx6. Is there any alternative to this, more than rewriting code to 
> > comply with cpu specific padconfigs?
> 
> Macros wee added exactly in the time they needed, and maybe a global
> look was missing.
> 
> However, can you provide much more detail about this ? Which macros, in
> which files ?
> 

The macros I refer to is the MX6_PAD_ ones. The semantics of them depends on
the target cpu. See arch/arm/include/asm/arch-mx6/mx6-pins.h

> 
> You should dropped them, and add them again once they are tested and if
> you really want to have them.
> 
Good. Then we agree there.

//Tapani

[U-Boot] SPL boot on iMX6

[Fabio Estevam]

Hi Tapani,

On Mon, Aug 26, 2013 at 4:17 AM, Tapani Utriainen <tapani@technexion.com> wrote:
>
> Hello all,
>
> we would like to have some comments how to architecture some patches we would
> like to submit.
>
> Background:
>
> We have got SPL boot working, circumventing the need to have separate u-boot

Could you please post your mx6 spl patches so that we can advance in
this discussion?

Thanks,

Fabio Estevam

[U-Boot] SPL boot on iMX6

[Stefano Babic]

Hi Tapani,

On 26/08/2013 13:12, Tapani wrote:
>>
>> Macros wee added exactly in the time they needed, and maybe a global
>> look was missing.
>>
>> However, can you provide much more detail about this ? Which macros, in
>> which files ?
>>
> 
> The macros I refer to is the MX6_PAD_ ones. The semantics of them depends on
> the target cpu. See arch/arm/include/asm/arch-mx6/mx6-pins.h

Ok - these files are not thought to be used in the same binary, we have
to change something, taking into account we should remain compatible
without breaking the currently supported boards.

Let's start with some proposals. Maybe you have already introduced a
CONFIG_ switch, because at the moment only one SOC per image is
supported, and one of MX6Q, MX6DL must be set. We have also the same
issue with -ddr files (mx6q-ddr and mx6dl-ddr). Let's say we add a
CONFIG_MX6_MULTI to support all SocS at the same time.

Then we could change the file you mention adding a suffix to each pin.
For example, in mx6q_pins.h we could add something like this:

#ifdef CONFIG_MX6_MULTI
#define PAD_SUFFIX _6Q
#else
#define PAD_SUFFIX
#endif

And we add the macro to each pin, such as
enum {
        MX6_PAD_SD2_DAT1__USDHC2_DAT1##PAD_SUFFIX

In this way we could have different names only if we support multiple
SOCs. We need then some accessors to get the right pin, something like
mx6_pin(soc_type, pin_name), that returns the right configuration. Of
course, this is a very first draft, and someone else can start with
different proposals.

Generally I would avoid to convert the enums into tables, because they
will increase the footprint for each board.

>> You should dropped them, and add them again once they are tested and if
>> you really want to have them.
>>
> Good. Then we agree there.

Right.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] SPL boot on iMX6

[Eric Nelson]

On 08/26/2013 06:33 AM, Stefano Babic wrote:
> Hi Tapani,
>
> On 26/08/2013 13:12, Tapani wrote:
>>>
>>> Macros wee added exactly in the time they needed, and maybe a global
>>> look was missing.
>>>
>>> However, can you provide much more detail about this ? Which macros, in
>>> which files ?
>>>
>>
>> The macros I refer to is the MX6_PAD_ ones. The semantics of them depends on
>> the target cpu. See arch/arm/include/asm/arch-mx6/mx6-pins.h
>
> Ok - these files are not thought to be used in the same binary, we have
> to change something, taking into account we should remain compatible
> without breaking the currently supported boards.
>
> Let's start with some proposals. Maybe you have already introduced a
> CONFIG_ switch, because at the moment only one SOC per image is
> supported, and one of MX6Q, MX6DL must be set. We have also the same
> issue with -ddr files (mx6q-ddr and mx6dl-ddr). Let's say we add a
> CONFIG_MX6_MULTI to support all SocS at the same time.
>
> Then we could change the file you mention adding a suffix to each pin.
> For example, in mx6q_pins.h we could add something like this:
>
> #ifdef CONFIG_MX6_MULTI
> #define PAD_SUFFIX _6Q
> #else
> #define PAD_SUFFIX
> #endif
>
> And we add the macro to each pin, such as
> enum {
>          MX6_PAD_SD2_DAT1__USDHC2_DAT1##PAD_SUFFIX
>
> In this way we could have different names only if we support multiple
> SOCs. We need then some accessors to get the right pin, something like
> mx6_pin(soc_type, pin_name), that returns the right configuration. Of
> course, this is a very first draft, and someone else can start with
> different proposals.
>
:)

This is where we started on i.MX6, with prefixes MX6Q and MX6DL.
See commit cfb8b9d.

> Generally I would avoid to convert the enums into tables, because they
> will increase the footprint for each board.
>

Functionally, we still need table(s) for any image which supports either
variant so the proper set of pads are configured.

See this for an example
	http://lists.denx.de/pipermail/u-boot/2012-October/136394.html

The construct used in that patch set was to define FOR_DL_SOLO,
then include the pad file.
	#ifdef CONFIG_MX6Q
	#include "pads.h"
	#endif
	#if defined(CONFIG_MX6DL) || defined(CONFIG_MX6S)
	#define FOR_DL_SOLO
	#include "pads.h"
	#endif

Troy's implementation used a naming convention of mx6q_X
and mx6dl_solo_X such that a board supporting both would have
variables

	static iomux_v3_cfg_t mx6q_usdhc3_pads = ...

followed by

	static iomux_v3_cfg_t mx6dl_solo_usdhc3_pads = ...

Some other data structures were also duplicated with the
same naming convention (see i2c_pads).

Regards,


Eric

[U-Boot] SPL boot on iMX6

[Tapani Utriainen]

On Mon, 26 Aug 2013 15:33:56 +0200
Stefano Babic <sbabic@denx.de> wrote:

> Hi Tapani,
> 
> > 
> > The macros I refer to is the MX6_PAD_ ones. The semantics of them depends on
> > the target cpu. See arch/arm/include/asm/arch-mx6/mx6-pins.h
> 
> Ok - these files are not thought to be used in the same binary, we have
> to change something, taking into account we should remain compatible
> without breaking the currently supported boards.
> 
> Let's start with some proposals. 

Yes, this is the productive approach.

> Maybe you have already introduced a
> CONFIG_ switch, because at the moment only one SOC per image is
> supported, and one of MX6Q, MX6DL must be set. We have also the same
> issue with -ddr files (mx6q-ddr and mx6dl-ddr). Let's say we add a
> CONFIG_MX6_MULTI to support all SocS at the same time.
> 
> Then we could change the file you mention adding a suffix to each pin.
> For example, in mx6q_pins.h we could add something like this:
> 
> #ifdef CONFIG_MX6_MULTI
> #define PAD_SUFFIX _6Q
> #else
> #define PAD_SUFFIX
> #endif
> 
> And we add the macro to each pin, such as
> enum {
>         MX6_PAD_SD2_DAT1__USDHC2_DAT1##PAD_SUFFIX
> 
> In this way we could have different names only if we support multiple
> SOCs. We need then some accessors to get the right pin, something like
> mx6_pin(soc_type, pin_name), that returns the right configuration. Of
> course, this is a very first draft, and someone else can start with
> different proposals.
> 

Your suggestion is similar to what I would first think of, but you do
the extra kludging to make it work with the current syntax. My approach
would be to introduce new namings in parallel to the current ones
(similar, if not the same, as the linux kernel uses) until most imx6
boards have been cleaned to use the new namings (which might take a while).

For the accessor macro, our experiences from the Wandboard kernel have been
very positive with the following:

To set the above mentioned pad, the board file does:

	IMX6_SETUP_PAD( SD2_DAT1__USDHC2_DAT1 );

where the macro IMX6_SETUP_PAD is defined as:

#define IMX6_SETUP_PAD(p) \
	if (cpu_is_mx6q()) \
		mxc_iomux_v3_setup_pad(MX6Q_PAD_##p);\
	else \
		mxc_iomux_v3_setup_pad(MX6DL_PAD_##p)


Of course the syntax can be different. One experience is that this de-clutters 
the board file (compared with having arrays and arrays of padconf definitions).

The catch is, I guess, that the generated code could be a little larger than 
otherwise, but defining cpu_is_mx6q() with __attribute__((pure)) (or similar) should 
cause gcc to optimize away most of the comparisons and calls to cpu_is_mx6q().

//Tapani

[U-Boot] SPL boot on iMX6

[Stefano Babic]

Hi Eric,

On 26/08/2013 16:23, Eric Nelson wrote:
> 
> Functionally, we still need table(s) for any image which supports either
> variant so the proper set of pads are configured.
> 
> See this for an example
>     http://lists.denx.de/pipermail/u-boot/2012-October/136394.html
> 

Ok - what I meant is to avoid to convert the static definitions (enums)
in the header in a sort of tables. I agree that using tables in the
board code is needed and makes the code more readable using
imx_iomux_v3_setup_multiple_pads().

> The construct used in that patch set was to define FOR_DL_SOLO,
> then include the pad file.
>     #ifdef CONFIG_MX6Q
>     #include "pads.h"
>     #endif
>     #if defined(CONFIG_MX6DL) || defined(CONFIG_MX6S)
>     #define FOR_DL_SOLO
>     #include "pads.h"
>     #endif
> 
> Troy's implementation used a naming convention of mx6q_X
> and mx6dl_solo_X such that a board supporting both would have
> variables
> 
>     static iomux_v3_cfg_t mx6q_usdhc3_pads = ...
> 
> followed by
> 
>     static iomux_v3_cfg_t mx6dl_solo_usdhc3_pads = ...
> 

ok, this is a solution. Let's wait for next Tapani's patch and when we
start the discussion ;-)

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] SPL boot on iMX6

[Stefano Babic]

Hi Tapani,

On 27/08/2013 06:07, Tapani Utriainen wrote:

> Your suggestion is similar to what I would first think of, but you do
> the extra kludging to make it work with the current syntax. My approach
> would be to introduce new namings in parallel to the current ones
> (similar, if not the same, as the linux kernel uses) until most imx6
> boards have been cleaned to use the new namings (which might take a while).

We will see when you post patches. My concern is that very often old
code remains unchanged and nobody takes care of it. If there is no
compatibility approach, we should try to clean up all boards.

Can you also take a look at the Troy's patches, mentioned by Eric ?

> 
> For the accessor macro, our experiences from the Wandboard kernel have been
> very positive with the following:
> 
> To set the above mentioned pad, the board file does:
> 
> 	IMX6_SETUP_PAD( SD2_DAT1__USDHC2_DAT1 );
> 
> where the macro IMX6_SETUP_PAD is defined as:
> 
> #define IMX6_SETUP_PAD(p) \
> 	if (cpu_is_mx6q()) \
> 		mxc_iomux_v3_setup_pad(MX6Q_PAD_##p);\
> 	else \
> 		mxc_iomux_v3_setup_pad(MX6DL_PAD_##p)
> 

Sure, I thought something like that, too.

> 
> Of course the syntax can be different. One experience is that this de-clutters 
> the board file (compared with having arrays and arrays of padconf definitions).

It is interesting how much the footprint increases.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] SPL boot on iMX6

[Eric Nelson]

Hi Tapani,

On 08/26/2013 09:07 PM, Tapani Utriainen wrote:
 > On Mon, 26 Aug 2013 15:33:56 +0200
 > Stefano Babic <sbabic@denx.de> wrote:
 >
 >> Hi Tapani,
 >>
 >>>
 >>> The macros I refer to is the MX6_PAD_ ones. The semantics of them 
depends on
 >>> the target cpu. See arch/arm/include/asm/arch-mx6/mx6-pins.h
 >>
 >> Ok - these files are not thought to be used in the same binary, we have
 >> to change something, taking into account we should remain compatible
 >> without breaking the currently supported boards.
 >>
 >> Let's start with some proposals.
 >
 > Yes, this is the productive approach.
 >
 >> Maybe you have already introduced a
 >> CONFIG_ switch, because at the moment only one SOC per image is
 >> supported, and one of MX6Q, MX6DL must be set. We have also the same
 >> issue with -ddr files (mx6q-ddr and mx6dl-ddr). Let's say we add a
 >> CONFIG_MX6_MULTI to support all SocS at the same time.
 >>
 >> Then we could change the file you mention adding a suffix to each pin.
 >> For example, in mx6q_pins.h we could add something like this:
 >>
 >> #ifdef CONFIG_MX6_MULTI
 >> #define PAD_SUFFIX _6Q
 >> #else
 >> #define PAD_SUFFIX
 >> #endif
 >>
 >> And we add the macro to each pin, such as
 >> enum {
 >>          MX6_PAD_SD2_DAT1__USDHC2_DAT1##PAD_SUFFIX
 >>
 >> In this way we could have different names only if we support multiple
 >> SOCs. We need then some accessors to get the right pin, something like
 >> mx6_pin(soc_type, pin_name), that returns the right configuration. Of
 >> course, this is a very first draft, and someone else can start with
 >> different proposals.
 >>
 >
 > Your suggestion is similar to what I would first think of, but you do
 > the extra kludging to make it work with the current syntax. My
 > approach would be to introduce new namings in parallel to the current
 > ones (similar, if not the same, as the linux kernel uses) until most
 > imx6 boards have been cleaned to use the new namings (which might
 > take a while).
 >

No matter how we implement this, there are some pre-requisites.

I believe we're all in agreement that we should be able to express
a pad value in one place which defines the use of a particular pad
on a particular board.

We need some cleanup to get there though.
For example, pad CSI0_DAT13 has this option in mx6q_pins.h:

     MX6_PAD_CSI0_DAT13__PCIE_CTRL_MUX_17

but this value in mx6dl_pins.h
     MX6_PAD_CSI0_DAT13__PCIE_CTRL_DIAG_STATUS_BUS_MUX_17

I'm created sorted lists of pad names for dual-quad and solo
processors for easy comparison:
	http://linode.boundarydevices.com/imx6quad-pad-names.txt
	http://linode.boundarydevices.com/imx6solo-pad-names.txt

Most of the differences are simply name changes, with an extra
underscore in one versus the other.

Some of them are functional differences though (e.g. SATA isn't
present on solo). No matter how the pad names get changed, these
should be easy to identify in the resulting code.

 > For the accessor macro, our experiences from the Wandboard kernel
 > have been very positive with the following:
 >
 > To set the above mentioned pad, the board file does:
 >
 >     IMX6_SETUP_PAD( SD2_DAT1__USDHC2_DAT1 );
 >
 > where the macro IMX6_SETUP_PAD is defined as:
 >
 > #define IMX6_SETUP_PAD(p) \
 >     if (cpu_is_mx6q()) \
 >         mxc_iomux_v3_setup_pad(MX6Q_PAD_##p);\
 >     else \
 >         mxc_iomux_v3_setup_pad(MX6DL_PAD_##p)
 >

Please, no!

You'd never write code like this by hand, and putting it behind a
macro doesn't make it better. It just hides the ugliness.

Regards,


Eric