lkml.org 
[lkml]   [2016]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 3/6] ACPI: parse SPCR and enable matching console
From
Date
On 03/22/2016 10:04 AM, Aleksey Makarov wrote:
>
>
> On 03/22/2016 07:07 PM, Peter Hurley wrote:
>> On 03/22/2016 04:09 AM, Andy Shevchenko wrote:
>>> On Tue, Mar 22, 2016 at 12:46 PM, Aleksey Makarov
>>> <aleksey.makarov@linaro.org> wrote:
>
>>>> + sprintf(opts, "%s,%s,0x%llx,%d", uart, iotype,
>>>> + table->serial_port.address, baud_rate);
>>>
>>> You may use snprintf(), though my question here what would happen on
>>> 32-bit kernel when you supply 64-bit address as an option?
>>
>> Yeah this should probably use %pa for the printf specifier.
>>
>> But note this exposes underlying bug in the earlycon support, because
>> that was originally written without 32/64-mixed bitness in mind; ie.,
>> the address is parsed and handled as unsigned long in most places.
>
> I don't quite follow this. table->serial_port.address is explicitly u64,
> not pointer, so, according to printk-formats.txt %llx is ok here, %pa is wrong.
> Am I missing something?

No, you're right.
There is still the underlying bug I noted; I'll fix that in this release cycle.


>>>> /*
>>>> - * Just 'earlycon' is a valid param for devicetree earlycons;
>>>> - * don't generate a warning from parse_early_params() in that case
>>>> + * Just 'earlycon' is a valid param for devicetree and ACPI SPCR
>>>> + * earlycons; don't generate a warning from parse_early_params()
>>>> + * in that case
>>>> */
>>>> - if (!buf || !buf[0])
>>>> - return early_init_dt_scan_chosen_serial();
>>>> + if (!buf || !buf[0]) {
>>>> + init_spcr_earlycon();
>>>
>>>> + early_init_dt_scan_chosen_serial();
>>>> + return 0;
>>>
>>> And you hide an error?
>>
>> Well, this is a little bit tricky because "earlycon" early parameter with
>> missing /chosen/stdout-path node is no longer an error, since ACPI may be
>> specifying the earlycon instead.
>
> Agree, but note the email by Rob Herring. The code should be like this:
>
> if (!buf || !buf[0]) {
> if (acpi_disabled) {
> return early_init_dt_scan_chosen_serial();
> } else {
> init_spcr_earlycon();
> return 0;
> }
> }

Ok.


> But that requires to have made ACPI/DT decision at this point.

It's an unfortunate command-line option dependency, but I think it's ok
for the moment. The same problem would exist if DT could be disabled
via the command line.


>
> Thank you
> Aleksey
>

\
 
 \ /
  Last update: 2016-03-22 19:01    [W:1.189 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site