From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=P8pxdWjE; spf=pass (domain: linaro.org, ip: 209.85.166.196, mailfrom: ard.biesheuvel@linaro.org) Received: from mail-it1-f196.google.com (mail-it1-f196.google.com [209.85.166.196]) by groups.io with SMTP; Tue, 09 Apr 2019 13:08:25 -0700 Received: by mail-it1-f196.google.com with SMTP id y10so7159664itc.1 for ; Tue, 09 Apr 2019 13:08:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=tqyF39UaA2dK3bELuIQ1SDeMrPYVxnMMYnsKldCOMzI=; b=P8pxdWjEC++zAYb7+cUTJo2qdRFaY79q8oXq60FLNU8sRvi78l9n5WBquJiFutveKq dvEMesQJqzPF7txXGUr7t5e+FuFo4uwoXaVWj4MpYr92mcCcFK9+HkY8lyiSg3DTwE8v ehj+BmWqX1ZBO45hhsy5REjJH2ljxffkKuPv44h7BqyY5mUxuHpkQm1TbVDvSer7b/Mm NNheIrD+2rGIZ4xhbMXjyYKAoKO8HoVnD+8x8okAbUvMvyOYHxIxzsuDH/lVdWuoL2ZP cHwU/Bk+m9m+x5e5NXeAJJrkgyok5pm7xbAR76Sw4MV6f/yqcxv+qDiqcVTGbfEezKCC qO8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=tqyF39UaA2dK3bELuIQ1SDeMrPYVxnMMYnsKldCOMzI=; b=KsvhaJFKwsw7lwMdCZrva9KriR2wD8VjDRIOaieLI//c3AjI6IPuYrnUegUS0aY1BX 1+V/vGHwyfQC3RsaYllqwFFBscPzqYosuVbIYUYJogoWz89dfpjJynuXbdqs0xQhpnz+ NnBS0ClmBmfe8kADZnNFUIvVF++ykq4tHUF9Fd/rMimLNe0KMyvomnOrt1Pwfw7BdK0X OpWqUhsqQt6GXh0fZ8ATdUpg+mi/ANba9Ogbu3lWwvgn9X4ZoFbf1vn186lKDWOp9M5Q tiWLYNfYgoXMrZWv7JQ/j64AsZ7IQ1q2RDZNJZG/2INhIceqbAnsK2jULcO/6oTmfs7u NWdA== X-Gm-Message-State: APjAAAXsmPhGxMuCeW+BvfqjNkCRO4tv4SjLzZ17Q7fVGyltJ8xgT/BB cTKuahKVWSwLpGIgE/QTP725FKV6LUuOva0M5Yo5KRkNkZRP0nEr X-Google-Smtp-Source: APXvYqyKbfQx+5I5o5dFQVExiyxpep1UjUILHaGOYrthEc389S6W8v06Nis1TG/dlPWgiF6ckoHZC+Z+OD9y4UkVPZ8= X-Received: by 2002:a24:41cd:: with SMTP id b74mr180822itd.100.1554840504225; Tue, 09 Apr 2019 13:08:24 -0700 (PDT) MIME-Version: 1.0 References: <20190409003327.3797-1-jeremy.linton@arm.com> <1593DFB9FE935497.8563@groups.io> In-Reply-To: <1593DFB9FE935497.8563@groups.io> From: "Ard Biesheuvel" Date: Tue, 9 Apr 2019 13:08:13 -0700 Message-ID: Subject: Re: [edk2-devel] [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting To: devel@edk2.groups.io, Ard Biesheuvel Cc: Marcin Wojtas , Jeremy Linton , Leif Lindholm , "Kinney, Michael D" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 9 Apr 2019 at 10:51, Ard Biesheuvel via Groups.Io wrote: > > On Mon, 8 Apr 2019 at 23:16, Marcin Wojtas wrote: > > > > Jeremy, > > > > wt., 9 kwi 2019 o 06:03 Jeremy Linton napisa= =C5=82(a): > > > > > > 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=3Duart,mmio32,0xf0512000' and > > > > 'earlycon=3Duart8250,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 con= sole > > > 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=3D', earlyprintk, earlycon, etc line at all. > > > > > > > I took the v5.1-rc3 and indeed it now boots without any parameter. Tha= t's great. > > > > > > > > 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. Tha= t > > > 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=3D2 combination to work. Hence my comment ab= out > > > changing the ACPI id for the console from the HISI identifier. > > > > > > > I always used earlycon with full parameter list, so I don't consider > > the change as very problematic :) You can add my: > > Tested-by: Marcin Wojtas > > > > Best regards, > > Marcin > > > > Reviewed-by: Leif Lindholm > Whoops, due to a recursive copy/paste error on my part, I ended up pasting this into the email, but the actual patch got pushed correctly as Reviewed-by: Ard Biesheuvel > Pushed as 0a32c15d2172..7d8dc6544c93 > > Thanks, > > > > > > > > > > > > > > > > > > Thanks, > > > > Marcin > > > > > > > > wt., 9 kwi 2019 o 02:33 Jeremy Linton napi= sa=C5=82(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=3D2 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 appea= r. > > > >> The obvious fix is to set reg-width=3D4 in DSDT, but that also ch= anges > > > >> 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=3Duart,mmio32,0xf0512000, but has the extremely useful f= eature > > > >> 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 > > > >> --- > > > >> Silicon/Marvell/Armada7k8k/AcpiTables/Spcr.aslc | 4 ++-- > > > >> 1 file changed, 2 insertions(+), 2 deletions(-) > > > >> > > > >> diff --git a/Silicon/Marvell/Armada7k8k/AcpiTables/Spcr.aslc b/Si= licon/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_TABL= E Spcr =3D { > > > >> { 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_11520= 0, // BaudRate > > > >> + 0, = // Keep Firmware Baud > > > >> EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_PARITY_NO_PARI= TY, // Parity > > > >> EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_STOP_BITS_1, = // StopBits > > > >> 0, = // FlowControl > > > >> -- > > > >> 2.20.1 > > > >> > > > > >=20 >