diff mbox

[U-Boot] DA8xx: fix LPSC numbering

Message ID ebf6efc6604b9b61981585d0f348fbe40429f6a6.1300720635.git.lwithers@guralp.com
State Changes Requested
Headers show

Commit Message

Laurence Withers March 21, 2011, 3:25 p.m. UTC 
The DA8xx chips have two modules PSC0 and PSC1 for the local power and sleep
controllers (LPSC). Each LPSC has up to 32 submodules over which it has control,
which are enumerated by the DAVINCI_LPSC_* symbols.

This commit fixes the definitions of a number of symbols to be consistent with
both the OMAP-L137 and OMAP-L138 data sheets (TI documents SPRS563D and SPRS586A
respectively): some minor renaming to reflect actual functionality and some
reordering of modules in PSC1 to be correct.

None of the affected symbols were actually used anywhere in the code, so there
are no related code changes.
---
 arch/arm/include/asm/arch-davinci/hardware.h |   29 +++++++++++++++++---------
 1 files changed, 19 insertions(+), 10 deletions(-)

Comments

Wolfgang Denk March 21, 2011, 6:43 p.m. UTC | #1 
Dear Laurence Withers,

In message <ebf6efc6604b9b61981585d0f348fbe40429f6a6.1300720635.git.lwithers@guralp.com> you wrote:
> The DA8xx chips have two modules PSC0 and PSC1 for the local power and sleep
> controllers (LPSC). Each LPSC has up to 32 submodules over which it has control,
> which are enumerated by the DAVINCI_LPSC_* symbols.
> 
> This commit fixes the definitions of a number of symbols to be consistent with
> both the OMAP-L137 and OMAP-L138 data sheets (TI documents SPRS563D and SPRS586A
> respectively): some minor renaming to reflect actual functionality and some
> reordering of modules in PSC1 to be correct.
> 
> None of the affected symbols were actually used anywhere in the code, so there
> are no related code changes.
> ---
>  arch/arm/include/asm/arch-davinci/hardware.h |   29 +++++++++++++++++---------
>  1 files changed, 19 insertions(+), 10 deletions(-)

SoB missing.

Lines in commit message too long.

Please fix also all other places where these identifiers are used in
the code.

Best regards,

Wolfgang Denk
Laurence Withers March 21, 2011, 8:38 p.m. UTC | #2 
On Mon, Mar 21, 2011 at 07:43:51PM +0100, Wolfgang Denk wrote:
> SoB missing.
> 
> Lines in commit message too long.

Thanks; I have fixed and will resend.

> Please fix also all other places where these identifiers are used in
> the code.

The identifiers changed in the patch were not used anywhere in the code. I
also verified that a "./MAKEALL -s davinci" gave the same results before
and after.

Many thanks for the feedback, and bye for now,
Wolfgang Denk March 21, 2011, 9:14 p.m. UTC | #3 
Dear Laurence Withers,

In message <20110321203839.GA15999@lwithers.me.uk> you wrote:
>
> > Please fix also all other places where these identifiers are used in
> > the code.
> 
> The identifiers changed in the patch were not used anywhere in the code. I
> also verified that a "./MAKEALL -s davinci" gave the same results before
> and after.

I cannot confirm this; for example, you change:

-	DAVINCI_LPSC_TPCC = 0,
+	DAVINCI_LPSC_TPCC0 = 0,

I see:

-> grep -R DAVINCI_LPSC_TPCC *
arch/arm/include/asm/arch-davinci/hardware.h:#define DAVINCI_LPSC_TPCC          2
arch/arm/include/asm/arch-davinci/hardware.h:   DAVINCI_LPSC_TPCC = 0,
board/davinci/dm355evm/dm355evm.c:      lpsc_on(DAVINCI_LPSC_TPCC);

you change:

-	DAVINCI_LPSC_MMC_SD,
+	DAVINCI_LPSC_MMC_SD0,

I see:

-> grep -R DAVINCI_LPSC_MMC_SD *
arch/arm/cpu/arm926ejs/davinci/psc.c:   case DAVINCI_LPSC_MMC_SD:
arch/arm/include/asm/arch-davinci/hardware.h:#define DAVINCI_LPSC_MMC_SD                15
arch/arm/include/asm/arch-davinci/hardware.h:   DAVINCI_LPSC_MMC_SD,


