[PATCH 0/4] Initialize the console as early as possible

[PATCH 0/4] Initialize the console as early as possible

[Xiang W]

Initializing the console as early as possible can output more useful
information. This series of patches is used to initialize the console
as early as possible.

This series of patches is based on a previous patch
http://lists.infradead.org/pipermail/opensbi/2024-May/006960.html

Xiang W (4):
  include: sbi_utils: fixup fdt_get_address
  lib: sbi: Add domains_init to sbi_console_device
  lib: utils/serial: add domains_init for 8250
  lib: sbi: Initialize the console as early as possible

 include/sbi/sbi_console.h           |  3 +++
 include/sbi_utils/fdt/fdt_helper.h  |  4 +++-
 include/sbi_utils/serial/uart8250.h |  1 +
 lib/sbi/sbi_domain.c                | 10 ++++++++++
 lib/sbi/sbi_init.c                  |  8 ++++----
 lib/utils/serial/uart8250.c         | 28 +++++++++++++++++++---------
 6 files changed, 40 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH 1/4] include: sbi_utils: fixup fdt_get_address

[Xiang W]

root.next_arg1 will be NULL before sbi_domain_init, which will cause
fdt address to be NULL before sbi_domain is initialized. This patch
fixes this.

Signed-off-by: Xiang W <wxjstz@126.com>
---
 include/sbi_utils/fdt/fdt_helper.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/sbi_utils/fdt/fdt_helper.h b/include/sbi_utils/fdt/fdt_helper.h
