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  
> >> 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-ker...@lists.infradead.org
> > 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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 mturque...@linaro.org 
  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-ker...@lists.infradead.org
  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tushar Behera]

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  
>> 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
> linux-arm-ker...@lists.infradead.org
> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tushar Behera]

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 mturque...@linaro.org 
 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
 linux-arm-ker...@lists.infradead.org
 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Doug Anderson]

Tushar,

On Tue, Jun 24, 2014 at 8:09 PM, Tushar Behera  wrote:
> On 06/25/2014 04:29 AM, Doug Anderson wrote:
>> Tushar,
>>
>> On Thu, Jun 12, 2014 at 12:40 AM, Tushar Behera  wrote:
>>> On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman  wrote:
 Tushar Behera  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
>> , 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?)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tushar Behera]

On 06/25/2014 04:29 AM, Doug Anderson wrote:
> Tushar,
> 
> On Thu, Jun 12, 2014 at 12:40 AM, Tushar Behera  wrote:
>> On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman  wrote:
>>> Tushar Behera  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
> , 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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Tushar Behera
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Doug Anderson]

Tushar,

On Thu, Jun 12, 2014 at 12:40 AM, Tushar Behera  wrote:
> On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman  wrote:
>> Tushar Behera  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
, but it would be nice to
get an official confirmation.

Thanks!

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Doug Anderson]

Tushar,

On Thu, Jun 12, 2014 at 12:40 AM, Tushar Behera tusha...@samsung.com wrote:
 On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman khil...@linaro.org wrote:
 Tushar Behera tusha...@samsung.com 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tushar Behera]

On 06/25/2014 04:29 AM, Doug Anderson wrote:
 Tushar,
 
 On Thu, Jun 12, 2014 at 12:40 AM, Tushar Behera tusha...@samsung.com wrote:
 On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman khil...@linaro.org wrote:
 Tushar Behera tusha...@samsung.com 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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 


-- 
Tushar Behera
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Doug Anderson]

Tushar,

On Tue, Jun 24, 2014 at 8:09 PM, Tushar Behera trbli...@gmail.com wrote:
 On 06/25/2014 04:29 AM, Doug Anderson wrote:
 Tushar,

 On Thu, Jun 12, 2014 at 12:40 AM, Tushar Behera tusha...@samsung.com wrote:
 On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman khil...@linaro.org wrote:
 Tushar Behera tusha...@samsung.com 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?)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tushar Behera]

On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman  wrote:
> Tushar Behera  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tushar Behera]

On Wed, Jun 11, 2014 at 10:58 PM, Tomasz Figa  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tushar Behera]

On Wed, Jun 11, 2014 at 10:20 PM, Mike Turquette  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tushar Behera]

On Wed, Jun 11, 2014 at 10:20 PM, Mike Turquette mturque...@linaro.org 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tushar Behera]

On Wed, Jun 11, 2014 at 10:58 PM, Tomasz Figa tomasz.f...@gmail.com 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tushar Behera]

On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman khil...@linaro.org wrote:
 Tushar Behera tusha...@samsung.com 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tomasz Figa]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Kevin Hilman]

Tushar Behera  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Mike Turquette]

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 
> Signed-off-by: Shaik Ameer Basha 
> ---
>  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(>dev, "pll_ref");
> pll_in = devm_clk_get(>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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Javier Martinez Canillas]

On Wed, Jun 11, 2014 at 7:32 AM, 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.
>
> Keeping the parents of AUDSS mux always enabled fixes this issue.
>
> Signed-off-by: Tushar Behera 
> Signed-off-by: Shaik Ameer Basha 
> ---
>  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(>dev, "pll_ref");
> pll_in = devm_clk_get(>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 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Javier Martinez Canillas]

On Wed, Jun 11, 2014 at 7:32 AM, Tushar Behera tusha...@samsung.com 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 tusha...@samsung.com
 Signed-off-by: Shaik Ameer Basha shaik.am...@samsung.com
 ---
  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 javier.marti...@collabora.co.uk
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Mike Turquette]

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 tusha...@samsung.com
 Signed-off-by: Shaik Ameer Basha shaik.am...@samsung.com
 ---
  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
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Kevin Hilman]

Tushar Behera tusha...@samsung.com 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tomasz Figa]

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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tushar Behera]

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 
Signed-off-by: Shaik Ameer Basha 
---
 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(>dev, "pll_ref");
pll_in = devm_clk_get(>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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Tushar Behera]

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 tusha...@samsung.com
Signed-off-by: Shaik Ameer Basha shaik.am...@samsung.com
---
 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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/