On Tue, Jul 20, 2021 at 01:13:56PM +0800, Sean Anderson wrote:
> On 7/20/21 12:59 AM, Heinrich Schuchardt wrote:
> > Am 20. Juli 2021 03:11:34 MESZ schrieb Sean Anderson <sean...@gmail.com>:
> >> On 7/19/21 4:28 PM, Heinrich Schuchardt wrote:
> >>> Let the sbi command display:
> >>>
> >>> * SBI implementation version
> >>> * machine vendor ID
> >>> * machine architecture ID
> >>> * machine implementation ID
> >>>
> >>> With this patch the output for the HiFive Unmatched looks like
> >>>
> >>> => sbi
> >>> SBI 0.3
> >>> OpenSBI 0.9
> >>> Machine:
> >>> Vendor ID 489
> >>> Architecture ID 8000000000000007
> >>> Implementation ID 20181004
> >>> Extensions:
> >>> sbi_set_timer
> >>> sbi_console_putchar
> >>> sbi_console_getchar
> >>> sbi_clear_ipi
> >>> sbi_send_ipi
> >>> sbi_remote_fence_i
> >>> sbi_remote_sfence_vma
> >>> sbi_remote_sfence_vma_asid
> >>> sbi_shutdown
> >>> SBI Base Functionality
> >>> Timer Extension
> >>> IPI Extension
> >>> RFENCE Extension
> >>> Hart State Management Extension
> >>> System Reset Extension
> >>>
> >>> Signed-off-by: Heinrich Schuchardt
> >> <heinrich.schucha...@canonical.com>
> >>> ---
> >>> cmd/riscv/sbi.c | 19 ++++++++++++++++++-
> >>> 1 file changed, 18 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/cmd/riscv/sbi.c b/cmd/riscv/sbi.c
> >>> index 90c0811e14..c0db763ba7 100644
> >>> --- a/cmd/riscv/sbi.c
> >>> +++ b/cmd/riscv/sbi.c
> >>> @@ -59,13 +59,30 @@ static int do_sbi(struct cmd_tbl *cmdtp, int
> >> flag, int argc,
> >>> if (ret >= 0) {
> >>> for (i = 0; i < ARRAY_SIZE(implementations); ++i) {
> >>> if (ret == implementations[i].id) {
> >>> - printf("%s\n", implementations[i].name);
> >>> + printf("%s", implementations[i].name);
> >>> + ret = sbi_get_impl_version();
> >>> + if (ret > 0) {
> >>
> >> Shouldn't this have to check to ensure that i == 1?
> >
> > Other SBI implementions may also provide a version number. I did not
> > analyze how the should be formatted.
>
> Right, so shouldn't we default to the raw hex string?
+1
>
> >
> >>
> >>> + /* OpenSBI specific version encoding */
> >>> + printf(" %ld", ret >> 16);
> >>> + printf(".%ld", ret & 0xffff);
> >>> + }
> >>> + printf("\n");
> >>> break;
> >>> }
> >>> }
> >>> if (i == ARRAY_SIZE(implementations))
> >>> printf("Unknown implementation ID %ld\n", ret);
> >>> }
> >>> + printf("Machine:\n");
> >>> + ret = sbi_get_mvendorid();
> >>> + if (ret != -ENOTSUPP)
> >
> > Should we use an unsigned int as return value and 0 to indicate a missing
> > implementation? mvendorid is only 32 bits wide.
> >
> >>> + printf(" Vendor ID %lx\n", ret);
> >>
> >> perhaps %0.8lx? And should we decode the JEDEC id?
> >
> > Leading zeros won't add any meaning here. Splitting into the 25 bit and 7
> > bit subfields could be reasonable.
>
> I think this would be a good option.
+1 with 25, 7 split.
IMHO, perhaps we could show only the "numbers" of the continuation code and the
"decoded offset" ?
Something like: vendor ID: 489, continuation code: 9, offset decoded: 0x89.
Best regards,
Leo
>
> > Decoding could result in up to 2^26 digits. I don't want to wait for all of
> > these on my serial console.
>
> Well, they're only up to 12 continuation codes, so imposing a small maximum
> would likely not be too onerous.
>
> --Sean
