[PATCH 00/27] Console code consolidation

[PATCH 00/27] Console code consolidation

[Andrey Smirnov]

Everyone:

While debugging the reason behind print_hex_dump() not producing
carriage return properly, when used in PBL, I realised that current
codebase contained:

 - at least 5 places where '\n' was replaced with '\n\r'
 - at least 3 almost identical implementations of puts()
 - at least 3 almost identical implementations of printf()

so this patcheset is an attempt to consolidate, share and simplify
console related code.

Feedback is welcome!

Thanks,
Andrey Smirnov

Andrey Smirnov (27):
  pbl: console: Introduce putc_func_t
  console: Unify console_simple.c and pbl/console.c
  pbl: console: Move '\n' handling into console_putc()
  console: Reconcile 3 different puts() implementations
  ratp: Add dependency on CONSOLE_FULL
  netconsole: Add dependency on CONSOLE_FULL
  input: Add dependency on CONSOLE_FULL
  console: Make use of __console_putc()
  console: Fix console_get_first_active()
  console: Simplify early console code
  console: Consolidate all implemenatations of ctrlc()
  console: Drop ARCH_HAS_CTRLC
  console: Consolidate DEBUG_LL and CONSOLE_* '\n' -> '\n\r' code
  console: Consolidate DEBUG_LL and CONSOLE_* puts() implementations
  console_simple: Use console_flush() from CONSOLE_FULL
  console_simple: Use tstc_raw() as tstc()
  console_simple: Use getc_raw() as getchar()
  console_simple: Get rid of global console pointer
  console_simple: Make use of list_add_tail()
  console: Share definition for printf with PBL
  pbl: console: Convert pr_print into a single line #define
  console: Convert pr_print into a single line #define
  console: Remove dputc()
  console: Simplify dputs()
  console: Introduce dvprintf()
  console: Convert printf() into a single line #define
  psci: console: Convert to use lib/console.c

 arch/arm/cpu/psci.c               |  34 +---
 arch/arm/include/asm/psci.h       |  11 --
 arch/sandbox/include/asm/common.h |   2 -
 commands/echo.c                   |   4 +-
 common/console.c                  | 142 ++++-----------
 common/console_common.c           | 168 ++++++++----------
 common/console_simple.c           |  75 +-------
 drivers/input/Kconfig             |   2 +-
 include/console.h                 |  18 +-
 include/debug_ll.h                |  61 +++++--
 include/printk.h                  |   8 +-
 include/stdio.h                   |  11 +-
 lib/Kconfig                       |   1 +
 lib/Makefile                      |   6 +-
 lib/console.c                     | 276 ++++++++++++++++++++++++++++++
 net/Kconfig                       |   2 +-
 pbl/console.c                     |  70 +-------
 17 files changed, 478 insertions(+), 413 deletions(-)
 create mode 100644 lib/console.c

-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 01/27] pbl: console: Introduce putc_func_t

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/cpu/psci.c | 4 ++--
 include/console.h   | 6 ++++--
 pbl/console.c       | 4 ++--
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/arm/cpu/psci.c b/arch/arm/cpu/psci.c
index 0a7e48f8f..ad132c0a3 100644
--- a/arch/arm/cpu/psci.c
+++ b/arch/arm/cpu/psci.c
@@ -33,10 +33,10 @@
  * warned.
  */
 
-static void (*__putc)(void *ctx, int c);
+static putc_func_t __putc;
 static void *putc_ctx;
 
-void psci_set_putc(void (*putcf)(void *ctx, int c), void *ctx)
+void psci_set_putc(putc_func_t putcf, void *ctx)
 {
         __putc = putcf;
         putc_ctx = ctx;
diff --git a/include/console.h b/include/console.h
index 673921331..c671be859 100644
--- a/include/console.h
+++ b/include/console.h
@@ -199,10 +199,12 @@ static inline int console_drain(struct console_device *cdev,
 	return __console_drain(is_timeout, cdev, fifo, buf, len, timeout);
 }
 
+typedef void (*putc_func_t) (void *ctx, int c);
+
 #ifdef CONFIG_PBL_CONSOLE
-void pbl_set_putc(void (*putcf)(void *ctx, int c), void *ctx);
+void pbl_set_putc(putc_func_t putcf, void *ctx);
 #else
-static inline void pbl_set_putc(void (*putcf)(void *ctx, int c), void *ctx) {}
+static inline void pbl_set_putc(putc_func_t putcf, void *ctx) {}
 #endif
 
 bool console_allow_color(void);
diff --git a/pbl/console.c b/pbl/console.c
index 007e4e4b8..75576ec79 100644
--- a/pbl/console.c
+++ b/pbl/console.c
@@ -6,7 +6,7 @@
  * Put these in the data section so that they survive the clearing of the
  * BSS segment.
  */
-static __attribute__ ((section(".data"))) void (*__putc)(void *ctx, int c);
+static __attribute__ ((section(".data"))) putc_func_t __putc;
 static __attribute__ ((section(".data"))) void *putc_ctx;
 
 /**
@@ -17,7 +17,7 @@ static __attribute__ ((section(".data"))) void *putc_ctx;
  * This sets the putc function which is afterwards used to output
  * characters in the PBL.
  */
-void pbl_set_putc(void (*putcf)(void *ctx, int c), void *ctx)
+void pbl_set_putc(putc_func_t putcf, void *ctx)
 {
 	__putc = putcf;
 	putc_ctx = ctx;
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 02/27] console: Unify console_simple.c and pbl/console.c

[Andrey Smirnov]

Move all of the shared code into lib/console.c and convert both files
to use that.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console_simple.c |  11 +---
 include/console.h       |   8 +++
 lib/Makefile            |   6 +-
 lib/console.c           | 130 ++++++++++++++++++++++++++++++++++++++++
 pbl/console.c           |  15 +----
 5 files changed, 148 insertions(+), 22 deletions(-)
 create mode 100644 lib/console.c

diff --git a/common/console_simple.c b/common/console_simple.c
index 9675cbb0a..62b811b3f 100644
--- a/common/console_simple.c
+++ b/common/console_simple.c
@@ -7,7 +7,7 @@
 
 LIST_HEAD(console_list);
 EXPORT_SYMBOL(console_list);
-static struct console_device *console;
+extern struct console_device *console;
 
 int console_puts(unsigned int ch, const char *str)
 {
@@ -28,14 +28,7 @@ EXPORT_SYMBOL(console_puts);
 
 void console_putc(unsigned int ch, char c)
 {
-	if (!console) {
-		putc_ll(c);
-		return;
-	}
-
-	console->putc(console, c);
-	if (c == '\n')
-		console->putc(console, '\r');
+	__console_putc(__console_get_default(), c);
 }
 EXPORT_SYMBOL(console_putc);
 
diff --git a/include/console.h b/include/console.h
index c671be859..b3001ad16 100644
--- a/include/console.h
+++ b/include/console.h
@@ -69,6 +69,7 @@ struct console_device {
 	struct cdev_operations fops;
 
 	struct serdev_device serdev;
+	void *ctx;
 };
 
 static inline struct serdev_device *to_serdev_device(struct device_d *d)
@@ -209,4 +210,11 @@ static inline void pbl_set_putc(putc_func_t putcf, void *ctx) {}
 
 bool console_allow_color(void);
 
+void __cdev_putc(struct console_device *cdev, char c);
+void __console_putc(struct console_device *cdev, char c);
+
+struct console_device *__console_get_default(void);
+void __console_set_putc(struct console_device *cdev,
+			putc_func_t putcf, void *ctx);
+
 #endif
diff --git a/lib/Makefile b/lib/Makefile
index 09c250a1c..44188dc5a 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -65,4 +65,8 @@ obj-y			+= int_sqrt.o
 obj-y			+= parseopt.o
 obj-y			+= clz_ctz.o
 obj-$(CONFIG_CRC_CCITT) += crc-ccitt.o
-obj-pbl-y		+= clock.o
\ No newline at end of file
+obj-pbl-y		+= clock.o
+pbl-$(CONFIG_PBL_CONSOLE)	+= console.o
+obj-$(CONFIG_CONSOLE_FULL)	+= console.o
+obj-$(CONFIG_CONSOLE_SIMPLE)	+= console.o
+
diff --git a/lib/console.c b/lib/console.c
new file mode 100644
index 000000000..d2d2ae775
--- /dev/null
+++ b/lib/console.c
@@ -0,0 +1,130 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/*
+ * File containing all of the console-related code that is used by all
+ * possible consumers: PBL, CONSOLE_FULL, CONSOLE_SIMPLE, etc.
+ *
+ * Copyright (C) 2018 Zodiac Inflight Innovations
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ */
+
+#include <debug_ll.h>
+#include <console.h>
+
+/*
+ * Put this in the data section so that it survives the clearing of
+ * the BSS segment.
+ */
+#define __data __attribute__ ((section(".data")))
+
+/**
+ * console_ll - most basic low-level console
+ */
+__data struct console_device console_ll = {
+	.putc = NULL,
+	.ctx  = NULL,
+};
+__data struct console_device *console;
+
+/**
+ * __console_ll_putc - Early, most primitive putc() implemenatation
+ *
+ * Internal function used as a .putc() callback in console_ll when
+ * nothing better is configured.
+ */
+static void __console_ll_putc(struct console_device *cdev, char c)
+{
+	putc_ll(c);
+}
+
+/**
+ * __console_get_default - Return default output console
+ *
+ * Internal function used to determine which console to use for early
+ * output. It has the following two use-cases:
+ *
+ *   1. PBL, where it falls back onto console_ll and whatever it is
+ *      set up to (either putc_ll or custome callback set with
+ *      pbl_set_putc())
+ *
+ *   2. CONSOLE_SIMPLE, where it falls back onto console_ll (which in
+ *      this case always boils down to a putc_ll() call) until first
+ *      (and only) console is registered via console_register().
+ */
+struct console_device *__console_get_default(void)
+{
+	/*
+	 * Doing on-demand initialization here as opposed to in the
+	 * definition of console_ll above has that advantage that on
+	 * some architecutres (e.g. AArch64) it allows us to avoid
+	 * additional relocation and makes it possible to use console
+	 * infrastructure before any relocation happens
+	 */
+	if (unlikely(!console_ll.putc))
+		console_ll.putc = __console_ll_putc;
+
+	if (IS_ENABLED(__PBL__) || !console)
+		return &console_ll;
+
+	return console;
+}
+
+/**
+ * __console_set_putc - Early console initalization helper
+ *
+ * @cdev:	Console device to initialize
+ * @putcf:	putc() implementation to be used for this console
+ * @ctx:	Context to pass to putc()
+ *
+ * Internal function used to initialize early/trivial console devices
+ * capable of only outputting a single character
+ */
+void __console_set_putc(struct console_device *cdev,
+			putc_func_t putcf, void *ctx)
+{
+	cdev->putc = (void *)putcf;
+	cdev->ctx = ctx;
+}
+
+/**
+ * __cdev_putc - Internal .putc() callback dispatcher
+ *
+ * @cdev:	Console device to use
+ * @c:		Character to print
+ *
+ * Internal .putc() callback dispatcher needed to correctly select
+ * which context to pass.
+ *
+ * This is needed becuase when being used in PBL in conjunction with
+ * pbl_set_putc(), .putc() callback is expecting to receive a void *
+ * context that was registered earlier.
+ *
+ * In the "normal" world, however all of the .putc() callback are
+ * written with expectation of receiving struct console_device * as a
+ * first argument.
+ *
+ * Accomodation of both of those use-cases is the purpoese of this
+ * function
+ */
+void __cdev_putc(struct console_device *cdev, char c)
+{
+	void *ctx = cdev->ctx ? : cdev;
+
+	cdev->putc(ctx, c);
+}
+
+/**
+ * __console_putc - Internal high-level putc() implementation
+ *
+ * @cdev:	Console device to use
+ * @c:		Character to print
+ *
+ * Internal high-level putc() implementation based on __cdev_putc()
+ * that performs correct '\n' -> '\n\r' substitution.
+ */
+void __console_putc(struct console_device *cdev, char c)
+{
+	__cdev_putc(cdev, c);
+	if (c == '\n')
+		__cdev_putc(cdev, '\r');
+}
diff --git a/pbl/console.c b/pbl/console.c
index 75576ec79..c1c3e1dde 100644
--- a/pbl/console.c
+++ b/pbl/console.c
@@ -2,12 +2,7 @@
 #include <debug_ll.h>
 #include <linux/err.h>
 
-/*
- * Put these in the data section so that they survive the clearing of the
- * BSS segment.
- */
-static __attribute__ ((section(".data"))) putc_func_t __putc;
-static __attribute__ ((section(".data"))) void *putc_ctx;
+extern struct console_device console_ll;
 
 /**
  * pbl_set_putc() - setup UART used for PBL console
@@ -19,16 +14,12 @@ static __attribute__ ((section(".data"))) void *putc_ctx;
  */
 void pbl_set_putc(putc_func_t putcf, void *ctx)
 {
-	__putc = putcf;
-	putc_ctx = ctx;
+	__console_set_putc(&console_ll, putcf, ctx);
 }
 
 void console_putc(unsigned int ch, char c)
 {
-	if (__putc)
-		__putc(putc_ctx, c);
-	else
-		putc_ll(c);
+	__cdev_putc(__console_get_default(), c);
 }
 
 int console_puts(unsigned int ch, const char *str)
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 02/27] console: Unify console_simple.c and pbl/console.c

[Sam Ravnborg]

Hi Andrey.

On Thu, Jun 14, 2018 at 09:11:10PM -0700, Andrey Smirnov wrote:
> Move all of the shared code into lib/console.c and convert both files
> to use that.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> +++ b/lib/console.c
> @@ -0,0 +1,130 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/*
> + * File containing all of the console-related code that is used by all
> + * possible consumers: PBL, CONSOLE_FULL, CONSOLE_SIMPLE, etc.
> + *
> + * Copyright (C) 2018 Zodiac Inflight Innovations
> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
> + */
> +
> +#include <debug_ll.h>
> +#include <console.h>
> +
> +/*
> + * Put this in the data section so that it survives the clearing of
> + * the BSS segment.
> + */
> +#define __data __attribute__ ((section(".data")))
This definition would be better placed in compiler.h
together with all the __naked etc. definitions.

And then it could be used in other places,
like for example first patch in this serie.

	Sam

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 02/27] console: Unify console_simple.c and pbl/console.c

[Andrey Smirnov]

On Thu, Jun 14, 2018 at 9:49 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Andrey.
>
> On Thu, Jun 14, 2018 at 09:11:10PM -0700, Andrey Smirnov wrote:
> > Move all of the shared code into lib/console.c and convert both files
> > to use that.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> > +++ b/lib/console.c
> > @@ -0,0 +1,130 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +/*
> > + * File containing all of the console-related code that is used by all
> > + * possible consumers: PBL, CONSOLE_FULL, CONSOLE_SIMPLE, etc.
> > + *
> > + * Copyright (C) 2018 Zodiac Inflight Innovations
> > + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
> > + */
> > +
> > +#include <debug_ll.h>
> > +#include <console.h>
> > +
> > +/*
> > + * Put this in the data section so that it survives the clearing of
> > + * the BSS segment.
> > + */
> > +#define __data __attribute__ ((section(".data")))
> This definition would be better placed in compiler.h
> together with all the __naked etc. definitions.
>

