Re: [PATCH v2 5/8] soc: sifive: ccache: Add StarFive JH7110 support

[Emil Renner Berthing]

On Tue, 22 Nov 2022 at 10:03, Hal Feng <hal.feng@xxxxxxxxxxxxxxxx> wrote:
>
> On Fri, 18 Nov 2022 19:45:57 +0800, Conor Dooley wrote:
> > Hey Emil/Hal,
> >
> > On Fri, Nov 18, 2022 at 09:17:11AM +0800, Hal Feng wrote:
> > > From: Emil Renner Berthing <kernel@xxxxxxxx>
> > >
> > > This adds support for the StarFive JH7110 SoC which also
> > > features this SiFive cache controller.
> > >
> > > Signed-off-by: Emil Renner Berthing <kernel@xxxxxxxx>
> > > Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> > > ---
> > >  arch/riscv/Kconfig.socs            | 1 +
> > >  drivers/soc/Makefile               | 2 +-
> > >  drivers/soc/sifive/Kconfig         | 2 +-
> > >  drivers/soc/sifive/sifive_ccache.c | 1 +
> > >  4 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
> > > index 69774bb362d6..5a40e05f8cab 100644
> > > --- a/arch/riscv/Kconfig.socs
> > > +++ b/arch/riscv/Kconfig.socs
> > > @@ -22,6 +22,7 @@ config SOC_STARFIVE
> > >     bool "StarFive SoCs"
> > >     select PINCTRL
> > >     select RESET_CONTROLLER
> > > +   select SIFIVE_CCACHE
> >
> > Please no. I am trying to get rid of these selects + I cannot figure out
> > why this driver is so important that you *need* to select it. Surely the
> > SoC is useable without it>
> > Is this a hang over from your vendor tree that uses the driver to do
> > non-coherent stuff for the jh7100?
>
> I have tested that the board can successfully boot up without the cache
> driver. The `select` can be removed for JH7110. @Emil, what do you think
> of this?

Yes, for the JH7110 this is not strictly needed, just like the
Unmatched board. For the StarFive JH7100 it is though.
So if you're only adding support for the JH7110 then it's not needed.

> >
> > >     select SIFIVE_PLIC
> > >     help
> > >       This enables support for StarFive SoC platform hardware.
> > > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > > index 69ba6508cf2c..534669840858 100644
> > > --- a/drivers/soc/Makefile
> > > +++ b/drivers/soc/Makefile
> > > @@ -26,7 +26,7 @@ obj-y                             += qcom/
> > >  obj-y                              += renesas/
> > >  obj-y                              += rockchip/
> > >  obj-$(CONFIG_SOC_SAMSUNG)  += samsung/
> > > -obj-$(CONFIG_SOC_SIFIVE)   += sifive/
> > > +obj-y                              += sifive/
> >
> > This bit is fine.
> >
> > >  obj-y                              += sunxi/
> > >  obj-$(CONFIG_ARCH_TEGRA)   += tegra/
> > >  obj-y                              += ti/
> > > diff --git a/drivers/soc/sifive/Kconfig b/drivers/soc/sifive/Kconfig
> > > index ed4c571f8771..e86870be34c9 100644
> > > --- a/drivers/soc/sifive/Kconfig
> > > +++ b/drivers/soc/sifive/Kconfig
> > > @@ -1,6 +1,6 @@
> > >  # SPDX-License-Identifier: GPL-2.0
> > >
> > > -if SOC_SIFIVE
> > > +if SOC_SIFIVE || SOC_STARFIVE
> >
> > As I suppose is this - but hardly scalable. I suppose it doesn't really
> > matter.
> >
> > >  config SIFIVE_CCACHE
> > >     bool "Sifive Composable Cache controller"
> > > diff --git a/drivers/soc/sifive/sifive_ccache.c b/drivers/soc/sifive/sifive_ccache.c
> > > index 1c171150e878..9489d1a90fbc 100644
> > > --- a/drivers/soc/sifive/sifive_ccache.c
> > > +++ b/drivers/soc/sifive/sifive_ccache.c
> > > @@ -107,6 +107,7 @@ static const struct of_device_id sifive_ccache_ids[] = {
> > >     { .compatible = "sifive,fu540-c000-ccache" },
> > >     { .compatible = "sifive,fu740-c000-ccache" },
> > >     { .compatible = "sifive,ccache0" },
> > > +   { .compatible = "starfive,jh7110-ccache" },
> >
> > Per my second reply to the previous patch, I am not sure why you do not
> > just have a fallback compatible in the binding/dt for the fu740 ccache
> > since you appear to have identical configuration?
>
> Yeah, I will use the compatible of fu740 and modify this patch.

No, the JH7110 should not pretend to be a fu740, but if you add

compatible = "starfive,jh7110-ccache", "sifive,ccache0";

then this driver should still match "sifive,ccache0" without adding
the "starfive,jh7110-ccache" entry.

>
> Best regards,
> Hal