Stephen, On Thu, Sep 13, 2012 at 12:35 PM, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 09/12/2012 04:10 PM, Tom Warren wrote: >> Signed-off-by: Tom Warren <twar...@nvidia.com> > > Hmm. This is rather large to review, but I tried to at least glance > through it all and spot obvious issues... > >> diff --git a/arch/arm/include/asm/arch-tegra30/ap30.h >> b/arch/arm/include/asm/arch-tegra30/ap30.h > >> +/* T30 Base physical address of SDRAM. */ >> +#define T30_BASE_PA_SDRAM 0x00000000 > > That should be 0x80000000. The fact anything still works with this issue > implies this constant isn't used anywhere? Aha, I see NV_PA_SDRAM_BASE > defined later with the correct value...
Yep, T30_BASE_PA_SDRAM isn't used anywhere - just converted from AP20_BASE_PA_SDRAM when I created ap30.h. I'll delete it. > >> +/* T30 Base physical address of flash. */ >> +#define T30_BASE_PA_NOR_FLASH 0xD0000000 > > I don't know where NOR actually is mapped, but it can't be there; that's > in the middle of SDRAM (2G..4G-1) Again, copied when converting ap20.h to ap30.h. NOR Flash is @ 0x4800:0000 on T30. I'll just delete it, as no one uses it. > >> diff --git a/arch/arm/include/asm/arch-tegra30/board.h >> b/arch/arm/include/asm/arch-tegra30/board.h > >> +#ifndef _TEGRA_BOARD_H_ >> +#define _TEGRA_BOARD_H_ > > I wonder if include guards shouldn't say TEGRA30 not just TEGRA. That > would avoid any conflict between the different > arch/arm/include/asm/arch-tegra* directories. Sure, that's unlikely > right now, but if we really continue to push device tree, maybe we'll > end up with a unified Tegra20/Tegra30 bootloader image, and need to > include both. I could change it to _TEGRA30_BOARD_H_. I'd haven't considered ever including both arch board.h files in a single build - as you say, it's unlikely. But I'll change it. > >> diff --git a/arch/arm/include/asm/arch-tegra30/clk_rst.h >> b/arch/arm/include/asm/arch-tegra30/clk_rst.h > >> +#ifndef _CLK_RST_H_ >> +#define _CLK_RST_H_ > > Hmm, and the naming format isn't consistent. Copied from Tegra20. In the (near) future, I'll have a combined clk_rst.h include for Tegra20/Tegra30. > >> +/* The clocks supported by the hardware */ >> +enum periph_id { >> + PERIPH_ID_FIRST, > ... >> +/* >> + * Clock peripheral IDs which sadly don't match up with PERIPH_ID. we want >> + * callers to use the PERIPH_ID for all access to peripheral clocks to avoid >> + * confusion bewteen PERIPH_ID_... and PERIPHC_... >> + * >> + * We don't call this CLOCK_PERIPH_ID or PERIPH_CLOCK_ID as it would just be >> + * confusing. >> + */ >> +enum periphc_internal_id { >> + /* 0x00 */ >> + PERIPHC_I2S1, >> + PERIPHC_I2S2, >> + PERIPHC_SPDIF_OUT, >> + PERIPHC_SPDIF_IN, >> + PERIPHC_PWM, >> + PERIPHC_05h, >> + PERIPHC_SBC2, >> + PERIPHC_SBC3, > ... > > Can you make sure that one/both (whichever is appropriate) of these line > up with the proposed Linux kernel Tegra30 clock binding clock IDs. I'm > not sure if the Tegra30 proposed binding has been published yet, but > Prashant Gaikwad can email you the patch off-list if you need. I'd rather leave it as is (copied from our internal T30 U-Boot code) for now, especially for a 'proposed' kernel change. Let's get a basic, working T30 build in there, and then we can make it match the kernel IDs as needed. > >> +void clock_enable(enum periph_id clkid); > > Hmm. I would have expected all the prototypes to be in a common header > between Tegra20 and Tegra30? See my comment in another thread. My goal is to get a working T30 build in (to cmd prompt), and then I'll look at common-izing all of the TegraXX code. > >> diff --git a/arch/arm/include/asm/arch-tegra30/funcmux.h >> b/arch/arm/include/asm/arch-tegra30/funcmux.h > >> +/* Configs supported by the func mux */ >> +enum { >> + FUNCMUX_DEFAULT = 0, /* default config */ >> + >> + /* UART configs */ >> + FUNCMUX_UART1_ULPI_UART2 = 0, >> + FUNCMUX_UART2_IRDA = 0, >> + FUNCMUX_UART4_GMC = 0, >> + >> + /* I2C configs */ >> + FUNCMUX_DVC_I2CP = 0, >> + FUNCMUX_I2C1_RM = 0, >> + FUNCMUX_I2C2_DDC = 0, >> + FUNCMUX_I2C2_PTA, >> + FUNCMUX_I2C3_DTF = 0, >> + >> + /* SDMMC configs */ >> + FUNCMUX_SDMMC1_SDIO1_4BIT = 0, >> + FUNCMUX_SDMMC2_DTA_DTD_8BIT = 0, >> + FUNCMUX_SDMMC3_SDB_4BIT = 0, >> + FUNCMUX_SDMMC3_SDB_SLXA_8BIT, >> + FUNCMUX_SDMMC4_ATC_ATD_8BIT = 0, >> + FUNCMUX_SDMMC4_ATB_GMA_4_BIT, >> + FUNCMUX_SDMMC4_ATB_GMA_GME_8_BIT, >> + >> + /* USB configs */ >> + FUNCMUX_USB2_ULPI = 0, >> + >> + /* Serial Flash configs */ >> + FUNCMUX_SPI1_GMC_GMD = 0, >> +}; > > Those all look like Tegra20 options. Tegra30 doesn't have mux pin groups > (muxing is per-pin now), so names like DTA, DTD, SDB, SLXA, ATB, GMA, > ... above don't make any sense (they're Tegra20 pin group names). > > In turn, I wonder if a funcmux.h-style API even makes sense for Tegra30, > since the number of legal configurations is probably too large to > usefully enumerate, but that is perhaps another question. Up to this point, there hasn't been much need for the funcmux stuff, so I haven't looked at it too thoroughly yet. The only pinmux I've messed with is the UART, since I've only been concerned with getting to the cmd prompt. I can take a look at converting this later, as part of the combo/common Tegra effort. > >> diff --git a/arch/arm/include/asm/arch-tegra30/gp_padctrl.h >> b/arch/arm/include/asm/arch-tegra30/gp_padctrl.h > >> +/* APB_MISC_GP and padctrl registers */ >> +struct apb_misc_gp_ctlr { >> + u32 modereg; /* 0x00: APB_MISC_GP_MODEREG */ >> + u32 hidrev; /* 0x04: APB_MISC_GP_HIDREV */ >> + u32 reserved0[22]; /* 0x08 - 0x5C: */ >> + u32 emu_revid; /* 0x60: APB_MISC_GP_EMU_REVID */ >> + u32 xactor_scratch; /* 0x64: APB_MISC_GP_XACTOR_SCRATCH */ >> + u32 aocfg1; /* 0x68: APB_MISC_GP_AOCFG1PADCTRL */ >> + u32 aocfg2; /* 0x6c: APB_MISC_GP_AOCFG2PADCTRL */ >> + u32 atcfg1; /* 0x70: APB_MISC_GP_ATCFG1PADCTRL */ >> + u32 atcfg2; /* 0x74: APB_MISC_GP_ATCFG2PADCTRL */ >> + u32 cdevcfg1; /* 0x78: APB_MISC_GP_CDEV1CFGPADCTRL */ >> + u32 cdevcfg2; /* 0x7C: APB_MISC_GP_CDEV2CFGPADCTRL */ >> + u32 csuscfg; /* 0x80: APB_MISC_GP_CSUSCFGPADCTRL */ >> + u32 dap1cfg; /* 0x84: APB_MISC_GP_DAP1CFGPADCTRL */ >> + u32 dap2cfg; /* 0x88: APB_MISC_GP_DAP2CFGPADCTRL */ >> + u32 dap3cfg; /* 0x8C: APB_MISC_GP_DAP3CFGPADCTRL */ >> + u32 dap4cfg; /* 0x90: APB_MISC_GP_DAP4CFGPADCTRL */ >> + u32 dbgcfg; /* 0x94: APB_MISC_GP_DBGCFGPADCTRL */ >> + u32 lcdcfg1; /* 0x98: APB_MISC_GP_LCDCFG1PADCTRL */ >> + u32 lcdcfg2; /* 0x9C: APB_MISC_GP_LCDCFG2PADCTRL */ >> + u32 sdmmc2_cfg; /* 0xA0: APB_MISC_GP_SDMMC2CFGPADCTRL */ >> + u32 sdmmc3_cfg; /* 0xA4: APB_MISC_GP_SDMMC3CFGPADCTRL */ >> + u32 spicfg; /* 0xA8: APB_MISC_GP_SPICFGPADCTRL */ >> + u32 uaacfg; /* 0xAC: APB_MISC_GP_UAACFGPADCTRL */ >> + u32 uabcfg; /* 0xB0: APB_MISC_GP_UABCFGPADCTRL */ >> + u32 uart2cfg; /* 0xB4: APB_MISC_GP_UART2CFGPADCTRL */ >> + u32 uart3cfg; /* 0xB8: APB_MISC_GP_UART3CFGPADCTRL */ >> + u32 vicfg1; /* 0xBC: APB_MISC_GP_VICFG1PADCTRL */ >> + u32 vicfg2; /* 0xC0: APB_MISC_GP_VICFG2PADCTRL */ >> + u32 xm2cfga; /* 0xC4: APB_MISC_GP_XM2CFGAPADCTRL */ >> + u32 xm2cfgc; /* 0xC8: APB_MISC_GP_XM2CFGCPADCTRL */ >> + u32 xm2cfgd; /* 0xCC: APB_MISC_GP_XM2CFGDPADCTRL */ >> + u32 xm2clkcfg; /* 0xD0: APB_MISC_GP_XM2CLKCFGPADCTRL */ >> + u32 memcomp; /* 0xD4: APB_MISC_GP_MEMCOMPPADCTRL */ >> +}; > > That is the Tegra20 layout. It's change for Tegra30. The set of fields > is probably even quite different. Yeah, again not really used yet, just included in ap30.c - I'd made a couple of passes at removing unused include files during the bringup, but I must have missed this one. I'll remove it for now, and add it back in with the proper T30 layout/names when it's needed. > >> +/* bit fields definitions for APB_MISC_GP_HIDREV register */ >> +#define HIDREV_CHIPID_SHIFT 8 >> +#define HIDREV_CHIPID_MASK (0xff << HIDREV_CHIPID_SHIFT) >> +#define HIDREV_MAJORPREV_SHIFT 4 >> +#define HIDREV_MAJORPREV_MASK (0xf << HIDREV_MAJORPREV_SHIFT) >> + >> +/* CHIPID field returned from APB_MISC_GP_HIDREV register */ >> +#define CHIPID_TEGRA20 0x20 > > There should be a Tegra30 define here, and since that register is common > between Tegra20 and Tegra30, I'd expect this to be in a common header. Common will happen later, and this'll have the correct Tegra30 IDs when I replace this header. > >> diff --git a/arch/arm/include/asm/arch-tegra30/mmc.h >> b/arch/arm/include/asm/arch-tegra30/mmc.h > >> +int tegra_mmc_init(int dev_index, int bus_width, int pwr_gpio, int cd_gpio); > > Wouldn't this be common too? Yep, it's identical to the Tegra20 version. When I do the common-izing, it'll go in arch/arm/include/arch-tegra/, etc. > >> diff --git a/arch/arm/include/asm/arch-tegra30/pinmux.h >> b/arch/arm/include/asm/arch-tegra30/pinmux.h > >> +/* >> + * Pin groups which we adjust. There are three basic attributes of each pin >> + * group which use this enum: >> + * >> + * - function >> + * - pullup / pulldown >> + * - tristate or normal >> + */ >> +enum pmux_pingrp { >> + PINGRP_ULPI_DATA0 = 0, /* offset 0x3000 */ >> + PINGRP_ULPI_DATA1, >> + PINGRP_ULPI_DATA2, >> + PINGRP_ULPI_DATA3, > > Hmmm. This enum appears to have been picked based on register order in > the pin controller which I suppose is fine. > > However, that order doesn't match the GPIO order, so if you ever want > function gpio_request(n) to program both the GPIO controller and pin > controller, then you'll need a table to convert between the two sets of > IDs. For better or worse, the device tree binding for the Tegra30 pin > controller lists the names based on GPIO ID order, and hence if the > bindings are ever converted to integer-based rather than string-based, > would probably number the pins based on GPIO order too. > > I guess this means that either way you'll need a mapping table, either > from gpio-or-binding-id to pinmux register, or from this enum to > pinmux-register. > > So I guess that means this enum is fine - it's just something to watch > out for. AFAIK, we don't change the pinmux on gpio_request calls in Tegra U-Boot. This may be a deficiency, but no ones mentioned the need for it before. We can certainly address that in a future patchset (for Tegra20 as well as Tegra30). > >> +/* >> + * Functions which can be assigned to each of the pin groups. The values >> here >> + * bear no relation to the values programmed into pinmux registers and are >> + * purely a convenience. The translation is done through a table search. >> + */ >> +enum pmux_func { >> + PMUX_FUNC_AHB_CLK, >> + PMUX_FUNC_APB_CLK, >> + PMUX_FUNC_AUDIO_SYNC, >> + PMUX_FUNC_CRT, >> + PMUX_FUNC_DAP1, > ... > > Could you possibly update the order of this enum to match the Linux > kernel; see drivers/pinctrl/pinctrl-tegra30.c. Again, such an order > would be more likely to match any integer-based device-tree pinmux binding. > > Re-ordering this shouldn't be an issue since as the comment says there > must be a conversion table anyway. > For now, I'd like to leave it as is. Perhaps after this patchset is in, we can revisit making it matchy-matchy with the kernel - certainly when we get around to using DT for all periph init, etc. That's still in my U-Boot task-list, albeit at a low priority (at least until T30 is upstreamed). >> +/* t30 pin drive group and pin mux registers */ >> +#define PDRIVE_PINGROUP_OFFSET (0x868 >> 2) >> +#define PMUX_OFFSET ((0x3000 >> 2) - PDRIVE_PINGROUP_OFFSET - \ >> + PDRIVE_PINGROUP_COUNT) >> +struct pmux_tri_ctlr { >> + uint pmt_reserved0; /* ABP_MISC_PP_ reserved offset 00 */ >> + uint pmt_reserved1; /* ABP_MISC_PP_ reserved offset 04 */ >> + uint pmt_strap_opt_a; /* _STRAPPING_OPT_A_0, offset 08 */ >> + uint pmt_reserved2; /* ABP_MISC_PP_ reserved offset 0C */ >> + uint pmt_reserved3; /* ABP_MISC_PP_ reserved offset 10 */ >> + uint pmt_reserved4[4]; /* _TRI_STATE_REG_A/B/C/D in t20 */ >> + uint pmt_cfg_ctl; /* _CONFIG_CTL_0, offset 24 */ >> + >> + uint pmt_reserved[528]; /* ABP_MISC_PP_ reserved offs 28-864 */ >> + >> + uint pmt_drive[PDRIVE_PINGROUP_COUNT]; /* pin drive grps offs 868 */ >> + uint pmt_reserved5[PMUX_OFFSET]; /* offset 0x3000 */ >> + uint pmt_ctl[PINGRP_COUNT]; /* pin mux/pupd/tristate regs */ >> +}; > > Isn't pmc_ctrl at 0x3000; the second comment above appears misleading. > Also, PMUX_OFFSET isn't right; the offset is 0x3000; it's more like > RESERVED5_SIZE. Yeah, that's odd. Must have missed it during the port. I'll take a look. > >> +/** >> + * Configuure a list of pin groups > > Typo above. Copied from tegra20/pinmux.h. I'll correct it. > >> diff --git a/arch/arm/include/asm/arch-tegra30/pmu.h >> b/arch/arm/include/asm/arch-tegra30/pmu.h > >> +/* Set core and CPU voltages to nominal levels */ >> +int pmu_set_nominal(void); > > That also seems common. I'll stop pointing this issue out. Common/combined Tegra code will follow the acceptance of a working T30 build. > >> diff --git a/arch/arm/include/asm/arch-tegra30/tegra30.h >> b/arch/arm/include/asm/arch-tegra30/tegra30.h > >> +/* These are the available SKUs (product types) for Tegra */ >> +enum { >> + SKU_ID_T20 = 0x8, >> + SKU_ID_T25SE = 0x14, >> + SKU_ID_AP25 = 0x17, >> + SKU_ID_T25 = 0x18, >> + SKU_ID_AP25E = 0x1b, >> + SKU_ID_T25E = 0x1c, >> + >> + SKU_ID_T30 = 0x81, /* Cardhu value */ >> +}; > > There's little point defining Tegra20-specific values unless this header > is shared between the two SoCs. Same for: > >> +enum { >> + TEGRA_SOC_T20, >> + TEGRA_SOC_T25, >> + TEGRA_SOC_T30, >> + TEGRA_SOC_T30_408MHZ, /* A T30 with faster PLLP */ >> + TEGRA_SOC2_SLOW, /* T2x needs to run at slow clock initially */ >> + >> + TEGRA_SOC_COUNT, >> + TEGRA_SOC_UNKNOWN = -1, >> +}; > Yep, I meant to scrub these during the port. Thanks, I'll clean 'em up. >> diff --git a/arch/arm/include/asm/arch-tegra30/warmboot.h >> b/arch/arm/include/asm/arch-tegra30/warmboot.h > > At a quick glance, this header appears identical to Tegra20, yet given > the differences between sleep modes on the two SoCs, I'd expect to see > quite a few differences. I know a lot of extra PMC scratch registers > were added on Tegra30 in order to support the sleep mode code, and hence > it works quite differently... It's included in common/board.c, but not used unless CONFIG_TEGRA_LP0 is enabled, which it isn't on T30. I didn't want to pollute common code too much with #ifdefs, but I'll see if adding CONFIG_TEGRA_LP0 around it's include works, and then remove it from arch-tegra30 until we port LP0, which as you say is quite a bit different than T20. > > Overall, it might help identify problems if you passed the "-C" option > to "git format-patch"; that would show up any headers that had simply > been copied from Tegra20 without necessary modifications. I always try to use -C w/format-patch, and according to my bash history, I did. But since I copied most of these files over from my original porting/bringup branch, I don't think it helped much. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot