| Message ID | 20210303022628.6540-1-taehyun.cho@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc3: make USB_DWC3_EXYNOS independent | expand |
On 03/03/2021 03:26, taehyun cho wrote: > 'ARCH_EXYNOS' is not suitable for DWC3_EXYNOS config. > 'USB_DWC3_EXYNOS' is glue layer which can be used with > Synopsys DWC3 controller on Exynos SoCs. USB_DWC3_EXYNOS' > can be used from Exynos5 to Exynos9. > > Signed-off-by: taehyun cho <taehyun.cho@samsung.com> NACK because you ignored comments from March. Please respond to them instead of resending the same patch. Anyway, when resending you need to version your patches and explain the differences. Please also Cc reviewers and other maintainers. I pointed out this before: scripts/get_maintainer.pl -f drivers/usb/dwc3/dwc3-exynos.c The driver - in current form - should not be available for other architectures. It would clutter other platforms and kernel config selection. If you want to change this, you need to provide rationale (usually by adding support to new non-Exynos platform). Best regards, Krzysztof
On Wed, Mar 03, 2021 at 11:24:01AM +0100, Krzysztof Kozlowski wrote: > On 03/03/2021 03:26, taehyun cho wrote: > > 'ARCH_EXYNOS' is not suitable for DWC3_EXYNOS config. > > 'USB_DWC3_EXYNOS' is glue layer which can be used with > > Synopsys DWC3 controller on Exynos SoCs. USB_DWC3_EXYNOS' > > can be used from Exynos5 to Exynos9. > > > > Signed-off-by: taehyun cho <taehyun.cho@samsung.com> > > NACK because you ignored comments from March. Please respond to them instead > of resending the same patch. > > Anyway, when resending you need to version your patches and explain the > differences. Please also Cc reviewers and other maintainers. I pointed out > this before: > scripts/get_maintainer.pl -f drivers/usb/dwc3/dwc3-exynos.c > > The driver - in current form - should not be available for other > architectures. It would clutter other platforms and kernel config selection. > If you want to change this, you need to provide rationale (usually by adding > support to new non-Exynos platform). No, these crazy "ARCH_FOO" things need to go away. For systems that want to build "universal" kernels, why are they being forced to enable "ARCH_*" just so they can pick specific drivers? That is not done on other architectures, why is ARM64 so "special" in this regard. How do you "know" that these cores/devices are tied to specific ARCH_ platforms? We don't, so that dependency should not be there. Just let any arch pick any driver if it can be built, you never know what it might be run on. Removing ARCH_ dependencies in Kconfig files is a good thing, please do not discourage that from happening. thanks, greg k-h
On Wed, Mar 03, 2021 at 11:30:38AM +0100, Greg Kroah-Hartman wrote: > On Wed, Mar 03, 2021 at 11:24:01AM +0100, Krzysztof Kozlowski wrote: > > On 03/03/2021 03:26, taehyun cho wrote: > > > 'ARCH_EXYNOS' is not suitable for DWC3_EXYNOS config. > > > 'USB_DWC3_EXYNOS' is glue layer which can be used with > > > Synopsys DWC3 controller on Exynos SoCs. USB_DWC3_EXYNOS' > > > can be used from Exynos5 to Exynos9. > > > > > > Signed-off-by: taehyun cho <taehyun.cho@samsung.com> > > > > NACK because you ignored comments from March. Please respond to them instead > > of resending the same patch. > > > > Anyway, when resending you need to version your patches and explain the > > differences. Please also Cc reviewers and other maintainers. I pointed out > > this before: > > scripts/get_maintainer.pl -f drivers/usb/dwc3/dwc3-exynos.c > > > > The driver - in current form - should not be available for other > > architectures. It would clutter other platforms and kernel config selection. > > If you want to change this, you need to provide rationale (usually by adding > > support to new non-Exynos platform). > > No, these crazy "ARCH_FOO" things need to go away. For systems that > want to build "universal" kernels, why are they being forced to enable > "ARCH_*" just so they can pick specific drivers? That is not done on > other architectures, why is ARM64 so "special" in this regard. > > How do you "know" that these cores/devices are tied to specific ARCH_ > platforms? We don't, so that dependency should not be there. > > Just let any arch pick any driver if it can be built, you never know > what it might be run on. Removing ARCH_ dependencies in Kconfig files > is a good thing, please do not discourage that from happening. It's getting more generic topic, so let me Cc Arnd and Guenter (I think once I discussed this with Guenter around watchdog). This is so far component of a SoC, so it cannot be re-used outside of SoC. Unless it appears in a new SoC (just like recent re-use of Samsung serial driver for Apple M1). Because of the architecture, you cannot build universal kernel without ARCH_EXYNOS. You need it. Otherwise the kernel won't boot on hardware with DWC Exynos. Since DWC Exynos won't work without ARCH_EXYNOS - the user will not get any usable binary - I think all, or almost all, SoC specific drivers are limited per ARCH. This limits the amount of choices for distro people and other kernel configuring folks, so they won't have to consider useless options. Anyway, that's the convention or consensus so far for entire SoC. If we want to change it - sure, but let's make it for everyone, not for just this one USB driver. Best regards, Krzysztof
On 03/03/2021 11:38, Krzysztof Kozlowski wrote: > On Wed, Mar 03, 2021 at 11:30:38AM +0100, Greg Kroah-Hartman wrote: >> >> Just let any arch pick any driver if it can be built, you never know >> what it might be run on. Removing ARCH_ dependencies in Kconfig files >> is a good thing, please do not discourage that from happening. If this is the consensus, then I'll add to my todo list removal of ARCH_EXYNOS, ARCH_S3C, ARCH_S5P (and later OMAP, QCOM, NXP and so on) from all drivers. Blindly. Because DWC Exynos is the same as watchdog, clocksource timer, PWM, GPIO/pinctrl Are everyone okay with that? I remember also someone from Suse wanted the opposite - be sure that none of SoC related options appear for his choices, when he configures his kernel without Exynos support. I can't remember the name though... Best regards, Krzysztof
On Wed, Mar 3, 2021 at 11:38 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Wed, Mar 03, 2021 at 11:30:38AM +0100, Greg Kroah-Hartman wrote: > > On Wed, Mar 03, 2021 at 11:24:01AM +0100, Krzysztof Kozlowski wrote: > > > On 03/03/2021 03:26, taehyun cho wrote: > > > > 'ARCH_EXYNOS' is not suitable for DWC3_EXYNOS config. > > > > 'USB_DWC3_EXYNOS' is glue layer which can be used with > > > > Synopsys DWC3 controller on Exynos SoCs. USB_DWC3_EXYNOS' > > > > can be used from Exynos5 to Exynos9. > > > > > > > > Signed-off-by: taehyun cho <taehyun.cho@samsung.com> > > > > > > NACK because you ignored comments from March. Please respond to them instead > > > of resending the same patch. > > > > > > Anyway, when resending you need to version your patches and explain the > > > differences. Please also Cc reviewers and other maintainers. I pointed out > > > this before: > > > scripts/get_maintainer.pl -f drivers/usb/dwc3/dwc3-exynos.c > > > > > > The driver - in current form - should not be available for other > > > architectures. It would clutter other platforms and kernel config selection. > > > If you want to change this, you need to provide rationale (usually by adding > > > support to new non-Exynos platform). > > > > No, these crazy "ARCH_FOO" things need to go away. For systems that > > want to build "universal" kernels, why are they being forced to enable > > "ARCH_*" just so they can pick specific drivers? That is not done on > > other architectures, why is ARM64 so "special" in this regard. > > > > How do you "know" that these cores/devices are tied to specific ARCH_ > > platforms? We don't, so that dependency should not be there. > > > > Just let any arch pick any driver if it can be built, you never know > > what it might be run on. Removing ARCH_ dependencies in Kconfig files > > is a good thing, please do not discourage that from happening. > > It's getting more generic topic, so let me Cc Arnd and Guenter (I think > once I discussed this with Guenter around watchdog). > > This is so far component of a SoC, so it cannot be re-used outside of > SoC. Unless it appears in a new SoC (just like recent re-use of Samsung > serial driver for Apple M1). Because of the architecture, you cannot > build universal kernel without ARCH_EXYNOS. You need it. Otherwise the > kernel won't boot on hardware with DWC Exynos. > > Since DWC Exynos won't work without ARCH_EXYNOS - the user will not get > any usable binary - I think all, or almost all, SoC specific drivers are > limited per ARCH. This limits the amount of choices for distro people > and other kernel configuring folks, so they won't have to consider > useless options. > > Anyway, that's the convention or consensus so far for entire SoC. If we > want to change it - sure, but let's make it for everyone, not for just > this one USB driver. I think we should keep it like this, and have most platform specific drivers be guarded in Kconfig like this. There are two main advantages for this: - Linus has mentioned several times that he does not want to be asked about new drivers that cannot run on x86 when doing 'make oldconfig', and I'm fairly sure this applies to other users as well. Life is too short to know 19000 Kconfig symbols and whether you should enable them or not. Drivers tend to not be tied to an instruction set, so if a driver is used on mips32, arm32 and arm64, but is tied to a particular SoC manufacturer, it should only need to depend on that platform (|| COMPILE_TEST). - A default config enables tons of device drivers, most of which are only ever used on a small number of SoCs. I like the convenience of being able to turn a generic config into one for my particular SoC by turning off all other platforms, and letting the platform specific drivers disappear because of the dependency. There are two related points: - For the specific question of drivers like DWC3_EXYNOS, I would indeed like to see fewer questions asked, and more of the customization done in a generic driver. The same is true for a lot of designware drivers (ethernet, pci, mmc, ...) and similar IP blocks from other vendors. We know these are all the same hardware design, just wired up in slightly different ways, but without help from the hardware designers we have no good way to come up with a generic driver that understands all the possible ways it can be wired up. - I don't like the way we deal with a lot of platform specific irqchip and clocksource drivers. Unlike most other subsystems, these drivers just get selected implicitly from a platform specific Kconfig symbol. The main disadvantage is that this is inconsistent with the rest of the kernel, but there is also a more general problem with the use of 'select' that causes a couple of issues when used too much. Arnd
On Wed, Mar 03, 2021 at 11:24:01AM +0100, Krzysztof Kozlowski wrote: > On 03/03/2021 03:26, taehyun cho wrote: > >'ARCH_EXYNOS' is not suitable for DWC3_EXYNOS config. > >'USB_DWC3_EXYNOS' is glue layer which can be used with > >Synopsys DWC3 controller on Exynos SoCs. USB_DWC3_EXYNOS' > >can be used from Exynos5 to Exynos9. > > > >Signed-off-by: taehyun cho <taehyun.cho@samsung.com> > > NACK because you ignored comments from March. Please respond to them instead > of resending the same patch. > > Anyway, when resending you need to version your patches and explain the > differences. Please also Cc reviewers and other maintainers. I pointed out > this before: > scripts/get_maintainer.pl -f drivers/usb/dwc3/dwc3-exynos.c > > The driver - in current form - should not be available for other > architectures. It would clutter other platforms and kernel config selection. > If you want to change this, you need to provide rationale (usually by adding > support to new non-Exynos platform). > > Best regards, > Krzysztof > Sorry for not answering the comments previously. I was confused with the policy. I couldn't use 'USB_DWC3_EXYNOS' when 'ARCH_EXYNOS' was not defined. That's why I changed 'ARCH_EXYNOS' to 'USB_DWC3'. I modified changelog text from previous commit. I saw some more discussion was done about 'ARCH_XX'. I will wait for the decision. Thanks. Best Regards, Taehyun Cho
On 03/03/2021 14:12, taehyun cho wrote: > On Wed, Mar 03, 2021 at 11:24:01AM +0100, Krzysztof Kozlowski wrote: >> On 03/03/2021 03:26, taehyun cho wrote: >>> 'ARCH_EXYNOS' is not suitable for DWC3_EXYNOS config. >>> 'USB_DWC3_EXYNOS' is glue layer which can be used with >>> Synopsys DWC3 controller on Exynos SoCs. USB_DWC3_EXYNOS' >>> can be used from Exynos5 to Exynos9. >>> >>> Signed-off-by: taehyun cho <taehyun.cho@samsung.com> >> >> NACK because you ignored comments from March. Please respond to them instead >> of resending the same patch. >> >> Anyway, when resending you need to version your patches and explain the >> differences. Please also Cc reviewers and other maintainers. I pointed out >> this before: >> scripts/get_maintainer.pl -f drivers/usb/dwc3/dwc3-exynos.c >> >> The driver - in current form - should not be available for other >> architectures. It would clutter other platforms and kernel config selection. >> If you want to change this, you need to provide rationale (usually by adding >> support to new non-Exynos platform). >> >> Best regards, >> Krzysztof >> > > Sorry for not answering the comments previously. I was confused with the policy. > I couldn't use 'USB_DWC3_EXYNOS' when 'ARCH_EXYNOS' was not defined. That's why > I changed 'ARCH_EXYNOS' to 'USB_DWC3'. I modified changelog text from previous > commit. I saw some more discussion was done about 'ARCH_XX'. I will wait for > the decision. You removed most of other people from Cc-list, except Felipe and USB list. Don't. Reply to all of us. You need to start working with upstream, otherwise your needs or use case won't be easy to understand. So far, for the mainline kernel there is no need to yse USB_DWC3_EXYNOS outside of ARCH_EXYNOS. There is no Exynos9 support. Start sending patches for these instead of customizing mainline to out-of-tree non-upstreamed kernel. Mostly we follow the rule - if it is not in upstream, it does not exist. DTSes are exceptions. But your downstream work is not a reason to tweak upstream without any explanations. Best regards, Krzysztof
On Wed, Mar 03, 2021 at 11:38:39AM +0100, Krzysztof Kozlowski wrote: > On Wed, Mar 03, 2021 at 11:30:38AM +0100, Greg Kroah-Hartman wrote: > > On Wed, Mar 03, 2021 at 11:24:01AM +0100, Krzysztof Kozlowski wrote: > > > On 03/03/2021 03:26, taehyun cho wrote: > > > > 'ARCH_EXYNOS' is not suitable for DWC3_EXYNOS config. > > > > 'USB_DWC3_EXYNOS' is glue layer which can be used with > > > > Synopsys DWC3 controller on Exynos SoCs. USB_DWC3_EXYNOS' > > > > can be used from Exynos5 to Exynos9. > > > > > > > > Signed-off-by: taehyun cho <taehyun.cho@samsung.com> > > > > > > NACK because you ignored comments from March. Please respond to them instead > > > of resending the same patch. > > > > > > Anyway, when resending you need to version your patches and explain the > > > differences. Please also Cc reviewers and other maintainers. I pointed out > > > this before: > > > scripts/get_maintainer.pl -f drivers/usb/dwc3/dwc3-exynos.c > > > > > > The driver - in current form - should not be available for other > > > architectures. It would clutter other platforms and kernel config selection. > > > If you want to change this, you need to provide rationale (usually by adding > > > support to new non-Exynos platform). > > > > No, these crazy "ARCH_FOO" things need to go away. For systems that > > want to build "universal" kernels, why are they being forced to enable > > "ARCH_*" just so they can pick specific drivers? That is not done on > > other architectures, why is ARM64 so "special" in this regard. > > > > How do you "know" that these cores/devices are tied to specific ARCH_ > > platforms? We don't, so that dependency should not be there. > > > > Just let any arch pick any driver if it can be built, you never know > > what it might be run on. Removing ARCH_ dependencies in Kconfig files > > is a good thing, please do not discourage that from happening. > > It's getting more generic topic, so let me Cc Arnd and Guenter (I think > once I discussed this with Guenter around watchdog). > > This is so far component of a SoC, so it cannot be re-used outside of > SoC. Unless it appears in a new SoC (just like recent re-use of Samsung > serial driver for Apple M1). Because of the architecture, you cannot > build universal kernel without ARCH_EXYNOS. You need it. Otherwise the > kernel won't boot on hardware with DWC Exynos. So, to create a "generic" arm64 kernel, I need to go enable all of the ARCH_* variants as well? I thought we were trying to NOT do the same mess that arm32 had for this type of thing. > Since DWC Exynos won't work without ARCH_EXYNOS - the user will not get > any usable binary - I think all, or almost all, SoC specific drivers are > limited per ARCH. This limits the amount of choices for distro people > and other kernel configuring folks, so they won't have to consider > useless options. Why do we have ARCH_EXYNOS at all? x86-64 doesn't have this, why is arm64 somehow special here? That's my complaint, it feels wrong that I have to go and enable all different ARCH_ symbols just to build these drivers. If people want 'default' configurations, then provide an exynos default config file, right? > Anyway, that's the convention or consensus so far for entire SoC. If we > want to change it - sure, but let's make it for everyone, not for just > this one USB driver. Great, let's change it for everyone, I don't see a need for ARCH_* symbols except for people who want to make it simpler for their one board type. And for that, use a defconfig. I've complained about this before, from a driver subsystem maintainer point of view, this is crazy, drivers should be building and working on everything. Worst case, it's a cpu-type issue, to build or not build a driver (i.e. s390, i386), best case it's a feature-type issue to depend on (i.e. USB, TTY, etc.). But never a "this one sub-architecture of this one cpu"-type issue. That feels crazy to me... thanks, greg k-h
On 3/3/21 6:05 AM, Greg Kroah-Hartman wrote: [ ... ] >> Anyway, that's the convention or consensus so far for entire SoC. If we >> want to change it - sure, but let's make it for everyone, not for just >> this one USB driver. > > Great, let's change it for everyone, I don't see a need for ARCH_* > symbols except for people who want to make it simpler for their one > board type. And for that, use a defconfig. > I don't think that will work in practice. Many ARCH_ symbols for various architectures contradict with each other. Almost all watchdog drivers only _build_ for specific platforms/architectures. Guenter
On Wed, Mar 03, 2021 at 06:56:38AM -0800, Guenter Roeck wrote: > On 3/3/21 6:05 AM, Greg Kroah-Hartman wrote: > [ ... ] > >> Anyway, that's the convention or consensus so far for entire SoC. If we > >> want to change it - sure, but let's make it for everyone, not for just > >> this one USB driver. > > > > Great, let's change it for everyone, I don't see a need for ARCH_* > > symbols except for people who want to make it simpler for their one > > board type. And for that, use a defconfig. > > > > I don't think that will work in practice. Many ARCH_ symbols for various > architectures contradict with each other. Almost all watchdog drivers > only _build_ for specific platforms/architectures. Great, that's horrible to hear, so much for a "generic arm64 kernel binary" which I _thought_ was the goal. ugh, you would have thought we would have learned our lesson with arm32... greg k-h
On 03/03/2021 15:05, Greg Kroah-Hartman wrote: > On Wed, Mar 03, 2021 at 11:38:39AM +0100, Krzysztof Kozlowski wrote: >> This is so far component of a SoC, so it cannot be re-used outside of >> SoC. Unless it appears in a new SoC (just like recent re-use of Samsung >> serial driver for Apple M1). Because of the architecture, you cannot >> build universal kernel without ARCH_EXYNOS. You need it. Otherwise the >> kernel won't boot on hardware with DWC Exynos. > > So, to create a "generic" arm64 kernel, I need to go enable all of the > ARCH_* variants as well? I thought we were trying to NOT do the same > mess that arm32 had for this type of thing. The kernel itself is generic and could work on all arm64 platforms. You have to however enable all ARCH_* because of the design choice: 1. device tree sources are toggled with ARCH_xxx 2. the given ARCH_xxx might select specific drivers needed for the kernel to work (or the drivers depend on it). Maybe except the device trees, the case 2. above could be solved not with dependency but "imply". >> Since DWC Exynos won't work without ARCH_EXYNOS - the user will not get >> any usable binary - I think all, or almost all, SoC specific drivers are >> limited per ARCH. This limits the amount of choices for distro people >> and other kernel configuring folks, so they won't have to consider >> useless options. > > Why do we have ARCH_EXYNOS at all? x86-64 doesn't have this, why is > arm64 somehow special here? Because x86 is plug and play? Has BIOS? You can have generic kernel? ARM is not like this - you need to load for example proper device tree blob matching your hardware. This could be loaded/passed/chosen by bootloader, but it's not the same as BIOS. > That's my complaint, it feels wrong that I have to go and enable all > different ARCH_ symbols just to build these drivers. If people want > 'default' configurations, then provide an exynos default config file, > right? If you refer to only building, then options are usually compile-testable. But if you think about having a working kernel, why having a ARCH_xxx for given platform feels wrong? Isn't it nice to hide all stuff behind one option? I think MIPS and RISC-V do similar. > >> Anyway, that's the convention or consensus so far for entire SoC. If we >> want to change it - sure, but let's make it for everyone, not for just >> this one USB driver. > > Great, let's change it for everyone, I don't see a need for ARCH_* > symbols except for people who want to make it simpler for their one > board type. And for that, use a defconfig. > > I've complained about this before, from a driver subsystem maintainer > point of view, this is crazy, drivers should be building and working on > everything. Worst case, it's a cpu-type issue, to build or not build a > driver (i.e. s390, i386), best case it's a feature-type issue to depend > on (i.e. USB, TTY, etc.). But never a "this one sub-architecture of > this one cpu"-type issue. That feels crazy to me... From the building point of view, I agree that the goal is to build them everywhere. This is why we have COMPILE_TEST. From the running/working point of view, these are not PCI or USB cards. These are dedicated blocks of System on Chip. They sometimes got reused on different SoCs but they do not exist outside the SoC. Is there a point to split a complex PCI driver into 10 different parts and be able to use each of this part separately? Usually not... Best regards, Krzysztof
On 03/03/2021 16:09, Greg Kroah-Hartman wrote: > On Wed, Mar 03, 2021 at 06:56:38AM -0800, Guenter Roeck wrote: >> On 3/3/21 6:05 AM, Greg Kroah-Hartman wrote: >> [ ... ] >>>> Anyway, that's the convention or consensus so far for entire SoC. If we >>>> want to change it - sure, but let's make it for everyone, not for just >>>> this one USB driver. >>> >>> Great, let's change it for everyone, I don't see a need for ARCH_* >>> symbols except for people who want to make it simpler for their one >>> board type. And for that, use a defconfig. >>> >> >> I don't think that will work in practice. Many ARCH_ symbols for various >> architectures contradict with each other. Almost all watchdog drivers >> only _build_ for specific platforms/architectures. > > Great, that's horrible to hear, so much for a "generic arm64 kernel > binary" which I _thought_ was the goal. > > ugh, you would have thought we would have learned our lesson with > arm32... I think Guenter here refers to drivers which actually came from arm32 and were not cleaned up to be build without machine-specific bits (arch/arm/mach-xxx). Most or all of the new code is made buildable outside of machine/ARCH_xxx (so COMPILE_TEST). Best regards, Krzysztof
On Wed, Mar 3, 2021 at 4:46 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 03/03/2021 16:09, Greg Kroah-Hartman wrote: > > On Wed, Mar 03, 2021 at 06:56:38AM -0800, Guenter Roeck wrote: > >> On 3/3/21 6:05 AM, Greg Kroah-Hartman wrote: > >> [ ... ] > >>>> Anyway, that's the convention or consensus so far for entire SoC. If we > >>>> want to change it - sure, but let's make it for everyone, not for just > >>>> this one USB driver. > >>> > >>> Great, let's change it for everyone, I don't see a need for ARCH_* > >>> symbols except for people who want to make it simpler for their one > >>> board type. And for that, use a defconfig. > >>> > >> > >> I don't think that will work in practice. Many ARCH_ symbols for various > >> architectures contradict with each other. Almost all watchdog drivers > >> only _build_ for specific platforms/architectures. > > > > Great, that's horrible to hear, so much for a "generic arm64 kernel > > binary" which I _thought_ was the goal. > > > > ugh, you would have thought we would have learned our lesson with > > arm32... I have no idea what you are talking about here. arm64 kernels have always been generic, but you still need drivers for each piece of hardware, we unfortunately can't stop SoC vendors from reinventing the wheel with each new platform and then having to add yet another driver for each subsystems. > I think Guenter here refers to drivers which actually came from arm32 > and were not cleaned up to be build without machine-specific bits > (arch/arm/mach-xxx). > > Most or all of the new code is made buildable outside of > machine/ARCH_xxx (so COMPILE_TEST). There are very few 32-bit arm platforms left that are mutually exclusive, they are largely the ones that have not seen much maintenance in the last ten years but still have users. Generally drivers for those platforms don't have any remaining compile-time dependencies though. Looking at watchdog drivers that can not coexist with others, I see: - 21285_WATCHDOG (ARMv4 CATS from 1998) - 977_WATCHDOG (ARMv4 netwinder from 1998) - IXP4XX_WATCHDOG (Intel/ARMv5 network chip from 2002) - IOP_WATCHDOG (Intel/ARMv5 storage chip from 2002) - SA1100_WATCHDOG (Intel/ARMv4 mobile chip from 1998) - EP93XX_WATCHDOG (Cirrus Logic ARMv4 embedded chip from 2003) - SC520_WDT (AMD ELAN x86 chip from 2001) - M54xx_WATCHDOG (m68k coldfire from early 2000s) - ATH79_WDT (mips) - RC32434_WDT (mips) - INDYDOG (mips) - WDT_MTX1 (mips) - SIBYTE_WDOG (mips) - AR7_WDT (mips) - TXX9_WDT (mips) - OCTEON_WDT (mips) - LANTIQ_WDT (mips) - LOONGSON1_WDT (mips) - MT7621_WDT (mips) - PIC32_WDT (mips) - PIC32_DMT (mips) Arnd
On Wed, Mar 03, 2021 at 05:33:46PM +0100, Arnd Bergmann wrote: > On Wed, Mar 3, 2021 at 4:46 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 03/03/2021 16:09, Greg Kroah-Hartman wrote: > > > On Wed, Mar 03, 2021 at 06:56:38AM -0800, Guenter Roeck wrote: > > >> On 3/3/21 6:05 AM, Greg Kroah-Hartman wrote: > > >> [ ... ] > > >>>> Anyway, that's the convention or consensus so far for entire SoC. If we > > >>>> want to change it - sure, but let's make it for everyone, not for just > > >>>> this one USB driver. > > >>> > > >>> Great, let's change it for everyone, I don't see a need for ARCH_* > > >>> symbols except for people who want to make it simpler for their one > > >>> board type. And for that, use a defconfig. > > >>> > > >> > > >> I don't think that will work in practice. Many ARCH_ symbols for various > > >> architectures contradict with each other. Almost all watchdog drivers > > >> only _build_ for specific platforms/architectures. > > > > > > Great, that's horrible to hear, so much for a "generic arm64 kernel > > > binary" which I _thought_ was the goal. > > > > > > ugh, you would have thought we would have learned our lesson with > > > arm32... > > I have no idea what you are talking about here. arm64 kernels have > always been generic, but you still need drivers for each piece of > hardware, we unfortunately can't stop SoC vendors from reinventing > the wheel with each new platform and then having to add yet another > driver for each subsystems. That's fine, drivers are easy, but when I see comments like "ARCH_ symbols contradict" that means that we can not make a generic kernel image. Otherwise there's no contradiction :) And "new drivers" are almost always not really "new" as everyone uses much the same IP blocks. As proof of this patch where the DWC3 IP block is being used by multiple SoC vendors. To handle that, you split out the SoC-specific portions into sub-drivers, so that you can build a single image of the driver that works on multiple platforms. Nothing new, we've been doing this for years, it's just that out-of-mainline SoC trees that think they can touch "core IP block code" break this all the time, which is what I am pushing back on. Anyway, this is just me as a driver subsystem maintainer being grumpy to see ARCH_ dependancies on tiny little things like SoC-portions for generic IP drivers. Or on individual drivers (i.e. Samsung serial port driver), where they don't belong at all. So the overall goal of the original patch here is great, I want to see that happen, as long as it's done in a way that does not ignore feedback of arch maintainers... thanks, greg k-h
On 03/03/2021 17:43, Greg Kroah-Hartman wrote: >>>>> I don't think that will work in practice. Many ARCH_ symbols for various >>>>> architectures contradict with each other. Almost all watchdog drivers >>>>> only _build_ for specific platforms/architectures. >>>> >>>> Great, that's horrible to hear, so much for a "generic arm64 kernel >>>> binary" which I _thought_ was the goal. >>>> >>>> ugh, you would have thought we would have learned our lesson with >>>> arm32... >> >> I have no idea what you are talking about here. arm64 kernels have >> always been generic, but you still need drivers for each piece of >> hardware, we unfortunately can't stop SoC vendors from reinventing >> the wheel with each new platform and then having to add yet another >> driver for each subsystems. > > That's fine, drivers are easy, but when I see comments like "ARCH_ > symbols contradict" that means that we can not make a generic kernel > image. Otherwise there's no contradiction :) No, they don't contradict. > > And "new drivers" are almost always not really "new" as everyone uses > much the same IP blocks. As proof of this patch where the DWC3 IP block > is being used by multiple SoC vendors. To handle that, you split out > the SoC-specific portions into sub-drivers, so that you can build a > single image of the driver that works on multiple platforms. Nothing > new, we've been doing this for years, it's just that out-of-mainline SoC > trees that think they can touch "core IP block code" break this all the > time, which is what I am pushing back on. I am perfectly fine with (and like it!) putting dwc3 exynos back into base/main dwc3 and getting rid of USB_DWC3_EXYNOS entirely. But this was not part of this patch... > > Anyway, this is just me as a driver subsystem maintainer being grumpy to > see ARCH_ dependancies on tiny little things like SoC-portions for > generic IP drivers. Or on individual drivers (i.e. Samsung serial port > driver), where they don't belong at all. At least with Samsung serial driver we see adding new SoC - Apple M1. Here, the guys in Samsung want to tweak several kernel parts to work with their out-of-tree code without contributing this code back. It's not a community-friendly approach. The upstream kernel should be tweaked to the out-of-tree unknown, hidden and uncontrollable code. Instead I expect from Samsung to contribute the basic Exynos9 support to the upstream. Best regards, Krzysztof
On 03/03/2021 17:49, Krzysztof Kozlowski wrote: >> And "new drivers" are almost always not really "new" as everyone uses >> much the same IP blocks. As proof of this patch where the DWC3 IP block >> is being used by multiple SoC vendors. To handle that, you split out >> the SoC-specific portions into sub-drivers, so that you can build a >> single image of the driver that works on multiple platforms. Nothing >> new, we've been doing this for years, it's just that out-of-mainline SoC >> trees that think they can touch "core IP block code" break this all the >> time, which is what I am pushing back on. > > I am perfectly fine with (and like it!) putting dwc3 exynos back into > base/main dwc3 and getting rid of USB_DWC3_EXYNOS entirely. But this > was not part of this patch... > >> >> Anyway, this is just me as a driver subsystem maintainer being grumpy to >> see ARCH_ dependancies on tiny little things like SoC-portions for >> generic IP drivers. Or on individual drivers (i.e. Samsung serial port >> driver), where they don't belong at all. > > At least with Samsung serial driver we see adding new SoC - Apple M1. > > Here, the guys in Samsung want to tweak several kernel parts to work > with their out-of-tree code without contributing this code back. It's > not a community-friendly approach. The upstream kernel should be tweaked > to the out-of-tree unknown, hidden and uncontrollable code. Eh, obviously I wanted to say: The upstream kernel should *not* be tweaked to the out-of-tree unknown, hidden and uncontrollable code. > > Instead I expect from Samsung to contribute the basic Exynos9 support to > the upstream. Best regards, Krzysztof
On Wed, Mar 03, 2021 at 05:49:01PM +0100, Krzysztof Kozlowski wrote: > On 03/03/2021 17:43, Greg Kroah-Hartman wrote: > > > > > > I don't think that will work in practice. Many ARCH_ symbols for various > > > > > > architectures contradict with each other. Almost all watchdog drivers > > > > > > only _build_ for specific platforms/architectures. > > > > > > > > > > Great, that's horrible to hear, so much for a "generic arm64 kernel > > > > > binary" which I _thought_ was the goal. > > > > > > > > > > ugh, you would have thought we would have learned our lesson with > > > > > arm32... > > > > > > I have no idea what you are talking about here. arm64 kernels have > > > always been generic, but you still need drivers for each piece of > > > hardware, we unfortunately can't stop SoC vendors from reinventing > > > the wheel with each new platform and then having to add yet another > > > driver for each subsystems. > > > > That's fine, drivers are easy, but when I see comments like "ARCH_ > > symbols contradict" that means that we can not make a generic kernel > > image. Otherwise there's no contradiction :) > > No, they don't contradict. > > > > > And "new drivers" are almost always not really "new" as everyone uses > > much the same IP blocks. As proof of this patch where the DWC3 IP block > > is being used by multiple SoC vendors. To handle that, you split out > > the SoC-specific portions into sub-drivers, so that you can build a > > single image of the driver that works on multiple platforms. Nothing > > new, we've been doing this for years, it's just that out-of-mainline SoC > > trees that think they can touch "core IP block code" break this all the > > time, which is what I am pushing back on. > > I am perfectly fine with (and like it!) putting dwc3 exynos back into > base/main dwc3 and getting rid of USB_DWC3_EXYNOS entirely. But this was > not part of this patch... I doubt that will happen, and it's not something that I expect. It's ok to have platform-specific "sub-drivers" for common IP blocks, we do it all the time. But making it separate is good, much like has been done for xhci as well. > > Anyway, this is just me as a driver subsystem maintainer being grumpy to > > see ARCH_ dependancies on tiny little things like SoC-portions for > > generic IP drivers. Or on individual drivers (i.e. Samsung serial port > > driver), where they don't belong at all. > > At least with Samsung serial driver we see adding new SoC - Apple M1. > > Here, the guys in Samsung want to tweak several kernel parts to work with > their out-of-tree code without contributing this code back. It's not a > community-friendly approach. The upstream kernel should be tweaked to the > out-of-tree unknown, hidden and uncontrollable code. Totally agreed, that's not ok. But a different issue than what is happening here. thanks, greg k-h
On Wed, Mar 3, 2021 at 3:05 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Wed, Mar 03, 2021 at 11:38:39AM +0100, Krzysztof Kozlowski wrote: > > On Wed, Mar 03, 2021 at 11:30:38AM +0100, Greg Kroah-Hartman wrote: > > > It's getting more generic topic, so let me Cc Arnd and Guenter (I think > > once I discussed this with Guenter around watchdog). > > > > This is so far component of a SoC, so it cannot be re-used outside of > > SoC. Unless it appears in a new SoC (just like recent re-use of Samsung > > serial driver for Apple M1). Because of the architecture, you cannot > > build universal kernel without ARCH_EXYNOS. You need it. Otherwise the > > kernel won't boot on hardware with DWC Exynos. > > So, to create a "generic" arm64 kernel, I need to go enable all of the > ARCH_* variants as well? I thought we were trying to NOT do the same > mess that arm32 had for this type of thing. Yes, same as on any other architecture that supports more than one platform at a time. > > Since DWC Exynos won't work without ARCH_EXYNOS - the user will not get > > any usable binary - I think all, or almost all, SoC specific drivers are > > limited per ARCH. This limits the amount of choices for distro people > > and other kernel configuring folks, so they won't have to consider > > useless options. > > Why do we have ARCH_EXYNOS at all? x86-64 doesn't have this, why is > arm64 somehow special here? There are only about five chip vendors for x86-64, and they largely just use the same drivers. You still have platform support that you need to enable to run on all machines, see: CONFIG_X86_NUMACHIP CONFIG_X86_VSMP CONFIG_X86_UV CONFIG_X86_GOLDFISH CONFIG_X86_INTEL_CE CONFIG_X86_INTEL_MID CONFIG_X86_AMD_PLATFORM_DEVICE CONFIG_KVM_GUEST CONFIG_JAILHOUSE_GUEST CONFIG_ACRN_GUEST CONFIG_CHROME_PLATFORMS CONFIG_MELLANOX_PLATFORM CONFIG_SURFACE_PLATFORMS laptop vendors in drivers/platform/x86/Kconfig Most of these have only a few drivers, while none of the interesting x86 platforms that modified from ARM or MIPS SoCs (Allwinner/Rockchip Sofia, Unisoc SC9861G-IA, Maxlinear XWAY, MobilEye EyeQ6) made it upstream so far, and probably never will. > That's my complaint, it feels wrong that I have to go and enable all > different ARCH_ symbols just to build these drivers. If people want > 'default' configurations, then provide an exynos default config file, > right? It is very intentional that there is only one defconfig, this helps ensure that none of the platform specific drivers conflicts with other platforms. > I've complained about this before, from a driver subsystem maintainer > point of view, this is crazy, drivers should be building and working on > everything. Worst case, it's a cpu-type issue, to build or not build a > driver (i.e. s390, i386), best case it's a feature-type issue to depend > on (i.e. USB, TTY, etc.). But never a "this one sub-architecture of > this one cpu"-type issue. That feels crazy to me... Basing on the CPU type seems way crazier to me, these have almost nothing to do with what kind of drivers one gets to use. SoC designers rarely care much about the CPU core they put in a SoC, they just license a part fits their needs. At the moment everyone is using ARM, but before that they had the same platforms on powerpc, mips, sh, or their own custom architecture. Some get acquired by Intel and start using x86 cores, and some others move to RISC-V. Arnd
On Wed, Mar 3, 2021 at 5:43 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Wed, Mar 03, 2021 at 05:33:46PM +0100, Arnd Bergmann wrote: > > > > On Wed, Mar 03, 2021 at 06:56:38AM -0800, Guenter Roeck wrote: > > > > > > > >> I don't think that will work in practice. Many ARCH_ symbols for various > > > >> architectures contradict with each other. Almost all watchdog drivers > > > >> only _build_ for specific platforms/architectures. > > That's fine, drivers are easy, but when I see comments like "ARCH_ > symbols contradict" that means that we can not make a generic kernel > image. Otherwise there's no contradiction :) I think the key part of Guenter's sentence above was 'for various architectures', which does not include arm64 or modern arm32 (armv6 or higher). On arm64 specifically, there is no platform specific code at all that is not "just a driver". 32-bit ARM was in that category, and is mostly converted now to allow combining arbitrary platforms within each of the three sets of slightly incompatible CPUs (armv4/v4t/v5 vs armv6/v6k/v7/v7ve/v8 vs nommu armv7-m). powerpc did this first, but still has at least five groups of incompatible CPU cores (8xx, 6xx, 4xx, e500 and everything 64-bit) with one or more platforms in each. mips is mostly incompatible between platforms, though there has been some progress in the past few years to make some of the common ones coexist. m68k can have all mmu-based platforms (mac, atari, amiga, ...) coexist, but the nommu platforms are all mutually exclusive. This is probably not a problem because they are also highly resource constrained. > And "new drivers" are almost always not really "new" as everyone uses > much the same IP blocks. As proof of this patch where the DWC3 IP block > is being used by multiple SoC vendors. To handle that, you split out > the SoC-specific portions into sub-drivers, so that you can build a > single image of the driver that works on multiple platforms. Nothing > new, we've been doing this for years, it's just that out-of-mainline SoC > trees that think they can touch "core IP block code" break this all the > time, which is what I am pushing back on. In those cases where more than one or two platforms share an identical driver, I agree we should just remove those dependencies. Across subsystems, I think those are a small minority, but they are more common in certain areas (usb, pci, networking) than others (clk, pinctrl, gpio). I did find it very helpful to have the dependencies when I removed a number of ARM platforms for linux-5.12 that had no remaining users , and I could just remove all the drivers along with those platforms. I found one networking driver in that set that was a generic licensed IP block that was probably shared by other platforms, but none of those had upstream Linux support, so I removed it as well. Arnd
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig index 2133acf8ee69..2a339b3b82c2 100644 --- a/drivers/usb/dwc3/Kconfig +++ b/drivers/usb/dwc3/Kconfig @@ -67,11 +67,12 @@ config USB_DWC3_OMAP config USB_DWC3_EXYNOS tristate "Samsung Exynos Platform" - depends on (ARCH_EXYNOS || COMPILE_TEST) && OF - default USB_DWC3 + depends on (USB_DWC3 || COMPILE_TEST) && OF help - Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP inside, - say 'Y' or 'M' if you have one such device. + 'USB_DWC3_EXYNOS' is glue layer which can be used with + Synopsys DWC3 controller on Exynos SoCs. USB_DWC3_EXYNOS' + can be used from Exynos5 to Exynos9. say 'Y' or 'M' + if you have one such devices. config USB_DWC3_PCI tristate "PCIe-based Platforms"
'ARCH_EXYNOS' is not suitable for DWC3_EXYNOS config. 'USB_DWC3_EXYNOS' is glue layer which can be used with Synopsys DWC3 controller on Exynos SoCs. USB_DWC3_EXYNOS' can be used from Exynos5 to Exynos9. Signed-off-by: taehyun cho <taehyun.cho@samsung.com> --- drivers/usb/dwc3/Kconfig | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)