[PATCH] Revert "spi: omap2-mcspi: convert to module_platform_driver"

[PATCH] Revert "spi: omap2-mcspi: convert to module_platform_driver"

[Aaro Koskinen]

This reverts commit 9fdca9dfe093c76fe1ac1a09888ba9679d46996a.

Changing omap2_mcspi_init() from subsys_initcall to device_initcall broke
the display initialization on N900 when all the drivers are compiled
built-in. Display subsystem drivers need a certain initialization order
and having all of them initialize with device_initcall seems to be too
fragile. Without this revert the display init fails and the boot hangs
with the following messages:

[    1.260955] acx565akm spi1.2: invalid display ID
[    1.265899] panel-acx565akm display0: acx_panel_probe panel detect error
[    1.273071] omapdss CORE error: driver probe failed: -19

[...]

[    1.902862] omapfb omapfb: no driver for display: lcd
[    1.908264] omapfb omapfb: cannot find default display

Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
---
 drivers/spi/spi-omap2-mcspi.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 0c73dd4..8cec17c 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -1289,5 +1289,18 @@ static struct platform_driver omap2_mcspi_driver = {
 	.remove =	__devexit_p(omap2_mcspi_remove),
 };
 
-module_platform_driver(omap2_mcspi_driver);
+
+static int __init omap2_mcspi_init(void)
+{
+	return platform_driver_register(&omap2_mcspi_driver);
+}
+subsys_initcall(omap2_mcspi_init);
+
+static void __exit omap2_mcspi_exit(void)
+{
+	platform_driver_unregister(&omap2_mcspi_driver);
+
+}
+module_exit(omap2_mcspi_exit);
+
 MODULE_LICENSE("GPL");
-- 
1.7.2.5

Re: [PATCH] Revert "spi: omap2-mcspi: convert to module_platform_driver"

[Grant Likely]

On Thu, Aug 2, 2012 at 5:08 PM, Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
> This reverts commit 9fdca9dfe093c76fe1ac1a09888ba9679d46996a.
>
> Changing omap2_mcspi_init() from subsys_initcall to device_initcall broke
> the display initialization on N900 when all the drivers are compiled
> built-in. Display subsystem drivers need a certain initialization order
> and having all of them initialize with device_initcall seems to be too
> fragile. Without this revert the display init fails and the boot hangs
> with the following messages:
>
> [    1.260955] acx565akm spi1.2: invalid display ID
> [    1.265899] panel-acx565akm display0: acx_panel_probe panel detect error
> [    1.273071] omapdss CORE error: driver probe failed: -19

The dependencies are all messed up. The reverted commit is part of
fixing that and I don't really want to go backwards on it. How many
drivers are failing? Can you try modifying the failure path of those
drivers to return -EPROBE_DEFER and see if that helps?

g.

Re: [PATCH] Revert "spi: omap2-mcspi: convert to module_platform_driver"

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2261 bytes --]

On Thu, 2012-08-02 at 17:54 -0600, Grant Likely wrote:
> On Thu, Aug 2, 2012 at 5:08 PM, Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
> > This reverts commit 9fdca9dfe093c76fe1ac1a09888ba9679d46996a.
> >
> > Changing omap2_mcspi_init() from subsys_initcall to device_initcall broke
> > the display initialization on N900 when all the drivers are compiled
> > built-in. Display subsystem drivers need a certain initialization order
> > and having all of them initialize with device_initcall seems to be too
> > fragile. Without this revert the display init fails and the boot hangs
> > with the following messages:
> >
> > [    1.260955] acx565akm spi1.2: invalid display ID
> > [    1.265899] panel-acx565akm display0: acx_panel_probe panel detect error
> > [    1.273071] omapdss CORE error: driver probe failed: -19
> 
> The dependencies are all messed up. The reverted commit is part of
> fixing that and I don't really want to go backwards on it. How many
> drivers are failing? Can you try modifying the failure path of those
> drivers to return -EPROBE_DEFER and see if that helps?

