[PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[taehyun cho]

'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(-)

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"
-- 
2.26.0

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Krzysztof Kozlowski]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Krzysztof Kozlowski]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Krzysztof Kozlowski]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Arnd Bergmann]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Guenter Roeck]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Krzysztof Kozlowski]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Arnd Bergmann]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Krzysztof Kozlowski]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Krzysztof Kozlowski]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Arnd Bergmann]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Krzysztof Kozlowski]

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

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Arnd Bergmann]

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

[PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[taehyun cho]

'ARCH_EXYNOS' is no more used. '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.

Change-Id: I965b415252a4c64ec5527488be7392113f7ceeb3
Signed-off-by: taehyun cho <taehyun.cho@samsung.com>
---
 drivers/usb/dwc3/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 2133acf8ee69..dc2f92ac8ef6 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -67,10 +67,10 @@ config USB_DWC3_OMAP
 
 config USB_DWC3_EXYNOS
 	tristate "Samsung Exynos Platform"
-	depends on (ARCH_EXYNOS || COMPILE_TEST) && OF
+	depends on (USB_DWC3 || COMPILE_TEST) && OF
 	default USB_DWC3
 	help
-	  Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP inside,
+	  Exynos SoCs chip with one DesignWare Core USB3 IP inside,
 	  say 'Y' or 'M' if you have one such device.
 
 config USB_DWC3_PCI
-- 
2.26.0

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Greg Kroah-Hartman]

On Mon, Feb 08, 2021 at 08:29:06PM +0900, taehyun cho wrote:
> 'ARCH_EXYNOS' is no more used. '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.
> 
> Change-Id: I965b415252a4c64ec5527488be7392113f7ceeb3

Please always run checkpatch.pl on your patches so you do not get grumpy
emails from maintainers telling you to run checkpatch.pl on your
patches...

Please fix and resend.

thanks,

greg k-h

[PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[taehyun cho]

'ARCH_EXYNOS' is no more used. '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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 2133acf8ee69..dc2f92ac8ef6 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -67,10 +67,10 @@ config USB_DWC3_OMAP
 
 config USB_DWC3_EXYNOS
 	tristate "Samsung Exynos Platform"
-	depends on (ARCH_EXYNOS || COMPILE_TEST) && OF
+	depends on (USB_DWC3 || COMPILE_TEST) && OF
 	default USB_DWC3
 	help
-	  Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP inside,
+	  Exynos SoCs chip with one DesignWare Core USB3 IP inside,
 	  say 'Y' or 'M' if you have one such device.
 
 config USB_DWC3_PCI
-- 
2.26.0

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Greg Kroah-Hartman]

On Mon, Feb 08, 2021 at 08:45:38PM +0900, taehyun cho wrote:
> 'ARCH_EXYNOS' is no more used.

I do not understand this sentance.  I see it used in many places in the
kernel tree still, is it being removed?

And if so, as of what commit is this true?

> '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.

This is fine, but the help text doesn't show this, why not put that
information there?

> 
> Signed-off-by: taehyun cho <taehyun.cho@samsung.com>
> ---
>  drivers/usb/dwc3/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 2133acf8ee69..dc2f92ac8ef6 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -67,10 +67,10 @@ config USB_DWC3_OMAP
>  
>  config USB_DWC3_EXYNOS
>  	tristate "Samsung Exynos Platform"
> -	depends on (ARCH_EXYNOS || COMPILE_TEST) && OF
> +	depends on (USB_DWC3 || COMPILE_TEST) && OF
>  	default USB_DWC3
>  	help
> -	  Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP inside,
> +	  Exynos SoCs chip with one DesignWare Core USB3 IP inside,

This does not make much sense anymore, can you reword it?

thanks,

greg k-h

Re: [PATCH] usb: dwc3: make USB_DWC3_EXYNOS independent

[Krzysztof Kozlowski]

On Mon, 8 Feb 2021 at 12:51, taehyun cho <taehyun.cho@samsung.com> wrote:
>
> 'ARCH_EXYNOS' is no more used. 'USB_DWC3_EXYNOS' is glue layer

As a maintainer of Samsung Exynos SoC, I am very surprised to hear
that ARCH_EXYNOS is not used anymore... Quite contrary, in my opinion
it is still used. You need to rephrase this sentence, because it's not
possible to understand in current form.

> which can be used with Synopsys DWC3 controller on Exynos SoCs.
> 'USB_DWC3_EXYNOS' can be used from Exynos5 to Exynos9.

Please provide here the answer to "why you want to remove
ARCH_EXYNOS". Quite precise answer, please.

In future, please Cc lists and people responsible for this drivers:
scripts/get_maintainer.pl -f drivers/usb/dwc3/dwc3-exynos.c

Best regards,
Krzysztof