[PATCH] ARM: PL011: add support for extended FIFO-size of PL011-r1p5

[PATCH] ARM: PL011: add support for extended FIFO-size of PL011-r1p5

[Stephen Warren]

On 04/12/2013 03:18 AM, Jongsung Kim wrote:
> The latest r1p5-revision of the ARM PL011 UART has 32-byte FIFOs, while all
> earlier ones have 16-byte FIFOs. This patch suggests a way to set the
> FIFO-size correctly & flexibly by using a
> function(vendor_data::get_fifosize) rather than using the
> vendor_data::fifosize variable. The function takes the UARTPeriphID, and
> returns the correct size.

This change (now part of 3.10-rc1) breaks the serial port on the BCM2835
ARM SoC (part of the Raspberry Pi). Sorry for not noticing this earlier;
a combination of my vacation and laziness I guess.

For reference, the AMBA periphid of the UART device there is 0x00341011.
The nibble "3" is the revision being tested in:

> +static unsigned int get_fifosize_arm(unsigned int periphid)
> +{
> +	unsigned int rev = (periphid >> 20) & 0xf;
> +	return rev < 3 ? 16 : 32;
> +}

Should that be <= not <, or is there just something more wrong in the
patch or bcm2835 HW? I wonder how r1p5 maps to 3 in the test above.

[PATCH] ARM: PL011: add support for extended FIFO-size of PL011-r1p5

[Jongsung Kim]

Stephen Warren <swarren@wwwdotorg.org> :
> For reference, the AMBA periphid of the UART device there is 0x00341011.
> The nibble "3" is the revision being tested in:

The UART device has periphid 0x00341011, and is compatible with the
original PL011 prior to r1p5. Not with r1p5. It could be a possible
way to specify the compatible periphid (such as 0x00241011) instead
of just 0x0 when initializing the amba_device for the UART.

> > +static unsigned int get_fifosize_arm(unsigned int periphid)
> > +{
> > +	unsigned int rev = (periphid >> 20) & 0xf;
> > +	return rev < 3 ? 16 : 32;
> > +}
>
> Should that be <= not <, or is there just something more wrong in the
> patch or bcm2835 HW? I wonder how r1p5 maps to 3 in the test above.

>From the PL011-r1p5 TRM, bits[7:4] of the UARTPeriphID2 register are
read as:

r1p0 - 0x0
r1p1 - 0x1
r1p3 - 0x2
r1p4 - 0x2
r1p5 - 0x3.

Doesn't the BCM2835 UART have anything different from the ARM PL011?
What about the UARTPCellID registers? They are set to 0xb105f00d with
the ARM PL011.

[PATCH] ARM: PL011: add support for extended FIFO-size of PL011-r1p5

[Stephen Warren]

On 05/14/2013 01:15 AM, Jongsung Kim wrote:
> Stephen Warren <swarren@wwwdotorg.org> :
>> For reference, the AMBA periphid of the UART device there is 0x00341011.
>> The nibble "3" is the revision being tested in:
> 
> The UART device has periphid 0x00341011, and is compatible with the
> original PL011 prior to r1p5. Not with r1p5. It could be a possible
> way to specify the compatible periphid (such as 0x00241011) instead
> of just 0x0 when initializing the amba_device for the UART.
> 
>>> +static unsigned int get_fifosize_arm(unsigned int periphid)
>>> +{
>>> +	unsigned int rev = (periphid >> 20) & 0xf;
>>> +	return rev < 3 ? 16 : 32;
>>> +}
...
> Doesn't the BCM2835 UART have anything different from the ARM PL011?
> What about the UARTPCellID registers? They are set to 0xb105f00d with
> the ARM PL011.

Looking at BCM2835-ARM-Peripherals.pdf (i.e. the public documentation
for the BCM2835 chip), I see:

=====
The UART provides:
* Separate 16x8 transmit and 16x12 receive FIFO memory.
...
For the in-depth UART overview, please, refer to the ARM PrimeCell UART
(PL011) Revision: r1p5 Technical Reference Manual.
=====

That seems to imply that not all r1p5 PL011s actually have a depth-32
FIFO. Perhaps this is a configurable property of the IP block, not
something that all r1p5 have?

I can't check the UARTPCellID registers right now.

[PATCH] ARM: PL011: add support for extended FIFO-size of PL011-r1p5

[Russell King - ARM Linux]

On Tue, May 14, 2013 at 03:03:14PM -0600, Stephen Warren wrote:
> On 05/14/2013 01:15 AM, Jongsung Kim wrote:
> > Stephen Warren <swarren@wwwdotorg.org> :
> >> For reference, the AMBA periphid of the UART device there is 0x00341011.
> >> The nibble "3" is the revision being tested in:
> > 
> > The UART device has periphid 0x00341011, and is compatible with the
> > original PL011 prior to r1p5. Not with r1p5. It could be a possible
> > way to specify the compatible periphid (such as 0x00241011) instead
> > of just 0x0 when initializing the amba_device for the UART.
> > 
> >>> +static unsigned int get_fifosize_arm(unsigned int periphid)
> >>> +{
> >>> +	unsigned int rev = (periphid >> 20) & 0xf;
> >>> +	return rev < 3 ? 16 : 32;
> >>> +}
> ...
> > Doesn't the BCM2835 UART have anything different from the ARM PL011?
> > What about the UARTPCellID registers? They are set to 0xb105f00d with
> > the ARM PL011.
> 
> Looking at BCM2835-ARM-Peripherals.pdf (i.e. the public documentation
> for the BCM2835 chip), I see:
> 
> =====
> The UART provides:
> * Separate 16x8 transmit and 16x12 receive FIFO memory.
> ...
> For the in-depth UART overview, please, refer to the ARM PrimeCell UART
> (PL011) Revision: r1p5 Technical Reference Manual.
> =====
> 
> That seems to imply that not all r1p5 PL011s actually have a depth-32
> FIFO. Perhaps this is a configurable property of the IP block, not
> something that all r1p5 have?
> 
> I can't check the UARTPCellID registers right now.

The PCellID value is a marker for primecells, and is common to all primecells
which implement the ID scheme.  It's the other ID registers you want. :)

[PATCH] ARM: PL011: add support for extended FIFO-size of PL011-r1p5

[Jongsung Kim]

Stephen Warren <swarren@wwwdotorg.org> :
> Looking at BCM2835-ARM-Peripherals.pdf (i.e. the public documentation for
> the BCM2835 chip), I see:
>
> =====
> The UART provides:
> * Separate 16x8 transmit and 16x12 receive FIFO memory.
> ...
> For the in-depth UART overview, please, refer to the ARM PrimeCell UART
> (PL011) Revision: r1p5 Technical Reference Manual.
> =====
>
> That seems to imply that not all r1p5 PL011s actually have a depth-32
FIFO.
> Perhaps this is a configurable property of the IP block, not something
that
> all r1p5 have?

All r1p5 have 32-byte FIFO depth and it's not configurable. From the PL011
TRM:

r1p4-r1p5	Contains the following differences in functionality:
		* The receive and transmit FIFOs are increased to a depth of
32.
		* The Revision field in the UARTPeriphID2 Register on page
3-24
		  bits [7:4] now reads back as 0x3.

[PATCH] ARM: PL011: add support for extended FIFO-size of PL011-r1p5

[Stephen Warren]

On 05/14/2013 07:00 PM, Jongsung Kim wrote:
> Stephen Warren <swarren@wwwdotorg.org> :
>> Looking at BCM2835-ARM-Peripherals.pdf (i.e. the public documentation for
>> the BCM2835 chip), I see:
>>
>> =====
>> The UART provides:
>> * Separate 16x8 transmit and 16x12 receive FIFO memory.
>> ...
>> For the in-depth UART overview, please, refer to the ARM PrimeCell UART
>> (PL011) Revision: r1p5 Technical Reference Manual.
>> =====
>>
>> That seems to imply that not all r1p5 PL011s actually have a depth-32 FIFO.
>> Perhaps this is a configurable property of the IP block, not something that
>> all r1p5 have?
> 
> All r1p5 have 32-byte FIFO depth and it's not configurable. From the PL011
> TRM:
> 
> r1p4-r1p5	Contains the following differences in functionality:
> 		* The receive and transmit FIFOs are increased to a depth of 32.
> 		* The Revision field in the UARTPeriphID2 Register on page 3-24
> 		  bits [7:4] now reads back as 0x3.

Well, that certainly isn't true in practice. I think we should revert
this commit until we can determine what the problem is.

I validated that the periphid register in HW contains the r1p5 revision
(3), and the pcellid register does indeed contain the expected
0xb105f00d value. However, if I run the following hacky code in U-Boot
to determine the FIFO depth, it comes out as 16, which explains the
symptoms I'm seeing:

void find_fifo_depth(void)
{
	volatile u8 *uart = 0x20201000;
	int depth = 0;

	/* Wait for TX FIFO empty */
	while (!(uart[0x18] & 0x80))
		;

	/* Disable UART */
	uart[0x30] &= ~1;

	/* Push chars into TX FIFO until full */
	for (;;) {
		uart[0] = 'A' + depth;
		depth++;
		/* Done if FIFO full */
		if (uart[0x18] & 0x20)
			break;
		if (depth > 64) {
			depth = -1;
			break;
		}
	}

	/* Re-enable UART */
	uart[0x30] |= 1;

	printf("FIFO depth: %d\n", depth);
}

[PATCH] ARM: PL011: add support for extended FIFO-size of PL011-r1p5

[Russell King - ARM Linux]

On Tue, May 14, 2013 at 10:59:58PM -0600, Stephen Warren wrote:
> Well, that certainly isn't true in practice. I think we should revert
> this commit until we can determine what the problem is.
> 
> I validated that the periphid register in HW contains the r1p5 revision
> (3), and the pcellid register does indeed contain the expected
> 0xb105f00d value. However, if I run the following hacky code in U-Boot
> to determine the FIFO depth, it comes out as 16, which explains the
> symptoms I'm seeing:

We could do that, just like we do in 8250.c to check the FIFO depth.
There's not much harm in doing that at boot time, it just needs to be
done carefully so that it doesn't disrupt any existing use of the UART.

[PATCH] ARM: PL011: add support for extended FIFO-size of PL011-r1p5

[Jongsung Kim]

Stephen Warren <swarren@wwwdotorg.org> :
>> All r1p5 have 32-byte FIFO depth and it's not configurable. From the
PL011
>> TRM:
>> 
>> r1p4-r1p5	Contains the following differences in functionality:
>> 		* The receive and transmit FIFOs are increased to a depth of
32.
>> 		* The Revision field in the UARTPeriphID2 Register on page
3-24
>> 		  bits [7:4] now reads back as 0x3.
>
> Well, that certainly isn't true in practice. I think we should revert
> this commit until we can determine what the problem is.

I asked to the ARM support about this. Waiting for reply..

[PATCH] ARM: PL011: add support for extended FIFO-size of PL011-r1p5

[Jongsung Kim]

Jongsung Kim <neidhard.kim@lge.com> :
> Stephen Warren <swarren@wwwdotorg.org> :
>>> All r1p5 have 32-byte FIFO depth and it's not configurable. From the 
>>> PL011
>>> TRM:
>>> 
>>> r1p4-r1p5	Contains the following differences in functionality:
>>> 		* The receive and transmit FIFOs are increased to a depth of
32.
>>> 		* The Revision field in the UARTPeriphID2 Register on page
3-24
>>> 		  bits [7:4] now reads back as 0x3.
>>
>> Well, that certainly isn't true in practice. I think we should revert 
>> this commit until we can determine what the problem is.
>
> I asked to the ARM support about this. Waiting for reply..

