RE: [RFC][DRAFT] TODO list for TI DSP BRIDGE

Kanigeri, Hari
2008-08-20T22:37:03Z

Felipe,

> We are trying to understand why this is not just a wrapper.

-- Thank you :)

> 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.
>
-- Why can't this some other place be the init function in the current clock module in Bridge ?

> How exactly is "clk_enable(BPWR_Clks[clkIdIndex].intClk)" less
> readable than "CLK_Enable(BPWR_Clks[clkIdIndex].intClk)" ?

-- Did you check the argument of CLK_Enable function ? The argument is not the same as you mentioned above? One more reason to say that this is not a direct wrapper ;).

But that's not my point when I mentioned about readability. By removing the code from clk.c and scattering it around will make it difficult to follow and manage the code. I think we can even move the DSPPeripheralClkCtrl function in tiomap_pwr.c to clk.c so that all the clock interfaces from DSP Bridge are in one file, and we can rename this file to bridge_clk_mgr to avoid the impression that clk module in Bridge is implementing its own clock framework or the functions are just acting as wrappers. I think this approach is clean and easy to manage. I see a placeholder for dspbridge clock module considering the number of clocks it has to manage.

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> Sent: Tuesday, August 19, 2008 6:46 PM
> To: Kanigeri, Hari
> Cc: Tony Lindgren; Trilok Soni; Pasam, Vijay; Hiroshi DOYU; linux-
> omap@vger.kernel.org; Woodruff, Richard; siarhei.siamashka@nokia.com;
> Ramirez Luna, Omar; Gupta, Ramesh
> Subject: Re: [RFC][DRAFT] TODO list for TI DSP BRIDGE
>
> Hi Hari,
>
> On Tue, Aug 19, 2008 at 11:46 PM, Kanigeri, Hari <h-kanigeri2@ti.com>
> 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