[PATCH 0/3] Fix boot-hang on Peach-pit and Enable audio

[PATCH 0/3] Fix boot-hang on Peach-pit and Enable audio

[Tushar Behera]

With next-20140610, Peach-pit/Peach-pi board hangs during boot if we
run 'sound init' during u-boot. The issue is fixed in following patches.
While at it, also enable audio support for Peach-pi board.

How to test audio on Peach-pi:
* On top of exynos_defconfig, enable SND_SOC_SNOW and PL330_DMA.
* Run 'sound init' at u-boot prompt.

Tushar Behera (3):
  clk: exynos-audss: Keep the parent of mout_audss always enabled
  ARM: dts: Update the parent for Audss clocks in Exynos5420
  ARM: dts: Enable audio support for Peach-pi board

 arch/arm/boot/dts/exynos5420.dtsi         |    2 +-
 arch/arm/boot/dts/exynos5800-peach-pi.dts |   31 +++++++++++++++++++++++++++++
 drivers/clk/samsung/clk-exynos-audss.c    |   17 +++++++++++++---
 3 files changed, 46 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[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 <tushar.b@samsung.com>
Signed-off-by: Shaik Ameer Basha <shaik.ameer@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

[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 <tushar.b@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 <tushar.b@samsung.com>
> Signed-off-by: Shaik Ameer Basha <shaik.ameer@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.martinez@collabora.co.uk>

[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 <tushar.b@samsung.com>
> Signed-off-by: Shaik Ameer Basha <shaik.ameer@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
> 

[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 <mturquette@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

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

[Mike Turquette]

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

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

[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 <mturquette@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-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


-- 
Tushar Behera

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

[Mike Turquette]

Quoting Tushar Behera (2014-07-10 23:18:54)
> On 06/13/2014 02:39 AM, Mike Turquette wrote:
> > Quoting Tushar Behera (2014-06-12 00:29:23)
> >> On Wed, Jun 11, 2014 at 10:20 PM, Mike Turquette <mturquette@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-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
> 
> -- 
> Tushar Behera

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

[Kevin Hilman]

Tushar Behera <tushar.b@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

[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 <khilman@linaro.org> wrote:
> Tushar Behera <tushar.b@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

[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 <tushar.b@samsung.com> wrote:
> On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman <khilman@linaro.org> wrote:
>> Tushar Behera <tushar.b@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

[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 <tushar.b@samsung.com> wrote:
>> On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman <khilman@linaro.org> wrote:
>>> Tushar Behera <tushar.b@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 majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Tushar Behera

[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 <trblinux@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 <tushar.b@samsung.com> wrote:
>>> On Wed, Jun 11, 2014 at 10:20 PM, Kevin Hilman <khilman@linaro.org> wrote:
>>>> Tushar Behera <tushar.b@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?)

[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

[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.figa@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

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Tushar Behera]

Currently CLK_FOUT_EPLL was set as one of the parents of AUDSS mux.
As per the user manual, it should be CLK_MAU_EPLL.

The problem surfaced when the bootloader in Peach-pit board set
the EPLL clock as the parent of AUDSS mux. While booting the kernel,
we used to get a system hang during late boot if CLK_MAU_EPLL was
disabled.

Signed-off-by: Tushar Behera <tushar.b@samsung.com>
Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
Reported-by: Kevin Hilman <khilman@linaro.org>
---
 arch/arm/boot/dts/exynos5420.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index e385322..79e9119 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -167,7 +167,7 @@
 		compatible = "samsung,exynos5420-audss-clock";
 		reg = <0x03810000 0x0C>;
 		#clock-cells = <1>;
-		clocks = <&clock CLK_FIN_PLL>, <&clock CLK_FOUT_EPLL>,
+		clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MAU_EPLL>,
 			 <&clock CLK_SCLK_MAUDIO0>, <&clock CLK_SCLK_MAUPCM0>;
 		clock-names = "pll_ref", "pll_in", "sclk_audio", "sclk_pcm_in";
 	};
-- 
1.7.9.5

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Javier Martinez Canillas]

On Wed, Jun 11, 2014 at 7:32 AM, Tushar Behera <tushar.b@samsung.com> wrote:
> Currently CLK_FOUT_EPLL was set as one of the parents of AUDSS mux.
> As per the user manual, it should be CLK_MAU_EPLL.
>
> The problem surfaced when the bootloader in Peach-pit board set
> the EPLL clock as the parent of AUDSS mux. While booting the kernel,
> we used to get a system hang during late boot if CLK_MAU_EPLL was
> disabled.
>
> Signed-off-by: Tushar Behera <tushar.b@samsung.com>
> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
> Reported-by: Kevin Hilman <khilman@linaro.org>
> ---
>  arch/arm/boot/dts/exynos5420.dtsi |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
> index e385322..79e9119 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -167,7 +167,7 @@
>                 compatible = "samsung,exynos5420-audss-clock";
>                 reg = <0x03810000 0x0C>;
>                 #clock-cells = <1>;
> -               clocks = <&clock CLK_FIN_PLL>, <&clock CLK_FOUT_EPLL>,
> +               clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MAU_EPLL>,
>                          <&clock CLK_SCLK_MAUDIO0>, <&clock CLK_SCLK_MAUPCM0>;
>                 clock-names = "pll_ref", "pll_in", "sclk_audio", "sclk_pcm_in";
>         };
> --
> 1.7.9.5
>
> --

Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Tushar Behera]

On 06/11/2014 09:28 PM, Javier Martinez Canillas wrote:
> On Wed, Jun 11, 2014 at 7:32 AM, Tushar Behera <tushar.b@samsung.com> wrote:
>> Currently CLK_FOUT_EPLL was set as one of the parents of AUDSS mux.
>> As per the user manual, it should be CLK_MAU_EPLL.
>>
>> The problem surfaced when the bootloader in Peach-pit board set
>> the EPLL clock as the parent of AUDSS mux. While booting the kernel,
>> we used to get a system hang during late boot if CLK_MAU_EPLL was
>> disabled.
>>
>> Signed-off-by: Tushar Behera <tushar.b@samsung.com>
>> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
>> Reported-by: Kevin Hilman <khilman@linaro.org>
>> ---
>>  arch/arm/boot/dts/exynos5420.dtsi |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
>> index e385322..79e9119 100644
>> --- a/arch/arm/boot/dts/exynos5420.dtsi
>> +++ b/arch/arm/boot/dts/exynos5420.dtsi
>> @@ -167,7 +167,7 @@
>>                 compatible = "samsung,exynos5420-audss-clock";
>>                 reg = <0x03810000 0x0C>;
>>                 #clock-cells = <1>;
>> -               clocks = <&clock CLK_FIN_PLL>, <&clock CLK_FOUT_EPLL>,
>> +               clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MAU_EPLL>,
>>                          <&clock CLK_SCLK_MAUDIO0>, <&clock CLK_SCLK_MAUPCM0>;
>>                 clock-names = "pll_ref", "pll_in", "sclk_audio", "sclk_pcm_in";
>>         };
>> --
>> 1.7.9.5
>>
>> --
> 
> Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> 

Kukjin,

Would you please take this patch as a fix for 3.16?

-- 
Tushar Behera

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Tushar Behera]

On Mon, Jun 16, 2014 at 4:56 PM, Tushar Behera <trblinux@gmail.com> wrote:
> On 06/11/2014 09:28 PM, Javier Martinez Canillas wrote:
>> On Wed, Jun 11, 2014 at 7:32 AM, Tushar Behera <tushar.b@samsung.com> wrote:
>>> Currently CLK_FOUT_EPLL was set as one of the parents of AUDSS mux.
>>> As per the user manual, it should be CLK_MAU_EPLL.
>>>
>>> The problem surfaced when the bootloader in Peach-pit board set
>>> the EPLL clock as the parent of AUDSS mux. While booting the kernel,
>>> we used to get a system hang during late boot if CLK_MAU_EPLL was
>>> disabled.
>>>
>>> Signed-off-by: Tushar Behera <tushar.b@samsung.com>
>>> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
>>> Reported-by: Kevin Hilman <khilman@linaro.org>
>>> ---
>>>  arch/arm/boot/dts/exynos5420.dtsi |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
>>> index e385322..79e9119 100644
>>> --- a/arch/arm/boot/dts/exynos5420.dtsi
>>> +++ b/arch/arm/boot/dts/exynos5420.dtsi
>>> @@ -167,7 +167,7 @@
>>>                 compatible = "samsung,exynos5420-audss-clock";
>>>                 reg = <0x03810000 0x0C>;
>>>                 #clock-cells = <1>;
>>> -               clocks = <&clock CLK_FIN_PLL>, <&clock CLK_FOUT_EPLL>,
>>> +               clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MAU_EPLL>,
>>>                          <&clock CLK_SCLK_MAUDIO0>, <&clock CLK_SCLK_MAUPCM0>;
>>>                 clock-names = "pll_ref", "pll_in", "sclk_audio", "sclk_pcm_in";
>>>         };
>>> --
>>> 1.7.9.5
>>>
>>> --
>>
>> Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>>
>
> Kukjin,
>
> Would you please take this patch as a fix for 3.16?
>
> --
> Tushar Behera

Kukjin,

Please pick this patch for 3.16. This is an essential fix required for
Peach-pit/Peach-pi board.

--
Tushar Behera

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Doug Anderson]

Tushar,

On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
> Currently CLK_FOUT_EPLL was set as one of the parents of AUDSS mux.
> As per the user manual, it should be CLK_MAU_EPLL.
>
> The problem surfaced when the bootloader in Peach-pit board set
> the EPLL clock as the parent of AUDSS mux. While booting the kernel,
> we used to get a system hang during late boot if CLK_MAU_EPLL was
> disabled.
>
> Signed-off-by: Tushar Behera <tushar.b@samsung.com>
> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
> Reported-by: Kevin Hilman <khilman@linaro.org>
> ---
>  arch/arm/boot/dts/exynos5420.dtsi |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I've tested this myself now as well.

Tested-by: Doug Anderson <dianders@chromium.org>

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Kevin Hilman]

Doug Anderson <dianders@google.com> writes:

> Tushar,
>
> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
>> Currently CLK_FOUT_EPLL was set as one of the parents of AUDSS mux.
>> As per the user manual, it should be CLK_MAU_EPLL.
>>
>> The problem surfaced when the bootloader in Peach-pit board set
>> the EPLL clock as the parent of AUDSS mux. While booting the kernel,
>> we used to get a system hang during late boot if CLK_MAU_EPLL was
>> disabled.
>>
>> Signed-off-by: Tushar Behera <tushar.b@samsung.com>
>> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
>> Reported-by: Kevin Hilman <khilman@linaro.org>
>> ---
>>  arch/arm/boot/dts/exynos5420.dtsi |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> I've tested this myself now as well.
>
> Tested-by: Doug Anderson <dianders@chromium.org>

For me, this patch alone (on top of -next) doesn't solve the boot hang.
I still need clk_ignore_unused for a successful boot.

So, this patch might be correct, but it doesn't prevent a boot hang
using a chain-loaded nv_uboot on peach-pi.  There's still another clock
being disabled that causes a hang.

Kevin

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Tushar Behera]

On Thu, Jun 26, 2014 at 4:51 AM, Kevin Hilman <khilman@linaro.org> wrote:
> Doug Anderson <dianders@google.com> writes:
>
>> Tushar,
>>
>> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
>>> Currently CLK_FOUT_EPLL was set as one of the parents of AUDSS mux.
>>> As per the user manual, it should be CLK_MAU_EPLL.
>>>
>>> The problem surfaced when the bootloader in Peach-pit board set
>>> the EPLL clock as the parent of AUDSS mux. While booting the kernel,
>>> we used to get a system hang during late boot if CLK_MAU_EPLL was
>>> disabled.
>>>
>>> Signed-off-by: Tushar Behera <tushar.b@samsung.com>
>>> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
>>> Reported-by: Kevin Hilman <khilman@linaro.org>
>>> ---
>>>  arch/arm/boot/dts/exynos5420.dtsi |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> I've tested this myself now as well.
>>
>> Tested-by: Doug Anderson <dianders@chromium.org>
>
> For me, this patch alone (on top of -next) doesn't solve the boot hang.
> I still need clk_ignore_unused for a successful boot.
>
> So, this patch might be correct, but it doesn't prevent a boot hang
> using a chain-loaded nv_uboot on peach-pi.  There's still another clock
> being disabled that causes a hang.
>
> Kevin

Kevin,

Can you please check if adding patch 1/3 alongwith patch 2/3 fixes the
issue for you?

Also can you please confirm that setting CLK_IGNORE_UNUSED flag
CLK_MAU_EPLL alone fixes the issue, without any need for
clk_ignore_unused in u-boot bootargs?

-- 
Tushar Behera

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Kevin Hilman]

Tushar Behera <trblinux@gmail.com> writes:

> On Thu, Jun 26, 2014 at 4:51 AM, Kevin Hilman <khilman@linaro.org> wrote:
>> Doug Anderson <dianders@google.com> writes:
>>
>>> Tushar,
>>>
>>> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
>>>> Currently CLK_FOUT_EPLL was set as one of the parents of AUDSS mux.
>>>> As per the user manual, it should be CLK_MAU_EPLL.
>>>>
>>>> The problem surfaced when the bootloader in Peach-pit board set
>>>> the EPLL clock as the parent of AUDSS mux. While booting the kernel,
>>>> we used to get a system hang during late boot if CLK_MAU_EPLL was
>>>> disabled.
>>>>
>>>> Signed-off-by: Tushar Behera <tushar.b@samsung.com>
>>>> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
>>>> Reported-by: Kevin Hilman <khilman@linaro.org>
>>>> ---
>>>>  arch/arm/boot/dts/exynos5420.dtsi |    2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> I've tested this myself now as well.
>>>
>>> Tested-by: Doug Anderson <dianders@chromium.org>
>>
>> For me, this patch alone (on top of -next) doesn't solve the boot hang.
>> I still need clk_ignore_unused for a successful boot.
>>
>> So, this patch might be correct, but it doesn't prevent a boot hang
>> using a chain-loaded nv_uboot on peach-pi.  There's still another clock
>> being disabled that causes a hang.
>>
>> Kevin
>
> Kevin,
>
> Can you please check if adding patch 1/3 alongwith patch 2/3 fixes the
> issue for you?

Yes, using patch 1/3 along with 2/3 fixes the issue.

> Also can you please confirm that setting CLK_IGNORE_UNUSED flag
> CLK_MAU_EPLL alone fixes the issue, without any need for
> clk_ignore_unused in u-boot bootargs?

Yes, I have this patch[1] in my local branch which fixes the issue
alone, without clk_ignore_unused on the command line.

Kevin


[1]
>From ab1627127730ef4507ce96cbf95047d626bbb53f Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@linaro.org>
Date: Thu, 5 Jun 2014 17:12:28 -0700
Subject: [PATCH] KJH: leave mau_epll enabled

---
 drivers/clk/samsung/clk-exynos5420.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
index 61eccf0dd72f..ed175088ee7e 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -911,7 +911,7 @@ static struct samsung_gate_clock exynos5x_gate_clks[] __initdata = {
 			SRC_MASK_TOP2, 24, 0, 0),
 
 	GATE(CLK_MAU_EPLL, "mau_epll", "mout_mau_epll_clk",
-			SRC_MASK_TOP7, 20, 0, 0),
+			SRC_MASK_TOP7, 20, CLK_IGNORE_UNUSED, 0),
 
 	/* sclk */
 	GATE(CLK_SCLK_UART0, "sclk_uart0", "dout_uart0",
-- 
1.9.2

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Tushar Behera]

On 06/26/2014 09:38 PM, Kevin Hilman wrote:
> Tushar Behera <trblinux@gmail.com> writes:
> 
>> On Thu, Jun 26, 2014 at 4:51 AM, Kevin Hilman <khilman@linaro.org> wrote:
>>> Doug Anderson <dianders@google.com> writes:
>>>
>>>> Tushar,
>>>>
>>>> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
>>>>> Currently CLK_FOUT_EPLL was set as one of the parents of AUDSS mux.
>>>>> As per the user manual, it should be CLK_MAU_EPLL.
>>>>>
>>>>> The problem surfaced when the bootloader in Peach-pit board set
>>>>> the EPLL clock as the parent of AUDSS mux. While booting the kernel,
>>>>> we used to get a system hang during late boot if CLK_MAU_EPLL was
>>>>> disabled.
>>>>>
>>>>> Signed-off-by: Tushar Behera <tushar.b@samsung.com>
>>>>> Signed-off-by: Shaik Ameer Basha <shaik.ameer@samsung.com>
>>>>> Reported-by: Kevin Hilman <khilman@linaro.org>
>>>>> ---
>>>>>  arch/arm/boot/dts/exynos5420.dtsi |    2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> I've tested this myself now as well.
>>>>
>>>> Tested-by: Doug Anderson <dianders@chromium.org>
>>>
>>> For me, this patch alone (on top of -next) doesn't solve the boot hang.
>>> I still need clk_ignore_unused for a successful boot.
>>>
>>> So, this patch might be correct, but it doesn't prevent a boot hang
>>> using a chain-loaded nv_uboot on peach-pi.  There's still another clock
>>> being disabled that causes a hang.
>>>
>>> Kevin
>>
>> Kevin,
>>
>> Can you please check if adding patch 1/3 alongwith patch 2/3 fixes the
>> issue for you?
> 
> Yes, using patch 1/3 along with 2/3 fixes the issue.
> 

Okay, that adds some more reason to re-investigate patch 1/3.

