Misc driver improvements

PATCH Superseded Anup Patel 2 participants 11 messages Jan 8, 2022 – Jan 9, 2022

lore

Share & tools

lore

Anup Patel

author

2022-01-08 04:03 UTC #0 lore raw

cover Misc driver improvements

This series does improvements/fixes to HTIF driver and ACLINT MSWI driver.

These patches can be found in driver_imp_v1 branch at:
https://github.com/avpatel/opensbi.git

Anup Patel (2):
lib: utils/ipi: Fix size check in aclint_mswi_cold_init()
lib: utils/sys: Extend HTIF library to allow custom base address

 include/sbi_utils/sys/htif.h       |  4 +-
 lib/utils/ipi/aclint_mswi.c        |  2 +-
 lib/utils/reset/fdt_reset_htif.c   |  8 +++-
 lib/utils/serial/fdt_serial_htif.c |  8 +++-
 lib/utils/sys/htif.c               | 74 ++++++++++++++++++++++++++----
 5 files changed, 81 insertions(+), 15 deletions(-)

Signature · 1 line

2.25.1

Anup Patel

author

2022-01-08 04:03 UTC #1 lore raw

PATCH 1/2 lib: utils/ipi: Fix size check in aclint_mswi_cold_init()

Currently, the ACLINT MSWI size check is forcing size to be at least 0x4000. This is inappropriate check because most systems will never utilize full 16KB for a single ACLINT MSWI device so instead we should check that ACLINT MSWI size is enough for on the associated HARTs.

Signed-off-by Anup Patel <[email protected]>

---
 lib/utils/ipi/aclint_mswi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/utils/ipi/aclint_mswi.c +1 -1

index a3de2f5..832e223 100644
@@ -74,7 +74,7 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
 
 	/* Sanity checks */
 	if (!mswi || (mswi->addr & (ACLINT_MSWI_ALIGN - 1)) ||
-	    (mswi->size < ACLINT_MSWI_SIZE) ||
+	    (mswi->size < (mswi->hart_count * sizeof(u32))) ||
 	    (mswi->first_hartid >= SBI_HARTMASK_MAX_BITS) ||
 	    (mswi->hart_count > ACLINT_MSWI_MAX_HARTS))
 		return SBI_EINVAL;

Signature · 1 line

2.25.1

Dong Du

2022-01-08 08:34 UTC #2 lore raw

PATCH 1/2 lib: utils/ipi: Fix size check in aclint_mswi_cold_init()

Quoted 15 lines · Anup Patel apatel at ventanamicro.com

Currently, the ACLINT MSWI size check is forcing size to be at least 0x4000. This is inappropriate check because most systems will never utilize full 16KB for a single ACLINT MSWI device so instead we should check that ACLINT MSWI size is enough for on the associated HARTs.
Signed-off-by Anup Patel <[email protected]>
---
lib/utils/ipi/aclint_mswi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
lib/utils/ipi/aclint_mswi.c +0 -0
index a3de2f5..832e223 100644

lib/utils/ipi/aclint_mswi.c int aclint_mswi_cold_init(struct aclint_mswi_data *mswi) · line 77

Quoted 5 lines

lib/utils/ipi/aclint_mswi.c +1 -1

@@ -74,7 +74,7 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
 
 	/* Sanity checks */
 	if (!mswi || (mswi->addr & (ACLINT_MSWI_ALIGN - 1)) ||
-	    (mswi->size < ACLINT_MSWI_SIZE) ||
+	    (mswi->size < (mswi->hart_count * sizeof(u32))) ||

Hi Anup,

Shouldn't we also change ACLINT_MSWI_SIZE in lib/utils/ipi/fdt_ipi_mswi.c?

Regards,
Dong

Quoted 11 lines

lib/utils/ipi/aclint_mswi.c +0 -1
 	    (mswi->first_hartid >= SBI_HARTMASK_MAX_BITS) ||
 	    (mswi->hart_count > ACLINT_MSWI_MAX_HARTS))
 		return SBI_EINVAL;
--
2.25.1
--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Anup Patel

author

2022-01-08 11:35 UTC #3 lore raw

PATCH 1/2 lib: utils/ipi: Fix size check in aclint_mswi_cold_init()

Quoted 21 lines · Dong Du

Quoted 16 lines · Anup Patel apatel at ventanamicro.com
Currently, the ACLINT MSWI size check is forcing size to be at least 0x4000. This is inappropriate check because most systems will never utilize full 16KB for a single ACLINT MSWI device so instead we should check that ACLINT MSWI size is enough for on the associated HARTs.
Signed-off-by Anup Patel <[email protected]>
---
lib/utils/ipi/aclint_mswi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
lib/utils/ipi/aclint_mswi.c +0 -0
index a3de2f5..832e223 100644
@@ -74,7 +74,7 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
 

Quoted 8 lines

