"Poddar, Sourav" <[email protected]> writes:
> Hi,
>
> On Tue, Sep 25, 2012 at 2:51 PM, Russell King - ARM Linux
> <[email protected]> wrote:
>> On Tue, Sep 25, 2012 at 12:11:14PM +0300, Felipe Balbi wrote:
>>> On Tue, Sep 25, 2012 at 10:12:28AM +0100, Russell King - ARM Linux wrote:
>>> > On Tue, Sep 25, 2012 at 11:31:20AM +0300, Felipe Balbi wrote:
>>> > > On Tue, Sep 25, 2012 at 09:30:29AM +0100, Russell King - ARM Linux wrote:
>>> > > > How is this happening? I think that needs proper investigation - or if
>>> > > > it's had more investigation, then the results needs to be included in
>>> > > > the commit description so that everyone can understand the issue here.
>>> > > >
>>> > > > We should not be resuming a device which hasn't been suspended. Maybe
>>> > > > the runtime PM enable sequence is wrong, and that's what should be fixed
>>> > > > instead?
>>> > > >
>>> > > > This sequence in the probe() function:
>>> > > >
>>> > > > pm_runtime_irq_safe(&pdev->dev);
>>> > > > pm_runtime_enable(&pdev->dev);
>>> > > > pm_runtime_get_sync(&pdev->dev);
>>> > > >
>>> > > > would enable runtime PM while the s/w state indicates that it's disabled,
>>> > > > and then that pm_runtime_get_sync() will want to resume the device. See
>>> > > > the section "5. Runtime PM Initialization, Device Probing and Removal"
>>> > > > in Documentation/power/runtime_pm.txt, specifically the second paragraph
>>> > > > of that section.
>>> > >
>>> > > that was tested. It worked in pandaboard but didn't work on beagleboard
>>> > > XM. Sourav tried to start a discussion about that, but it simply died...
>>> > >
>>> > > In any case, pm_runtime_get_sync() in probe will always call
>>> > > runtime_resume callback, right ?
>>> >
>>> > Well, if the runtime PM state says it's suspended, and then you enable
>>> > runtime PM, the first call to pm_runtime_get_sync() will trigger a resume
>>> > attempt. The patch description is complaining about resume events without
>>> > there being a preceding suspend event.
>>> >
>>> > This could well be why.
>>>
>>> that's most likely, of course. But should we cause a regression to
>>> beagleboard XM because of that ?
>>
>> What would cause a regression on beagleboard XM? I have not suggested
>> any change other than more investigation of the issue and a fuller patch
>> description - yet you're screaming (idiotically IMHO) that mere
>> investigation would break beagleboard.
>>
>> Well, if it's _that_ fragile, that mere investigation of this issue by
>> someone elsewhere on the planet would break your beagleboard, maybe it
>> deserves to be broken!
>
> The issue was observed at serial init itself in the N800 board and the
> log does not
> show up much.
> http://www.pwsan.com/omap/testlogs/test_tty_next_e36851d0/20120910020323/boot/2420n800/2420n800_log.txt
> What we thought the problem might be with n800 is that it tries to
> resume when it didn't suspend before.
>
> There are two ways through which we thought of handling this issue:
>
> a) set device as active before enabling pm (which will prevent
>
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
>
> OR
>
> b) adding a "suspended" flag to struct omap_uart_port which gets set on
> suspend and cleared on resume. Then on resume you can check:
>
> if (!up->suspended)
> return 0;
>
> But using "pm_runtime_set_active" approach breaks things even on
> beagle board xm, though
> it works fine on Panda.
> Therefore, we used the "suspended" flag approach.
>
> So. I just wanted to get some feedback from community about how using
> "pm_runtime_set_active"
> behaves differently in omap3 and omap4.
As Russell has already pointed out in great detail, the difference is
simply a mismatch between assumed HW stated and actual hardware state
between various boards. Put simply, the driver assumes the HW is
disabled (runtime suspended) when it loads, and the first runtime resume
is meant to enable the HW. If that assumption is wrong, it needs to be
fixed.
Have you figured out why the HW is already active on OMAP2? (probably
bootloader?)
That being said, already active HW should not cause this problem. In
fact, because of possible early console use, the hwmod init of the UART
hwmods does not idle/reset them on boot, so they are left in the state
that the bootloader set them in.
When the hwmod is later enabled for real during probe, the hwmod muxing
is done for that IP. So, I suspect what is really happening is that the
mux settings are not right for the UARTS on n800, so when the probe
happens, the UART mux settings are changed and you lose the UART.
Can you double check the UART mux settings for that board? You might
need some different mux settings in the board file.
Kevin
Hi Sourav, Felipe,
any progress on fixing the N800 problem? Would be good to keep it booting
since we use it as our primary 2420 test platform.
- Paul
Hi Paul,
There are
On Thursday 11 October 2012 11:58 PM, Paul Walmsley wrote:
> Hi Sourav, Felipe,
>
> any progress on fixing the N800 problem? Would be good to keep it booting
> since we use it as our primary 2420 test platform.
>
>
> - Paul
The patch sent inlined below might help us to get rid of the serial init
issue.
Unfortunately, I dont have a N800 board with me to test it and will require
your help to do so.
-----------
From: Sourav Poddar <[email protected]>
Date: Wed, 1 Aug 2012 15:44:12 +0530
Subject: [RFT/PATCH] serial: omap: Fix N800 serial init issue.
This patch might solve the N800 serial init issue.
This patch will also give pointers if there is any mux settings issue
with N800 OR
a mismatch between the initial harware state, runtime PM state and omap
hwmod state.
I don't have a N800 schematics to check about the mux settings getting used.
The observation on beagle board XM with this patch on different boards
looks flaky,
so your feedback on beagle board will also be very helpful.
Cc: Felipe Balbi <[email protected]>
Signed-off-by: Sourav Poddar <[email protected]>
---
drivers/tty/serial/omap-serial.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c
b/drivers/tty/serial/omap-serial.c
index 6ede6fd..3fbc7f7 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1414,6 +1414,7 @@ static int __devinit serial_omap_probe(struct
platform_device *pdev)
INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
platform_set_drvdata(pdev, up);
+ pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_set_autosuspend_delay(&pdev->dev,
--
1.7.1
Sourav <[email protected]> writes:
> Hi Paul,
>
> There are
> On Thursday 11 October 2012 11:58 PM, Paul Walmsley wrote:
>> Hi Sourav, Felipe,
>>
>> any progress on fixing the N800 problem? Would be good to keep it booting
>> since we use it as our primary 2420 test platform.
>>
>>
>> - Paul
> The patch sent inlined below might help us to get rid of the serial
> init issue.
> Unfortunately, I dont have a N800 board with me to test it and will require
> your help to do so.
> -----------
> From: Sourav Poddar <[email protected]>
> Date: Wed, 1 Aug 2012 15:44:12 +0530
> Subject: [RFT/PATCH] serial: omap: Fix N800 serial init issue.
>
>
> This patch might solve the N800 serial init issue.
>
> This patch will also give pointers if there is any mux settings issue
> with N800 OR
> a mismatch between the initial harware state, runtime PM state and
> omap hwmod state.
> I don't have a N800 schematics to check about the mux settings getting used.
>
> The observation on beagle board XM with this patch on different boards
> looks flaky,
> so your feedback on beagle board will also be very helpful.
>
> Cc: Felipe Balbi <[email protected]>
> Signed-off-by: Sourav Poddar <[email protected]>
> ---
> drivers/tty/serial/omap-serial.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c
> b/drivers/tty/serial/omap-serial.c
> index 6ede6fd..3fbc7f7 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -1414,6 +1414,7 @@ static int __devinit serial_omap_probe(struct
> platform_device *pdev)
> INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
>
> platform_set_drvdata(pdev, up);
> + pm_runtime_set_active(&pdev->dev);
NAK.
This will obviously break platforms where the UARTs are not active
before driver loads.
Kevin
> pm_runtime_enable(&pdev->dev);
> pm_runtime_use_autosuspend(&pdev->dev);
> pm_runtime_set_autosuspend_delay(&pdev->dev,
On Fri, Oct 12, 2012 at 09:35:54AM -0700, Kevin Hilman wrote:
> Sourav <[email protected]> writes:
> > diff --git a/drivers/tty/serial/omap-serial.c
> > b/drivers/tty/serial/omap-serial.c
> > index 6ede6fd..3fbc7f7 100644
> > --- a/drivers/tty/serial/omap-serial.c
> > +++ b/drivers/tty/serial/omap-serial.c
> > @@ -1414,6 +1414,7 @@ static int __devinit serial_omap_probe(struct
> > platform_device *pdev)
> > INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
> >
> > platform_set_drvdata(pdev, up);
> > + pm_runtime_set_active(&pdev->dev);
>
> NAK.
>
> This will obviously break platforms where the UARTs are not active
> before driver loads.
I thought I had proposed a solution for this issue, which was this
sequence:
omap_device_enable(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
Yes, I can understand people not liking the omap_device_enable()
there, but I also notice that the email suggesting that never got a
reply either - not even a "I tried this and it doesn't work" or "it
does work".
As such, it seems this issue isn't making any progress as we had
already established that merely doing a "pm_runtime_set_active()"
before "pm_runtime_enable()" was going to break other platforms.
Hi Russell,
________________________________________
From: Russell King - ARM Linux [[email protected]]
Sent: Friday, October 12, 2012 10:12 PM
To: Kevin Hilman
Cc: Poddar, Sourav; Paul Walmsley; Balbi, Felipe; [email protected]; [email protected]; [email protected]; Shilimkar, Santosh; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
On Fri, Oct 12, 2012 at 09:35:54AM -0700, Kevin Hilman wrote:
> Sourav <[email protected]> writes:
> > diff --git a/drivers/tty/serial/omap-serial.c
> > b/drivers/tty/serial/omap-serial.c
> > index 6ede6fd..3fbc7f7 100644
> > --- a/drivers/tty/serial/omap-serial.c
> > +++ b/drivers/tty/serial/omap-serial.c
> > @@ -1414,6 +1414,7 @@ static int __devinit serial_omap_probe(struct
> > platform_device *pdev)
> > INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
> >
> > platform_set_drvdata(pdev, up);
> > + pm_runtime_set_active(&pdev->dev);
>
> NAK.
>
> This will obviously break platforms where the UARTs are not active
> before driver loads.
I thought I had proposed a solution for this issue, which was this
sequence:
omap_device_enable(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
Yes, I can understand people not liking the omap_device_enable()
there, but I also notice that the email suggesting that never got a
reply either - not even a "I tried this and it doesn't work" or "it
does work".
Sorry for the late reply on this. I tried this sequence and it worked perfectly fine on
panda and beagle.
As such, it seems this issue isn't making any progress as we had
already established that merely doing a "pm_runtime_set_active()"
before "pm_runtime_enable()" was going to break other platforms.
I was trying to analyse your explanations on this and since omap_device_enable is not generally
recommended, I was trying to see if anything else can be done to get around this.
I send this patch for N800 testing so as to see how it behaves. (We are suspecting that there might be
mux setting issue also with N800).
~Sourav
Russell King - ARM Linux <[email protected]> writes:
> On Fri, Oct 12, 2012 at 09:35:54AM -0700, Kevin Hilman wrote:
>> Sourav <[email protected]> writes:
>> > diff --git a/drivers/tty/serial/omap-serial.c
>> > b/drivers/tty/serial/omap-serial.c
>> > index 6ede6fd..3fbc7f7 100644
>> > --- a/drivers/tty/serial/omap-serial.c
>> > +++ b/drivers/tty/serial/omap-serial.c
>> > @@ -1414,6 +1414,7 @@ static int __devinit serial_omap_probe(struct
>> > platform_device *pdev)
>> > INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
>> >
>> > platform_set_drvdata(pdev, up);
>> > + pm_runtime_set_active(&pdev->dev);
>>
>> NAK.
>>
>> This will obviously break platforms where the UARTs are not active
>> before driver loads.
>
> I thought I had proposed a solution for this issue, which was this
> sequence:
>
> omap_device_enable(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
>
> Yes, I can understand people not liking the omap_device_enable()
> there, but I also notice that the email suggesting that never got a
> reply either - not even a "I tried this and it doesn't work" or "it
> does work".
Yes, that solution would work (though I didn't actually try it.)
However, we can't use omap_device_enable() in the driver. We're trying
to clean all the drivers of OMAP-specific APIs. That being said,
something similar could be done in the device/board init code to ensure
the UART HW is in the state that the driver is expecting it, but again,
that would just mask the real problem which is that a (re)init of the
console UART on 2420/n800 is causing output to disappear.
As I detailed in an earlier response, I still think it's the fact that
the pinmux is not setup correctly for the console UART pins in the board
file, so when the UART is initialized, its mux settings are changed from
the bootloader defaults, causing output to disappear.
> As such, it seems this issue isn't making any progress as we had
> already established that merely doing a "pm_runtime_set_active()"
> before "pm_runtime_enable()" was going to break other platforms.
Agreed.
Kevin
On Fri, Oct 12, 2012 at 05:29:55PM +0000, Poddar, Sourav wrote:
> Hi Russell,
> ________________________________________
> From: Russell King - ARM Linux [[email protected]]
> Sent: Friday, October 12, 2012 10:12 PM
> To: Kevin Hilman
> Cc: Poddar, Sourav; Paul Walmsley; Balbi, Felipe; [email protected]; [email protected]; [email protected]; Shilimkar, Santosh; [email protected]; [email protected]; [email protected]; [email protected]
> Subject: Re: [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
>
> On Fri, Oct 12, 2012 at 09:35:54AM -0700, Kevin Hilman wrote:
> > Sourav <[email protected]> writes:
> > > diff --git a/drivers/tty/serial/omap-serial.c
> > > b/drivers/tty/serial/omap-serial.c
> > > index 6ede6fd..3fbc7f7 100644
> > > --- a/drivers/tty/serial/omap-serial.c
> > > +++ b/drivers/tty/serial/omap-serial.c
> > > @@ -1414,6 +1414,7 @@ static int __devinit serial_omap_probe(struct
> > > platform_device *pdev)
> > > INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
> > >
> > > platform_set_drvdata(pdev, up);
> > > + pm_runtime_set_active(&pdev->dev);
> >
> > NAK.
> >
> > This will obviously break platforms where the UARTs are not active
> > before driver loads.
>
> I thought I had proposed a solution for this issue, which was this
> sequence:
>
> omap_device_enable(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
>
> Yes, I can understand people not liking the omap_device_enable()
> there, but I also notice that the email suggesting that never got a
> reply either - not even a "I tried this and it doesn't work" or "it
> does work".
>
> Sorry for the late reply on this. I tried this sequence and it worked perfectly fine on
> panda and beagle.
>
> As such, it seems this issue isn't making any progress as we had
> already established that merely doing a "pm_runtime_set_active()"
> before "pm_runtime_enable()" was going to break other platforms.
>
> I was trying to analyse your explanations on this and since
> omap_device_enable is not generally recommended, I was trying to see
> if anything else can be done to get around this.
Right, so what you need is explanation about why I believe the above
sequence to be necessary.
What is happening is that we're starting from a device in unknown state.
We don't know whether it is enabled or disabled. We don't know the
state of the clocks or the power domain.
PM runtime state is initialized at device creation in the "device is
suspended" state. If we merely enable PM runtime from that state, we
are telling the PM runtime subsystem that the device is _indeed_
suspended (disabled) at boot time.
So, that causes the first pm_runtime_get() call to resume the device.
Due to the OMAP runtime PM hooks at the bus layer, this causes
_od_runtime_resume() to be called.
_od_runtime_resume() does two things. It calls omap_device_enable()
to ensure that the device is woken up (such as, ensuring that the
power domain is on, and turning on the clocks etc.) It then goes on
to call the device PM layers to call the driver specific runtime PM
resume hook.
So, in summary, what this sequence does is:
pm_runtime_enable(&pdev->dev);
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_set_autosuspend_delay(&pdev->dev,
omap_up_info->autosuspend_timeout);
pm_runtime_irq_safe(&pdev->dev);
pm_runtime_get_sync(&pdev->dev);
is, at the last call, it calls:
_od_runtime_resume()
omap_device_enable()
serial_omap_runtime_resume()
Your original patch at the head of this thread says that the driver
specific runtime resume call causes a problem for N800 - because the
device is already enabled and setup.
Okay, so the initial device state does not match the runtime PM state.
So, what happens if we _do_ make it match your state - as required by
the runtime PM documentation - by adding a call before the sequence:
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_set_autosuspend_delay(&pdev->dev,
omap_up_info->autosuspend_timeout);
pm_runtime_irq_safe(&pdev->dev);
pm_runtime_get_sync(&pdev->dev);
Right, now runtime PM knows that the device is enabled and alive prior
to that pm_runtime_get_sync() call, and it will _not_ call the runtime
resume hooks.
However, this breaks beaglebone, because the device is disabled when
this driver probes. So, we have exactly the opposite problem here -
the device is disabled, but runtime PM thinks it is enabled.
The _two_ problems are precisely the same problem: the runtime PM state
does not accurately reflect the actual hardware state - again, as required
by the runtime PM documentation. The only sane solution to this is to
ensure that the hardware _is_ in a known state prior to enabling runtime
PM.
How do we do that? Well, the clue is in the bus layer runtime resume
handler - that's what is missing from the beaglebone situation.
Calling this before calling pm_runtime_set_active() gets the hardware
into a known state (enabled), and we then tell the runtime PM code
that the harware _is_ enabled. Now, runtime PM can be sure what the
initial state is, and everything works.
What's the longer term answer?
Well, I _bet_ all OMAP drivers are doing something along the lines of:
pm_runtime_enable(dev);
pm_runtime_get(dev);
in their probe functions.
PCI already solved this problem - partly because it has far too many drivers
to convert. I took that solution over to the AMBA bus layer, because I
didn't want to have a flag day for all the drivers to convert over in one
massive patch. What is this solution?
You get the bus layer to handle the initial setup of runtime PM state like
this - in OMAP's case:
omap_device_enable(pdev);
pm_runtime_get_noresume(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
You do this prior to calling the device probe function. If the device
probe fails, then you can undo those actions. You also need to undo them
when the device is unbound from the driver:
pm_runtime_disable(&pdev->dev);
pm_runtime_set_suspended(&pdev->dev);
pm_runtime_put_noidle(&pdev->dev);
(It is probably dangerous to call omap_device_disable() here for certain
devices...)
This gets rid of all that driver specific runtime PM initialization, with
questionable starting state. It also means that your devices all get
runtime PM support in so far as if they're bound to a driver, they will
be runtime PM resumed, and when unbound, they will be runtime PM suspended.
However, it means that the driver has to do something to make runtime PM
work. In the probe function, just before it returns success, it has to
'put' that pm_runtime_get_noresume() reference to allow the device to
enter runtime PM states. And more importantly, on a remove call, it
_must_ balance that 'put' in the probe with an appropriate 'get' - no
exceptions to that.
And that is how we get to a sane state over runtime PM here, which will
work in every situation on every device, without throwing calls to
omap_device_enable() into every OMAP device driver.
This also has another advantage - by doing that, the OMAP specific
omap_device_enable() call ends up being in bus layer code, not driver
layer, which is the right place for it to be - after all, it's the
bus layer which is already handling that stuff in its runtime PM support
code.
On Fri, Oct 12, 2012 at 10:59:22AM -0700, Kevin Hilman wrote:
> Russell King - ARM Linux <[email protected]> writes:
>
> > On Fri, Oct 12, 2012 at 09:35:54AM -0700, Kevin Hilman wrote:
> >> Sourav <[email protected]> writes:
> >> > diff --git a/drivers/tty/serial/omap-serial.c
> >> > b/drivers/tty/serial/omap-serial.c
> >> > index 6ede6fd..3fbc7f7 100644
> >> > --- a/drivers/tty/serial/omap-serial.c
> >> > +++ b/drivers/tty/serial/omap-serial.c
> >> > @@ -1414,6 +1414,7 @@ static int __devinit serial_omap_probe(struct
> >> > platform_device *pdev)
> >> > INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
> >> >
> >> > platform_set_drvdata(pdev, up);
> >> > + pm_runtime_set_active(&pdev->dev);
> >>
> >> NAK.
> >>
> >> This will obviously break platforms where the UARTs are not active
> >> before driver loads.
> >
> > I thought I had proposed a solution for this issue, which was this
> > sequence:
> >
> > omap_device_enable(dev);
> > pm_runtime_set_active(dev);
> > pm_runtime_enable(dev);
> >
> > Yes, I can understand people not liking the omap_device_enable()
> > there, but I also notice that the email suggesting that never got a
> > reply either - not even a "I tried this and it doesn't work" or "it
> > does work".
>
> Yes, that solution would work (though I didn't actually try it.)
>
> However, we can't use omap_device_enable() in the driver. We're trying
> to clean all the drivers of OMAP-specific APIs. That being said,
> something similar could be done in the device/board init code to ensure
> the UART HW is in the state that the driver is expecting it, but again,
> that would just mask the real problem which is that a (re)init of the
> console UART on 2420/n800 is causing output to disappear.
See my more expansive suggestion just posted.
Whatever way, this discrepancy between runtime PM state and actual device
state is what is biting you, and it is that which needs fixing. It's
fairly easy to fix given the right design, one which several other bus
types are already using.
Given the route that OMAP went down when adopting runtime PM, it's going
to be a big change across many drivers, because there's no way to gradually
transition them, but that's unfortunately one of the results of ignoring
requirements of the layers being used. Sooner or later the oversights
come back to haunt. Just make sure it's not the ghost of Jaws.
Russell King - ARM Linux <[email protected]> writes:
> On Fri, Oct 12, 2012 at 10:59:22AM -0700, Kevin Hilman wrote:
>> Russell King - ARM Linux <[email protected]> writes:
>>
>> > On Fri, Oct 12, 2012 at 09:35:54AM -0700, Kevin Hilman wrote:
>> >> Sourav <[email protected]> writes:
>> >> > diff --git a/drivers/tty/serial/omap-serial.c
>> >> > b/drivers/tty/serial/omap-serial.c
>> >> > index 6ede6fd..3fbc7f7 100644
>> >> > --- a/drivers/tty/serial/omap-serial.c
>> >> > +++ b/drivers/tty/serial/omap-serial.c
>> >> > @@ -1414,6 +1414,7 @@ static int __devinit serial_omap_probe(struct
>> >> > platform_device *pdev)
>> >> > INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
>> >> >
>> >> > platform_set_drvdata(pdev, up);
>> >> > + pm_runtime_set_active(&pdev->dev);
>> >>
>> >> NAK.
>> >>
>> >> This will obviously break platforms where the UARTs are not active
>> >> before driver loads.
>> >
>> > I thought I had proposed a solution for this issue, which was this
>> > sequence:
>> >
>> > omap_device_enable(dev);
>> > pm_runtime_set_active(dev);
>> > pm_runtime_enable(dev);
>> >
>> > Yes, I can understand people not liking the omap_device_enable()
>> > there, but I also notice that the email suggesting that never got a
>> > reply either - not even a "I tried this and it doesn't work" or "it
>> > does work".
>>
>> Yes, that solution would work (though I didn't actually try it.)
>>
>> However, we can't use omap_device_enable() in the driver. We're trying
>> to clean all the drivers of OMAP-specific APIs. That being said,
>> something similar could be done in the device/board init code to ensure
>> the UART HW is in the state that the driver is expecting it, but again,
>> that would just mask the real problem which is that a (re)init of the
>> console UART on 2420/n800 is causing output to disappear.
>
> See my more expansive suggestion just posted.
>
> Whatever way, this discrepancy between runtime PM state and actual device
> state is what is biting you, and it is that which needs fixing.
I'm not conviced (yet) that a mismatch is the root cause. Yes, that's
what the author of $SUBJECT patch assumed and stated, but I'm not
pursuaded.
If it's an improperly configured mux issue, then the UART will break
whenever the device is actually omap_device_enable'd, whether in the
driver or in the bus layer.
Kevin
* Kevin Hilman <[email protected]> [121012 13:34]:
>
> I'm not conviced (yet) that a mismatch is the root cause. Yes, that's
> what the author of $SUBJECT patch assumed and stated, but I'm not
> pursuaded.
>
> If it's an improperly configured mux issue, then the UART will break
> whenever the device is actually omap_device_enable'd, whether in the
> driver or in the bus layer.
I tried booting n800 here with CONFIG_OMAP_MUX disabled, and no
change. Serial console output stops right when the console initializes.
Regards,
Tony
Tony Lindgren <[email protected]> writes:
> * Kevin Hilman <[email protected]> [121012 13:34]:
>>
>> I'm not conviced (yet) that a mismatch is the root cause. Yes, that's
>> what the author of $SUBJECT patch assumed and stated, but I'm not
>> pursuaded.
>>
>> If it's an improperly configured mux issue, then the UART will break
>> whenever the device is actually omap_device_enable'd, whether in the
>> driver or in the bus layer.
>
> I tried booting n800 here with CONFIG_OMAP_MUX disabled, and no
> change. Serial console output stops right when the console initializes.
OK, since it's not mux, and since those who actually maintain this
driver don't seem to be taking care of this, I did some digging today.
Russell is right. It's a mismatch between assumed runtime PM state
(disabled) and actual HW state.
During init, all omap_devices are idled by default, so that they are
correctly in the state that the runtime PM framework will later expect
them to be. That is, all devices *except* the console UART. For that
one, we use the special hwmod flag to not idle/reset the UART since that
will cause problems during earlyprintk usage, and the switch between
the earlyprintk console and the real console driver.
Since the console uart was left enabled during init, it needs to be
fully enabled and the runtime PM status set accordingly.
The patch below does this, and works fine on 2420/n810, as well as on
3530/Overo, 3730/OveroSTORM, 3730/Beagle-XM and 4430/PandaES.
Will send patch with a proper changelog shortly,
Kevin
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 0405c81..37b5dbe 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -327,6 +327,11 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
if ((console_uart_id == bdata->id) && no_console_suspend)
omap_device_disable_idle_on_suspend(pdev);
+ if (console_uart_id == bdata->id) {
+ omap_device_enable(pdev);
+ pm_runtime_set_active(&pdev->dev);
+ }
+
oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
oh->dev_attr = uart;