From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Nelson Date: Wed, 07 Mar 2012 08:54:38 -0700 Subject: [U-Boot] [PATCH] i.MX6: imx_ccm is a constant that points to a register set In-Reply-To: <4F573BB6.70202@denx.de> References: <1330965246-21031-1-git-send-email-eric.nelson@boundarydevices.com> <4F5601B2.9020202@denx.de> <4F562075.6070907@boundarydevices.com> <4F573BB6.70202@denx.de> Message-ID: <4F5784BE.5090909@boundarydevices.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 03/07/2012 03:43 AM, Stefano Babic wrote: > On 06/03/2012 15:34, Eric Nelson wrote: > > Hi Eric, > >>> ok, but why are we going to use global pointers ? We have global >>> constants (in imx-regs.h) and each part of code sets its local copy >>> without using global pointers, that require also some rules and naming >>> conventions to avoid conflicts. >>> >> >> I'm just trying to follow convention here. My preference would be >> to make these constants visible to the compiler in some way. > > At the moment we have not an abstraction layer, but all constants > related to physycal addresses are visible because defined in imx-regs.h > >> >>>> arch/arm/cpu/armv7/mx6/clock.c | 2 +- >>>> arch/arm/include/asm/arch-mx6/imx-regs.h | 2 ++ >>>> 2 files changed, 3 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/arch/arm/cpu/armv7/mx6/clock.c >>>> b/arch/arm/cpu/armv7/mx6/clock.c >>>> index ef98563..eb1f09b 100644 >>>> --- a/arch/arm/cpu/armv7/mx6/clock.c >>>> +++ b/arch/arm/cpu/armv7/mx6/clock.c >>>> @@ -34,7 +34,7 @@ enum pll_clocks { >>>> PLL_ENET, /* ENET PLL */ >>>> }; >>>> >>>> -struct imx_ccm_reg *imx_ccm = (struct imx_ccm_reg *)CCM_BASE_ADDR; >>>> +struct imx_ccm_reg *const imx_ccm = (struct imx_ccm_reg >>>> *)CCM_BASE_ADDR; >>> >>> As far as I see, this is used only in clock.c - and it must be static. >>> >> >> I ran into this when trying to fix setup_sata() in mx6qsabrelite.c as >> shown in >> this patch. >> >> The same sort of question (how to define the register sets) will occur for >> other register sets. >> >> For the iomux, I used a #define, but that seems wrong too >> (not least because it isn't UPPERCASE). >> >> #define iomuxc ((struct iomuxc_base_regs *)IOMUXC_BASE_ADDR) >> >> The other way around this is to hide all register accesses behind the >> drivers. IOW, add routines for each of the accesses and hide the >> details in clock.c >> >> void mx6_enable_sata_clock(); >> ... >> >> void mx6_enable_enet_pll(); >> >> but there will almost certainly be board-specific tweaks and I'm not >> sure the extra level of abstraction helps. > > Yes - this makes such abstraction difficult. However, we have not such > kind of abstraction at the moment, and all boards define local pointers > using the global constants. This is the current status for the CCM > (clock module) block for all i.MX. > > If you see for MX3 / MX5 boards, you see that board files are > implementing something like: > > struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)MXC_CCM_BASE; > > and then they are using the mxc_ccm_reg structure. If we want to > abstract this, we should get rid of special peripheral function (a mx6_ > function) and write a function callable from other Socs. I do not think > it is a good idea to have SOC-related functions (mx5_, mx6_, mx35_...), > even if I know that specially for mx31 (the first i.MX SOC merged into > U-boot) some cleanup is still required. If we can factorize, then we > need a general mxc_enable_sata_clock(), whose implementation could be > SOC specific. > > Take a look if it is possible to factorize - however, at the moment, I > think it is not bad if you define a pointer in your setup_sata() > function and then you enable directly the clock. > Will do. > Best regards, > Stefano Babic >