Amba CLCD register definitions

Amba CLCD register definitions

[Michael Hunold]

Hello,

I'm working on adding support for panning and FBIO_WAITFORVSYNC ioctl to
the Amba CLCD driver.

While working on that, I came across the following oddity. The Amba CLCD
driver supports both PL110 and PL111 primecells.

The PL110 register summary can be found here:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0161e/I904416.html

The PL111 register summary can be found here:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0293c/Chdehhhf.html

In include/linux/amba/clcd.h the following register definitions are made:

#define CLCD_PL110_IENB		0x00000018
#define CLCD_PL110_CNTL		0x0000001c
#define CLCD_PL110_STAT		0x00000020
#define CLCD_PL110_INTR 	0x00000024
#define CLCD_PL110_UCUR		0x00000028
#define CLCD_PL110_LCUR		0x0000002C

#define CLCD_PL111_CNTL		0x00000018
#define CLCD_PL111_IENB		0x0000001c
#define CLCD_PL111_RIS		0x00000020
#define CLCD_PL111_MIS		0x00000024
#define CLCD_PL111_ICR		0x00000028
#define CLCD_PL111_UCUR		0x0000002c
#define CLCD_PL111_LCUR		0x00000030

According to the PL110 register summary, STAT is called RIS as well and
INTR is called MIS as well, similar to the PL111 naming.

UCUR and LCUR definitions are wrong, according to the PL110 register
summary. UCUR should be at 0x2c and LCUR should be at 0x30, the same as
for PL111.

It seems PL110 has an ICR register as well, at 0x28, the same as the PL111.

As a sidenote, IENB is called IMSC in both register summaries, and
indeed CNTL and IENB/IMSC registers are swapped for PL110 and PL111.

So I would propose to rename and reorder the register definitions like this:

#define CLCD_TIM0		0x00000000
#define CLCD_TIM1 		0x00000004
#define CLCD_TIM2 		0x00000008
#define CLCD_TIM3 		0x0000000c
#define CLCD_UBAS 		0x00000010
#define CLCD_LBAS 		0x00000014
#define CLCD_PL110_IMSC		0x00000018
#define CLCD_PL110_CNTL		0x0000001c
#define CLCD_PL111_CNTL		0x00000018
#define CLCD_PL111_IMSC		0x0000001c
#define CLCD_RIS		0x00000020
#define CLCD_MIS		0x00000024
#define CLCD_ICR		0x00000028
#define CLCD_UCUR		0x0000002c
#define CLCD_LCUR		0x00000030

Please note that these registers are currently *not* used in the driver,
so renaming them has not impact on existing code, except for IENB.
Probably this is the reason, why nobody has noticed the naming problems
so far and cared about it.

Comments?

Should I sent a patch that accomplishes this change?

CU
Michael.

Amba CLCD register definitions

[Russell King - ARM Linux]

On Tue, Mar 08, 2011 at 09:57:42AM +0100, Michael Hunold wrote:
> While working on that, I came across the following oddity. The Amba CLCD
> driver supports both PL110 and PL111 primecells.
> 
> The PL110 register summary can be found here:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0161e/I904416.html

PL110 is rather screwed up because there's various revisions and
implementations within ARM which have different register layouts and
register namings.  Eg, you'll find Versatile has a PL110 fitted but has
the PL111 register layout with IENB and CNTL swapped.

I don't remember the ancestry of the names, but DDI0161D only gives the
registers long names.  They probably came from an implementation provided
by ARM in years gone by, which would've been created by those doing
platform support back in the early days.

It's only with DDI0161E that they've been given short names.

I don't think we should change the names just because someone decided to
add such things to the latest revision of documentation.

Amba CLCD register definitions

[Michael Hunold]

Hello Russell,

on 03/08/2011 10:36 AM Russell King - ARM Linux said the following:
> On Tue, Mar 08, 2011 at 09:57:42AM +0100, Michael Hunold wrote:

>> While working on that, I came across the following oddity. The Amba CLCD
>> driver supports both PL110 and PL111 primecells.
>>
>> The PL110 register summary can be found here:
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0161e/I904416.html
> 
> PL110 is rather screwed up because there's various revisions and
> implementations within ARM which have different register layouts and
> register namings.  Eg, you'll find Versatile has a PL110 fitted but has
> the PL111 register layout with IENB and CNTL swapped.

I understand.

> I don't remember the ancestry of the names, but DDI0161D only gives the
> registers long names.  They probably came from an implementation provided
> by ARM in years gone by, which would've been created by those doing
> platform support back in the early days.
> 
> It's only with DDI0161E that they've been given short names.

Ok. These defines are not used in the kernel up to now. Why not change
to the names ARM suggests in their latest technical reference manual?

> I don't think we should change the names just because someone decided to
> add such things to the latest revision of documentation.

I could agree with regard to IENB/IMSC.

But what about STAT==RIS, INTR==MIS and ICR? These are the same for both
PL110 and PL111.

As I said, I'm working on adding VSYNC support and this involves
fiddling with these registers. At the moment, there are no users and
renaming would be easily possible. Then I could use this for both
platforms.

	/* clear pending irq */
	writel(INTR_VCOMP, fb->regs + CLCD_ICR);

If we keep the naming for STAT==RIS, INTR==MIS and ICR, which define
should I use?

And there is no define for ICR for the PL110, which has to be added
anyway. My target hardware is PL110-based and definately has the ICR
register at offset 0x28, which shows that

#define CLCD_PL110_UCUR		0x00000028

is wrong, at least for my hardware. For other hardware that does not
matter, we don't have to enable VSYNC support for any PL11x platform by
default.

Best regards
Michael.