[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