I thought about it, but AFAIK, there's only a single user of this in
the whole codebase and __data, unlike, say, __naked, seems like a
generic enough name to cause name collisions, so I opted at keeping it
in .c file.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 02/27] console: Unify console_simple.c and pbl/console.c

[Sam Ravnborg]

Hi Andrey.

> +
> +/**
> + * __console_get_default - Return default output console
> + *
> + * Internal function used to determine which console to use for early
> + * output. It has the following two use-cases:
> + *
> + *   1. PBL, where it falls back onto console_ll and whatever it is
> + *      set up to (either putc_ll or custome callback set with
> + *      pbl_set_putc())
> + *
> + *   2. CONSOLE_SIMPLE, where it falls back onto console_ll (which in
> + *      this case always boils down to a putc_ll() call) until first
> + *      (and only) console is registered via console_register().
> + */
> +struct console_device *__console_get_default(void)

If it is used as internal only then it should be static.
(Have not read the the remaining patches)

> +{
> +	/*
> +	 * Doing on-demand initialization here as opposed to in the
> +	 * definition of console_ll above has that advantage that on
> +	 * some architecutres (e.g. AArch64) it allows us to avoid
> +	 * additional relocation and makes it possible to use console
> +	 * infrastructure before any relocation happens
> +	 */
> +	if (unlikely(!console_ll.putc))
> +		console_ll.putc = __console_ll_putc;
> +
> +	if (IS_ENABLED(__PBL__) || !console)
> +		return &console_ll;
> +
> +	return console;
> +}
> +
> +/**
> + * __console_set_putc - Early console initalization helper
> + *
> + * @cdev:	Console device to initialize
> + * @putcf:	putc() implementation to be used for this console
> + * @ctx:	Context to pass to putc()
> + *
> + * Internal function used to initialize early/trivial console devices
> + * capable of only outputting a single character
> + */
> +void __console_set_putc(struct console_device *cdev,
> +			putc_func_t putcf, void *ctx)
Likewise, and so on for the reimaining __xxx functions.


> +{
> +	cdev->putc = (void *)putcf;
> +	cdev->ctx = ctx;
> +}
> +
> +/**
> + * __cdev_putc - Internal .putc() callback dispatcher
> + *
> + * @cdev:	Console device to use
> + * @c:		Character to print
> + *
> + * Internal .putc() callback dispatcher needed to correctly select
> + * which context to pass.
> + *
> + * This is needed becuase when being used in PBL in conjunction with
> + * pbl_set_putc(), .putc() callback is expecting to receive a void *
> + * context that was registered earlier.
> + *
> + * In the "normal" world, however all of the .putc() callback are
> + * written with expectation of receiving struct console_device * as a
> + * first argument.
> + *
> + * Accomodation of both of those use-cases is the purpoese of this
> + * function
> + */
> +void __cdev_putc(struct console_device *cdev, char c)
> +{
> +	void *ctx = cdev->ctx ? : cdev;
Looks strange to me.
What is ctx assigned in the "true" case?
Maybe there is some subtle C rule that I have missed/forgot.
Point is, if this is valid code it may also make other puzzeled.

> +
> +	cdev->putc(ctx, c);
> +}
> +
> +/**
> + * __console_putc - Internal high-level putc() implementation
> + *
> + * @cdev:	Console device to use
> + * @c:		Character to print
> + *
> + * Internal high-level putc() implementation based on __cdev_putc()
> + * that performs correct '\n' -> '\n\r' substitution.
> + */
> +void __console_putc(struct console_device *cdev, char c)
> +{
> +	__cdev_putc(cdev, c);
> +	if (c == '\n')
> +		__cdev_putc(cdev, '\r');
> +}
> diff --git a/pbl/console.c b/pbl/console.c
> index 75576ec79..c1c3e1dde 100644
> --- a/pbl/console.c
> +++ b/pbl/console.c
> @@ -2,12 +2,7 @@
>  #include <debug_ll.h>
>  #include <linux/err.h>
>  
> -/*
> - * Put these in the data section so that they survive the clearing of the
> - * BSS segment.
> - */
> -static __attribute__ ((section(".data"))) putc_func_t __putc;
> -static __attribute__ ((section(".data"))) void *putc_ctx;
> +extern struct console_device console_ll;

I mentioned these in previous mail, and I see them gone now. OK.

	Sam

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 02/27] console: Unify console_simple.c and pbl/console.c

[Sascha Hauer]

On Fri, Jun 15, 2018 at 06:58:05AM +0200, Sam Ravnborg wrote:
> Hi Andrey.
> 
> > +
> > +/**
> > + * __console_get_default - Return default output console
> > + *
> > + * Internal function used to determine which console to use for early
> > + * output. It has the following two use-cases:
> > + *
> > + *   1. PBL, where it falls back onto console_ll and whatever it is
> > + *      set up to (either putc_ll or custome callback set with
> > + *      pbl_set_putc())
> > + *
> > + *   2. CONSOLE_SIMPLE, where it falls back onto console_ll (which in
> > + *      this case always boils down to a putc_ll() call) until first
> > + *      (and only) console is registered via console_register().
> > + */
> > +struct console_device *__console_get_default(void)
> 
> If it is used as internal only then it should be static.
> (Have not read the the remaining patches)
> 
> > +{
> > +	/*
> > +	 * Doing on-demand initialization here as opposed to in the
> > +	 * definition of console_ll above has that advantage that on
> > +	 * some architecutres (e.g. AArch64) it allows us to avoid
> > +	 * additional relocation and makes it possible to use console
> > +	 * infrastructure before any relocation happens
> > +	 */
> > +	if (unlikely(!console_ll.putc))
> > +		console_ll.putc = __console_ll_putc;
> > +
> > +	if (IS_ENABLED(__PBL__) || !console)
> > +		return &console_ll;
> > +
> > +	return console;
> > +}
> > +
> > +/**
> > + * __console_set_putc - Early console initalization helper
> > + *
> > + * @cdev:	Console device to initialize
> > + * @putcf:	putc() implementation to be used for this console
> > + * @ctx:	Context to pass to putc()
> > + *
> > + * Internal function used to initialize early/trivial console devices
> > + * capable of only outputting a single character
> > + */
> > +void __console_set_putc(struct console_device *cdev,
> > +			putc_func_t putcf, void *ctx)
> Likewise, and so on for the reimaining __xxx functions.
> 
> 
> > +{
> > +	cdev->putc = (void *)putcf;
> > +	cdev->ctx = ctx;
> > +}
> > +
> > +/**
> > + * __cdev_putc - Internal .putc() callback dispatcher
> > + *
> > + * @cdev:	Console device to use
> > + * @c:		Character to print
> > + *
> > + * Internal .putc() callback dispatcher needed to correctly select
> > + * which context to pass.
> > + *
> > + * This is needed becuase when being used in PBL in conjunction with
> > + * pbl_set_putc(), .putc() callback is expecting to receive a void *
> > + * context that was registered earlier.
> > + *
> > + * In the "normal" world, however all of the .putc() callback are
> > + * written with expectation of receiving struct console_device * as a
> > + * first argument.
> > + *
> > + * Accomodation of both of those use-cases is the purpoese of this
> > + * function
> > + */
> > +void __cdev_putc(struct console_device *cdev, char c)
> > +{
> > +	void *ctx = cdev->ctx ? : cdev;
> Looks strange to me.
> What is ctx assigned in the "true" case?
> Maybe there is some subtle C rule that I have missed/forgot.
> Point is, if this is valid code it may also make other puzzeled.

It's a shortcut for a ? a : b and I like kind of like it. I'm also still
not there that I can read fluently over it, but I think it's worth
learning it.

What puzzles me here is that the putc callback is either passed a
context pointer or cdev itself if there is no context. I haven't read
the full patch yet, but that is one thing that I don't like.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 02/27] console: Unify console_simple.c and pbl/console.c

[Andrey Smirnov]

On Fri, Jun 15, 2018 at 12:26 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Fri, Jun 15, 2018 at 06:58:05AM +0200, Sam Ravnborg wrote:
> > Hi Andrey.
> >
> > > +
> > > +/**
> > > + * __console_get_default - Return default output console
> > > + *
> > > + * Internal function used to determine which console to use for early
> > > + * output. It has the following two use-cases:
> > > + *
> > > + *   1. PBL, where it falls back onto console_ll and whatever it is
> > > + *      set up to (either putc_ll or custome callback set with
> > > + *      pbl_set_putc())
> > > + *
> > > + *   2. CONSOLE_SIMPLE, where it falls back onto console_ll (which in
> > > + *      this case always boils down to a putc_ll() call) until first
> > > + *      (and only) console is registered via console_register().
> > > + */
> > > +struct console_device *__console_get_default(void)
> >
> > If it is used as internal only then it should be static.
> > (Have not read the the remaining patches)
> >
> > > +{
> > > +   /*
> > > +    * Doing on-demand initialization here as opposed to in the
> > > +    * definition of console_ll above has that advantage that on
> > > +    * some architecutres (e.g. AArch64) it allows us to avoid
> > > +    * additional relocation and makes it possible to use console
> > > +    * infrastructure before any relocation happens
> > > +    */
> > > +   if (unlikely(!console_ll.putc))
> > > +           console_ll.putc = __console_ll_putc;
> > > +
> > > +   if (IS_ENABLED(__PBL__) || !console)
> > > +           return &console_ll;
> > > +
> > > +   return console;
> > > +}
> > > +
> > > +/**
> > > + * __console_set_putc - Early console initalization helper
> > > + *
> > > + * @cdev:  Console device to initialize
> > > + * @putcf: putc() implementation to be used for this console
> > > + * @ctx:   Context to pass to putc()
> > > + *
> > > + * Internal function used to initialize early/trivial console devices
> > > + * capable of only outputting a single character
> > > + */
> > > +void __console_set_putc(struct console_device *cdev,
> > > +                   putc_func_t putcf, void *ctx)
> > Likewise, and so on for the reimaining __xxx functions.
> >
> >
> > > +{
> > > +   cdev->putc = (void *)putcf;
> > > +   cdev->ctx = ctx;
> > > +}
> > > +
> > > +/**
> > > + * __cdev_putc - Internal .putc() callback dispatcher
> > > + *
> > > + * @cdev:  Console device to use
> > > + * @c:             Character to print
> > > + *
> > > + * Internal .putc() callback dispatcher needed to correctly select
> > > + * which context to pass.
> > > + *
> > > + * This is needed becuase when being used in PBL in conjunction with
> > > + * pbl_set_putc(), .putc() callback is expecting to receive a void *
> > > + * context that was registered earlier.
> > > + *
> > > + * In the "normal" world, however all of the .putc() callback are
> > > + * written with expectation of receiving struct console_device * as a
> > > + * first argument.
> > > + *
> > > + * Accomodation of both of those use-cases is the purpoese of this
> > > + * function
> > > + */
> > > +void __cdev_putc(struct console_device *cdev, char c)
> > > +{
> > > +   void *ctx = cdev->ctx ? : cdev;
> > Looks strange to me.
> > What is ctx assigned in the "true" case?
> > Maybe there is some subtle C rule that I have missed/forgot.
> > Point is, if this is valid code it may also make other puzzeled.
>
> It's a shortcut for a ? a : b and I like kind of like it. I'm also still
> not there that I can read fluently over it, but I think it's worth
> learning it.
>
> What puzzles me here is that the putc callback is either passed a
> context pointer or cdev itself if there is no context. I haven't read
> the full patch yet, but that is one thing that I don't like.
>

This is needed to be able to use the same generic console code with
functions like pbl_set_putc() and psci_set_putc() which, unlike the
rest of console world, take a void * context that is expected to be
passed to putc() callback.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 02/27] console: Unify console_simple.c and pbl/console.c

[Andrey Smirnov]

On Thu, Jun 14, 2018 at 9:58 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Andrey.
>
> > +
> > +/**
> > + * __console_get_default - Return default output console
> > + *
> > + * Internal function used to determine which console to use for early
> > + * output. It has the following two use-cases:
> > + *
> > + *   1. PBL, where it falls back onto console_ll and whatever it is
> > + *      set up to (either putc_ll or custome callback set with
> > + *      pbl_set_putc())
> > + *
> > + *   2. CONSOLE_SIMPLE, where it falls back onto console_ll (which in
> > + *      this case always boils down to a putc_ll() call) until first
> > + *      (and only) console is registered via console_register().
> > + */
> > +struct console_device *__console_get_default(void)
>
> If it is used as internal only then it should be static.
> (Have not read the the remaining patches)

This function is defined in lib/console.c and used in pbl/console.c
all as a part of this patch. It cannot be declared static.

> > +{
> > +     /*
> > +      * Doing on-demand initialization here as opposed to in the
> > +      * definition of console_ll above has that advantage that on
> > +      * some architecutres (e.g. AArch64) it allows us to avoid
> > +      * additional relocation and makes it possible to use console
> > +      * infrastructure before any relocation happens
> > +      */
> > +     if (unlikely(!console_ll.putc))
> > +             console_ll.putc = __console_ll_putc;
> > +
> > +     if (IS_ENABLED(__PBL__) || !console)
> > +             return &console_ll;
> > +
> > +     return console;
> > +}
> > +
> > +/**
> > + * __console_set_putc - Early console initalization helper
> > + *
> > + * @cdev:    Console device to initialize
> > + * @putcf:   putc() implementation to be used for this console
> > + * @ctx:     Context to pass to putc()
> > + *
> > + * Internal function used to initialize early/trivial console devices
> > + * capable of only outputting a single character
> > + */
> > +void __console_set_putc(struct console_device *cdev,
> > +                     putc_func_t putcf, void *ctx)
> Likewise, and so on for the reimaining __xxx functions.
>

Same as above.

>
> > +{
> > +     cdev->putc = (void *)putcf;
> > +     cdev->ctx = ctx;
> > +}
> > +
> > +/**
> > + * __cdev_putc - Internal .putc() callback dispatcher
> > + *
> > + * @cdev:    Console device to use
> > + * @c:               Character to print
> > + *
> > + * Internal .putc() callback dispatcher needed to correctly select
> > + * which context to pass.
> > + *
> > + * This is needed becuase when being used in PBL in conjunction with
> > + * pbl_set_putc(), .putc() callback is expecting to receive a void *
> > + * context that was registered earlier.
> > + *
> > + * In the "normal" world, however all of the .putc() callback are
> > + * written with expectation of receiving struct console_device * as a
> > + * first argument.
> > + *
> > + * Accomodation of both of those use-cases is the purpoese of this
> > + * function
> > + */
> > +void __cdev_putc(struct console_device *cdev, char c)
> > +{
> > +     void *ctx = cdev->ctx ? : cdev;
> Looks strange to me.
> What is ctx assigned in the "true" case?
> Maybe there is some subtle C rule that I have missed/forgot.
> Point is, if this is valid code it may also make other puzzeled.
>

Sascha already commented on this one.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 03/27] pbl: console: Move '\n' handling into console_putc()

[Andrey Smirnov]

To make putchar() work correctly in PBL we need to make sure that '\n'
-> '\r\n' replacement is done in console_putc() instead of
console_puts(). This also allows us to share a bit more code by
moving console_putc() to lib/console.c.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console_simple.c | 6 ------
 lib/console.c           | 5 +++++
 pbl/console.c           | 8 --------
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/common/console_simple.c b/common/console_simple.c
index 62b811b3f..d560f8534 100644
--- a/common/console_simple.c
+++ b/common/console_simple.c
@@ -26,12 +26,6 @@ int console_puts(unsigned int ch, const char *str)
 }
 EXPORT_SYMBOL(console_puts);
 
