[v1,1/7] RISC-V: introduce ARCH_FOO kconfig aliases for SOC_FOO symbols

Commit Message

Conor Dooley Nov. 21, 2022, 10:14 p.m. UTC

From: Conor Dooley <[e-mail]>

To facilitate a transfer from SOC_FOO to ARCH_FOO, over a release cycle,
introduce some aliases so that drivers etc that use the SOC_FOO symbols
can be converted.

Signed-off-by: Conor Dooley <[e-mail]>
---
To me, the most straight-forward conversion looks like so:
- this patch is applied in week 2 of the merge window, to avoid
  any conflicts with the Renesas tree
- all users of the SOC_ variants can be converted over a release cycle
  (or more) & no trees need to merge an immutable branch.
- we convert defconfig etc over after all users are converted
- doing it over at least one release cycle means that `make oldconfig`
  will keep people's configs working as they upgrade
- any new SoC families added uses ARCH_FOO
---
 arch/riscv/Kconfig.socs | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Geert Uytterhoeven Jan. 10, 2023, 9:14 p.m. UTC | #1

Hi Conor,

On Mon, Nov 21, 2022 at 11:18 PM Conor Dooley <[e-mail]> wrote:
> From: Conor Dooley <[e-mail]>
>
> To facilitate a transfer from SOC_FOO to ARCH_FOO, over a release cycle,
> introduce some aliases so that drivers etc that use the SOC_FOO symbols
> can be converted.
>
> Signed-off-by: Conor Dooley <[e-mail]>

