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

Commit Message

Tushar Behera June 11, 2014, 5:32 a.m. UTC

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.

Keeping the parents of AUDSS mux always enabled fixes this issue.

Signed-off-by: Tushar Behera <[e-mail]>
Signed-off-by: Shaik Ameer Basha <[e-mail]>
---
 drivers/clk/samsung/clk-exynos-audss.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Javier Martinez Canillas June 11, 2014, 3:58 p.m. UTC | #1

On Wed, Jun 11, 2014 at 7:32 AM, Tushar Behera <[e-mail]> wrote:
> 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.
>
> Keeping the parents of AUDSS mux always enabled fixes this issue.
>
> Signed-off-by: Tushar Behera <[e-mail]>
> Signed-off-by: Shaik Ameer Basha <[e-mail]>
> ---
>  drivers/clk/samsung/clk-exynos-audss.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
> index 13eae14c..1542f30 100644
> --- a/drivers/clk/samsung/clk-exynos-audss.c
> +++ b/drivers/clk/samsung/clk-exynos-audss.c
> @@ -30,6 +30,8 @@ static struct clk **clk_table;
>  static void __iomem *reg_base;
>  static struct clk_onecell_data clk_data;
>
> +static struct clk *pll_ref, *pll_in;
> +
>  #define ASS_CLK_SRC 0x0
>  #define ASS_CLK_DIV 0x4
>  #define ASS_CLK_GATE 0x8
> @@ -83,7 +85,7 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
>         const char *mout_audss_p[] = {"fin_pll", "fout_epll"};
>         const char *mout_i2s_p[] = {"mout_audss", "cdclk0", "sclk_audio0"};
>         const char *sclk_pcm_p = "sclk_pcm0";
> -       struct clk *pll_ref, *pll_in, *cdclk, *sclk_audio, *sclk_pcm_in;
> +       struct clk *cdclk, *sclk_audio, *sclk_pcm_in;
>         const struct of_device_id *match;
>         enum exynos_audss_clk_type variant;
>
> @@ -113,10 +115,14 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
>
>         pll_ref = devm_clk_get(&pdev->dev, "pll_ref");
>         pll_in = devm_clk_get(&pdev->dev, "pll_in");
> -       if (!IS_ERR(pll_ref))
> +       if (!IS_ERR(pll_ref)) {
>                 mout_audss_p[0] = __clk_get_name(pll_ref);
> -       if (!IS_ERR(pll_in))
> +               clk_prepare_enable(pll_ref);
> +       }
> +       if (!IS_ERR(pll_in)) {
>                 mout_audss_p[1] = __clk_get_name(pll_in);
> +               clk_prepare_enable(pll_in);
> +       }
>         clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss",
>                                 mout_audss_p, ARRAY_SIZE(mout_audss_p),
>                                 CLK_SET_RATE_NO_REPARENT,
> @@ -217,6 +223,11 @@ static int exynos_audss_clk_remove(struct platform_device *pdev)
>                         clk_unregister(clk_table[i]);
>         }
>
> +       if (!IS_ERR(pll_in))
> +               clk_disable_unprepare(pll_in);
> +       if (!IS_ERR(pll_ref))
> +               clk_disable_unprepare(pll_ref);
> +
>         return 0;
>  }
>
> --
> 1.7.9.5
>

Tested-by: Javier Martinez Canillas <[e-mail]>

Mike Turquette June 11, 2014, 4:50 p.m. UTC | #2

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?

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?

Regards,
Mike

> 
> Keeping the parents of AUDSS mux always enabled fixes this issue.
> 
> Signed-off-by: Tushar Behera <[e-mail]>
> Signed-off-by: Shaik Ameer Basha <[e-mail]>
> ---
>  drivers/clk/samsung/clk-exynos-audss.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
> index 13eae14c..1542f30 100644
> --- a/drivers/clk/samsung/clk-exynos-audss.c
> +++ b/drivers/clk/samsung/clk-exynos-audss.c
> @@ -30,6 +30,8 @@ static struct clk **clk_table;
>  static void __iomem *reg_base;
>  static struct clk_onecell_data clk_data;
>  
> +static struct clk *pll_ref, *pll_in;
> +
>  #define ASS_CLK_SRC 0x0
>  #define ASS_CLK_DIV 0x4
>  #define ASS_CLK_GATE 0x8
> @@ -83,7 +85,7 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
>         const char *mout_audss_p[] = {"fin_pll", "fout_epll"};
>         const char *mout_i2s_p[] = {"mout_audss", "cdclk0", "sclk_audio0"};
>         const char *sclk_pcm_p = "sclk_pcm0";
> -       struct clk *pll_ref, *pll_in, *cdclk, *sclk_audio, *sclk_pcm_in;
> +       struct clk *cdclk, *sclk_audio, *sclk_pcm_in;
>         const struct of_device_id *match;
>         enum exynos_audss_clk_type variant;
>  
> @@ -113,10 +115,14 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
>  
>         pll_ref = devm_clk_get(&pdev->dev, "pll_ref");
>         pll_in = devm_clk_get(&pdev->dev, "pll_in");
> -       if (!IS_ERR(pll_ref))
> +       if (!IS_ERR(pll_ref)) {
>                 mout_audss_p[0] = __clk_get_name(pll_ref);
> -       if (!IS_ERR(pll_in))
> +               clk_prepare_enable(pll_ref);
> +       }
> +       if (!IS_ERR(pll_in)) {
>                 mout_audss_p[1] = __clk_get_name(pll_in);
> +               clk_prepare_enable(pll_in);
> +       }
>         clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss",
>                                 mout_audss_p, ARRAY_SIZE(mout_audss_p),
>                                 CLK_SET_RATE_NO_REPARENT,
> @@ -217,6 +223,11 @@ static int exynos_audss_clk_remove(struct platform_device *pdev)
>                         clk_unregister(clk_table[i]);
>         }
>  
> +       if (!IS_ERR(pll_in))
> +               clk_disable_unprepare(pll_in);
> +       if (!IS_ERR(pll_ref))
> +               clk_disable_unprepare(pll_ref);
> +
>         return 0;
>  }
>  
> -- 
> 1.7.9.5
>

Kevin Hilman June 11, 2014, 4:50 p.m. UTC | #3

Tushar Behera <[e-mail]> writes:

> 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.
>
> Keeping the parents of AUDSS mux always enabled fixes this issue.

While this patch works (and fixes the boot problem for me), it seems
like it's papering over the real problem.

Seems like the right fix is actually modelling the clocks properly so
that enabling a child clock ensures that the parent is also enabled.

Kevin

Tomasz Figa June 11, 2014, 5:28 p.m. UTC | #4

Hi Tushar,

On 11.06.2014 07:32, Tushar Behera wrote:
> 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.

Could you provide more data about this oops? E.g. kernel log, platforms
it affects (just peach-pit or also others), test case, extra patches
applied on top of mainline (if any).

I don't like this solution, because keeping a clock always enabled is
usually not desirable and this driver is also used on other platforms
than peach-pit, so this change will affect all of them.

Best regards,
Tomasz

Tushar Behera June 12, 2014, 7:29 a.m. UTC | #5

On Wed, Jun 11, 2014 at 10:20 PM, Mike Turquette <[e-mail]> 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).

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

Tushar Behera June 12, 2014, 7:30 a.m. UTC | #6

On Wed, Jun 11, 2014 at 10:58 PM, Tomasz Figa <[e-mail]> wrote:
> Hi Tushar,
>
> On 11.06.2014 07:32, Tushar Behera wrote:
>> 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.
>
> Could you provide more data about this oops? E.g. kernel log, platforms
> it affects (just peach-pit or also others), test case, extra patches
> applied on top of mainline (if any).
>
> I don't like this solution, because keeping a clock always enabled is
> usually not desirable and this driver is also used on other platforms
> than peach-pit, so this change will affect all of them.
>

I will update later after doing similar tests on other platforms.

> Best regards,
> Tomasz

Tushar Behera June 12, 2014, 7:40 a.m. UTC | #7

On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman <[e-mail]> wrote:
> Tushar Behera <[e-mail]> writes:
>
>> 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.
>>
>> Keeping the parents of AUDSS mux always enabled fixes this issue.
>
> While this patch works (and fixes the boot problem for me), it seems
> like it's papering over the real problem.
>

Thanks for testing.

> Seems like the right fix is actually modelling the clocks properly so
> that enabling a child clock ensures that the parent is also enabled.
>

Patch 2/3 was to ensure we have proper clock tree defined for
Exynos5420. While testing with audio disabled, that patch alone fixed
the issue. But when audio was enabled (and hence I2S0 was trying to
access the clocks), we got some kernel oops during late booting, hence
I came up this solution.

The solution might be a little half-baked because of the urgency to
push the fix, but will try to dig more into the issue on Monday when I
resume office.

> Kevin

Thanks,
--
Tushar

Mike Turquette June 12, 2014, 9:09 p.m. UTC | #8

Quoting Tushar Behera (2014-06-12 00:29:23)
> On Wed, Jun 11, 2014 at 10:20 PM, Mike Turquette <[e-mail]> 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.

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)

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.

I might be totally wrong about the cause of the hang, but that's my best
guess based on everyone's bug reports.

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

Doug Anderson June 24, 2014, 10:59 p.m. UTC | #9

Tushar,

On Thu, Jun 12, 2014 at 12:40 AM, Tushar Behera <[e-mail]> wrote:
> On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman <[e-mail]> wrote:
>> Tushar Behera <[e-mail]> writes:
>>
>>> 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.
>>>
>>> Keeping the parents of AUDSS mux always enabled fixes this issue.
>>
>> While this patch works (and fixes the boot problem for me), it seems
>> like it's papering over the real problem.
>>
>
> Thanks for testing.
>
>> Seems like the right fix is actually modelling the clocks properly so
>> that enabling a child clock ensures that the parent is also enabled.
>>
>
> Patch 2/3 was to ensure we have proper clock tree defined for
> Exynos5420. While testing with audio disabled, that patch alone fixed
> the issue. But when audio was enabled (and hence I2S0 was trying to
> access the clocks), we got some kernel oops during late booting, hence
> I came up this solution.
>
> The solution might be a little half-baked because of the urgency to
> push the fix, but will try to dig more into the issue on Monday when I
> resume office.

Which Monday were you referring to?  ;)

...but in all seriousness do you have an official status update on
this patch?  It seems as if it's not needed and all you need is
<https://patchwork.kernel.org/patch/4333581/>, but it would be nice to
get an official confirmation.

Thanks!

-Doug

Tushar Behera June 25, 2014, 3:09 a.m. UTC | #10

On 06/25/2014 04:29 AM, Doug Anderson wrote:
> Tushar,
> 
> On Thu, Jun 12, 2014 at 12:40 AM, Tushar Behera <[e-mail]> wrote:
>> On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman <[e-mail]> wrote:
>>> Tushar Behera <[e-mail]> writes:
>>>
>>>> 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.
>>>>
>>>> Keeping the parents of AUDSS mux always enabled fixes this issue.
>>>
>>> While this patch works (and fixes the boot problem for me), it seems
>>> like it's papering over the real problem.
>>>
>>
>> Thanks for testing.
>>
>>> Seems like the right fix is actually modelling the clocks properly so
>>> that enabling a child clock ensures that the parent is also enabled.
>>>
>>
>> Patch 2/3 was to ensure we have proper clock tree defined for
>> Exynos5420. While testing with audio disabled, that patch alone fixed
>> the issue. But when audio was enabled (and hence I2S0 was trying to
>> access the clocks), we got some kernel oops during late booting, hence
>> I came up this solution.
>>
>> The solution might be a little half-baked because of the urgency to
>> push the fix, but will try to dig more into the issue on Monday when I
>> resume office.
> 
> Which Monday were you referring to?  ;)
> 

Sorry that I couldn't get deeper into this issue. Thanks for reminding
though.

> ...but in all seriousness do you have an official status update on
> this patch?  It seems as if it's not needed and all you need is
> <https://patchwork.kernel.org/patch/4333581/>, but it would be nice to
> get an official confirmation.

I have tested various scenarios with only patch 2/3, which seems to be
sufficient for the time being. I have not encountered the older issue
till now. I was thinking of testing a bit further, but given that you
have already asked for, we can go ahead with only patch 2/3 right now.

In case any further issue comes up, I will post patch 1/3 as per the
review comments that I have got.

> 
> Thanks!
> 
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Doug Anderson June 25, 2014, 4:02 a.m. UTC | #11

Tushar,

