[alsa-devel] [PATCH 1/6] Bugfixes and clean-ups bound for the v3.6 RCs
This patch-set includes some important changes which should make their way to the Mainline Release Candidates for the v3.6 release. Without them Audio doesn't even probe (at all) when booting with Device Tree selected. The kernel can't compile with ux500 audio enabled. Even if we can get that far SoC Core will assume we're using regmaps and attempt to use them; however, none exist for this driver, so the kernel will oops.
arch/arm/mach-ux500/Kconfig | 1 + arch/arm/mach-ux500/board-mop500-msp.c | 2 +- arch/arm/mach-ux500/board-mop500.c | 9 ++++----- sound/soc/codecs/ab8500-codec.c | 4 ++++ sound/soc/soc-dapm.c | 2 -- 5 files changed, 10 insertions(+), 8 deletions(-)
If codec->control_data is not populated SoC Core assumes we want to use regmap, which fails catastrophically, as we don't have one:
Unable to handle kernel NULL pointer dereference at virtual address 00000080 pgd = c0004000 [00000080] *pgd=00000000 Internal error: Oops: 17 [#1] PREEMPT SMP ARM Modules linked in: CPU: 1 Not tainted (3.5.0-rc6-00884-g0b2419e-dirty #130) PC is at regmap_read+0x10/0x5c LR is at hw_read+0x80/0x90 pc : [<c01a91b8>] lr : [<c0216804>] psr: 60000013
Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/codecs/ab8500-codec.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c index 3c79592..23b4018 100644 --- a/sound/soc/codecs/ab8500-codec.c +++ b/sound/soc/codecs/ab8500-codec.c @@ -2406,6 +2406,10 @@ static int ab8500_codec_probe(struct snd_soc_codec *codec)
/* Setup AB8500 according to board-settings */ pdata = (struct ab8500_platform_data *)dev_get_platdata(dev->parent); + + /* Inform SoC Core that we have our own I/O arrangements. */ + codec->control_data = (void *)true; + status = ab8500_audio_setup_mics(codec, &pdata->codec->amics); if (status < 0) { pr_err("%s: Failed to setup mics (%d)!\n", __func__, status);
If a list of widgets is provided and one of them fails to be added as a control, the present semantics fail all subsequent widgets. A better solution would be to only fail that widget, but pursue in attempting to add the rest of the list.
Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/soc-dapm.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index eded657..7d9c154 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -3095,8 +3095,6 @@ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm, dev_err(dapm->dev, "ASoC: Failed to create DAPM control %s\n", widget->name); - ret = -ENOMEM; - break; } widget++; }
On Tue, Jul 31, 2012 at 02:31:23PM +0100, Lee Jones wrote:
If a list of widgets is provided and one of them fails to be added as a control, the present semantics fail all subsequent widgets. A better solution would be to only fail that widget, but pursue in attempting to add the rest of the list.
You're posting this *again* without bothering to respond to my review comments.
On 31/07/12 14:42, Mark Brown wrote:
On Tue, Jul 31, 2012 at 02:31:23PM +0100, Lee Jones wrote:
If a list of widgets is provided and one of them fails to be added as a control, the present semantics fail all subsequent widgets. A better solution would be to only fail that widget, but pursue in attempting to add the rest of the list.
You're posting this *again* without bothering to respond to my review comments.
I didn't see any comments on this.
On Tue, Jul 31, 2012 at 03:25:07PM +0100, Lee Jones wrote:
On 31/07/12 14:42, Mark Brown wrote:
You're posting this *again* without bothering to respond to my review comments.
I didn't see any comments on this.
Read your email. 20120726115450.GE3099@opensource.wolfsonmicro.com and 20120729202510.GB4384@opensource.wolfsonmicro.com, the second one even includes a complaint about you ignoring the first mail.
On 31/07/12 15:28, Mark Brown wrote:
On Tue, Jul 31, 2012 at 03:25:07PM +0100, Lee Jones wrote:
On 31/07/12 14:42, Mark Brown wrote:
You're posting this *again* without bothering to respond to my review comments.
I didn't see any comments on this.
Read your email. 20120726115450.GE3099@opensource.wolfsonmicro.com and 20120729202510.GB4384@opensource.wolfsonmicro.com, the second one even includes a complaint about you ignoring the first mail.
Neither of those are in my Inbox. Blame Mozilla. :)
It's better because the whole audio system doesn't fail in the case of minor failure. It'd be like calling off a football game (or whatever you're into) because one of the substitutes ruptured an eyelash.
During start-up the ux500 has a couple of very unimportant widgets fail. It's the wrong behavior to force failure on the everything audio just because of that.
On Tue, Jul 31, 2012 at 03:38:02PM +0100, Lee Jones wrote:
Neither of those are in my Inbox. Blame Mozilla. :)
You might want to look at a better mail program.
It's better because the whole audio system doesn't fail in the case of minor failure. It'd be like calling off a football game (or whatever you're into) because one of the substitutes ruptured an eyelash.
It shouldn't make any difference to startup - we should still be checking errors and failing the init if we're failing to add links, this isn't something that's likely to randomly break on a particular boot, it's more something that indicates nobody bothered testing.
It's certainly totally inappropriate for an "urgent" bugfix.
During start-up the ux500 has a couple of very unimportant widgets fail. It's the wrong behavior to force failure on the everything audio just because of that.
Fixes for those errors, however...
On 31/07/12 15:54, Mark Brown wrote:
On Tue, Jul 31, 2012 at 03:38:02PM +0100, Lee Jones wrote:
Neither of those are in my Inbox. Blame Mozilla. :)
You might want to look at a better mail program.
Willingly. Any good suggestions?
It's better because the whole audio system doesn't fail in the case of minor failure. It'd be like calling off a football game (or whatever you're into) because one of the substitutes ruptured an eyelash.
It shouldn't make any difference to startup - we should still be checking errors and failing the init if we're failing to add links, this isn't something that's likely to randomly break on a particular boot, it's more something that indicates nobody bothered testing.
It's certainly totally inappropriate for an "urgent" bugfix.
Well it just means that audio won't work for the ux500 for this kernel release, but as we're waiting on clocks, this isn't a big issue for us. If you do take it (with or without the return code), feel free to add it to for-next instead of the -rc:s
During start-up the ux500 has a couple of very unimportant widgets fail. It's the wrong behavior to force failure on the everything audio just because of that.
Fixes for those errors, however...
I'll leave that to the audio expert. I'd like to move to to something else (that you maintain - perhaps I'll go and mess-up regulators next). :)
On Tue, Jul 31, 2012 at 04:15:23PM +0100, Lee Jones wrote:
On 31/07/12 15:54, Mark Brown wrote:
You might want to look at a better mail program.
Willingly. Any good suggestions?
mutt works for me.
It's certainly totally inappropriate for an "urgent" bugfix.
Well it just means that audio won't work for the ux500 for this kernel release, but as we're waiting on clocks, this isn't a big issue for us. If you do take it (with or without the return code), feel free to add it to for-next instead of the -rc:s
I'm not going to apply this patch. This isn't a vendor BSP, we shouldn't be putting random hacks like this in core code.
On 31/07/12 16:18, Mark Brown wrote:
It's certainly totally inappropriate for an "urgent" bugfix.
Well it just means that audio won't work for the ux500 for this kernel release, but as we're waiting on clocks, this isn't a big issue for us. If you do take it (with or without the return code), feel free to add it to for-next instead of the -rc:s
I'm not going to apply this patch. This isn't a vendor BSP, we shouldn't be putting random hacks like this in core code.
BSP kernel or otherwise, it still seems wrong to me to fail and entire audio driver just because of a broken link.
On Wed, Aug 01, 2012 at 08:19:28AM +0100, Lee Jones wrote:
On 31/07/12 16:18, Mark Brown wrote:
I'm not going to apply this patch. This isn't a vendor BSP, we shouldn't be putting random hacks like this in core code.
BSP kernel or otherwise, it still seems wrong to me to fail and entire audio driver just because of a broken link.
No, really. Random disconnections in the DAPM graph are just endless pain from a support and debug point of view. This isn't something that randomly breaks on specific hardware where we'd expect random errors at runtime, it's something that will never have worked - it seems clear nobody tested the mainline submission.
It's very disappointing to see such an error exist, and even more disappointing that there's no interest in fixing the driver.
On 01/08/12 14:20, Mark Brown wrote:
On Wed, Aug 01, 2012 at 08:19:28AM +0100, Lee Jones wrote:
On 31/07/12 16:18, Mark Brown wrote:
I'm not going to apply this patch. This isn't a vendor BSP, we shouldn't be putting random hacks like this in core code.
BSP kernel or otherwise, it still seems wrong to me to fail and entire audio driver just because of a broken link.
No, really. Random disconnections in the DAPM graph are just endless pain from a support and debug point of view. This isn't something that randomly breaks on specific hardware where we'd expect random errors at runtime, it's something that will never have worked - it seems clear nobody tested the mainline submission.
I am under the impression that it was tested. Perhaps before it was rebased, but tests were completed. Ola was very surprised when I told him there were link failures. The only issue is that I only found out and told him a day before he was due to take his Summer leave.
It's very disappointing to see such an error exist, and even more disappointing that there's no interest in fixing the driver.
This is incorrect. I'm sure the driver will be fixed post-haste when Ola returns back from vacation. If I can find some time I might dabble in the mean-time also.
On Wed, Aug 01, 2012 at 02:50:32PM +0100, Lee Jones wrote:
I am under the impression that it was tested. Perhaps before it was rebased, but tests were completed. Ola was very surprised when I told him there were link failures. The only issue is that I only found out and told him a day before he was due to take his Summer leave.
That's really surprising. What are the actual issues here, you've never shown the actual errors?
It's very disappointing to see such an error exist, and even more disappointing that there's no interest in fixing the driver.
This is incorrect. I'm sure the driver will be fixed post-haste when Ola returns back from vacation. If I can find some time I might dabble in the mean-time also.
It may not be what you're intending but it's unfortuantely what's I'm seeing.
On Wed, Aug 01, 2012 at 05:08:24PM +0100, Mark Brown wrote:
On Wed, Aug 01, 2012 at 02:50:32PM +0100, Lee Jones wrote:
It's very disappointing to see such an error exist, and even more disappointing that there's no interest in fixing the driver.
This is incorrect. I'm sure the driver will be fixed post-haste when Ola returns back from vacation. If I can find some time I might dabble in the mean-time also.
It may not be what you're intending but it's unfortuantely what's I'm seeing.
Just to expand on this a bit since I've found myself pushing back in this sort of way far too often with these recent serieses and it's making everyone grumpy:
What I'm seeing here is a lot of patches getting sent with problems that I'd really not expect from someone sending such a high volume of patches. Things like the lack of documentation for the DT bindings for example, it's something that's mandatory and which people doing lots of device tree work really ought to be aware of. There's also noticable pushback with fixing some of these issues, and like I say this happens often enough to be really noticeable.
This isn't awesome from a review point of view, it's not nice to find issues in things and when it happens a lot for the wrong sort of thing it ends up seeming like the time spent doing the reviews isn't valued.
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: den 1 augusti 2012 15:20 To: Lee Jones Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; STEricsson_nomadik_linux@list.st.com; linus.walleij@stericsson.com; arnd@arndb.de; olalilja@yahoo.se; ola.o.lilja@stericsson.com; alsa- devel@alsa-project.org; lrg@ti.com Subject: Re: [PATCH 1/6] ASoC: dapm: If one widget fails, do not force all subsequent widgets to fail too
On Wed, Aug 01, 2012 at 08:19:28AM +0100, Lee Jones wrote:
On 31/07/12 16:18, Mark Brown wrote:
I'm not going to apply this patch. This isn't a vendor BSP, we shouldn't be putting random hacks like this in core code.
BSP kernel or otherwise, it still seems wrong to me to fail and
entire
audio driver just because of a broken link.
No, really. Random disconnections in the DAPM graph are just endless pain from a support and debug point of view. This isn't something that randomly breaks on specific hardware where we'd expect random errors at runtime, it's something that will never have worked - it seems clear nobody tested the mainline submission.
It's very disappointing to see such an error exist, and even more disappointing that there's no interest in fixing the driver.
(Yes, I know this mailer isn't configured correctly, but I'm on vacation and have no Linux-computer/community-mailer available. However I find it important to answer this) Mark, you very well know that I have put in a lot of effort in getting our Ux500-driver mainlined. This is something I have driven without really getting sanctioned directly at working, rendering it even harder to find time for it. Accusing me of having "no interest in fixing the driver" is just absurd regarding the time I've spent on this. I'm also still driving for mainlining our upcoming drivers, so there is no lack of interest, nor lack of activity at our side. I really think you could afford a bit more polite attitude when doing reviews. It is not easy to fulfill every single aspect of mainlining directly and there is (most likely) no one that purposely do break any community rules. At least not from my side.
Regarding the problem with the failing DAPM-widget I can probably guess What is going wrong when Lee is trying it out. There will be two failing clock-supply widgets due to the fact that on the mainline-code these clocks simply is not there yet. I have, of course, tested this driver before submitting it, and I wouldn't dream on submitting a driver where there were failing widgets/routes. Internally, I have put a patch with our clock-tree for Ux500 on, but this is not mainline-quality code and that is why it is not submitted with the other patches I sent. The clocks are in the moment of writing being worked on by other persons in ST-Ericsson, and I would not have had any time to be doing all this which is not in the scope of my responsibilities (which is the audio-domain). Before you told me to create the clock-supply widget-type, I had only warnings for these failing clocks, as an intermediate solution, before the clock-tree was submitted, but now they are implemented with the clock- supply-type and there will be route-errors instead. Linus W. could probably shed some light of when the missing clocks are to be submitted.
Regards, Ola
On Thu, Aug 02, 2012 at 07:58:24AM +0200, Ola Lilja wrote:
Accusing me of having "no interest in fixing the driver" is just absurd regarding the time I've spent on this. I'm also still driving for
Sorry, this is more directed at the current round of fixes that are being sent than the driver itself - the patches to bodge around the issue are being pushed fairly aggressively as an urgent fix without even mentioning what the issue is. I don't have any concerns with the driver, only with the way it's being fixed.
Regarding the problem with the failing DAPM-widget I can probably guess What is going wrong when Lee is trying it out. There will be two failing clock-supply widgets due to the fact that on the mainline-code these clocks simply is not there yet. I have, of course, tested this driver before submitting it, and I wouldn't dream on submitting a driver where there were failing widgets/routes. Internally, I have put a patch with our clock-tree for Ux500 on, but this is not mainline-quality code and that is
This sort of reliance on out of tree patches which any real user of the device would be expected to have sounds like exactly the sort of thing I'd expect to have caused an issue like this, and obviously the fix here is to get the clocks in place (even if just as stubs, though I'd expect there to be a major impact on the functionality of the device if it's not clocked...) rather than to just bodge out the error checking.
It does also suggest that the DT binding for this device will need to include the setup of these clocks (I believe the clock bindings for DT have just gone into mainline this merge window but I'd need to check).
(As for the thread, which got flamy, let's put it to rest, and Ola: we are all impressed with your work on the ux500 ALSA SoC driver, no doubt about that, this was all ever about the DT patch set.)
On Thu, Aug 2, 2012 at 7:58 AM, Ola Lilja olalilja@yahoo.se wrote:
Linus W. could probably shed some light of when the missing clocks are to be submitted.
Ulf Hansson is working on this but he's on vacation FTM. However he has shown good progress and can probably present a prototype clock driver when he arrives back. IIRC there was some clock reparenting business left to sort out.
Yours, Linus Walleij
Patch withdrawn.
On Tue, Jul 31, 2012 at 02:31:23PM +0100, Lee Jones wrote:
If a list of widgets is provided and one of them fails to be added as a control, the present semantics fail all subsequent widgets. A better solution would be to only fail that widget, but pursue in attempting to add the rest of the list.
Signed-off-by: Lee Jones lee.jones@linaro.org
sound/soc/soc-dapm.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index eded657..7d9c154 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -3095,8 +3095,6 @@ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm, dev_err(dapm->dev, "ASoC: Failed to create DAPM control %s\n", widget->name);
ret = -ENOMEM;break;-- 1.7.9.5
This was left over during a recent clean-up which removed Device Tree helper structs. There is no longer a requirement for it, so we can just remove it.
Signed-off-by: Lee Jones lee.jones@linaro.org --- arch/arm/mach-ux500/board-mop500.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c index 80b4f0e..e641003 100644 --- a/arch/arm/mach-ux500/board-mop500.c +++ b/arch/arm/mach-ux500/board-mop500.c @@ -726,11 +726,6 @@ MACHINE_END
#ifdef CONFIG_MACH_UX500_DT
-static struct platform_device *snowball_of_platform_devs[] __initdata = { - &snowball_led_dev, - &snowball_key_dev, -}; - struct of_dev_auxdata u8500_auxdata_lookup[] __initdata = { /* Requires call-back bindings. */ OF_DEV_AUXDATA("arm,cortex-a9-pmu", 0, "arm-pmu", &db8500_pmu_platdata),
If codec->control_data is not populated SoC Core assumes we want to use regmap, which fails catastrophically, as we don't have one:
Unable to handle kernel NULL pointer dereference at virtual address 00000080 pgd = c0004000 [00000080] *pgd=00000000 Internal error: Oops: 17 [#1] PREEMPT SMP ARM Modules linked in: CPU: 1 Not tainted (3.5.0-rc6-00884-g0b2419e-dirty #130) PC is at regmap_read+0x10/0x5c LR is at hw_read+0x80/0x90 pc : [<c01a91b8>] lr : [<c0216804>] psr: 60000013
Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/codecs/ab8500-codec.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c index 3c79592..23b4018 100644 --- a/sound/soc/codecs/ab8500-codec.c +++ b/sound/soc/codecs/ab8500-codec.c @@ -2406,6 +2406,10 @@ static int ab8500_codec_probe(struct snd_soc_codec *codec)
/* Setup AB8500 according to board-settings */ pdata = (struct ab8500_platform_data *)dev_get_platdata(dev->parent); + + /* Inform SoC Core that we have our own I/O arrangements. */ + codec->control_data = (void *)true; + status = ab8500_audio_setup_mics(codec, &pdata->codec->amics); if (status < 0) { pr_err("%s: Failed to setup mics (%d)!\n", __func__, status);
The platform attempts to register platform device 'snd_soc_u8500' which doesn't actually exist. Here we change the reference to the correct one 'snd_soc_mop500'.
Signed-off-by: Lee Jones lee.jones@linaro.org --- arch/arm/mach-ux500/board-mop500-msp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c index 9960480..1b6a193 100644 --- a/arch/arm/mach-ux500/board-mop500-msp.c +++ b/arch/arm/mach-ux500/board-mop500-msp.c @@ -228,7 +228,7 @@ int mop500_msp_init(struct device *parent) struct platform_device *msp1;
pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__); - platform_device_register(&snd_soc_u8500); + platform_device_register(&snd_soc_mop500);
pr_info("Initialize MSP I2S-devices.\n"); db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
Hello.
On 07/31/2012 05:31 PM, Lee Jones wrote:
Subject doesn't parse for me...
The platform attempts to register platform device 'snd_soc_u8500' which doesn't actually exist. Here we change the reference to the correct one 'snd_soc_mop500'.
Signed-off-by: Lee Jones lee.jones@linaro.org
WBR, Sergei
On 31/07/12 17:46, Sergei Shtylyov wrote:
Hello.
On 07/31/2012 05:31 PM, Lee Jones wrote:
Subject doesn't parse for me...
The platform attempts to register platform device 'snd_soc_u8500' which doesn't actually exist. Here we change the reference to the correct one 'snd_soc_mop500'.
Signed-off-by: Lee Jones lee.jones@linaro.org
Actually, it does make sense?
As a sentence: "This patch fixes merge error: so such structure 'struct snd_soc_u8500'.
On 01/08/12 08:37, Lee Jones wrote:
On 31/07/12 17:46, Sergei Shtylyov wrote:
Hello.
On 07/31/2012 05:31 PM, Lee Jones wrote:
Subject doesn't parse for me...The platform attempts to register platform device 'snd_soc_u8500' which doesn't actually exist. Here we change the reference to the correct one 'snd_soc_mop500'.
Signed-off-by: Lee Jones lee.jones@linaro.org
Actually, it does make sense?
As a sentence: "This patch fixes merge error: so such structure 'struct snd_soc_u8500'.
That's interesting. Not only when I read it did it seem correct, I also wrote it wrong "as a sentence". Of course "so" should be "no".
From: Lee Jones lee.jones@linaro.org Date: Fri, 27 Jul 2012 13:10:52 +0100 Subject: [PATCH 1/1] ARM: ux500: Fix merge error, no matching driver name for 'snd_soc_u8500'
The platform attempts to register platform device 'snd_soc_u8500' which doesn't actually exist. Here we change the reference to the correct one 'snd_soc_mop500'.
Signed-off-by: Lee Jones lee.jones@linaro.org --- arch/arm/mach-ux500/board-mop500-msp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c index 9960480..df15646 100644 --- a/arch/arm/mach-ux500/board-mop500-msp.c +++ b/arch/arm/mach-ux500/board-mop500-msp.c @@ -191,9 +191,9 @@ static struct platform_device *db8500_add_msp_i2s(struct device *parent, return pdev; }
-/* Platform device for ASoC U8500 machine */ -static struct platform_device snd_soc_u8500 = { - .name = "snd-soc-u8500", +/* Platform device for ASoC MOP500 machine */ +static struct platform_device snd_soc_mop500 = { + .name = "snd-soc-mop500", .id = 0, .dev = { .platform_data = NULL, @@ -227,8 +227,8 @@ int mop500_msp_init(struct device *parent) { struct platform_device *msp1;
- pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__); - platform_device_register(&snd_soc_u8500); + pr_info("%s: Register platform-device 'snd-soc-mop500'.\n", __func__); + platform_device_register(&snd_soc_mop500);
pr_info("Initialize MSP I2S-devices.\n"); db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
This was left over during a recent clean-up which removed Device Tree helper structs. There is no longer a requirement for it, so we can just remove it.
Signed-off-by: Lee Jones lee.jones@linaro.org --- arch/arm/mach-ux500/board-mop500.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c index 80b4f0e..e641003 100644 --- a/arch/arm/mach-ux500/board-mop500.c +++ b/arch/arm/mach-ux500/board-mop500.c @@ -726,11 +726,6 @@ MACHINE_END
#ifdef CONFIG_MACH_UX500_DT
-static struct platform_device *snowball_of_platform_devs[] __initdata = { - &snowball_led_dev, - &snowball_key_dev, -}; - struct of_dev_auxdata u8500_auxdata_lookup[] __initdata = { /* Requires call-back bindings. */ OF_DEV_AUXDATA("arm,cortex-a9-pmu", 0, "arm-pmu", &db8500_pmu_platdata),
On Tuesday 31 July 2012, Lee Jones wrote:
This was left over during a recent clean-up which removed Device Tree helper structs. There is no longer a requirement for it, so we can just remove it.
Signed-off-by: Lee Jones lee.jones@linaro.org
Acked-by: Arnd Bergmann arnd@arndb.de
Previous attempts to add platform probing of the Audio related devices only call from non-DT initialisation functions. This patch extends that functionality to the Device Tree related ones too.
Signed-off-by: Lee Jones lee.jones@linaro.org --- arch/arm/mach-ux500/board-mop500.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c index e641003..87a5cd7 100644 --- a/arch/arm/mach-ux500/board-mop500.c +++ b/arch/arm/mach-ux500/board-mop500.c @@ -794,6 +794,7 @@ static void __init u8500_init_machine(void) ARRAY_SIZE(mop500_platform_devs));
mop500_sdi_init(parent); + mop500_msp_init(parent); i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices); i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs); i2c_register_board_info(2, mop500_i2c2_devices, @@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
mop500_uib_init();
+ } else if (of_machine_is_compatible("calaosystems,snowball-a9500")) { + mop500_of_msp_init(parent); } else if (of_machine_is_compatible("st-ericsson,hrefv60+")) { /* * The HREFv60 board removed a GPIO expander and routed @@ -812,6 +815,7 @@ static void __init u8500_init_machine(void) ARRAY_SIZE(mop500_platform_devs));
hrefv60_sdi_init(parent); + mop500_msp_init(parent);
i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices); i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;
The platform attempts to register platform device 'snd_soc_u8500' which doesn't actually exist. Here we change the reference to the correct one 'snd_soc_mop500'.
Signed-off-by: Lee Jones lee.jones@linaro.org --- arch/arm/mach-ux500/board-mop500-msp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c index 9960480..1b6a193 100644 --- a/arch/arm/mach-ux500/board-mop500-msp.c +++ b/arch/arm/mach-ux500/board-mop500-msp.c @@ -228,7 +228,7 @@ int mop500_msp_init(struct device *parent) struct platform_device *msp1;
pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__); - platform_device_register(&snd_soc_u8500); + platform_device_register(&snd_soc_mop500);
pr_info("Initialize MSP I2S-devices.\n"); db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
On Tuesday 31 July 2012, Lee Jones wrote:
arch/arm/mach-ux500/board-mop500-msp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c index 9960480..1b6a193 100644 --- a/arch/arm/mach-ux500/board-mop500-msp.c +++ b/arch/arm/mach-ux500/board-mop500-msp.c @@ -228,7 +228,7 @@ int mop500_msp_init(struct device *parent) struct platform_device *msp1;
pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__);
platform_device_register(&snd_soc_u8500);
platform_device_register(&snd_soc_mop500); pr_info("Initialize MSP I2S-devices.\n"); db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0,
Against which tree is this? In upstream I only see the snd_soc_u8500 device, not snd_soc_mop500.
Arnd
The current kernel commandline for ux500 based devices includes hard-coded allocations for things like mali and hwmem, which actually run over lowmem. Here we enable highmem in order to avoid memory corruption errors.
Signed-off-by: Lee Jones lee.jones@linaro.org --- arch/arm/mach-ux500/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig index c013bbf..f51c351 100644 --- a/arch/arm/mach-ux500/Kconfig +++ b/arch/arm/mach-ux500/Kconfig @@ -28,6 +28,7 @@ config MACH_MOP500 select I2C select I2C_NOMADIK select SOC_BUS + select HIGHMEM help Include support for the MOP500 development platform.
On Tue, Jul 31, 2012 at 02:31:30PM +0100, Lee Jones wrote:
The current kernel commandline for ux500 based devices includes hard-coded allocations for things like mali and hwmem, which actually run over lowmem. Here we enable highmem in order to avoid memory corruption errors.
Err what? highmem should make no difference if things are being done correctly.
In other words, your patch description is too vague to understand what the problem is and why you have to force highmem on - and nothing should _require_ highmem.
On 31/07/12 14:56, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2012 at 02:31:30PM +0100, Lee Jones wrote:
The current kernel commandline for ux500 based devices includes hard-coded allocations for things like mali and hwmem, which actually run over lowmem. Here we enable highmem in order to avoid memory corruption errors.
Err what? highmem should make no difference if things are being done correctly.
In other words, your patch description is too vague to understand what the problem is and why you have to force highmem on - and nothing should _require_ highmem.
I think I must have misunderstood some advice which was recently given to me. I'll drop the patch.
Thanks for reviewing.
On Tue, Jul 31, 2012 at 03:29:48PM +0100, Lee Jones wrote:
On 31/07/12 14:56, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2012 at 02:31:30PM +0100, Lee Jones wrote:
The current kernel commandline for ux500 based devices includes hard-coded allocations for things like mali and hwmem, which actually run over lowmem. Here we enable highmem in order to avoid memory corruption errors.
Err what? highmem should make no difference if things are being done correctly.
In other words, your patch description is too vague to understand what the problem is and why you have to force highmem on - and nothing should _require_ highmem.
I think I must have misunderstood some advice which was recently given to me. I'll drop the patch.
Thanks for reviewing.
There are two reasons for not doing this:
1. it is wrong to force core optional features on by using 'select' 2. it is wrong to use 'select' on user visible symbols because it creates confusion, and prevents the user from disabling the option.
Note that highmem is just a hack to make larger memory systems work before 64-bit systems happen - it is _known_ to make systems unstable with large amounts of highmem (even on x86) due to the requirement to have a certain amount of metadata stored in lowmem.
Nothing should ever force anyone to have highmem enabled.
I still fail to see how not having highmem enabled would ever cause memory corruption errors (unless something dealing with memory in a very very wrong way - iow, not using one of the reservation or memory allocation methods provided by the kernel.)
On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
I still fail to see how not having highmem enabled would ever cause memory corruption errors (unless something dealing with memory in a very very wrong way - iow, not using one of the reservation or memory allocation methods provided by the kernel.)
The problem is that all users of ux500 systems pass a command line like
vmalloc=256M mem=128M@0 mali.mali_mem=32M@128M hwmem=168M@160M mem=48M@328M mem_issw=1M@383M mem=640M@384M
This is of course totally bogus and should not be done. If I understand Lee correctly, one of the issues resulting from passing a command line like this without enabling highmem is memory corruption.
"Doctor it hurts when I do this ..."
Arnd
On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
I still fail to see how not having highmem enabled would ever cause memory corruption errors (unless something dealing with memory in a very very wrong way - iow, not using one of the reservation or memory allocation methods provided by the kernel.)
The problem is that all users of ux500 systems pass a command line like
vmalloc=256M mem=128M@0 mali.mali_mem=32M@128M hwmem=168M@160M mem=48M@328M mem_issw=1M@383M mem=640M@384M
This is of course totally bogus and should not be done. If I understand Lee correctly, one of the issues resulting from passing a command line like this without enabling highmem is memory corruption.
But the question is _why_ does that corruption happen.
From the above, we will end up with the kernel getting:
0x00000000 - 0x07ffffff (128M @ 0) 0x14800000 - 0x177fffff (48M @ 328M) 0x18000000 - 0x3fffffff (640M @ 384M)
with:
0x08000000 - 0x081fffff used for mali 0x0a000000 - 0x147fffff used for hwmem 0x17f00000 - 0x17ffffff used for mem_issw
Now, with highmem disabled, the kernel should still map exactly the regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the direct mapped region, and truncate the 0x18000000 - 0x3fffffff region appropriately, reducing the amount of memory available such that it won't overlap the vmalloc area (which you've specified to be a minimum of 256M.)
This should _NOT_ cause any memory corruption.
So, come on guys. Debugging is *mandatory* for this kind of problem. Papering over it is obscene.
On 31/07/12 23:01, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
I still fail to see how not having highmem enabled would ever cause memory corruption errors (unless something dealing with memory in a very very wrong way - iow, not using one of the reservation or memory allocation methods provided by the kernel.)
The problem is that all users of ux500 systems pass a command line like
vmalloc=256M mem=128M@0 mali.mali_mem=32M@128M hwmem=168M@160M mem=48M@328M mem_issw=1M@383M mem=640M@384M
This is of course totally bogus and should not be done. If I understand Lee correctly, one of the issues resulting from passing a command line like this without enabling highmem is memory corruption.
But the question is _why_ does that corruption happen.
From the above, we will end up with the kernel getting:
0x00000000 - 0x07ffffff (128M @ 0) 0x14800000 - 0x177fffff (48M @ 328M) 0x18000000 - 0x3fffffff (640M @ 384M)
with:
0x08000000 - 0x081fffff used for mali 0x0a000000 - 0x147fffff used for hwmem 0x17f00000 - 0x17ffffff used for mem_issw
Now, with highmem disabled, the kernel should still map exactly the regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the direct mapped region, and truncate the 0x18000000 - 0x3fffffff region appropriately, reducing the amount of memory available such that it won't overlap the vmalloc area (which you've specified to be a minimum of 256M.)
This should _NOT_ cause any memory corruption.
So, come on guys. Debugging is *mandatory* for this kind of problem. Papering over it is obscene.
Actually I didn't go any further with it, as I changed to another identical piece of hardware and couldn't reproduce the issue.
FYI, here's the boot log from the broken board:
http://paste.ubuntu.com/1102017/
On Wed, Aug 01, 2012 at 08:56:14AM +0100, Lee Jones wrote:
On 31/07/12 23:01, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
I still fail to see how not having highmem enabled would ever cause memory corruption errors (unless something dealing with memory in a very very wrong way - iow, not using one of the reservation or memory allocation methods provided by the kernel.)
The problem is that all users of ux500 systems pass a command line like
vmalloc=256M mem=128M@0 mali.mali_mem=32M@128M hwmem=168M@160M mem=48M@328M mem_issw=1M@383M mem=640M@384M
This is of course totally bogus and should not be done. If I understand Lee correctly, one of the issues resulting from passing a command line like this without enabling highmem is memory corruption.
But the question is _why_ does that corruption happen.
From the above, we will end up with the kernel getting:
0x00000000 - 0x07ffffff (128M @ 0) 0x14800000 - 0x177fffff (48M @ 328M) 0x18000000 - 0x3fffffff (640M @ 384M)
with:
0x08000000 - 0x081fffff used for mali 0x0a000000 - 0x147fffff used for hwmem 0x17f00000 - 0x17ffffff used for mem_issw
Now, with highmem disabled, the kernel should still map exactly the regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the direct mapped region, and truncate the 0x18000000 - 0x3fffffff region appropriately, reducing the amount of memory available such that it won't overlap the vmalloc area (which you've specified to be a minimum of 256M.)
This should _NOT_ cause any memory corruption.
So, come on guys. Debugging is *mandatory* for this kind of problem. Papering over it is obscene.
Actually I didn't go any further with it, as I changed to another identical piece of hardware and couldn't reproduce the issue.
FYI, here's the boot log from the broken board:
Well, the good thing is this:
8 Truncating RAM at 18000000-3fffffff to -2c3fffff (vmalloc region overlap).
which means the RAM was properly truncated before it is passed to memblock, etc.
That oops dump looks very much like an ASoC problem, where dapm_widget_power_check() recurses into dapm_supply_check_power() which then recurses back into dapm_widget_power_check(), and it eventually overflows the kernel stack, corrupting the thread_info and the pages below.
Given the address of the stack pointer (ebc480a8) I don't think we can be too sure where it was supposed to be, and where the top of stack should have been, so we don't know how many pages have been stomped on and corrupted.
Stopping that recursion is the first thing that needs to be done so that the cause of it can then be properly debugged without the kernel itself corrupting memory below the kernel stack.
On 01/08/12 09:41, Russell King - ARM Linux wrote:
On Wed, Aug 01, 2012 at 08:56:14AM +0100, Lee Jones wrote:
On 31/07/12 23:01, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2012 at 08:50:02PM +0000, Arnd Bergmann wrote:
On Tuesday 31 July 2012, Russell King - ARM Linux wrote:
I still fail to see how not having highmem enabled would ever cause memory corruption errors (unless something dealing with memory in a very very wrong way - iow, not using one of the reservation or memory allocation methods provided by the kernel.)
The problem is that all users of ux500 systems pass a command line like
vmalloc=256M mem=128M@0 mali.mali_mem=32M@128M hwmem=168M@160M mem=48M@328M mem_issw=1M@383M mem=640M@384M
This is of course totally bogus and should not be done. If I understand Lee correctly, one of the issues resulting from passing a command line like this without enabling highmem is memory corruption.
But the question is _why_ does that corruption happen.
From the above, we will end up with the kernel getting:
0x00000000 - 0x07ffffff (128M @ 0) 0x14800000 - 0x177fffff (48M @ 328M) 0x18000000 - 0x3fffffff (640M @ 384M)
with:
0x08000000 - 0x081fffff used for mali 0x0a000000 - 0x147fffff used for hwmem 0x17f00000 - 0x17ffffff used for mem_issw
Now, with highmem disabled, the kernel should still map exactly the regions: 0x00000000 - 0x07ffffff, 0x14800000 - 0x177fffff, into the direct mapped region, and truncate the 0x18000000 - 0x3fffffff region appropriately, reducing the amount of memory available such that it won't overlap the vmalloc area (which you've specified to be a minimum of 256M.)
This should _NOT_ cause any memory corruption.
So, come on guys. Debugging is *mandatory* for this kind of problem. Papering over it is obscene.
Actually I didn't go any further with it, as I changed to another identical piece of hardware and couldn't reproduce the issue.
FYI, here's the boot log from the broken board:
Well, the good thing is this:
8 Truncating RAM at 18000000-3fffffff to -2c3fffff (vmalloc region overlap).which means the RAM was properly truncated before it is passed to memblock, etc.
That oops dump looks very much like an ASoC problem, where dapm_widget_power_check() recurses into dapm_supply_check_power() which then recurses back into dapm_widget_power_check(), and it eventually overflows the kernel stack, corrupting the thread_info and the pages below.
Given the address of the stack pointer (ebc480a8) I don't think we can be too sure where it was supposed to be, and where the top of stack should have been, so we don't know how many pages have been stomped on and corrupted.
Stopping that recursion is the first thing that needs to be done so that the cause of it can then be properly debugged without the kernel itself corrupting memory below the kernel stack.
Those were my thoughts.
Here was my cry for help: https://lkml.org/lkml/2012/7/23/181
Previous attempts to add platform probing of the Audio related devices only call from non-DT initialisation functions. This patch extends that functionality to the Device Tree related ones too.
Signed-off-by: Lee Jones lee.jones@linaro.org --- arch/arm/mach-ux500/board-mop500.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c index e641003..87a5cd7 100644 --- a/arch/arm/mach-ux500/board-mop500.c +++ b/arch/arm/mach-ux500/board-mop500.c @@ -794,6 +794,7 @@ static void __init u8500_init_machine(void) ARRAY_SIZE(mop500_platform_devs));
mop500_sdi_init(parent); + mop500_msp_init(parent); i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices); i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs); i2c_register_board_info(2, mop500_i2c2_devices, @@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
mop500_uib_init();
+ } else if (of_machine_is_compatible("calaosystems,snowball-a9500")) { + mop500_of_msp_init(parent); } else if (of_machine_is_compatible("st-ericsson,hrefv60+")) { /* * The HREFv60 board removed a GPIO expander and routed @@ -812,6 +815,7 @@ static void __init u8500_init_machine(void) ARRAY_SIZE(mop500_platform_devs));
hrefv60_sdi_init(parent); + mop500_msp_init(parent);
i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices); i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;
On Tuesday 31 July 2012, Lee Jones wrote:
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c index e641003..87a5cd7 100644 --- a/arch/arm/mach-ux500/board-mop500.c +++ b/arch/arm/mach-ux500/board-mop500.c @@ -794,6 +794,7 @@ static void __init u8500_init_machine(void) ARRAY_SIZE(mop500_platform_devs));
mop500_sdi_init(parent);
mop500_msp_init(parent); i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices); i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs); i2c_register_board_info(2, mop500_i2c2_devices,@@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
mop500_uib_init();
} else if (of_machine_is_compatible("calaosystems,snowball-a9500")) {mop500_of_msp_init(parent); } else if (of_machine_is_compatible("st-ericsson,hrefv60+")) { /* * The HREFv60 board removed a GPIO expander and routed@@ -812,6 +815,7 @@ static void __init u8500_init_machine(void) ARRAY_SIZE(mop500_platform_devs));
hrefv60_sdi_init(parent);
mop500_msp_init(parent); i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices); i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;
Looks like you're adding the same call to each of the three cases: mop500, snowball and hrefv60+. How about moving it before or after the if/elseif block?
Arnd
On 31/07/12 21:54, Arnd Bergmann wrote:
On Tuesday 31 July 2012, Lee Jones wrote:
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c index e641003..87a5cd7 100644 --- a/arch/arm/mach-ux500/board-mop500.c +++ b/arch/arm/mach-ux500/board-mop500.c @@ -794,6 +794,7 @@ static void __init u8500_init_machine(void) ARRAY_SIZE(mop500_platform_devs));
mop500_sdi_init(parent);
mop500_msp_init(parent); i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices); i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs); i2c_register_board_info(2, mop500_i2c2_devices,@@ -801,6 +802,8 @@ static void __init u8500_init_machine(void)
mop500_uib_init();
} else if (of_machine_is_compatible("calaosystems,snowball-a9500")) {mop500_of_msp_init(parent); } else if (of_machine_is_compatible("st-ericsson,hrefv60+")) { /* * The HREFv60 board removed a GPIO expander and routed@@ -812,6 +815,7 @@ static void __init u8500_init_machine(void) ARRAY_SIZE(mop500_platform_devs));
hrefv60_sdi_init(parent);
mop500_msp_init(parent); i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices); i2c0_devs -= NUM_PRE_V60_I2C0_DEVICES;Looks like you're adding the same call to each of the three cases: mop500, snowball and hrefv60+. How about moving it before or after the if/elseif block?
The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.
On Wednesday 01 August 2012, Lee Jones wrote:
Looks like you're adding the same call to each of the three cases: mop500, snowball and hrefv60+. How about moving it before or after the if/elseif block?
The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.
Ah, I see. Is there a problem in always using mop500_of_msp_init then? I would guess that this just means you'd have to put the msp into the device tree files, which don't yet exist for mop500 and hrefv60.
Arnd
On 01/08/12 14:32, Arnd Bergmann wrote:
On Wednesday 01 August 2012, Lee Jones wrote:
Looks like you're adding the same call to each of the three cases: mop500, snowball and hrefv60+. How about moving it before or after the if/elseif block?
The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.
Ah, I see. Is there a problem in always using mop500_of_msp_init then? I would guess that this just means you'd have to put the msp into the device tree files, which don't yet exist for mop500 and hrefv60.
Although that is true, it's only part of the issue.
mop500_of_msp_init is only a temporary solution to aid in step-by-step enablement of DT for audio. By the end of the patch-set it has been removed (along with the call to it if 'of_machine_is_compatible ("calaosystems,snowball-a9500")'. What I can do it write a patch to consolidate the calls _after_ "ARM: ux500: Remove platform registration of MSP devices", as a bolt-on.
On Wednesday 01 August 2012, Lee Jones wrote:
On 01/08/12 14:32, Arnd Bergmann wrote:
On Wednesday 01 August 2012, Lee Jones wrote:
Looks like you're adding the same call to each of the three cases: mop500, snowball and hrefv60+. How about moving it before or after the if/elseif block?
The Snowball one is different 'mop500_msp_init' -> 'mop500_of_msp_init'.
Ah, I see. Is there a problem in always using mop500_of_msp_init then? I would guess that this just means you'd have to put the msp into the device tree files, which don't yet exist for mop500 and hrefv60.
Although that is true, it's only part of the issue.
mop500_of_msp_init is only a temporary solution to aid in step-by-step enablement of DT for audio. By the end of the patch-set it has been removed (along with the call to it if 'of_machine_is_compatible ("calaosystems,snowball-a9500")'. What I can do it write a patch to consolidate the calls after "ARM: ux500: Remove platform registration of MSP devices", as a bolt-on.
I see. Your solution looks fine then.
Arnd
The current kernel commandline for ux500 based devices includes hard-coded allocations for things like mali and hwmem, which actually run over lowmem. Here we enable highmem in order to avoid memory corruption errors.
Signed-off-by: Lee Jones lee.jones@linaro.org --- arch/arm/mach-ux500/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig index c013bbf..f51c351 100644 --- a/arch/arm/mach-ux500/Kconfig +++ b/arch/arm/mach-ux500/Kconfig @@ -28,6 +28,7 @@ config MACH_MOP500 select I2C select I2C_NOMADIK select SOC_BUS + select HIGHMEM help Include support for the MOP500 development platform.
In the initial submission of the MSP driver msp1 and msp3's associated pinctrl mechanism was passed back to platform code using a plat_init() call-back routine, but it has no place in platform code. The MSP driver should set this up for the appropriate ports. Instead we use a use_pinctrl identifier which is passed from platform_data/Device Tree which indicates which ports should use pinctrl.
Signed-off-by: Lee Jones lee.jones@linaro.org --- arch/arm/mach-ux500/board-mop500-msp.c | 75 +------------------------------- arch/arm/mach-ux500/include/mach/msp.h | 2 - sound/soc/ux500/ux500_msp_i2s.c | 75 +++++++++++++++++++++++++------- sound/soc/ux500/ux500_msp_i2s.h | 2 - 4 files changed, 60 insertions(+), 94 deletions(-)
diff --git a/arch/arm/mach-ux500/board-mop500-msp.c b/arch/arm/mach-ux500/board-mop500-msp.c index 1b6a193..af65fb0 100644 --- a/arch/arm/mach-ux500/board-mop500-msp.c +++ b/arch/arm/mach-ux500/board-mop500-msp.c @@ -23,53 +23,6 @@ #include "devices-db8500.h" #include "pins-db8500.h"
-/* MSP1/3 Tx/Rx usage protection */ -static DEFINE_SPINLOCK(msp_rxtx_lock); - -/* Reference Count */ -static int msp_rxtx_ref; - -/* Pin modes */ -struct pinctrl *msp1_p; -struct pinctrl_state *msp1_def; -struct pinctrl_state *msp1_sleep; - -int msp13_i2s_init(void) -{ - int retval = 0; - unsigned long flags; - - spin_lock_irqsave(&msp_rxtx_lock, flags); - if (msp_rxtx_ref == 0 && !(IS_ERR(msp1_p) || IS_ERR(msp1_def))) { - retval = pinctrl_select_state(msp1_p, msp1_def); - if (retval) - pr_err("could not set MSP1 defstate\n"); - } - if (!retval) - msp_rxtx_ref++; - spin_unlock_irqrestore(&msp_rxtx_lock, flags); - - return retval; -} - -int msp13_i2s_exit(void) -{ - int retval = 0; - unsigned long flags; - - spin_lock_irqsave(&msp_rxtx_lock, flags); - WARN_ON(!msp_rxtx_ref); - msp_rxtx_ref--; - if (msp_rxtx_ref == 0 && !(IS_ERR(msp1_p) || IS_ERR(msp1_sleep))) { - retval = pinctrl_select_state(msp1_p, msp1_sleep); - if (retval) - pr_err("could not set MSP1 sleepstate\n"); - } - spin_unlock_irqrestore(&msp_rxtx_lock, flags); - - return retval; -} - static struct stedma40_chan_cfg msp0_dma_rx = { .high_priority = true, .dir = STEDMA40_PERIPH_TO_MEM, @@ -132,8 +85,6 @@ static struct msp_i2s_platform_data msp1_platform_data = { .id = MSP_I2S_1, .msp_i2s_dma_rx = NULL, .msp_i2s_dma_tx = &msp1_dma_tx, - .msp_i2s_init = msp13_i2s_init, - .msp_i2s_exit = msp13_i2s_exit, };
static struct stedma40_chan_cfg msp2_dma_rx = { @@ -219,47 +170,23 @@ static struct msp_i2s_platform_data msp3_platform_data = { .id = MSP_I2S_3, .msp_i2s_dma_rx = &msp1_dma_rx, .msp_i2s_dma_tx = NULL, - .msp_i2s_init = msp13_i2s_init, - .msp_i2s_exit = msp13_i2s_exit, };
int mop500_msp_init(struct device *parent) { - struct platform_device *msp1; - pr_info("%s: Register platform-device 'snd-soc-u8500'.\n", __func__); platform_device_register(&snd_soc_mop500);
pr_info("Initialize MSP I2S-devices.\n"); db8500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0, &msp0_platform_data); - msp1 = db8500_add_msp_i2s(parent, 1, U8500_MSP1_BASE, IRQ_DB8500_MSP1, + db8500_add_msp_i2s(parent, 1, U8500_MSP1_BASE, IRQ_DB8500_MSP1, &msp1_platform_data); db8500_add_msp_i2s(parent, 2, U8500_MSP2_BASE, IRQ_DB8500_MSP2, &msp2_platform_data); db8500_add_msp_i2s(parent, 3, U8500_MSP3_BASE, IRQ_DB8500_MSP1, &msp3_platform_data);
- /* Get the pinctrl handle for MSP1 */ - if (msp1) { - msp1_p = pinctrl_get(&msp1->dev); - if (IS_ERR(msp1_p)) - dev_err(&msp1->dev, "could not get MSP1 pinctrl\n"); - else { - msp1_def = pinctrl_lookup_state(msp1_p, - PINCTRL_STATE_DEFAULT); - if (IS_ERR(msp1_def)) { - dev_err(&msp1->dev, - "could not get MSP1 defstate\n"); - } - msp1_sleep = pinctrl_lookup_state(msp1_p, - PINCTRL_STATE_SLEEP); - if (IS_ERR(msp1_sleep)) - dev_err(&msp1->dev, - "could not get MSP1 idlestate\n"); - } - } - pr_info("%s: Register platform-device 'ux500-pcm'\n", __func__); platform_device_register(&ux500_pcm);
diff --git a/arch/arm/mach-ux500/include/mach/msp.h b/arch/arm/mach-ux500/include/mach/msp.h index 798be19..3cc7142 100644 --- a/arch/arm/mach-ux500/include/mach/msp.h +++ b/arch/arm/mach-ux500/include/mach/msp.h @@ -22,8 +22,6 @@ struct msp_i2s_platform_data { enum msp_i2s_id id; struct stedma40_chan_cfg *msp_i2s_dma_rx; struct stedma40_chan_cfg *msp_i2s_dma_tx; - int (*msp_i2s_init) (void); - int (*msp_i2s_exit) (void); };
#endif diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c index 36be11e..2cbfc54 100644 --- a/sound/soc/ux500/ux500_msp_i2s.c +++ b/sound/soc/ux500/ux500_msp_i2s.c @@ -15,6 +15,7 @@
#include <linux/module.h> #include <linux/platform_device.h> +#include <linux/pinctrl/consumer.h> #include <linux/delay.h> #include <linux/slab.h>
@@ -25,6 +26,17 @@
#include "ux500_msp_i2s.h"
+/* MSP1/3 Tx/Rx usage protection */ +static DEFINE_SPINLOCK(msp_rxtx_lock); + +/* Pin modes */ +struct pinctrl *pinctrl_p; +struct pinctrl_state *pinctrl_def; +struct pinctrl_state *pinctrl_sleep; + +/* Reference Count */ +int pinctrl_rxtx_ref; + /* Protocol desciptors */ static const struct msp_protdesc prot_descs[] = { { /* I2S */ @@ -352,17 +364,23 @@ static int configure_multichannel(struct ux500_msp *msp,
static int enable_msp(struct ux500_msp *msp, struct ux500_msp_config *config) { - int status = 0; + int status = 0, retval = 0; u32 reg_val_DMACR, reg_val_GCR; + unsigned long flags;
/* Check msp state whether in RUN or CONFIGURED Mode */ - if ((msp->msp_state == MSP_STATE_IDLE) && (msp->plat_init)) { - status = msp->plat_init(); - if (status) { - dev_err(msp->dev, "%s: ERROR: Failed to init MSP (%d)!\n", - __func__, status); - return status; + if (msp->msp_state == MSP_STATE_IDLE) { + spin_lock_irqsave(&msp_rxtx_lock, flags); + if (pinctrl_rxtx_ref == 0 && + !(IS_ERR(pinctrl_p) || IS_ERR(pinctrl_def))) { + retval = pinctrl_select_state(pinctrl_p, + pinctrl_def); + if (retval) + pr_err("could not set MSP defstate\n"); } + if (!retval) + pinctrl_rxtx_ref++; + spin_unlock_irqrestore(&msp_rxtx_lock, flags); }
/* Configure msp with protocol dependent settings */ @@ -620,7 +638,8 @@ int ux500_msp_i2s_trigger(struct ux500_msp *msp, int cmd, int direction)
int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir) { - int status = 0; + int status = 0, retval = 0; + unsigned long flags;
dev_dbg(msp->dev, "%s: Enter (dir = 0x%01x).\n", __func__, dir);
@@ -631,12 +650,19 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir) writel((readl(msp->registers + MSP_GCR) & (~(FRAME_GEN_ENABLE | SRG_ENABLE))), msp->registers + MSP_GCR); - if (msp->plat_exit) - status = msp->plat_exit(); - if (status) - dev_warn(msp->dev, - "%s: WARN: ux500_msp_i2s_exit failed (%d)!\n", - __func__, status); + + spin_lock_irqsave(&msp_rxtx_lock, flags); + WARN_ON(!pinctrl_rxtx_ref); + pinctrl_rxtx_ref--; + if (pinctrl_rxtx_ref == 0 && + !(IS_ERR(pinctrl_p) || IS_ERR(pinctrl_sleep))) { + retval = pinctrl_select_state(pinctrl_p, + pinctrl_sleep); + if (retval) + pr_err("could not set MSP sleepstate\n"); + } + spin_unlock_irqrestore(&msp_rxtx_lock, flags); + writel(0, msp->registers + MSP_GCR); writel(0, msp->registers + MSP_TCF); writel(0, msp->registers + MSP_RCF); @@ -678,8 +704,6 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev,
msp->id = platform_data->id; msp->dev = &pdev->dev; - msp->plat_init = platform_data->msp_i2s_init; - msp->plat_exit = platform_data->msp_i2s_exit; msp->dma_cfg_rx = platform_data->msp_i2s_dma_rx; msp->dma_cfg_tx = platform_data->msp_i2s_dma_tx;
@@ -717,6 +741,25 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev, dev_dbg(&pdev->dev, "I2S device-name: '%s'\n", i2s_cont->name); msp->i2s_cont = i2s_cont;
+ pinctrl_p = pinctrl_get(msp->dev); + if (IS_ERR(pinctrl_p)) + dev_err(&pdev->dev, "could not get MSP pinctrl\n"); + else { + pinctrl_def = pinctrl_lookup_state(pinctrl_p, + PINCTRL_STATE_DEFAULT); + if (IS_ERR(pinctrl_def)) { + dev_err(&pdev->dev, + "could not get MSP defstate (%li)\n", + PTR_ERR(pinctrl_def)); + } + pinctrl_sleep = pinctrl_lookup_state(pinctrl_p, + PINCTRL_STATE_SLEEP); + if (IS_ERR(pinctrl_sleep)) + dev_err(&pdev->dev, + "could not get MSP idlestate (%li)\n", + PTR_ERR(pinctrl_def)); + } + return 0;
err_i2s_cont: diff --git a/sound/soc/ux500/ux500_msp_i2s.h b/sound/soc/ux500/ux500_msp_i2s.h index 2d9136d..1ce336f 100644 --- a/sound/soc/ux500/ux500_msp_i2s.h +++ b/sound/soc/ux500/ux500_msp_i2s.h @@ -524,8 +524,6 @@ struct ux500_msp { struct dma_chan *rx_pipeid; enum msp_state msp_state; int (*transfer) (struct ux500_msp *msp, struct i2s_message *message); - int (*plat_init) (void); - int (*plat_exit) (void); struct timer_list notify_timer; int def_elem_len; unsigned int dir_busy;
On Tue, Jul 31, 2012 at 02:31:21PM +0100, Lee Jones wrote:
This patch-set includes some important changes which should make their way to the Mainline Release Candidates for the v3.6 release.
You appear to have double sent this or something...
On 31/07/12 14:40, Mark Brown wrote:
On Tue, Jul 31, 2012 at 02:31:21PM +0100, Lee Jones wrote:
This patch-set includes some important changes which should make their way to the Mainline Release Candidates for the v3.6 release.
You appear to have double sent this or something...
I know *embarrassed smiley*.
I didn't clear out my patches directory before re-sending.
I'll re-send this patch-set, less the HIGHMEM patch.
participants (7)
-
Arnd Bergmann -
Lee Jones -
Linus Walleij -
Mark Brown -
Ola Lilja -
Russell King - ARM Linux -
Sergei Shtylyov