Hello,
> On 19 Nov 2020, at 10:16 am, Julien Grall <jul...@xen.org> wrote:
>
>
>
> On 19/11/2020 09:53, Jan Beulich wrote:
>> On 19.11.2020 10:21, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 19/11/2020 09:05, Jan Beulich wrote:
>>>> On 18.11.2020 16:50, Julien Grall wrote:
>>>>> On 16/11/2020 12: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.
>>>>> >
>>>>>> Introducing new kconfig option CONFIG_HAS_NS16550_PCI to support
>>>>>> ns16550 PCI for X86.
>>>>>>
>>>>>> For X86 platforms it is enabled by default. For ARM platforms it is
>>>>>> disabled by default, once we have proper support for NS16550 PCI for
>>>>>> ARM we can enable it.
>>>>>>
>>>>>> No functional change.
>>>>>
>>>>> NIT: I would say "No functional change intended" to make clear this is
>>>>> an expectation and hopefully will be correct :).
>>>>>
>>>>> Regarding the commit message itself, I would suggest the following to
>>>>> address Jan's concern:
>>>>
>>>> While indeed this is a much better description, I continue to think
>>>> that the proposed Kconfig option is undesirable to have.
>>>
>>> I am yet to see an argument into why we should keep the PCI code
>>> compiled on Arm when there will be no-use....
>> Well, see my patch suppressing building of quite a part of it.
>
> I will let Rahul figuring out whether your patch series is sufficient to fix
> compilation issues (this is what matters right now).
I just checked the compilation error for ARM after enabling the HAS_PCI on ARM.
I am observing the same compilation error what I observed previously.
There are two new errors related to struct uart_config and struct part_param as
those struct defined globally but used under X86 flags.
At top level:
ns16550.c:179:48: error: ‘uart_config’ defined but not used
[-Werror=unused-const-variable=]
static const struct ns16550_config __initconst uart_config[] =
^~~~~~~~~~~
ns16550.c:104:54: error: ‘uart_param’ defined but not used
[-Werror=unused-const-variable=]
static const struct ns16550_config_param __initconst uart_param[] = {
>
>>>> Either,
>>>> following the patch I've just sent, truly x86-specific things (at
>>>> least as far as current state goes - if any of this was to be
>>>> re-used by a future port, suitable further abstraction may be
>>>> needed) should be guarded by CONFIG_X86 (or abstracted into arch
>>>> hooks), or the HAS_PCI_MSI proposal would at least want further
>>>> investigating as to its feasibility to address the issues at hand.
>>>
>>> I would be happy with CONFIG_X86, despite the fact that this is only
>>> deferring the problem.
>>>
>>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given
>>> that we are not going to use NS16550 PCI on Arm in the forseeable
>>> future.
>> And I continue to fail to see what would guarantee this: As soon
>> as you can plug in such a card into an Arm system, people will
>> want to be able use it. That's why we had to add support for it
>> on x86, after all.
>
> Well, plug-in PCI cards on Arm has been available for quite a while... Yet I
> haven't heard anyone asking for NS16550 PCI support.
>
> This is probably because SBSA compliant server should always provide an SBSA
> UART (a cut-down version of the PL011). So why would bother to lose a PCI
> slot for yet another UART?
>
>> >> So why do we need a finer graine Kconfig?
>> Because most of the involved code is indeed MSI-related?
>
> Possibly, yet it would not be necessary if we don't want NS16550 PCI
> support...
>
To fix compilation error on ARM as per the discussion there are below options
please suggest which one to use to proceed further.
1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps
also non-x86 architecture in the future not to have compilation error
what we are observing now when HAS_PCI is enabled.
2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new
CONFIG_HAS_PCI_MSI options to fix the MSI related compilation error.
Once we have proper support for MSI and PCI for ARM (HAS_PCI_MSI and HAS_PCI
enabled for ARM in Kconfig ) I am not sure if NS16550 PCI will work out of the
box on ARM .In that case, we might need to come back again to fix NS16550
driver.
> Cheers,
>
> --
> Julien Grall
Regards,
Rahul