On Tue, Jun 24, 2014 at 8:09 PM, Tushar Behera <[e-mail]> wrote:
> On 06/25/2014 04:29 AM, Doug Anderson wrote:
>> Tushar,
>>
>> On Thu, Jun 12, 2014 at 12:40 AM, Tushar Behera <[e-mail]> wrote:
>>> On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman <[e-mail]> wrote:
>>>> Tushar Behera <[e-mail]> writes:
>>>>
>>>>> 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.
>>>>>
>>>>> Keeping the parents of AUDSS mux always enabled fixes this issue.
>>>>
>>>> While this patch works (and fixes the boot problem for me), it seems
>>>> like it's papering over the real problem.
>>>>
>>>
>>> Thanks for testing.
>>>
>>>> Seems like the right fix is actually modelling the clocks properly so
>>>> that enabling a child clock ensures that the parent is also enabled.
>>>>
>>>
>>> Patch 2/3 was to ensure we have proper clock tree defined for
>>> Exynos5420. While testing with audio disabled, that patch alone fixed
>>> the issue. But when audio was enabled (and hence I2S0 was trying to
>>> access the clocks), we got some kernel oops during late booting, hence
>>> I came up this solution.
>>>
>>> The solution might be a little half-baked because of the urgency to
>>> push the fix, but will try to dig more into the issue on Monday when I
>>> resume office.
>>
>> Which Monday were you referring to?  ;)
>>
>
> Sorry that I couldn't get deeper into this issue. Thanks for reminding
> though.

No problem.  I know how it is.  I was trying to be funny though
sometimes that doesn't come through very well over email.


>> ...but in all seriousness do you have an official status update on
>> this patch?  It seems as if it's not needed and all you need is
>> <https://patchwork.kernel.org/patch/4333581/>, but it would be nice to
>> get an official confirmation.
>
> I have tested various scenarios with only patch 2/3, which seems to be
> sufficient for the time being. I have not encountered the older issue
> till now. I was thinking of testing a bit further, but given that you
> have already asked for, we can go ahead with only patch 2/3 right now.
>
> In case any further issue comes up, I will post patch 1/3 as per the
> review comments that I have got.

Sounds like a plan.  Thanks for getting the original patch sent out so
quickly after I reported it.

Hopefully Kukjin will apply pack 2/3 soon (today?)

Tushar Behera July 11, 2014, 6:18 a.m. UTC | #12

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 <[e-mail]> 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.

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

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


> 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
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Mike Turquette July 29, 2014, 6:58 a.m. UTC | #13

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 <[e-mail]> 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
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
> 
> -- 
> Tushar Behera

Patch

diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index 13eae14c..1542f30 100644
--- a/drivers/clk/samsung/clk-exynos-audss.c
+++ b/drivers/clk/samsung/clk-exynos-audss.c
@@ -30,6 +30,8 @@  static struct clk **clk_table;
 static void __iomem *reg_base;
 static struct clk_onecell_data clk_data;
 
+static struct clk *pll_ref, *pll_in;
+
 #define ASS_CLK_SRC 0x0
 #define ASS_CLK_DIV 0x4
 #define ASS_CLK_GATE 0x8
@@ -83,7 +85,7 @@  static int exynos_audss_clk_probe(struct platform_device *pdev)
 	const char *mout_audss_p[] = {"fin_pll", "fout_epll"};
 	const char *mout_i2s_p[] = {"mout_audss", "cdclk0", "sclk_audio0"};
 	const char *sclk_pcm_p = "sclk_pcm0";
-	struct clk *pll_ref, *pll_in, *cdclk, *sclk_audio, *sclk_pcm_in;
+	struct clk *cdclk, *sclk_audio, *sclk_pcm_in;
 	const struct of_device_id *match;
 	enum exynos_audss_clk_type variant;
 
@@ -113,10 +115,14 @@  static int exynos_audss_clk_probe(struct platform_device *pdev)
 
 	pll_ref = devm_clk_get(&pdev->dev, "pll_ref");
 	pll_in = devm_clk_get(&pdev->dev, "pll_in");
-	if (!IS_ERR(pll_ref))
+	if (!IS_ERR(pll_ref)) {
 		mout_audss_p[0] = __clk_get_name(pll_ref);
-	if (!IS_ERR(pll_in))
+		clk_prepare_enable(pll_ref);
+	}
+	if (!IS_ERR(pll_in)) {
 		mout_audss_p[1] = __clk_get_name(pll_in);
+		clk_prepare_enable(pll_in);
+	}
 	clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss",
 				mout_audss_p, ARRAY_SIZE(mout_audss_p),
 				CLK_SET_RATE_NO_REPARENT,
@@ -217,6 +223,11 @@  static int exynos_audss_clk_remove(struct platform_device *pdev)
 			clk_unregister(clk_table[i]);
 	}
 
+	if (!IS_ERR(pll_in))
+		clk_disable_unprepare(pll_in);
+	if (!IS_ERR(pll_ref))
+		clk_disable_unprepare(pll_ref);
+
 	return 0;
 }