public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
From: Eric Nelson <eric.nelson@boundarydevices.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH] i.MX6: imx_ccm is a constant that points to a register set
Date: Tue, 06 Mar 2012 07:34:29 -0700	[thread overview]
Message-ID: <4F562075.6070907@boundarydevices.com> (raw)
In-Reply-To: <4F5601B2.9020202@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

  reply	other threads:[~2012-03-06 14:34 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-03-05 16:34 [U-Boot] [PATCH] i.MX6: imx_ccm is a constant that points to a register set Eric Nelson
2012-03-06 12:23 ` Stefano Babic
2012-03-06 14:34   ` Eric Nelson [this message]
2012-03-07 10:43     ` Stefano Babic
2012-03-07 15:54       ` Eric Nelson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4F562075.6070907@boundarydevices.com \
    --to=eric.nelson@boundarydevices.com \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox