RE: [PATCH 01/21] arm64: Add ADI ADSP-SC598 SoC

[Artamonovs, Arturs]

> -----Original Message-----
> From: Arnd Bergmann <arnd@xxxxxxxx>
> Sent: Friday, September 13, 2024 9:16 AM
> To: Artamonovs, Arturs <Arturs.Artamonovs@xxxxxxxxxx>; Catalin Marinas
> <catalin.marinas@xxxxxxx>; Will Deacon <will@xxxxxxxxxx>; Greg Malysa
> <greg.malysa@xxxxxxxxxxx>; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>; Rob
> Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor
> Dooley <conor+dt@xxxxxxxxxx>; Agarwal, Utsav <Utsav.Agarwal@xxxxxxxxxx>;
> Michael Turquette <mturquette@xxxxxxxxxxxx>; Stephen Boyd
> <sboyd@xxxxxxxxxx>; Linus Walleij <linus.walleij@xxxxxxxxxx>; Bartosz
> Golaszewski <brgl@xxxxxxxx>; Thomas Gleixner <tglx@xxxxxxxxxxxxx>; Andi Shyti
> <andi.shyti@xxxxxxxxxx>; Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>;
> Jiri Slaby <jirislaby@xxxxxxxxxx>; Olof Johansson <olof@xxxxxxxxx>;
> soc@xxxxxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx; open list:GPIO
> SUBSYSTEM <linux-gpio@xxxxxxxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx; linux-
> serial@xxxxxxxxxxxxxxx; Linux Factory <adsp-linux@xxxxxxxxxx>; Nathan Barrett-
> Morrison <nathan.morrison@xxxxxxxxxxx>
> Subject: Re: [PATCH 01/21] arm64: Add ADI ADSP-SC598 SoC
> 
> [External]
> 
> On Thu, Sep 12, 2024, at 18:24, Arturs Artamonovs via B4 Relay wrote:
> > From: Arturs Artamonovs <arturs.artamonovs@xxxxxxxxxx>
> >
> > Add ADSP-SC598 platform.
> >
> 
> > --- a/arch/arm64/Kconfig.platforms
> > +++ b/arch/arm64/Kconfig.platforms
> > @@ -292,6 +292,19 @@ config ARCH_ROCKCHIP
> >  	  This enables support for the ARMv8 based Rockchip chipsets,
> >  	  like the RK3368.
> >
> > +config ARCH_SC59X_64
> > +	bool "ADI 64-bit SC59X Platforms"
> > +	select TIMER_OF
> > +	select GPIOLIB
> > +	select PINCTRL
> > +	select COMMON_CLK_ADI_SC598
> > +	select PINCTRL_ADSP
> > +	select ADI_ADSP_IRQ
> > +	select COUNTER
> 
> You can remove the 'select' statements above and just
> make your drivers 'default ARCH_SC59X_64'.
> 
> It may also help to pick a more generic name for the platform
> in case someone wants to add support for SC57x/SC58x later,
> assuming these use some of the same drivers,.
> 
> The Kconfig change can normally go into the same patch
> as the MAINTAINERS file update, but should be separate
> from any of the drivers.
> 

Hi, yes future plan is too add other platforms  like
SC57x/SC58x and SC594. Drivers are compatible. 

> > --- /dev/null
> > +++ b/drivers/soc/adi/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +# todo modularize; already depends on CONFIG_ARCH_SC59X_64 though
> > +
> > +obj-y += system.o
> > diff --git a/drivers/soc/adi/system.c b/drivers/soc/adi/system.c
> 
> I'm confused about the purpose of this driver. Please
> split this out into a separate patch and add a detailed
> description of how it is actually being used, since it
> does not interact with any of the normal subsystems.
> 

Hi, yes we cleaned this driver as much as possible, will
make effort to remove it. 

> > diff --git a/include/linux/soc/adi/adsp-gpio-port.h
> > b/include/linux/soc/adi/adsp-gpio-port.h
> 
> > --- /dev/null
> > +++ b/include/linux/soc/adi/cpu.h
> 
> > --- /dev/null
> > +++ b/include/linux/soc/adi/rcu.h
> > @@ -0,0 +1,55 @@
> 
> > diff --git a/include/linux/soc/adi/sc59x.h
> > b/include/linux/soc/adi/sc59x.h
> 
> > --- /dev/null
> > +++ b/include/linux/soc/adi/sc59x.h
> 
> I don't see these files being included in the driver you add
> here, maybe they got added by accident here?
> 

Should be used in reset driver its removed during rebase, will fix that 
In next series. 

>        Arnd