| Message ID | CAEM3b1Aajpt-V1YJCpAJa+ZZ5o7qF5OaUD2rVB-d+YQD75j-jQ@mail.gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
Hi! > socfpga: Consolidating reset code into reset_manager.c. Also separating > reset configuration for virtual target and real hardware Cyclone V > development kit > > Signed-off-by: Chin Liang See <clsee@altera.com> > +++ b/arch/arm/cpu/armv7/socfpga/reset_manager.c > @@ -0,0 +1,50 @@ > +/* > + * Copyright Altera Corporation (C) <2013>. All rights reserved > + * > + * This program is free software; you can redistribute it and/or > +modify it > + * under the terms and conditions of the GNU General Public I sense some word wrapping... > @@ -21,6 +21,7 @@ > void reset_cpu(ulong addr); > void reset_deassert_peripherals_handoff(void); > > +#if defined(CONFIG_SOCFPGA_VIRTUAL_TARGET) > struct socfpga_reset_manager { > u32 padding1; > u32 ctrl; > @@ -31,7 +32,23 @@ struct socfpga_reset_manager { > u32 per2_mod_reset; > u32 brg_mod_reset; > }; > +#else > +struct socfpga_reset_manager { > + u32 status; > + u32 ctrl; > + u32 counts; > + u32 padding1; > + u32 mpu_mod_reset; > + u32 per_mod_reset; > + u32 per2_mod_reset; > + u32 brg_mod_reset; > +}; > +#endif > Is it really needed to have two definitions of the struct? AFAICT, structures are same, except that some padding fields have names on real hardware. Thus, if we simply use "real-hardware" version on the emulator, it should work. Perhaps with some comments "this is not emulated on virtual target"...? Thanks, Pavel
Hi Pavel, On Fri, 2013-06-28 at 13:44 +0200, ZY - pavel wrote: > Hi! > > > socfpga: Consolidating reset code into reset_manager.c. Also separating > > reset configuration for virtual target and real hardware Cyclone V > > development kit > > > > Signed-off-by: Chin Liang See <clsee@altera.com> > > > +++ b/arch/arm/cpu/armv7/socfpga/reset_manager.c > > @@ -0,0 +1,50 @@ > > +/* > > + * Copyright Altera Corporation (C) <2013>. All rights reserved > > + * > > + * This program is free software; you can redistribute it and/or > > +modify it > > + * under the terms and conditions of the GNU General Public > > I sense some word wrapping... Noted and will send new patch > > > @@ -21,6 +21,7 @@ > > void reset_cpu(ulong addr); > > void reset_deassert_peripherals_handoff(void); > > > > +#if defined(CONFIG_SOCFPGA_VIRTUAL_TARGET) > > struct socfpga_reset_manager { > > u32 padding1; > > u32 ctrl; > > @@ -31,7 +32,23 @@ struct socfpga_reset_manager { > > u32 per2_mod_reset; > > u32 brg_mod_reset; > > }; > > +#else > > +struct socfpga_reset_manager { > > + u32 status; > > + u32 ctrl; > > + u32 counts; > > + u32 padding1; > > + u32 mpu_mod_reset; > > + u32 per_mod_reset; > > + u32 per2_mod_reset; > > + u32 brg_mod_reset; > > +}; > > +#endif > > > > Is it really needed to have two definitions of the struct? AFAICT, > structures are same, except that some padding fields have names on > real hardware. Thus, if we simply use "real-hardware" version on the > emulator, it should work. Perhaps with some comments "this is not > emulated on virtual target"...? > We decided to leave the Virtual Platform code support within existing code. We need to do that as we have some discrepancy between the real hardware and the virtual platform. But this is only applicable for Altera specific IP. :) Thanks Chin Liang > Thanks, > Pavel
Hi! > > > @@ -21,6 +21,7 @@ > > > void reset_cpu(ulong addr); > > > void reset_deassert_peripherals_handoff(void); > > > > > > +#if defined(CONFIG_SOCFPGA_VIRTUAL_TARGET) > > > struct socfpga_reset_manager { > > > u32 padding1; > > > u32 ctrl; > > > @@ -31,7 +32,23 @@ struct socfpga_reset_manager { > > > u32 per2_mod_reset; > > > u32 brg_mod_reset; > > > }; > > > +#else > > > +struct socfpga_reset_manager { > > > + u32 status; > > > + u32 ctrl; > > > + u32 counts; > > > + u32 padding1; > > > + u32 mpu_mod_reset; > > > + u32 per_mod_reset; > > > + u32 per2_mod_reset; > > > + u32 brg_mod_reset; > > > +}; > > > +#endif > > > > > > > Is it really needed to have two definitions of the struct? AFAICT, > > structures are same, except that some padding fields have names on > > real hardware. Thus, if we simply use "real-hardware" version on the > > emulator, it should work. Perhaps with some comments "this is not > > emulated on virtual target"...? > > We decided to leave the Virtual Platform code support within existing > code. We need to do that as we have some discrepancy between the real > hardware and the virtual platform. But this is only applicable for > Altera specific IP. :) That is okay... But notice that structure is same on both real hardware and virtual platform... (Just some fields have "paddingX" instead of name on virtual platform). If you remove the #ifdef it will work just fine. (You could add /* this is unimplemented on virtual platform */, or maybe even per-field ifdef. It will still be more readable.) Thanks, Pavel
Hi Pavel, On Mon, 2013-07-01 at 12:46 +0200, ZY - pavel wrote: > Hi! > > > > > @@ -21,6 +21,7 @@ > > > > void reset_cpu(ulong addr); > > > > void reset_deassert_peripherals_handoff(void); > > > > > > > > +#if defined(CONFIG_SOCFPGA_VIRTUAL_TARGET) > > > > struct socfpga_reset_manager { > > > > u32 padding1; > > > > u32 ctrl; > > > > @@ -31,7 +32,23 @@ struct socfpga_reset_manager { > > > > u32 per2_mod_reset; > > > > u32 brg_mod_reset; > > > > }; > > > > +#else > > > > +struct socfpga_reset_manager { > > > > + u32 status; > > > > + u32 ctrl; > > > > + u32 counts; > > > > + u32 padding1; > > > > + u32 mpu_mod_reset; > > > > + u32 per_mod_reset; > > > > + u32 per2_mod_reset; > > > > + u32 brg_mod_reset; > > > > +}; > > > > +#endif > > > > > > > > > > Is it really needed to have two definitions of the struct? AFAICT, > > > structures are same, except that some padding fields have names on > > > real hardware. Thus, if we simply use "real-hardware" version on the > > > emulator, it should work. Perhaps with some comments "this is not > > > emulated on virtual target"...? > > > > We decided to leave the Virtual Platform code support within existing > > code. We need to do that as we have some discrepancy between the real > > hardware and the virtual platform. But this is only applicable for > > Altera specific IP. :) > > That is okay... But notice that structure is same on both real > hardware and virtual platform... (Just some fields have "paddingX" > instead of name on virtual platform). If you remove the #ifdef it will > work just fine. > > (You could add /* this is unimplemented on virtual platform */, or > maybe even per-field ifdef. It will still be more readable.) Oh.. I got your point now :) Its a good suggestion and let me do it for next revision. Chin Liang > > Thanks, > Pavel
Hi! > > > > > @@ -21,6 +21,7 @@ > > > > > void reset_cpu(ulong addr); > > > > > void reset_deassert_peripherals_handoff(void); > > > > > > > > > > +#if defined(CONFIG_SOCFPGA_VIRTUAL_TARGET) > > > > > struct socfpga_reset_manager { > > > > > u32 padding1; > > > > > u32 ctrl; > > > > > @@ -31,7 +32,23 @@ struct socfpga_reset_manager { > > > > > u32 per2_mod_reset; > > > > > u32 brg_mod_reset; > > > > > }; > > > > > +#else > > > > > +struct socfpga_reset_manager { > > > > > + u32 status; > > > > > + u32 ctrl; > > > > > + u32 counts; > > > > > + u32 padding1; > > > > > + u32 mpu_mod_reset; > > > > > + u32 per_mod_reset; > > > > > + u32 per2_mod_reset; > > > > > + u32 brg_mod_reset; > > > > > +}; > > > > > +#endif > > > > > > > (You could add /* this is unimplemented on virtual platform */, or > > maybe even per-field ifdef. It will still be more readable.) > > Oh.. I got your point now :) > Its a good suggestion and let me do it for next revision. Looks good now. Thanks! Pavel
diff --git a/arch/arm/cpu/armv7/socfpga/Makefile b/arch/arm/cpu/armv7/socfpga/Makefile index 376a4bd..518e67a 100644 --- a/arch/arm/cpu/armv7/socfpga/Makefile +++ b/arch/arm/cpu/armv7/socfpga/Makefile @@ -29,7 +29,7 @@ include $(TOPDIR)/config.mk LIB = $(obj)lib$(SOC).o SOBJS := lowlevel_init.o -COBJS-y := misc.o timer.o +COBJS-y := misc.o timer.o reset_manager.o COBJS-$(CONFIG_SPL_BUILD) += spl.o COBJS := $(COBJS-y) diff --git a/arch/arm/cpu/armv7/socfpga/misc.c b/arch/arm/cpu/armv7/socfpga/misc.c index fa16424..59f5b94 100644 --- a/arch/arm/cpu/armv7/socfpga/misc.c +++ b/arch/arm/cpu/armv7/socfpga/misc.c @@ -17,36 +17,9 @@ #include <common.h> #include <asm/io.h> -#include <asm/arch/reset_manager.h> DECLARE_GLOBAL_DATA_PTR; -static const struct socfpga_reset_manager *reset_manager_base = - (void *)SOCFPGA_RSTMGR_ADDRESS; - -/* - * Write the reset manager register to cause reset - */ -void reset_cpu(ulong addr) -{ - /* request a warm reset */ - writel(RSTMGR_CTRL_SWWARMRSTREQ_LSB, &reset_manager_base->ctrl); - /* - * infinite loop here as watchdog will trigger and reset - * the processor - */ - while (1) - ; -} - -/* - * Release peripherals from reset based on handoff - */ -void reset_deassert_peripherals_handoff(void) -{ - writel(0, &reset_manager_base->per_mod_reset); -} - int dram_init(void) { gd->ram_size = get_ram_size((long *)PHYS_SDRAM_1, PHYS_SDRAM_1_SIZE); diff --git a/arch/arm/cpu/armv7/socfpga/reset_manager.c b/arch/arm/cpu/armv7/socfpga/reset_manager.c new file mode 100644 index 0000000..b0cc399 --- /dev/null +++ b/arch/arm/cpu/armv7/socfpga/reset_manager.c @@ -0,0 +1,50 @@ +/* + * Copyright Altera Corporation (C) <2013>. All rights reserved + * + * This program is free software; you can redistribute it and/or +modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but +WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY +or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public +License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <common.h> +#include <asm/io.h> +#include <asm/arch/reset_manager.h> + +DECLARE_GLOBAL_DATA_PTR; + +static const struct socfpga_reset_manager *reset_manager_base = + (void *)SOCFPGA_RSTMGR_ADDRESS; + +/* + * Write the reset manager register to cause reset */ void +reset_cpu(ulong addr) { + /* request a warm reset */ + writel((1 << RSTMGR_CTRL_SWWARMRSTREQ_LSB), + &reset_manager_base->ctrl); + /* + * infinite loop here as watchdog will trigger and reset + * the processor + */ + while (1) + ; +} + +/* + * Release peripherals from reset based on handoff */ void +reset_deassert_peripherals_handoff(void) +{ + writel(0, &reset_manager_base->per_mod_reset); +} + + diff --git a/arch/arm/include/asm/arch-socfpga/reset_manager.h b/arch/arm/include/asm/arch-socfpga/reset_manager.h index d9d2c1c..58d85e3 100644 --- a/arch/arm/include/asm/arch-socfpga/reset_manager.h +++ b/arch/arm/include/asm/arch-socfpga/reset_manager.h @@ -21,6 +21,7 @@ void reset_cpu(ulong addr); void reset_deassert_peripherals_handoff(void); +#if defined(CONFIG_SOCFPGA_VIRTUAL_TARGET) struct socfpga_reset_manager { u32 padding1; u32 ctrl; @@ -31,7 +32,23 @@ struct socfpga_reset_manager { u32 per2_mod_reset; u32 brg_mod_reset; }; +#else +struct socfpga_reset_manager { + u32 status; + u32 ctrl; + u32 counts; + u32 padding1; + u32 mpu_mod_reset; + u32 per_mod_reset; + u32 per2_mod_reset; + u32 brg_mod_reset; +}; +#endif +#if defined(CONFIG_SOCFPGA_VIRTUAL_TARGET) +#define RSTMGR_CTRL_SWWARMRSTREQ_LSB 2 +#else #define RSTMGR_CTRL_SWWARMRSTREQ_LSB 1 +#endif #endif /* _RESET_MANAGER_H_ */
socfpga: Consolidating reset code into reset_manager.c. Also separating reset configuration for virtual target and real hardware Cyclone V development kit Signed-off-by: Chin Liang See <clsee@altera.com> --- arch/arm/cpu/armv7/socfpga/Makefile | 2 +- arch/arm/cpu/armv7/socfpga/misc.c | 27 ----------- arch/arm/cpu/armv7/socfpga/reset_manager.c | 50 +++++++++++++++++++++ arch/arm/include/asm/arch-socfpga/reset_manager.h | 17 +++++++ 4 files changed, 68 insertions(+), 28 deletions(-) create mode 100644 arch/arm/cpu/armv7/socfpga/reset_manager.c -- 1.7.7.4 Confidentiality Notice. This message may contain information that is confidential or otherwise protected from disclosure. If you are not the intended recipient, you are hereby notified that any use, disclosure, dissemination, distribution, or copying of this message, or any attachments, is strictly prohibited. If you have received this message in error, please advise the sender by reply e-mail, and delete the message and any attachments. Thank you.