[U-Boot] [PATCH] i.MX6: imx_ccm is a constant that points to a register set

[U-Boot] [PATCH] i.MX6: imx_ccm is a constant that points to a register set

[Eric Nelson]

If we're going to use globals to point at register banks, they
should be constant and visible.
---
 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;
 
 void enable_usboh3_clk(unsigned char enable)
 {
diff --git a/arch/arm/include/asm/arch-mx6/imx-regs.h b/arch/arm/include/asm/arch-mx6/imx-regs.h
index 3e5c4c2..3eae704 100644
--- a/arch/arm/include/asm/arch-mx6/imx-regs.h
+++ b/arch/arm/include/asm/arch-mx6/imx-regs.h
@@ -297,5 +297,7 @@ struct aipstz_regs {
 	u32	opacr4;
 };
 
+extern struct imx_ccm_reg *const imx_ccm ;
+
 #endif /* __ASSEMBLER__*/
 #endif /* __ASM_ARCH_MX6_IMX_REGS_H__ */
-- 
1.7.9

[U-Boot] [PATCH] i.MX6: imx_ccm is a constant that points to a register set

[Stefano Babic]

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.

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

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] i.MX6: imx_ccm is a constant that points to a register set

[Eric Nelson]

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

[U-Boot] [PATCH] i.MX6: imx_ccm is a constant that points to a register set

[Stefano Babic]

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.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] i.MX6: imx_ccm is a constant that points to a register set

[Eric Nelson]

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
>