ARM support said they doesn't have information about BCM2835 UART. Does
anyone have a communication channel to Broadcom? It takes time for me to
get contact point to Broadcom.. (I'm trying)

However, ARM support also said:

"If the Broadcom part definitely has 16-deep FIFOs, it cannot be based
on a PL011 r1p5, so I might guess that Broadcom have just referenced
the latest version of the documentation on our website, but have actually
implemented an earlier version."

[PATCH] ARM: PL011: add support for extended FIFO-size of PL011-r1p5

[Stephen Warren]

On 05/20/2013 07:39 PM, Jongsung Kim wrote:
> Jongsung Kim <neidhard.kim@lge.com> :
>> Stephen Warren <swarren@wwwdotorg.org> :
>>>> All r1p5 have 32-byte FIFO depth and it's not configurable. From the 
>>>> PL011
>>>> TRM:
>>>>
>>>> r1p4-r1p5	Contains the following differences in functionality:
>>>> 		* The receive and transmit FIFOs are increased to a depth of
> 32.
>>>> 		* The Revision field in the UARTPeriphID2 Register on page
> 3-24
>>>> 		  bits [7:4] now reads back as 0x3.
>>>
>>> Well, that certainly isn't true in practice. I think we should revert 
>>> this commit until we can determine what the problem is.
>>
>> I asked to the ARM support about this. Waiting for reply..
> 
> ARM support said they doesn't have information about BCM2835 UART. Does
> anyone have a communication channel to Broadcom? It takes time for me to
> get contact point to Broadcom.. (I'm trying)
> 
> However, ARM support also said:
> 
> "If the Broadcom part definitely has 16-deep FIFOs, it cannot be based
> on a PL011 r1p5, so I might guess that Broadcom have just referenced
> the latest version of the documentation on our website, but have actually
> implemented an earlier version."

This all seems rather academic. Irrespective of what the cause of the
problem is, the commit actively breaks a previously working
configuration. I still believe we should revert it first, then find out
exactly what's going on later. Should I sent the revert commit?

[PATCH] ARM: bcm2835: override the HW UART periphid

[Jongsung Kim]

Stephen Warren reported the recent commit 78506f2 (add support for
extended FIFO-size of PL011-r1p5) breaks the serial port on the
BCM2835 ARM SoC.

A UART compatible with the ARM PL011-r1p5 should have 32-deep FIFOs.
The BCM2835 UART just looks like an ARM PL011-r1p5, but has 16-deep
FIFOs just like PL011-r1p4 or earlier revisions. As a workaround for
this compatibility issue, this patch overrides the HW UART periphid
register values with the actually compatible UART periphid 0x00241011
(r1p3 or r1p4).

Reported-by: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
---
 arch/arm/boot/dts/bcm2835.dtsi |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index f0052dc..1e12aef 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -44,6 +44,7 @@
 			reg = <0x7e201000 0x1000>;
 			interrupts = <2 25>;
 			clock-frequency = <3000000>;
+			arm,primecell-periphid = <0x00241011>;
 		};
 
 		gpio: gpio {
-- 
1.7.1

[PATCH] ARM: bcm2835: override the HW UART periphid

[Jongsung Kim]

Jongsung Kim <neidhard.kim@lge.com> :
> diff --git a/arch/arm/boot/dts/bcm2835.dtsi
b/arch/arm/boot/dts/bcm2835.dtsi
> index f0052dc..1e12aef 100644
> --- a/arch/arm/boot/dts/bcm2835.dtsi
> +++ b/arch/arm/boot/dts/bcm2835.dtsi
> @@ -44,6 +44,7 @@
>  			reg = <0x7e201000 0x1000>;
>  			interrupts = <2 25>;
>  			clock-frequency = <3000000>;
> +			arm,primecell-periphid = <0x00241011>;
>  		};
>  
>  		gpio: gpio {

Stephen, how do you think about this kind of approach instead?

[PATCH] ARM: bcm2835: override the HW UART periphid

[Gordon Hollingworth]

Have checked with the designer of the UART block and he confirmed that
the 2835 PL011 contains a 16 deep fifo not 32 deep...

Hardware guys, they can never just leave it alone!!!

Gordon

On 21 May 2013 07:07, Jongsung Kim <neidhard.kim@lge.com> wrote:
> Jongsung Kim <neidhard.kim@lge.com> :
>> diff --git a/arch/arm/boot/dts/bcm2835.dtsi
> b/arch/arm/boot/dts/bcm2835.dtsi
>> index f0052dc..1e12aef 100644
>> --- a/arch/arm/boot/dts/bcm2835.dtsi
>> +++ b/arch/arm/boot/dts/bcm2835.dtsi
>> @@ -44,6 +44,7 @@
>>                       reg = <0x7e201000 0x1000>;
>>                       interrupts = <2 25>;
>>                       clock-frequency = <3000000>;
>> +                     arm,primecell-periphid = <0x00241011>;
>>               };
>>
>>               gpio: gpio {
>
> Stephen, how do you think about this kind of approach instead?
>
>
> _______________________________________________
> linux-rpi-kernel mailing list
> linux-rpi-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

[PATCH] ARM: bcm2835: override the HW UART periphid

[Stephen Warren]

On 05/21/2013 12:02 AM, Jongsung Kim wrote:
> Stephen Warren reported the recent commit 78506f2 (add support for
> extended FIFO-size of PL011-r1p5) breaks the serial port on the
> BCM2835 ARM SoC.
> 
> A UART compatible with the ARM PL011-r1p5 should have 32-deep FIFOs.
> The BCM2835 UART just looks like an ARM PL011-r1p5, but has 16-deep
> FIFOs just like PL011-r1p4 or earlier revisions. As a workaround for
> this compatibility issue, this patch overrides the HW UART periphid
> register values with the actually compatible UART periphid 0x00241011
> (r1p3 or r1p4).
> 
> Reported-by: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>

I know this will work, because I tried out the same thing last week.

However, I'm not convinced that it's the correct approach. What other
changes exist between r1p4 and r1p5; can you check in the TRM? Faking
the periphid would prevent the driver from taking account of any other
changes. Should we instead add a DT property solely to override the FIFO
size, and then set that for bcm2835? I guess if there really aren't any
other SW-visible changes in r1p5, this approach is fine.

[PATCH] ARM: bcm2835: override the HW UART periphid

[Stephen Warren]

On 05/21/2013 10:34 AM, Stephen Warren wrote:
> On 05/21/2013 12:02 AM, Jongsung Kim wrote:
>> Stephen Warren reported the recent commit 78506f2 (add support for
>> extended FIFO-size of PL011-r1p5) breaks the serial port on the
>> BCM2835 ARM SoC.
>>
>> A UART compatible with the ARM PL011-r1p5 should have 32-deep FIFOs.
>> The BCM2835 UART just looks like an ARM PL011-r1p5, but has 16-deep
>> FIFOs just like PL011-r1p4 or earlier revisions. As a workaround for
>> this compatibility issue, this patch overrides the HW UART periphid
>> register values with the actually compatible UART periphid 0x00241011
>> (r1p3 or r1p4).
>>
>> Reported-by: Stephen Warren <swarren@wwwdotorg.org>
>> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> 
> I know this will work, because I tried out the same thing last week.
> 
> However, I'm not convinced that it's the correct approach. What other
> changes exist between r1p4 and r1p5; can you check in the TRM?

Looking at the TRM, it seems this is really the only change, according
to the changelog in the documentation (although it's a little difficult
to tell since the document seems to have a bunch of changes that
presumably don't affect behaviour). So, faking the periphid seems OK.

Acked-by: Stephen Warren <swarren@wwwdotorg.org>

Let's apply for 3.10.

[PATCH] ARM: bcm2835: override the HW UART periphid

[Jongsung Kim]

Stephen Warren <swarren@wwwdotorg.org> :
> Looking at the TRM, it seems this is really the only change, according
> to the changelog in the documentation (although it's a little difficult
> to tell since the document seems to have a bunch of changes that
presumably
> don't affect behaviour). So, faking the periphid seems OK.
>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
>
> Let's apply for 3.10.

Thank you, Stephen.