The description of 9fdca9dfe093c76fe1ac1a09888ba9679d46996a seems to be
overly positive: "this will delete a few lines of code, no functional
changes".

But yes, the probe dependencies with omap's display are quite fragile.
The probe order should be something like (from first to last):

- omapdss, i2c, spi, etc.
- panel drivers
- omapfb

I fear that if we'd use EPROBE_DEFER in a panel driver, it'll lead to
omapfb missing the panel. And we can't use EPROBE_DEFER in omapfb,
because there's no way to know if all the panel drivers are loaded or
not.

I think the only reliable way would be to manage the panels dynamically,
so that omapfb will handle them no matter when they are added/removed.
But that's not done by just changing a few lines...

So, it's broken. One option, although hacky, would be to move all the
spi probe stuff from acx565akm's probe to acx_panel_enable. That will be
called by omapfb during late_initcall, at which point spi should be
available.

Well, I'll see how difficult the dynamic panel add/remove would be to
implement. And probably even dynamic add would be enough for the start.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] Revert "spi: omap2-mcspi: convert to module_platform_driver"

[Aaro Koskinen]

Hi,

On Fri, Aug 03, 2012 at 11:05:27AM +0300, Tomi Valkeinen wrote:
> On Thu, 2012-08-02 at 17:54 -0600, Grant Likely wrote:
> > On Thu, Aug 2, 2012 at 5:08 PM, Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
> > > This reverts commit 9fdca9dfe093c76fe1ac1a09888ba9679d46996a.
> > >
> > > Changing omap2_mcspi_init() from subsys_initcall to device_initcall broke
> > > the display initialization on N900 when all the drivers are compiled
> > > built-in. Display subsystem drivers need a certain initialization order
> > > and having all of them initialize with device_initcall seems to be too
> > > fragile. Without this revert the display init fails and the boot hangs
> > > with the following messages:
> > >
> > > [    1.260955] acx565akm spi1.2: invalid display ID
> > > [    1.265899] panel-acx565akm display0: acx_panel_probe panel detect error
> > > [    1.273071] omapdss CORE error: driver probe failed: -19
> > 
> > The dependencies are all messed up. The reverted commit is part of
> > fixing that and I don't really want to go backwards on it. How many
> > drivers are failing? Can you try modifying the failure path of those
> > drivers to return -EPROBE_DEFER and see if that helps?
> 
> The description of 9fdca9dfe093c76fe1ac1a09888ba9679d46996a seems to be
> overly positive: "this will delete a few lines of code, no functional
> changes".
> 
> But yes, the probe dependencies with omap's display are quite fragile.
> The probe order should be something like (from first to last):
> 
> - omapdss, i2c, spi, etc.

I looked more into this, and it seems this particular problem is related
to probe order within SPI (and a special case with chip selects). The
panel driver on N900 is spi1.2. It's listed in the board file before
spi1.0 (touch controller). When the panel driver is probed, SPI transfers
appear to work (no errors) but all returned data zero:

[    1.371704] omap2_mcspi omap2_mcspi.1: registered master spi1
[    1.378143] spi spi1.2: setup: speed 6000000, sample leading edge, clk normal
[    1.385864] spi spi1.2: setup mode 0, 8 bits/w, 6000000 Hz max --> 0
[    1.393005] acx565akm spi1.2: acx565akm_spi_probe
[    1.398345] panel-acx565akm display0: acx_panel_probe
[    1.416931] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.425415] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.433868] acx565akm spi1.2: LCD panel not enabled by bootloader (status 0x0000)
[    1.441894] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.450347] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.458801] acx565akm spi1.2: MIPI display ID: 000000
[    1.464202] acx565akm spi1.2: invalid display ID
[    1.469085] panel-acx565akm display0: acx_panel_probe panel detect error
[    1.476196] omapdss CORE error: driver probe failed: -19
[    1.482269] omap2_mcspi omap2_mcspi.1: registered child spi1.2
[    1.488525] spi spi1.0: setup: speed 6000000, sample leading edge, clk normal
[    1.496124] spi spi1.0: setup mode 0, 8 bits/w, 6000000 Hz max --> 0
[    1.503265] omap2_mcspi omap2_mcspi.1: registered child spi1.0

If I modify the board file so that child spi1.0 is registered before
spi1.2, then it starts to work properly:

[    1.371978] omap2_mcspi omap2_mcspi.1: registered master spi1
[    1.378417] spi spi1.0: setup: speed 6000000, sample leading edge, clk normal
[    1.386108] spi spi1.0: setup mode 0, 8 bits/w, 6000000 Hz max --> 0
[    1.393249] omap2_mcspi omap2_mcspi.1: registered child spi1.0
[    1.399505] spi spi1.2: setup: speed 6000000, sample leading edge, clk normal
[    1.407104] spi spi1.2: setup mode 0, 8 bits/w, 6000000 Hz max --> 0
[    1.414245] acx565akm spi1.2: acx565akm_spi_probe
[    1.419586] panel-acx565akm display0: acx_panel_probe
[    1.440429] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.448883] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.457305] acx565akm spi1.2: LCD panel enabled by bootloader (status 0x80730417)
[    1.465301] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.473724] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.482147] acx565akm spi1.2: MIPI display ID: 108a77
[    1.487548] acx565akm spi1.2: omapfb: acx565akm rev 8a LCD detected
[    1.494689] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.503112] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.511566] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.520019] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.528442] acx565akm spi1.2: acx565akm_bl_update_status
[    1.534149] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.542572] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.550964] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.559417] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.567840] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.576293] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.584716] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.593109] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
[    1.601867] omap2_mcspi omap2_mcspi.1: registered child spi1.2

So an altenative hack to fix this is to modify the board file
(board-rx51-peripherals.c):

	 /* list all spi devices here */
	 enum {
	 	RX51_SPI_WL1251,
	-	RX51_SPI_MIPID,		/* LCD panel */
	 	RX51_SPI_TSC2005,	/* Touch Controller */
	+	RX51_SPI_MIPID,		/* LCD panel */
	 };

I guess the proper fix would be to modify SPI core so that it first does
spi_setup for all the children/chip selects, before calling any of the
probe functions of SPI devices? (Initializing the controller driver at
subsys_initcall is one way to accomplish this.)

A.

Re: [PATCH] Revert "spi: omap2-mcspi: convert to module_platform_driver"

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6639 bytes --]

Hi,

