Message ID | 20200724145401.2566-3-krzk@kernel.org |
---|---|
State | Rejected |
Headers | show |
Series | mips: jz4780: Kconfig cleanup | expand |
Hi Krzysztof, Le ven. 24 juil. 2020 à 16:54, Krzysztof Kozlowski <krzk@kernel.org> a écrit : > Enabling the MTD_NAND_JZ4780 driver makes sense only for specific > hardware - the Ingenic SoC architecture. Set it's dependency to > MACH_INGENIC so it will not appear on unrelated architectures (easier > job for downstream/distro kernel engineers). Disagreed. It was done this way so that distro kernels can support multiple SoCs. -Paul > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > Changes since v2: > 1. New patch > --- > drivers/mtd/nand/raw/ingenic/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/raw/ingenic/Kconfig > b/drivers/mtd/nand/raw/ingenic/Kconfig > index 96c5ae8b1bbc..2e3936543ba6 100644 > --- a/drivers/mtd/nand/raw/ingenic/Kconfig > +++ b/drivers/mtd/nand/raw/ingenic/Kconfig > @@ -1,7 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0-only > config MTD_NAND_JZ4780 > tristate "JZ4780 NAND controller" > - depends on MIPS || COMPILE_TEST > + depends on MACH_INGENIC || COMPILE_TEST > depends on JZ4780_NEMC > help > Enables support for NAND Flash connected to the NEMC on JZ4780 SoC > -- > 2.17.1 >
On Fri, 24 Jul 2020 at 17:19, Paul Cercueil <paul@crapouillou.net> wrote: > > Hi Krzysztof, > > > Le ven. 24 juil. 2020 à 16:54, Krzysztof Kozlowski <krzk@kernel.org> a > écrit : > > Enabling the MTD_NAND_JZ4780 driver makes sense only for specific > > hardware - the Ingenic SoC architecture. Set it's dependency to > > MACH_INGENIC so it will not appear on unrelated architectures (easier > > job for downstream/distro kernel engineers). > > Disagreed. It was done this way so that distro kernels can support > multiple SoCs. They will still be able to support multiple SoCs. Nothing changed here. The same we do for all ARM drivers (SoCs are multiplatform)... Unless you want to say that it is possible to support Ingenic SoC without MACH_INGENIC? Best regards, Krzysztof
Le ven. 24 juil. 2020 à 17:33, Krzysztof Kozlowski <krzk@kernel.org> a écrit : > On Fri, 24 Jul 2020 at 17:19, Paul Cercueil <paul@crapouillou.net> > wrote: >> >> Hi Krzysztof, >> >> >> Le ven. 24 juil. 2020 à 16:54, Krzysztof Kozlowski >> <krzk@kernel.org> a >> écrit : >> > Enabling the MTD_NAND_JZ4780 driver makes sense only for specific >> > hardware - the Ingenic SoC architecture. Set it's dependency to >> > MACH_INGENIC so it will not appear on unrelated architectures >> (easier >> > job for downstream/distro kernel engineers). >> >> Disagreed. It was done this way so that distro kernels can support >> multiple SoCs. > > They will still be able to support multiple SoCs. Nothing changed > here. The same we do for all ARM drivers (SoCs are multiplatform)... > Unless you want to say that it is possible to support Ingenic SoC > without MACH_INGENIC? On MIPS, the SoC selection is a Kconfig "choice", so you can only support one SoC family, unfortunately. I'm the one to blame for using "depends on MIPS || COMPILE_TEST" on ingenic drivers, maybe it should depend on MACH_INGENIC indeed, but then it should be made possible to support more than one SoC family. That's something that should be pointed out to the MIPS mailing list, I believe. Btw: Does that mean you are the new maintainer for drivers/memory/? Cheers, -Paul > Best regards, > Krzysztof
On Fri, Jul 24, 2020 at 05:50:06PM +0200, Paul Cercueil wrote: > > > Le ven. 24 juil. 2020 à 17:33, Krzysztof Kozlowski <krzk@kernel.org> a écrit > : > > On Fri, 24 Jul 2020 at 17:19, Paul Cercueil <paul@crapouillou.net> > > wrote: > > > > > > Hi Krzysztof, > > > > > > > > > Le ven. 24 juil. 2020 à 16:54, Krzysztof Kozlowski > > > <krzk@kernel.org> a > > > écrit : > > > > Enabling the MTD_NAND_JZ4780 driver makes sense only for specific > > > > hardware - the Ingenic SoC architecture. Set it's dependency to > > > > MACH_INGENIC so it will not appear on unrelated architectures > > > (easier > > > > job for downstream/distro kernel engineers). > > > > > > Disagreed. It was done this way so that distro kernels can support > > > multiple SoCs. > > > > They will still be able to support multiple SoCs. Nothing changed > > here. The same we do for all ARM drivers (SoCs are multiplatform)... > > Unless you want to say that it is possible to support Ingenic SoC > > without MACH_INGENIC? > > On MIPS, the SoC selection is a Kconfig "choice", so you can only support > one SoC family, unfortunately. Let's say someone selected then some other architecture (MIPS_ALCHEMY). They could select this MTD driver. Does it mean they would be able to run it on Ingenic hardware? > I'm the one to blame for using "depends on > MIPS || COMPILE_TEST" on ingenic drivers, maybe it should depend on > MACH_INGENIC indeed, but then it should be made possible to support more > than one SoC family. > > That's something that should be pointed out to the MIPS mailing list, I > believe. Somehow JZ4780 entries in Maintainers do not mention MIPS list... > > Btw: Does that mean you are the new maintainer for drivers/memory/? Yes, that's the coming change. https://lore.kernel.org/lkml/20200724140345.GB13472@kozik-lap/T/#m91ca20920a7ec5f228a595f1816c15b6c85b6a09 Best regards, Krzysztof
Le ven. 24 juil. 2020 à 17:54, Krzysztof Kozlowski <krzk@kernel.org> a écrit : > On Fri, Jul 24, 2020 at 05:50:06PM +0200, Paul Cercueil wrote: >> >> >> Le ven. 24 juil. 2020 à 17:33, Krzysztof Kozlowski >> <krzk@kernel.org> a écrit >> : >> > On Fri, 24 Jul 2020 at 17:19, Paul Cercueil <paul@crapouillou.net> >> > wrote: >> > > >> > > Hi Krzysztof, >> > > >> > > >> > > Le ven. 24 juil. 2020 à 16:54, Krzysztof Kozlowski >> > > <krzk@kernel.org> a >> > > écrit : >> > > > Enabling the MTD_NAND_JZ4780 driver makes sense only for >> specific >> > > > hardware - the Ingenic SoC architecture. Set it's >> dependency to >> > > > MACH_INGENIC so it will not appear on unrelated architectures >> > > (easier >> > > > job for downstream/distro kernel engineers). >> > > >> > > Disagreed. It was done this way so that distro kernels can >> support >> > > multiple SoCs. >> > >> > They will still be able to support multiple SoCs. Nothing changed >> > here. The same we do for all ARM drivers (SoCs are >> multiplatform)... >> > Unless you want to say that it is possible to support Ingenic SoC >> > without MACH_INGENIC? >> >> On MIPS, the SoC selection is a Kconfig "choice", so you can only >> support >> one SoC family, unfortunately. > > Let's say someone selected then some other architecture > (MIPS_ALCHEMY). > They could select this MTD driver. > > Does it mean they would be able to run it on Ingenic hardware? In *theory* yes, as long as the Kconfig options that MACH_INGENIC selects are enabled, the kernel should boot and work on Ingenic SoCs. -Paul >> I'm the one to blame for using "depends on >> MIPS || COMPILE_TEST" on ingenic drivers, maybe it should depend on >> MACH_INGENIC indeed, but then it should be made possible to support >> more >> than one SoC family. >> >> That's something that should be pointed out to the MIPS mailing >> list, I >> believe. > > Somehow JZ4780 entries in Maintainers do not mention MIPS list... > >> >> Btw: Does that mean you are the new maintainer for drivers/memory/? > > Yes, that's the coming change. > > https://lore.kernel.org/lkml/20200724140345.GB13472@kozik-lap/T/#m91ca20920a7ec5f228a595f1816c15b6c85b6a09 > > Best regards, > Krzysztof >
On Sat, Jul 25, 2020 at 2:17 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le ven. 24 juil. 2020 à 17:54, Krzysztof Kozlowski <krzk@kernel.org> a > écrit : > > On Fri, Jul 24, 2020 at 05:50:06PM +0200, Paul Cercueil wrote: > >> Le ven. 24 juil. 2020 à 17:33, Krzysztof Kozlowski > >> <krzk@kernel.org> a écrit: > >> > On Fri, 24 Jul 2020 at 17:19, Paul Cercueil <paul@crapouillou.net> > >> > wrote: > >> > >> On MIPS, the SoC selection is a Kconfig "choice", so you can only > >> support > >> one SoC family, unfortunately. > > > > Let's say someone selected then some other architecture > > (MIPS_ALCHEMY). > > They could select this MTD driver. > > > > Does it mean they would be able to run it on Ingenic hardware? > > In *theory* yes, as long as the Kconfig options that MACH_INGENIC > selects are enabled, the kernel should boot and work on Ingenic SoCs. Right now, this won't work yet, because there are platform specific functions that are implemented by each of the platforms in arch/mips, e.g. arch/mips/generic/init.c and arch/mips/jz4740/setup.c. A lot of the newer platforms are part of arch/mips/generic (CONFIG_MIPS_GENERIC), which roughly corresponds to CONFIG_ARCH_MULTIPLATFORM on in arch/arm/. Similarly, there are header files in arch/mips/include/asm/mach-*/ that conflict and you need to have the right one. To have more than one platform enabled, each one needs to have all of that platform code converted to fit into the MIPS_GENERIC framework. This can be a lot of work, but I suppose the ingenic platform would be a candidate for which this makes sense, as long as new SoCs of that family still come out. Arnd
Le sam. 25 juil. 2020 à 20:30, Arnd Bergmann <arnd@arndb.de> a écrit : > On Sat, Jul 25, 2020 at 2:17 PM Paul Cercueil <paul@crapouillou.net> > wrote: >> Le ven. 24 juil. 2020 à 17:54, Krzysztof Kozlowski >> <krzk@kernel.org> a >> écrit : >> > On Fri, Jul 24, 2020 at 05:50:06PM +0200, Paul Cercueil wrote: >> >> Le ven. 24 juil. 2020 à 17:33, Krzysztof Kozlowski >> >> <krzk@kernel.org> a écrit: >> >> > On Fri, 24 Jul 2020 at 17:19, Paul Cercueil >> <paul@crapouillou.net> >> >> > wrote: >> >> >> >> On MIPS, the SoC selection is a Kconfig "choice", so you can >> only >> >> support >> >> one SoC family, unfortunately. >> > >> > Let's say someone selected then some other architecture >> > (MIPS_ALCHEMY). >> > They could select this MTD driver. >> > >> > Does it mean they would be able to run it on Ingenic hardware? >> >> In *theory* yes, as long as the Kconfig options that MACH_INGENIC >> selects are enabled, the kernel should boot and work on Ingenic >> SoCs. > > Right now, this won't work yet, because there are platform specific > functions that are implemented by each of the platforms in arch/mips, > e.g. arch/mips/generic/init.c and arch/mips/jz4740/setup.c. > > A lot of the newer platforms are part of arch/mips/generic > (CONFIG_MIPS_GENERIC), which roughly corresponds to > CONFIG_ARCH_MULTIPLATFORM on in arch/arm/. > Similarly, there are header files in arch/mips/include/asm/mach-*/ > that conflict and you need to have the right one. > > To have more than one platform enabled, each one needs to > have all of that platform code converted to fit into the > MIPS_GENERIC framework. This can be a lot of work, but > I suppose the ingenic platform would be a candidate for > which this makes sense, as long as new SoCs of that family > still come out. It should be much less work now that the vast majority of arch/mips/jz4740 is gone. The code left is pretty much generic. I can have a look to convert it to the MIPS_GENERIC framework. Although the code there is scary. Have a look at arch/mips/include/asm/{machine,mips_machine}.h, you'll see what I mean. Anyway, I think we're way past the scope of this patch, so for this one: Acked-by: Paul Cercueil <paul@crapouillou.net> Cheers, -Paul
On Sat, Jul 25, 2020 at 08:30:56PM +0200, Arnd Bergmann wrote: > On Sat, Jul 25, 2020 at 2:17 PM Paul Cercueil <paul@crapouillou.net> wrote: > > Le ven. 24 juil. 2020 à 17:54, Krzysztof Kozlowski <krzk@kernel.org> a > > écrit : > > > On Fri, Jul 24, 2020 at 05:50:06PM +0200, Paul Cercueil wrote: > > >> Le ven. 24 juil. 2020 à 17:33, Krzysztof Kozlowski > > >> <krzk@kernel.org> a écrit: > > >> > On Fri, 24 Jul 2020 at 17:19, Paul Cercueil <paul@crapouillou.net> > > >> > wrote: > > >> > > >> On MIPS, the SoC selection is a Kconfig "choice", so you can only > > >> support > > >> one SoC family, unfortunately. > > > > > > Let's say someone selected then some other architecture > > > (MIPS_ALCHEMY). > > > They could select this MTD driver. > > > > > > Does it mean they would be able to run it on Ingenic hardware? > > > > In *theory* yes, as long as the Kconfig options that MACH_INGENIC > > selects are enabled, the kernel should boot and work on Ingenic SoCs. > > Right now, this won't work yet, because there are platform specific > functions that are implemented by each of the platforms in arch/mips, > e.g. arch/mips/generic/init.c and arch/mips/jz4740/setup.c. I would say even more - no DTS would be provided for such configuration. All Ingenic DTSes are included only with MACH_INGENIC. You cannot build a kernel working on Ingenic without MACH_INGENIC. Even in theory. > > A lot of the newer platforms are part of arch/mips/generic > (CONFIG_MIPS_GENERIC), which roughly corresponds to > CONFIG_ARCH_MULTIPLATFORM on in arch/arm/. > Similarly, there are header files in arch/mips/include/asm/mach-*/ > that conflict and you need to have the right one. > > To have more than one platform enabled, each one needs to > have all of that platform code converted to fit into the > MIPS_GENERIC framework. This can be a lot of work, but > I suppose the ingenic platform would be a candidate for > which this makes sense, as long as new SoCs of that family > still come out. I can therefore change the patch to: depends on MACH_INGENIC || MIPS_GENERIC || COMPILE_TEST Other solution is to leave MTD driver as is and for the memory driver go with my v2 approach: https://lore.kernel.org/lkml/20200724074038.5597-6-krzk@kernel.org/ Best regards, Krzysztof
Le dim. 26 juil. 2020 à 18:06, Krzysztof Kozlowski <krzk@kernel.org> a écrit : > On Sat, Jul 25, 2020 at 08:30:56PM +0200, Arnd Bergmann wrote: >> On Sat, Jul 25, 2020 at 2:17 PM Paul Cercueil >> <paul@crapouillou.net> wrote: >> > Le ven. 24 juil. 2020 à 17:54, Krzysztof Kozlowski >> <krzk@kernel.org> a >> > écrit : >> > > On Fri, Jul 24, 2020 at 05:50:06PM +0200, Paul Cercueil wrote: >> > >> Le ven. 24 juil. 2020 à 17:33, Krzysztof Kozlowski >> > >> <krzk@kernel.org> a écrit: >> > >> > On Fri, 24 Jul 2020 at 17:19, Paul Cercueil >> <paul@crapouillou.net> >> > >> > wrote: >> > >> >> > >> On MIPS, the SoC selection is a Kconfig "choice", so you can >> only >> > >> support >> > >> one SoC family, unfortunately. >> > > >> > > Let's say someone selected then some other architecture >> > > (MIPS_ALCHEMY). >> > > They could select this MTD driver. >> > > >> > > Does it mean they would be able to run it on Ingenic hardware? >> > >> > In *theory* yes, as long as the Kconfig options that MACH_INGENIC >> > selects are enabled, the kernel should boot and work on Ingenic >> SoCs. >> >> Right now, this won't work yet, because there are platform specific >> functions that are implemented by each of the platforms in >> arch/mips, >> e.g. arch/mips/generic/init.c and arch/mips/jz4740/setup.c. > > I would say even more - no DTS would be provided for such > configuration. > All Ingenic DTSes are included only with MACH_INGENIC. You cannot > build > a kernel working on Ingenic without MACH_INGENIC. Even in theory. But the devicetree doesn't have to be built-in. -Paul >> >> A lot of the newer platforms are part of arch/mips/generic >> (CONFIG_MIPS_GENERIC), which roughly corresponds to >> CONFIG_ARCH_MULTIPLATFORM on in arch/arm/. >> Similarly, there are header files in arch/mips/include/asm/mach-*/ >> that conflict and you need to have the right one. >> >> To have more than one platform enabled, each one needs to >> have all of that platform code converted to fit into the >> MIPS_GENERIC framework. This can be a lot of work, but >> I suppose the ingenic platform would be a candidate for >> which this makes sense, as long as new SoCs of that family >> still come out. > > I can therefore change the patch to: > depends on MACH_INGENIC || MIPS_GENERIC || COMPILE_TEST > > Other solution is to leave MTD driver as is and for the memory driver > go > with my v2 approach: > https://lore.kernel.org/lkml/20200724074038.5597-6-krzk@kernel.org/ > > Best regards, > Krzysztof >
On Sun, Jul 26, 2020 at 06:12:27PM +0200, Paul Cercueil wrote: > > > Le dim. 26 juil. 2020 à 18:06, Krzysztof Kozlowski <krzk@kernel.org> a écrit > : > > On Sat, Jul 25, 2020 at 08:30:56PM +0200, Arnd Bergmann wrote: > > > On Sat, Jul 25, 2020 at 2:17 PM Paul Cercueil > > > <paul@crapouillou.net> wrote: > > > > Le ven. 24 juil. 2020 à 17:54, Krzysztof Kozlowski > > > <krzk@kernel.org> a > > > > écrit : > > > > > On Fri, Jul 24, 2020 at 05:50:06PM +0200, Paul Cercueil wrote: > > > > >> Le ven. 24 juil. 2020 à 17:33, Krzysztof Kozlowski > > > > >> <krzk@kernel.org> a écrit: > > > > >> > On Fri, 24 Jul 2020 at 17:19, Paul Cercueil > > > <paul@crapouillou.net> > > > > >> > wrote: > > > > >> > > > > >> On MIPS, the SoC selection is a Kconfig "choice", so you can > > > only > > > > >> support > > > > >> one SoC family, unfortunately. > > > > > > > > > > Let's say someone selected then some other architecture > > > > > (MIPS_ALCHEMY). > > > > > They could select this MTD driver. > > > > > > > > > > Does it mean they would be able to run it on Ingenic hardware? > > > > > > > > In *theory* yes, as long as the Kconfig options that MACH_INGENIC > > > > selects are enabled, the kernel should boot and work on Ingenic > > > SoCs. > > > > > > Right now, this won't work yet, because there are platform specific > > > functions that are implemented by each of the platforms in > > > arch/mips, > > > e.g. arch/mips/generic/init.c and arch/mips/jz4740/setup.c. > > > > I would say even more - no DTS would be provided for such configuration. > > All Ingenic DTSes are included only with MACH_INGENIC. You cannot build > > a kernel working on Ingenic without MACH_INGENIC. Even in theory. > > But the devicetree doesn't have to be built-in. > OK, that's true. Anyway, I don't have strong opinion on any of this. I just followed Arnd's hint. For the memory driver (and MTD NAND as well) which one you prefer: 1. https://lore.kernel.org/lkml/20200724074038.5597-6-krzk@kernel.org/ 2. depends on MACH_INGENIC || MIPS_GENERIC || COMPILE_TEST ? Best regards, Krzysztof
Le dim. 26 juil. 2020 à 18:15, Krzysztof Kozlowski <krzk@kernel.org> a écrit : > On Sun, Jul 26, 2020 at 06:12:27PM +0200, Paul Cercueil wrote: >> >> >> Le dim. 26 juil. 2020 à 18:06, Krzysztof Kozlowski >> <krzk@kernel.org> a écrit >> : >> > On Sat, Jul 25, 2020 at 08:30:56PM +0200, Arnd Bergmann wrote: >> > > On Sat, Jul 25, 2020 at 2:17 PM Paul Cercueil >> > > <paul@crapouillou.net> wrote: >> > > > Le ven. 24 juil. 2020 à 17:54, Krzysztof Kozlowski >> > > <krzk@kernel.org> a >> > > > écrit : >> > > > > On Fri, Jul 24, 2020 at 05:50:06PM +0200, Paul Cercueil >> wrote: >> > > > >> Le ven. 24 juil. 2020 à 17:33, Krzysztof Kozlowski >> > > > >> <krzk@kernel.org> a écrit: >> > > > >> > On Fri, 24 Jul 2020 at 17:19, Paul Cercueil >> > > <paul@crapouillou.net> >> > > > >> > wrote: >> > > > >> >> > > > >> On MIPS, the SoC selection is a Kconfig "choice", so you >> can >> > > only >> > > > >> support >> > > > >> one SoC family, unfortunately. >> > > > > >> > > > > Let's say someone selected then some other architecture >> > > > > (MIPS_ALCHEMY). >> > > > > They could select this MTD driver. >> > > > > >> > > > > Does it mean they would be able to run it on Ingenic >> hardware? >> > > > >> > > > In *theory* yes, as long as the Kconfig options that >> MACH_INGENIC >> > > > selects are enabled, the kernel should boot and work on >> Ingenic >> > > SoCs. >> > > >> > > Right now, this won't work yet, because there are platform >> specific >> > > functions that are implemented by each of the platforms in >> > > arch/mips, >> > > e.g. arch/mips/generic/init.c and arch/mips/jz4740/setup.c. >> > >> > I would say even more - no DTS would be provided for such >> configuration. >> > All Ingenic DTSes are included only with MACH_INGENIC. You >> cannot build >> > a kernel working on Ingenic without MACH_INGENIC. Even in theory. >> >> But the devicetree doesn't have to be built-in. >> > > OK, that's true. Anyway, I don't have strong opinion on any of this. I > just followed Arnd's hint. > > For the memory driver (and MTD NAND as well) which one you prefer: > 1. https://lore.kernel.org/lkml/20200724074038.5597-6-krzk@kernel.org/ > 2. depends on MACH_INGENIC || MIPS_GENERIC || COMPILE_TEST > > ? I'd say a slightly modified #1. The driver shouldn't be "default y" in the first place, so the patch could be to disable it by default. And when the Ingenic code is merged into the MIPS generic framework, I'll send a set of patches to change all driver dependencies on MIPS to MIPS_GENERIC. -Paul
On Sun, Jul 26, 2020 at 6:20 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le dim. 26 juil. 2020 à 18:15, Krzysztof Kozlowski <krzk@kernel.org> a écrit : > > On Sun, Jul 26, 2020 at 06:12:27PM +0200, Paul Cercueil wrote: > >> Le dim. 26 juil. 2020 à 18:06, Krzysztof Kozlowski <krzk@kernel.org> a écrit > > > OK, that's true. Anyway, I don't have strong opinion on any of this. I > > just followed Arnd's hint. > > > > For the memory driver (and MTD NAND as well) which one you prefer: > > 1. https://lore.kernel.org/lkml/20200724074038.5597-6-krzk@kernel.org/ > > 2. depends on MACH_INGENIC || MIPS_GENERIC || COMPILE_TEST > > > > ? > > I'd say a slightly modified #1. The driver shouldn't be "default y" in > the first place, so the patch could be to disable it by default. If it defaults to 'n' even for MACH_INGENIC, you may have to enable it in the four defconfig files for these machines to avoid surprises. > And when the Ingenic code is merged into the MIPS generic framework, I'll > send a set of patches to change all driver dependencies on MIPS to > MIPS_GENERIC. The way we do it on Arm, the machine Kconfig identifiers stay around even for multiplatform targets (which now make up basically actively maintained machines). I don't think it makes any sense for a driver to depend on MIPS_GENERIC: either it is a generic driver that should always be visible or it is specific to a set of SoCs and should depend on some corresponding vendor specific identifiers. Arnd
On Mon, Jul 27, 2020 at 09:55:54AM +0200, Arnd Bergmann wrote: > On Sun, Jul 26, 2020 at 6:20 PM Paul Cercueil <paul@crapouillou.net> wrote: > > Le dim. 26 juil. 2020 à 18:15, Krzysztof Kozlowski <krzk@kernel.org> a écrit : > > > On Sun, Jul 26, 2020 at 06:12:27PM +0200, Paul Cercueil wrote: > > >> Le dim. 26 juil. 2020 à 18:06, Krzysztof Kozlowski <krzk@kernel.org> a écrit > > > > > OK, that's true. Anyway, I don't have strong opinion on any of this. I > > > just followed Arnd's hint. > > > > > > For the memory driver (and MTD NAND as well) which one you prefer: > > > 1. https://lore.kernel.org/lkml/20200724074038.5597-6-krzk@kernel.org/ > > > 2. depends on MACH_INGENIC || MIPS_GENERIC || COMPILE_TEST > > > > > > ? > > > > I'd say a slightly modified #1. The driver shouldn't be "default y" in > > the first place, so the patch could be to disable it by default. > > If it defaults to 'n' even for MACH_INGENIC, you may have to enable > it in the four defconfig files for these machines to avoid surprises. Exactly. Nothing else selects JZ4780_NEMC, so either we keep default y ("if MACH_INGENIC || MIPS_GENERIC"), or you select it directly from MACH_INGENIC/MIPS_GENERIC. A related question is how essential are these drivers? At least for ARM platforms, all essential SoC blocks/IPs are selected by default, if support for chosen SoC is enabled. Only non-essential stuff is left, e.g. DRM, cpufreq, devfreq, ADC, crypto, video, USB, eMMC (although one could argue that it is essential), IOMMU. > > And when the Ingenic code is merged into the MIPS generic framework, I'll > > send a set of patches to change all driver dependencies on MIPS to > > MIPS_GENERIC. > > The way we do it on Arm, the machine Kconfig identifiers stay around > even for multiplatform targets (which now make up basically actively > maintained machines). > > I don't think it makes any sense for a driver to depend on MIPS_GENERIC: > either it is a generic driver that should always be visible or it is specific > to a set of SoCs and should depend on some corresponding vendor > specific identifiers. If support for Ingenic is provided also by MIPS_GENERIC (without selecting MACH_INGENIC), then it makes sense. This would be just a different way than ARM of building multi-platform kernel. Best regards, Krzysztof
Le lun. 27 juil. 2020 à 19:03, Krzysztof Kozlowski <krzk@kernel.org> a écrit : > On Mon, Jul 27, 2020 at 09:55:54AM +0200, Arnd Bergmann wrote: >> On Sun, Jul 26, 2020 at 6:20 PM Paul Cercueil >> <paul@crapouillou.net> wrote: >> > Le dim. 26 juil. 2020 à 18:15, Krzysztof Kozlowski >> <krzk@kernel.org> a écrit : >> > > On Sun, Jul 26, 2020 at 06:12:27PM +0200, Paul Cercueil wrote: >> > >> Le dim. 26 juil. 2020 à 18:06, Krzysztof Kozlowski >> <krzk@kernel.org> a écrit >> > >> > > OK, that's true. Anyway, I don't have strong opinion on any of >> this. I >> > > just followed Arnd's hint. >> > > >> > > For the memory driver (and MTD NAND as well) which one you >> prefer: >> > > 1. >> https://lore.kernel.org/lkml/20200724074038.5597-6-krzk@kernel.org/ >> > > 2. depends on MACH_INGENIC || MIPS_GENERIC || COMPILE_TEST >> > > >> > > ? >> > >> > I'd say a slightly modified #1. The driver shouldn't be "default >> y" in >> > the first place, so the patch could be to disable it by default. >> >> If it defaults to 'n' even for MACH_INGENIC, you may have to enable >> it in the four defconfig files for these machines to avoid >> surprises. > > Exactly. Nothing else selects JZ4780_NEMC, so either we keep default y > ("if MACH_INGENIC || MIPS_GENERIC"), or you select it directly from > MACH_INGENIC/MIPS_GENERIC. > > A related question is how essential are these drivers? At least for > ARM > platforms, all essential SoC blocks/IPs are selected by default, if > support for chosen SoC is enabled. Only non-essential stuff is left, > e.g. DRM, cpufreq, devfreq, ADC, crypto, video, USB, eMMC (although > one > could argue that it is essential), IOMMU. They are only used for NAND access, which is not really essential (some boards only use MMC storage), that's why I said they shouldn't have been enabled by default in the first place. -Paul > >> > And when the Ingenic code is merged into the MIPS generic >> framework, I'll >> > send a set of patches to change all driver dependencies on MIPS to >> > MIPS_GENERIC. >> >> The way we do it on Arm, the machine Kconfig identifiers stay around >> even for multiplatform targets (which now make up basically actively >> maintained machines). >> >> I don't think it makes any sense for a driver to depend on >> MIPS_GENERIC: >> either it is a generic driver that should always be visible or it >> is specific >> to a set of SoCs and should depend on some corresponding vendor >> specific identifiers. > > If support for Ingenic is provided also by MIPS_GENERIC (without > selecting MACH_INGENIC), then it makes sense. This would be just a > different way than ARM of building multi-platform kernel. > > Best regards, > Krzysztof
On Mon, Jul 27, 2020 at 7:03 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Mon, Jul 27, 2020 at 09:55:54AM +0200, Arnd Bergmann wrote: > > > > The way we do it on Arm, the machine Kconfig identifiers stay around > > even for multiplatform targets (which now make up basically actively > > maintained machines). > > > > I don't think it makes any sense for a driver to depend on MIPS_GENERIC: > > either it is a generic driver that should always be visible or it is specific > > to a set of SoCs and should depend on some corresponding vendor > > specific identifiers. > > If support for Ingenic is provided also by MIPS_GENERIC (without > selecting MACH_INGENIC), then it makes sense. This would be just a > different way than ARM of building multi-platform kernel. Yes, it would work just as well, my point was just that it is somewhat confusing to have every architecture do it differently, and that I prefer the way Arm (and also ppc, x86 etc) handles it today. On MIPS, most platforms are not yet part of MIPS_GENERIC, so they are fairly free to pick whatever method works best and is consistent with the rest of the kernel. Arnd
Hello, Arnd Bergmann <arnd@arndb.de> wrote on Mon, 27 Jul 2020 19:28:48 +0200: > On Mon, Jul 27, 2020 at 7:03 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Mon, Jul 27, 2020 at 09:55:54AM +0200, Arnd Bergmann wrote: > > > > > > The way we do it on Arm, the machine Kconfig identifiers stay around > > > even for multiplatform targets (which now make up basically actively > > > maintained machines). > > > > > > I don't think it makes any sense for a driver to depend on MIPS_GENERIC: > > > either it is a generic driver that should always be visible or it is specific > > > to a set of SoCs and should depend on some corresponding vendor > > > specific identifiers. > > > > If support for Ingenic is provided also by MIPS_GENERIC (without > > selecting MACH_INGENIC), then it makes sense. This would be just a > > different way than ARM of building multi-platform kernel. > > Yes, it would work just as well, my point was just that it is somewhat > confusing to have every architecture do it differently, and that I > prefer the way Arm (and also ppc, x86 etc) handles it today. > > On MIPS, most platforms are not yet part of MIPS_GENERIC, so > they are fairly free to pick whatever method works best and is > consistent with the rest of the kernel. In the end, shall I apply Krzysztof patch or shall I wait for an update (eg. without 'default y')? Thanks, Miquèl
On Mon, 3 Aug 2020 at 10:36, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hello, > > Arnd Bergmann <arnd@arndb.de> wrote on Mon, 27 Jul 2020 19:28:48 +0200: > > > On Mon, Jul 27, 2020 at 7:03 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > On Mon, Jul 27, 2020 at 09:55:54AM +0200, Arnd Bergmann wrote: > > > > > > > > The way we do it on Arm, the machine Kconfig identifiers stay around > > > > even for multiplatform targets (which now make up basically actively > > > > maintained machines). > > > > > > > > I don't think it makes any sense for a driver to depend on MIPS_GENERIC: > > > > either it is a generic driver that should always be visible or it is specific > > > > to a set of SoCs and should depend on some corresponding vendor > > > > specific identifiers. > > > > > > If support for Ingenic is provided also by MIPS_GENERIC (without > > > selecting MACH_INGENIC), then it makes sense. This would be just a > > > different way than ARM of building multi-platform kernel. > > > > Yes, it would work just as well, my point was just that it is somewhat > > confusing to have every architecture do it differently, and that I > > prefer the way Arm (and also ppc, x86 etc) handles it today. > > > > On MIPS, most platforms are not yet part of MIPS_GENERIC, so > > they are fairly free to pick whatever method works best and is > > consistent with the rest of the kernel. > > In the end, shall I apply Krzysztof patch or shall I wait for an update > (eg. without 'default y')? No, this patch should be dropped as we decided to leave it as is. At least that was my understanding. The other similar changes - relating to memory driver - were already applied: https://lore.kernel.org/lkml/20200728104503.23655-2-krzk@kernel.org/ Best regards, Krzysztof
diff --git a/drivers/mtd/nand/raw/ingenic/Kconfig b/drivers/mtd/nand/raw/ingenic/Kconfig index 96c5ae8b1bbc..2e3936543ba6 100644 --- a/drivers/mtd/nand/raw/ingenic/Kconfig +++ b/drivers/mtd/nand/raw/ingenic/Kconfig @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only config MTD_NAND_JZ4780 tristate "JZ4780 NAND controller" - depends on MIPS || COMPILE_TEST + depends on MACH_INGENIC || COMPILE_TEST depends on JZ4780_NEMC help Enables support for NAND Flash connected to the NEMC on JZ4780 SoC
Enabling the MTD_NAND_JZ4780 driver makes sense only for specific hardware - the Ingenic SoC architecture. Set it's dependency to MACH_INGENIC so it will not appear on unrelated architectures (easier job for downstream/distro kernel engineers). Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- Changes since v2: 1. New patch --- drivers/mtd/nand/raw/ingenic/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)