| Message ID | 1371849720-14506-2-git-send-email-festevam@gmail.com |
|---|---|
| State | New |
| Headers | show |
Dear Fabio Estevam, > From: Fabio Estevam <fabio.estevam@freescale.com> > > Since commit 9e13f345887 (ASoC: sgtl5000: Let the codec acquire its > clock)it is necessary to pass a codec clock to the sgtl5000 driver. > > On some boards the sgtl5000 codec is driven via SAIF MCLK signal, so enable > it early in machine code to allow the I2C codec access to succeed. > > Since the saif mclk cannot be passed as a real clock within the clock > framework, let's pass a dummy one to the codec driver. > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > Tested on a mx28evk board running linux-next 20130621 > > arch/arm/boot/dts/imx28-apx4devkit.dts | 2 +- > arch/arm/boot/dts/imx28-evk.dts | 2 +- > arch/arm/boot/dts/imx28-m28evk.dts | 2 +- > arch/arm/mach-mxs/mach-mxs.c | 41 > ++++++++++++++++++++++++++++++++++ 4 files changed, 44 insertions(+), 3 > deletions(-) > > diff --git a/arch/arm/boot/dts/imx28-apx4devkit.dts > b/arch/arm/boot/dts/imx28-apx4devkit.dts index 43bf3c7..f26bf2f 100644 > --- a/arch/arm/boot/dts/imx28-apx4devkit.dts > +++ b/arch/arm/boot/dts/imx28-apx4devkit.dts > @@ -147,7 +147,7 @@ > reg = <0x0a>; > VDDA-supply = <®_3p3v>; > VDDIO-supply = <®_3p3v>; > - > + clocks = <&clks 65>; > }; > > pcf8563: rtc@51 { > diff --git a/arch/arm/boot/dts/imx28-evk.dts > b/arch/arm/boot/dts/imx28-evk.dts index 3637bf3..28f20b6 100644 > --- a/arch/arm/boot/dts/imx28-evk.dts > +++ b/arch/arm/boot/dts/imx28-evk.dts > @@ -193,7 +193,7 @@ > reg = <0x0a>; > VDDA-supply = <®_3p3v>; > VDDIO-supply = <®_3p3v>; > - > + clocks = <&clks 65>; > }; > > at24@51 { > diff --git a/arch/arm/boot/dts/imx28-m28evk.dts > b/arch/arm/boot/dts/imx28-m28evk.dts index 880df2f..a09c5bf 100644 > --- a/arch/arm/boot/dts/imx28-m28evk.dts > +++ b/arch/arm/boot/dts/imx28-m28evk.dts > @@ -184,7 +184,7 @@ > reg = <0x0a>; > VDDA-supply = <®_3p3v>; > VDDIO-supply = <®_3p3v>; > - > + clocks = <&clks 65>; > }; > > eeprom: eeprom@51 { > diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c > index 6298adb..57ab042 100644 > --- a/arch/arm/mach-mxs/mach-mxs.c > +++ b/arch/arm/mach-mxs/mach-mxs.c > @@ -27,6 +27,7 @@ > #include <linux/phy.h> > #include <linux/pinctrl/consumer.h> > #include <linux/sys_soc.h> > +#include <linux/stmp_device.h> > #include <asm/mach/arch.h> > #include <asm/mach/map.h> > #include <asm/mach/time.h> > @@ -59,6 +60,8 @@ > #define MXS_CLR_ADDR 0x8 > #define MXS_TOG_ADDR 0xc > > +#define SAIF0 0x100 > + > static u32 chipid; > static u32 socid; > > @@ -264,11 +267,46 @@ static inline void enable_clk_enet_out(void) > clk_prepare_enable(clk); > } > > +static void __init mxs_enable_mclk(void) > +{ > + struct device_node *np; > + void __iomem *clkctrl_base, *saif_base; > + > + np = of_find_compatible_node(NULL, NULL, "fsl,clkctrl"); > + clkctrl_base = of_iomap(np, 0); > + > + if (!clkctrl_base) > + return; > + > + /* > + * DIV_FRAC_EN, MCLK frequency = 44.1KHz * 512 = 22.5792MHz > + * This initial MCLK frequency is just to allow I2C access to the > + * SGLT5000 codec to work > + */ > + writel(0x10c0b, clkctrl_base + SAIF0); > + > + iounmap(clkctrl_base); > + of_node_put(np); > + > + np = of_find_compatible_node(NULL, NULL, "fsl,imx28-saif"); > + saif_base = of_iomap(np, 0); > + > + if (!saif_base) > + return; > + > + stmp_reset_block(saif_base); > + writel(1, saif_base + STMP_OFFSET_REG_SET); > + > + iounmap(saif_base); > + of_node_put(np); Should the SAIF block reset not happen in the driver? > +} > + > static void __init imx28_evk_init(void) > { > update_fec_mac_prop(OUI_FSL); > > mxs_saif_clkmux_select(MXS_DIGCTL_SAIF_CLKMUX_EXTMSTR0); > + mxs_enable_mclk(); > } > > static void __init imx28_evk_post_init(void) > @@ -293,6 +331,7 @@ static void __init apx4devkit_init(void) > if (IS_BUILTIN(CONFIG_PHYLIB)) > phy_register_fixup_for_uid(PHY_ID_KSZ8051, MICREL_PHY_ID_MASK, > apx4devkit_phy_fixup); > + mxs_enable_mclk(); > } > > #define ENET0_MDC__GPIO_4_0 MXS_GPIO_NR(4, 0) > @@ -483,6 +522,8 @@ static void __init mxs_machine_init(void) > of_machine_is_compatible("crystalfontz,cfa10055") || > of_machine_is_compatible("crystalfontz,cfa10057")) > crystalfontz_init(); > + else if (of_machine_is_compatible("denx,m28evk")) > + mxs_enable_mclk(); > > of_platform_populate(NULL, of_default_bus_match_table, > mxs_auxdata_lookup, parent); Best regards, Marek Vasut
On Fri, Jun 21, 2013 at 11:31 PM, Marek Vasut <marex@denx.de> wrote:
> Should the SAIF block reset not happen in the driver?
We need to clear SFTRST and CLKGATE here in order to get MCLK output.
On Sat, Jun 22, 2013 at 12:47 AM, Fabio Estevam <festevam@gmail.com> wrote: > On Fri, Jun 21, 2013 at 11:31 PM, Marek Vasut <marex@denx.de> wrote: > >> Should the SAIF block reset not happen in the driver? > > We need to clear SFTRST and CLKGATE here in order to get MCLK output. Just to clarify: the sgtl500 i2c probe happens prior to the mxs saif probe, so that's why we can't wait for the mxs-saif to turn on the clocks.
Dear Fabio Estevam, > On Sat, Jun 22, 2013 at 12:47 AM, Fabio Estevam <festevam@gmail.com> wrote: > > On Fri, Jun 21, 2013 at 11:31 PM, Marek Vasut <marex@denx.de> wrote: > >> Should the SAIF block reset not happen in the driver? > > > > We need to clear SFTRST and CLKGATE here in order to get MCLK output. > > Just to clarify: the sgtl500 i2c probe happens prior to the mxs saif > probe, so that's why we can't wait for the mxs-saif to turn on the > clocks. Yuck, I see. This is really ugly :( Just an idea here, can we not maybe defer the SGTL5000 I2C probe? Best regards, Marek Vasut
On Sat, Jun 22, 2013 at 10:24 AM, Marek Vasut <marex@denx.de> wrote: > Yuck, I see. This is really ugly :( > > Just an idea here, can we not maybe defer the SGTL5000 I2C probe? This was already discussed in the alsa-devel list, and the recommendation was that sgtl5000 should acquire its clock in i2c probe.
On Sat, Jun 22, 2013 at 11:04:41AM -0300, Fabio Estevam wrote: > On Sat, Jun 22, 2013 at 10:24 AM, Marek Vasut <marex@denx.de> wrote: > > > Yuck, I see. This is really ugly :( > > > > Just an idea here, can we not maybe defer the SGTL5000 I2C probe? > > This was already discussed in the alsa-devel list, and the > recommendation was that sgtl5000 should acquire its clock in i2c > probe. Yes, we should have sgtl5000 acquire its clock, but when it fails to do so, there is nothing wrong with deferring it to see if mxs-saif would provide the clock later. That said, I do not like the patch, and we should try to have mxs-saif driver implement and register the clock. Shawn
On Mon, Jun 24, 2013 at 4:29 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > Yes, we should have sgtl5000 acquire its clock, but when it fails to do > so, there is nothing wrong with deferring it to see if mxs-saif would > provide the clock later. I didn't manage to get mx28evk to probe audio correctly by using -EPROBE_DEFER.
On Mon, Jun 24, 2013 at 10:54:11AM -0300, Fabio Estevam wrote: > On Mon, Jun 24, 2013 at 4:29 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > > > Yes, we should have sgtl5000 acquire its clock, but when it fails to do > > so, there is nothing wrong with deferring it to see if mxs-saif would > > provide the clock later. > > I didn't manage to get mx28evk to probe audio correctly by using -EPROBE_DEFER. When you was doing that, did you have the clock that sgtl5000 is requesting registered somewhere? Shawn
On Mon, Jun 24, 2013 at 11:07 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Mon, Jun 24, 2013 at 10:54:11AM -0300, Fabio Estevam wrote: >> On Mon, Jun 24, 2013 at 4:29 AM, Shawn Guo <shawn.guo@linaro.org> wrote: >> >> > Yes, we should have sgtl5000 acquire its clock, but when it fails to do >> > so, there is nothing wrong with deferring it to see if mxs-saif would >> > provide the clock later. >> >> I didn't manage to get mx28evk to probe audio correctly by using -EPROBE_DEFER. > > When you was doing that, did you have the clock that sgtl5000 is > requesting registered somewhere? I registered it as dummy one (same as done in this patch). Then regmap_read fails (from sgtl5000 driver), as it tries to read the codec revision via I2C and the clock is not available yet. Then I changed to return a -EDEFER_PROBE in case of failure: [ 1.754258] mxs-lradc 80050000.lradc: Touchscreen not enabled. [ 1.783755] ******* reading I2C failed [ 1.789748] i2c 0-000a: Driver sgtl5000 requests probe deferral [ 1.801720] mxs-sgtl5000 sound.12: ASoC: CODEC (null) not registered [ 1.809608] mxs-sgtl5000 sound.12: snd_soc_register_card failed (-517) [ 1.816675] platform sound.12: Driver mxs-sgtl5000 requests probe deferral [ 1.825852] TCP: cubic registered [ 1.829375] NET: Registered protocol family 17 [ 1.835999] Key type dns_resolver registered [ 1.844549] mmc0: new SDHC card at address f955 [ 1.852887] registered taskstats version 1 [ 1.860535] mmcblk0: mmc0:f955 SD04G 3.69 GiB [ 1.878653] sgtl5000 0-000a: sgtl5000 revision 0x11 [ 1.887430] mmcblk0: p1 p2 p3 [ 1.894824] mxs-sgtl5000 sound.12: ASoC: CODEC (null) not registered [ 1.901468] mxs-sgtl5000 sound.12: snd_soc_register_card failed (-517) Then it succeeds in the following attemp, but snd_soc_register_card() fails.
On Mon, Jun 24, 2013 at 11:18:52AM -0300, Fabio Estevam wrote: > I registered it as dummy one (same as done in this patch). > We need to register a real one. > Then regmap_read fails (from sgtl5000 driver), as it tries to read the > codec revision via I2C and the clock is not available yet. Then I > changed to return a -EDEFER_PROBE in case of failure: > > [ 1.754258] mxs-lradc 80050000.lradc: Touchscreen not enabled. > [ 1.783755] ******* reading I2C failed > [ 1.789748] i2c 0-000a: Driver sgtl5000 requests probe deferral > [ 1.801720] mxs-sgtl5000 sound.12: ASoC: CODEC (null) not registered > [ 1.809608] mxs-sgtl5000 sound.12: snd_soc_register_card failed (-517) > [ 1.816675] platform sound.12: Driver mxs-sgtl5000 requests probe deferral > [ 1.825852] TCP: cubic registered > [ 1.829375] NET: Registered protocol family 17 > [ 1.835999] Key type dns_resolver registered > [ 1.844549] mmc0: new SDHC card at address f955 > [ 1.852887] registered taskstats version 1 > [ 1.860535] mmcblk0: mmc0:f955 SD04G 3.69 GiB > [ 1.878653] sgtl5000 0-000a: sgtl5000 revision 0x11 > [ 1.887430] mmcblk0: p1 p2 p3 > [ 1.894824] mxs-sgtl5000 sound.12: ASoC: CODEC (null) not registered > [ 1.901468] mxs-sgtl5000 sound.12: snd_soc_register_card failed (-517) > > Then it succeeds in the following attemp, but snd_soc_register_card() fails. This is expected, because CODEC (sgtl5000) gets a probe deferral. snd_soc_register_card() will always return -EDEFER_PROBE, until sgtl5000 gets the clock to finish its probe. I will try to see if I can cook up a patch for it. Shawn
On Mon, Jun 24, 2013 at 11:18:52AM -0300, Fabio Estevam wrote: > On Mon, Jun 24, 2013 at 11:07 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > > On Mon, Jun 24, 2013 at 10:54:11AM -0300, Fabio Estevam wrote: > >> On Mon, Jun 24, 2013 at 4:29 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > >> > >> > Yes, we should have sgtl5000 acquire its clock, but when it fails to do > >> > so, there is nothing wrong with deferring it to see if mxs-saif would > >> > provide the clock later. > >> > >> I didn't manage to get mx28evk to probe audio correctly by using -EPROBE_DEFER. > > > > When you was doing that, did you have the clock that sgtl5000 is > > requesting registered somewhere? > > I registered it as dummy one (same as done in this patch). > > Then regmap_read fails (from sgtl5000 driver), as it tries to read the > codec revision via I2C and the clock is not available yet. Then I > changed to return a -EDEFER_PROBE in case of failure: > > [ 1.754258] mxs-lradc 80050000.lradc: Touchscreen not enabled. > [ 1.783755] ******* reading I2C failed > [ 1.789748] i2c 0-000a: Driver sgtl5000 requests probe deferral > [ 1.801720] mxs-sgtl5000 sound.12: ASoC: CODEC (null) not registered > [ 1.809608] mxs-sgtl5000 sound.12: snd_soc_register_card failed (-517) > [ 1.816675] platform sound.12: Driver mxs-sgtl5000 requests probe deferral > [ 1.825852] TCP: cubic registered > [ 1.829375] NET: Registered protocol family 17 > [ 1.835999] Key type dns_resolver registered > [ 1.844549] mmc0: new SDHC card at address f955 > [ 1.852887] registered taskstats version 1 > [ 1.860535] mmcblk0: mmc0:f955 SD04G 3.69 GiB > [ 1.878653] sgtl5000 0-000a: sgtl5000 revision 0x11 Oh, look at this line, you have the clock work for sgtl5000. So it seems that sgtl5000_i2c_probe() fails at somewhere else on the second try. Shawn > [ 1.887430] mmcblk0: p1 p2 p3 > [ 1.894824] mxs-sgtl5000 sound.12: ASoC: CODEC (null) not registered > [ 1.901468] mxs-sgtl5000 sound.12: snd_soc_register_card failed (-517) > > Then it succeeds in the following attemp, but snd_soc_register_card() fails.
On Tue, Jun 25, 2013 at 5:12 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > Oh, look at this line, you have the clock work for sgtl5000. So it > seems that sgtl5000_i2c_probe() fails at somewhere else on the second > try. Yes, I noticed that. Codec revision is read succesfully in the second attempt, but snd_soc_register_card() still returns -517 (-EPROBE_DEFER).
On Tue, Jun 25, 2013 at 07:28:28AM -0300, Fabio Estevam wrote: > On Tue, Jun 25, 2013 at 5:12 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > > > Oh, look at this line, you have the clock work for sgtl5000. So it > > seems that sgtl5000_i2c_probe() fails at somewhere else on the second > > try. > > Yes, I noticed that. Codec revision is read succesfully in the second > attempt, but > snd_soc_register_card() still returns -517 (-EPROBE_DEFER). It seems to me that it fails at sgtl5000_fill_defaults(). Did you test it on imx28 when your commit af8ee11 (ASoC: sgtl5000: Fix driver probe after reset) goes in? Shawn
On Tue, Jun 25, 2013 at 9:19 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > It seems to me that it fails at sgtl5000_fill_defaults(). Did you > test it on imx28 when your commit af8ee11 (ASoC: sgtl5000: Fix driver probe > after reset) goes in? I am running linux-next, which contains af8ee11.
On Tue, Jun 25, 2013 at 09:22:10AM -0300, Fabio Estevam wrote: > On Tue, Jun 25, 2013 at 9:19 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > > > It seems to me that it fails at sgtl5000_fill_defaults(). Did you > > test it on imx28 when your commit af8ee11 (ASoC: sgtl5000: Fix driver probe > > after reset) goes in? > > I am running linux-next, which contains af8ee11. What I'm saying is why you are seeing error "snd_soc_register_card failed (-517)" on the second attempt is just because of commit af8ee11. IOW, I doubt you did not test commit af8ee11 on imx28 when you was submitting the patch. Shawn
On Tue, Jun 25, 2013 at 08:52:42PM +0800, Shawn Guo wrote: > On Tue, Jun 25, 2013 at 09:22:10AM -0300, Fabio Estevam wrote: > > On Tue, Jun 25, 2013 at 9:19 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > > > > > It seems to me that it fails at sgtl5000_fill_defaults(). Did you > > > test it on imx28 when your commit af8ee11 (ASoC: sgtl5000: Fix driver probe > > > after reset) goes in? > > > > I am running linux-next, which contains af8ee11. > > What I'm saying is why you are seeing error "snd_soc_register_card > failed (-517)" on the second attempt is just because of commit af8ee11. > IOW, I doubt you did not test commit af8ee11 on imx28 when you was s/doubt/guess Shawn > submitting the patch.
diff --git a/arch/arm/boot/dts/imx28-apx4devkit.dts b/arch/arm/boot/dts/imx28-apx4devkit.dts index 43bf3c7..f26bf2f 100644 --- a/arch/arm/boot/dts/imx28-apx4devkit.dts +++ b/arch/arm/boot/dts/imx28-apx4devkit.dts @@ -147,7 +147,7 @@ reg = <0x0a>; VDDA-supply = <®_3p3v>; VDDIO-supply = <®_3p3v>; - + clocks = <&clks 65>; }; pcf8563: rtc@51 { diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts index 3637bf3..28f20b6 100644 --- a/arch/arm/boot/dts/imx28-evk.dts +++ b/arch/arm/boot/dts/imx28-evk.dts @@ -193,7 +193,7 @@ reg = <0x0a>; VDDA-supply = <®_3p3v>; VDDIO-supply = <®_3p3v>; - + clocks = <&clks 65>; }; at24@51 { diff --git a/arch/arm/boot/dts/imx28-m28evk.dts b/arch/arm/boot/dts/imx28-m28evk.dts index 880df2f..a09c5bf 100644 --- a/arch/arm/boot/dts/imx28-m28evk.dts +++ b/arch/arm/boot/dts/imx28-m28evk.dts @@ -184,7 +184,7 @@ reg = <0x0a>; VDDA-supply = <®_3p3v>; VDDIO-supply = <®_3p3v>; - + clocks = <&clks 65>; }; eeprom: eeprom@51 { diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c index 6298adb..57ab042 100644 --- a/arch/arm/mach-mxs/mach-mxs.c +++ b/arch/arm/mach-mxs/mach-mxs.c @@ -27,6 +27,7 @@ #include <linux/phy.h> #include <linux/pinctrl/consumer.h> #include <linux/sys_soc.h> +#include <linux/stmp_device.h> #include <asm/mach/arch.h> #include <asm/mach/map.h> #include <asm/mach/time.h> @@ -59,6 +60,8 @@ #define MXS_CLR_ADDR 0x8 #define MXS_TOG_ADDR 0xc +#define SAIF0 0x100 + static u32 chipid; static u32 socid; @@ -264,11 +267,46 @@ static inline void enable_clk_enet_out(void) clk_prepare_enable(clk); } +static void __init mxs_enable_mclk(void) +{ + struct device_node *np; + void __iomem *clkctrl_base, *saif_base; + + np = of_find_compatible_node(NULL, NULL, "fsl,clkctrl"); + clkctrl_base = of_iomap(np, 0); + + if (!clkctrl_base) + return; + + /* + * DIV_FRAC_EN, MCLK frequency = 44.1KHz * 512 = 22.5792MHz + * This initial MCLK frequency is just to allow I2C access to the + * SGLT5000 codec to work + */ + writel(0x10c0b, clkctrl_base + SAIF0); + + iounmap(clkctrl_base); + of_node_put(np); + + np = of_find_compatible_node(NULL, NULL, "fsl,imx28-saif"); + saif_base = of_iomap(np, 0); + + if (!saif_base) + return; + + stmp_reset_block(saif_base); + writel(1, saif_base + STMP_OFFSET_REG_SET); + + iounmap(saif_base); + of_node_put(np); +} + static void __init imx28_evk_init(void) { update_fec_mac_prop(OUI_FSL); mxs_saif_clkmux_select(MXS_DIGCTL_SAIF_CLKMUX_EXTMSTR0); + mxs_enable_mclk(); } static void __init imx28_evk_post_init(void) @@ -293,6 +331,7 @@ static void __init apx4devkit_init(void) if (IS_BUILTIN(CONFIG_PHYLIB)) phy_register_fixup_for_uid(PHY_ID_KSZ8051, MICREL_PHY_ID_MASK, apx4devkit_phy_fixup); + mxs_enable_mclk(); } #define ENET0_MDC__GPIO_4_0 MXS_GPIO_NR(4, 0) @@ -483,6 +522,8 @@ static void __init mxs_machine_init(void) of_machine_is_compatible("crystalfontz,cfa10055") || of_machine_is_compatible("crystalfontz,cfa10057")) crystalfontz_init(); + else if (of_machine_is_compatible("denx,m28evk")) + mxs_enable_mclk(); of_platform_populate(NULL, of_default_bus_match_table, mxs_auxdata_lookup, parent);