Hello Jan,
> On 20 Nov 2020, at 12:14 am, Stefano Stabellini <sstabell...@kernel.org>
> wrote:
>
> On Thu, 19 Nov 2020, Julien Grall wrote:
>> On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabell...@kernel.org>
>> wrote:
>> On Thu, 19 Nov 2020, Rahul Singh 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.
>>
>>
>> It doesn't matter too much to me, let's just choose one option so that
>> you
>> get unblocked soon.
>>
>> It looks like Jan prefers option 2) and both Julien and I are OK with
>> it. So let's do 2). Jan, please confirm too :-)
>>
>>
>> Please don't put words in my mouth...
>
> Sorry Julien, I misinterpreted one of your previous comments. Sometimes
> it is difficult to do things by email. It is good that you clarified as
> my goal was to reach an agreement.
>
>
>> I think introducing HAS_PCI_MSI is short sighted.
>>
>> There are no clear benefits of it when NS16550 PCI support is not going to
>> be enable in the foreseeable future.
>
> I agree
>
>
>> I would be ok with moving everything under CONFIG_X86. IHMO this is still
>> shortsighted but at least we don't introduce a config that's not
>> going to help Arm or other any architecture to disable completely PCI
>> support in NS16550.
>
> So you are suggesting a new option:
>
> 3. Guard the remaining x86 specific code *and* the MSI related
> compilation errors with CONFIG_X86
>
> Is that right?
>
>
> My preference is actually option 1) but this series is already at v3 and
> I don't think this decision is as important as much as unblocking
> Rahul, so I am OK with the other alternatives too.
>
> I tend to agree with you that 3) is better than 2) for the reasons you
> wrote above.
Can you please provide your suggestion how to proceed on this so that I can
send my next patch.
I am waiting for your reply if you are also ok for the options 3.
Thanks in advance.
Regards,
Rahul
