[U-Boot] [v3 25/30] arm: socfpga: arria10: Added drivers for Arria10 Reset Manager
Chee, Tien Fong
tien.fong.chee at intel.com
Mon Jan 9 12:11:14 CET 2017
On Jum, 2017-01-06 at 17:03 -0600, Dinh Nguyen wrote:
>
> On 01/06/2017 05:19 AM, Chee Tien Fong wrote:
> >
> > From: Tien Fong Chee <tien.fong.chee at intel.com>
> >
> > Drivers for reset manager is restructured such that common
> > functions,
> > gen5 drivers and Arria10 drivers are moved to reset_manager.c,
> > reset_manager_gen5.c and reset_manager_arria10.c respectively.
> >
> In Linux, I was able to use the same reset manager driver to support
> both gen5 and Arria10 devices, are you sure you can't do the same
> here?
>
> Take a look at this commit:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commi
> /drivers/rese/resetsocfpga.c?id=27e44646dc0083c931b71bbb8e179aeb38010
> d31
>
> My guess is that you can probably use the same driver but with
> different
> macro defines.
>
We have some prepocessing required before triggering the simple reset
assert/deassert on the right peripheral. Some new functions are created
for specific group reset purpose like all bridges reset based on
handoff. As per agreement between marek and CL, those functions merging
which require #ifdef arria10/gen5, then for the sake of clean code, it
is better putting them in different files based on device type.
> >
> > Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
> > Cc: Marek Vasut <marex at denx.de>
> > Cc: Dinh Nguyen <dinguyen at kernel.org>
> > Cc: Chin Liang See <chin.liang.see at intel.com>
> > Cc: Tien Fong <skywindctf at gmail.com>
> > ---
> > Changes for V3
> > - no changes
> > Changes for V2
> > - Reverted license changes, removing extern and volatile
> > declaration
> > ---
> > arch/arm/mach-socfpga/Makefile | 16 +-
> > arch/arm/mach-socfpga/include/mach/reset_manager.h | 155 ++++++--
> > arch/arm/mach-socfpga/reset_manager.c | 112 +------
> > arch/arm/mach-socfpga/reset_manager_arria10.c | 407
> > ++++++++++++++++++++
> > .../{reset_manager.c => reset_manager_gen5.c} | 94 ++---
> > 5 files changed, 570 insertions(+), 214 deletions(-)
> > create mode 100644 arch/arm/mach-socfpga/reset_manager_arria10.c
> > copy arch/arm/mach-socfpga/{reset_manager.c =>
> > reset_manager_gen5.c} (58%)
> >
> > diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-
> > socfpga/Makefile
> > index 809cd47..b8fcf6e 100644
> > --- a/arch/arm/mach-socfpga/Makefile
> > +++ b/arch/arm/mach-socfpga/Makefile
> > @@ -2,21 +2,27 @@
> > # (C) Copyright 2000-2003
> > # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> > #
> > -# Copyright (C) 2012 Altera Corporation <www.altera.com>
> > +# Copyright (C) 2012-2016 Altera Corporation <www.altera.com>
> > #
> > # SPDX-License-Identifier: GPL-2.0+
> > #
> >
> > obj-y += misc.o timer.o reset_manager.o system_manager.o
> > clock_manager.o \
> > fpga_manager.o board.o
> > +obj-$(CONFIG_TARGET_SOCFPGA_ARRIA10) += reset_manager_arria10.o
> > +obj-$(CONFIG_TARGET_SOCFPGA_GEN5) += scan_manager.o
> > wrap_pll_config.o \
> > + reset_manager_gen5.o
> >
> > -obj-$(CONFIG_SPL_BUILD) += spl.o freeze_controller.o
> > +ifdef CONFIG_SPL_BUILD
> > +obj-y += spl.o
> > +obj-$(CONFIG_TARGET_SOCFPGA_GEN5) += freeze_controller.o
> > wrap_iocsr_config.o \
> > + wrap_pinmux_config.o
> > wrap_sdram_config.o
> > +endif
> >
> > +ifdef CONFIG_TARGET_SOCFPGA_GEN5
> > # QTS-generated config file wrappers
> > -obj-$(CONFIG_TARGET_SOCFPGA_GEN5) += scan_manager.o
> > wrap_pll_config.o
> > -obj-$(CONFIG_SPL_BUILD) += wrap_iocsr_config.o
> > wrap_pinmux_config.o \
> > - wrap_sdram_config.o
> > CFLAGS_wrap_iocsr_config.o +=
> > -I$(srctree)/board/$(BOARDDIR)
> > CFLAGS_wrap_pinmux_config.o +=
> > -I$(srctree)/board/$(BOARDDIR)
> > CFLAGS_wrap_pll_config.o += -I$(srctree)/board/$(BOARDDIR)
> > CFLAGS_wrap_sdram_config.o +=
> > -I$(srctree)/board/$(BOARDDIR)
> > +endif
> > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > index 6225118..13f9731 100644
> > --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (C) 2012 Altera Corporation <www.altera.com>
> > + * Copyright (C) 2012-2016 Altera Corporation <www.altera.com>
> > *
> > * SPDX-License-Identifier: GPL-2.0+
> > */
> > @@ -7,15 +7,27 @@
> > #ifndef _RESET_MANAGER_H_
> > #define _RESET_MANAGER_H_
> >
> > +/* Common function prototypes */
> > void reset_cpu(ulong addr);
> > -void reset_deassert_peripherals_handoff(void);
> > -
> > void socfpga_bridges_reset(int enable);
> > -
> > void socfpga_per_reset(u32 reset, int set);
> > void socfpga_per_reset_all(void);
> >
> > #if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> > +void reset_deassert_peripherals_handoff(void);
> > +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > +void watchdog_disable(void);
> > +void reset_deassert_noc_ddr_scheduler(void);
> > +int is_wdt_in_reset(void);
> > +void emac_manage_reset(ulong emacbase, uint state);
> > +int reset_deassert_bridges_handoff(void);
> > +void reset_assert_fpga_connected_peripherals(void);
> > +void reset_deassert_osc1wd0(void);
> > +void reset_assert_uart(void);
> > +void reset_deassert_uart(void);
> Not sure why you need these new functions instead of re-using
> socfpga_per_reset()?
>
Those reset functions are not just simple reset, there are some
preprocessing or populating based on handoff required before triggering
reset on right peripheral. For example, U-boot can reset deassert
correct UART port based on the handoff. So, we tidy up those
preprocessing and reset into one function. But for the
socfpga_per_reset, we have different register name in both
gen5/arria10, so i prefer to put them in separate file based on device
type. i believe it helps to avoid confusion when mapping the name to
our registers datasheet.
> >
> > +#endif
> > +
> > +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> > struct socfpga_reset_manager {
> > u32 status;
> > u32 ctrl;
> > @@ -29,40 +41,40 @@ struct socfpga_reset_manager {
> > u32 padding2[12];
> > u32 tstscratch;
> > };
> > -#else
> > +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > struct socfpga_reset_manager {
> > - u32 stat;
> > - u32 ramstat;
> > - u32 miscstat;
> > - u32 ctrl;
> > - u32 hdsken;
> > - u32 hdskreq;
> > - u32 hdskack;
> > - u32 counts;
> > - u32 mpu_mod_reset;
> > - u32 per_mod_reset; /* stated as
> > per0_mod_reset in A10 datasheet */
> > - u32 per2_mod_reset; /* stated as
> > per1_mod_reset in A10 datasheet */
> > - u32 brg_mod_reset;
> > - u32 misc_mod_reset; /* stated as
> > sys_mod_reset in A10 datasheet */
> > - u32 coldmodrst;
> > - u32 nrstmodrst;
> > - u32 dbgmodrst;
> > - u32 mpuwarmmask;
> > - u32 per0warmmask;
> > - u32 per1warmmask;
> > - u32 brgwarmmask;
> > - u32 syswarmmask;
> > - u32 nrstwarmmask;
> > - u32 l3warmmask;
> > - u32 tststa;
> > - u32 tstscratch;
> > - u32 hdsktimeout;
> > - u32 hmcintr;
> > - u32 hmcintren;
> > - u32 hmcintrens;
> > - u32 hmcintrenr;
> > - u32 hmcgpout;
> > - u32 hmcgpin;
> > + uint32_t stat;
> > + uint32_t ramstat;
> > + uint32_t miscstat;
> > + uint32_t ctrl;
> > + uint32_t hdsken;
> > + uint32_t hdskreq;
> > + uint32_t hdskack;
> > + uint32_t counts;
> > + uint32_t mpumodrst;
> > + uint32_t per0modrst;
> > + uint32_t per1modrst;
> > + uint32_t brgmodrst;
> > + uint32_t sysmodrst;
> > + uint32_t coldmodrst;
> > + uint32_t nrstmodrst;
> > + uint32_t dbgmodrst;
> > + uint32_t mpuwarmmask;
> > + uint32_t per0warmmask;
> > + uint32_t per1warmmask;
> > + uint32_t brgwarmmask;
> > + uint32_t syswarmmask;
> > + uint32_t nrstwarmmask;
> > + uint32_t l3warmmask;
> > + uint32_t tststa;
> > + uint32_t tstscratch;
> > + uint32_t hdsktimeout;
> > + uint32_t hmcintr;
> > + uint32_t hmcintren;
> > + uint32_t hmcintrens;
> > + uint32_t hmcintrenr;
> > + uint32_t hmcgpout;
> > + uint32_t hmcgpin;
> > };
> > #endif
> >
> Why did you have to change the type for these reset manager
> variables?
>
> Dinh
I recalled i did some checking on these two types, for u32 type, the
build support on this type can be turned off, and some comment for the
limitation of using this type, like "avoid exported outside the kernel
to avoid name space clashes". However, uint32_t is widely accepted and
using in U-boot. For details, i think you need to dig more from the
codes, then i believe you will think uint32_t is better.
More information about the U-Boot
mailing list