Re: [PATCH 2/2] cmd/sbi: add missing SBI information

[Leo Liang <ycliang@andestech.com>]

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 <seanga2@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.schuchardt@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