Hello Paul,
Thanks for the review comments. My replies below.
On Fri, 18-Dec-09 7:12 AM +0530, Paul Walmsley wrote:
>> +/* Clocks for AM35XX */
>> +static struct clk emac_ick = {
>> + .name = "emac_ick",
>> + .ops = &clkops_omap2_dflt,
>
> Shouldn't this clock use &clkops_omap2_dflt_wait (or rather, some custom
> version that knows how to read the _ACK bits in IPSS_CLK_CTRL) and test
> CPGMAC_VBUSP_CLK_EN_ACK? Same for the other IPSS VBUSP clocks? (I
guess
> "VBUSP" is a synonym for "interface"?)
I will ad a custom clkops to address this. Do we need a find_companion
to be defined? The VBUSP clock ACK's do not depend on functional clocks
and the functional clocks themselves don't have ant ACK or status bits.
EMAC, VPFE amd MUSBOTG are initiators, but the their ACK bits are just
for the target idle status.
One of the issues on AM35xx is regarding the status indicator for new
clocks. Here 1 indicates as enabled and 0 as not ready state, similar to
24xx case but just opposite to 34xx clocks. And now we have a mix of
these two on AM35xx. In omap2_cm_wait_idlest, I do not get a per clock
granularity to decide what should be the status check for that
particular clock. One of the approach I am thinking is to define a new
clock flag (say INVERT_ENABLE_STATUS) and set it for the new AM35xx
clocks. And in omap2_module_wait_ready I will do the cpu check and flag
check on a per clock basis and determine the status that need to be
checked and pass it to omap2_module_wait_ready to check what we
want. omap2_module_wait_ready will not be doing any cpu checks as its
done today. Let me know what you think.
>> +static struct clk emac_fck = {
>> + .name = "emac_fck",
>> + .ops = &clkops_omap2_dflt,
>> + .clkdm_name = "core_l3_clkdm",
>
> Is this the correct clockdomain for this clock? It seems unlikely that a
> 50MHz functional clock would be in core_l3_clkdm. Usually these are all
> interface clocks. This applies for all the other functional clocks
listed
> in this file also.
Would it be OK if we just don't set clkdm_name. I am not sure whether we
can map it to any existing OMAP3 clock domains that way
>
>> + .rate = 50000000,
>
> What's the parent of this clock? Looking at TRM section 4.7.7.12 it
> appears that it gets this from an off-chip source, "rmii_clk"? Guess
that
> should probably be defined as the fixed source clock, and emac_fck should
> list rmii_clk as its parent?
>
>> + .enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
>> + .enable_bit = AM35XX_CPGMAC_FCLK_SHIFT,
>> + .recalc = &omap2_clksel_recalc,
>
> This .recalc field is wrong, since there's no .clksel field defined. If
> you define that parent, then this should be followparent_recalc. The
> parent should have .flags = RATE_FIXED and no .recalc.
Do we really need to define an rmii_ck? Can we make emac_fck itself as
the root clock with no parent? Here is what I was thinking of. Let me
know if you see any issues with this
static struct clk emac_fck = {
.name = "emac_fck",
.ops = &clkops_omap2_dflt,
.flags = RATE_FIXED,
.rate = 50000000,
.enable_reg = OMAP343X_CTRL_REGADDR(OMAP3517_CONTROL_IP_CLK_CTRL),
.enable_bit = OMAP3517_CPGMAC_FCLK_SHIFT,
};
>> +static struct clk vpfe_fck = {
>> + .name = "vpfe_fck",
>> + .ops = &clkops_omap2_dflt,
>> + .clkdm_name = "core_l3_clkdm",
>> + .rate = 27000000,
>> + .enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
>> + .enable_bit = AM35XX_VPFE_FCLK_SHIFT,
>> + .recalc = &omap2_clksel_recalc,
>
> This fixed rate clock isn't right, for the same reasons as described
> above.
Would make it similar to emac_fck depending on what we decide.
>> + CLK("musb_hdrc", "ick", &hsotgusb_ick_am35xx, CK_AM35XX),
>> + CLK("musb_hdrc", "fck", &hsotgusb_fck_am35xx, CK_AM35XX),
>> + CLK(NULL, "hecc_ck", &hecc_ck, CK_AM35XX),
>> + CLK("vpfe-capture", "master", &vpfe_ick, CK_AM35XX),
>> + CLK("vpfe-capture", "slave", &vpfe_fck, CK_AM35XX),
>> + CLK(NULL, "uart4_ick", &uart4_ick, CK_AM35XX),
>
> Please align the new structure entries with the previous entries.
Will do that.
- Ranjith
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
