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