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

Simon Goldschmidt
2019-08-20T09:55:53Z

Marek Vasut <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>
> > ---
> > .../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