Stephen,
On Wed, Oct 3, 2012 at 1:31 PM, Stephen Warren <swar...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
