Hi Hari,
On Tue, Aug 19, 2008 at 11:46 PM, Kanigeri, Hari <h-kanigeri2@xxxxxx> wrote:
> Felipe,
>
> Thanks for your comments.
>
>> DSPPeripheralClkCtrl is already mapping some clock ids to some other
>> clock ids (SERVICES_ClkId) before calling the functions in clk.c. This
>> clk_enable/disable can be called there.
>
> - DSPPeripheralClkCtrl is used for managing the peripheral clock requests that come from DSP. You cannot use this function to enable IVA2 and mailbox clocks.
That's right, you call clk_enable directly for iva2 and mailbox
clocks. You already know the clock, no need to translate.
> Felipe, I see your point of removing the clock module, but please don't view this just as a wrapper; it provides more functionality than just being the direct wrapper. It does hide the implementation to translate the clock id to kernel clock structure, the clock init does the job of obtaining the references to the clock ids in its initialization, and more importantly isolates the tracing to one module.
We are trying to understand why this is not just a wrapper.
a) It does hide the implementation to translate the clock id to kernel
clock structure
I already mentioned that DSPPeripheralClkCtrl is doing that translation.
{(u32) BPWR_GPTimer5, SERVICESCLK_gpt5_fck, SERVICESCLK_gpt5_ick},
Those SERVICESCLK_* are used to find the clk handle, but if you
already have BPWR_GPTimer5 you know which clk handles you need, why
not store them in some place where DSPPeripheralClkCtrl can find them?
if (BPWR_GPTimer5) { clk_enable(gpt5_fck_handle); clk_enable(gpt5_ick_handle); }
Or if you store the clk handle directly into the BPWR_Clk_t structure:
clk_enable(BPWR_Clks[clkIdIndex].intClk);
clk_enable(BPWR_Clks[clkIdIndex].funClk);
b) the clock init does the job of obtaining the references to the
clock ids in its initialization
Those could be initialized in some other place as well.
c) and more importantly isolates the tracing to one module.
I already mentioned how to enable debugging while using clk_enable,
just enable a macro with the same name.
> There are multiple ways to implement, and yes we can remove the clock module in Bridge and move the code in these functions to other places, but in the end we will have the new code that has lesser code readability and debugging ability :).
How exactly is "clk_enable(BPWR_Clks[clkIdIndex].intClk)" less
readable than "CLK_Enable(BPWR_Clks[clkIdIndex].intClk)" ?
>> Didn't Tony just explained how to do that without a wrapper?
> -- I was under the impression that Tony is thinking that the DSP Bridge is not calling kernel's clk_enable function. My above explanation holds here about the wrappers.
I'm not sure, but in any case I also explained an alternative.
Best regards.
--
Felipe Contreras
--
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
