Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1367172 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART Christopher Covington <cov@codeaurora.org> - 2016-03-30 14:40 +0200
    Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-03-30 19:00 +0200
      Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART Timur Tabi <timur@codeaurora.org> - 2016-03-30 19:10 +0200
        Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-03-30 20:10 +0200
          Re: [PATCH] tty: amba-pl011: Use 32-bit accesses for SBSA UART Timur Tabi <timur@codeaurora.org> - 2016-03-30 20:20 +0200
      [PATCH v3] tty: amba-pl011: Use 32-bit accesses for SBSA UART Christopher Covington <cov@codeaurora.org> - 2016-04-01 23:30 +0200

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

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

[toc] | [next] | [standalone]

#1367436

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

[toc] | [prev] | [next] | [standalone]

#1367467

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.

[toc] | [prev] | [next] | [standalone]

#1367506

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?

[toc] | [prev] | [next] | [standalone]

#1367508

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web