On Fri, Aug 03, 2012 at 12:45:53PM +0300, Aaro Koskinen wrote:
> Hi,
> 
> On Fri, Aug 03, 2012 at 11:05:27AM +0300, Tomi Valkeinen wrote:
> > On Thu, 2012-08-02 at 17:54 -0600, Grant Likely wrote:
> > > On Thu, Aug 2, 2012 at 5:08 PM, Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
> > > > This reverts commit 9fdca9dfe093c76fe1ac1a09888ba9679d46996a.
> > > >
> > > > Changing omap2_mcspi_init() from subsys_initcall to device_initcall broke
> > > > the display initialization on N900 when all the drivers are compiled
> > > > built-in. Display subsystem drivers need a certain initialization order
> > > > and having all of them initialize with device_initcall seems to be too
> > > > fragile. Without this revert the display init fails and the boot hangs
> > > > with the following messages:
> > > >
> > > > [    1.260955] acx565akm spi1.2: invalid display ID
> > > > [    1.265899] panel-acx565akm display0: acx_panel_probe panel detect error
> > > > [    1.273071] omapdss CORE error: driver probe failed: -19
> > > 
> > > The dependencies are all messed up. The reverted commit is part of
> > > fixing that and I don't really want to go backwards on it. How many
> > > drivers are failing? Can you try modifying the failure path of those
> > > drivers to return -EPROBE_DEFER and see if that helps?
> > 
> > The description of 9fdca9dfe093c76fe1ac1a09888ba9679d46996a seems to be
> > overly positive: "this will delete a few lines of code, no functional
> > changes".
> > 
> > But yes, the probe dependencies with omap's display are quite fragile.
> > The probe order should be something like (from first to last):
> > 
> > - omapdss, i2c, spi, etc.
> 
> I looked more into this, and it seems this particular problem is related
> to probe order within SPI (and a special case with chip selects). The
> panel driver on N900 is spi1.2. It's listed in the board file before
> spi1.0 (touch controller). When the panel driver is probed, SPI transfers
> appear to work (no errors) but all returned data zero:
> 
> [    1.371704] omap2_mcspi omap2_mcspi.1: registered master spi1
> [    1.378143] spi spi1.2: setup: speed 6000000, sample leading edge, clk normal
> [    1.385864] spi spi1.2: setup mode 0, 8 bits/w, 6000000 Hz max --> 0
> [    1.393005] acx565akm spi1.2: acx565akm_spi_probe
> [    1.398345] panel-acx565akm display0: acx_panel_probe
> [    1.416931] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.425415] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.433868] acx565akm spi1.2: LCD panel not enabled by bootloader (status 0x0000)
> [    1.441894] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.450347] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.458801] acx565akm spi1.2: MIPI display ID: 000000
> [    1.464202] acx565akm spi1.2: invalid display ID
> [    1.469085] panel-acx565akm display0: acx_panel_probe panel detect error
> [    1.476196] omapdss CORE error: driver probe failed: -19
> [    1.482269] omap2_mcspi omap2_mcspi.1: registered child spi1.2
> [    1.488525] spi spi1.0: setup: speed 6000000, sample leading edge, clk normal
> [    1.496124] spi spi1.0: setup mode 0, 8 bits/w, 6000000 Hz max --> 0
> [    1.503265] omap2_mcspi omap2_mcspi.1: registered child spi1.0
> 
> If I modify the board file so that child spi1.0 is registered before
> spi1.2, then it starts to work properly:

that sounds quite odd... Shubhro, have you seen this before on any of
our platforms ?

> [    1.371978] omap2_mcspi omap2_mcspi.1: registered master spi1
> [    1.378417] spi spi1.0: setup: speed 6000000, sample leading edge, clk normal
> [    1.386108] spi spi1.0: setup mode 0, 8 bits/w, 6000000 Hz max --> 0
> [    1.393249] omap2_mcspi omap2_mcspi.1: registered child spi1.0
> [    1.399505] spi spi1.2: setup: speed 6000000, sample leading edge, clk normal
> [    1.407104] spi spi1.2: setup mode 0, 8 bits/w, 6000000 Hz max --> 0
> [    1.414245] acx565akm spi1.2: acx565akm_spi_probe
> [    1.419586] panel-acx565akm display0: acx_panel_probe
> [    1.440429] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.448883] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.457305] acx565akm spi1.2: LCD panel enabled by bootloader (status 0x80730417)
> [    1.465301] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.473724] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.482147] acx565akm spi1.2: MIPI display ID: 108a77
> [    1.487548] acx565akm spi1.2: omapfb: acx565akm rev 8a LCD detected
> [    1.494689] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.503112] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.511566] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.520019] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.528442] acx565akm spi1.2: acx565akm_bl_update_status
> [    1.534149] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.542572] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.550964] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.559417] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.567840] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.576293] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.584716] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.593109] acx565akm spi1.2: setup: speed 6000000, sample trailing edge, clk inverted
> [    1.601867] omap2_mcspi omap2_mcspi.1: registered child spi1.2
> 
> So an altenative hack to fix this is to modify the board file
> (board-rx51-peripherals.c):
> 
> 	 /* list all spi devices here */
> 	 enum {
> 	 	RX51_SPI_WL1251,
> 	-	RX51_SPI_MIPID,		/* LCD panel */
> 	 	RX51_SPI_TSC2005,	/* Touch Controller */
> 	+	RX51_SPI_MIPID,		/* LCD panel */
> 	 };
> 
> I guess the proper fix would be to modify SPI core so that it first does
> spi_setup for all the children/chip selects, before calling any of the
> probe functions of SPI devices? (Initializing the controller driver at
> subsys_initcall is one way to accomplish this.)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] Revert "spi: omap2-mcspi: convert to module_platform_driver"

