Re: Usefulness of CONFIG_MACH_JZ47*

[Paul Cercueil]

Le mar., sept. 20 2022 at 16:19:41 +0200, H. Nikolaus Schaller 
<hns@xxxxxxxxxxxxx> a écrit :
> Hi Paul,
>
> >  Am 20.09.2022 um 15:33 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>:
> >
> >
> >
> >  Le mar., sept. 20 2022 at 14:31:38 +0200, H. Nikolaus Schaller 
> > <hns@xxxxxxxxxxxxx> a écrit :
> > >  Hi Paul,
> > > >  Am 20.09.2022 um 11:09 schrieb Paul Cercueil 
> > > > <paul@xxxxxxxxxxxxxxx>:
> > > >  Hi Nikolaus,
> > > >  Le mar., sept. 20 2022 at 08:31:30 +0200, H. Nikolaus Schaller 
> > > > <hns@xxxxxxxxxxxxx> a écrit :
> > > > >  Hi Paul,
> > > > >  it seems as if there aren't many places left over where the 
> > > > > MACH_JZ47* configs are still in use:
> > > > >  drivers/char/hw_ramdom/Kconfig
> > > > >  drivers/clk/ingenic/Kconfig
> > > > >  drivers/gpu/drm/ingenic/Kconfig
> > > > >  drivers/pinctrl/pinctrl-ingenic.c
> > > > >  Is it possible to get rid of them and just have 
> > > > > CONFIG_MACH_INGENIC_GENERIC?
> > > > >  This might simplify my defconfig for multiple machines.
> > > >  CONFIG_MIPS_GENERIC_KERNEL=y
> > >  This breaks compilation for me, e.g.
> > >  arch/mips/mm/cache.c:203:6: error: 'cpu_has_tx39_cache' undeclared 
> > > (first use in this function)
> >
> >  v6.0-rc does not have 'cpu_has_tx39_cache' anywhere in that file, 
> > or in arch/mips/ for that matter. It was removed in v5.18.
> >
> >  And a v5.17 kernel compiles fine here with these options enabled. 
> > So it's a problem on your side, I guess.
>
> Ah, you were right.
>
> I have a patch included which was provided by 
> zhouyanjie@xxxxxxxxxxxxxx
> ("MIPS: mm: Add Ingenic XBurst SoCs specific cache driver.").
>
> It is intended to improve caching and is part of jz4780 SMP support 
> which we wanted to have.
> AFAIR it was either not posted to lkml or rejected or superseded.
>
> > > >  CONFIG_BOARD_INGENIC=y
> > >  This config option does not exist (at least in v6.0-rc). Probably 
> > > you refer to CONFIG_INGENIC_GENERIC_BOARD.
> >
> >  No, I do not, and yes, it exists.
>
> Ah, I grepped for CONFIG_BOARD_INGENIC but it exists only in one 
> Kconfig as BOARD_INGENIC.
> But what is then the difference to CONFIG_INGENIC_GENERIC_BOARD and 
> CONFIG_MACH_INGENIC_GENERIC?

CONFIG_BOARD_INGENIC=y means you are building a generic MIPS kernel 
that happens to support Ingenic boards. In this case, the kernel has a 
lot of features enabled that might not be useful on specific Ingenic 
boards, because the kernel needs to support other SoCs. This option 
selects CONFIG_MACH_INGENIC_GENERIC=y, which in turn selects 
MACH_INGENIC=y and every MACH_JZ* and MACH_X* there is.

If you instead set MACH_INGENIC_SOC=y as your "Machine Selection -> 
System Type", this means your kernel will run specifically on Ingenic 
SoCs and isn't expected to work anywhere else. By default the "Machine 
Type" will be CONFIG_INGENIC_GENERIC_BOARD=y aka. "Generic board", 
which can be used to support any board with an Ingenic SoC. This option 
also selects CONFIG_MACH_INGENIC_GENERIC=y. If you choose a different 
"machine type", for instance JZ4780_CI20=y, then only MACH_JZ4780=y 
will be selected, and all the code unrelated to the JZ4780 SoC will be 
disabled.

Cheers,
-Paul

> > >  As far as I see, this does not choose to build any device tree 
> > > blob.
> > >  I tried some patch to get the .dtb built, but the resulting kernel 
> > > does not show any activity.
> > >  If I e.g. switch back from CONFIG_INGENIC_GENERIC_BOARD=y to 
> > > CONFIG_JZ4780_CI20=y the kernel works.
> >
> >  Because in the first case you build a generic kernel, which does 
> > not embed any .dtb, and you are responsible for providing the kernel 
> > with the blob at boot time; while if you build with 
> > CONFIG_JZ4780_CI20=y it embeds the .dtb inside the kernel.
> >
> >  You can embed the .dtb into the generic kernel at compile-time too, 
> > have a look at "Kernel type -> Kernel appended dtb support." Not 
> > sure why you'd want that for a generic kernel, though.
>
> Ah, I remember. Since I usually code 99% of my time for ARM systems 
> with separate .dtb files chosen by the boot loader, I forgot that we 
> have to append the .dtb on the CI20 and Alpha400. So there is no good 
> solution for a "universal" kernel binary either.
>
> > > >  Then you can support all Ingenic-based boards alongside other 
> > > > MIPS boards.
> > >  Yes, I know, but why are the MACH_JZ47* not replaced by 
> > > CONFIG_MACH_INGENIC_GENERIC if they are almost unused or completely 
> > > removed?
> >
> >  They *are* used.
>
> Well, only in a handful of places as it looks like.
>
> > >  BTW: there are also seems to be some board specific CONFIGs in 
> > > processor specific code (e.g. CONFIG_JZ4780_CI20 in irqchip code).
> >
> >  rgrep CI20 drivers/irqchip/
> >
> >  returns nothing for me.
>
> Ah, again an inofficial patch which is part of the SMP stuff 
> ("irqchip: Ingenic: Add percpu IRQ support for X2000.").
>
> > >  So selecting a MACH is not sufficient to get these features.
> > >  All this looks a little fragile and incomplete... Maybe if I find 
> > > some time (which is unfortunately quite unlikely) I can propose 
> > > some fixes.
> >
> >  It is not "fragile and incomplete", it works as intended, and it's 
> > a feature I use often.
>
> Yes, seems as if you are right. We may have added too many useful 
> patches which did not go upstreamand get in conflict with upstream 
> features.
>
> BR and thanks for helping to better understand,
> Nikolaus
>
> >  -Paul