[U-Boot] [PATCH] Convert CONFIG_SYS_DV_CLKMODE et al to Kconfig

Thu Jan 11 18:49:53 UTC 2018

On 01/11/2018 12:36 PM, Adam Ford wrote:
> On Thu, Jan 11, 2018 at 12:11 PM, David Lechner <david at lechnology.com> wrote:
>> On 01/11/2018 10:06 AM, Adam Ford wrote:
>>>
>>> This converts the following to Kconfig:
>>>      CONFIG_SYS_DV_CLKMODE
>>>      CONFIG_SYS_DA850_PLL0_POSTDIV
>>>      CONFIG_SYS_DA850_PLL0_PLLDIV1
>>>      CONFIG_SYS_DA850_PLL0_PLLDIV2
>>>      CONFIG_SYS_DA850_PLL0_PLLDIV3
>>>      CONFIG_SYS_DA850_PLL0_PLLDIV4
>>>      CONFIG_SYS_DA850_PLL0_PLLDIV5
>>>      CONFIG_SYS_DA850_PLL0_PLLDIV7
>>>      CONFIG_SYS_DA850_PLL1_POSTDIV
>>>      CONFIG_SYS_DA850_PLL1_PLLDIV1
>>>      CONFIG_SYS_DA850_PLL1_PLLDIV2
>>>      CONFIG_SYS_DA850_PLL1_PLLDIV3
>>>
>>> Signed-off-by: Adam Ford <aford173 at gmail.com>
>>> ---
>>>
>>> This patch is a continuation of Convert CONFIG_SOC_DA8XX et al
>>> This also showed warnings when used with the legoev3_defconfig, however
>>> it seems like the ev3 board was defining things in the header it didn't
>>> need
>>> since it doesn't seem to be building da850_lowlevel.c.  I don't have the
>>> hardware to test that board.
>>
>>
>> I have a whole series of patches cleaning up the LEGO EV3 board and adding
>> device tree support, but I have been holding off sending because it
>> conflicts
>> with the changes you have been making. There is quite a bit of stuff there
>> that was cargo-culted and is going away, including the stuff that is causing
>> warnings here.
>>
>>
>>>
>>>    arch/arm/mach-davinci/Kconfig   | 59
>>> ++++++++++++++++++++++++++++++++++++++++-
>>>    configs/omapl138_lcdk_defconfig |  1 +
>>>    include/configs/calimain.h      | 13 ---------
>>>    include/configs/da850evm.h      | 13 ---------
>>>    include/configs/ipam390.h       | 13 ---------
>>>    include/configs/legoev3.h       | 13 ---------
>>>    include/configs/omapl138_lcdk.h | 13 ---------
>>>    scripts/config_whitelist.txt    | 12 ---------
>>>    8 files changed, 59 insertions(+), 78 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig
>>> index ae9c0fd..c9c517e 100644
>>> --- a/arch/arm/mach-davinci/Kconfig
>>> +++ b/arch/arm/mach-davinci/Kconfig
>>> @@ -24,7 +24,7 @@ config TARGET_EA20
>>>      config TARGET_OMAPL138_LCDK
>>>          bool "OMAPL138 LCDK"
>>> -       select SOC_DA8XX
>>> +       select SOC_DA850
>>>          select SUPPORT_SPL
>>>      config TARGET_CALIMAIN
>>> @@ -63,6 +63,63 @@ config SOC_DA8XX
>>>    config MACH_DAVINCI_DA850_EVM
>>>          bool
>>>    +if SYS_DA850_PLL_INIT
>>> +comment "DA850 PLL Initialization Parameters"
>>> +
>>> +config SYS_DV_CLKMODE
>>> +       int "SYS_DV_CLKMODE"
>>
>>
>> Is DV short for DaVinci? If so, please spell it out. If not, please
>> add a "DA850" prefix like the other parameters and add a description
>> to explain what this option does.
> 
> I don't know where the name originate.  I am simply making the Kconfig
> match the names that were given before, so the tool that migrates from
> include/configs/* can operate.  If we change the names, this tool
> doesn't work cleanly.
> 
>>
>>> +       default 0 if SOC_DA850
>>> +
>>> +config SYS_DA850_PLL0_POSTDIV
>>> +       int "SYS_DA850_PLL0_POSTDIV"
>>> +       default 1 if SOC_DA850
>>
>>
>> All of these POSTDIV parameters don't really seem to me like something
>> that should be in Kconfig. I think it would be much cleaner to just
>> define a static array with these values and pass it to the
>> da850_pll_init() function.
> 
> 
> I don't disagree at all with you.  My main goal was to shrink the
> include/config/*.h file options and move them into Kconfig.  It's not
> part of the scope of what I'm trying to do to make an assessment of
> the quality of the code or question. The vast majority of these
> options are the same for every board, but someone where thought it was
> important to make them a configuration option, so I left it
> configurable.  There is one board that uses a slightly different value
> than the rest of the boards, so I don't think that putting all these
> 'configs' into the source file would be the best.
>>
>>
>>> +
>>> +config SYS_DA850_PLL0_PLLDIV1
>>> +       hex "SYS_DA850_PLL0_PLLDIV1"
>>> +       default 0x8000 if SOC_DA850
>>> +
>>> +config SYS_DA850_PLL0_PLLDIV2
>>> +       hex "SYS_DA850_PLL0_PLLDIV2"
>>> +       default 0x8001 if SOC_DA850
>>> +
>>> +config SYS_DA850_PLL0_PLLDIV3
>>> +       hex "SYS_DA850_PLL0_PLLDIV3"
>>> +       default 0x8002 if SOC_DA850
>>> +
>>> +config SYS_DA850_PLL0_PLLDIV4
>>> +       hex "SYS_DA850_PLL0_PLLDIV4"
>>> +       default 0x8003 if SOC_DA850
>>> +
>>> +config SYS_DA850_PLL0_PLLDIV5
>>> +       hex "SYS_DA850_PLL0_PLLDIV5"
>>> +       default 0x8002 if SOC_DA850
>>> +
>>> +config SYS_DA850_PLL0_PLLDIV7
>>> +       hex "SYS_DA850_PLL0_PLLDIV7"
>>> +       default 0x8005 if SOC_DA850
>>> +
>>> +config SYS_DA850_PLL1_POSTDIV
>>> +       hex "SYS_DA850_PLL1_POSTDIV"
>>> +       default 1 if SOC_DA850
>>> +
>>> +config SYS_DA850_PLL1_PLLDIV1
>>> +       hex "SYS_DA850_PLL1_PLLDIV1"
>>> +       default 0x8000 if SOC_DA850
>>> +
>>> +config SYS_DA850_PLL1_PLLDIV2
>>> +       hex "SYS_DA850_PLL1_PLLDIV2"
>>> +       default 0x8001 if SOC_DA850
>>> +
>>> +config SYS_DA850_PLL1_PLLDIV3
>>> +       hex "SYS_DA850_PLL1_PLLDIV3"
>>> +       default 0x8002 if SOC_DA850
>>> +
>>> +config SYS_DA850_PLL1_PLLM
>>> +       int
>>> +       default 21 if SOC_DA850
>>> +
>>> +endif
>>> +
>>
>>
>> If all of these really need to be Kconfig options, they need explanations
>> of what the values mean, for example, 0x8002 means that you are setting
>> the enable bit at bit 15 and you are setting the divider value to divide
>> by 3 because the register value is one less than the actual value.
> 
> I honestly don't have any idea what these values mean or how they were
> derrived.  I didn't bother looking.  I'm just trying to help the
> U-Boot community migrate from include/configs/*.h to Kconfig.
>>
>>
> 
> [snip]
> 
>>
>> So, to sum up. Basically, this looks fine other than the lack of
>> descriptions in the Kconfig. But, I don't think this stuff should
> 
> I tried find descriptions, but there weren't any README files that
> referenced these values, so I wasn't sure what to put.
> 
>> have been implemented using config options in the first place. I
>> would rather see da850_lowlevel.c get cleaned up to be more generic
>> and pass stuff as function parameters rather than using config
>> macros. But, since LEGO EV3 doesn't use any of this, it won't
>> really make a difference to me either way.
> 
> I'd like to see lowlevel.c get cleaned up too, but in my mind that's a
> separate task from the config.h ->  Kconfig migration.
> 

Understood. If you would like to add a short description to these
options, you can just say that the value will be written directly
to the PLLDIVn register (or PLLM register). That should be good
enough.