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

[Michael Kurz]

Hi Vikas,

On Thu, 1 Dec 2016, Vikas MANOCHA wrote:

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

In my opinion split the struct in two structs like you suggested would be 
the best solution. But i think that is mostly based on personal 
preference.

I ordered a stm32f429-disco board so I could change (and test the changes) 
for both boards once it arrives. Or i can revert the changes and use the 
macros. I'm don't have any strong preference to either one.

Regards,
Michael

>>
>>>>
>>>>> 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)
>>>>>>
>>>>>
>>>>> [...]
>