Re: [PATCH 2/2] ARM: clk: imx35 sound admux_gate bugfix

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> >  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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]