[U-Boot] [PATCH 05/11] ARM: tegra: pinctrl: remove duplication

[Stephen Warren <swarren@wwwdotorg.org>]

On 03/14/2014 01:37 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On 13 March 2014 11:42, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Much of arch/arm/cpu/tegra*-common/pinmux.c is identical. Remove the
>> duplication by creating pinmux-common.c for all the identical code.
>>
>> This leaves:
>> * arch/arm/include/asm/arch-tegra*/pinmux.h defining only the names of
>>   the various pins/pin groups, drive groups, and mux functions.
>> * arch/arm/cpu/tegra*-common/pinmux.c containing only the lookup table
>>   stating which pin groups support which mux functions.
>>
>> The code in pinmux-common.c is semantically identical to that in the
>> various original pinmux.c, but had some consistency and cleanup fixes
>> applied during migration.
> 
> This patch is very welcome, as You know I was not keen on the
> duplication going in in the first place.
> 
>>
>> I removed the definition of struct pmux_tri_ctlr, since this is different
>> between SoCs (especially Tegra20 vs all others), and it's much simpler to
>> deal with this via the new REG/MUX_REG/... defines. spl.c, warmboot.c,
>> and warmboot_avp.c needed updates due to this, since they previously
>> hijacked this struct to encode the location of some non-pinmux registers.
>> Now, that code simply calculates these register addresses directly using
>> simple and obvious math. I like this method better irrespective of the
>> pinmux code cleanup anyway.
> 
> Not as keen as you - U-Boot normally uses structures for access to
> hardware registers.

I tend to disagree with this approach. All other SW I'm familiar with
uses simple #defines for offsets. This makes U-Boot rather unfamiliar to
engineers. All HW documentation uses numerical offsets. SW #defines are
extremely easy to validate against the HW documentation, whereas with
structs you have to count out reserved arrays for gaps, put comments in
that contain the offsets to help you match everything up and validate
it, etc.

Still ...

>> diff --git a/arch/arm/cpu/arm720t/tegra-common/spl.c b/arch/arm/cpu/arm720t/tegra-common/spl.c
>> index 5171a8f907a1..4097f3b04362 100644
>> --- a/arch/arm/cpu/arm720t/tegra-common/spl.c
>> +++ b/arch/arm/cpu/arm720t/tegra-common/spl.c
>> @@ -19,10 +19,10 @@
>>
>>  void spl_board_init(void)
>>  {
>> -       struct pmux_tri_ctlr *pmt = (struct pmux_tri_ctlr *)NV_PA_APB_MISC_BASE;
>> +       u32 *cfg_ctl = (u32 *)(NV_PA_APB_MISC_BASE + 0x24);
> 
> Open-coded address offset? To me it seems better to have a specific
> Tegra20 structure (normal U-Boot approach), or failing  that, worst
> case, a #define for this field. Also you should ask your hardware
> designers to stop moving things around :-)

Ah, it looks like there's already
./arch/arm/include/asm/arch-tegra20/apb_misc.h that defines the
registers at the start of the apb_misc space that aren't related to
pinmux. I'll uses this struct since it's already there, and add the one
missing field.

>> diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c b/arch/arm/cpu/tegra-common/pinmux-common.c

>> +/* return 1 if a pin_pupd_is in range */
>> +#define pmux_pin_pupd_isvalid(pupd) \
>> +       (((pupd) >= PMUX_PULL_NORMAL) && ((pupd) <= PMUX_PULL_UP))
>> +
>> +/* return 1 if a pin_tristate_is in range */
>> +#define pmux_pin_tristate_isvalid(tristate) \
>> +       (((tristate) >= PMUX_TRI_NORMAL) && ((tristate) <= PMUX_TRI_TRISTATE))
>> +
>> +#ifdef TEGRA_PMX_HAS_PIN_IO_BIT_ETC
> 
> Do we need this #Ifdef?
>
>> +/* return 1 if a pin_io_is in range */
>> +#define pmux_pin_io_isvalid(io) \
>> +       (((io) >= PMUX_PIN_OUTPUT) && ((io) <= PMUX_PIN_INPUT))

We certainly need not to compile this code, since e.g. PMUX_PIN_INPUT
doesn't exist on Tegra20 due to equivalent #ifdefs in pinmux.h.

I do explicitly want to keep the ifdefs in pinmux.h, so that APIs are
not prototyped, and values not defined, for features that don't exist on
the SoC that U-Boot is being built for. This validates@compile time
that code isn't using invalid APIs. While pinmux.h could be split up
into a few separate header files to avoid the ifdefs, I think that would
make the header situation far more complex than it needs to be.

I don't see anything wrong with having the same ifdefs in pinmux.c; it's
a very consistent structure. It also means that all the conditional
logic is in code, all implemented consistently via C pre-processor,
rather than some being in the header file and some in the Makefiles, and
hence I think it's easier to keep the two in sync, since they work
identically.

So in summary, I'd like to keep the ifdefs. I think they're pretty
simple and not a maintenance burden. Do you object?