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=XLxaGhRo; spf=pass (domain: linaro.org, ip: 209.85.166.67, mailfrom: ard.biesheuvel@linaro.org) Received: from mail-io1-f67.google.com (mail-io1-f67.google.com [209.85.166.67]) by groups.io with SMTP; Tue, 09 Apr 2019 10:51:26 -0700 Received: by mail-io1-f67.google.com with SMTP id v4so15100442ioj.5 for ; Tue, 09 Apr 2019 10:51: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=UbA5DbqUJBBJmEjQGEW+4CZsVTU0NvcnGsONcsFgyYk=; b=XLxaGhRos1V4i7MyU8fT7XTbqhNQmAR+2B+m4VmRg10x2Bry8Jjld5IS2dkak6BeH8 UMhZ7I7ctGehL0DkOZj/q2y8hqYPZoMS6dtgUFwfm/wT1qESgWpdfmRgjTfkFrS342Ld jxEusvtGbXkUBNexLqRdKysS00lE0SxJMBOuBGOwp12XiuKE9dMAhMMNxAujTLhs+B0m y/sFj9mq0SC9u/94tsRhTnjnidLpaE+UKVUKhRoW+sUJt9Ho1mGojqnFvpYV8ioIBu72 MOjwBG+bUHuD0ytJ1Q+aZUs3yarP43qOXTk9C1UP4MQf1OvAO+8p1SrnIKO6hqFlvlXQ MgHQ== 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=UbA5DbqUJBBJmEjQGEW+4CZsVTU0NvcnGsONcsFgyYk=; b=XBz/sBa0nwXKWPZhDE2ojBcbI3Liem7UpNypCKtBZJWXW+GqYfj2O1jWDwbU8zwKn1 2SEERaWxeg106t+ojKtlE0UmiPPLzXtL7COAxq8Xm2Mbzlot3VoDiGc/opoOZdYhuUAX Az0/jyGFwrw78Dxu6Q1mmlrM4r+NZ3/KliipHDGZl/mskt2BAL7bjwzEqtgaFUWuDFtP 8RujvgUIvXNw5JoCaJK3ZpGbXHTaU3JULnf4AeS7+v93i4c/aue5JVeHLpTHnVUaLZZm PLbNn59/Vqi05ttw5i6RUCbevhG+YEsXV5+HJoTt6+Oj4ETHvQx5dsNcmFd3ELTvomOD IRpw== X-Gm-Message-State: APjAAAVUIthxDIcptew9eyhmC8Elb/LAmWRb0jnudOWiBcnz9UDfwVSb KDvGtj85g0O33GpPtmJyuuuaSsCF36FpcgPgfS45eA== X-Google-Smtp-Source: APXvYqwz1RjAyDMeHEcOh3iP8RXBYYst5TgkRLNzGNOAgXELwfAXfoGgMLokSx8z2SsrMMrXb6IB8UBj1jDOrPVcKyY= X-Received: by 2002:a5d:840d:: with SMTP id i13mr17601616ion.186.1554832285179; Tue, 09 Apr 2019 10:51:25 -0700 (PDT) MIME-Version: 1.0 References: <20190409003327.3797-1-jeremy.linton@arm.com> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 9 Apr 2019 10:51:14 -0700 Message-ID: Subject: Re: [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting To: Marcin Wojtas Cc: Jeremy Linton , devel@edk2.groups.io, Ard Biesheuvel , Leif Lindholm , "Kinney, Michael D" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 an= d > > > 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 consol= e > > boot. You shouldn't need any parameters at all with recent kernels (i'v= e > > 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. That'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. Bu= t > > 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 SPC= R > > 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 about > > 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 Pushed as 0a32c15d2172..7d8dc6544c93 Thanks, > > > > > > > > > > > > Thanks, > > > Marcin > > > > > > wt., 9 kwi 2019 o 02:33 Jeremy Linton napisa= =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 appear. > > >> The obvious fix is to set reg-width=3D4 in DSDT, but that also chang= es > > >> 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 feat= ure > > >> 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/Silic= on/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 S= pcr =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_GI= C, // 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 > > >> > >