-void console_putc(unsigned int ch, char c)
-{
-	__console_putc(__console_get_default(), c);
-}
-EXPORT_SYMBOL(console_putc);
-
 int tstc(void)
 {
 	if (!console)
diff --git a/lib/console.c b/lib/console.c
index d2d2ae775..94c20dada 100644
--- a/lib/console.c
+++ b/lib/console.c
@@ -128,3 +128,8 @@ void __console_putc(struct console_device *cdev, char c)
 	if (c == '\n')
 		__cdev_putc(cdev, '\r');
 }
+
+__weak void console_putc(unsigned int ch, char c)
+{
+	__console_putc(__console_get_default(), c);
+}
diff --git a/pbl/console.c b/pbl/console.c
index c1c3e1dde..b76c8cd75 100644
--- a/pbl/console.c
+++ b/pbl/console.c
@@ -17,19 +17,11 @@ void pbl_set_putc(putc_func_t putcf, void *ctx)
 	__console_set_putc(&console_ll, putcf, ctx);
 }
 
-void console_putc(unsigned int ch, char c)
-{
-	__cdev_putc(__console_get_default(), c);
-}
-
 int console_puts(unsigned int ch, const char *str)
 {
 	int n = 0;
 
 	while (*str) {
-		if (*str == '\n')
-			console_putc(CONSOLE_STDOUT, '\r');
-
 		console_putc(CONSOLE_STDOUT, *str);
 		str++;
 		n++;
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 04/27] console: Reconcile 3 different puts() implementations

[Andrey Smirnov]

PBL, CONSOLE_FULL and CONSOLE_SIMPLE contain almost identical puts()
implementations some of which also perform additional '\n' -> '\n\r'
compensation.

Move all of that code into a central location and share as much of it
as possible.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console.c        | 16 ----------
 common/console_simple.c | 17 -----------
 include/console.h       |  3 +-
 lib/console.c           | 67 +++++++++++++++++++++++++++++++++++++++--
 pbl/console.c           | 13 --------
 5 files changed, 67 insertions(+), 49 deletions(-)

diff --git a/common/console.c b/common/console.c
index ab3d4d397..d64a7be25 100644
--- a/common/console.c
+++ b/common/console.c
@@ -253,22 +253,6 @@ static void console_set_stdoutpath(struct console_device *cdev)
 	free(str);
 }
 
