| Message ID | 506F3CFE.8020305@boundarydevices.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Oct 05, 2012 at 01:03:10PM -0700, Eric Nelson wrote: > On 10/05/2012 12:00 PM, Tom Rini wrote: [snip] > >And we can't deal with this by factoring the code differently? > > > Hi Tom, > > There are two bits to this question: > - Can we represent the policy differences outside of a > board structure? These differences are all inside of > include/configs/nitrogen6x. > > I'm not certain how, but I suspect that we can get > a different _config to work for this. > > - Can we represent the board differences without a > board structure? This is a bit harder, since the > boards are slightly different. The Nitrogen6X has > a different ethernet PHY reset pin and an optional > SDIO Wi-Fi module. > > We could add code to SABRE Lite to accommodate these, > but it seems that sets a bad precedent. Would this > be done for every vendor that bases a design on > SABRE Lite? > > The precise diffs for the configs and sources is attached for > reference. > > I've also been pondering how to simply re-use the code within > the board setup file (mx6qsabrelite.c), but I haven't figured > anything out. Clearly a lot of the code is duplicated, but at > the same time it's board-specific. > > For example, we could create a common module that sets up > the SD card pads "like SABRE Lite", and a similar one to > configure ethernet pads. Since SABRE Lite is a reference design, > perhaps that makes sense. > > Does anybody have thoughts about how and where this might be > sliced? Dice some bits of board/freescale/mx6qsabrelite/mx6qsabrelite.c into arch/arm/cpu/armv7/mx6/board.c (or some other filename in mx6/) ? This is what we do on the various OMAP families, and use a few hooks for "let the board fill in this part of setup". Possibly re-use and update mx6/soc.c even.
On 05/10/2012 22:03, Eric Nelson wrote: > On 10/05/2012 12:00 PM, Tom Rini wrote: >> On Fri, Oct 05, 2012 at 11:42:19AM -0700, Eric Nelson wrote: >>> On 10/05/2012 10:24 AM, Albert ARIBAUD wrote: >>>> Hi Eric, >>>> >>>> On Thu, 4 Oct 2012 12:49:07 -0700, Eric Nelson >>>> <eric.nelson@boundarydevices.com> wrote: >>>> >>>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com> >>>>> --- >>>>> board/boundary/nitrogen6x/Makefile | 41 ++ >>>>> board/boundary/nitrogen6x/README | 77 +++ >>>>> board/boundary/nitrogen6x/nitrogen6x.c | 840 >>>>> ++++++++++++++++++++++++++++++++ >>>>> boards.cfg | 1 + >>>>> include/configs/nitrogen6x.h | 242 +++++++++ >>>>> 5 files changed, 1201 insertions(+), 0 deletions(-) >>>>> create mode 100644 board/boundary/nitrogen6x/Makefile >>>>> create mode 100644 board/boundary/nitrogen6x/README >>>>> create mode 100644 board/boundary/nitrogen6x/nitrogen6x.c >>>>> create mode 100644 include/configs/nitrogen6x.h >>>> >>>> If this is essentially a copy of sabrelite, I am surprised that git >>>> does not report any copies. Did you use -C with git format-patch ? >>>> >>> >>> Hi Albert, >>> >>> I didn't use '-C' (didn't know about it: thanks for the tip!). >>> >>> It wouldn't have detected copies though, because I made slight >>> changes in each of these files, replacing the board names and >>> file names in board/boundary/ and altering the default environment >>> (policy bits) in nitrogen6x.h. >> >> And we can't deal with this by factoring the code differently? >> > Hi Tom, > > There are two bits to this question: > - Can we represent the policy differences outside of a > board structure? These differences are all inside of > include/configs/nitrogen6x. > What we have already done is to set a common config header that is included by both include/configs/nitrogen6x and include/configs/sabrelite. See for example mx6qsabre_common.h, with the boards mx6qsabresd and mx6qsabreauto. > - Can we represent the board differences without a > board structure? This is a bit harder, since the > boards are slightly different. The Nitrogen6X has > a different ethernet PHY reset pin and an optional > SDIO Wi-Fi module. Ona major point is *how* you want to represent your board into U-boot, even if it is derived from a Freescale's evaluation board. In many case a vendor directory is desired (freescale vs boundary). > > We could add code to SABRE Lite to accommodate these, > but it seems that sets a bad precedent. Would this > be done for every vendor that bases a design on > SABRE Lite? I am afraid that it is easy to reach the case when changes for a vendor will break other boards, and getting all in sync can be problematic. > > The precise diffs for the configs and sources is attached for > reference. > > I've also been pondering how to simply re-use the code within > the board setup file (mx6qsabrelite.c), but I haven't figured > anything out. Clearly a lot of the code is duplicated, but at > the same time it's board-specific. > > For example, we could create a common module that sets up > the SD card pads "like SABRE Lite", and a similar one to > configure ethernet pads. Since SABRE Lite is a reference design, > perhaps that makes sense. I think that another example in u-boot doing this is for davinci (8xx) boards, At least three boards share the same board structure, see board/davinci/da8xxevm/Makefile. Very board related functions are compiled using CONFIG_MACH: COBJS-$(CONFIG_MACH_DAVINCI_DA830_EVM) += da830evm.o COBJS-$(CONFIG_MACH_DAVINCI_DA850_EVM) += da850evm.o COBJS-$(CONFIG_MACH_DAVINCI_HAWK) += hawkboard.o Best regards, Stefano Babic
Hi Stefano, On 10/08/2012 05:54 AM, Stefano Babic wrote: > On 05/10/2012 22:03, Eric Nelson wrote: >> On 10/05/2012 12:00 PM, Tom Rini wrote: >>> On Fri, Oct 05, 2012 at 11:42:19AM -0700, Eric Nelson wrote: >>>> On 10/05/2012 10:24 AM, Albert ARIBAUD wrote: >>>>> Hi Eric, >>>>> >>>>> On Thu, 4 Oct 2012 12:49:07 -0700, Eric Nelson >>>>> <eric.nelson@boundarydevices.com> wrote: >>>>> >>>>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com> >>>>>> --- >>>>>> board/boundary/nitrogen6x/Makefile | 41 ++ >>>>>> board/boundary/nitrogen6x/README | 77 +++ >>>>>> board/boundary/nitrogen6x/nitrogen6x.c | 840 >>>>>> ++++++++++++++++++++++++++++++++ >>>>>> boards.cfg | 1 + >>>>>> include/configs/nitrogen6x.h | 242 +++++++++ >>>>>> 5 files changed, 1201 insertions(+), 0 deletions(-) >>>>>> create mode 100644 board/boundary/nitrogen6x/Makefile >>>>>> create mode 100644 board/boundary/nitrogen6x/README >>>>>> create mode 100644 board/boundary/nitrogen6x/nitrogen6x.c >>>>>> create mode 100644 include/configs/nitrogen6x.h >>>>> >>>>> If this is essentially a copy of sabrelite, I am surprised that git >>>>> does not report any copies. Did you use -C with git format-patch ? >>>>> >>>> >>>> Hi Albert, >>>> >>>> I didn't use '-C' (didn't know about it: thanks for the tip!). >>>> >>>> It wouldn't have detected copies though, because I made slight >>>> changes in each of these files, replacing the board names and >>>> file names in board/boundary/ and altering the default environment >>>> (policy bits) in nitrogen6x.h. >>> >>> And we can't deal with this by factoring the code differently? >>> >> Hi Tom, >> >> There are two bits to this question: >> - Can we represent the policy differences outside of a >> board structure? These differences are all inside of >> include/configs/nitrogen6x. >> > > What we have already done is to set a common config header that is > included by both include/configs/nitrogen6x and include/configs/sabrelite. > See for example mx6qsabre_common.h, with the boards mx6qsabresd and > mx6qsabreauto. > The question I have is "what is common?". Looking at mx6qsabre_common.h, it seems that there's a mixture of things that might be common to all i.MX6-based boards (inclusion of imx-regs is the clear example), but most of this header file is enforcing policy like #define CONFIG_CMDLINE_EDITING and #define CONFIG_ENV_IS_IN_MMC >> - Can we represent the board differences without a >> board structure? This is a bit harder, since the >> boards are slightly different. The Nitrogen6X has >> a different ethernet PHY reset pin and an optional >> SDIO Wi-Fi module. > > Ona major point is *how* you want to represent your board into U-boot, > even if it is derived from a Freescale's evaluation board. In many case > a vendor directory is desired (freescale vs boundary). > It's nice to have some brand exposure, but mostly we just want to make things simple to define and maintain so we can help get products to market. >> >> We could add code to SABRE Lite to accommodate these, >> but it seems that sets a bad precedent. Would this >> be done for every vendor that bases a design on >> SABRE Lite? > > I am afraid that it is easy to reach the case when changes for a vendor > will break other boards, and getting all in sync can be problematic. > I agree. That, and the question of policy are the primary drivers for the separate board directory. >> >> The precise diffs for the configs and sources is attached for >> reference. >> >> I've also been pondering how to simply re-use the code within >> the board setup file (mx6qsabrelite.c), but I haven't figured >> anything out. Clearly a lot of the code is duplicated, but at >> the same time it's board-specific. >> >> For example, we could create a common module that sets up >> the SD card pads "like SABRE Lite", and a similar one to >> configure ethernet pads. Since SABRE Lite is a reference design, >> perhaps that makes sense. > > I think that another example in u-boot doing this is for davinci (8xx) > boards, At least three boards share the same board structure, see > board/davinci/da8xxevm/Makefile. > > Very board related functions are compiled using CONFIG_MACH: > > COBJS-$(CONFIG_MACH_DAVINCI_DA830_EVM) += da830evm.o > COBJS-$(CONFIG_MACH_DAVINCI_DA850_EVM) += da850evm.o > COBJS-$(CONFIG_MACH_DAVINCI_HAWK) += hawkboard.o > Thanks for the pointers. I was about to ask Tom whether he could point me at specific cases to help illustrate what he's thinking. That said, I'm not quite grokking how the commonality of these boards is expressed. In some ways, I think Nitrogen6X is a case study in how a board derived from a reference design can be supported and maintained in U-Boot, either in the main-line tree or off-line as many vendors do. -- The schematics are almost identical, so 99% of the pad settings are the same. -- The native peripheral set for Nitrogen6X is a superset of those on Sabre Lite. -- The set of accessories supplied for both is the same (Android button board, LVDS and RGB displays). It seems a useful exercise at least to look at what can be done to express this in a useful and scalable way. Board designers often cut & paste big hunks of schematic when building a custom board based on a reference design. For example, the mx6qsabreauto design also seems to have DISP0 mapped to a 40-pin parallel display connector with the same I2C interface as SABRE Lite and Nitrogen6x. For our part, we actively encourage that for our Nitrogen6X SOM customers to help speed them to market. I think there's a way of mapping these decisions through to board-specific files. Because talking about in the abstract is difficult, I took a stab at coding this over the weekend and found that the easiest bits to express are the pad/pinmux settings. This also has the biggest impact. I'd like to propose that we place something like this in a common header corresponding to the reference design: #define SABRELITE_RGB_PADS \ MX6Q_PAD_DI0_DISP_CLK__IPU1_DI0_DISP_CLK, \ MX6Q_PAD_DI0_PIN15__IPU1_DI0_PIN15, \ ... Then, in board-specific file, the implementer could re-use them like so: static iomux_v3_cfg_t const rgb_pads[] = { SABRELITE_RGB_PADS }; To follow through, if a board designer uses the Atheros ethernet PHY, they might use bits from the SABREAUTO design: iomux_v3_cfg_t const enet_pads[] = { SABREAUTO_ENET_PADS }; If another board really does require different muxing or pad setup, there's nothing precluding cut & paste. As much as I dislike excessive macro-fu, using macros rather than external data declarations will avoid any symbol name conflicts or data segment bloat for unused bits. If changes are needed, it should also be simple to grep for boards using, say "SABRELITE_RGB_PADS" if an when an update is needed. For example, it's possible that the choice of 120 ohm drive strength is sub-optimal. If we determine that 80 ohms is better in the general case, we could change it in one place, but a board could easily override it if they have a particular panel that needs something else. As Stefano pointed out, other bits of re-factoring of the SABRE Lite code are possible and appropriate. In particular: - keyboard handling via GPIOs could go into drivers/input as a proper "gpio_keyboard" driver or somesuch. - some parts of display support could be moved into separate modules (though I'm not yet sure where all of the dividing lines should go). If this is done, it seems that very little is left. For example, is there a way to meaningfully make this bit of common code between SABRE Lite and Nitrogen6X modular? int misc_init_r(void) { #ifdef CONFIG_PREBOOT preboot_keys(); #endif #ifdef CONFIG_CMD_BMODE add_board_boot_modes(board_boot_modes); #endif return 0; } If so, is there a point? I'm not sure. As always, let me know your thoughts. Clearly, all of this needs feedback from Fabio and Jason. Regards, Eric
--- board/freescale/mx6qsabrelite/mx6qsabrelite.c 2012-10-03 10:12:16.416131784 -0700 +++ board/boundary/nitrogen6x/nitrogen6x.c 2012-10-04 12:28:10.798317824 -0700 @@ -69,6 +69,18 @@ PAD_CTL_DSE_40ohm | PAD_CTL_HYS | \ PAD_CTL_ODE | PAD_CTL_SRE_FAST) +#define WEAK_PULLUP (PAD_CTL_PKE | PAD_CTL_PUE | \ + PAD_CTL_PUS_100K_UP | PAD_CTL_SPEED_MED | \ + PAD_CTL_DSE_40ohm | PAD_CTL_HYS | \ + PAD_CTL_SRE_SLOW) + +#define WEAK_PULLDOWN (PAD_CTL_PKE | PAD_CTL_PUE | \ + PAD_CTL_PUS_100K_DOWN | PAD_CTL_SPEED_MED | \ + PAD_CTL_DSE_40ohm | PAD_CTL_HYS | \ + PAD_CTL_SRE_SLOW) + +#define OUTPUT_40OHM (PAD_CTL_SPEED_MED|PAD_CTL_DSE_40ohm) + int dram_init(void) { gd->ram_size = get_ram_size((void *)PHYS_SDRAM, PHYS_SDRAM_SIZE); @@ -174,6 +186,7 @@ MX6Q_PAD_RGMII_RX_CTL__GPIO_6_24 | MUX_PAD_CTRL(NO_PAD_CTRL), /* pin 42 PHY nRST */ MX6Q_PAD_EIM_D23__GPIO_3_23 | MUX_PAD_CTRL(NO_PAD_CTRL), + MX6Q_PAD_ENET_RXD0__GPIO_1_27 | MUX_PAD_CTRL(NO_PAD_CTRL), }; iomux_v3_cfg_t const enet_pads2[] = { @@ -185,6 +198,16 @@ MX6Q_PAD_RGMII_RX_CTL__RGMII_RX_CTL | MUX_PAD_CTRL(ENET_PAD_CTRL), }; +/* wl1271 pads on nitrogen6x */ +iomux_v3_cfg_t const wl12xx_pads[] = { + (MX6Q_PAD_NANDF_CS1__GPIO_6_14 & ~MUX_PAD_CTRL_MASK) | MUX_PAD_CTRL(WEAK_PULLDOWN), + (MX6Q_PAD_NANDF_CS2__GPIO_6_15 & ~MUX_PAD_CTRL_MASK) | MUX_PAD_CTRL(OUTPUT_40OHM), + (MX6Q_PAD_NANDF_CS3__GPIO_6_16 & ~MUX_PAD_CTRL_MASK) | MUX_PAD_CTRL(OUTPUT_40OHM), +}; +#define WL12XX_WL_IRQ_GP IMX_GPIO_NR(6,14) +#define WL12XX_WL_ENABLE_GP IMX_GPIO_NR(6,15) +#define WL12XX_BT_ENABLE_GP IMX_GPIO_NR(6,16) + /* Button assignments for J14 */ static iomux_v3_cfg_t const button_pads[] = { /* Menu */ @@ -203,7 +226,8 @@ static void setup_iomux_enet(void) { - gpio_direction_output(IMX_GPIO_NR(3, 23), 0); + gpio_direction_output(IMX_GPIO_NR(3, 23), 0); /* SABRE Lite PHY rst */ + gpio_direction_output(IMX_GPIO_NR(1, 27), 0); /* Nitrogen6X PHY rst */ gpio_direction_output(IMX_GPIO_NR(6, 30), 1); gpio_direction_output(IMX_GPIO_NR(6, 25), 1); gpio_direction_output(IMX_GPIO_NR(6, 27), 1); @@ -214,7 +238,8 @@ /* Need delay 10ms according to KSZ9021 spec */ udelay(1000 * 10); - gpio_set_value(IMX_GPIO_NR(3, 23), 1); + gpio_set_value(IMX_GPIO_NR(3, 23), 1); /* SABRE Lite PHY reset */ + gpio_set_value(IMX_GPIO_NR(1, 27), 1); /* Nitrogen6X PHY reset */ imx_iomux_v3_setup_multiple_pads(enet_pads2, ARRAY_SIZE(enet_pads2)); } @@ -692,6 +717,13 @@ int board_early_init_f(void) { setup_iomux_uart(); + + /* Disable wl1271 For Nitrogen6w */ + gpio_direction_input(WL12XX_WL_IRQ_GP); + gpio_direction_output(WL12XX_WL_ENABLE_GP,0); + gpio_direction_output(WL12XX_BT_ENABLE_GP,0); + + imx_iomux_v3_setup_multiple_pads(wl12xx_pads, ARRAY_SIZE(wl12xx_pads)); setup_buttons(); #if defined(CONFIG_VIDEO_IPUV3) @@ -730,7 +762,10 @@ int checkboard(void) { - puts("Board: MX6Q-Sabre Lite\n"); + if (gpio_get_value(WL12XX_WL_IRQ_GP)) { + puts("Board: Nitrogen6X\n"); + } else + puts("Board: SABRE Lite\n"); return 0; }