Fix boot-hang on Peach-pit and Enable audio

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 <[email protected]>

Signed-off-by Shaik Ameer Basha <[email protected]>

---
 drivers/clk/samsung/clk-exynos-audss.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

drivers/clk/samsung/clk-exynos-audss.c +14 -4

index 13eae14c..1542f30 100644
@@ -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

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 <[email protected]>

Signed-off-by Shaik Ameer Basha <[email protected]>

---
 drivers/clk/samsung/clk-exynos-audss.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

drivers/clk/samsung/clk-exynos-audss.c +14 -3

index 13eae14c..1542f30 100644
@@ -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;
 }
 

Signature · 1 line

1.7.9.5

Quoting Tushar Behera (2014-06-10 22:32:17)

Quoted 4 lines

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?

Quoted 8 lines

Quoting Tushar Behera (2014-06-10 22:32:17)

Quoted 4 lines

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,

Quoted 2 lines

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

Quoted 6 lines

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

Quoting Tushar Behera (2014-06-12 00:29:23)

Quoted 9 lines · Mike Turquette

Quoted 8 lines

Quoting Tushar Behera (2014-06-10 22:32:17)

Quoted 4 lines

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,

Quoted 7 lines

Quoted 2 lines

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

Quoted 12 lines

Quoted 6 lines

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.

Quoted 6 lines

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

Quoted 23 lines

Quoting Tushar Behera (2014-06-12 00:29:23)

Quoted 9 lines · Mike Turquette

Quoted 8 lines

Quoting Tushar Behera (2014-06-10 22:32:17)

Quoted 4 lines

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,

Quoted 7 lines

Quoted 2 lines

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

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 9 lines

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?

Quoted 4 lines

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.

Quoted 29 lines

Regards,
Mike

Quoted 12 lines

Quoted 6 lines

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.

Quoted 6 lines

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

Signature · 1 line

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.

Quoted 3 lines · 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.

Quoted 4 lines

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

Quoted 5 lines · Kevin Hilman

Quoted 3 lines · 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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Tushar,

Quoted 27 lines · Tushar Behera

Quoted 5 lines · Kevin Hilman

Quoted 3 lines · 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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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

Which Monday were you referring to? ;)

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

Thanks!

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-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

Quoted 34 lines · Doug Anderson

Tushar,

Quoted 30 lines · Tushar Behera

Quoted 5 lines · Kevin Hilman

Quoted 3 lines · 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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

Which Monday were you referring to? ;)

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

Quoted 4 lines

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

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,

Quoted 4 lines · 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.

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

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 <[email protected]>

Signed-off-by Shaik Ameer Basha <[email protected]>

Reported-by Kevin Hilman <[email protected]>

---
 arch/arm/boot/dts/exynos5420.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/boot/dts/exynos5420.dtsi +1 -2

index e385322..79e9119 100644
@@ -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

--

Quoted 31 lines · 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 <[email protected]>

Signed-off-by Shaik Ameer Basha <[email protected]>

Reported-by Kevin Hilman <[email protected]>

---
 arch/arm/boot/dts/exynos5420.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/boot/dts/exynos5420.dtsi +1 -2

index e385322..79e9119 100644
@@ -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

Quoted 1 line

--

Tested-by Javier Martinez Canillas <[email protected]>

Quoted 32 lines · Javier Martinez Canillas

Quoted 31 lines · 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 <[email protected]>

Signed-off-by Shaik Ameer Basha <[email protected]>

Reported-by Kevin Hilman <[email protected]>

---
 arch/arm/boot/dts/exynos5420.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/boot/dts/exynos5420.dtsi +1 -2

index e385322..79e9119 100644
@@ -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

Quoted 4 lines

Quoted 1 line

--

Tested-by Javier Martinez Canillas <[email protected]>

Kukjin,

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

--
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 <[email protected]>

Signed-off-by Shaik Ameer Basha <[email protected]>

Reported-by Kevin Hilman <[email protected]>

---
 arch/arm/boot/dts/exynos5420.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Tushar,

Quoted 8 lines · 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.

Quoted 6 lines

Signed-off-by Tushar Behera <[email protected]>

Signed-off-by Shaik Ameer Basha <[email protected]>

Reported-by Kevin Hilman <[email protected]>

---
 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 <[email protected]>

Quoted 18 lines · Doug Anderson

Tushar,

Quoted 8 lines · 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.

Quoted 6 lines

Signed-off-by Tushar Behera <[email protected]>

Signed-off-by Shaik Ameer Basha <[email protected]>

Reported-by Kevin Hilman <[email protected]>

---
 arch/arm/boot/dts/exynos5420.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 3 lines

I've tested this myself now as well.

Tested-by Doug Anderson <[email protected]>

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

Quoted 31 lines · Kevin Hilman

Quoted 18 lines · Doug Anderson

Tushar,

Quoted 8 lines · 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.

Quoted 6 lines

Signed-off-by Tushar Behera <[email protected]>

Signed-off-by Shaik Ameer Basha <[email protected]>

Reported-by Kevin Hilman <[email protected]>

---
 arch/arm/boot/dts/exynos5420.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 3 lines

I've tested this myself now as well.

Tested-by Doug Anderson <[email protected]>

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.

Quoted 1 line

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?

From ab1627127730ef4507ce96cbf95047d626bbb53f Mon Sep 17 00:00:00 2001

Quoted 32 lines · Tushar Behera

Quoted 31 lines · Kevin Hilman

Quoted 18 lines · Doug Anderson

Tushar,

Quoted 8 lines · 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.

Quoted 6 lines

Signed-off-by Tushar Behera <[email protected]>

Signed-off-by Shaik Ameer Basha <[email protected]>

Reported-by Kevin Hilman <[email protected]>

