[U-Boot] [PATCH v2 5/5] x86: acpi: Generate SPCR table

[Alexander Graf]

On 15.11.18 20:43, Andy Shevchenko wrote:
> On Thu, Nov 15, 2018 at 8:27 PM Alexander Graf <agraf at suse.de> wrote:
>> On 15.11.18 18:58, Andy Shevchenko wrote:
>>> Microsoft specifies a SPCR (Serial Port Console Redirection Table) [1].
>>> Let's provide it in U-Boot.
>>>
>>> [1] https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx
> 
>>> +     ret = serial_getinfo(&info);
>>> +     if (ret)
>>> +             spcr->interface_type = ACPI_DBG2_UNKNOWN;
>>> +     else
>>> +             spcr->interface_type = ACPI_DBG2_16550_COMPATIBLE;
>>
>> This sounds like pretty subtle semantics. Could you just include the
>> interface type in &info?
> 
> It might be another step if you provide a cool way to do so. Otherwise

Define an enum with serial_type where 0 is unknown and 1 is
16550_compatible. Provide that enum in getinfo().

Bonus points for keeping the enum and ACPI_DBG2 IDs in sync, so that we
don't need too much code to copy from one to the other.

> we need first to de-x86 acpi stuff. I won't do it right now since it
> so-o out of the scope of this series.

Well, we will probably want to be able to do that in the future. And
anything that hard codes more x86 assumptions is a step in the wrong
direction.

> 
>> The main problem I'm seeing is that your UART might be a PL011
>> compatible which could still implement getinfo() but then wouldn't be
>> 16660 compatible.
> 
> Yes, I know. Perhaps I forgot to mark it in "Known issues" section.
> 
> Have you seen, btw, PL011 on x86 environment?

I can create one for you in QEMU if you like?

> 
>> Are any higher (and lower) ones specified too? If so, you may want to
>> add them as well.
> 
> There is a link to a specification. Care to read?

A simple "no, the spec only defines the ones above" would've been a
better answer and saved both of us a few seconds of our lives ;).

> 
>>> +     default:
>>> +             spcr->baud_rate = 0;
>>> +             break;
> 
> Going ahead.
> This part actually something to develop through specifications, but I
> need to ping Microsoft and our internal ACPI people for that.
> 
>>> +             break;
>>> +     default:
>>> +             access_size = ACPI_ACCESS_SIZE_UNDEFINED;
>>> +             break;
>>> +     }
>>> +
>>> +     spcr->serial_port.space_id = ACPI_ADDRESS_SPACE_MEMORY;
>>
>> Can you guarantee that? Should probably be part of &info as well if addr is.
> 
> Not now.
> As per moving to info, see my comment about de-x86 of ACPI in U-Boot.

I don't understand how describing your address space correctly has
anything to do with de-x86'ing anything? x86 is the only (living) user
of non-MMIO address space out there.

> 
>>> +     /* REVISIT: Hard coded values for now */
>>> +     spcr->parity = 0;
>>> +     spcr->stop_bits = 1;
>>
>> IMHO those should be part of &info as well. If you have to hard code
>> them, better hard code them in the driver rather than the generic ACPI
>> generation code.
> 
> Care to read cover letter carefully?

Again, if you think your patch is terrible and shouldn't be applied
anyway, please mark it as RFC. I'll be happy to wait with review until
you've made up your mind then :).


Alex