Re: [PATCH] ACPI: SPCR: Use access width to determine mmio usage

[Jon Mason]

On Mon, May 08, 2017 at 01:51:20PM -0700, Loc Ho wrote:
> Hi Jon,
> 
> On Mon, May 8, 2017 at 1:34 PM, Jon Mason <jon.mason@xxxxxxxxxxxx> wrote:
> > On Mon, May 8, 2017 at 3:57 PM, Loc Ho <lho@xxxxxxx> wrote:
> >> Hi Jon,
> >>
> >>>>
> >>>>>> The current SPCR code does not check the access width of the mmio, and
> >>>>>> uses a default of 8bit register accesses.  This prevents devices that
> >>>>>> only do 16 or 32bit register accesses from working.  By simply checking
> >>>>>> this field and setting the mmio string appropriately, this issue can be
> >>>>>> corrected.  To prevent any legacy issues, the code will default to 8bit
> >>>>>> accesses if the value is anything but 16 or 32.
> >>>>>
> >>>>> Thanks for this. Just as an FYI I've a running discussion with Microsoft
> >>>>> about defining additional UART subtypes in the DBG2 for special case
> >>>>> UARTs. Specifically, I want to address AppliedMicro's special 8250 dw IP
> >>>>> that also has a non-standard clock. At this time, there is general
> >>>>> agreement to use the access width for some cases rather than defining
> >>>>> yet more subtypes - so your patch is good.
> >>>>>
> >>>>> Loc/Applied: please track this thread, incorporate feedback, and also
> >>>>> track the other general recent discussions of 8250 dw from this week.
> >>>>
> >>>> Thanks for forward me this patch. This patch does not work with X-Gene
> >>>> v1 and v2 SoC's. As BIOS SPCR encodes these info as:
> >>>>
> >>>> Bit Width: 32
> >>>> Bit Offset: 0
> >>>> Encoded Access Width: 01 (Byte Access)
> >>>>
> >>>> With this patch, it would use the "mmio" instead the "mmio32" as with
> >>>> this patch - https://patchwork.kernel.org/patch/9460959
> >>>
> >>> I think this is why we need the DBG2 subtype for Applied X-Gene1. I'm
> >>> hoping the update to the SPCR/DBG2 spec is done soon.
> >>
> >> We can't rely on the BIOS change to support this new subtype as we
> >> have system that is already in production deployment. When these
> >> system upgrade to new version of the OS (stock, RHELSA, or whatever),
> >> they will break. We need the patch from
> >> https://patchwork.kernel.org/patch/9460959/ rolled upstream.
> >
> > There is no reason why the patch you reference cannot co-exist with
> > the one I am submitting here.  In this case, my patch would set it to
> > mmio, then the patch you link above would reset it to mmio32.
> > Personally, I would recommend a big, fat comment on why this extra
> > step is necessary, but it should work as desired.  Alternatively, we
> > could add some kind of quirk library (similar to
> > qdf2400_erratum_44_present) where the OEM/OEM Table ID is referenced
> > and workaround applied.  Thoughts?
> 
> That's was my first version but after seeing both versions, I think
> they are better solution as it works for more SoC's than just our. As
> you had suggested, we should apply your patch and
> https://patchwork.kernel.org/patch/9460959. The third patch -
> https://patchwork.kernel.org/patch/9462183/ - conflicts with your.
> 
> Summary:
> 1. Applied your - https://lkml.org/lkml/2017/5/4/450
> 2. Applied this one - https://patchwork.kernel.org/patch/9460959/
> 
> -Loc

What if we simply applied the following (100% untested) patch to add
the quirk framework I was suggesting?  It can be applied on top of the
patch I submitted previously.


diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index 63b690d142f1..c87569b345e3 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -36,6 +36,14 @@ static bool qdf2400_erratum_44_present(struct acpi_table_header *h)
 	return false;
 }
 
+static void spcr_quirks(struct acpi_table_spcr *table, char **iotype)
+{
+	struct acpi_table_header *h = &table->header;
+
+	if (!memcmp(h->oem_table_id, "XGENESPC", ACPI_OEM_TABLE_ID_SIZE))
+		*iotype = "mmio32";
+}
+
 /**
  * parse_spcr() - parse ACPI SPCR table and add preferred console
  *
@@ -88,6 +96,9 @@ int __init parse_spcr(bool earlycon)
 			iotype = "mmio32";
 			break;
 		}
+
+		/* Fixup any non-standard compliant devices */
+		spcr_quirks(table, &iotype);
 	} else
 		iotype = "io";