Patchwork [U-Boot,1/1] socfpga: Consolidating reset code into reset_manager.c. Also separating reset configuration for virtual target and real hardware Cyclone V development kit

login
register
mail settings
Submitter Chin Liang See
Date June 27, 2013, 1:27 p.m.
Message ID <CAEM3b1Aajpt-V1YJCpAJa+ZZ5o7qF5OaUD2rVB-d+YQD75j-jQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/255082/
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Comments

Chin Liang See - June 27, 2013, 1:27 p.m.
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.
Pavel Machek - June 28, 2013, 11:44 a.m.
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
Chin Liang See - June 28, 2013, 4:22 p.m.
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
Pavel Machek - July 1, 2013, 10:46 a.m.
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
Chin Liang See - July 1, 2013, 1:43 p.m.
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
Pavel Machek - July 2, 2013, 11:52 a.m.
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

Patch

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_ */