Hi Stephen, On 13 March 2014 11:42, Stephen Warren <swar...@wwwdotorg.org> wrote: > From: Stephen Warren <swar...@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. > > Signed-off-by: Stephen Warren <swar...@nvidia.com> > --- > arch/arm/cpu/arm720t/tegra-common/spl.c | 4 +- > arch/arm/cpu/tegra-common/Makefile | 6 +- > arch/arm/cpu/tegra-common/pinmux-common.c | 507 > ++++++++++++++++++++++++++++ > arch/arm/cpu/tegra114-common/pinmux.c | 430 +---------------------- > arch/arm/cpu/tegra124-common/pinmux.c | 429 +---------------------- > arch/arm/cpu/tegra20-common/pinmux.c | 124 +------ > arch/arm/cpu/tegra20-common/warmboot.c | 5 +- > arch/arm/cpu/tegra20-common/warmboot_avp.c | 4 +- > arch/arm/cpu/tegra30-common/pinmux.c | 400 +--------------------- > arch/arm/include/asm/arch-tegra/pinmux.h | 189 +++++++++++ > arch/arm/include/asm/arch-tegra114/pinmux.h | 219 +----------- > arch/arm/include/asm/arch-tegra124/pinmux.h | 216 +----------- > arch/arm/include/asm/arch-tegra20/pinmux.h | 85 +---- > arch/arm/include/asm/arch-tegra30/pinmux.h | 199 +---------- > 14 files changed, 738 insertions(+), 2079 deletions(-) > create mode 100644 arch/arm/cpu/tegra-common/pinmux-common.c > create mode 100644 arch/arm/include/asm/arch-tegra/pinmux.h > > 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 :-) > > /* enable JTAG */ > - writel(0xC0, &pmt->pmt_cfg_ctl); > + writel(0xC0, cfg_ctl); > > board_init_uart_f(); > > diff --git a/arch/arm/cpu/tegra-common/Makefile > b/arch/arm/cpu/tegra-common/Makefile > index 34d57349afb5..892556e64451 100644 > --- a/arch/arm/cpu/tegra-common/Makefile > +++ b/arch/arm/cpu/tegra-common/Makefile > @@ -7,6 +7,10 @@ > # SPDX-License-Identifier: GPL-2.0+ > # > > +obj-y += ap.o > +obj-y += board.o > +obj-y += cache.o > +obj-y += clock.o > obj-y += lowlevel_init.o > -obj-y += ap.o board.o clock.o cache.o > +obj-y += pinmux-common.o > obj-$(CONFIG_DISPLAY_CPUINFO) += sys_info.o > diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c > b/arch/arm/cpu/tegra-common/pinmux-common.c > new file mode 100644 > index 000000000000..10f68ba98b52 > --- /dev/null > +++ b/arch/arm/cpu/tegra-common/pinmux-common.c > @@ -0,0 +1,507 @@ > +/* > + * Copyright (c) 2010-2013, NVIDIA CORPORATION. All rights reserved. > + * Copyright (c) 2011 The Chromium OS Authors. > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <common.h> > +#include <asm/io.h> > +#include <asm/arch/pinmux.h> > + > +/* return 1 if a pingrp is in range */ > +#define pmux_pingrp_isvalid(pin) (((pin) >= 0) && ((pin) < PINGRP_COUNT)) > + > +/* return 1 if a pmux_func is in range */ > +#define pmux_func_isvalid(func) \ > + ((((func) >= 0) && ((func) < PMUX_FUNC_COUNT)) || \ > + (((func) >= PMUX_FUNC_RSVD1) && ((func) <= PMUX_FUNC_RSVD4))) > + > +/* 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)) > + > +/* return 1 if a pin_lock is in range */ > +#define pmux_pin_lock_isvalid(lock) \ > + (((lock) >= PMUX_PIN_LOCK_DISABLE) && ((lock) <= > PMUX_PIN_LOCK_ENABLE)) > + > +/* return 1 if a pin_od is in range */ > +#define pmux_pin_od_isvalid(od) \ > + (((od) >= PMUX_PIN_OD_DISABLE) && ((od) <= PMUX_PIN_OD_ENABLE)) > + > +/* return 1 if a pin_ioreset_is in range */ > +#define pmux_pin_ioreset_isvalid(ioreset) \ > + (((ioreset) >= PMUX_PIN_IO_RESET_DISABLE) && \ > + ((ioreset) <= PMUX_PIN_IO_RESET_ENABLE)) > + > +#ifdef TEGRA_PMX_HAS_RCV_SEL > +/* return 1 if a pin_rcv_sel_is in range */ > +#define pmux_pin_rcv_sel_isvalid(rcv_sel) \ > + (((rcv_sel) >= PMUX_PIN_RCV_SEL_NORMAL) && \ > + ((rcv_sel) <= PMUX_PIN_RCV_SEL_HIGH)) > +#endif /* TEGRA_PMX_HAS_RCV_SEL */ > +#endif /* TEGRA_PMX_HAS_PIN_IO_BIT_ETC */ Do we need this #ifdef? > + > +#define _R(offset) (u32 *)(NV_PA_APB_MISC_BASE + (offset)) > + > +#if defined(CONFIG_TEGRA20) > + > +#define MUX_REG(grp) _R(0x80 + ((tegra_soc_pingroups[grp].ctl_id / 16) * > 4)) > +#define MUX_SHIFT(grp) ((tegra_soc_pingroups[grp].ctl_id % 16) * 2) > + > +#define PULL_REG(grp) _R(0xa0 + ((tegra_soc_pingroups[grp].pull_id / 16) * > 4)) > +#define PULL_SHIFT(grp) ((tegra_soc_pingroups[grp].pull_id % 16) * 2) > + > +#define TRI_REG(grp) _R(0x14 + (((grp) / 32) * 4)) > +#define TRI_SHIFT(grp) ((grp) % 32) > + > +#else > + > +#define REG(pin) _R(0x3000 + ((pin) * 4)) > + > +#define MUX_REG(pin) REG(pin) > +#define MUX_SHIFT(pin) 0 > + > +#define PULL_REG(pin) REG(pin) > +#define PULL_SHIFT(pin) 2 > + > +#define TRI_REG(pin) REG(pin) > +#define TRI_SHIFT(pin) 4 > + > +#endif /* CONFIG_TEGRA20 */ > + > +#define DRV_REG(group) _R(0x868 + ((group) * 4)) > + > +#define IO_SHIFT 5 > +#define OD_SHIFT 6 > +#define LOCK_SHIFT 7 > +#define IO_RESET_SHIFT 8 > +#define RCV_SEL_SHIFT 9 > + > +void pinmux_set_func(enum pmux_pingrp pin, enum pmux_func func) > +{ > + u32 *reg = MUX_REG(pin); > + int i, mux = -1; > + u32 val; > + > + /* Error check on pin and func */ > + assert(pmux_pingrp_isvalid(pin)); > + assert(pmux_func_isvalid(func)); > + > + if (func & PMUX_FUNC_RSVD1) { > + mux = func & 3; > + } else { > + /* Search for the appropriate function */ > + for (i = 0; i < 4; i++) { > + if (tegra_soc_pingroups[pin].funcs[i] == func) { > + mux = i; > + break; > + } > + } > + } > + assert(mux != -1); > + > + val = readl(reg); > + val &= ~(3 << MUX_SHIFT(pin)); > + val |= (mux << MUX_SHIFT(pin)); > + writel(val, reg); > +} > + > +void pinmux_set_pullupdown(enum pmux_pingrp pin, enum pmux_pull pupd) > +{ > + u32 *reg = PULL_REG(pin); > + u32 val; > + > + /* Error check on pin and pupd */ > + assert(pmux_pingrp_isvalid(pin)); > + assert(pmux_pin_pupd_isvalid(pupd)); > + > + val = readl(reg); > + val &= ~(3 << PULL_SHIFT(pin)); > + val |= (pupd << PULL_SHIFT(pin)); > + writel(val, reg); > +} > + > +void pinmux_set_tristate(enum pmux_pingrp pin, int tri) > +{ > + u32 *reg = TRI_REG(pin); > + u32 val; > + > + /* Error check on pin */ > + assert(pmux_pingrp_isvalid(pin)); > + assert(pmux_pin_tristate_isvalid(tri)); > + > + val = readl(reg); > + if (tri == PMUX_TRI_TRISTATE) > + val |= (1 << TRI_SHIFT(pin)); > + else > + val &= ~(1 << TRI_SHIFT(pin)); > + writel(val, reg); > +} > + > +void pinmux_tristate_enable(enum pmux_pingrp pin) > +{ > + pinmux_set_tristate(pin, PMUX_TRI_TRISTATE); > +} > + > +void pinmux_tristate_disable(enum pmux_pingrp pin) > +{ > + pinmux_set_tristate(pin, PMUX_TRI_NORMAL); > +} > + > +#ifdef TEGRA_PMX_HAS_PIN_IO_BIT_ETC > +void pinmux_set_io(enum pmux_pingrp pin, enum pmux_pin_io io) > +{ > + u32 *reg = REG(pin); > + u32 val; > + > + if (io == PMUX_PIN_NONE) > + return; > + > + /* Error check on pin and io */ > + assert(pmux_pingrp_isvalid(pin)); > + assert(pmux_pin_io_isvalid(io)); > + > + val = readl(reg); > + if (io == PMUX_PIN_INPUT) > + val |= (io & 1) << IO_SHIFT; > + else > + val &= ~(1 << IO_SHIFT); > + writel(val, reg); > +} > + > +static void pinmux_set_lock(enum pmux_pingrp pin, enum pmux_pin_lock lock) > +{ > + u32 *reg = REG(pin); > + u32 val; > + > + if (lock == PMUX_PIN_LOCK_DEFAULT) > + return; > + > + /* Error check on pin and lock */ > + assert(pmux_pingrp_isvalid(pin)); > + assert(pmux_pin_lock_isvalid(lock)); > + > + val = readl(reg); > + if (lock == PMUX_PIN_LOCK_ENABLE) { > + val |= (1 << LOCK_SHIFT); > + } else { > + if (val & (1 << LOCK_SHIFT)) > + printf("%s: Cannot clear LOCK bit!\n", __func__); > + val &= ~(1 << LOCK_SHIFT); > + } > + writel(val, reg); > + > + return; > +} > + > +static void pinmux_set_od(enum pmux_pingrp pin, enum pmux_pin_od od) > +{ > + u32 *reg = REG(pin); > + u32 val; > + > + if (od == PMUX_PIN_OD_DEFAULT) > + return; > + > + /* Error check on pin and od */ > + assert(pmux_pingrp_isvalid(pin)); > + assert(pmux_pin_od_isvalid(od)); > + > + val = readl(reg); > + if (od == PMUX_PIN_OD_ENABLE) > + val |= (1 << OD_SHIFT); > + else > + val &= ~(1 << OD_SHIFT); > + writel(val, reg); > + > + return; > +} > + > +static void pinmux_set_ioreset(enum pmux_pingrp pin, > + enum pmux_pin_ioreset ioreset) > +{ > + u32 *reg = REG(pin); > + u32 val; > + > + if (ioreset == PMUX_PIN_IO_RESET_DEFAULT) > + return; > + > + /* Error check on pin and ioreset */ > + assert(pmux_pingrp_isvalid(pin)); > + assert(pmux_pin_ioreset_isvalid(ioreset)); > + > + val = readl(reg); > + if (ioreset == PMUX_PIN_IO_RESET_ENABLE) > + val |= (1 << IO_RESET_SHIFT); > + else > + val &= ~(1 << IO_RESET_SHIFT); > + writel(val, reg); > + > + return; > +} > + > +#ifdef TEGRA_PMX_HAS_RCV_SEL > +static void pinmux_set_rcv_sel(enum pmux_pingrp pin, > + enum pmux_pin_rcv_sel rcv_sel) > +{ > + u32 *reg = REG(pin); > + u32 val; > + > + if (rcv_sel == PMUX_PIN_RCV_SEL_DEFAULT) > + return; > + > + /* Error check on pin and rcv_sel */ > + assert(pmux_pingrp_isvalid(pin)); > + assert(pmux_pin_rcv_sel_isvalid(rcv_sel)); > + > + val = readl(reg); > + if (rcv_sel == PMUX_PIN_RCV_SEL_HIGH) > + val |= (1 << RCV_SEL_SHIFT); > + else > + val &= ~(1 << RCV_SEL_SHIFT); > + writel(val, reg); > + > + return; > +} > +#endif /* TEGRA_PMX_HAS_RCV_SEL */ > +#endif /* TEGRA_PMX_HAS_PIN_IO_BIT_ETC */ > + > +void pinmux_config_pingroup(const struct pingroup_config *config) > +{ > + enum pmux_pingrp pin = config->pingroup; > + > + pinmux_set_func(pin, config->func); > + pinmux_set_pullupdown(pin, config->pull); > + pinmux_set_tristate(pin, config->tristate); > +#ifdef TEGRA_PMX_HAS_PIN_IO_BIT_ETC > + pinmux_set_io(pin, config->io); > + pinmux_set_lock(pin, config->lock); > + pinmux_set_od(pin, config->od); > + pinmux_set_ioreset(pin, config->ioreset); > +#ifdef TEGRA_PMX_HAS_RCV_SEL > + pinmux_set_rcv_sel(pin, config->rcv_sel); > +#endif > +#endif > +} > + > +void pinmux_config_table(const struct pingroup_config *config, int len) > +{ > + int i; > + > + for (i = 0; i < len; i++) > + pinmux_config_pingroup(&config[i]); > +} > + > +#ifdef TEGRA_PMX_HAS_PADGRPS It's really unfortunately to add all these #ifdefs I think. I wonder if it might be possible to break the pinmux code into several files and use the Makefile to determine what support is brought in? > + > +#define pmux_padgrp_isvalid(pd) (((pd) >= 0) && ((pd) < > PDRIVE_PINGROUP_COUNT)) > + > +#define pmux_pad_slw_isvalid(slw) \ > + (((slw) >= PGRP_SLWF_MIN) && ((slw) <= PGRP_SLWF_MAX)) > + > +#define pmux_pad_drv_isvalid(drv) \ > + (((drv) >= PGRP_DRVUP_MIN) && ((drv) <= PGRP_DRVUP_MAX)) > + > +#define pmux_pad_lpmd_isvalid(lpm) \ > + (((lpm) >= PGRP_LPMD_X8) && ((lpm) <= PGRP_LPMD_X)) > + > +#define pmux_pad_schmt_isvalid(schmt) \ > + (((schmt) >= PGRP_SCHMT_DISABLE) && ((schmt) <= PGRP_SCHMT_ENABLE)) > + > +#define pmux_pad_hsm_isvalid(hsm) \ > + (((hsm) >= PGRP_HSM_DISABLE) && ((hsm) <= PGRP_HSM_ENABLE)) > + > +#define HSM_SHIFT 2 > +#define SCHMT_SHIFT 3 > +#define LPMD_SHIFT 4 > +#define LPMD_MASK (3 << LPMD_SHIFT) > +#define DRVDN_SHIFT 12 > +#define DRVDN_MASK (0x7F << DRVDN_SHIFT) > +#define DRVUP_SHIFT 20 > +#define DRVUP_MASK (0x7F << DRVUP_SHIFT) > +#define SLWR_SHIFT 28 > +#define SLWR_MASK (3 << SLWR_SHIFT) > +#define SLWF_SHIFT 30 > +#define SLWF_MASK (3 << SLWF_SHIFT) > + > +static void padgrp_set_drvup_slwf(enum pdrive_pingrp grp, int slwf) > +{ > + u32 *reg = DRV_REG(grp); > + u32 val; > + > + /* NONE means unspecified/do not change/use POR value */ > + if (slwf == PGRP_SLWF_NONE) > + return; > + > + /* Error check on pad and slwf */ > + assert(pmux_padgrp_isvalid(grp)); > + assert(pmux_pad_slw_isvalid(slwf)); > + > + val = readl(reg); > + val &= ~SLWF_MASK; > + val |= (slwf << SLWF_SHIFT); > + writel(val, reg); > + > + return; > +} > + > +static void padgrp_set_drvdn_slwr(enum pdrive_pingrp grp, int slwr) > +{ > + u32 *reg = DRV_REG(grp); > + u32 val; > + > + /* NONE means unspecified/do not change/use POR value */ > + if (slwr == PGRP_SLWR_NONE) > + return; > + > + /* Error check on pad and slwr */ > + assert(pmux_padgrp_isvalid(grp)); > + assert(pmux_pad_slw_isvalid(slwr)); > + > + val = readl(reg); > + val &= ~SLWR_MASK; > + val |= (slwr << SLWR_SHIFT); > + writel(val, reg); > + > + return; > +} > + > +static void padgrp_set_drvup(enum pdrive_pingrp grp, int drvup) > +{ > + u32 *reg = DRV_REG(grp); > + u32 val; > + > + /* NONE means unspecified/do not change/use POR value */ > + if (drvup == PGRP_DRVUP_NONE) > + return; > + > + /* Error check on pad and drvup */ > + assert(pmux_padgrp_isvalid(grp)); > + assert(pmux_pad_drv_isvalid(drvup)); > + > + val = readl(reg); > + val &= ~DRVUP_MASK; > + val |= (drvup << DRVUP_SHIFT); > + writel(val, reg); > + > + return; > +} > + > +static void padgrp_set_drvdn(enum pdrive_pingrp grp, int drvdn) > +{ > + u32 *reg = DRV_REG(grp); > + u32 val; > + > + /* NONE means unspecified/do not change/use POR value */ > + if (drvdn == PGRP_DRVDN_NONE) > + return; > + > + /* Error check on pad and drvdn */ > + assert(pmux_padgrp_isvalid(grp)); > + assert(pmux_pad_drv_isvalid(drvdn)); > + > + val = readl(reg); > + val &= ~DRVDN_MASK; > + val |= (drvdn << DRVDN_SHIFT); > + writel(val, reg); > + > + return; > +} > + > +static void padgrp_set_lpmd(enum pdrive_pingrp grp, enum pgrp_lpmd lpmd) > +{ > + u32 *reg = DRV_REG(grp); > + u32 val; > + > + /* NONE means unspecified/do not change/use POR value */ > + if (lpmd == PGRP_LPMD_NONE) > + return; > + > + /* Error check pad and lpmd value */ > + assert(pmux_padgrp_isvalid(grp)); > + assert(pmux_pad_lpmd_isvalid(lpmd)); > + > + val = readl(reg); > + val &= ~LPMD_MASK; > + val |= (lpmd << LPMD_SHIFT); > + writel(val, reg); You can use clrsetbits_le32() for this sort of thing if you like. > + > + return; > +} > + > +static void padgrp_set_schmt(enum pdrive_pingrp grp, enum pgrp_schmt schmt) > +{ > + u32 *reg = DRV_REG(grp); > + u32 val; > + > + /* NONE means unspecified/do not change/use POR value */ > + if (schmt == PGRP_SCHMT_NONE) > + return; > + > + /* Error check pad */ > + assert(pmux_padgrp_isvalid(grp)); > + assert(pmux_pad_schmt_isvalid(schmt)); > + > + val = readl(reg); > + if (schmt == PGRP_SCHMT_ENABLE) > + val |= (1 << SCHMT_SHIFT); > + else > + val &= ~(1 << SCHMT_SHIFT); > + writel(val, reg); > + > + return; > +} > + > +static void padgrp_set_hsm(enum pdrive_pingrp grp, enum pgrp_hsm hsm) > +{ > + u32 *reg = DRV_REG(grp); > + u32 val; > + > + /* NONE means unspecified/do not change/use POR value */ > + if (hsm == PGRP_HSM_NONE) > + return; > + > + /* Error check pad */ > + assert(pmux_padgrp_isvalid(grp)); > + assert(pmux_pad_hsm_isvalid(hsm)); > + > + val = readl(reg); > + if (hsm == PGRP_HSM_ENABLE) > + val |= (1 << HSM_SHIFT); > + else > + val &= ~(1 << HSM_SHIFT); > + writel(val, reg); > + > + return; > +} > + > +static void padctrl_config_pingroup(const struct padctrl_config *config) > +{ > + enum pdrive_pingrp grp = config->padgrp; > + > + padgrp_set_drvup_slwf(grp, config->slwf); > + padgrp_set_drvdn_slwr(grp, config->slwr); > + padgrp_set_drvup(grp, config->drvup); > + padgrp_set_drvdn(grp, config->drvdn); > + padgrp_set_lpmd(grp, config->lpmd); > + padgrp_set_schmt(grp, config->schmt); > + padgrp_set_hsm(grp, config->hsm); > +} > + > +void padgrp_config_table(const struct padctrl_config *config, int len) > +{ > + int i; > + > + for (i = 0; i < len; i++) > + padctrl_config_pingroup(&config[i]); > +} > +#endif /* TEGRA_PMX_HAS_PADGRPS */ Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot