Re: [PATCH A 09/10] OMAP2/3: Remove OMAP_PRM_REGADDR, OMAP_CM_REGADDR

[Kevin Hilman <khilman@deeprootsystems.com>]

Paul Walmsley <paul@pwsan.com> writes:

> Hi Russell,
>
> On Wed, 28 Jan 2009, Russell King - ARM Linux wrote:
>
>> On Tue, Jan 27, 2009 at 07:13:16PM -0700, Paul Walmsley wrote:
>> > diff --git a/arch/arm/mach-omap2/clock.c b/arch/arm/mach-omap2/clock.c
>> > index c4b80a4..55c5d67 100644
>> > --- a/arch/arm/mach-omap2/clock.c
>> > +++ b/arch/arm/mach-omap2/clock.c
>> > @@ -27,6 +27,7 @@
>> >  #include <mach/clock.h>
>> >  #include <mach/clockdomain.h>
>> >  #include <mach/cpu.h>
>> > +#include <mach/prcm.h>
>> >  #include <asm/div64.h>
>> >  
>> >  #include "memory.h"
>> > @@ -247,9 +248,9 @@ static void omap2_clk_wait_ready(struct clk *clk)
>> >  	/* REVISIT: What are the appropriate exclusions for 34XX? */
>> >  	/* OMAP3: ignore DSS-mod clocks */
>> >  	if (cpu_is_omap34xx() &&
>> > -	    (((u32)reg & ~0xff) == (u32)OMAP_CM_REGADDR(OMAP3430_DSS_MOD, 0) ||
>> > -	     ((((u32)reg & ~0xff) == (u32)OMAP_CM_REGADDR(CORE_MOD, 0)) &&
>> > -	     clk->enable_bit == OMAP3430_EN_SSI_SHIFT)))
>> > +	    (((u32)reg & ~0xff) == cm_read_mod_reg(OMAP3430_DSS_MOD, 0) ||
>> > +	     ((((u32)reg & ~0xff) == cm_read_mod_reg(CORE_MOD, 0)) &&
>> > +	      clk->enable_bit == OMAP3430_EN_SSI_SHIFT)))
>> >  		return;
>> 
>> As suggested in patch 2, I hate this approach.  It's far better to deal
>> with this by changing the way we handle enabling and disabling clocks,
>> hence my
>> 
>> "[ARM] omap: eliminate unnecessary conditionals in omap2_clk_wait_ready"
>
> This function is radically cleaned up in a following patch, patch D 10.  I 
> regret that I wasn't able to compress the above patch with D 10, but the 
> change delta between the two patches was quite large.
>
>> > @@ -476,6 +466,37 @@ static int __init omap2_clk_arch_init(void)
>> >  }
>> >  arch_initcall(omap2_clk_arch_init);
>> >  
>> > +static u32 prm_base;
>> > +static u32 cm_base;
>> > +
>> > +/*
>> > + * Since we share clock data for 242x and 243x, we need to rewrite some
>> > + * some register base offsets. Assume offset is at prm_base if flagged,
>> > + * else assume it's cm_base.
>> > + */
>> > +static inline void omap2_clk_check_reg(u32 flags, void __iomem **reg)
>> > +{
>> > +	u32 tmp = (__force u32)*reg;
>> > +
>> > +	if ((tmp >> 24) != 0)
>> > +		return;
>> > +
>> > +	if (flags & OFFSET_GR_MOD)
>> > +		tmp += prm_base;
>> > +	else
>> > +		tmp += cm_base;
>> > +
>> > +	*reg = (__force void __iomem *)tmp;
>> > +}
>> > +
>> > +void __init omap2_clk_rewrite_base(struct clk *clk)
>> > +{
>> > +	omap2_clk_check_reg(clk->flags, &clk->clksel_reg);
>> > +	omap2_clk_check_reg(clk->flags, &clk->enable_reg);
>> > +	if (clk->dpll_data)
>> > +		omap2_clk_check_reg(0, &clk->dpll_data->mult_div1_reg);
>> > +}
>> > +
>> >  int __init omap2_clk_init(void)
>> >  {
>> >  	struct prcm_config *prcm;
>> > @@ -487,6 +508,12 @@ int __init omap2_clk_init(void)
>> >  	else if (cpu_is_omap2430())
>> >  		cpu_mask = RATE_IN_243X;
>> >  
>> > +	for (clkp = onchip_24xx_clks;
>> > +	     clkp < onchip_24xx_clks + ARRAY_SIZE(onchip_24xx_clks);
>> > +	     clkp++) {
>> > +			omap2_clk_rewrite_base(*clkp);
>> > +	}
>> > +
>> 
>> I'm afraid this also fails to satisfy my decency filter.  Let's summarise
>> what's going on here.
>> 
>> - the structure initializers are setup to cast integer register offsets
>>   to void __iomem *.
>> - the code above walks all clock structures, looking for what could be
>>   considered an offset (iow, bits 31-24 of the pointer being zero).
>> - it looks at the OFFSET_GR_MOD flag to determine which base to add in
>> - the base address is not 'void __iomem *' but 'u32'.
>> - we update the void __iomem * pointers in the structure.
>> 
>> So, we have a base address which is an integer, and an offset which is
>> an void __iomem *.
>> 
>> I can see why you want to do this, but I think it needs more thought.
>> I'll sit on this patch for a while.
>
> I agree with your decency filter.  I believe that patch was written for 
> expediency.  It turns out that all of the register rewriting code is 
> ultimately unnecessary, and is wiped out in patch D 08.
>

What about the rest of the *_PRM_REGADDR() accessor macro changes in
this patch?

The rest of the PM core code uses these and so does not build on top
of the Russell's omap-clks3 branch.

Kevin