---
 arch/arm/boot/dts/exynos5420.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 3 lines

I've tested this myself now as well.

Tested-by Doug Anderson <[email protected]>

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.

Quoted 6 lines

Quoted 1 line

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.

Quoted 3 lines

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 <[email protected]>
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(-)

drivers/clk/samsung/clk-exynos5420.c +1 -1

index 61eccf0dd72f..ed175088ee7e 100644
@@ -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",

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.

Quoted 4 lines · Tushar Behera

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.

Quoted 11 lines · Kevin Hilman

Quoted 4 lines · Tushar Behera

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

Quoted 13 lines · Kevin Hilman

Quoted 11 lines · Kevin Hilman

Quoted 4 lines · Tushar Behera

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.

Quoted 5 lines

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

Quoted 26 lines · Tushar Behera

Quoted 16 lines · Kevin Hilman

Quoted 9 lines · Kevin Hilman

Quoted 5 lines · Tushar Behera

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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

Quoted 4 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 7 lines

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

Quoted 4 lines

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

Hello Tushar,

Quoted 4 lines · Tushar Behera

Quoted 4 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

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.

Quoted 10 lines

Quoted 4 lines

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.

Quoted 3 lines

--
Tushar Behera
--

Best regards,
Javier

Quoted 31 lines · Javier Martinez Canillas

Hello Tushar,

Quoted 4 lines · Tushar Behera

Quoted 4 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

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.

Quoted 10 lines

Quoted 4 lines

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.

Quoted 4 lines

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?

arch/arm/configs/exynos_defconfig +5 -0

index e07a227..d6056ab 100644
@@ -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

Hello Tushar,

Quoted 63 lines · Tushar Behera

Quoted 31 lines · Javier Martinez Canillas

Hello Tushar,

Quoted 4 lines · Tushar Behera

Quoted 4 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

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.

Quoted 10 lines

Quoted 4 lines

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.

Quoted 4 lines

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?

arch/arm/configs/exynos_defconfig +7 -0

index e07a227..d6056ab 100644
@@ -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

arch/arm/configs/exynos_defconfig CONFIG_MMC_DW_IDMAC=y · line 121

Quoted 5 lines

arch/arm/configs/exynos_defconfig +0 -0

 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.

Javier,

Quoted 66 lines · Javier Martinez Canillas

Hello Tushar,

Quoted 63 lines · Tushar Behera

Quoted 31 lines · Javier Martinez Canillas

Hello Tushar,

Quoted 4 lines · Tushar Behera

Quoted 4 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

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.

Quoted 10 lines

Quoted 4 lines

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.

Quoted 4 lines

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?

arch/arm/configs/exynos_defconfig +7 -0

index e07a227..d6056ab 100644
@@ -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

Quoted 8 lines

arch/arm/configs/exynos_defconfig CONFIG_MMC_DW_IDMAC=y · line 121

Quoted 5 lines

arch/arm/configs/exynos_defconfig +0 -0

 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?

Hello Doug,

Quoted 79 lines · Doug Anderson

Javier,

Quoted 74 lines · Javier Martinez Canillas

Hello Tushar,

Quoted 60 lines · Tushar Behera

Quoted 31 lines · Javier Martinez Canillas

Hello Tushar,

Quoted 4 lines · Tushar Behera

Quoted 4 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

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.

Quoted 10 lines

Quoted 4 lines

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.

Quoted 4 lines

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?

arch/arm/configs/exynos_defconfig +5 -0

index e07a227..d6056ab 100644
@@ -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

arch/arm/configs/exynos_defconfig CONFIG_MMC_DW_IDMAC=y · line 121

Quoted 8 lines

arch/arm/configs/exynos_defconfig +2 -0

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

Quoted 2 lines

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.

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

Signed-off-by Tushar Behera <[email protected]>

---
 arch/arm/boot/dts/exynos5800-peach-pi.dts |   31 +++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

arch/arm/boot/dts/exynos5800-peach-pi.dts +1 -0

index f3af207..76f5966 100644
@@ -78,9 +78,27 @@
                pinctrl-0 = <&usb301_vbus_en>;
                enable-active-high;
        };
+

Quoted 1 line · Tushar Behera

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

Quoted 8 lines · Doug Anderson

Quoted 1 line · Tushar Behera

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

Mark,

Quoted 11 lines · Mark Brown

Quoted 8 lines · Doug Anderson

Quoted 1 line · Tushar Behera

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

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.

Quoted 6 lines · Doug Anderson

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.

Tushar,

Quoted 1 line · Tushar Behera

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

Quoted 22 lines

Signed-off-by Tushar Behera <[email protected]>

---
 arch/arm/boot/dts/exynos5800-peach-pi.dts |   31 +++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

arch/arm/boot/dts/exynos5800-peach-pi.dts +8 -0

index f3af207..76f5966 100644
@@ -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>;
+       };
+};

arch/arm/boot/dts/exynos5800-peach-pi.dts line 91

Quoted 3 lines

arch/arm/boot/dts/exynos5800-peach-pi.dts +3 -0

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

Quoted 10 lines · Doug Anderson

Tushar,

Quoted 1 line · Tushar Behera

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?

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! ;)

Quoted 3 lines · Doug Anderson

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?

Mark,

Quoted 7 lines · Mark Brown

Quoted 3 lines · Doug Anderson

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

Tushar,

Quoted 13 lines · Tushar Behera

Quoted 10 lines · Doug Anderson

Tushar,

Quoted 1 line · Tushar Behera

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,

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 29 lines · Doug Anderson

Tushar,

Quoted 13 lines · Tushar Behera

Quoted 10 lines · Doug Anderson

Tushar,

Quoted 1 line · Tushar Behera

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,

Quoted 7 lines

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>

Quoted 7 lines

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.