[U-Boot] [PATCH v3 3/9] ARM: stm32: cleanup stm32f7 files

[Vikas MANOCHA]

Thanks Joe for your feedback,

> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershberger at gmail.com]
> Sent: Thursday, December 01, 2016 11:13 AM
> To: Vikas MANOCHA <vikas.manocha at st.com>
> Cc: Michael Kurz <michi.kurz at gmail.com>; u-boot at lists.denx.de; Toshifumi NISHINAGA <tnishinaga.dev at gmail.com>; Albert
> Aribaud <albert.u.boot at aribaud.net>; Joe Hershberger <joe.hershberger at ni.com>; Kamil Lulko <kamil.lulko at gmail.com>
> Subject: Re: [U-Boot] [PATCH v3 3/9] ARM: stm32: cleanup stm32f7 files
> 
> On Thu, Dec 1, 2016 at 1:09 PM, Vikas MANOCHA <vikas.manocha at st.com> wrote:
> > Hi Joe,
> >
> >> -----Original Message-----
> >> From: Joe Hershberger [mailto:joe.hershberger at gmail.com]
> >> Sent: Thursday, December 01, 2016 10:42 AM
> >> To: Vikas MANOCHA <vikas.manocha at st.com>
> >> Cc: Michael Kurz <michi.kurz at gmail.com>; u-boot at lists.denx.de;
> >> Toshifumi NISHINAGA <tnishinaga.dev at gmail.com>; Albert Aribaud
> >> <albert.u.boot at aribaud.net>; Joe Hershberger
> >> <joe.hershberger at ni.com>; Kamil Lulko <kamil.lulko at gmail.com>
> >> Subject: Re: [U-Boot] [PATCH v3 3/9] ARM: stm32: cleanup stm32f7
> >> files
> >>
> >> On Thu, Dec 1, 2016 at 12:18 PM, vikas <vikas.manocha at st.com> wrote:
> >> > Hi Michael,
> >> >
> >> > On 11/24/2016 11:10 AM, Michael Kurz wrote:
> >> >> Cleanup stm32f7 files:
> >> >> - use BIT macro
> >> >> - use GENMASK macro
> >> >
> >> > good.
> >> >
> >> >> - use rcc struct instead of macro additions
> >> >
> >> > Macro definitions are better than struct to make rcc compatible
> >> > throughout the stm32f7 family in case of additional registers and
> >> also to reuse it for stm32f4. At present we cant use same rcc struct
> >> for stm32f4 and stm32f7 because of two additional registers in stm32f7.
> >> > So keep the macros for rcc, we would move it for both stm32f7 and stm32f4.
> >>
> >> Just 2 extra regs, or they change the position of the existing regs?
> >
> > only extra registers (infact only 1 extra register, not 2).
> 
> If just extra, then use the struct. Who cares if there is an extra reg that you don't use?

The extra register is to select the clock source for peripherals like u(s)arts, i2c  etc. I don’t want to neglect it.
But again, today its one, tomorrow there would be more. Off-course the locations and definitions might change (unlikey) but macros provide better compatibility.
My first choice is also structures as it makes debugging/maintenance easier but it not scalable. Do you agree ?

Another approach could be to have another additional struct like "rcc_ext_f7" pointing to end of "rcc_common" struct. What's your opinion about it ?

Cheers,
Vikas

> 
> >>
> >> > rcc struct shouldn't be in for stm32f7 in first place, the last patch which added it went unnoticed.
> >> >
> >> >>
> >> >> Signed-off-by: Michael Kurz <michi.kurz at gmail.com>
> >> >>
> >> >> ---
> >> >>
> >> >> Changes in v3:
> >> >> - Removed 'prefix all constants with STM32_'
> >> >> - Reverted move of header into source file (rcc.h -> clock.c)
> >> >>
> >> >> Changes in v2:
> >> >> - Add cleanup patch
> >> >>
> >> >>  arch/arm/include/asm/arch-stm32f7/fmc.h    |   6 +-
> >> >>  arch/arm/include/asm/arch-stm32f7/gpt.h    |   6 +-
> >> >>  arch/arm/include/asm/arch-stm32f7/rcc.h    |  50 ++++++----
> >> >>  arch/arm/include/asm/arch-stm32f7/stm32.h  |   8 +-
> >> >>  arch/arm/mach-stm32/stm32f7/clock.c        | 154 ++++++++++++-----------------
> >> >>  board/st/stm32f746-disco/stm32f746-disco.c |   7 +-
> >> >>  6 files changed, 107 insertions(+), 124 deletions(-)
> >> >>
> >> >> diff --git a/arch/arm/include/asm/arch-stm32f7/fmc.h
> >> >> b/arch/arm/include/asm/arch-stm32f7/fmc.h
> >> >> index 7dd5077..d61a86f 100644
> >> >> --- a/arch/arm/include/asm/arch-stm32f7/fmc.h
> >> >> +++ b/arch/arm/include/asm/arch-stm32f7/fmc.h
> >> >> @@ -58,12 +58,12 @@ struct stm32_fmc_regs {
> >> >>  #define FMC_SDCMR_MODE_SELFREFRESH     5
> >> >>  #define FMC_SDCMR_MODE_POWERDOWN       6
> >> >>
> >> >> -#define FMC_SDCMR_BANK_1               (1 << 4)
> >> >> -#define FMC_SDCMR_BANK_2               (1 << 3)
> >> >> +#define FMC_SDCMR_BANK_1               BIT(4)
> >> >> +#define FMC_SDCMR_BANK_2               BIT(3)
> >> >>
> >> >
> >> > [...]