Re: [Xen-devel] [XEN PATCH v2 1/2] xen/arm: Rename all early printk macro

From: Anthony PERARD <anthony.perard@citrix.com>
To: Julien Grall <julien@xen.org>
Cc: xen-devel@lists.xenproject.org,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [Xen-devel] [XEN PATCH v2 1/2] xen/arm: Rename all early printk macro
Date: Mon, 9 Mar 2020 12:06:40 +0000	[thread overview]
Message-ID: <20200309120640.GC2152@perard.uk.xensource.com> (raw)
In-Reply-To: <cfc8dcc2-6b3a-e175-4189-097e526e39b4@xen.org>

On Sun, Mar 08, 2020 at 05:57:32PM +0000, Julien Grall wrote:
> On 06/03/2020 17:42, Anthony PERARD wrote:
> > We also reuse CONFIG_EARLY_PRINTK provided by users to enable or not
> > early printk, thus we need to override the value in makefile.
[...]
> >   ifneq ($(EARLY_PRINTK_INC),)
> > -EARLY_PRINTK := y
> > +override CONFIG_EARLY_PRINTK := y
> 
> I am not sure to understand why you are using the directive override here.
> Why can't you just prefix the variable with CONFIG_?

override is needed if someone run make like this:
    make CONFIG_EARLY_PRINTK=sun7i
otherwise the value can't be changed.
But that dosn't work when run like this:
    export CONFIG_EARLY_PRINTK=sun7i
    make

So I'm going to have to rename the variable in the second patch.

> >   endif
> > -CFLAGS-$(EARLY_PRINTK) += -DCONFIG_EARLY_PRINTK
> > -CFLAGS-$(EARLY_PRINTK_INIT_UART) += -DEARLY_PRINTK_INIT_UART
> > -CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK_INC=\"debug-$(EARLY_PRINTK_INC).inc\"
> > -CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK_BAUD=$(EARLY_PRINTK_BAUD)
> > -CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_BASE_ADDRESS=$(EARLY_UART_BASE_ADDRESS)
> > -CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_REG_SHIFT=$(EARLY_UART_REG_SHIFT)
> > +CFLAGS-$(CONFIG_EARLY_PRINTK) += -DCONFIG_EARLY_PRINTK
> > +CFLAGS-$(EARLY_PRINTK_INIT_UART) += -DCONFIG_EARLY_UART_INIT
> > +CFLAGS-$(CONFIG_EARLY_PRINTK) += -DCONFIG_EARLY_PRINTK_INC=\"debug-$(EARLY_PRINTK_INC).inc\"
> > +CFLAGS-$(CONFIG_EARLY_PRINTK) += -DCONFIG_EARLY_UART_BAUD_RATE=$(EARLY_PRINTK_BAUD)
> 
> The baud rate is only used by the PL011 in rare cases. So I would add PL011
> in the name.

Sound fine, I'll rename it.

> > +CFLAGS-$(CONFIG_EARLY_PRINTK) += -DCONFIG_EARLY_UART_BASE_ADDRESS=$(EARLY_UART_BASE_ADDRESS)
> > +CFLAGS-$(CONFIG_EARLY_PRINTK) += -DCONFIG_EARLY_UART_8250_REG_SHIFT=$(EARLY_UART_REG_SHIFT)
> 
> The name clearly suggests that this should be protected with an "if 8250".
> Maybe in the similar way as for the scif specific variables. But... I am not
> going to push for it as the next patch is going to remove it.

Yep, some macro gets defined without a value.  :-)
But that gets fix in the next patch.

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel