| Message ID | 1344207819-3415-4-git-send-email-anton.vorontsov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Aug 5, 2012 at 6:03 PM, Anton Vorontsov <anton.vorontsov@linaro.org> wrote: > The driver uses platform-specific mxc_set_irq_fiq() with the VIRQ cookie > passed to it, so it's pretty clear that the driver is absolutely sure > that the FIQ is routed via platform-specific IC, and that the cookie can > be used to mask/unmask FIQs. So, let's switch to the genirq routines, > since we're about to remove FIQ-specific variants. I have a semi-related question about this; Firstly, I was planning on (re-)submitting a patch for the arch/arm/plat-mxc/ssi-fiq.S code which made it build in ARM mode (since the code isn't Thumb compatible for various reasons) as it was a blocker for a Thumb2-compiled kernel. Since the code was only needed on ARM-capable processors it wouldn't cause a problem. Sascha signed off on this a long while back and I've been testing it on all my internal kernel versions, and I don't see any ill effects (that said I don't have an i.MX28 or so to really verify it, I can't see why it would not work). I realise this is redundant right now, anyway, since it's only really enabled on imx_v4_v5 configs and they don't support Thumb2 kernels anyway. What might be worth submitting is a switch to add the ".arm" directive anyway simply for correctness since it could never be compiled for Thumb anyway. We all know what gnu as is like. Looking at it again on the back of these patches, I noticed the ssi-fiq.S code is compiled in when SND_IMX_SOC is enabled - of course, it's only needed in the kernel if SND_SOC_IMX_PCM_FIQ is enabled (the code that uses the FIQ stuff is only compiled then) but here on the Efika MX builds it's being built, and I noticed it when it broke my build because of the above. I'm therefore going to submit a patch which changes the ifdef SND_SOC_IMX to ifdef SND_SOC_IMX_PCM_FIQ so it's not compiled in unless absolutely necessary. However, there was a rumble that this code may disappear or be reworked in the future making this also quite redundant. Since it's not in the imx_v6_v7_defconfig anyway, I'm sure this only didn't get noticed because nobody's building Thumb2 kernels and nobody is trying configs with audio enabled anyway.. This begs the question, could there be something somewhere hidden deep in the kernel that is enabled by default or in some config somewhere that requires "select FIQ" in it's Kconfig entry, but isn't being enabled? On i.MX the only thing turning it on is that code, but other ARM arch enable it by default. Since things have been moved to more generic routines I can't in my mind guarantee that such a patch would be well tested here since I would be relying on symbols missing or defines not there anymore.. I have no real way to ensure that it would work on boards I don't have. So, is the first patch (ssi-fiq.S .arm) worth it? I think the SND_SOC_IMX_PCM_FIQ patch is worth it for v6_v7 systems anyway, but maybe this should have been caught sooner, so should I update the defconfig to enable some kind of audio bus support (Babbage has it in the DT so is a needful thing for testing, I figure..)? And does anyone forsee any problems with that option changing and the only "user" of "FIQ" in the Kconfigs going away now all the FIQ-specific symbols went away outside of the generic irq subsystem? I will probably throw out all three today anyway, but I thought it would make a good discussion anyway.
On Mon, Aug 06, 2012 at 10:19:50AM -0500, Matt Sealey wrote: > it's not compiled in unless absolutely necessary. However, there was a > rumble that this code may disappear or be reworked in the future > making this also quite redundant. Since it's not in the There's no point in the FIQ driver if the DMA drivers are supported.
On Mon, Aug 6, 2012 at 10:49 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Aug 06, 2012 at 10:19:50AM -0500, Matt Sealey wrote: > >> it's not compiled in unless absolutely necessary. However, there was a >> rumble that this code may disappear or be reworked in the future >> making this also quite redundant. Since it's not in the > > There's no point in the FIQ driver if the DMA drivers are supported. http://patchwork.ozlabs.org/patch/128853/ Russell's sage advise; "It's worth pointing out that people end up using FIQs for certain things because the hardware requires you to do it. So if a platform is using them, they're probably not doing it out of choice, but are doing it because it's a baseline requirement to get something working." I don't recall Sascha's response to this, I thought it was on the ML but it may have been in private, but something is broken on the MX27/28 SSI FIFO which means FIQ is the only way to get reliable audio since the DMA unit cannot get accurate alarms (this seems a common bug in Freescale processors :) - I'll let him elaborate since I've never even seen an MX28 let alone booted one, and our MX27 board never got tested without the FIQ code if I recall correctly. So that needs to stay, the issue here is why did nobody catch ssi-fiq.S breaking in testing MX51 Babbage and building a Thumb2 kernel, for example? Why did nobody notice it was building when configuring for MX3/5/6 boards (which do actually have working SSI and DMA, as you assume, and do not need this code, nor build the imx-pcm-dma-fiq part of the driver anyway)? I'm willing to fix all of the above, but if there's an obvious deficiency in testing at some point we need to fix that too.. Of course if Sascha says the fiq dma hack can disappear forever that's absolutely fine, I'm also willing to be the one to delete it... :)
On Mon, Aug 06, 2012 at 01:09:26PM -0500, Matt Sealey wrote: > So that needs to stay, the issue here is why did nobody catch > ssi-fiq.S breaking in testing MX51 > Babbage and building a Thumb2 kernel, for example? Why did nobody > notice it was building when > configuring for MX3/5/6 boards (which do actually have working SSI and > DMA, as you assume, and > do not need this code, nor build the imx-pcm-dma-fiq part of the > driver anyway)? I'm willing to fix all > of the above, but if there's an obvious deficiency in testing at some > point we need to fix that too.. As far as I can tell nobody's really running much up to date mainline on older i.MX processors, all the work is going on the new stuff and most of the board are on either vendor BSPs or older kernels.
On Mon, Aug 06, 2012 at 08:37:34PM +0100, Mark Brown wrote: > As far as I can tell nobody's really running much up to date mainline > on older i.MX processors, all the work is going on the new stuff and > most of the board are on either vendor BSPs or older kernels. That's not true; we still run MX25, MX27, MX35, MX28 on mainline in active projects. rsc
On Mon, Aug 6, 2012 at 3:16 PM, Robert Schwebel <r.schwebel@pengutronix.de> wrote: > On Mon, Aug 06, 2012 at 08:37:34PM +0100, Mark Brown wrote: >> As far as I can tell nobody's really running much up to date mainline >> on older i.MX processors, all the work is going on the new stuff and >> most of the board are on either vendor BSPs or older kernels. > > That's not true; we still run MX25, MX27, MX35, MX28 on mainline in > active projects. I think Shawn Guo (FSL/Linaro) would also disagree, since he's just posted a large amount of MXS patches to fix up the board for device trees, and Arnd is pulling them. Work is ongoing, it would be awful to delete something people really relied on or were in progress fixing (ahem). If they get up to audio in the near future the audio FIQ stuff would just end up being resubmitted almost verbatim... that seems like unnecessary churn. As far as I know, the FIQ usage is quite valid for the processor it needs to run on (MX21/27/28, right?) in the modes it runs in (AC97 on these processors, and maybe MX35 too), and I'm just trying to figure out what the steps are for * making sure it doesn't get built for architectures/variants it's certainly NOT required on (imx_v6_v7_defconfig, if it actually enabled audio, that is - in fact, audio should be enabled as more one of the boards supported defines it in the device tree) which would amount to two seperate patches, one for the defconfig, one for the CONFIG item. I did note that SND_SOC_EUKREA_TLV320 enables FIQ but it also depends on MX51 which makes me think this need to be split, too, so that MX51 boards don't have it but MX2/MX3 do. * if it is built then it's built as ARM code (redundant, as previously stated, but would have stopped me from swearing at my build box when I hit the issue yet again) which could be a patch or could be ignored. I'd rather discuss this here than clutter the ML with several respins of a patch, let's get an opinion first - to .arm or no to .arm? * make sure there's no weird FIQ stuff floating around that has so far relied on SND_SOC_IMX_PCM_FIQ doing select FIQ before I make it not compile in for other boards (since otherwise no i.MX processor selects FIQ if they're not using that driver, it could be simply coincidence nobody noticed). I don't want to be the one submitting a patch I can't test which mysteriously breaks every MX28 on the planet (since Rob, Shawn, Sascha might be the ones swearing at me instead) * making sure someone's actually testing audio as above... and where/if/when the MX28 audio stuff is going in the future und so weiter.. I guess I need Sascha to pipe up and tell me what the code is needed for exactly again.. and someone to test the result of the changes..
On Mon, Aug 06, 2012 at 03:39:50PM -0500, Matt Sealey wrote: > On Mon, Aug 6, 2012 at 3:16 PM, Robert Schwebel > > That's not true; we still run MX25, MX27, MX35, MX28 on mainline in > > active projects. > I think Shawn Guo (FSL/Linaro) would also disagree, since he's just > posted a large amount of MXS patches to fix up the board for device > trees, and Arnd is pulling them. MXS != i.MX. > As far as I know, the FIQ usage is quite valid for the processor it > needs to run on (MX21/27/28, right?) in the modes it runs in (AC97 on > these processors, and maybe MX35 too), and I'm just trying to figure > out what the steps are for Oh, ick. What is the problem that means people want to use the FIQ on these processors? There's been i.MX2x audio DMA since forever, it had DMA in mainline before any of the later i.MXs due to them having SDMA. The original mainline i.MX audio support was done on i.MX27 and didn't have any issue here, this is the fist I've heard of a problem. > I did note that SND_SOC_EUKREA_TLV320 enables FIQ but it also depends > on MX51 which makes me think this need to be split, too, so that MX51 > boards don't have it but MX2/MX3 do. Is that not bitrot due to it being there before SDMA support was?
On Mon, Aug 6, 2012 at 4:41 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Aug 06, 2012 at 03:39:50PM -0500, Matt Sealey wrote: >> On Mon, Aug 6, 2012 at 3:16 PM, Robert Schwebel > >> > That's not true; we still run MX25, MX27, MX35, MX28 on mainline in >> > active projects. > >> I think Shawn Guo (FSL/Linaro) would also disagree, since he's just >> posted a large amount of MXS patches to fix up the board for device >> trees, and Arnd is pulling them. > > MXS != i.MX. > >> As far as I know, the FIQ usage is quite valid for the processor it >> needs to run on (MX21/27/28, right?) in the modes it runs in (AC97 on >> these processors, and maybe MX35 too), and I'm just trying to figure >> out what the steps are for > > Oh, ick. What is the problem that means people want to use the FIQ on > these processors? There's been i.MX2x audio DMA since forever, it had > DMA in mainline before any of the later i.MXs due to them having SDMA. > The original mainline i.MX audio support was done on i.MX27 and didn't > have any issue here, this is the fist I've heard of a problem. For SSI in I2S mode, I think it works great, AC97 not so much. It's the AC97 mode the FIQ stuff is there to fix. It seems a bunch of boards use AC97 codecs (for no good reason) >> I did note that SND_SOC_EUKREA_TLV320 enables FIQ but it also depends >> on MX51 which makes me think this need to be split, too, so that MX51 >> boards don't have it but MX2/MX3 do. > > Is that not bitrot due to it being there before SDMA support was? Possibly. I'm going to wait for Sascha to explain it again.. if AC97 was the predicate for FIQ being required, it doesn't make any sense for a codec that says in it's description that it's I2S. It may not be bitrot but pure, unadulterated zeal on the part of whoever wrote that Kconfig snippet - enable everything, in case it fails (actually that kind of thing is why I fear for nuking the building of this driver since if FIQ stuff is silently required by something else and all that kept it in there was some badly thought out or bitrotted Kconfig entries... I don't want to cause a world of hurt).
On Mon, Aug 06, 2012 at 03:39:50PM -0500, Matt Sealey wrote: > * make sure there's no weird FIQ stuff floating around that has so far > relied on SND_SOC_IMX_PCM_FIQ doing select FIQ before I make it not Acked on changing SND_IMX_SOC to SND_SOC_IMX_PCM_FIQ in arch/arm/plat-mxc/Makefile. > compile in for other boards (since otherwise no i.MX processor selects > FIQ if they're not using that driver, it could be simply coincidence > nobody noticed). I don't want to be the one submitting a patch I can't > test which mysteriously breaks every MX28 on the planet (since Rob, > Shawn, Sascha might be the ones swearing at me instead) > Though i.MX23 and i.MX28 are named in i.MX family, they are actually a different architecture from IMX/MXC. It's MXS, nothing to do with SSI. Folder sound/soc/mxs/ is the one for i.MX28.
On Mon, Aug 06, 2012 at 10:41:22PM +0100, Mark Brown wrote: > On Mon, Aug 06, 2012 at 03:39:50PM -0500, Matt Sealey wrote: > > On Mon, Aug 6, 2012 at 3:16 PM, Robert Schwebel > > > > That's not true; we still run MX25, MX27, MX35, MX28 on mainline in > > > active projects. > > > I think Shawn Guo (FSL/Linaro) would also disagree, since he's just > > posted a large amount of MXS patches to fix up the board for device > > trees, and Arnd is pulling them. > > MXS != i.MX. > > > As far as I know, the FIQ usage is quite valid for the processor it > > needs to run on (MX21/27/28, right?) in the modes it runs in (AC97 on > > these processors, and maybe MX35 too), and I'm just trying to figure > > out what the steps are for > > Oh, ick. What is the problem that means people want to use the FIQ on > these processors? There's been i.MX2x audio DMA since forever, it had > DMA in mainline before any of the later i.MXs due to them having SDMA. > The original mainline i.MX audio support was done on i.MX27 and didn't > have any issue here, this is the fist I've heard of a problem. Originally the FIQ support was only used because there hasn't been SDMA support available at that time. Nowadays the FIQ support is necessary only for AC97. The AC97 support in the SSI unit is buggy: It does not allow you to select the slots you want to receive. At least the wm9712 codec always sends (apart from the stereo data) data in slot (I think it is) 12. You find this data mixed in your audio stream. The FIQ driver skips this data to get a valid audio stream. One other way to solve this would be to use dma here and to filter out the data afterwards. Sascha
On Mon, Aug 06, 2012 at 10:19:50AM -0500, Matt Sealey wrote: > On Sun, Aug 5, 2012 at 6:03 PM, Anton Vorontsov > <anton.vorontsov@linaro.org> wrote: > > The driver uses platform-specific mxc_set_irq_fiq() with the VIRQ cookie > > passed to it, so it's pretty clear that the driver is absolutely sure > > that the FIQ is routed via platform-specific IC, and that the cookie can > > be used to mask/unmask FIQs. So, let's switch to the genirq routines, > > since we're about to remove FIQ-specific variants. > > I have a semi-related question about this; > > Firstly, I was planning on (re-)submitting a patch for the > arch/arm/plat-mxc/ssi-fiq.S code which made it build in ARM mode > (since the code isn't Thumb compatible for various reasons) as it was > a blocker for a Thumb2-compiled kernel. Since the code was only needed > on ARM-capable processors it wouldn't cause a problem. Sascha signed > off on this a long while back and I've been testing it on all my > internal kernel versions, and I don't see any ill effects (that said I > don't have an i.MX28 or so to really verify it, I can't see why it > would not work). I realise this is redundant right now, anyway, since > it's only really enabled on imx_v4_v5 configs and they don't support > Thumb2 kernels anyway. What might be worth submitting is a switch to > add the ".arm" directive anyway simply for correctness since it could > never be compiled for Thumb anyway. We all know what gnu as is like. > > Looking at it again on the back of these patches, I noticed the > ssi-fiq.S code is compiled in when SND_IMX_SOC is enabled - of course, > it's only needed in the kernel if SND_SOC_IMX_PCM_FIQ is enabled (the > code that uses the FIQ stuff is only compiled then) but here on the > Efika MX builds it's being built, and I noticed it when it broke my > build because of the above. I'm therefore going to submit a patch > which changes the ifdef SND_SOC_IMX to ifdef SND_SOC_IMX_PCM_FIQ so > it's not compiled in unless absolutely necessary. However, there was a > rumble that this code may disappear or be reworked in the future > making this also quite redundant. Since it's not in the > imx_v6_v7_defconfig anyway, I'm sure this only didn't get noticed > because nobody's building Thumb2 kernels and nobody is trying configs > with audio enabled anyway.. > > This begs the question, could there be something somewhere hidden deep > in the kernel that is enabled by default or in some config somewhere > that requires "select FIQ" in it's Kconfig entry, but isn't being > enabled? On i.MX the only thing turning it on is that code, but other > ARM arch enable it by default. Since things have been moved to more > generic routines I can't in my mind guarantee that such a patch would > be well tested here since I would be relying on symbols missing or > defines not there anymore.. I have no real way to ensure that it would > work on boards I don't have. > > So, is the first patch (ssi-fiq.S .arm) worth it? I think the > SND_SOC_IMX_PCM_FIQ patch is worth it for v6_v7 systems anyway, but > maybe this should have been caught sooner, so should I update the > defconfig to enable some kind of audio bus support (Babbage has it in > the DT so is a needful thing for testing, I figure..)? And does anyone > forsee any problems with that option changing and the only "user" of > "FIQ" in the Kconfigs going away now all the FIQ-specific symbols went > away outside of the generic irq subsystem? I hit this issue some time ago when I was trying to do a Thumb2 build of this kernel. I don't remember having to fix the generic FIQ code just for this, but I had a patch somewhere to swap r13 with another register in ssi-fiq.S. I think that use of r13 in ways not permitted for Thumb was the only problem I found. I can try to dig it out if you want. In any case, it didn't seem worth pushing at the time, because it seemed that the SSI FIQ code was not relevant to any v7 or later platform -- so building that code for Thumb presumably doesn't make sense. Cheers ---Dave
On Tue, Aug 07, 2012 at 08:35:58AM +0200, Sascha Hauer wrote: > Nowadays the FIQ support is necessary only for AC97. The AC97 support in > the SSI unit is buggy: It does not allow you to select the slots you > want to receive. At least the wm9712 codec always sends (apart from the > stereo data) data in slot (I think it is) 12. You find this data mixed > in your audio stream. The FIQ driver skips this data to get a valid > audio stream. Right, any device with GPIO support will do this - it's how GPIO works in AC'97. > One other way to solve this would be to use dma here and to filter out > the data afterwards. Yup. That's probably more sane but also more work to implement.
On Sun, Aug 05, 2012 at 04:03:34PM -0700, Anton Vorontsov wrote: > The driver uses platform-specific mxc_set_irq_fiq() with the VIRQ cookie > passed to it, so it's pretty clear that the driver is absolutely sure > that the FIQ is routed via platform-specific IC, and that the cookie can > be used to mask/unmask FIQs. So, let's switch to the genirq routines, > since we're about to remove FIQ-specific variants. > > Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org> Acked-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > sound/soc/fsl/imx-pcm-fiq.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/fsl/imx-pcm-fiq.c b/sound/soc/fsl/imx-pcm-fiq.c > index ee27ba3..993e37d 100644 > --- a/sound/soc/fsl/imx-pcm-fiq.c > +++ b/sound/soc/fsl/imx-pcm-fiq.c > @@ -139,7 +139,7 @@ static int snd_imx_pcm_trigger(struct snd_pcm_substream *substream, int cmd) > hrtimer_start(&iprtd->hrt, ns_to_ktime(iprtd->poll_time_ns), > HRTIMER_MODE_REL); > if (++fiq_enable == 1) > - enable_fiq(imx_pcm_fiq); > + enable_irq(imx_pcm_fiq); > > break; > > @@ -149,7 +149,7 @@ static int snd_imx_pcm_trigger(struct snd_pcm_substream *substream, int cmd) > atomic_set(&iprtd->running, 0); > > if (--fiq_enable == 0) > - disable_fiq(imx_pcm_fiq); > + disable_irq(imx_pcm_fiq); > > break; > default: > -- > 1.7.10.4 > >
diff --git a/sound/soc/fsl/imx-pcm-fiq.c b/sound/soc/fsl/imx-pcm-fiq.c index ee27ba3..993e37d 100644 --- a/sound/soc/fsl/imx-pcm-fiq.c +++ b/sound/soc/fsl/imx-pcm-fiq.c @@ -139,7 +139,7 @@ static int snd_imx_pcm_trigger(struct snd_pcm_substream *substream, int cmd) hrtimer_start(&iprtd->hrt, ns_to_ktime(iprtd->poll_time_ns), HRTIMER_MODE_REL); if (++fiq_enable == 1) - enable_fiq(imx_pcm_fiq); + enable_irq(imx_pcm_fiq); break; @@ -149,7 +149,7 @@ static int snd_imx_pcm_trigger(struct snd_pcm_substream *substream, int cmd) atomic_set(&iprtd->running, 0); if (--fiq_enable == 0) - disable_fiq(imx_pcm_fiq); + disable_irq(imx_pcm_fiq); break; default:
The driver uses platform-specific mxc_set_irq_fiq() with the VIRQ cookie passed to it, so it's pretty clear that the driver is absolutely sure that the FIQ is routed via platform-specific IC, and that the cookie can be used to mask/unmask FIQs. So, let's switch to the genirq routines, since we're about to remove FIQ-specific variants. Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org> --- sound/soc/fsl/imx-pcm-fiq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)