lib/utils/ipi/aclint_mswi.c int aclint_mswi_cold_init(struct aclint_mswi_data *mswi) · line 77
Quoted 4 lines
lib/utils/ipi/aclint_mswi.c +1 -1
      /* Sanity checks */
      if (!mswi || (mswi->addr & (ACLINT_MSWI_ALIGN - 1)) ||
-         (mswi->size < ACLINT_MSWI_SIZE) ||
+         (mswi->size < (mswi->hart_count * sizeof(u32))) ||
Hi Anup,
Shouldn't we also change ACLINT_MSWI_SIZE in lib/utils/ipi/fdt_ipi_mswi.c?

ACLINT_MSWI_SIZE represents the upper-limit because one MSWI device can be associated with up to 4095 HARTs.

I think we also should fail on "mswi->size > ACLINT_MSWI_SIZE".

Regards,
Anup

Quoted 15 lines

Regards,
Dong
Quoted 11 lines
lib/utils/ipi/aclint_mswi.c +0 -1
          (mswi->first_hartid >= SBI_HARTMASK_MAX_BITS) ||
          (mswi->hart_count > ACLINT_MSWI_MAX_HARTS))
              return SBI_EINVAL;
--
2.25.1
--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Dong Du

2022-01-08 14:50 UTC #4 lore raw

PATCH 1/2 lib: utils/ipi: Fix size check in aclint_mswi_cold_init()

Quoted 29 lines · Anup Patel apatel at ventanamicro.com

Quoted 28 lines · Dong Du
Quoted 15 lines · Anup Patel apatel at ventanamicro.com
Currently, the ACLINT MSWI size check is forcing size to be at least 0x4000. This is inappropriate check because most systems will never utilize full 16KB for a single ACLINT MSWI device so instead we should check that ACLINT MSWI size is enough for on the associated HARTs.
Signed-off-by Anup Patel <[email protected]>
---
lib/utils/ipi/aclint_mswi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
lib/utils/ipi/aclint_mswi.c +0 -0
index a3de2f5..832e223 100644
lib/utils/ipi/aclint_mswi.c int aclint_mswi_cold_init(struct aclint_mswi_data *mswi) · line 77
Quoted 5 lines
lib/utils/ipi/aclint_mswi.c +1 -1
@@ -74,7 +74,7 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
 
      /* Sanity checks */
      if (!mswi || (mswi->addr & (ACLINT_MSWI_ALIGN - 1)) ||
-         (mswi->size < ACLINT_MSWI_SIZE) ||
+         (mswi->size < (mswi->hart_count * sizeof(u32))) ||
Hi Anup,

Quoted 6 lines

Quoted 1 line
Shouldn't we also change ACLINT_MSWI_SIZE in lib/utils/ipi/fdt_ipi_mswi.c?
ACLINT_MSWI_SIZE represents the upper-limit because one MSWI device can be associated with up to 4095 HARTs.
I think we also should fail on "mswi->size > ACLINT_MSWI_SIZE".

Hi Anup,

What I see is "if ((ms->size - offset) < ACLINT_MSWI_SIZE)"@ipi_mswi_cold_init() (fdt_ipi_mswi.c), which treats the ACLINT_MSWI_SIZE as the lower bound.

Regards,
Dong

Quoted 19 lines

Regards,
Anup
Quoted 15 lines
Regards,
Dong
Quoted 11 lines
lib/utils/ipi/aclint_mswi.c +0 -1
          (mswi->first_hartid >= SBI_HARTMASK_MAX_BITS) ||
          (mswi->hart_count > ACLINT_MSWI_MAX_HARTS))
              return SBI_EINVAL;
--
2.25.1
--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Anup Patel

author

2022-01-08 17:09 UTC #5 lore raw

PATCH 1/2 lib: utils/ipi: Fix size check in aclint_mswi_cold_init()

Quoted 37 lines · Dong Du

Quoted 34 lines · Anup Patel apatel at ventanamicro.com
Quoted 21 lines · Dong Du
Quoted 16 lines · Anup Patel apatel at ventanamicro.com
Currently, the ACLINT MSWI size check is forcing size to be at least 0x4000. This is inappropriate check because most systems will never utilize full 16KB for a single ACLINT MSWI device so instead we should check that ACLINT MSWI size is enough for on the associated HARTs.
Signed-off-by Anup Patel <[email protected]>
---
lib/utils/ipi/aclint_mswi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
lib/utils/ipi/aclint_mswi.c +0 -0
index a3de2f5..832e223 100644
@@ -74,7 +74,7 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
 