Seems your change would break a few boards...

Best regards,

Wolfgang Denk
Laurence Withers March 21, 2011, 9:25 p.m. UTC | #4 
On Mon, Mar 21, 2011 at 10:14:18PM +0100, Wolfgang Denk wrote:
> -> grep -R DAVINCI_LPSC_TPCC *
> arch/arm/include/asm/arch-davinci/hardware.h:#define DAVINCI_LPSC_TPCC          2
> arch/arm/include/asm/arch-davinci/hardware.h:   DAVINCI_LPSC_TPCC = 0,
> board/davinci/dm355evm/dm355evm.c:      lpsc_on(DAVINCI_LPSC_TPCC);
> 
> you change:
> 
> -	DAVINCI_LPSC_MMC_SD,
> +	DAVINCI_LPSC_MMC_SD0,
> 
> I see:
> 
> -> grep -R DAVINCI_LPSC_MMC_SD *
> arch/arm/cpu/arm926ejs/davinci/psc.c:   case DAVINCI_LPSC_MMC_SD:
> arch/arm/include/asm/arch-davinci/hardware.h:#define DAVINCI_LPSC_MMC_SD                15
> arch/arm/include/asm/arch-davinci/hardware.h:   DAVINCI_LPSC_MMC_SD,
> 
> 
> Seems your change would break a few boards...

There are several chips in the Davinci family; my commit changes the LPSC
definitions for the DA8xx (the DA830/DA850) processors only, leaving the
other processors alone (they are quite different).

Looking at the hardware.h file that I changed, just above the enum { } block
where my changes reside are a set of #defines for the same symbols. The
#defines are used when CONFIG_SOC_DA8XX is not defined; the enum is used when
it is.

The existing uses in the code are special cases for the non-DA8xx processors;
DAVINCI_LPSC_TPCC is used in the DM355 EVM kit board code, and while
DAVINCI_LPSC_MMC_SD is used in the processor generic C code it is wrapped in
a #ifdef CONFIG_SOC_DM644X (another chip) with a comment about special
treatment.

So, I can change the non-DA8xx names to match. But that changes existing,
working code and the names would no longer match the datasheets.

Or, I can leave the old names be, and then have the new names with suffices, e.g.:

	DAVINCI_LPSC_MMC_SD,	/* actually MMC_SD0 on DA8xx */
	DAVINCI_LPSC_MMC_SD1,

It seems a little inconsistent to me, but I'm happy to do it if preferred.
Or, I can do what I have done, and change only the DA8xx names.

Which is the preferred option? Or something else entirely?

Many thanks, and bye for now,
Wolfgang Denk March 21, 2011, 9:44 p.m. UTC | #5 
Dear Laurence Withers,

In message <20110321212544.GB15999@lwithers.me.uk> you wrote:
>
> Looking at the hardware.h file that I changed, just above the enum { } block
> where my changes reside are a set of #defines for the same symbols. The
> #defines are used when CONFIG_SOC_DA8XX is not defined; the enum is used when
> it is.

Argh.   Can we please clean up such a mess, then?

> Which is the preferred option? Or something else entirely?

We should agree on a common way to implement this - either #define
_or_ enum, but not a mix of both.

Having a closer look, it turns out that all these "indices" are
actually register names, and functions like lpsc_on() use horrible
code like this:

	mdstat = REG_P(PSC_MDSTAT_BASE + (id * 4));

to perform address magic, even worse, perform device I/O without using
any I/O acessors (as in dsp_on()).


I am aware that this is unrelated to your patch, but this needs to be
cleaned up ASAP!



As for your patch - maybe we can at least unify the #define versus
enum mixture?

Best regards,

Wolfgang Denk
Laurence Withers March 21, 2011, 10:28 p.m. UTC | #6 
On Mon, Mar 21, 2011 at 10:44:11PM +0100, Wolfgang Denk wrote:
> We should agree on a common way to implement this - either #define
> _or_ enum, but not a mix of both.
> 
> Having a closer look, it turns out that all these "indices" are
> actually register names, and functions like lpsc_on() use horrible
> code like this:
> 
> 	mdstat = REG_P(PSC_MDSTAT_BASE + (id * 4));
> 
> to perform address magic, even worse, perform device I/O without using
> any I/O acessors (as in dsp_on()).

