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

[Marek Vasut]

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

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.