Re: [PATCH] scripts/dtc: dt_to_config - report kernel config options for a devicetree

[Gaurav Minocha]

On Thu, Apr 28, 2016 at 4:31 PM, Frank Rowand <frowand.list@xxxxxxxxx> wrote:
> On 4/28/2016 3:32 PM, Rob Herring wrote:
>> On Thu, Apr 28, 2016 at 4:46 PM, Frank Rowand <frowand.list@xxxxxxxxx> wrote:
>>> From: Frank Rowand <frank.rowand@xxxxxxxxxxx>
>>>
>>> Determining which kernel config options need to be enabled for a
>>> given devicetree can be a painful process.  Create a new tool to
>>> find the drivers that may match a devicetree node compatible,
>>> find the kernel config options that enable the driver, and
>>> optionally report whether the kernel config option is enabled.
>>
>> I would find this more useful to output a config fragment with all the
>> options enabled. The hard part there is enabling the options a given
>> option is dependent on which I don't think kbuild takes care of.
>>
>>> Signed-off-by: Gaurav Minocha <gaurav.minocha.os@xxxxxxxxx>
>>> Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxxxxx>
>>>
>>> ---
>>>  scripts/dtc/dt_to_config | 1061 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 1061 insertions(+)
>>>
>>> Index: b/scripts/dtc/dt_to_config
>>> ===================================================================
>>> --- /dev/null
>>> +++ b/scripts/dtc/dt_to_config
>>> @@ -0,0 +1,1061 @@
>>> +#!/usr/bin/perl
>>
>> I don't do perl...
>>
>>> +
>>> +#   Copyright 2016 by Frank Rowand
>>> +# Å Copyright 2016 by Gaurav Minocha
>>      ^
>> Is this supposed to be a copyright symbol?
>
> Yes.  Gaurav, can we drop this symbol?
>

Yes, Of course.

