[U-Boot] [PATCH 1/9] Tegra: T30: Add include files

[Stephen Warren]

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...

> +/* 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)

> 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.

> 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.

> +/* 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.

> +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?

> 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.

> 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.

> +/* 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.

> 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?

> 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.

> +/*
> + * 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.

> +/* 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.

> +/**
> + * Configuure a list of pin groups

Typo above.

> 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.

> 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,
> +};

> 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...

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.