Kevin,

Would you please provide me the environment setting of your u-boot?
U-boot environment on my board has been over-written, I would like to
set it same as yours and try to reproduce the issue at my end. With only
'sound init', I don't seem to hit this issue anymore.

>> Also can you please confirm that setting CLK_IGNORE_UNUSED flag
>> CLK_MAU_EPLL alone fixes the issue, without any need for
>> clk_ignore_unused in u-boot bootargs?
> 
> Yes, I have this patch[1] in my local branch which fixes the issue
> alone, without clk_ignore_unused on the command line.
> 
> Kevin
> 
> 
> [1]
> From ab1627127730ef4507ce96cbf95047d626bbb53f Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@linaro.org>
> Date: Thu, 5 Jun 2014 17:12:28 -0700
> Subject: [PATCH] KJH: leave mau_epll enabled
> 
> ---
>  drivers/clk/samsung/clk-exynos5420.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> index 61eccf0dd72f..ed175088ee7e 100644
> --- a/drivers/clk/samsung/clk-exynos5420.c
> +++ b/drivers/clk/samsung/clk-exynos5420.c
> @@ -911,7 +911,7 @@ static struct samsung_gate_clock exynos5x_gate_clks[] __initdata = {
>  			SRC_MASK_TOP2, 24, 0, 0),
>  
>  	GATE(CLK_MAU_EPLL, "mau_epll", "mout_mau_epll_clk",
> -			SRC_MASK_TOP7, 20, 0, 0),
> +			SRC_MASK_TOP7, 20, CLK_IGNORE_UNUSED, 0),
>  
>  	/* sclk */
>  	GATE(CLK_SCLK_UART0, "sclk_uart0", "dout_uart0",
> 


-- 
Tushar Behera

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Kevin Hilman]

On Thu, Jun 26, 2014 at 8:38 PM, Tushar Behera <trblinux@gmail.com> wrote:

> Would you please provide me the environment setting of your u-boot?
> U-boot environment on my board has been over-written, I would like to
> set it same as yours and try to reproduce the issue at my end. With only
> 'sound init', I don't seem to hit this issue anymore.

Attached is a full boot log using v3.16-rc2 with my patch adding
CLK_IGNORE_UNUSED to mau_epll and Doug's aclk66_peric patch.  In the
boot log, you'll see the output of 'printenv' inside u-boot where the
environment is dumped.

Hope that helps,

Kevin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: boot-octa.log.gz
Type: application/x-gzip
Size: 7519 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140627/e2a51df0/attachment.bin>

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Kevin Hilman]

On Fri, Jun 27, 2014 at 7:18 AM, Kevin Hilman <khilman@linaro.org> wrote:
> On Thu, Jun 26, 2014 at 8:38 PM, Tushar Behera <trblinux@gmail.com> wrote:
>
>> Would you please provide me the environment setting of your u-boot?
>> U-boot environment on my board has been over-written, I would like to
>> set it same as yours and try to reproduce the issue at my end. With only
>> 'sound init', I don't seem to hit this issue anymore.
>
> Attached is a full boot log using v3.16-rc2 with my patch adding
> CLK_IGNORE_UNUSED to mau_epll and Doug's aclk66_peric patch.  In the
> boot log, you'll see the output of 'printenv' inside u-boot where the
> environment is dumped.

Oops, I sent you a boot log for the octa board.  Here's the one for
peach-pi with the same kernel (built with upstream exynos_defconfig)

Kevin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: boot-chromebook2.log.gz
Type: application/x-gzip
Size: 8850 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140627/a95b6b32/attachment.bin>

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Tushar Behera]

On 06/27/2014 08:18 PM, Kevin Hilman wrote:
> On Fri, Jun 27, 2014 at 7:18 AM, Kevin Hilman <khilman@linaro.org> wrote:
>> On Thu, Jun 26, 2014 at 8:38 PM, Tushar Behera <trblinux@gmail.com> wrote:
>>
>>> Would you please provide me the environment setting of your u-boot?
>>> U-boot environment on my board has been over-written, I would like to
>>> set it same as yours and try to reproduce the issue at my end. With only
>>> 'sound init', I don't seem to hit this issue anymore.
>>
>> Attached is a full boot log using v3.16-rc2 with my patch adding
>> CLK_IGNORE_UNUSED to mau_epll and Doug's aclk66_peric patch.  In the
>> boot log, you'll see the output of 'printenv' inside u-boot where the
>> environment is dumped.
> 
> Oops, I sent you a boot log for the octa board.  Here's the one for
> peach-pi with the same kernel (built with upstream exynos_defconfig)
> 
> Kevin
> 

The u-boot version is a little different on my Peach-Pi as compared to
the market release version. Not sure if that is making any difference.

Peach # version

U-Boot 2013.04 (Feb 13 2014 - 16:35:03) for Peach
armv7a-cros-linux-gnueabi-gcc.real (4.8.1_cos_gg_feea904_4.8.1-r66)
4.8.x-google 20130905 (prerelease)
GNU ld (binutils-2.22_cos_gg_2) 2.22

-- 
Tushar Behera

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Kukjin Kim]

On 07/01/14 20:59, Tushar Behera wrote:
> On 06/27/2014 08:18 PM, Kevin Hilman wrote:
>> On Fri, Jun 27, 2014 at 7:18 AM, Kevin Hilman<khilman@linaro.org>  wrote:
>>> On Thu, Jun 26, 2014 at 8:38 PM, Tushar Behera<trblinux@gmail.com>  wrote:
>>>
>>>> Would you please provide me the environment setting of your u-boot?
>>>> U-boot environment on my board has been over-written, I would like to
>>>> set it same as yours and try to reproduce the issue at my end. With only
>>>> 'sound init', I don't seem to hit this issue anymore.
>>>
>>> Attached is a full boot log using v3.16-rc2 with my patch adding
>>> CLK_IGNORE_UNUSED to mau_epll and Doug's aclk66_peric patch.  In the
>>> boot log, you'll see the output of 'printenv' inside u-boot where the
>>> environment is dumped.
>>
>> Oops, I sent you a boot log for the octa board.  Here's the one for
>> peach-pi with the same kernel (built with upstream exynos_defconfig)
>>
>> Kevin
>>
>
> The u-boot version is a little different on my Peach-Pi as compared to
> the market release version. Not sure if that is making any difference.
>
> Peach # version
>
> U-Boot 2013.04 (Feb 13 2014 - 16:35:03) for Peach
> armv7a-cros-linux-gnueabi-gcc.real (4.8.1_cos_gg_feea904_4.8.1-r66)
> 4.8.x-google 20130905 (prerelease)
> GNU ld (binutils-2.22_cos_gg_2) 2.22
>

Note that I've applied this only from this series so I'm not sure how 
much the problem can be solved...any updates for 1/3 and 3/3?

- Kukjin

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Tushar Behera]

On 07/08/2014 05:04 AM, Kukjin Kim wrote:
> On 07/01/14 20:59, Tushar Behera wrote:
>> On 06/27/2014 08:18 PM, Kevin Hilman wrote:
>>> On Fri, Jun 27, 2014 at 7:18 AM, Kevin Hilman<khilman@linaro.org> 
>>> wrote:
>>>> On Thu, Jun 26, 2014 at 8:38 PM, Tushar Behera<trblinux@gmail.com> 
>>>> wrote:
>>>>
>>>>> Would you please provide me the environment setting of your u-boot?
>>>>> U-boot environment on my board has been over-written, I would like to
>>>>> set it same as yours and try to reproduce the issue at my end. With
>>>>> only
>>>>> 'sound init', I don't seem to hit this issue anymore.
>>>>
>>>> Attached is a full boot log using v3.16-rc2 with my patch adding
>>>> CLK_IGNORE_UNUSED to mau_epll and Doug's aclk66_peric patch.  In the
>>>> boot log, you'll see the output of 'printenv' inside u-boot where the
>>>> environment is dumped.
>>>
>>> Oops, I sent you a boot log for the octa board.  Here's the one for
>>> peach-pi with the same kernel (built with upstream exynos_defconfig)
>>>
>>> Kevin
>>>
>>
>> The u-boot version is a little different on my Peach-Pi as compared to
>> the market release version. Not sure if that is making any difference.
>>
>> Peach # version
>>
>> U-Boot 2013.04 (Feb 13 2014 - 16:35:03) for Peach
>> armv7a-cros-linux-gnueabi-gcc.real (4.8.1_cos_gg_feea904_4.8.1-r66)
>> 4.8.x-google 20130905 (prerelease)
>> GNU ld (binutils-2.22_cos_gg_2) 2.22
>>
> 
> Note that I've applied this only from this series so I'm not sure how
> much the problem can be solved...any updates for 1/3 and 3/3?
> 
> - Kukjin

