Re: [PATCH 1/3] clk: exynos-audss: Keep the parent of mout_audss always enabled

[Mike Turquette]

Quoting Tushar Behera (2014-07-10 23:18:54)
> On 06/13/2014 02:39 AM, Mike Turquette wrote:
> > Quoting Tushar Behera (2014-06-12 00:29:23)
> >> On Wed, Jun 11, 2014 at 10:20 PM, Mike Turquette <mturquette@xxxxxxxxxx> wrote:
> >>> Quoting Tushar Behera (2014-06-10 22:32:17)
> >>>> When the output clock of AUDSS mux is disabled, we are getting kernel
> >>>> oops while doing a clk_get() on other clocks provided by AUDSS. Though
> >>>> user manual doesn't specify this dependency, we came across this issue
> >>>> while disabling the parent of AUDSS mux clocks.
> >>>
> >>> Hi Tushar,
> >>>
> >>> Can you help me understand better what the actual problem is? What is
> >>> the root cause of the kernel oops?
> >>
> >> Currently AUDSS mux has two parents, XXTI crystal and MAU_EPLL clock.
> >> As per observation, when the output of AUDSS mux is gated, we are not
> >> able to do any operation on the clocks provided by MAU block (mostly
> >> the clocks used by ADMA and audio blocks).
> > 
> > I tried to get a datasheet for Exynos 54xx but could not find it. I even
> > looked at the public 5250 data sheet, but it is completely missing
> > Chapter 34, "Audio Subsystem", which apparently contains Figure 34-3,
> > "Clock names and clock tree diagram of MAUDIO_BLK".
> > 
> > So without any clue about your hardware (not for lack of trying) I would
> > guess that somewhere in the parent hierarchy you have an interface clock
> > which must be enabled in order for you to touch the registers pertaining
> > to the downstream audio clocks.
> > 
> 
> Yes, right. As per observation, we need to keep the output of AUDSS mux
> enabled to access the registers present in MAU block.
> 
> > The right way to handle this requires two steps:
> > 
> > 1) model your interface clock in the Linux clock framework if you
> > haven't already (I assume it is a gate clock, or the child of a gate
> > clock)
> > 
> 
> The interface clock is already part of the clock framework.

Great!

> 
> > 2) the clk_ops callbacks for the affected audio clocks should wrap their
> > operations (i.e. critical secion) with a clk_enable/clk_disable pair,
> > where the clock being enables/disable is the interface clock mentioned
> > above in #1
> > 
> > The CCF is reentrant, so you can do this by simply using the top-level
> > clk.h API from within your clk_ops callbacks.
> > 
> 
> Right now, the clocks are registered with clk_register_mux,
> clk_register_div and clk_register_gate calls which in turn set
> appropriate clk_ops callbacks. If I need to wrap the register access
> during these clk_ops callbacks with clk_enable/clk_disable of interface
> lock, I would have to reimplement the clk_ops callbacks in
> clk-exynos-audss driver.
> 
> Is that the approach that you are suggesting?

Is there another way? This is one of the reasons why I don't like the
basic clock types being subclassed by clock drivers. It's a brittle
design that tends to fall over as soon as the basic clock types don't do
what you need. And don't even get me started on how ugly clk-composite.c
is! ;-)

One hack you can do is to subclass the clk_ops for each of the basic
clock types you use and add .prepare and .unprepare callbacks to them
which enable/disable the interface clock. Some examples of this are
merged and it may be what your clock driver does already. However this
may not be very optimal if your consumer driver simply calls
clk_prepare_enable at probe-time and never turns the interface clock off
again...

> 
> > I might be totally wrong about the cause of the hang, but that's my best
> > guess based on everyone's bug reports.
> > 
> 
> There are 5 gate clocks within MAU block. While disabling the unused
> clocks, if CLK_MAU_EPLL is disabled first, then we are getting this
> system hang.

Right. Then your accesses to the clock control register need to be
protected by a clk_enable/clk_disable critical section.

Regards,
Mike

> 
> 
> > Regards,
> > Mike
> > 
> >>
> >>>
> >>> You mention calling clk_get on child clocks of the AUDSS mux fails, but
> >>> I cannot imagine why. How can the enable/disable state of a clock affect
> >>> the ability to clk_get other clocks?
> >>>
> >>
> >> I might have a little vogue while updating the commit message
> >> (mentioning about clk_get which surely is only a s/w operation), but
> >> there is definitely some issue with handling those clocks under given
> >> scenario.
> >>
> >> I am on leave till end of this week, so I will update you more with
> >> the logs on Monday.
> >>
> >> Thanks,
> >> --
> >> Tushar
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
> 
> -- 
> Tushar Behera
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/