[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, 12:15:

> 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.
>

Yes, it's a bit strange, but on the other hand, we have some big structs
where we only use some registers, and I think that makes it hard to read.

The different registers should

then probably have some SOCFPGA_<SOC>_
> prefix or something.
>