Re: [kvm-unit-tests PATCH v2 1/5] lib: arm: Use UART address from generated config.h

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Feb 04, 2019 at 09:54:03AM +0000, Alexandru Elisei wrote:
> On 2/2/19 3:48 PM, Andrew Jones wrote:
> > On Fri, Feb 01, 2019 at 11:16:37AM +0000, Alexandru Elisei wrote:
> >> Generate lib/config.h when configuring kvm-unit-tests. The file is empty
> >> for all architectures except for arm and arm64, where it is used to store
> >> the UART base address. This removes the hardcoded address from lib/arm/io.c
> >> and provides a mechanism for using different UART addresses in the future.
> >>
> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> >> ---
> >>  configure    | 17 +++++++++++++++++
> >>  Makefile     |  2 +-
> >>  lib/arm/io.c |  5 ++---
> >>  .gitignore   |  1 +
> >>  4 files changed, 21 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/configure b/configure
> >> index df8581e3a906..44708b026422 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -198,3 +198,20 @@ ENVIRON_DEFAULT=$environ_default
> >>  ERRATATXT=errata.txt
> >>  U32_LONG_FMT=$u32_long
> >>  EOF
> >> +
> >> +cat <<EOF > lib/config.h
> >> +#ifndef CONFIG_H
> >> +#define CONFIG_H 1
> >> +/*
> >> + * Generated file. DO NOT MODIFY.
> >> + *
> >> + */
> >> +EOF
> >> +if [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
> >> +cat <<EOF >> lib/config.h
> >> +
> >> +#define UART_EARLY_BASE (unsigned long)0x09000000
> > The (unsigned long) cast should be in the assignment in lib/arm/io.c,
> > not here, i.e.
> >
> >  static volatile u8 *uart0_base = (u8 *)(unsigned long)UART_EARLY_BASE;
> 
> I know you have mentioned this before, I forgot to explain why I'm casting it
> here and not in io.c. The address UART_EARLY_BASE is used in 3 places in
> lib/arm/io.c: in the example above, and when a warning is displayed if the
> uart_base from the dtb doesn't match the expected uart address. I tried casting
> the address each time and the code looked a bit awkward to me.
> 
> A had a earlier suggestion to make arm_uart_early_addr=0x09000000UL, but you
> disagreed with that because an user might want to supply it themselves with an
> 'UL' at the end, and we would end up with 0x09000000ULUL. But we assign that
> value to arm_uart_early_addr ourselves, I don't think that's an issue.
> 
> In the end, it's your choice, I'm fine with casting it to unsigned long before
> each use in lib/arm/io.c.

How about calling it CONFIG_UART_EARLY_BASE in config.h and then doing

#define UART_EARLY_BASE (uint8_t *)(unsigned long)CONFIG_UART_EARLY_BASE
in lib/arm/io.c?

Thanks,
drew

> 
> >
> >
> >> +
> >> +EOF
> >> +fi
> >> +echo "#endif" >> lib/config.h
> >> diff --git a/Makefile b/Makefile
> >> index e9f02272e156..643af05678ad 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -115,7 +115,7 @@ libfdt_clean:
> >>  	$(LIBFDT_objdir)/.*.d
> >>  
> >>  distclean: clean libfdt_clean
> >> -	$(RM) lib/asm config.mak $(TEST_DIR)-run msr.out cscope.* build-head
> >> +	$(RM) lib/asm lib/config.h config.mak $(TEST_DIR)-run msr.out cscope.* build-head
> >>  	$(RM) -r tests logs logs.old
> >>  
> >>  cscope: cscope_dirs = lib lib/libfdt lib/linux $(TEST_DIR) $(ARCH_LIBDIRS) lib/asm-generic
> >> diff --git a/lib/arm/io.c b/lib/arm/io.c
> >> index d2c1a07c19ee..0973885d19f5 100644
> >> --- a/lib/arm/io.c
> >> +++ b/lib/arm/io.c
> >> @@ -11,6 +11,7 @@
> >>  #include <libcflat.h>
> >>  #include <devicetree.h>
> >>  #include <chr-testdev.h>
> >> +#include <config.h>
> >>  #include <asm/spinlock.h>
> >>  #include <asm/io.h>
> >>  
> >> @@ -18,6 +19,7 @@
> >>  
> >>  extern void halt(int code);
> >>  
> >> +static struct spinlock uart_lock;
> >>  /*
> >>   * Use this guess for the pl011 base in order to make an attempt at
> >>   * having earlier printf support. We'll overwrite it with the real
> >> @@ -25,9 +27,6 @@ extern void halt(int code);
> >>   * the address we expect QEMU's mach-virt machine type to put in
> >>   * its generated device tree.
> >>   */
> > The QEMU references in the above comment should be removed or modified to
> > also reference kvmtool.
> >
> >> -#define UART_EARLY_BASE 0x09000000UL
> >> -
> >> -static struct spinlock uart_lock;
> >>  static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;
> > As mentioned above, we should put the (unsigned long) cast here.
> >
> >>  
> >>  static void uart0_init(void)
> >> diff --git a/.gitignore b/.gitignore
> >> index 2405a8087ae5..483f7c7a09ea 100644
> >> --- a/.gitignore
> >> +++ b/.gitignore
> >> @@ -10,6 +10,7 @@ patches
> >>  cscope.*
> >>  *.swp
> >>  /lib/asm
> >> +/lib/config.h
> >>  /config.mak
> >>  /*-run
> >>  /msr.out
> >> -- 
> >> 2.17.0
> >>
> > Thanks,
> > drew 



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux