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

Frank Rowand
2016-04-28T23:31:24Z

On 4/28/2016 3:32 PM, Rob Herring wrote:
> On Thu, Apr 28, 2016 at 4:46 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@am.sony.com>
>>
>> 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@gmail.com>
>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>>
>> ---
>> 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?

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