-static int __console_puts(struct console_device *cdev, const char *s)
-{
-	int n = 0;
-
-	while (*s) {
-		if (*s == '\n') {
-			cdev->putc(cdev, '\r');
-			n++;
-		}
-		cdev->putc(cdev, *s);
-		n++;
-		s++;
-	}
-	return n;
-}
-
 static int fops_open(struct cdev *cdev, unsigned long flags)
 {
 	struct console_device *priv = cdev->priv;
diff --git a/common/console_simple.c b/common/console_simple.c
index d560f8534..898f68a48 100644
--- a/common/console_simple.c
+++ b/common/console_simple.c
@@ -9,23 +9,6 @@ LIST_HEAD(console_list);
 EXPORT_SYMBOL(console_list);
 extern struct console_device *console;
 
-int console_puts(unsigned int ch, const char *str)
-{
-	const char *s = str;
-	int i = 0;
-
-	while (*s) {
-		console_putc(ch, *s);
-		if (*s == '\n')
-			console_putc(ch, '\r');
-		s++;
-		i++;
-	}
-
-	return i;
-}
-EXPORT_SYMBOL(console_puts);
-
 int tstc(void)
 {
 	if (!console)
diff --git a/include/console.h b/include/console.h
index b3001ad16..81a5a5534 100644
--- a/include/console.h
+++ b/include/console.h
@@ -211,7 +211,8 @@ static inline void pbl_set_putc(putc_func_t putcf, void *ctx) {}
 bool console_allow_color(void);
 
 void __cdev_putc(struct console_device *cdev, char c);
-void __console_putc(struct console_device *cdev, char c);
+int __console_putc(struct console_device *cdev, char c);
+int __console_puts(struct console_device *cdev, const char *str);
 
 struct console_device *__console_get_default(void);
 void __console_set_putc(struct console_device *cdev,
diff --git a/lib/console.c b/lib/console.c
index 94c20dada..9f0fa9fc9 100644
--- a/lib/console.c
+++ b/lib/console.c
@@ -121,15 +121,78 @@ void __cdev_putc(struct console_device *cdev, char c)
  *
  * Internal high-level putc() implementation based on __cdev_putc()
  * that performs correct '\n' -> '\n\r' substitution.
+ *
+ * Function returns number of printed characters
  */
-void __console_putc(struct console_device *cdev, char c)
+int __console_putc(struct console_device *cdev, char c)
 {
+	int n = 0;
+
 	__cdev_putc(cdev, c);
-	if (c == '\n')
+	n++;
+	if (c == '\n') {
 		__cdev_putc(cdev, '\r');
+		n++;
+	}
+
+	return n;
 }
 
+/**
+ * console_putc - Default console_putc() implementation
+ *
+ * @cdev:	Console device to use
+ * @c:		Character to print
+ *
+ * This is default console_putc() implementation used as is by PBL and
+ * CONSOLE_SIMPLE. Declared as __weak in order to allow other
+ * implementation to override it.
+ */
 __weak void console_putc(unsigned int ch, char c)
 {
 	__console_putc(__console_get_default(), c);
 }
+
+/**
+ * __console_puts - Default console device .puts() implementation
+ *
+ * @cdev:	Console device to use
+ * @str:	String to print
+ *
+ * This is default implementation of .puts() callback used:
+ *
+ * a) In PBL and CONSOLE_SIMPLE to implement console_puts() API (see
+ *    below)
+ *
+ * b) In CONSOLE_FULL as a default .puts() implementation when no
+ *    altenative implementation was provided by underlying low-level
+ *    driver
+ *
+ * Function returns number of printed characters
+ */
+int __console_puts(struct console_device *cdev, const char *str)
+{
+	int n = 0;
+
+	while (*str) {
+		n += __console_putc(cdev, *str);
+		str++;
+	}
+
+	return n;
+}
+
+/**
+ * console_puts - Default console_puts() implementation
+ *
+ * @cdev:	Console device to use
+ * @str:	String to print
+ *
+ * This is default console_puts() implementation used as is by PBL and
+ * CONSOLE_SIMPLE. Declared as __weak in order to allow other
+ * implementation to override it.
+ */
+__weak int console_puts(unsigned int ch, const char *str)
+{
+	return __console_puts(__console_get_default(), str);
+}
\ No newline at end of file
diff --git a/pbl/console.c b/pbl/console.c
index b76c8cd75..e9194b480 100644
--- a/pbl/console.c
+++ b/pbl/console.c
@@ -17,19 +17,6 @@ void pbl_set_putc(putc_func_t putcf, void *ctx)
 	__console_set_putc(&console_ll, putcf, ctx);
 }
 
-int console_puts(unsigned int ch, const char *str)
-{
-	int n = 0;
-
-	while (*str) {
-		console_putc(CONSOLE_STDOUT, *str);
-		str++;
-		n++;
-	}
-
-	return n;
-}
-
 int printf(const char *fmt, ...)
 {
 	va_list args;
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 04/27] console: Reconcile 3 different puts() implementations

[Sascha Hauer]

On Thu, Jun 14, 2018 at 09:11:13PM -0700, Andrey Smirnov wrote:
> PBL, CONSOLE_FULL and CONSOLE_SIMPLE contain almost identical puts()
> implementations some of which also perform additional '\n' -> '\n\r'
> compensation.
> 
> Move all of that code into a central location and share as much of it
> as possible.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  common/console.c        | 16 ----------
>  common/console_simple.c | 17 -----------
>  include/console.h       |  3 +-
>  lib/console.c           | 67 +++++++++++++++++++++++++++++++++++++++--
>  pbl/console.c           | 13 --------
>  5 files changed, 67 insertions(+), 49 deletions(-)
> 
> diff --git a/common/console.c b/common/console.c
> index ab3d4d397..d64a7be25 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -253,22 +253,6 @@ static void console_set_stdoutpath(struct console_device *cdev)
>  	free(str);
>  }
>  
> -static int __console_puts(struct console_device *cdev, const char *s)
> -{
> -	int n = 0;
> -
> -	while (*s) {
> -		if (*s == '\n') {
> -			cdev->putc(cdev, '\r');
> -			n++;
> -		}
> -		cdev->putc(cdev, *s);
> -		n++;
> -		s++;
> -	}
> -	return n;
> -}
> -
>  static int fops_open(struct cdev *cdev, unsigned long flags)
>  {
>  	struct console_device *priv = cdev->priv;
> diff --git a/common/console_simple.c b/common/console_simple.c
> index d560f8534..898f68a48 100644
> --- a/common/console_simple.c
> +++ b/common/console_simple.c
> @@ -9,23 +9,6 @@ LIST_HEAD(console_list);
>  EXPORT_SYMBOL(console_list);
>  extern struct console_device *console;
>  
> -int console_puts(unsigned int ch, const char *str)
> -{
> -	const char *s = str;
> -	int i = 0;
> -
> -	while (*s) {
> -		console_putc(ch, *s);
> -		if (*s == '\n')
> -			console_putc(ch, '\r');
> -		s++;
> -		i++;
> -	}
> -
> -	return i;
> -}
> -EXPORT_SYMBOL(console_puts);
> -
>  int tstc(void)
>  {
>  	if (!console)
> diff --git a/include/console.h b/include/console.h
> index b3001ad16..81a5a5534 100644
> --- a/include/console.h
> +++ b/include/console.h
> @@ -211,7 +211,8 @@ static inline void pbl_set_putc(putc_func_t putcf, void *ctx) {}
>  bool console_allow_color(void);
>  
>  void __cdev_putc(struct console_device *cdev, char c);
> -void __console_putc(struct console_device *cdev, char c);
> +int __console_putc(struct console_device *cdev, char c);
> +int __console_puts(struct console_device *cdev, const char *str);
>  
>  struct console_device *__console_get_default(void);
>  void __console_set_putc(struct console_device *cdev,
> diff --git a/lib/console.c b/lib/console.c
> index 94c20dada..9f0fa9fc9 100644
> --- a/lib/console.c
> +++ b/lib/console.c
> @@ -121,15 +121,78 @@ void __cdev_putc(struct console_device *cdev, char c)
>   *
>   * Internal high-level putc() implementation based on __cdev_putc()
>   * that performs correct '\n' -> '\n\r' substitution.
> + *
> + * Function returns number of printed characters
>   */
> -void __console_putc(struct console_device *cdev, char c)
> +int __console_putc(struct console_device *cdev, char c)
>  {
> +	int n = 0;
> +
>  	__cdev_putc(cdev, c);
> -	if (c == '\n')
> +	n++;
> +	if (c == '\n') {
>  		__cdev_putc(cdev, '\r');
> +		n++;
> +	}
> +
> +	return n;
>  }

The correct line ending is "\r\n", not the other way round. Please use
that one.

>  
> +/**
> + * console_putc - Default console_putc() implementation
> + *
> + * @cdev:	Console device to use

There is no @cdev parameter to this function.

> + * @c:		Character to print
> + *
> + * This is default console_putc() implementation used as is by PBL and
> + * CONSOLE_SIMPLE. Declared as __weak in order to allow other
> + * implementation to override it.
> + */
>  __weak void console_putc(unsigned int ch, char c)
>  {
>  	__console_putc(__console_get_default(), c);
>  }
> +
> +/**
> + * console_puts - Default console_puts() implementation
> + *
> + * @cdev:	Console device to use

ditto.

Sascha

> + *
> + * This is default console_puts() implementation used as is by PBL and
> + * CONSOLE_SIMPLE. Declared as __weak in order to allow other
> + * implementation to override it.
> + */
> +__weak int console_puts(unsigned int ch, const char *str)
> +{
> +	return __console_puts(__console_get_default(), str);
> +}
> \ No newline at end of file
> diff --git a/pbl/console.c b/pbl/console.c
> index b76c8cd75..e9194b480 100644
> --- a/pbl/console.c
> +++ b/pbl/console.c
> @@ -17,19 +17,6 @@ void pbl_set_putc(putc_func_t putcf, void *ctx)
>  	__console_set_putc(&console_ll, putcf, ctx);
>  }
>  
> -int console_puts(unsigned int ch, const char *str)
> -{
> -	int n = 0;
> -
> -	while (*str) {
> -		console_putc(CONSOLE_STDOUT, *str);
> -		str++;
> -		n++;
> -	}
> -
> -	return n;
> -}
> -
>  int printf(const char *fmt, ...)
>  {
>  	va_list args;
> -- 
> 2.17.0
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 04/27] console: Reconcile 3 different puts() implementations

[Andrey Smirnov]

On Fri, Jun 15, 2018 at 12:37 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Thu, Jun 14, 2018 at 09:11:13PM -0700, Andrey Smirnov wrote:
> > PBL, CONSOLE_FULL and CONSOLE_SIMPLE contain almost identical puts()
> > implementations some of which also perform additional '\n' -> '\n\r'
> > compensation.
> >
> > Move all of that code into a central location and share as much of it
> > as possible.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  common/console.c        | 16 ----------
> >  common/console_simple.c | 17 -----------
> >  include/console.h       |  3 +-
> >  lib/console.c           | 67 +++++++++++++++++++++++++++++++++++++++--
> >  pbl/console.c           | 13 --------
> >  5 files changed, 67 insertions(+), 49 deletions(-)
> >
> > diff --git a/common/console.c b/common/console.c
> > index ab3d4d397..d64a7be25 100644
> > --- a/common/console.c
> > +++ b/common/console.c
> > @@ -253,22 +253,6 @@ static void console_set_stdoutpath(struct console_device *cdev)
> >       free(str);
> >  }
> >
> > -static int __console_puts(struct console_device *cdev, const char *s)
> > -{
> > -     int n = 0;
> > -
> > -     while (*s) {
> > -             if (*s == '\n') {
> > -                     cdev->putc(cdev, '\r');
> > -                     n++;
> > -             }
> > -             cdev->putc(cdev, *s);
> > -             n++;
> > -             s++;
> > -     }
> > -     return n;
> > -}
> > -
> >  static int fops_open(struct cdev *cdev, unsigned long flags)
> >  {
> >       struct console_device *priv = cdev->priv;
> > diff --git a/common/console_simple.c b/common/console_simple.c
> > index d560f8534..898f68a48 100644
> > --- a/common/console_simple.c
> > +++ b/common/console_simple.c
> > @@ -9,23 +9,6 @@ LIST_HEAD(console_list);
> >  EXPORT_SYMBOL(console_list);
> >  extern struct console_device *console;
> >
> > -int console_puts(unsigned int ch, const char *str)
> > -{
> > -     const char *s = str;
> > -     int i = 0;
> > -
> > -     while (*s) {
> > -             console_putc(ch, *s);
> > -             if (*s == '\n')
> > -                     console_putc(ch, '\r');
> > -             s++;
> > -             i++;
> > -     }
> > -
> > -     return i;
> > -}
> > -EXPORT_SYMBOL(console_puts);
> > -
> >  int tstc(void)
> >  {
> >       if (!console)
> > diff --git a/include/console.h b/include/console.h
> > index b3001ad16..81a5a5534 100644
> > --- a/include/console.h
> > +++ b/include/console.h
> > @@ -211,7 +211,8 @@ static inline void pbl_set_putc(putc_func_t putcf, void *ctx) {}
> >  bool console_allow_color(void);
> >
> >  void __cdev_putc(struct console_device *cdev, char c);
> > -void __console_putc(struct console_device *cdev, char c);
> > +int __console_putc(struct console_device *cdev, char c);
> > +int __console_puts(struct console_device *cdev, const char *str);
> >
> >  struct console_device *__console_get_default(void);
> >  void __console_set_putc(struct console_device *cdev,
> > diff --git a/lib/console.c b/lib/console.c
> > index 94c20dada..9f0fa9fc9 100644
> > --- a/lib/console.c
> > +++ b/lib/console.c
> > @@ -121,15 +121,78 @@ void __cdev_putc(struct console_device *cdev, char c)
> >   *
> >   * Internal high-level putc() implementation based on __cdev_putc()
> >   * that performs correct '\n' -> '\n\r' substitution.
> > + *
> > + * Function returns number of printed characters
> >   */
> > -void __console_putc(struct console_device *cdev, char c)
> > +int __console_putc(struct console_device *cdev, char c)
> >  {
> > +     int n = 0;
> > +
> >       __cdev_putc(cdev, c);
> > -     if (c == '\n')
> > +     n++;
> > +     if (c == '\n') {
> >               __cdev_putc(cdev, '\r');
> > +             n++;
> > +     }
> > +
> > +     return n;
> >  }
>
> The correct line ending is "\r\n", not the other way round. Please use
> that one.
>

Sure, will do in v2.

> >
> > +/**
> > + * console_putc - Default console_putc() implementation
> > + *
> > + * @cdev:    Console device to use
>
> There is no @cdev parameter to this function.
>

Yeah, will fix in v2.

> > + * @c:               Character to print
> > + *
> > + * This is default console_putc() implementation used as is by PBL and
> > + * CONSOLE_SIMPLE. Declared as __weak in order to allow other
> > + * implementation to override it.
> > + */
> >  __weak void console_putc(unsigned int ch, char c)
> >  {
> >       __console_putc(__console_get_default(), c);
> >  }
> > +
> > +/**
> > + * console_puts - Default console_puts() implementation
> > + *
> > + * @cdev:    Console device to use
>
> ditto.
>

Will fix in v2.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 05/27] ratp: Add dependency on CONSOLE_FULL

[Andrey Smirnov]

CONSOLE_SIMPLE does not provide implementation of
console_set_active(). Instead of providing a dummy implementation to
cover for that, add dependency on CONSOLE_FULL since RATP would be
severely limited by single console device limitation of
CONSOLE_SIMPLE.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 lib/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/Kconfig b/lib/Kconfig
index 3d0665570..659f51bd1 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -95,6 +95,7 @@ config STMP_DEVICE
 
 config RATP
 	select CRC16
+	depends on CONSOLE_FULL
 	bool "RATP protocol support"
 	help
 	  Reliable Asynchronous Transfer Protocol (RATP) is a protocol for reliably
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 06/27] netconsole: Add dependency on CONSOLE_FULL

[Andrey Smirnov]

CONSOLE_SIMPLE allows only a single console device. Since first
console to be registerd for majority of the boards would be debug
serial, there's no straightforward way to make netconsole work with
CONSOLE_SIMPLE there.

Reflect that by making NET_NETCONSOLE depend on CONSOLE_FULL.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 net/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/Kconfig b/net/Kconfig
index 12b6bdb56..b396bc830 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -9,7 +9,7 @@ config NET_NFS
 
 config NET_NETCONSOLE
 	bool
-	depends on !CONSOLE_NONE
+	depends on CONSOLE_FULL
 	prompt "network console support"
 	help
 	  This option adds support for a simple udp based network console.
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 07/27] input: Add dependency on CONSOLE_FULL

[Andrey Smirnov]

CONSOLE_SIMPLE allows only a single console device. Since first
console to be registerd for majority of the boards would be debug
serial there's no straightforward way to make input framework work
with CONSOLE_SIMPLE there.

Reflect that by making "Input device support" depend on CONSOLE_FULL.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/input/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
index 1f89ae389..6abad444e 100644
--- a/drivers/input/Kconfig
+++ b/drivers/input/Kconfig
@@ -3,7 +3,7 @@
 #
 
 menu "Input device support"
-	depends on !CONSOLE_NONE
+	depends on CONSOLE_FULL
 
 config INPUT
 	bool
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 08/27] console: Make use of __console_putc()

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/common/console.c b/common/console.c
index d64a7be25..0c098c5eb 100644
--- a/common/console.c
+++ b/common/console.c
@@ -503,9 +503,7 @@ void console_putc(unsigned int ch, char c)
 	case CONSOLE_INIT_FULL:
 		for_each_console(cdev) {
 			if (cdev->f_active & ch) {
-				if (c == '\n')
-					cdev->putc(cdev, '\r');
-				cdev->putc(cdev, c);
+				__console_putc(cdev, c);
 			}
 		}
 		return;
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 09/27] console: Fix console_get_first_active()

[Andrey Smirnov]

Fix if condition in console_get_first_active() to actually check that
both bit are set.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console_common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/console_common.c b/common/console_common.c
index 00e020bd3..0131a1190 100644
--- a/common/console_common.c
+++ b/common/console_common.c
@@ -301,12 +301,13 @@ EXPORT_SYMBOL(console_get_by_name);
 struct console_device *console_get_first_active(void)
 {
 	struct console_device *cdev;
+	const unsigned char active = CONSOLE_STDIN | CONSOLE_STDOUT;
 	/*
 	 * Assumption to have BOTH CONSOLE_STDIN AND STDOUT in the
 	 * same output console
 	 */
 	for_each_console(cdev) {
-		if ((cdev->f_active & (CONSOLE_STDIN | CONSOLE_STDOUT)))
+		if ((cdev->f_active & active) == active)
 			return cdev;
 	}
 
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 10/27] console: Simplify early console code

[Andrey Smirnov]

Simplify early console code by converting both FIFO output buffering
as well as DEBUG_LL output into dummy console devices that are added
early on and removed as soon as they are not needed.

This change allows us to remove a whole bunch of corner case
handngling from both console_puts() and console_putc() in
CONSOLE_FULL.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console.c | 54 +++++++++++++++++++++++++++++++-----------------
 lib/console.c    | 10 ++++++++-
 2 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/common/console.c b/common/console.c
index 0c098c5eb..4d056b2de 100644
--- a/common/console.c
+++ b/common/console.c
@@ -59,6 +59,17 @@ static struct kfifo __console_output_fifo;
 static struct kfifo *console_input_fifo = &__console_input_fifo;
 static struct kfifo *console_output_fifo = &__console_output_fifo;
 
+static void __console_early_putc(struct console_device *cdev, char c)
+{
+	kfifo_putc(console_output_fifo, c);
+}
+
+static struct console_device console_early = {
+	.f_active = CONSOLE_STDOUT | CONSOLE_STDERR,
+	.puts = __console_puts,
+	.putc = __console_early_putc,
+};
+
 int console_open(struct console_device *cdev)
 {
 	int ret;
@@ -92,6 +103,8 @@ int console_close(struct console_device *cdev)
 	return 0;
 }
 
+static void console_destroy_early(void);
+
 int console_set_active(struct console_device *cdev, unsigned flag)
 {
 	int ret;
@@ -125,6 +138,9 @@ int console_set_active(struct console_device *cdev, unsigned flag)
 		puts_ll("Switch to console [");
 		puts_ll(dev_name(&cdev->class_dev));
 		puts_ll("]\n");
+
+		console_destroy_early();
+
 		barebox_banner();
 		while (kfifo_getc(console_output_fifo, &ch) == 0)
 			console_putc(CONSOLE_STDOUT, ch);
@@ -225,14 +241,27 @@ static int console_baudrate_set(struct param_d *param, void *priv)
 
 static void console_init_early(void)
 {
+	struct console_device *console_ll = __console_get_default();
+
 	kfifo_init(console_input_fifo, console_input_buffer,
 			CONSOLE_BUFFER_SIZE);
 	kfifo_init(console_output_fifo, console_output_buffer,
 			CONSOLE_BUFFER_SIZE);
 
+	list_add_tail(&console_ll->list, &console_list);
+	list_add_tail(&console_early.list, &console_list);
+
 	initialized = CONSOLE_INITIALIZED_BUFFER;
 }
 
+static void console_destroy_early(void)
+{
+	struct console_device *console_ll = __console_get_default();
+
+	list_del(&console_ll->list);
+	list_del(&console_early.list);
+}
+
 static void console_set_stdoutpath(struct console_device *cdev)
 {
 	int id;
@@ -496,9 +525,7 @@ void console_putc(unsigned int ch, char c)
 		/* fall through */
 
 	case CONSOLE_INITIALIZED_BUFFER:
-		kfifo_putc(console_output_fifo, c);
-		putc_ll(c);
-		return;
+		/* fall through */
 
 	case CONSOLE_INIT_FULL:
 		for_each_console(cdev) {
@@ -519,26 +546,15 @@ EXPORT_SYMBOL(console_putc);
 int console_puts(unsigned int ch, const char *str)
 {
 	struct console_device *cdev;
-	const char *s = str;
 	int n = 0;
 
-	if (initialized == CONSOLE_INIT_FULL) {
-		for_each_console(cdev) {
-			if (cdev->f_active & ch) {
-				n = cdev->puts(cdev, str);
-			}
-		}
-		return n;
-	}
+	if (initialized == CONSOLE_UNINITIALIZED)
+		console_init_early();
 
-	while (*s) {
-		if (*s == '\n') {
-			console_putc(ch, '\r');
-			n++;
+	for_each_console(cdev) {
+		if (cdev->f_active & ch) {
+			n = cdev->puts(cdev, str);
 		}
-		console_putc(ch, *s);
-		n++;
-		s++;
 	}
 	return n;
 }
diff --git a/lib/console.c b/lib/console.c
index 9f0fa9fc9..a8be7b7e3 100644
--- a/lib/console.c
+++ b/lib/console.c
@@ -21,6 +21,10 @@
  * console_ll - most basic low-level console
  */
 __data struct console_device console_ll = {
+#if defined(CONFIG_DEBUG_LL)
+	.f_active = CONSOLE_STDOUT | CONSOLE_STDERR,
+#endif
+	.puts = __console_puts,
 	.putc = NULL,
 	.ctx  = NULL,
 };
@@ -41,7 +45,7 @@ static void __console_ll_putc(struct console_device *cdev, char c)
  * __console_get_default - Return default output console
  *
  * Internal function used to determine which console to use for early
- * output. It has the following two use-cases:
+ * output. It has the following three use-cases:
  *
  *   1. PBL, where it falls back onto console_ll and whatever it is
  *      set up to (either putc_ll or custome callback set with
@@ -50,6 +54,10 @@ static void __console_ll_putc(struct console_device *cdev, char c)
  *   2. CONSOLE_SIMPLE, where it falls back onto console_ll (which in
  *      this case always boils down to a putc_ll() call) until first
  *      (and only) console is registered via console_register().
+ *
+ *   3. CONSOLE_FULL, where it is used to obtain properly initialized
+ *      early console to be used before the rest of console subsytem
+ *      gets properly initialized
  */
 struct console_device *__console_get_default(void)
 {
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 11/27] console: Consolidate all implemenatations of ctrlc()

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console.c        | 13 -------------
 common/console_simple.c | 11 -----------
 include/stdio.h         | 17 +++++++++--------
 lib/console.c           | 16 +++++++++++++++-
 pbl/console.c           |  5 -----
 5 files changed, 24 insertions(+), 38 deletions(-)

diff --git a/common/console.c b/common/console.c
index 4d056b2de..6f195e882 100644
--- a/common/console.c
+++ b/common/console.c
@@ -571,18 +571,5 @@ void console_flush(void)
 }
 EXPORT_SYMBOL(console_flush);
 
-#ifndef ARCH_HAS_CTRLC
-/* test if ctrl-c was pressed */
-int ctrlc (void)
-{
-	poller_call();
-
-	if (tstc() && getchar() == 3)
-		return 1;
-	return 0;
-}
-EXPORT_SYMBOL(ctrlc);
-#endif /* ARCH_HAS_CTRC */
-
 BAREBOX_MAGICVAR_NAMED(global_linux_bootargs_console, global.linux.bootargs.console,
 		"console= argument for Linux from the linux,stdout-path property in /chosen node");
diff --git a/common/console_simple.c b/common/console_simple.c
index 898f68a48..e2456e987 100644
--- a/common/console_simple.c
+++ b/common/console_simple.c
@@ -33,17 +33,6 @@ void console_flush(void)
 }
 EXPORT_SYMBOL(console_flush);
 
-#ifndef ARCH_HAS_CTRLC
-/* test if ctrl-c was pressed */
-int ctrlc (void)
-{
-	if (tstc() && getchar() == 3)
-		return 1;
-	return 0;
-}
-EXPORT_SYMBOL(ctrlc);
-#endif /* ARCH_HAS_CTRC */
-
 int console_register(struct console_device *newcdev)
 {
 	if (console)
diff --git a/include/stdio.h b/include/stdio.h
index 7b2a42b81..f5004cd07 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -22,6 +22,15 @@ int vasprintf(char **strp, const char *fmt, va_list ap);
 int vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
 int vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
 
+#if !defined(ARCH_HAS_CTRLC) && \
+	(defined(CONFIG_CONSOLE_NONE) || defined(__PBL__))
+/* test if ctrl-c was pressed */
+static inline int ctrlc (void)
+{
+	return 0;
+}
+#endif /* ARCH_HAS_CTRLC */
+
 #ifndef CONFIG_CONSOLE_NONE
 /* stdin */
 int tstc(void);
@@ -58,14 +67,6 @@ static inline int vprintf(const char *fmt, va_list args)
 	return 0;
 }
 
-#ifndef ARCH_HAS_CTRLC
-/* test if ctrl-c was pressed */
-static inline int ctrlc (void)
-{
-	return 0;
-}
-#endif /* ARCH_HAS_CTRLC */
-
 #endif
 
 #if (!defined(__PBL__) && !defined(CONFIG_CONSOLE_NONE)) || \
diff --git a/lib/console.c b/lib/console.c
index a8be7b7e3..356392e54 100644
--- a/lib/console.c
+++ b/lib/console.c
@@ -203,4 +203,18 @@ int __console_puts(struct console_device *cdev, const char *str)
 __weak int console_puts(unsigned int ch, const char *str)
 {
 	return __console_puts(__console_get_default(), str);
-}
\ No newline at end of file
+}
+
+#if !defined(ARCH_HAS_CTRLC) && !defined(__PBL__)
+/* test if ctrl-c was pressed */
+int ctrlc (void)
+{
+	if (IS_ENABLED(CONFIG_CONSOLE_FULL))
+		poller_call();
+
+	if (tstc() && getchar() == 3)
+		return 1;
+	return 0;
+}
+EXPORT_SYMBOL(ctrlc);
+#endif /* ARCH_HAS_CTRC */
\ No newline at end of file
diff --git a/pbl/console.c b/pbl/console.c
index e9194b480..6c277052f 100644
--- a/pbl/console.c
+++ b/pbl/console.c
@@ -46,8 +46,3 @@ int pr_print(int level, const char *fmt, ...)
 
 	return i;
 }
-
-int ctrlc(void)
-{
-	return 0;
-}
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 12/27] console: Drop ARCH_HAS_CTRLC

[Andrey Smirnov]

If we declare our default ctrlc() implementation as __weak we should
be easily override it with a platform specific implementation without
needing ARCH_HAS_CTRLC.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/sandbox/include/asm/common.h | 2 --
 include/stdio.h                   | 9 ---------
 lib/console.c                     | 9 +++++----
 3 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/arch/sandbox/include/asm/common.h b/arch/sandbox/include/asm/common.h
index 9b8bd2d94..b0e6b7fb1 100644
--- a/arch/sandbox/include/asm/common.h
+++ b/arch/sandbox/include/asm/common.h
@@ -1,6 +1,4 @@
 #ifndef ASM_COMMON_H
 #define ASM_COMMON_H
 
-#define ARCH_HAS_CTRLC
-
 #endif /* ASM_COMMON_H */
diff --git a/include/stdio.h b/include/stdio.h
index f5004cd07..d918f3682 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -22,15 +22,6 @@ int vasprintf(char **strp, const char *fmt, va_list ap);
 int vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
 int vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
 
-#if !defined(ARCH_HAS_CTRLC) && \
-	(defined(CONFIG_CONSOLE_NONE) || defined(__PBL__))
-/* test if ctrl-c was pressed */
-static inline int ctrlc (void)
-{
-	return 0;
-}
-#endif /* ARCH_HAS_CTRLC */
-
 #ifndef CONFIG_CONSOLE_NONE
 /* stdin */
 int tstc(void);
diff --git a/lib/console.c b/lib/console.c
index 356392e54..12c0ae2de 100644
--- a/lib/console.c
+++ b/lib/console.c
@@ -205,10 +205,12 @@ __weak int console_puts(unsigned int ch, const char *str)
 	return __console_puts(__console_get_default(), str);
 }
 
-#if !defined(ARCH_HAS_CTRLC) && !defined(__PBL__)
 /* test if ctrl-c was pressed */
-int ctrlc (void)
+__weak int ctrlc (void)
 {
+	if (IS_ENABLED(__PBL__) || IS_ENABLED(CONFIG_CONSOLE_NONE))
+		return 0;
+
 	if (IS_ENABLED(CONFIG_CONSOLE_FULL))
 		poller_call();
 
@@ -216,5 +218,4 @@ int ctrlc (void)
 		return 1;
 	return 0;
 }
-EXPORT_SYMBOL(ctrlc);
-#endif /* ARCH_HAS_CTRC */
\ No newline at end of file
+EXPORT_SYMBOL(ctrlc);
\ No newline at end of file
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 13/27] console: Consolidate DEBUG_LL and CONSOLE_* '\n' -> '\n\r' code

[Andrey Smirnov]

Consolidate code doing '\n' -> '\n\r' compensation in DEBUG_LL and
CONSOLE_* subsystems. While at it move it from puts_ll() to putc_ll()
in order to match the semantics of other puts()/putc()
implementations.

This is done as a macro in order to avoid putting any restrictions on
the signature of __putc.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 include/debug_ll.h | 38 ++++++++++++++++++++++++++++++++------
 lib/console.c      | 17 +++++++----------
 2 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/include/debug_ll.h b/include/debug_ll.h
index 504751639..4032ced95 100644
--- a/include/debug_ll.h
+++ b/include/debug_ll.h
@@ -31,9 +31,32 @@
 #include <mach/debug_ll.h>
 #endif
 
+/**
+ * __do_putc - Macro implementing '\n'-> '\n\r' substituting putc()
+ *
+ * @__putc:	Single argument or a macro that implements plain putc()
+ * @__c:	Character to print
+ *
+ * Internal macro used to implement putc_ll() and __console_putc() and
+ * intended to be the only place where '\n' -> '\n\r' substitution is
+ * codified
+ */
+#define __do_putc(__putc, ___c)			\
+	({					\
+		typeof(___c) __c = (___c);	\
+		int __n = 1;			\
+						\
+		__putc(__c);			\
+		if (__c == '\n') {		\
+			__putc('\r');		\
+			__n = 2;		\
+		}				\
+		__n;				\
+	})
+
 #if defined (CONFIG_DEBUG_LL)
 
-static inline void putc_ll(char value)
+static inline void __putc_ll(char value)
 {
 	PUTC_LL(value);
 }
@@ -45,10 +68,11 @@ static inline void puthex_ll(unsigned long value)
 	for (i = sizeof(unsigned long) * 2; i--; ) {
 		ch = ((value >> (i * 4)) & 0xf);
 		ch += (ch >= 10) ? 'a' - 10 : '0';
-		putc_ll(ch);
+		__putc_ll(ch);
 	}
 }
 
+static inline int putc_ll(char value);
 /*
  * Be careful with puts_ll, it only works if the binary is running at the
  * link address which often is not the case during early startup. If in doubt
@@ -57,9 +81,6 @@ static inline void puthex_ll(unsigned long value)
 static inline void puts_ll(const char * str)
 {
 	while (*str) {
-		if (*str == '\n')
-			putc_ll('\r');
-
 		putc_ll(*str);
 		str++;
 	}
@@ -67,7 +88,7 @@ static inline void puts_ll(const char * str)
 
 #else
 
-static inline void putc_ll(char value)
+static inline void __putc_ll(char value)
 {
 }
 
@@ -86,4 +107,9 @@ static inline void puts_ll(const char * str)
 
 #endif
 
+static inline int putc_ll(char value)
+{
+	return __do_putc(__putc_ll, value);
+}
+
 #endif  /* __INCLUDE_DEBUG_LL_H__ */
diff --git a/lib/console.c b/lib/console.c
index 12c0ae2de..f97deca97 100644
--- a/lib/console.c
+++ b/lib/console.c
@@ -134,16 +134,13 @@ void __cdev_putc(struct console_device *cdev, char c)
  */
 int __console_putc(struct console_device *cdev, char c)
 {
-	int n = 0;
-
-	__cdev_putc(cdev, c);
-	n++;
-	if (c == '\n') {
-		__cdev_putc(cdev, '\r');
-		n++;
-	}
-
-	return n;
+/*
+ * __do_putc expects a macro or a function of a single argument so we
+ * create this dummy adapter to work around that
+ */
+#define __CDEV_PUTC(__c)	__cdev_putc(cdev, __c)
+	
+	return __do_putc(__CDEV_PUTC, c);
 }
 
 /**
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 13/27] console: Consolidate DEBUG_LL and CONSOLE_* '\n' -> '\n\r' code

[Sascha Hauer]

On Thu, Jun 14, 2018 at 09:11:22PM -0700, Andrey Smirnov wrote:
> Consolidate code doing '\n' -> '\n\r' compensation in DEBUG_LL and
> CONSOLE_* subsystems. While at it move it from puts_ll() to putc_ll()
> in order to match the semantics of other puts()/putc()
> implementations.
> 
> This is done as a macro in order to avoid putting any restrictions on
> the signature of __putc.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  include/debug_ll.h | 38 ++++++++++++++++++++++++++++++++------
>  lib/console.c      | 17 +++++++----------
>  2 files changed, 39 insertions(+), 16 deletions(-)
> 
> diff --git a/include/debug_ll.h b/include/debug_ll.h
> index 504751639..4032ced95 100644
> --- a/include/debug_ll.h
> +++ b/include/debug_ll.h
> @@ -31,9 +31,32 @@
>  #include <mach/debug_ll.h>
>  #endif
>  
> +/**
> + * __do_putc - Macro implementing '\n'-> '\n\r' substituting putc()
> + *
> + * @__putc:	Single argument or a macro that implements plain putc()
> + * @__c:	Character to print
> + *
> + * Internal macro used to implement putc_ll() and __console_putc() and
> + * intended to be the only place where '\n' -> '\n\r' substitution is
> + * codified
> + */
> +#define __do_putc(__putc, ___c)			\
> +	({					\
> +		typeof(___c) __c = (___c);	\
> +		int __n = 1;			\
> +						\
> +		__putc(__c);			\
> +		if (__c == '\n') {		\
> +			__putc('\r');		\
> +			__n = 2;		\
> +		}				\
> +		__n;				\
> +	})
> +
>  #if defined (CONFIG_DEBUG_LL)
>  

...

> +/*
> + * __do_putc expects a macro or a function of a single argument so we
> + * create this dummy adapter to work around that
> + */
> +#define __CDEV_PUTC(__c)	__cdev_putc(cdev, __c)
> +	
> +	return __do_putc(__CDEV_PUTC, c);
>  }

Creating a macro for the "\n" -> "\r\n" conversion doesn't look like an
improvement to me, especially when another macro is necessary to make
the first one usable.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 13/27] console: Consolidate DEBUG_LL and CONSOLE_* '\n' -> '\n\r' code

