Re: [PATCH v1 14/14] xen/riscv: add basic UART support

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1784 bytes --]


On 4/15/25 6:03 PM, Jan Beulich wrote:
> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/setup.c
>> +++ b/xen/arch/riscv/setup.c
>> @@ -4,11 +4,16 @@
>>   #include <xen/bug.h>
>>   #include <xen/bootfdt.h>
>>   #include <xen/compile.h>
>> +#include <xen/console.h>
>>   #include <xen/device_tree.h>
>>   #include <xen/init.h>
>>   #include <xen/irq.h>
>> +#include <xen/keyhandler.h>
> Why's this one needed?

It isn't needed anymore, just missed to drop.
I thought that it would be needed to test UART working fine by checking if Ctrl+AAA is working.

>
>>   #include <xen/mm.h>
>> +#include <xen/percpu.h>
>> +#include <xen/serial.h>
>>   #include <xen/shutdown.h>
>> +#include <xen/timer.h>
>>   #include <xen/vmap.h>
>>   #include <xen/xvmalloc.h>
>>   
>> @@ -73,6 +78,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>>   
>>       remove_identity_mapping();
>>   
>> +    percpu_init_areas();
> I'll trust you that it's needed now, but the addition looks unrelated here,
> and also isn't mentioned as intentional in the description.

I had  this patch when I used polling mode for UART, for this case percpu is used to receive
serial port info:
   struct serial_port *port = this_cpu(poll_port);

So percpu isn't really needed at the current development state. I'll drop this change or as an option
move to separate patch.

>> --- a/xen/drivers/char/Kconfig
>> +++ b/xen/drivers/char/Kconfig
>> @@ -3,7 +3,6 @@ config GENERIC_UART_INIT
>>   
>>   config HAS_NS16550
>>   	bool "NS16550 UART driver" if ARM
>> -	default n if RISCV
>>   	default y
> Just to double-check: Unlike Arm you don't want this to be user-(un)selectable
> on RISC-V?

Thanks for noticing that. I want to have this selectable by user. I will add RISC-V here.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 2923 bytes --]