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