[Andrey Smirnov]

On Mon, Jun 18, 2018 at 1:18 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Thu, Jun 14, 2018 at 09:11:22PM -0700, Andrey Smirnov wrote:
> > Consolidate code doing '\n' -> '\n\r' compensation in DEBUG_LL and
> > CONSOLE_* subsystems. While at it move it from puts_ll() to putc_ll()
> > in order to match the semantics of other puts()/putc()
> > implementations.
> >
> > This is done as a macro in order to avoid putting any restrictions on
> > the signature of __putc.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  include/debug_ll.h | 38 ++++++++++++++++++++++++++++++++------
> >  lib/console.c      | 17 +++++++----------
> >  2 files changed, 39 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/debug_ll.h b/include/debug_ll.h
> > index 504751639..4032ced95 100644
> > --- a/include/debug_ll.h
> > +++ b/include/debug_ll.h
> > @@ -31,9 +31,32 @@
> >  #include <mach/debug_ll.h>
> >  #endif
> >
> > +/**
> > + * __do_putc - Macro implementing '\n'-> '\n\r' substituting putc()
> > + *
> > + * @__putc:  Single argument or a macro that implements plain putc()
> > + * @__c:     Character to print
> > + *
> > + * Internal macro used to implement putc_ll() and __console_putc() and
> > + * intended to be the only place where '\n' -> '\n\r' substitution is
> > + * codified
> > + */
> > +#define __do_putc(__putc, ___c)                      \
> > +     ({                                      \
> > +             typeof(___c) __c = (___c);      \
> > +             int __n = 1;                    \
> > +                                             \
> > +             __putc(__c);                    \
> > +             if (__c == '\n') {              \
> > +                     __putc('\r');           \
> > +                     __n = 2;                \
> > +             }                               \
> > +             __n;                            \
> > +     })
> > +
> >  #if defined (CONFIG_DEBUG_LL)
> >
>
> ...
>
> > +/*
> > + * __do_putc expects a macro or a function of a single argument so we
> > + * create this dummy adapter to work around that
> > + */
> > +#define __CDEV_PUTC(__c)     __cdev_putc(cdev, __c)
> > +
> > +     return __do_putc(__CDEV_PUTC, c);
> >  }
>
> Creating a macro for the "\n" -> "\r\n" conversion doesn't look like an
> improvement to me, especially when another macro is necessary to make
> the first one usable.
>

OK, will drop this in v2.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 14/27] console: Consolidate DEBUG_LL and CONSOLE_* puts() implementations

[Andrey Smirnov]

Similar to previous commit, consolidate DEBUG_LL and CONSOLE_* puts()
implementations by putting them into a shared macro.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 include/debug_ll.h | 23 +++++++++++++++++++----
 lib/console.c      | 13 ++++++-------
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/include/debug_ll.h b/include/debug_ll.h
index 4032ced95..4dcbd0434 100644
--- a/include/debug_ll.h
+++ b/include/debug_ll.h
@@ -54,6 +54,24 @@
 		__n;				\
 	})
 
+/**
+ * __do_puts - Macro implementing puts()
+ *
+ * @__putc:	Single argument or a macro that implements putc()
+ * @___s:	String to print
+ *
+ * Internal macro used to implement puts_ll() and __console_puts()
+ */
+#define __do_puts(__putc, ___s)			\
+	({					\
+		const char *__s = (___s);	\
+		int __n = 0;			\
+						\
+		while (*__s)			\
+			__n += __putc(*__s++);	\
+		__n;				\
+	})
+
 #if defined (CONFIG_DEBUG_LL)
 
 static inline void __putc_ll(char value)
@@ -80,10 +98,7 @@ static inline int putc_ll(char value);
  */
 static inline void puts_ll(const char * str)
 {
-	while (*str) {
-		putc_ll(*str);
-		str++;
-	}
+	__do_puts(putc_ll, str);
 }
 
 #else
diff --git a/lib/console.c b/lib/console.c
index f97deca97..91c446d6e 100644
--- a/lib/console.c
+++ b/lib/console.c
@@ -177,14 +177,13 @@ __weak void console_putc(unsigned int ch, char c)
  */
 int __console_puts(struct console_device *cdev, const char *str)
 {
-	int n = 0;
-
-	while (*str) {
-		n += __console_putc(cdev, *str);
-		str++;
-	}
+/*
+ * __do_puts expects a macro or a function of a single argument so we
+ * create this dummy adapter to work around that
+ */
+#define __CONSOLE_PUTC(__c)	__console_putc(cdev, __c)
 
-	return n;
+	return __do_puts(__CONSOLE_PUTC, str);
 }
 
 /**
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 14/27] console: Consolidate DEBUG_LL and CONSOLE_* puts() implementations

[Sascha Hauer]

On Thu, Jun 14, 2018 at 09:11:23PM -0700, Andrey Smirnov wrote:
> Similar to previous commit, consolidate DEBUG_LL and CONSOLE_* puts()
> implementations by putting them into a shared macro.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  include/debug_ll.h | 23 +++++++++++++++++++----
>  lib/console.c      | 13 ++++++-------
>  2 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/include/debug_ll.h b/include/debug_ll.h
> index 4032ced95..4dcbd0434 100644
> --- a/include/debug_ll.h
> +++ b/include/debug_ll.h
> @@ -54,6 +54,24 @@
>  		__n;				\
>  	})
>  
> +/**
> + * __do_puts - Macro implementing puts()
> + *
> + * @__putc:	Single argument or a macro that implements putc()
> + * @___s:	String to print
> + *
> + * Internal macro used to implement puts_ll() and __console_puts()
> + */
> +#define __do_puts(__putc, ___s)			\
> +	({					\
> +		const char *__s = (___s);	\
> +		int __n = 0;			\
> +						\
> +		while (*__s)			\
> +			__n += __putc(*__s++);	\
> +		__n;				\
> +	})

I don't like this. It's a simple loop and there's nothing bad with it
when the same loop is present twice in barebox. No need to dig into
CPP here.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 14/27] console: Consolidate DEBUG_LL and CONSOLE_* puts() implementations

