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

[Ley Foon Tan]

On Tue, Aug 20, 2019 at 5:50 PM Marek Vasut <marex at denx.de> wrote:
>
> 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.
These macros call to writel() and readl() underlying, it just add base
address to it.
So, it should have type checking as call writel()/readl() directly?
If want to use address from DT, we need get address every time need to
use the address.
For non-DM, it is easier to use constant address. Let me know if you
still prefer to use address from DT.
>
> >  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.
There are few registers same across all devices and use in their
common functions. These common registers need to have same macro
names.

For those different registers, they will be something like these
(macro names will be longer):
SOCFPGA_A10_RSTMGR_MPUMODRST
SOCFPGA_G5_RSTMGR_MPUMODRST
SOCFPGA_S10_RSTMGR_MPUMODRST
SOCFPGA_AGILEX_RSTMGR_MPUMODRST

Regards
Ley Foon