[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Tivy, Robert]

> -----Original Message-----
> From: Chemparathy, Cyril
> Sent: Monday, December 03, 2012 12:13 PM
> To: Nori, Sekhar
> Cc: Tivy, Robert; davinci-linux-open-source at linux.davincidsp.com;
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for
> OMAP-L138 DSP
> 
> On 12/03/2012 09:41 AM, Sekhar Nori wrote:
> > Hi Rob,
> >
> > On 12/1/2012 7:41 AM, Tivy, Robert wrote:
> >> Hi Sekhar,
> >>
> >>> -----Original Message-----
> >>> From: Nori, Sekhar
> >>> Sent: Friday, November 30, 2012 2:51 AM
> >>> To: Tivy, Robert
> >>> Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm-
> >>> kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
> >>> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support
> >>> for
> >>> OMAP-L138 DSP
> >>>
> >>> Hi Rob,
> >>>
> >>> On 11/29/2012 7:08 AM, Tivy, Robert wrote:
> >>>> Hi Sekhar,
> >>>>
> >>>> Please see comments and noob questions below...
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Nori, Sekhar
> >>>>> Sent: Tuesday, November 20, 2012 4:27 AM
> >>>>> To: Tivy, Robert
> >>>>> Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm-
> >>>>> kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
> >>>>> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board
> support
> >>>>> for
> >>>>> OMAP-L138 DSP
> >>>>>
> >>>>> On 11/14/2012 6:03 AM, Robert Tivy wrote:
> >>>>>> Requires CMA services.
> >>>>>>
> >>>>>> New 'clk_reset()' API added so that the DSP's reset state can be
> >>>>>> toggled separately from its enable/disable operation.
> >>>>>
> >>>>> But we cannot add a private clk_ API without it being defined in
> >>>>> include/linux/clk.h. So, this requires wider alignment.
> >>>>>
> >>>>> And I am not sure clock API should be extended to support reset
> >>> since
> >>>>> "resetting a clock" does not make a lot of sense. On DaVinci,
> >>>>> clock gating and reset are handled by the same module, but that
> >>>>> need not always be the case.
> >>>>>
> >>>>> What you need is a way to reset an IP. I do not know of an
> >>>>> existing framework to do this; likely a new API needs to be
> >>>>> created. I am guessing this never existed so far because most of
> >>>>> the IPs can be reset internally (by writing a bit within its own
> >>>>> register space). I guess DSP is different since its actually a
> >>>>> co-processor and may not have such a bit.
> >>>>>
> >>>>> How about simulating a reset by making the DSP jump to its reset
> >>>>> address. While I am sure its not quite the same as resetting the
> >>>>> DSP using PSC, may be it could be used while you wait for
> >>>>> consensus around handling module reset in kernel?
> >>>>
> >>>> Since the ARM can't write the DSP's program counter I suspect such
> >>>> a
> >>> temporary solution is not possible.
> >>>
> >>> Okay. I think the way forward on this is to start a separate thread
> >>> including the ARM list, LKML and the remoteproc folks to see if it
> >>> makes sense to create a reset API in kernel. You can describe the
> >>> usecase you have.
> >>
> >> Instead of fighting that fight, I thought of another way.  The
> da8xx_dsp platform_device has private data registered in its device
> struct.  This private data can contain a function pointer for a "DSP
> reset" function, and davinci_remoteproc.c can call the registered
> function.  Does that sound OK?
> >
> > Passing function pointers from platform code will not work when
> > converting to device tree (DT). DA850 will gain DT boot with v3.8 and
> > there is work ongoing on converting drivers to be DT-aware. Adding a
> > new driver which is DT-incompatible will not be right. Hence, I will
> > not recommend this now.
> >
> 
> Not sure if this solves your problem, but you could add a new clock
> type
> (PSC_LRESET?) as a child under the PSC clock node for the DSP.
> Something like:
> 
>    |
>    +-- PSC x (DSP0 clock)
>    |    |
>    |    +-- PSC-LRESET x (DSP0 reset control)
>    |
>    +-- PSC y (DSP1 clock)
>    |    |
>    |    +-- PSC-LRESET y (DSP1 reset control)
>    |
>    +-- PSC z (DSP2 clock)
>    |    |
>    |    +-- PSC-LRESET z (DSP2 reset control)
> 
> 
> The idea here being that if you enable the PSC clocks, you enable the
> clock gates, but leave the DSPs in reset.  On the other hand, if you
> enable the reset clock, the code implicitly enables the gate (via
> parent), and takes the DSP out of reset as well.
> 
> This is not quite the cleanest way to do it, considering that reset
> lines have no business in the clock tree, but then, this is probably
> the simplest way to fit in.  Thoughts?
> 
> BTW, we have the same situation on Keystone.
> 
> Thanks
> -- Cyril.

Cyril,

Thankyou for the good suggestion.

In trying it out to see if it would work I ran into an issue where the clk_register() function failed when called for the new child clock, complaining that its parent has no rate.  This is true, since the parent was previously just a leaf and handled by the PSC (and the parent's parent is a CLK_PLL clock).

So, I would need to add code to arch/arm/mach-davinci/clock.c that prevents rate operations from being performed on a clock of this PSC_LRESET type (as well as code that allows for no "rate" or "parent->rate" to be associated with it, instead of failing).  I can modify clock.c to allow for this new clock type in this way, unless you have a better suggestion (I'm not too thrilled about changing clock.c, seems I should be able to live within its framework).

Regards,

- Rob