[Andrey Smirnov]

On Mon, Jun 18, 2018 at 1:22 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Thu, Jun 14, 2018 at 09:11:23PM -0700, Andrey Smirnov wrote:
> > Similar to previous commit, consolidate DEBUG_LL and CONSOLE_* puts()
> > implementations by putting them into a shared macro.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  include/debug_ll.h | 23 +++++++++++++++++++----
> >  lib/console.c      | 13 ++++++-------
> >  2 files changed, 25 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/debug_ll.h b/include/debug_ll.h
> > index 4032ced95..4dcbd0434 100644
> > --- a/include/debug_ll.h
> > +++ b/include/debug_ll.h
> > @@ -54,6 +54,24 @@
> >               __n;                            \
> >       })
> >
> > +/**
> > + * __do_puts - Macro implementing puts()
> > + *
> > + * @__putc:  Single argument or a macro that implements putc()
> > + * @___s:    String to print
> > + *
> > + * Internal macro used to implement puts_ll() and __console_puts()
> > + */
> > +#define __do_puts(__putc, ___s)                      \
> > +     ({                                      \
> > +             const char *__s = (___s);       \
> > +             int __n = 0;                    \
> > +                                             \
> > +             while (*__s)                    \
> > +                     __n += __putc(*__s++);  \
> > +             __n;                            \
> > +     })
>
> I don't like this. It's a simple loop and there's nothing bad with it
> when the same loop is present twice in barebox. No need to dig into
> CPP here.
>

OK, will drop in v2.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 15/27] console_simple: Use console_flush() from CONSOLE_FULL

[Andrey Smirnov]

Both implementations of console_flush() should behave the same way.
Arguably CONSOLE_FULL's implementation is a bit bulkier that what
CONSOLE_SIMPLE has, but this replacement allows us to share more code
between CONSOLE_SIMPLE and CONSOLE_FULL as well as reduce our
dependencies on global console pointer in CONSOLE_SIMPLE.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console.c        | 14 --------------
 common/console_common.c | 11 +++++++++++
 common/console_simple.c |  9 ---------
 lib/console.c           |  2 ++
 4 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/common/console.c b/common/console.c
index 6f195e882..f454f91c0 100644
--- a/common/console.c
+++ b/common/console.c
@@ -38,9 +38,6 @@
 #include <linux/stringify.h>
 #include <debug_ll.h>
 
-LIST_HEAD(console_list);
-EXPORT_SYMBOL(console_list);
-
 #define CONSOLE_UNINITIALIZED		0
 #define CONSOLE_INITIALIZED_BUFFER	1
 #define CONSOLE_INIT_FULL		2
@@ -560,16 +557,5 @@ int console_puts(unsigned int ch, const char *str)
 }
 EXPORT_SYMBOL(console_puts);
 
-void console_flush(void)
-{
-	struct console_device *cdev;
-
-	for_each_console(cdev) {
-		if (cdev->flush)
-			cdev->flush(cdev);
-	}
-}
-EXPORT_SYMBOL(console_flush);
-
 BAREBOX_MAGICVAR_NAMED(global_linux_bootargs_console, global.linux.bootargs.console,
 		"console= argument for Linux from the linux,stdout-path property in /chosen node");
diff --git a/common/console_common.c b/common/console_common.c
index 0131a1190..324d42915 100644
--- a/common/console_common.c
+++ b/common/console_common.c
@@ -315,6 +315,17 @@ struct console_device *console_get_first_active(void)
 }
 EXPORT_SYMBOL(console_get_first_active);
 
+void console_flush(void)
+{
+	struct console_device *cdev;
+
+	for_each_console(cdev) {
+		if (cdev->flush)
+			cdev->flush(cdev);
+	}
+}
+EXPORT_SYMBOL(console_flush);
+
 #endif /* !CONFIG_CONSOLE_NONE */
 
 int dprintf(int file, const char *fmt, ...)
diff --git a/common/console_simple.c b/common/console_simple.c
index e2456e987..9da516ee9 100644
--- a/common/console_simple.c
+++ b/common/console_simple.c
@@ -5,8 +5,6 @@
 #include <debug_ll.h>
 #include <console.h>
 
-LIST_HEAD(console_list);
-EXPORT_SYMBOL(console_list);
 extern struct console_device *console;
 
 int tstc(void)
@@ -26,13 +24,6 @@ int getchar(void)
 }
 EXPORT_SYMBOL(getchar);
 
-void console_flush(void)
-{
-	if (console && console->flush)
-		console->flush(console);
-}
-EXPORT_SYMBOL(console_flush);
-
 int console_register(struct console_device *newcdev)
 {
 	if (console)
diff --git a/lib/console.c b/lib/console.c
index 91c446d6e..b8d6456c9 100644
--- a/lib/console.c
+++ b/lib/console.c
@@ -29,6 +29,8 @@ __data struct console_device console_ll = {
 	.ctx  = NULL,
 };
 __data struct console_device *console;
+LIST_HEAD(console_list);
+EXPORT_SYMBOL(console_list);
 
 /**
  * __console_ll_putc - Early, most primitive putc() implemenatation
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 16/27] console_simple: Use tstc_raw() as tstc()

[Andrey Smirnov]

Semantics implemented by tstc_raw() in CONSOLE_FULL is reasonably
close to what testc() in CONSOLE_SIMPLE does. Arguably tstc_raw() is
bulkier that what CONSOLE_SIMPLE has, but this replacement allows us
to share more code between CONSOLE_SIMPLE and CONSOLE_FULL as well as
reduce our dependencies on global console pointer in CONSOLE_SIMPLE.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console.c        | 14 --------------
 common/console_common.c | 17 +++++++++++++++++
 common/console_simple.c |  9 ---------
 include/console.h       |  2 ++
 4 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/common/console.c b/common/console.c
index f454f91c0..c8c413a5e 100644
--- a/common/console.c
+++ b/common/console.c
@@ -454,20 +454,6 @@ static int getc_raw(void)
 	}
 }
 
-static int tstc_raw(void)
-{
-	struct console_device *cdev;
-
-	for_each_console(cdev) {
-		if (!(cdev->f_active & CONSOLE_STDIN))
-			continue;
-		if (cdev->tstc(cdev))
-			return 1;
-	}
-
-	return 0;
-}
-
 int getchar(void)
 {
 	unsigned char ch;
diff --git a/common/console_common.c b/common/console_common.c
index 324d42915..0653cb916 100644
--- a/common/console_common.c
+++ b/common/console_common.c
@@ -326,6 +326,23 @@ void console_flush(void)
 }
 EXPORT_SYMBOL(console_flush);
 
+int tstc_raw(void)
+{
+	struct console_device *cdev;
+
+	for_each_console(cdev) {
+		if (!(cdev->f_active & CONSOLE_STDIN))
+			continue;
+		if (cdev->tstc(cdev))
+			return 1;
+	}
+
+	return 0;
+}
+
+__weak int tstc(void) __alias(tstc_raw);
+EXPORT_SYMBOL(tstc);
+
 #endif /* !CONFIG_CONSOLE_NONE */
 
 int dprintf(int file, const char *fmt, ...)
diff --git a/common/console_simple.c b/common/console_simple.c
index 9da516ee9..475d96b68 100644
--- a/common/console_simple.c
+++ b/common/console_simple.c
@@ -7,15 +7,6 @@
 
 extern struct console_device *console;
 
-int tstc(void)
-{
-	if (!console)
-		return 0;
-
-	return console->tstc(console);
-}
-EXPORT_SYMBOL(tstc);
-
 int getchar(void)
 {
 	if (!console)
diff --git a/include/console.h b/include/console.h
index 81a5a5534..d8d36f219 100644
--- a/include/console.h
+++ b/include/console.h
@@ -218,4 +218,6 @@ struct console_device *__console_get_default(void);
 void __console_set_putc(struct console_device *cdev,
 			putc_func_t putcf, void *ctx);
 
+int tstc_raw(void);
+
 #endif
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 17/27] console_simple: Use getc_raw() as getchar()

[Andrey Smirnov]

Semantics implemented by getc_raw() in CONSOLE_FULL is reasonably
close to what getchar() in CONSOLE_SIMPLE does. Arguably getc_raw() is
bulkier than what CONSOLE_SIMPLE has, but this replacement allows us
to share more code between CONSOLE_SIMPLE and CONSOLE_FULL as well as
reduce our dependencies on global console pointer in CONSOLE_SIMPLE.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console.c        | 27 ---------------------------
 common/console_common.c | 29 +++++++++++++++++++++++++++++
 common/console_simple.c |  8 --------
 include/console.h       |  1 +
 4 files changed, 30 insertions(+), 35 deletions(-)

diff --git a/common/console.c b/common/console.c
index c8c413a5e..a6f8ac445 100644
--- a/common/console.c
+++ b/common/console.c
@@ -427,33 +427,6 @@ int console_unregister(struct console_device *cdev)
 }
 EXPORT_SYMBOL(console_unregister);
 
-static int getc_raw(void)
-{
-	struct console_device *cdev;
-	int active = 0;
-
-	while (1) {
-		for_each_console(cdev) {
-			if (!(cdev->f_active & CONSOLE_STDIN))
-				continue;
-			active = 1;
-			if (cdev->tstc(cdev)) {
-				int ch = cdev->getc(cdev);
-
-				if (IS_ENABLED(CONFIG_RATP) && ch == 0x01) {
-					barebox_ratp(cdev);
-					return -1;
-				}
-
-				return ch;
-			}
-		}
-		if (!active)
-			/* no active console found. bail out */
-			return -1;
-	}
-}
-
 int getchar(void)
 {
 	unsigned char ch;
diff --git a/common/console_common.c b/common/console_common.c
index 0653cb916..06ed74fc2 100644
--- a/common/console_common.c
+++ b/common/console_common.c
@@ -30,6 +30,7 @@
 #include <clock.h>
 #include <malloc.h>
 #include <asm-generic/div64.h>
+#include <ratp_bb.h>
 
 #ifndef CONFIG_CONSOLE_NONE
 
@@ -343,6 +344,34 @@ int tstc_raw(void)
 __weak int tstc(void) __alias(tstc_raw);
 EXPORT_SYMBOL(tstc);
 
+int getc_raw(void)
+{
+	struct console_device *cdev;
+	int active = 0;
+
+	while (1) {
+		for_each_console(cdev) {
+			if (!(cdev->f_active & CONSOLE_STDIN))
+				continue;
+			active = 1;
+			if (cdev->tstc(cdev)) {
+				int ch = cdev->getc(cdev);
+
+				if (IS_ENABLED(CONFIG_RATP) && ch == 0x01) {
+					barebox_ratp(cdev);
+					return -1;
+				}
+
+				return ch;
+			}
+		}
+		if (!active)
+			/* no active console found. bail out */
+			return -1;
+	}
+}
+__weak int getchar(void) __alias(getc_raw);
+
 #endif /* !CONFIG_CONSOLE_NONE */
 
 int dprintf(int file, const char *fmt, ...)
diff --git a/common/console_simple.c b/common/console_simple.c
index 475d96b68..ac22a2592 100644
--- a/common/console_simple.c
+++ b/common/console_simple.c
@@ -7,14 +7,6 @@
 
 extern struct console_device *console;
 
-int getchar(void)
-{
-	if (!console)
-		return -EINVAL;
-	return console->getc(console);
-}
-EXPORT_SYMBOL(getchar);
-
 int console_register(struct console_device *newcdev)
 {
 	if (console)
diff --git a/include/console.h b/include/console.h
index d8d36f219..2a9f5fb79 100644
--- a/include/console.h
+++ b/include/console.h
@@ -219,5 +219,6 @@ void __console_set_putc(struct console_device *cdev,
 			putc_func_t putcf, void *ctx);
 
 int tstc_raw(void);
+int getc_raw(void);
 
 #endif
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 18/27] console_simple: Get rid of global console pointer

[Andrey Smirnov]

We already add registered console to global console_list, so there
isn't really a need to have a global console pointer. Now, with
majority of references to console pointer gone, we can easily ditch it
and use console_list directly instead.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console_simple.c | 5 +----
 lib/console.c           | 7 +++----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/common/console_simple.c b/common/console_simple.c
index ac22a2592..91b6cb6cd 100644
--- a/common/console_simple.c
+++ b/common/console_simple.c
@@ -5,14 +5,11 @@
 #include <debug_ll.h>
 #include <console.h>
 
-extern struct console_device *console;
-
 int console_register(struct console_device *newcdev)
 {
-	if (console)
+	if (!list_empty(&console_list))
 		return -EBUSY;
 
-	console = newcdev;
 	console_list.prev = console_list.next = &newcdev->list;
 	newcdev->list.prev = newcdev->list.next = &console_list;
 
diff --git a/lib/console.c b/lib/console.c
index b8d6456c9..03563c9de 100644
--- a/lib/console.c
+++ b/lib/console.c
@@ -28,8 +28,7 @@ __data struct console_device console_ll = {
 	.putc = NULL,
 	.ctx  = NULL,
 };
-__data struct console_device *console;
-LIST_HEAD(console_list);
+__data LIST_HEAD(console_list);
 EXPORT_SYMBOL(console_list);
 
 /**
@@ -73,10 +72,10 @@ struct console_device *__console_get_default(void)
 	if (unlikely(!console_ll.putc))
 		console_ll.putc = __console_ll_putc;
 
-	if (IS_ENABLED(__PBL__) || !console)
+	if (IS_ENABLED(__PBL__) || list_empty(&console_list))
 		return &console_ll;
 
-	return console;
+	return list_first_entry(&console_list, struct console_device, list);
 }
 
 /**
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 19/27] console_simple: Make use of list_add_tail()

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console_simple.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/common/console_simple.c b/common/console_simple.c
index 91b6cb6cd..f13f9d71a 100644
--- a/common/console_simple.c
+++ b/common/console_simple.c
@@ -10,8 +10,7 @@ int console_register(struct console_device *newcdev)
 	if (!list_empty(&console_list))
 		return -EBUSY;
 
-	console_list.prev = console_list.next = &newcdev->list;
-	newcdev->list.prev = newcdev->list.next = &console_list;
+	list_add_tail(&newcdev->list, &console_list);
 
 	if (newcdev->setbrg) {
 		newcdev->baudrate = CONFIG_BAUDRATE;
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 20/27] console: Share definition for printf with PBL

[Andrey Smirnov]

Both CONSOLE_* and PBL have almost identical implementations of
pritnf(). Move the code to a common location so it can be share
between the two.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console_common.c | 22 ----------------------
 lib/console.c           | 24 +++++++++++++++++++++++-
 pbl/console.c           | 15 ---------------
 3 files changed, 23 insertions(+), 38 deletions(-)

diff --git a/common/console_common.c b/common/console_common.c
index 06ed74fc2..f440a9448 100644
--- a/common/console_common.c
+++ b/common/console_common.c
@@ -227,28 +227,6 @@ void log_print(unsigned flags)
 	}
 }
 
-int printf(const char *fmt, ...)
-{
-	va_list args;
-	int i;
-	char printbuffer[CFG_PBSIZE];
-
-	va_start(args, fmt);
-
-	/*
-	 * For this to work, printbuffer must be larger than
-	 * anything we ever want to print.
-	 */
-	i = vsprintf (printbuffer, fmt, args);
-	va_end(args);
-
-	/* Print the string */
-	puts(printbuffer);
-
-	return i;
-}
-EXPORT_SYMBOL(printf);
-
 int vprintf(const char *fmt, va_list args)
 {
 	int i;
diff --git a/lib/console.c b/lib/console.c
index 03563c9de..57c3578d6 100644
--- a/lib/console.c
+++ b/lib/console.c
@@ -215,4 +215,26 @@ __weak int ctrlc (void)
 		return 1;
 	return 0;
 }
-EXPORT_SYMBOL(ctrlc);
\ No newline at end of file
+EXPORT_SYMBOL(ctrlc);
+
+int printf(const char *fmt, ...)
+{
+	va_list args;
+	int i;
+	char printbuffer[CFG_PBSIZE];
+
+	va_start(args, fmt);
+
+	/*
+	 * For this to work, printbuffer must be larger than
+	 * anything we ever want to print.
+	 */
+	i = vsprintf(printbuffer, fmt, args);
+	va_end(args);
+
+	/* Print the string */
+	puts(printbuffer);
+
+	return i;
+}
+EXPORT_SYMBOL(printf);
\ No newline at end of file
diff --git a/pbl/console.c b/pbl/console.c
index 6c277052f..18df1efca 100644
--- a/pbl/console.c
+++ b/pbl/console.c
@@ -17,21 +17,6 @@ void pbl_set_putc(putc_func_t putcf, void *ctx)
 	__console_set_putc(&console_ll, putcf, ctx);
 }
 
-int printf(const char *fmt, ...)
-{
-	va_list args;
-	uint i;
-	char printbuffer[CFG_PBSIZE];
-
-	va_start(args, fmt);
-	i = vsprintf(printbuffer, fmt, args);
-	va_end(args);
-
-	console_puts(CONSOLE_STDOUT, printbuffer);
-
-	return i;
-}
-
 int pr_print(int level, const char *fmt, ...)
 {
 	va_list args;
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 21/27] pbl: console: Convert pr_print into a single line #define

[Andrey Smirnov]

PBL's implementation of pr_print(), doesn't use its 'level' argument
at all, so it is identical in its behaviour to simple printf(). In
light of that drop the custom code implementing that and convert it to
be a CPP macro that is expanded into simple printf().

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 include/printk.h |  5 +++--
 pbl/console.c    | 14 --------------
 2 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/include/printk.h b/include/printk.h
index b4ae0b321..9f1831fbe 100644
--- a/include/printk.h
+++ b/include/printk.h
@@ -34,10 +34,11 @@ static inline int dev_printf(int level, const struct device_d *dev, const char *
 }
 #endif
 
-#if (!defined(__PBL__) && !defined(CONFIG_CONSOLE_NONE)) || \
-	(defined(__PBL__) && defined(CONFIG_PBL_CONSOLE))
+#if (!defined(__PBL__) && !defined(CONFIG_CONSOLE_NONE))
 int pr_print(int level, const char *format, ...)
 	__attribute__ ((format(__printf__, 2, 3)));
+#elif defined(__PBL__) && defined(CONFIG_PBL_CONSOLE)
+#define pr_print(level, format, arg...)	printf(format, ## arg)
 #else
 static int pr_print(int level, const char *format, ...)
 	__attribute__ ((format(__printf__, 2, 3)));
diff --git a/pbl/console.c b/pbl/console.c
index 18df1efca..78beab688 100644
--- a/pbl/console.c
+++ b/pbl/console.c
@@ -17,17 +17,3 @@ void pbl_set_putc(putc_func_t putcf, void *ctx)
 	__console_set_putc(&console_ll, putcf, ctx);
 }
 
-int pr_print(int level, const char *fmt, ...)
-{
-	va_list args;
-	uint i;
-	char printbuffer[CFG_PBSIZE];
-
-	va_start(args, fmt);
-	i = vsprintf(printbuffer, fmt, args);
-	va_end(args);
-
-	console_puts(CONSOLE_STDOUT, printbuffer);
-
-	return i;
-}
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 22/27] console: Convert pr_print into a single line #define

[Andrey Smirnov]

If we change dev_printf() to not print device related info at the
start if NULL is passed as device pointer, we can easliy use that
function to do what pr_print does. With this change we can convert
pr_print to be a simple macro that expands into a dev_printf() and
drop a bit of exctra code.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console_common.c | 28 +++++-----------------------
 include/printk.h        |  3 +--
 2 files changed, 6 insertions(+), 25 deletions(-)

diff --git a/common/console_common.c b/common/console_common.c
index f440a9448..296b181d9 100644
--- a/common/console_common.c
+++ b/common/console_common.c
@@ -124,26 +124,6 @@ static void print_colored_log_level(const int level)
 	pr_puts(level, colored_log_level[level]);
 }
 
-int pr_print(int level, const char *fmt, ...)
-{
-	va_list args;
-	int i;
-	char printbuffer[CFG_PBSIZE];
-
-	if (!IS_ENABLED(CONFIG_LOGBUF) && level > barebox_loglevel)
-		return 0;
-
-	print_colored_log_level(level);
-
-	va_start(args, fmt);
-	i = vsprintf(printbuffer, fmt, args);
-	va_end(args);
-
-	pr_puts(level, printbuffer);
-
-	return i;
-}
-
 int dev_printf(int level, const struct device_d *dev, const char *format, ...)
 {
 	va_list args;
@@ -155,10 +135,12 @@ int dev_printf(int level, const struct device_d *dev, const char *format, ...)
 
 	print_colored_log_level(level);
 
-	if (dev->driver && dev->driver->name)
-		ret += sprintf(printbuffer, "%s ", dev->driver->name);
+	if (dev) {
+		if (dev->driver && dev->driver->name)
+			ret += sprintf(printbuffer, "%s ", dev->driver->name);
 
-	ret += sprintf(printbuffer + ret, "%s: ", dev_name(dev));
+		ret += sprintf(printbuffer + ret, "%s: ", dev_name(dev));
+	}
 
 	va_start(args, format);
 
diff --git a/include/printk.h b/include/printk.h
index 9f1831fbe..2bf0cb25b 100644
--- a/include/printk.h
+++ b/include/printk.h
@@ -35,8 +35,7 @@ static inline int dev_printf(int level, const struct device_d *dev, const char *
 #endif
 
 #if (!defined(__PBL__) && !defined(CONFIG_CONSOLE_NONE))
-int pr_print(int level, const char *format, ...)
-	__attribute__ ((format(__printf__, 2, 3)));
+#define pr_print(level, format, arg...)	dev_printf(level, NULL, format, ## arg)
 #elif defined(__PBL__) && defined(CONFIG_PBL_CONSOLE)
 #define pr_print(level, format, arg...)	printf(format, ## arg)
 #else
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 23/27] console: Remove dputc()

[Andrey Smirnov]

The only use of dputc() is commands/echo.c and the code there can
easily be converted to dputs(). Do that and drop dputc() to avoid
duplicating fd selection logic already found in dputs().

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 commands/echo.c         |  4 ++--
 common/console_common.c | 15 +--------------
 include/stdio.h         |  1 -
 3 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/commands/echo.c b/commands/echo.c
index 8853ee0a3..233f804a2 100644
--- a/commands/echo.c
+++ b/commands/echo.c
@@ -81,7 +81,7 @@ exit_parse:
 
 	for (i = optind; i < argc; i++) {
 		if (i > optind)
-			dputc(fd, ' ');
+			dputs(fd, " ");
 		if (process_escape) {
 			process_escape_sequence(argv[i], str, CONFIG_CBSIZE);
 			dputs(fd, str);
@@ -91,7 +91,7 @@ exit_parse:
 	}
 
 	if (newline)
-		dputc(fd, '\n');
+		dputs(fd, "\n");
 
 	if (file)
 		close(fd);
diff --git a/common/console_common.c b/common/console_common.c
index 296b181d9..ce7e50dbc 100644
--- a/common/console_common.c
+++ b/common/console_common.c
@@ -362,17 +362,4 @@ int dputs(int fd, const char *s)
 	else
 		return write(fd, s, strlen(s));
 }
-EXPORT_SYMBOL(dputs);
-
-int dputc(int fd, char c)
-{
-	if (fd == 1)
-		putchar(c);
-	else if (fd == 2)
-		console_putc(CONSOLE_STDERR, c);
-	else
-		return write(fd, &c, 1);
-
-	return 0;
-}
-EXPORT_SYMBOL(dputc);
+EXPORT_SYMBOL(dputs);
\ No newline at end of file
diff --git a/include/stdio.h b/include/stdio.h
index d918f3682..3b80b4bd7 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -95,6 +95,5 @@ static inline void putchar(char c)
 
 int dprintf(int file, const char *fmt, ...) __attribute__ ((format(__printf__, 2, 3)));
 int dputs(int file, const char *s);
-int dputc(int file, const char c);
 
 #endif /* __STDIO_H */
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 24/27] console: Simplify dputs()

[Andrey Smirnov]

Knowing that puts() is just a wrapper around a call to
console_puts(CONSOLE_STDOUT, str) we can simplify the code of
dputs(). While at it, make use of *_FILENO constants to get rid of
magic numbers.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console_common.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/common/console_common.c b/common/console_common.c
index ce7e50dbc..6e098b27d 100644
--- a/common/console_common.c
+++ b/common/console_common.c
@@ -355,11 +355,19 @@ EXPORT_SYMBOL(dprintf);
 
 int dputs(int fd, const char *s)
 {
-	if (fd == 1)
-		return puts(s);
-	else if (fd == 2)
-		return console_puts(CONSOLE_STDERR, s);
-	else
+	unsigned int ch;
+
+	switch (fd) {
+	case STDOUT_FILENO:
+		ch = CONSOLE_STDOUT;
+		break;
+	case STDERR_FILENO:
+		ch = CONSOLE_STDERR;
+		break;
+	default:
 		return write(fd, s, strlen(s));
+	}
+
+	return console_puts(ch, s);
 }
 EXPORT_SYMBOL(dputs);
\ No newline at end of file
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 25/27] console: Introduce dvprintf()

[Andrey Smirnov]

In an effort to consolidate various printf re-implementation,
introduce the most generic form of printf() to be used to implement
other flavours of it.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console_common.c | 18 ----------------
 lib/console.c           | 47 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 19 deletions(-)

diff --git a/common/console_common.c b/common/console_common.c
index 6e098b27d..121f511e6 100644
--- a/common/console_common.c
+++ b/common/console_common.c
@@ -209,24 +209,6 @@ void log_print(unsigned flags)
 	}
 }
 
-int vprintf(const char *fmt, va_list args)
-{
-	int i;
-	char printbuffer[CFG_PBSIZE];
-
-	/*
-	 * For this to work, printbuffer must be larger than
-	 * anything we ever want to print.
-	 */
-	i = vsprintf(printbuffer, fmt, args);
-
-	/* Print the string */
-	puts(printbuffer);
-
-	return i;
-}
-EXPORT_SYMBOL(vprintf);
-
 struct console_device *console_get_by_dev(struct device_d *dev)
 {
 	struct console_device *cdev;
diff --git a/lib/console.c b/lib/console.c
index 57c3578d6..43eeb4cdf 100644
--- a/lib/console.c
+++ b/lib/console.c
@@ -237,4 +237,49 @@ int printf(const char *fmt, ...)
 
 	return i;
 }
-EXPORT_SYMBOL(printf);
\ No newline at end of file
+EXPORT_SYMBOL(printf);
+
+/**
+ * dputs() - Dummy dputs() implementation
+ *
+ * @fd:		File descriptor to print to
+ * @s:		String to print
+ *
+ * This is a dummy dputs() implementation intended to be used in PBL
+ * only. CONSOLE_* code overrides it with more sophisticated version
+ * that actually does correct output routing.
+ */
+__weak int dputs(int fd, const char *s)
+{
+	return console_puts(CONSOLE_STDOUT, s);
+}
+
+/**
+ * dvprintf() - A hybrid between dprintf() and vprintf()
+ *
+ * @fd:		File descriptor to print to
+ * @fmt:	Format string to use
+ * @args:	va_list with the rest of the arguments to be used
+ *
+ * This function is a most generic version of printf-family intended
+ * to be used to implement the rest of the printf-like functions
+ */
+static int dvprintf(int fd, const char *fmt, va_list args)
+{
+	char printbuffer[CFG_PBSIZE];
+
+	/*
+	 * For this to work, printbuffer must be larger than
+	 * anything we ever want to print.
+	 */
+	vsprintf(printbuffer, fmt, args);
+
+	/* Print the string */
+	return dputs(fd, printbuffer);
+}
+
+int vprintf(const char *fmt, va_list args)
+{
+	return dvprintf(STDOUT_FILENO, fmt, args);
+}
+EXPORT_SYMBOL(vprintf);
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 26/27] console: Convert printf() into a single line #define

[Andrey Smirnov]

Since printf() is just a special case of dprintf(), we can re-arrange
our codebase to share that definition for dprintf() and have printf()
as a simple marcro expanding into appropriate call to dprintf() thus
allowing us to drop a bit of code.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/console_common.c | 19 -------------------
 include/stdio.h         |  2 +-
 lib/console.c           | 35 +++++++++++++----------------------
 3 files changed, 14 insertions(+), 42 deletions(-)

diff --git a/common/console_common.c b/common/console_common.c
index 121f511e6..8211589b3 100644
--- a/common/console_common.c
+++ b/common/console_common.c
@@ -316,25 +316,6 @@ __weak int getchar(void) __alias(getc_raw);
 
 #endif /* !CONFIG_CONSOLE_NONE */
 
-int dprintf(int file, const char *fmt, ...)
-{
-	va_list args;
-	char printbuffer[CFG_PBSIZE];
-
-	va_start(args, fmt);
-
-	/*
-	 * For this to work, printbuffer must be larger than
-	 * anything we ever want to print.
-	 */
-	vsprintf(printbuffer, fmt, args);
-	va_end(args);
-
-	/* Print the string */
-	return dputs(file, printbuffer);
-}
-EXPORT_SYMBOL(dprintf);
-
 int dputs(int fd, const char *s)
 {
 	unsigned int ch;
diff --git a/include/stdio.h b/include/stdio.h
index 3b80b4bd7..ae3904340 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -62,7 +62,7 @@ static inline int vprintf(const char *fmt, va_list args)
 
 #if (!defined(__PBL__) && !defined(CONFIG_CONSOLE_NONE)) || \
 	(defined(__PBL__) && defined(CONFIG_PBL_CONSOLE))
-int printf(const char *fmt, ...) __attribute__ ((format(__printf__, 1, 2)));
+#define printf(fmt, arg...)	dprintf(STDOUT_FILENO, fmt, ## arg)
 #else
 static int printf(const char *fmt, ...) __attribute__ ((format(__printf__, 1, 2)));
 static inline int printf(const char *fmt, ...)
diff --git a/lib/console.c b/lib/console.c
index 43eeb4cdf..84f2b5126 100644
--- a/lib/console.c
+++ b/lib/console.c
@@ -217,28 +217,6 @@ __weak int ctrlc (void)
 }
 EXPORT_SYMBOL(ctrlc);
 
-int printf(const char *fmt, ...)
-{
-	va_list args;
-	int i;
-	char printbuffer[CFG_PBSIZE];
-
-	va_start(args, fmt);
-
-	/*
-	 * For this to work, printbuffer must be larger than
-	 * anything we ever want to print.
-	 */
-	i = vsprintf(printbuffer, fmt, args);
-	va_end(args);
-
-	/* Print the string */
-	puts(printbuffer);
-
-	return i;
-}
-EXPORT_SYMBOL(printf);
-
 /**
  * dputs() - Dummy dputs() implementation
  *
@@ -283,3 +261,16 @@ int vprintf(const char *fmt, va_list args)
 	return dvprintf(STDOUT_FILENO, fmt, args);
 }
 EXPORT_SYMBOL(vprintf);
+
+int dprintf(int file, const char *fmt, ...)
+{
+	int i;
+	va_list args;
+
+	va_start(args, fmt);
+	i = dvprintf(file, fmt, args);
+	va_end(args);
+
+	return i;
+}
+EXPORT_SYMBOL(dprintf);
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 27/27] psci: console: Convert to use lib/console.c

[Andrey Smirnov]

Convert PSCI debug facilities to use shared code in lib/console.c,
similar to what's been done to PBL_CONSOLE.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/cpu/psci.c         | 34 +++++++---------------------------
 arch/arm/include/asm/psci.h | 11 -----------
 2 files changed, 7 insertions(+), 38 deletions(-)

diff --git a/arch/arm/cpu/psci.c b/arch/arm/cpu/psci.c
index ad132c0a3..435ec92cc 100644
--- a/arch/arm/cpu/psci.c
+++ b/arch/arm/cpu/psci.c
@@ -33,35 +33,15 @@
  * warned.
  */
 
-static putc_func_t __putc;
-static void *putc_ctx;
+static void __console_psci_putc(struct console_device *cdev, char c) {}
 
-void psci_set_putc(putc_func_t putcf, void *ctx)
-{
-        __putc = putcf;
-        putc_ctx = ctx;
-}
-
-void psci_putc(char c)
-{
-	if (__putc)
-		__putc(putc_ctx, c);
-}
+static struct console_device console_psci = {
+	.putc = __console_psci_putc,
+};
 
-int psci_puts(const char *str)
+void psci_set_putc(putc_func_t putcf, void *ctx)
 {
-	int n = 0;
-
-	while (*str) {
-		if (*str == '\n')
-			psci_putc('\r');
-
-		psci_putc(*str);
-		str++;
-		n++;
-	}
-
-	return n;
+	__console_set_putc(&console_psci, putcf, ctx);
 }
 
 int psci_printf(const char *fmt, ...)
@@ -74,7 +54,7 @@ int psci_printf(const char *fmt, ...)
 	i = vsprintf(printbuffer, fmt, args);
 	va_end(args);
 
-	psci_puts(printbuffer);
+	__console_puts(&console_psci, printbuffer);
 
         return i;
 }
diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
index e0c452538..d2b40907f 100644
--- a/arch/arm/include/asm/psci.h
+++ b/arch/arm/include/asm/psci.h
@@ -110,8 +110,6 @@ void psci_cpu_entry(void);
 
 #ifdef CONFIG_ARM_PSCI_DEBUG
 void psci_set_putc(void (*putcf)(void *ctx, int c), void *ctx);
-void psci_putc(char c);
-int psci_puts(const char *str);
 int psci_printf(const char *fmt, ...)
             __attribute__ ((format(__printf__, 1, 2)));
 #else
@@ -120,15 +118,6 @@ static inline void psci_set_putc(void (*putcf)(void *ctx, int c), void *ctx)
 {
 }
 
-static inline void psci_putc(char c)
-{
-}
-
-static inline int psci_puts(const char *str)
-{
-	return 0;
-}
-
 static inline int psci_printf(const char *fmt, ...)
 {
 	return 0;
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 00/27] Console code consolidation

[Sascha Hauer]

Hi Andrey,

On Thu, Jun 14, 2018 at 09:11:06PM -0700, Andrey Smirnov wrote:
> Everyone:
> 
> While debugging the reason behind print_hex_dump() not producing
> carriage return properly, when used in PBL, I realised that current
> codebase contained:
> 
>  - at least 5 places where '\n' was replaced with '\n\r'
>  - at least 3 almost identical implementations of puts()
>  - at least 3 almost identical implementations of printf()
> 
> so this patcheset is an attempt to consolidate, share and simplify
> console related code.

The console support really deserves some cleanup. We have the LL console
support, PBL console support, regular console support and simple console
support, all mixed into a single codebase so that it's sometimes really
hard to understand what is going on.

Instead of optimizing the different variants for better code sharing I
wonder if we could not consolidate some of the console types to reduce
the number of variants in the code.  PBL console works by calling
pbl_set_putc() to specify a putc function.  PUTC_LL instead works by
putting a PUTC_LL function into a SoC specific header function. Instead
each board could provide its own putc function, say board_putc() or so.
This would be enough to replace the DEBUG_LL and the PBL console
support.

Then I don't like weak functions. It can provide nifty solutions to some
problems, but I think it also often leads to situations where you don't
really know if something has been overwritten or with what is has been
overwritten with. I don't consider replacing ifdefs with weak functions
a general improvment.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 00/27] Console code consolidation

[Andrey Smirnov]

On Fri, Jun 15, 2018 at 2:28 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> Hi Andrey,
>
> On Thu, Jun 14, 2018 at 09:11:06PM -0700, Andrey Smirnov wrote:
> > Everyone:
> >
> > While debugging the reason behind print_hex_dump() not producing
> > carriage return properly, when used in PBL, I realised that current
> > codebase contained:
> >
> >  - at least 5 places where '\n' was replaced with '\n\r'
> >  - at least 3 almost identical implementations of puts()
> >  - at least 3 almost identical implementations of printf()
> >
> > so this patcheset is an attempt to consolidate, share and simplify
> > console related code.
>
> The console support really deserves some cleanup. We have the LL console
> support, PBL console support, regular console support and simple console
> support, all mixed into a single codebase so that it's sometimes really
> hard to understand what is going on.
>
> Instead of optimizing the different variants for better code sharing I
> wonder if we could not consolidate some of the console types to reduce
> the number of variants in the code.  PBL console works by calling
> pbl_set_putc() to specify a putc function.  PUTC_LL instead works by
> putting a PUTC_LL function into a SoC specific header function. Instead
> each board could provide its own putc function, say board_putc() or so.
> This would be enough to replace the DEBUG_LL and the PBL console
> support.
>

- That still leaves psci_set_putc(), which currently is handled the
same way pbl_set_putc() does
- Dropping pbl_set_putc(), would require making direct changes to the
code for boards I don't have access to (as opposed to indirect API
changes that I can test with on boards that I do have)

IMHO, what you are proposing is orthogonal to the work in this
patchset. One can unify PUTC_LL and pbl_set_putc() usecases, but it
wouldn't change the fact that PBL code has it's own, yet another,
implementation of puts() and printf().

I guess what I am saying is that I don't see a reason why one has to
be done _instead_ of the other and simplification that you propose
could (and IMHO should) be done on top of this work.

More importantly, I think that that work is really outside of the
scope of this patch. I am happy to bring this patchset over the finish
line and get it into merge-able state, but I already spent as much
time as I could on this, so any further improvement might have to wait
until some other time or some other developer.

> Then I don't like weak functions. It can provide nifty solutions to some
> problems, but I think it also often leads to situations where you don't
> really know if something has been overwritten or with what is has been
> overwritten with.

And with #ifdefs you do? I regularly find myself having to inject
#error statements or look at the final disassembly to make sure I
interpret the preprocessor logic right, but that might be my unique
experience.

> I don't consider replacing ifdefs with weak functions
> a general improvment.
>

OK, do you want me to get rid of all of the uses of __weak in this
patch set or does your comment apply only to "console: Drop
ARCH_HAS_CTRLC"?

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 00/27] Console code consolidation

[Sascha Hauer]

On Fri, Jun 15, 2018 at 05:11:17AM -0700, Andrey Smirnov wrote:
> On Fri, Jun 15, 2018 at 2:28 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > Hi Andrey,
> >
> > On Thu, Jun 14, 2018 at 09:11:06PM -0700, Andrey Smirnov wrote:
> > > Everyone:
> > >
> > > While debugging the reason behind print_hex_dump() not producing
> > > carriage return properly, when used in PBL, I realised that current
> > > codebase contained:
> > >
> > >  - at least 5 places where '\n' was replaced with '\n\r'
> > >  - at least 3 almost identical implementations of puts()
> > >  - at least 3 almost identical implementations of printf()
> > >
> > > so this patcheset is an attempt to consolidate, share and simplify
> > > console related code.
> >
> > The console support really deserves some cleanup. We have the LL console
> > support, PBL console support, regular console support and simple console
> > support, all mixed into a single codebase so that it's sometimes really
> > hard to understand what is going on.
> >
> > Instead of optimizing the different variants for better code sharing I
> > wonder if we could not consolidate some of the console types to reduce
> > the number of variants in the code.  PBL console works by calling
> > pbl_set_putc() to specify a putc function.  PUTC_LL instead works by
> > putting a PUTC_LL function into a SoC specific header function. Instead
> > each board could provide its own putc function, say board_putc() or so.
> > This would be enough to replace the DEBUG_LL and the PBL console
> > support.
> >
> 
> - That still leaves psci_set_putc(), which currently is handled the
> same way pbl_set_putc() does
> - Dropping pbl_set_putc(), would require making direct changes to the
> code for boards I don't have access to (as opposed to indirect API
> changes that I can test with on boards that I do have)
> 
> IMHO, what you are proposing is orthogonal to the work in this
> patchset. One can unify PUTC_LL and pbl_set_putc() usecases, but it
> wouldn't change the fact that PBL code has it's own, yet another,
> implementation of puts() and printf().

Ok, I buy this argument.

> > Then I don't like weak functions. It can provide nifty solutions to some
> > problems, but I think it also often leads to situations where you don't
> > really know if something has been overwritten or with what is has been
> > overwritten with.
> 
> And with #ifdefs you do? I regularly find myself having to inject
> #error statements or look at the final disassembly to make sure I
> interpret the preprocessor logic right, but that might be my unique
> experience.

I know this problem and in fact it was one problem that has driven me
away from U-Boot. No, you are right, #fdefs are not better than weak
functions. I just have the feeling that allowing weak functions is yet
another can of worms, also admittedly a less ugly one than #ifdefs.

> 
> OK, do you want me to get rid of all of the uses of __weak in this
> patch set or does your comment apply only to "console: Drop
> ARCH_HAS_CTRLC"?

Leave them in for now. So far I haven't seen the major selling point for
this series. It makes some things better, but others just look
different.

One thing I noticed is that the resulting barebox.bin for
imx_v7_defconfig gets 5k bigger with this series. I have no idea why
this happens, nothing obvious pops up, but that's not what I expect
from a series which tries to consolidate things.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 00/27] Console code consolidation

[Andrey Smirnov]

On Mon, Jun 18, 2018 at 1:49 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Fri, Jun 15, 2018 at 05:11:17AM -0700, Andrey Smirnov wrote:
> > On Fri, Jun 15, 2018 at 2:28 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >
> > > Hi Andrey,
> > >
> > > On Thu, Jun 14, 2018 at 09:11:06PM -0700, Andrey Smirnov wrote:
> > > > Everyone:
> > > >
> > > > While debugging the reason behind print_hex_dump() not producing
> > > > carriage return properly, when used in PBL, I realised that current
> > > > codebase contained:
> > > >
> > > >  - at least 5 places where '\n' was replaced with '\n\r'
> > > >  - at least 3 almost identical implementations of puts()
> > > >  - at least 3 almost identical implementations of printf()
> > > >
> > > > so this patcheset is an attempt to consolidate, share and simplify
> > > > console related code.
> > >
> > > The console support really deserves some cleanup. We have the LL console
> > > support, PBL console support, regular console support and simple console
> > > support, all mixed into a single codebase so that it's sometimes really
> > > hard to understand what is going on.
> > >
> > > Instead of optimizing the different variants for better code sharing I
> > > wonder if we could not consolidate some of the console types to reduce
> > > the number of variants in the code.  PBL console works by calling
> > > pbl_set_putc() to specify a putc function.  PUTC_LL instead works by
> > > putting a PUTC_LL function into a SoC specific header function. Instead
> > > each board could provide its own putc function, say board_putc() or so.
> > > This would be enough to replace the DEBUG_LL and the PBL console
> > > support.
> > >
> >
> > - That still leaves psci_set_putc(), which currently is handled the
> > same way pbl_set_putc() does
> > - Dropping pbl_set_putc(), would require making direct changes to the
> > code for boards I don't have access to (as opposed to indirect API
> > changes that I can test with on boards that I do have)
> >
> > IMHO, what you are proposing is orthogonal to the work in this
> > patchset. One can unify PUTC_LL and pbl_set_putc() usecases, but it
> > wouldn't change the fact that PBL code has it's own, yet another,
> > implementation of puts() and printf().
>
> Ok, I buy this argument.
>
> > > Then I don't like weak functions. It can provide nifty solutions to some
> > > problems, but I think it also often leads to situations where you don't
> > > really know if something has been overwritten or with what is has been
> > > overwritten with.
> >
> > And with #ifdefs you do? I regularly find myself having to inject
> > #error statements or look at the final disassembly to make sure I
> > interpret the preprocessor logic right, but that might be my unique
> > experience.
>
> I know this problem and in fact it was one problem that has driven me
> away from U-Boot. No, you are right, #fdefs are not better than weak
> functions. I just have the feeling that allowing weak functions is yet
> another can of worms, also admittedly a less ugly one than #ifdefs.
>
> >
> > OK, do you want me to get rid of all of the uses of __weak in this
> > patch set or does your comment apply only to "console: Drop
> > ARCH_HAS_CTRLC"?
>
> Leave them in for now. So far I haven't seen the major selling point for
> this series. It makes some things better, but others just look
> different.

OK, understood. In that case, IMHO, 27 patches is too much to be
merged in without any major selling point. Keeping things status quo
seems like the best approach in terms of risk and saving both time
you'd have to spend reviewing and I making the v2. Let's drop this
set.

Please do consider applying "console: Fix console_get_first_active()"
since it fixes an actual bug in console_get_first_active().

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 00/27] Console code consolidation

[Sascha Hauer]

On Mon, Jun 18, 2018 at 04:26:28PM -0700, Andrey Smirnov wrote:
> On Mon, Jun 18, 2018 at 1:49 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > On Fri, Jun 15, 2018 at 05:11:17AM -0700, Andrey Smirnov wrote:
> > Leave them in for now. So far I haven't seen the major selling point for
> > this series. It makes some things better, but others just look
> > different.
> 
> OK, understood. In that case, IMHO, 27 patches is too much to be
> merged in without any major selling point. Keeping things status quo
> seems like the best approach in terms of risk and saving both time
> you'd have to spend reviewing and I making the v2. Let's drop this
> set.
> 
> Please do consider applying "console: Fix console_get_first_active()"
> since it fixes an actual bug in console_get_first_active().

Ok, did that. I just sent out a series that fixes some actual bugs you
might have stumbled upon when creating your series, you might want to
have a look.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox