Re: [PATCH v2 4/8] RISC-V: Kconfig.socs: Add Renesas RZ/Five SoC kconfig option

[Conor.Dooley]

On 18/08/2022 19:19, Lad, Prabhakar wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Geert,
> 
> Thank you for the review.
> 
> On Thu, Aug 18, 2022 at 4:16 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>>
>> Hi Prabhakar,
>>
>> On Mon, Aug 15, 2022 at 5:16 PM Lad Prabhakar
>> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> wrote:
>>> Introduce SOC_RENESAS_RZFIVE config option to enable Renesas RZ/Five
>>> (R9A07G043) SoC, along side also add ARCH_RENESAS config option as most
>>> of the Renesas drivers depend on this config option.
>>>
>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>>
>> Thanks for your patch!
>>
>> The technical part LGTM, so
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>>
>>> --- a/arch/riscv/Kconfig.socs
>>> +++ b/arch/riscv/Kconfig.socs
>>> @@ -80,4 +80,18 @@ config SOC_CANAAN_K210_DTB_SOURCE
>>>
>>>  endif # SOC_CANAAN
>>>
>>> +config ARCH_RENESAS
>>
>> We definitely want ARCH_RENESAS, as it serves as a gatekeeper for
>> Kconfig options for IP cores found on Renesas ARM and RISC-V SoCs.
>>
> Agreed, or else we will end up touching too many Kconfig files.
> 
>>> +       bool
>>> +       select GPIOLIB
>>> +       select PINCTRL
>>> +       select SOC_BUS
>>> +
>>> +config SOC_RENESAS_RZFIVE
>>
>> Do we need this symbol? You could as well make ARCH_RENESAS above
>> visible, and defer the actual SoC selection to ARCH_R9A07G043 in
>> drivers/soc/renesas/Kconfig[1].
>>
> I think we could drop it and just defer the actual SoC selection to
> ARCH_R9A07G043 as you said.
> 
>> I don't know what is the policy on RISC-V. ARM64 has a "single-symbol
>> in arch/arm64/Kconfig.platforms"-policy, so we handle SoC selection
>> in drivers/soc/renesas/Kconfig, and that is fine, as it avoids merge
>> conflicts.
>>
> Agreed.
> 
> @Conor - Does the above sound OK?

It's not my decision to be honest - Palmer's the boss :)

I would rather have a single symbol & a single approach so that we are
all doing the same thing here. As of now, we have all basically done
different things for each SOC that was added - there's SOC_SIFIVE &
SOC_MICROCHIP_POLARFIRE which are obviously not doing the same thing
for starters & then how the symbol is used: selects vs depends + default
all varies between the symbols.

I tried to make some changes to the PolarFire one a few months ago to
add some peripherals but Palmer was not too keen on the changes. We had
a conversation on IRC, the upshot of which was deciding to talk about it
at Plumbers (which is in 3 weeks) as none of them follow his original
intent:
<quote>
the original idea behind Kconfig.socs was to provide an easy place for
users to say "I want all the support for SOC X", and then just have one
Kconfig to turn that on
<\quote>

In theory, that's lovely but not really maintainable & none of us were
doing it anyway. Hopefully we can come up with a plan at Plumbers - so
feel free to chime in (or maybe it gets sorted out here and I don't
have to do any public speaking 😍).

I like Geert's suggestion, I am leaning towards moving everyone to use
ARCH_FOO as its more generic & people that aren't starting with RISC-V
are already likely to be using it. It's the same with the d1 stuff, why
add an extra symbol and layer of indirection why there's a perfectly
good ARCH_SUNXI everywhere that can be reused.

But as I said, not my decision to make & I certainly don't want to be
standing in the way (although I'd say this is unlikely to be applied
before LPC given recent application timelines).

Hope that helps? Or at least explains a bit of where I am coming from..
Conor.