RE: [PATCH] ARM: config: Add Realtek RTD16XX defconfig

[James Tai[æåå]]

> Subject: Re: [PATCH] ARM: config: Add Realtek RTD16XX defconfig
> 
> On Thu, Sep 5, 2019 at 10:14 AM <jamestai.sky@xxxxxxxxx> wrote:
> >
> > From: "james.tai" <james.tai@xxxxxxxxxxx>
> >
> > Add a defconfig for Realtek RTD16XX platform.
> >
> > Signed-off-by: james.tai <james.tai@xxxxxxxxxxx>
> > ---
> >  arch/arm/configs/rtd16xx_defconfig | 427
> > +++++++++++++++++++++++++++++
> 
> We usually try to have one defconfig per vendor. Expecting that there will be
> other Realtek SoCs in the future that we may add here, I would name this
> 'rtd_defconfig' or 'realtek_defconfig'.
> 
I will modify config file name in new version patch.

> Please also add the set of options you want to multi_v7_defconfig so you are
> able to boot with that.

OK, I understand.

> >  1 file changed, 427 insertions(+)
> >  create mode 100644 arch/arm/configs/rtd16xx_defconfig
> >
> > diff --git a/arch/arm/configs/rtd16xx_defconfig
> > b/arch/arm/configs/rtd16xx_defconfig
> > new file mode 100644
> > index 000000000000..49bcbe6c6af8
> > --- /dev/null
> > +++ b/arch/arm/configs/rtd16xx_defconfig
> > @@ -0,0 +1,427 @@
> > +CONFIG_SYSVIPC=y
> > +CONFIG_NO_HZ=y
> > +CONFIG_HIGH_RES_TIMERS=y
> > +CONFIG_CGROUPS=y
> > +CONFIG_BLK_DEV_INITRD=y
> > +CONFIG_EMBEDDED=y
> 
> I normally would not turn on CONFIG_EMBEDDED, this is only needed to
> change some rare options.

I will turn off CONFIG_EMBEDDED in new version patch.

> > +CONFIG_PERF_EVENTS=y
> > +CONFIG_ARCH_REALTEK=y
> > +CONFIG_ARCH_RTD16XX=y
> > +CONFIG_ARM_THUMBEE=y
> 
> ThumbEE is deprecated in ARMv8, and one usually should not rely on it. If you
> don't actually need it, just turn it off.

I will turn off CONFIG_ARM_THUMBEE in new version patch.

> (note: this is unrelated to regular thumb execution, which is enabled by
> default)
> 
> > +# CONFIG_CACHE_L2X0 is not set
> > +# CONFIG_ARM_ERRATA_643719 is not set
> CONFIG_ARM_ERRATA_814220=y
> > +CONFIG_SMP=y CONFIG_SCHED_MC=y CONFIG_SCHED_SMT=y
> 
> If you don't have SMT in the CPU, there is no need ot enable this.

RTD16XX platform don't have SMT in the CPU.
I will turn off CONFIG_SCHED_SMT in new version patch.

> > +CONFIG_HAVE_ARM_ARCH_TIMER=y
> > +CONFIG_MCPM=y
> > +CONFIG_NR_CPUS=6
> > +CONFIG_HZ_250=y
> > +CONFIG_OABI_COMPAT=y
> 
> It seems unlikely you want OABI_COMPAT
> 

I will turn off CONFIG_OABI_COMPAT in new version patch.

> > +CONFIG_HIGHMEM=y
> > +CONFIG_FORCE_MAX_ZONEORDER=12
> > +CONFIG_SECCOMP=y
> > +CONFIG_ARM_APPENDED_DTB=y
> > +CONFIG_ARM_ATAG_DTB_COMPAT=y
> > +CONFIG_KEXEC=y
> > +CONFIG_EFI=y
> 
> What method do you actually use for booting? New platforms should generally
> not require CONFIG_ARM_APPENDED_DTB or
> CONFIG_ARM_ATAG_DTB_COMPAT, and I suspect you don't use EFI.
> 
RTD16XX platform don't use EFI.
I will turn off these config in new version patch.

> > +CONFIG_CPUFREQ_DT=y
> > +CONFIG_QORIQ_CPUFREQ=y
> 
> QORIQ_CPUFREQ is a platform specific option that you won't need.
> 
I will turn off CONFIG_QORIQ_CPUFREQ in new version patch.

> > +CONFIG_NET_DSA=m
> > +CONFIG_CAN=y
> > +CONFIG_CAN_FLEXCAN=m
> > +CONFIG_CAN_RCAR=m
> > +CONFIG_BT=m
> > +CONFIG_BT_HCIUART=m
> > +CONFIG_BT_HCIUART_BCM=y
> > +CONFIG_BT_MRVL=m
> > +CONFIG_BT_MRVL_SDIO=m
> 
> Many more hardware specific drivers here that you should turn off
>
> > +CONFIG_MTD=y
> > +CONFIG_MTD_CMDLINE_PARTS=y
> > +CONFIG_MTD_BLOCK=y
> > +CONFIG_MTD_CFI=y
> > +CONFIG_MTD_PHYSMAP=y
> > +CONFIG_MTD_PHYSMAP_OF=y
> > +CONFIG_MTD_RAW_NAND=y
> > +CONFIG_MTD_NAND_DENALI_DT=y
> > +CONFIG_MTD_NAND_BRCMNAND=y
> 
> and here.

I will turn off these config in new version patch.

> > +CONFIG_BLK_DEV_LOOP=y
> > +CONFIG_BLK_DEV_RAM=y
> > +CONFIG_BLK_DEV_RAM_SIZE=65536
> 
> Do you require BLK_DEV_RAM for initrd? Normally one uses initramfs instead
> or tmpfs instead.
> 
RTD16XX platform have use 'initramfs'.
I will turn off this config in new version patch.

> > +# CONFIG_NET_VENDOR_3COM is not set
> > +# CONFIG_NET_VENDOR_ADAPTEC is not set #
> CONFIG_NET_VENDOR_AGERE is
> > +not set # CONFIG_NET_VENDOR_ALACRITECH is not set #
> > +CONFIG_NET_VENDOR_ALTEON is not set
> 
> I would trim the list here, just leave all network device vendors enabled, they
> don't hurt.
> 
I will turn on these config in new version patch.

> > +CONFIG_USB_PEGASUS=y
> > +CONFIG_USB_RTL8152=m
> > +CONFIG_USB_LAN78XX=m
> > +CONFIG_USB_USBNET=y
> > +CONFIG_USB_NET_SMSC75XX=y
> > +CONFIG_USB_NET_SMSC95XX=y
> > +CONFIG_BRCMFMAC=m
> > +CONFIG_MWIFIEX=m
> > +CONFIG_MWIFIEX_SDIO=m
> > +CONFIG_RT2X00=m
> > +CONFIG_RT2800USB=m
> 
> Do you need all of the above? It's no problem to enable them if you do, it just
> seems unusual.

RTD16XX platform don't need all config.
I will turn off these config in new version patch.

>         Arnd
> 
> ------Please consider the environment before printing this e-mail.