Message ID | 20210609112806.3565057-2-thierry.reding@gmail.com |
---|---|
State | Rejected |
Headers | show |
Series | memory: tegra: Fixes for COMPILE_TEST | expand |
09.06.2021 14:28, Thierry Reding пишет: > From: Thierry Reding <treding@nvidia.com> > > When enabling the COMPILE_TEST Kconfig option, the Tegra memory > controller can be built without ARCH_TEGRA being selected. However, the > driver implicitly depends on some symbols pulled in via ARCH_TEGRA, > which causes the build to break. > > Add explicit dependencies for OF_EARLY_FLATTREE and OF_RESERVED_MEM to > the Tegra MC Kconfig option to make sure they are selected even if > ARCH_TEGRA is not. > > Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/memory/tegra/Kconfig | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig > index f9bae36c03a3..ecfb071fc4f4 100644 > --- a/drivers/memory/tegra/Kconfig > +++ b/drivers/memory/tegra/Kconfig > @@ -48,6 +48,8 @@ config TEGRA124_EMC > config TEGRA210_EMC_TABLE > bool > depends on ARCH_TEGRA_210_SOC || COMPILE_TEST > + select OF_EARLY_FLATTREE > + select OF_RESERVED_MEM > > config TEGRA210_EMC > tristate "NVIDIA Tegra210 External Memory Controller driver" > Will this work if CONFIG_OF is disabled?
On 09/06/2021 13:58, Dmitry Osipenko wrote: > 09.06.2021 14:28, Thierry Reding пишет: >> From: Thierry Reding <treding@nvidia.com> >> >> When enabling the COMPILE_TEST Kconfig option, the Tegra memory >> controller can be built without ARCH_TEGRA being selected. However, the >> driver implicitly depends on some symbols pulled in via ARCH_TEGRA, >> which causes the build to break. >> >> Add explicit dependencies for OF_EARLY_FLATTREE and OF_RESERVED_MEM to >> the Tegra MC Kconfig option to make sure they are selected even if >> ARCH_TEGRA is not. >> >> Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >> Signed-off-by: Thierry Reding <treding@nvidia.com> >> --- >> drivers/memory/tegra/Kconfig | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig >> index f9bae36c03a3..ecfb071fc4f4 100644 >> --- a/drivers/memory/tegra/Kconfig >> +++ b/drivers/memory/tegra/Kconfig >> @@ -48,6 +48,8 @@ config TEGRA124_EMC >> config TEGRA210_EMC_TABLE >> bool >> depends on ARCH_TEGRA_210_SOC || COMPILE_TEST >> + select OF_EARLY_FLATTREE >> + select OF_RESERVED_MEM >> >> config TEGRA210_EMC >> tristate "NVIDIA Tegra210 External Memory Controller driver" >> > > Will this work if CONFIG_OF is disabled? Yeah, good question. That's why I propose "depends on". No issues with unmet or circular dependencies. Best regards, Krzysztof
09.06.2021 16:19, Krzysztof Kozlowski пишет: > On 09/06/2021 13:58, Dmitry Osipenko wrote: >> 09.06.2021 14:28, Thierry Reding пишет: >>> From: Thierry Reding <treding@nvidia.com> >>> >>> When enabling the COMPILE_TEST Kconfig option, the Tegra memory >>> controller can be built without ARCH_TEGRA being selected. However, the >>> driver implicitly depends on some symbols pulled in via ARCH_TEGRA, >>> which causes the build to break. >>> >>> Add explicit dependencies for OF_EARLY_FLATTREE and OF_RESERVED_MEM to >>> the Tegra MC Kconfig option to make sure they are selected even if >>> ARCH_TEGRA is not. >>> >>> Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >>> Signed-off-by: Thierry Reding <treding@nvidia.com> >>> --- >>> drivers/memory/tegra/Kconfig | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig >>> index f9bae36c03a3..ecfb071fc4f4 100644 >>> --- a/drivers/memory/tegra/Kconfig >>> +++ b/drivers/memory/tegra/Kconfig >>> @@ -48,6 +48,8 @@ config TEGRA124_EMC >>> config TEGRA210_EMC_TABLE >>> bool >>> depends on ARCH_TEGRA_210_SOC || COMPILE_TEST >>> + select OF_EARLY_FLATTREE >>> + select OF_RESERVED_MEM >>> >>> config TEGRA210_EMC >>> tristate "NVIDIA Tegra210 External Memory Controller driver" >>> >> >> Will this work if CONFIG_OF is disabled? > > Yeah, good question. That's why I propose "depends on". No issues with > unmet or circular dependencies. What about to add stub for RESERVEDMEM_OF_DECLARE() + CONFIG_OF_RESERVED_MEM=n? diff --git a/include/linux/of.h b/include/linux/of.h index d8db8d3592fd..9c2e71e202d1 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -1329,6 +1329,12 @@ static inline int of_get_available_child_count(const struct device_node *np) return num; } +#define _OF_DECLARE_STUB(table, name, compat, fn, fn_type) \ + static const struct of_device_id __of_table_##name \ + __attribute__((unused)) \ + = { .compatible = compat, \ + .data = (fn == (fn_type)NULL) ? fn : fn } + #if defined(CONFIG_OF) && !defined(MODULE) #define _OF_DECLARE(table, name, compat, fn, fn_type) \ static const struct of_device_id __of_table_##name \ @@ -1338,10 +1344,7 @@ static inline int of_get_available_child_count(const struct device_node *np) .data = (fn == (fn_type)NULL) ? fn : fn } #else #define _OF_DECLARE(table, name, compat, fn, fn_type) \ - static const struct of_device_id __of_table_##name \ - __attribute__((unused)) \ - = { .compatible = compat, \ - .data = (fn == (fn_type)NULL) ? fn : fn } + _OF_DECLARE_STUB(table, name, compat, fn, fn_type) #endif typedef int (*of_init_fn_2)(struct device_node *, struct device_node *); diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h index 76e4a0fffba4..4de2a24cadc9 100644 --- a/include/linux/of_reserved_mem.h +++ b/include/linux/of_reserved_mem.h @@ -27,11 +27,11 @@ struct reserved_mem_ops { typedef int (*reservedmem_of_init_fn)(struct reserved_mem *rmem); +#ifdef CONFIG_OF_RESERVED_MEM + #define RESERVEDMEM_OF_DECLARE(name, compat, init) \ _OF_DECLARE(reservedmem, name, compat, init, reservedmem_of_init_fn) -#ifdef CONFIG_OF_RESERVED_MEM - int of_reserved_mem_device_init_by_idx(struct device *dev, struct device_node *np, int idx); int of_reserved_mem_device_init_by_name(struct device *dev, @@ -41,6 +41,10 @@ void of_reserved_mem_device_release(struct device *dev); struct reserved_mem *of_reserved_mem_lookup(struct device_node *np); #else + +#define RESERVEDMEM_OF_DECLARE(name, compat, init) \ + _OF_DECLARE_STUB(reservedmem, name, compat, init, reservedmem_of_init_fn) + static inline int of_reserved_mem_device_init_by_idx(struct device *dev, struct device_node *np, int idx) {
On Wed, Jun 09, 2021 at 03:19:12PM +0200, Krzysztof Kozlowski wrote: > On 09/06/2021 13:58, Dmitry Osipenko wrote: > > 09.06.2021 14:28, Thierry Reding пишет: > >> From: Thierry Reding <treding@nvidia.com> > >> > >> When enabling the COMPILE_TEST Kconfig option, the Tegra memory > >> controller can be built without ARCH_TEGRA being selected. However, the > >> driver implicitly depends on some symbols pulled in via ARCH_TEGRA, > >> which causes the build to break. > >> > >> Add explicit dependencies for OF_EARLY_FLATTREE and OF_RESERVED_MEM to > >> the Tegra MC Kconfig option to make sure they are selected even if > >> ARCH_TEGRA is not. > >> > >> Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > >> Signed-off-by: Thierry Reding <treding@nvidia.com> > >> --- > >> drivers/memory/tegra/Kconfig | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig > >> index f9bae36c03a3..ecfb071fc4f4 100644 > >> --- a/drivers/memory/tegra/Kconfig > >> +++ b/drivers/memory/tegra/Kconfig > >> @@ -48,6 +48,8 @@ config TEGRA124_EMC > >> config TEGRA210_EMC_TABLE > >> bool > >> depends on ARCH_TEGRA_210_SOC || COMPILE_TEST > >> + select OF_EARLY_FLATTREE > >> + select OF_RESERVED_MEM > >> > >> config TEGRA210_EMC > >> tristate "NVIDIA Tegra210 External Memory Controller driver" > >> > > > > Will this work if CONFIG_OF is disabled? > > Yeah, good question. That's why I propose "depends on". No issues with > unmet or circular dependencies. I couldn't find a way to make this work with "depends on" because OF_RESERVED_MEM is not user-visible and the only way to get it enabled is if something also selects OF_EARLY_FLATTREE, which is only ever done at the architecture Kconfig level (and for OF unit testing). So switching this to a "depends on" causes the TEGRA210_EMC never to get enabled. However, with OF disabled, the above causes issues because it can lead to unmet direct dependencies. That, in turn, can be fixed by appending && OF to the COMPILE_TEST branch, which seems like a good enough compromise. Here's what I have on top of the above patch and that seems to do the trick. --- >8 --- diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig index ecfb071fc4f4..1c553895160c 100644 --- a/drivers/memory/tegra/Kconfig +++ b/drivers/memory/tegra/Kconfig @@ -47,13 +47,13 @@ config TEGRA124_EMC config TEGRA210_EMC_TABLE bool - depends on ARCH_TEGRA_210_SOC || COMPILE_TEST + depends on ARCH_TEGRA_210_SOC || (COMPILE_TEST && OF) select OF_EARLY_FLATTREE select 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 || (COMPILE_TEST && OF) select TEGRA210_EMC_TABLE help This driver is for the External Memory Controller (EMC) found on --- >8 --- So in a nutshell this will only get compile-tested if OF is enabled, but then it will select OF_RESERVED_MEM and OF_EARLY_FLATTREE to get the required symbols. Thierry
On 09/06/2021 19:00, Thierry Reding wrote: > On Wed, Jun 09, 2021 at 03:19:12PM +0200, Krzysztof Kozlowski wrote: >> On 09/06/2021 13:58, Dmitry Osipenko wrote: >>> 09.06.2021 14:28, Thierry Reding пишет: >>>> From: Thierry Reding <treding@nvidia.com> >>>> >>>> When enabling the COMPILE_TEST Kconfig option, the Tegra memory >>>> controller can be built without ARCH_TEGRA being selected. However, the >>>> driver implicitly depends on some symbols pulled in via ARCH_TEGRA, >>>> which causes the build to break. >>>> >>>> Add explicit dependencies for OF_EARLY_FLATTREE and OF_RESERVED_MEM to >>>> the Tegra MC Kconfig option to make sure they are selected even if >>>> ARCH_TEGRA is not. >>>> >>>> Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >>>> Signed-off-by: Thierry Reding <treding@nvidia.com> >>>> --- >>>> drivers/memory/tegra/Kconfig | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig >>>> index f9bae36c03a3..ecfb071fc4f4 100644 >>>> --- a/drivers/memory/tegra/Kconfig >>>> +++ b/drivers/memory/tegra/Kconfig >>>> @@ -48,6 +48,8 @@ config TEGRA124_EMC >>>> config TEGRA210_EMC_TABLE >>>> bool >>>> depends on ARCH_TEGRA_210_SOC || COMPILE_TEST >>>> + select OF_EARLY_FLATTREE >>>> + select OF_RESERVED_MEM >>>> >>>> config TEGRA210_EMC >>>> tristate "NVIDIA Tegra210 External Memory Controller driver" >>>> >>> >>> Will this work if CONFIG_OF is disabled? >> >> Yeah, good question. That's why I propose "depends on". No issues with >> unmet or circular dependencies. > > I couldn't find a way to make this work with "depends on" because > OF_RESERVED_MEM is not user-visible and the only way to get it enabled > is if something also selects OF_EARLY_FLATTREE, which is only ever done > at the architecture Kconfig level (and for OF unit testing). > > So switching this to a "depends on" causes the TEGRA210_EMC never to get > enabled. Right. > > However, with OF disabled, the above causes issues because it can lead > to unmet direct dependencies. That, in turn, can be fixed by appending > && OF to the COMPILE_TEST branch, which seems like a good enough > compromise. > > Here's what I have on top of the above patch and that seems to do the > trick. > > --- >8 --- > diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig > index ecfb071fc4f4..1c553895160c 100644 > --- a/drivers/memory/tegra/Kconfig > +++ b/drivers/memory/tegra/Kconfig > @@ -47,13 +47,13 @@ config TEGRA124_EMC > > config TEGRA210_EMC_TABLE > bool > - depends on ARCH_TEGRA_210_SOC || COMPILE_TEST > + depends on ARCH_TEGRA_210_SOC || (COMPILE_TEST && OF) > select OF_EARLY_FLATTREE > select 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 || (COMPILE_TEST && OF) > select TEGRA210_EMC_TABLE > help > This driver is for the External Memory Controller (EMC) found on > --- >8 --- > > So in a nutshell this will only get compile-tested if OF is enabled, but > then it will select OF_RESERVED_MEM and OF_EARLY_FLATTREE to get the > required symbols. Could be also separate "depends on OF", even though it is included in ARCH_TEGRA_XXX, but I am fine with this here. Best regards, Krzysztof
On 09/06/2021 18:57, Dmitry Osipenko wrote: > 09.06.2021 16:19, Krzysztof Kozlowski пишет: >> On 09/06/2021 13:58, Dmitry Osipenko wrote: >>> 09.06.2021 14:28, Thierry Reding пишет: >>>> From: Thierry Reding <treding@nvidia.com> >>>> >>>> When enabling the COMPILE_TEST Kconfig option, the Tegra memory >>>> controller can be built without ARCH_TEGRA being selected. However, the >>>> driver implicitly depends on some symbols pulled in via ARCH_TEGRA, >>>> which causes the build to break. >>>> >>>> Add explicit dependencies for OF_EARLY_FLATTREE and OF_RESERVED_MEM to >>>> the Tegra MC Kconfig option to make sure they are selected even if >>>> ARCH_TEGRA is not. >>>> >>>> Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >>>> Signed-off-by: Thierry Reding <treding@nvidia.com> >>>> --- >>>> drivers/memory/tegra/Kconfig | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig >>>> index f9bae36c03a3..ecfb071fc4f4 100644 >>>> --- a/drivers/memory/tegra/Kconfig >>>> +++ b/drivers/memory/tegra/Kconfig >>>> @@ -48,6 +48,8 @@ config TEGRA124_EMC >>>> config TEGRA210_EMC_TABLE >>>> bool >>>> depends on ARCH_TEGRA_210_SOC || COMPILE_TEST >>>> + select OF_EARLY_FLATTREE >>>> + select OF_RESERVED_MEM >>>> >>>> config TEGRA210_EMC >>>> tristate "NVIDIA Tegra210 External Memory Controller driver" >>>> >>> >>> Will this work if CONFIG_OF is disabled? >> >> Yeah, good question. That's why I propose "depends on". No issues with >> unmet or circular dependencies. > > What about to add stub for RESERVEDMEM_OF_DECLARE() + CONFIG_OF_RESERVED_MEM=n? > > diff --git a/include/linux/of.h b/include/linux/of.h > index d8db8d3592fd..9c2e71e202d1 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -1329,6 +1329,12 @@ static inline int of_get_available_child_count(const struct device_node *np) > return num; > } > > +#define _OF_DECLARE_STUB(table, name, compat, fn, fn_type) \ > + static const struct of_device_id __of_table_##name \ > + __attribute__((unused)) \ > + = { .compatible = compat, \ > + .data = (fn == (fn_type)NULL) ? fn : fn } > + > #if defined(CONFIG_OF) && !defined(MODULE) > #define _OF_DECLARE(table, name, compat, fn, fn_type) \ > static const struct of_device_id __of_table_##name \ > @@ -1338,10 +1344,7 @@ static inline int of_get_available_child_count(const struct device_node *np) > .data = (fn == (fn_type)NULL) ? fn : fn } > #else > #define _OF_DECLARE(table, name, compat, fn, fn_type) \ > - static const struct of_device_id __of_table_##name \ > - __attribute__((unused)) \ > - = { .compatible = compat, \ > - .data = (fn == (fn_type)NULL) ? fn : fn } > + _OF_DECLARE_STUB(table, name, compat, fn, fn_type) > #endif > > typedef int (*of_init_fn_2)(struct device_node *, struct device_node *); > diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h > index 76e4a0fffba4..4de2a24cadc9 100644 > --- a/include/linux/of_reserved_mem.h > +++ b/include/linux/of_reserved_mem.h > @@ -27,11 +27,11 @@ struct reserved_mem_ops { > > typedef int (*reservedmem_of_init_fn)(struct reserved_mem *rmem); > > +#ifdef CONFIG_OF_RESERVED_MEM > + > #define RESERVEDMEM_OF_DECLARE(name, compat, init) \ > _OF_DECLARE(reservedmem, name, compat, init, reservedmem_of_init_fn) > > -#ifdef CONFIG_OF_RESERVED_MEM > - > int of_reserved_mem_device_init_by_idx(struct device *dev, > struct device_node *np, int idx); > int of_reserved_mem_device_init_by_name(struct device *dev, > @@ -41,6 +41,10 @@ void of_reserved_mem_device_release(struct device *dev); > > struct reserved_mem *of_reserved_mem_lookup(struct device_node *np); > #else > + > +#define RESERVEDMEM_OF_DECLARE(name, compat, init) \ > + _OF_DECLARE_STUB(reservedmem, name, compat, init, reservedmem_of_init_fn) > + > static inline int of_reserved_mem_device_init_by_idx(struct device *dev, > struct device_node *np, int idx) > { The stubs might be good idea anyway, but the driver explicitly needs for runtime working reservedmem, so it should select it. Best regards, Krzysztof
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.
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. > I will send the patch. If OF maintainers will have objections, then adding the dependency may become a more reasonable option.
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. > I will send the patch. If OF maintainers will have objections, then > adding the dependency may become a more reasonable option. Thanks! Best regards, Krzysztof
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.
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. 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. 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. Thierry
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.
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. Best regards, Krzysztof
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.
Hi Thierry,
I love your patch! Yet something to improve:
[auto build test ERROR on tegra/for-next]
[also build test ERROR on next-20210616]
[cannot apply to pza/reset/next tegra-drm/drm/tegra/for-next v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Thierry-Reding/memory-tegra-Fixes-for-COMPILE_TEST/20210616-154340
base: https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: parisc-randconfig-c023-20210616 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/0d0e9cbf83822694f35eca16dce8c5406b4f464f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Thierry-Reding/memory-tegra-Fixes-for-COMPILE_TEST/20210616-154340
git checkout 0d0e9cbf83822694f35eca16dce8c5406b4f464f
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/tty/serial/earlycon.c: In function 'of_setup_earlycon':
>> drivers/tty/serial/earlycon.c:258:9: error: implicit declaration of function 'of_flat_dt_translate_address' [-Werror=implicit-function-declaration]
258 | addr = of_flat_dt_translate_address(node);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for OF_EARLY_FLATTREE
Depends on OF
Selected by
- TEGRA210_EMC_TABLE && MEMORY && TEGRA_MC && (ARCH_TEGRA_210_SOC || COMPILE_TEST
WARNING: unmet direct dependencies detected for OF_RESERVED_MEM
Depends on OF && OF_EARLY_FLATTREE
Selected by
- TEGRA210_EMC_TABLE && MEMORY && TEGRA_MC && (ARCH_TEGRA_210_SOC || COMPILE_TEST
vim +/of_flat_dt_translate_address +258 drivers/tty/serial/earlycon.c
8477614d9f7c5c Peter Hurley 2016-01-16 245
c90fe9c0394b06 Peter Hurley 2016-01-16 246 int __init of_setup_earlycon(const struct earlycon_id *match,
088da2a17619cf Peter Hurley 2016-01-16 247 unsigned long node,
4d118c9a866590 Peter Hurley 2016-01-16 248 const char *options)
b0b6abd34c1b50 Rob Herring 2014-03-27 249 {
b0b6abd34c1b50 Rob Herring 2014-03-27 250 int err;
b0b6abd34c1b50 Rob Herring 2014-03-27 251 struct uart_port *port = &early_console_dev.port;
088da2a17619cf Peter Hurley 2016-01-16 252 const __be32 *val;
088da2a17619cf Peter Hurley 2016-01-16 253 bool big_endian;
c90fe9c0394b06 Peter Hurley 2016-01-16 254 u64 addr;
b0b6abd34c1b50 Rob Herring 2014-03-27 255
e1dd3bef6d03c9 Geert Uytterhoeven 2015-11-27 256 spin_lock_init(&port->lock);
b0b6abd34c1b50 Rob Herring 2014-03-27 257 port->iotype = UPIO_MEM;
c90fe9c0394b06 Peter Hurley 2016-01-16 @258 addr = of_flat_dt_translate_address(node);
c90fe9c0394b06 Peter Hurley 2016-01-16 259 if (addr == OF_BAD_ADDR) {
c90fe9c0394b06 Peter Hurley 2016-01-16 260 pr_warn("[%s] bad address\n", match->name);
c90fe9c0394b06 Peter Hurley 2016-01-16 261 return -ENXIO;
c90fe9c0394b06 Peter Hurley 2016-01-16 262 }
b0b6abd34c1b50 Rob Herring 2014-03-27 263 port->mapbase = addr;
b0b6abd34c1b50 Rob Herring 2014-03-27 264
088da2a17619cf Peter Hurley 2016-01-16 265 val = of_get_flat_dt_prop(node, "reg-offset", NULL);
088da2a17619cf Peter Hurley 2016-01-16 266 if (val)
088da2a17619cf Peter Hurley 2016-01-16 267 port->mapbase += be32_to_cpu(*val);
1f66dd36bb1843 Greentime Hu 2018-02-13 268 port->membase = earlycon_map(port->mapbase, SZ_4K);
1f66dd36bb1843 Greentime Hu 2018-02-13 269
088da2a17619cf Peter Hurley 2016-01-16 270 val = of_get_flat_dt_prop(node, "reg-shift", NULL);
088da2a17619cf Peter Hurley 2016-01-16 271 if (val)
088da2a17619cf Peter Hurley 2016-01-16 272 port->regshift = be32_to_cpu(*val);
088da2a17619cf Peter Hurley 2016-01-16 273 big_endian = of_get_flat_dt_prop(node, "big-endian", NULL) != NULL ||
088da2a17619cf Peter Hurley 2016-01-16 274 (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) &&
088da2a17619cf Peter Hurley 2016-01-16 275 of_get_flat_dt_prop(node, "native-endian", NULL) != NULL);
088da2a17619cf Peter Hurley 2016-01-16 276 val = of_get_flat_dt_prop(node, "reg-io-width", NULL);
088da2a17619cf Peter Hurley 2016-01-16 277 if (val) {
088da2a17619cf Peter Hurley 2016-01-16 278 switch (be32_to_cpu(*val)) {
088da2a17619cf Peter Hurley 2016-01-16 279 case 1:
088da2a17619cf Peter Hurley 2016-01-16 280 port->iotype = UPIO_MEM;
088da2a17619cf Peter Hurley 2016-01-16 281 break;
088da2a17619cf Peter Hurley 2016-01-16 282 case 2:
088da2a17619cf Peter Hurley 2016-01-16 283 port->iotype = UPIO_MEM16;
088da2a17619cf Peter Hurley 2016-01-16 284 break;
088da2a17619cf Peter Hurley 2016-01-16 285 case 4:
088da2a17619cf Peter Hurley 2016-01-16 286 port->iotype = (big_endian) ? UPIO_MEM32BE : UPIO_MEM32;
088da2a17619cf Peter Hurley 2016-01-16 287 break;
088da2a17619cf Peter Hurley 2016-01-16 288 default:
088da2a17619cf Peter Hurley 2016-01-16 289 pr_warn("[%s] unsupported reg-io-width\n", match->name);
088da2a17619cf Peter Hurley 2016-01-16 290 return -EINVAL;
088da2a17619cf Peter Hurley 2016-01-16 291 }
088da2a17619cf Peter Hurley 2016-01-16 292 }
088da2a17619cf Peter Hurley 2016-01-16 293
31cb9a8575ca04 Eugeniy Paltsev 2017-08-21 294 val = of_get_flat_dt_prop(node, "current-speed", NULL);
31cb9a8575ca04 Eugeniy Paltsev 2017-08-21 295 if (val)
31cb9a8575ca04 Eugeniy Paltsev 2017-08-21 296 early_console_dev.baud = be32_to_cpu(*val);
31cb9a8575ca04 Eugeniy Paltsev 2017-08-21 297
814453adea7d08 Michal Simek 2018-04-10 298 val = of_get_flat_dt_prop(node, "clock-frequency", NULL);
814453adea7d08 Michal Simek 2018-04-10 299 if (val)
814453adea7d08 Michal Simek 2018-04-10 300 port->uartclk = be32_to_cpu(*val);
814453adea7d08 Michal Simek 2018-04-10 301
4d118c9a866590 Peter Hurley 2016-01-16 302 if (options) {
31cb9a8575ca04 Eugeniy Paltsev 2017-08-21 303 early_console_dev.baud = simple_strtoul(options, NULL, 0);
4d118c9a866590 Peter Hurley 2016-01-16 304 strlcpy(early_console_dev.options, options,
4d118c9a866590 Peter Hurley 2016-01-16 305 sizeof(early_console_dev.options));
4d118c9a866590 Peter Hurley 2016-01-16 306 }
05d961320ba624 Peter Hurley 2016-01-16 307 earlycon_init(&early_console_dev, match->name);
4d118c9a866590 Peter Hurley 2016-01-16 308 err = match->setup(&early_console_dev, options);
f28295cc8ce14b Hsin-Yi Wang 2020-09-15 309 earlycon_print_info(&early_console_dev);
b0b6abd34c1b50 Rob Herring 2014-03-27 310 if (err < 0)
b0b6abd34c1b50 Rob Herring 2014-03-27 311 return err;
b0b6abd34c1b50 Rob Herring 2014-03-27 312 if (!early_console_dev.con->write)
b0b6abd34c1b50 Rob Herring 2014-03-27 313 return -ENODEV;
b0b6abd34c1b50 Rob Herring 2014-03-27 314
b0b6abd34c1b50 Rob Herring 2014-03-27 315
b0b6abd34c1b50 Rob Herring 2014-03-27 316 register_console(early_console_dev.con);
b0b6abd34c1b50 Rob Herring 2014-03-27 317 return 0;
b0b6abd34c1b50 Rob Herring 2014-03-27 318 }
8477614d9f7c5c Peter Hurley 2016-01-16 319
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig index f9bae36c03a3..ecfb071fc4f4 100644 --- a/drivers/memory/tegra/Kconfig +++ b/drivers/memory/tegra/Kconfig @@ -48,6 +48,8 @@ config TEGRA124_EMC config TEGRA210_EMC_TABLE bool depends on ARCH_TEGRA_210_SOC || COMPILE_TEST + select OF_EARLY_FLATTREE + select OF_RESERVED_MEM config TEGRA210_EMC tristate "NVIDIA Tegra210 External Memory Controller driver"