Hi Olof, On 05/01/19 6:27 AM, Olof Johansson wrote: > On Wed, Jan 2, 2019 at 9:58 PM Faiz Abbas <faiz_abbas@xxxxxx> wrote: >> >> Hi Olof, Eduardo, >> >> On 03/01/19 1:26 AM, Eduardo Valentin wrote: >>> On Wed, Jan 02, 2019 at 10:29:31AM -0800, Olof Johansson wrote: >>>> Hi, >>>> >>>> ... >>>> >>>> This is throwing errors on builds of keystone_defconfig in next and mainline: >>>> >>>> http://arm-soc.lixom.net/buildlogs/next/next-20190102/buildall.arm.keystone_defconfig.log.passed >>>> >>>> WARNING: unmet direct dependencies detected for TI_SOC_THERMAL >>>> Depends on [n]: THERMAL [=y] && (ARCH_HAS_BANDGAP [=n] || >>>> COMPILE_TEST [=n]) && HAS_IOMEM [=y] >>>> Selected by [y]: >>>> - MMC_SDHCI_OMAP [=y] && MMC [=y] && MMC_SDHCI_PLTFM [=y] && OF [=y] >>>> >>>> So, thermal depends on ARCH_HAS_BANDGAP, which keystone doesn't provide. >>>> >>>> Selecting a major framework such as THERMAL from a driver config is >>>> likely not the right solution anyway, especially since THERMAL does >>>> provide stubbed out versions of the functions if it's not enabled. >>> >>> Yeah, that seams a bit up-side-down. Can you guys give a bit more of >>> context? Why do you need the cpu thermal zone ? From patch description, >>> looks like you want to have your own zone then apply different tuning >>> values based on temperature (range?). Why do you need to mess up with >>> cpu_thermal zone? Don't you have a bandgap in the mem controller for >>> this application? >>> >> >> Thats correct. We don't have a bandgap in the MMC controller and thus we >> have to use the cpu one to measure temperature. >> >> THERMAL is critical for tuning. The interface is supposed to fail if we >> can't get temperature. So IMO we should ensure that it is present. >> >> I can fix this by "select TI_SOC_THERMAL if ARCH_HAS_BANDGAP" if you >> guys agree. > > Building elaborate select statements is usually fragile, once > dependencies for TI_SOC_THERMAL changes you need to come back here to > fixup the select. > > Supposedly this driver works on keystone (or does it?), it doesn't Yes. This driver works on keystone. > actually need TI_SOC_THERMAL for basic functionality beyond tuning? > Or, at least, it needs to fall back to a reasonable behavior if it's > unavailable on keystone. The scenario is this. dra7 devices (omap2plus_defconfig) support tuning which needs THERMAL and TI_SOC_THERMAL. Keystone devices don't need thermal at all. The tuning part of the code will never be touched by the keystone device. In omap2plus_defconfig, THERMAL AND TI_SOC_THERMAL are modules by default. I need them to be built-in because MMC is built-in. > > Having the driver print a warning and refuse to tune to higher speeds > is a reasonable way to do this, I think. That would carry to all > platforms, i.e. even the ones who have TI_SOC_THERMAL and > ARCH_HAS_BANDGAP, without adding the select. > The MMC core today doesn't support a fallback to lower speeds when tuning fails. The interface just fails. This means a bunch cards that were working will now fail. Also, people building with olddefconfig will get build errors because of THERMAL=m. Thus the Kconfig architecture should automatically select the dependencies for SDHCI_OMAP in DRA7. An "imply TI_SOC_THERMAL" is even better here. It won't force TI_SOC_THERMAL for keystone and doesn't cause any warnings. So I propose the following patch: diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index e26b8145efb3..bc61eefcb695 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -978,7 +978,7 @@ config MMC_SDHCI_OMAP tristate "TI SDHCI Controller Support" depends on MMC_SDHCI_PLTFM && OF select THERMAL - select TI_SOC_THERMAL + imply TI_SOC_THERMAL help -- Thanks, Faiz
