* [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting @ 2019-04-09 0:33 jeremy.linton 2019-04-09 1:31 ` Marcin Wojtas 2019-04-15 19:50 ` Ard Biesheuvel 0 siblings, 2 replies; 8+ messages in thread From: jeremy.linton @ 2019-04-09 0:33 UTC (permalink / raw) To: devel; +Cc: mw, ard.biesheuvel, leif.lindholm, michael.d.kinney, Jeremy Linton 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting 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-15 19:50 ` Ard Biesheuvel 1 sibling, 1 reply; 8+ messages in thread From: Marcin Wojtas @ 2019-04-09 1:31 UTC (permalink / raw) To: Jeremy Linton; +Cc: devel, Ard Biesheuvel, Leif Lindholm, Kinney, Michael D 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? 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting 2019-04-09 1:31 ` Marcin Wojtas @ 2019-04-06 13:20 ` Jeremy Linton 2019-04-09 6:15 ` Marcin Wojtas 0 siblings, 1 reply; 8+ messages in thread From: Jeremy Linton @ 2019-04-06 13:20 UTC (permalink / raw) To: Marcin Wojtas; +Cc: devel, Ard Biesheuvel, Leif Lindholm, Kinney, Michael D 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 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting 2019-04-06 13:20 ` Jeremy Linton @ 2019-04-09 6:15 ` Marcin Wojtas 2019-04-09 17:51 ` Ard Biesheuvel [not found] ` <1593DFB9FE935497.8563@groups.io> 0 siblings, 2 replies; 8+ messages in thread From: Marcin Wojtas @ 2019-04-09 6:15 UTC (permalink / raw) To: Jeremy Linton; +Cc: devel, Ard Biesheuvel, Leif Lindholm, Kinney, Michael D 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 > > > > > > > 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 > >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting 2019-04-09 6:15 ` Marcin Wojtas @ 2019-04-09 17:51 ` Ard Biesheuvel [not found] ` <1593DFB9FE935497.8563@groups.io> 1 sibling, 0 replies; 8+ messages in thread From: Ard Biesheuvel @ 2019-04-09 17:51 UTC (permalink / raw) To: Marcin Wojtas Cc: Jeremy Linton, devel, Ard Biesheuvel, Leif Lindholm, Kinney, Michael D 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 > > >> > > ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <1593DFB9FE935497.8563@groups.io>]
* Re: [edk2-devel] [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting [not found] ` <1593DFB9FE935497.8563@groups.io> @ 2019-04-09 20:08 ` Ard Biesheuvel 0 siblings, 0 replies; 8+ messages in thread From: Ard Biesheuvel @ 2019-04-09 20:08 UTC (permalink / raw) To: devel, Ard Biesheuvel Cc: Marcin Wojtas, Jeremy Linton, Leif Lindholm, Kinney, Michael D On Tue, 9 Apr 2019 at 10:51, Ard Biesheuvel via Groups.Io <ard.biesheuvel=linaro.org@groups.io> wrote: > > 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> > 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 <ard.biesheuvel@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 > > > >> > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting 2019-04-09 0:33 [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting jeremy.linton 2019-04-09 1:31 ` Marcin Wojtas @ 2019-04-15 19:50 ` Ard Biesheuvel 2019-04-17 16:35 ` Jeremy Linton 1 sibling, 1 reply; 8+ messages in thread From: Ard Biesheuvel @ 2019-04-15 19:50 UTC (permalink / raw) To: edk2-devel-groups-io, Jeremy Linton Cc: Marcin Wojtas, Leif Lindholm, Kinney, Michael D On Mon, 8 Apr 2019 at 17:33, Jeremy Linton <jeremy.linton@arm.com> wrote: > > 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. > Given the breakage on the BSDs and Marcin's followup patch, I'd like to understand why this breaks. To me, it seems we are conflating register bit width with minimum access size. However, as far as I can tell, mmio32 sets both the register width and access size to 32 bits. So is the reason that it works for earlycon that we only use it for output? > 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 > > > ------------ > Groups.io Links: You receive all messages sent to this group. > > View/Reply Online (#38670): https://edk2.groups.io/g/devel/message/38670 > Mute This Topic: https://groups.io/mt/30980002/1761188 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ard.biesheuvel@linaro.org] > ------------ > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH] Marvell/Armada7k8k: Remove SPCR baud rate setting 2019-04-15 19:50 ` Ard Biesheuvel @ 2019-04-17 16:35 ` Jeremy Linton 0 siblings, 0 replies; 8+ messages in thread From: Jeremy Linton @ 2019-04-17 16:35 UTC (permalink / raw) To: Ard Biesheuvel, edk2-devel-groups-io Cc: Marcin Wojtas, Leif Lindholm, Kinney, Michael D Hi, On 4/15/19 2:50 PM, Ard Biesheuvel wrote: > On Mon, 8 Apr 2019 at 17:33, Jeremy Linton <jeremy.linton@arm.com> wrote: >> >> 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. >> > > Given the breakage on the BSDs and Marcin's followup patch, I'd like > to understand why this breaks. > > To me, it seems we are conflating register bit width with minimum > access size. However, as far as I can tell, mmio32 sets both the > register width and access size to 32 bits. So is the reason that it > works for earlycon that we only use it for output? Yes, I went back and looked at the SPCR/earlycon code and that is likely the core problem. It should be passing both the access width and the register size. I will look at tossing a patch in the next couple days. I'm wondering how many of the quirks are in there to work around this deficiency. As far as the mcbin, that appears to be the case, just writing the output shift register at the wrong width (32-bit rather than 8) seems to be ok (per earlycon), but anything beyond that is a problem. > >> 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 >> >> >> ------------ >> Groups.io Links: You receive all messages sent to this group. >> >> View/Reply Online (#38670): https://edk2.groups.io/g/devel/message/38670 >> Mute This Topic: https://groups.io/mt/30980002/1761188 >> Group Owner: devel+owner@edk2.groups.io >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ard.biesheuvel@linaro.org] >> ------------ >> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-17 16:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
[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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox