diff mbox series

tty: serial: bcm63xx: use more precise Kconfig symbol

Message ID 20220310072239.4489-1-zajec5@gmail.com (mailing list archive)
State Superseded
Headers show
Series tty: serial: bcm63xx: use more precise Kconfig symbol | expand

Commit Message

Rafał Miłecki March 10, 2022, 7:22 a.m. UTC 
From: Rafał Miłecki <rafal@milecki.pl>

Patches lowering SERIAL_BCM63XX dependencies led to a discussion and
documentation change regarding "depends" usage. Adjust Kconfig entry to
match current guidelines. Make this symbol available for relevant
architectures only.

Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Ref: f35a07f92616 ("tty: serial: bcm63xx: lower driver dependencies")
Ref: 18084e435ff6 ("Documentation/kbuild: Document platform dependency practises")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/tty/serial/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Geert Uytterhoeven March 10, 2022, 8:19 a.m. UTC | #1 
Hi Rafał,

On Thu, Mar 10, 2022 at 8:22 AM Rafał Miłecki <zajec5@gmail.com> wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> Patches lowering SERIAL_BCM63XX dependencies led to a discussion and
> documentation change regarding "depends" usage. Adjust Kconfig entry to
> match current guidelines. Make this symbol available for relevant
> architectures only.
>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Ref: f35a07f92616 ("tty: serial: bcm63xx: lower driver dependencies")
> Ref: 18084e435ff6 ("Documentation/kbuild: Document platform dependency practises")
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Thanks for your patch!

> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1100,7 +1100,8 @@ config SERIAL_TIMBERDALE
>  config SERIAL_BCM63XX
>         tristate "Broadcom BCM63xx/BCM33xx UART support"
>         select SERIAL_CORE
> -       depends on COMMON_CLK
> +       depends on MIPS || ARM || ARM64 || COMPILE_TEST
> +       default ARCH_BCM4908 || BCM63XX || BMIPS_GENERIC

So ARCH_BCM4908 covers ARM64, and BCM63XX || BMIPS_GENERIC
cover MIPS.  Is there some symbol covering ARM so we can change the
depends to

    depends on FOO || ARCH_BCM4908 || BCM63XX || BMIPS_GENERIC || COMPILE_TEST

?

Anyway, this is definitely a step in the good direction!

>         help
>           This enables the driver for the onchip UART core found on
>           the following chipsets:

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Rafał Miłecki March 10, 2022, 8:33 a.m. UTC | #2 
On 10.03.2022 09:19, Geert Uytterhoeven wrote:
> On Thu, Mar 10, 2022 at 8:22 AM Rafał Miłecki <zajec5@gmail.com> wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Patches lowering SERIAL_BCM63XX dependencies led to a discussion and
>> documentation change regarding "depends" usage. Adjust Kconfig entry to
>> match current guidelines. Make this symbol available for relevant
>> architectures only.
>>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> Ref: f35a07f92616 ("tty: serial: bcm63xx: lower driver dependencies")
>> Ref: 18084e435ff6 ("Documentation/kbuild: Document platform dependency practises")
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> 
> Thanks for your patch!
> 
>> --- a/drivers/tty/serial/Kconfig
>> +++ b/drivers/tty/serial/Kconfig
>> @@ -1100,7 +1100,8 @@ config SERIAL_TIMBERDALE
>>   config SERIAL_BCM63XX
>>          tristate "Broadcom BCM63xx/BCM33xx UART support"
>>          select SERIAL_CORE
>> -       depends on COMMON_CLK
>> +       depends on MIPS || ARM || ARM64 || COMPILE_TEST
>> +       default ARCH_BCM4908 || BCM63XX || BMIPS_GENERIC
> 
> So ARCH_BCM4908 covers ARM64, and BCM63XX || BMIPS_GENERIC
> cover MIPS.  Is there some symbol covering ARM so we can change the
> depends to
> 
>      depends on FOO || ARCH_BCM4908 || BCM63XX || BMIPS_GENERIC || COMPILE_TEST
> 
> ?

Florian, Kevin: do you know what other platforms need that driver?

Ref: c0ec3fd123e9 ("tty: serial: bcm63xx: Allow bcm63xx_uart to be built on other platforms")
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c0ec3fd123e9e64e095fb221ace841e00c04e40b
Florian Fainelli March 10, 2022, 9:49 p.m. UTC | #3 
On 3/10/22 12:33 AM, Rafał Miłecki wrote:
> On 10.03.2022 09:19, Geert Uytterhoeven wrote:
>> On Thu, Mar 10, 2022 at 8:22 AM Rafał Miłecki <zajec5@gmail.com> wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Patches lowering SERIAL_BCM63XX dependencies led to a discussion and
>>> documentation change regarding "depends" usage. Adjust Kconfig entry to
>>> match current guidelines. Make this symbol available for relevant
>>> architectures only.
>>>
>>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>>> Ref: f35a07f92616 ("tty: serial: bcm63xx: lower driver dependencies")
>>> Ref: 18084e435ff6 ("Documentation/kbuild: Document platform
>>> dependency practises")
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>
>> Thanks for your patch!
>>
>>> --- a/drivers/tty/serial/Kconfig
>>> +++ b/drivers/tty/serial/Kconfig
>>> @@ -1100,7 +1100,8 @@ config SERIAL_TIMBERDALE
>>>   config SERIAL_BCM63XX
>>>          tristate "Broadcom BCM63xx/BCM33xx UART support"
>>>          select SERIAL_CORE
>>> -       depends on COMMON_CLK
>>> +       depends on MIPS || ARM || ARM64 || COMPILE_TEST
>>> +       default ARCH_BCM4908 || BCM63XX || BMIPS_GENERIC
>>
>> So ARCH_BCM4908 covers ARM64, and BCM63XX || BMIPS_GENERIC
>> cover MIPS.  Is there some symbol covering ARM so we can change the
>> depends to
>>
>>      depends on FOO || ARCH_BCM4908 || BCM63XX || BMIPS_GENERIC ||
>> COMPILE_TEST
>>
>> ?
> 
> Florian, Kevin: do you know what other platforms need that driver?

Yes that would be ARCH_BCM_63XX.
Geert Uytterhoeven March 11, 2022, 9:13 a.m. UTC | #4 
Hi Florian,

On Thu, Mar 10, 2022 at 10:49 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 3/10/22 12:33 AM, Rafał Miłecki wrote:
> > On 10.03.2022 09:19, Geert Uytterhoeven wrote:
> >> On Thu, Mar 10, 2022 at 8:22 AM Rafał Miłecki <zajec5@gmail.com> wrote:
> >>> From: Rafał Miłecki <rafal@milecki.pl>
> >>>
> >>> Patches lowering SERIAL_BCM63XX dependencies led to a discussion and
> >>> documentation change regarding "depends" usage. Adjust Kconfig entry to
> >>> match current guidelines. Make this symbol available for relevant
> >>> architectures only.
> >>>
> >>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> >>> Ref: f35a07f92616 ("tty: serial: bcm63xx: lower driver dependencies")
> >>> Ref: 18084e435ff6 ("Documentation/kbuild: Document platform
> >>> dependency practises")
> >>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> >>
> >> Thanks for your patch!
> >>
> >>> --- a/drivers/tty/serial/Kconfig
> >>> +++ b/drivers/tty/serial/Kconfig
> >>> @@ -1100,7 +1100,8 @@ config SERIAL_TIMBERDALE
> >>>   config SERIAL_BCM63XX
> >>>          tristate "Broadcom BCM63xx/BCM33xx UART support"
> >>>          select SERIAL_CORE
> >>> -       depends on COMMON_CLK
> >>> +       depends on MIPS || ARM || ARM64 || COMPILE_TEST
> >>> +       default ARCH_BCM4908 || BCM63XX || BMIPS_GENERIC
> >>
> >> So ARCH_BCM4908 covers ARM64, and BCM63XX || BMIPS_GENERIC
> >> cover MIPS.  Is there some symbol covering ARM so we can change the
> >> depends to
> >>
> >>      depends on FOO || ARCH_BCM4908 || BCM63XX || BMIPS_GENERIC ||
> >> COMPILE_TEST
> >>
> >> ?
> >
> > Florian, Kevin: do you know what other platforms need that driver?
>
> Yes that would be ARCH_BCM_63XX.

Had to read this twice, before I realized this is different from BCM63XX.

As we already have

    drivers/char/hw_random/Kconfig:    ARCH_BCM_63XX || BCM63XX ||
BMIPS_GENERIC || COMPILE_TEST
    drivers/spi/Kconfig: depends on BCM63XX || BMIPS_GENERIC ||
ARCH_BCM_63XX || COMPILE_TEST

and drivers/tty/serial/Kconfig would be a third case, would it make
sense to unify BCM63XX and ARCH_BCM_63XX into a single symbol?
We already share several ARCH_* symbols between arm and arm64,
and I expect more will be shared with riscv in the future
(e.g. ARCH_RENESAS).

Or are MIPS BCM63xx and ARM BCM63xx too dissimilar?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Florian Fainelli March 11, 2022, 8:38 p.m. UTC | #5 
On 3/11/22 1:13 AM, Geert Uytterhoeven wrote:
> Hi Florian,
> 
> On Thu, Mar 10, 2022 at 10:49 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 3/10/22 12:33 AM, Rafał Miłecki wrote:
>>> On 10.03.2022 09:19, Geert Uytterhoeven wrote:
>>>> On Thu, Mar 10, 2022 at 8:22 AM Rafał Miłecki <zajec5@gmail.com> wrote:
>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>
>>>>> Patches lowering SERIAL_BCM63XX dependencies led to a discussion and
>>>>> documentation change regarding "depends" usage. Adjust Kconfig entry to
>>>>> match current guidelines. Make this symbol available for relevant
>>>>> architectures only.
>>>>>
>>>>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>>>>> Ref: f35a07f92616 ("tty: serial: bcm63xx: lower driver dependencies")
>>>>> Ref: 18084e435ff6 ("Documentation/kbuild: Document platform
>>>>> dependency practises")
>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>
>>>> Thanks for your patch!
>>>>
>>>>> --- a/drivers/tty/serial/Kconfig
>>>>> +++ b/drivers/tty/serial/Kconfig
>>>>> @@ -1100,7 +1100,8 @@ config SERIAL_TIMBERDALE
>>>>>   config SERIAL_BCM63XX
>>>>>          tristate "Broadcom BCM63xx/BCM33xx UART support"
>>>>>          select SERIAL_CORE
>>>>> -       depends on COMMON_CLK
>>>>> +       depends on MIPS || ARM || ARM64 || COMPILE_TEST
>>>>> +       default ARCH_BCM4908 || BCM63XX || BMIPS_GENERIC
>>>>
>>>> So ARCH_BCM4908 covers ARM64, and BCM63XX || BMIPS_GENERIC
>>>> cover MIPS.  Is there some symbol covering ARM so we can change the
>>>> depends to
>>>>
>>>>      depends on FOO || ARCH_BCM4908 || BCM63XX || BMIPS_GENERIC ||
>>>> COMPILE_TEST
>>>>
>>>> ?
>>>
>>> Florian, Kevin: do you know what other platforms need that driver?
>>
>> Yes that would be ARCH_BCM_63XX.
> 
> Had to read this twice, before I realized this is different from BCM63XX.
> 
> As we already have
> 
>     drivers/char/hw_random/Kconfig:    ARCH_BCM_63XX || BCM63XX ||
> BMIPS_GENERIC || COMPILE_TEST
>     drivers/spi/Kconfig: depends on BCM63XX || BMIPS_GENERIC ||
> ARCH_BCM_63XX || COMPILE_TEST
> 
> and drivers/tty/serial/Kconfig would be a third case, would it make
> sense to unify BCM63XX and ARCH_BCM_63XX into a single symbol?
> We already share several ARCH_* symbols between arm and arm64,
> and I expect more will be shared with riscv in the future
> (e.g. ARCH_RENESAS).

In hindsight, I could have used BCM63XX, however back then a number of
drivers that depended on BCM63XX were either not applicable, or not able
to build with an ARM-based kernel. For consistency with the other
platforms defined in arch/arm/mach-bcm/Kconfig, I went with
ARCH_BCM6_63XX to denote the difference.

> 
> Or are MIPS BCM63xx and ARM BCM63xx too dissimilar?

The chips are an incremental update in a number of ways and did take a
number of the "legacy" cores present in previous chips in the UBUS bus
bridge, such as UART, GPIO, timer, watchdog, USB device controller, but
also added new ones in the AXI bus.

Changing the symbol names could be done, but it feels like it could be
some unnecessary churn now, no?
diff mbox series

Patch

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index e952ec5c7a7c..99313e16c2be 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1100,7 +1100,8 @@  config SERIAL_TIMBERDALE
 config SERIAL_BCM63XX
 	tristate "Broadcom BCM63xx/BCM33xx UART support"
 	select SERIAL_CORE
-	depends on COMMON_CLK
+	depends on MIPS || ARM || ARM64 || COMPILE_TEST
+	default ARCH_BCM4908 || BCM63XX || BMIPS_GENERIC
 	help
 	  This enables the driver for the onchip UART core found on
 	  the following chipsets: