Re: [PATCH v2] serial: earlycon: rework common framework for earlycon declarations

[Peter Hurley <peter@hurleysoftware.com>]

On 02/12/2016 02:36 AM, Masahiro Yamada wrote:
> Commit 71f50c6d9a22 ("of: drop symbols declared by _OF_DECLARE() from
> modules") fixed the link error introduced by commit b8d20e06eaad
> ("serial: 8250_uniphier: add earlycon support").
> 
> After commit 2eaa790989e0 ("earlycon: Use common framework for
> earlycon declarations") was applied, the link error came back
> because the commit reworked OF_EARLYCON_DECLARE() to not use
> _OF_DECLARE().

What link error are you getting?


> This commit reworks OF_EARLYCON_DECLARE() again based on
> _OF_DECLARE().  My motivations are:
> 
>  - Fix the link error in a generic way (without patching
>    drivers/tty/serial/8250/8250_uniphier.c)
> 
>  - Delete struct earlycon_id.  This struct is re-inventing the wheel;
>    the fields "name", "compatible", "setup" can be implemented in
>    struct of_device_id as other frameworks do.

This patch basically reverts the common framework.
What happens when not building with OF? Does it still declare the
earlycon table?


> The difference between OF_EARLYCON_DECLARE() and other users of
> _OF_DECLARE is that OF_EARLYCON_DECLARE() needs to fill the "name"
> field of struct of_device_id.  To make it easier, a new macro
> _OF_DECLARE_WITH_NAME() has been added.  It fills the "name" field
> with the given argument.  On the other hand, _OF_DECLARE() sets it
> to a null string; this is the same behavior as before.
> 
> Fixes: 2eaa790989e0 ("earlycon: Use common framework for earlycon declarations")
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> I confirmed this patch can apply onto "next-20160212" tag of linux-next.
> 
> In order to apply it onto the "tty-next" branch of TTY subsytem,
> Greg will have to pull v4.5-rc3 in.
> 
> 
> Changes in v2:
>   Minor updates of git-log
>    - fix a typo framework/frameworks/).
>    - s/Grag/Greg/   (Sorry!)
> 
>  drivers/of/fdt.c                  |  7 ++-----
>  drivers/tty/serial/earlycon.c     | 19 +++++++++++++------
>  include/asm-generic/vmlinux.lds.h | 12 ++----------
>  include/linux/of.h                | 22 +++++++++++++++-------
>  include/linux/serial_core.h       | 23 +++++++----------------
>  5 files changed, 39 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index e2295b2..ad262e9 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -802,7 +802,7 @@ static int __init early_init_dt_scan_chosen_serial(void)
>  	int offset;
>  	const char *p, *q, *options = NULL;
>  	int l;
> -	const struct earlycon_id *match;
> +	const struct of_device_id *match;
>  	const void *fdt = initial_boot_params;
>  
>  	offset = fdt_path_offset(fdt, "/chosen");
> @@ -829,10 +829,7 @@ static int __init early_init_dt_scan_chosen_serial(void)
>  		return 0;
>  	}
>  
> -	for (match = __earlycon_table; match < __earlycon_table_end; match++) {
> -		if (!match->compatible[0])
> -			continue;
> -
> +	for (match = __earlycon_of_table; match->compatible[0]; match++) {
>  		if (fdt_node_check_compatible(fdt, offset, match->compatible))
>  			continue;
>  
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index 067783f..4888ced 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -38,6 +38,9 @@ static struct earlycon_device early_console_dev = {
>  	.con = &early_con,
>  };
>  
> +static const struct of_device_id __earlycon_of_table_sentinel
> +	__used __section(__earlycon_of_table_end);
> +
>  static void __iomem * __init earlycon_map(unsigned long paddr, size_t size)
>  {
>  	void __iomem *base;
> @@ -127,9 +130,10 @@ static int __init parse_options(struct earlycon_device *device, char *options)
>  	return 0;
>  }
>  
> -static int __init register_earlycon(char *buf, const struct earlycon_id *match)
> +static int __init register_earlycon(char *buf, const struct of_device_id *match)
>  {
>  	int err;
> +	int (*setup)(struct earlycon_device *, const char *options);
>  	struct uart_port *port = &early_console_dev.port;
>  
>  	/* On parsing error, pass the options buf to the setup function */
> @@ -142,7 +146,8 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
>  		port->membase = earlycon_map(port->mapbase, 64);
>  
>  	earlycon_init(&early_console_dev, match->name);
> -	err = match->setup(&early_console_dev, buf);
> +	setup = match->data;
> +	err = setup(&early_console_dev, buf);
>  	if (err < 0)
>  		return err;
>  	if (!early_console_dev.con->write)
> @@ -172,7 +177,7 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
>   */
>  int __init setup_earlycon(char *buf)
>  {
> -	const struct earlycon_id *match;
> +	const struct of_device_id *match;
>  
>  	if (!buf || !buf[0])
>  		return -EINVAL;
> @@ -180,7 +185,7 @@ int __init setup_earlycon(char *buf)
>  	if (early_con.flags & CON_ENABLED)
>  		return -EALREADY;
>  
> -	for (match = __earlycon_table; match < __earlycon_table_end; match++) {
> +	for (match = __earlycon_of_table; match->name[0]; match++) {
>  		size_t len = strlen(match->name);
>  
>  		if (strncmp(buf, match->name, len))
> @@ -220,12 +225,13 @@ early_param("earlycon", param_setup_earlycon);
>  
>  #ifdef CONFIG_OF_EARLY_FLATTREE
>  
> -int __init of_setup_earlycon(const struct earlycon_id *match,
> +int __init of_setup_earlycon(const struct of_device_id *match,
>  			     unsigned long node,
>  			     const char *options)
>  {
>  	int err;
>  	struct uart_port *port = &early_console_dev.port;
> +	int (*setup)(struct earlycon_device *, const char *options);
>  	const __be32 *val;
>  	bool big_endian;
>  	u64 addr;
> @@ -273,7 +279,8 @@ int __init of_setup_earlycon(const struct earlycon_id *match,
>  			sizeof(early_console_dev.options));
>  	}
>  	earlycon_init(&early_console_dev, match->name);
> -	err = match->setup(&early_console_dev, options);
> +	setup = match->data;
> +	err = setup(&early_console_dev, options);
>  	if (err < 0)
>  		return err;
>  	if (!early_console_dev.con->write)
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index e9a81a6..699f4a5 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -153,15 +153,6 @@
>  #define TRACE_SYSCALLS()
>  #endif
>  
> -#ifdef CONFIG_SERIAL_EARLYCON
> -#define EARLYCON_TABLE() STRUCT_ALIGN();			\
> -			 VMLINUX_SYMBOL(__earlycon_table) = .;	\
> -			 *(__earlycon_table)			\
> -			 VMLINUX_SYMBOL(__earlycon_table_end) = .;
> -#else
> -#define EARLYCON_TABLE()
> -#endif
> -
>  #define ___OF_TABLE(cfg, name)	_OF_TABLE_##cfg(name)
>  #define __OF_TABLE(cfg, name)	___OF_TABLE(cfg, name)
>  #define OF_TABLE(cfg, name)	__OF_TABLE(config_enabled(cfg), name)
> @@ -179,6 +170,7 @@
>  #define RESERVEDMEM_OF_TABLES()	OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
>  #define CPU_METHOD_OF_TABLES()	OF_TABLE(CONFIG_SMP, cpu_method)
>  #define CPUIDLE_METHOD_OF_TABLES() OF_TABLE(CONFIG_CPU_IDLE, cpuidle_method)
> +#define EARLYCON_OF_TABLES()	OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
>  
>  #ifdef CONFIG_ACPI
>  #define ACPI_PROBE_TABLE(name)						\
> @@ -525,7 +517,7 @@
>  	IRQCHIP_OF_MATCH_TABLE()					\
>  	ACPI_PROBE_TABLE(irqchip)					\
>  	ACPI_PROBE_TABLE(clksrc)					\
> -	EARLYCON_TABLE()
> +	EARLYCON_OF_TABLES()
>  
>  #define INIT_TEXT							\
>  	*(.init.text)							\
> diff --git a/include/linux/of.h b/include/linux/of.h
> index dc6e396..6e57245 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -930,19 +930,27 @@ static inline int of_get_available_child_count(const struct device_node *np)
>  }
>  
>  #if defined(CONFIG_OF) && !defined(MODULE)
> -#define _OF_DECLARE(table, name, compat, fn, fn_type)			\
> -	static const struct of_device_id __of_table_##name		\
> +#define __OF_DECLARE(table, name1, name2, compat, fn, fn_type)		\
> +	static const struct of_device_id __UNIQUE_ID(__of_table_##name1)\
>  		__used __section(__##table##_of_table)			\
> -		 = { .compatible = compat,				\
> +		 = { .name = name2,					\
> +		     .compatible = compat,				\
>  		     .data = (fn == (fn_type)NULL) ? fn : fn  }
>  #else
> -#define _OF_DECLARE(table, name, compat, fn, fn_type)			\
> -	static const struct of_device_id __of_table_##name		\
> +#define __OF_DECLARE(table, name1, name2, compat, fn, fn_type)		\
> +	static const struct of_device_id __UNIQUE_ID(__of_table_##name1)\
>  		__attribute__((unused))					\
> -		 = { .compatible = compat,				\
> -		     .data = (fn == (fn_type)NULL) ? fn : fn }
> +		 = { .name = name2,					\
> +		     .compatible = compat,				\
> +		     .data = (fn == (fn_type)NULL) ? fn : fn  }
>  #endif
>  
> +#define _OF_DECLARE(table, name, compat, fn, fn_type)	\
> +	__OF_DECLARE(table, name, "", compat, fn, fn_type)
> +
> +#define _OF_DECLARE_WITH_NAME(table, name, compat, fn, fn_type)	\
> +	__OF_DECLARE(table, name, __stringify(name), compat, fn, fn_type)
> +
>  typedef int (*of_init_fn_2)(struct device_node *, struct device_node *);
>  typedef void (*of_init_fn_1)(struct device_node *);
>  
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index cbfcf38..0f13160 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -24,6 +24,7 @@
>  #include <linux/compiler.h>
>  #include <linux/interrupt.h>
>  #include <linux/circ_buf.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/spinlock.h>
>  #include <linux/sched.h>
>  #include <linux/tty.h>
> @@ -340,26 +341,16 @@ struct earlycon_device {
>  	unsigned int baud;
>  };
>  
> -struct earlycon_id {
> -	char	name[16];
> -	char	compatible[128];
> -	int	(*setup)(struct earlycon_device *, const char *options);
> -} __aligned(32);
> +extern const struct of_device_id __earlycon_of_table[];
>  
> -extern const struct earlycon_id __earlycon_table[];
> -extern const struct earlycon_id __earlycon_table_end[];
> +#define OF_EARLYCON_DECLARE(name, compat, fn)				\
> +	_OF_DECLARE_WITH_NAME(earlycon, name, compat, fn, void *)
>  
> -#define OF_EARLYCON_DECLARE(_name, compat, fn)				\
> -	static const struct earlycon_id __UNIQUE_ID(__earlycon_##_name)	\
> -	     __used __section(__earlycon_table)				\
> -		= { .name = __stringify(_name),				\
> -		    .compatible = compat,				\
> -		    .setup = fn  }
> -
> -#define EARLYCON_DECLARE(_name, fn)	OF_EARLYCON_DECLARE(_name, "", fn)
> +#define EARLYCON_DECLARE(_name, fn)	\
> +	OF_EARLYCON_DECLARE(_name, "-earlycon-dummy-compat-string-", fn)
>  
>  extern int setup_earlycon(char *buf);
> -extern int of_setup_earlycon(const struct earlycon_id *match,
> +extern int of_setup_earlycon(const struct of_device_id *match,
>  			     unsigned long node,
>  			     const char *options);
>  
>