So, as a rough way forward, does the following sound sane?

 - drop REG_P() and REG() macros, use readl()/writel()/etc. instead.
 - drop address magic, use structures/arrays instead.
 - choose either enum or #define (I'd say enum as it's mostly a set of
   incrementing integers, but it depends on what the result looks like).
 - try to unify the various Davinci LPSC modules as much as possible.

I'm willing to give this a go, but I am unable to physically test the results
on any of the Davinci family except the DA850.

Bye for now,
diff mbox

Patch

diff --git a/arch/arm/include/asm/arch-davinci/hardware.h b/arch/arm/include/asm/arch-davinci/hardware.h
index df3f549..d0a3036 100644
--- a/arch/arm/include/asm/arch-davinci/hardware.h
+++ b/arch/arm/include/asm/arch-davinci/hardware.h
@@ -214,12 +214,12 @@  typedef volatile unsigned int *	dv_reg_p;
 #else /* CONFIG_SOC_DA8XX */
 
 enum davinci_lpsc_ids {
-	DAVINCI_LPSC_TPCC = 0,
+	DAVINCI_LPSC_TPCC0 = 0,
 	DAVINCI_LPSC_TPTC0,
 	DAVINCI_LPSC_TPTC1,
 	DAVINCI_LPSC_AEMIF,
 	DAVINCI_LPSC_SPI0,
-	DAVINCI_LPSC_MMC_SD,
+	DAVINCI_LPSC_MMC_SD0,
 	DAVINCI_LPSC_AINTC,
 	DAVINCI_LPSC_ARM_RAM_ROM,
 	DAVINCI_LPSC_SECCTL_KEYMGR,
@@ -227,31 +227,40 @@  enum davinci_lpsc_ids {
 	DAVINCI_LPSC_SCR0,
 	DAVINCI_LPSC_SCR1,
 	DAVINCI_LPSC_SCR2,
-	DAVINCI_LPSC_DMAX,
+	DAVINCI_LPSC_PRUSS,
 	DAVINCI_LPSC_ARM,
 	DAVINCI_LPSC_GEM,
 	/* for LPSCs in PSC1, offset from 32 for differentiation */
 	DAVINCI_LPSC_PSC1_BASE = 32,
-	DAVINCI_LPSC_USB11,
+	DAVINCI_LPSC_TPCC1 = 32,
 	DAVINCI_LPSC_USB20,
+	DAVINCI_LPSC_USB11,
 	DAVINCI_LPSC_GPIO,
 	DAVINCI_LPSC_UHPI,
 	DAVINCI_LPSC_EMAC,
 	DAVINCI_LPSC_DDR_EMIF,
 	DAVINCI_LPSC_McASP0,
-	DAVINCI_LPSC_McASP1,
-	DAVINCI_LPSC_McASP2,
+	DAVINCI_LPSC_McASP1_SATA,
+	DAVINCI_LPSC_McASP2_VPIF,
 	DAVINCI_LPSC_SPI1,
 	DAVINCI_LPSC_I2C1,
 	DAVINCI_LPSC_UART1,
 	DAVINCI_LPSC_UART2,
+	DAVINCI_LPSC_McBSP0,
+	DAVINCI_LPSC_McBSP1,
 	DAVINCI_LPSC_LCDC,
 	DAVINCI_LPSC_ePWM,
+	DAVINCI_LPSC_MMC_SD1,
+	DAVINCI_LPSC_uPP,
 	DAVINCI_LPSC_eCAP,
-	DAVINCI_LPSC_eQEP,
-	DAVINCI_LPSC_SCR_P0,
-	DAVINCI_LPSC_SCR_P1,
-	DAVINCI_LPSC_CR_P3,
+	DAVINCI_LPSC_eQEP_TPTC2,
+	DAVINCI_LPSC_SCR_F0 = 56,
+	DAVINCI_LPSC_SCR_F1,
+	DAVINCI_LPSC_SCR_F2,
+	DAVINCI_LPSC_SCR_F6,
+	DAVINCI_LPSC_SCR_F7,
+	DAVINCI_LPSC_SCR_F8,
+	DAVINCI_LPSC_BR_F7,
 	DAVINCI_LPSC_L3_CBA_RAM
 };