On Thu, Jan 11, 2018 at 12:11 PM, David Lechner <da...@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 <aford...@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.