Thanks for applying 2/3. I am working on 1/3 to see if we are following
the right approach to fix Kevin's issue (unfortunately, I am not hitting
the bug on my board ATM). 3/3 has already been merged through a
different patchset.

-- 
Tushar Behera

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Javier Martinez Canillas]

Hello Tushar,

On Tue, Jul 8, 2014 at 5:00 AM, Tushar Behera <trblinux@gmail.com> wrote:
>>>
>>> The u-boot version is a little different on my Peach-Pi as compared to
>>> the market release version. Not sure if that is making any difference.
>>>
>>> Peach # version
>>>
>>> U-Boot 2013.04 (Feb 13 2014 - 16:35:03) for Peach
>>> armv7a-cros-linux-gnueabi-gcc.real (4.8.1_cos_gg_feea904_4.8.1-r66)
>>> 4.8.x-google 20130905 (prerelease)
>>> GNU ld (binutils-2.22_cos_gg_2) 2.22
>>>
>>

I'm using the same U-Boot version than Kevin (U-Boot 2013.04-gb98ed09)
and on my setup using chained nv-uboot I also need patch 1/3 along
with 2/3 to fix the issue.

>> Note that I've applied this only from this series so I'm not sure how
>> much the problem can be solved...any updates for 1/3 and 3/3?
>>
>> - Kukjin
>
> Thanks for applying 2/3. I am working on 1/3 to see if we are following
> the right approach to fix Kevin's issue (unfortunately, I am not hitting
> the bug on my board ATM). 3/3 has already been merged through a
> different patchset.
>

I'm sending as an attachment my complete boot log when booting today's
next (20140709) until it hangs and my u-boot env vars. I hope that
helps.

> --
> Tushar Behera
> --

Best regards,
Javier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: boot_log
Type: application/octet-stream
Size: 16442 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140709/8d6422b3/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: uboot_env
Type: application/octet-stream
Size: 3614 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140709/8d6422b3/attachment-0003.obj>

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Tushar Behera]

On 07/09/2014 03:44 PM, Javier Martinez Canillas wrote:
> Hello Tushar,
> 
> On Tue, Jul 8, 2014 at 5:00 AM, Tushar Behera <trblinux@gmail.com> wrote:
>>>>
>>>> The u-boot version is a little different on my Peach-Pi as compared to
>>>> the market release version. Not sure if that is making any difference.
>>>>
>>>> Peach # version
>>>>
>>>> U-Boot 2013.04 (Feb 13 2014 - 16:35:03) for Peach
>>>> armv7a-cros-linux-gnueabi-gcc.real (4.8.1_cos_gg_feea904_4.8.1-r66)
>>>> 4.8.x-google 20130905 (prerelease)
>>>> GNU ld (binutils-2.22_cos_gg_2) 2.22
>>>>
>>>
> 
> I'm using the same U-Boot version than Kevin (U-Boot 2013.04-gb98ed09)
> and on my setup using chained nv-uboot I also need patch 1/3 along
> with 2/3 to fix the issue.
> 
>>> Note that I've applied this only from this series so I'm not sure how
>>> much the problem can be solved...any updates for 1/3 and 3/3?
>>>
>>> - Kukjin
>>
>> Thanks for applying 2/3. I am working on 1/3 to see if we are following
>> the right approach to fix Kevin's issue (unfortunately, I am not hitting
>> the bug on my board ATM). 3/3 has already been merged through a
>> different patchset.
>>
> 
> I'm sending as an attachment my complete boot log when booting today's
> next (20140709) until it hangs and my u-boot env vars. I hope that
> helps.
> 

Would you please check the behaviour after enabling following config
options?

diff --git a/arch/arm/configs/exynos_defconfig
b/arch/arm/configs/exynos_defconfig
index e07a227..d6056ab 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -93,6 +93,11 @@ CONFIG_FRAMEBUFFER_CONSOLE=y
 CONFIG_FONTS=y
 CONFIG_FONT_7x14=y
 CONFIG_LOGO=y
+CONFIG_SOUND=y
+CONFIG_SND=y
+CONFIG_SND_SOC=y
+CONFIG_SND_SOC_SAMSUNG=y
+CONFIG_SND_SOC_SNOW=y
 CONFIG_USB=y
 CONFIG_USB_EHCI_HCD=y
 CONFIG_USB_EHCI_EXYNOS=y
@@ -109,6 +114,8 @@ CONFIG_MMC_DW_IDMAC=y
 CONFIG_MMC_DW_EXYNOS=y
 CONFIG_RTC_CLASS=y
 CONFIG_RTC_DRV_S3C=y
+CONFIG_DMADEVICES=y
+CONFIG_PL330_DMA=y
 CONFIG_COMMON_CLK_MAX77686=y
 CONFIG_EXT2_FS=y
 CONFIG_EXT3_FS=y


>> --
>> Tushar Behera
>> --
> 
> Best regards,
> Javier
> 


-- 
Tushar Behera

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Javier Martinez Canillas]

Hello Tushar,

On Wed, Jul 9, 2014 at 2:11 PM, Tushar Behera <trblinux@gmail.com> wrote:
> On 07/09/2014 03:44 PM, Javier Martinez Canillas wrote:
>> Hello Tushar,
>>
>> On Tue, Jul 8, 2014 at 5:00 AM, Tushar Behera <trblinux@gmail.com> wrote:
>>>>>
>>>>> The u-boot version is a little different on my Peach-Pi as compared to
>>>>> the market release version. Not sure if that is making any difference.
>>>>>
>>>>> Peach # version
>>>>>
>>>>> U-Boot 2013.04 (Feb 13 2014 - 16:35:03) for Peach
>>>>> armv7a-cros-linux-gnueabi-gcc.real (4.8.1_cos_gg_feea904_4.8.1-r66)
>>>>> 4.8.x-google 20130905 (prerelease)
>>>>> GNU ld (binutils-2.22_cos_gg_2) 2.22
>>>>>
>>>>
>>
>> I'm using the same U-Boot version than Kevin (U-Boot 2013.04-gb98ed09)
>> and on my setup using chained nv-uboot I also need patch 1/3 along
>> with 2/3 to fix the issue.
>>
>>>> Note that I've applied this only from this series so I'm not sure how
>>>> much the problem can be solved...any updates for 1/3 and 3/3?
>>>>
>>>> - Kukjin
>>>
>>> Thanks for applying 2/3. I am working on 1/3 to see if we are following
>>> the right approach to fix Kevin's issue (unfortunately, I am not hitting
>>> the bug on my board ATM). 3/3 has already been merged through a
>>> different patchset.
>>>
>>
>> I'm sending as an attachment my complete boot log when booting today's
>> next (20140709) until it hangs and my u-boot env vars. I hope that
>> helps.
>>
>
> Would you please check the behaviour after enabling following config
> options?
>
> diff --git a/arch/arm/configs/exynos_defconfig
> b/arch/arm/configs/exynos_defconfig
> index e07a227..d6056ab 100644
> --- a/arch/arm/configs/exynos_defconfig
> +++ b/arch/arm/configs/exynos_defconfig
> @@ -93,6 +93,11 @@ CONFIG_FRAMEBUFFER_CONSOLE=y
>  CONFIG_FONTS=y
>  CONFIG_FONT_7x14=y
>  CONFIG_LOGO=y
> +CONFIG_SOUND=y
> +CONFIG_SND=y
> +CONFIG_SND_SOC=y
> +CONFIG_SND_SOC_SAMSUNG=y
> +CONFIG_SND_SOC_SNOW=y
>  CONFIG_USB=y
>  CONFIG_USB_EHCI_HCD=y
>  CONFIG_USB_EHCI_EXYNOS=y
> @@ -109,6 +114,8 @@ CONFIG_MMC_DW_IDMAC=y
>  CONFIG_MMC_DW_EXYNOS=y
>  CONFIG_RTC_CLASS=y
>  CONFIG_RTC_DRV_S3C=y
> +CONFIG_DMADEVICES=y
> +CONFIG_PL330_DMA=y
>  CONFIG_COMMON_CLK_MAX77686=y
>  CONFIG_EXT2_FS=y
>  CONFIG_EXT3_FS=y
>
>

With those Kconfig options enabled the kernel does not hang anymore so
patch 1/3 is not needed in that case.

Best regards,
Javier

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Doug Anderson]

Javier,

On Wed, Jul 9, 2014 at 6:03 AM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> Hello Tushar,
>
> On Wed, Jul 9, 2014 at 2:11 PM, Tushar Behera <trblinux@gmail.com> wrote:
>> On 07/09/2014 03:44 PM, Javier Martinez Canillas wrote:
>>> Hello Tushar,
>>>
>>> On Tue, Jul 8, 2014 at 5:00 AM, Tushar Behera <trblinux@gmail.com> wrote:
>>>>>>
>>>>>> The u-boot version is a little different on my Peach-Pi as compared to
>>>>>> the market release version. Not sure if that is making any difference.
>>>>>>
>>>>>> Peach # version
>>>>>>
>>>>>> U-Boot 2013.04 (Feb 13 2014 - 16:35:03) for Peach
>>>>>> armv7a-cros-linux-gnueabi-gcc.real (4.8.1_cos_gg_feea904_4.8.1-r66)
>>>>>> 4.8.x-google 20130905 (prerelease)
>>>>>> GNU ld (binutils-2.22_cos_gg_2) 2.22
>>>>>>
>>>>>
>>>
>>> I'm using the same U-Boot version than Kevin (U-Boot 2013.04-gb98ed09)
>>> and on my setup using chained nv-uboot I also need patch 1/3 along
>>> with 2/3 to fix the issue.
>>>
>>>>> Note that I've applied this only from this series so I'm not sure how
>>>>> much the problem can be solved...any updates for 1/3 and 3/3?
>>>>>
>>>>> - Kukjin
>>>>
>>>> Thanks for applying 2/3. I am working on 1/3 to see if we are following
>>>> the right approach to fix Kevin's issue (unfortunately, I am not hitting
>>>> the bug on my board ATM). 3/3 has already been merged through a
>>>> different patchset.
>>>>
>>>
>>> I'm sending as an attachment my complete boot log when booting today's
>>> next (20140709) until it hangs and my u-boot env vars. I hope that
>>> helps.
>>>
>>
>> Would you please check the behaviour after enabling following config
>> options?
>>
>> diff --git a/arch/arm/configs/exynos_defconfig
>> b/arch/arm/configs/exynos_defconfig
>> index e07a227..d6056ab 100644
>> --- a/arch/arm/configs/exynos_defconfig
>> +++ b/arch/arm/configs/exynos_defconfig
>> @@ -93,6 +93,11 @@ CONFIG_FRAMEBUFFER_CONSOLE=y
>>  CONFIG_FONTS=y
>>  CONFIG_FONT_7x14=y
>>  CONFIG_LOGO=y
>> +CONFIG_SOUND=y
>> +CONFIG_SND=y
>> +CONFIG_SND_SOC=y
>> +CONFIG_SND_SOC_SAMSUNG=y
>> +CONFIG_SND_SOC_SNOW=y
>>  CONFIG_USB=y
>>  CONFIG_USB_EHCI_HCD=y
>>  CONFIG_USB_EHCI_EXYNOS=y
>> @@ -109,6 +114,8 @@ CONFIG_MMC_DW_IDMAC=y
>>  CONFIG_MMC_DW_EXYNOS=y
>>  CONFIG_RTC_CLASS=y
>>  CONFIG_RTC_DRV_S3C=y
>> +CONFIG_DMADEVICES=y
>> +CONFIG_PL330_DMA=y
>>  CONFIG_COMMON_CLK_MAX77686=y
>>  CONFIG_EXT2_FS=y
>>  CONFIG_EXT3_FS=y
>>
>>
>
> With those Kconfig options enabled the kernel does not hang anymore so
> patch 1/3 is not needed in that case.

Just checking: did you happen to confirm whether it's the PL330 /
DMADEVICES that fixes things or do you actually need the sound stuff?

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Javier Martinez Canillas]

Hello Doug,

On Wed, Jul 9, 2014 at 6:01 PM, Doug Anderson <dianders@google.com> wrote:
> Javier,
>
> On Wed, Jul 9, 2014 at 6:03 AM, Javier Martinez Canillas
> <javier@dowhile0.org> wrote:
>> Hello Tushar,
>>
>> On Wed, Jul 9, 2014 at 2:11 PM, Tushar Behera <trblinux@gmail.com> wrote:
>>> On 07/09/2014 03:44 PM, Javier Martinez Canillas wrote:
>>>> Hello Tushar,
>>>>
>>>> On Tue, Jul 8, 2014 at 5:00 AM, Tushar Behera <trblinux@gmail.com> wrote:
>>>>>>>
>>>>>>> The u-boot version is a little different on my Peach-Pi as compared to
>>>>>>> the market release version. Not sure if that is making any difference.
>>>>>>>
>>>>>>> Peach # version
>>>>>>>
>>>>>>> U-Boot 2013.04 (Feb 13 2014 - 16:35:03) for Peach
>>>>>>> armv7a-cros-linux-gnueabi-gcc.real (4.8.1_cos_gg_feea904_4.8.1-r66)
>>>>>>> 4.8.x-google 20130905 (prerelease)
>>>>>>> GNU ld (binutils-2.22_cos_gg_2) 2.22
>>>>>>>
>>>>>>
>>>>
>>>> I'm using the same U-Boot version than Kevin (U-Boot 2013.04-gb98ed09)
>>>> and on my setup using chained nv-uboot I also need patch 1/3 along
>>>> with 2/3 to fix the issue.
>>>>
>>>>>> Note that I've applied this only from this series so I'm not sure how
>>>>>> much the problem can be solved...any updates for 1/3 and 3/3?
>>>>>>
>>>>>> - Kukjin
>>>>>
>>>>> Thanks for applying 2/3. I am working on 1/3 to see if we are following
>>>>> the right approach to fix Kevin's issue (unfortunately, I am not hitting
>>>>> the bug on my board ATM). 3/3 has already been merged through a
>>>>> different patchset.
>>>>>
>>>>
>>>> I'm sending as an attachment my complete boot log when booting today's
>>>> next (20140709) until it hangs and my u-boot env vars. I hope that
>>>> helps.
>>>>
>>>
>>> Would you please check the behaviour after enabling following config
>>> options?
>>>
>>> diff --git a/arch/arm/configs/exynos_defconfig
>>> b/arch/arm/configs/exynos_defconfig
>>> index e07a227..d6056ab 100644
>>> --- a/arch/arm/configs/exynos_defconfig
>>> +++ b/arch/arm/configs/exynos_defconfig
>>> @@ -93,6 +93,11 @@ CONFIG_FRAMEBUFFER_CONSOLE=y
>>>  CONFIG_FONTS=y
>>>  CONFIG_FONT_7x14=y
>>>  CONFIG_LOGO=y
>>> +CONFIG_SOUND=y
>>> +CONFIG_SND=y
>>> +CONFIG_SND_SOC=y
>>> +CONFIG_SND_SOC_SAMSUNG=y
>>> +CONFIG_SND_SOC_SNOW=y
>>>  CONFIG_USB=y
>>>  CONFIG_USB_EHCI_HCD=y
>>>  CONFIG_USB_EHCI_EXYNOS=y
>>> @@ -109,6 +114,8 @@ CONFIG_MMC_DW_IDMAC=y
>>>  CONFIG_MMC_DW_EXYNOS=y
>>>  CONFIG_RTC_CLASS=y
>>>  CONFIG_RTC_DRV_S3C=y
>>> +CONFIG_DMADEVICES=y
>>> +CONFIG_PL330_DMA=y
>>>  CONFIG_COMMON_CLK_MAX77686=y
>>>  CONFIG_EXT2_FS=y
>>>  CONFIG_EXT3_FS=y
>>>
>>>
>>
>> With those Kconfig options enabled the kernel does not hang anymore so
>> patch 1/3 is not needed in that case.
>
> Just checking: did you happen to confirm whether it's the PL330 /
> DMADEVICES that fixes things or do you actually need the sound stuff?

Sorry I should had mentioned this before. The DMADEVICES and PL330
Kconfig are enough to avoid the kernel to hang, the sound config
options are not actually required.

Best regards,
Javier

[PATCH 2/3] ARM: dts: Update the parent for Audss clocks in Exynos5420

[Doug Anderson]

Javier,