index ab4a80f..67e6dc7 100644
--- a/include/sbi_utils/fdt/fdt_helper.h
+++ b/include/sbi_utils/fdt/fdt_helper.h
@@ -116,7 +116,9 @@ int fdt_parse_compat_addr(void *fdt, uint64_t *addr,
 
 static inline void *fdt_get_address(void)
 {
-	return (void *)root.next_arg1;
+	if (root.next_arg1)
+		return (void *)root.next_arg1;
+	return sbi_scratch_thishart_arg1_ptr();
 }
 
 #endif /* __FDT_HELPER_H__ */
-- 
2.43.0

[PATCH 2/4] lib: sbi: Add domains_init to sbi_console_device

[Xiang W]

To initialize the console before the domain is initialized, an
interface needs to be reserved for adding the mem region to the
root domain after the console is initialized.

Signed-off-by: Xiang W <wxjstz@126.com>
---
 include/sbi/sbi_console.h |  3 +++
 lib/sbi/sbi_domain.c      | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h
index 0979765..5c7eae7 100644
--- a/include/sbi/sbi_console.h
+++ b/include/sbi/sbi_console.h
@@ -24,6 +24,9 @@ struct sbi_console_device {
 
 	/** Read a character from the console input */
 	int (*console_getc)(void);
+
+	/* This function is used to add memregion to the root domain. */
+	int (*domains_init)(void);
 };
 
 #define __printf(a, b) __attribute__((format(printf, a, b)))
diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index 374ac36..eb392b7 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -696,6 +696,16 @@ int sbi_domain_finalize(struct sbi_scratch *scratch, u32 cold_hartid)
 	struct sbi_domain *dom;
 	const struct sbi_platform *plat = sbi_platform_ptr(scratch);
 
+	/* Initialize and populate domains for the console */
+	if (sbi_console_get_device()->domains_init) {
+		rc = sbi_console_get_device()->domains_init();
+		if (rc) {
+			sbi_printf("%s: console domains_init() failed (error %d)\n",
+			   __func__, rc);
+			return rc;
+		}
+	}
+
 	/* Initialize and populate domains for the platform */
 	rc = sbi_platform_domains_init(plat);
 	if (rc) {
-- 
2.43.0

[PATCH 3/4] lib: utils/serial: add domains_init for 8250

[Xiang W]

8250 needs to add memregion to root_domain. Move this part of
operation to sbi_console_device.domains_init

Signed-off-by: Xiang W <wxjstz@126.com>
---
 include/sbi_utils/serial/uart8250.h |  1 +
 lib/utils/serial/uart8250.c         | 28 +++++++++++++++++++---------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/include/sbi_utils/serial/uart8250.h b/include/sbi_utils/serial/uart8250.h
index be9483a..0949b96 100644
--- a/include/sbi_utils/serial/uart8250.h
+++ b/include/sbi_utils/serial/uart8250.h
@@ -18,6 +18,7 @@ struct uart8250_device {
 	u32 baudrate;
 	u32 reg_shift;
 	u32 reg_width;
+	u32 reg_offset;
 };
 
 void uart8250_putc(struct uart8250_device *dev, char ch);
diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c
index db35ec3..754548f 100644
--- a/lib/utils/serial/uart8250.c
+++ b/lib/utils/serial/uart8250.c
@@ -92,10 +92,21 @@ static int uart8250_console_getc(void)
 	return uart8250_getc(&console_dev);
 }
 
+static int uart8250_domains_init(void)
+{
+	unsigned long base = (unsigned long)console_dev.base
+					- console_dev.reg_offset;
+
+	return sbi_domain_root_add_memrange(base, PAGE_SIZE, PAGE_SIZE,
+					(SBI_DOMAIN_MEMREGION_MMIO |
+					SBI_DOMAIN_MEMREGION_SHARED_SURW_MRW));
+}
+
 static struct sbi_console_device uart8250_console = {
 	.name = "uart8250",
 	.console_putc = uart8250_console_putc,
-	.console_getc = uart8250_console_getc
+	.console_getc = uart8250_console_getc,
+	.domains_init = uart8250_domains_init
 };
 
 int uart8250_init(struct uart8250_device * dev, unsigned long base, u32 in_freq,
@@ -103,11 +114,12 @@ int uart8250_init(struct uart8250_device * dev, unsigned long base, u32 in_freq,
 {
 	u16 bdiv = 0;
 
-	dev->base      = (volatile char *)base + reg_offset;
-	dev->reg_shift = reg_shift;
-	dev->reg_width = reg_width;
-	dev->in_freq   = in_freq;
-	dev->baudrate  = baudrate;
+	dev->base       = (volatile char *)base + reg_offset;
+	dev->reg_offset = reg_offset;
+	dev->reg_shift  = reg_shift;
+	dev->reg_width  = reg_width;
+	dev->in_freq    = in_freq;
+	dev->baudrate   = baudrate;
 
 	if (baudrate)
 		bdiv = (in_freq + 8 * baudrate) / (16 * baudrate);
@@ -147,7 +159,5 @@ int uart8250_console_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg
 
 	sbi_console_set_device(&uart8250_console);
 
-	return sbi_domain_root_add_memrange(base, PAGE_SIZE, PAGE_SIZE,
-					    (SBI_DOMAIN_MEMREGION_MMIO |
-					    SBI_DOMAIN_MEMREGION_SHARED_SURW_MRW));
+	return 0;
 }
-- 
2.43.0

[PATCH 4/4] lib: sbi: Initialize the console as early as possible

[Xiang W]

Move console initialization to the front of init_coldboot so that
more information can be output

Signed-off-by: Xiang W <wxjstz@126.com>
---
 lib/sbi/sbi_init.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
index 389172a..b14cd03 100644
--- a/lib/sbi/sbi_init.c
+++ b/lib/sbi/sbi_init.c
@@ -214,6 +214,10 @@ static void __noreturn init_coldboot(struct sbi_scratch *scratch, u32 hartid)
 	unsigned long *count;
 	const struct sbi_platform *plat = sbi_platform_ptr(scratch);
 
+	rc = sbi_console_init(scratch);
+	if (rc)
+		sbi_hart_hang();
+
 	/* Note: This has to be first thing in coldboot init sequence */
 	rc = sbi_scratch_init(scratch);
 	if (rc)
@@ -260,10 +264,6 @@ static void __noreturn init_coldboot(struct sbi_scratch *scratch, u32 hartid)
 	if (rc)
 		sbi_hart_hang();
 
-	rc = sbi_console_init(scratch);
-	if (rc)
-		sbi_hart_hang();
-
 	rc = sbi_sse_init(scratch, true);
 	if (rc) {
 		sbi_printf("%s: sse init failed (error %d)\n", __func__, rc);
-- 
2.43.0

[PATCH 0/4] Initialize the console as early as possible

[Anup Patel]

On Thu, Jun 6, 2024 at 2:47?PM Xiang W <wxjstz@126.com> wrote:
>
> Initializing the console as early as possible can output more useful
> information. This series of patches is used to initialize the console
> as early as possible.
>
> This series of patches is based on a previous patch
> http://lists.infradead.org/pipermail/opensbi/2024-May/006960.html
>
> Xiang W (4):
>   include: sbi_utils: fixup fdt_get_address
>   lib: sbi: Add domains_init to sbi_console_device
>   lib: utils/serial: add domains_init for 8250
>   lib: sbi: Initialize the console as early as possible

The problem with this series is that it is changing the order
of platform callback in coldboot path because sbi_scratch_init,
sbi_heap_init, and sbi_domain_init should always be the
first things done in the coldboot path.

Instead, I suggest the following:
1) Update generic platform to call generic_console_init()
     from generic_early_init() in coldboot path and don't
     provide the console_init() callback.
2) Update all other platform to do similar thing as #1
3) Drop sbi_console_init(), sbi_platform_console_init() and
    console_init() callback.

For prints before sbi_platform_early_init(), the sbi_console.c
can implement a small circular buffer which is flushed whenever
some driver sets a console device.

>
>  include/sbi/sbi_console.h           |  3 +++
>  include/sbi_utils/fdt/fdt_helper.h  |  4 +++-
>  include/sbi_utils/serial/uart8250.h |  1 +
>  lib/sbi/sbi_domain.c                | 10 ++++++++++
>  lib/sbi/sbi_init.c                  |  8 ++++----
>  lib/utils/serial/uart8250.c         | 28 +++++++++++++++++++---------
>  6 files changed, 40 insertions(+), 14 deletions(-)
>
> --
> 2.43.0
>

Regards,
Anup

[PATCH 0/4] Initialize the console as early as possible

[Andrew Jones]

On Fri, Jun 21, 2024 at 05:37:07PM GMT, Anup Patel wrote:
> On Thu, Jun 6, 2024 at 2:47?PM Xiang W <wxjstz@126.com> wrote:
> >
> > Initializing the console as early as possible can output more useful
> > information. This series of patches is used to initialize the console
> > as early as possible.
> >
> > This series of patches is based on a previous patch
> > http://lists.infradead.org/pipermail/opensbi/2024-May/006960.html
> >
> > Xiang W (4):
> >   include: sbi_utils: fixup fdt_get_address
> >   lib: sbi: Add domains_init to sbi_console_device
> >   lib: utils/serial: add domains_init for 8250
> >   lib: sbi: Initialize the console as early as possible
> 
> The problem with this series is that it is changing the order
> of platform callback in coldboot path because sbi_scratch_init,
> sbi_heap_init, and sbi_domain_init should always be the
> first things done in the coldboot path.
> 
> Instead, I suggest the following:
> 1) Update generic platform to call generic_console_init()
>      from generic_early_init() in coldboot path and don't
>      provide the console_init() callback.
> 2) Update all other platform to do similar thing as #1
> 3) Drop sbi_console_init(), sbi_platform_console_init() and
>     console_init() callback.
> 
> For prints before sbi_platform_early_init(), the sbi_console.c
> can implement a small circular buffer which is flushed whenever
> some driver sets a console device.

