From mboxrd@z Thu Jan  1 00:00:00 1970
From: Eric Nelson <eric.nelson@boundarydevices.com>
Date: Tue, 06 Mar 2012 07:34:29 -0700
Subject: [U-Boot] [PATCH] i.MX6: imx_ccm is a constant that points to a
 register set
In-Reply-To: <4F5601B2.9020202@denx.de>
References: <1330965246-21031-1-git-send-email-eric.nelson@boundarydevices.com>
	<4F5601B2.9020202@denx.de>
Message-ID: <4F562075.6070907@boundarydevices.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 03/06/2012 05:23 AM, Stefano Babic wrote:
> On 05/03/2012 17:34, Eric Nelson wrote:
>
> Hi Eric,
>
>> If we're going to use globals to point at register banks, they
>> should be constant and visible.
>> ---
>
> 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.

>>   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.

Let me know your thoughts about how to structure this stuff.

Regards,


Eric