tty: amba-pl011: Use 32-bit accesses for SBSA UART
diff mbox series

Message ID 1457678154-2272-1-git-send-email-cov@codeaurora.org
State New, archived
Headers show
Series
  • tty: amba-pl011: Use 32-bit accesses for SBSA UART
Related show

Commit Message

Christopher Covington March 11, 2016, 6:35 a.m. UTC
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(+)

Comments

Peter Hurley March 11, 2016, 3:02 p.m. UTC | #1
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,
>
Christopher Covington March 11, 2016, 11:38 p.m. UTC | #2
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
Andre Przywara March 15, 2016, 10:08 a.m. UTC | #3
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,
>
Christopher Covington March 30, 2016, 12:30 p.m. UTC | #4
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
Greg KH March 30, 2016, 4:55 p.m. UTC | #5
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
Timur Tabi March 30, 2016, 5:01 p.m. UTC | #6
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.
Greg KH March 30, 2016, 6:01 p.m. UTC | #7
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?
Timur Tabi March 30, 2016, 6:11 p.m. UTC | #8
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.

Patch
diff mbox series

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,