[PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[Christopher Covington]

From: Christopher Covington <christopher.covington@linaro.org>

Version 2 of the Server Base System Architecture (SBSA) describes the
Generic UART registers as 32 bits wide. At least one implementation, found
of the Qualcomm Technologies QDF2432, only supports 32 bit accesses. While
other implementations may also support smaller sized accesses, simply use
32 bit accesses all the time for the SBSA UART for simple, broad,
compatibility.

Signed-off-by: Christopher Covington <cov@codeaurora.org>
---
 drivers/tty/serial/amba-pl011.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index c0da0cc..ffb5eb8 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -121,6 +121,7 @@ static struct vendor_data vendor_arm = {
 
 static struct vendor_data vendor_sbsa = {
 	.reg_offset		= pl011_std_offsets,
+	.access_32b		= true,
 	.oversampling		= false,
 	.dma_threshold		= false,
 	.cts_event_workaround	= false,
-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[Mark Langsdorf]

On 03/07/2016 11:43 PM, Christopher Covington wrote:
> From: Christopher Covington <christopher.covington@linaro.org>
>
> Version 2 of the Server Base System Architecture (SBSA) describes the
> Generic UART registers as 32 bits wide. At least one implementation, found
> of the Qualcomm Technologies QDF2432, only supports 32 bit accesses. While
> other implementations may also support smaller sized accesses, simply use
> 32 bit accesses all the time for the SBSA UART for simple, broad,
> compatibility.
>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> ---

I can boot the v4.5-rc7 on my QDF2432 platform with this patch,
and could not otherwise.

Tested-by: Mark Langsdorf <mlangsdo@redhat.com>

[PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[Christopher Covington]

Version 2 of the Server Base System Architecture (SBSAv2) describes the
Generic UART registers as 32 bits wide. At least one implementation, found
on the Qualcomm Technologies QDF2432, only supports 32 bit accesses.
SBSAv3, which describes supported access sizes in greater detail,
explicitly requires support for both 16 and 32 bit accesses to all
registers (and 8 bit accesses to some but not all). Therefore, for broad
compatibility, simply use 32 bit accessors for the SBSA UART.

Tested-by: Mark Langsdorf <mlangsdo@redhat.com>
Signed-off-by: Christopher Covington <cov@codeaurora.org>
---
Changes new in v2:
* Fixed from address
* Elaborated on forward (SBSAv3) compatibility in commit message
* Included Mark Langsdorf's Tested-by, which now covers:
    QDF2432
    Seattle
    X-Gene 1
---
 drivers/tty/serial/amba-pl011.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index c0da0cc..ffb5eb8 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -121,6 +121,7 @@ static struct vendor_data vendor_arm = {
 
 static struct vendor_data vendor_sbsa = {
 	.reg_offset		= pl011_std_offsets,
+	.access_32b		= true,
 	.oversampling		= false,
 	.dma_threshold		= false,
 	.cts_event_workaround	= false,
-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[Peter Hurley]

Hi Christopher,

On 03/10/2016 10:35 PM, Christopher Covington wrote:
> Version 2 of the Server Base System Architecture (SBSAv2) describes the
> Generic UART registers as 32 bits wide. At least one implementation, found
> on the Qualcomm Technologies QDF2432, only supports 32 bit accesses.
> SBSAv3, which describes supported access sizes in greater detail,
> explicitly requires support for both 16 and 32 bit accesses to all
> registers (and 8 bit accesses to some but not all). Therefore, for broad
> compatibility, simply use 32 bit accessors for the SBSA UART.

So this eliminates the need to configure SBSA port via ACPI, correct?
Thus, Aleksey can drop his "serial: pl011: use SPCR to setup 32-bit access"?


> Tested-by: Mark Langsdorf <mlangsdo@redhat.com>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> ---
> Changes new in v2:
> * Fixed from address
> * Elaborated on forward (SBSAv3) compatibility in commit message
> * Included Mark Langsdorf's Tested-by, which now covers:
>     QDF2432
>     Seattle
>     X-Gene 1
> ---
>  drivers/tty/serial/amba-pl011.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index c0da0cc..ffb5eb8 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -121,6 +121,7 @@ static struct vendor_data vendor_arm = {
>  
>  static struct vendor_data vendor_sbsa = {
>  	.reg_offset		= pl011_std_offsets,
> +	.access_32b		= true,
>  	.oversampling		= false,
>  	.dma_threshold		= false,
>  	.cts_event_workaround	= false,
> 

Re: [PATCH v2] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[Christopher Covington]

On March 11, 2016 10:02:14 PM GMT+07:00, Peter Hurley <peter@hurleysoftware.com> wrote:
>Hi Christopher,
>
>On 03/10/2016 10:35 PM, Christopher Covington wrote:
>> Version 2 of the Server Base System Architecture (SBSAv2) describes
>the
>> Generic UART registers as 32 bits wide. At least one implementation,
>found
>> on the Qualcomm Technologies QDF2432, only supports 32 bit accesses.
>> SBSAv3, which describes supported access sizes in greater detail,
>> explicitly requires support for both 16 and 32 bit accesses to all
>> registers (and 8 bit accesses to some but not all). Therefore, for
>broad
>> compatibility, simply use 32 bit accessors for the SBSA UART.
>
>So this eliminates the need to configure SBSA port via ACPI, correct?
>Thus, Aleksey can drop his "serial: pl011: use SPCR to setup 32-bit
>access"?

Yes.

Thanks,
Christopher Covington

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Sent from my Snapdragon powered Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[Andre Przywara]

Hi Christopher,

On 11/03/16 06:35, Christopher Covington wrote:
> Version 2 of the Server Base System Architecture (SBSAv2) describes the
> Generic UART registers as 32 bits wide. At least one implementation, found
> on the Qualcomm Technologies QDF2432, only supports 32 bit accesses.
> SBSAv3, which describes supported access sizes in greater detail,
> explicitly requires support for both 16 and 32 bit accesses to all
> registers (and 8 bit accesses to some but not all). Therefore, for broad
> compatibility, simply use 32 bit accessors for the SBSA UART.
> 
> Tested-by: Mark Langsdorf <mlangsdo@redhat.com>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>

So I gave this a try on a Juno and a Midway. Both have a normal PL011,
but I changed the DT to advertise an SBSA UART instead.
This worked fine with the 32bit accessors.
Also according to some research on the hardware size at least the
current ARM PL011 implementation are totally fine with 32-bit (as well
as 16-bit) accesses.
There is some reluctance about whether this is true for _every_ older
PL011 implementation, but they are out of scope here, as we are talking
about the SBSA only.
So:

Tested-by: Andre Przywara <andre.przywara@arm.com>
Acked-by: Andre Przywara <andre.przywara@arm.com>

You can add Juno and Midway to the list of tested systems.

Cheers,
Andre.

> ---
> Changes new in v2:
> * Fixed from address
> * Elaborated on forward (SBSAv3) compatibility in commit message
> * Included Mark Langsdorf's Tested-by, which now covers:
>     QDF2432
>     Seattle
>     X-Gene 1






> ---
>  drivers/tty/serial/amba-pl011.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index c0da0cc..ffb5eb8 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -121,6 +121,7 @@ static struct vendor_data vendor_arm = {
>  
>  static struct vendor_data vendor_sbsa = {
>  	.reg_offset		= pl011_std_offsets,
> +	.access_32b		= true,
>  	.oversampling		= false,
>  	.dma_threshold		= false,
>  	.cts_event_workaround	= false,
> 

Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[Christopher Covington]

Hi Greg,

On 03/15/2016 06:08 AM, Andre Przywara wrote:
> Hi Christopher,
> 
> On 11/03/16 06:35, Christopher Covington wrote:
>> Version 2 of the Server Base System Architecture (SBSAv2) describes the
>> Generic UART registers as 32 bits wide. At least one implementation, found
>> on the Qualcomm Technologies QDF2432, only supports 32 bit accesses.
>> SBSAv3, which describes supported access sizes in greater detail,
>> explicitly requires support for both 16 and 32 bit accesses to all
>> registers (and 8 bit accesses to some but not all). Therefore, for broad
>> compatibility, simply use 32 bit accessors for the SBSA UART.
>>
>> Tested-by: Mark Langsdorf <mlangsdo@redhat.com>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> 
> So I gave this a try on a Juno and a Midway. Both have a normal PL011,
> but I changed the DT to advertise an SBSA UART instead.
> This worked fine with the 32bit accessors.
> Also according to some research on the hardware size at least the
> current ARM PL011 implementation are totally fine with 32-bit (as well
> as 16-bit) accesses.
> There is some reluctance about whether this is true for _every_ older
> PL011 implementation, but they are out of scope here, as we are talking
> about the SBSA only.
> So:
> 
> Tested-by: Andre Przywara <andre.przywara@arm.com>
> Acked-by: Andre Przywara <andre.przywara@arm.com>
> 
> You can add Juno and Midway to the list of tested systems.

>> Changes new in v2:

Apologies for omitting the v2 prefix in the second version of the patch
that I sent out.

>> * Fixed from address
>> * Elaborated on forward (SBSAv3) compatibility in commit message
>> * Included Mark Langsdorf's Tested-by, which now covers:
>>     QDF2432
>>     Seattle
>>     X-Gene 1

>> ---
>>  drivers/tty/serial/amba-pl011.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>> index c0da0cc..ffb5eb8 100644
>> --- a/drivers/tty/serial/amba-pl011.c
>> +++ b/drivers/tty/serial/amba-pl011.c
>> @@ -121,6 +121,7 @@ static struct vendor_data vendor_arm = {
>>  
>>  static struct vendor_data vendor_sbsa = {
>>  	.reg_offset		= pl011_std_offsets,
>> +	.access_32b		= true,
>>  	.oversampling		= false,
>>  	.dma_threshold		= false,
>>  	.cts_event_workaround	= false,
>>

Do you consider this patch suitable to be included in a 4.6 release
candidate? It fixes an issue running this driver on certain hardware,
and with gracious assistance we've performed due diligence to check that
it does not adversely affect the driver running on other hardware. Would
it be useful to send a v3 collecting the acks and tested-bys?

Thanks,
Christopher Covington

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[Greg Kroah-Hartman]

On Wed, Mar 30, 2016 at 08:30:34AM -0400, Christopher Covington wrote:
> Hi Greg,
> 
> On 03/15/2016 06:08 AM, Andre Przywara wrote:
> > Hi Christopher,
> > 
> > On 11/03/16 06:35, Christopher Covington wrote:
> >> Version 2 of the Server Base System Architecture (SBSAv2) describes the
> >> Generic UART registers as 32 bits wide. At least one implementation, found
> >> on the Qualcomm Technologies QDF2432, only supports 32 bit accesses.
> >> SBSAv3, which describes supported access sizes in greater detail,
> >> explicitly requires support for both 16 and 32 bit accesses to all
> >> registers (and 8 bit accesses to some but not all). Therefore, for broad
> >> compatibility, simply use 32 bit accessors for the SBSA UART.
> >>
> >> Tested-by: Mark Langsdorf <mlangsdo@redhat.com>
> >> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> > 
> > So I gave this a try on a Juno and a Midway. Both have a normal PL011,
> > but I changed the DT to advertise an SBSA UART instead.
> > This worked fine with the 32bit accessors.
> > Also according to some research on the hardware size at least the
> > current ARM PL011 implementation are totally fine with 32-bit (as well
> > as 16-bit) accesses.
> > There is some reluctance about whether this is true for _every_ older
> > PL011 implementation, but they are out of scope here, as we are talking
> > about the SBSA only.
> > So:
> > 
> > Tested-by: Andre Przywara <andre.przywara@arm.com>
> > Acked-by: Andre Przywara <andre.przywara@arm.com>
> > 
> > You can add Juno and Midway to the list of tested systems.
> 
> >> Changes new in v2:
> 
> Apologies for omitting the v2 prefix in the second version of the patch
> that I sent out.
> 
> >> * Fixed from address
> >> * Elaborated on forward (SBSAv3) compatibility in commit message
> >> * Included Mark Langsdorf's Tested-by, which now covers:
> >>     QDF2432
> >>     Seattle
> >>     X-Gene 1
> 
> >> ---
> >>  drivers/tty/serial/amba-pl011.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> >> index c0da0cc..ffb5eb8 100644
> >> --- a/drivers/tty/serial/amba-pl011.c
> >> +++ b/drivers/tty/serial/amba-pl011.c
> >> @@ -121,6 +121,7 @@ static struct vendor_data vendor_arm = {
> >>  
> >>  static struct vendor_data vendor_sbsa = {
> >>  	.reg_offset		= pl011_std_offsets,
> >> +	.access_32b		= true,
> >>  	.oversampling		= false,
> >>  	.dma_threshold		= false,
> >>  	.cts_event_workaround	= false,
> >>
> 
> Do you consider this patch suitable to be included in a 4.6 release
> candidate? It fixes an issue running this driver on certain hardware,
> and with gracious assistance we've performed due diligence to check that
> it does not adversely affect the driver running on other hardware. Would
> it be useful to send a v3 collecting the acks and tested-bys?

If this isn't a bug fix or regression fix, it's not ok for 4.6-final, it
will have to wait for 4.7-rc1.

And yes, collecting the acks and tested-bys would be great, I'm always
glad to see that, it makes my job easier.  Now that 4.6-rc1 is out, I'll
start to dig through the list of pending patches here, give me a few
weeks to get all of them, especially due to the conference travel I'm
currently doing...

thanks,

greg k-h

Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[Timur Tabi]

Greg Kroah-Hartman wrote:
> If this isn't a bug fix or regression fix, it's not ok for 4.6-final, it
> will have to wait for 4.7-rc1.

It fixes a problem on our platform (QDF2432).  Without this patch, we 
can't use the PL011 at all.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[Greg Kroah-Hartman]

On Wed, Mar 30, 2016 at 12:01:56PM -0500, Timur Tabi wrote:
> Greg Kroah-Hartman wrote:
> >If this isn't a bug fix or regression fix, it's not ok for 4.6-final, it
> >will have to wait for 4.7-rc1.
> 
> It fixes a problem on our platform (QDF2432).  Without this patch, we can't
> use the PL011 at all.

Did it ever work before?  Or is this new functionality?

Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[Timur Tabi]

Greg Kroah-Hartman wrote:
> On Wed, Mar 30, 2016 at 12:01:56PM -0500, Timur Tabi wrote:
>> Greg Kroah-Hartman wrote:
>>> If this isn't a bug fix or regression fix, it's not ok for 4.6-final, it
>>> will have to wait for 4.7-rc1.
>>
>> It fixes a problem on our platform (QDF2432).  Without this patch, we can't
>> use the PL011 at all.
>
> Did it ever work before?  Or is this new functionality?

No, it never worked before, so it's not a regression.  I guess it all 
depends on how you define "fix".  The driver loads and attempts to use 
the hardware, but it fails without this patch.  The system locks up 
completely (I guess it throws an unhandled exception or something).

I guess if you take a very limited definition of "fix", then this isn't 
a fix.  I can understand if you didn't want to take it for 4.6-rc7 or 
something, but for 4.6-rc2, I don't think it's inappropriate.

That's my two cents.  We'd like to see it in 4.6-rc2, but the decision 
is yours.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[PATCH v3] tty: amba-pl011: Use 32-bit accesses for SBSA UART

[Christopher Covington]

Version 2 of the Server Base System Architecture (SBSAv2) describes UART
hardware registers as 32 bits wide, giving no guidance on access sizes. The
SBSA UART driver previously assumed partial-length 16 and 8 bit accesses would
work. But the SBSAv2 UART hardware on the Qualcomm Technologies QDF2432 only
supports full-length 32 bit register accesses, so use those exclusively. This
is compatible with SBSAv3, which explicitly requires UART hardware support 32
(and 16 and sometimes 8) bit accesses.

Tested on Juno, Midway, QDF2432, Seattle, and X-Gene 1.

Tested-by: Mark Langsdorf <mlangsdo@redhat.com>
Tested-by: Andre Przywara <andre.przywara@arm.com>
Acked-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Christopher Covington <cov@codeaurora.org>
---
Changes since v2:
* Updated commit message
* Added Andre's Acked-by and Tested-by.
---
 drivers/tty/serial/amba-pl011.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 7c198e0..a2aa655 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -121,6 +121,7 @@ static struct vendor_data vendor_arm = {
 
 static struct vendor_data vendor_sbsa = {
 	.reg_offset		= pl011_std_offsets,
+	.access_32b		= true,
 	.oversampling		= false,
 	.dma_threshold		= false,
 	.cts_event_workaround	= false,
-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project