Message ID | ebf6efc6604b9b61981585d0f348fbe40429f6a6.1300720635.git.lwithers@guralp.com |
---|---|
State | Changes Requested |
Headers | show |
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
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,
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
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,
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
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 --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 };