Re: [PATCH] ARM: Add support for Realtek SOC

[Arnd Bergmann]

On Wed, Sep 11, 2019 at 9:46 AM James Tai[æåå] <james.tai@xxxxxxxxxxx> wrote:
> > Subject: Re: [PATCH] ARM: Add support for Realtek SOC

> > > @@ -148,6 +148,7 @@ endif
> > >  textofs-$(CONFIG_ARCH_MSM8X60) := 0x00208000
> > >  textofs-$(CONFIG_ARCH_MSM8960) := 0x00208000
> > >  textofs-$(CONFIG_ARCH_MESON) := 0x00208000
> > > +textofs-$(CONFIG_ARCH_REALTEK) := 0x00208000
> > >  textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000
> >
> > Can you explain why this is needed for your platform?
> >
> We need to reserve memory (0x00000000 ~ 0x001B0000) for rom and boot code.

Ok.

> > > +config ARCH_RTD16XX
> > > +       bool "Enable support for RTD1619"
> > > +       depends on ARCH_REALTEK
> > > +       select ARM_GIC_V3
> > > +       select ARM_PSCI
> >
> > As I understand, this chip uses a Cortex-A55. What is the reason for adding
> > support only to the 32-bit ARM architecture rather than 64-bit?
>
> The RTD16XX platform also support the 64-bit ARM architecture.
> I will add the 64-bit ARM architecture in new version patch.
>
> > Most 64-bit SoCs are only supported with arch/arm64, but generally speaking
> > that is not a requirement. My rule of thumb is that on systems with 1GB of
> > RAM or more, one would want to run a 64-bit kernel, while systems with less
> > than that are better off with a 32-bit one, but that is clearly not the only reason
> > for picking one over the other.
> >
> Support 32-bit ARM architecture is for application compatibility.

Generally speaking, a 64-bit kernel should work better on 64-bit hardware
even when you are running mostly 32-bit applications. However, you may
have device drivers that do not correctly set compat_ioctl handlers.

As I said, it's no problem to allow both, just explain this in the
changelog text for the driver, along with the need for the textofs
setting.

> > It's very unusual to see custom smp operations on an ARMv8 system, as we
> > normally use PSCI here. Can you explain what is going on here? Are you able to
> > use a boot wrapper that implements these in psci instead?
> >
> The smp operations is porting form other Realtek platform.
>
> Currently, The RTD16XX platform can use the PSCI method.
> I will add PSCI method in new version patch.

Ok, perfect!

> > > +       timer_probe();
> > > +       tick_setup_hrtimer_broadcast(); }
> >
> > What do you need tick_setup_hrtimer_broadcast() for? I don't see any other
> > platform calling this.
> >
> I want to initialize the HR timer.

I'm still unsure about this one. My feeling is that it should not be
in the platform
code, but I don't quite understand which hardware needs it. I see that
Lorenzo Pieralisi added the same code to arm64 in commit 9358d755bd5c
("arm64: kernel: initialize broadcast hrtimer based clock event device"),
but nothing ever calls it on 32-bit arch/arm even though the code does
get built in to the kernel.

My feeling is that either you don't really need it, or this is something
that other platforms should really do as well, and it should be called from
the generic time_init() function in arch/arm/kernel/time.c instead.

Can you try to find out more of the background here, and then
move the call to time_init() assuming it is indeed useful?

       Arnd