public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ard Biesheuvel" <ard.biesheuvel@linaro.org>
To: Marcin Wojtas <mw@semihalf.com>
Cc: Jeremy Linton <jeremy.linton@arm.com>,
	devel@edk2.groups.io,  Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Leif Lindholm <leif.lindholm@linaro.org>,
	 "Kinney, Michael D" <michael.d.kinney@intel.com>
Subject: Re: [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting
Date: Tue, 9 Apr 2019 10:51:14 -0700	[thread overview]
Message-ID: <CAKv+Gu-TBrzGqZwL85iE0G5Q+5HomVk+t9gMpgT+UPGxGu7ccw@mail.gmail.com> (raw)
In-Reply-To: <CAPv3WKdE44uRvbB0rUoTEwfFM9Dd0-LekjcQmLYtR9xCJNWZ4A@mail.gmail.com>

On Mon, 8 Apr 2019 at 23:16, Marcin Wojtas <mw@semihalf.com> wrote:
>
> Jeremy,
>
> wt., 9 kwi 2019 o 06:03 Jeremy Linton <jeremy.linton@arm.com> napisał(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=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.
> >
>
> 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. 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.
> >
>
> 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 <mw@semihalf.com>
>
> Best regards,
> Marcin
>

Reviewed-by: Leif Lindholm <leif.lindholm at linaro.org>

Pushed as 0a32c15d2172..7d8dc6544c93

Thanks,

> >
> >
> >
> > >
> > > 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
> > >>
> >

  reply	other threads:[~2019-04-09 17:51 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-09  0:33 [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting jeremy.linton
2019-04-09  1:31 ` Marcin Wojtas
2019-04-06 13:20   ` Jeremy Linton
2019-04-09  6:15     ` Marcin Wojtas
2019-04-09 17:51       ` Ard Biesheuvel [this message]
     [not found]       ` <1593DFB9FE935497.8563@groups.io>
2019-04-09 20:08         ` [edk2-devel] " Ard Biesheuvel
2019-04-15 19:50 ` Ard Biesheuvel
2019-04-17 16:35   ` Jeremy Linton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKv+Gu-TBrzGqZwL85iE0G5Q+5HomVk+t9gMpgT+UPGxGu7ccw@mail.gmail.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox