Stephen,
On Wed, Oct 3, 2012 at 1:31 PM, Stephen Warren swarren@wwwdotorg.org wrote:
On 10/02/2012 04:45 PM, Tom Warren wrote:
Common Tegra files are in arch-tegra, shared between T20 and T30. Tegra30-specific headers are in arch-tegra30. Note that some of these will be filled in as more T30 support is added (drivers, WB/LP0 support, etc.).
diff --git a/arch/arm/include/asm/arch-tegra/gp_padctrl.h b/arch/arm/include/asm/arch-tegra/gp_padctrl.h
+/* 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)
Patch 1 added the following:
#define GP_HIDREV 0x804
... to arch/arm/cpu/arm720t/tegra-common/cpu.h. I wonder if that line wouldn't be better place here, alongside the macros that allow you to use it?
Sure, that would be better. The code that uses this in cpu.c was munged a few times and I may have added this in the wrong file.
diff --git a/arch/arm/include/asm/arch-tegra30/emc.h b/arch/arm/include/asm/arch-tegra30/emc.h
This file seems to be:
a) An exact duplicate of the Tegra20 file. Did the register layout really not change at all? That seems unlikely. If so, shouldn't the file be shared?
b) Not used by anything in this patch series, so shouldn't be needed.
c) Incorrect; struct emc_ctlr doesn't actually match the register layout for Tegra20 or Tegra30 (at least, offset 0 is interrupt status in HW in both chips, not cfg as in the struct). Some fields match though.
I'll revisit it in our internal U-Boot source. I tried to go through all the include files and make sure any not used on T30 initially were removed, but if it's used in a common file, I didn't want to add #if defined(CONFIG_TEGRA20) fencing if not necessary.
diff --git a/arch/arm/include/asm/arch-tegra30/funcmux.h b/arch/arm/include/asm/arch-tegra30/funcmux.h
+/**
- Select a config for a particular peripheral.
- Each peripheral can operate through a number of configurations,
- which are sets of pins that it uses to bring out its signals.
- The basic config is 0, and higher numbers indicate different
- pinmux settings to bring the peripheral out on other pins,
- This function also disables tristate for the function's pins,
- so that they operate in normal mode.
- @param id Peripheral id
- @param config Configuration to use (FUNCMUX_...), 0 for default
- @return 0 if ok, -1 on error (e.g. incorrect id or config)
- */
+int funcmux_select(enum periph_id id, int config);
That prototype is identical to Tegra20. Shouldn't there be a common funcmux.h that defines the prototype, either include from the SoC-specific file, or itself including the SoC-specific file? This is important since common code (stuff that's not specific to Tegra20 or Tegra30) calls this function, so it shouldn't be prototyped in multiple places (with associated possibility of divergence) even if the implementation is entirely separate for the two SoCs.
Yep, I can do that.
diff --git a/arch/arm/include/asm/arch-tegra30/gpio.h b/arch/arm/include/asm/arch-tegra30/gpio.h
- The Tegra 3x GPIO controller has 246 GPIOS in 8 banks of 4 ports,
- each with 8 GPIOs.
Extra space there after *.
diff --git a/arch/arm/include/asm/arch-tegra30/hardware.h b/arch/arm/include/asm/arch-tegra30/hardware.h
This file is empty except for comments. Is it really useful? The Tegra20 file is empty too
It's included from include/asm/hardware.h, which is included in 3 or so arm720t files, so it's mandatory. I hate empty files, but I don't see a way around it without polluting some arm720t files with #ifdef's. the integratorap_cm720t does the same thing.
diff --git a/arch/arm/include/asm/arch-tegra30/pinmux-config-common.h b/arch/arm/include/asm/arch-tegra30/pinmux-config-common.h
The content of this file presumably describes Cardhu (which revision?) Surely it should be in board/nvidia/cardhu/*.c?
On the previous review cycle (before I commonized Tegra files), this file lived in board/nvidia/cardhu, and I think it was Tom Rini (or maybe Simon) that pointed out that it had no 'cardhu' identifiers in it, so it should be moved to a more Tegra30-centric area, hence the move to arch-tegra30. Again, this is from our internal repo, and I don't believe there's any rev info in that file (except for commented-out tables that aren't used, so I removed them). I think it's valid to say this file is common to all Tegra30 builds, and additional sparse pinmux tables could be added in board files/areas for board-specific / rev-specific mux changes.
diff --git a/arch/arm/include/asm/arch-tegra30/pinmux.h b/arch/arm/include/asm/arch-tegra30/pinmux.h
+/*
- 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_RSVD = 0x8000,PMUX_FUNC_RSVD0 = PMUX_FUNC_RSVD,PMUX_FUNC_BAD = 0x4000, /* Invalid! */PMUX_FUNC_NONE = 0,I think all four of those should be deleted.
I'll look into it after the pinmux table rewrite you pointed out.
PMUX_FUNC_I2C,PMUX_FUNC_I2C1 = PMUX_FUNC_I2C,I don't think there's a need for duplicate names for any of these.
I'll look into it.
Thanks,
Tom