[U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Simon Goldschmidt]

Marek Vasut <marex at denx.de> schrieb am Di., 20. Aug. 2019, 11:50:

> On 8/20/19 4:35 AM, Ley Foon Tan wrote:
> > Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
> > to defines.
> > No functional change.
> >
> > Signed-off-by: Ley Foon Tan <ley.foon.tan at intel.com>
> > ---
> >  .../mach-socfpga/include/mach/reset_manager.h | 12 +++++
> >  .../include/mach/reset_manager_arria10.h      | 41 +++-------------
> >  .../include/mach/reset_manager_gen5.h         | 20 +++-----
> >  .../include/mach/reset_manager_s10.h          | 33 ++-----------
> >  arch/arm/mach-socfpga/misc_gen5.c             |  6 +--
> >  arch/arm/mach-socfpga/reset_manager_arria10.c | 49 +++++++++----------
> >  arch/arm/mach-socfpga/reset_manager_gen5.c    | 26 +++++-----
> >  arch/arm/mach-socfpga/reset_manager_s10.c     | 35 ++++++-------
> >  drivers/sysreset/sysreset_socfpga.c           |  6 +--
> >  9 files changed, 86 insertions(+), 142 deletions(-)
> >
> > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > index 6ad037e325..c460e89d22 100644
> > --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > @@ -6,6 +6,18 @@
> >  #ifndef _RESET_MANAGER_H_
> >  #define _RESET_MANAGER_H_
> >
> > +#define RSTMGR_READL(reg)                    \
> > +     readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
> > +
> > +#define RSTMGR_WRITEL(data, reg)             \
> > +     writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
> > +
> > +#define RSTMGR_CLRBITS(reg, mask)            \
> > +     clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > +
> > +#define RSTMGR_SETBITS(reg, mask)            \
> > +     setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > +
>
> No, don't introduce such macros. Use readl()/writel()/... in the driver.
> The address should come from DT. Besides, there is no type checking in
> such macros.
>
> >  void reset_cpu(ulong addr);
> >
> >  void socfpga_per_reset(u32 reset, int set);
> > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > index 6623ebee65..8b72f41498 100644
> > --- a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > @@ -14,40 +14,13 @@ int socfpga_reset_deassert_bridges_handoff(void);
> >  void socfpga_reset_deassert_osc1wd0(void);
> >  int socfpga_bridges_reset(void);
> >
> > -struct socfpga_reset_manager {
> > -     u32     stat;
> > -     u32     ramstat;
> > -     u32     miscstat;
> > -     u32     ctrl;
> > -     u32     hdsken;
> > -     u32     hdskreq;
> > -     u32     hdskack;
> > -     u32     counts;
> > -     u32     mpumodrst;
> > -     u32     per0modrst;
> > -     u32     per1modrst;
> > -     u32     brgmodrst;
> > -     u32     sysmodrst;
> > -     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;
> > -};
> > +#define RSTMGR_STATUS                0
> > +#define RSTMGR_CTRL          0xc
> > +#define RSTMGR_MPUMODRST     0x20
> > +#define RSTMGR_PER0MODRST    0x24
> > +#define RSTMGR_PER1MODRST    0x28
> > +#define RSTMGR_BRGMODRST     0x2c
> > +#define RSTMGR_SYSMODRST     0x30
>
> It would be much better to have some SOCFPGA_ prefix here, to clearly
> identify those macros. Also, you are missing quite a few registers.
>

I agree on the prefix, but I thought leaving away unused registers was one
of the positive sides of this change?

That also allows using the same defines for S10 and Agilex even if some
registers are different. That's how we came to this patchset, if I remember
correctly.

Regards,
Simon