* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Felipe Contreras @ 2012-02-03 12:12 UTC (permalink / raw)
To: NeilBrown
Cc: Paul Walmsley, Greg KH, greg, khilman, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <20120203150708.386951d2@notabene.brown>
On Fri, Feb 3, 2012 at 6:07 AM, NeilBrown <neilb@suse.de> wrote:
> If I then enabled runtime_pm with a 5 second autosuspend delay:
> CORE is still permanently ON (I think the USB might be doing that).
Maybe related to this:
http://thread.gmane.org/gmane.linux.usb.general/56096
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: NeilBrown @ 2012-02-03 12:11 UTC (permalink / raw)
To: Grazvydas Ignotas
Cc: Paul Walmsley, Greg KH, greg, khilman, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <CANOLnONsZKHJPbEx2+ic3EZ1PqcD30VT4Z2bR5iW-JNQJQL0GA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]
On Fri, 3 Feb 2012 13:42:13 +0200 Grazvydas Ignotas <notasas@gmail.com> wrote:
> On Fri, Feb 3, 2012 at 11:54 AM, NeilBrown <neilb@suse.de> wrote:
> > On Thu, 2 Feb 2012 22:45:53 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
> >> On Fri, 3 Feb 2012, NeilBrown wrote:
> >>
> >> > then CPUIDLE enters lower states and I think it uses less power but I
> >> > sometimes lose the first char I type (that is known) but also I sometimes get
> >> > corruption on output.
> >>
> >> I don't see any output corruption on 35xx BeagleBoard, either with or
> >> without these patches. So this is presumably a 37xx-centric problem, and
> >> unrelated to these patches, I would guess.
> >
> > Maybe it is 37xx specific. I think this is a DM3730.
>
> Not sure if it's the same problem but with 3530 on 3.2 with
> sleep_timeout set, I usually get first char dropped (as expected) but
> sometimes I get corrupted char instead too. Corrupt char seems to
> almost always happen if I set cpufreq to powersave, on performace it's
> almost always ok, so maybe it's some timing problem,
I see that too - I'm glad someone else does :-)
If I look at the corruption as compared to the expected character, it is
always the case some it has been shifted down a few bits, with '1's shifted
in the top.
As bytes are sent lsb first followed by stop bits which are '1', this is
completely consistent with clocking the bits in a little bit late.
I see this with baud rates of 115200 and higher, but never with lower.
My theory is that there is a delay between the falling RX line waking the
system up, and the CPU enabling the UART - whether enabling the clocks or
doing a full config, I am not sure - though I think the former.
Maybe if we could enable the UART clocks immediately after returning from the
WFI instruction we could avoid the corruption....
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: NeilBrown @ 2012-02-03 12:07 UTC (permalink / raw)
To: Govindraj
Cc: Paul Walmsley, greg, khilman, govindraj.raja, tomi.valkeinen,
linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <CAAL8m4ztuB-kJvLGd=8PqPCLFcvPW9AQcdcFQ+e03vDT240vog@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6882 bytes --]
On Fri, 3 Feb 2012 12:26:14 +0530 Govindraj <govindraj.ti@gmail.com> wrote:
> On Fri, Feb 3, 2012 at 9:37 AM, NeilBrown <neilb@suse.de> wrote:
> > On Thu, 2 Feb 2012 13:03:01 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
> >
> >> Hi Greg,
> >>
> >> On Thu, 26 Jan 2012, Paul Walmsley wrote:
> >>
> >> > On Thu, 26 Jan 2012, Greg KH wrote:
> >> >
> >> > > Ok, I've just reverted both of these patches for now, please send new
> >> > > ones when you have them.
> >> >
> >> > Thanks. A pull request is at the bottom of this message. The patches
> >> > are also available from the mailing list archives here:
> >> >
> >> > http://marc.info/?l=linux-arm-kernel&m=132754676014389&w=2
> >> > http://marc.info/?l=linux-arm-kernel&m=132754677914395&w=2
> >> > http://marc.info/?l=linux-arm-kernel&m=132754676014388&w=2
> >>
> >> Any comments on these?
> >
> > Can I comment??... They are good but ....
> >
> > I've tried two approaches to getting serial behaviour that I am happy with.
> > They are with and without runtime pm.
> >
> > If I enable runtime pm by setting power/autosuspend_delay_ms (e.g. to 5000)
> > then CPUIDLE enters lower states and I think it uses less power but I
> > sometimes lose the first char I type (that is known) but also I sometimes get
> > corruption on output.
> >
> > The problem seems to be that we turn off the clocks when the kernel's ring
> > buffer is empty rather than waiting for the UART's fifo to be empty.
> > So I get corruption near the end of a stream of output ... not right at the
> > end so something must be turning the clocks back on somehow.
> >
> > I can remove this effect with:
> >
> > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> > index f809041..c7ef760 100644
> > --- a/drivers/tty/serial/omap-serial.c
> > +++ b/drivers/tty/serial/omap-serial.c
> > @@ -440,7 +440,8 @@ static unsigned int serial_omap_tx_empty(struct uart_port *port)
> > spin_lock_irqsave(&up->port.lock, flags);
> > ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
> > spin_unlock_irqrestore(&up->port.lock, flags);
> > - pm_runtime_put(&up->pdev->dev);
> > + pm_runtime_mark_last_busy(&up->pdev->dev);
> > + pm_runtime_put_autosuspend(&up->pdev->dev);
> > return ret;
> > }
> >
>
> But looking into it UART_LSR_TEMT("include/linux/serial_reg.h") => 0x40
>
> from omap trm:
>
> TX_SR_E => bit 6
> "Read 0x1: Transmitter hold (TX FIFO) and shift registers are empty."
>
> So it means all data from tx fifo has shifted out and no pending data in
> tx fifo.
>
> But we had used UART_LSR_THRE (0x20) here for tx fifo emptiness comparison
> then from omap trm
>
> TX_FIFO_E => bit 5
>
> "Read 0x1: Transmit hold register (TX FIFO) is empty.
> The transmission is not necessarily complete"
>
> So I think all data has been shifted out from tx fifo for
> serial_omap_tx_empty check.
Useful and interesting - thanks.
However what it really shows me is that I was misunderstanding the code
(If I spent the time to verify every conclusion that I jumped to, I'd never
get anywhere :-( ). Better clarify that now.
So this function - serial_omap_tx_empty() is called to test if the
tx queue is empty.
It is called in a loop at the end of uart_wait_until_sent.
uart_wait_until_sent it calls from various places, but particularly when
doing an 'stty' ioctl.
The loop isn't a very tight loop though it would be tighter if jiffies were
smaller. As it is it checks every jiffy and usually loops 3 times when I
see corruption.
So when I
cat somefile
to the serial console, most of the file comes out fine. But after 'cat'
finishes and returns to the shell - while some chars are still in the FIFO -
the shell does an 'stty' ioctl to make sure the settings are still OK.
This ioctl calls serial_omap_tx_empty which calls pm_resume_put() which
immediately suspends the uart, which seems to stop some clock - even though
we think it shouldn't.
(If is use 'dash' instead of 'bash', that shell doesn't fiddle with stty, and
I don't see any corruption).
After a bit more experimentation, I find that if either UART3 or UART4 (which
are numbers 2 and 3 is sysfs!!!) have auto_suspend_delay set to -1, then I
don't see any corruption.
But if both are set to 5000, then I do.
(The settings of UART1 and UART2 seem to be irrelevant - presumably because
they are in CORE, not PER).
So if either uart wants PER_48M_FCLK on, then it stays on. But if neither
explicitly request it and are only keeping it on by being active ... then
there is room for a hiccup it seems.
Or maybe it isn't the clocks ... maybe the problem is that PER goes into
RET mode, which it does for about 40msec.
If I run
cat /sys/kernel/debug/pm_debug/time; stty 115200; \
cat /sys/kernel/debug/pm_debug/time
then I see corruption near the end of the first output of the 'time' file,
and the time that PER is in RET goes up by about 40msec.
This could be because the clock goes off, or maybe it has some other cause
that I'm not aware of.
So yeah - I was quite wrong in my original analysis, thanks for encouraging
me to sort it out.
This is awfully similar to the problem I had with HDQ - clock seeming to
disappear when it should not.
There is something important I'm missing.... time to go back to the TRM I
guess..
>
> >
> > i.e. when the tx buffer is empty, so turn the clocks off immediately, but
> > wait a while for the fifo to empty. Hopefully the auto-suspend time is
> > enough.
> >
>
> [...]
>
> >
> > p.s. who should I formally submit OMAP-UART patches to? I have a couple of
> > others such as the below that I should submit somewhere.
> >
> >
> > diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> > index 247d894..35a649f 100644
> > --- a/arch/arm/mach-omap2/serial.c
> > +++ b/arch/arm/mach-omap2/serial.c
> > @@ -54,11 +54,9 @@
> >
> > struct omap_uart_state {
> > int num;
> > - int can_sleep;
> >
> > struct list_head node;
> > struct omap_hwmod *oh;
> > - struct platform_device *pdev;
> > };
> >
> > static LIST_HEAD(uart_list);
> > @@ -381,8 +379,6 @@ void __init omap_serial_init_port(struct omap_board_data *bd
> >
> > oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
> >
> > - uart->pdev = pdev;
> > -
> > oh->dev_attr = uart;
> >
> > if (((cpu_is_omap34xx() || cpu_is_omap44xx()) && bdata->pads)
>
> Acked-by: Govindraj.R <govindraj.raja@ti.com>
>
> Please post out this part as a patch,
> I think this change has to go through linux-omap tree.
I'll do that, thanks.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Grazvydas Ignotas @ 2012-02-03 11:42 UTC (permalink / raw)
To: NeilBrown
Cc: Paul Walmsley, Greg KH, greg, khilman, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <20120203205401.5ddf241d@notabene.brown>
On Fri, Feb 3, 2012 at 11:54 AM, NeilBrown <neilb@suse.de> wrote:
> On Thu, 2 Feb 2012 22:45:53 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
>> On Fri, 3 Feb 2012, NeilBrown wrote:
>>
>> > then CPUIDLE enters lower states and I think it uses less power but I
>> > sometimes lose the first char I type (that is known) but also I sometimes get
>> > corruption on output.
>>
>> I don't see any output corruption on 35xx BeagleBoard, either with or
>> without these patches. So this is presumably a 37xx-centric problem, and
>> unrelated to these patches, I would guess.
>
> Maybe it is 37xx specific. I think this is a DM3730.
Not sure if it's the same problem but with 3530 on 3.2 with
sleep_timeout set, I usually get first char dropped (as expected) but
sometimes I get corrupted char instead too. Corrupt char seems to
almost always happen if I set cpufreq to powersave, on performace it's
almost always ok, so maybe it's some timing problem,
--
Gražvydas
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/7 v2] dmaengine: add a simple dma library
From: Guennadi Liakhovetski @ 2012-02-03 10:21 UTC (permalink / raw)
To: Shimoda, Yoshihiro
Cc: linux-kernel, linux-sh, Vinod Koul, Magnus Damm, linux-mmc,
alsa-devel, linux-serial, Paul Mundt
In-Reply-To: <4F2B9F34.60308@renesas.com>
On Fri, 3 Feb 2012, Shimoda, Yoshihiro wrote:
> Hi Guennadi-san,
>
> 2012/02/03 7:19, Guennadi Liakhovetski wrote:
> [ snip ]
> >>
> >> Thank you for your comment.
> >>
> >> I investigaed the issue. I found out the renesas_usbhs driver may
> >> call tx_submit() in the callback() of the dma-simple finally.
> >
> > Sorry, in my first reply to this your email I misread the renesas_usbhs
> > for an issue with your new SUDMAC driver. Since this is not the case and
> > my patch seems to be causing a regression, I'll look at it.
>
> Thank you for the comment.
>
> I found out that if the "power_up" is 0 in the sample_tx_submit(),
> the pm_state will be changed to "PENDING".
> And then, even if the renesas_usbhs calls dma_async_issue_pending(),
> the dma-simple driver doesn't call the simple_chan_xfer_ld_queue().
> This is because the driver will call the simple_chan_xfer_ld_queue()
> when the pm_state is "ESTABLISHED" only.
>
> So I guess that we have to modify the simple_issue_pending().
Ok, I looked at this code, there's now a comment in simple_tx_submit():
if (power_up) {
...
} else {
/*
* Tell .device_issue_pending() not to run the queue, interrupts
* will do it anyway
*/
schan->pm_state = DMA_SIMPLE_PM_PENDING;
}
And that's exactly what should be happening: an interrupt should trigger
the IRQ thread should call simple_chan_xfer_ld_queue(), so, looks like the
interrupt is not coming? It's also strange, why and how it worked without
dma-simple? I don't think I changed that with the transition...
Investigating.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: NeilBrown @ 2012-02-03 9:54 UTC (permalink / raw)
To: Paul Walmsley
Cc: Greg KH, greg, khilman, govindraj.raja, tomi.valkeinen,
linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1202022137150.586@utopia.booyaka.com>
[-- Attachment #1: Type: text/plain, Size: 10517 bytes --]
On Thu, 2 Feb 2012 22:45:53 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
> Hello Neil.
>
> On Fri, 3 Feb 2012, NeilBrown wrote:
>
> > Can I comment??... They are good but ....
> >
> > I've tried two approaches to getting serial behaviour that I am happy with.
> > They are with and without runtime pm.
> >
> > If I enable runtime pm by setting power/autosuspend_delay_ms (e.g. to 5000)
>
> Runtime PM should be enabled even with power/autosuspend_delay_ms = 0.
> I think you are simply enabling the autosuspend timer. There was a
> significant behavior change here from 3.2, I believe.
However the default autosuspend_delay_ms is "-1", not "0", and "-1" does
disable runbtime_pm completely. It increments usage_count (see
update_autosuspend()) so runtime_pm can never suspend the device.
>
> > then CPUIDLE enters lower states and I think it uses less power but I
> > sometimes lose the first char I type (that is known) but also I sometimes get
> > corruption on output.
>
> I don't see any output corruption on 35xx BeagleBoard, either with or
> without these patches. So this is presumably a 37xx-centric problem, and
> unrelated to these patches, I would guess.
Maybe it is 37xx specific. I think this is a DM3730.
>
> > The problem seems to be that we turn off the clocks when the kernel's ring
> > buffer is empty rather than waiting for the UART's fifo to be empty.
>
> pm_runtime_put*() calls will write to the CM_*CLKEN registers if the
> usecount goes to zero. But the PRCM should not actually turn off the
> clocks if the UART isn't idle. So I would suggest that you first try some
> hwmod CLOCKACTIVITY changes to fix this (described briefly below).
Hmm... I thought it was the other way around - CLKEN could gate the clock
off, and PRCM could also turn it off after a handshake. But I finally found
the relevant text:
The software effect is immediate and direct. The functional clock is
turned on as soon as the bit is set, and turned off if the bit is cleared
and the clock is not required by any module.
so it seems I was wrong.
Still - something is definitely going wrong because I definitely an regularly
see garbage characters. And the patch fixes it. So some runtime-suspend
handler must be doing something bad, and it must happen while characters
are being written.
>
> > I can remove this effect with:
> >
> > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> > index f809041..c7ef760 100644
> > --- a/drivers/tty/serial/omap-serial.c
> > +++ b/drivers/tty/serial/omap-serial.c
> > @@ -440,7 +440,8 @@ static unsigned int serial_omap_tx_empty(struct uart_port *port)
> > spin_lock_irqsave(&up->port.lock, flags);
> > ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
> > spin_unlock_irqrestore(&up->port.lock, flags);
> > - pm_runtime_put(&up->pdev->dev);
> > + pm_runtime_mark_last_busy(&up->pdev->dev);
> > + pm_runtime_put_autosuspend(&up->pdev->dev);
> > return ret;
> > }
>
> Well this change probably makes sense anyway, just to keep the autosuspend
> behavior consistent when an autosuspend timeout is set. But the effect of
> this patch may be a little different than what you think; see below.
>
> > i.e. when the tx buffer is empty, so turn the clocks off immediately,
> > but wait a while for the fifo to empty. Hopefully the auto-suspend time
> > is enough.
>
> Hmm, this statement is a little unclear to me. The clocks won't be turned
> off immediately - the request to disable the clocks only happens when the
> autosuspend timer expires. And even then, as mentioned above, it's just a
> request. The hardware should not actually disable the functional clock
> until the UART FIFO is drained...
If you pm_runtime_put_autosuspend(), the suspend won't happen immediately but
will wait the timeout.
If you pm_runtime_put_sync(), the suspend happens straight away.
If you pm_runtime_put(), the suspend is schedule immediately in another
thread, so it happens very soon. It doesn't wait for a timer to expire (no
timer is ticking at this point).
Just because an autosuspend timeout was set earlier, that won't cause
pm_runtime_put() to delay in suspending the device when it is called.
So I think it does request that the clocks be turned off immediately.
>
> Anyway, consider trying a different CLOCKACTIVITY setting for the UARTs.
My TRM saids that SYSC for the UARTs doesn't have CLOCKACTIVITY. Only
AUTOIDLE SOFTRESET ENAWAKEUP IDLEMODE
Is it worth trying anyway?
> There are fields and flags for this in the hwmod code; see for example the
> I2C SYSCONFIG settings in mach-omap2/omap_hwmod_3xxx_data.c. It's
> possible that the UART is currently assuming that its functional clock
> will stay on when it idle-acks. That might cause the corruption that you
> are seeing.
>
> > But I decided not to pursue this further as turning off the clocks seems
> > like the wrong thing to be doing.
>
> The clocks should be getting disabled when the autosuspend timer is 0
> also. It's just that the UART driver will request to disable the clocks
> immediately, rather than waiting.
But not when it is -1.
>
> > The OMAP UARTs have auto-suspend/auto-wake so I would rather depend on
> > that. And turning off the clocks loses that first character at 115200
> > and above (not below).
>
> If you make a change that causes the kernel to keep the UART clocks on,
> the enclosing clockdomain and any associated clockdomains (probably
> CORE_L3, CORE_L4 & PER) will be prevented from going to sleep. So just be
> aware that the strategy you are considering will incur an energy
> consumption cost. The lowest C-state you should manage to reach should be
> C4, at least in mainline terms.
>
> Incidentally, it's unclear to me how you are keeping the clocks on? Are
> you disabling runtime PM for the driver in some way?
Yes, leaving autosuspend_delay_ms at the default of -1 .... :-)
>
> > So I explored leaving the runtime_pm setting as it was. This set a maximum
> > latency of 4444 usec (which should be 5555 as there are 10 bits per char)
> > so it didn't get very low down the CPUIDLE levels.
> >
> > So I disabled the latency setting with hardware handshaking is used (as you
> > suggested once):
>
> [snip]
>
> > and now CPUIDLE uses lower states (especially if I enable_off_mode).
>
> That's good. One important note is that the CPUIdle statistics don't keep
> track of what C-state was actually entered -- it simply tracks what
> C-state that CPUIdle intended to enter. So you'll want to check
> /debug/pm_debug/count to be sure. Also, the PM debug counts are
> approximate.
Yes, I'd begun to realise that - thanks.
>
> Incidentally, I have some patches to fix the latency calculation here that
> are in the works, similar to the ones you describe. The current
> calculation in the driver is pretty broken, but since the changes to fix
> the calculation are rather involved, Kevin and I thought it would be best
> to defer most of them to the v3.4 merge window. The calculation fix in
> the 3.3-rc series is simply intended to deal with a very basic power
> management regression, as you know - not to make it ideal, which is more
> complicated. Anyway, the patches are at git://git.pwsan.com/linux-2.6 in
> the branch 'omap_serial_fix_pm_qos_formula_devel_3.4'. Keep in mind that
> at least one patch in that branch is broken, but perhaps you might get an
> idea of where they are trying to go. Comments welcome.
>
> > However I am using a lot more power than in 3.2.
>
> Is this without disabling the UART clocks? If the driver is keeping the
> UART clocks enabled, then increased power consumption is definitely
> expected.
Both. With clocks kept on (autosuspend == -1) I'm using about 30 mA more than
before. With clocks allowed to go off it is closer to 40mA more !!! Weird,
isn't it?
>
> > That probably isn't all UART-related, but there is one interesting
> > observation.
> >
> > If I watch /sys/kernel/debug/pm_debug/time and see where the time is spent
> > over a 30second period when the systems is mostly idle:
> >
> > With runtime_pm disabled for UARTs, both PER and CORE are permanently ON,
> > and MPU is OFF nearly all the time. (This is with off_mode enabled).
>
> How are you disabling runtime PM? Is this just with the autosuspend
> timeout set to 0?
-1.
>
> > If I then enabled runtime_pm with a 5 second autosuspend delay:
> > CORE is still permanently ON (I think the USB might be doing that).
> > PER is OFF (7 seconds) RET (15 seconds) and ON (8 seconds).
> > but more surprising
> > MPU is OFF (8 sec) RET (8 sec) INA (9 sec) ON (4 secs).
> >
> > So we get to turn PER off at the cost of turning the MPU on a lot.
> > (the periods in each state are only repeatable to a precision of 20%-50%).
> >
> > I understand that PER won't go OFF without runtime PM, but I would expect
> > it to at least go to INA as the UART is in smart-idle mode
>
> You are using UART3? That is in PER and its functional clock comes via
> the PER clockdomain. If the UART functional clock is prevented from being
> turned off, how can PER go inactive when the UART3 functional clock is
> still enabled?
I was thinking the auto-suspend would kick in - but I had it backwards it
seems. I'll have to rethink things.
(yes, UART3).
>
> > The net result is that with runtime_pm enabled I'm seeing an extra 7mA (at
> > 4V). That might not be very precise, but it is in the opposite direction to
> > my expectations.
> >
> > So there is something odd here... ideas?
> >
> > NeilBrown
> >
> > p.s. who should I formally submit OMAP-UART patches to? I have a couple of
> > others such as the below that I should submit somewhere.
>
> Would suggest sending them to linux-serial@vger.kernel.org,
> linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, and Alan
> Cox since he is the serial maintainer? And you should probably also cc
> me, since I seem to be stuck with fixing this driver so I can test my
> other patches; and cc Govindraj as well.
>
>
> - Paul
Thanks for your time.
I'm going to have to come up with a better theory for why my patch works.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: [PATCH 1/7 v2] dmaengine: add a simple dma library
From: Shimoda, Yoshihiro @ 2012-02-03 8:47 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-kernel, linux-sh, Vinod Koul, Magnus Damm, linux-mmc,
alsa-devel, linux-serial, Paul Mundt
In-Reply-To: <Pine.LNX.4.64.1202022317560.13860@axis700.grange>
Hi Guennadi-san,
2012/02/03 7:19, Guennadi Liakhovetski wrote:
[ snip ]
>>
>> Thank you for your comment.
>>
>> I investigaed the issue. I found out the renesas_usbhs driver may
>> call tx_submit() in the callback() of the dma-simple finally.
>
> Sorry, in my first reply to this your email I misread the renesas_usbhs
> for an issue with your new SUDMAC driver. Since this is not the case and
> my patch seems to be causing a regression, I'll look at it.
Thank you for the comment.
I found out that if the "power_up" is 0 in the sample_tx_submit(),
the pm_state will be changed to "PENDING".
And then, even if the renesas_usbhs calls dma_async_issue_pending(),
the dma-simple driver doesn't call the simple_chan_xfer_ld_queue().
This is because the driver will call the simple_chan_xfer_ld_queue()
when the pm_state is "ESTABLISHED" only.
So I guess that we have to modify the simple_issue_pending().
Best regards,
Yoshihiro Shimoda
> Thanks
> Guennadi
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
--
Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Govindraj @ 2012-02-03 6:56 UTC (permalink / raw)
To: NeilBrown
Cc: Paul Walmsley, Greg KH, greg, khilman, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <20120203150708.386951d2@notabene.brown>
On Fri, Feb 3, 2012 at 9:37 AM, NeilBrown <neilb@suse.de> wrote:
> On Thu, 2 Feb 2012 13:03:01 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
>
>> Hi Greg,
>>
>> On Thu, 26 Jan 2012, Paul Walmsley wrote:
>>
>> > On Thu, 26 Jan 2012, Greg KH wrote:
>> >
>> > > Ok, I've just reverted both of these patches for now, please send new
>> > > ones when you have them.
>> >
>> > Thanks. A pull request is at the bottom of this message. The patches
>> > are also available from the mailing list archives here:
>> >
>> > http://marc.info/?l=linux-arm-kernel&m=132754676014389&w=2
>> > http://marc.info/?l=linux-arm-kernel&m=132754677914395&w=2
>> > http://marc.info/?l=linux-arm-kernel&m=132754676014388&w=2
>>
>> Any comments on these?
>
> Can I comment??... They are good but ....
>
> I've tried two approaches to getting serial behaviour that I am happy with.
> They are with and without runtime pm.
>
> If I enable runtime pm by setting power/autosuspend_delay_ms (e.g. to 5000)
> then CPUIDLE enters lower states and I think it uses less power but I
> sometimes lose the first char I type (that is known) but also I sometimes get
> corruption on output.
>
> The problem seems to be that we turn off the clocks when the kernel's ring
> buffer is empty rather than waiting for the UART's fifo to be empty.
> So I get corruption near the end of a stream of output ... not right at the
> end so something must be turning the clocks back on somehow.
>
> I can remove this effect with:
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index f809041..c7ef760 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -440,7 +440,8 @@ static unsigned int serial_omap_tx_empty(struct uart_port *port)
> spin_lock_irqsave(&up->port.lock, flags);
> ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
> spin_unlock_irqrestore(&up->port.lock, flags);
> - pm_runtime_put(&up->pdev->dev);
> + pm_runtime_mark_last_busy(&up->pdev->dev);
> + pm_runtime_put_autosuspend(&up->pdev->dev);
> return ret;
> }
>
But looking into it UART_LSR_TEMT("include/linux/serial_reg.h") => 0x40
from omap trm:
TX_SR_E => bit 6
"Read 0x1: Transmitter hold (TX FIFO) and shift registers are empty."
So it means all data from tx fifo has shifted out and no pending data in
tx fifo.
But we had used UART_LSR_THRE (0x20) here for tx fifo emptiness comparison
then from omap trm
TX_FIFO_E => bit 5
"Read 0x1: Transmit hold register (TX FIFO) is empty.
The transmission is not necessarily complete"
So I think all data has been shifted out from tx fifo for
serial_omap_tx_empty check.
>
> i.e. when the tx buffer is empty, so turn the clocks off immediately, but
> wait a while for the fifo to empty. Hopefully the auto-suspend time is
> enough.
>
[...]
>
> p.s. who should I formally submit OMAP-UART patches to? I have a couple of
> others such as the below that I should submit somewhere.
>
>
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 247d894..35a649f 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -54,11 +54,9 @@
>
> struct omap_uart_state {
> int num;
> - int can_sleep;
>
> struct list_head node;
> struct omap_hwmod *oh;
> - struct platform_device *pdev;
> };
>
> static LIST_HEAD(uart_list);
> @@ -381,8 +379,6 @@ void __init omap_serial_init_port(struct omap_board_data *bd
>
> oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
>
> - uart->pdev = pdev;
> -
> oh->dev_attr = uart;
>
> if (((cpu_is_omap34xx() || cpu_is_omap44xx()) && bdata->pads)
Acked-by: Govindraj.R <govindraj.raja@ti.com>
Please post out this part as a patch,
I think this change has to go through linux-omap tree.
--
Thanks,
Govindraj.R
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-03 5:45 UTC (permalink / raw)
To: NeilBrown
Cc: Greg KH, greg, khilman, govindraj.raja, tomi.valkeinen,
linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <20120203150708.386951d2@notabene.brown>
Hello Neil.
On Fri, 3 Feb 2012, NeilBrown wrote:
> Can I comment??... They are good but ....
>
> I've tried two approaches to getting serial behaviour that I am happy with.
> They are with and without runtime pm.
>
> If I enable runtime pm by setting power/autosuspend_delay_ms (e.g. to 5000)
Runtime PM should be enabled even with power/autosuspend_delay_ms = 0.
I think you are simply enabling the autosuspend timer. There was a
significant behavior change here from 3.2, I believe.
> then CPUIDLE enters lower states and I think it uses less power but I
> sometimes lose the first char I type (that is known) but also I sometimes get
> corruption on output.
I don't see any output corruption on 35xx BeagleBoard, either with or
without these patches. So this is presumably a 37xx-centric problem, and
unrelated to these patches, I would guess.
> The problem seems to be that we turn off the clocks when the kernel's ring
> buffer is empty rather than waiting for the UART's fifo to be empty.
pm_runtime_put*() calls will write to the CM_*CLKEN registers if the
usecount goes to zero. But the PRCM should not actually turn off the
clocks if the UART isn't idle. So I would suggest that you first try some
hwmod CLOCKACTIVITY changes to fix this (described briefly below).
> I can remove this effect with:
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index f809041..c7ef760 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -440,7 +440,8 @@ static unsigned int serial_omap_tx_empty(struct uart_port *port)
> spin_lock_irqsave(&up->port.lock, flags);
> ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
> spin_unlock_irqrestore(&up->port.lock, flags);
> - pm_runtime_put(&up->pdev->dev);
> + pm_runtime_mark_last_busy(&up->pdev->dev);
> + pm_runtime_put_autosuspend(&up->pdev->dev);
> return ret;
> }
Well this change probably makes sense anyway, just to keep the autosuspend
behavior consistent when an autosuspend timeout is set. But the effect of
this patch may be a little different than what you think; see below.
> i.e. when the tx buffer is empty, so turn the clocks off immediately,
> but wait a while for the fifo to empty. Hopefully the auto-suspend time
> is enough.
Hmm, this statement is a little unclear to me. The clocks won't be turned
off immediately - the request to disable the clocks only happens when the
autosuspend timer expires. And even then, as mentioned above, it's just a
request. The hardware should not actually disable the functional clock
until the UART FIFO is drained...
Anyway, consider trying a different CLOCKACTIVITY setting for the UARTs.
There are fields and flags for this in the hwmod code; see for example the
I2C SYSCONFIG settings in mach-omap2/omap_hwmod_3xxx_data.c. It's
possible that the UART is currently assuming that its functional clock
will stay on when it idle-acks. That might cause the corruption that you
are seeing.
> But I decided not to pursue this further as turning off the clocks seems
> like the wrong thing to be doing.
The clocks should be getting disabled when the autosuspend timer is 0
also. It's just that the UART driver will request to disable the clocks
immediately, rather than waiting.
> The OMAP UARTs have auto-suspend/auto-wake so I would rather depend on
> that. And turning off the clocks loses that first character at 115200
> and above (not below).
If you make a change that causes the kernel to keep the UART clocks on,
the enclosing clockdomain and any associated clockdomains (probably
CORE_L3, CORE_L4 & PER) will be prevented from going to sleep. So just be
aware that the strategy you are considering will incur an energy
consumption cost. The lowest C-state you should manage to reach should be
C4, at least in mainline terms.
Incidentally, it's unclear to me how you are keeping the clocks on? Are
you disabling runtime PM for the driver in some way?
> So I explored leaving the runtime_pm setting as it was. This set a maximum
> latency of 4444 usec (which should be 5555 as there are 10 bits per char)
> so it didn't get very low down the CPUIDLE levels.
>
> So I disabled the latency setting with hardware handshaking is used (as you
> suggested once):
[snip]
> and now CPUIDLE uses lower states (especially if I enable_off_mode).
That's good. One important note is that the CPUIdle statistics don't keep
track of what C-state was actually entered -- it simply tracks what
C-state that CPUIdle intended to enter. So you'll want to check
/debug/pm_debug/count to be sure. Also, the PM debug counts are
approximate.
Incidentally, I have some patches to fix the latency calculation here that
are in the works, similar to the ones you describe. The current
calculation in the driver is pretty broken, but since the changes to fix
the calculation are rather involved, Kevin and I thought it would be best
to defer most of them to the v3.4 merge window. The calculation fix in
the 3.3-rc series is simply intended to deal with a very basic power
management regression, as you know - not to make it ideal, which is more
complicated. Anyway, the patches are at git://git.pwsan.com/linux-2.6 in
the branch 'omap_serial_fix_pm_qos_formula_devel_3.4'. Keep in mind that
at least one patch in that branch is broken, but perhaps you might get an
idea of where they are trying to go. Comments welcome.
> However I am using a lot more power than in 3.2.
Is this without disabling the UART clocks? If the driver is keeping the
UART clocks enabled, then increased power consumption is definitely
expected.
> That probably isn't all UART-related, but there is one interesting
> observation.
>
> If I watch /sys/kernel/debug/pm_debug/time and see where the time is spent
> over a 30second period when the systems is mostly idle:
>
> With runtime_pm disabled for UARTs, both PER and CORE are permanently ON,
> and MPU is OFF nearly all the time. (This is with off_mode enabled).
How are you disabling runtime PM? Is this just with the autosuspend
timeout set to 0?
> If I then enabled runtime_pm with a 5 second autosuspend delay:
> CORE is still permanently ON (I think the USB might be doing that).
> PER is OFF (7 seconds) RET (15 seconds) and ON (8 seconds).
> but more surprising
> MPU is OFF (8 sec) RET (8 sec) INA (9 sec) ON (4 secs).
>
> So we get to turn PER off at the cost of turning the MPU on a lot.
> (the periods in each state are only repeatable to a precision of 20%-50%).
>
> I understand that PER won't go OFF without runtime PM, but I would expect
> it to at least go to INA as the UART is in smart-idle mode
You are using UART3? That is in PER and its functional clock comes via
the PER clockdomain. If the UART functional clock is prevented from being
turned off, how can PER go inactive when the UART3 functional clock is
still enabled?
> The net result is that with runtime_pm enabled I'm seeing an extra 7mA (at
> 4V). That might not be very precise, but it is in the opposite direction to
> my expectations.
>
> So there is something odd here... ideas?
>
> NeilBrown
>
> p.s. who should I formally submit OMAP-UART patches to? I have a couple of
> others such as the below that I should submit somewhere.
Would suggest sending them to linux-serial@vger.kernel.org,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, and Alan
Cox since he is the serial maintainer? And you should probably also cc
me, since I seem to be stuck with fixing this driver so I can test my
other patches; and cc Govindraj as well.
- Paul
^ permalink raw reply
* [Patch] tty: fix a build failure on sparc
From: Cong Wang @ 2012-02-03 5:22 UTC (permalink / raw)
To: linux-kernel
Cc: Andrew Morton, Paul Gortmaker, Greg Kroah-Hartman, WANG Cong,
Alan Cox, Jamie Iles, Stephen Warren, linux-serial
On sparc, there is a build failure:
drivers/tty/serial/8250/8250.c:48:21: error: suncore.h: No such file or directory
drivers/tty/serial/8250/8250.c:3275: error: implicit declaration of function 'sunserial_register_minors'
drivers/tty/serial/8250/8250.c:3305: error: implicit declaration of function 'sunserial_unregister_minors'
this is due to commit 9bef3d4197379a995fa80f81950bbbf8d32e9e8b
(serial: group all the 8250 related code together) moved these files
into 8250/ subdirectory, but forgot to change the reference
to drivers/tty/serial/suncore.h.
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
---
diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index 9f50c4e..9b7336f 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -45,7 +45,7 @@
#include "8250.h"
#ifdef CONFIG_SPARC
-#include "suncore.h"
+#include "../suncore.h"
#endif
/*
^ permalink raw reply related
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: NeilBrown @ 2012-02-03 4:07 UTC (permalink / raw)
To: Paul Walmsley
Cc: Greg KH, greg, khilman, govindraj.raja, tomi.valkeinen,
linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1202021302070.586@utopia.booyaka.com>
[-- Attachment #1: Type: text/plain, Size: 6400 bytes --]
On Thu, 2 Feb 2012 13:03:01 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
> Hi Greg,
>
> On Thu, 26 Jan 2012, Paul Walmsley wrote:
>
> > On Thu, 26 Jan 2012, Greg KH wrote:
> >
> > > Ok, I've just reverted both of these patches for now, please send new
> > > ones when you have them.
> >
> > Thanks. A pull request is at the bottom of this message. The patches
> > are also available from the mailing list archives here:
> >
> > http://marc.info/?l=linux-arm-kernel&m=132754676014389&w=2
> > http://marc.info/?l=linux-arm-kernel&m=132754677914395&w=2
> > http://marc.info/?l=linux-arm-kernel&m=132754676014388&w=2
>
> Any comments on these?
Can I comment??... They are good but ....
I've tried two approaches to getting serial behaviour that I am happy with.
They are with and without runtime pm.
If I enable runtime pm by setting power/autosuspend_delay_ms (e.g. to 5000)
then CPUIDLE enters lower states and I think it uses less power but I
sometimes lose the first char I type (that is known) but also I sometimes get
corruption on output.
The problem seems to be that we turn off the clocks when the kernel's ring
buffer is empty rather than waiting for the UART's fifo to be empty.
So I get corruption near the end of a stream of output ... not right at the
end so something must be turning the clocks back on somehow.
I can remove this effect with:
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index f809041..c7ef760 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -440,7 +440,8 @@ static unsigned int serial_omap_tx_empty(struct uart_port *port)
spin_lock_irqsave(&up->port.lock, flags);
ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
spin_unlock_irqrestore(&up->port.lock, flags);
- pm_runtime_put(&up->pdev->dev);
+ pm_runtime_mark_last_busy(&up->pdev->dev);
+ pm_runtime_put_autosuspend(&up->pdev->dev);
return ret;
}
i.e. when the tx buffer is empty, so turn the clocks off immediately, but
wait a while for the fifo to empty. Hopefully the auto-suspend time is
enough.
But I decided not to pursue this further as turning off the clocks seems like
the wrong thing to be doing. The OMAP UARTs have auto-suspend/auto-wake so
I would rather depend on that. And turning off the clocks loses that first
character at 115200 and above (not below).
So I explored leaving the runtime_pm setting as it was. This set a maximum
latency of 4444 usec (which should be 5555 as there are 10 bits per char)
so it didn't get very low down the CPUIDLE levels.
So I disabled the latency setting with hardware handshaking is used (as you
suggested once):
@@ -707,28 +708,37 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
unsigned char cval = 0;
unsigned char efr = 0;
unsigned long flags = 0;
- unsigned int baud, quot;
+ unsigned int baud, quot, bits;
switch (termios->c_cflag & CSIZE) {
case CS5:
cval = UART_LCR_WLEN5;
+ bits = 5;
break;
case CS6:
cval = UART_LCR_WLEN6;
+ bits = 6;
break;
case CS7:
cval = UART_LCR_WLEN7;
+ bits = 7;
break;
default:
case CS8:
cval = UART_LCR_WLEN8;
+ bits = 8;
break;
}
- if (termios->c_cflag & CSTOPB)
+ bits += 2; /* start bit plus stop bit */
+ if (termios->c_cflag & CSTOPB) {
cval |= UART_LCR_STOP;
- if (termios->c_cflag & PARENB)
+ bits++;
+ }
+ if (termios->c_cflag & PARENB) {
cval |= UART_LCR_PARITY;
+ bits++;
+ }
if (!(termios->c_cflag & PARODD))
cval |= UART_LCR_EPAR;
@@ -740,8 +750,12 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
quot = serial_omap_get_divisor(port, baud);
/* calculate wakeup latency constraint */
- up->calc_latency = (USEC_PER_SEC * up->port.fifosize) / (baud / 8);
+ if (termios->c_cflag & CRTSCTS)
+ up->calc_latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
+ else
+ up->calc_latency = (USEC_PER_SEC * up->port.fifosize) / (baud / bits);
up->latency = up->calc_latency;
+ printk("Calc latency: %lld\n", (unsigned long long) up->calc_latency);
schedule_work(&up->qos_work);
up->dll = quot & 0xff;
and now CPUIDLE uses lower states (especially if I enable_off_mode).
However I am using a lot more power than in 3.2. That probably isn't all
UART-related, but there is one interesting observation.
If I watch /sys/kernel/debug/pm_debug/time and see where the time is spent
over a 30second period when the systems is mostly idle:
With runtime_pm disabled for UARTs, both PER and CORE are permanently ON,
and MPU is OFF nearly all the time. (This is with off_mode enabled).
If I then enabled runtime_pm with a 5 second autosuspend delay:
CORE is still permanently ON (I think the USB might be doing that).
PER is OFF (7 seconds) RET (15 seconds) and ON (8 seconds).
but more surprising
MPU is OFF (8 sec) RET (8 sec) INA (9 sec) ON (4 secs).
So we get to turn PER off at the cost of turning the MPU on a lot.
(the periods in each state are only repeatable to a precision of 20%-50%).
I understand that PER won't go OFF without runtime PM, but I would expect
it to at least go to INA as the UART is in smart-idle mode
The net result is that with runtime_pm enabled I'm seeing an extra 7mA (at
4V). That might not be very precise, but it is in the opposite direction to
my expectations.
So there is something odd here... ideas?
NeilBrown
p.s. who should I formally submit OMAP-UART patches to? I have a couple of
others such as the below that I should submit somewhere.
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 247d894..35a649f 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -54,11 +54,9 @@
struct omap_uart_state {
int num;
- int can_sleep;
struct list_head node;
struct omap_hwmod *oh;
- struct platform_device *pdev;
};
static LIST_HEAD(uart_list);
@@ -381,8 +379,6 @@ void __init omap_serial_init_port(struct omap_board_data *bd
oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
- uart->pdev = pdev;
-
oh->dev_attr = uart;
if (((cpu_is_omap34xx() || cpu_is_omap44xx()) && bdata->pads)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply related
* Re: [PATCH 1/7 v2] dmaengine: add a simple dma library
From: Guennadi Liakhovetski @ 2012-02-02 22:19 UTC (permalink / raw)
To: Shimoda, Yoshihiro
Cc: linux-kernel, linux-sh, Vinod Koul, Magnus Damm, linux-mmc,
alsa-devel, linux-serial, Paul Mundt
In-Reply-To: <4F27CA7D.601@renesas.com>
Shimoda-san
On Tue, 31 Jan 2012, Shimoda, Yoshihiro wrote:
> Hi Guennadi-san,
>
> 2012/01/27 17:48, Guennadi Liakhovetski wrote:
> > Hi Shimoda-san
> >
> > On Fri, 27 Jan 2012, Shimoda, Yoshihiro wrote:
> [ snip ]
> >>>
> >>> 1. switch from using a tasklet to threaded IRQ, which allowed to
> >>
> >> I tested this driver on 2 environment, but it could not work correctly.
> >> - renesas_usbhs driver with shdma driver on the mackerel board
> >> - renesas_usbhs driver with experimental SUDMAC driver
> >>
> >> In this case, should we modify the renesas_usbhs driver?
> >
> > Yes, you do. Please, see the respective version of the shdma patch. It
> > doesn't request channel IRQs any more directly, instead, it uses a
> > dma-simple wrapper function. Also operations change a bit.
>
> Thank you for your comment.
>
> I investigaed the issue. I found out the renesas_usbhs driver may
> call tx_submit() in the callback() of the dma-simple finally.
Sorry, in my first reply to this your email I misread the renesas_usbhs
for an issue with your new SUDMAC driver. Since this is not the case and
my patch seems to be causing a regression, I'll look at it.
Thanks
Guennadi
> In this case, the value of power_up in the simple_tx_submit is 0,
> the start_xfer() is not called. And then, even if the ld_queue is
> not empty, the DMA doesn't start.
>
>
> So, if I add codes like the following in the dma-simple.c,
> the renesas_usbhs driver can work correctly without the driver changes.
> If you think the code is good, would you merge it to your code?
> ======= chan_irqt() =======
> simple_chan_ld_cleanup(schan, false);
>
> + spin_lock_irq(&schan->chan_lock);
> + /* Next desc */
> + simple_chan_xfer_ld_queue(schan);
> + spin_unlock_irq(&schan->chan_lock);
>
> return IRQ_HANDLED;
> ==============
>
> Best regards,
> Yoshihiro Shimoda
>
> > Thanks
> > Guennadi
> >
> >> For reference, if I changed the threaded IRQ to tasklet,
> >> the 2 environment worked correctly:
> >> ==============
> >> diff --git a/drivers/dma/dma-simple.c b/drivers/dma/dma-simple.c
> >> index 49d8f7d..65a5170 100644
> >> --- a/drivers/dma/dma-simple.c
> >> +++ b/drivers/dma/dma-simple.c
> >> @@ -715,16 +715,18 @@ static irqreturn_t chan_irq(int irq, void *dev)
> >>
> >> spin_lock(&schan->chan_lock);
> >>
> >> - ret = ops->chan_irq(schan, irq) ? IRQ_WAKE_THREAD : IRQ_NONE;
> >> + ret = ops->chan_irq(schan, irq) ? IRQ_HANDLED : IRQ_NONE;
> >> + if (ret == IRQ_HANDLED)
> >> + tasklet_schedule(&schan->tasklet);
> >>
> >> spin_unlock(&schan->chan_lock);
> >>
> >> return ret;
> >> }
> >>
> >> -static irqreturn_t chan_irqt(int irq, void *dev)
> >> +static void chan_irqt(unsigned long dev)
> >> {
> >> - struct dma_simple_chan *schan = dev;
> >> + struct dma_simple_chan *schan = (struct dma_simple_chan *)dev;
> >> const struct dma_simple_ops *ops =
> >> to_simple_dev(schan->dma_chan.device)->ops;
> >> struct dma_simple_desc *sdesc;
> >> @@ -744,15 +746,13 @@ static irqreturn_t chan_irqt(int irq, void *dev)
> >> spin_unlock_irq(&schan->chan_lock);
> >>
> >> simple_chan_ld_cleanup(schan, false);
> >> -
> >> - return IRQ_HANDLED;
> >> }
> >>
> >> int dma_simple_request_irq(struct dma_simple_chan *schan, int irq,
> >> unsigned long flags, const char *name)
> >> {
> >> - int ret = request_threaded_irq(irq, chan_irq, chan_irqt,
> >> - flags, name, schan);
> >> + int ret = request_irq(irq, chan_irq, flags, name, schan);
> >> + tasklet_init(&schan->tasklet, chan_irqt, (unsigned long)schan);
> >>
> >> schan->irq = ret < 0 ? ret : irq;
> >>
> >> diff --git a/include/linux/dma-simple.h b/include/linux/dma-simple.h
> >> index 5336674..beb1489 100644
> >> --- a/include/linux/dma-simple.h
> >> +++ b/include/linux/dma-simple.h
> >> @@ -68,6 +68,7 @@ struct dma_simple_chan {
> >> int id; /* Raw id of this channel */
> >> int irq; /* Channel IRQ */
> >> enum dma_simple_pm_state pm_state;
> >> + struct tasklet_struct tasklet;
> >> };
> >>
> >> /**
> >> ==============
> >>
> >> Best regards,
> >> Yoshihiro Shimoda
> >>
> >
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> >
>
>
> --
> Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> EC No.
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Greg KH @ 2012-02-02 21:02 UTC (permalink / raw)
To: Paul Walmsley
Cc: khilman, govindraj.raja, tomi.valkeinen, linux-serial, linux-omap,
linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1201261225520.32667@utopia.booyaka.com>
On Thu, Jan 26, 2012 at 12:34:50PM -0700, Paul Walmsley wrote:
> On Thu, 26 Jan 2012, Greg KH wrote:
>
> > Ok, I've just reverted both of these patches for now, please send new
> > ones when you have them.
>
> Thanks. A pull request is at the bottom of this message. The patches
> are also available from the mailing list archives here:
>
> http://marc.info/?l=linux-arm-kernel&m=132754676014389&w=2
> http://marc.info/?l=linux-arm-kernel&m=132754677914395&w=2
> http://marc.info/?l=linux-arm-kernel&m=132754676014388&w=2
Now applied.
greg k-h
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Greg KH @ 2012-02-02 20:22 UTC (permalink / raw)
To: Paul Walmsley
Cc: khilman, govindraj.raja, tomi.valkeinen, linux-serial, linux-omap,
linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1202021302070.586@utopia.booyaka.com>
On Thu, Feb 02, 2012 at 01:03:01PM -0700, Paul Walmsley wrote:
> Hi Greg,
>
> On Thu, 26 Jan 2012, Paul Walmsley wrote:
>
> > On Thu, 26 Jan 2012, Greg KH wrote:
> >
> > > Ok, I've just reverted both of these patches for now, please send new
> > > ones when you have them.
> >
> > Thanks. A pull request is at the bottom of this message. The patches
> > are also available from the mailing list archives here:
> >
> > http://marc.info/?l=linux-arm-kernel&m=132754676014389&w=2
> > http://marc.info/?l=linux-arm-kernel&m=132754677914395&w=2
> > http://marc.info/?l=linux-arm-kernel&m=132754676014388&w=2
>
> Any comments on these?
They are in my to-apply queue, I'm getting to them...
greg k-h
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-02 20:03 UTC (permalink / raw)
To: Greg KH, greg
Cc: khilman, govindraj.raja, tomi.valkeinen, linux-serial, linux-omap,
linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1201261225520.32667@utopia.booyaka.com>
Hi Greg,
On Thu, 26 Jan 2012, Paul Walmsley wrote:
> On Thu, 26 Jan 2012, Greg KH wrote:
>
> > Ok, I've just reverted both of these patches for now, please send new
> > ones when you have them.
>
> Thanks. A pull request is at the bottom of this message. The patches
> are also available from the mailing list archives here:
>
> http://marc.info/?l=linux-arm-kernel&m=132754676014389&w=2
> http://marc.info/?l=linux-arm-kernel&m=132754677914395&w=2
> http://marc.info/?l=linux-arm-kernel&m=132754676014388&w=2
Any comments on these?
- Paul
> - Paul
>
> The following changes since commit dcd6c92267155e70a94b3927bce681ce74b80d1f:
>
> Linux 3.3-rc1 (2012-01-19 15:04:48 -0800)
>
> are available in the git repository at:
> git://git.pwsan.com/linux-2.6 omap_serial_fixes_3.3rc
>
> Paul Walmsley (3):
> tty: serial: OMAP: use a 1-byte RX FIFO threshold in PIO mode
> tty: serial: OMAP: block idle while the UART is transferring data in PIO mode
> tty: serial: OMAP: wakeup latency constraint is in microseconds, not milliseconds
>
> arch/arm/mach-omap2/serial.c | 8 ++++----
> drivers/tty/serial/omap-serial.c | 30 +++++++++++++++++++++++++-----
> 2 files changed, 29 insertions(+), 9 deletions(-)
>
- Paul
^ permalink raw reply
* Re: [PATCH 4/4] qcserial: don't enable autosuspend
From: Oliver Neukum @ 2012-02-02 8:40 UTC (permalink / raw)
To: Thomas Tuttle
Cc: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20120201210745.GD16802-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Am Mittwoch, 1. Februar 2012, 22:07:45 schrieb Thomas Tuttle:
> We've found that qcserial devices will not always wake up reliably
> from autosuspend, so disable it.
This might quite hurt power consumption. Can you also solve this
with the RESET_RESUME quirk?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/4] qcserial: log when an interface has too many altsettings
From: ttuttle @ 2012-02-01 22:25 UTC (permalink / raw)
To: Greg KH; +Cc: linux-serial, linux-usb
In-Reply-To: <20120201222322.GA2333@kroah.com>
On Wed, Feb 1, 2012 at 17:23, Greg KH <gregkh@linuxfoundation.org> wrote:
> No problem, I'll just pick the 2 good ones, thanks.
Thanks!
-- ttuttle
^ permalink raw reply
* Re: [PATCH 2/4] qcserial: log when an interface has too many altsettings
From: Greg KH @ 2012-02-01 22:23 UTC (permalink / raw)
To: ttuttle
Cc: linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CADyrwZQNMpxh5yTBnH68+0KK9r5NFvFMgad1K8qXouWAHY=1Rg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Feb 01, 2012 at 05:15:57PM -0500, ttuttle wrote:
> On Wed, Feb 1, 2012 at 17:02, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > As the code properly handles such an error, my point is, if a user sees
> > this new scary message, what are they supposed to do with it?
>
> The intent was that when we saw it in logs, we'd know to look straight
> at the USB configuration, but okay.
>
> > So yes, please drop it.
>
> Alright.
>
> > Are you going to resend this series based on the comments so far?
>
> I can, if it's easier for you than just selectively applying the two
> remaining ones.
No problem, I'll just pick the 2 good ones, thanks.
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/4] qcserial: log when an interface has too many altsettings
From: ttuttle @ 2012-02-01 22:15 UTC (permalink / raw)
To: Greg KH; +Cc: linux-serial, linux-usb
In-Reply-To: <20120201220232.GB893@kroah.com>
On Wed, Feb 1, 2012 at 17:02, Greg KH <gregkh@linuxfoundation.org> wrote:
> As the code properly handles such an error, my point is, if a user sees
> this new scary message, what are they supposed to do with it?
The intent was that when we saw it in logs, we'd know to look straight
at the USB configuration, but okay.
> So yes, please drop it.
Alright.
> Are you going to resend this series based on the comments so far?
I can, if it's easier for you than just selectively applying the two
remaining ones.
^ permalink raw reply
* Re: [PATCH 2/4] qcserial: log when an interface has too many altsettings
From: Greg KH @ 2012-02-01 22:02 UTC (permalink / raw)
To: ttuttle; +Cc: linux-serial, linux-usb
In-Reply-To: <CADyrwZSN6xeibZzgaPCL7qZfpwrTLipkXwQUOTOMZGpg0uRhHw@mail.gmail.com>
On Wed, Feb 01, 2012 at 04:47:22PM -0500, ttuttle wrote:
> On Wed, Feb 1, 2012 at 16:11, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> + else if (serial->interface->num_altsetting > 2) {
> >> + dev_err(&serial->dev->dev,
> >> + "too many altsettings: %u",
> >> + serial->interface->num_altsetting);
> >
> > Why would this happen? And what does spitting out an error do about it?
> > What are we supposed to do with this information?
>
> This would happen if we found a device that was returning an
> unexpected USB configuration. We've never actually seen this happen
> -- it was just there in case. I can drop it if you think it's not
> worth the trouble.
As the code properly handles such an error, my point is, if a user sees
this new scary message, what are they supposed to do with it?
So yes, please drop it.
Are you going to resend this series based on the comments so far?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/4] qcserial: log when an interface has too many altsettings
From: ttuttle @ 2012-02-01 21:47 UTC (permalink / raw)
To: Greg KH
Cc: linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20120201211116.GA30758-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Wed, Feb 1, 2012 at 16:11, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
>> + else if (serial->interface->num_altsetting > 2) {
>> + dev_err(&serial->dev->dev,
>> + "too many altsettings: %u",
>> + serial->interface->num_altsetting);
>
> Why would this happen? And what does spitting out an error do about it?
> What are we supposed to do with this information?
This would happen if we found a device that was returning an
unexpected USB configuration. We've never actually seen this happen
-- it was just there in case. I can drop it if you think it's not
worth the trouble.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/4] qcserial: log when an interface has too many altsettings
From: Greg KH @ 2012-02-01 21:11 UTC (permalink / raw)
To: Thomas Tuttle; +Cc: linux-serial, linux-usb
In-Reply-To: <20120201210723.GB16802@google.com>
On Wed, Feb 01, 2012 at 04:07:23PM -0500, Thomas Tuttle wrote:
> Signed-off-by: Thomas Tuttle <ttuttle@chromium.org>
> ---
> drivers/usb/serial/qcserial.c | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/serial/qcserial.c b/drivers/usb/serial/qcserial.c
> index 26e3e30..5a817d6 100644
> --- a/drivers/usb/serial/qcserial.c
> +++ b/drivers/usb/serial/qcserial.c
> @@ -145,8 +145,12 @@ static int qcprobe(struct usb_serial *serial, const struct usb_device_id *id)
> /* Gobi 2000 has a single altsetting, older ones have two */
> if (serial->interface->num_altsetting == 2)
> intf = &serial->interface->altsetting[1];
> - else if (serial->interface->num_altsetting > 2)
> + else if (serial->interface->num_altsetting > 2) {
> + dev_err(&serial->dev->dev,
> + "too many altsettings: %u",
> + serial->interface->num_altsetting);
Why would this happen? And what does spitting out an error do about it?
What are we supposed to do with this information?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 3/4] qcserial: remove unused "debug" parameter
From: ttuttle @ 2012-02-01 21:13 UTC (permalink / raw)
To: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
Cc: linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20120201210735.GC16802-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
...ignore this patch. debug is not unused -- the dbg macro references
it, but is defined elsewhere.
Sorry for the noise; turns out I was not actually building my modified tree. >.<
On Wed, Feb 1, 2012 at 16:07, Thomas Tuttle <ttuttle-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> Signed-off-by: Thomas Tuttle <ttuttle-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> drivers/usb/serial/qcserial.c | 5 -----
> 1 files changed, 0 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/serial/qcserial.c b/drivers/usb/serial/qcserial.c
> index 5a817d6..0f549c6 100644
> --- a/drivers/usb/serial/qcserial.c
> +++ b/drivers/usb/serial/qcserial.c
> @@ -22,8 +22,6 @@
> #define DRIVER_AUTHOR "Qualcomm Inc"
> #define DRIVER_DESC "Qualcomm USB Serial driver"
>
> -static bool debug;
> -
> static const struct usb_device_id id_table[] = {
> {USB_DEVICE(0x05c6, 0x9211)}, /* Acer Gobi QDL device */
> {USB_DEVICE(0x05c6, 0x9212)}, /* Acer Gobi Modem Device */
> @@ -298,6 +296,3 @@ module_exit(qcexit);
> MODULE_AUTHOR(DRIVER_AUTHOR);
> MODULE_DESCRIPTION(DRIVER_DESC);
> MODULE_LICENSE("GPL v2");
> -
> -module_param(debug, bool, S_IRUGO | S_IWUSR);
> -MODULE_PARM_DESC(debug, "Debug enabled or not");
> --
> 1.7.7.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 4/4] qcserial: don't enable autosuspend
From: Thomas Tuttle @ 2012-02-01 21:07 UTC (permalink / raw)
To: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
Cc: linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
We've found that qcserial devices will not always wake up reliably
from autosuspend, so disable it.
Signed-off-by: Thomas Tuttle <ttuttle-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
drivers/usb/serial/qcserial.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/qcserial.c b/drivers/usb/serial/qcserial.c
index 0f549c6..be9050f 100644
--- a/drivers/usb/serial/qcserial.c
+++ b/drivers/usb/serial/qcserial.c
@@ -135,8 +135,6 @@ static int qcprobe(struct usb_serial *serial, const struct usb_device_id *id)
spin_lock_init(&data->susp_lock);
- usb_enable_autosuspend(serial->dev);
^ permalink raw reply related
* [PATCH 3/4] qcserial: remove unused "debug" parameter
From: Thomas Tuttle @ 2012-02-01 21:07 UTC (permalink / raw)
To: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
Cc: linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
Signed-off-by: Thomas Tuttle <ttuttle-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
drivers/usb/serial/qcserial.c | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/serial/qcserial.c b/drivers/usb/serial/qcserial.c
index 5a817d6..0f549c6 100644
--- a/drivers/usb/serial/qcserial.c
+++ b/drivers/usb/serial/qcserial.c
@@ -22,8 +22,6 @@
#define DRIVER_AUTHOR "Qualcomm Inc"
#define DRIVER_DESC "Qualcomm USB Serial driver"
-static bool debug;
-
static const struct usb_device_id id_table[] = {
{USB_DEVICE(0x05c6, 0x9211)}, /* Acer Gobi QDL device */
{USB_DEVICE(0x05c6, 0x9212)}, /* Acer Gobi Modem Device */
@@ -298,6 +296,3 @@ module_exit(qcexit);
MODULE_AUTHOR(DRIVER_AUTHOR);
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL v2");
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox