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

[Sean Anderson]

On 7/20/21 12:59 AM, Heinrich Schuchardt wrote:
> 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.

Right, so shouldn't we default to the raw hex string?

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

> 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