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

[Dmitry Osipenko]

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.

> What Krzysztof had originally proposed, as far as I understand, is to
> add "depends on OF_RESERVED_MEM" so that the dependency is always there
> (including the COMPILE_TEST case). However, that's a bit problematic, as
> I said earlier, because OF_RESERVED_MEM is not user-visible and neither
> is OF_EARLY_FLATTREE, so there's no way to enable OF_RESERVED_MEM unless
> the architecture selected it, which it doesn't on x86, so it kind of
> defeats the purpose of COMPILE_TEST.

Indeed, the QCOM driver isn't compile-tested as much as it could be.
That driver already shouldn't have any problems with compile-testing,
maybe some stubs were missing when driver was originally added.

> So I think if this really has to be compile-test enabled, the only way
> to do that is to either make this select OF_EARLY_FLATTREE, or add the
> stubs.
> 
> Another option would perhaps be to enable OF_UNITTEST along with
> COMPILE_TEST, since that also pulls in OF_EARLY_FLATTREE and would allow
> this driver to be built even on x86.

For the universal compile-testing it should be enough to fix the stub.
We may consider other options if it won't be enough, thank you for the
suggestions.