Re: [PATCH 0/7] microblaze: fix various problems in building boot images

[Masahiro Yamada]

On Wed, Dec 12, 2018 at 11:11 PM Masahiro Yamada
<yamada.masahiro@xxxxxxxxxxxxx> wrote:
>
> On Wed, Dec 12, 2018 at 10:50 PM Michal Simek <michal.simek@xxxxxxxxxx> wrote:
> >
> > On 08. 12. 18 7:14, Masahiro Yamada wrote:
> > > On Sat, Dec 8, 2018 at 12:20 AM Michal Simek <michal.simek@xxxxxxxxxx> wrote:
> > >>
> > >> On 07. 12. 18 14:29, Michal Simek wrote:
> > >>> On 07. 12. 18 12:29, Masahiro Yamada wrote:
> > >>>> On Thu, Dec 6, 2018 at 11:55 PM Michal Simek <monstr@xxxxxxxxx> wrote:
> > >>>>>
> > >>>>> On 03. 12. 18 8:50, Masahiro Yamada wrote:
> > >>>>>> This patch set fixes various issues in microblaze Makefiles.
> > >>>>>>
> > >>>>>> BTW, "simpleImage.<dt>" works like a phony target to generate the
> > >>>>>> following four images, where the first three are just aliases.
> > >>>>>>
> > >>>>>>  - arch/microblaze/boot/simpleImage.<dt>:
> > >>>>>>          identical to arch/microblaze/boot/linux.bin
> > >>>>>>
> > >>>>>>  - arch/microblaze/boot/simpleImage.<dt>.unstrip:
> > >>>>>>          identical to vmlinux
> > >>>>>>
> > >>>>>>  - arch/microblaze/boot/simpleImage.<dt>.ub:
> > >>>>>>          identical to arch/microblaze/boot/linux.bin.ub
> > >>>>>>
> > >>>>>>  - arch/microblaze/boot/simpleImage.<dt>.strip:
> > >>>>>>          stripped vmlinux
> > >>>>>>
> > >>>>>> I am not sure how much useful those copies are,
> > >>>>>> but, I tried my best to keep the same behavior.
> > >>>>>>
> > >>>>>> IMHO, I guess DTB=<dt> would be more sensible,
> > >>>>>> but it is up to Michal.
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>> Masahiro Yamada (7):
> > >>>>>>   microblaze: fix cleaning of boot images
> > >>>>>>   microblaze: adjust the help to the real behavior
> > >>>>>>   microblaze: move "... is ready" message to arch/microblaze/Makefile
> > >>>>>>   microblaze: fix multiple bugs in arch/microblaze/boot/Makefile
> > >>>>>>   microblaze: add linux.bin* and simpleImage.* to PHONY
> > >>>>>>   microblaze: fix race condition in building boot images
> > >>>>>>   microblaze: remove the unneeded code just in case file copy fails
> > >>>>>>
> > >>>>>>  arch/microblaze/Makefile          | 14 +++++++++-----
> > >>>>>>  arch/microblaze/boot/Makefile     | 33 +++++++++++++++++----------------
> > >>>>>>  arch/microblaze/boot/dts/Makefile |  5 +----
> > >>>>>>  3 files changed, 27 insertions(+), 25 deletions(-)
> > >>>>>>
> > >>>>>
> > >>>>> One more thing I have in my mind for a while is that will be good to
> > >>>>> configure kernel build flags from DT and completely get rid of these
> > >>>>> symbols.
> > >>>>>
> > >>>>> XILINX_MICROBLAZE0_USE_MSR_INSTR
> > >>>>> XILINX_MICROBLAZE0_USE_PCMP_INSTR
> > >>>>> XILINX_MICROBLAZE0_USE_BARREL
> > >>>>> XILINX_MICROBLAZE0_USE_DIV
> > >>>>> XILINX_MICROBLAZE0_USE_HW_MUL
> > >>>>> XILINX_MICROBLAZE0_USE_FPU
> > >>>>>
> > >>>>> It means setup CPUFLAGS based on extracting that values from DT that it
> > >>>>> all the time match the hardware.
> > >>>>> It will also simplify all the CPUFLAGS logic which is in
> > >>>>> arch/microblaze/Makefile.
> > >>>>>
> > >>>>> Do you have any idea how this can be done?
> > >>>>
> > >>>>
> > >>>> I have no idea.
> > >>>>
> > >>>> Parsing DTS with sed or something would be possible, but ugly.
> > >>>>
> > >>>> In my opinion, the kernel should be multi platform image,
> > >>>> in other words, it is the least common denominator
> > >>>> of supported platforms.
> > >>>>
> > >>>> So, the kernel should enable all features that may or may not be used
> > >>>> depending on platform.
> > >>>>
> > >>>> I do not know if this works for MB.
> > >>>
> > >>> Microblaze is soft core CPU where you can select if you want to have it
> > >>> with multiplier, divider, barrel shifter, etc.
> > >>> You can of course say that you don't have them and you have "universal"
> > >>> kernel but very slow.
> > >>> That's why user has to setup these configs which are converted to cflags
> > >>> to say GCC what can be used.
> > >>> And these configs can be simply parsed from dt.
> > >>>
> > >>> For example like this based on dtb (quick hack)
> > >>>
> > >>> for i in `echo use-msr-instr use-pcmp-instr use-barrel use-div
> > >>> use-hw-mul use-fpu`; do
> > >>>       UPPER=`echo $i | tr '-' '_' | tr '[a-z]' '[A-Z}'`
> > >>>       echo $i $UPPER;
> > >>>
> > >>>       VAR=`fdtget -t i $FILE/arch/microblaze/boot/dts/system.dtb /cpus/cpu@0/
> > >>> xlnx,$i`
> > >>>       FLAGS+="CONFIG_XILINX_MICROBLAZE0_${UPPER}=${VAR} "
> > >>> done
> > >>>
> > >>> make $FLAGS
> > >>>
> > >>>
> > >>> When simpleImage is requested dt could be parsed to setup proper build
> > >>> flags.
> > >>
> > >> One more thing I found is that I have done these changes
> > >>
> > >> diff --git a/arch/microblaze/kernel/setup.c b/arch/microblaze/kernel/setup.c
> > >> index bbd6968ce55b..dc6a6fee3ae2 100644
> > >> --- a/arch/microblaze/kernel/setup.c
> > >> +++ b/arch/microblaze/kernel/setup.c
> > >> @@ -153,11 +153,13 @@ void __init machine_early_init(const char
> > >> *cmdline, unsigned int ram,
> > >>  #endif
> > >>
> > >>  #if CONFIG_XILINX_MICROBLAZE0_USE_MSR_INSTR
> > >> +#error MSR is enabled
> > >>         if (msr) {
> > >>                 pr_info("!!!Your kernel has setup MSR instruction but ");
> > >>                 pr_cont("CPU don't have it %x\n", msr);
> > >>         }
> > >>  #else
> > >> +#error MSR is not enabled
> > >>         if (!msr) {
> > >>                 pr_info("!!!Your kernel not setup MSR instruction but ");
> > >>                 pr_cont("CPU have it %x\n", msr);
> > >>
> > >> and run MSR with 1
> > >> make defconfig && make CONFIG_XILINX_MICROBLAZE0_USE_MSR_INSTR=1
> > >> arch/microblaze/kernel/setup.o
> > >> it errors #error MSR is enabled
> > >> and all is good.
> > >>
> > >> And when I run MSR with 0
> > >> make defconfig && make CONFIG_XILINX_MICROBLAZE0_USE_MSR_INSTR=0
> > >> arch/microblaze/kernel/setup.o
> > >> also getting #error MSR is enabled
> > >> which is wrong.
> > >>
> > >> Is there any rule what can be setup at compilation time?
> > >
> > > You are trying to do very specific things,
> > > I do not have a clean solution.
> > >
> > > I do not mind whatever you do in arch-Makefile.
> >
> > ok.
> >
> > >
> > > If you look at stack_protector_prepare in arch/powerpc/Makefile,
> > > it is parsing a file with awk to setup compiler flags.
> >
> > Ok. Will do more experiments with it.
> >
> > Can you please at least confirm me that config options passed via
> > command line as above should be used instead of that one in .config?
> > (Just want to understand why USE_MSR is so special that it is not
> > working properly).
>
>
>
>
> make defconfig && make CONFIG_XILINX_MICROBLAZE0_USE_MSR_INSTR=1
>
> does not work.
>
> It overrides only references in Makefiles.
>
>
>
> C files still refer to include/generated/autoconf.h
> which is created based on Kconfig.
>
>
> KBUILD_CPPFLAGS +=
> -DCONFIG_XILINX_MICROBLAZE0_USE_MSR_INSTR=$(CONFIG_XILINX_MICROBLAZE0_USE_MSR_INSTR)
>
> might work although it is ugly.
>
> (At least, CONFIG_ prefix should be ripped off)



Perhaps, another idea might be merge_config.


arch/mips/Makefile manipulates .config
by using scripts/kconfig/merge_config.sh


You can enable/disable CONFIG_XILINX_MICROBLAZE0_USE*
based on the information extracted from DT.




-- 
Best Regards
Masahiro Yamada