Re: [PATCH 1/4] xen: arm64: Add Basic Platform support for APM X-Gene Storm.

[Julien Grall <julien.grall@linaro.org>]

On 09/20/2013 10:52 AM, Pranavkumar Sawargaonkar wrote:
> This patch adds following things:
> Early printk support for APM X-Gene platform.

Does the UART is compatible with 8250?

If so, can you rename the your debug-xgene-storm.inc in debug-8250.inc?
Future ARM64 server with 8250-compatible UART will be able to use the
same code.

Also, can you divide this patch in 2 part:
        - Early printk support;
	- Initial platform support.

> Initial platform stubs for APM X-Gene.
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> ---
>  xen/arch/arm/Rules.mk                    |  5 +++
>  xen/arch/arm/arm64/debug-xgene-storm.inc | 55 ++++++++++++++++++++++++++++++
>  xen/arch/arm/platforms/Makefile          |  1 +
>  xen/arch/arm/platforms/xgene-storm.c     | 57 ++++++++++++++++++++++++++++++++
>  4 files changed, 118 insertions(+)
>  create mode 100644 xen/arch/arm/arm64/debug-xgene-storm.inc
>  create mode 100644 xen/arch/arm/platforms/xgene-storm.c
> 
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index bd79b26..b36120e 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -77,6 +77,11 @@ EARLY_PRINTK_INC := 8250
>  EARLY_UART_BASE_ADDRESS := 0x01c28000
>  EARLY_UART_REG_SHIFT := 2
>  endif
> +ifeq ($(CONFIG_EARLY_PRINTK), xgene-storm)
> +EARLY_PRINTK_INC := xgene-storm
> +EARLY_PRINTK_BAUD := 115200
> +EARLY_UART_BASE_ADDRESS := 0x1c020000
> +endif
>  
>  ifneq ($(EARLY_PRINTK_INC),)
>  EARLY_PRINTK := y
> diff --git a/xen/arch/arm/arm64/debug-xgene-storm.inc b/xen/arch/arm/arm64/debug-xgene-storm.inc
> new file mode 100644
> index 0000000..ebbb8d2
> --- /dev/null
> +++ b/xen/arch/arm/arm64/debug-xgene-storm.inc
> @@ -0,0 +1,55 @@
> +/*
> + * xen/arch/arm/arm64/debug-xgene-storm.inc
> + *
> + * X-Gene Storm specific debug code
> + *
> + * Copyright (c) 2013 Applied Micro.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <xen/8250-uart.h>
> +
> +#define XGENE_STORM_UART_LSR    0x14
> +#define XGENE_STORM_UART_THR    0x00

I guess these defines contains the shift (=4), right?

To be more generic, you should allow a different shift via
EARLY_UART_REG_SHIFT (see arm32/debug-8250.inc) and use this define.

> +
> +/* UART initialization
> + * rb: register which contains the UART base address
> + * rc: scratch register 1
> + * rd: scratch register 2 */
> +.macro early_uart_init rb rc rd
> +.endm

early_uart_init is only needed if EARLY_PRINTK_INIT_UART is defined.
In your case it's not defined, so you can remove this macro.


> +
> +/* UART wait UART to be ready to transmit
> + * xb: register which contains the UART base address
> + * c: scratch register */
> +.macro early_uart_ready xb c
> +1:
> +       ldrb  w\c, [\xb, #XGENE_STORM_UART_LSR]
> +       and w\c, w\c, #UART_LSR_THRE
> +       cmp w\c, #UART_LSR_THRE
> +       b.ne 1b
> +.endm
> +
> +/* UART transmit character
> + * xb: register which contains the UART base address
> + * wt: register which contains the character to transmit */
> +.macro early_uart_transmit xb wt
> +        /* UART_THR  transmit holding */
> +        strb   \wt, [\xb, #XGENE_STORM_UART_THR]
> +.endm
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index 4aa82e8..8a6709f 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -2,3 +2,4 @@ obj-y += vexpress.o
>  obj-y += exynos5.o
>  obj-y += midway.o
>  obj-y += omap5.o
> +obj-y += xgene-storm.o

Shouldn't it be only enabled on ARM64?

> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> new file mode 100644
> index 0000000..8e2b3b6
> --- /dev/null
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -0,0 +1,57 @@
> +/*
> + * xen/arch/arm/platforms/xgene-storm.c
> + *
> + * Applied Micro's X-Gene specific settings
> + *
> + * Pranavkumar Sawargaonkar <psawargaonkar@apm.com>
> + * Anup Patel <apatel@apm.com>
> + * Copyright (c) 2013 Applied Micro.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <xen/config.h>
> +#include <xen/device_tree.h>
> +#include <xen/domain_page.h>
> +#include <xen/mm.h>
> +#include <xen/vmap.h>
> +#include <asm/platform.h>
> +#include <asm/early_printk.h>
> +
> +static void xgene_storm_reset(void)
> +{
> +}
> +
> +static int xgene_storm_init(void)
> +{
> +        return 0;
> +}

You don't need these empty functions, the platform code copes with empty
callback.

> +static const char const *xgene_storm_dt_compat[] __initdata =
> +{
> +    "apm,xgene-storm",
> +    NULL
> +};
> +
> +PLATFORM_START(xgene_storm, "APM X-GENE STORM")
> +    .compatible = xgene_storm_dt_compat,
> +    .init = xgene_storm_init,
> +    .reset = xgene_storm_reset,
> +PLATFORM_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> 


-- 
Julien Grall