Re: [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting

["Jeremy Linton" <jeremy.linton@arm.com>]

Hi Marcin,

On 4/8/19 8:31 PM, Marcin Wojtas wrote:
> Hi Jeremy,
> 
> Thanks for the patch. However I played with booting with ACPI with and
> without your patch - I may be missing something, but I don't see a
> difference, when using earlycon (both
> 'earlycon=uart,mmio32,0xf0512000' and
> 'earlycon=uart8250,mmio32,0xf0512000'). Can you please show what
> booting scenario is fixed with the patch?

Sorry, should have been more clear, this patch fixes normal ACPI console 
boot. You shouldn't need any parameters at all with recent kernels (i've 
been testing with mainline/5.1rc3/4) to get the console and boot log. 
AKA no 'console=', earlyprintk, earlycon, etc line at all.


To fix earlycon by itself (so you can see the boot log with just 
'earlycon') only the baud rate change in the subject line is needed. But 
once I fixed that, I also wanted the console to work by default. That 
requires both the MMIO access type and the address to match between SPCR 
and the DSDT entry. Hence the long explanation about why I "broke" 
earlycon by changing the access type from GAS32 to GAS8. There is an 
existing "quirk" in the 8250_dw module for the armada parts for DT, and 
I suspect a similar thing is needed for ACPI (this might be a longer 
discussion elsewhere) because without it, I couldn't get a 
GAS32/MMIO32/reg_offset=2 combination to work. Hence my comment about 
changing the ACPI id for the console from the HISI identifier.




> 
> Thanks,
> Marcin
> 
> wt., 9 kwi 2019 o 02:33 Jeremy Linton <jeremy.linton@arm.com> napisał(a):
>>
>> The mcbin (and likely others) have a nonstandard uart clock.
>> This means that the earlycon programming will incorrectly set
>> the baud rate if it is specified. The way around this is to tell
>> the kernel to continue using the preprogrammed baud rate. This
>> is done by setting the baud to 0.
>>
>> Further, the SPCR and DSDT serial port need to match the port
>> address and port access type for the kernel to conclude they
>> are the same.
>>
>> So while ARM_GAS32 is correct for earlycon (it can be used alone
>> on the  kernel command line) by providing the reg-shift=2 value,
>> it also sets the io type to MMIO32, which doesn't match the DSDT
>> defined MMIO. This means that the actual console will never appear.
>> The obvious fix is to set reg-width=4 in DSDT, but that also changes
>> the accesssors to 32-bits (similarly to earlycon) and results in
>> console failure.
>>
>> So the less obvious fix, is to use the GAS8 specifier. This means
>> that earlycon needs to be fully specified as
>> earlycon=uart,mmio32,0xf0512000, but has the extremely useful feature
>> that the console default works without any user interaction.
>>
>> If in the future marvell decides to define their own ACPI id for the
>> console and upstream a quirk, the ARM_GAS8 portion of this should
>> be reverted.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   Silicon/Marvell/Armada7k8k/AcpiTables/Spcr.aslc | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Silicon/Marvell/Armada7k8k/AcpiTables/Spcr.aslc b/Silicon/Marvell/Armada7k8k/AcpiTables/Spcr.aslc
>> index e78bb9036f..06c7af069c 100644
>> --- a/Silicon/Marvell/Armada7k8k/AcpiTables/Spcr.aslc
>> +++ b/Silicon/Marvell/Armada7k8k/AcpiTables/Spcr.aslc
>> @@ -30,11 +30,11 @@ EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE Spcr = {
>>     { EFI_ACPI_RESERVED_BYTE,
>>       EFI_ACPI_RESERVED_BYTE,
>>       EFI_ACPI_RESERVED_BYTE },                                           // Reserved1[3]
>> -  ARM_GAS32 (FixedPcdGet64(PcdSerialRegisterBase)),                     // BaseAddress
>> +  ARM_GAS8 (FixedPcdGet64(PcdSerialRegisterBase)),                      // BaseAddress
>>     EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_INTERRUPT_TYPE_GIC,    // InterruptType
>>     0,                                                                    // Irq
>>     51,                                                                   // GlobalSystemInterrupt
>> -  EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_BAUD_RATE_115200,      // BaudRate
>> +  0,                                                                    // Keep Firmware Baud
>>     EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_PARITY_NO_PARITY,      // Parity
>>     EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_STOP_BITS_1,           // StopBits
>>     0,                                                                    // FlowControl
>> --
>> 2.20.1
>>