>
>>> +#
>>> +# This file is subject to the terms and conditions of the GNU General Public
>>> +# License v2.
>>
>> [...]
>>
>>> +# ----- magic compatibles, do not have a driver
>>> +#
>>> +# Will not search for drivers for these compatibles.
>>> +
>>> +%compat_white_list = (
>>> +       'fixed-partitions'      => '1',
>>
>> Enabling CONFIG_MTD would be useful.
>
> Thanks!  I'll dig deeper, but it likes like maybe
> CONFIG_MTD_OF_PARTS also.
>
>
>>> +       'none'                  => '1',
>>
>> Is this an actual string used somewhere?
>
> Yes.  Looking at the output from running this against all .dts
> files has led to some interesting information.
>
> An example of compatible = "none" is node mct@10050000 from
> arch/arm/boot/dts/exynos4210-universal_c210.dts
>
>>
>>> +       'pci'                   => '1',
>>
>> ditto?
>
> arch/x86/platform/ce4100/falconfalls.dts, node pci@3fc
>
>
>>> +       'simple-bus'            => '1',
>>> +);
>>> +
>>> +# magic compatibles, have a driver
>>> +#
>>> +# Will not search for drivers for these compatibles.
>>> +# Will instead use the drivers and config options listed here.
>>> +#
>>> +# If you add an entry to this hash, add the corresponding entry
>>> +# to %driver_config_hard_code_list.
>>> +#
>>> +# These compatibles have a very large number of false positives.
>>
>> What does this mean?
>
> That means that a _lot_ of garbage, bogus matches are reported,
> creating a lot of noise in the report.
>
>
>>> +#
>>> +# 'hardcoded_no_driver' is a magic value.  Other code knows this
>>> +# magic value.  Do not use 'no_driver' here!
>>> +#
>>> +# TODO: Revisit each 'hardcoded_no_driver' to see how the compatible
>>> +#       is used.  Are there drivers that can be provided?
>>> +
>>> +%driver_hard_code_list = (
>>> +       'cache'                 => ['hardcoded_no_driver'],
>>> +       'eeprom'                => ['hardcoded_no_driver'],
>>> +       'gpio'                  => ['hardcoded_no_driver'],
>>> +       'gpios'                 => ['drivers/leds/leds-tca6507.c'],
>>> +       'gpio-keys'             => ['drivers/input/keyboard/gpio_keys.c'],
>>> +       'i2c-gpio'              => ['drivers/i2c/busses/i2c-gpio.c'],
>>> +       'isa'                   => ['arch/mips/mti-malta/malta-dt.c',
>>> +                                    'arch/x86/kernel/devicetree.c'],
>>> +       'led'                   => ['hardcoded_no_driver'],
>>> +       'm25p32'                => ['hardcoded_no_driver'],
>>> +       'm25p64'                => ['hardcoded_no_driver'],
>>> +       'm25p80'                => ['hardcoded_no_driver'],
>>> +       'mtd_ram'               => ['drivers/mtd/maps/physmap_of.c'],
>>> +       'pwm-backlight'         => ['drivers/video/backlight/pwm_bl.c'],
>>> +       'spidev'                => ['hardcoded_no_driver'],
>>> +       'syscon'                => ['drivers/mfd/syscon.c'],
>>> +       'tlv320aic23'           => ['hardcoded_no_driver'],
>>> +       'wm8731'                => ['hardcoded_no_driver'],
>>> +);
>>> +
>>> +%driver_config_hard_code_list = (
>>> +
>>> +       # this one needed even if %driver_hard_code_list is empty
>>> +       'no_driver'                             => ['no_config'],
>>> +       'hardcoded_no_driver'                   => ['no_config'],
>>> +
>>> +       'drivers/leds/leds-tca6507.c'           => ['CONFIG_LEDS_TCA6507'],
>>> +       'drivers/input/keyboard/gpio_keys.c'    => ['CONFIG_KEYBOARD_GPIO'],
>>> +       'drivers/i2c/busses/i2c-gpio.c'         => ['CONFIG_I2C_GPIO'],
>>> +       'arch/mips/mti-malta/malta-dt.c'        => ['obj-y'],
>>> +       'arch/x86/kernel/devicetree.c'          => ['CONFIG_OF'],
>>> +       'drivers/mtd/maps/physmap_of.c'         => ['CONFIG_MTD_PHYSMAP_OF'],
>>> +       'drivers/video/backlight/pwm_bl.c'      => ['CONFIG_BACKLIGHT_PWM'],
>>> +       'drivers/mfd/syscon.c'                  => ['CONFIG_MFD_SYSCON'],
>>
>> I don't understand why some of these are not searchable by compatible strings.
>
> I will have to get back to you later on this question.
>
>
>>> +
>>> +       # drivers/usb/host/ehci-ppc-of.c
>>> +       # drivers/usb/host/ehci-xilinx-of.c
>>> +       #  are included from:
>>> +       #    drivers/usb/host/ehci-hcd.c
>>> +       #  thus the search of Makefile for the included .c files is incorrect
>>> +       # ehci-hcd.c wraps the includes with ifdef CONFIG_USB_EHCI_HCD_..._OF
>>> +       #
>>> +       # similar model for ohci-hcd.c (but no ohci-xilinx-of.c)
>>> +       #
>>> +       # similarly, uhci-hcd.c includes uhci-platform.c
>>> +
>>> +       'drivers/usb/host/ehci-ppc-of.c'        => ['CONFIG_USB_EHCI_HCD',
>>> +                                                   'CONFIG_USB_EHCI_HCD_PPC_OF'],
>>> +       'drivers/usb/host/ohci-ppc-of.c'        => ['CONFIG_USB_OHCI_HCD',
>>> +                                                   'CONFIG_USB_OHCI_HCD_PPC_OF'],
>>> +
>>> +       'drivers/usb/host/ehci-xilinx-of.c'     => ['CONFIG_USB_EHCI_HCD',
>>> +                                                   'CONFIG_USB_EHCI_HCD_XILINX'],
>>> +
>>> +       'drivers/usb/host/uhci-platform.c'      => ['CONFIG_USB_UHCI_HCD',
>>> +                                                   'CONFIG_USB_UHCI_PLATFORM'],
>>> +
>>> +       # scan_makefile will find only one of these config options:
>>> +       #    ifneq ($(CONFIG_SOC_IMX6)$(CONFIG_SOC_LS1021A),)
>>> +       'arch/arm/mach-imx/platsmp.c'           => ['CONFIG_SOC_IMX6 && CONFIG_SMP',
>>> +                                                   'CONFIG_SOC_LS1021A && CONFIG_SMP'],
>>
>> These will never get updated when the files or config options change.
>> Conversely, how do I know if I need to add something here? Is this
>> list complete or based on testing some set of dts files. IMO, this
>> list has to go to merge this.
>
> That is a concern that I have also.  I added a warning flag in the
> flags field to indicate when one of these hard-coded values was used.
> The flag does not solve the issue, but does make it more visible.
>
> The list is a result of running against every dts file in the kernel
> tree and looking at the reports.
>
>
>>> +);
>>> +
>>> +
>>> +# 'virt/kvm/arm/.*' are controlled by makefiles in other directories,
>>> +# using relative paths, such as 'KVM := ../../../virt/kvm'.  Do not
>>> +# add complexity to find_kconfig() to deal with this.  There is a long
>>> +# term intent to change the kvm related makefiles to the normal kernel
>>> +# style.  After that is done, this entry can be removed from the
>>> +# driver_black_list.
>>> +
>>> +@driver_black_list = (
>>> +       'virt/kvm/arm/.*',
>>> +);
>>
>> A small number of exceptions like this I could stomach.
>>
>> The rest is just write-only language nonsense...
>>
>> Rob
>>
>