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