On Wed, Jul 9, 2014 at 10:46 AM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> Hello Doug,
>
> On Wed, Jul 9, 2014 at 6:01 PM, Doug Anderson <dianders@google.com> wrote:
>> Javier,
>>
>> On Wed, Jul 9, 2014 at 6:03 AM, Javier Martinez Canillas
>> <javier@dowhile0.org> wrote:
>>> Hello Tushar,
>>>
>>> On Wed, Jul 9, 2014 at 2:11 PM, Tushar Behera <trblinux@gmail.com> wrote:
>>>> On 07/09/2014 03:44 PM, Javier Martinez Canillas wrote:
>>>>> Hello Tushar,
>>>>>
>>>>> On Tue, Jul 8, 2014 at 5:00 AM, Tushar Behera <trblinux@gmail.com> wrote:
>>>>>>>>
>>>>>>>> The u-boot version is a little different on my Peach-Pi as compared to
>>>>>>>> the market release version. Not sure if that is making any difference.
>>>>>>>>
>>>>>>>> Peach # version
>>>>>>>>
>>>>>>>> U-Boot 2013.04 (Feb 13 2014 - 16:35:03) for Peach
>>>>>>>> armv7a-cros-linux-gnueabi-gcc.real (4.8.1_cos_gg_feea904_4.8.1-r66)
>>>>>>>> 4.8.x-google 20130905 (prerelease)
>>>>>>>> GNU ld (binutils-2.22_cos_gg_2) 2.22
>>>>>>>>
>>>>>>>
>>>>>
>>>>> I'm using the same U-Boot version than Kevin (U-Boot 2013.04-gb98ed09)
>>>>> and on my setup using chained nv-uboot I also need patch 1/3 along
>>>>> with 2/3 to fix the issue.
>>>>>
>>>>>>> Note that I've applied this only from this series so I'm not sure how
>>>>>>> much the problem can be solved...any updates for 1/3 and 3/3?
>>>>>>>
>>>>>>> - Kukjin
>>>>>>
>>>>>> Thanks for applying 2/3. I am working on 1/3 to see if we are following
>>>>>> the right approach to fix Kevin's issue (unfortunately, I am not hitting
>>>>>> the bug on my board ATM). 3/3 has already been merged through a
>>>>>> different patchset.
>>>>>>
>>>>>
>>>>> I'm sending as an attachment my complete boot log when booting today's
>>>>> next (20140709) until it hangs and my u-boot env vars. I hope that
>>>>> helps.
>>>>>
>>>>
>>>> Would you please check the behaviour after enabling following config
>>>> options?
>>>>
>>>> diff --git a/arch/arm/configs/exynos_defconfig
>>>> b/arch/arm/configs/exynos_defconfig
>>>> index e07a227..d6056ab 100644
>>>> --- a/arch/arm/configs/exynos_defconfig
>>>> +++ b/arch/arm/configs/exynos_defconfig
>>>> @@ -93,6 +93,11 @@ CONFIG_FRAMEBUFFER_CONSOLE=y
>>>>  CONFIG_FONTS=y
>>>>  CONFIG_FONT_7x14=y
>>>>  CONFIG_LOGO=y
>>>> +CONFIG_SOUND=y
>>>> +CONFIG_SND=y
>>>> +CONFIG_SND_SOC=y
>>>> +CONFIG_SND_SOC_SAMSUNG=y
>>>> +CONFIG_SND_SOC_SNOW=y
>>>>  CONFIG_USB=y
>>>>  CONFIG_USB_EHCI_HCD=y
>>>>  CONFIG_USB_EHCI_EXYNOS=y
>>>> @@ -109,6 +114,8 @@ CONFIG_MMC_DW_IDMAC=y
>>>>  CONFIG_MMC_DW_EXYNOS=y
>>>>  CONFIG_RTC_CLASS=y
>>>>  CONFIG_RTC_DRV_S3C=y
>>>> +CONFIG_DMADEVICES=y
>>>> +CONFIG_PL330_DMA=y
>>>>  CONFIG_COMMON_CLK_MAX77686=y
>>>>  CONFIG_EXT2_FS=y
>>>>  CONFIG_EXT3_FS=y
>>>>
>>>>
>>>
>>> With those Kconfig options enabled the kernel does not hang anymore so
>>> patch 1/3 is not needed in that case.
>>
>> Just checking: did you happen to confirm whether it's the PL330 /
>> DMADEVICES that fixes things or do you actually need the sound stuff?
>
> Sorry I should had mentioned this before. The DMADEVICES and PL330
> Kconfig are enough to avoid the kernel to hang, the sound config
> options are not actually required.

OK, makes sense.  Possibly the correct fix is just a Kconfig change
that somehow enforces that we have these two configs on.

-Doug

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Tushar Behera]

Peach-pi board has MAX98090 audio codec connected on HSI2C-7 bus.

Signed-off-by: Tushar Behera <tushar.b@samsung.com>
---
 arch/arm/boot/dts/exynos5800-peach-pi.dts |   31 +++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
index f3af207..76f5966 100644
--- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
@@ -78,9 +78,27 @@
 		pinctrl-0 = <&usb301_vbus_en>;
 		enable-active-high;
 	};
+
+	sound {
+		compatible = "google,snow-audio-max98090";
+
+		samsung,i2s-controller = <&i2s0>;
+		samsung,audio-codec = <&max98090>;
+	};
+};
+
+&i2s0 {
+	status = "okay";
 };
 
 &pinctrl_0 {
+	max98090_irq: max98090-irq {
+		samsung,pins = "gpx0-2";
+		samsung,pin-function = <0>;
+		samsung,pin-pud = <0>;
+		samsung,pin-drv = <0>;
+	};
+
 	tpm_irq: tpm-irq {
 		samsung,pins = "gpx1-0";
 		samsung,pin-function = <0>;
@@ -207,6 +225,19 @@
 	samsung,invert-vclk;
 };
 
+&hsi2c_7 {
+	status = "okay";
+
+	max98090: codec at 10 {
+		compatible = "maxim,max98090";
+		reg = <0x10>;
+		interrupts = <2 0>;
+		interrupt-parent = <&gpx0>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&max98090_irq>;
+	};
+};
+
 &hsi2c_9 {
 	status = "okay";
 	clock-frequency = <400000>;
-- 
1.7.9.5

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Doug Anderson]

Tushar,

On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
> Peach-pi board has MAX98090 audio codec connected on HSI2C-7 bus.

If you want to be a stickler about it, peach-pi actually has a
max98091.  That requires code changes to the i2c driver, though.
...and unfortunately listing two compatible strings for i2c devices is
broken.  :(


> Signed-off-by: Tushar Behera <tushar.b@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5800-peach-pi.dts |   31 +++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
> index f3af207..76f5966 100644
> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
> @@ -78,9 +78,27 @@
>                 pinctrl-0 = <&usb301_vbus_en>;
>                 enable-active-high;
>         };
> +
> +       sound {
> +               compatible = "google,snow-audio-max98090";
> +
> +               samsung,i2s-controller = <&i2s0>;
> +               samsung,audio-codec = <&max98090>;
> +       };
> +};
> +
> +&i2s0 {
> +       status = "okay";

It would be awfully nice to keep diffs between exynos5420-peach-pit
and exynos5800-peach-pi clean.  They're 99% the same.  I know this has
already gotten messed up with DP/HDMI were added, but there's no need
to make it worse.

Could you add these nodes in the same place within the dts they were
added in exynos5420-peach-pit?

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Mark Brown]

On Fri, Jun 13, 2014 at 10:03:50AM -0700, Doug Anderson wrote:
> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:

> > Peach-pi board has MAX98090 audio codec connected on HSI2C-7 bus.

> If you want to be a stickler about it, peach-pi actually has a
> max98091.  That requires code changes to the i2c driver, though.
> ...and unfortunately listing two compatible strings for i2c devices is
> broken.  :(

It is?  We should fix that if it's the case...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140613/4f707c80/attachment.sig>

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Doug Anderson]

Mark,

On Fri, Jun 13, 2014 at 10:05 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jun 13, 2014 at 10:03:50AM -0700, Doug Anderson wrote:
>> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
>
>> > Peach-pi board has MAX98090 audio codec connected on HSI2C-7 bus.
>
>> If you want to be a stickler about it, peach-pi actually has a
>> max98091.  That requires code changes to the i2c driver, though.
>> ...and unfortunately listing two compatible strings for i2c devices is
>> broken.  :(
>
> It is?  We should fix that if it's the case...

Yah, I mentioned it to Mark Rutland at the last ELC and he said he
might take a look at it, but I probably should have posted something
up to the i2c list.

I made a half-assed attempt to fix it locally in the ChromeOS but
quickly found that it was going to be a much bigger job than I had
time for...

https://chromium-review.googlesource.com/#/c/184406/

IIRC i2c_new_device didn't return an error like I thought it would,
probably trying to deal with the fact that devices might show up at a
later point in time.


Hrm, now that I think about it I wonder if the right answer is just to
call i2c_new_device for all the compatible strings even if it doesn't
return an error.  I'd have to go back and try that and re-explore this
code...

-Doug

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Doug Anderson]

Mark,

On Fri, Jun 13, 2014 at 10:13 AM, Doug Anderson <dianders@google.com> wrote:
> Mark,
>
> On Fri, Jun 13, 2014 at 10:05 AM, Mark Brown <broonie@kernel.org> wrote:
>> On Fri, Jun 13, 2014 at 10:03:50AM -0700, Doug Anderson wrote:
>>> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
>>
>>> > Peach-pi board has MAX98090 audio codec connected on HSI2C-7 bus.
>>
>>> If you want to be a stickler about it, peach-pi actually has a
>>> max98091.  That requires code changes to the i2c driver, though.
>>> ...and unfortunately listing two compatible strings for i2c devices is
>>> broken.  :(
>>
>> It is?  We should fix that if it's the case...
>
> Yah, I mentioned it to Mark Rutland at the last ELC and he said he
> might take a look at it, but I probably should have posted something
> up to the i2c list.
>
> I made a half-assed attempt to fix it locally in the ChromeOS but
> quickly found that it was going to be a much bigger job than I had
> time for...
>
> https://chromium-review.googlesource.com/#/c/184406/
>
> IIRC i2c_new_device didn't return an error like I thought it would,
> probably trying to deal with the fact that devices might show up at a
> later point in time.
>
>
> Hrm, now that I think about it I wonder if the right answer is just to
> call i2c_new_device for all the compatible strings even if it doesn't
> return an error.  I'd have to go back and try that and re-explore this
> code...

Nope, that didn't work either.  Now I remember trying that before,
too.  It doesn't like you registering two different devices with the
same address:

[    2.582539] DOUG: /i2c at 12CD0000/codec at 10 (0) max98091
[    2.587360] DOUG: /i2c at 12CD0000/codec at 10 (0) max98091
[    2.591160] DOUG: /i2c at 12CD0000/codec at 10 (1) max98090
[    2.596686] i2c i2c-7: Failed to register i2c client max98090 at 0x10 (-16)

If you hack out the check for address business:

sysfs: cannot create duplicate filename '/devices/12cd0000.i2c/i2c-7/7-0010'

Anyway, suffice to say that the i2c core needs to be extended to
handle the idea that a single device has more than one "compatible"
string.  I'll leave it to an eager reader of this thread to implement
this since we can also fix our own problem by just listing "max98091"
in "sound/soc/codecs/max98090.c" like has always been done in the
past.


-Doug

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Mark Brown]

On Fri, Jun 13, 2014 at 02:58:26PM -0700, Doug Anderson wrote:

> Anyway, suffice to say that the i2c core needs to be extended to
> handle the idea that a single device has more than one "compatible"
> string.  I'll leave it to an eager reader of this thread to implement
> this since we can also fix our own problem by just listing "max98091"
> in "sound/soc/codecs/max98090.c" like has always been done in the
> past.

Why do you need to register multiple compatible strings (I guess for
fallback purposes?).  A quick fix that is about as good is to take the
first compatible only.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140613/49590638/attachment.sig>

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Doug Anderson]

Mark,

On Fri, Jun 13, 2014 at 3:04 PM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jun 13, 2014 at 02:58:26PM -0700, Doug Anderson wrote:
>
>> Anyway, suffice to say that the i2c core needs to be extended to
>> handle the idea that a single device has more than one "compatible"
>> string.  I'll leave it to an eager reader of this thread to implement
>> this since we can also fix our own problem by just listing "max98091"
>> in "sound/soc/codecs/max98090.c" like has always been done in the
>> past.
>
> Why do you need to register multiple compatible strings (I guess for
> fallback purposes?).

I'm no expert, but I think that's part of device tree isn't it?

In the case of max98090 and max98091, they are incredibly similar
pieces of hardware (I think the max98091 simply has more microphones).
If you've got a driver for a max98090 it will work just fine for a
max98091 but you just won't get the extra microphones.

In cases like this then device tree theory says that you should list
both compatible strings: max98091 and max98090, right?  If your OS has
a driver for max98091 it will use it.  ...if it doesn't but it has a
max98090 driver it will try that one.

As far as I understand we _shouldn't_ lie and just say that we have a
max98090 when we really have a max98091.  The device tree is supposed
to describe the hardware and isn't support to care that the OS has a
driver for max98090 but not max98091.

Ironically in our case we have a driver that supports both the 98090
and the 98091 via autodetect.  However, it doesn't know about the
98091 compatible string so if you list yourself as compatible with
98091 then it won't find the driver.

> A quick fix that is about as good is to take the
> first compatible only.

That's how the code works today, actually.  ...but as per above the
current 98090 driver doesn't know about the 98091 compatible string,
so:

compatible = "maxim,max98091", "maxim,max98090";

...won't find the right driver.

--

The quick fix is to add max98091 to the max98090 driver and is what
I'd suggest in this case.  ...but I still think that the above logic
is valid and eventually the i2c core should be fixed.  Please correct
me if I'm wrong.

-Doug

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Tushar Behera]

On 06/13/2014 10:33 PM, Doug Anderson wrote:
> Tushar,
> 
> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
>> Peach-pi board has MAX98090 audio codec connected on HSI2C-7 bus.
> 
> If you want to be a stickler about it, peach-pi actually has a
> max98091.  That requires code changes to the i2c driver, though.
> ...and unfortunately listing two compatible strings for i2c devices is
> broken.  :(
> 
Hi Doug,

You are right. I checked the boot logs, the detected codec type is
MAX98091. Since both these CODECs are supported through a single driver
and the detection of chip is done during runtime, I would suggest we go
ahead with "max98090" compatible string. I will update the commit
message accordingly.

Does that sound okay to you?

> 
>> Signed-off-by: Tushar Behera <tushar.b@samsung.com>
>> ---
>>  arch/arm/boot/dts/exynos5800-peach-pi.dts |   31 +++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> index f3af207..76f5966 100644
>> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> @@ -78,9 +78,27 @@
>>                 pinctrl-0 = <&usb301_vbus_en>;
>>                 enable-active-high;
>>         };
>> +
>> +       sound {
>> +               compatible = "google,snow-audio-max98090";
>> +
>> +               samsung,i2s-controller = <&i2s0>;
>> +               samsung,audio-codec = <&max98090>;
>> +       };
>> +};
>> +
>> +&i2s0 {
>> +       status = "okay";
> 
> It would be awfully nice to keep diffs between exynos5420-peach-pit
> and exynos5800-peach-pi clean.  They're 99% the same.  I know this has
> already gotten messed up with DP/HDMI were added, but there's no need
> to make it worse.
> 

If you so desire, I will submit a patch to sort peach-pi device-tree
nodes (w.r.t. peach-pit dts file).

> Could you add these nodes in the same place within the dts they were
> added in exynos5420-peach-pit?
> 

Okay, I will add it after watchdog node.

-- 
Tushar Behera

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Doug Anderson]

Tushar,

On Mon, Jun 16, 2014 at 4:19 AM, Tushar Behera <trblinux@gmail.com> wrote:
> On 06/13/2014 10:33 PM, Doug Anderson wrote:
>> Tushar,
>>
>> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
>>> Peach-pi board has MAX98090 audio codec connected on HSI2C-7 bus.
>>
>> If you want to be a stickler about it, peach-pi actually has a
>> max98091.  That requires code changes to the i2c driver, though.
>> ...and unfortunately listing two compatible strings for i2c devices is
>> broken.  :(
>>
> Hi Doug,
>
> You are right. I checked the boot logs, the detected codec type is
> MAX98091. Since both these CODECs are supported through a single driver
> and the detection of chip is done during runtime, I would suggest we go
> ahead with "max98090" compatible string. I will update the commit
> message accordingly.
>
> Does that sound okay to you?

As per my understanding you shouldn't do this.  You should have two patches:

1. Add "max98091".  You could simply post Wonjoon's patch from
<https://chromium-review.googlesource.com/184091>

2. Change the device tree to refer to "max98091"

The argument that the "current kernel driver has a single driver" is
an argument that you're not supposed to make for device tree.  The
same device tree is supposed to work for U-Boot, BSD, or any other
platform.  On those platforms it might not be a shared driver.


> If you so desire, I will submit a patch to sort peach-pi device-tree
> nodes (w.r.t. peach-pit dts file).

Yes please.  I think there's supposed to be some official ordering of
things.  If anyone reading this has a pointer to the official sort
order of things in the device tree I'd love to see it!  ;)

-Doug

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Mark Brown]

On Mon, Jun 16, 2014 at 09:49:26AM -0700, Doug Anderson wrote:

> Yes please.  I think there's supposed to be some official ordering of
> things.  If anyone reading this has a pointer to the official sort
> order of things in the device tree I'd love to see it!  ;)

Most exact first I believe?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140616/2589c554/attachment.sig>

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Doug Anderson]

Mark,

On Mon, Jun 16, 2014 at 9:51 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jun 16, 2014 at 09:49:26AM -0700, Doug Anderson wrote:
>
>> Yes please.  I think there's supposed to be some official ordering of
>> things.  If anyone reading this has a pointer to the official sort
>> order of things in the device tree I'd love to see it!  ;)
>
> Most exact first I believe?

More specifically I'm looking for the ordering between nodes and
between properties in a node.  For instance:

1. It appears to be convention to sort children of the "pinctrl" nodes
by the first pin number in that group.  That is:

ec_spi_cs: ec-spi-cs {
  samsung,pins = "gpb1-2";
  ...
};

...comes before:
usb300_vbus_en: usb300-vbus-en {
  samsung,pins = "gph0-0";
  ...
};

...that's one really good and well-defined ordering.


2. I have no idea how general properties should be sorted.  I tend to
see "compatible" first but that's above the only rule I've seen.
Sometimes I've seen "status" first, sometimes last, sometimes
alphabetically sorted, and sometimes in a random place.  Examples:

