[U-Boot] [Patch v3] Add TQ Systems TQMa6 board support

[Markus Niebel]

Hello Stefano,

Am 10.07.2014 14:59, wrote Stefano Babic:
> Hi Markus,
> 
> some minor points. I cannot find the original patch in my mailbox, that
> is the reason my answer can look weird. ;-)

Abolutely not - thanks for review. Will go through your comments and
clean up. Just a few points:

> 
> On 10/07/2014 09:24, Stefano Babic wrote:
>> Hi Markus,
>>
>> On 10/07/2014 09:11, Markus Niebel wrote:
>>> Am 23.06.2014 17:56, wrote Markus Niebel:
>>>> From: Markus Niebel <Markus.Niebel at tq-group.com>
>>>>
>>>> This patch adds the changes to boards.cfg and the board directory
>>>> under board/tqc.
>>>>
>>>> TQMa6 is a family of modules based on Freescale i.MX6. It consists of
>>>> TQMa6Q (i.MX6 Quad), TQMa6D (i.MX6 Dual) featuring eMMC, and 1 GiB DDR3
>>>> TQMa6S (i.MX6 Solo)  featuring eMMC and 512 MiB DDR3
>>>>
>>>> The modules need a baseboard. Initially the MBa6x starterkit mainboard is
>>>> supported. To easy support for other mainboards the functionality is splitted
>>>> in one file for the module (tqma6.c) and one file for the baseboard (tqma6_
>>>> mba6).
>>>>
>>>> The modules can be boot from eMMC (on USDHC3) and SPI flash.
>>>>
>>>> The following features are supported:
>>>> - MMC: eMMC on module (on USDHC3) and SD-card (on MBa6x mainboard)
>>>> - Ethernet: RGMII using micrel KSZ9031 phy on MBa6x mainboard for TQMa6<x> module.
>>>>   The phy needs special configurations for the pad skew registers to adjust for
>>>>   the signal routing.
>>>>   Also support for standard ethernet commands and uppdate via tftp.
>>>> - SPI: ECSPI1 with bootable serial flash on module and two additional
>>>>   chip selects on MBa6x
>>>> - I2C: This patch adds support for the I2C busses on the TQMa6<x> modules (I2C3)
>>>>   and MBa6x baseboards (I2C1). The LM75 temperature sensors on TQMa6<x> and MBa6x
>>>>   are also configured.
>>>> - USB: high speed host 1 on MBa6x and support for USB storage
>>>> - PMIC: support for pfuze 100 on TQMa6<x>
>>>>
>>>> Signed-off-by: Markus Niebel <Markus.Niebel at tq-group.com>
>>>> ---
>>>
>>> Ping ... any comments or shall I rebase after 2014.07 will be released?
>>
>> No, please wait...I am reviewing your patch for merging into -next, even
>> before 2014.07 is released. If I still see open issues, I will inform you.
>>
> 
> There is a ton of warning by checkpatch because line exceeds 80 chars.
> They should be fixed.

These 24 or so come from pin multiplexing tables. You are right, will fix it.

> 
> Then:
> 
> +		printf("Warning: failed to initialize eMMC dev\n");
> 
> Use puts instead of printf for static strings. I think there are also a
> couple of other identical cases.

ok

> 
> You set the variable "board" in your code. "board" is quite generic and
> you use it to set the name. We have already a case in U-Boot doing this
> with am335x boards. Use "board_name" (even "board_rev" if required) to
> set target's name to be consistent with other boards.
> 

good point

> I have not understood why you put  board_mmc_getcd() and
> board_mmc_getwp() in the module file (tqma6.c). They refer then to board
> specific code, for example tqma6_bb_board_mmc_getcd(). Should they not
> belong to the baseboard file ?
> 

the modules have an eMMC - this needs to set WP to zero and CD to present.
Baseboards may or may not implement SD-Card slots with different
configurations.

> +	power_pfuze100_init(2);
> 
> Understood, but maybe better to have a constant for the bus number
> 
> +#if defined(CONFIG_MX6Q)
> +#define MBA6X_KSZ9031_CTRL_SKEW	0x0032
> +#define MBA6X_KSZ9031_CLK_SKEW	0x03ff
> +#define MBA6X_KSZ9031_RX_SKEW	0x3333
> +#define MBA6X_KSZ9031_TX_SKEW	0x2036
> +#elif defined(CONFIG_MX6S)
> +#define MBA6X_KSZ9031_CTRL_SKEW	0x0030
> 
> Does the skew depend on SOC type or maybe on the different baseboard ?
> Is it correct to make the #ifdef dependig on the SOC ?

for MBa6 it depends on whether we have a TQMa6S (i.MX6S) Module or a
TQMa6Q/D (i.MX6Q/D) Module. So at least for MBa6x it depends on which SOC
is on the Module.

> 
> There are several new undocumented CONFIG_ in your configuration.
> A CONFIG_ should be documented, but it seems you use only locally in
> your tqma6.h. Then I will suggest to change their name to better
> understand they are not general configuration switches for U-Boot.

Oh, sorry - I missed this. 

> 
> Best regards,
> Stefano Babic
> 

Best Regards
Markus Niebel