Quoted 8 lines
lib/utils/ipi/aclint_mswi.c int aclint_mswi_cold_init(struct aclint_mswi_data *mswi) · line 77
Quoted 4 lines
lib/utils/ipi/aclint_mswi.c +1 -1
      /* Sanity checks */
      if (!mswi || (mswi->addr & (ACLINT_MSWI_ALIGN - 1)) ||
-         (mswi->size < ACLINT_MSWI_SIZE) ||
+         (mswi->size < (mswi->hart_count * sizeof(u32))) ||
Hi Anup,
Shouldn't we also change ACLINT_MSWI_SIZE in lib/utils/ipi/fdt_ipi_mswi.c?
ACLINT_MSWI_SIZE represents the upper-limit because one MSWI device can be associated with up to 4095 HARTs.

Quoted 6 lines

Quoted 1 line
I think we also should fail on "mswi->size > ACLINT_MSWI_SIZE".
Hi Anup,
What I see is "if ((ms->size - offset) < ACLINT_MSWI_SIZE)"@ipi_mswi_cold_init() (fdt_ipi_mswi.c), which treats the ACLINT_MSWI_SIZE as the lower bound.

That's only SiFive CLINT backward compatibility because match->data != NULL only for SiFive CLINT.

Regards,
Anup

Quoted 23 lines

Regards,
Dong
Quoted 19 lines
Regards,
Anup
Quoted 15 lines
Regards,
Dong
Quoted 11 lines
lib/utils/ipi/aclint_mswi.c +0 -1
          (mswi->first_hartid >= SBI_HARTMASK_MAX_BITS) ||
          (mswi->hart_count > ACLINT_MSWI_MAX_HARTS))
              return SBI_EINVAL;
--
2.25.1
--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Dong Du

2022-01-09 04:45 UTC #6 lore raw

PATCH 1/2 lib: utils/ipi: Fix size check in aclint_mswi_cold_init()

Quoted 40 lines · Anup Patel apatel at ventanamicro.com

Quoted 39 lines · Dong Du
Quoted 35 lines · Anup Patel apatel at ventanamicro.com
Quoted 21 lines · Dong Du
Quoted 16 lines · Anup Patel apatel at ventanamicro.com
Currently, the ACLINT MSWI size check is forcing size to be at least 0x4000. This is inappropriate check because most systems will never utilize full 16KB for a single ACLINT MSWI device so instead we should check that ACLINT MSWI size is enough for on the associated HARTs.
Signed-off-by Anup Patel <[email protected]>
---
lib/utils/ipi/aclint_mswi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
lib/utils/ipi/aclint_mswi.c +0 -0
index a3de2f5..832e223 100644
@@ -74,7 +74,7 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
 
Quoted 8 lines
lib/utils/ipi/aclint_mswi.c int aclint_mswi_cold_init(struct aclint_mswi_data *mswi) · line 77
Quoted 4 lines
lib/utils/ipi/aclint_mswi.c +1 -1
      /* Sanity checks */
      if (!mswi || (mswi->addr & (ACLINT_MSWI_ALIGN - 1)) ||
-         (mswi->size < ACLINT_MSWI_SIZE) ||
+         (mswi->size < (mswi->hart_count * sizeof(u32))) ||
Hi Anup,
Shouldn't we also change ACLINT_MSWI_SIZE in lib/utils/ipi/fdt_ipi_mswi.c?
ACLINT_MSWI_SIZE represents the upper-limit because one MSWI device can be associated with up to 4095 HARTs.
I think we also should fail on "mswi->size > ACLINT_MSWI_SIZE".

Quoted 8 lines

Quoted 5 lines
Hi Anup,
What I see is "if ((ms->size - offset) < ACLINT_MSWI_SIZE)" at ipi_mswi_cold_init() (fdt_ipi_mswi.c),
which treats the ACLINT_MSWI_SIZE as the lower bound.
That's only SiFive CLINT backward compatibility because match->data != NULL only for SiFive CLINT.

Thanks for the clarification.

Reviewed-by Dong Du <[email protected]>

Regards,
Dong

Quoted 27 lines

Regards,
Anup
Quoted 23 lines
Regards,
Dong
Quoted 19 lines
Regards,
Anup
Quoted 15 lines
Regards,
Dong
Quoted 11 lines
lib/utils/ipi/aclint_mswi.c +0 -1
          (mswi->first_hartid >= SBI_HARTMASK_MAX_BITS) ||
          (mswi->hart_count > ACLINT_MSWI_MAX_HARTS))
              return SBI_EINVAL;
--
2.25.1
--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Anup Patel

author

2022-01-08 04:03 UTC #7 lore raw

PATCH 2/2 lib: utils/sys: Extend HTIF library to allow custom base address

In reply to Anup Patel

Some of RISC-V emulators provide HTIF at fixed base address so for such emulators users have to hard-code HTIF base address in the linker script.

To address this problem, we let users optionally provide fixed HTIF base address via platform support (or device tree).

Signed-off-by Anup Patel <[email protected]>

---
 include/sbi_utils/sys/htif.h       |  4 +-
 lib/utils/reset/fdt_reset_htif.c   |  8 +++-
 lib/utils/serial/fdt_serial_htif.c |  8 +++-
 lib/utils/sys/htif.c               | 74 ++++++++++++++++++++++++++----
 4 files changed, 80 insertions(+), 14 deletions(-)

