Re: [PATCH 04/10] ARM: OMAP2: Remove OMAP_PRM_REGADDR, OMAP_CM_REGADDR
Hi Russell, I've been on holiday, hence the delayed E-mail. On Mon, 27 Oct 2008, Russell King - ARM Linux wrote: > If I take this further, and detect clocks which result in > omap2_clk_wait_ready() being called, but we don't have a corresponding > [if]ck, I see at least these four warnings: > > CLK: omapctrl_ick.0: no other_clk but asked to wait_ready > CLK: sdrc_ick.0: no other_clk but asked to wait_ready > CLK: dpll4_m2x2_ck.0: no other_clk but asked to wait_ready > CLK: omap_32ksync_ick.0: no other_clk but asked to wait_ready > > So, presumably these ick clocks don't have corresponding fck clocks, > which means checking the fck register for the bit would be wrong? Yes, that's right. > So, here's the question: should any of the above clocks result in > omap2_wait_clock_ready() being called? If the answer is no, that's > great. If yes, are there missing clk structures for OMAP3? No, omap2_wait_clock_ready() shouldn't be called for those clocks. The current linux-omap git head does not call omap2_wait_clock_ready() in these cases, as far as I know. - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] ARM: OMAP2: Remove OMAP_PRM_REGADDR, OMAP_CM_REGADDR
On Tue, Oct 07, 2008 at 08:12:11AM -0600, Paul Walmsley wrote: > Hello Russell, > > On Tue, 7 Oct 2008, Russell King - ARM Linux wrote: > > > One thing I've noticed, however, is that there seem to be clocks which > > result in omap2_clk_wait_ready() being called, which checks the "other" > > F or I clock register, but there's no corresponding clock defined in > > the source for that bit. Eg, iva1_ifck in clock24xx.h. What does it > > mean? > > Bug. (Harmless, but a bug nonetheless.) It's fixed in the current > linux-omap head. Those patches are scheduled for a future merge window, > but would be pleased to send those sooner if you like. Right, now that I have one of my OMAP3 platforms sort-of up and running, I can start checking out some ideas. I've taken the easy way out of modifying all the struct clk structures for the time being, and thrown some code at initialization time to take the information already there and manipulate it to the new structure. This includes working out the associations between the 'ick' and 'fck' clocks. On OMAP3430 ES2.2, 2.6.28-rc2, I tripped over these: CLK: no clock to associate dpll3_m3x2_ck.0 with CLK: no clock to associate dpll4_m2x2_ck.0 with CLK: no clock to associate dpll4_m3x2_ck.0 with CLK: no clock to associate dpll4_m4x2_ck.0 with CLK: no clock to associate dpll4_m5x2_ck.0 with CLK: no clock to associate dpll4_m6x2_ck.0 with CLK: no clock to associate iva2_ck.0 with CLK: no clock to associate hsotgusb_ick.0 with CLK: no clock to associate sdrc_ick.0 with CLK: no clock to associate pka_ick.0 with CLK: no clock to associate icr_ick.0 with CLK: no clock to associate aes2_ick.0 with CLK: no clock to associate sha12_ick.0 with CLK: no clock to associate des2_ick.0 with CLK: no clock to associate mailboxes_ick.0 with CLK: no clock to associate omapctrl_ick.0 with CLK: no clock to associate aes1_ick.0 with CLK: no clock to associate rng_ick.0 with CLK: no clock to associate sha11_ick.0 with CLK: no clock to associate usbhost_120m_fck.0 with CLK: no clock to associate wdt1_ick.0 with CLK: no clock to associate omap_32ksync_ick.0 with CLK: no clock to associate gpt12_ick.0 with CLK: no clock to associate sr1_fck.0 with CLK: no clock to associate sr2_fck.0 with which all don't have a corresponding clock for their "other" [if]ck. If I take this further, and detect clocks which result in omap2_clk_wait_ready() being called, but we don't have a corresponding [if]ck, I see at least these four warnings: CLK: omapctrl_ick.0: no other_clk but asked to wait_ready CLK: sdrc_ick.0: no other_clk but asked to wait_ready CLK: dpll4_m2x2_ck.0: no other_clk but asked to wait_ready CLK: omap_32ksync_ick.0: no other_clk but asked to wait_ready So, presumably these ick clocks don't have corresponding fck clocks, which means checking the fck register for the bit would be wrong? So, here's the question: should any of the above clocks result in omap2_wait_clock_ready() being called? If the answer is no, that's great. If yes, are there missing clk structures for OMAP3? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] ARM: OMAP2: Remove OMAP_PRM_REGADDR, OMAP_CM_REGADDR
Hello Russell, On Tue, 7 Oct 2008, Russell King - ARM Linux wrote: > One thing I've noticed, however, is that there seem to be clocks which > result in omap2_clk_wait_ready() being called, which checks the "other" > F or I clock register, but there's no corresponding clock defined in > the source for that bit. Eg, iva1_ifck in clock24xx.h. What does it > mean? Bug. (Harmless, but a bug nonetheless.) It's fixed in the current linux-omap head. Those patches are scheduled for a future merge window, but would be pleased to send those sooner if you like. The omap2_clk_wait_ready() function is one of the worst legacies of the old code. Code aside, one might reasonably assume that it waits for a clock to become ready. But it actually waits for a device, not a clock, to indicate that it is no longer idle. The practical impact of this is that a device will trigger interconnect errors if its registers are accessed while it's still idle. Only a subset of devices can indicate readiness. The plan in the medium term is to move this function to a device layer, and call it something like "omap2_device_wait_ready()". Comments and ideas very welcome, - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] ARM: OMAP2: Remove OMAP_PRM_REGADDR, OMAP_CM_REGADDR
Hello Russell,
On Mon, 6 Oct 2008, Russell King - ARM Linux wrote:
> On Thu, Oct 02, 2008 at 10:37:52AM -0600, Paul Walmsley wrote:
> > From: Tony Lindgren <[EMAIL PROTECTED]>
> >
> > Remove OMAP_PRM_REGADDR and OMAP_CM_REGADDR. Use
> > prm_read/write_mod_reg() or cm_read/write_mod_reg() instead. For
> > assembly, use OMAP_PRM_REGADDR or OMAP_CM_REGADDR macros.
>
> I do have concerns about this patch as well - hating those casts
> that are required to store an offset in 'enable_reg', which then
> have to be un-casted to add the correct base address.
Agreed - that code has been removed already, but the patches haven't
been posted for your review yet. They are queued for a future merge
window with you (they are currently part of Tony's tree). Those
patches can be posted now for your review, if you'd prefer. The
current struct clk is similar to what you propose:
struct clk {
...
s16 prcm_mod;
u32 enable_reg;
u16 clksel_reg;
u8 enable_bit;
u8 idlest_bit;
...
int (*enable)(struct clk *);
void(*disable)(struct clk *);
...
};
The *_reg fields should be called *_offset, as in your example. Those
are computed as CM_BASE + prcm_mod + enable_reg (as an example).
(enable_reg is temporarily a u32 until OMAP1 clock code is revised.
For OMAP2/3 it can be a u16.)
We currently use the following code for clock register loads and stores
in mach-omap2/clock.c:
/*
* _omap2_clk_read_reg - read a clock register
* @clk: struct clk *
*
* Given a struct clk *, returns the value of the clock's register.
*/
static u32 _omap2_clk_read_reg(u16 reg_offset, struct clk *clk)
{
if (clk->prcm_mod & CLK_REG_IN_SCM)
return omap_ctrl_readl(reg_offset);
else if (clk->prcm_mod & CLK_REG_IN_PRM)
return prm_read_mod_reg(clk->prcm_mod & PRCM_MOD_ADDR_MASK,
reg_offset);
else
return cm_read_mod_reg(clk->prcm_mod, reg_offset);
}
/*
* _omap2_clk_write_reg - write a clock's register
* @v: value to write to the clock's enable_reg
* @clk: struct clk *
*
* Given a register value @v and struct clk * @clk, writes the value
* of @v to the clock's enable register. No return value.
*/
static void _omap2_clk_write_reg(u32 v, u16 reg_offset,
struct clk *clk)
{
if (clk->prcm_mod & CLK_REG_IN_SCM)
omap_ctrl_writel(v, reg_offset);
else if (clk->prcm_mod & CLK_REG_IN_PRM)
prm_write_mod_reg(v, clk->prcm_mod & PRCM_MOD_ADDR_MASK,
reg_offset);
else
cm_write_mod_reg(v, clk->prcm_mod, reg_offset);
}
Does this look reasonable?
- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] ARM: OMAP2: Remove OMAP_PRM_REGADDR, OMAP_CM_REGADDR
On Mon, Oct 06, 2008 at 05:18:49PM +0100, Russell King - ARM Linux wrote: > On Thu, Oct 02, 2008 at 10:37:52AM -0600, Paul Walmsley wrote: > > From: Tony Lindgren <[EMAIL PROTECTED]> > > > > Remove OMAP_PRM_REGADDR and OMAP_CM_REGADDR. Use > > prm_read/write_mod_reg() or cm_read/write_mod_reg() instead. For > > assembly, use OMAP_PRM_REGADDR or OMAP_CM_REGADDR macros. > > I do have concerns about this patch as well - hating those casts > that are required to store an offset in 'enable_reg', which then > have to be un-casted to add the correct base address. > > I've been trying to work out if there's a better way to do this > using the existing structures. How about: I've modified this idea slightly, since using a mask instead of bit numbers makes it possible to remove more special casing... and I also have a perl script to convert the clock definitions. One thing I've noticed, however, is that there seem to be clocks which result in omap2_clk_wait_ready() being called, which checks the "other" F or I clock register, but there's no corresponding clock defined in the source for that bit. Eg, iva1_ifck in clock24xx.h. What does it mean? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] ARM: OMAP2: Remove OMAP_PRM_REGADDR, OMAP_CM_REGADDR
On Thu, Oct 02, 2008 at 10:37:52AM -0600, Paul Walmsley wrote:
> From: Tony Lindgren <[EMAIL PROTECTED]>
>
> Remove OMAP_PRM_REGADDR and OMAP_CM_REGADDR. Use
> prm_read/write_mod_reg() or cm_read/write_mod_reg() instead. For
> assembly, use OMAP_PRM_REGADDR or OMAP_CM_REGADDR macros.
I do have concerns about this patch as well - hating those casts
that are required to store an offset in 'enable_reg', which then
have to be un-casted to add the correct base address.
I've been trying to work out if there's a better way to do this
using the existing structures. How about:
struct clk {
...
void __iomem*base;
struct clk *other_clk; /* associated func/interface clock */
u16 enable_offset;
u16 idlest_offset;
u16 clksel_offset;
u8 enable_bit;
u8 idlest_bit;
...
int (*enable)(struct clk *);
void(*disable)(struct clk *);
...
};
(or use u8 if the offsets always fit.)
The setup of apll96_ck becomes:
static struct clk apll96_ck = {
.name = "apll96_ck",
.parent = &sys_ck,
.rate = 9600,
.flags = CLOCK_IN_OMAP242X | CLOCK_IN_OMAP243X |
RATE_FIXED | RATE_PROPAGATES | ENABLE_ON_INIT,
.clkdm_name = "wkup_clkdm",
.enable_offset = CM_REG_OFFSET(PLL_MOD, CM_CLKEN),
.idlest_offset = CM_REG_OFFSET(PLL_MOD, CM_IDLEST),
.enable_bit = OFFSET_OF_BIT(EN_APLL_LOCKED <<
OMAP24XX_EN_96M_PLL_SHIFT),
.idlest_bit = OFFSET_OF_BIT(OMAP24XX_ST_96M_APLL),
.enable = clkll_enable32_and_wait,
.disable= clkll_disable32,
.recalc = &propagate_rate,
};
(replacing OFFSET_OF_BIT() with the appropriate real bit position number).
You register all such clocks thusly:
void clk_register_offset_clks(void __iomem *base, struct clk **clks, size_t num)
{
size_t i;
for (i = 0; i < num; i++) {
struct clk *clk = clks[i];
clk->base = base;
clk_register(clk);
}
}
static struct clk cm_clks[] = {
&apll96_ck,
};
clk_register_offset_clks(cm_base, cm_clks, ARRAY_SIZE(cm_clks);
and, eg, the accesses to the enable register become:
int clkll_enable32(struct clk *clk)
{
u32 val, mask = 1 << clk->enable_bit;
val = __raw_readl(clk->base + clk->enable_offset);
if (clk->flags & INVERT_ENABLE)
val &= ~mask;
else
val |= mask;
__raw_writel(val, clk->base + clk->enable_offset);
wmb();
return 0;
}
void clkll_disable32(struct clk *clk)
{
u32 val, mask = 1 << clk->enable_bit;
val = __raw_readl(clk->base + clk->enable_offset);
if (clk->flags & INVERT_ENABLE)
val |= mask;
else
val &= ~mask;
__raw_writel(val, clk->base + clk->enable_offset);
wmb();
}
int clkll_is_running32(struct clk *clk)
{
u32 val, mask = 1 << clk->enable_bit;
val = __raw_readl(clk->base + clk->enable_offset) & mask;
return clk->flags & INVERT_ENABLE ? !val : !!val;
}
int clkll_enable32_and_wait(struct clk *clk)
{
u32 mask, result, val;
unsigned int tries = 0;
if (!clkll_is_running32(clk))
clkll_enable32(clk);
/* check if other clock, if any, is running */
if (clk->other_clk && !clkll_is_running32(clk->other_clk))
return 0;
val = mask = 1 << clk->idlest_bit;
if (inverted_on_this_cpu)
val = 0;
for (tries = 0; tries < MAX_CLOCK_ENABLE_WAIT; tries++, udelay(1)) {
result = __raw_readl(clk->base + clk->idlest_offset) & mask;
if (result == val)
break;
}
if (result == val)
pr_debug("Clock %s stable after %uus\n", clk->name, tries);
else
pr_err("Clock %s failed to stablize after %uus\n",
clk->name, tries);
return result == val ? 0 : -ETIMEDOUT;
}
Then, you use clkll_enable32() if you don't need to wait for the clock
to stablize, or clkll_enable32_and_wait() if you do.
If you need 16-bit, which I think I've seen, obviously create the
corresponding versions.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
