[PATCH 2/2] lib: utils/sys: Extend HTIF library to allow custom base address
Dong Du
dd_nirvana at sjtu.edu.cn
Sat Jan 8 20:51:39 PST 2022
On Jan 8, 2022, at 7:44 PM, Anup Patel apatel at ventanamicro.com wrote:
> On Sat, Jan 8, 2022 at 2:38 PM Dong Du <dd_nirvana at sjtu.edu.cn> wrote:
>>
>>
>>
>> On Jan 8, 2022, at 12:03 PM, Anup Patel apatel at ventanamicro.com wrote:
>>
>> > 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 <apatel at ventanamicro.com>
>> > ---
>> > 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(-)
>> >
>> > diff --git a/include/sbi_utils/sys/htif.h b/include/sbi_utils/sys/htif.h
>> > index 9cc9634..106a096 100644
>> > --- a/include/sbi_utils/sys/htif.h
>> > +++ b/include/sbi_utils/sys/htif.h
>> > @@ -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
>> > diff --git a/lib/utils/reset/fdt_reset_htif.c b/lib/utils/reset/fdt_reset_htif.c
>> > index dd08660..ab55198 100644
>> > --- a/lib/utils/reset/fdt_reset_htif.c
>> > +++ b/lib/utils/reset/fdt_reset_htif.c
>> > @@ -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[] = {
>> > diff --git a/lib/utils/serial/fdt_serial_htif.c
>> > b/lib/utils/serial/fdt_serial_htif.c
>> > index fae55b8..e8bdbb4 100644
>> > --- a/lib/utils/serial/fdt_serial_htif.c
>> > +++ b/lib/utils/serial/fdt_serial_htif.c
>> > @@ -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 = {
>> > diff --git a/lib/utils/sys/htif.c b/lib/utils/sys/htif.c
>> > index 7c69c7f..fbee4c7 100644
>> > --- a/lib/utils/sys/htif.c
>> > +++ b/lib/utils/sys/htif.c
>> > @@ -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())
>> > __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.
OK, make sense to me.
>
>>
>> > + 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.
Great.
Regards,
Dong
>
> Regards,
> Anup
>
>>
>> Besides the above minor issues, looks good to me.
>>
>> Reviewed-by: Dong Du <Dd_nirvana at sjtu.edu.cn>
>>
>> Regards,
>> Dong
>>
>> > + }
>> > +
>> > + 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
More information about the opensbi
mailing list