Re: [PATCH 1/2] memory: tegra: Add missing dependencies

From: Dmitry Osipenko <digetx@gmail.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
	Thierry Reding <thierry.reding@gmail.com>
Cc: Jon Hunter <jonathanh@nvidia.com>,
	linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
	Philipp Zabel <p.zabel@pengutronix.de>
Subject: Re: [PATCH 1/2] memory: tegra: Add missing dependencies
Date: Mon, 14 Jun 2021 17:16:35 +0300	[thread overview]
Message-ID: <334bb138-f336-9759-bc0b-351b2590cc84@gmail.com> (raw)
In-Reply-To: <8efc327c-1321-4e04-36de-4b52b2b8fcf7@canonical.com>

14.06.2021 14:50, Krzysztof Kozlowski пишет:
> On 11/06/2021 15:40, Dmitry Osipenko wrote:
>> 11.06.2021 14:00, Thierry Reding пишет:
>>> On Fri, Jun 11, 2021 at 10:21:41AM +0300, Dmitry Osipenko wrote:
>>>> 11.06.2021 09:50, Krzysztof Kozlowski пишет:
>>>>> On 10/06/2021 18:23, Dmitry Osipenko wrote:
>>>>>> 10.06.2021 18:50, Dmitry Osipenko пишет:
>>>>>>> 10.06.2021 09:43, Krzysztof Kozlowski пишет:
>>>>>>>> The stubs might be good idea anyway, but the driver explicitly needs for
>>>>>>>> runtime working reservedmem, so it should select it.
>>>>>>>
>>>>>>> The OF and reservedmem are both selected by the ARCH for the runtime
>>>>>>> use. They may not be selected in the case of compile-testing.
>>>>>>>
>>>>>>> Both OF core and reservedmem provide stubs needed for compile-testing,
>>>>>>> it's only the RESERVEDMEM_OF_DECLARE() that is missing the stub. Adding
>>>>>>> the missing stub should be a more appropriate solution than adding extra
>>>>>>> Kconfig dependencies, IMO.
>>>>>
>>>>> Ah, in such case everything looks good. Stubs is indeed proper choice.
>>>>
>>>> Although, I see that there are only two Kconfigs that have
>>>> OF_RESERVED_MEM, one defines the OF_RESERVED_MEM, the other is QCOM
>>>> Kconfig which depends on OF_RESERVED_MEM. The OF_RESERVED_MEM is enabled
>>>> by default in defconfig.
>>>>
>>>> You're right, we need the Kconfig change to be entirely correct, since
>>>> driver won't work properly without OF_RESERVED_MEM.
>>>>
>>>> config TEGRA210_EMC
>>>> 	tristate "NVIDIA Tegra210 External Memory Controller driver"
>>>> -	depends on ARCH_TEGRA_210_SOC || COMPILE_TEST
>>>> +	depends on (ARCH_TEGRA_210_SOC && OF_RESERVED_MEM) || COMPILE_TEST
>>>>
>>>> I will send that change later today.
>>>
>>> That's completely unnecessary. OF_RESERVED_MEM is enabled by default if
>>> OF_EARLY_FLATTREE is enabled, which it is for ARM64 and that is always
>>> enabled for ARCH_TEGRA_210_SOC.
>>
>> But it doesn't stop you from disabling OF_RESERVED_MEM. The Kconfig
>> dependencies should reflect the build and runtime requirements of the
>> driver, otherwise only driver author knows which config options are need.
> 
> OF_RESERVED_MEM is not selectable, so it cannot be disabled for regular
> builds.
> 
> When I proposed to add dependencies, I indeed did not check whether
> these are selectable symbols. My advise was not accurate.
> 
> Usually we do not add dependencies to each driver on each kernel
> non-selectable feature. It would be too much (depends on HAS_IOMEM,
> REGMAP, OF, MFD_SYSCON, OF_RESERVED_MEM and probably many more)... There
> are of course exceptions and mistakes (I think I should not add
> OF_ADDRESS to OMAP_GPMC).
> 
> If such features are non-selectable, usually architecture enforces them
> so the driver does not need to. For compile testing, these features
> should come with stubs. If there are no stubs, the dependency for
> compile testing could be added:
> ARCH_TEGRA_210_SOC || (COMPILE_TEST && OF_RESERVED_MEM)
> 
> We add the dependencies for selectable options which driver needs, e.g.
> DEVFREQ, CPUFREQ, PM_xxx
> 
> Since you proposed already to add stubs for OF_RESERVED_MEM, adding
> dependency on it does not bring any benefit.

Right, I also missed that OF_RESERVED_MEM is non-selectable.