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

Marek Vasut
2019-08-20T10:15:13Z

On 8/20/19 11:55 AM, Simon Goldschmidt wrote:
>
>
> Marek Vasut <marex at denx.de <mailto:marex@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@intel.com
> <mailto:ley.foon.tan@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.

What happens if you want to use that register then ? I guess you can add
it to the list, although it feels a bit strange. But maybe that's OK.
The different registers should then probably have some SOCFPGA_<SOC>_
prefix or something.