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

[Simon Glass]

Hi Stephen,

On 20 March 2014 12:57, Stephen Warren <swarren at wwwdotorg.org> wrote:
>
> On 03/14/2014 01:37 PM, Simon Glass wrote:
> > Hi Stephen,
> >
> > On 13 March 2014 11:42, Stephen Warren <swarren at wwwdotorg.org> wrote:
> >> From: Stephen Warren <swarren at 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 at 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.

Arguably you have created this problem by having #ifdefs in the C file
- if there was a separate file for each SoC then it would be much
harder to mess this up.

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

No. I can't possibly object given that you have completed such a great clean-up.

Regards,
Simon