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

Ley Foon Tan
2019-08-21T01:50:40Z

On Tue, Aug 20, 2019 at 5:50 PM Marek Vasut <marex@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@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