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

[Heinrich Schuchardt]

Am 20. Juli 2021 03:11:34 MESZ schrieb Sean Anderson <seanga2 at 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 at 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.

>
>> +					/* 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.

Decoding could result in up to 2^26 digits. I don't want to wait for all of these on my serial console.

Best regards

Heinrich

>
>> +	ret = sbi_get_marchid();
>> +	if (ret != -ENOTSUPP)
>> +		printf("  Architecture ID %lx\n", ret);
>> +	ret = sbi_get_mimpid();
>> +	if (ret != -ENOTSUPP)
>> +		printf("  Implementation ID %lx\n", ret);
>>   	printf("Extensions:\n");
>>   	for (i = 0; i < ARRAY_SIZE(extensions); ++i) {
>>   		ret = sbi_probe_extension(extensions[i].id);
>> --
>> 2.30.2
>>