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

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

[Jon Mason]

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.

Signed-off-by: Jon Mason <jon.mason@broadcom.com>
---
 drivers/acpi/spcr.c     | 18 ++++++++++++++++--
 include/acpi/acrestyp.h |  7 +++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index 01c9466..11233f6 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -74,8 +74,22 @@ int __init parse_spcr(bool earlycon)
 		goto done;
 	}
 
-	iotype = table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY ?
-			"mmio" : "io";
+	if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+		switch (table->serial_port.access_width) {
+		default:
+			pr_err("Unexpected SPCR Access Width.  Defaulting to byte size\n");
+		case ACPI_ACCESS_SIZE_BYTE:
+			iotype = "mmio";
+			break;
+		case ACPI_ACCESS_SIZE_WORD:
+			iotype = "mmio16";
+			break;
+		case ACPI_ACCESS_SIZE_DWORD:
+			iotype = "mmio32";
+			break;
+		}
+	} else
+		iotype = "io";
 
 	switch (table->interface_type) {
 	case ACPI_DBG2_ARM_SBSA_32BIT:
diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h
index f0f7403..781cb55 100644
--- a/include/acpi/acrestyp.h
+++ b/include/acpi/acrestyp.h
@@ -372,6 +372,13 @@ struct acpi_resource_generic_register {
 	u64 address;
 };
 
+/* Generic Address Space Access Sizes */
+#define ACPI_ACCESS_SIZE_UNDEFINED		0
+#define ACPI_ACCESS_SIZE_BYTE			1
+#define ACPI_ACCESS_SIZE_WORD			2
+#define ACPI_ACCESS_SIZE_DWORD			3
+#define ACPI_ACCESS_SIZE_QWORD			4
+
 struct acpi_resource_gpio {
 	u8 revision_id;
 	u8 connection_type;
-- 
2.7.4

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

[Moore, Robert]

Jon,

You might want to take a look at using the acpi_read and acpi_write ACPICA interfaces that accept a GAS structure and handle the access width (etc.) automatically.

Bob


> -----Original Message-----
> From: Jon Mason [mailto:jon.mason@broadcom.com]
> Sent: Thursday, May 4, 2017 8:06 AM
> To: Rafael Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
> Moore, Robert <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>
> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org;
> devel@acpica.org; bcm-kernel-feedback-list@broadcom.com
> Subject: [PATCH] ACPI: SPCR: Use access width to determine mmio usage
> 
> 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.
> 
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> ---
>  drivers/acpi/spcr.c     | 18 ++++++++++++++++--
>  include/acpi/acrestyp.h |  7 +++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c index
> 01c9466..11233f6 100644
> --- a/drivers/acpi/spcr.c
> +++ b/drivers/acpi/spcr.c
> @@ -74,8 +74,22 @@ int __init parse_spcr(bool earlycon)
>  		goto done;
>  	}
> 
> -	iotype = table->serial_port.space_id ==
> ACPI_ADR_SPACE_SYSTEM_MEMORY ?
> -			"mmio" : "io";
> +	if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +		switch (table->serial_port.access_width) {
> +		default:
> +			pr_err("Unexpected SPCR Access Width.  Defaulting to
> byte size\n");
> +		case ACPI_ACCESS_SIZE_BYTE:
> +			iotype = "mmio";
> +			break;
> +		case ACPI_ACCESS_SIZE_WORD:
> +			iotype = "mmio16";
> +			break;
> +		case ACPI_ACCESS_SIZE_DWORD:
> +			iotype = "mmio32";
> +			break;
> +		}
> +	} else
> +		iotype = "io";
> 
>  	switch (table->interface_type) {
>  	case ACPI_DBG2_ARM_SBSA_32BIT:
> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h index
> f0f7403..781cb55 100644
> --- a/include/acpi/acrestyp.h
> +++ b/include/acpi/acrestyp.h
> @@ -372,6 +372,13 @@ struct acpi_resource_generic_register {
>  	u64 address;
>  };
> 
> +/* Generic Address Space Access Sizes */
> +#define ACPI_ACCESS_SIZE_UNDEFINED		0
> +#define ACPI_ACCESS_SIZE_BYTE			1
> +#define ACPI_ACCESS_SIZE_WORD			2
> +#define ACPI_ACCESS_SIZE_DWORD			3
> +#define ACPI_ACCESS_SIZE_QWORD			4
> +
>  struct acpi_resource_gpio {
>  	u8 revision_id;
>  	u8 connection_type;
> --
> 2.7.4

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

[Moore, Robert]

Jon,

You might want to take a look at using the acpi_read and acpi_write ACPICA interfaces that accept a GAS structure and handle the access width (etc.) automatically.

Bob


> -----Original Message-----
> From: Jon Mason [mailto:jon.mason@broadcom.com]
> Sent: Thursday, May 4, 2017 8:06 AM
> To: Rafael Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
> Moore, Robert <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>
> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org;
> devel@acpica.org; bcm-kernel-feedback-list@broadcom.com
> Subject: [PATCH] ACPI: SPCR: Use access width to determine mmio usage
> 
> 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.
> 
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> ---
>  drivers/acpi/spcr.c     | 18 ++++++++++++++++--
>  include/acpi/acrestyp.h |  7 +++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c index
> 01c9466..11233f6 100644
> --- a/drivers/acpi/spcr.c
> +++ b/drivers/acpi/spcr.c
> @@ -74,8 +74,22 @@ int __init parse_spcr(bool earlycon)
>  		goto done;
>  	}
> 
> -	iotype = table->serial_port.space_id ==
> ACPI_ADR_SPACE_SYSTEM_MEMORY ?
> -			"mmio" : "io";
> +	if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +		switch (table->serial_port.access_width) {
> +		default:
> +			pr_err("Unexpected SPCR Access Width.  Defaulting to
> byte size\n");
> +		case ACPI_ACCESS_SIZE_BYTE:
> +			iotype = "mmio";
> +			break;
> +		case ACPI_ACCESS_SIZE_WORD:
> +			iotype = "mmio16";
> +			break;
> +		case ACPI_ACCESS_SIZE_DWORD:
> +			iotype = "mmio32";
> +			break;
> +		}
> +	} else
> +		iotype = "io";
> 
>  	switch (table->interface_type) {
>  	case ACPI_DBG2_ARM_SBSA_32BIT:
> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h index
> f0f7403..781cb55 100644
> --- a/include/acpi/acrestyp.h
> +++ b/include/acpi/acrestyp.h
> @@ -372,6 +372,13 @@ struct acpi_resource_generic_register {
>  	u64 address;
>  };
> 
> +/* Generic Address Space Access Sizes */
> +#define ACPI_ACCESS_SIZE_UNDEFINED		0
> +#define ACPI_ACCESS_SIZE_BYTE			1
> +#define ACPI_ACCESS_SIZE_WORD			2
> +#define ACPI_ACCESS_SIZE_DWORD			3
> +#define ACPI_ACCESS_SIZE_QWORD			4
> +
>  struct acpi_resource_gpio {
>  	u8 revision_id;
>  	u8 connection_type;
> --
> 2.7.4

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

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 2799 bytes --]

Jon,

You might want to take a look at using the acpi_read and acpi_write ACPICA interfaces that accept a GAS structure and handle the access width (etc.) automatically.

Bob


> -----Original Message-----
> From: Jon Mason [mailto:jon.mason(a)broadcom.com]
> Sent: Thursday, May 4, 2017 8:06 AM
> To: Rafael Wysocki <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>;
> Moore, Robert <robert.moore(a)intel.com>; Zheng, Lv <lv.zheng(a)intel.com>
> Cc: linux-acpi(a)vger.kernel.org; linux-kernel(a)vger.kernel.org;
> devel(a)acpica.org; bcm-kernel-feedback-list(a)broadcom.com
> Subject: [PATCH] ACPI: SPCR: Use access width to determine mmio usage
> 
> 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.
> 
> Signed-off-by: Jon Mason <jon.mason(a)broadcom.com>
> ---
>  drivers/acpi/spcr.c     | 18 ++++++++++++++++--
>  include/acpi/acrestyp.h |  7 +++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c index
> 01c9466..11233f6 100644
> --- a/drivers/acpi/spcr.c
> +++ b/drivers/acpi/spcr.c
> @@ -74,8 +74,22 @@ int __init parse_spcr(bool earlycon)
>  		goto done;
>  	}
> 
> -	iotype = table->serial_port.space_id ==
> ACPI_ADR_SPACE_SYSTEM_MEMORY ?
> -			"mmio" : "io";
> +	if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +		switch (table->serial_port.access_width) {
> +		default:
> +			pr_err("Unexpected SPCR Access Width.  Defaulting to
> byte size\n");
> +		case ACPI_ACCESS_SIZE_BYTE:
> +			iotype = "mmio";
> +			break;
> +		case ACPI_ACCESS_SIZE_WORD:
> +			iotype = "mmio16";
> +			break;
> +		case ACPI_ACCESS_SIZE_DWORD:
> +			iotype = "mmio32";
> +			break;
> +		}
> +	} else
> +		iotype = "io";
> 
>  	switch (table->interface_type) {
>  	case ACPI_DBG2_ARM_SBSA_32BIT:
> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h index
> f0f7403..781cb55 100644
> --- a/include/acpi/acrestyp.h
> +++ b/include/acpi/acrestyp.h
> @@ -372,6 +372,13 @@ struct acpi_resource_generic_register {
>  	u64 address;
>  };
> 
> +/* Generic Address Space Access Sizes */
> +#define ACPI_ACCESS_SIZE_UNDEFINED		0
> +#define ACPI_ACCESS_SIZE_BYTE			1
> +#define ACPI_ACCESS_SIZE_WORD			2
> +#define ACPI_ACCESS_SIZE_DWORD			3
> +#define ACPI_ACCESS_SIZE_QWORD			4
> +
>  struct acpi_resource_gpio {
>  	u8 revision_id;
>  	u8 connection_type;
> --
> 2.7.4

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

[Jon Mason]

On Thu, May 4, 2017 at 12:28 PM, Moore, Robert <robert.moore@intel.com> wrote:
> Jon,
>
> You might want to take a look at using the acpi_read and acpi_write ACPICA interfaces that accept a GAS structure and handle the access width (etc.) automatically.
>
> Bob

This function in SPCR is using common code for the early console.  So
the acpi_read/write would not be usable in that common code.

Thanks,
Jon

>
>
>> -----Original Message-----
>> From: Jon Mason [mailto:jon.mason@broadcom.com]
>> Sent: Thursday, May 4, 2017 8:06 AM
>> To: Rafael Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
>> Moore, Robert <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>
>> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devel@acpica.org; bcm-kernel-feedback-list@broadcom.com
>> Subject: [PATCH] ACPI: SPCR: Use access width to determine mmio usage
>>
>> 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.
>>
>> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
>> ---
>>  drivers/acpi/spcr.c     | 18 ++++++++++++++++--
>>  include/acpi/acrestyp.h |  7 +++++++
>>  2 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c index
>> 01c9466..11233f6 100644
>> --- a/drivers/acpi/spcr.c
>> +++ b/drivers/acpi/spcr.c
>> @@ -74,8 +74,22 @@ int __init parse_spcr(bool earlycon)
>>               goto done;
>>       }
>>
>> -     iotype = table->serial_port.space_id ==
>> ACPI_ADR_SPACE_SYSTEM_MEMORY ?
>> -                     "mmio" : "io";
>> +     if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>> +             switch (table->serial_port.access_width) {
>> +             default:
>> +                     pr_err("Unexpected SPCR Access Width.  Defaulting to
>> byte size\n");
>> +             case ACPI_ACCESS_SIZE_BYTE:
>> +                     iotype = "mmio";
>> +                     break;
>> +             case ACPI_ACCESS_SIZE_WORD:
>> +                     iotype = "mmio16";
>> +                     break;
>> +             case ACPI_ACCESS_SIZE_DWORD:
>> +                     iotype = "mmio32";
>> +                     break;
>> +             }
>> +     } else
>> +             iotype = "io";
>>
>>       switch (table->interface_type) {
>>       case ACPI_DBG2_ARM_SBSA_32BIT:
>> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h index
>> f0f7403..781cb55 100644
>> --- a/include/acpi/acrestyp.h
>> +++ b/include/acpi/acrestyp.h
>> @@ -372,6 +372,13 @@ struct acpi_resource_generic_register {
>>       u64 address;
>>  };
>>
>> +/* Generic Address Space Access Sizes */
>> +#define ACPI_ACCESS_SIZE_UNDEFINED           0
>> +#define ACPI_ACCESS_SIZE_BYTE                        1
>> +#define ACPI_ACCESS_SIZE_WORD                        2
>> +#define ACPI_ACCESS_SIZE_DWORD                       3
>> +#define ACPI_ACCESS_SIZE_QWORD                       4
>> +
>>  struct acpi_resource_gpio {
>>       u8 revision_id;
>>       u8 connection_type;
>> --
>> 2.7.4
>

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

[Jon Mason]

On Thu, May 4, 2017 at 12:28 PM, Moore, Robert <robert.moore@intel.com> wrote:
> Jon,
>
> You might want to take a look at using the acpi_read and acpi_write ACPICA interfaces that accept a GAS structure and handle the access width (etc.) automatically.
>
> Bob

This function in SPCR is using common code for the early console.  So
the acpi_read/write would not be usable in that common code.

Thanks,
Jon

>
>
>> -----Original Message-----
>> From: Jon Mason [mailto:jon.mason@broadcom.com]
>> Sent: Thursday, May 4, 2017 8:06 AM
>> To: Rafael Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
>> Moore, Robert <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>
>> Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devel@acpica.org; bcm-kernel-feedback-list@broadcom.com
>> Subject: [PATCH] ACPI: SPCR: Use access width to determine mmio usage
>>
>> 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.
>>
>> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
>> ---
>>  drivers/acpi/spcr.c     | 18 ++++++++++++++++--
>>  include/acpi/acrestyp.h |  7 +++++++
>>  2 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c index
>> 01c9466..11233f6 100644
>> --- a/drivers/acpi/spcr.c
>> +++ b/drivers/acpi/spcr.c
>> @@ -74,8 +74,22 @@ int __init parse_spcr(bool earlycon)
>>               goto done;
>>       }
>>
>> -     iotype = table->serial_port.space_id ==
>> ACPI_ADR_SPACE_SYSTEM_MEMORY ?
>> -                     "mmio" : "io";
>> +     if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>> +             switch (table->serial_port.access_width) {
>> +             default:
>> +                     pr_err("Unexpected SPCR Access Width.  Defaulting to
>> byte size\n");
>> +             case ACPI_ACCESS_SIZE_BYTE:
>> +                     iotype = "mmio";
>> +                     break;
>> +             case ACPI_ACCESS_SIZE_WORD:
>> +                     iotype = "mmio16";
>> +                     break;
>> +             case ACPI_ACCESS_SIZE_DWORD:
>> +                     iotype = "mmio32";
>> +                     break;
>> +             }
>> +     } else
>> +             iotype = "io";
>>
>>       switch (table->interface_type) {
>>       case ACPI_DBG2_ARM_SBSA_32BIT:
>> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h index
>> f0f7403..781cb55 100644
>> --- a/include/acpi/acrestyp.h
>> +++ b/include/acpi/acrestyp.h
>> @@ -372,6 +372,13 @@ struct acpi_resource_generic_register {
>>       u64 address;
>>  };
>>
>> +/* Generic Address Space Access Sizes */
>> +#define ACPI_ACCESS_SIZE_UNDEFINED           0
>> +#define ACPI_ACCESS_SIZE_BYTE                        1
>> +#define ACPI_ACCESS_SIZE_WORD                        2
>> +#define ACPI_ACCESS_SIZE_DWORD                       3
>> +#define ACPI_ACCESS_SIZE_QWORD                       4
>> +
>>  struct acpi_resource_gpio {
>>       u8 revision_id;
>>       u8 connection_type;
>> --
>> 2.7.4
>

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

[Zheng, Lv]

Hi,

> From: Jon Mason [mailto:jon.mason@broadcom.com]
> Sent: Thursday, May 4, 2017 11:06 PM
> Subject: [PATCH] ACPI: SPCR: Use access width to determine mmio usage
> 
> 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.
> 
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> ---
>  drivers/acpi/spcr.c     | 18 ++++++++++++++++--
>  include/acpi/acrestyp.h |  7 +++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
> index 01c9466..11233f6 100644
> --- a/drivers/acpi/spcr.c
> +++ b/drivers/acpi/spcr.c
> @@ -74,8 +74,22 @@ int __init parse_spcr(bool earlycon)
>  		goto done;
>  	}
> 
> -	iotype = table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY ?
> -			"mmio" : "io";
> +	if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +		switch (table->serial_port.access_width) {
> +		default:
> +			pr_err("Unexpected SPCR Access Width.  Defaulting to byte size\n");
> +		case ACPI_ACCESS_SIZE_BYTE:
> +			iotype = "mmio";
> +			break;
> +		case ACPI_ACCESS_SIZE_WORD:
> +			iotype = "mmio16";
> +			break;
> +		case ACPI_ACCESS_SIZE_DWORD:
> +			iotype = "mmio32";
> +			break;
> +		}
> +	} else
> +		iotype = "io";
> 
>  	switch (table->interface_type) {
>  	case ACPI_DBG2_ARM_SBSA_32BIT:
> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h
> index f0f7403..781cb55 100644
> --- a/include/acpi/acrestyp.h
> +++ b/include/acpi/acrestyp.h
> @@ -372,6 +372,13 @@ struct acpi_resource_generic_register {
>  	u64 address;
>  };
> 
> +/* Generic Address Space Access Sizes */
> +#define ACPI_ACCESS_SIZE_UNDEFINED		0
> +#define ACPI_ACCESS_SIZE_BYTE			1
> +#define ACPI_ACCESS_SIZE_WORD			2
> +#define ACPI_ACCESS_SIZE_DWORD			3
> +#define ACPI_ACCESS_SIZE_QWORD			4
> +
>  struct acpi_resource_gpio {
>  	u8 revision_id;
>  	u8 connection_type;

Please don't define this.
It's possible to calculate 8/16/32/64 from the access width value.
Try:
1 << (access_width + 2）

Thanks
Lv

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

[Zheng, Lv]

Hi,

> From: Jon Mason [mailto:jon.mason@broadcom.com]
> Sent: Thursday, May 4, 2017 11:06 PM
> Subject: [PATCH] ACPI: SPCR: Use access width to determine mmio usage
> 
> 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.
> 
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> ---
>  drivers/acpi/spcr.c     | 18 ++++++++++++++++--
>  include/acpi/acrestyp.h |  7 +++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
> index 01c9466..11233f6 100644
> --- a/drivers/acpi/spcr.c
> +++ b/drivers/acpi/spcr.c
> @@ -74,8 +74,22 @@ int __init parse_spcr(bool earlycon)
>  		goto done;
>  	}
> 
> -	iotype = table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY ?
> -			"mmio" : "io";
> +	if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +		switch (table->serial_port.access_width) {
> +		default:
> +			pr_err("Unexpected SPCR Access Width.  Defaulting to byte size\n");
> +		case ACPI_ACCESS_SIZE_BYTE:
> +			iotype = "mmio";
> +			break;
> +		case ACPI_ACCESS_SIZE_WORD:
> +			iotype = "mmio16";
> +			break;
> +		case ACPI_ACCESS_SIZE_DWORD:
> +			iotype = "mmio32";
> +			break;
> +		}
> +	} else
> +		iotype = "io";
> 
>  	switch (table->interface_type) {
>  	case ACPI_DBG2_ARM_SBSA_32BIT:
> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h
> index f0f7403..781cb55 100644
> --- a/include/acpi/acrestyp.h
> +++ b/include/acpi/acrestyp.h
> @@ -372,6 +372,13 @@ struct acpi_resource_generic_register {
>  	u64 address;
>  };
> 
> +/* Generic Address Space Access Sizes */
> +#define ACPI_ACCESS_SIZE_UNDEFINED		0
> +#define ACPI_ACCESS_SIZE_BYTE			1
> +#define ACPI_ACCESS_SIZE_WORD			2
> +#define ACPI_ACCESS_SIZE_DWORD			3
> +#define ACPI_ACCESS_SIZE_QWORD			4
> +
>  struct acpi_resource_gpio {
>  	u8 revision_id;
>  	u8 connection_type;

Please don't define this.
It's possible to calculate 8/16/32/64 from the access width value.
Try:
1 << (access_width + 2）

Thanks
Lv

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

[Zheng, Lv]

[-- Attachment #1: Type: text/plain, Size: 2430 bytes --]

Hi,

> From: Jon Mason [mailto:jon.mason(a)broadcom.com]
> Sent: Thursday, May 4, 2017 11:06 PM
> Subject: [PATCH] ACPI: SPCR: Use access width to determine mmio usage
> 
> 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.
> 
> Signed-off-by: Jon Mason <jon.mason(a)broadcom.com>
> ---
>  drivers/acpi/spcr.c     | 18 ++++++++++++++++--
>  include/acpi/acrestyp.h |  7 +++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
> index 01c9466..11233f6 100644
> --- a/drivers/acpi/spcr.c
> +++ b/drivers/acpi/spcr.c
> @@ -74,8 +74,22 @@ int __init parse_spcr(bool earlycon)
>  		goto done;
>  	}
> 
> -	iotype = table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY ?
> -			"mmio" : "io";
> +	if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +		switch (table->serial_port.access_width) {
> +		default:
> +			pr_err("Unexpected SPCR Access Width.  Defaulting to byte size\n");
> +		case ACPI_ACCESS_SIZE_BYTE:
> +			iotype = "mmio";
> +			break;
> +		case ACPI_ACCESS_SIZE_WORD:
> +			iotype = "mmio16";
> +			break;
> +		case ACPI_ACCESS_SIZE_DWORD:
> +			iotype = "mmio32";
> +			break;
> +		}
> +	} else
> +		iotype = "io";
> 
>  	switch (table->interface_type) {
>  	case ACPI_DBG2_ARM_SBSA_32BIT:
> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h
> index f0f7403..781cb55 100644
> --- a/include/acpi/acrestyp.h
> +++ b/include/acpi/acrestyp.h
> @@ -372,6 +372,13 @@ struct acpi_resource_generic_register {
>  	u64 address;
>  };
> 
> +/* Generic Address Space Access Sizes */
> +#define ACPI_ACCESS_SIZE_UNDEFINED		0
> +#define ACPI_ACCESS_SIZE_BYTE			1
> +#define ACPI_ACCESS_SIZE_WORD			2
> +#define ACPI_ACCESS_SIZE_DWORD			3
> +#define ACPI_ACCESS_SIZE_QWORD			4
> +
>  struct acpi_resource_gpio {
>  	u8 revision_id;
>  	u8 connection_type;

Please don't define this.
It's possible to calculate 8/16/32/64 from the access width value.
Try:
1 << (access_width + 2）

Thanks
Lv

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

[Jon Mason]

On Thu, May 4, 2017 at 11:09 PM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi,
>
>> From: Jon Mason [mailto:jon.mason@broadcom.com]
>> Sent: Thursday, May 4, 2017 11:06 PM
>> Subject: [PATCH] ACPI: SPCR: Use access width to determine mmio usage
>>
>> 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.
>>
>> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
>> ---
>>  drivers/acpi/spcr.c     | 18 ++++++++++++++++--
>>  include/acpi/acrestyp.h |  7 +++++++
>>  2 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>> index 01c9466..11233f6 100644
>> --- a/drivers/acpi/spcr.c
>> +++ b/drivers/acpi/spcr.c
>> @@ -74,8 +74,22 @@ int __init parse_spcr(bool earlycon)
>>               goto done;
>>       }
>>
>> -     iotype = table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY ?
>> -                     "mmio" : "io";
>> +     if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>> +             switch (table->serial_port.access_width) {
>> +             default:
>> +                     pr_err("Unexpected SPCR Access Width.  Defaulting to byte size\n");
>> +             case ACPI_ACCESS_SIZE_BYTE:
>> +                     iotype = "mmio";
>> +                     break;
>> +             case ACPI_ACCESS_SIZE_WORD:
>> +                     iotype = "mmio16";
>> +                     break;
>> +             case ACPI_ACCESS_SIZE_DWORD:
>> +                     iotype = "mmio32";
>> +                     break;
>> +             }
>> +     } else
>> +             iotype = "io";
>>
>>       switch (table->interface_type) {
>>       case ACPI_DBG2_ARM_SBSA_32BIT:
>> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h
>> index f0f7403..781cb55 100644
>> --- a/include/acpi/acrestyp.h
>> +++ b/include/acpi/acrestyp.h
>> @@ -372,6 +372,13 @@ struct acpi_resource_generic_register {
>>       u64 address;
>>  };
>>
>> +/* Generic Address Space Access Sizes */
>> +#define ACPI_ACCESS_SIZE_UNDEFINED           0
>> +#define ACPI_ACCESS_SIZE_BYTE                        1
>> +#define ACPI_ACCESS_SIZE_WORD                        2
>> +#define ACPI_ACCESS_SIZE_DWORD                       3
>> +#define ACPI_ACCESS_SIZE_QWORD                       4
>> +
>>  struct acpi_resource_gpio {
>>       u8 revision_id;
>>       u8 connection_type;
>
> Please don't define this.
> It's possible to calculate 8/16/32/64 from the access width value.
> Try:
> 1 << (access_width + 2）

Thanks, I'll send out v2 of the patch with this change.

> Thanks
> Lv
>

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

[Jon Mason]

On Thu, May 4, 2017 at 11:09 PM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi,
>
>> From: Jon Mason [mailto:jon.mason@broadcom.com]
>> Sent: Thursday, May 4, 2017 11:06 PM
>> Subject: [PATCH] ACPI: SPCR: Use access width to determine mmio usage
>>
>> 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.
>>
>> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
>> ---
>>  drivers/acpi/spcr.c     | 18 ++++++++++++++++--
>>  include/acpi/acrestyp.h |  7 +++++++
>>  2 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>> index 01c9466..11233f6 100644
>> --- a/drivers/acpi/spcr.c
>> +++ b/drivers/acpi/spcr.c
>> @@ -74,8 +74,22 @@ int __init parse_spcr(bool earlycon)
>>               goto done;
>>       }
>>
>> -     iotype = table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY ?
>> -                     "mmio" : "io";
>> +     if (table->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>> +             switch (table->serial_port.access_width) {
>> +             default:
>> +                     pr_err("Unexpected SPCR Access Width.  Defaulting to byte size\n");
>> +             case ACPI_ACCESS_SIZE_BYTE:
>> +                     iotype = "mmio";
>> +                     break;
>> +             case ACPI_ACCESS_SIZE_WORD:
>> +                     iotype = "mmio16";
>> +                     break;
>> +             case ACPI_ACCESS_SIZE_DWORD:
>> +                     iotype = "mmio32";
>> +                     break;
>> +             }
>> +     } else
>> +             iotype = "io";
>>
>>       switch (table->interface_type) {
>>       case ACPI_DBG2_ARM_SBSA_32BIT:
>> diff --git a/include/acpi/acrestyp.h b/include/acpi/acrestyp.h
>> index f0f7403..781cb55 100644
>> --- a/include/acpi/acrestyp.h
>> +++ b/include/acpi/acrestyp.h
>> @@ -372,6 +372,13 @@ struct acpi_resource_generic_register {
>>       u64 address;
>>  };
>>
>> +/* Generic Address Space Access Sizes */
>> +#define ACPI_ACCESS_SIZE_UNDEFINED           0
>> +#define ACPI_ACCESS_SIZE_BYTE                        1
>> +#define ACPI_ACCESS_SIZE_WORD                        2
>> +#define ACPI_ACCESS_SIZE_DWORD                       3
>> +#define ACPI_ACCESS_SIZE_QWORD                       4
>> +
>>  struct acpi_resource_gpio {
>>       u8 revision_id;
>>       u8 connection_type;
>
> Please don't define this.
> It's possible to calculate 8/16/32/64 from the access width value.
> Try:
> 1 << (access_width + 2）

Thanks, I'll send out v2 of the patch with this change.

> Thanks
> Lv
>

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

[Jon Masters]

On 05/04/2017 11:05 AM, Jon Mason wrote:
> 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.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

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

[Loc Ho]

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

-Loc

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

[Jon Masters]

On 05/08/2017 03:11 PM, Loc Ho 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.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

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

[Loc Ho]

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.

-Loc

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

[Jon Mason]

On Mon, May 8, 2017 at 3:57 PM, Loc Ho <lho@apm.com> 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?

Thanks,
Jon Mas(on)


>
> -Loc

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

[Loc Ho]

Hi Jon,

On Mon, May 8, 2017 at 1:34 PM, Jon Mason <jon.mason@broadcom.com> wrote:
> On Mon, May 8, 2017 at 3:57 PM, Loc Ho <lho@apm.com> 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

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@broadcom.com> wrote:
> > On Mon, May 8, 2017 at 3:57 PM, Loc Ho <lho@apm.com> 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";
 

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

[Jon Masters]

Sorry for top post. We would need to also need to handle other OEMs like HPE m400. The set is limited but we want to key off the right ID. You could also key off DMI data if it were later in boot. But probably too early at this point.

-- 
Computer Architect | Sent from my 64-bit #ARM Powered phone

> On May 8, 2017, at 17:24, Jon Mason <jon.mason@broadcom.com> wrote:
> 
>> 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@broadcom.com> wrote:
>>>> On Mon, May 8, 2017 at 3:57 PM, Loc Ho <lho@apm.com> 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";
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Loc Ho]

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.

It is a bit more complex that this simple patch. How about this one
(my original version). As for Jon Master question on McDivitt, not
sure what they use for the ACPI table for SPCR. If they used our
reference, then this might work for them too. This version would limit
to just the existent firmware or until the SPCR table gets changed.


tty: 8250: 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. The SPCR always assumes fully compatible
8250. 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>
---
 drivers/acpi/spcr.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index 3afa8c1..77b45a0 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
acpi_table_header *h)
        return false;
 }

+/*
+ * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
+ * register aligned to 32-bit. This function detects this errata condition.
+ */
+static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
+{
+       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
+               return false;
+
+       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
+               return false;
+
+       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
+                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
+               return true;
+
+       return false;
+}
+
 /**
  * parse_spcr() - parse ACPI SPCR table and add preferred console
  *
@@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)

        if (qdf2400_erratum_44_present(&table->header))
                uart = "qdf2400_e44";
+       if (xgene_8250_erratum_present(table))
+               iotype = "mmio32";

        snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
                 table->serial_port.address, baud_rate);

-Loc

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

[Loc Ho]

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.
>
> It is a bit more complex that this simple patch. How about this one
> (my original version). As for Jon Master question on McDivitt, not
> sure what they use for the ACPI table for SPCR. If they used our
> reference, then this might work for them too. This version would limit
> to just the existent firmware or until the SPCR table gets changed.
>
>
> tty: 8250: 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. The SPCR always assumes fully compatible
> 8250. 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>
> ---
>  drivers/acpi/spcr.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
> index 3afa8c1..77b45a0 100644
> --- a/drivers/acpi/spcr.c
> +++ b/drivers/acpi/spcr.c
> @@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
> acpi_table_header *h)
>         return false;
>  }
>
> +/*
> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
> + * register aligned to 32-bit. This function detects this errata condition.
> + */
> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
> +{
> +       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
> +               return false;
> +
> +       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
> +               return false;
> +
> +       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
> +                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
> +               return true;
> +
> +       return false;
> +}
> +
>  /**
>   * parse_spcr() - parse ACPI SPCR table and add preferred console
>   *
> @@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)
>
>         if (qdf2400_erratum_44_present(&table->header))
>                 uart = "qdf2400_e44";
> +       if (xgene_8250_erratum_present(table))
> +               iotype = "mmio32";
>
>         snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
>                  table->serial_port.address, baud_rate);
>

I didn't see a follow up email on this. What was the conclusion to
this patch series?

-Loc

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

[Jon Mason]

On Tue, Jun 20, 2017 at 2:33 PM, Loc Ho <lho@apm.com> 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.
>>
>> It is a bit more complex that this simple patch. How about this one
>> (my original version). As for Jon Master question on McDivitt, not
>> sure what they use for the ACPI table for SPCR. If they used our
>> reference, then this might work for them too. This version would limit
>> to just the existent firmware or until the SPCR table gets changed.
>>
>>
>> tty: 8250: 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. The SPCR always assumes fully compatible
>> 8250. 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>
>> ---
>>  drivers/acpi/spcr.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>> index 3afa8c1..77b45a0 100644
>> --- a/drivers/acpi/spcr.c
>> +++ b/drivers/acpi/spcr.c
>> @@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
>> acpi_table_header *h)
>>         return false;
>>  }
>>
>> +/*
>> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>> + * register aligned to 32-bit. This function detects this errata condition.
>> + */
>> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>> +{
>> +       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>> +               return false;
>> +
>> +       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
>> +               return false;
>> +
>> +       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>> +                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>> +               return true;
>> +
>> +       return false;
>> +}
>> +
>>  /**
>>   * parse_spcr() - parse ACPI SPCR table and add preferred console
>>   *
>> @@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)
>>
>>         if (qdf2400_erratum_44_present(&table->header))
>>                 uart = "qdf2400_e44";
>> +       if (xgene_8250_erratum_present(table))
>> +               iotype = "mmio32";
>>
>>         snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
>>                  table->serial_port.address, baud_rate);
>>
>
> I didn't see a follow up email on this. What was the conclusion to
> this patch series?

I have not received an ack, nack, or gtfo from any of the maintainers
of this file.  Per
./scripts/get_maintainer.pl drivers/acpi/spcr.c
"Rafael J. Wysocki" <rjw@rjwysocki.net> (supporter:ACPI)
Len Brown <lenb@kernel.org> (supporter:ACPI)
linux-acpi@vger.kernel.org (open list:ACPI)
linux-kernel@vger.kernel.org (open list)

Is there someone else I should be directing this patch through?

Thanks,
Jon

> -Loc

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

[Loc Ho]

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.
>>>
>>> It is a bit more complex that this simple patch. How about this one
>>> (my original version). As for Jon Master question on McDivitt, not
>>> sure what they use for the ACPI table for SPCR. If they used our
>>> reference, then this might work for them too. This version would limit
>>> to just the existent firmware or until the SPCR table gets changed.
>>>
>>>
>>> tty: 8250: 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. The SPCR always assumes fully compatible
>>> 8250. 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>
>>> ---
>>>  drivers/acpi/spcr.c | 21 +++++++++++++++++++++
>>>  1 file changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>>> index 3afa8c1..77b45a0 100644
>>> --- a/drivers/acpi/spcr.c
>>> +++ b/drivers/acpi/spcr.c
>>> @@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
>>> acpi_table_header *h)
>>>         return false;
>>>  }
>>>
>>> +/*
>>> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>>> + * register aligned to 32-bit. This function detects this errata condition.
>>> + */
>>> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>>> +{
>>> +       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>>> +               return false;
>>> +
>>> +       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
>>> +               return false;
>>> +
>>> +       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>>> +                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>>> +               return true;
>>> +
>>> +       return false;
>>> +}
>>> +
>>>  /**
>>>   * parse_spcr() - parse ACPI SPCR table and add preferred console
>>>   *
>>> @@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)
>>>
>>>         if (qdf2400_erratum_44_present(&table->header))
>>>                 uart = "qdf2400_e44";
>>> +       if (xgene_8250_erratum_present(table))
>>> +               iotype = "mmio32";
>>>
>>>         snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
>>>                  table->serial_port.address, baud_rate);
>>>
>>
>> I didn't see a follow up email on this. What was the conclusion to
>> this patch series?
>
> I have not received an ack, nack, or gtfo from any of the maintainers
> of this file.  Per
> ./scripts/get_maintainer.pl drivers/acpi/spcr.c
> "Rafael J. Wysocki" <rjw@rjwysocki.net> (supporter:ACPI)
> Len Brown <lenb@kernel.org> (supporter:ACPI)
> linux-acpi@vger.kernel.org (open list:ACPI)
> linux-kernel@vger.kernel.org (open list)
>
> Is there someone else I should be directing this patch through?

Can you generate a new patch set that includes all the required
patches (including mine workaround for X-Gene) and re-post? Then we
check with Rafael or Len again.

-Loc

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

[Loc Ho]

[-- Attachment #1: Type: text/plain, Size: 6570 bytes --]

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.
>>>
>>> It is a bit more complex that this simple patch. How about this one
>>> (my original version). As for Jon Master question on McDivitt, not
>>> sure what they use for the ACPI table for SPCR. If they used our
>>> reference, then this might work for them too. This version would limit
>>> to just the existent firmware or until the SPCR table gets changed.
>>>
>>>
>>> tty: 8250: 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. The SPCR always assumes fully compatible
>>> 8250. 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(a)apm.com>
>>> ---
>>>  drivers/acpi/spcr.c | 21 +++++++++++++++++++++
>>>  1 file changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>>> index 3afa8c1..77b45a0 100644
>>> --- a/drivers/acpi/spcr.c
>>> +++ b/drivers/acpi/spcr.c
>>> @@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
>>> acpi_table_header *h)
>>>         return false;
>>>  }
>>>
>>> +/*
>>> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>>> + * register aligned to 32-bit. This function detects this errata condition.
>>> + */
>>> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>>> +{
>>> +       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>>> +               return false;
>>> +
>>> +       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
>>> +               return false;
>>> +
>>> +       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>>> +                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>>> +               return true;
>>> +
>>> +       return false;
>>> +}
>>> +
>>>  /**
>>>   * parse_spcr() - parse ACPI SPCR table and add preferred console
>>>   *
>>> @@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)
>>>
>>>         if (qdf2400_erratum_44_present(&table->header))
>>>                 uart = "qdf2400_e44";
>>> +       if (xgene_8250_erratum_present(table))
>>> +               iotype = "mmio32";
>>>
>>>         snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
>>>                  table->serial_port.address, baud_rate);
>>>
>>
>> I didn't see a follow up email on this. What was the conclusion to
>> this patch series?
>
> I have not received an ack, nack, or gtfo from any of the maintainers
> of this file.  Per
> ./scripts/get_maintainer.pl drivers/acpi/spcr.c
> "Rafael J. Wysocki" <rjw(a)rjwysocki.net> (supporter:ACPI)
> Len Brown <lenb(a)kernel.org> (supporter:ACPI)
> linux-acpi(a)vger.kernel.org (open list:ACPI)
> linux-kernel(a)vger.kernel.org (open list)
>
> Is there someone else I should be directing this patch through?

Can you generate a new patch set that includes all the required
patches (including mine workaround for X-Gene) and re-post? Then we
check with Rafael or Len again.

-Loc

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

[Rafael J. Wysocki]

On Tue, Jun 20, 2017 at 11:27 PM, Jon Mason <jon.mason@broadcom.com> wrote:
> On Tue, Jun 20, 2017 at 2:33 PM, Loc Ho <lho@apm.com> 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.
>>>
>>> It is a bit more complex that this simple patch. How about this one
>>> (my original version). As for Jon Master question on McDivitt, not
>>> sure what they use for the ACPI table for SPCR. If they used our
>>> reference, then this might work for them too. This version would limit
>>> to just the existent firmware or until the SPCR table gets changed.
>>>
>>>
>>> tty: 8250: 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. The SPCR always assumes fully compatible
>>> 8250. 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>
>>> ---
>>>  drivers/acpi/spcr.c | 21 +++++++++++++++++++++
>>>  1 file changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>>> index 3afa8c1..77b45a0 100644
>>> --- a/drivers/acpi/spcr.c
>>> +++ b/drivers/acpi/spcr.c
>>> @@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
>>> acpi_table_header *h)
>>>         return false;
>>>  }
>>>
>>> +/*
>>> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>>> + * register aligned to 32-bit. This function detects this errata condition.
>>> + */
>>> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>>> +{
>>> +       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>>> +               return false;
>>> +
>>> +       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
>>> +               return false;
>>> +
>>> +       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>>> +                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>>> +               return true;
>>> +
>>> +       return false;
>>> +}
>>> +
>>>  /**
>>>   * parse_spcr() - parse ACPI SPCR table and add preferred console
>>>   *
>>> @@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)
>>>
>>>         if (qdf2400_erratum_44_present(&table->header))
>>>                 uart = "qdf2400_e44";
>>> +       if (xgene_8250_erratum_present(table))
>>> +               iotype = "mmio32";
>>>
>>>         snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
>>>                  table->serial_port.address, baud_rate);
>>>
>>
>> I didn't see a follow up email on this. What was the conclusion to
>> this patch series?
>
> I have not received an ack, nack, or gtfo from any of the maintainers
> of this file.  Per
> ./scripts/get_maintainer.pl drivers/acpi/spcr.c
> "Rafael J. Wysocki" <rjw@rjwysocki.net> (supporter:ACPI)
> Len Brown <lenb@kernel.org> (supporter:ACPI)
> linux-acpi@vger.kernel.org (open list:ACPI)
> linux-kernel@vger.kernel.org (open list)
>
> Is there someone else I should be directing this patch through?

Generally, whoever is going to be affected by this change.

git seems to tell me that the spcr.c file went in through the tty tree
and Aleksey introduced it.

I can apply it if no one has any objections.

Thanks,
Rafael

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

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 6779 bytes --]

On Tue, Jun 20, 2017 at 11:27 PM, Jon Mason <jon.mason(a)broadcom.com> wrote:
> On Tue, Jun 20, 2017 at 2:33 PM, Loc Ho <lho(a)apm.com> 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.
>>>
>>> It is a bit more complex that this simple patch. How about this one
>>> (my original version). As for Jon Master question on McDivitt, not
>>> sure what they use for the ACPI table for SPCR. If they used our
>>> reference, then this might work for them too. This version would limit
>>> to just the existent firmware or until the SPCR table gets changed.
>>>
>>>
>>> tty: 8250: 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. The SPCR always assumes fully compatible
>>> 8250. 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(a)apm.com>
>>> ---
>>>  drivers/acpi/spcr.c | 21 +++++++++++++++++++++
>>>  1 file changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>>> index 3afa8c1..77b45a0 100644
>>> --- a/drivers/acpi/spcr.c
>>> +++ b/drivers/acpi/spcr.c
>>> @@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
>>> acpi_table_header *h)
>>>         return false;
>>>  }
>>>
>>> +/*
>>> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>>> + * register aligned to 32-bit. This function detects this errata condition.
>>> + */
>>> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>>> +{
>>> +       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>>> +               return false;
>>> +
>>> +       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
>>> +               return false;
>>> +
>>> +       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>>> +                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>>> +               return true;
>>> +
>>> +       return false;
>>> +}
>>> +
>>>  /**
>>>   * parse_spcr() - parse ACPI SPCR table and add preferred console
>>>   *
>>> @@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)
>>>
>>>         if (qdf2400_erratum_44_present(&table->header))
>>>                 uart = "qdf2400_e44";
>>> +       if (xgene_8250_erratum_present(table))
>>> +               iotype = "mmio32";
>>>
>>>         snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
>>>                  table->serial_port.address, baud_rate);
>>>
>>
>> I didn't see a follow up email on this. What was the conclusion to
>> this patch series?
>
> I have not received an ack, nack, or gtfo from any of the maintainers
> of this file.  Per
> ./scripts/get_maintainer.pl drivers/acpi/spcr.c
> "Rafael J. Wysocki" <rjw(a)rjwysocki.net> (supporter:ACPI)
> Len Brown <lenb(a)kernel.org> (supporter:ACPI)
> linux-acpi(a)vger.kernel.org (open list:ACPI)
> linux-kernel(a)vger.kernel.org (open list)
>
> Is there someone else I should be directing this patch through?

Generally, whoever is going to be affected by this change.

git seems to tell me that the spcr.c file went in through the tty tree
and Aleksey introduced it.

I can apply it if no one has any objections.

Thanks,
Rafael

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

[Loc Ho]

Hi Rafael,

>>>> >> >>>>>> 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.
>>>>
>>>> It is a bit more complex that this simple patch. How about this one
>>>> (my original version). As for Jon Master question on McDivitt, not
>>>> sure what they use for the ACPI table for SPCR. If they used our
>>>> reference, then this might work for them too. This version would limit
>>>> to just the existent firmware or until the SPCR table gets changed.
>>>>
>>>>
>>>> tty: 8250: 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. The SPCR always assumes fully compatible
>>>> 8250. 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>
>>>> ---
>>>>  drivers/acpi/spcr.c | 21 +++++++++++++++++++++
>>>>  1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>>>> index 3afa8c1..77b45a0 100644
>>>> --- a/drivers/acpi/spcr.c
>>>> +++ b/drivers/acpi/spcr.c
>>>> @@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
>>>> acpi_table_header *h)
>>>>         return false;
>>>>  }
>>>>
>>>> +/*
>>>> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>>>> + * register aligned to 32-bit. This function detects this errata condition.
>>>> + */
>>>> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>>>> +{
>>>> +       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>>>> +               return false;
>>>> +
>>>> +       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
>>>> +               return false;
>>>> +
>>>> +       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>>>> +                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>>>> +               return true;
>>>> +
>>>> +       return false;
>>>> +}
>>>> +
>>>>  /**
>>>>   * parse_spcr() - parse ACPI SPCR table and add preferred console
>>>>   *
>>>> @@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)
>>>>
>>>>         if (qdf2400_erratum_44_present(&table->header))
>>>>                 uart = "qdf2400_e44";
>>>> +       if (xgene_8250_erratum_present(table))
>>>> +               iotype = "mmio32";
>>>>
>>>>         snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
>>>>                  table->serial_port.address, baud_rate);
>>>>
>>>
>>> I didn't see a follow up email on this. What was the conclusion to
>>> this patch series?
>>
>> I have not received an ack, nack, or gtfo from any of the maintainers
>> of this file.  Per
>> ./scripts/get_maintainer.pl drivers/acpi/spcr.c
>> "Rafael J. Wysocki" <rjw@rjwysocki.net> (supporter:ACPI)
>> Len Brown <lenb@kernel.org> (supporter:ACPI)
>> linux-acpi@vger.kernel.org (open list:ACPI)
>> linux-kernel@vger.kernel.org (open list)
>>
>> Is there someone else I should be directing this patch through?
>
> Generally, whoever is going to be affected by this change.
>
> git seems to tell me that the spcr.c file went in through the tty tree
> and Aleksey introduced it.
>
> I can apply it if no one has any objections.

I think this is on the mailing list long enough and I don't see anyone
follow up with any objection. Would you applies Jon Mason patch and my
patch on May 8 or inline below? If you need a more formal patch email
for the below patch, I can send one to this email list as well.

==========

tty: 8250: 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. The SPCR always assumes fully compatible
8250. 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>
---
 drivers/acpi/spcr.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index 3afa8c1..77b45a0 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
acpi_table_header *h)
        return false;
 }

+/*
+ * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
+ * register aligned to 32-bit. This function detects this errata condition.
+ */
+static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
+{
+       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
+               return false;
+
+       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
+               return false;
+
+       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
+                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
+               return true;
+
+       return false;
+}
+
 /**
  * parse_spcr() - parse ACPI SPCR table and add preferred console
  *
@@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)

        if (qdf2400_erratum_44_present(&table->header))
                uart = "qdf2400_e44";
+       if (xgene_8250_erratum_present(table))
+               iotype = "mmio32";

        snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
                 table->serial_port.address, baud_rate);

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

[Loc Ho]

[-- Attachment #1: Type: text/plain, Size: 8851 bytes --]

Hi Rafael,

>>>> >> >>>>>> 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.
>>>>
>>>> It is a bit more complex that this simple patch. How about this one
>>>> (my original version). As for Jon Master question on McDivitt, not
>>>> sure what they use for the ACPI table for SPCR. If they used our
>>>> reference, then this might work for them too. This version would limit
>>>> to just the existent firmware or until the SPCR table gets changed.
>>>>
>>>>
>>>> tty: 8250: 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. The SPCR always assumes fully compatible
>>>> 8250. 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(a)apm.com>
>>>> ---
>>>>  drivers/acpi/spcr.c | 21 +++++++++++++++++++++
>>>>  1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>>>> index 3afa8c1..77b45a0 100644
>>>> --- a/drivers/acpi/spcr.c
>>>> +++ b/drivers/acpi/spcr.c
>>>> @@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
>>>> acpi_table_header *h)
>>>>         return false;
>>>>  }
>>>>
>>>> +/*
>>>> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>>>> + * register aligned to 32-bit. This function detects this errata condition.
>>>> + */
>>>> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>>>> +{
>>>> +       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>>>> +               return false;
>>>> +
>>>> +       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
>>>> +               return false;
>>>> +
>>>> +       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>>>> +                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>>>> +               return true;
>>>> +
>>>> +       return false;
>>>> +}
>>>> +
>>>>  /**
>>>>   * parse_spcr() - parse ACPI SPCR table and add preferred console
>>>>   *
>>>> @@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)
>>>>
>>>>         if (qdf2400_erratum_44_present(&table->header))
>>>>                 uart = "qdf2400_e44";
>>>> +       if (xgene_8250_erratum_present(table))
>>>> +               iotype = "mmio32";
>>>>
>>>>         snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
>>>>                  table->serial_port.address, baud_rate);
>>>>
>>>
>>> I didn't see a follow up email on this. What was the conclusion to
>>> this patch series?
>>
>> I have not received an ack, nack, or gtfo from any of the maintainers
>> of this file.  Per
>> ./scripts/get_maintainer.pl drivers/acpi/spcr.c
>> "Rafael J. Wysocki" <rjw(a)rjwysocki.net> (supporter:ACPI)
>> Len Brown <lenb(a)kernel.org> (supporter:ACPI)
>> linux-acpi(a)vger.kernel.org (open list:ACPI)
>> linux-kernel(a)vger.kernel.org (open list)
>>
>> Is there someone else I should be directing this patch through?
>
> Generally, whoever is going to be affected by this change.
>
> git seems to tell me that the spcr.c file went in through the tty tree
> and Aleksey introduced it.
>
> I can apply it if no one has any objections.

I think this is on the mailing list long enough and I don't see anyone
follow up with any objection. Would you applies Jon Mason patch and my
patch on May 8 or inline below? If you need a more formal patch email
for the below patch, I can send one to this email list as well.

==========

tty: 8250: 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. The SPCR always assumes fully compatible
8250. 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(a)apm.com>
---
 drivers/acpi/spcr.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
index 3afa8c1..77b45a0 100644
--- a/drivers/acpi/spcr.c
+++ b/drivers/acpi/spcr.c
@@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
acpi_table_header *h)
        return false;
 }

+/*
+ * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
+ * register aligned to 32-bit. This function detects this errata condition.
+ */
+static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
+{
+       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
+               return false;
+
+       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
+               return false;
+
+       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
+                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
+               return true;
+
+       return false;
+}
+
 /**
  * parse_spcr() - parse ACPI SPCR table and add preferred console
  *
@@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)

        if (qdf2400_erratum_44_present(&table->header))
                uart = "qdf2400_e44";
+       if (xgene_8250_erratum_present(table))
+               iotype = "mmio32";

        snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
                 table->serial_port.address, baud_rate);

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

[Rafael J. Wysocki]

On Mon, Jul 3, 2017 at 8:27 PM, Loc Ho <lho@apm.com> wrote:
> Hi Rafael,
>
>>>>> >> >>>>>> 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.
>>>>>
>>>>> It is a bit more complex that this simple patch. How about this one
>>>>> (my original version). As for Jon Master question on McDivitt, not
>>>>> sure what they use for the ACPI table for SPCR. If they used our
>>>>> reference, then this might work for them too. This version would limit
>>>>> to just the existent firmware or until the SPCR table gets changed.
>>>>>
>>>>>
>>>>> tty: 8250: 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. The SPCR always assumes fully compatible
>>>>> 8250. 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>
>>>>> ---
>>>>>  drivers/acpi/spcr.c | 21 +++++++++++++++++++++
>>>>>  1 file changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>>>>> index 3afa8c1..77b45a0 100644
>>>>> --- a/drivers/acpi/spcr.c
>>>>> +++ b/drivers/acpi/spcr.c
>>>>> @@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
>>>>> acpi_table_header *h)
>>>>>         return false;
>>>>>  }
>>>>>
>>>>> +/*
>>>>> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>>>>> + * register aligned to 32-bit. This function detects this errata condition.
>>>>> + */
>>>>> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>>>>> +{
>>>>> +       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>>>>> +               return false;
>>>>> +
>>>>> +       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
>>>>> +               return false;
>>>>> +
>>>>> +       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>>>>> +                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>>>>> +               return true;
>>>>> +
>>>>> +       return false;
>>>>> +}
>>>>> +
>>>>>  /**
>>>>>   * parse_spcr() - parse ACPI SPCR table and add preferred console
>>>>>   *
>>>>> @@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)
>>>>>
>>>>>         if (qdf2400_erratum_44_present(&table->header))
>>>>>                 uart = "qdf2400_e44";
>>>>> +       if (xgene_8250_erratum_present(table))
>>>>> +               iotype = "mmio32";
>>>>>
>>>>>         snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
>>>>>                  table->serial_port.address, baud_rate);
>>>>>
>>>>
>>>> I didn't see a follow up email on this. What was the conclusion to
>>>> this patch series?
>>>
>>> I have not received an ack, nack, or gtfo from any of the maintainers
>>> of this file.  Per
>>> ./scripts/get_maintainer.pl drivers/acpi/spcr.c
>>> "Rafael J. Wysocki" <rjw@rjwysocki.net> (supporter:ACPI)
>>> Len Brown <lenb@kernel.org> (supporter:ACPI)
>>> linux-acpi@vger.kernel.org (open list:ACPI)
>>> linux-kernel@vger.kernel.org (open list)
>>>
>>> Is there someone else I should be directing this patch through?
>>
>> Generally, whoever is going to be affected by this change.
>>
>> git seems to tell me that the spcr.c file went in through the tty tree
>> and Aleksey introduced it.
>>
>> I can apply it if no one has any objections.
>
> I think this is on the mailing list long enough and I don't see anyone
> follow up with any objection. Would you applies Jon Mason patch and my
> patch on May 8 or inline below? If you need a more formal patch email
> for the below patch, I can send one to this email list as well.

Yes, I do.

Actually, please send both patches as a series with CCs to Greg and
Aleksey.  You can add your sign off to the resend of the Jon's patch.

Thanks,
Rafael

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

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 7428 bytes --]

On Mon, Jul 3, 2017 at 8:27 PM, Loc Ho <lho(a)apm.com> wrote:
> Hi Rafael,
>
>>>>> >> >>>>>> 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.
>>>>>
>>>>> It is a bit more complex that this simple patch. How about this one
>>>>> (my original version). As for Jon Master question on McDivitt, not
>>>>> sure what they use for the ACPI table for SPCR. If they used our
>>>>> reference, then this might work for them too. This version would limit
>>>>> to just the existent firmware or until the SPCR table gets changed.
>>>>>
>>>>>
>>>>> tty: 8250: 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. The SPCR always assumes fully compatible
>>>>> 8250. 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(a)apm.com>
>>>>> ---
>>>>>  drivers/acpi/spcr.c | 21 +++++++++++++++++++++
>>>>>  1 file changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>>>>> index 3afa8c1..77b45a0 100644
>>>>> --- a/drivers/acpi/spcr.c
>>>>> +++ b/drivers/acpi/spcr.c
>>>>> @@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
>>>>> acpi_table_header *h)
>>>>>         return false;
>>>>>  }
>>>>>
>>>>> +/*
>>>>> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>>>>> + * register aligned to 32-bit. This function detects this errata condition.
>>>>> + */
>>>>> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>>>>> +{
>>>>> +       if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>>>>> +               return false;
>>>>> +
>>>>> +       if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
>>>>> +               return false;
>>>>> +
>>>>> +       if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>>>>> +                   ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>>>>> +               return true;
>>>>> +
>>>>> +       return false;
>>>>> +}
>>>>> +
>>>>>  /**
>>>>>   * parse_spcr() - parse ACPI SPCR table and add preferred console
>>>>>   *
>>>>> @@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)
>>>>>
>>>>>         if (qdf2400_erratum_44_present(&table->header))
>>>>>                 uart = "qdf2400_e44";
>>>>> +       if (xgene_8250_erratum_present(table))
>>>>> +               iotype = "mmio32";
>>>>>
>>>>>         snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
>>>>>                  table->serial_port.address, baud_rate);
>>>>>
>>>>
>>>> I didn't see a follow up email on this. What was the conclusion to
>>>> this patch series?
>>>
>>> I have not received an ack, nack, or gtfo from any of the maintainers
>>> of this file.  Per
>>> ./scripts/get_maintainer.pl drivers/acpi/spcr.c
>>> "Rafael J. Wysocki" <rjw(a)rjwysocki.net> (supporter:ACPI)
>>> Len Brown <lenb(a)kernel.org> (supporter:ACPI)
>>> linux-acpi(a)vger.kernel.org (open list:ACPI)
>>> linux-kernel(a)vger.kernel.org (open list)
>>>
>>> Is there someone else I should be directing this patch through?
>>
>> Generally, whoever is going to be affected by this change.
>>
>> git seems to tell me that the spcr.c file went in through the tty tree
>> and Aleksey introduced it.
>>
>> I can apply it if no one has any objections.
>
> I think this is on the mailing list long enough and I don't see anyone
> follow up with any objection. Would you applies Jon Mason patch and my
> patch on May 8 or inline below? If you need a more formal patch email
> for the below patch, I can send one to this email list as well.

Yes, I do.

Actually, please send both patches as a series with CCs to Greg and
Aleksey.  You can add your sign off to the resend of the Jon's patch.

Thanks,
Rafael