usb301_vbus_reg: regulator-usb301 {
  compatible = "regulator-fixed";
  regulator-name = "P5.0V_USB3CON1";
  regulator-min-microvolt = <5000000>;
  regulator-max-microvolt = <5000000>;
  gpio = <&gph0 1 0>;
  pinctrl-names = "default";
  pinctrl-0 = <&usb301_vbus_en>;
  enable-active-high;
};

&hdmi {
  status = "okay";
  hpd-gpio = <&gpx3 7 GPIO_ACTIVE_HIGH>;
  pinctrl-names = "default";
  pinctrl-0 = <&hdmi_hpd_irq>;
  ddc = <&i2c_2>;
};


3. I have no idea how to sort nodes.  In theory you could say that
they should be sorted by base address:

i2s0: i2s at 03830000 {
  ...
};

hsi2c_7: i2c at 12CD0000 {
  ...
};

i2s1: i2s at 12D60000 {
  ...
};

...that works until someone argues that all of the "i2s" nodes should
be together.  It also doesn't work so well with the board convention
of using aliases to refer to things in the SoC, like:

&i2s0 {
  status = "okay";
};

&hsi2c_7 {
  status = "okay";
};

...it's not at all obvious in the board file what the base address in
the SoC was.

---

Anyway, none of this is earth shattering and it doesn't matter all
that much.  It's just nice to have an official order to make diffing
easier and also to avoid merge conflicts (unlikely someone changing
different properties will both add them in the same place in the
ordering).


-Doug

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Tushar Behera]

On Mon, Jun 16, 2014 at 10:32 PM, Doug Anderson <dianders@google.com> wrote:
> Mark,
>
> On Mon, Jun 16, 2014 at 9:51 AM, Mark Brown <broonie@kernel.org> wrote:
>> On Mon, Jun 16, 2014 at 09:49:26AM -0700, Doug Anderson wrote:
>>
>>> Yes please.  I think there's supposed to be some official ordering of
>>> things.  If anyone reading this has a pointer to the official sort
>>> order of things in the device tree I'd love to see it!  ;)
>>
>> Most exact first I believe?
>
> More specifically I'm looking for the ordering between nodes and
> between properties in a node.  For instance:
>
> 1. It appears to be convention to sort children of the "pinctrl" nodes
> by the first pin number in that group.  That is:
>
> ec_spi_cs: ec-spi-cs {
>   samsung,pins = "gpb1-2";
>   ...
> };
>
> ...comes before:
> usb300_vbus_en: usb300-vbus-en {
>   samsung,pins = "gph0-0";
>   ...
> };
>
> ...that's one really good and well-defined ordering.
>
>
> 2. I have no idea how general properties should be sorted.  I tend to
> see "compatible" first but that's above the only rule I've seen.
> Sometimes I've seen "status" first, sometimes last, sometimes
> alphabetically sorted, and sometimes in a random place.  Examples:
>
> usb301_vbus_reg: regulator-usb301 {
>   compatible = "regulator-fixed";
>   regulator-name = "P5.0V_USB3CON1";
>   regulator-min-microvolt = <5000000>;
>   regulator-max-microvolt = <5000000>;
>   gpio = <&gph0 1 0>;
>   pinctrl-names = "default";
>   pinctrl-0 = <&usb301_vbus_en>;
>   enable-active-high;
> };
>
> &hdmi {
>   status = "okay";
>   hpd-gpio = <&gpx3 7 GPIO_ACTIVE_HIGH>;
>   pinctrl-names = "default";
>   pinctrl-0 = <&hdmi_hpd_irq>;
>   ddc = <&i2c_2>;
> };
>
>
> 3. I have no idea how to sort nodes.  In theory you could say that
> they should be sorted by base address:
>
> i2s0: i2s at 03830000 {
>   ...
> };
>
> hsi2c_7: i2c at 12CD0000 {
>   ...
> };
>
> i2s1: i2s at 12D60000 {
>   ...
> };
>
> ...that works until someone argues that all of the "i2s" nodes should
> be together.  It also doesn't work so well with the board convention
> of using aliases to refer to things in the SoC, like:
>
> &i2s0 {
>   status = "okay";
> };
>
> &hsi2c_7 {
>   status = "okay";
> };
>
> ...it's not at all obvious in the board file what the base address in
> the SoC was.
>

In case where we are using only aliases in board file, sorting them
alphabetically would be reasonable. This rule would be easy to
reinforce.

> ---
>
> Anyway, none of this is earth shattering and it doesn't matter all
> that much.  It's just nice to have an official order to make diffing
> easier and also to avoid merge conflicts (unlikely someone changing
> different properties will both add them in the same place in the
> ordering).
>
>
> -Doug



-- 
Tushar Behera

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Tushar Behera]

On Mon, Jun 16, 2014 at 10:19 PM, Doug Anderson <dianders@google.com> wrote:
> Tushar,
>
> On Mon, Jun 16, 2014 at 4:19 AM, Tushar Behera <trblinux@gmail.com> wrote:
>> On 06/13/2014 10:33 PM, Doug Anderson wrote:
>>> Tushar,
>>>
>>> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
>>>> Peach-pi board has MAX98090 audio codec connected on HSI2C-7 bus.
>>>
>>> If you want to be a stickler about it, peach-pi actually has a
>>> max98091.  That requires code changes to the i2c driver, though.
>>> ...and unfortunately listing two compatible strings for i2c devices is
>>> broken.  :(
>>>
>> Hi Doug,
>>
>> You are right. I checked the boot logs, the detected codec type is
>> MAX98091. Since both these CODECs are supported through a single driver
>> and the detection of chip is done during runtime, I would suggest we go
>> ahead with "max98090" compatible string. I will update the commit
>> message accordingly.
>>
>> Does that sound okay to you?
>
> As per my understanding you shouldn't do this.  You should have two patches:
>
> 1. Add "max98091".  You could simply post Wonjoon's patch from
> <https://chromium-review.googlesource.com/184091>
>
> 2. Change the device tree to refer to "max98091"
>
> The argument that the "current kernel driver has a single driver" is
> an argument that you're not supposed to make for device tree.  The
> same device tree is supposed to work for U-Boot, BSD, or any other
> platform.  On those platforms it might not be a shared driver.
>

My argument is that the device type is getting detected during
runtime, hence there is no need to differentiate between these two.

But if you prefer that way, I will repost.

>
>> If you so desire, I will submit a patch to sort peach-pi device-tree
>> nodes (w.r.t. peach-pit dts file).
>
> Yes please.  I think there's supposed to be some official ordering of
> things.  If anyone reading this has a pointer to the official sort
> order of things in the device tree I'd love to see it!  ;)
>
> -Doug

-- 
Tushar Behera

[PATCH 3/3] ARM: dts: Enable audio support for Peach-pi board

[Doug Anderson]

Tushar,

On Mon, Jun 16, 2014 at 8:36 PM, Tushar Behera <trblinux@gmail.com> wrote:
> On Mon, Jun 16, 2014 at 10:19 PM, Doug Anderson <dianders@google.com> wrote:
>> Tushar,
>>
>> On Mon, Jun 16, 2014 at 4:19 AM, Tushar Behera <trblinux@gmail.com> wrote:
>>> On 06/13/2014 10:33 PM, Doug Anderson wrote:
>>>> Tushar,
>>>>
>>>> On Tue, Jun 10, 2014 at 10:32 PM, Tushar Behera <tushar.b@samsung.com> wrote:
>>>>> Peach-pi board has MAX98090 audio codec connected on HSI2C-7 bus.
>>>>
>>>> If you want to be a stickler about it, peach-pi actually has a
>>>> max98091.  That requires code changes to the i2c driver, though.
>>>> ...and unfortunately listing two compatible strings for i2c devices is
>>>> broken.  :(
>>>>
>>> Hi Doug,
>>>
>>> You are right. I checked the boot logs, the detected codec type is
>>> MAX98091. Since both these CODECs are supported through a single driver
>>> and the detection of chip is done during runtime, I would suggest we go
>>> ahead with "max98090" compatible string. I will update the commit
>>> message accordingly.
>>>
>>> Does that sound okay to you?
>>
>> As per my understanding you shouldn't do this.  You should have two patches:
>>
>> 1. Add "max98091".  You could simply post Wonjoon's patch from
>> <https://chromium-review.googlesource.com/184091>
>>
>> 2. Change the device tree to refer to "max98091"
>>
>> The argument that the "current kernel driver has a single driver" is
>> an argument that you're not supposed to make for device tree.  The
>> same device tree is supposed to work for U-Boot, BSD, or any other
>> platform.  On those platforms it might not be a shared driver.
>>
>
> My argument is that the device type is getting detected during
> runtime, hence there is no need to differentiate between these two.
>
> But if you prefer that way, I will repost.

Yes please.

True that it is possible to detect 98090 vs. 98091.  ...but it's also
possible to detect exynos5250 vs. exynos5420 vs. exynos5800.  ...yet
they have different compatible strings.