Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2020 16:02, Rahul Singh wrote:
> Hello Jan,
> 
>> On 17 Nov 2020, at 10:55 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 16.11.2020 13:25, Rahul Singh wrote:
>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>>> is enabled for ARM, compilation error is observed for ARM architecture
>>> because ARM platforms do not have full PCI support available.
>>
>> While you've extended the sentence, it remains unclear to me what
>> compilation error it is that results here. I've requested such
>> clarification for v2 already.
> 
> Compilation error is related to the code that refer to x86  functions 
> (create_irq()..) and MSI implementation related error. 
> For more details please find the attached file for compilation error.

The use of mmio_ro_ranges is almost quite possibly going to remain
x86-specific, but then I guess this wants abstracting in a suitable
way.

The remaining look to all be MSI-related, so perhaps what you want
to avoid is just that part rather than everything PCI-ish?

>>> --- a/xen/drivers/char/ns16550.c
>>> +++ b/xen/drivers/char/ns16550.c
>>> @@ -16,7 +16,7 @@
>>> #include <xen/timer.h>
>>> #include <xen/serial.h>
>>> #include <xen/iocap.h>
>>> -#ifdef CONFIG_HAS_PCI
>>> +#ifdef CONFIG_HAS_NS16550_PCI
>>> #include <xen/pci.h>
>>> #include <xen/pci_regs.h>
>>> #include <xen/pci_ids.h>
>>
>> ... #undef-ining CONFIG_HAS_PCI at a suitable position in this
>> file (e.g. after all #include-s, to make sure all structure
>> layouts remain correct)? This would then be far easier to revert
>> down the road, and would confine the oddity to a single file
>> (and there a single place) in the code base.
>>
> 
> As for ARM platforms, PCI implementation is in the development process and I 
> am not sure if after completion of PCI work, ns16500 PCI part of code will 
> work out of the box. I think there is some effort required to test the 
> ns16550 PCI part of the code on ARM.
> As this code is tested on X86 only so I make the options depends on X86 and 
> enable it by default for x86.  
> 
> I feel that adding a new Kconfig options is ok to enable/disable the PCI 
> NS16550 support as compared to #undef CONFIG_HAS_PCI in the particular file. 
> If in future other architecture wants to implement the PCI they will face the 
> similar compilation error issues.
> 
> Please suggest how we can proceed on this.

Introduce CONFIG_HAS_PCI_MSI (selected only by x86), if there's no
immediate plan to support it on Arm together with the rest of PCI?

Jan