Thanks for your patch, which is now commit fc43211939bb6874
("RISC-V: kconfig.socs: convert usage of SOC_CANAAN to
ARCH_CANAAN") in riscv/for-next

> --- a/arch/riscv/Kconfig.socs
> +++ b/arch/riscv/Kconfig.socs

> @@ -73,6 +91,9 @@ config SOC_CANAAN_K210_DTB_BUILTIN
>           This option should be selected if no bootloader is being used.
>           If unsure, say Y.
>
> +config ARCH_CANAAN_K210_DTB_SOURCE
> +       def_bool SOC_CANAAN_K210_DTB_SOURCE

This is not correct, as SOC_CANAAN_K210_DTB_SOURCE below is
not a bool, but a string.

> +
>  config SOC_CANAAN_K210_DTB_SOURCE
>         string "Source file for the Canaan Kendryte K210 builtin DTB"
>         depends on SOC_CANAAN

Hence

    obj-$(CONFIG_ARCH_CANAAN_K210_DTB_BUILTIN) += $(addsuffix .dtb.o,
$(CONFIG_ARCH_CANAAN_K210_DTB_SOURCE))

will do the wrong thing later, and I get a non-bootable system (no output)
on my MAiX-BiT.

Unfortunately there is no def_string, so I don't think we can fix this
in a backwards-compatible way, and have to replace all
SOC_CANAAN_K210_DTB_SOURCE by ARCH_CANAAN_K210_DTB_SOURCE,
and urging users to update their .config manually.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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

Conor Dooley Jan. 10, 2023, 9:39 p.m. UTC | #2

Hi Geert...

On Tue, Jan 10, 2023 at 10:14:51PM +0100, Geert Uytterhoeven wrote:
> On Mon, Nov 21, 2022 at 11:18 PM Conor Dooley <[e-mail]> wrote:
> > From: Conor Dooley <[e-mail]>
> >
> > To facilitate a transfer from SOC_FOO to ARCH_FOO, over a release cycle,
> > introduce some aliases so that drivers etc that use the SOC_FOO symbols
> > can be converted.
> >
> > Signed-off-by: Conor Dooley <[e-mail]>
> 
> Thanks for your patch, which is now commit fc43211939bb6874
> ("RISC-V: kconfig.socs: convert usage of SOC_CANAAN to
> ARCH_CANAAN") in riscv/for-next

I see this and I immediately know it is going to be bad news!

> > --- a/arch/riscv/Kconfig.socs
> > +++ b/arch/riscv/Kconfig.socs
> 
> > @@ -73,6 +91,9 @@ config SOC_CANAAN_K210_DTB_BUILTIN
> >           This option should be selected if no bootloader is being used.
> >           If unsure, say Y.
> >
> > +config ARCH_CANAAN_K210_DTB_SOURCE
> > +       def_bool SOC_CANAAN_K210_DTB_SOURCE
> 
> This is not correct, as SOC_CANAAN_K210_DTB_SOURCE below is
> not a bool, but a string.
> 
> > +
> >  config SOC_CANAAN_K210_DTB_SOURCE
> >         string "Source file for the Canaan Kendryte K210 builtin DTB"
> >         depends on SOC_CANAAN
> 
> Hence
> 
>     obj-$(CONFIG_ARCH_CANAAN_K210_DTB_BUILTIN) += $(addsuffix .dtb.o,
> $(CONFIG_ARCH_CANAAN_K210_DTB_SOURCE))
> 
> will do the wrong thing later, and I get a non-bootable system (no output)
> on my MAiX-BiT.
> 
> Unfortunately there is no def_string, so I don't think we can fix this
> in a backwards-compatible way, and have to replace all
> SOC_CANAAN_K210_DTB_SOURCE by ARCH_CANAAN_K210_DTB_SOURCE,
> and urging users to update their .config manually.

That sucks. I'm not sure how I missed this - I had tested originally on
my k210, but evidently there was something wrong with how I had done it.
I must have not tested a subsequent revision on the k210. Mea cupla.

As it wasn't my intention to inflict this change without time for the
symbols to appear in configs, my immediate feeling is that this part of
the change should be reverted.

Of course, we could so something like:
diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
index 34a54e5310a1..d36a5f39f13a 100644
--- a/arch/riscv/Kconfig.socs
+++ b/arch/riscv/Kconfig.socs
@@ -79,7 +79,8 @@ config SOC_CANAAN_K210_DTB_BUILTIN
          If unsure, say Y.
 
 config ARCH_CANAAN_K210_DTB_SOURCE
-       def_bool SOC_CANAAN_K210_DTB_SOURCE
+       string
+       default SOC_CANAAN_K210_DTB_SOURCE
 
 config SOC_CANAAN_K210_DTB_SOURCE
        string "Source file for the Canaan Kendryte K210 builtin DTB"

But I am not sure of how that interacts with the various methods of
updating ones config.
From, admittedly limited, testing it does get updated if
SOC_CANAAN_K210_DTB_SOURCE is changed using menuconfig. Similarly, if
one alters that symbol and does olddefconfig it also gets updated.

I'm sure I am overlooking something here though, what is it?

Thanks & apologies,
Conor.

Damien Le Moal Jan. 10, 2023, 10:22 p.m. UTC | #3

On 1/11/23 06:14, Geert Uytterhoeven wrote:
> Hi Conor,
> 
> On Mon, Nov 21, 2022 at 11:18 PM Conor Dooley <[e-mail]> wrote:
>> From: Conor Dooley <[e-mail]>
>>
>> To facilitate a transfer from SOC_FOO to ARCH_FOO, over a release cycle,
>> introduce some aliases so that drivers etc that use the SOC_FOO symbols
>> can be converted.
>>
>> Signed-off-by: Conor Dooley <[e-mail]>
> 
> Thanks for your patch, which is now commit fc43211939bb6874
> ("RISC-V: kconfig.socs: convert usage of SOC_CANAAN to
> ARCH_CANAAN") in riscv/for-next
> 
>> --- a/arch/riscv/Kconfig.socs
>> +++ b/arch/riscv/Kconfig.socs
> 
>> @@ -73,6 +91,9 @@ config SOC_CANAAN_K210_DTB_BUILTIN
>>           This option should be selected if no bootloader is being used.
>>           If unsure, say Y.
>>
>> +config ARCH_CANAAN_K210_DTB_SOURCE
>> +       def_bool SOC_CANAAN_K210_DTB_SOURCE
> 
> This is not correct, as SOC_CANAAN_K210_DTB_SOURCE below is
> not a bool, but a string.
> 
>> +
>>  config SOC_CANAAN_K210_DTB_SOURCE
>>         string "Source file for the Canaan Kendryte K210 builtin DTB"
>>         depends on SOC_CANAAN
> 
> Hence
> 
>     obj-$(CONFIG_ARCH_CANAAN_K210_DTB_BUILTIN) += $(addsuffix .dtb.o,
> $(CONFIG_ARCH_CANAAN_K210_DTB_SOURCE))
> 
> will do the wrong thing later, and I get a non-bootable system (no output)
> on my MAiX-BiT.
> 
> Unfortunately there is no def_string, so I don't think we can fix this
> in a backwards-compatible way, and have to replace all
> SOC_CANAAN_K210_DTB_SOURCE by ARCH_CANAAN_K210_DTB_SOURCE,
> and urging users to update their .config manually.

Yes, the default built-in dtb is set to k210_generic.dts since we have a
only generic nommu_k210[_sdcard] defconfig. We could add defconfigs per
board type, but that is not the common approach.

By the way, I recall some pushback with the renaming of SOC_XXX to
ARCH_XXX. I personally do not like it and Christoph suggested the revers
approach of renaming arm ARCH_XXX to something else instead. I did not
follow further on that discussion, but it looks like the riscv SOC_XXX
renaming went through ?

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
> 
> 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

Conor Dooley Jan. 10, 2023, 10:34 p.m. UTC | #4

On Wed, Jan 11, 2023 at 07:22:47AM +0900, Damien Le Moal wrote:
> By the way, I recall some pushback with the renaming of SOC_XXX to
> ARCH_XXX. I personally do not like it and Christoph suggested the revers
> approach of renaming arm ARCH_XXX to something else instead. I did not
> follow further on that discussion, but it looks like the riscv SOC_XXX
> renaming went through ?

We asked the ARM guys about it, I think on IRC as they'd not replied to
the thread, and cold water was poured on the idea.

It's Murphy's Law that the thing with the problem would be the k210 too
isn't it.
I could've sworn I booted one of each vendor so kinda kicking myself
right now.

Damien Le Moal Jan. 10, 2023, 10:47 p.m. UTC | #5

On 1/11/23 07:34, Conor Dooley wrote:
> On Wed, Jan 11, 2023 at 07:22:47AM +0900, Damien Le Moal wrote:
>> By the way, I recall some pushback with the renaming of SOC_XXX to
>> ARCH_XXX. I personally do not like it and Christoph suggested the revers
>> approach of renaming arm ARCH_XXX to something else instead. I did not
>> follow further on that discussion, but it looks like the riscv SOC_XXX
>> renaming went through ?
> 
> We asked the ARM guys about it, I think on IRC as they'd not replied to
> the thread, and cold water was poured on the idea.

That is really too bad. Naming something ARCH_XXX that is not a CPU
architecture is simply very confusing I think.

> It's Murphy's Law that the thing with the problem would be the k210 too
> isn't it.
> I could've sworn I booted one of each vendor so kinda kicking myself
> right now.

Geert Uytterhoeven Jan. 11, 2023, 7:46 a.m. UTC | #6

Hi Conor,

On Tue, Jan 10, 2023 at 10:40 PM Conor Dooley <[e-mail]> wrote:
> On Tue, Jan 10, 2023 at 10:14:51PM +0100, Geert Uytterhoeven wrote:
> > On Mon, Nov 21, 2022 at 11:18 PM Conor Dooley <[e-mail]> wrote:
> > > From: Conor Dooley <[e-mail]>
> > >
> > > To facilitate a transfer from SOC_FOO to ARCH_FOO, over a release cycle,
> > > introduce some aliases so that drivers etc that use the SOC_FOO symbols
> > > can be converted.
> > >
> > > Signed-off-by: Conor Dooley <[e-mail]>
> >
> > Thanks for your patch, which is now commit fc43211939bb6874
> > ("RISC-V: kconfig.socs: convert usage of SOC_CANAAN to
> > ARCH_CANAAN") in riscv/for-next
>
> I see this and I immediately know it is going to be bad news!
>
> > > --- a/arch/riscv/Kconfig.socs
> > > +++ b/arch/riscv/Kconfig.socs
> >
> > > @@ -73,6 +91,9 @@ config SOC_CANAAN_K210_DTB_BUILTIN
> > >           This option should be selected if no bootloader is being used.
> > >           If unsure, say Y.
> > >
> > > +config ARCH_CANAAN_K210_DTB_SOURCE
> > > +       def_bool SOC_CANAAN_K210_DTB_SOURCE
> >
> > This is not correct, as SOC_CANAAN_K210_DTB_SOURCE below is
> > not a bool, but a string.
> >
> > > +
> > >  config SOC_CANAAN_K210_DTB_SOURCE
> > >         string "Source file for the Canaan Kendryte K210 builtin DTB"
> > >         depends on SOC_CANAAN
> >
> > Hence
> >
> >     obj-$(CONFIG_ARCH_CANAAN_K210_DTB_BUILTIN) += $(addsuffix .dtb.o,
> > $(CONFIG_ARCH_CANAAN_K210_DTB_SOURCE))
> >
> > will do the wrong thing later, and I get a non-bootable system (no output)
> > on my MAiX-BiT.
> >
> > Unfortunately there is no def_string, so I don't think we can fix this
> > in a backwards-compatible way, and have to replace all
> > SOC_CANAAN_K210_DTB_SOURCE by ARCH_CANAAN_K210_DTB_SOURCE,
> > and urging users to update their .config manually.
>
> That sucks. I'm not sure how I missed this - I had tested originally on
> my k210, but evidently there was something wrong with how I had done it.
> I must have not tested a subsequent revision on the k210. Mea cupla.
>
> As it wasn't my intention to inflict this change without time for the
> symbols to appear in configs, my immediate feeling is that this part of
> the change should be reverted.
>
> Of course, we could so something like:
> diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
> index 34a54e5310a1..d36a5f39f13a 100644
> --- a/arch/riscv/Kconfig.socs
> +++ b/arch/riscv/Kconfig.socs
> @@ -79,7 +79,8 @@ config SOC_CANAAN_K210_DTB_BUILTIN
>           If unsure, say Y.
>
>  config ARCH_CANAAN_K210_DTB_SOURCE
> -       def_bool SOC_CANAAN_K210_DTB_SOURCE
> +       string
> +       default SOC_CANAAN_K210_DTB_SOURCE

Thanks, that works!
I guess it was too late in the evening for me, to realize that the lack of
"def_string" can be worked around using "string" and "default".

>  config SOC_CANAAN_K210_DTB_SOURCE
>         string "Source file for the Canaan Kendryte K210 builtin DTB"
>
> But I am not sure of how that interacts with the various methods of
> updating ones config.
> From, admittedly limited, testing it does get updated if
> SOC_CANAAN_K210_DTB_SOURCE is changed using menuconfig. Similarly, if
> one alters that symbol and does olddefconfig it also gets updated.

With the above, the value of SOC_CANAAN_K210_DTB_SOURCE in existing
config files will be copied to ARCH_CANAAN_K210_DTB_SOURCE when running
"make oldconfig".
Hence in v6.4 you can drop the old SOC_CANAAN_K210_DTB_SOURCE.  As that
symbol is not present in defconfigs, there is nothing to update there,

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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

Geert Uytterhoeven Jan. 11, 2023, 7:50 a.m. UTC | #7

Hi Damien,

On Tue, Jan 10, 2023 at 11:47 PM Damien Le Moal
<[e-mail]> wrote:
> On 1/11/23 07:34, Conor Dooley wrote:
> > On Wed, Jan 11, 2023 at 07:22:47AM +0900, Damien Le Moal wrote:
> >> By the way, I recall some pushback with the renaming of SOC_XXX to
> >> ARCH_XXX. I personally do not like it and Christoph suggested the revers
> >> approach of renaming arm ARCH_XXX to something else instead. I did not
> >> follow further on that discussion, but it looks like the riscv SOC_XXX
> >> renaming went through ?
> >
> > We asked the ARM guys about it, I think on IRC as they'd not replied to
> > the thread, and cold water was poured on the idea.
>
> That is really too bad. Naming something ARCH_XXX that is not a CPU
> architecture is simply very confusing I think.

I agree it is.
However, fixing that means adding backwards-compatiblity symbols,
updating the few thousand existing users, and dropping the old symbols.
And handling all the fall-out of people who didn't run "make oldconfig"
in between, now wondering why their kernel no longer boots.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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

Patch

diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
index 75fb0390d6bd..58cd2304b82d 100644
--- a/arch/riscv/Kconfig.socs
+++ b/arch/riscv/Kconfig.socs
@@ -1,5 +1,8 @@ 
 menu "SoC selection"
 
+config ARCH_MICROCHIP_POLARFIRE
+	def_bool SOC_MICROCHIP_POLARFIRE
+
 config SOC_MICROCHIP_POLARFIRE
 	bool "Microchip PolarFire SoCs"
 	select MCHP_CLK_MPFS
@@ -12,6 +15,9 @@  config ARCH_RENESAS
 	help
 	  This enables support for the RISC-V based Renesas SoCs.
 
+config ARCH_SIFIVE
+	def_bool SOC_SIFIVE
+
 config SOC_SIFIVE
 	bool "SiFive SoCs"
 	select SERIAL_SIFIVE if TTY
@@ -23,6 +29,9 @@  config SOC_SIFIVE
 	help
 	  This enables support for SiFive SoC platform hardware.
 
+config ARCH_STARFIVE
+	def_bool SOC_STARFIVE
+
 config SOC_STARFIVE
 	bool "StarFive SoCs"
 	select PINCTRL
@@ -31,6 +40,9 @@  config SOC_STARFIVE
 	help
 	  This enables support for StarFive SoC platform hardware.
 
+config ARCH_VIRT
+	def_bool SOC_VIRT
+	
 config SOC_VIRT
 	bool "QEMU Virt Machine"
 	select CLINT_TIMER if RISCV_M_MODE
@@ -46,6 +58,9 @@  config SOC_VIRT
 	help
 	  This enables support for QEMU Virt Machine.
 
+config ARCH_CANAAN
+	def_bool SOC_CANAAN
+
 config SOC_CANAAN
 	bool "Canaan Kendryte K210 SoC"
 	depends on !MMU
@@ -62,6 +77,9 @@  config SOC_CANAAN
 
 if SOC_CANAAN
 
+config ARCH_CANAAN_K210_DTB_BUILTIN
+	def_bool SOC_CANAAN_K210_DTB_BUILTIN
+
 config SOC_CANAAN_K210_DTB_BUILTIN
 	bool "Builtin device tree for the Canaan Kendryte K210"
 	depends on SOC_CANAAN
@@ -73,6 +91,9 @@  config SOC_CANAAN_K210_DTB_BUILTIN
 	  This option should be selected if no bootloader is being used.
 	  If unsure, say Y.
 
+config ARCH_CANAAN_K210_DTB_SOURCE
+	def_bool SOC_CANAAN_K210_DTB_SOURCE
+
 config SOC_CANAAN_K210_DTB_SOURCE
 	string "Source file for the Canaan Kendryte K210 builtin DTB"
 	depends on SOC_CANAAN