| Message ID | 1510222764-11746-3-git-send-email-bhupinder.thakur@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 09, 2017 at 03:49:24PM +0530, Bhupinder Thakur wrote: > The console was not working on HP Moonshot (HPE Proliant Aarch64) because > the UART registers were accessed as 8-bit aligned addresses. However, > registers are 32-bit aligned for HP Moonshot. > > Since ACPI/SPCR table does not specify the register shift to be applied to the > register offset, this patch implements an erratum to correctly set the register > shift for HP Moonshot. > > Similar erratum was implemented in linux: > > commit 79a648328d2a604524a30523ca763fbeca0f70e3 > Author: Loc Ho <lho@apm.com> > Date: Mon Jul 3 14:33:09 2017 -0700 > > ACPI: SPCR: Workaround for APM X-Gene 8250 UART 32-alignment errata > > APM X-Gene verion 1 and 2 have an 8250 UART with its register > aligned to 32-bit. In addition, the latest released BIOS > encodes the access field as 8-bit access instead 32-bit access. > This causes no console with ACPI boot as the console > will not match X-Gene UART port due to the lack of mmio32 > option. > > Signed-off-by: Loc Ho <lho@apm.com> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Any particular reason you offset this whole commit description by four spaces? > > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > --- > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Ian Jackson <ian.jackson@eu.citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Tim Deegan <tim@xen.org> > CC: Wei Liu <wei.liu2@citrix.com> > CC: Julien Grall <julien.grall@arm.com> > > xen/drivers/char/ns16550.c | 42 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 40 insertions(+), 2 deletions(-) This is v2 posting, but I don't see what changed. Usually you do something like this: v1: New posting v2: Nothing changed from v1. or v1: New posting v2: Added more folks on CC Added consts in XYZ.. > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c > index cf42fce..bb01c46 100644 > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -1517,6 +1517,33 @@ static int ns16550_init_dt(struct ns16550 *uart, > > #ifdef CONFIG_ACPI > #include <xen/acpi.h> > +/* > + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its > + * register aligned to 32-bit. In addition, the BIOS also encoded the > + * access width to be 8 bits. This function detects this errata condition. > + */ > +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb) > +{ > + bool xgene_8250 = false; > + > + if ( tb->interface_type != ACPI_DBG2_16550_COMPATIBLE ) > + return false; > + > + if ( memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE) && > + memcmp(tb->header.oem_id, "HPE ", ACPI_OEM_ID_SIZE) ) > + return false; > + > + if ( !memcmp(tb->header.oem_table_id, "XGENESPC", > + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0 ) > + xgene_8250 = true; Why not just 'return true' ? > + > + if ( !memcmp(tb->header.oem_table_id, "ProLiant", > + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1 ) > + xgene_8250 = true; And return true here too? > + > + return xgene_8250; And then this is just 'return false' and you don't have xgen_8250 on the stack? > +} > + > static int ns16550_init_acpi(struct ns16550 *uart, > const void *data) > { > @@ -1539,9 +1566,20 @@ static int ns16550_init_acpi(struct ns16550 *uart, > uart->io_base = spcr->serial_port.address; > uart->irq = spcr->interrupt; > uart->reg_width = spcr->serial_port.bit_width / 8; > - uart->reg_shift = 0; > - uart->io_size = UART_MAX_REG << uart->reg_shift; > > + if ( xgene_8250_erratum_present(spcr) ) > + { > + /* > + * for xgene v1 and v2 the registers are 32-bit and so a s/for/For/ > + * register shift of 2 has to be applied to get the > + * correct register offset. > + */ > + uart->reg_shift = 2; > + } > + else > + uart->reg_shift = 0; > + > + uart->io_size = UART_MAX_REG << uart->reg_shift; > irq_set_type(spcr->interrupt, spcr->interrupt_type); > > return 0; > -- > 2.7.4 >
On Nov 15, 2017, at 9:20 PM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > On Thu, Nov 09, 2017 at 03:49:24PM +0530, Bhupinder Thakur wrote: >> The console was not working on HP Moonshot (HPE Proliant Aarch64) because >> the UART registers were accessed as 8-bit aligned addresses. However, >> registers are 32-bit aligned for HP Moonshot. >> >> Since ACPI/SPCR table does not specify the register shift to be applied to the >> register offset, this patch implements an erratum to correctly set the register >> shift for HP Moonshot. >> >> Similar erratum was implemented in linux: >> >> commit 79a648328d2a604524a30523ca763fbeca0f70e3 >> Author: Loc Ho <lho@apm.com> >> Date: Mon Jul 3 14:33:09 2017 -0700 >> >> ACPI: SPCR: Workaround for APM X-Gene 8250 UART 32-alignment errata >> >> APM X-Gene verion 1 and 2 have an 8250 UART with its register >> aligned to 32-bit. In addition, the latest released BIOS >> encodes the access field as 8-bit access instead 32-bit access. >> This causes no console with ACPI boot as the console >> will not match X-Gene UART port due to the lack of mmio32 >> option. >> >> Signed-off-by: Loc Ho <lho@apm.com> >> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Any particular reason you offset this whole commit description by four spaces? I get this effect when I use “git show” to look at a changeset for some reason. Bhupinder, did you perhaps export a changeset as a patch using “git show” and then re-import it? In any case, this needs to be fixed. -George
Hi, On Thursday 16 November 2017 03:26 PM, George Dunlap wrote: > On Nov 15, 2017, at 9:20 PM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> On Thu, Nov 09, 2017 at 03:49:24PM +0530, Bhupinder Thakur wrote: >>> The console was not working on HP Moonshot (HPE Proliant Aarch64) because >>> the UART registers were accessed as 8-bit aligned addresses. However, >>> registers are 32-bit aligned for HP Moonshot. >>> >>> Since ACPI/SPCR table does not specify the register shift to be applied to the >>> register offset, this patch implements an erratum to correctly set the register >>> shift for HP Moonshot. >>> >>> Similar erratum was implemented in linux: >>> >>> commit 79a648328d2a604524a30523ca763fbeca0f70e3 >>> Author: Loc Ho <lho@apm.com> >>> Date: Mon Jul 3 14:33:09 2017 -0700 >>> >>> ACPI: SPCR: Workaround for APM X-Gene 8250 UART 32-alignment errata >>> >>> APM X-Gene verion 1 and 2 have an 8250 UART with its register >>> aligned to 32-bit. In addition, the latest released BIOS >>> encodes the access field as 8-bit access instead 32-bit access. >>> This causes no console with ACPI boot as the console >>> will not match X-Gene UART port due to the lack of mmio32 >>> option. >>> >>> Signed-off-by: Loc Ho <lho@apm.com> >>> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> Any particular reason you offset this whole commit description by four spaces? > I get this effect when I use “git show” to look at a changeset for some reason. Bhupinder, did you perhaps export a changeset as a patch using “git show” and then re-import it? > > In any case, this needs to be fixed. Yes I copied the commit message from git show. I will align the text. > -George > Regards, Bhupinder
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index cf42fce..bb01c46 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -1517,6 +1517,33 @@ static int ns16550_init_dt(struct ns16550 *uart, #ifdef CONFIG_ACPI #include <xen/acpi.h> +/* + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its + * register aligned to 32-bit. In addition, the BIOS also encoded the + * access width to be 8 bits. This function detects this errata condition. + */ +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb) +{ + bool xgene_8250 = false; + + if ( tb->interface_type != ACPI_DBG2_16550_COMPATIBLE ) + return false; + + if ( memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE) && + memcmp(tb->header.oem_id, "HPE ", ACPI_OEM_ID_SIZE) ) + return false; + + if ( !memcmp(tb->header.oem_table_id, "XGENESPC", + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0 ) + xgene_8250 = true; + + if ( !memcmp(tb->header.oem_table_id, "ProLiant", + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1 ) + xgene_8250 = true; + + return xgene_8250; +} + static int ns16550_init_acpi(struct ns16550 *uart, const void *data) { @@ -1539,9 +1566,20 @@ static int ns16550_init_acpi(struct ns16550 *uart, uart->io_base = spcr->serial_port.address; uart->irq = spcr->interrupt; uart->reg_width = spcr->serial_port.bit_width / 8; - uart->reg_shift = 0; - uart->io_size = UART_MAX_REG << uart->reg_shift; + if ( xgene_8250_erratum_present(spcr) ) + { + /* + * for xgene v1 and v2 the registers are 32-bit and so a + * register shift of 2 has to be applied to get the + * correct register offset. + */ + uart->reg_shift = 2; + } + else + uart->reg_shift = 0; + + uart->io_size = UART_MAX_REG << uart->reg_shift; irq_set_type(spcr->interrupt, spcr->interrupt_type); return 0;
The console was not working on HP Moonshot (HPE Proliant Aarch64) because the UART registers were accessed as 8-bit aligned addresses. However, registers are 32-bit aligned for HP Moonshot. Since ACPI/SPCR table does not specify the register shift to be applied to the register offset, this patch implements an erratum to correctly set the register shift for HP Moonshot. Similar erratum was implemented in linux: commit 79a648328d2a604524a30523ca763fbeca0f70e3 Author: Loc Ho <lho@apm.com> Date: Mon Jul 3 14:33:09 2017 -0700 ACPI: SPCR: Workaround for APM X-Gene 8250 UART 32-alignment errata APM X-Gene verion 1 and 2 have an 8250 UART with its register aligned to 32-bit. In addition, the latest released BIOS encodes the access field as 8-bit access instead 32-bit access. This causes no console with ACPI boot as the console will not match X-Gene UART port due to the lack of mmio32 option. Signed-off-by: Loc Ho <lho@apm.com> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> --- CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Ian Jackson <ian.jackson@eu.citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Tim Deegan <tim@xen.org> CC: Wei Liu <wei.liu2@citrix.com> CC: Julien Grall <julien.grall@arm.com> xen/drivers/char/ns16550.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)