[U-Boot] [PATCH 1/9] Tegra: T30: Add include files
Tom Warren
twarren.nvidia at gmail.com
Thu Sep 13 22:51:44 CEST 2012
Stephen,
On Thu, Sep 13, 2012 at 12:35 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 09/12/2012 04:10 PM, Tom Warren wrote:
>> Signed-off-by: Tom Warren <twarren at 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.
More information about the U-Boot
mailing list