| Message ID | 1364385420.5442.12.camel@pizza.hi.pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Philipp, On Wed, Mar 27, 2013 at 12:57:00PM +0100, Philipp Zabel wrote: > Hi Markus, > > Am Mittwoch, den 27.03.2013, 12:22 +0100 schrieb Markus Pargmann: > > imx-ssi needs admux_gate clock to work on imx35. This patch registers > > this clock and enables it in the imx-ssi driver. > > I have no idea what the admux_gate really is, but the ssi driver doesn't > seem like the right place. Isn't this more closely related to the > imx-audmux.c driver? Actually admux clock is not audmux clock, that one is seperated. So I am not sure where this belongs. As imx-ssi was not working without it, I added it here. But you are right, I also spoke with Sascha and I will simply move the clock enable to imx35-dt.c. > > > A reboot without this patch results in a failing AC97 for example on > > pcm043. > > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > Cc: stable@vger.kernel.org > > --- > > arch/arm/mach-imx/clk-imx25.c | 6 ++++-- > > arch/arm/mach-imx/clk-imx27.c | 6 ++++-- > > arch/arm/mach-imx/clk-imx31.c | 6 ++++-- > > arch/arm/mach-imx/clk-imx35.c | 6 ++++-- > > arch/arm/mach-imx/clk-imx51-imx53.c | 9 ++++++--- > > sound/soc/fsl/imx-ssi.c | 16 +++++++++++++++- > > sound/soc/fsl/imx-ssi.h | 2 ++ > > 7 files changed, 39 insertions(+), 12 deletions(-) > > > > diff --git a/arch/arm/mach-imx/clk-imx25.c b/arch/arm/mach-imx/clk-imx25.c > > index 69858c7..174ff8b 100644 > > --- a/arch/arm/mach-imx/clk-imx25.c > > +++ b/arch/arm/mach-imx/clk-imx25.c > > @@ -285,8 +285,10 @@ int __init mx25_clocks_init(void) > > clk_register_clkdev(clk[lcdc_ipg], "ipg", "imx21-fb.0"); > > clk_register_clkdev(clk[lcdc_ahb], "ahb", "imx21-fb.0"); > > clk_register_clkdev(clk[wdt_ipg], NULL, "imx2-wdt.0"); > > - clk_register_clkdev(clk[ssi1_ipg], NULL, "imx-ssi.0"); > > - clk_register_clkdev(clk[ssi2_ipg], NULL, "imx-ssi.1"); > > + clk_register_clkdev(clk[ssi1_ipg], "ipg", "imx-ssi.0"); > > + clk_register_clkdev(clk[ssi2_ipg], "ipg", "imx-ssi.1"); > > + clk_register_clkdev(clk[dummy], "admux", "imx-ssi.0"); > > + clk_register_clkdev(clk[dummy], "admux", "imx-ssi.1"); > > clk_register_clkdev(clk[esdhc1_ipg_per], "per", "sdhci-esdhc-imx25.0"); > > clk_register_clkdev(clk[esdhc1_ipg], "ipg", "sdhci-esdhc-imx25.0"); > > clk_register_clkdev(clk[esdhc1_ahb], "ahb", "sdhci-esdhc-imx25.0"); > > diff --git a/arch/arm/mach-imx/clk-imx27.c b/arch/arm/mach-imx/clk-imx27.c > > index 30b3242..439d5bd 100644 > > --- a/arch/arm/mach-imx/clk-imx27.c > > +++ b/arch/arm/mach-imx/clk-imx27.c > > @@ -252,8 +252,10 @@ int __init mx27_clocks_init(unsigned long fref) > > clk_register_clkdev(clk[usb_div], "per", "mxc-ehci.2"); > > clk_register_clkdev(clk[usb_ipg_gate], "ipg", "mxc-ehci.2"); > > clk_register_clkdev(clk[usb_ahb_gate], "ahb", "mxc-ehci.2"); > > - clk_register_clkdev(clk[ssi1_ipg_gate], NULL, "imx-ssi.0"); > > - clk_register_clkdev(clk[ssi2_ipg_gate], NULL, "imx-ssi.1"); > > + clk_register_clkdev(clk[ssi1_ipg_gate], "ipg", "imx-ssi.0"); > > + clk_register_clkdev(clk[ssi2_ipg_gate], "ipg", "imx-ssi.1"); > > + clk_register_clkdev(clk[dummy], "admux", "imx-ssi.0"); > > + clk_register_clkdev(clk[dummy], "admux", "imx-ssi.1"); > > clk_register_clkdev(clk[nfc_baud_gate], NULL, "imx27-nand.0"); > > clk_register_clkdev(clk[vpu_baud_gate], "per", "coda-imx27.0"); > > clk_register_clkdev(clk[vpu_ahb_gate], "ahb", "coda-imx27.0"); > > diff --git a/arch/arm/mach-imx/clk-imx31.c b/arch/arm/mach-imx/clk-imx31.c > > index b5b65f3..478b33e 100644 > > --- a/arch/arm/mach-imx/clk-imx31.c > > +++ b/arch/arm/mach-imx/clk-imx31.c > > @@ -171,8 +171,10 @@ int __init mx31_clocks_init(unsigned long fref) > > clk_register_clkdev(clk[owire_gate], NULL, "mxc_w1.0"); > > clk_register_clkdev(clk[sdhc1_gate], NULL, "imx31-mmc.0"); > > clk_register_clkdev(clk[sdhc2_gate], NULL, "imx31-mmc.1"); > > - clk_register_clkdev(clk[ssi1_gate], NULL, "imx-ssi.0"); > > - clk_register_clkdev(clk[ssi2_gate], NULL, "imx-ssi.1"); > > + clk_register_clkdev(clk[ssi1_gate], "ipg", "imx-ssi.0"); > > + clk_register_clkdev(clk[ssi2_gate], "ipg", "imx-ssi.1"); > > + clk_register_clkdev(clk[dummy], "admux", "imx-ssi.0"); > > + clk_register_clkdev(clk[dummy], "admux", "imx-ssi.1"); > > clk_register_clkdev(clk[firi_gate], "firi", NULL); > > clk_register_clkdev(clk[ata_gate], NULL, "pata_imx"); > > clk_register_clkdev(clk[rtic_gate], "rtic", NULL); > > diff --git a/arch/arm/mach-imx/clk-imx35.c b/arch/arm/mach-imx/clk-imx35.c > > index b95898a..4ab18ac 100644 > > --- a/arch/arm/mach-imx/clk-imx35.c > > +++ b/arch/arm/mach-imx/clk-imx35.c > > @@ -233,8 +233,10 @@ int __init mx35_clocks_init(void) > > clk_register_clkdev(clk[kpp_gate], NULL, "imx-keypad"); > > clk_register_clkdev(clk[owire_gate], NULL, "mxc_w1"); > > clk_register_clkdev(clk[sdma_gate], NULL, "imx35-sdma"); > > - clk_register_clkdev(clk[ssi1_gate], NULL, "imx-ssi.0"); > > - clk_register_clkdev(clk[ssi2_gate], NULL, "imx-ssi.1"); > > + clk_register_clkdev(clk[ssi1_gate], "ipg", "imx-ssi.0"); > > + clk_register_clkdev(clk[ssi2_gate], "ipg", "imx-ssi.1"); > > + clk_register_clkdev(clk[admux_gate], "admux", "imx-ssi.0"); > > + clk_register_clkdev(clk[admux_gate], "admux", "imx-ssi.1"); > > /* i.mx35 has the i.mx21 type uart */ > > clk_register_clkdev(clk[uart1_gate], "per", "imx21-uart.0"); > > clk_register_clkdev(clk[ipg], "ipg", "imx21-uart.0"); > > diff --git a/arch/arm/mach-imx/clk-imx51-imx53.c b/arch/arm/mach-imx/clk-imx51-imx53.c > > index 0f39f8c..b413a1e 100644 > > --- a/arch/arm/mach-imx/clk-imx51-imx53.c > > +++ b/arch/arm/mach-imx/clk-imx51-imx53.c > > @@ -275,9 +275,12 @@ static void __init mx5_clocks_common_init(unsigned long rate_ckil, > > clk_register_clkdev(clk[usboh3_gate], "ipg", "imx-udc-mx51"); > > clk_register_clkdev(clk[usboh3_gate], "ahb", "imx-udc-mx51"); > > clk_register_clkdev(clk[nfc_gate], NULL, "imx51-nand"); > > - clk_register_clkdev(clk[ssi1_ipg_gate], NULL, "imx-ssi.0"); > > - clk_register_clkdev(clk[ssi2_ipg_gate], NULL, "imx-ssi.1"); > > - clk_register_clkdev(clk[ssi3_ipg_gate], NULL, "imx-ssi.2"); > > + clk_register_clkdev(clk[ssi1_ipg_gate], "ipg", "imx-ssi.0"); > > + clk_register_clkdev(clk[ssi2_ipg_gate], "ipg", "imx-ssi.1"); > > + clk_register_clkdev(clk[ssi3_ipg_gate], "ipg", "imx-ssi.2"); > > + clk_register_clkdev(clk[dummy], "admux", "imx-ssi.0"); > > + clk_register_clkdev(clk[dummy], "admux", "imx-ssi.1"); > > + clk_register_clkdev(clk[dummy], "admux", "imx-ssi.2"); > > I think those should be removed completely. If really needed, it should > rather be added to the device tree for i.MX5: Oh yes, I grepped for imx-ssi.. However next patch version will not touch any of the other imxs code. > > --- a/arch/arm/boot/dts/imx53.dtsi > +++ b/arch/arm/boot/dts/imx53.dtsi > @@ -162,7 +162,8 @@ > compatible = "fsl,imx53-ssi", "fsl,imx21-ssi"; > reg = <0x50014000 0x4000>; > interrupts = <30>; > - clocks = <&clks 49>; > + clocks = <&clks 49>, <&clks 0>; > + clock-names = "ipg", "admux"; > fsl,fifo-depth = <15>; > fsl,ssi-dma-events = <25 24 23 22>; /* TX0 RX0 TX1 RX1 */ > status = "disabled"; > @@ -791,7 +792,8 @@ > compatible = "fsl,imx53-ssi", "fsl,imx21-ssi"; > reg = <0x63fcc000 0x4000>; > interrupts = <29>; > - clocks = <&clks 48>; > + clocks = <&clks 48>, <&clks 0>; > + clock-names = "ipg", "admux"; > fsl,fifo-depth = <15>; > fsl,ssi-dma-events = <29 28 27 26>; /* TX0 RX0 TX1 RX1 */ > status = "disabled"; > @@ -815,7 +817,8 @@ > compatible = "fsl,imx53-ssi", "fsl,imx21-ssi"; > reg = <0x63fe8000 0x4000>; > interrupts = <96>; > - clocks = <&clks 50>; > + clocks = <&clks 50>, <&clks 0>; > + clock-names = "ipg", "admux"; > fsl,fifo-depth = <15>; > fsl,ssi-dma-events = <47 46 45 44>; /* TX0 RX0 TX1 RX1 */ > status = "disabled"; > > But again, I'm not convinced the ssi driver is the right place to enable > the "admux" gate. > > > clk_register_clkdev(clk[ssi_ext1_gate], "ssi_ext1", NULL); > > clk_register_clkdev(clk[ssi_ext2_gate], "ssi_ext2", NULL); > > clk_register_clkdev(clk[sdma_gate], NULL, "imx35-sdma"); > > diff --git a/sound/soc/fsl/imx-ssi.c b/sound/soc/fsl/imx-ssi.c > > index 55464a5..9f96785 100644 > > --- a/sound/soc/fsl/imx-ssi.c > > +++ b/sound/soc/fsl/imx-ssi.c > > @@ -535,7 +535,7 @@ static int imx_ssi_probe(struct platform_device *pdev) > > > > ssi->irq = platform_get_irq(pdev, 0); > > > > - ssi->clk = devm_clk_get(&pdev->dev, NULL); > > + ssi->clk = devm_clk_get(&pdev->dev, "ipg"); > > if (IS_ERR(ssi->clk)) { > > ret = PTR_ERR(ssi->clk); > > dev_err(&pdev->dev, "Cannot get the clock: %d\n", > > @@ -544,6 +544,15 @@ static int imx_ssi_probe(struct platform_device *pdev) > > } > > clk_prepare_enable(ssi->clk); > > > > + ssi->admux_clk = devm_clk_get(&pdev->dev, "admux"); > > + if (IS_ERR(ssi->admux_clk)) { > > + if (PTR_ERR(ssi->admux_clk) == -ENOMEM) > > + goto failed_admux_clk_mem; > > + ssi->admux_clk = NULL; > > + } else { > > + clk_prepare_enable(ssi->admux_clk); > > + } > > + > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > if (!res) { > > ret = -ENODEV; > > @@ -631,6 +640,9 @@ failed_pdev_fiq_alloc: > > failed_register: > > release_mem_region(res->start, resource_size(res)); > > failed_get_resource: > > + if (ssi->admux_clk) > > + clk_disable_unprepare(ssi->admux_clk); > > +failed_admux_clk_mem: > > clk_disable_unprepare(ssi->clk); > > failed_clk: > > > > @@ -652,6 +664,8 @@ static int imx_ssi_remove(struct platform_device *pdev) > > > > release_mem_region(res->start, resource_size(res)); > > clk_disable_unprepare(ssi->clk); > > + if (ssi->admux_clk) > > + clk_disable_unprepare(ssi->admux_clk); > > > > return 0; > > } > > diff --git a/sound/soc/fsl/imx-ssi.h b/sound/soc/fsl/imx-ssi.h > > index dc114bd..3dfbd92 100644 > > --- a/sound/soc/fsl/imx-ssi.h > > +++ b/sound/soc/fsl/imx-ssi.h > > @@ -194,6 +194,8 @@ struct imx_ssi { > > > > struct snd_soc_dai *imx_ac97; > > struct clk *clk; > > + /* imx35 has a admux clock that has to be active for sound to work */ > > + struct clk *admux_clk; > > void __iomem *base; > > int irq; > > int fiq_enable; > > regards > Philipp > > Thanks, Markus
On Wed, Mar 27, 2013 at 12:57:00PM +0100, Philipp Zabel wrote: > Am Mittwoch, den 27.03.2013, 12:22 +0100 schrieb Markus Pargmann: > > imx-ssi needs admux_gate clock to work on imx35. This patch registers > > this clock and enables it in the imx-ssi driver. > I have no idea what the admux_gate really is, but the ssi driver doesn't > seem like the right place. Isn't this more closely related to the > imx-audmux.c driver? This is a patch to the ASoC driver? I just ignored it since it said it was an ARM patch... Anyway, what I'd expect should be happening here is that the clock API walks up the tree enabling supply clocks - that would mean that either that code isn't there or there's a missing parent child relationship in the data.
Hi Mark, On Wed, Mar 27, 2013 at 12:37:00PM +0000, Mark Brown wrote: > On Wed, Mar 27, 2013 at 12:57:00PM +0100, Philipp Zabel wrote: > > Am Mittwoch, den 27.03.2013, 12:22 +0100 schrieb Markus Pargmann: > > > imx-ssi needs admux_gate clock to work on imx35. This patch registers > > > this clock and enables it in the imx-ssi driver. > > > I have no idea what the admux_gate really is, but the ssi driver doesn't > > seem like the right place. Isn't this more closely related to the > > imx-audmux.c driver? > > This is a patch to the ASoC driver? I just ignored it since it said it > was an ARM patch... Anyway, what I'd expect should be happening here is > that the clock API walks up the tree enabling supply clocks - that would > mean that either that code isn't there or there's a missing parent child > relationship in the data. It changes some things in imx-ssi, sorry, perhaps I should have splitted it into two patches. admux clock does not seem to be a physical parent of the ssi clock, so I don't think it should be a parent in the clock tree. But imx-ssi (or some other evice) needs this clock to work. As mentioned in the other response, I will put it directly into clk-imx35.c so this won't be a patch for ASoC in the next version. Regards, Markus
On Wed, Mar 27, 2013 at 04:04:20PM +0100, Markus Pargmann wrote: > On Wed, Mar 27, 2013 at 12:37:00PM +0000, Mark Brown wrote: > > This is a patch to the ASoC driver? I just ignored it since it said it > > was an ARM patch... Anyway, what I'd expect should be happening here is > > that the clock API walks up the tree enabling supply clocks - that would > > mean that either that code isn't there or there's a missing parent child > > relationship in the data. > It changes some things in imx-ssi, sorry, perhaps I should have > splitted it into two patches. > admux clock does not seem to be a physical parent of the ssi clock, so > I don't think it should be a parent in the clock tree. But imx-ssi (or > some other evice) needs this clock to work. As mentioned in the other > response, I will put it directly into clk-imx35.c so this won't be a > patch for ASoC in the next version. If it's the IP that needs the clock then I'd expect the IP to be requesting it so changing the driver does make sense. Do we actually know what the clock does?
On Wed, Mar 27, 2013 at 03:50:11PM +0000, Mark Brown wrote: > On Wed, Mar 27, 2013 at 04:04:20PM +0100, Markus Pargmann wrote: > > On Wed, Mar 27, 2013 at 12:37:00PM +0000, Mark Brown wrote: > > > > This is a patch to the ASoC driver? I just ignored it since it said it > > > was an ARM patch... Anyway, what I'd expect should be happening here is > > > that the clock API walks up the tree enabling supply clocks - that would > > > mean that either that code isn't there or there's a missing parent child > > > relationship in the data. > > > It changes some things in imx-ssi, sorry, perhaps I should have > > splitted it into two patches. > > > admux clock does not seem to be a physical parent of the ssi clock, so > > I don't think it should be a parent in the clock tree. But imx-ssi (or > > some other evice) needs this clock to work. As mentioned in the other > > response, I will put it directly into clk-imx35.c so this won't be a > > patch for ASoC in the next version. > > If it's the IP that needs the clock then I'd expect the IP to be > requesting it so changing the driver does make sense. Do we actually > know what the clock does? No we don't know exactly. imx35 reference manual does not give any further explanation about admux. But I just found this old patch: http://lists.infradead.org/pipermail/linux-arm-kernel/2009-November/003843.html So I tested to register admux as audmux clock for imx-audmux, which seems to solve the problem. However, I am not sure why the old patch of Sascha was not applied. Regards, Markus
--- a/arch/arm/boot/dts/imx53.dtsi +++ b/arch/arm/boot/dts/imx53.dtsi @@ -162,7 +162,8 @@ compatible = "fsl,imx53-ssi", "fsl,imx21-ssi"; reg = <0x50014000 0x4000>; interrupts = <30>; - clocks = <&clks 49>; + clocks = <&clks 49>, <&clks 0>; + clock-names = "ipg", "admux"; fsl,fifo-depth = <15>; fsl,ssi-dma-events = <25 24 23 22>; /* TX0 RX0 TX1 RX1 */ status = "disabled"; @@ -791,7 +792,8 @@ compatible = "fsl,imx53-ssi", "fsl,imx21-ssi"; reg = <0x63fcc000 0x4000>; interrupts = <29>; - clocks = <&clks 48>; + clocks = <&clks 48>, <&clks 0>; + clock-names = "ipg", "admux"; fsl,fifo-depth = <15>; fsl,ssi-dma-events = <29 28 27 26>; /* TX0 RX0 TX1 RX1 */ status = "disabled"; @@ -815,7 +817,8 @@ compatible = "fsl,imx53-ssi", "fsl,imx21-ssi"; reg = <0x63fe8000 0x4000>; interrupts = <96>; - clocks = <&clks 50>; + clocks = <&clks 50>, <&clks 0>; + clock-names = "ipg", "admux"; fsl,fifo-depth = <15>; fsl,ssi-dma-events = <47 46 45 44>; /* TX0 RX0 TX1 RX1 */ status = "disabled";