[Qemu-devel] [PATCH] PL011: Fix ID reporting

[Qemu-devel] [PATCH] PL011: Fix ID reporting

[Marek Vasut]

The AMBA IDs are supposed to be at the end of 0x2000 block, which the PL011 UART
allocates. Current QEMU implementation puts those IDs at 0x1000 offset, which is
wrong. The QEMU implementation also allocates only 0x1000 instead of 0x2000 of
space.

The fix is tested to work with Linux's PL011 driver and U-Boot running in QEMU.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
---
 hw/pl011.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pl011.c b/hw/pl011.c
index 1b05d76..49d4de0 100644
--- a/hw/pl011.c
+++ b/hw/pl011.c
@@ -60,8 +60,8 @@ static uint64_t pl011_read(void *opaque, target_phys_addr_t offset,
     pl011_state *s = (pl011_state *)opaque;
     uint32_t c;
 
-    if (offset >= 0xfe0 && offset < 0x1000) {
-        return s->id[(offset - 0xfe0) >> 2];
+    if (offset >= 0x1fe0 && offset < 0x2000) {
+        return s->id[(offset - 0x1fe0) >> 2];
     }
     switch (offset >> 2) {
     case 0: /* UARTDR */
@@ -260,7 +260,7 @@ static int pl011_init(SysBusDevice *dev, const unsigned char *id)
 {
     pl011_state *s = FROM_SYSBUS(pl011_state, dev);
 
-    memory_region_init_io(&s->iomem, &pl011_ops, s, "pl011", 0x1000);
+    memory_region_init_io(&s->iomem, &pl011_ops, s, "pl011", 0x2000);
     sysbus_init_mmio(dev, &s->iomem);
     sysbus_init_irq(dev, &s->irq);
     s->id = id;
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH] PL011: Fix ID reporting

[Peter Maydell]

On 7 January 2012 20:11, Marek Vasut <marek.vasut@gmail.com> wrote:
> The AMBA IDs are supposed to be at the end of 0x2000 block, which the PL011 UART
> allocates. Current QEMU implementation puts those IDs at 0x1000 offset, which is
> wrong. The QEMU implementation also allocates only 0x1000 instead of 0x2000 of
> space.

Why do you think this change is correct? The PL011 TRM
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183g/I18381.html
says the ID registers are at 0xFE0..0xFFC.
and for instance on the PBX-A9 devboard:
http://infocenter.arm.com/help/topic/com.arm.doc.dui0440b/Bbajihec.html
the UARTs are at 0x10009000, 0x1000A000, 0x1000B000, 0x1000C000, so
they clearly can't be 0x2000 in size.

-- PMM

Re: [Qemu-devel] [PATCH] PL011: Fix ID reporting

[Marek Vasut]

> On 7 January 2012 20:11, Marek Vasut <marek.vasut@gmail.com> wrote:
> > The AMBA IDs are supposed to be at the end of 0x2000 block, which the
> > PL011 UART allocates. Current QEMU implementation puts those IDs at
> > 0x1000 offset, which is wrong. The QEMU implementation also allocates
> > only 0x1000 instead of 0x2000 of space.
> 
> Why do you think this change is correct? The PL011 TRM
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183g/I18381.html
> says the ID registers are at 0xFE0..0xFFC.
> and for instance on the PBX-A9 devboard:
> http://infocenter.arm.com/help/topic/com.arm.doc.dui0440b/Bbajihec.html
> the UARTs are at 0x10009000, 0x1000A000, 0x1000B000, 0x1000C000, so
> they clearly can't be 0x2000 in size.

Then we have a problem, because eg. on freescale mx28 they are 0x2000 big. The 
only conclusion I can draw from this is that the size of the segment that can be 
assigned to PL011 is variable and the ID-octet is always at the end.

M
> 
> -- PMM

Re: [Qemu-devel] [PATCH] PL011: Fix ID reporting

[Peter Maydell]

On 7 January 2012 20:56, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 7 January 2012 20:11, Marek Vasut <marek.vasut@gmail.com> wrote:
>> > The AMBA IDs are supposed to be at the end of 0x2000 block, which the
>> > PL011 UART allocates. Current QEMU implementation puts those IDs at
>> > 0x1000 offset, which is wrong. The QEMU implementation also allocates
>> > only 0x1000 instead of 0x2000 of space.
>>
>> Why do you think this change is correct? The PL011 TRM
>> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183g/I18381.html
>> says the ID registers are at 0xFE0..0xFFC.
>> and for instance on the PBX-A9 devboard:
>> http://infocenter.arm.com/help/topic/com.arm.doc.dui0440b/Bbajihec.html
>> the UARTs are at 0x10009000, 0x1000A000, 0x1000B000, 0x1000C000, so
>> they clearly can't be 0x2000 in size.
>
> Then we have a problem, because eg. on freescale mx28 they are 0x2000 big. The
> only conclusion I can draw from this is that the size of the segment that can be
> assigned to PL011 is variable and the ID-octet is always at the end.

Can you point me to some documentation? I looked at
http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf?fsrch=1&sr=7
(i.MX28 Applications Processor Reference Manual) and although it has
a number of UARTs none of them looked obviously like PL011s.

-- PMM

Re: [Qemu-devel] [PATCH] PL011: Fix ID reporting

[Marek Vasut]

> On 7 January 2012 20:56, Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 7 January 2012 20:11, Marek Vasut <marek.vasut@gmail.com> wrote:
> >> > The AMBA IDs are supposed to be at the end of 0x2000 block, which the
> >> > PL011 UART allocates. Current QEMU implementation puts those IDs at
> >> > 0x1000 offset, which is wrong. The QEMU implementation also allocates
> >> > only 0x1000 instead of 0x2000 of space.
> >> 
> >> Why do you think this change is correct? The PL011 TRM
> >> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0183g/I18381.html
> >> says the ID registers are at 0xFE0..0xFFC.
> >> and for instance on the PBX-A9 devboard:
> >> http://infocenter.arm.com/help/topic/com.arm.doc.dui0440b/Bbajihec.html
> >> the UARTs are at 0x10009000, 0x1000A000, 0x1000B000, 0x1000C000, so
> >> they clearly can't be 0x2000 in size.
> > 
> > Then we have a problem, because eg. on freescale mx28 they are 0x2000
> > big. The only conclusion I can draw from this is that the size of the
> > segment that can be assigned to PL011 is variable and the ID-octet is
> > always at the end.
> 
> Can you point me to some documentation? I looked at
> http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf?fsrch=1&s
> r=7 (i.MX28 Applications Processor Reference Manual) and although it has a
> number of UARTs none of them looked obviously like PL011s.

That's it, look at DUART section and Memory map section.

M
> 
> -- PMM

Re: [Qemu-devel] [PATCH] PL011: Fix ID reporting

[Peter Maydell]

On 8 January 2012 16:02, Marek Vasut <marek.vasut@gmail.com> wrote:
>> Can you point me to some documentation? I looked at
>> http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf?fsrch=1&s
>> r=7 (i.MX28 Applications Processor Reference Manual) and although it has a
>> number of UARTs none of them looked obviously like PL011s.
>
> That's it, look at DUART section and Memory map section.

That doesn't document that it implements the ID registers at all :-)
Does the hardware give the ID registers the same values as stock
PL011 or are the part number/designer/etc fields different?

Anyway, the right approach to this is going to be to have either
(a) a qdev device with a different name [this is how we handle
the pl061 variant in the Stellaris Luminary, for instance]
(b) qdev properties
so you can get the mx28 right.

-- PMM

Re: [Qemu-devel] [PATCH] PL011: Fix ID reporting

[Marek Vasut]

> On 8 January 2012 16:02, Marek Vasut <marek.vasut@gmail.com> wrote:
> >> Can you point me to some documentation? I looked at
> >> http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf?fsrch=
> >> 1&s r=7 (i.MX28 Applications Processor Reference Manual) and although it
> >> has a number of UARTs none of them looked obviously like PL011s.
> > 
> > That's it, look at DUART section and Memory map section.
> 
> That doesn't document that it implements the ID registers at all :-)

Well ... can't be helped.

> Does the hardware give the ID registers the same values as stock
> PL011 or are the part number/designer/etc fields different?

Slightly different, but compatible with Linux:

80075fe0: 00000011 00000010 00000024 00000000    ........$.......
80075ff0: 0000000d 000000f0 00000005 000000b1    ................
> 
> Anyway, the right approach to this is going to be to have either
> (a) a qdev device with a different name [this is how we handle
> the pl061 variant in the Stellaris Luminary, for instance]
> (b) qdev properties
> so you can get the mx28 right.

This seems more correct/extensible approach.

M

> 
> -- PMM

Re: [Qemu-devel] [PATCH] PL011: Fix ID reporting

[Peter Maydell]

On 8 January 2012 16:57, Marek Vasut <marek.vasut@gmail.com> wrote:
>Peter Maydell wrote:
>> Does the hardware give the ID registers the same values as stock
>> PL011 or are the part number/designer/etc fields different?
>
> Slightly different, but compatible with Linux:
>
> 80075fe0: 00000011 00000010 00000024 00000000    ........$.......
> 80075ff0: 0000000d 000000f0 00000005 000000b1    ................

OK, that's just the patchlevel -- QEMU currently reports
itself as r1p1 (0x14 in UARTPeriphID2), and mx28 is r1p3
or r1p4 (0x24). The manual describes no functional differences
between r1p0 up to r1p4 (r1p5 has a different fifo size) so
I'd be inclined not to worry about the ID registers here.

-- PMM

Re: [Qemu-devel] [PATCH] PL011: Fix ID reporting

[Marek Vasut]

> On 8 January 2012 16:57, Marek Vasut <marek.vasut@gmail.com> wrote:
> >Peter Maydell wrote:
> >> Does the hardware give the ID registers the same values as stock
> >> PL011 or are the part number/designer/etc fields different?
> > 
> > Slightly different, but compatible with Linux:
> > 
> > 80075fe0: 00000011 00000010 00000024 00000000    ........$.......
> > 80075ff0: 0000000d 000000f0 00000005 000000b1    ................
> 
> OK, that's just the patchlevel -- QEMU currently reports
> itself as r1p1 (0x14 in UARTPeriphID2), and mx28 is r1p3
> or r1p4 (0x24). The manual describes no functional differences
> between r1p0 up to r1p4 (r1p5 has a different fifo size) so
> I'd be inclined not to worry about the ID registers here.

It's just their position that's different indeed. And even this can be tuned via 
props. I'll update the patch once I get FEC operational.

M
> 
> -- PMM

Re: [Qemu-devel] [PATCH] PL011: Fix ID reporting

[Peter Maydell]

On 8 January 2012 19:44, Marek Vasut <marek.vasut@gmail.com> wrote:
> It's just their position that's different indeed. And even this can be tuned via
> props. I'll update the patch once I get FEC operational.

Incidentally, since this is only needed for a platform we don't
currently support, you should submit the patch as part of a
series adding support for that platform. We don't generally
take this kind of thing as a standalone patch, since there's
no justification for it without the platform that needs it.

-- PMM

Re: [Qemu-devel] [PATCH] PL011: Fix ID reporting

[Marek Vasut]

> On 8 January 2012 19:44, Marek Vasut <marek.vasut@gmail.com> wrote:
> > It's just their position that's different indeed. And even this can be
> > tuned via props. I'll update the patch once I get FEC operational.
> 
> Incidentally, since this is only needed for a platform we don't
> currently support, you should submit the patch as part of a
> series adding support for that platform.

Yep, will do!

> We don't generally
> take this kind of thing as a standalone patch, since there's
> no justification for it without the platform that needs it.

Definitelly.

> 
> -- PMM