[PATCHv5 11/35] ARM: OMAP2+: clock: move clock provider infrastructure to clock driver

[Tero Kristo]

On 03/26/2015 07:30 PM, Tony Lindgren wrote:
> * Tero Kristo <t-kristo at ti.com> [150326 03:55]:
>> On 03/26/2015 09:24 AM, Tero Kristo wrote:
>>> On 03/26/2015 01:17 AM, Tony Lindgren wrote:
>>>> * Tero Kristo <t-kristo at ti.com> [150325 08:12]:
>>>>>
>>>>> Splits the clock provider init out of the PRM driver and moves it to
>>>>> clock driver. This is needed so that once the PRCM drivers are
>>>>> separated,
>>>>> they can logically just access the clock driver not needing to go
>>>>> through
>>>>> common PRM code. This would be wrong in the case of control module for
>>>>> example.
>>>> ...
>>>>
>>>>> --- a/arch/arm/mach-omap2/clock.c
>>>>> +++ b/arch/arm/mach-omap2/clock.c
>>>> ...
>>>>> -u32 omap2_clk_readl(struct clk_hw_omap *clk, void __iomem *reg)
>>>>> +u32 omap2_clk_memmap_readl(void __iomem *reg)
>>>>>   {
>>>>> -    u32 val;
>>>>> +    struct clk_omap_reg *r = (struct clk_omap_reg *)®
>>>>>
>>>>> -    if (clk->flags & MEMMAP_ADDRESSING) {
>>>>> -        struct clk_omap_reg *r = (struct clk_omap_reg *)®
>>>>> -        val = readl_relaxed(clk_memmaps[r->index] + r->offset);
>>>>> -    } else {
>>>>> -        val = readl_relaxed(reg);
>>>>> -    }
>>>>> +    return readl_relaxed(clk_memmaps[r->index] + r->offset);
>>>>> +}
>>>>
>>>> The cast from void __iomem *reg to struct clk_omap_reg *r looks still
>>>> nasty.. Why don't you add the IO address into struct clk_omap_reg:
>>>>
>>>> struct clk_omap_reg {
>>>>     u16 offset;
>>>>     u16 index;
>>>>     struct regmap *regmap;
>>>>     void __iomem *addr;
>>>> };
>>>> ...
>>>>
>>>> Then populate it during init and then have the clock code use it
>>>> directly if available? Then it seems you would not need the
>>>> static struct clk_iomap *clk_memmaps[CLK_MAX_MEMMAPS] at all?
>>>
>>> Doing a change like this should probably be planned, but it is a larger
>>> modification. Currently none of the low-level clock APIs support this,
>>> but instead expect a direct iomem pointer against which they can do
>>> arithmetic operations. The major problem is the companion clocks, which
>>> just XOR some bits in the registers to get ICLK / IDLEST register offset
>> >from FCLK.
>>>
>>> So, for now, clock code just uses the void __iomem pointer as a storage
>>> class for struct clk_omap_reg, on which arithmetic operations can be done.
>
> Well how about keep the check if (clk->flags & MEMMAP_ADDRESSING) at
> least? Maybe WARN_ON(!(clk->flags & MEMMAP_ADDRESSING))?
>
> Otherwise this could be a nightmare to debug if anything goes wrong.

Ok, force update pushed to branch now, the patch is basically pretty 
much to the format where it was before this discussion, with the 
addition of WARN_ON_ONCE in case MEMMAP_ADDRESSING is missing.

-Tero

>
>> I did this change as a trial, and this is the diff required to get it
>> working:
>>
>>   arch/arm/mach-omap2/clkt_iclk.c |   20 ++++++++---------
>>   arch/arm/mach-omap2/clock.c     |   47
>> +++++++++++++++++----------------------
>>   arch/arm/mach-omap2/clock.h     |    8 +++----
>>   arch/arm/mach-omap2/clock2430.c |    5 +++--
>>   arch/arm/mach-omap2/clock34xx.c |   36 ++++++++++++++----------------
>>   arch/arm/mach-omap2/clock3517.c |   20 ++++++++---------
>>   arch/arm/mach-omap2/cm.h        |    4 +++-
>>   arch/arm/mach-omap2/cm2xxx.c    |    9 +++-----
>>   arch/arm/mach-omap2/cm3xxx.c    |   10 +++------
>>   drivers/clk/ti/clk.c            |   10 +++++----
>>   drivers/clk/ti/divider.c        |   24 ++++++++++++++------
>>   drivers/clk/ti/dpll.c           |   11 ++++-----
>>   drivers/clk/ti/gate.c           |   21 +++++++++++------
>>   drivers/clk/ti/interface.c      |    9 ++++----
>>   drivers/clk/ti/mux.c            |   22 ++++++++++++------
>>   include/linux/clk/ti.h          |    5 +++--
>>   16 files changed, 137 insertions(+), 124 deletions(-)
>>
>> I think we should probably keep this out of this set now and do this while
>> moving the OMAP core clock support code under clock driver... just to keep
>> it more easily manageable.
>
> OK fine with me to do that as a follow-up patch.
>
> Regards,
>
> Tony
>