[Shubhrajyoti]

On Friday 03 August 2012 03:28 PM, Felipe Balbi wrote:
>> [    1.496124] spi spi1.0: setup mode 0, 8 bits/w, 6000000 Hz max --> 0
>> > [    1.503265] omap2_mcspi omap2_mcspi.1: registered child spi1.0
>> > 
>> > If I modify the board file so that child spi1.0 is registered before
>> > spi1.2, then it starts to work properly:
> that sounds quite odd... Shubhro, have you seen this before on any of
> our platforms ?
>
I havent seen this behaviour may be I didn't have this configuration.
Will try to see if I have any way of recreating and testing the multi cs
on one of our boards.

BTW I didnt understand why does that matter at all the cs order.

Aaro,
I am missing what is making the order matter.
What is it that I am missing?

Re: [PATCH] Revert "spi: omap2-mcspi: convert to module_platform_driver"

[Aaro Koskinen]

Hi,

On Fri, Aug 03, 2012 at 05:20:48PM +0530, Shubhrajyoti wrote:
> On Friday 03 August 2012 03:28 PM, Felipe Balbi wrote:
> >> [    1.496124] spi spi1.0: setup mode 0, 8 bits/w, 6000000 Hz max --> 0
> >> > [    1.503265] omap2_mcspi omap2_mcspi.1: registered child spi1.0
> >> > 
> >> > If I modify the board file so that child spi1.0 is registered before
> >> > spi1.2, then it starts to work properly:
> > that sounds quite odd... Shubhro, have you seen this before on any of
> > our platforms ?
> >
> I havent seen this behaviour may be I didn't have this configuration.
> Will try to see if I have any way of recreating and testing the multi cs
> on one of our boards.
> 
> BTW I didnt understand why does that matter at all the cs order.
> 
> Aaro,
> I am missing what is making the order matter.
> What is it that I am missing?

With multiple CS you need to configure all CHCONFx regs before any
transfers work. This has been seen earlier with context save and restore
for PM. See e.g. commit 89c05372d08f3982eeb94d1ea22a60a5eaa8cd6d.

A.

Re: [PATCH] Revert "spi: omap2-mcspi: convert to module_platform_driver"

[Jarkko Nikula]

On Fri, 3 Aug 2012 12:45:53 +0300
Aaro Koskinen <aaro.koskinen@iki.fi> wrote:

> So an altenative hack to fix this is to modify the board file
> (board-rx51-peripherals.c):
> 
> 	 /* list all spi devices here */
> 	 enum {
> 	 	RX51_SPI_WL1251,
> 	-	RX51_SPI_MIPID,		/* LCD panel */
> 	 	RX51_SPI_TSC2005,	/* Touch Controller */
> 	+	RX51_SPI_MIPID,		/* LCD panel */
> 	 };
> 
> I guess the proper fix would be to modify SPI core so that it first does
> spi_setup for all the children/chip selects, before calling any of the
> probe functions of SPI devices? (Initializing the controller driver at
> subsys_initcall is one way to accomplish this.)
> 
As the N900 framebuffer still appears to be broken would it make sense to queue above change as a workaround for 3.5 and 3.6?

Aaro: For above change you could add my 'Tested-by: Jarkko Nikula <jarkko.nikula@bitmer.com>'

-- 
Jarkko