[U-Boot] [PATCH 3/7] Tegra30: Add common CPU (shared) files
Tom Warren
twarren.nvidia at gmail.com
Wed Oct 3 22:27:10 CEST 2012
Stephen,
On Wed, Oct 3, 2012 at 12:49 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 10/02/2012 04:45 PM, Tom Warren wrote:
>> These files are used by both SPL and main U-Boot.
>> Also made minor changes to shared Tegra code to support
>> T30 differences.
>
>> diff --git a/arch/arm/cpu/tegra-common/ap.c b/arch/arm/cpu/tegra-common/ap.c
>
>> @@ -58,6 +62,20 @@ int tegra_get_chip_type(void)
>> return TEGRA_SOC_T25;
>> }
>> break;
>> + case CHIPID_TEGRA30:
>> + switch (tegra_sku_id) {
>> + case SKU_ID_T30:
>> + /*
>> + * T30 has two options. We will return TEGRA_SOC_T30
>> + * until we have the fdt set up when it may change to
>> + * TEGRA_SOC_T30_408MHZ depending on the PLLP freq.
>> + */
>> + if (clock_get_rate(CLOCK_ID_PERIPH) == 408000000)
>> + return TEGRA_SOC_T30_408MHZ;
>> + else
>> + return TEGRA_SOC_T30;
>
> Hmmm. I wonder why this doesn't return just the actual chip type; isn't
> the different in PLL_P rates a SW option and not a facet of the HW? The
> whole TEGRA_SOC_T30_408MHZ thing seems strange to me.
As I remember the history of early T30 & our internal U-Boot bringup,
we started out at the slower clock rate (216MHz, IIRC), and then
phased in 408MHz once things were stable, chips rev'd, etc. I can
look into going directly to 408MHz, or phase it in in the next pass so
boards have an option of using either speed, much as we have in
Tegra20 with T20 and T25.
>
>> diff --git a/arch/arm/cpu/tegra-common/board.c b/arch/arm/cpu/tegra-common/board.c
>
>> @@ -59,9 +59,12 @@ unsigned int query_sdram_size(void)
>> case 1:
>> return 0x10000000; /* 256 MB */
>> case 2:
>> - default:
>> return 0x20000000; /* 512 MB */
>> - case 3:
>> + case 4:
>> + return 0x40000000; /* 1GB */
>> + case 8:
>> + return 0x7ff00000; /* 2GB - 1MB */
>> + default:
>> return 0x40000000; /* 1GB */
>> }
>> }
>
> I think we should be a little more explicit about the Tegra20/Tegra30
> differences here. At least for case 0, there's a difference in what this
> field means for the two chips.
>
> Value Tegra20 Tegra30
> 0 512 256
> 1 256 256
> 2 512 512
> 3 1024 768
> 4 undef 1024
> 5 undef undef
> 6 undef undef
> 7 undef undef
> 8 n/a 2048-1
>
> At least the Toshiba AC100 uses value "0" in this field and has 512MB
> not 1GB of RAM.
Good point - I've only dealt with 1GB boards here so I'd missed the
impact of changing the 'default' option. I'll redo it in V2.
>
> I also note that for Tegra30, this field is bits 31:28, whereas for
> Tegra20 it's bits 30:28; we aren't masking off the top bit as we should
> in the current code. Again, the Toshiba AC100 appears to set this extra
> top bit, and only succeeds in getting 512M as the result of this
> function because of the default case.
I thought that we'd made the ODMDATA definitions compatible between
T20 and T30. Must have missed it. I'll fix this too in V2.
>
>> diff --git a/arch/arm/cpu/tegra30-common/pinmux.c b/arch/arm/cpu/tegra30-common/pinmux.c
>
> Hmmm. The header file this relies on doesn't exist yet. But perhaps this
> is OK since there's no boards.cfg entry for Tegra30 yet.
Should be OK. As I said in another thread, I'll double-check with git
bisect/git am before posting V2.
>
>> +const struct tegra_pingroup_desc tegra_soc_pingroups[PINGRP_COUNT] = {
>> + /* NAME VDD f0 f1 f2 f3 fSafe io */
>
> "fSafe io" should be deleted; those columns don't exist in the table.
>
> I compared this table to that in the upstream kernel, which I spent a
> lot of time on correlating with the various documentation sources and
> our downstream kernel. Some changes should be made:
>
>> + PINI(DAP3_FS, BB, I2S2, RSVD1, DISPA, DISPB),
>> + PINI(DAP3_DIN, BB, I2S2, RSVD1, DISPA, DISPB),
>> + PINI(DAP3_DOUT, BB, I2S2, RSVD1, DISPA, DISPB),
>> + PINI(DAP3_SCLK, BB, I2S2, RSVD1, DISPA, DISPB),
>
> In the kernel at least, the RSVD values are only used in particular
> slots, i.e. RSVD1 always yields function 0, RSVD2==func1, RSVD3==func2,
> RSVD4==func3. That way, when representing the "safe" value, one can use
> RSVDn to guarantee to get func(n-1). I suspect, it'd be a good idea for
> U-Boot to adopt the same policy. So, s/RSVD1/RSVD2/ above, and make
> similar changes throughout this table.
>
>> + PINI(GPIO_PV0, BB, RSVD, RSVD, RSVD, RSVD),
>> + PINI(GPIO_PV1, BB, RSVD, RSVD, RSVD, RSVD),
>
> And likewise, plain "RSVD" doesn't exist. If you duplicate RSVD into
> each column of the table, there's no way to know which function
> pinmux_set_func(foo, RSVD) might pick, since it matches them all.
>
>> + PINI(SDMMC1_CLK, SDMMC1, SDMMC1, RSVD1, RSVD2, BAD),
>> + PINI(SDMMC1_CMD, SDMMC1, SDMMC1, RSVD1, RSVD2, BAD),
>> + PINI(SDMMC1_DAT3, SDMMC1, SDMMC1, RSVD1, UARTE, BAD),
>> + PINI(SDMMC1_DAT2, SDMMC1, SDMMC1, RSVD1, UARTE, BAD),
>> + PINI(SDMMC1_DAT1, SDMMC1, SDMMC1, RSVD1, UARTE, BAD),
>> + PINI(SDMMC1_DAT0, SDMMC1, SDMMC1, RSVD1, UARTE, BAD),
>
> I believe the final column for all those entries should be UARTA.
>
>> + PINI(GPIO_PV3, SDMMC1, BAD, RSVD1, RSVD2, RSVD3),
>
> BAD should be CLK_12M_OUT.
>
>> + PINO(LCD_PWR2, LCD, DISPA, DISPB, SPI5, BAD),
>> + PINO(LCD_SDOUT, LCD, DISPA, DISPB, SPI5, BAD),
>> + PINO(LCD_WR_N, LCD, DISPA, DISPB, SPI5, BAD),
>> + PINO(LCD_SCK, LCD, DISPA, DISPB, SPI5, BAD),
>> + PINO(LCD_PWR0, LCD, DISPA, DISPB, SPI5, BAD),
>
> BAD should be HDCP for all of those.
>
>> + PINI(HDMI_INT, LCD, RSVD, RSVD, RSVD, RSVD),
>
> The first reserved should be HDMI.
>
>> + PINI(VI_D0, VI, BAD, RSVD1, VI, RSVD2),
>> + PINI(VI_D1, VI, BAD, SDMMC2, VI, RSVD1),
>> + PINI(VI_D2, VI, BAD, SDMMC2, VI, RSVD1),
>> + PINI(VI_D3, VI, BAD, SDMMC2, VI, RSVD1),
>> + PINI(VI_D4, VI, BAD, SDMMC2, VI, RSVD1),
>> + PINI(VI_D5, VI, BAD, SDMMC2, VI, RSVD1),
>> + PINI(VI_D6, VI, BAD, SDMMC2, VI, RSVD1),
>> + PINI(VI_D7, VI, BAD, SDMMC2, VI, RSVD1),
>> + PINI(VI_D8, VI, BAD, SDMMC2, VI, RSVD1),
>> + PINI(VI_D9, VI, BAD, SDMMC2, VI, RSVD1),
> + PINI(VI_D10, VI, BAD, RSVD1, VI, RSVD2),
>> + PINI(VI_D11, VI, BAD, RSVD1, VI, RSVD2),
>
> BAD should be DDR for all of those.
>
>> + PINI(VI_MCLK, VI, VI, BAD, BAD, BAD),
>
> All the functions for this pin select VI.
>
>> + PINI(VI_VSYNC, VI, BAD, RSVD1, VI, RSVD2),
>> + PINI(VI_HSYNC, VI, BAD, RSVD1, VI, RSVD2),
>
> BAD should be DDR for both of those.
>
>> + PINI(UART2_RXD, UART, IRDA, SPDIF, UARTA, SPI4),
>> + PINI(UART2_TXD, UART, IRDA, SPDIF, UARTA, SPI4),
>
> IRDA just means UARTB, so I think remove IRDA, and replace all usage
> with UARTB everywhere.
>
>> + PINI(GMI_CS0_N, GMI, RSVD1, NAND, GMI, BAD),
>> + PINI(GMI_A17, GMI, UARTD, SPI4, GMI, BAD),
>> + PINI(GMI_A18, GMI, UARTD, SPI4, GMI, BAD),
>
> BAD should be DTV in all of those.
>
>> + PINI(GEN2_I2C_SCL, GMI, I2C2, BAD, GMI, RSVD3),
>> + PINI(GEN2_I2C_SDA, GMI, I2C2, BAD, GMI, RSVD3),
>
> BAD should be HDCP in both of those.
>
>> + PINI(CAM_MCLK, CAM, VI, BAD, VI_ALT2, POPSDMMC4),
>
> We should rename POPSDMMC4 to just SDMMC4 throughout; there's just a
> single SDMMC4 controller; no a separate POPSDMMC4 controller.
>
>> + PINI(CAM_I2C_SCL, CAM, BAD, I2C3, RSVD2, POPSDMMC4),
>> + PINI(CAM_I2C_SDA, CAM, BAD, I2C3, RSVD2, POPSDMMC4),
>
> BAD should be VGP1 and VGP2 in those two lines.
>
>> + PINI(KB_ROW3, SYS, KBC, BAD, RSVD2, BAD),
>
> For all of KBC_ROW* and KBC_COL*, the BAD in func1 (i.e. the first BAD)
> should be NAND.
>
> The second BAD there should be RSVD4.
>
>> + PINI(KB_ROW6, SYS, KBC, BAD, SDMMC2, BAD),
>> + PINI(KB_ROW7, SYS, KBC, BAD, SDMMC2, BAD),
>> + PINI(KB_ROW8, SYS, KBC, BAD, SDMMC2, BAD),
>> + PINI(KB_ROW9, SYS, KBC, BAD, SDMMC2, BAD),
>> + PINI(KB_ROW10, SYS, KBC, BAD, SDMMC2, BAD),
>> + PINI(KB_ROW11, SYS, KBC, BAD, SDMMC2, BAD),
>> + PINI(KB_ROW12, SYS, KBC, BAD, SDMMC2, BAD),
>> + PINI(KB_ROW13, SYS, KBC, BAD, SDMMC2, BAD),
>> + PINI(KB_ROW14, SYS, KBC, BAD, SDMMC2, BAD),
>> + PINI(KB_ROW15, SYS, KBC, BAD, SDMMC2, BAD),
>
> The second bad (final column) for all of those should be MIO.
>
>> + PINI(KB_COL0, SYS, KBC, BAD, TRACE, BAD),
>> + PINI(KB_COL1, SYS, KBC, BAD, TRACE, BAD),
>
> The second bad (final column) for both of those should be TEST.
>
>> + PINI(KB_COL6, SYS, KBC, BAD, TRACE, BAD),
>> + PINI(KB_COL7, SYS, KBC, BAD, TRACE, BAD),
>
> The second bad (final column) for both of those should be MIO.
>
>> + PINI(CORE_PWR_REQ, SYS, RSVD, RSVD, RSVD, RSVD),
>
> func0 (the first RSVD) should be CORE_PWR_REQ.
>
>> + PINI(CPU_PWR_REQ, SYS, RSVD, RSVD, RSVD, RSVD),
>
> func0 (the first RSVD) should be CPU_PWR_REQ.
>
>> + PINI(PWR_INT_N, SYS, RSVD, RSVD, RSVD, RSVD),
>
> func0 (the first RSVD) should be PWR_INT_N.
>
>> + PINI(CLK_32K_IN, SYS, RSVD, RSVD, RSVD, RSVD),
>
> func0 (the first RSVD) should be CLK_32K_IN.
>
>> + PINI(OWR, SYS, OWR, RSVD, RSVD, RSVD),
>
> func1 (the first RSVD) should be CEC.
>
>> + PINI(SPDIF_IN, AUDIO, SPDIF, HDA, BAD, DAPSDMMC2),
>> + PINI(SPDIF_OUT, AUDIO, SPDIF, RSVD1, BAD, DAPSDMMC2),
>
> BAD should be I2C1 for both of those.
>
> Rename DAPSDMMC2 to SDMMC2.
>
>> + PINI(SPI2_MOSI, AUDIO, SPI6, SPI2, BAD, GMI),
>> + PINI(SPI2_MISO, AUDIO, SPI6, SPI2, BAD, GMI),
>> + PINI(SPI2_CS0_N, AUDIO, SPI6, SPI2, BAD, GMI),
>> + PINI(SPI2_SCK, AUDIO, SPI6, SPI2, BAD, GMI),
>
> BAD should be GMI for all of those.
>
>> + PINI(SPI1_MOSI, AUDIO, SPI2, SPI1, BAD, GMI),
>> + PINI(SPI1_SCK, AUDIO, SPI2, SPI1, BAD, GMI),
>> + PINI(SPI1_CS0_N, AUDIO, SPI2, SPI1, BAD, GMI),
>> + PINI(SPI1_MISO, AUDIO, BAD, SPI1, BAD, RSVD3),
>
> SPI1_MISO's first BAD should be SPI3.
>
> For all of those 4, func2's BAD should be SPI2 or rather SPI2_ALT.
>
>> + PINI(SPI2_CS1_N, AUDIO, BAD, SPI2, BAD, BAD),
>> + PINI(SPI2_CS2_N, AUDIO, BAD, SPI2, BAD, BAD),
>
> In both of those, the first BAD should be SPI3, the second BAD SPI2_ALT,
> and the third BAD I2C1.
>
>> + PINI(SDMMC3_CLK, SDMMC3, UARTA, PWM2, SDMMC3, BAD),
>
> BAD should be SPI3.
>
>> + PINI(SDMMC3_CMD, SDMMC3, UARTA, PWM3, SDMMC3, BAD),
>
> BAD should be SPI2.
>
>> + PINI(SDMMC3_DAT0, SDMMC3, RSVD0, RSVD1, SDMMC3, BAD),
>> + PINI(SDMMC3_DAT1, SDMMC3, RSVD0, RSVD1, SDMMC3, BAD),
>> + PINI(SDMMC3_DAT2, SDMMC3, RSVD0, PWM1, SDMMC3, BAD),
>> + PINI(SDMMC3_DAT3, SDMMC3, RSVD0, PWM0, SDMMC3, BAD),
>
> BAD should be SPI3 in all of those.
>
>> + PINI(SDMMC3_DAT4, SDMMC3, PWM1, BAD, SDMMC3, BAD),
>> + PINI(SDMMC3_DAT5, SDMMC3, PWM0, BAD, SDMMC3, BAD),
>> + PINI(SDMMC3_DAT6, SDMMC3, SPDIF, BAD, SDMMC3, BAD),
>> + PINI(SDMMC3_DAT7, SDMMC3, SPDIF, BAD, SDMMC3, BAD),
>
> func1's BAD should be SPI4 in all of those.
>
> func3's BAD should be SPI2 in all of those.
Holy cow! Very thorough review! Thanks - I'll make those changes. Note
that this is the table we're still using in our internal T30 U-Boot
(IIRC) - we should update it there, too.
>
>> +void pinmux_set_func(enum pmux_pingrp pin, enum pmux_func func)
>
>> + /* Handle special values */
>> + if (func == PMUX_FUNC_SAFE)
>> + func = tegra_soc_pingroups[pin].func_safe;
>> +
>> + if (func & PMUX_FUNC_RSVD) {
>> + mux = func & 0x3;
>
> Indeed, this is the way the code should work, but in order for this code
> to work correctly, RSVD1 must only be used in the func0 column in the
> data, RSVD2 in the func1 column, etc.
I ported this from our internal codebase w/o looking at it too much -
it just worked (for the very limited pinmuxing done to get UART output
working). I'll check all this out on the next rev. Thanks.
>
>> +static int pinmux_set_lock(enum pmux_pingrp pin, enum pmux_pin_lock lock)
>
>> + reg = readl(pin_lock);
>> + reg &= ~(0x1 << PMUX_LOCK_SHIFT);
>> + if (lock == PMUX_PIN_LOCK_ENABLE)
>> + reg |= (0x1 << PMUX_LOCK_SHIFT);
>> + writel(reg, pin_lock);
>
> Note that the LOCK bit, once set, can never be set back to zero. Perhaps
> a diagnostic should be issued and/or an error returned if this is
> attempted? The kernel does that.
Good idea - I'll add it.
Thanks again for the hard work,
Tom
More information about the U-Boot
mailing list