Almost two years ago I wrote an SBI console ring buffer patch. I never
polished and posted it, but I could maybe dig it back up.

Thanks,
drew

> 
> >
> >  include/sbi/sbi_console.h           |  3 +++
> >  include/sbi_utils/fdt/fdt_helper.h  |  4 +++-
> >  include/sbi_utils/serial/uart8250.h |  1 +
> >  lib/sbi/sbi_domain.c                | 10 ++++++++++
> >  lib/sbi/sbi_init.c                  |  8 ++++----
> >  lib/utils/serial/uart8250.c         | 28 +++++++++++++++++++---------
> >  6 files changed, 40 insertions(+), 14 deletions(-)
> >
> > --
> > 2.43.0
> >
> 
> Regards,
> Anup
> 
> -- 
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 0/4] Initialize the console as early as possible

[Xiang W]

? 2024-06-21???? 16:02 +0200?Andrew Jones???
> On Fri, Jun 21, 2024 at 05:37:07PM GMT, Anup Patel wrote:
> > On Thu, Jun 6, 2024 at 2:47?PM Xiang W <wxjstz@126.com> wrote:
> > > 
> > > Initializing the console as early as possible can output more useful
> > > information. This series of patches is used to initialize the console
> > > as early as possible.
> > > 
> > > This series of patches is based on a previous patch
> > > http://lists.infradead.org/pipermail/opensbi/2024-May/006960.html
> > > 
> > > Xiang W (4):
> > > ? include: sbi_utils: fixup fdt_get_address
> > > ? lib: sbi: Add domains_init to sbi_console_device
> > > ? lib: utils/serial: add domains_init for 8250
> > > ? lib: sbi: Initialize the console as early as possible
> > 
> > The problem with this series is that it is changing the order
> > of platform callback in coldboot path because sbi_scratch_init,
> > sbi_heap_init, and sbi_domain_init should always be the
> > first things done in the coldboot path.
> > 
> > Instead, I suggest the following:
> > 1) Update generic platform to call generic_console_init()
> > ???? from generic_early_init() in coldboot path and don't
> > ???? provide the console_init() callback.
> > 2) Update all other platform to do similar thing as #1
> > 3) Drop sbi_console_init(), sbi_platform_console_init() and
> > ??? console_init() callback.
> > 
> > For prints before sbi_platform_early_init(), the sbi_console.c
> > can implement a small circular buffer which is flushed whenever
> > some driver sets a console device.
> 
> Almost two years ago I wrote an SBI console ring buffer patch. I never
> polished and posted it, but I could maybe dig it back up.

