Stephen,
On Thu, Sep 13, 2012 at 12:35 PM, Stephen Warren swarren@wwwdotorg.org wrote:
On 09/12/2012 04:10 PM, Tom Warren wrote:
Signed-off-by: Tom Warren twarren@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.