From patchwork Fri May 10 21:49:01 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andy Fleming X-Patchwork-Id: 243072 X-Patchwork-Delegate: afleming@freescale.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id 8BCF72C00AB for ; Sat, 11 May 2013 07:49:26 +1000 (EST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 1B5094A154; Fri, 10 May 2013 23:49:23 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id MEL9YxioXsCJ; Fri, 10 May 2013 23:49:22 +0200 (CEST) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 5512E4A14C; Fri, 10 May 2013 23:49:20 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id E64314A14C for ; Fri, 10 May 2013 23:49:14 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id OUnut-PIjQmU for ; Fri, 10 May 2013 23:49:10 +0200 (CEST) X-Greylist: delayed 5575 seconds by postgrey-1.27 at theia; Fri, 10 May 2013 23:49:03 CEST X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from mail-wi0-f174.google.com (mail-wi0-f174.google.com [209.85.212.174]) by theia.denx.de (Postfix) with ESMTPS id 4AD9B4A149 for ; Fri, 10 May 2013 23:49:03 +0200 (CEST) Received: by mail-wi0-f174.google.com with SMTP id m6so1076199wiv.7 for ; Fri, 10 May 2013 14:49:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type; bh=kO9hL3mIDeTvLYbD6vWgU0687ndu+gP1rGrV770LFHQ=; b=t6PZ0DLLz96++0f3zzc4NfaADM+hnMD/jnnAbRP2PhXnUfNdtDCx5raDRgNWtRpHpq 2MmYiQs68gQ2inusvl3pKOJD5UhW2dcuuBUoI3jM2Qj1SvrKkgv8nFm7htdRCteLD979 vQH/8CPOXCZRgZjLCs3hclQpq2xnnyJi2jmSLMS6/cjmQ8gDY1O7BIDsT/CdsdXE+QVd SCS1VLnUk36mHNzGiXh434OLxAPfvp2zUDCwkvTgvNBMJedsinN1kkoFB5oFRucC8tvG 5ofsJB18TKkvGRnE6Ut7DNTvY9JXssWEJiBW6i726TU52uBEl8XvTq6pkUI0D66NPj79 iubw== MIME-Version: 1.0 X-Received: by 10.180.206.172 with SMTP id lp12mr6421147wic.11.1368222541965; Fri, 10 May 2013 14:49:01 -0700 (PDT) Received: by 10.194.8.194 with HTTP; Fri, 10 May 2013 14:49:01 -0700 (PDT) In-Reply-To: <1363973052-25918-1-git-send-email-yorksun@freescale.com> References: <1363973052-25918-1-git-send-email-yorksun@freescale.com> Date: Fri, 10 May 2013 16:49:01 -0500 Message-ID: From: Andy Fleming To: York Sun , Mingkai Hu X-Content-Filtered-By: Mailman/MimeDel 2.1.11 Cc: U-Boot list , Andy Fleming Subject: Re: [U-Boot] [PATCH 03/21] fsl_ifc: add support for different IFC bank count X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.11 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: u-boot-bounces@lists.denx.de Errors-To: u-boot-bounces@lists.denx.de On Fri, Mar 22, 2013 at 12:23 PM, York Sun wrote: > From: Mingkai Hu > > Calculate reserved fields according to IFC bank count > > 1. Move csor_ext register behind csor register and fix res offset > 2. move ifc bank count to config_mpc85xx.h to support 8 bank count > > There's no IFC controller instead of eLBC controller on some platforms, > such as MPC8536, P2041, P3041, P4080 etc, so there's no macro definition > for the number of IFC chip select(CONFIG_SYS_FSL_IFC_BANK_COUNT) which > is used in the IFC controller header file fsl_ifc.h on these platforms. > This paragraph is pretty confusing. Is this just explaining that IFC_BANK_COUNT isn't being defined for devices that don't use IFC? Or is it explaining why you have to guard fsl_ifc.h with a #ifdef? > > Signed-off-by: Mingkai Hu > --- > arch/powerpc/cpu/mpc85xx/cpu.c | 2 +- > arch/powerpc/cpu/mpc8xxx/fsl_ifc.c | 58 > ++++++++++++++++++++++++++++- > arch/powerpc/include/asm/config_mpc85xx.h | 7 ++++ > arch/powerpc/include/asm/fsl_ifc.h | 42 +++++++++++++++------ > 4 files changed, 96 insertions(+), 13 deletions(-) > > diff --git a/arch/powerpc/cpu/mpc85xx/cpu.c > b/arch/powerpc/cpu/mpc85xx/cpu.c > index df2ab6d..379a7df 100644 > --- a/arch/powerpc/cpu/mpc85xx/cpu.c > +++ b/arch/powerpc/cpu/mpc85xx/cpu.c > @@ -34,8 +34,8 @@ > #include > #include > #include > -#include > #include > +#include > #include > #include > #include > diff --git a/arch/powerpc/cpu/mpc8xxx/fsl_ifc.c > b/arch/powerpc/cpu/mpc8xxx/fsl_ifc.c > index 56b319f..f0da355 100644 > --- a/arch/powerpc/cpu/mpc8xxx/fsl_ifc.c > +++ b/arch/powerpc/cpu/mpc8xxx/fsl_ifc.c > @@ -26,7 +26,7 @@ void print_ifc_regs(void) > int i, j; > > printf("IFC Controller Registers\n"); > - for (i = 0; i < FSL_IFC_BANK_COUNT; i++) { > + for (i = 0; i < CONFIG_SYS_FSL_IFC_BANK_COUNT; i++) { > printf("CSPR%d:0x%08X\tAMASK%d:0x%08X\tCSOR%d:0x%08X\n", > i, get_ifc_cspr(i), i, get_ifc_amask(i), > i, get_ifc_csor(i)); > @@ -94,4 +94,60 @@ void init_early_memctl_regs(void) > set_ifc_amask(IFC_CS3, CONFIG_SYS_AMASK3); > set_ifc_csor(IFC_CS3, CONFIG_SYS_CSOR3); > #endif > + > +#ifdef CONFIG_SYS_CSPR4_EXT > + set_ifc_cspr_ext(IFC_CS4, CONFIG_SYS_CSPR4_EXT); > +#endif > +#if defined(CONFIG_SYS_CSPR4) && defined(CONFIG_SYS_CSOR4) > There seem to be a large number of CONFIG options associated with each and every new bank. It's following the pattern of the existing code, so I'll accept it, but I can't help but think that some of these config options are entirely redundant (would we ever have CSPR4, and *not* CSOR4?). This is, admittedly, based only on a high-level view of the code -- I'm not familiar with the specifics of the IFC design. [...] [...] @@ -907,6 +910,22 @@ struct fsl_ifc_gpcm { u32 res4[0x1F3]; }; +#ifdef CONFIG_SYS_FSL_IFC_BANK_COUNT +#if (CONFIG_SYS_FSL_IFC_BANK_COUNT <= 8) +#define CONFIG_SYS_FSL_IFC_CSPR_RES \ + (0x25 - CONFIG_SYS_FSL_IFC_BANK_COUNT * 3) +#define CONFIG_SYS_FSL_IFC_AMASK_RES \ + (0x24 - CONFIG_SYS_FSL_IFC_BANK_COUNT * 3) +#define CONFIG_SYS_FSL_IFC_CSOR_RES \ + (0x24 - CONFIG_SYS_FSL_IFC_BANK_COUNT * 3) +#define CONFIG_SYS_FSL_IFC_FTIM_RES \ + (0x90 - CONFIG_SYS_FSL_IFC_BANK_COUNT * 0xc) These aren't really config values. They are values derived from a CONFIG value, and they only have local scope (so there's no need for the very global scoping of the names). Also... Are these correct? 0x25 is a strange amount of gap in register space. All that aside, I think we should just define the register offsets to cover all existing (or even predicted) registers, and make the gap hard-coded as before. There's no real advantage to specifying only as many as are implemented, and this method seems ripe for potential bugs in the future. +#else +#error IFC BANK count not vaild +#endif +#else +#error IFC BANK count not defined +#endif /* * IFC Controller Registers @@ -918,24 +937,24 @@ struct fsl_ifc { u32 cspr_ext; u32 cspr; u32 res2; - } cspr_cs[FSL_IFC_BANK_COUNT]; - u32 res3[0x19]; + } cspr_cs[CONFIG_SYS_FSL_IFC_BANK_COUNT]; + u32 res3[CONFIG_SYS_FSL_IFC_CSPR_RES]; struct { u32 amask; u32 res4[0x2]; - } amask_cs[FSL_IFC_BANK_COUNT]; - u32 res5[0x17]; + } amask_cs[CONFIG_SYS_FSL_IFC_BANK_COUNT]; + u32 res5[CONFIG_SYS_FSL_IFC_AMASK_RES]; struct { - u32 csor_ext; u32 csor; + u32 csor_ext; u32 res6; - } csor_cs[FSL_IFC_BANK_COUNT]; - u32 res7[0x19]; + } csor_cs[CONFIG_SYS_FSL_IFC_BANK_COUNT]; + u32 res7[CONFIG_SYS_FSL_IFC_CSOR_RES]; struct { u32 ftim[4]; u32 res8[0x8]; - } ftim_cs[FSL_IFC_BANK_COUNT]; - u32 res9[0x60]; + } ftim_cs[CONFIG_SYS_FSL_IFC_BANK_COUNT]; + u32 res9[CONFIG_SYS_FSL_IFC_FTIM_RES]; u32 rb_stat; u32 res10[0x2]; u32 ifc_gcr; @@ -961,6 +980,7 @@ struct fsl_ifc { #undef CSPR_MSEL_NOR #define CSPR_MSEL_NOR CSPR_MSEL_GPCM #endif +#endif /* CONFIG_FSL_IFC */ #endif /* __ASSEMBLY__ */ #endif /* __ASM_PPC_FSL_IFC_H */ --- 1.7.9.5 diff --git a/arch/powerpc/include/asm/fsl_ifc.h b/arch/powerpc/include/asm/fsl_ifc.h index ba41b73..debcb6b 100644 --- a/arch/powerpc/include/asm/fsl_ifc.h +++ b/arch/powerpc/include/asm/fsl_ifc.h @@ -21,6 +21,7 @@ #ifndef __ASM_PPC_FSL_IFC_H #define __ASM_PPC_FSL_IFC_H +#ifdef CONFIG_FSL_IFC #include #include