include/sbi_utils/sys/htif.h +2 -2

index 9cc9634..106a096 100644
@@ -10,8 +10,8 @@
 
 #include <sbi/sbi_types.h>
 
-int htif_serial_init(void);
+int htif_serial_init(bool custom_base, unsigned long custom_base_addr);
 
-int htif_system_reset_init(void);
+int htif_system_reset_init(bool custom_base, unsigned long custom_base_addr);
 
 #endif

lib/utils/reset/fdt_reset_htif.c +7 -1

index dd08660..ab55198 100644
@@ -14,7 +14,13 @@
 static int htif_reset_init(void *fdt, int nodeoff,
 			   const struct fdt_match *match)
 {
-	return htif_system_reset_init();
+	bool custom;
+	uint64_t addr = 0;
+
+	custom = fdt_get_node_addr_size(fdt, nodeoff, 0, &addr, NULL) ?
+		 false : true;
+
+	return htif_system_reset_init(custom, addr);
 }
 
 static const struct fdt_match htif_reset_match[] = {

lib/utils/serial/fdt_serial_htif.c +7 -1

index fae55b8..e8bdbb4 100644
@@ -19,7 +19,13 @@ static const struct fdt_match serial_htif_match[] = {
 static int serial_htif_init(void *fdt, int nodeoff,
 			    const struct fdt_match *match)
 {
-	return htif_serial_init();
+	bool custom;
+	uint64_t addr = 0;
+
+	custom = fdt_get_node_addr_size(fdt, nodeoff, 0, &addr, NULL) ?
+		 false : true;
+
+	return htif_serial_init(custom, addr);
 }
 
 struct fdt_serial fdt_serial_htif = {

lib/utils/sys/htif.c +64 ~~-10~~

121 lines not on this page · raw message

Signature · 1 line

2.25.1

Dong Du

2022-01-08 09:08 UTC #8 lore raw

PATCH 2/2 lib: utils/sys: Extend HTIF library to allow custom base address

Quoted 135 lines · Anup Patel apatel at ventanamicro.com

Some of RISC-V emulators provide HTIF at fixed base address so for such emulators users have to hard-code HTIF base address in the linker script.

To address this problem, we let users optionally provide fixed HTIF base address via platform support (or device tree).

Signed-off-by Anup Patel <[email protected]>

---

include/sbi_utils/sys/htif.h       |  4 +-
lib/utils/reset/fdt_reset_htif.c   |  8 +++-
lib/utils/serial/fdt_serial_htif.c |  8 +++-
lib/utils/sys/htif.c               | 74 ++++++++++++++++++++++++++----
4 files changed, 80 insertions(+), 14 deletions(-)

include/sbi_utils/sys/htif.h +2 -2

index 9cc9634..106a096 100644
@@ -10,8 +10,8 @@
 
 #include <sbi/sbi_types.h>
 
-int htif_serial_init(void);
+int htif_serial_init(bool custom_base, unsigned long custom_base_addr);
 
-int htif_system_reset_init(void);
+int htif_system_reset_init(bool custom_base, unsigned long custom_base_addr);
 
 #endif

lib/utils/reset/fdt_reset_htif.c +7 -1

index dd08660..ab55198 100644
@@ -14,7 +14,13 @@
 static int htif_reset_init(void *fdt, int nodeoff,
 			   const struct fdt_match *match)
 {
-	return htif_system_reset_init();
+	bool custom;
+	uint64_t addr = 0;
+
+	custom = fdt_get_node_addr_size(fdt, nodeoff, 0, &addr, NULL) ?
+		 false : true;
+
+	return htif_system_reset_init(custom, addr);
 }
 
 static const struct fdt_match htif_reset_match[] = {

lib/utils/serial/fdt_serial_htif.c +7 -1

index fae55b8..e8bdbb4 100644
@@ -19,7 +19,13 @@ static const struct fdt_match serial_htif_match[] = {
 static int serial_htif_init(void *fdt, int nodeoff,
 			    const struct fdt_match *match)
 {
-	return htif_serial_init();
+	bool custom;
+	uint64_t addr = 0;
+
+	custom = fdt_get_node_addr_size(fdt, nodeoff, 0, &addr, NULL) ?
+		 false : true;
+
+	return htif_serial_init(custom, addr);
 }
 
 struct fdt_serial fdt_serial_htif = {

lib/utils/sys/htif.c +34 -3

index 7c69c7f..fbee4c7 100644
@@ -7,6 +7,7 @@
 
 #include <sbi/riscv_locks.h>
 #include <sbi/sbi_console.h>
+#include <sbi/sbi_error.h>
 #include <sbi/sbi_system.h>
 #include <sbi_utils/sys/htif.h>

@@ -47,15 +48,45 @@
 
 volatile uint64_t tohost __attribute__((section(".htif")));
 volatile uint64_t fromhost __attribute__((section(".htif")));
+
+static uint64_t *htif_base = NULL;
+static bool htif_have_base = false;
+
 static int htif_console_buf;
 static spinlock_t htif_lock = SPIN_LOCK_INITIALIZER;
 
+static inline uint64_t __read_tohost(void)
+{
+	return (htif_have_base) ? htif_base[1] : tohost;
+}
+
+static inline void __write_tohost(uint64_t val)
+{
+	if (htif_have_base)
+		htif_base[1] = val;
+	else
+		tohost = val;
+}
+
+static inline uint64_t __read_fromhost(void)
+{
+	return (htif_have_base) ? htif_base[0] : fromhost;
+}
+
+static inline void __write_fromhost(uint64_t val)
+{
+	if (htif_have_base)
+		htif_base[0] = val;
+	else
+		fromhost = val;
+}
+
 static void __check_fromhost()
 {
-	uint64_t fh = fromhost;
+	uint64_t fh = __read_fromhost();
 	if (!fh)
 		return;
-	fromhost = 0;
+	__write_fromhost(0);
 
 	/* this should be from the console */
 	if (FROMHOST_DEV(fh) != HTIF_DEV_CONSOLE)

@@ -73,9 +104,22 @@ static void __check_fromhost()
 
 static void __set_tohost(uint64_t dev, uint64_t cmd, uint64_t data)
 {
-	while (tohost)
+	while (__read_tohost())

lib/utils/sys/htif.c static void __check_fromhost() · line 116

Quoted 10 lines

lib/utils/sys/htif.c +8 -1

 		__check_fromhost();
-	tohost = TOHOST_CMD(dev, cmd, data);
+	__write_tohost(TOHOST_CMD(dev, cmd, data));
+}
+
+static int set_custom_base(bool custom_base, unsigned long custom_base_addr)
+{
+	if (custom_base) {
+		if (htif_have_base &&
+		    custom_base_addr != (unsigned long)htif_base)

The above could be simplified to: if (htif_have_base)

lib/utils/sys/htif.c static void __check_fromhost() · line 119

Quoted 3 lines

lib/utils/sys/htif.c +3 -0

+			return SBI_EINVAL;
+		htif_have_base = true;
+		htif_base = (uint64_t *)custom_base_addr;

Minor suggestion: it would be better to set the value (htif_base) before setting the flag (htif_have_base).

Besides the above minor issues, looks good to me.

Reviewed-by Dong Du <[email protected]>

Regards,
Dong

Quoted 60 lines

lib/utils/sys/htif.c +19 -7

+	}
+
+	return 0;
 }
 
 #if __riscv_xlen == 32

@@ -148,10 +192,15 @@ static struct sbi_console_device htif_console = {
 	.console_getc = htif_getc
 };
 
-int htif_serial_init(void)
+int htif_serial_init(bool custom_base, unsigned long custom_base_addr)
 {
-	sbi_console_set_device(&htif_console);
+	int rc;
+
+	rc = set_custom_base(custom_base, custom_base_addr);
+	if (rc)
+		return rc;
 
+	sbi_console_set_device(&htif_console);
 	return 0;
 }

@@ -163,8 +212,8 @@ static int htif_system_reset_check(u32 type, u32 reason)
 static void htif_system_reset(u32 type, u32 reason)
 {
 	while (1) {
-		fromhost = 0;
-		tohost = 1;
+		__write_fromhost(0);
+		__write_tohost(1);
 	}
 }

@@ -174,9 +223,14 @@ static struct sbi_system_reset_device htif_reset = {
 	.system_reset = htif_system_reset
 };
 
-int htif_system_reset_init(void)
+int htif_system_reset_init(bool custom_base, unsigned long custom_base_addr)
 {
-	sbi_system_reset_add_device(&htif_reset);
+	int rc;
+
+	rc = set_custom_base(custom_base, custom_base_addr);
+	if (rc)
+		return rc;
 
+	sbi_system_reset_add_device(&htif_reset);
 	return 0;
 }
--

2.25.1

--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Anup Patel

author

2022-01-08 11:44 UTC #9 lore raw

PATCH 2/2 lib: utils/sys: Extend HTIF library to allow custom base address

Quoted 136 lines · Dong Du

Quoted 131 lines · Anup Patel apatel at ventanamicro.com

Some of RISC-V emulators provide HTIF at fixed base address so for such emulators users have to hard-code HTIF base address in the linker script.

To address this problem, we let users optionally provide fixed HTIF base address via platform support (or device tree).

Signed-off-by Anup Patel <[email protected]>

---

include/sbi_utils/sys/htif.h       |  4 +-
lib/utils/reset/fdt_reset_htif.c   |  8 +++-
lib/utils/serial/fdt_serial_htif.c |  8 +++-
lib/utils/sys/htif.c               | 74 ++++++++++++++++++++++++++----
4 files changed, 80 insertions(+), 14 deletions(-)

include/sbi_utils/sys/htif.h +2 -2

index 9cc9634..106a096 100644
@@ -10,8 +10,8 @@
 
 #include <sbi/sbi_types.h>
 
-int htif_serial_init(void);
+int htif_serial_init(bool custom_base, unsigned long custom_base_addr);
 
-int htif_system_reset_init(void);
+int htif_system_reset_init(bool custom_base, unsigned long custom_base_addr);
 
 #endif

lib/utils/reset/fdt_reset_htif.c +7 -1

index dd08660..ab55198 100644
@@ -14,7 +14,13 @@
 static int htif_reset_init(void *fdt, int nodeoff,
                         const struct fdt_match *match)
 {
-     return htif_system_reset_init();
+     bool custom;
+     uint64_t addr = 0;
+
+     custom = fdt_get_node_addr_size(fdt, nodeoff, 0, &addr, NULL) ?
+              false : true;
+
+     return htif_system_reset_init(custom, addr);
 }
 
 static const struct fdt_match htif_reset_match[] = {

lib/utils/serial/fdt_serial_htif.c +7 -1

index fae55b8..e8bdbb4 100644
@@ -19,7 +19,13 @@ static const struct fdt_match serial_htif_match[] = {
 static int serial_htif_init(void *fdt, int nodeoff,
                          const struct fdt_match *match)
 {
-     return htif_serial_init();
+     bool custom;
+     uint64_t addr = 0;
+
+     custom = fdt_get_node_addr_size(fdt, nodeoff, 0, &addr, NULL) ?
+              false : true;
+
+     return htif_serial_init(custom, addr);
 }
 
 struct fdt_serial fdt_serial_htif = {

lib/utils/sys/htif.c +33 -2

index 7c69c7f..fbee4c7 100644
@@ -7,6 +7,7 @@
 
 #include <sbi/riscv_locks.h>
 #include <sbi/sbi_console.h>
+#include <sbi/sbi_error.h>
 #include <sbi/sbi_system.h>
 #include <sbi_utils/sys/htif.h>

@@ -47,15 +48,45 @@
 
 volatile uint64_t tohost __attribute__((section(".htif")));
 volatile uint64_t fromhost __attribute__((section(".htif")));
+
+static uint64_t *htif_base = NULL;
+static bool htif_have_base = false;
+
 static int htif_console_buf;
 static spinlock_t htif_lock = SPIN_LOCK_INITIALIZER;
 
+static inline uint64_t __read_tohost(void)
+{
+     return (htif_have_base) ? htif_base[1] : tohost;
+}
+
+static inline void __write_tohost(uint64_t val)
+{
+     if (htif_have_base)
+             htif_base[1] = val;
+     else
+             tohost = val;
+}
+
+static inline uint64_t __read_fromhost(void)
+{
+     return (htif_have_base) ? htif_base[0] : fromhost;
+}
+
+static inline void __write_fromhost(uint64_t val)
+{
+     if (htif_have_base)
+             htif_base[0] = val;
+     else
+             fromhost = val;
+}
+
 static void __check_fromhost()
 {
-     uint64_t fh = fromhost;
+     uint64_t fh = __read_fromhost();
      if (!fh)
              return;
-     fromhost = 0;
+     __write_fromhost(0);
 
      /* this should be from the console */
      if (FROMHOST_DEV(fh) != HTIF_DEV_CONSOLE)

@@ -73,9 +104,22 @@ static void __check_fromhost()

Quoted 16 lines

Quoted 4 lines

lib/utils/sys/htif.c +1 -1

 static void __set_tohost(uint64_t dev, uint64_t cmd, uint64_t data)
 {
-     while (tohost)
+     while (__read_tohost())

lib/utils/sys/htif.c static void __check_fromhost() · line 116

Quoted 10 lines

lib/utils/sys/htif.c +8 -1

              __check_fromhost();
-     tohost = TOHOST_CMD(dev, cmd, data);
+     __write_tohost(TOHOST_CMD(dev, cmd, data));
+}
+
+static int set_custom_base(bool custom_base, unsigned long custom_base_addr)
+{
+     if (custom_base) {
+             if (htif_have_base &&
+                 custom_base_addr != (unsigned long)htif_base)

The above could be simplified to: if (htif_have_base)

If we only check htif_have_base then it fails to boot because set_custom_base() is called from two places htif_serial_init() and htif_reset_init(). The second check prevents failure when same address is set twice.

Quoted 6 lines

lib/utils/sys/htif.c static void __check_fromhost() · line 119
Quoted 4 lines
lib/utils/sys/htif.c +3 -0
+                     return SBI_EINVAL;
+             htif_have_base = true;
+             htif_base = (uint64_t *)custom_base_addr;
Minor suggestion: it would be better to set the value (htif_base) before setting the flag (htif_have_base).

The order does not matter here because set_custom_base() is always called in the cold boot path but I will change it anyway.

I think we can make set_custom_base() even more generic by allowing separate addresses for "fromhost" and "tohost". I will update this in the next revision.

Regards,
Anup

Quoted 68 lines

Besides the above minor issues, looks good to me.

Reviewed-by Dong Du <[email protected]>

Regards,
Dong

Quoted 60 lines

lib/utils/sys/htif.c +19 -7

+     }
+
+     return 0;
 }
 
 #if __riscv_xlen == 32

@@ -148,10 +192,15 @@ static struct sbi_console_device htif_console = {
      .console_getc = htif_getc
 };
 
-int htif_serial_init(void)
+int htif_serial_init(bool custom_base, unsigned long custom_base_addr)
 {
-     sbi_console_set_device(&htif_console);
+     int rc;
+
+     rc = set_custom_base(custom_base, custom_base_addr);
+     if (rc)
+             return rc;
 
+     sbi_console_set_device(&htif_console);
      return 0;
 }

@@ -163,8 +212,8 @@ static int htif_system_reset_check(u32 type, u32 reason)
 static void htif_system_reset(u32 type, u32 reason)
 {
      while (1) {
-             fromhost = 0;
-             tohost = 1;
+             __write_fromhost(0);
+             __write_tohost(1);
      }
 }

@@ -174,9 +223,14 @@ static struct sbi_system_reset_device htif_reset = {
      .system_reset = htif_system_reset
 };
 
-int htif_system_reset_init(void)
+int htif_system_reset_init(bool custom_base, unsigned long custom_base_addr)
 {
-     sbi_system_reset_add_device(&htif_reset);
+     int rc;
+
+     rc = set_custom_base(custom_base, custom_base_addr);
+     if (rc)
+             return rc;
 
+     sbi_system_reset_add_device(&htif_reset);
      return 0;
 }
--

2.25.1

--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Dong Du

2022-01-09 04:51 UTC #10 lore raw

PATCH 2/2 lib: utils/sys: Extend HTIF library to allow custom base address

Quoted 154 lines · Anup Patel apatel at ventanamicro.com

Quoted 136 lines · Dong Du

Quoted 131 lines · Anup Patel apatel at ventanamicro.com

Some of RISC-V emulators provide HTIF at fixed base address so for such emulators users have to hard-code HTIF base address in the linker script.

To address this problem, we let users optionally provide fixed HTIF base address via platform support (or device tree).

Signed-off-by Anup Patel <[email protected]>

---

include/sbi_utils/sys/htif.h       |  4 +-
lib/utils/reset/fdt_reset_htif.c   |  8 +++-
lib/utils/serial/fdt_serial_htif.c |  8 +++-
lib/utils/sys/htif.c               | 74 ++++++++++++++++++++++++++----
4 files changed, 80 insertions(+), 14 deletions(-)

include/sbi_utils/sys/htif.h +2 -2

index 9cc9634..106a096 100644
@@ -10,8 +10,8 @@
 
 #include <sbi/sbi_types.h>
 
-int htif_serial_init(void);
+int htif_serial_init(bool custom_base, unsigned long custom_base_addr);
 
-int htif_system_reset_init(void);
+int htif_system_reset_init(bool custom_base, unsigned long custom_base_addr);
 
 #endif

lib/utils/reset/fdt_reset_htif.c +7 -1

index dd08660..ab55198 100644
@@ -14,7 +14,13 @@
 static int htif_reset_init(void *fdt, int nodeoff,
                         const struct fdt_match *match)
 {
-     return htif_system_reset_init();
+     bool custom;
+     uint64_t addr = 0;
+
+     custom = fdt_get_node_addr_size(fdt, nodeoff, 0, &addr, NULL) ?
+              false : true;
+
+     return htif_system_reset_init(custom, addr);
 }
 
 static const struct fdt_match htif_reset_match[] = {

lib/utils/serial/fdt_serial_htif.c +7 -1

index fae55b8..e8bdbb4 100644
@@ -19,7 +19,13 @@ static const struct fdt_match serial_htif_match[] = {
 static int serial_htif_init(void *fdt, int nodeoff,
                          const struct fdt_match *match)
 {
-     return htif_serial_init();
+     bool custom;
+     uint64_t addr = 0;
+
+     custom = fdt_get_node_addr_size(fdt, nodeoff, 0, &addr, NULL) ?
+              false : true;
+
+     return htif_serial_init(custom, addr);
 }
 
 struct fdt_serial fdt_serial_htif = {

lib/utils/sys/htif.c +33 -2

index 7c69c7f..fbee4c7 100644
@@ -7,6 +7,7 @@
 
 #include <sbi/riscv_locks.h>
 #include <sbi/sbi_console.h>
+#include <sbi/sbi_error.h>
 #include <sbi/sbi_system.h>
 #include <sbi_utils/sys/htif.h>

@@ -47,15 +48,45 @@
 
 volatile uint64_t tohost __attribute__((section(".htif")));
 volatile uint64_t fromhost __attribute__((section(".htif")));
+
+static uint64_t *htif_base = NULL;
+static bool htif_have_base = false;
+
 static int htif_console_buf;
 static spinlock_t htif_lock = SPIN_LOCK_INITIALIZER;
 
+static inline uint64_t __read_tohost(void)
+{
+     return (htif_have_base) ? htif_base[1] : tohost;
+}
+
+static inline void __write_tohost(uint64_t val)
+{
+     if (htif_have_base)
+             htif_base[1] = val;
+     else
+             tohost = val;
+}
+
+static inline uint64_t __read_fromhost(void)
+{
+     return (htif_have_base) ? htif_base[0] : fromhost;
+}
+
+static inline void __write_fromhost(uint64_t val)
+{
+     if (htif_have_base)
+             htif_base[0] = val;
+     else
+             fromhost = val;
+}
+
 static void __check_fromhost()
 {
-     uint64_t fh = fromhost;
+     uint64_t fh = __read_fromhost();
      if (!fh)
              return;
-     fromhost = 0;
+     __write_fromhost(0);
 
      /* this should be from the console */
      if (FROMHOST_DEV(fh) != HTIF_DEV_CONSOLE)

@@ -73,9 +104,22 @@ static void __check_fromhost()

Quoted 16 lines

Quoted 4 lines

lib/utils/sys/htif.c +1 -1

 static void __set_tohost(uint64_t dev, uint64_t cmd, uint64_t data)
 {
-     while (tohost)
+     while (__read_tohost())

lib/utils/sys/htif.c static void __check_fromhost() · line 116

Quoted 10 lines

lib/utils/sys/htif.c +8 -1

              __check_fromhost();
-     tohost = TOHOST_CMD(dev, cmd, data);
+     __write_tohost(TOHOST_CMD(dev, cmd, data));
+}
+
+static int set_custom_base(bool custom_base, unsigned long custom_base_addr)
+{
+     if (custom_base) {
+             if (htif_have_base &&
+                 custom_base_addr != (unsigned long)htif_base)

The above could be simplified to: if (htif_have_base)

Quoted 4 lines

If we only check htif_have_base then it fails to boot because set_custom_base() is called from two places htif_serial_init() and htif_reset_init(). The second check prevents failure when same address is set twice.

OK, make sense to me.

Quoted 9 lines

Quoted 8 lines
lib/utils/sys/htif.c static void __check_fromhost() · line 119
Quoted 5 lines
lib/utils/sys/htif.c +3 -0
+                     return SBI_EINVAL;
+             htif_have_base = true;
+             htif_base = (uint64_t *)custom_base_addr;
Minor suggestion: it would be better to set the value (htif_base) before setting the flag (htif_have_base).

Quoted 6 lines

The order does not matter here because set_custom_base() is always called in the cold boot path but I will change it anyway.
I think we can make set_custom_base() even more generic by allowing separate addresses for "fromhost" and "tohost". I will update this in the next revision.

Great.

Regards,
Dong

Quoted 72 lines

Regards,
Anup

Quoted 68 lines

Besides the above minor issues, looks good to me.

Reviewed-by Dong Du <[email protected]>

Regards,
Dong

Quoted 60 lines

lib/utils/sys/htif.c +19 -7

+     }
+
+     return 0;
 }
 
 #if __riscv_xlen == 32

@@ -148,10 +192,15 @@ static struct sbi_console_device htif_console = {
      .console_getc = htif_getc
 };
 
-int htif_serial_init(void)
+int htif_serial_init(bool custom_base, unsigned long custom_base_addr)
 {
-     sbi_console_set_device(&htif_console);
+     int rc;
+
+     rc = set_custom_base(custom_base, custom_base_addr);
+     if (rc)
+             return rc;
 
+     sbi_console_set_device(&htif_console);
      return 0;
 }

@@ -163,8 +212,8 @@ static int htif_system_reset_check(u32 type, u32 reason)
 static void htif_system_reset(u32 type, u32 reason)
 {
      while (1) {
-             fromhost = 0;
-             tohost = 1;
+             __write_fromhost(0);
+             __write_tohost(1);
      }
 }

@@ -174,9 +223,14 @@ static struct sbi_system_reset_device htif_reset = {
      .system_reset = htif_system_reset
 };
 
-int htif_system_reset_init(void)
+int htif_system_reset_init(bool custom_base, unsigned long custom_base_addr)
 {
-     sbi_system_reset_add_device(&htif_reset);
+     int rc;
+
+     rc = set_custom_base(custom_base, custom_base_addr);
+     if (rc)
+             return rc;
 
+     sbi_system_reset_add_device(&htif_reset);
      return 0;
 }
--

2.25.1

--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi