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

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

[Hiroshi DOYU]

Hi all,

This is a draft proposal for the bridge TODO list just as a
starter. For the details, lots of comments could be added and
discussed here.

The basic concept is that, at first, we will make every components
a little bit more basic, small, primitive and independent(small kernel
modules), pushing out the kind of operating policies from modules to
more upper layers, and then reconstruct the system again with the
above primitive components to be more *unix-like* way. This may be the
bottom up approach, replacing every component **step by step** with
the always working system and finally we will get the new one. This
may allow us always to keep the working system and also be more
flexible to have multiple implementation(bridge, dspgateway and
something new) easily, on the top of the above basic
components. Ideally it would be nice if some of components were pushed
out in userland, but there may be the trade-off of performance.


The major points are listed below:

Small cleanups
--------------
There are still quite lots of trivial things, naming convention,
etc. In detail, please check:

- Documentation/CodingStyle
- Documentation/SubmitChecklist
- Documentation/SubmittingPatches

This can be cleaned up along with the other changes gradually.


Replace home-brewed APIs by native kernel APIs
----------------------------------------------

Most of the files under "drivers/dsp/bridge/services" and
"drivers/dsp/bridge/gen" seem just to provide the almost same
basic in-OS functionalities as Linux in-kernel APIs provided, like
"list", "ISR", "softIRQ", "Notifier", "locking", "bitmap", "clk" and
so on with some bridge specific debug tracing feature. The fundamental
kernel APIs should be covered by native Linux kernel APIs.

In-kernel APIs(ex: PRCM) should be used, instead of direct access for
registers.

I expect that this task would reduces the code size quite a lot.


Use "arch/arm/plat-omap/mmu.o" as generic mmu operations
---------------------------------------------------------
The "dspgateway" has to be modified a little, too. IVA[1,2],  C5x and
Camera can use the same algorithm with this driver.
	

Use "arch/arm/plat-omap/mailbox.o" as generic mailbox operations
-----------------------------------------------------------------
The "dspgateway" has to be modified a little, too.


Make communication "protocol" more independent
-----------------------------------------------
<TBD>
The communication which "shared memory" and "interrupt based mailbox"
provide shouldn't impose any operation policies on the above "mmu" and
"mailbox" drivers.


Push out some components into userland
---------------------------------------
At least, The "doff loader" should be pushed out because it reads
files from in-kernel. There's already sample implementation which
Trilok has done before. Need to evaluate the performance again.


Define bridge Interfaces
------------------------
<TBD>
This was already described in the following thread:
http://linux.omap.com/pipermail/linux-omap-open-source/2007-April/009540.html
I think that this is also tightly related to TI's roadmap, support
plan and so on.....

Probably I missed lots of things here, any comments/corrections would
be appreciated. I'd like to get the rough impression of feasibility of
the above items from TI.

If people have to think about the multi OS support, then that would be
the first to discuss. It may be beyond my imagination, though....

BTW: The latest update from omapzoom has been updated in the bridge patches:

  http://www.muru.com/linux/tidspbridge/

	Hiroshi DOYU

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

[Pasam, Vijay]

Hi Hiroshi:

> Small cleanups
> --------------
> There are still quite lots of trivial things, naming 
> convention, etc. In detail, please check:
> 
> - Documentation/CodingStyle
> - Documentation/SubmitChecklist
> - Documentation/SubmittingPatches
> 
> This can be cleaned up along with the other changes gradually.

I would say this is #1 priority task. We did spend lot of time in
reowrking the code to adhere to the linux kernel coding style and most
the changes should be available. If you have any specific comments,
please let us know and we will look into it.

> Replace home-brewed APIs by native kernel APIs
> ----------------------------------------------
> 
> Most of the files under "drivers/dsp/bridge/services" and 
> "drivers/dsp/bridge/gen" seem just to provide the almost same 
> basic in-OS functionalities as Linux in-kernel APIs provided, 
> like "list", "ISR", "softIRQ", "Notifier", "locking", 
> "bitmap", "clk" and so on with some bridge specific debug 
> tracing feature. The fundamental kernel APIs should be 
> covered by native Linux kernel APIs.
> 
> In-kernel APIs(ex: PRCM) should be used, instead of direct 
> access for registers.
> 
> I expect that this task would reduces the code size quite a lot.

Some of the above items are being looked at like replacing CSL, isr
etc.. But there are no equivalent API's or functionality available in
kernel for many of them. We should replace the API's with the avaolable
kernel API's and the remaining need to be looked at from long term.


> Use "arch/arm/plat-omap/mmu.o" as generic mmu operations
> ---------------------------------------------------------
> The "dspgateway" has to be modified a little, too. IVA[1,2],  
> C5x and Camera can use the same algorithm with this driver.
>

I remember you mentioning gateway doesn't support OMAP3. Lot of MMU code
is bridge is OMAP3 specific and also it uses TWL feature for dynamic
memory mapping. I am not sure what do we gain by separating the generic
MMU operations if gateway doesn't use MMU the same way as Bridge. 	

> 
> Use "arch/arm/plat-omap/mailbox.o" as generic mailbox operations
> -----------------------------------------------------------------
> The "dspgateway" has to be modified a little, too.

Agree. This is in TI roadmap for next silicon revision. We will send out
the design for comments once it is ready.


> Make communication "protocol" more independent
> -----------------------------------------------
> <TBD>
> The communication which "shared memory" and "interrupt based mailbox"
> provide shouldn't impose any operation policies on the above 
> "mmu" and "mailbox" drivers.
> 
> 
> Push out some components into userland
> ---------------------------------------
> At least, The "doff loader" should be pushed out because it 
> reads files from in-kernel. There's already sample 
> implementation which Trilok has done before. Need to evaluate 
> the performance again.
>
> Define bridge Interfaces
> ------------------------
> <TBD>
> This was already described in the following thread:
> http://linux.omap.com/pipermail/linux-omap-open-source/2007-Ap
ril/009540.html
> I think that this is also tightly related to TI's roadmap, 
> support plan and so on.....

I agree these should be looked from a longer term standpoint. As Richard
stated earlier, providing a stable bridge version is one our top
requirement as this is being used by several TI users.  We need to pick
the ones that helps us most and focus efforts on them. Probably we can
pick 1 or 2 tops items and focus on them.  Note that these changes would
require lot of effort and potentially impact the stability of the code
if not properly tested. For example, moving doff loader to user land
might be easy for static loading, but supporting dynamic loading would
need access to Node interface in the kernel. This might either require
part of the node functionality to user mode or have context switches to
access this from kernel mode. We need to have some protoyping done and
performance measurements done before making the call on design.

TI is committed to contribute Bridge to the open source community. We
will continue to work closely to evolve bridge that is beneficial to all
the users of Bridge.

Best Regards
Vijay
 

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

[Hiroshi DOYU]

Vijay, Thank you for your quick reply.

From: "ext Pasam, Vijay" <vpasam@ti.com>
Subject: RE: [RFC][DRAFT] TODO list for TI DSP BRIDGE
Date: Mon, 18 Aug 2008 09:28:00 -0500

> Hi Hiroshi:
> 
> > Small cleanups
> > --------------
> > There are still quite lots of trivial things, naming 
> > convention, etc. In detail, please check:
> > 
> > - Documentation/CodingStyle
> > - Documentation/SubmitChecklist
> > - Documentation/SubmittingPatches
> > 
> > This can be cleaned up along with the other changes gradually.
> 
> I would say this is #1 priority task. We did spend lot of time in
> reowrking the code to adhere to the linux kernel coding style and most
> the changes should be available. If you have any specific comments,
> please let us know and we will look into it.

The following is just an example, "CodingStyle" says,
....
		Chapter 4: Naming

C is a Spartan language, and so should your naming be.  Unlike Modula-2
and Pascal programmers, C programmers do not use cute names like
ThisVariableIsATemporaryCounter.  A C programmer would call that
variable "tmp", which is much easier to write, and not the least more
difficult to understand.

HOWEVER, while mixed-case names are frowned upon, descriptive names for
global variables are a must.  To call a global function "foo" is a
shooting offense.
......

But these are too trivial, then fix it occasionally along with other
important fixes. You can find lots of too trivial examples on like
this the above document. I won't explain one by one, though....


> > Replace home-brewed APIs by native kernel APIs
> > ----------------------------------------------
> > 
> > Most of the files under "drivers/dsp/bridge/services" and 
> > "drivers/dsp/bridge/gen" seem just to provide the almost same 
> > basic in-OS functionalities as Linux in-kernel APIs provided, 
> > like "list", "ISR", "softIRQ", "Notifier", "locking", 
> > "bitmap", "clk" and so on with some bridge specific debug 
> > tracing feature. The fundamental kernel APIs should be 
> > covered by native Linux kernel APIs.
> > 
> > In-kernel APIs(ex: PRCM) should be used, instead of direct 
> > access for registers.
> > 
> > I expect that this task would reduces the code size quite a lot.
> 
> Some of the above items are being looked at like replacing CSL, isr
> etc.. But there are no equivalent API's or functionality available in
> kernel for many of them. We should replace the API's with the avaolable
> kernel API's and the remaining need to be looked at from long term.

I think that this may be relatively an easier part. Just implementing
the same functionality by using native kernel APIs. So I guess that it
can be in short term actually.


> > Use "arch/arm/plat-omap/mmu.o" as generic mmu operations
> > ---------------------------------------------------------
> > The "dspgateway" has to be modified a little, too. IVA[1,2],  
> > C5x and Camera can use the same algorithm with this driver.
> >
> 
> I remember you mentioning gateway doesn't support OMAP3. Lot of MMU code
> is bridge is OMAP3 specific and also it uses TWL feature for dynamic
> memory mapping. I am not sure what do we gain by separating the generic
> MMU operations if gateway doesn't use MMU the same way as Bridge.

Yes, "dspgateway" won't support OMAP3 but if I remember correctly that
TWL is already in OMAP2? and also does Camera use the same mechanism
of this MMU? This may be benefit or a must, I guess..


> > Use "arch/arm/plat-omap/mailbox.o" as generic mailbox operations
> > -----------------------------------------------------------------
> > The "dspgateway" has to be modified a little, too.
> 
> Agree. This is in TI roadmap for next silicon revision. We will send out
> the design for comments once it is ready.
> 
> > Make communication "protocol" more independent
> > -----------------------------------------------
> > <TBD>
> > The communication which "shared memory" and "interrupt based mailbox"
> > provide shouldn't impose any operation policies on the above 
> > "mmu" and "mailbox" drivers.
> > 
> > 
> > Push out some components into userland
> > ---------------------------------------
> > At least, The "doff loader" should be pushed out because it 
> > reads files from in-kernel. There's already sample 
> > implementation which Trilok has done before. Need to evaluate 
> > the performance again.
> >
> > Define bridge Interfaces
> > ------------------------
> > <TBD>
> > This was already described in the following thread:
> > http://linux.omap.com/pipermail/linux-omap-open-source/2007-Ap
> ril/009540.html
> > I think that this is also tightly related to TI's roadmap, 
> > support plan and so on.....
> 
> I agree these should be looked from a longer term standpoint. As Richard
> stated earlier, providing a stable bridge version is one our top
> requirement as this is being used by several TI users.  We need to pick
> the ones that helps us most and focus efforts on them. Probably we can
> pick 1 or 2 tops items and focus on them.  Note that these changes would
> require lot of effort and potentially impact the stability of the code
> if not properly tested. For example, moving doff loader to user land
> might be easy for static loading, but supporting dynamic loading would
> need access to Node interface in the kernel. This might either require
> part of the node functionality to user mode or have context switches to
> access this from kernel mode. We need to have some protoyping done and
> performance measurements done before making the call on design.

Agreed. Need more investigation and prototype actually.

> TI is committed to contribute Bridge to the open source community. We
> will continue to work closely to evolve bridge that is beneficial to all
> the users of Bridge.

I think it would be nice if you sends bridge paches against "l-o", and
also this would be beneficial for "omapzoom", too. As for "bridge",
"omapzoom" repository just git-pull from "l-o" and it will get the
latest bridge code automatically, as is.

       Hiroshi DOYU

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

[Kanigeri, Hari]

Hi,

> Some of the above items are being looked at like replacing CSL, isr etc..
> But there are no equivalent API's or functionality available in kernel for
> many of them. We should replace the API's with the avaolable kernel API's
> and the remaining need to be looked at from long term.
>

-- I agree with Vijay. We can remove the simple wrapper functions like the ones in CSL and ISR, but some of the other modules in the services provide more functionality than just being the wrappers. We don't want to compromise on any current debugging features that are provided by these modules at the expense of replacing the services functions with direct kernel calls. This would require careful analysis.

If any one has done any analysis on the services functions that can be removed without compromising the functionality and debugging feature, please share it on the mailing list. 

On the comment to remove clk.c, I don't agree with it. I think it provides an advantage of having a clk module in the DSP Bridge as all the calls to clock module are centralized in one location, which would aid in Bridge debugging.


Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Pasam, Vijay
> Sent: Monday, August 18, 2008 9:28 AM
> To: Hiroshi DOYU; linux-omap@vger.kernel.org
> Cc: Woodruff, Richard; felipe.contreras@gmail.com;
> siarhei.siamashka@nokia.com; Ramirez Luna, Omar; Gupta, Ramesh; Kanigeri,
> Hari; tony@atomide.com; soni.trilok@gmail.com
> Subject: RE: [RFC][DRAFT] TODO list for TI DSP BRIDGE
> 
> Hi Hiroshi:
> 
> > Small cleanups
> > --------------
> > There are still quite lots of trivial things, naming
> > convention, etc. In detail, please check:
> >
> > - Documentation/CodingStyle
> > - Documentation/SubmitChecklist
> > - Documentation/SubmittingPatches
> >
> > This can be cleaned up along with the other changes gradually.
> 
> I would say this is #1 priority task. We did spend lot of time in
> reowrking the code to adhere to the linux kernel coding style and most the
> changes should be available. If you have any specific comments, please let
> us know and we will look into it.
> 
> > Replace home-brewed APIs by native kernel APIs
> > ----------------------------------------------
> >
> > Most of the files under "drivers/dsp/bridge/services" and
> > "drivers/dsp/bridge/gen" seem just to provide the almost same
> > basic in-OS functionalities as Linux in-kernel APIs provided,
> > like "list", "ISR", "softIRQ", "Notifier", "locking",
> > "bitmap", "clk" and so on with some bridge specific debug
> > tracing feature. The fundamental kernel APIs should be
> > covered by native Linux kernel APIs.
> >
> > In-kernel APIs(ex: PRCM) should be used, instead of direct
> > access for registers.
> >
> > I expect that this task would reduces the code size quite a lot.
> 
> Some of the above items are being looked at like replacing CSL, isr etc..
> But there are no equivalent API's or functionality available in kernel for
> many of them. We should replace the API's with the avaolable kernel API's
> and the remaining need to be looked at from long term.
> 
> 
> > Use "arch/arm/plat-omap/mmu.o" as generic mmu operations
> > ---------------------------------------------------------
> > The "dspgateway" has to be modified a little, too. IVA[1,2],
> > C5x and Camera can use the same algorithm with this driver.
> >
> 
> I remember you mentioning gateway doesn't support OMAP3. Lot of MMU code
> is bridge is OMAP3 specific and also it uses TWL feature for dynamic
> memory mapping. I am not sure what do we gain by separating the generic
> MMU operations if gateway doesn't use MMU the same way as Bridge.
> 
> >
> > Use "arch/arm/plat-omap/mailbox.o" as generic mailbox operations
> > -----------------------------------------------------------------
> > The "dspgateway" has to be modified a little, too.
> 
> Agree. This is in TI roadmap for next silicon revision. We will send out
> the design for comments once it is ready.
> 
> 
> > Make communication "protocol" more independent
> > -----------------------------------------------
> > <TBD>
> > The communication which "shared memory" and "interrupt based mailbox"
> > provide shouldn't impose any operation policies on the above
> > "mmu" and "mailbox" drivers.
> >
> >
> > Push out some components into userland
> > ---------------------------------------
> > At least, The "doff loader" should be pushed out because it
> > reads files from in-kernel. There's already sample
> > implementation which Trilok has done before. Need to evaluate
> > the performance again.
> >
> > Define bridge Interfaces
> > ------------------------
> > <TBD>
> > This was already described in the following thread:
> > http://linux.omap.com/pipermail/linux-omap-open-source/2007-Ap
> ril/009540.html
> > I think that this is also tightly related to TI's roadmap,
> > support plan and so on.....
> 
> I agree these should be looked from a longer term standpoint. As Richard
> stated earlier, providing a stable bridge version is one our top
> requirement as this is being used by several TI users.  We need to pick
> the ones that helps us most and focus efforts on them. Probably we can
> pick 1 or 2 tops items and focus on them.  Note that these changes would
> require lot of effort and potentially impact the stability of the code if
> not properly tested. For example, moving doff loader to user land might be
> easy for static loading, but supporting dynamic loading would need access
> to Node interface in the kernel. This might either require part of the
> node functionality to user mode or have context switches to access this
> from kernel mode. We need to have some protoyping done and performance
> measurements done before making the call on design.
> 
> TI is committed to contribute Bridge to the open source community. We will
> continue to work closely to evolve bridge that is beneficial to all the
> users of Bridge.
> 
> Best Regards
> Vijay
> 

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

[Pasam, Vijay]

Hi Hiroshi,

> Yes, "dspgateway" won't support OMAP3 but if I remember 
> correctly that TWL is already in OMAP2? and also does Camera 
> use the same mechanism of this MMU? This may be benefit or a 
> must, I guess..

I agree. If we are certain that camera driver or some other driver uses
MMU the same way bridge does and the MMU architecture is the same, it
makes sense to make this generic code. My point is that if DSPGateway
doesn't use TWL, it will not benefit from using the Bridge MMU code
which has TWL enabled. 

On the other hand, we have a usecase for mailbox driver being used by
multiple IPC stacks. Hence it makes sense to make this code generic.

Best Regards
Vijay
 

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

[Trilok Soni]

Hi Hari,

On Mon, Aug 18, 2008 at 9:57 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> Hi,
>
>> Some of the above items are being looked at like replacing CSL, isr etc..
>> But there are no equivalent API's or functionality available in kernel for
>> many of them. We should replace the API's with the avaolable kernel API's
>> and the remaining need to be looked at from long term.
>>
>
> -- I agree with Vijay. We can remove the simple wrapper functions like the ones in CSL and ISR, but some of the other modules in the services provide more functionality than just being the wrappers. We don't want to compromise on any current debugging features that are provided by these modules at the expense of replacing the services functions with direct kernel calls. This would require careful analysis.
>
> If any one has done any analysis on the services functions that can be removed without compromising the functionality and debugging feature, please share it on the mailing list.
>
> On the comment to remove clk.c, I don't agree with it. I think it provides an advantage of having a clk module in the DSP Bridge as all the calls to clock module are centralized in one location, which would aid in Bridge debugging.
>

If you mean there is separate clock infrastructure for DSPBridge other
than clock framework we have for OMAP, then DSPBridge should convert
to the one we have for OMAP. As recent work by Paul for clock
framework on OMAP3 and Power domains we need centralized code for clk
and power domains not separate clock infrastructure for DSPBridge.

Richard and Paul can probably explain more about OMAP clock framework
and why it has to be used by DSPBridge too.

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

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

[Trilok Soni]

Hi Vijay,

On Mon, Aug 18, 2008 at 10:42 PM, Pasam, Vijay <vpasam@ti.com> wrote:
> Hi Hiroshi,
>
>> Yes, "dspgateway" won't support OMAP3 but if I remember
>> correctly that TWL is already in OMAP2? and also does Camera
>> use the same mechanism of this MMU? This may be benefit or a
>> must, I guess..
>
> I agree. If we are certain that camera driver or some other driver uses
> MMU the same way bridge does and the MMU architecture is the same, it
> makes sense to make this generic code. My point is that if DSPGateway
> doesn't use TWL, it will not benefit from using the Bridge MMU code
> which has TWL enabled.
>

I think recently some one from TI posted OMAP3 camera driver at
linux-v4l2 mailing list and which I think was using camera MMU, but
not the common MMU framework it seems from the description. They
should first start converting to the common MMU infrastructure then.

http://lists-archives.org/video4linux/23215-omap3-camera-driver.html

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

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

[Trilok Soni]

Hi Hari,

On Mon, Aug 18, 2008 at 9:57 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> Hi,
>
>> Some of the above items are being looked at like replacing CSL, isr etc..
>> But there are no equivalent API's or functionality available in kernel for
>> many of them. We should replace the API's with the avaolable kernel API's
>> and the remaining need to be looked at from long term.
>>
>
> -- I agree with Vijay. We can remove the simple wrapper functions like the ones in CSL and ISR, but some of the other >modules in the services provide more functionality than just being the wrappers. We don't want to compromise on any >current debugging features that are provided by these modules at the expense of replacing the services functions with >direct kernel calls. This would require careful analysis.

What are those __other__ modules? Could you please list out them so
that other can help you why direct kernel API won't work there?

Now there are beagleboards out in the market, other developers can
also help you guys in submitting/cleaning up patches, as far as there
is no duplicate work going on and regular visibility on work done by
TI/Nokia on Bridge internally.


-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

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

[Kanigeri, Hari]

> If you mean there is separate clock infrastructure for DSPBridge other
> than clock framework we have for OMAP, then DSPBridge should convert
> to the one we have for OMAP. As recent work by Paul for clock
> framework on OMAP3 and Power domains we need centralized code for clk
> and power domains not separate clock infrastructure for DSPBridge.
>
--- Bridge is not implementing separate clock infrastructure. It is just centralizing the calls to clk_enable in one location as this is called from multiple files in Bridge code. This helps because one can then turn on the traces only for clock module to check if the clocks that were expected to be enabled are enabled or not. As I mentioned before, this helps in debugging.

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Trilok Soni [mailto:soni.trilok@gmail.com]
> Sent: Monday, August 18, 2008 12:17 PM
> To: Kanigeri, Hari
> Cc: Pasam, Vijay; Hiroshi DOYU; linux-omap@vger.kernel.org; Woodruff,
> Richard; felipe.contreras@gmail.com; siarhei.siamashka@nokia.com; Ramirez
> Luna, Omar; Gupta, Ramesh; tony@atomide.com
> Subject: Re: [RFC][DRAFT] TODO list for TI DSP BRIDGE
> 
> Hi Hari,
> 
> On Mon, Aug 18, 2008 at 9:57 PM, Kanigeri, Hari <h-kanigeri2@ti.com>
> wrote:
> > Hi,
> >
> >> Some of the above items are being looked at like replacing CSL, isr
> etc..
> >> But there are no equivalent API's or functionality available in kernel
> for
> >> many of them. We should replace the API's with the avaolable kernel
> API's
> >> and the remaining need to be looked at from long term.
> >>
> >
> > -- I agree with Vijay. We can remove the simple wrapper functions like
> the ones in CSL and ISR, but some of the other modules in the services
> provide more functionality than just being the wrappers. We don't want to
> compromise on any current debugging features that are provided by these
> modules at the expense of replacing the services functions with direct
> kernel calls. This would require careful analysis.
> >
> > If any one has done any analysis on the services functions that can be
> removed without compromising the functionality and debugging feature,
> please share it on the mailing list.
> >
> > On the comment to remove clk.c, I don't agree with it. I think it
> provides an advantage of having a clk module in the DSP Bridge as all the
> calls to clock module are centralized in one location, which would aid in
> Bridge debugging.
> >
> 
> If you mean there is separate clock infrastructure for DSPBridge other
> than clock framework we have for OMAP, then DSPBridge should convert
> to the one we have for OMAP. As recent work by Paul for clock
> framework on OMAP3 and Power domains we need centralized code for clk
> and power domains not separate clock infrastructure for DSPBridge.
> 
> Richard and Paul can probably explain more about OMAP clock framework
> and why it has to be used by DSPBridge too.
> 
> --
> ---Trilok Soni
> http://triloksoni.wordpress.com
> http://www.linkedin.com/in/triloksoni

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

[Felipe Contreras]

On Mon, Aug 18, 2008 at 10:28 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
>> If you mean there is separate clock infrastructure for DSPBridge other
>> than clock framework we have for OMAP, then DSPBridge should convert
>> to the one we have for OMAP. As recent work by Paul for clock
>> framework on OMAP3 and Power domains we need centralized code for clk
>> and power domains not separate clock infrastructure for DSPBridge.
>>
> --- Bridge is not implementing separate clock infrastructure. It is just centralizing the calls to clk_enable in one location as this is called from multiple files in Bridge code. This helps because one can then turn on the traces only for clock module to check if the clocks that were expected to be enabled are enabled or not. As I mentioned before, this helps in debugging.

That only makes the code harder to understand.

#define clk_enable(...) my_debug_function(__VA_ARGS__)

Achieves the same thing.

-- 
Felipe Contreras

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

[Tony Lindgren]

* Trilok Soni <soni.trilok@gmail.com> [080818 20:21]:
> Hi Vijay,
> 
> On Mon, Aug 18, 2008 at 10:42 PM, Pasam, Vijay <vpasam@ti.com> wrote:
> > Hi Hiroshi,
> >
> >> Yes, "dspgateway" won't support OMAP3 but if I remember
> >> correctly that TWL is already in OMAP2? and also does Camera
> >> use the same mechanism of this MMU? This may be benefit or a
> >> must, I guess..
> >
> > I agree. If we are certain that camera driver or some other driver uses
> > MMU the same way bridge does and the MMU architecture is the same, it
> > makes sense to make this generic code. My point is that if DSPGateway
> > doesn't use TWL, it will not benefit from using the Bridge MMU code
> > which has TWL enabled.
> >
> 
> I think recently some one from TI posted OMAP3 camera driver at
> linux-v4l2 mailing list and which I think was using camera MMU, but
> not the common MMU framework it seems from the description. They
> should first start converting to the common MMU infrastructure then.
> 
> http://lists-archives.org/video4linux/23215-omap3-camera-driver.html

We must make the MMU framework generic for the coprocessors, otherwise
maintenance will be a nightmare and the code will never get merged
upstream.

Tony

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

[Tony Lindgren]

* Felipe Contreras <felipe.contreras@gmail.com> [080818 23:09]:
> On Mon, Aug 18, 2008 at 10:28 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> >> If you mean there is separate clock infrastructure for DSPBridge other
> >> than clock framework we have for OMAP, then DSPBridge should convert
> >> to the one we have for OMAP. As recent work by Paul for clock
> >> framework on OMAP3 and Power domains we need centralized code for clk
> >> and power domains not separate clock infrastructure for DSPBridge.
> >>
> > --- Bridge is not implementing separate clock infrastructure. It is just centralizing the calls to clk_enable in one location as this is called from multiple files in Bridge code. This helps because one can then turn on the traces only for clock module to check if the clocks that were expected to be enabled are enabled or not. As I mentioned before, this helps in debugging.
> 
> That only makes the code harder to understand.
> 
> #define clk_enable(...) my_debug_function(__VA_ARGS__)
> 
> Achieves the same thing.

We must use clk_enable() and clk_disable() in the drivers. That's the
Linux clock interface. Anything else won't get merged upstream.

If you need to combine multiple clocks into a single virtual dsp
clock, please register a new custom clock using clk_register().

That allows you to do debugging in the custom clock functions too
if necessary.

Regards,

Tony

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

[Woodruff, Richard]

> > I think recently some one from TI posted OMAP3 camera driver at
> > linux-v4l2 mailing list and which I think was using camera MMU, but
> > not the common MMU framework it seems from the description. They
> > should first start converting to the common MMU infrastructure then.
> >
> > http://lists-archives.org/video4linux/23215-omap3-camera-driver.html
>
> We must make the MMU framework generic for the coprocessors, otherwise
> maintenance will be a nightmare and the code will never get merged
> upstream.

Main issues are at outer interfaces which deal with OS memory system interaction.  The other aspects of the code have been self contained.

The local MMU usages are not maintenance intensive like linux PTE structures as Linux more strongly couples with the hardware table representations.

Seems some of the subsystems like v4l2 do provide local abstractions but there isn't always global ones (vmalloc to sg list for example).

Seems main benefit would be if there were common map conversion and flushing code.

Regards,
Richard W.

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

[Kanigeri, Hari]

Hi All,

> >
> > That only makes the code harder to understand.
> >
> > #define clk_enable(...) my_debug_function(__VA_ARGS__)
> >
> > Achieves the same thing.
> 
> We must use clk_enable() and clk_disable() in the drivers. That's the
> Linux clock interface. Anything else won't get merged upstream.
> 
> If you need to combine multiple clocks into a single virtual dsp
> clock, please register a new custom clock using clk_register().
> 
> That allows you to do debugging in the custom clock functions too
> if necessary.


--- The clock manager in DSP Bridge is in charge of translating the clock requests that come from tasks that are running on DSP to the clk structure that is understandable to the kernel's clk_enable function.

The request for clocks from DSP side of the Bridge comes in the form of the clock ids that are defined in clk.h (The Bridge clk.h file). So, for example, if the DSP task wants GPT6 clocks to be enabled, then a request comes to MPU Bridge with the id for that clock. The Clock module in the MPU Bridge does an internal lookup to translate this id to the proper clock structure and then calls the kernel's clk_enable function with this clock structure as the argument. So, in a way we can call this clock module as the clock manager.

One another reason for centralizing the calls to kernel's clk_enable function in one module is to debug the cases where a task running on the DSP is expecting some clock to be enabled but it might not have been enabled. 

Example: If there is an issue with the load monitoring not working as expected on the DSP, the first thing you may want to check is if the clocks that are used for the load monitoring is enabled or not. The easiest way is just enabling the traces only for clock module and based on that you can tell if the right clocks were enabled or not.  This feature is extremely useful when debugging multimedia test cases, where there are multiple tasks running on DSP sending requests to clocks.

Refer to clk.c and clk.h files in DSP Bridge.

I suggest we keep this clock module in DSP Bridge. May be we can change its name to dsp clock manager if that makes more sense.

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Tony Lindgren [mailto:tony@atomide.com]
> Sent: Tuesday, August 19, 2008 1:24 AM
> To: Felipe Contreras
> Cc: Kanigeri, Hari; 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
> 
> * Felipe Contreras <felipe.contreras@gmail.com> [080818 23:09]:
> > On Mon, Aug 18, 2008 at 10:28 PM, Kanigeri, Hari <h-kanigeri2@ti.com>
> wrote:
> > >> If you mean there is separate clock infrastructure for DSPBridge
> other
> > >> than clock framework we have for OMAP, then DSPBridge should convert
> > >> to the one we have for OMAP. As recent work by Paul for clock
> > >> framework on OMAP3 and Power domains we need centralized code for clk
> > >> and power domains not separate clock infrastructure for DSPBridge.
> > >>
> > > --- Bridge is not implementing separate clock infrastructure. It is
> just centralizing the calls to clk_enable in one location as this is
> called from multiple files in Bridge code. This helps because one can then
> turn on the traces only for clock module to check if the clocks that were
> expected to be enabled are enabled or not. As I mentioned before, this
> helps in debugging.
> >
> > That only makes the code harder to understand.
> >
> > #define clk_enable(...) my_debug_function(__VA_ARGS__)
> >
> > Achieves the same thing.
> 
> We must use clk_enable() and clk_disable() in the drivers. That's the
> Linux clock interface. Anything else won't get merged upstream.
> 
> If you need to combine multiple clocks into a single virtual dsp
> clock, please register a new custom clock using clk_register().
> 
> That allows you to do debugging in the custom clock functions too
> if necessary.
> 
> Regards,
> 
> Tony

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

[Kanigeri, Hari]

> What are those __other__ modules? Could you please list out them so
> that other can help you why direct kernel API won't work there?

--- You can check dsp bridge services folder on omap3/4+ tree for the __other__ modules :). 

CSL, ISR, PRCS modules can be removed. I provided an explanation on why clk.c cannot be removed in previous email. Some time ago I provided you an explanation on why dpc module cannot be replaced with direct calls to tasklets. 

If you have any questions on the rest of the modules please let me know.

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Trilok Soni [mailto:soni.trilok@gmail.com]
> Sent: Monday, August 18, 2008 12:24 PM
> To: Kanigeri, Hari
> Cc: Pasam, Vijay; Hiroshi DOYU; linux-omap@vger.kernel.org; Woodruff,
> Richard; felipe.contreras@gmail.com; siarhei.siamashka@nokia.com; Ramirez
> Luna, Omar; Gupta, Ramesh; tony@atomide.com
> Subject: Re: [RFC][DRAFT] TODO list for TI DSP BRIDGE
> 
> Hi Hari,
> 
> On Mon, Aug 18, 2008 at 9:57 PM, Kanigeri, Hari <h-kanigeri2@ti.com>
> wrote:
> > Hi,
> >
> >> Some of the above items are being looked at like replacing CSL, isr
> etc..
> >> But there are no equivalent API's or functionality available in kernel
> for
> >> many of them. We should replace the API's with the avaolable kernel
> API's
> >> and the remaining need to be looked at from long term.
> >>
> >
> > -- I agree with Vijay. We can remove the simple wrapper functions like
> the ones in CSL and ISR, but some of the other >modules in the services
> provide more functionality than just being the wrappers. We don't want to
> compromise on any >current debugging features that are provided by these
> modules at the expense of replacing the services functions with >direct
> kernel calls. This would require careful analysis.
> 
> What are those __other__ modules? Could you please list out them so
> that other can help you why direct kernel API won't work there?
> 
> Now there are beagleboards out in the market, other developers can
> also help you guys in submitting/cleaning up patches, as far as there
> is no duplicate work going on and regular visibility on work done by
> TI/Nokia on Bridge internally.
> 
> 
> --
> ---Trilok Soni
> http://triloksoni.wordpress.com
> http://www.linkedin.com/in/triloksoni

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

[Felipe Contreras]

On Tue, Aug 19, 2008 at 9:24 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> Hi All,
>
>> >
>> > That only makes the code harder to understand.
>> >
>> > #define clk_enable(...) my_debug_function(__VA_ARGS__)
>> >
>> > Achieves the same thing.
>>
>> We must use clk_enable() and clk_disable() in the drivers. That's the
>> Linux clock interface. Anything else won't get merged upstream.
>>
>> If you need to combine multiple clocks into a single virtual dsp
>> clock, please register a new custom clock using clk_register().
>>
>> That allows you to do debugging in the custom clock functions too
>> if necessary.
>
>
> --- The clock manager in DSP Bridge is in charge of translating the clock requests that come from tasks that are running on DSP to the clk structure that is understandable to the kernel's clk_enable function.
>
> The request for clocks from DSP side of the Bridge comes in the form of the clock ids that are defined in clk.h (The Bridge clk.h file). So, for example, if the DSP task wants GPT6 clocks to be enabled, then a request comes to MPU Bridge with the id for that clock. The Clock module in the MPU Bridge does an internal lookup to translate this id to the proper clock structure and then calls the kernel's clk_enable function with this clock structure as the argument. So, in a way we can call this clock module as the clock manager.

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.

> One another reason for centralizing the calls to kernel's clk_enable function in one module is to debug the cases where a task running on the DSP is expecting some clock to be enabled but it might not have been enabled.
>
> Example: If there is an issue with the load monitoring not working as expected on the DSP, the first thing you may want to check is if the clocks that are used for the load monitoring is enabled or not. The easiest way is just enabling the traces only for clock module and based on that you can tell if the right clocks were enabled or not.  This feature is extremely useful when debugging multimedia test cases, where there are multiple tasks running on DSP sending requests to clocks.

Didn't Tony just explained how to do that without a wrapper?

Best regards.

-- 
Felipe Contreras

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

[Kanigeri, Hari]

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. 
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. 
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 :). 

> 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.

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> Sent: Tuesday, August 19, 2008 2:10 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
> 
> On Tue, Aug 19, 2008 at 9:24 PM, Kanigeri, Hari <h-kanigeri2@ti.com>
> wrote:
> > Hi All,
> >
> >> >
> >> > That only makes the code harder to understand.
> >> >
> >> > #define clk_enable(...) my_debug_function(__VA_ARGS__)
> >> >
> >> > Achieves the same thing.
> >>
> >> We must use clk_enable() and clk_disable() in the drivers. That's the
> >> Linux clock interface. Anything else won't get merged upstream.
> >>
> >> If you need to combine multiple clocks into a single virtual dsp
> >> clock, please register a new custom clock using clk_register().
> >>
> >> That allows you to do debugging in the custom clock functions too
> >> if necessary.
> >
> >
> > --- The clock manager in DSP Bridge is in charge of translating the
> clock requests that come from tasks that are running on DSP to the clk
> structure that is understandable to the kernel's clk_enable function.
> >
> > The request for clocks from DSP side of the Bridge comes in the form of
> the clock ids that are defined in clk.h (The Bridge clk.h file). So, for
> example, if the DSP task wants GPT6 clocks to be enabled, then a request
> comes to MPU Bridge with the id for that clock. The Clock module in the
> MPU Bridge does an internal lookup to translate this id to the proper
> clock structure and then calls the kernel's clk_enable function with this
> clock structure as the argument. So, in a way we can call this clock
> module as the clock manager.
> 
> 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.
> 
> > One another reason for centralizing the calls to kernel's clk_enable
> function in one module is to debug the cases where a task running on the
> DSP is expecting some clock to be enabled but it might not have been
> enabled.
> >
> > Example: If there is an issue with the load monitoring not working as
> expected on the DSP, the first thing you may want to check is if the
> clocks that are used for the load monitoring is enabled or not. The
> easiest way is just enabling the traces only for clock module and based on
> that you can tell if the right clocks were enabled or not.  This feature
> is extremely useful when debugging multimedia test cases, where there are
> multiple tasks running on DSP sending requests to clocks.
> 
> Didn't Tony just explained how to do that without a wrapper?
> 
> Best regards.
> 
> --
> Felipe Contreras

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

[Felipe Contreras]

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

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

[Hiroshi DOYU]

From: "ext Tony Lindgren" <tony@atomide.com>
Subject: Re: [RFC][DRAFT] TODO list for TI DSP BRIDGE
Date: Tue, 19 Aug 2008 09:24:23 +0300

> * Felipe Contreras <felipe.contreras@gmail.com> [080818 23:09]:
> > On Mon, Aug 18, 2008 at 10:28 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> > >> If you mean there is separate clock infrastructure for DSPBridge other
> > >> than clock framework we have for OMAP, then DSPBridge should convert
> > >> to the one we have for OMAP. As recent work by Paul for clock
> > >> framework on OMAP3 and Power domains we need centralized code for clk
> > >> and power domains not separate clock infrastructure for DSPBridge.
> > >>
> > > --- Bridge is not implementing separate clock infrastructure. It is just centralizing the calls to clk_enable in one location as this is called from multiple files in Bridge code. This helps because one can then turn on the traces only for clock module to check if the clocks that were expected to be enabled are enabled or not. As I mentioned before, this helps in debugging.
> > 
> > That only makes the code harder to understand.
> > 
> > #define clk_enable(...) my_debug_function(__VA_ARGS__)
> > 
> > Achieves the same thing.
> 
> We must use clk_enable() and clk_disable() in the drivers. That's the
> Linux clock interface. Anything else won't get merged upstream.
> 
> If you need to combine multiple clocks into a single virtual dsp
> clock, please register a new custom clock using clk_register().
> 
> That allows you to do debugging in the custom clock functions too
> if necessary.

I sent some patches based on the above suggestion.

  http://marc.info/?l=linux-omap&m=121922597019873&w=2
  http://marc.info/?l=linux-omap&m=121922593919766&w=2

I think that this virtual clock may allow any arbitrary combination of
clocks and it may be possible to have multiple hierarchy of virtual
clocks like:

   dsp_clocks
   |
   |-- peripheral_clocks
   |     |
   |     |-- gpt5
   |     |    |- gpt5_ick
   |     |    `- gpt5_fck
   |
   |-- ....
   ..

Any comments would be appreciated.

    Hiroshi DOYU

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

[Woodruff, Richard]

Hi Hiroshi,

> I sent some patches based on the above suggestion.
>
>   http://marc.info/?l=linux-omap&m=121922597019873&w=2
>   http://marc.info/?l=linux-omap&m=121922593919766&w=2
>
> I think that this virtual clock may allow any arbitrary combination of
> clocks and it may be possible to have multiple hierarchy of virtual
> clocks like:
>
>    dsp_clocks
>    |
>    |-- peripheral_clocks
>    |     |
>    |     |-- gpt5
>    |     |    |- gpt5_ick
>    |     |    `- gpt5_fck
>    |
>    |-- ....
>    ..
>
> Any comments would be appreciated.

A few quick comments:

- First off, I do like the idea of virtual clocks for grouping.  I did do this for OMAP2 for completely internal and dependent root clocks.  I also thought it might be nice to extend clock frame work to allow external clock registration (say for a codec chip).  In our local tree I did even add it.  One main issue was the internal clock structure is supposed to be opaque and you need to set internals to add a clock node.

- I worry a bit about grouping of internal resources which may be allocated differently per implementation.  Perhaps a board specific resource can handle it, but you've not included that in the patch.

- Where is the board specific side of this call?  I see clock calls added to clock.c but not resources added to mach-omap2/xyz-board.c.

- You're not preserving the enable error path back to the virtual clock.  A recovery from a failure would be messy.

- Will these clocks always be handled as a group?  If I were to try and optimize I might shut down a gpt5 clock individually to allow a PER domain to power off.  The DSP should aggressively be letting go of resource when it doesn't need them to allow power to work.  1 virtual resource with all related clocks would not be right.

If you wanted grouping their might be like DSP-CORE, DSP-Streaming-Audio, Dsp-...  You will want better granularity.  So the enable/disables are effective for power savings.  This is especially true if your not even using say a McBSP and are doing something like JPEG decode.


Regards,
Richard W.

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

[Sakari Ailus]

Hi,

ext Woodruff, Richard wrote:
>>> I think recently some one from TI posted OMAP3 camera driver at 
>>> linux-v4l2 mailing list and which I think was using camera MMU,
>>> but not the common MMU framework it seems from the description.
>>> They should first start converting to the common MMU
>>> infrastructure then.
>>> 
>>> http://lists-archives.org/video4linux/23215-omap3-camera-driver.html
>>> 
>> We must make the MMU framework generic for the coprocessors,
>> otherwise maintenance will be a nightmare and the code will never
>> get merged upstream.

I agree with Tony.

Many implementations are more likely have more bugs than one, too.

> Main issues are at outer interfaces which deal with OS memory system
> interaction.  The other aspects of the code have been self contained.
> 
> 
> The local MMU usages are not maintenance intensive like linux PTE
> structures as Linux more strongly couples with the hardware table
> representations.
> 
> Seems some of the subsystems like v4l2 do provide local abstractions
> but there isn't always global ones (vmalloc to sg list for example).

The V4L2 provides functions for mapping the video buffers to kernel 
memory, but that's not the point. The OMAP 3 ISP driver for example 
would benefit from common MMU framework directly as it just wants to map 
the video buffers continuously to some address and sometimes flush them.

Creating page tables for the mappings is necessary in practice.

This sounds very much like something that would be useful for a number 
of drivers for OMAP 3 in addition to the ISP driver.

If there was a generic way for asking the MMU to map the buffer it
would be simple for the ISP driver to use that.

(Besides, it'd be easy to add MMU support for the OMAP 2 camera driver 
as well if there was a generic framework. Aren't the MMUs similar or 
even the same?? :))

(I'm adding Mohit Jalori to Cc:.)

Regards,

-- 
Sakari Ailus
sakari.ailus@nokia.com

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

[Kanigeri, Hari]

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

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

[Hiroshi DOYU]

From: "ext Woodruff, Richard" <r-woodruff2@ti.com>
Subject: RE: [RFC][DRAFT] TODO list for TI DSP BRIDGE
Date: Wed, 20 Aug 2008 09:32:32 -0500

> Hi Hiroshi,
> 
> > I sent some patches based on the above suggestion.
> >
> >   http://marc.info/?l=linux-omap&m=121922597019873&w=2
> >   http://marc.info/?l=linux-omap&m=121922593919766&w=2
> >
> > I think that this virtual clock may allow any arbitrary combination of
> > clocks and it may be possible to have multiple hierarchy of virtual
> > clocks like:
> >
> >    dsp_clocks
> >    |
> >    |-- peripheral_clocks
> >    |     |
> >    |     |-- gpt5
> >    |     |    |- gpt5_ick
> >    |     |    `- gpt5_fck
> >    |
> >    |-- ....
> >    ..
> >
> > Any comments would be appreciated.
> 
> A few quick comments:
> 
> - First off, I do like the idea of virtual clocks for grouping.  I
> did do this for OMAP2 for completely internal and dependent root
> clocks.  I also thought it might be nice to extend clock frame work
> to allow external clock registration (say for a codec chip).  In our
> local tree I did even add it.  One main issue was the internal clock
> structure is supposed to be opaque and you need to set internals to
> add a clock node.

Can I find your OMAP2 implementation in omapzoom for virtual clock?

> - I worry a bit about grouping of internal resources which may be
> allocated differently per implementation.  Perhaps a board specific
> resource can handle it, but you've not included that in the patch.
> 
> - Where is the board specific side of this call?  I see clock calls
> added to clock.c but not resources added to mach-omap2/xyz-board.c.

One example is "arch/arm/mach-omap2/mcbsp.c" in the second patch and I
think that board specific grouping is also expected.

> - You're not preserving the enable error path back to the virtual
> clock.  A recovery from a failure would be messy.

Right, this is updated in the latest patch.

> - Will these clocks always be handled as a group?  If I were to try
> and optimize I might shut down a gpt5 clock individually to allow a
> PER domain to power off.  The DSP should aggressively be letting go
> of resource when it doesn't need them to allow power to work.  1
> virtual resource with all related clocks would not be right.

If this is the case, vclk wouldn't work so nicely.

> If you wanted grouping their might be like DSP-CORE,
> DSP-Streaming-Audio, Dsp-...  You will want better granularity.  So
> the enable/disables are effective for power savings.  This is
> especially true if your not even using say a McBSP and are doing
> something like JPEG decode.

The above grouping proposal sounds quite promissing to me. In any
case, how to group up dsp clocks is tightly coupled with dsp system
operating policy. It's bit hard to guess it just from the bridge code
now.

Thank you for your comments.

      Hiroshi DOYU

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

[Felipe Contreras]

On Thu, Aug 21, 2008 at 1:34 AM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> 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 ?

That depends on what is left of clk.c after reorganization.

>> 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 ;).

Did you read what I wrote?

>> 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);

So .intClk wouldn't be "enum SERVICES_ClkId", but "struct clk".

> 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.

If you see clk_enable there's nothing more to follow, so it's not hard
to follow. Others explained why virtual clocks make sense, which would
make the code even easier to follow.

The only two places where clock functions are used are tiomap3430.c
and tiomap3430_pwr.c. In tiomap3430.c, clk_enable functions can be
used directly, there's no translation needed, and I already explained
before, you can add debugging quite easily, so no need of clk.c for
that. In tiomap3430_pwr.c the translation can happen in
DSPPeripheralClkCtrl already, so again, no need of clk.c.

If you reorganize the code this way the only thing left in clk.c would
be clock initialization. I don't think it makes sense to keep one file
for one small function which would be even smaller with virtual
clocks.

Best regards.

-- 
Felipe Contreras

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

[Woodruff, Richard]

> From: Hiroshi DOYU [mailto:Hiroshi.DOYU@nokia.com]
...
> > A few quick comments:
> >
> > - First off, I do like the idea of virtual clocks for grouping.  I
> > did do this for OMAP2 for completely internal and dependent root
> > clocks.  I also thought it might be nice to extend clock frame work
> > to allow external clock registration (say for a codec chip).  In our
> > local tree I did even add it.  One main issue was the internal clock
> > structure is supposed to be opaque and you need to set internals to
> > add a clock node.
>
> Can I find your OMAP2 implementation in omapzoom for virtual clock?

Guess not.  The flag for OMAP2 is still there in linux-omap clock.h for virt_prcm_set but the implementation of it evolved to just using custom recalc/rate/round functions.  Today the flag just would tell a user something is funny about the clock but no rules are enforced.

Actually in looking at this does bring up another very relevant question.

For a combined clock especially like the IVA-system, how do you do set_rate/round_rate/get_rate/set_parent so it is meaningful?

For virt_prcm_set the MPU clock rate is used as in index.  For OMAP2 for most cases there are very strict rules about speed ratio's. As such if you know the speed for 1 clock you can set the group.  mpu_rate isn't perfect but it is good enough.

You need operations for more than enable/disable.

How do you set parent to your timer in your example?  How do you query individual other clocks rate?

If you need to do actions on individuals in the group you still need to grab handles for all of them.  This puts you back where you started before exploring the idea.

Regards,
Richard W.

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

[Kanigeri, Hari]

Hi Doyu-san,

> I think that this virtual clock may allow any arbitrary combination of
> clocks and it may be possible to have multiple hierarchy of virtual
> clocks like:
> 
>    dsp_clocks
>    |
>    |-- peripheral_clocks
>    |     |
>    |     |-- gpt5
>    |     |    |- gpt5_ick
>    |     |    `- gpt5_fck
>    |
>    |-- ....
>    ..
> 
> Any comments would be appreciated.


The feature looks good, but I think we cannot use this in DSP Bridge for following reasons.

This implementation is under the assumption that we will either enable or disable all the registered Bridge Peripherals clocks, which is not the case in reality.

In Bridge, we cannot make the assumption that all the registered clocks will be enabled. Bridge cannot use the vclk_enable function because this will enable all the registered clocks, which is not accepted. The clocks should be enabled only based on the request from DSP...it is use case driven.

Similarly, we cannot call vclk_disable(struct clk *clk) as this will result in calling clk_disable for the clocks that might not have been enabled.

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Hiroshi DOYU [mailto:Hiroshi.DOYU@nokia.com]
> Sent: Wednesday, August 20, 2008 5:11 AM
> To: tony@atomide.com
> Cc: felipe.contreras@gmail.com; Kanigeri, Hari; soni.trilok@gmail.com;
> Pasam, Vijay; 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
> 
> From: "ext Tony Lindgren" <tony@atomide.com>
> Subject: Re: [RFC][DRAFT] TODO list for TI DSP BRIDGE
> Date: Tue, 19 Aug 2008 09:24:23 +0300
> 
> > * Felipe Contreras <felipe.contreras@gmail.com> [080818 23:09]:
> > > On Mon, Aug 18, 2008 at 10:28 PM, Kanigeri, Hari <h-kanigeri2@ti.com>
> wrote:
> > > >> If you mean there is separate clock infrastructure for DSPBridge
> other
> > > >> than clock framework we have for OMAP, then DSPBridge should
> convert
> > > >> to the one we have for OMAP. As recent work by Paul for clock
> > > >> framework on OMAP3 and Power domains we need centralized code for
> clk
> > > >> and power domains not separate clock infrastructure for DSPBridge.
> > > >>
> > > > --- Bridge is not implementing separate clock infrastructure. It is
> just centralizing the calls to clk_enable in one location as this is
> called from multiple files in Bridge code. This helps because one can then
> turn on the traces only for clock module to check if the clocks that were
> expected to be enabled are enabled or not. As I mentioned before, this
> helps in debugging.
> > >
> > > That only makes the code harder to understand.
> > >
> > > #define clk_enable(...) my_debug_function(__VA_ARGS__)
> > >
> > > Achieves the same thing.
> >
> > We must use clk_enable() and clk_disable() in the drivers. That's the
> > Linux clock interface. Anything else won't get merged upstream.
> >
> > If you need to combine multiple clocks into a single virtual dsp
> > clock, please register a new custom clock using clk_register().
> >
> > That allows you to do debugging in the custom clock functions too
> > if necessary.
> 
> I sent some patches based on the above suggestion.
> 
>   http://marc.info/?l=linux-omap&m=121922597019873&w=2
>   http://marc.info/?l=linux-omap&m=121922593919766&w=2
> 
> I think that this virtual clock may allow any arbitrary combination of
> clocks and it may be possible to have multiple hierarchy of virtual
> clocks like:
> 
>    dsp_clocks
>    |
>    |-- peripheral_clocks
>    |     |
>    |     |-- gpt5
>    |     |    |- gpt5_ick
>    |     |    `- gpt5_fck
>    |
>    |-- ....
>    ..
> 
> Any comments would be appreciated.
> 
>     Hiroshi DOYU
> 

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

[Hiroshi DOYU]

Hi Hari,

From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
Subject: RE: [RFC][DRAFT] TODO list for TI DSP BRIDGE
Date: Thu, 21 Aug 2008 10:37:12 -0500

> Hi Doyu-san,
> 
> > I think that this virtual clock may allow any arbitrary combination of
> > clocks and it may be possible to have multiple hierarchy of virtual
> > clocks like:
> > 
> >    dsp_clocks
> >    |
> >    |-- peripheral_clocks
> >    |     |
> >    |     |-- gpt5
> >    |     |    |- gpt5_ick
> >    |     |    `- gpt5_fck
> >    |
> >    |-- ....
> >    ..
> > 
> > Any comments would be appreciated.
> 
> 
> The feature looks good, but I think we cannot use this in DSP Bridge
> for following reasons.
> 
> This implementation is under the assumption that we will either
> enable or disable all the registered Bridge Peripherals clocks,
> which is not the case in reality.
> 
> In Bridge, we cannot make the assumption that all the registered
> clocks will be enabled. Bridge cannot use the vclk_enable function
> because this will enable all the registered clocks, which is not
> accepted. The clocks should be enabled only based on the request
> from DSP...it is use case driven.

Agreed, thinking about the above use case(operation).

	Hiroshi DOYU

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

[Hiroshi DOYU]

Hi Richard,

From: "ext Woodruff, Richard" <r-woodruff2@ti.com>
Subject: RE: [RFC][DRAFT] TODO list for TI DSP BRIDGE
Date: Thu, 21 Aug 2008 07:34:56 -0500

> 
> > From: Hiroshi DOYU [mailto:Hiroshi.DOYU@nokia.com]
> ...
> > > A few quick comments:
> > >
> > > - First off, I do like the idea of virtual clocks for grouping.  I
> > > did do this for OMAP2 for completely internal and dependent root
> > > clocks.  I also thought it might be nice to extend clock frame work
> > > to allow external clock registration (say for a codec chip).  In our
> > > local tree I did even add it.  One main issue was the internal clock
> > > structure is supposed to be opaque and you need to set internals to
> > > add a clock node.
> >
> > Can I find your OMAP2 implementation in omapzoom for virtual clock?
> 
> Guess not.  The flag for OMAP2 is still there in linux-omap clock.h
> for virt_prcm_set but the implementation of it evolved to just using
> custom recalc/rate/round functions.  Today the flag just would tell
> a user something is funny about the clock but no rules are enforced.
> 
> Actually in looking at this does bring up another very relevant
> question.
> 
> For a combined clock especially like the IVA-system, how do you do
> set_rate/round_rate/get_rate/set_parent so it is meaningful?
>
> For virt_prcm_set the MPU clock rate is used as in index.  For OMAP2
> for most cases there are very strict rules about speed ratio's. As
> such if you know the speed for 1 clock you can set the group.
> mpu_rate isn't perfect but it is good enough.
> 
> You need operations for more than enable/disable.
> 
> How do you set parent to your timer in your example?  How do you
> query individual other clocks rate?

I think that, in this virtual clock, frequency changing will be done
in the totally different path(just in a normal clock tree). This
virtual clock resides in an independent clock tree and supposed to be
used aggressively just to turn on/off clocks(enable/disable), which
drivers use, for saving power consumption. So the relationship between
"clk" and "vclk" may be similar to "struct device" and "struct class"
in the linux driver-model, which means that, the former is based on
H/W connection and the latter is based on functionality. Of course
there is a possibility that other clk methods can be overwritten, but
it's has to be used for different purpose than a normal clock does,
not to break the H/W clock tree consistency.

Also, making a connection among virtual clocks(parenting) can be done
by overwriting "set_parent" for vclk, which is original set NULL, if
necessary.

A virtual clock is composed of clocks which are called "child", but
they are not children actually, from the clock framework point of
view. These clocks reside in a H/W clock tree and a virtual clock just
provide operation to handle them at once, from completely independent
(virtual)clock (tree), for device driver.

In the previous example,

>    dsp_clocks(v)
>    |
>    |-- peripheral_clocks(v)
>    |     |
>    |     |-- gpt5(v)
>    |     |    |- gpt5_ick(r)
>    |     |    `- gpt5_fck(r)
>    |
>    |-- ....
>    ..

Virtually we can make the connection between "dsp_clocks",
"peripheral_clocks" and "gpt5", if we overwrite set_parent for
vclk. but there's no connection between gpt5(virt) and gpt5_*(real)
from the clock framework point of view.

(v) -> (v) OK
(r) -> (r) OK
(v) -> (r) NG : cause inconsistency on h/w clock tree.

v: virtual clock
r: real clock
some flags can be used to prevent the above inconsistency.

Probably original "virt_prcm_set" might have a wider solution,
though....

      Hiroshi DOYU

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

[Woodruff, Richard]

Hi Hiroshi,

> -----Original Message-----
> From: Hiroshi DOYU [mailto:Hiroshi.DOYU@nokia.com]

<snip>

> I think that, in this virtual clock, frequency changing will be done
> in the totally different path(just in a normal clock tree). This
> virtual clock resides in an independent clock tree and supposed to be
> used aggressively just to turn on/off clocks(enable/disable), which
> drivers use, for saving power consumption. So the relationship between
> "clk" and "vclk" may be similar to "struct device" and "struct class"
> in the linux driver-model, which means that, the former is based on
> H/W connection and the latter is based on functionality. Of course
> there is a possibility that other clk methods can be overwritten, but
> it's has to be used for different purpose than a normal clock does,
> not to break the H/W clock tree consistency.

Ok. I 'think' I better understand your idea.  Thanks for the description.

To me it seems like it could be slightly confusing to maintain 2 clock structures.  One for v and one for p.  In the end it is probably not very hard to lean.

It would reduce some amount of code in the more complex subsystems/drivers resulting in a cleaner driver.

The interface used in drivers with the clock frame work is pretty simple so it might seem more about clean up then reducing complexity in the driver.

Do you feel a lot of drivers will benefit or it will expand to auto handle resources better?  If it is just 1 or 2 drivers then it might be the new code in clock framework and platform will add some complexity which is higher than what was removed from the driver.  If lots of drivers use it then it would be positive.

<snip>

> Probably original "virt_prcm_set" might have a wider solution,
> though....

No not really.  It just provided a way to group dependent clocks.  These clocks were so tightly coupled it was better to treat them as one.

It does have some nice properties for OMAP2 which can be exploited.  For instance the underlying pclock speeds in a vclock set are fixed by ratio.  So doing a clock set on the vclock will allow switching between one of the supported ratio sets.  Doing a clock set on the pclock(dpll) will allow you to change the base speed which the ratio is applied.

Regards,
Richard W.

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

[Hiroshi DOYU]

Hi Richard,

From: "ext Woodruff, Richard" <r-woodruff2@ti.com>
Subject: RE: [RFC][DRAFT] TODO list for TI DSP BRIDGE
Date: Thu, 21 Aug 2008 14:43:34 -0500

> Hi Hiroshi,
> 
> > -----Original Message-----
> > From: Hiroshi DOYU [mailto:Hiroshi.DOYU@nokia.com]
> 
> <snip>
> 
> > I think that, in this virtual clock, frequency changing will be done
> > in the totally different path(just in a normal clock tree). This
> > virtual clock resides in an independent clock tree and supposed to be
> > used aggressively just to turn on/off clocks(enable/disable), which
> > drivers use, for saving power consumption. So the relationship between
> > "clk" and "vclk" may be similar to "struct device" and "struct class"
> > in the linux driver-model, which means that, the former is based on
> > H/W connection and the latter is based on functionality. Of course
> > there is a possibility that other clk methods can be overwritten, but
> > it's has to be used for different purpose than a normal clock does,
> > not to break the H/W clock tree consistency.
> 
> Ok. I 'think' I better understand your idea.  Thanks for the description.
> 
> To me it seems like it could be slightly confusing to maintain 2
> clock structures.  One for v and one for p.  In the end it is
> probably not very hard to lean.
> 
> It would reduce some amount of code in the more complex
> subsystems/drivers resulting in a cleaner driver.
> 
> The interface used in drivers with the clock frame work is pretty
> simple so it might seem more about clean up then reducing complexity
> in the driver.
> 
> Do you feel a lot of drivers will benefit or it will expand to auto
> handle resources better?  If it is just 1 or 2 drivers then it might
> be the new code in clock framework and platform will add some
> complexity which is higher than what was removed from the driver.
> If lots of drivers use it then it would be positive.

I have no idea right now about how much drivers and the feature
evolution. But basically I feel positive because for now this won't
affect the existing clock tree(or framework) at all and this is just a
helper function for driver's clock initialization and can be
considered as a kind of independent add-on feature from the clock
framework perspective.

I agree that there is a possibility that some smart guy can extend
this to handle all device driver related resources(platform
dependent?) together.

	Hiroshi DOYU