Re: [PATCH v4 2/5] RISC-V: Add SBI debug console helper routines
On Mon, Nov 20, 2023 at 1:35 PM Andrew Jones wrote:
>
> On Sat, Nov 18, 2023 at 09:08:56AM +0530, Anup Patel wrote:
> > Let us provide SBI debug console helper routines which can be
> > shared by serial/earlycon-riscv-sbi.c and hvc/hvc_riscv_sbi.c.
> >
> > Signed-off-by: Anup Patel
> > ---
> > arch/riscv/include/asm/sbi.h | 5 +
> > arch/riscv/kernel/sbi.c | 43
> > 2 files changed, 48 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 66f3933c14f6..ee7aef5f6233 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -334,6 +334,11 @@ static inline unsigned long sbi_mk_version(unsigned
> > long major,
> > }
> >
> > int sbi_err_map_linux_errno(int err);
> > +
> > +extern bool sbi_debug_console_available;
> > +int sbi_debug_console_write(unsigned int num_bytes, phys_addr_t base_addr);
> > +int sbi_debug_console_read(unsigned int num_bytes, phys_addr_t base_addr);
> > +
> > #else /* CONFIG_RISCV_SBI */
> > static inline int sbi_remote_fence_i(const struct cpumask *cpu_mask) {
> > return -1; }
> > static inline void sbi_init(void) {}
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 5a62ed1da453..73a9c22c3945 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -571,6 +571,44 @@ long sbi_get_mimpid(void)
> > }
> > EXPORT_SYMBOL_GPL(sbi_get_mimpid);
> >
> > +bool sbi_debug_console_available;
> > +
> > +int sbi_debug_console_write(unsigned int num_bytes, phys_addr_t base_addr)
> > +{
> > + struct sbiret ret;
> > +
> > + if (!sbi_debug_console_available)
> > + return -EOPNOTSUPP;
> > +
> > + if (IS_ENABLED(CONFIG_32BIT))
> > + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
> > + num_bytes, lower_32_bits(base_addr),
> > + upper_32_bits(base_addr), 0, 0, 0);
> > + else
> > + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
> > + num_bytes, base_addr, 0, 0, 0, 0);
> > +
> > + return ret.error ? sbi_err_map_linux_errno(ret.error) : ret.value;
>
> We can't get perfect mappings, but I wonder if we can do better than
> returning ENOTSUPP for "Failed to write the byte due to I/O errors."
>
> How about
>
> if (ret.error == SBI_ERR_FAILURE)
> return -EIO;
>
> return ret.error ? sbi_err_map_linux_errno(ret.error) : ret.value;
Seems overkill but I will update anyway.
>
>
> > +}
> > +
> > +int sbi_debug_console_read(unsigned int num_bytes, phys_addr_t base_addr)
> > +{
> > + struct sbiret ret;
> > +
> > + if (!sbi_debug_console_available)
> > + return -EOPNOTSUPP;
> > +
> > + if (IS_ENABLED(CONFIG_32BIT))
> > + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_READ,
> > + num_bytes, lower_32_bits(base_addr),
> > + upper_32_bits(base_addr), 0, 0, 0);
> > + else
> > + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_READ,
> > + num_bytes, base_addr, 0, 0, 0, 0);
> > +
> > + return ret.error ? sbi_err_map_linux_errno(ret.error) : ret.value;
>
> Same comment as above.
Okay.
>
> > +}
> > +
> > void __init sbi_init(void)
> > {
> > int ret;
> > @@ -612,6 +650,11 @@ void __init sbi_init(void)
> > sbi_srst_reboot_nb.priority = 192;
> > register_restart_handler(&sbi_srst_reboot_nb);
> > }
> > + if ((sbi_spec_version >= sbi_mk_version(2, 0)) &&
> > + (sbi_probe_extension(SBI_EXT_DBCN) > 0)) {
> > + pr_info("SBI DBCN extension detected\n");
> > + sbi_debug_console_available = true;
> > + }
> > } else {
> > __sbi_set_timer = __sbi_set_timer_v01;
> > __sbi_send_ipi = __sbi_send_ipi_v01;
> > --
> > 2.34.1
> >
>
> Otherwise,
>
> Reviewed-by: Andrew Jones
>
> Thanks,
> drew
Regards,
Anup
Re: [PATCH v4 2/5] RISC-V: Add SBI debug console helper routines
On Wed, Nov 22, 2023 at 4:15 AM Samuel Holland
wrote:
>
> Hi Anup,
>
> On 2023-11-17 9:38 PM, Anup Patel wrote:
> > Let us provide SBI debug console helper routines which can be
> > shared by serial/earlycon-riscv-sbi.c and hvc/hvc_riscv_sbi.c.
> >
> > Signed-off-by: Anup Patel
> > ---
> > arch/riscv/include/asm/sbi.h | 5 +
> > arch/riscv/kernel/sbi.c | 43
> > 2 files changed, 48 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 66f3933c14f6..ee7aef5f6233 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -334,6 +334,11 @@ static inline unsigned long sbi_mk_version(unsigned
> > long major,
> > }
> >
> > int sbi_err_map_linux_errno(int err);
> > +
> > +extern bool sbi_debug_console_available;
> > +int sbi_debug_console_write(unsigned int num_bytes, phys_addr_t base_addr);
> > +int sbi_debug_console_read(unsigned int num_bytes, phys_addr_t base_addr);
> > +
> > #else /* CONFIG_RISCV_SBI */
> > static inline int sbi_remote_fence_i(const struct cpumask *cpu_mask) {
> > return -1; }
> > static inline void sbi_init(void) {}
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 5a62ed1da453..73a9c22c3945 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -571,6 +571,44 @@ long sbi_get_mimpid(void)
> > }
> > EXPORT_SYMBOL_GPL(sbi_get_mimpid);
> >
> > +bool sbi_debug_console_available;
> > +
> > +int sbi_debug_console_write(unsigned int num_bytes, phys_addr_t base_addr)
> > +{
> > + struct sbiret ret;
> > +
> > + if (!sbi_debug_console_available)
> > + return -EOPNOTSUPP;
> > +
> > + if (IS_ENABLED(CONFIG_32BIT))
> > + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
> > + num_bytes, lower_32_bits(base_addr),
> > + upper_32_bits(base_addr), 0, 0, 0);
> > + else
> > + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
> > + num_bytes, base_addr, 0, 0, 0, 0);
> > +
> > + return ret.error ? sbi_err_map_linux_errno(ret.error) : ret.value;
> > +}
> > +
> > +int sbi_debug_console_read(unsigned int num_bytes, phys_addr_t base_addr)
> > +{
> > + struct sbiret ret;
> > +
> > + if (!sbi_debug_console_available)
> > + return -EOPNOTSUPP;
> > +
> > + if (IS_ENABLED(CONFIG_32BIT))
> > + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_READ,
> > + num_bytes, lower_32_bits(base_addr),
> > + upper_32_bits(base_addr), 0, 0, 0);
> > + else
> > + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_READ,
> > + num_bytes, base_addr, 0, 0, 0, 0);
> > +
> > + return ret.error ? sbi_err_map_linux_errno(ret.error) : ret.value;
> > +}
>
> Since every place that calls these functions will need to do the vmalloc
> lookup,
> would it make sense to do it here, and have these take a pointer instead?
Yes, that's better. I will update.
Regards,
Anup
Re: [PATCH v4 2/5] RISC-V: Add SBI debug console helper routines
Hi Anup,
On 2023-11-17 9:38 PM, Anup Patel wrote:
> Let us provide SBI debug console helper routines which can be
> shared by serial/earlycon-riscv-sbi.c and hvc/hvc_riscv_sbi.c.
>
> Signed-off-by: Anup Patel
> ---
> arch/riscv/include/asm/sbi.h | 5 +
> arch/riscv/kernel/sbi.c | 43
> 2 files changed, 48 insertions(+)
>
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 66f3933c14f6..ee7aef5f6233 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -334,6 +334,11 @@ static inline unsigned long sbi_mk_version(unsigned long
> major,
> }
>
> int sbi_err_map_linux_errno(int err);
> +
> +extern bool sbi_debug_console_available;
> +int sbi_debug_console_write(unsigned int num_bytes, phys_addr_t base_addr);
> +int sbi_debug_console_read(unsigned int num_bytes, phys_addr_t base_addr);
> +
> #else /* CONFIG_RISCV_SBI */
> static inline int sbi_remote_fence_i(const struct cpumask *cpu_mask) {
> return -1; }
> static inline void sbi_init(void) {}
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index 5a62ed1da453..73a9c22c3945 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -571,6 +571,44 @@ long sbi_get_mimpid(void)
> }
> EXPORT_SYMBOL_GPL(sbi_get_mimpid);
>
> +bool sbi_debug_console_available;
> +
> +int sbi_debug_console_write(unsigned int num_bytes, phys_addr_t base_addr)
> +{
> + struct sbiret ret;
> +
> + if (!sbi_debug_console_available)
> + return -EOPNOTSUPP;
> +
> + if (IS_ENABLED(CONFIG_32BIT))
> + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
> + num_bytes, lower_32_bits(base_addr),
> + upper_32_bits(base_addr), 0, 0, 0);
> + else
> + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
> + num_bytes, base_addr, 0, 0, 0, 0);
> +
> + return ret.error ? sbi_err_map_linux_errno(ret.error) : ret.value;
> +}
> +
> +int sbi_debug_console_read(unsigned int num_bytes, phys_addr_t base_addr)
> +{
> + struct sbiret ret;
> +
> + if (!sbi_debug_console_available)
> + return -EOPNOTSUPP;
> +
> + if (IS_ENABLED(CONFIG_32BIT))
> + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_READ,
> + num_bytes, lower_32_bits(base_addr),
> + upper_32_bits(base_addr), 0, 0, 0);
> + else
> + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_READ,
> + num_bytes, base_addr, 0, 0, 0, 0);
> +
> + return ret.error ? sbi_err_map_linux_errno(ret.error) : ret.value;
> +}
Since every place that calls these functions will need to do the vmalloc lookup,
would it make sense to do it here, and have these take a pointer instead?
Regards,
Samuel
Re: [PATCH v4 2/5] RISC-V: Add SBI debug console helper routines
On Sat, Nov 18, 2023 at 09:08:56AM +0530, Anup Patel wrote:
> Let us provide SBI debug console helper routines which can be
> shared by serial/earlycon-riscv-sbi.c and hvc/hvc_riscv_sbi.c.
>
> Signed-off-by: Anup Patel
> ---
> arch/riscv/include/asm/sbi.h | 5 +
> arch/riscv/kernel/sbi.c | 43
> 2 files changed, 48 insertions(+)
>
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 66f3933c14f6..ee7aef5f6233 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -334,6 +334,11 @@ static inline unsigned long sbi_mk_version(unsigned long
> major,
> }
>
> int sbi_err_map_linux_errno(int err);
> +
> +extern bool sbi_debug_console_available;
> +int sbi_debug_console_write(unsigned int num_bytes, phys_addr_t base_addr);
> +int sbi_debug_console_read(unsigned int num_bytes, phys_addr_t base_addr);
> +
> #else /* CONFIG_RISCV_SBI */
> static inline int sbi_remote_fence_i(const struct cpumask *cpu_mask) {
> return -1; }
> static inline void sbi_init(void) {}
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index 5a62ed1da453..73a9c22c3945 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -571,6 +571,44 @@ long sbi_get_mimpid(void)
> }
> EXPORT_SYMBOL_GPL(sbi_get_mimpid);
>
> +bool sbi_debug_console_available;
> +
> +int sbi_debug_console_write(unsigned int num_bytes, phys_addr_t base_addr)
> +{
> + struct sbiret ret;
> +
> + if (!sbi_debug_console_available)
> + return -EOPNOTSUPP;
> +
> + if (IS_ENABLED(CONFIG_32BIT))
> + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
> + num_bytes, lower_32_bits(base_addr),
> + upper_32_bits(base_addr), 0, 0, 0);
> + else
> + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
> + num_bytes, base_addr, 0, 0, 0, 0);
> +
> + return ret.error ? sbi_err_map_linux_errno(ret.error) : ret.value;
We can't get perfect mappings, but I wonder if we can do better than
returning ENOTSUPP for "Failed to write the byte due to I/O errors."
How about
if (ret.error == SBI_ERR_FAILURE)
return -EIO;
return ret.error ? sbi_err_map_linux_errno(ret.error) : ret.value;
> +}
> +
> +int sbi_debug_console_read(unsigned int num_bytes, phys_addr_t base_addr)
> +{
> + struct sbiret ret;
> +
> + if (!sbi_debug_console_available)
> + return -EOPNOTSUPP;
> +
> + if (IS_ENABLED(CONFIG_32BIT))
> + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_READ,
> + num_bytes, lower_32_bits(base_addr),
> + upper_32_bits(base_addr), 0, 0, 0);
> + else
> + ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_READ,
> + num_bytes, base_addr, 0, 0, 0, 0);
> +
> + return ret.error ? sbi_err_map_linux_errno(ret.error) : ret.value;
Same comment as above.
> +}
> +
> void __init sbi_init(void)
> {
> int ret;
> @@ -612,6 +650,11 @@ void __init sbi_init(void)
> sbi_srst_reboot_nb.priority = 192;
> register_restart_handler(&sbi_srst_reboot_nb);
> }
> + if ((sbi_spec_version >= sbi_mk_version(2, 0)) &&
> + (sbi_probe_extension(SBI_EXT_DBCN) > 0)) {
> + pr_info("SBI DBCN extension detected\n");
> + sbi_debug_console_available = true;
> + }
> } else {
> __sbi_set_timer = __sbi_set_timer_v01;
> __sbi_send_ipi = __sbi_send_ipi_v01;
> --
> 2.34.1
>
Otherwise,
Reviewed-by: Andrew Jones
Thanks,
drew
[PATCH v4 2/5] RISC-V: Add SBI debug console helper routines
Let us provide SBI debug console helper routines which can be
shared by serial/earlycon-riscv-sbi.c and hvc/hvc_riscv_sbi.c.
Signed-off-by: Anup Patel
---
arch/riscv/include/asm/sbi.h | 5 +
arch/riscv/kernel/sbi.c | 43
2 files changed, 48 insertions(+)
diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 66f3933c14f6..ee7aef5f6233 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -334,6 +334,11 @@ static inline unsigned long sbi_mk_version(unsigned long
major,
}
int sbi_err_map_linux_errno(int err);
+
+extern bool sbi_debug_console_available;
+int sbi_debug_console_write(unsigned int num_bytes, phys_addr_t base_addr);
+int sbi_debug_console_read(unsigned int num_bytes, phys_addr_t base_addr);
+
#else /* CONFIG_RISCV_SBI */
static inline int sbi_remote_fence_i(const struct cpumask *cpu_mask) { return
-1; }
static inline void sbi_init(void) {}
diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
index 5a62ed1da453..73a9c22c3945 100644
--- a/arch/riscv/kernel/sbi.c
+++ b/arch/riscv/kernel/sbi.c
@@ -571,6 +571,44 @@ long sbi_get_mimpid(void)
}
EXPORT_SYMBOL_GPL(sbi_get_mimpid);
+bool sbi_debug_console_available;
+
+int sbi_debug_console_write(unsigned int num_bytes, phys_addr_t base_addr)
+{
+ struct sbiret ret;
+
+ if (!sbi_debug_console_available)
+ return -EOPNOTSUPP;
+
+ if (IS_ENABLED(CONFIG_32BIT))
+ ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
+ num_bytes, lower_32_bits(base_addr),
+ upper_32_bits(base_addr), 0, 0, 0);
+ else
+ ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
+ num_bytes, base_addr, 0, 0, 0, 0);
+
+ return ret.error ? sbi_err_map_linux_errno(ret.error) : ret.value;
+}
+
+int sbi_debug_console_read(unsigned int num_bytes, phys_addr_t base_addr)
+{
+ struct sbiret ret;
+
+ if (!sbi_debug_console_available)
+ return -EOPNOTSUPP;
+
+ if (IS_ENABLED(CONFIG_32BIT))
+ ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_READ,
+ num_bytes, lower_32_bits(base_addr),
+ upper_32_bits(base_addr), 0, 0, 0);
+ else
+ ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_READ,
+ num_bytes, base_addr, 0, 0, 0, 0);
+
+ return ret.error ? sbi_err_map_linux_errno(ret.error) : ret.value;
+}
+
void __init sbi_init(void)
{
int ret;
@@ -612,6 +650,11 @@ void __init sbi_init(void)
sbi_srst_reboot_nb.priority = 192;
register_restart_handler(&sbi_srst_reboot_nb);
}
+ if ((sbi_spec_version >= sbi_mk_version(2, 0)) &&
+ (sbi_probe_extension(SBI_EXT_DBCN) > 0)) {
+ pr_info("SBI DBCN extension detected\n");
+ sbi_debug_console_available = true;
+ }
} else {
__sbi_set_timer = __sbi_set_timer_v01;
__sbi_send_ipi = __sbi_send_ipi_v01;
--
2.34.1