waitting for your patch.

Regards,
Xiang W
> 
> Thanks,
> drew
> 
> > 
> > > 
> > > ?include/sbi/sbi_console.h?????????? |? 3 +++
> > > ?include/sbi_utils/fdt/fdt_helper.h? |? 4 +++-
> > > ?include/sbi_utils/serial/uart8250.h |? 1 +
> > > ?lib/sbi/sbi_domain.c??????????????? | 10 ++++++++++
> > > ?lib/sbi/sbi_init.c????????????????? |? 8 ++++----
> > > ?lib/utils/serial/uart8250.c???????? | 28 +++++++++++++++++++---------
> > > ?6 files changed, 40 insertions(+), 14 deletions(-)
> > > 
> > > --
> > > 2.43.0
> > > 
> > 
> > Regards,
> > Anup
> > 
> > -- 
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 0/4] Initialize the console as early as possible

[Xiang W]

? 2024-06-21???? 17:37 +0530?Anup Patel???
> On Thu, Jun 6, 2024 at 2:47?PM Xiang W <wxjstz@126.com> wrote:
> > 
> > Initializing the console as early as possible can output more useful
> > information. This series of patches is used to initialize the console
> > as early as possible.
> > 
> > This series of patches is based on a previous patch
> > http://lists.infradead.org/pipermail/opensbi/2024-May/006960.html
> > 
> > Xiang W (4):
> > ? include: sbi_utils: fixup fdt_get_address
> > ? lib: sbi: Add domains_init to sbi_console_device
> > ? lib: utils/serial: add domains_init for 8250
> > ? lib: sbi: Initialize the console as early as possible
> 
> The problem with this series is that it is changing the order
> of platform callback in coldboot path because sbi_scratch_init,
> sbi_heap_init, and sbi_domain_init should always be the
> first things done in the coldboot path.
> 
> Instead, I suggest the following:
> 1) Update generic platform to call generic_console_init()
> ???? from generic_early_init() in coldboot path and don't
> ???? provide the console_init() callback.
> 2) Update all other platform to do similar thing as #1
> 3) Drop sbi_console_init(), sbi_platform_console_init() and
> ??? console_init() callback.
> 
> For prints before sbi_platform_early_init(), the sbi_console.c
> can implement a small circular buffer which is flushed whenever
> some driver sets a console device.
> 

If there is a buffer to cache the earlier print, I think it is possible
not to move the console initialization forward. But we need to try to call
console_init to output the contents of the buffer when an error occurs.

Regards,
Xiang W
> > 
> > ?include/sbi/sbi_console.h?????????? |? 3 +++
> > ?include/sbi_utils/fdt/fdt_helper.h? |? 4 +++-
> > ?include/sbi_utils/serial/uart8250.h |? 1 +
> > ?lib/sbi/sbi_domain.c??????????????? | 10 ++++++++++
> > ?lib/sbi/sbi_init.c????????????????? |? 8 ++++----
> > ?lib/utils/serial/uart8250.c???????? | 28 +++++++++++++++++++---------
> > ?6 files changed, 40 insertions(+), 14 deletions(-)
> > 
> > --
> > 2.43.0
> > 
> 
> Regards,
> Anup