Re: [PATCH 2/3] serial: 8250_omap: check how many bytes were injected
On 08/14/2015 12:01 PM, Sebastian Andrzej Siewior wrote: > The function tty_insert_flip_string() returns an int and as such it > might fail. So the result is that I kindly asked to insert 48 bytes and > the function only insterted 32. > I have no idea what to do with the remaining 16 so I think dropping them > is the only option. I also increase the buf_overrun counter so userpace > has a clue that we lost bytes. No objection to the patch but I'm curious whether this is something you've actually observed and under what circumstances. Regards, Peter Hurley > Cc: Greg Kroah-Hartman > Signed-off-by: Sebastian Andrzej Siewior > --- > drivers/tty/serial/8250/8250_omap.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_omap.c > b/drivers/tty/serial/8250/8250_omap.c > index 2ac63c8bd946..933f7ef2c004 100644 > --- a/drivers/tty/serial/8250/8250_omap.c > +++ b/drivers/tty/serial/8250/8250_omap.c > @@ -738,6 +738,7 @@ static void __dma_rx_do_complete(struct uart_8250_port > *p, bool error) > struct dma_tx_state state; > int count; > unsigned long flags; > + int ret; > > dma_sync_single_for_cpu(dma->rxchan->device->dev, dma->rx_addr, > dma->rx_size, DMA_FROM_DEVICE); > @@ -753,8 +754,10 @@ static void __dma_rx_do_complete(struct uart_8250_port > *p, bool error) > > count = dma->rx_size - state.residue; > > - tty_insert_flip_string(tty_port, dma->rx_buf, count); > - p->port.icount.rx += count; > + ret = tty_insert_flip_string(tty_port, dma->rx_buf, count); > + > + p->port.icount.rx += ret; > + p->port.icount.buf_overrun += count - ret; > unlock: > spin_unlock_irqrestore(&priv->rx_dma_lock, flags); > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] serial: 8250: move rx_running out of the bitfield
On 08/26/2015 05:37 AM, Sekhar Nori wrote: > On Friday 14 August 2015 09:31 PM, Sebastian Andrzej Siewior wrote: >> From: John Ogness >> >> That bitfield is modified by read + or + write operation. If someone >> sets any of the other two bits it might render the lock useless. >> >> While at it, remove other bitfields as well to avoid more such >> errors. >> >> Cc: Greg Kroah-Hartman >> Signed-off-by: John Ogness >> Signed-off-by: Sebastian Andrzej Siewior > > Tested with wilink BT module on TI's DRA7 EVM. > > Tested-by: Sekhar Nori Already in Greg's tty-next tree (and 4.3-rc1 pull request), Sekhar. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported
On 08/10/2015 07:54 AM, Peter Ujfalusi wrote: > On 08/07/2015 11:00 PM, Sebastian Andrzej Siewior wrote: > I don't think this is good thing for the stable _and_ for the mainline at the > same time: > in stable the rx DMA should not be allowed since the stable kernels does not > allow pause/resume with omap-dma, so there the rx DMA should be just disabled > for UART. This change will cause regression since it introduce a WARN_ON_ONCE, > which will be printed if the user tries to use non working feature. > > In mainline you will eventually going to have pause/resume support so this > patch will make no sense there. No, the whole point of this mess is that omap-dma does not provide pause/resume support (without data loss). omap-dma will only be suitable for pause/terminate dma. And adding pause support doesn't address the underlying problem that dmaengine is not providing a means of determining if suitable support is available for use by serial drivers, so this same problem is just waiting to happen again. I think the way forward is, For -stable, disable dma in the 8250_omap driver. Then for mainline, * extend the dma_get_slave_caps() api to differentiate the types of pause support * query dma_get_slave_caps() when setting up the slave channel in 8250_dma & 8250_omap and only enable dma if suitable pause support is available * add required dmaengine error checking in 8250_dma & 8250_omap _for unexpected errors_ (so _not_ WARNs) * do whatever with omap-dma. Not even sure it's worth trying to support dma with that; it still won't fully support tx dma which is forcing all kinds of goofy workarounds Russell seemed to think that the current dma operation was necessary information to differentiate types of pause support, but I don't think that's required. As Sebastian's omap-dma driver patch shows, partial pause support has more to do with how it's being used. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
On 08/07/2015 02:32 PM, Russell King - ARM Linux wrote: > On Fri, Aug 07, 2015 at 02:21:59PM -0400, Peter Hurley wrote: >> [ + Heikki ] >> >> On 08/07/2015 12:33 PM, Russell King - ARM Linux wrote: >>> What you have is a race condition in the code you a responsible for >>> maintaining, caused by poorly implemented code. Fix it, rather than >>> whinging about drivers outside of your subsystem having never implemented >>> _optional_ things that you choose to merge broken code which relied upon >>> it _without_ checking that the operation succeeded. >>> >>> It is _entirely_ your code which is wrong here. >>> >>> I will wait for that to be fixed before acking the omap-dma change since >>> you obviously need something to test with. >> >> I'm not sure to what you're referring here. >> >> A WARNing fixes nothing. > > The warning can wait. > >> If you mean some patch, as yet unwritten, that handles the dma cases when >> dmaengine_pause() is unimplemented without data loss, ok, but please confirm >> that's what you mean. > > But the regression needs fixing. I too would prefer the bug to be fixed. But calling it a regression is incorrect. There is no previous SHA in which this problem didn't exist, except before either 8250_dma or 8250_omap was added. >From the outset, both the 8250 dma code and the 8250_omap driver (mistakenly) relied on dmaengine_pause. >> However, at some point one must look at the api and wonder if the separation >> of concern has been drawn in the right place. > > It _is_ in the right place. dmaengine_pause() always has been permitted > to fail. It's the responsibility of the user of this API to _check_ the > return code to find out whether it had the desired effect. Not checking > the return code is a bug in the caller's code. > > If that wasn't the case, dmaengine_pause() would have a void return type. > It doesn't. It has an 'int' to allow failure A resource error is significantly different than ENOSYS or EINVAL. > or to allow non- > implementation for cases where the underlying hardware can't pause the > channel without causing data loss. That's your assertion; I've seen no documentation to back that up (other than the de facto commit). And quite frankly, that's absurd. 1. No other driver implements _only some_ use-cases of dmaengine_pause(). 2. The number of users expecting dmaengine_pause to be implemented for non-cyclic dma transfers _dwarfs_ cyclic users. 3. There's a dedicated query interface, dma_get_slave_caps(), for which omap-dma returns /true/ -- not /maybe/ -- to indicate dmaengine_pause() is implemented. As a consumer of the api, I'd much rather opt-out at device initialization time knowing that a required feature is unimplemented, than discover it at i/o time when it's too late. > What would you think is better: an API which silently loses data, or > one which refuses to stop the transfer and reports an error code back > to the caller. An api which provides a means of determining if necessary functionality is implemented _during setup_. That way the consumer of the api can determine if the feature is supportable. For example, dma_get_slave_caps() could differentiate * pause for cyclic support * pause for non-cyclic support * pause and resume support * pause and terminate support > You seem to be arguing for the former, and as such, there's no way I > can take you seriously. Leaping to conclusions. > In any case, Greg has now commented on the patch adding the feature, > basically refusing it for stable tree inclusion. So the matter is > settled: omap-dma isn't going to get the pause feature added in stable > trees any time soon. So a different solution now needs to be found, > which is what I've been saying all along... While Sebastian's initial patch is a good first-cut at addressing 8250_omap's use of omap-dma, none of the patches address the general design problem I have outlined above; namely, that simply returning an error at use time for an unimplemented slave transaction is fundamentally flawed. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause()
On 08/07/2015 04:00 PM, Sebastian Andrzej Siewior wrote:
> In 8250-omap I learned it the hard way that ignoring the return code
> of dmaengine_pause() might be bad because the underlying DMA driver
> might not support the function at all and so not doing what one is
> expecting.
> This patch adds the __must_check annotation as suggested by Russell King.
>
> Signed-off-by: Sebastian Andrzej Siewior
> ---
> include/linux/dmaengine.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 8ad9a4e839f6..4eac4716bded 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan
> *chan)
> return -ENOSYS;
> }
>
> -static inline int dmaengine_pause(struct dma_chan *chan)
> +static inline int __must_check dmaengine_pause(struct dma_chan *chan)
> {
> if (chan->device->device_pause)
> return chan->device->device_pause(chan);
>
Not that this is your responsibility, Sebastian, but considering there are
fewer than 20 users of dmaengine_pause() in the entire tree, we should add
WARN_ON_ONCE() around those uses with this patch to avoid a bunch needless
one-off "fixes".
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported
On 08/07/2015 04:00 PM, Sebastian Andrzej Siewior wrote:
> The 8250-omap driver requires the DMA-engine driver to support the pause
> command in order to properly turn off programmed RX transfer before the
> driver stars manually reading from the FIFO.
> The lacking support of the requirement has been discovered recently. In
> order to stay safe here we disable support for RX-DMA as soon as we
> notice that it does not work. This should happen very early.
> If the user does not want to see this backtrace he can either disable
> DMA support (completely or RX-only) or backport the required patches for
> edma / omap-dma once they hit mainline.
>
> Cc:
> Signed-off-by: Sebastian Andrzej Siewior
> ---
> drivers/tty/serial/8250/8250_omap.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_omap.c
> b/drivers/tty/serial/8250/8250_omap.c
> index 0340ee6ba970..07a11e0935e4 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -112,6 +112,7 @@ struct omap8250_priv {
> struct work_struct qos_work;
> struct uart_8250_dma omap8250_dma;
> spinlock_t rx_dma_lock;
> + bool rx_dma_broken;
> };
>
> static u32 uart_read(struct uart_8250_port *up, u32 reg)
> @@ -761,6 +762,7 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port
> *p)
> struct omap8250_priv*priv = p->port.private_data;
> struct uart_8250_dma*dma = p->dma;
> unsigned long flags;
> + int ret;
>
> spin_lock_irqsave(&priv->rx_dma_lock, flags);
>
> @@ -769,7 +771,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port
> *p)
> return;
> }
>
> - dmaengine_pause(dma->rxchan);
> + ret = dmaengine_pause(dma->rxchan);
> + if (WARN_ON_ONCE(ret))
> + priv->rx_dma_broken = true;
No offense, Sebastian, but it boggles my mind that anyone could defend this
as solid api design. We're in the middle of an interrupt handler and the
slave dma driver is /just/ telling us now that it doesn't implement this
functionality?!!?
The dmaengine api has _so much_ setup and none of it contemplates telling the
consumer that critical functionality is missing?
Even dma_get_slave_caps() returns _true_ for cmd_pause support; ok, that
interface is pointless.
Rather than losing /critical data/ here, the interrupt handler should just
busy-wait until dmaengine_tx_status() returns DMA_COMPLETE for the rx_cookie.
Regards,
Peter Hurley
> spin_unlock_irqrestore(&priv->rx_dma_lock, flags);
>
> @@ -813,6 +817,9 @@ static int omap_8250_rx_dma(struct uart_8250_port *p,
> unsigned int iir)
> break;
> }
>
> + if (priv->rx_dma_broken)
> + return -EINVAL;
> +
> spin_lock_irqsave(&priv->rx_dma_lock, flags);
>
> if (dma->rx_running)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
[ + Heikki ]
On 08/07/2015 12:33 PM, Russell King - ARM Linux wrote:
> On Fri, Aug 07, 2015 at 12:07:11PM -0400, Peter Hurley wrote:
>> On 08/07/2015 11:29 AM, Russell King - ARM Linux wrote:
>>> On Fri, Aug 07, 2015 at 11:08:48AM -0400, Peter Hurley wrote:
>>>> [ + Greg KH ]
>>>>
>>>> On 08/07/2015 09:57 AM, Russell King - ARM Linux wrote:
>>>>> As it is something that the driver has _not_ supported, you are clearly
>>>>> adding a feature to an existing driver. It's not a bug fix.
>>>>>
>>>>>>> If something else has been converted to pause channels and that is
>>>>>>> causing
>>>>>>> a problem, then _that_ conversion is where the bug lies, not the lack of
>>>>>>> support in the omap-dma.
>>>>
>>>> FWIW, the actual bug is the api that silently does nothing.
>>>
>>> Incorrect.
>>>
>>> static int omap_dma_pause(struct dma_chan *chan)
>>> {
>>> struct omap_chan *c = to_omap_dma_chan(chan);
>>>
>>> /* Pause/Resume only allowed with cyclic mode */
>>> if (!c->cyclic)
>>> return -EINVAL;
>>>
>>> Asking for the channel to be paused will return an error code indicating
>>> that the request failed. That will be propagated back through to the
>>> return code of dmaengine_pause().
>>>
>>> If we look at what 8250-dma.c is doing:
>>>
>>> if (dma->rx_running) {
>>> dmaengine_pause(dma->rxchan);
>>>
>>> It's 8250-dma.c which is silently _ignoring_ the return code, failing
>>> to check that the operation it requested worked. Maybe this should be
>>> WARN_ON(dmaengine_pause(dma->rxchan)) or at least it should print a
>>> message?
>>
>> Thanks for the suggestion; I'll hold on to that and push it after we add
>> the 8250 omap dma pause in mainline.
>
> Why wait? You're hiding a data loss bug which is clearly the result of
> code you allegedly maintain.
Because it will generate tons of unnecessary reports when the patch that
WARNs inevitably ends up in mainline a version earlier than the patch that
fixes it.
>> Well, instead of me saying something snide about the lack of upstream serial
>> driver unit tests, I'll say I've been working on cleaning up and organizing
>> my own tty/serial subsystem and driver units tests which I hope to upstream
>> in the fall.
>>
>> Those include i/o validators that ran this driver for days at time without
>> error at max line rate. Unfortunately, that hardware does not exhibit the
>> same problem as the DRA7 noted in the changelog.
>
> What you have is a race condition in the code you a responsible for
> maintaining, caused by poorly implemented code. Fix it, rather than
> whinging about drivers outside of your subsystem having never implemented
> _optional_ things that you choose to merge broken code which relied upon
> it _without_ checking that the operation succeeded.
>
> It is _entirely_ your code which is wrong here.
>
> I will wait for that to be fixed before acking the omap-dma change since
> you obviously need something to test with.
I'm not sure to what you're referring here.
A WARNing fixes nothing.
If you mean some patch, as yet unwritten, that handles the dma cases when
dmaengine_pause() is unimplemented without data loss, ok, but please confirm
that's what you mean.
However, at some point one must look at the api and wonder if the separation
of concern has been drawn in the right place.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
On 08/07/2015 12:39 PM, Russell King - ARM Linux wrote:
> On Fri, Aug 07, 2015 at 05:44:03PM +0200, Sebastian Andrzej Siewior wrote:
>> On 08/07/2015 05:29 PM, Russell King - ARM Linux wrote:
>>> On Fri, Aug 07, 2015 at 11:08:48AM -0400, Peter Hurley wrote:
>>>> [ + Greg KH ]
>>>>
>>>> On 08/07/2015 09:57 AM, Russell King - ARM Linux wrote:
>>>>> As it is something that the driver has _not_ supported, you are clearly
>>>>> adding a feature to an existing driver. It's not a bug fix.
>>>>>
>>>>>>> If something else has been converted to pause channels and that is
>>>>>>> causing
>>>>>>> a problem, then _that_ conversion is where the bug lies, not the lack of
>>>>>>> support in the omap-dma.
>>>>
>>>> FWIW, the actual bug is the api that silently does nothing.
>>>
>>> Incorrect.
>>>
>>> static int omap_dma_pause(struct dma_chan *chan)
>>> {
>>> struct omap_chan *c = to_omap_dma_chan(chan);
>>>
>>> /* Pause/Resume only allowed with cyclic mode */
>>> if (!c->cyclic)
>>> return -EINVAL;
>>>
>>> Asking for the channel to be paused will return an error code indicating
>>> that the request failed. That will be propagated back through to the
>>> return code of dmaengine_pause().
>>>
>>> If we look at what 8250-dma.c is doing:
>>>
>>> if (dma->rx_running) {
>>> dmaengine_pause(dma->rxchan);
>>>
>>> It's 8250-dma.c which is silently _ignoring_ the return code, failing
>>> to check that the operation it requested worked. Maybe this should be
>>> WARN_ON(dmaengine_pause(dma->rxchan)) or at least it should print a
>>> message?
>>
>> I think this is what Peter meant by saying "silently does nothing".
>
> Maybe Peter should phrase his replies better. "the actual bug is the
> api that silently does nothing." to me means he is complaining that
> dmaengine_pause() had no effect. _That_ is what I'm objecting to,
> and claiming that Peter's comment is wrong.
Yes, I missed that the api included a return code which the 8250 dma
code should be checking.
> He's now blaming me for snide remarks. I could call his one above an
> incorrect snide remark against the quality of DMA engine code.
You'd be reading a lot into my statement.
> He's
> totally wrong, and been proven wrong by my analysis above, plain and
> simple.
>
> He should now accept that he's wrong
Done.
> and move along with sorting out
> this mess _without_ requiring optional features to be implemented in
> other subsystems to fix bugs in code he's supposed to be maintaining.
This is simply a bug that flew under the radar, much like every other
bug that gets fixed daily in mainline.
The omap-serial driver which doesn't use dma is still the preferred
stable driver for omap, for the moment.
One of the main features of the 8250_omap integration was the addition
of dma support. Without it, 8250_omap is ttyO in ttyS clothing.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
On 08/07/2015 11:29 AM, Russell King - ARM Linux wrote:
> On Fri, Aug 07, 2015 at 11:08:48AM -0400, Peter Hurley wrote:
>> [ + Greg KH ]
>>
>> On 08/07/2015 09:57 AM, Russell King - ARM Linux wrote:
>>> As it is something that the driver has _not_ supported, you are clearly
>>> adding a feature to an existing driver. It's not a bug fix.
>>>
>>>>> If something else has been converted to pause channels and that is causing
>>>>> a problem, then _that_ conversion is where the bug lies, not the lack of
>>>>> support in the omap-dma.
>>
>> FWIW, the actual bug is the api that silently does nothing.
>
> Incorrect.
>
> static int omap_dma_pause(struct dma_chan *chan)
> {
> struct omap_chan *c = to_omap_dma_chan(chan);
>
> /* Pause/Resume only allowed with cyclic mode */
> if (!c->cyclic)
> return -EINVAL;
>
> Asking for the channel to be paused will return an error code indicating
> that the request failed. That will be propagated back through to the
> return code of dmaengine_pause().
>
> If we look at what 8250-dma.c is doing:
>
> if (dma->rx_running) {
> dmaengine_pause(dma->rxchan);
>
> It's 8250-dma.c which is silently _ignoring_ the return code, failing
> to check that the operation it requested worked. Maybe this should be
> WARN_ON(dmaengine_pause(dma->rxchan)) or at least it should print a
> message?
Thanks for the suggestion; I'll hold on to that and push it after we add
the 8250 omap dma pause in mainline.
>>> Right, so the patch which caused the regression is the one which arranged
>>> for the 8250-dma + omap-dma combination to work together, not the missing
>>> pause support in omap-dma.
>>
>> That would be the original submission patch set for an entire driver,
>> the 8250_omap driver.
>
> Well, that's where the bug lies, and I don't agree with your assessment
> that it would mean reverting the whole thing.
>
> The binding between the two drivers is controlled via DT. DT tells it
> which DMA controller and which DMA input to use. So, as DMA is currently
> broken on this, the solution is to break that link so that 8250-omap goes
> back to using PIO only.
>
>>> As the binding of drivers is controlled by DT, you can disable the binding
>>> of these two drivers
>>
>> No. 8250 dma is not a stand-alone driver. Even if it were, how would you go
>> back and fix DTs in the wild?
>
> Well, we have reached an impass then.
>
> I'm not going to accept a feature addition to a driver as a stable patch
> without it being adequately tested over _several_ kernel revisions to
> ensure that we don't end up cocking up some driver which uses the DMA
> interfaces correctly. It's too big a risk.
>
> So, I guess that means that older kernels will just have to remain broken -
> all because the basic testing of the original code was never undertaken
> to ensure that basic stuff like reception of characters worked properly.
Well, instead of me saying something snide about the lack of upstream serial
driver unit tests, I'll say I've been working on cleaning up and organizing
my own tty/serial subsystem and driver units tests which I hope to upstream
in the fall.
Those include i/o validators that ran this driver for days at time without
error at max line rate. Unfortunately, that hardware does not exhibit the
same problem as the DRA7 noted in the changelog.
> Sorry, I have little sympathy here.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
[ + Greg KH ] On 08/07/2015 09:57 AM, Russell King - ARM Linux wrote: > On Fri, Aug 07, 2015 at 03:42:06PM +0200, Sebastian Andrzej Siewior wrote: >> On 08/07/2015 03:22 PM, Russell King - ARM Linux wrote: >>> On Fri, Aug 07, 2015 at 12:36:14PM +0200, Sebastian Andrzej Siewior wrote: >>>> On 08/07/2015 11:44 AM, Peter Ujfalusi wrote: >>>>> with a short testing audio did not broke (the only user of pause/resume) >>>>> Some comments embedded. >>>>> >>>>>> Cc: >>>>> >>>>> Why stable? This is not fixing any bugs since the PAUSE was not allowed >>>>> for >>>>> non cyclic transfers. >>>> >>>> Hmmm. The DRA7x was using pause before for UART. I just did not see it >>>> coming that it was not allowed here. John made a similar change to the >>>> edma driver and I assumed it went stable but now I see that it was just >>>> cherry-picked into the ti tree. >>> >>> This is *NOT* stable material. >>> >>> Pause of these channels is something that omap-dma has *never* supported. >>> Therefore, it is *not* a regression. What you are doing is *adding* a >>> feature to the omap-dma driver. That is not stable material in any sense. >>> Stable is for bug fixes to existing code, not feature enhancements. >> >> I didn't consider this as a feature. > > As it is something that the driver has _not_ supported, you are clearly > adding a feature to an existing driver. It's not a bug fix. > >>> If something else has been converted to pause channels and that is causing >>> a problem, then _that_ conversion is where the bug lies, not the lack of >>> support in the omap-dma. FWIW, the actual bug is the api that silently does nothing. >> So we had the 8250-DMA doing pause and all its current users implement >> it. We have a DMA driver tree which is not used and it not implementing >> pause (not implementing pause at all). Later we get a combo of 8250-DMA >> + DMA driver that is broken because the lack of pause and this is >> noticed a few kernel releases later. > > Right, so the patch which caused the regression is the one which arranged > for the 8250-dma + omap-dma combination to work together, not the missing > pause support in omap-dma. That would be the original submission patch set for an entire driver, the 8250_omap driver. > It doesn't matter that it's several releases old, it's that change caused > the regression you have today. That change is incorrect today, and it was > just as incorrect at the time that it was merged. > >> The only way of fixing the bug is by implementing the pause feature. > > That's not the only way of fixing the bug. > > As the binding of drivers is controlled by DT, you can disable the binding > of these two drivers No. 8250 dma is not a stand-alone driver. Even if it were, how would you go back and fix DTs in the wild? The "binding" is built-in with a CONFIG_ switch. > there and 8250 will go back to using non-DMA mode - > which is the situation which existed prior to the change which coupled the > two drivers together. That's an acceptable change for -stable trees, > because it's reverting the change which caused the regression, taking us > back to a situation we _know_ worked previously. What you're suggesting here is only possible if the entire 8250_omap driver is removed, so that's a non-starter. I suggest to wait on any solution until the correct fix is mainlined and backported, as you note below. Regards, Peter Hurley > Then, in mainline during the next merge window, we can introduce the pause > feature to omap-dma, and re-enable the 8250 driver to use it. _Once_ that's > proven stable, we can then take a view whether those changes should _then_ > be backported to stable kernels with greater confidence that backporting > the feature addition won't itself cause a new regression. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
On 08/07/2015 09:25 AM, Russell King - ARM Linux wrote: > On Fri, Aug 07, 2015 at 03:22:56PM +0200, Sebastian Andrzej Siewior wrote: >> For TX-transfers, I would need to update the start-address so the >> transfers begins where it stopped. However based on your concern I >> can't really assume that the position reported by the HW is the correct >> one. > > Exactly - I don't believe that existing OMAP DMA hardware can ever support > pausing a mem-to-device transfer without data loss as you have no idea > how many bytes have been prefetched by the DMA, and therefore you have > no idea how many bytes to unwind the hardware position. It gets worse > than that when you have to cross into the previous descriptor. It's > really not nice. > > So, disallowing pause for mem-to-device is entirely reasonable given the > data loss implications. Thanks for adding your hard-won knowledge to this discussion, Russell. This saves us a bunch of wasted effort trying to fix x_char with DMA (and TCSANOW termios changes and throttling). Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown
On 08/06/2015 09:59 AM, Sebastian Andrzej Siewior wrote: > On 08/06/2015 02:31 PM, Sebastian Andrzej Siewior wrote: > > Hi Peter, > >>> I'll look at/test this this weekend, ok? >> >> Sure. I'm currently re-spinning the patches so have everything in >> proper pieces. While at it I will take a look at x_char. > > So now that I actually look at it. If I read this right, we never send > the x_char if the TX-DMA never fails to do its job. That's what I saw too; almost all the dma drivers are broken wrt x_char. The amba-pl011 driver gets it right. > The comment above uart_send_xchar() says it is high priority. 'High' priority is meant relative to previously written data which has not yet been sent. > What do you suggest, wait > until the transfer completes, send the x_char _or_ pause the transfer > send that byte and then send the byte? 'Better' would be sending the x_char when the current dma transfer completes. However, it will probably have /some/ impact on what line rates software flow control can be used. Worst case @ 115Kbaud is 35ms delay in sending. 'Best' would be pausing the dma and sending the byte. However, I'm not even sure if this is possible on OMAP; the TRM is woefully under-documented in that regard. > In both cases we have to wait until for the FIFO-empty interrupt to > make sure we don't overrun that TX-FIFO. > > I *think* waiting until the transfer completes would be simpler but it > is not necessarily high priority. I agree; this is what we should do first because someone might want it for backports. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown
Hi Sebastian, On 08/04/2015 07:58 AM, Sebastian Andrzej Siewior wrote: > On 08/03/2015 09:32 PM, Peter Hurley wrote: > >>> You mean a function in 8250-dma API which does what I did just here >>> with the wait_event() and the wake_up in the callback? That way I could >>> move the termios_wait into the dma struct instead of keeping in the >>> omap specific part. I am also not sure if OMAP is the only one that may >>> hang here or the other people just didn't notice it yet. >> >> Exactly; and we need to fix DMA wrt x_char anyway. >> >> Going back to the dmaengine api, I think something like this might work >> (as a first approximation): >> >> dma_sync_wait(dma->txchan, dma->tx_cookie); >> dmaengine_pause(dma->txchan); >> >> /* remainder of set_termios */ >> >> dmaengine_resume(dma->txchan); >> >> We could require 8250 core dma to support pause/resume. > > I would prefer the waitqueue approach. > You can't do this while holding the port lock. The lock is taken with > irqs off so may not see the transfer completing. > Why do you pause the channel? It may not work without an active > descriptor and a start without "resume" should work. Also you must > ensure that DMA's complete callback does not start another transfer if > there is something queued up (that is why I had the tx_running dance). > I am not sure if a transfer that is active and then paused will not > trigger the hang bug if we change the termios in between. I'll look at/test this this weekend, ok? Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown
On 08/03/2015 12:54 PM, Sebastian Andrzej Siewior wrote:
> On 08/03/2015 06:34 PM, Peter Hurley wrote:
>> Hi Sebastian,
>
> Hi Peter,
>
>>> struct old_serial_port {
>>> diff --git a/drivers/tty/serial/8250/8250_omap.c
>>> b/drivers/tty/serial/8250/8250_omap.c
>>> index d9a37191a1ae..12249125a218 100644
>>> --- a/drivers/tty/serial/8250/8250_omap.c
>>> +++ b/drivers/tty/serial/8250/8250_omap.c
>>> @@ -100,9 +100,9 @@ struct omap8250_priv {
>>> u8 wer;
>>> u8 xon;
>>> u8 xoff;
>>> - u8 delayed_restore;
>>> u16 quot;
>>>
>>> + wait_queue_head_t termios_wait;
>>> bool is_suspending;
>>> int wakeirq;
>>> int wakeups_enabled;
>>> @@ -256,18 +256,6 @@ static void omap8250_update_mdr1(struct uart_8250_port
>>> *up,
>>> static void omap8250_restore_regs(struct uart_8250_port *up)
>>> {
>>> struct omap8250_priv *priv = up->port.private_data;
>>> - struct uart_8250_dma*dma = up->dma;
>>> -
>>> - if (dma && dma->tx_running) {
>>> - /*
>>> -* TCSANOW requests the change to occur immediately however if
>>> -* we have a TX-DMA operation in progress then it has been
>>> -* observed that it might stall and never complete. Therefore we
>>> -* delay DMA completes to prevent this hang from happen.
>>> -*/
>>> - priv->delayed_restore = 1;
>>> - return;
>>> - }
>>>
>>> serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>>> serial_out(up, UART_EFR, UART_EFR_ECB);
>>> @@ -309,6 +297,7 @@ static void omap8250_restore_regs(struct uart_8250_port
>>> *up)
>>> up->port.ops->set_mctrl(&up->port, up->port.mctrl);
>>> }
>>>
>>> +static void omap_8250_dma_tx_complete(void *param);
>>> /*
>>> * OMAP can use "CLK / (16 or 13) / div" for baud rate. And then we have
>>> have
>>> * some differences in how we want to handle flow control.
>>> @@ -322,6 +311,7 @@ static void omap_8250_set_termios(struct uart_port
>>> *port,
>>> struct omap8250_priv *priv = up->port.private_data;
>>> unsigned char cval = 0;
>>> unsigned int baud;
>>> + unsigned int complete_dma = 0;
>>>
>>> switch (termios->c_cflag & CSIZE) {
>>> case CS5:
>>> @@ -473,6 +463,25 @@ static void omap_8250_set_termios(struct uart_port
>>> *port,
>>> if (termios->c_iflag & IXANY)
>>> up->mcr |= UART_MCR_XONANY;
>>> }
>>> +
>>> + if (up->dma && up->dma->tx_running) {
>>> + struct uart_8250_dma*dma = up->dma;
>>> +
>>> + /*
>>> +* TCSANOW requests the change to occur immediately however if
>>> +* we have a TX-DMA operation in progress then it has been
>>> +* observed that it might stall and never complete. Therefore we
>>> +* wait until DMA completes to prevent this hang from happen.
>>> +*/
>>> +
>>> + dma->tx_running = 2;
>>> +
>>> + spin_unlock_irq(&up->port.lock);
>>> + wait_event(priv->termios_wait,
>>> + dma->tx_running == 3);
>>
>> Doesn't the dmaengine api offer a race-free way to wait for pending tx dma
>> to complete?
>
> Not that I know of. You still need to ensure that once that DMA
> completed, nobody triggers another TX transfer before you do what you
> planned. This is ensures by the tx_running != 0 and the spin lock.
>
>> Maybe we could wrap that in the 8250 dma api?
>
> You mean a function in 8250-dma API which does what I did just here
> with the wait_event() and the wake_up in the callback? That way I could
> move the termios_wait into the dma struct instead of keeping in the
> omap specific part. I am also not sure if OMAP is the only one that may
> hang here or the other people just didn't notice it yet.
Exactly; and we need to fix DMA wrt x_char anyway.
Going back to the dmaengine api, I think something like this might work
(as a first approximation):
dma_sync_wait(dma->txchan, dma->tx_cookie);
dmaengine_pause(dma->txchan);
/* remainder of set_termios */
dmaengine_resume(dma->txchan);
We cou
Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown
[ +cc Heikki ]
Hi Sebastian,
On 08/03/2015 12:09 PM, Sebastian Andrzej Siewior wrote:
> * Peter Hurley | 2015-07-30 20:51:10 [-0400]:
>
>> Hi John,
> Hi Peter,
>
>> I was never really a fan of the deferred set_termios();
>> I think it's more appropriate to wait for tx dma to
>> complete in omap_8250_set_termios().
>
> So you want something like this? This was only compile + boot tested
> (without triggering the corner case) and I know that 8250.h piece has to
> go in a separated patch (as requested in 2/3 of this series). Just checking
> if this is what you had in mind.
>
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index c43f74c53cd9..a407757dcecc 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -42,9 +42,9 @@ struct uart_8250_dma {
> size_t rx_size;
> size_t tx_size;
>
> - unsigned char tx_running:1;
> - unsigned char tx_err: 1;
> - unsigned char rx_running:1;
> + unsigned char tx_running;
> + unsigned char tx_err;
> + unsigned char rx_running;
> };
This part is ok.
> struct old_serial_port {
> diff --git a/drivers/tty/serial/8250/8250_omap.c
> b/drivers/tty/serial/8250/8250_omap.c
> index d9a37191a1ae..12249125a218 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -100,9 +100,9 @@ struct omap8250_priv {
> u8 wer;
> u8 xon;
> u8 xoff;
> - u8 delayed_restore;
> u16 quot;
>
> + wait_queue_head_t termios_wait;
> bool is_suspending;
> int wakeirq;
> int wakeups_enabled;
> @@ -256,18 +256,6 @@ static void omap8250_update_mdr1(struct uart_8250_port
> *up,
> static void omap8250_restore_regs(struct uart_8250_port *up)
> {
> struct omap8250_priv *priv = up->port.private_data;
> - struct uart_8250_dma*dma = up->dma;
> -
> - if (dma && dma->tx_running) {
> - /*
> - * TCSANOW requests the change to occur immediately however if
> - * we have a TX-DMA operation in progress then it has been
> - * observed that it might stall and never complete. Therefore we
> - * delay DMA completes to prevent this hang from happen.
> - */
> - priv->delayed_restore = 1;
> - return;
> - }
>
> serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
> serial_out(up, UART_EFR, UART_EFR_ECB);
> @@ -309,6 +297,7 @@ static void omap8250_restore_regs(struct uart_8250_port
> *up)
> up->port.ops->set_mctrl(&up->port, up->port.mctrl);
> }
>
> +static void omap_8250_dma_tx_complete(void *param);
> /*
> * OMAP can use "CLK / (16 or 13) / div" for baud rate. And then we have have
> * some differences in how we want to handle flow control.
> @@ -322,6 +311,7 @@ static void omap_8250_set_termios(struct uart_port *port,
> struct omap8250_priv *priv = up->port.private_data;
> unsigned char cval = 0;
> unsigned int baud;
> + unsigned int complete_dma = 0;
>
> switch (termios->c_cflag & CSIZE) {
> case CS5:
> @@ -473,6 +463,25 @@ static void omap_8250_set_termios(struct uart_port *port,
> if (termios->c_iflag & IXANY)
> up->mcr |= UART_MCR_XONANY;
> }
> +
> + if (up->dma && up->dma->tx_running) {
> + struct uart_8250_dma*dma = up->dma;
> +
> + /*
> + * TCSANOW requests the change to occur immediately however if
> + * we have a TX-DMA operation in progress then it has been
> + * observed that it might stall and never complete. Therefore we
> + * wait until DMA completes to prevent this hang from happen.
> + */
> +
> + dma->tx_running = 2;
> +
> + spin_unlock_irq(&up->port.lock);
> + wait_event(priv->termios_wait,
> +dma->tx_running == 3);
Doesn't the dmaengine api offer a race-free way to wait for pending tx dma
to complete?
Maybe we could wrap that in the 8250 dma api?
Regards,
Peter Hurley
> + spin_lock_irq(&up->port.lock);
> + complete_dma = 1;
> + }
> omap8250_restore_regs(up);
>
> spin_unlock_irq(&up->port.lock);
> @@ -488,6 +497,8 @@ static void omap_8250_set_termios(struct uart_port *port,
> /* Don't rewrite B0 */
> if (tty_termi
Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown
Hi John,
On 07/30/2015 06:54 PM, John Ogness wrote:
> If DMA is active during a shutdown, a delayed restore of the
> registers may be pending. The restore must be performed after
> the DMA is stopped, otherwise the delayed restore remains
> pending and will fire upon the first DMA TX complete of a
> totally different serial session.
>
> Signed-off-by: John Ogness
> ---
> drivers/tty/serial/8250/8250_omap.c |8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_omap.c
> b/drivers/tty/serial/8250/8250_omap.c
> index 5b39892..25f6255 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -657,9 +657,15 @@ static void omap_8250_shutdown(struct uart_port *port)
> up->ier = 0;
> serial_out(up, UART_IER, 0);
>
> - if (up->dma)
> + if (up->dma) {
> serial8250_release_dma(up);
>
> + if (priv->delayed_restore) {
> + priv->delayed_restore = 0;
> + omap8250_restore_regs(up);
> + }
I was never really a fan of the deferred set_termios();
I think it's more appropriate to wait for tx dma to
complete in omap_8250_set_termios().
Regards,
Peter Hurley
> + }
> +
> /*
>* Disable break condition and FIFOs
>*/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] serial: 8250: unlock port for uart_write_wakeup()
On 07/30/2015 07:15 PM, Peter Hurley wrote: > On 07/30/2015 06:54 PM, John Ogness wrote: >> uart_write_wakeup() should be called without holding the port lock. >> Otherwise a possible recursive spinlock issue can occur, such as >> the following callchain: >> >> 8250_core.c:serial8250_tx_chars() - called with port locked >> serial_core.c:uart_write_wakeup() >> tty_io.c:tty_wakeup() >>st_core.c:st_tty_wakeup() >> st_core.c:st_tx_wakeup() >> st_core.c:st_int_write() >> serial_core.c:uart_write() - locks port > > NAK. > > This is a bug in the N_TI_WL line discipline, specifically in the > st_tx_wakeup() function, which cannot perform the write synchronously. > > This is a common line discipline bug, and typically fixed by performing > the wakeup operations from a kworker instead. Also, seriously consider if you want to use that TI line discipline at all. If you're using it only for bluetooth w/ kernel bluetooth stack, you don't need btwilink + st_drv anyway. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] serial: 8250: move rx_running out of the bitfield
On 07/30/2015 06:54 PM, John Ogness wrote:
> That bitfield is modified by read + or + write operation. If someone
> sets any of the other two bits it might render the lock useless.
Good catch.
Let's just make all of the fields not bitfield though.
Regards,
Peter Hurley
> Signed-off-by: John Ogness
> Signed-off-by: Sebastian Andrzej Siewior
> ---
> drivers/tty/serial/8250/8250.h |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index c43f74c..78f5e3a 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -44,7 +44,7 @@ struct uart_8250_dma {
>
> unsigned char tx_running:1;
> unsigned char tx_err: 1;
> - unsigned char rx_running:1;
> + unsigned char rx_running;
> };
>
> struct old_serial_port {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] serial: 8250: unlock port for uart_write_wakeup()
On 07/30/2015 06:54 PM, John Ogness wrote: > uart_write_wakeup() should be called without holding the port lock. > Otherwise a possible recursive spinlock issue can occur, such as > the following callchain: > > 8250_core.c:serial8250_tx_chars() - called with port locked > serial_core.c:uart_write_wakeup() > tty_io.c:tty_wakeup() >st_core.c:st_tty_wakeup() > st_core.c:st_tx_wakeup() > st_core.c:st_int_write() > serial_core.c:uart_write() - locks port NAK. This is a bug in the N_TI_WL line discipline, specifically in the st_tx_wakeup() function, which cannot perform the write synchronously. This is a common line discipline bug, and typically fixed by performing the wakeup operations from a kworker instead. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/7] serial: 8250_omap: workaround for IP errata
On 07/14/2015 04:02 AM, Sekhar Nori wrote: > This series works around "Advisory 21" as documented in > AM437x SoC errata[1]. This errata prevents UART module > from idling after DMA is used. AM335x and DRA7x also suffer > from the same errata and chip design team is in the process > of updating the errata documents of those devices as well. > > Patch 1/7 fixes a related bug but can be applied independently. Looks good. Thanks Sekhar! For series, Reviewed-by: Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] tty: 8250: workaround errata on disabling UART after using DMA
On 07/09/2015 10:15 AM, Sekhar Nori wrote:
> On Thursday 09 July 2015 07:03 AM, Peter Hurley wrote:
[...]
>>> @@ -1307,6 +1320,36 @@ static int omap8250_runtime_suspend(struct device
>>> *dev)
>>> return -EBUSY;
>>> }
>>>
>>> + if (priv->habit & UART_ERRATA_CLOCK_DISABLE) {
>>> + int sysc;
>>> + int syss;
>>> + int timeout = 100;
>>> +
>>> + sysc = serial_in(up, UART_OMAP_SYSC);
>>> +
>>> + /* softreset the UART */
>>> + sysc |= OMAP_UART_SYSC_SOFTRESET;
>>> + serial_out(up, UART_OMAP_SYSC, sysc);
>>> +
>>> + /* By experiments, 1us enough for reset complete on AM335x */
>>> + do {
>>> + udelay(1);
>>> + syss = serial_in(up, UART_OMAP_SYSS);
>>> + } while (--timeout && !(syss & OMAP_UART_SYSS_RESETDONE));
>>
>>
>> If there is not a omap helper function to reset modules, there should be.
>> Open-coding the module reset is not appropriate.
>
> There is work planned to have something implemented for OMAP as part of
> drivers/reset/ API. AFAIK, this depends on some PRCM code clean-up
> affecting multiple OMAP platforms and is couple of merge windows away
> from taking shape.
>
> Meanwhile, I think this is the best we can do. Other drivers like
> i2c-omap have done something similar (see omap_i2c_reset()).
Ok, then please make the reset a separate local function
(returning success/failure?). omap_8250_reset()?
A TODO or FIXME above it explaining
the temporary nature of the function would be appreciated :)
>>
>>> + if (!timeout) {
>>> + dev_err(dev, "timed out waiting for reset done\n");
>>> + return -ETIMEDOUT;
>>> + }
>>> +
>>> + /*
>>> +* For UART module wake-up to work, MDR1.MODESELECT should
>>> +* not be set to "Disable", so update it.
>>> +*/
>>> + if (device_may_wakeup(dev))
>>> + omap8250_update_mdr1(up, priv);
>>
>> Should this be unconditional?
>
> I do not see it doing any harm if done unconditionally. But it will be
> unnecessary. Unless the UART is being used for wakeup, we don't need
> MDR1 restored at this stage. And omap8250_restore_regs() will eventually
> restore the register anyway.
Yeah, I understand that, but special-casing it isn't adding any value;
it's not as if you actually want the module to not be in UART mode.
And the comment could be single-line:
/* Restore to UART mode after reset (for wakeup) */
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] tty: 8250: workaround errata on disabling UART after using DMA
, there should be.
Open-coding the module reset is not appropriate.
> + if (!timeout) {
> + dev_err(dev, "timed out waiting for reset done\n");
> + return -ETIMEDOUT;
> + }
> +
> + /*
> + * For UART module wake-up to work, MDR1.MODESELECT should
> + * not be set to "Disable", so update it.
> + */
> + if (device_may_wakeup(dev))
> + omap8250_update_mdr1(up, priv);
Should this be unconditional?
Regards,
Peter Hurley
> + }
> +
> if (up->dma && up->dma->rxchan)
> omap_8250_rx_dma(up, UART_IIR_RX_TIMEOUT);
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] tty: 8250: omap: introduce function to update mdr1
Hi Sekhar,
On 07/06/2015 05:47 AM, Sekhar Nori wrote:
> updating mdr1 register on OMAP needs to take care of
> errata i202. Introduce a function to update mdr1.
>
> This will be useful later on when mdr1 needs to be
> written to from other places. No functional change.
This changelog is not clear. May I suggest:
serial: 8250_omap: Refactor MDR1 update
The errata [1] workaround implemented in follow-on patch,
"serial: 8250_omap: workaround errata on disabling UART after using DMA",
requires MDR1 register programming.
Extract MDR1 register update into helper function, omap8250_update_mdr1().
[1] Advisory 21 in http://www.ti.com/lit/er/sprz408b/sprz408b.pdf
Regards,
Peter Hurley
> Signed-off-by: Sekhar Nori
> ---
> drivers/tty/serial/8250/8250_omap.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_omap.c
> b/drivers/tty/serial/8250/8250_omap.c
> index 20c5b9c4c288..d9c96b993a84 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -232,6 +232,15 @@ static void omap8250_update_scr(struct uart_8250_port
> *up,
> serial_out(up, UART_OMAP_SCR, priv->scr);
> }
>
> +static void omap8250_update_mdr1(struct uart_8250_port *up,
> + struct omap8250_priv *priv)
> +{
> + if (priv->habit & UART_ERRATA_i202_MDR1_ACCESS)
> + omap_8250_mdr1_errataset(up, priv);
> + else
> + serial_out(up, UART_OMAP_MDR1, priv->mdr1);
> +}
> +
> static void omap8250_restore_regs(struct uart_8250_port *up)
> {
> struct omap8250_priv *priv = up->port.private_data;
> @@ -282,11 +291,9 @@ static void omap8250_restore_regs(struct uart_8250_port
> *up)
> serial_out(up, UART_XOFF1, priv->xoff);
>
> serial_out(up, UART_LCR, up->lcr);
> - /* need mode A for FCR */
> - if (priv->habit & UART_ERRATA_i202_MDR1_ACCESS)
> - omap_8250_mdr1_errataset(up, priv);
> - else
> - serial_out(up, UART_OMAP_MDR1, priv->mdr1);
> +
> + omap8250_update_mdr1(up, priv);
> +
> up->port.ops->set_mctrl(&up->port, up->port.mctrl);
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/7] tty: 8250: omap eliminate use of of_machine_is_compatible()
Hi Sekhar,
On 07/06/2015 05:47 AM, Sekhar Nori wrote:
> Use of of_machine_is_compatible() for AM335x specific DMA
> quirk in 8250_omap driver makes it ugly to extend the
> quirk for other platforms. Instead use a new compatible.
>
> The new compatible will also make it easier to care of other
> quirks specific to AM335x and like SoCs.
>
> This patch does break backward DTB compatibility for users of
> 8250_omap driver on AM335x boards.
Not ok.
8250_omap was released with 3.19 and has been the default driver for
BeagleBone since 4.0.
Regards,
Peter Hurley
> However, the 8250_omap driver
> is new and omap_serial is still the default choice driver for UART
> and so choosing to break compatibility over keeping the code
> around forever.
>
> Signed-off-by: Sekhar Nori
> ---
> .../devicetree/bindings/serial/omap_serial.txt | 1 +
> arch/arm/boot/dts/am33xx.dtsi | 12
> drivers/tty/serial/8250/8250_omap.c| 35
> +-
> 3 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/omap_serial.txt
> b/Documentation/devicetree/bindings/serial/omap_serial.txt
> index d3bd2b1ec401..0ee88209b341 100644
> --- a/Documentation/devicetree/bindings/serial/omap_serial.txt
> +++ b/Documentation/devicetree/bindings/serial/omap_serial.txt
> @@ -5,6 +5,7 @@ Required properties:
> - compatible : should be "ti,omap3-uart" for OMAP3 controllers
> - compatible : should be "ti,omap4-uart" for OMAP4 controllers
> - compatible : should be "ti,am4372-uart" for AM437x controllers
> +- compatible : should be "ti,am3352-uart" for AM335x controllers
> - reg : address and length of the register space
> - interrupts or interrupts-extended : Should contain the uart interrupt
>specifier or both the interrupt
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index 21fcc440fc1a..b76f9a2ce05d 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -210,7 +210,7 @@
> };
>
> uart0: serial@44e09000 {
> - compatible = "ti,omap3-uart";
> + compatible = "ti,am3352-uart", "ti,omap3-uart";
> ti,hwmods = "uart1";
> clock-frequency = <4800>;
> reg = <0x44e09000 0x2000>;
> @@ -221,7 +221,7 @@
> };
>
> uart1: serial@48022000 {
> - compatible = "ti,omap3-uart";
> + compatible = "ti,am3352-uart", "ti,omap3-uart";
> ti,hwmods = "uart2";
> clock-frequency = <4800>;
> reg = <0x48022000 0x2000>;
> @@ -232,7 +232,7 @@
> };
>
> uart2: serial@48024000 {
> - compatible = "ti,omap3-uart";
> + compatible = "ti,am3352-uart", "ti,omap3-uart";
> ti,hwmods = "uart3";
> clock-frequency = <4800>;
> reg = <0x48024000 0x2000>;
> @@ -243,7 +243,7 @@
> };
>
> uart3: serial@481a6000 {
> - compatible = "ti,omap3-uart";
> + compatible = "ti,am3352-uart", "ti,omap3-uart";
> ti,hwmods = "uart4";
> clock-frequency = <4800>;
> reg = <0x481a6000 0x2000>;
> @@ -252,7 +252,7 @@
> };
>
> uart4: serial@481a8000 {
> - compatible = "ti,omap3-uart";
> + compatible = "ti,am3352-uart", "ti,omap3-uart";
> ti,hwmods = "uart5";
> clock-frequency = <4800>;
> reg = <0x481a8000 0x2000>;
> @@ -261,7 +261,7 @@
> };
>
> uart5: serial@481aa000 {
> - compatible = "ti,omap3-uart";
> + compatible = "ti,am3352-uart", "ti,omap3-uart";
> ti,hwmods = "uart6";
> clock-frequency = <4800>;
> reg = <0x481aa000 0x2000>;
> diff --git a/drivers/tty/serial/8250/8250_omap.c
> b/drivers/tty/serial/8250/8250_omap.c
> index d9c96b993a84
Re: [PATCH RESEND 1/7] tty: serial: 8250: omap: fix kernel crash in suspend-to-ram
Hi Sekhar,
On 07/06/2015 05:47 AM, Sekhar Nori wrote:
> omap_device infrastructure has a suspend_noirq hook which
> runtime suspends all devices late in the suspend cycle (see
> _od_suspend_noirq() in arch/arm/mach-omap2/omap_device.c)
Why is omap2 the only arch/SoC that does this; ie., call the runtime
callbacks after the system pm callbacks?
Whatever positive it brings, it's a mess at the driver level.
For example, this driver has to hook prepare()/complete() so it can
set local state so that it can detect when the runtime suspend
is being called during system suspend.
Regards,
Peter Hurley
> This leads to a NULL pointer exception in 8250_omap driver
> since by the time omap8250_runtime_suspend() is called, 8250_dma
> driver has already set rxchan to NULL via serial8250_release_dma().
>
> Make an explicit check to see if rxchan is NULL in
> runtime_{suspend|resume} hooks to fix this.
>
> Signed-off-by: Sekhar Nori
> ---
> Previous version: http://www.spinics.net/lists/linux-omap/msg119459.html
> No change in this version except rebased to v4.2-rc1
>
> drivers/tty/serial/8250/8250_omap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_omap.c
> b/drivers/tty/serial/8250/8250_omap.c
> index d75a66c72750..20c5b9c4c288 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1285,7 +1285,7 @@ static int omap8250_runtime_suspend(struct device *dev)
> return -EBUSY;
> }
>
> - if (up->dma)
> + if (up->dma && up->dma->rxchan)
> omap_8250_rx_dma(up, UART_IIR_RX_TIMEOUT);
>
> priv->latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
> @@ -1310,7 +1310,7 @@ static int omap8250_runtime_resume(struct device *dev)
> if (loss_cntx)
> omap8250_restore_regs(up);
>
> - if (up->dma)
> + if (up->dma && up->dma->rxchan)
> omap_8250_rx_dma(up, 0);
>
> priv->latency = priv->calc_latency;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] serial: 8250_omap: Remove auto-IXON flow control
OMAP h/w-assisted IXON flow control is borked. The transmitter becomes
stuck if XON is never received; clearing the fifos or resetting the
rx flow control bits has no effect.
Remove auto-IXANY as well, since without auto-IXON, it has no purpose.
Signed-off-by: Peter Hurley
---
drivers/tty/serial/8250/8250_omap.c | 16 ++--
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 9782043..7e58750 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -427,12 +427,9 @@ static void omap_8250_set_termios(struct uart_port *port,
priv->efr |= UART_EFR_CTS;
} else if (up->port.flags & UPF_SOFT_FLOW) {
/*
-* IXON Flag:
-* Enable XON/XOFF flow control on input.
-* Receiver compares XON1, XOFF1.
+* OMAP rx s/w flow control is borked; the transmitter remains
+* stuck off even if rx flow control is subsequently disabled
*/
- if (termios->c_iflag & IXON)
- priv->efr |= OMAP_UART_SW_RX;
/*
* IXOFF Flag:
@@ -443,15 +440,6 @@ static void omap_8250_set_termios(struct uart_port *port,
up->port.status |= UPSTAT_AUTOXOFF;
priv->efr |= OMAP_UART_SW_TX;
}
-
- /*
-* IXANY Flag:
-* Enable any character to restart output.
-* Operation resumes after receiving any
-* character after recognition of the XOFF character
-*/
- if (termios->c_iflag & IXANY)
- up->mcr |= UART_MCR_XONANY;
}
omap8250_restore_regs(up);
--
2.4.5
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] serial: 8250_omap: provide complete custom startup & shutdown callbacks
On 05/20/2015 04:07 PM, Sebastian Andrzej Siewior wrote:
> The currently in-use port->startup and port->shutdown are "okay". The
> startup part for instance does the tiny omap extra part and invokes
> serial8250_do_startup() for the remaining pieces. The workflow in
> serial8250_do_startup() is okay except for the part where UART_RX is
> read without a check if there is something to read. I tried to
> workaround it in commit 0aa525d11859 ("tty: serial: 8250_core: read only
> RX if there is something in the FIFO") but then reverted it later in
> commit ca8bb4aefb9 ("serial: 8250: Revert "tty: serial: 8250_core: read
> only RX if there is something in the FIFO"").
>
> This is the second attempt to get it to work on older OMAPs without
> breaking other chips this time
> Peter Hurley suggested to pull in the few needed lines from
> serial8250_do_startup() and drop everything else that is not required
> including making it simpler like using just request_irq() instead the
> chain handler like it is doing now.
> So lets try that.
Thanks, Sebastian.
Reviewed-by: Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] am437x-gp-evm: add wilink8 support
nmux_wlan_pins_sleep {
> + pinctrl-single,pins = <
> + 0x50 (PIN_OUTPUT_PULLDOWN | MUX_MODE7) /*
> gpmc_a4.gpio1_20 WL_EN */
> + 0x5c (PIN_INPUT | WAKEUP_ENABLE | MUX_MODE7)/*
> gpmc_a7.gpio1_23 WL_IRQ*/
> + 0x40 (PIN_OUTPUT_PULLUP | MUX_MODE7)/*
> gpmc_a0.gpio1_16 BT_EN*/
> + >;
> + };
> +
> + uart3_pins: uart3_pins {
> + pinctrl-single,pins = <
> + 0x228 (PIN_INPUT | MUX_MODE0) /*
> uart3_rxd.uart3_rxd */
> + 0x22c (PIN_OUTPUT_PULLDOWN | MUX_MODE0) /*
> uart3_txd.uart3_txd */
> + 0x230 (PIN_INPUT_PULLUP | MUX_MODE0)/*
> uart3_ctsn.uart3_ctsn */
> + 0x234 (PIN_OUTPUT_PULLDOWN | MUX_MODE0) /*
> uart3_rtsn.uart3_rtsn */
> + >;
> + };
> };
>
> &i2c0 {
> @@ -446,6 +519,10 @@
> status = "okay";
> };
>
> +&gpio1 {
> + status = "okay";
> +};
> +
> &gpio3 {
> status = "okay";
> };
> @@ -468,6 +545,43 @@
> cd-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
> };
>
> +&mmc3 {
> + status = "okay";
> + /* these are on the crossbar and are outlined in the
> +xbar-event-map element */
> + dmas = <&edma 30
> + &edma 31>;
> + dma-names = "tx", "rx";
> + vmmc-supply = <&vmmcwl_fixed>;
> + bus-width = <4>;
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&mmc3_pins_default>;
> + pinctrl-1 = <&mmc3_pins_sleep>;
So the wilink8 module is the only thing that can be plugged
into the camera header?
I think all this really needs to go into a DT overlay.
Regards,
Peter Hurley
> + cap-power-off-card;
> + keep-power-in-suspend;
> + ti,non-removable;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + wlcore: wlcore@0 {
> + compatible = "ti,wl1835";
> + reg = <2>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +};
> +
> +&edma {
> + ti,edma-xbar-event-map = /bits/ 16 <1 30
> + 2 31>;
> +};
> +
> +&uart3 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart3_pins>;
> +};
> +
> &usb2_phy1 {
> status = "okay";
> };
> diff --git a/include/dt-bindings/pinctrl/am43xx.h
> b/include/dt-bindings/pinctrl/am43xx.h
> index 5f4d0189..b00bbc9 100644
> --- a/include/dt-bindings/pinctrl/am43xx.h
> +++ b/include/dt-bindings/pinctrl/am43xx.h
> @@ -21,6 +21,7 @@
> #define SLEWCTRL_SLOW(1 << 19)
> #define SLEWCTRL_FAST0
> #define DS0_PULL_UP_DOWN_EN (1 << 27)
> +#define WAKEUP_ENABLE(1 << 29)
>
> #define PIN_OUTPUT (PULL_DISABLE)
> #define PIN_OUTPUT_PULLUP(PULL_UP)
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] ARM: omap2plus_defconfig: Switch over to using 8250 driver
On 04/11/2015 02:27 PM, Nishanth Menon wrote: > On 04/10/2015 02:40 PM, Peter Hurley wrote: >> [ +Sebastian, +Tony ] >> >> On 04/10/2015 02:18 PM, Nishanth Menon wrote: >>> 8250 driver should be relatively feature complete. It can co-exist >>> with omap-serial driver >> >> Not really; if the omap_8250 is selected then it is probed first >> and will be the only driver claiming omap UART ports. >> >> omap-serial would just be dead-weight. > > true.. my bad.. > >> >>> , so just enable 8250 OMAP layer driver and >>> route all ttyOx references to ttySx through the standard 8250 driver >>> to ensure no breakage of userspace occurs. >> >> Not quite; the only automatic handling is for console only, not for >> userspace. Expect lots of userspace breakage. >> > > > Yep - overall, looking through individual logs, in my testing, it seems > to work at least for console for all platforms - even though the > filesystems are mostly going bonkers -> older fs did not have the udev > redirection to take care of this - mostly to do with the getty hooked on > to static consoles. > > > There are infact two issues: > a) bootloader change for cmdline -> O2 to S2 -> in many cases we are > lesser inclined to change the bootloader, hence the fixup configuration Just an FYI - support for handling of _any_ console command line string by _any_ driver was accepted for 4.01; the console can declare a match() function which will be called at registration time for every console command line, and can opt to perform console setup using any criteria. This provides a migration path for _any_ driver (and also allow any earlycon-to-console handoff for non-8250 drivers by using a defined match() function to match the appropriate earlycon= command line; the particulars are in the univ8250_console_match() kernel-doc header in -next). > b) fs changes - these are sometimes more realistic to do, but is a clear > breakage risk. > > Overall, keeping two equally functional drivers in the system sounds a > bunch of maintenance burden for all of us and not to mention confusion > for the future. I think for the moment we should just freeze omap-serial and let most of userspace catch up first; a lot of the official and unofficial vender support is still stuck in 3.14. By the time, 3.19+ is de rigueur we'll hopefully have figured out the ttyS sharing and how to migrate without breaking userspace. > Btw, I am not exactly proposing this for 4.1 kernel.. :) I knew that. > instead, we should > probably discuss how to best introduce this in and throw out the older > omap_serial driver - just reuse 8250 and co-exist with the rest of the > good world folks ;).. I agree -- thanks for bringing the topic up. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] ARM: omap2plus_defconfig: Switch over to using 8250 driver
[ +Sebastian, +Tony ] On 04/10/2015 02:18 PM, Nishanth Menon wrote: > 8250 driver should be relatively feature complete. It can co-exist > with omap-serial driver Not really; if the omap_8250 is selected then it is probed first and will be the only driver claiming omap UART ports. omap-serial would just be dead-weight. > , so just enable 8250 OMAP layer driver and > route all ttyOx references to ttySx through the standard 8250 driver > to ensure no breakage of userspace occurs. Not quite; the only automatic handling is for console only, not for userspace. Expect lots of userspace breakage. Regards, Peter Hurley > Signed-off-by: Nishanth Menon > --- > Current upstream next-20150410 status: (all boards pass without this patch) > Test is a basic boot test (using omap2plus_defconfig ofcourse).. > > Ofcourse: > [0.001035] WARNING: Your 'console=ttyO0' has been replaced by 'ttyS0' > does not help userspace when they are not able to dynamically handle switch. > > Just curious if folks feel we are ready for this switch yet... > > ttyS-change > 1: am335x-evm: BOOT: PASS: > http://paste.ubuntu.org.cn/2551733 > 2: am335x-sk: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551734 > 3: am3517-evm: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551735 > 4: am37x-evm: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551736 > 5: am437x-sk: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551737 > 6:am43xx-epos: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551738 > 7: am43xx-gpevm: BOOT: PASS: > http://paste.ubuntu.org.cn/2551739 > 8: BeagleBoard-XM: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551740 > 9:beagleboard-vanilla: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551741 > 10: beaglebone-black: BOOT: PASS: > http://paste.ubuntu.org.cn/2551742 > 11: beaglebone: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551743 > 12: craneboard: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551744 > 13: dra72x-evm: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551745 > 14: dra7xx-evm: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551746 > 15: OMAP3430-Labrador(LDP): BOOT: FAIL: > http://paste.ubuntu.org.cn/2551747 > 16: n900: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551748 > 17: omap5-evm: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551749 > 18: pandaboard-es: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551750 > 19: pandaboard-vanilla: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551751 > 20:sdp2430: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551752 > 21:sdp3430: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551753 > 22:sdp4430: BOOT: FAIL: > http://paste.ubuntu.org.cn/2551754 > TOTAL = 22 boards, Booted Boards = 3, No Boot boards = 19 > > arc/arm/configs/omap2plus_defconfig |2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm/configs/omap2plus_defconfig > b/arch/arm/configs/omap2plus_defconfig > index 9ff7b54b2a83..6ef76856ac8e 100644 > --- a/arch/arm/configs/omap2plus_defconfig > +++ b/arch/arm/configs/omap2plus_defconfig > @@ -220,6 +220,8 @@ CONFIG_SERIAL_8250_MANY_PORTS=y > CONFIG_SERIAL_8250_SHARE_IRQ=y > CONFIG_SERIAL_8250_DETECT_IRQ=y > CONFIG_SERIAL_8250_RSA=y > +CONFIG_SERIAL_8250_OMAP=y > +CONFIG_SERIAL_8250_OMAP_TTYO_FIXUP=y > CONFIG_SERIAL_OF_PLATFORM=y > CONFIG_SERIAL_OMAP=y > CONFIG_SERIAL_OMAP_CONSOLE=y > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/16] tty: serial: 8250_core: read only RX if there is something in the FIFO
On 02/12/2015 02:23 PM, Sebastian Andrzej Siewior wrote:
> * Peter Hurley | 2015-02-12 11:32:04 [-0500]:
>
>> That said, I don't think serial8250_do_startup() is really doing that much
>> for OMAP h/w startup; open-coding what omap_8250 really needs is probably
>> < 10 loc.
>
> 10 loc? I have a few more.
:)
> serial8250_clear_fifos(),
> serial_link_irq_chain() aren't exported. serial8250_set_mctrl() can
> maybe accessed via uart_ops->set_mctrl(). Maybe I'm not removing the
> obvious not required code but here it looks better to just a BUG flag for
> the Omap.
Ok.
FWIW,
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -557,9 +557,74 @@ static int omap_8250_startup(struct uart_port *port)
>
> pm_runtime_get_sync(port->dev);
>
> - ret = serial8250_do_startup(port);
> - if (ret)
> - goto err;
> + up->mcr = 0;
> +
> + /*
> + * Clear the FIFO buffers and disable them.
> + * (they will be reenabled in set_termios())
> + */
> + serial8250_clear_fifos(up);
For omap this would just be:
serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
In any event, the fifo enable/disable is probably not happening since
FIFO_EN is ignored unless the divisor == 0.
> +
> + /*
> + * Clear the interrupt registers.
> + */
> + if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
> + serial_port_in(port, UART_RX);
> + serial_port_in(port, UART_IIR);
> + serial_port_in(port, UART_MSR);
> +
> + retval = serial_link_irq_chain(up);
> + if (retval)
> + goto out;
omap doesn't really need the legacy irq chain handling; this could just be
request_irq().
In the 8250 split I'll be posting soon, all the irq chaining and
polling-via-timeout workarounds stays in the universal/legacy driver so
other 8250 drivers can opt-out.
> +
> + /*
> + * Now, initialize the UART
> + */
> + serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
> +
--- >% ---
> + spin_lock_irqsave(&port->lock, flags);
> + /*
> + * Most PC uarts need OUT2 raised to enable interrupts.
> + */
> + if (port->irq)
> + up->port.mctrl |= TIOCM_OUT2;
> +
> + serial8250_set_mctrl(port, port->mctrl);
> +
> + spin_unlock_irqrestore(&port->lock, flags);
> +
None of this is required because there is no OUT2 on omap.
-
--- >%
> + /*
> + * Clear the interrupt registers again for luck, and clear the
> + * saved flags to avoid getting false values from polling
> + * routines or the previous session.
> + */
> + if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
> + serial_port_in(port, UART_RX);
> + serial_port_in(port, UART_IIR);
> + serial_port_in(port, UART_MSR);
None of this is required because none of the probing is taking place.
> + up->lsr_saved_flags = 0;
> + up->msr_saved_flags = 0;
> +
> + /*
> + * Request DMA channels for both RX and TX.
> + */
> + if (up->dma) {
> + retval = serial8250_request_dma(up);
> + if (retval) {
> + pr_warn_ratelimited("ttyS%d - failed to request DMA\n",
> + serial_index(port));
> + up->dma = NULL;
> + }
> + }
> +
> + /*
> + * Finally, enable interrupts. Note: Modem status interrupts
> + * are set via set_termios(), which will be occurring imminently
> + * anyway, so we don't enable them here.
> + */
> + up->ier = UART_IER_RLSI | UART_IER_RDI;
> + serial_port_out(port, UART_IER, up->ier);
> +
>
> #ifdef CONFIG_PM
> up->capabilities |= UART_CAP_RPM;
>
> Sebastian
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/16] tty: serial: 8250_core: read only RX if there is something in the FIFO
On 02/12/2015 03:45 AM, Sebastian Andrzej Siewior wrote: > On 02/11/2015 09:42 PM, Peter Hurley wrote: > >>> Reverting makes sense to me if it has caused a regression. Maybe Sebastian >>> can update his patch to do this based on some quirk flag instead? >> >> That's fine with me. There's a 'bugs' field in struct 8250_uart_port and >> UART_BUG_* defines in 8250/8250.h for that purpose. > > Makes sense. Reading an empty FIFO does not look right. Maybe we should > do the bug flag the other way around? But I can do what I am told to so > if there is more fallout than just this Marvell UART I could come > around with a patch to the bug field for the older OMAP. I agree with Russell on this; better to stick with the rx read that's been running on 20 years of hardware. That said, I don't think serial8250_do_startup() is really doing that much for OMAP h/w startup; open-coding what omap_8250 really needs is probably < 10 loc. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/16] tty: serial: 8250_core: read only RX if there is something in the FIFO
On 02/11/2015 03:03 PM, Tony Lindgren wrote: > * Peter Hurley [150211 12:05]: >> On 02/10/2015 12:46 PM, Peter Hurley wrote: >>> On 02/10/2015 07:04 AM, Nicolas Schichan wrote: >>>> On 02/10/2015 12:34 AM, Peter Hurley wrote: >>>>> Hi Nicolas, >>>>> >>>>> Thanks for the report. >>>>> >>>> [...] >>>>>> When a caracter is received on the UART while the kernel is printing >>>>>> the boot messages, as soon as the kernel configures the UART for >>>>>> receiving (after root filesystem mount), it gets stuck printing the >>>>>> following message repeatedly: >>>>>> >>>>>> serial8250: too much work for irq29 >>>>>> >>>>>> Once stuck, the reception of another character allows the boot process >>>>>> to finish. >>>>>> >>>>>> From what I can gather, when we hit that, the UART_IIR_NO_INT is 0 (so >>>>>> the >>>>>> interrupt is raised), but the UART_LSR_DR bit is 0 as well so the UART_RX >>>>>> register is never read to clear the interrupt. >>>>> >>>>> The "too much work" message means serial8250_handle_irq() is returning 0, >>>>> ie., not handled. Which in turn means IIR indicates no interrupt is >>>>> pending >>>>> (UART_IIR_NO_INT == 1). >>>>> >>>>> Can you log the register values for LSR and IIR at both patch locations >>>>> in serial8250_do_startup()? >>>>> >>>>> (I can get you a debug patch, if necessary. Let me know) >>>> >>>> Hi Peter, >>>> >>>> Thanks for your reply. >>>> >>>> Here is what I have when the issue is triggered: >>>> >>>> [ 12.154877] lsr 0x60 / iir 0x01 >>>> [ 12.158071] lsr 0x60 / iir 0x01 >>>> [ 12.161438] serial8250: too much work for irq29 >>>> [ 12.165982] lsr 0x60 / iir 0x0c >>>> [ 12.169354] serial8250: too much work for irq29 >>>> [ 12.173900] lsr 0x60 / iir 0x0c >>>> (previous two messages are repeated and printk_ratelimited()) >>> >>> Thanks for this information; I see I was wrong about the cause of message. >>> >>> I think what happens during startup is that on this silicon clearing >>> the rx fifo (by serial8250_clear_fifos()) clears data ready but not >>> the rx timeout condition which causes a spurious rx interrupt when >>> interrupts are enabled. >>> >>> So caught between two broken UARTs: one that underflows its rx fifo because >>> of unsolicited rx reads and the other that generates spurious interrupt >>> without unsolicited rx reads. >>> >>> >>>> When the issue is not triggered: >>>> >>>> [ 10.784871] lsr 0x60 / iir 0x01 >>>> [ 10.788066] lsr 0x60 / iir 0x01 >>>> [ 10.794734] VFS: Mounted root (nfs filesystem) readonly on device 0:13. >>>> [ 10.801654] devtmpfs: mounted >>>> [ 10.805169] Freeing unused kernel memory: 184K (807be000 - 807ec000) >>>> (userland takes over after that) >>>> >>>> I have also displayed the IIR and LSR registers when the "too much fork for >>>> IRQ" condition is triggered. >>>> >>>> In the serial8250_do_startup(), before the interrupt are unmasked at the >>>> end, >>>> the IIR looks sane and UART_IIR_NO_INT bit is set. When stuck >>>> serial8250_interrupt(), UART_IIR_NO_INT is cleared and the interrupt ID is >>>> set >>>> to 0xc which is not handled by the kernel at this time (the Kirkwood >>>> datasheet >>>> indicates that it is some kind of timeout condition from what I can >>>> gather). >>> >>> Yes, IIR == UART_IIR_RX_TIMEOUT is to used indicate that data is in the rx >>> fifo >>> but has not reached the rx trigger level yet. >>> >>> ATM, I'm not exactly sure if there is a safe way to clear the spurious >>> interrupt >>> from the interrupt handler. >>> >>> I'm fairly certain the only way to clear the rx timeout interrupt is to read >>> the rx fifo, but I think this would race with actual data arrival. IOW, >>> there >>> might not be a way to determine if the data read is spurious or not. >> >> Yep, I see no safe way to clear the spurious interrupt [1] and no idea how to >> keep it from happening (other than via the unsolicited RX reads in >> serial8250_do_startup). >> >> Unfortunately, I think this means we'll have to revert Sebastian's commit: >> >> commit 0aa525d11859c1a4d5b78fdc704148e2ae03ae13 >> Author: Sebastian Andrzej Siewior >> Date: Wed Sep 10 21:29:58 2014 +0200 >> >> tty: serial: 8250_core: read only RX if there is something in the FIFO >> >> which just means OMAP3630 will be limited to using the omap_serial driver. > > Reverting makes sense to me if it has caused a regression. Maybe Sebastian > can update his patch to do this based on some quirk flag instead? That's fine with me. There's a 'bugs' field in struct 8250_uart_port and UART_BUG_* defines in 8250/8250.h for that purpose. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/16] tty: serial: 8250_core: read only RX if there is something in the FIFO
On 02/10/2015 12:46 PM, Peter Hurley wrote: > On 02/10/2015 07:04 AM, Nicolas Schichan wrote: >> On 02/10/2015 12:34 AM, Peter Hurley wrote: >>> Hi Nicolas, >>> >>> Thanks for the report. >>> >> [...] >>>> When a caracter is received on the UART while the kernel is printing >>>> the boot messages, as soon as the kernel configures the UART for >>>> receiving (after root filesystem mount), it gets stuck printing the >>>> following message repeatedly: >>>> >>>> serial8250: too much work for irq29 >>>> >>>> Once stuck, the reception of another character allows the boot process >>>> to finish. >>>> >>>> From what I can gather, when we hit that, the UART_IIR_NO_INT is 0 (so the >>>> interrupt is raised), but the UART_LSR_DR bit is 0 as well so the UART_RX >>>> register is never read to clear the interrupt. >>> >>> The "too much work" message means serial8250_handle_irq() is returning 0, >>> ie., not handled. Which in turn means IIR indicates no interrupt is pending >>> (UART_IIR_NO_INT == 1). >>> >>> Can you log the register values for LSR and IIR at both patch locations >>> in serial8250_do_startup()? >>> >>> (I can get you a debug patch, if necessary. Let me know) >> >> Hi Peter, >> >> Thanks for your reply. >> >> Here is what I have when the issue is triggered: >> >> [ 12.154877] lsr 0x60 / iir 0x01 >> [ 12.158071] lsr 0x60 / iir 0x01 >> [ 12.161438] serial8250: too much work for irq29 >> [ 12.165982] lsr 0x60 / iir 0x0c >> [ 12.169354] serial8250: too much work for irq29 >> [ 12.173900] lsr 0x60 / iir 0x0c >> (previous two messages are repeated and printk_ratelimited()) > > Thanks for this information; I see I was wrong about the cause of message. > > I think what happens during startup is that on this silicon clearing > the rx fifo (by serial8250_clear_fifos()) clears data ready but not > the rx timeout condition which causes a spurious rx interrupt when > interrupts are enabled. > > So caught between two broken UARTs: one that underflows its rx fifo because > of unsolicited rx reads and the other that generates spurious interrupt > without unsolicited rx reads. > > >> When the issue is not triggered: >> >> [ 10.784871] lsr 0x60 / iir 0x01 >> [ 10.788066] lsr 0x60 / iir 0x01 >> [ 10.794734] VFS: Mounted root (nfs filesystem) readonly on device 0:13. >> [ 10.801654] devtmpfs: mounted >> [ 10.805169] Freeing unused kernel memory: 184K (807be000 - 807ec000) >> (userland takes over after that) >> >> I have also displayed the IIR and LSR registers when the "too much fork for >> IRQ" condition is triggered. >> >> In the serial8250_do_startup(), before the interrupt are unmasked at the end, >> the IIR looks sane and UART_IIR_NO_INT bit is set. When stuck >> serial8250_interrupt(), UART_IIR_NO_INT is cleared and the interrupt ID is >> set >> to 0xc which is not handled by the kernel at this time (the Kirkwood >> datasheet >> indicates that it is some kind of timeout condition from what I can gather). > > Yes, IIR == UART_IIR_RX_TIMEOUT is to used indicate that data is in the rx > fifo > but has not reached the rx trigger level yet. > > ATM, I'm not exactly sure if there is a safe way to clear the spurious > interrupt > from the interrupt handler. > > I'm fairly certain the only way to clear the rx timeout interrupt is to read > the rx fifo, but I think this would race with actual data arrival. IOW, there > might not be a way to determine if the data read is spurious or not. Yep, I see no safe way to clear the spurious interrupt [1] and no idea how to keep it from happening (other than via the unsolicited RX reads in serial8250_do_startup). Unfortunately, I think this means we'll have to revert Sebastian's commit: commit 0aa525d11859c1a4d5b78fdc704148e2ae03ae13 Author: Sebastian Andrzej Siewior Date: Wed Sep 10 21:29:58 2014 +0200 tty: serial: 8250_core: read only RX if there is something in the FIFO which just means OMAP3630 will be limited to using the omap_serial driver. Regards, Peter Hurley [1] To clear the RX timeout interrupt requires reading the rx fifo even though LSR[data ready] indicates no data. However, this could result in dropped data if the data became available just before clearing the RX timeout. For example, CPU | Device | irq handler (simplified)|
Re: [PATCH 03/16] tty: serial: 8250_core: read only RX if there is something in the FIFO
On 02/10/2015 07:04 AM, Nicolas Schichan wrote:
> On 02/10/2015 12:34 AM, Peter Hurley wrote:
>> Hi Nicolas,
>>
>> Thanks for the report.
>>
> [...]
>>> When a caracter is received on the UART while the kernel is printing
>>> the boot messages, as soon as the kernel configures the UART for
>>> receiving (after root filesystem mount), it gets stuck printing the
>>> following message repeatedly:
>>>
>>> serial8250: too much work for irq29
>>>
>>> Once stuck, the reception of another character allows the boot process
>>> to finish.
>>>
>>> From what I can gather, when we hit that, the UART_IIR_NO_INT is 0 (so the
>>> interrupt is raised), but the UART_LSR_DR bit is 0 as well so the UART_RX
>>> register is never read to clear the interrupt.
>>
>> The "too much work" message means serial8250_handle_irq() is returning 0,
>> ie., not handled. Which in turn means IIR indicates no interrupt is pending
>> (UART_IIR_NO_INT == 1).
>>
>> Can you log the register values for LSR and IIR at both patch locations
>> in serial8250_do_startup()?
>>
>> (I can get you a debug patch, if necessary. Let me know)
>
> Hi Peter,
>
> Thanks for your reply.
>
> Here is what I have when the issue is triggered:
>
> [ 12.154877] lsr 0x60 / iir 0x01
> [ 12.158071] lsr 0x60 / iir 0x01
> [ 12.161438] serial8250: too much work for irq29
> [ 12.165982] lsr 0x60 / iir 0x0c
> [ 12.169354] serial8250: too much work for irq29
> [ 12.173900] lsr 0x60 / iir 0x0c
> (previous two messages are repeated and printk_ratelimited())
Thanks for this information; I see I was wrong about the cause of message.
I think what happens during startup is that on this silicon clearing
the rx fifo (by serial8250_clear_fifos()) clears data ready but not
the rx timeout condition which causes a spurious rx interrupt when
interrupts are enabled.
So caught between two broken UARTs: one that underflows its rx fifo because
of unsolicited rx reads and the other that generates spurious interrupt
without unsolicited rx reads.
> When the issue is not triggered:
>
> [ 10.784871] lsr 0x60 / iir 0x01
> [ 10.788066] lsr 0x60 / iir 0x01
> [ 10.794734] VFS: Mounted root (nfs filesystem) readonly on device 0:13.
> [ 10.801654] devtmpfs: mounted
> [ 10.805169] Freeing unused kernel memory: 184K (807be000 - 807ec000)
> (userland takes over after that)
>
> I have also displayed the IIR and LSR registers when the "too much fork for
> IRQ" condition is triggered.
>
> In the serial8250_do_startup(), before the interrupt are unmasked at the end,
> the IIR looks sane and UART_IIR_NO_INT bit is set. When stuck
> serial8250_interrupt(), UART_IIR_NO_INT is cleared and the interrupt ID is set
> to 0xc which is not handled by the kernel at this time (the Kirkwood datasheet
> indicates that it is some kind of timeout condition from what I can gather).
Yes, IIR == UART_IIR_RX_TIMEOUT is to used indicate that data is in the rx fifo
but has not reached the rx trigger level yet.
ATM, I'm not exactly sure if there is a safe way to clear the spurious interrupt
from the interrupt handler.
I'm fairly certain the only way to clear the rx timeout interrupt is to read
the rx fifo, but I think this would race with actual data arrival. IOW, there
might not be a way to determine if the data read is spurious or not.
Regards,
Peter Hurley
> Here is the corresponding debug patch, for reference (against a 3.19 kernel
> this time):
>
> diff --git a/drivers/tty/serial/8250/8250_core.c
> b/drivers/tty/serial/8250/8250_
> index 11c6685..ed93741 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1661,6 +1661,12 @@ static int exar_handle_irq(struct uart_port *port)
> return ret;
> }
>
> +static inline void show_lsr_iir(struct uart_port *port)
> +{
> + printk("lsr 0x%02x / iir 0x%02x\n", serial_port_in(port, UART_LSR),
> + serial_port_in(port, UART_IIR));
> +}
> +
> /*
> * This is the serial driver's interrupt routine.
> *
> @@ -1705,6 +1711,8 @@ static irqreturn_t serial8250_interrupt(int irq, void
> *dev
> /* If we hit this, we're dead. */
> printk_ratelimited(KERN_ERR
> "serial8250: too much work for irq%d\n", irq);
> + if (printk_ratelimit())
> + show_lsr_iir(port);
> break;
> }
> } while (l != end);
> @@ -2107,6 +2115,7 @@ int serial8250_do_startup(struct uart_port *port)
Re: [PATCH 03/16] tty: serial: 8250_core: read only RX if there is something in the FIFO
Hi Nicolas, Thanks for the report. On 02/09/2015 08:34 AM, Nicolas Schichan wrote: > On 09/10/2014 09:29 PM, Sebastian Andrzej Siewior wrote: >> The serial8250_do_startup() function unconditionally clears the >> interrupts and for that it reads from the RX-FIFO without checking if >> there is a byte in the FIFO or not. This works fine on OMAP4+ HW like >> AM335x or DRA7. >> OMAP3630 ES1.1 (which means probably all OMAP3 and earlier) does not like >> this: > > Hello, > > Sorry to wake up an old thread, but I'm affraid that this patch causes > problems on Marvell 88f6282 (Kirkwood). > > When a caracter is received on the UART while the kernel is printing > the boot messages, as soon as the kernel configures the UART for > receiving (after root filesystem mount), it gets stuck printing the > following message repeatedly: > > serial8250: too much work for irq29 > > Once stuck, the reception of another character allows the boot process > to finish. > > From what I can gather, when we hit that, the UART_IIR_NO_INT is 0 (so the > interrupt is raised), but the UART_LSR_DR bit is 0 as well so the UART_RX > register is never read to clear the interrupt. The "too much work" message means serial8250_handle_irq() is returning 0, ie., not handled. Which in turn means IIR indicates no interrupt is pending (UART_IIR_NO_INT == 1). Can you log the register values for LSR and IIR at both patch locations in serial8250_do_startup()? (I can get you a debug patch, if necessary. Let me know) Regards, Peter Hurley > We are using the second UART multiplexed on mpps 15 and 16. > > Reverting this particular patch fixes the issue. > > We are seing the problem on a 3.18 kernel. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] tty: Remove external interface for tty_set_termios()
tty_set_termios() is an internal helper intended for file scope use.
UART drivers which are capable of driving the RTS pin must
properly handle the tiocmset() method, regardless of termios settings.
A failure to do so is a UART driver bug and should be fixed there.
Do not use this interface to workaround UART driver bugs.
Cc: Marcel Holtmann
Cc: Johan Hedberg
Cc:
Signed-off-by: Peter Hurley
---
drivers/bluetooth/hci_ath.c | 15 ++-
drivers/tty/tty_ioctl.c | 3 +--
include/linux/tty.h | 1 -
3 files changed, 3 insertions(+), 16 deletions(-)
diff --git a/drivers/bluetooth/hci_ath.c b/drivers/bluetooth/hci_ath.c
index 2ff6dfd..9c4dcf4 100644
--- a/drivers/bluetooth/hci_ath.c
+++ b/drivers/bluetooth/hci_ath.c
@@ -51,33 +51,22 @@ struct ath_struct {
static int ath_wakeup_ar3k(struct tty_struct *tty)
{
- struct ktermios ktermios;
int status = tty->driver->ops->tiocmget(tty);
if (status & TIOCM_CTS)
return status;
- /* Disable Automatic RTSCTS */
- ktermios = tty->termios;
- ktermios.c_cflag &= ~CRTSCTS;
- tty_set_termios(tty, &ktermios);
-
/* Clear RTS first */
- status = tty->driver->ops->tiocmget(tty);
+ tty->driver->ops->tiocmget(tty);
tty->driver->ops->tiocmset(tty, 0x00, TIOCM_RTS);
mdelay(20);
/* Set RTS, wake up board */
- status = tty->driver->ops->tiocmget(tty);
+ tty->driver->ops->tiocmget(tty);
tty->driver->ops->tiocmset(tty, TIOCM_RTS, 0x00);
mdelay(20);
status = tty->driver->ops->tiocmget(tty);
-
- /* Enable Automatic RTSCTS */
- ktermios.c_cflag |= CRTSCTS;
- status = tty_set_termios(tty, &ktermios);
-
return status;
}
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index d73f695..69fc0b8 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -533,7 +533,7 @@ EXPORT_SYMBOL(tty_termios_hw_change);
* Locking: termios_rwsem
*/
-int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
+static int tty_set_termios(struct tty_struct *tty, struct ktermios
*new_termios)
{
struct ktermios old_termios;
struct tty_ldisc *ld;
@@ -566,7 +566,6 @@ int tty_set_termios(struct tty_struct *tty, struct ktermios
*new_termios)
up_write(&tty->termios_rwsem);
return 0;
}
-EXPORT_SYMBOL_GPL(tty_set_termios);
/**
* set_termios - set termios values for a tty
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 0992238..8c316b6 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -491,7 +491,6 @@ static inline speed_t tty_get_baud_rate(struct tty_struct
*tty)
extern void tty_termios_copy_hw(struct ktermios *new, struct ktermios *old);
extern int tty_termios_hw_change(struct ktermios *a, struct ktermios *b);
-extern int tty_set_termios(struct tty_struct *tty, struct ktermios *kt);
extern struct tty_ldisc *tty_ldisc_ref(struct tty_struct *);
extern void tty_ldisc_deref(struct tty_ldisc *);
--
2.2.2
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] serial: omap: Fix RTS handling
The OMAP UART ignores MCR[1] (ie., RTS) when in autoRTS mode. This
makes it impossible for either the serial core or userspace to
manually flow control the sender.
Disable autoRTS mode when RTS is lowered and restore the previous
mode when RTS is raised.
Signed-off-by: Peter Hurley
---
drivers/tty/serial/omap-serial.c | 22 ++
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index b28cdca..e01b65e 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -681,7 +681,7 @@ static unsigned int serial_omap_get_mctrl(struct uart_port
*port)
static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
{
struct uart_omap_port *up = to_uart_omap_port(port);
- unsigned char mcr = 0, old_mcr;
+ unsigned char mcr = 0, old_mcr, lcr;
dev_dbg(up->port.dev, "serial_omap_set_mctrl+%d\n", up->port.line);
if (mctrl & TIOCM_RTS)
@@ -701,6 +701,17 @@ static void serial_omap_set_mctrl(struct uart_port *port,
unsigned int mctrl)
UART_MCR_DTR | UART_MCR_RTS);
up->mcr = old_mcr | mcr;
serial_out(up, UART_MCR, up->mcr);
+
+ /* Turn off autoRTS if RTS is lowered; restore autoRTS if RTS raised */
+ lcr = serial_in(up, UART_LCR);
+ serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+ if ((mctrl & TIOCM_RTS) && (port->status & UPSTAT_AUTORTS))
+ up->efr |= UART_EFR_RTS;
+ else
+ up->efr &= UART_EFR_RTS;
+ serial_out(up, UART_EFR, up->efr);
+ serial_out(up, UART_LCR, lcr);
+
pm_runtime_mark_last_busy(up->dev);
pm_runtime_put_autosuspend(up->dev);
}
@@ -756,8 +767,6 @@ static int serial_omap_startup(struct uart_port *port)
* (they will be reenabled in set_termios())
*/
serial_omap_clear_fifos(up);
- /* For Hardware flow control */
- serial_out(up, UART_MCR, UART_MCR_RTS);
/*
* Clear the interrupt registers.
@@ -1056,12 +1065,9 @@ serial_omap_set_termios(struct uart_port *port, struct
ktermios *termios,
up->port.status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS | UPSTAT_AUTOXOFF);
if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW) {
- /* Enable AUTORTS and AUTOCTS */
+ /* Enable AUTOCTS (autoRTS is enabled when RTS is raised) */
up->port.status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS;
- up->efr |= UART_EFR_CTS | UART_EFR_RTS;
-
- /* Ensure MCR RTS is asserted */
- up->mcr |= UART_MCR_RTS;
+ up->efr |= UART_EFR_CTS;
} else {
/* Disable AUTORTS and AUTOCTS */
up->efr &= ~(UART_EFR_CTS | UART_EFR_RTS);
--
2.2.2
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] serial: 8250_omap: Use UPSTAT_AUTORTS for RTS handling
Commit 88838d3112702 ("serial: omap_8250: Fix RTS handling") fixed
RTS pin control when in autoRTS mode.
New support added in "serial: core: Rework hw-assisted flow control support"
enables a much simpler approach; rather than masking out autoRTS
whenever writing the EFR register, use the UPSTAT_* mode to determine if
autoRTS should be enabled when raising RTS (in omap8250_set_mctrl()).
Signed-off-by: Peter Hurley
---
drivers/tty/serial/8250/8250_omap.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 4473881..59d6e90 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -120,10 +120,11 @@ static void omap8250_set_mctrl(struct uart_port *port,
unsigned int mctrl)
*/
lcr = serial_in(up, UART_LCR);
serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
- if (mctrl & TIOCM_RTS)
- serial_out(up, UART_EFR, priv->efr);
+ if ((mctrl & TIOCM_RTS) && (port->status & UPSTAT_AUTORTS))
+ priv->efr |= UART_EFR_RTS;
else
- serial_out(up, UART_EFR, priv->efr & ~UART_EFR_RTS);
+ priv->efr &= ~UART_EFR_RTS;
+ serial_out(up, UART_EFR, priv->efr);
serial_out(up, UART_LCR, lcr);
}
@@ -272,10 +273,7 @@ static void omap8250_restore_regs(struct uart_8250_port
*up)
serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
serial_dl_write(up, priv->quot);
- if (up->port.mctrl & TIOCM_RTS)
- serial_out(up, UART_EFR, priv->efr);
- else
- serial_out(up, UART_EFR, priv->efr & ~UART_EFR_RTS);
+ serial_out(up, UART_EFR, priv->efr);
/* Configure flow control */
serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
@@ -424,9 +422,9 @@ static void omap_8250_set_termios(struct uart_port *port,
up->port.status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS | UPSTAT_AUTOXOFF);
if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW) {
- /* Enable AUTORTS and AUTOCTS */
+ /* Enable AUTOCTS (autoRTS is enabled when RTS is raised) */
up->port.status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS;
- priv->efr |= UART_EFR_CTS | UART_EFR_RTS;
+ priv->efr |= UART_EFR_CTS;
} else if (up->port.flags & UPF_SOFT_FLOW) {
/*
* IXON Flag:
--
2.2.2
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] serial: core: Rework hw-assisted flow control support
hw-assisted flow control support was added to the serial core
in v3.8 with commits,
dba05832cbe4f ("SERIAL: core: add hardware assisted h/w flow control support")
2cbacafd7af0f ("SERIAL: core: add hardware assisted s/w flow control support")
9aba8d5b01119 ("SERIAL: core: add throttle/unthrottle callbacks for hardware
assisted flow control")
Since then, additional requirements for serial core support have arisen.
Specifically,
1. Separate tx and rx flow control settings for UARTs which only support
tx flow control (ie., autoCTS).
2. Disable sw-assisted CTS flow control in autoCTS mode
3. Support for RTS flow control by serial core and userspace in autoRTS mode
Distinguish mode from capability; introduce UPSTAT_AUTORTS, UPSTAT_AUTOCTS
and UPSTAT_AUTOXOFF which, when set by the uart driver, enable serial core
support for hw-assisted rx, hw-assisted tx and hw-assisted in-band/IXOFF
rx flow control, respectively. [Note: hw-assisted in-band/IXON tx flow
control does not require serial core support/intervention and can be
enabled by the uart driver when required.]
These modes must be set/reset in the driver's set_termios() method, based
on termios settings, and thus can be safely queried in any context in which
one of the port lock, port mutex or termios rwsem are held. Set these modes
in the 2 in-tree drivers, omap-serial and 8250_omap, which currently
use UPF_HARD_FLOW/UPF_SOFT_FLOW support.
Retain UPF_HARD_FLOW and UPF_SOFT_FLOW as capabilities; re-define
UPF_HARD_FLOW as both UPF_AUTO_RTS and UPF_AUTO_CTS to allow for distinct
and separate rx and tx flow control capabilities.
Disable sw-assisted CTS flow control when UPSTAT_AUTOCTS is enabled.
Signed-off-by: Peter Hurley
---
drivers/tty/serial/8250/8250_omap.c | 7 +++-
drivers/tty/serial/omap-serial.c| 7 +++-
drivers/tty/serial/serial_core.c| 76 ++---
include/linux/serial_core.h | 21 --
4 files changed, 59 insertions(+), 52 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 316c37b..4473881 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -421,8 +421,11 @@ static void omap_8250_set_termios(struct uart_port *port,
priv->efr = 0;
up->mcr &= ~(UART_MCR_RTS | UART_MCR_XONANY);
+ up->port.status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS | UPSTAT_AUTOXOFF);
+
if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW) {
/* Enable AUTORTS and AUTOCTS */
+ up->port.status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS;
priv->efr |= UART_EFR_CTS | UART_EFR_RTS;
} else if (up->port.flags & UPF_SOFT_FLOW) {
/*
@@ -438,8 +441,10 @@ static void omap_8250_set_termios(struct uart_port *port,
* Enable XON/XOFF flow control on output.
* Transmit XON1, XOFF1
*/
- if (termios->c_iflag & IXOFF)
+ if (termios->c_iflag & IXOFF) {
+ up->port.status |= UPSTAT_AUTOXOFF;
priv->efr |= OMAP_UART_SW_TX;
+ }
/*
* IXANY Flag:
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 2e1073d..b28cdca 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1053,8 +1053,11 @@ serial_omap_set_termios(struct uart_port *port, struct
ktermios *termios,
serial_out(up, UART_TI752_TCR, OMAP_UART_TCR_TRIG);
+ up->port.status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS | UPSTAT_AUTOXOFF);
+
if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW) {
/* Enable AUTORTS and AUTOCTS */
+ up->port.status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS;
up->efr |= UART_EFR_CTS | UART_EFR_RTS;
/* Ensure MCR RTS is asserted */
@@ -1081,8 +1084,10 @@ serial_omap_set_termios(struct uart_port *port, struct
ktermios *termios,
* Enable XON/XOFF flow control on output.
* Transmit XON1, XOFF1
*/
- if (termios->c_iflag & IXOFF)
+ if (termios->c_iflag & IXOFF) {
+ up->port.status |= UPSTAT_AUTOXOFF;
up->efr |= OMAP_UART_SW_TX;
+ }
/*
* IXANY Flag:
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 37dc808..a21b15c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -179,14 +179,6 @@ static int uart_port_startup(struct tty_struct *tty,
struct uart_state *state,
if (tty->termios.c_cflag &
[PATCH 0/4] Rework hw-assisted flow control
Hi Greg, This patch series re-implements hw-assisted flow control support, (aka autoRTS, autoCTS and auto XON/XOFF) in the serial core. Out-of-tree driver maintainers please note this changes the UART driver interface for flow control steering. This series separates mode handling from the capabilities flags. UPF_HARD_FLOW and UPF_SOFT_FLOW continue to indicate the hardware is capable of hw-assisted flow control; in addition, UPF_HARD_FLOW is split into separate capabilities (UPF_AUTO_CTS and UPF_AUTO_RTS) should the driver require the distinction. However, the serial core only checks UPF_SOFT_FLOW (to determine if the UART driver is capable of auto XON/XOFF flow control and thus needs its set_termios() method called). Flow control steering in the serial core is now determined by the uart_port::status field, and the newly introduced UPSTAT_AUTOCTS, UPSTAT_AUTORTS and UPSTAT_AUTOXOFF for autoCTS tx flow control, autoRTS rx flow control and auto IXOFF flow control respectively. [Core support for auto IXON did not seem necessary, as core intervention does not seem required -- please let me know if that's not the case.] Typically, the driver should set or reset these flags in their set_termios() method (in the port lock critical section, if port lock is used), depending on the hardware capabilities and requirement. All drivers which enable hardware autoCTS mode MUST set UPSTAT_AUTOCTS to prevent the serial core from inadvertently disabling tx. In this mode, hw_stopped = 0 and uart_handle_cts_change() perform no action (other than bumping the cts stat for diagnostic purposes). For hardware which does not automatically disable autoRTS mode when RTS is lowered or otherwise desires to implement throttle()/unthrottle() methods, the driver MUST set UPSTAT_AUTORTS. In this mode, the uart driver throttle()/unthrottle() methods are called, rather than lowering RTS via set_mctrl(). UPSTAT_AUTORTS _must not_ be set for drivers which do not implement throttle()/unthrottle() methods; in this case, set_mctrl() is still used to control the RTS pin. Even for drivers using UPSTAT_AUTORTS, the driver is still responsible for properly implementing RTS handling in set_mctrl(), regardless of current termios settings or hardware mode. That is, if the hardware is capable of driving the RTS pin, the driver's set_mctrl() method must lower RTS if !TIOCM_RTS. If that requires disabling autoRTS mode, then that shall be done. The serial core and userspace must be able to manually flow control with RTS. When handling set_mctrl(), the driver may check the uart_port::status UPSTAT_AUTORTS bit to determine if it set autoRTS mode at set_termios(), and thus whether to restore its autoRTS mode when RTS is raised. In other words, the driver does not need to set/check its own private state. All drivers which enable hardware auto IXOFF flow control mode MUST set UPSTAT_AUTOXOFF. As with UPSTAT_AUTORTS, throttle()/unthrottle() methods are required. When this mode is set, the core will _not_ send START/STOP, and expects the driver/hardware to do so. This patch series converts in-tree drivers to this 'new' interface. The last patch rips out a private interface hack being used by a bluetooth host controller driver to tickle RTS for device wakeup; this is no longer required now that the offending in-tree driver, omap-serial, properly drives RTS in autoRTS mode. Regards, Peter Hurley (4): serial: core: Rework hw-assisted flow control support serial: 8250_omap: Use UPSTAT_AUTORTS for RTS handling serial: omap: Fix RTS handling tty: Remove external interface for tty_set_termios() drivers/bluetooth/hci_ath.c | 15 +--- drivers/tty/serial/8250/8250_omap.c | 23 ++- drivers/tty/serial/omap-serial.c| 29 +- drivers/tty/serial/serial_core.c| 76 ++--- drivers/tty/tty_ioctl.c | 3 +- include/linux/serial_core.h | 21 -- include/linux/tty.h | 1 - 7 files changed, 83 insertions(+), 85 deletions(-) -- 2.2.2 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] serial: 8250: Make ISA ports optional
On 01/09/2015 03:48 AM, Arnd Bergmann wrote: > On Friday 09 January 2015 00:13:28 Peter Hurley wrote: >> On 01/08/2015 05:05 PM, Arnd Bergmann wrote: >>> On Thursday 08 January 2015 11:11:51 Peter Hurley wrote: >>>> >>>> This interface is just storage and minor allocation, since the >>>> port-reuse behavior will be limited to the "universal" driver. >>>> From a sub-driver perspective, the shared storage is actually >>>> a hindrance, so that reduces the requirement to minor allocation. >>>> >>>> And that's where I'm stuck at the moment -- how to share ttyS >>>> minor allocation. ttyS console is a related problem. >>> >>> One idea that has come up in the past but never saw an implementation >>> is to make the ttyS namespace and minor numbers completely generic and >>> let any serial port driver use it. This would be a major rework, but >>> have the added advantage of cleaning up a number of other namespace >>> issues as well. There also lots of open question, in particular how >>> to maintain compatibility with existing drivers. One could imagine >>> that each uart always gets a ttyS device and optionally also gets >>> a device node for the same port with a driver specific chardev as >>> most of them do today. Or it could be an either/or decision that is >>> made at compile time or as a module parameter. >> >> I'm not totally convinced that sharing minor allocation is the right >> solution. ISTM that the main goal is for at least one serial driver >> to represent ttyS (assuming that it is a suitable work-alike to the >> 8250 driver) while other ttyS serial drivers must still be able to >> load and operate. > > Unfortunately, we have already seen a case where we need to have > two drivers enabled at the same time that both use the ttyS > namespace and minor numbers at the moment. Maybe you misunderstood me here. I'm suggesting that every ttyS-capable driver would be able to load into a pre-determined, unique alternate namespace. So at least one ttyS driver would be present, without naming conflicts, and every other ttyS-conflicting driver would be loaded in their respective alternate namespace. Unless what you mean is there is some userspace requirement for the ttyS namespace to be shared. If so, would you please expand on this use case so I can better understand the requirements. >> So for example, a UART that is ttyS in a typical configuration, might >> be ttyX when another driver has been configured as ttyS instead. >> Assuming that every uart driver has a unique, alternate base name, >> then the problem is reduced to selecting the appropriate driver for >> ttyS. >> >> Some use cases would require selecting the ttyS driver at build time. >> Others would probably require the notion of a preferred driver. > > Build-time selection looks problematic to me in the case of an > ARM distro kernel that wants to support dozens of different > uart drivers. At the moment this works for the most part because > the conflicts are rare, but it's getting worse. A dt property could indicate which uart is the preferred ttyS. >> What got me thinking about this approach instead of sharing minor >> allocation was based on the 'first-come, first-served' question >> I asked back in Dec. What happens when probe order varies and >> console=ttyS0/login prompt doesn't show up where it's supposed to? >> I've had this experience with multiple ethernet adapters and it's >> not fun. > > In a lot of cases, we can solve the problem for a particular board > by hardcoding the 'aliases' in the board specific dts file, at least > when devicetree is used (as it is for the majority of the cases that > are causing problems now). > > Using the aliases correctly at the moment is really awkward when you > have multiple drivers for uarts on the same system and your serial0 > and serial1 uart turn into ttyS0 and ttyX1. I'm concerned that this will devolve into some attempt at a persistent naming scheme from kernel-space; which has repeatedly been shown in other subsystems to be fruitless. That was my point about the ethernet adapters. I have no problem sequentially numbering uarts as ttyS, but it would suck if that amount of work ultimately proved useless, when udev rules would have sufficed or became necessary anyway. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] serial: 8250: Make ISA ports optional
On 01/08/2015 05:05 PM, Arnd Bergmann wrote: > On Thursday 08 January 2015 11:11:51 Peter Hurley wrote: >> >> This interface is just storage and minor allocation, since the >> port-reuse behavior will be limited to the "universal" driver. >> From a sub-driver perspective, the shared storage is actually >> a hindrance, so that reduces the requirement to minor allocation. >> >> And that's where I'm stuck at the moment -- how to share ttyS >> minor allocation. ttyS console is a related problem. > > One idea that has come up in the past but never saw an implementation > is to make the ttyS namespace and minor numbers completely generic and > let any serial port driver use it. This would be a major rework, but > have the added advantage of cleaning up a number of other namespace > issues as well. There also lots of open question, in particular how > to maintain compatibility with existing drivers. One could imagine > that each uart always gets a ttyS device and optionally also gets > a device node for the same port with a driver specific chardev as > most of them do today. Or it could be an either/or decision that is > made at compile time or as a module parameter. I'm not totally convinced that sharing minor allocation is the right solution. ISTM that the main goal is for at least one serial driver to represent ttyS (assuming that it is a suitable work-alike to the 8250 driver) while other ttyS serial drivers must still be able to load and operate. So for example, a UART that is ttyS in a typical configuration, might be ttyX when another driver has been configured as ttyS instead. Assuming that every uart driver has a unique, alternate base name, then the problem is reduced to selecting the appropriate driver for ttyS. Some use cases would require selecting the ttyS driver at build time. Others would probably require the notion of a preferred driver. What got me thinking about this approach instead of sharing minor allocation was based on the 'first-come, first-served' question I asked back in Dec. What happens when probe order varies and console=ttyS0/login prompt doesn't show up where it's supposed to? I've had this experience with multiple ethernet adapters and it's not fun. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] serial: 8250: Make ISA ports optional
On 01/08/2015 05:36 PM, One Thousand Gnomes wrote:
>> One idea that has come up in the past but never saw an implementation
>> is to make the ttyS namespace and minor numbers completely generic and
>> let any serial port driver use it. This would be a major rework, but
>> have the added advantage of cleaning up a number of other namespace
>> issues as well. There also lots of open question, in particular how
>> to maintain compatibility with existing drivers. One could imagine
>> that each uart always gets a ttyS device and optionally also gets
>> a device node for the same port with a driver specific chardev as
>> most of them do today. Or it could be an either/or decision that is
>> made at compile time or as a module parameter.
>
> I'm not sure we should use ttyS for it. ttyS means 8250, a lot of
> existing code knows what ttyS is and what sort of ioctls and the like are
> assumed.
That genie has already escaped the bottle.
./arch/powerpc/platforms/powermac/setup.c: char *devname = "ttyS";
./arch/powerpc/platforms/chrp/setup.c: add_preferred_console("ttyS",
0, NULL);
./arch/powerpc/kernel/legacy_serial.c: return
add_preferred_console("ttyS", offset, opt);
./arch/xtensa/platforms/iss/console.c: serial_driver->name = "ttyS";
./arch/mips/sni/setup.c:add_preferred_console("ttyS",
port,
./arch/mips/sgi-ip32/ip32-setup.c:
add_preferred_console("ttyS", *(con + 1) == '2' ? 1 : 0,
./arch/mips/jazz/setup.c: add_preferred_console("ttyS", 0, "9600");
./arch/mips/sgi-ip22/ip22-setup.c: add_preferred_console("ttyS",
*(ctype + 1) == '2' ? 1 : 0,
./arch/arm/mach-davinci/board-da830-evm.c: return
add_preferred_console("ttyS", 2, "115200");
./arch/arm/mach-davinci/board-mityomapl138.c: return
add_preferred_console("ttyS", 1, "115200");
./arch/arm/mach-davinci/board-da850-evm.c: return
add_preferred_console("ttyS", 2, "115200");
./arch/arm/mach-davinci/board-omapl138-hawk.c: return
add_preferred_console("ttyS", 2, "115200");
./arch/cris/arch-v10/kernel/debugport.c:name : "ttyS",
./arch/ia64/hp/sim/simserial.c: hp_simserial_driver->name = "ttyS";
./arch/um/drivers/ssl.c:.device_name= "ttyS",
./arch/s390/kernel/setup.c: add_preferred_console("ttyS",
1, NULL);
./drivers/tty/amiserial.c: .name = "ttyS",
./drivers/tty/serial/serial_txx9.c:#define TXX9_TTY_NAME "ttyS"
./drivers/tty/serial/sunsab.c: .dev_name = "ttyS",
./drivers/tty/serial/mcf.c: .dev_name = "ttyS",
./drivers/tty/serial/atmel_serial.c:#define ATMEL_DEVICENAME"ttyS"
./drivers/tty/serial/bcm63xx_uart.c:.dev_name = "ttyS",
./drivers/tty/serial/sunsu.c: .dev_name = "ttyS",
./drivers/tty/serial/mrst_max3110.c:.dev_name = "ttyS",
./drivers/tty/serial/apbuart.c: .name = "ttyS",
./drivers/tty/serial/apbuart.c: .dev_name = "ttyS",
./drivers/tty/serial/tilegx.c:#define TILEGX_UART_NAME "ttyS"
./drivers/tty/serial/pmac_zilog.c:#define PMACZILOG_NAME"ttyS"
./drivers/tty/serial/pnx8xxx_uart.c:.dev_name = "ttyS",
./drivers/tty/serial/m32r_sio.c:.dev_name = "ttyS",
./drivers/tty/serial/ip22zilog.c: .dev_name = "ttyS",
./drivers/tty/serial/zs.c: .dev_name = "ttyS",
./drivers/tty/serial/68328serial.c: serial_driver->name = "ttyS";
./drivers/tty/serial/crisv10.c: driver->name = "ttyS";
/drivers/tty/serial/8250/8250_core.c: .dev_name = "ttyS",
./drivers/tty/serial/sunzilog.c:.dev_name = "ttyS",
./drivers/tty/serial/pxa.c: .dev_name = "ttyS",
./drivers/tty/serial/dz.c: .dev_name = "ttyS",
./drivers/tty/serial/sunhv.c: .dev_name = "ttyS",
./drivers/s390/char/sclp_vt220.c:#define SCLP_VT220_CONSOLE_NAME
"ttyS"
./drivers/s390/char/sclp_con.c:#define sclp_console_name "ttyS"
./drivers/s390/char/con3215.c: driver->name = "ttyS";
Besides, despite its pedigree, omap is no closer to 8250 than anything
else in drivers/tty/serial/
> A generic namespace needs not to be attached to a legacy naming
>
> (eg also put all serial ports in /dev/serial/0 /dev/serial/1 etc either
> via the tty layer or via systemd/udev)
This doesn't address pre-init.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] serial: 8250: Make ISA ports optional
On 01/08/2015 08:10 AM, One Thousand Gnomes wrote: > On Mon, 5 Jan 2015 22:09:45 -0500 > Peter Hurley wrote: > >> Some arches have no need to create unprobed 8250 ports; these phantom >> ports are primarily required for ISA ports which have no probe >> mechanism or to provide non-operational ports for userspace to >> configure (via TIOCSSERIAL and TIOCSERCONFIG ioctls). >> >> Provide CONFIG_SERIAL_8250_PHANTOM_UARTS knob to disable phantom port >> registration; ie., CONFIG_SERIAL_8250_PHANTOM_UARTS=N only registers >> probed ports (ACPI/PNP, "serial8250" platform devices, PCI, etc). > > Just #define serial8250_isa_devs to 0 on such platforms. gcc should then > be bright enough to do the rest for you. Well, this patch has the "advantage" of allowing the user to override the config and create blank ports anyway. That said, I'm exploring a completely different approach: I've split the driver from the port operations. The idea being that the existing "universal" driver supports everything it does now, while allowing for a new driver(s) to only support a subset. At this point, I've only done the actual split. So the "universal" driver module has: * ISA/"serial8250" platform driver * PNP init * uart driver and console definitions * all the existing module parameters * all the globals (except the const uart_config[] table) * RSA port handling * Chained interrupt handling * port i/o by timer handling * early_serial_setup() * sun ttyS shared registration * serial8250_ports[] table and how it re-uses existing ports The "universal" driver also has the sub-driver interface, which is: * serial8250_register_8250_port * serial8250_unregister_port * serial8250_suspend/resume_port * serial8250_find_port (hack for 8250_early) * serial8250_get_port (hack for runtime pm) This interface is just storage and minor allocation, since the port-reuse behavior will be limited to the "universal" driver. >From a sub-driver perspective, the shared storage is actually a hindrance, so that reduces the requirement to minor allocation. And that's where I'm stuck at the moment -- how to share ttyS minor allocation. ttyS console is a related problem. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] serial: 8250: Make ISA ports optional
On 01/06/2015 02:43 PM, Arnd Bergmann wrote: > On Tuesday 06 January 2015 09:32:02 Peter Hurley wrote: >> On 01/06/2015 08:13 AM, Arnd Bergmann wrote: >>> On Monday 05 January 2015 22:09:45 Peter Hurley wrote: >>>> Some arches have no need to create unprobed 8250 ports; these phantom >>>> ports are primarily required for ISA ports which have no probe >>>> mechanism or to provide non-operational ports for userspace to >>>> configure (via TIOCSSERIAL and TIOCSERCONFIG ioctls). >>>> >>>> Provide CONFIG_SERIAL_8250_PHANTOM_UARTS knob to disable phantom port >>>> registration; ie., CONFIG_SERIAL_8250_PHANTOM_UARTS=N only registers >>>> probed ports (ACPI/PNP, "serial8250" platform devices, PCI, etc). >>>> >>>> Cc: Sebastian Andrzej Siewior >>>> Cc: Tony Lindgren >>>> Cc: Grant Likely >>>> Cc: Arnd Bergmann >>>> Signed-off-by: Peter Hurley >>> >>> The intent is definitely right, but I think a better approach is >>> possible. >>> >>> I haven't tried it here, but how about moving the serial8250_init >>> function into a separate module, along with all the other parts >>> that are only used for ISA devices, but leaving the actual core >>> (all exported symbols) in this file? >> >> Unfortunately, I don't see a way to remove the stacked initialization >> without risking tons of breakage. >> >> Since later probes can "find" an already-existing port and >> re-initialize it, the probe order is crucial. For example, a PCI >> probe can "find" an existing "serial8250" platform device port, >> resulting in only one device node. > > I'm probably missing something important, by why would that > be any different if the PCI driver gets loaded first and the > ISA driver second? Well, the PCI driver would have the proper irq, for one. So, if the the platform driver re-initialized the port to the wrong irq... >> And the configuration knob will be required on all arches anyway because >> that's how user-configurable device nodes are created. > > I think that's fine: The user-configurable ports are the same as > the "ISA" or "phantom" ports we were talking about above, right? Yes. > If those are part of a separate (possibly loadable) module, having > a configuration knob is the obvious way to do it. A lot of architectures > can just turn it off because they know exactly which ports are present > and there is no need for user-configurability. The ones that don't know > can load the module. Let me give this some more thought. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] serial: 8250: Make ISA ports optional
On 01/06/2015 08:13 AM, Arnd Bergmann wrote:
> On Monday 05 January 2015 22:09:45 Peter Hurley wrote:
>> Some arches have no need to create unprobed 8250 ports; these phantom
>> ports are primarily required for ISA ports which have no probe
>> mechanism or to provide non-operational ports for userspace to
>> configure (via TIOCSSERIAL and TIOCSERCONFIG ioctls).
>>
>> Provide CONFIG_SERIAL_8250_PHANTOM_UARTS knob to disable phantom port
>> registration; ie., CONFIG_SERIAL_8250_PHANTOM_UARTS=N only registers
>> probed ports (ACPI/PNP, "serial8250" platform devices, PCI, etc).
>>
>> Cc: Sebastian Andrzej Siewior
>> Cc: Tony Lindgren
>> Cc: Grant Likely
>> Cc: Arnd Bergmann
>> Signed-off-by: Peter Hurley
>
> The intent is definitely right, but I think a better approach is
> possible.
>
> I haven't tried it here, but how about moving the serial8250_init
> function into a separate module, along with all the other parts
> that are only used for ISA devices, but leaving the actual core
> (all exported symbols) in this file?
Unfortunately, I don't see a way to remove the stacked initialization
without risking tons of breakage.
Since later probes can "find" an already-existing port and
re-initialize it, the probe order is crucial. For example, a PCI
probe can "find" an existing "serial8250" platform device port,
resulting in only one device node.
And the configuration knob will be required on all arches anyway because
that's how user-configurable device nodes are created.
> At the same time, the serial8250_pnp_init/serial8250_pnp_exit calls
> can be removed from the serial8250_init function and become
> standalone initcalls.
PNP probe must occur before the phantom ports are registered.
See commit 835d844d1a28efba81d5aca7385e24c29d3a6db2
("8250_pnp: do pnp probe before legacy probe").
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] serial: 8250: Make ISA ports optional
Some arches have no need to create unprobed 8250 ports; these phantom
ports are primarily required for ISA ports which have no probe
mechanism or to provide non-operational ports for userspace to
configure (via TIOCSSERIAL and TIOCSERCONFIG ioctls).
Provide CONFIG_SERIAL_8250_PHANTOM_UARTS knob to disable phantom port
registration; ie., CONFIG_SERIAL_8250_PHANTOM_UARTS=N only registers
probed ports (ACPI/PNP, "serial8250" platform devices, PCI, etc).
Cc: Sebastian Andrzej Siewior
Cc: Tony Lindgren
Cc: Grant Likely
Cc: Arnd Bergmann
Signed-off-by: Peter Hurley
---
drivers/tty/serial/8250/8250.h | 6 ++
drivers/tty/serial/8250/8250_core.c | 13 +++--
drivers/tty/serial/8250/Kconfig | 24 +++-
3 files changed, 36 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index b008368..bda82f7 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -96,6 +96,12 @@ struct serial8250_config {
#define SERIAL8250_SHARE_IRQS 0
#endif
+#ifdef CONFIG_SERIAL_8250_PHANTOM_UARTS
+#define SERIAL8250_PHANTOM_UARTS 1
+#else
+#define SERIAL8250_PHANTOM_UARTS 0
+#endif
+
static inline int serial_in(struct uart_8250_port *up, int offset)
{
return up->port.serial_in(&up->port, offset);
diff --git a/drivers/tty/serial/8250/8250_core.c
b/drivers/tty/serial/8250/8250_core.c
index 3bfcfdb..1b27b2f 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -52,10 +52,13 @@
* Configuration:
* share_irqs - whether we pass IRQF_SHARED to request_irq(). This option
*is unsafe when used on edge-triggered interrupts.
+ * nr_uarts - max # of ports which can be registered
+ * phantom_uarts - whether we pre-register "nr_uarts". Required for ISA ports
+ * and providing unprobed ports for userspace to configure.
*/
static unsigned int share_irqs = SERIAL8250_SHARE_IRQS;
-
static unsigned int nr_uarts = CONFIG_SERIAL_8250_RUNTIME_UARTS;
+static unsigned int phantom_uarts = SERIAL8250_PHANTOM_UARTS;
static struct uart_driver serial8250_reg;
@@ -3155,6 +3158,9 @@ serial8250_register_ports(struct uart_driver *drv, struct
device *dev)
{
int i;
+ if (!phantom_uarts)
+ return;
+
for (i = 0; i < nr_uarts; i++) {
struct uart_8250_port *up = &serial8250_ports[i];
@@ -3662,7 +3668,7 @@ void serial8250_unregister_port(int line)
mutex_lock(&serial_mutex);
uart_remove_one_port(&serial8250_reg, &uart->port);
- if (serial8250_isa_devs) {
+ if (serial8250_isa_devs && phantom_uarts) {
uart->port.flags &= ~UPF_BOOT_AUTOCONF;
uart->port.type = PORT_UNKNOWN;
uart->port.dev = &serial8250_isa_devs->dev;
@@ -3769,6 +3775,9 @@ MODULE_PARM_DESC(share_irqs, "Share IRQs with other
non-8250/16x50 devices"
module_param(nr_uarts, uint, 0644);
MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-"
__MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
+module_param(phantom_uarts, uint, 0644);
+MODULE_PARM_DESC(phantom_uarts, "Enable UARTs with no hardware");
+
module_param(skip_txen_test, uint, 0644);
MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init
time");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 0fcbcd2..87e649b 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -145,15 +145,29 @@ config SERIAL_8250_NR_UARTS
via hot-plug, or any ISA multi-port serial cards.
config SERIAL_8250_RUNTIME_UARTS
- int "Number of 8250/16550 serial ports to register at runtime"
+ int "Maximum number of 8250/16550 serial ports to enable"
depends on SERIAL_8250
range 0 SERIAL_8250_NR_UARTS
default "4"
help
- Set this to the maximum number of serial ports you want
- the kernel to register at boot time. This can be overridden
- with the module parameter "nr_uarts", or boot-time parameter
- 8250.nr_uarts
+ Set this to the maximum number of 8250/16550 serial ports you
+ want the kernel to allow. This can be overridden with the module
+ parameter "nr_uarts", or boot-time parameter, 8250.nr_uarts,
+ up to the maximum.
+
+config SERIAL_8250_PHANTOM_UARTS
+ bool
+ prompt "Enable phantom 8250/16550 serial ports" if !ISA
+ depends on SERIAL_8250
+ default y
+ ---help---
+ Say Y here to create all 8250/16550 serial ports at module load time.
+ Saying N here prevents the creation of unprobed ports, but also
+ disables userspace probe/configure (since no device node is
[PATCH] serial: omap_8250: Fix RTS handling, part B
Because the OMAP3 UART ignores MCR[1] (RTS) in autoRTS mode, autoRTS
mode must not be enabled unless RTS is set (or port->mctrl & TIOCM_RTS,
which is equivalent).
Fixes premature raising of RTS in omap_8250_set_termios() -- RTS was
raised even before UART mode was selected.
Fixes raise of RTS after port has been shutdown; omap_8250_pm() re-enabled
RTS after omap_8250_shutdown().
Cc: Sebastian Andrzej Siewior
Signed-off-by: Peter Hurley
---
drivers/tty/serial/8250/8250_omap.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 428f384..316c37b 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -272,7 +272,10 @@ static void omap8250_restore_regs(struct uart_8250_port
*up)
serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
serial_dl_write(up, priv->quot);
- serial_out(up, UART_EFR, priv->efr);
+ if (up->port.mctrl & TIOCM_RTS)
+ serial_out(up, UART_EFR, priv->efr);
+ else
+ serial_out(up, UART_EFR, priv->efr & ~UART_EFR_RTS);
/* Configure flow control */
serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
@@ -468,18 +471,18 @@ static void omap_8250_set_termios(struct uart_port *port,
static void omap_8250_pm(struct uart_port *port, unsigned int state,
unsigned int oldstate)
{
- struct uart_8250_port *up =
- container_of(port, struct uart_8250_port, port);
- struct omap8250_priv *priv = up->port.private_data;
+ struct uart_8250_port *up = up_to_u8250p(port);
+ u8 efr;
pm_runtime_get_sync(port->dev);
serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
- serial_out(up, UART_EFR, priv->efr | UART_EFR_ECB);
+ efr = serial_in(up, UART_EFR);
+ serial_out(up, UART_EFR, efr | UART_EFR_ECB);
serial_out(up, UART_LCR, 0);
serial_out(up, UART_IER, (state != 0) ? UART_IERX_SLEEP : 0);
serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
- serial_out(up, UART_EFR, priv->efr);
+ serial_out(up, UART_EFR, efr);
serial_out(up, UART_LCR, 0);
pm_runtime_mark_last_busy(port->dev);
--
2.2.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] serial: omap_8250: Fix RTS handling
The OMAP3 UART ignores MCR[1] (ie., UART_MCR_RTS) when in autoRTS
mode (UPF_HARD_FLOW + CRTSCTS). This makes it impossible for either
the serial core or userspace to manually flow control the sender.
Disable autoRTS mode when RTS is lowered and restore the previous
mode when RTS is raised.
Note that the OMAP3 UART provides no mechanism for switching from
autoRTS mode without corrupting incoming data; to access the
necessary register, the line control settings must be set to 8-e-2
and thus any data received during that time will be interpreted with
those settings. This corruption has been observed in practice.
Cc: Sebastian Andrzej Siewior
Signed-off-by: Peter Hurley
---
drivers/tty/serial/8250/8250_core.c | 12 +++-
drivers/tty/serial/8250/8250_omap.c | 25 ++---
include/linux/serial_8250.h | 1 +
include/linux/serial_core.h | 1 +
4 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c
b/drivers/tty/serial/8250/8250_core.c
index 11c6685..3bfcfdb 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1924,7 +1924,7 @@ static unsigned int serial8250_get_mctrl(struct uart_port
*port)
return ret;
}
-static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
+void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
{
struct uart_8250_port *up = up_to_u8250p(port);
unsigned char mcr = 0;
@@ -1944,6 +1944,14 @@ static void serial8250_set_mctrl(struct uart_port *port,
unsigned int mctrl)
serial_port_out(port, UART_MCR, mcr);
}
+EXPORT_SYMBOL_GPL(serial8250_do_set_mctrl);
+
+static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+ if (port->set_mctrl)
+ return port->set_mctrl(port, mctrl);
+ return serial8250_do_set_mctrl(port, mctrl);
+}
static void serial8250_break_ctl(struct uart_port *port, int break_state)
{
@@ -3605,6 +3613,8 @@ int serial8250_register_8250_port(struct uart_8250_port
*up)
/* Possibly override set_termios call */
if (up->port.set_termios)
uart->port.set_termios = up->port.set_termios;
+ if (up->port.set_mctrl)
+ uart->port.set_mctrl = up->port.set_mctrl;
if (up->port.startup)
uart->port.startup = up->port.startup;
if (up->port.shutdown)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 96b69bf..428f384 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -106,6 +106,27 @@ static u32 uart_read(struct uart_8250_port *up, u32 reg)
return readl(up->port.membase + (reg << up->port.regshift));
}
+static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+ struct uart_8250_port *up = up_to_u8250p(port);
+ struct omap8250_priv *priv = up->port.private_data;
+ u8 lcr;
+
+ serial8250_do_set_mctrl(port, mctrl);
+
+ /*
+* Turn off autoRTS if RTS is lowered and restore autoRTS setting
+* if RTS is raised
+*/
+ lcr = serial_in(up, UART_LCR);
+ serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+ if (mctrl & TIOCM_RTS)
+ serial_out(up, UART_EFR, priv->efr);
+ else
+ serial_out(up, UART_EFR, priv->efr & ~UART_EFR_RTS);
+ serial_out(up, UART_LCR, lcr);
+}
+
/*
* Work Around for Errata i202 (2430, 3430, 3630, 4430 and 4460)
* The access to uart register after MDR1 Access
@@ -400,9 +421,6 @@ static void omap_8250_set_termios(struct uart_port *port,
if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW) {
/* Enable AUTORTS and AUTOCTS */
priv->efr |= UART_EFR_CTS | UART_EFR_RTS;
-
- /* Ensure MCR RTS is asserted */
- up->mcr |= UART_MCR_RTS;
} else if (up->port.flags & UPF_SOFT_FLOW) {
/*
* IXON Flag:
@@ -1007,6 +1025,7 @@ static int omap8250_probe(struct platform_device *pdev)
up.capabilities |= UART_CAP_RPM;
#endif
up.port.set_termios = omap_8250_set_termios;
+ up.port.set_mctrl = omap8250_set_mctrl;
up.port.pm = omap_8250_pm;
up.port.startup = omap_8250_startup;
up.port.shutdown = omap_8250_shutdown;
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index e02acf0..245b959 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -126,6 +126,7 @@ extern int serial8250_do_startup(struct uart_port *port);
extern void serial8250_do_shutdown(struct uart_port *port);
extern void serial8250_do_pm(struct uart_port *port, unsigned int state,
unsi
Re: am335x: cpsw: interrupt failure
On 12/29/2014 04:33 AM, Yegor Yefremov wrote: > On Fri, Dec 12, 2014 at 8:19 PM, Yegor Yefremov > wrote: >> On Fri, Dec 12, 2014 at 6:32 PM, Felipe Balbi wrote: >>> Hi, >>> >>> On Fri, Dec 12, 2014 at 01:00:51PM +0100, Yegor Yefremov wrote: >>>> U-Boot version: 2014.07 >>>> Kernel config is omap2plus with enabled USB >>>> >>>> # cat /proc/version >>>> Linux version 3.18.0 (user@user-VirtualBox) (gcc version 4.8.3 >>>> 20140320 (prerelease) (Sourcery CodeBench Lite 2014.05-29) ) #6 SMP >>>> Mon Dec 8 22:47:43 CET 2014 >>> >>> Wasn't GCC 4.8.x total crap for building ARM kernels ? IIRC it was even >>> blacklisted. Can you try with 4.9.x just to make sure ? >> >> Will do. > > Adding linux-omap. Beginning of this discussion: > http://comments.gmane.org/gmane.linux.network/341427 > > Quick summary: starting with kernel 3.18 or commit > 55601c9f24670ba926ebdd4d712ac3b177232330 am335x (at least BBB and some > custom boards) stalls at high network load. Reproducible via nuttcp > within some minutes > > nuttcp -S (on BBB) > nuttcp -t -N 4 -T30m 192.168.1.235 (on host) > > As Felipe Balbi suggested, I tried both 4.8.3 and 4.9.2 toolchains, > but both show the same behavior. > > Linux version 3.18.0 (user@user-VirtualBox) (gcc version 4.8.3 > 20140320 (prerelease) (Sourcery CodeBench Lite 2014.05-29) ) #6 SMP > Mon Dec 8 22:47:43 CET 2014 > Linux version 3.18.1 (user@user-VirtualBox) (gcc version 4.9.2 > (Buildroot 2015.02-git-00582-g10b9761) ) #1 SMP Mon Dec 29 09:22:29 > CET 2014 > > Let me know, if you can reproduce this issue. I have seen the irq 0 error messages on the black since 3.18+, but didn't bisect it yet. For me, these errors occurred with a slightly misconfigured emacs24-nox, which drove the cpu load way up - over 50% - with just cursor movement (it still gets above 20% which seems unacceptably high). I'm not sure if all the crashes were over ssh; I hadn't considered the cpsw relevant until reading this. I'll retest over the serial console. I have seen abrupt resets without messages on earlier kernels so perhaps the commit is not the root cause. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: Blacklist GCC 4.8.0 to GCC 4.8.2 - PR58854
On 10/15/2014 08:18 PM, Russell King - ARM Linux wrote: > Hence why I recommend that Linaro takes down their buggy compiler. > Their 4.8.3 version should not be used *anywhere*, just the same as > the stock 4.8 to 4.8.2 inclusive should also not be used anywhere on > ARM either. Completely agree. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: Blacklist GCC 4.8.0 to GCC 4.8.2 - PR58854
On 10/15/2014 05:56 PM, Russell King wrote:
> These stock GCC versions miscompile the kernel by incorrectly optimising
> the function epilogue code - by first increasing the stack pointer, and
> then loading entries from below the stack. This means that an opportune
> interrupt or exception will corrupt the current function's saved state,
> which may result in the parent function seeing different register
> values.
>
> As this bug has been known to result in corrupted filesystems, and these
> buggy compiler versions seem to be frequently used, we have little
> option but to blacklist these compiler versions.
>
> Distributions may have fixed PR58854, but as their compilers are totally
> indistinguishable from the buggy versions, it is unfortunate that this
> also results in those also being blacklisted. Given the filesystem
> corruption potential of the original, this is the lesser evil. People
> who want to build with their fixed compiler versions will need to adjust
> the kernel source. (Distros need to think about the implications of
> fixing such a compiler bug, and consider how to ensure that their fixed
> compiler versions can be detected if they wish to avoid this.)
>
> Signed-off-by: Russell King
> ---
>
> This is what I came up with - this places the build check right at the
> beginning of the kernel build, rather than at some point where
> linux/compiler.h gets included. Note that this is where we have
> previous ARM specific GCC version blacklisting. I'm blacklisting
> GCC 4.8.0 to GCC 4.8.2 inclussive, which seems to be the right versions
> for stock GCC.
>
> I was in two minds whether to include 4.8.3 as Linaro released a buggy
> toolchain which identifies itself as 4.8.3, but I decided that's also
> a distro problem. IMHO Linaro should really think about taking that
> compiler down given the seriousness of this bug and it being
> indistinguishable from the fixed stock version.
Maybe it's unfair to blame them; Linaro just took a snapshot and
released what was there.
If gcc is going to retain the "change release number then add all the
new features" model, some kind of prerelease indicator would help
eliminate this kind of problem. And that indicator should be both
a preprocessor define and parseable from the command line :)
> arch/arm/kernel/asm-offsets.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
> index 713e807621d2..e14c1a12b414 100644
> --- a/arch/arm/kernel/asm-offsets.c
> +++ b/arch/arm/kernel/asm-offsets.c
> @@ -10,6 +10,7 @@
> * it under the terms of the GNU General Public License version 2 as
> * published by the Free Software Foundation.
> */
> +#include
> #include
> #include
> #include
> @@ -39,10 +40,19 @@
> * GCC 3.2.x: miscompiles NEW_AUX_ENT in fs/binfmt_elf.c
> *(http://gcc.gnu.org/PR8896) and incorrect structure
> * initialisation in fs/jffs2/erase.c
> + * GCC 4.8.0-4.8.2: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58854
> + * miscompiles find_get_entry(), and can result in EXT3 and EXT4
> + * filesystem corruption (possibly other FS too).
> */
> +#ifdef __GNUC__
> #if (__GNUC__ == 3 && __GNUC_MINOR__ < 3)
> #error Your compiler is too buggy; it is known to miscompile kernels.
> -#errorKnown good compilers: 3.3
> +#errorKnown good compilers: 3.3, 4.x
> +#endif
> +#if GCC_VERSION >= 40800 || GCC_VERSION < 40803
^^
&&
> +#error Your compiler is too buggy; it is known to miscompile kernels
> +#error and result in filesystem corruption and oopses.
> +#endif
> #endif
>
> int main(void)
>
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RCU bug with v3.17-rc3 ?
On 10/13/2014 10:06 PM, Greg KH wrote:
> On Mon, Oct 13, 2014 at 12:43:07PM +0100, Russell King - ARM Linux wrote:
>> On Mon, Oct 13, 2014 at 09:11:34AM +, David Laight wrote:
>>> From: Nathan Lynch
>>>> On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:
>>>>>
>>>>> Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
>>>>> it seems that this has been known about for some time.)
>>>>
>>>> Looking at http://gcc.gnu.org/PR58854 it seems that all 4.8.x for x < 3
>>>> are affected, as well as 4.9.0.
>>>>
>>>>> We can blacklist these GCC versions quite easily. We already have GCC
>>>>> 3.3 blacklisted, and it's trivial to add others. I would want to include
>>>>> some proper details about the bug, just like the other existing entries
>>>>> we already have in asm-offsets.c, where we name the functions that the
>>>>> compiler is known to break where appropriate.
>>>>
>>>> Before blacklisting anything, it's worth considering that simple version
>>>> checks would break existing pre-4.8.3 compilers that have been patched
>>>> for PR58854. It looks like Yocto and Buildroot issued releases with
>>>> patched 4.8.2 compilers well before the (fixed) 4.8.3 release. I think
>>>> the most we can reasonably do without breaking some correctly-behaving
>>>> toolchains is to emit a warning.
>>>
>>> Is it possible to compile a small code fragment and check the generated
>>> code for the bug?
>>> Possibly predicated on the broken version number to avoid false positives.
>>
>> I don't see how - it looks like it requires an interrupt to occur at an
>> opportune moment to provoke the function to fail. The alternative would
>> be to parse the assembly generated by the compiler to determine how it
>> is dealing with the stack.
>>
>> I think the only viable solution here is that:
>>
>> 1. We blacklist the bad compiler versions outright in the kernel.
>
> Yes, please do this, it's what we have done for other buggy compiler
> versions, no need to do something different here.
>
>> Remember, it's the distro's choice to fix these buggy compilers, so the
>> onus is on _them_ to deal with the mess they've created by doing so.
>
> I totally agree.
>
> Is someone going to send this patch, or do I have to write it myself?
I did on Friday (arm: Blacklist gcc 4.8.[012] ...) but Russell said he
was doing it himself.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RCU bug with v3.17-rc3 ?
On 10/11/2014 10:51 AM, Otavio Salvador wrote:
> Hello Russell,
>
> On Sat, Oct 11, 2014 at 11:16 AM, Russell King - ARM Linux
> wrote:
>> On Sat, Oct 11, 2014 at 11:54:32AM +0800, Peter Chen wrote:
>>> On Fri, Oct 10, 2014 at 08:44:33PM -0500, Nathan Lynch wrote:
On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:
>
> Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
> it seems that this has been known about for some time.)
Looking at http://gcc.gnu.org/PR58854 it seems that all 4.8.x for x < 3
are affected, as well as 4.9.0.
> We can blacklist these GCC versions quite easily. We already have GCC
> 3.3 blacklisted, and it's trivial to add others. I would want to include
> some proper details about the bug, just like the other existing entries
> we already have in asm-offsets.c, where we name the functions that the
> compiler is known to break where appropriate.
Before blacklisting anything, it's worth considering that simple version
checks would break existing pre-4.8.3 compilers that have been patched
for PR58854. It looks like Yocto and Buildroot issued releases with
patched 4.8.2 compilers well before the (fixed) 4.8.3 release. I think
the most we can reasonably do without breaking some correctly-behaving
toolchains is to emit a warning.
>>>
>>> Yocto has PR58854 problem patch.
>>>
>>> http://git.yoctoproject.org/cgit/cgit.cgi/poky/tree/meta/recipes-devtools/gcc/gcc-4.8/0048-PR58854_fix_arm_apcs_epilogue.patch?h=daisy
>>
>> Right, and we can provide links to these in the comments above the #error
>> so people have the right places to do a bit of research into whether their
>> compiler is safe.
>>
>> It is unfortunate that they are indistinguishable from the broken versions,
>> but that's really a distro problem for causing that issue themselves -
>> especially given how serious this bug is.
>
> What about checking if GCC_PR58854_FIXED is not defined for error? So
> build systems and people could easily define it if they know their GCC
> has the fix applied.
If the distro/build system/individual is capable of patching gcc, then it
seems reasonable that the same distro/build system/individual is capable
of carrying a patch on top of mainline kernel for building with their
"special" compiler.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RCU bug with v3.17-rc3 ?
On 10/10/2014 09:44 PM, Nathan Lynch wrote:
> On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:
>>
>> Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
>> it seems that this has been known about for some time.)
>
> Looking at http://gcc.gnu.org/PR58854 it seems that all 4.8.x for x < 3
> are affected, as well as 4.9.0.
>
>> We can blacklist these GCC versions quite easily. We already have GCC
>> 3.3 blacklisted, and it's trivial to add others. I would want to include
>> some proper details about the bug, just like the other existing entries
>> we already have in asm-offsets.c, where we name the functions that the
>> compiler is known to break where appropriate.
>
> Before blacklisting anything, it's worth considering that simple version
> checks would break existing pre-4.8.3 compilers that have been patched
> for PR58854. It looks like Yocto and Buildroot issued releases with
> patched 4.8.2 compilers well before the (fixed) 4.8.3 release. I think
> the most we can reasonably do without breaking some correctly-behaving
> toolchains is to emit a warning.
Providing a manual switch to override blacklisting is way more sane
than a build warning that no one's looking at.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13 v10] omap 8250 based UART + DMA
On 09/29/2014 02:06 PM, Sebastian Andrzej Siewior wrote: > The queue is getting smaller. The highlights of v9…v10 > - the DMA stall Frans Klaver reported which popped up in yocto is gone. It > also seems that the "ack the err-irq even if nothing happened" in EDMA > can be dropped. > - the RX- and TX-DMA callbacks are now OMAP-only and no "bugs" flags are > introduced into the generic DMA code. This also means that there is > custom IRQ routine in case of DMA. For the series: Reviewed-by: Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/13] tty: serial: 8250: make serial8250_console_setup() non _init
On 09/29/2014 02:06 PM, Sebastian Andrzej Siewior wrote: > if I boot with console=ttyS0 and the omap driver is module I end up with > > | console [ttyS0] disabled Note to self: make ISA config optional. > | omap8250 44e09000.serial: ttyS0 at MMIO 0x44e09000 (irq = 88, base_baud = > 300) is a 8250 > | Unable to handle kernel paging request at virtual address c07a9de0 > | Modules linked in: 8250_omap(+) > | CPU: 0 PID: 908 Comm: modprobe Not tainted 3.17.0-rc5+ #1593 > | PC is at serial8250_console_setup+0x0/0xc8 > | LR is at register_console+0x13c/0x3a4 > | [] (register_console) from [] > (uart_add_one_port+0x3cc/0x420) > | [] (uart_add_one_port) from [] > (serial8250_register_8250_port+0x298/0x39c) > | [] (serial8250_register_8250_port) from [] > (omap8250_probe+0x218/0x3dc [8250_omap]) > | [] (omap8250_probe [8250_omap]) from [] > (platform_drv_probe+0x2c/0x5c) > | [] (platform_drv_probe) from [] > (driver_probe_device+0x104/0x228) > … > | [] (SyS_init_module) from [] (ret_fast_syscall+0x0/0x30) > | Code: 7823603b f8314620 051b3013 491ed416 (44792204) > > because serial8250_console_setup() is already gone. > > Signed-off-by: Sebastian Andrzej Siewior Reviewed-by: Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
On 09/25/2014 06:42 AM, Sebastian Andrzej Siewior wrote:
> * Sebastian Andrzej Siewior | 2014-09-24 09:53:46 [+0200]:
>
>> * Peter Hurley | 2014-09-23 13:03:51 [-0400]:
>>
>>> But DMA is cheating if the UART driver's tx_empty() method is saying the
>>> transmitter is empty while TX DMA is still running.
>> This shouldn't be the case. But I will check this once I able to.
>
> I added
>
> |#define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
> |trace_printk("delay <%d>\n", (lsr & BOTH_EMPTY) == BOTH_EMPTY ? 1 : 0);
>
> in my set_termios() and the trace shows
> | vi-949 [000] d...70.477002: omap8250_restore_regs: delay
> <0>
>
> so no, it does not wait until TX FIFO is empty. It looks like it uses
> TCSANOW instead of TCSADRAIN. And since this looks "legal" I will delay
> it until TX-DMA is complete because it is known to stall the DMA operation.
I just verified that GNU readline6 uses ioctl(TCSETSW, ...) to do the
set_termios
(which is the ioctl that libc should use for tcsetattr(TCSADRAIN)).
Maybe this userspace is using a readline()-alike that has a bug by not using the
correct tcsetattr() action?
Or maybe the glibc-equivalent has the bug, and tcsetattr(TCSADRAIN) is not using
ioctl(TCSETSW)?
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] tty: omap-serial: use threaded interrupt handler
On 09/23/2014 04:24 AM, Frans Klaver wrote: > On Wed, Sep 17, 2014 at 02:13:03PM +0200, Frans Klaver wrote: >> On Wed, Sep 17, 2014 at 08:01:08AM -0400, Peter Hurley wrote: >>> On 09/16/2014 04:50 AM, Frans Klaver wrote: >>>> On Mon, Sep 15, 2014 at 01:31:56PM -0400, Peter Hurley wrote: >>>>> On 09/15/2014 11:39 AM, Peter Hurley wrote: >>>>>> On 09/15/2014 10:00 AM, Frans Klaver wrote: >>>>>>> At 3.6Mbaud, with slightly over 2Mbit/s data coming in, we see 1600 uart >>>>>>> rx buffer overflows within 30 seconds. Threading the interrupt handling >>>>>>> reduces >>>>>>> this to about 170 overflows in 10 minutes. >>>>>> >>>>>> Why is the threadirqs kernel boot option not sufficient? >>>>>> Or conversely, shouldn't this be selectable? >>>>> >>>> >>>> I wasn't aware of the threadirqs boot option. I also wouldn't know if >>>> this should be selectable. What would be a reason to favor the >>>> non-threaded irq over the threaded irq? >>> >>> Not everyone cares enough about serial to dedicate kthreads to it :) >> >> Fair enough. In that light, we might not care enough about other >> subsystems to dedicate kthreads to it :). Selectable seems reasonable in >> that case. >> >> >>>>> Also, do you see the same performance differential when you implement this >>>>> in the 8250 driver (that is, on top of Sebastian's omap->8250 conversion)? >>>>> >>>> >>>> I haven't gotten Sebastian's driver to work properly yet on the console. >>>> There was no reason for me yet to throw my omap changes on top of >>>> Sebastian's queue. > > Doing the threaded interrupt change on the 8250 driver doesn't seem as > trivial. Unless I'm mistaken, that version of this patch would mess with > all other 8250 based serial drivers, if it's done properly. Incidentally > I did try using threadirqs, but that didn't give my any significant > results. I mostly noticed a difference in the console. > > >>>> >>>>>> PS - To overflow the 64 byte RX FIFO at those data rates means interrupt >>>>>> latency in excess of 250us? >>>> >>>> At 3686400 baud it should take about 174 us to fill a 64 byte buffer. I >>>> haven't done any measurements on the interrupt latency though. If you >>>> consider that we're sending about 1kB of data, 240 times a second, we're >>>> spending a lot of time reading data from the uart. I can imagine the >>>> system has other work to do as well. >>> >>> System work should not keep irqs from being serviced. Even 174us is a long >>> time not to service an interrupt. Maybe console writes are keeping the isr >>> from running? >> >> That's quite possible. I'll have to redo the test setup I had for this to >> give you a decent answer. I'll have to do that anyway as Sebastian's >> 8250 conversion improves. > > I haven't had time yet to look into this any further. I'll accept that > this patch may fix a case most people aren't the least interested in. > I'll also happily accept that I probably need a better argumentation > than "this works better for us".Would it make sense to drop this patch > and resubmit the other three? As I mentioned in the previous run, I > think these are useful in any case. I would've thought the first 2 patches had already been picked up because they fix div-by-zero faults. I don't really have a problem with the patch (except for it should be selectable, even if that's just a CONFIG_ setting). At the same time, the performance results don't really make sense; so if there's actually an underlying problem, I'd rather that get addressed (and the long interrupt latency may be the underlying problem). As far as the 8250 driver and threaded irqs go, I just was hoping for another data point with a simple hard-coded test jig, not a full-blown patch series for all of them. Sorry for the misunderstanding. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
On 09/21/2014 04:41 PM, Sebastian Andrzej Siewior wrote: > * Frans Klaver | 2014-09-17 12:28:12 [+0200]: > >> - Bone Black: Yocto poky, core-image-minimal >> Login, "less file" locks up, doesn't show anything. I can exit using >> Ctrl-C. > > So I have the same with my and the serial-omap driver. No difference > here. The trace looks like this: > | -0 [000] d.h. 444.393585: serial8250_handle_irq: iir > cc lsr 61 > | -0 [000] d.h. 444.393605: serial8250_rx_chars: get 0d > received the enter key > > | -0 [000] d.h. 444.393609: serial8250_rx_chars: insert > d lsr 61 > | -0 [000] d.h. 444.393614: uart_insert_char: 1 > | -0 [000] d.h. 444.393617: uart_insert_char: 2 > | -0 [000] dnh. 444.393636: serial8250_tx_chars: empty > | kworker/0:2-753 [000] d... 444.393686: serial8250_start_tx: empty?1 > | kworker/0:2-753 [000] d.h. 444.393699: serial8250_handle_irq: iir > c2 lsr 60 > | kworker/0:2-753 [000] d.h. 444.393705: serial8250_tx_chars: empty > | sh-1042 [000] d... 444.393822: serial8250_start_tx: empty?1 > | sh-1042 [000] d.h. 444.393836: serial8250_handle_irq: iir > c2 lsr 60 > | sh-1042 [000] d.h. 444.393842: serial8250_tx_chars: empty > | sh-1042 [000] d... 444.393855: serial8250_start_tx: empty?0 > | sh-1042 [000] d.h. 444.393863: serial8250_handle_irq: iir > c2 lsr 60 > | sh-1042 [000] d.h. 444.393867: serial8250_tx_chars: put 0d > | sh-1042 [000] d.h. 444.393871: serial8250_tx_chars: put 0a > > shell responded with "\r\n" which I see and then > > | sh-1042 [000] d.h. 444.394057: serial8250_handle_irq: iir > c2 lsr 60 > | sh-1042 [000] d.h. 444.394065: serial8250_tx_chars: empty > > nothing more. less isn't sending data for some reason. Exactly the same > thing happens in a Debian environment except that it continues: > … > | bash-2468 [000] d.h.99.657899: serial8250_tx_chars: put 0a > | bash-2468 [000] d.h.99.658089: serial8250_handle_irq: iir > c2 lsr 60 > | bash-2468 [000] d.h.99.658095: serial8250_tx_chars: empty > => > | less-2474 [000] d...99.696038: serial8250_start_tx: empty?0 > | less-2474 [000] d.h.99.696069: serial8250_handle_irq: iir > c2 lsr 60 > | less-2474 [000] d.h.99.696078: serial8250_tx_chars: put 1b > | less-2474 [000] d.h.99.696082: serial8250_tx_chars: put 5b > | less-2474 [000] d.h.99.696085: serial8250_tx_chars: put 3f > | less-2474 [000] d.h.99.696087: serial8250_tx_chars: put 31 > > It has to be something about the environment. Booting Debian and chroot > into this RFS and less works perfectly. But since it behaves like that > with both drivers, I guess the problem is somewhere else… > >> vi runs normally, only occupies part of the total screen estate in >> minicom. After quitting, a weird character shows up (typically I see >> ÿ there), but minicom can use the rest of the screen estate again. >> If we disregard the odd character, this is much like the behavior we >> have on the omap-serial driver. >> - Custom board: Yocto poky, custom image >> Login, "less file" locks up, showing only "ÿ" in the top left corner >> of the screen. Can get out of there by having something dumped through >> /dev/kmsg. > > I managed to run into something like that with vi on dra7 and with > little more patience on am335x as well by "vi *" and then ":n". > > This gets fixed indeed by writing. Hours of debugging and a lot of hair > less later: the yocto RFS calls set_termios quite a lot. readline() does this; it 'saves' the caller's termios, sets termios for non-canonical reads, reads one char, and 'restores' the caller's termios. > This includes > changing the baudrate (not by yocto but the driver sets it to 0 and then > to the requested one) and this seems to be responsible for the "bad > bytes". I haven't figured out yet I don't see this with omap-serial. > Even worse: If this (set_termios()) happens while the DMA is still > active then it might stall it. A write into the FIFO seems to fix it and > this is where your "echo >/dev/kmsg" fixes things. > If I delay the restore_registers part of set_termios() until TX-DMA is > complete then it seems that the TX-DMA stall does not tall anymore. The tty core calls the driver's wait_unti
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
On 09/19/2014 06:58 AM, Sebastian Andrzej Siewior wrote: > On 09/19/2014 12:22 PM, Heikki Krogerus wrote: >> Couldn't you just replace the handle_irq with a custom irq routine in >> the omap driver like you did with set_termios? Where you would do >> those tricks and/or call serial8250_handle_irq()? > > Tricks within serial8250_handle_irq(), see [0]. It is not a lot but > still. I could provide my own handle irq, just asking what you would > prefer. > > [0] > http://git.breakpoint.cc/cgit/bigeasy/linux.git/commit/?h=uart_v10_pre2&id=f26f161d998ee4a84a0aa6ddff69a435c25f204d > >> The 8250_core changes in that patch #10 only modify >> serial8250_handle_irg right? > > Correct. However there is another change due to the RX_DMA_BUG. A while > ago you said that this RX_DMA_BUG thing might be something that other > SoC could use, too. > If you ask me now for my own irq routine it would make sense to move RX > bug handling into omap specific code as well. I'm not excited at the prospect of an omap-specific irq handler, especially if this is just a silicon bug. I think it will create and encourage unnecessary code variation in the 8250 driver. The inertia of an omap-specific irq handler will mean it will probable stay forever. Suppose this tx dma bug is later discovered to be fixable inline (rather than by state), then we'll be stuck with an irq handler that no one will want to integrate. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
On 09/16/2014 12:55 PM, Sebastian Andrzej Siewior wrote: > On 09/15/2014 07:01 PM, Sebastian Andrzej Siewior wrote: >> On 09/11/2014 04:35 PM, Peter Hurley wrote: >>> I do need to find out if omap hardware sets UART_MSR_DCTS when auto CTS >>> is on. Would you mind running the debug patch below with HW flow control on? >> >> I didn't forget about this. However I told minicom to use hardware flow >> control (and I see the driver set the HW bit) but I haven't seen that >> uart_handle_cts_change() has been invoked at all. I'm going to check >> two other boards and report then. > > No, I don't get into this at all function. Ok, good to know. Thanks for testing that. > So I connected my am335x-evm > with beagle board xm because both of them have an old fashion UART > connector (instead those uart-to-usb). Both configured with HW-Flow and > I haven't seen the function invoked but I saw "port->icount.overrun" > being incremented. This shouldn't happen. So I am a little puzzled here… Yeah, that's weird. Do you have a break-out box to confirm that RTS/CTS are being driven? Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] tty: omap-serial: use threaded interrupt handler
On 09/16/2014 04:50 AM, Frans Klaver wrote: > On Mon, Sep 15, 2014 at 01:31:56PM -0400, Peter Hurley wrote: >> On 09/15/2014 11:39 AM, Peter Hurley wrote: >>> On 09/15/2014 10:00 AM, Frans Klaver wrote: >>>> At 3.6Mbaud, with slightly over 2Mbit/s data coming in, we see 1600 uart >>>> rx buffer overflows within 30 seconds. Threading the interrupt handling >>>> reduces >>>> this to about 170 overflows in 10 minutes. >>> >>> Why is the threadirqs kernel boot option not sufficient? >>> Or conversely, shouldn't this be selectable? >> > > I wasn't aware of the threadirqs boot option. I also wouldn't know if > this should be selectable. What would be a reason to favor the > non-threaded irq over the threaded irq? Not everyone cares enough about serial to dedicate kthreads to it :) >> Also, do you see the same performance differential when you implement this >> in the 8250 driver (that is, on top of Sebastian's omap->8250 conversion)? >> > > I haven't gotten Sebastian's driver to work properly yet on the console. > There was no reason for me yet to throw my omap changes on top of > Sebastian's queue. > >>> PS - To overflow the 64 byte RX FIFO at those data rates means interrupt >>> latency in excess of 250us? > > At 3686400 baud it should take about 174 us to fill a 64 byte buffer. I > haven't done any measurements on the interrupt latency though. If you > consider that we're sending about 1kB of data, 240 times a second, we're > spending a lot of time reading data from the uart. I can imagine the > system has other work to do as well. System work should not keep irqs from being serviced. Even 174us is a long time not to service an interrupt. Maybe console writes are keeping the isr from running? Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] tty: omap-serial: use threaded interrupt handler
On 09/15/2014 11:39 AM, Peter Hurley wrote:
> On 09/15/2014 10:00 AM, Frans Klaver wrote:
>> At 3.6Mbaud, with slightly over 2Mbit/s data coming in, we see 1600 uart
>> rx buffer overflows within 30 seconds. Threading the interrupt handling
>> reduces
>> this to about 170 overflows in 10 minutes.
>
> Why is the threadirqs kernel boot option not sufficient?
> Or conversely, shouldn't this be selectable?
Also, do you see the same performance differential when you implement this
in the 8250 driver (that is, on top of Sebastian's omap->8250 conversion)?
> Regards,
> Peter Hurley
>
> PS - To overflow the 64 byte RX FIFO at those data rates means interrupt
> latency in excess of 250us?
>
>> In practice this therefore reduces the need for hardware flow control,
>> meaning the sending side doesn't have to buffer as much either.
>>
>> Signed-off-by: Frans Klaver
>> ---
>> drivers/tty/serial/omap-serial.c | 30 +++---
>> 1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c
>> b/drivers/tty/serial/omap-serial.c
>> index 7d3f557..398139a 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -575,6 +575,20 @@ static void serial_omap_rdi(struct uart_omap_port *up,
>> unsigned int lsr)
>> }
>>
>> /**
>> + * serial_omap_fast_irq() - schedule interrupt handling
>> + */
>> +static irqreturn_t serial_omap_fast_irq(int irq, void *dev_id)
>> +{
>> +struct uart_omap_port *up = dev_id;
>> +unsigned int iir = serial_in(up, UART_IIR);
>> +
>> +if (iir & UART_IIR_NO_INT)
>> +return IRQ_NONE;
>> +
>> +return IRQ_WAKE_THREAD;
>> +}
>> +
>> +/**
>> * serial_omap_irq() - This handles the interrupt from one port
>> * @irq: uart port irq number
>> * @dev_id: uart port info
>> @@ -584,7 +598,6 @@ static irqreturn_t serial_omap_irq(int irq, void *dev_id)
>> struct uart_omap_port *up = dev_id;
>> unsigned int iir, lsr;
>> unsigned int type;
>> -irqreturn_t ret = IRQ_NONE;
>> int max_count = 256;
>>
>> spin_lock(&up->port.lock);
>> @@ -595,7 +608,6 @@ static irqreturn_t serial_omap_irq(int irq, void *dev_id)
>> if (iir & UART_IIR_NO_INT)
>> break;
>>
>> -ret = IRQ_HANDLED;
>> lsr = serial_in(up, UART_LSR);
>>
>> /* extract IRQ type from IIR register */
>> @@ -634,7 +646,7 @@ static irqreturn_t serial_omap_irq(int irq, void *dev_id)
>> pm_runtime_put_autosuspend(up->dev);
>> up->port_activity = jiffies;
>>
>> -return ret;
>> +return IRQ_HANDLED;
>> }
>>
>> static unsigned int serial_omap_tx_empty(struct uart_port *port)
>> @@ -731,15 +743,19 @@ static int serial_omap_startup(struct uart_port *port)
>> /*
>> * Allocate the IRQ
>> */
>> -retval = request_irq(up->port.irq, serial_omap_irq, up->port.irqflags,
>> -up->name, up);
>> +retval = request_threaded_irq(up->port.irq, serial_omap_fast_irq,
>> +serial_omap_irq,
>> +IRQF_ONESHOT |
>> up->port.irqflags,
>> +up->name, up);
>> if (retval)
>> return retval;
>>
>> /* Optional wake-up IRQ */
>> if (up->wakeirq) {
>> -retval = request_irq(up->wakeirq, serial_omap_irq,
>> - up->port.irqflags, up->name, up);
>> +retval = request_threaded_irq(up->wakeirq, serial_omap_fast_irq,
>> +serial_omap_irq,
>> +IRQF_ONESHOT |
>> up->port.irqflags,
>> +up->name, up);
>> if (retval) {
>> free_irq(up->port.irq, up);
>> return retval;
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] tty: omap-serial: use threaded interrupt handler
On 09/15/2014 10:00 AM, Frans Klaver wrote:
> At 3.6Mbaud, with slightly over 2Mbit/s data coming in, we see 1600 uart
> rx buffer overflows within 30 seconds. Threading the interrupt handling
> reduces
> this to about 170 overflows in 10 minutes.
Why is the threadirqs kernel boot option not sufficient?
Or conversely, shouldn't this be selectable?
Regards,
Peter Hurley
PS - To overflow the 64 byte RX FIFO at those data rates means interrupt
latency in excess of 250us?
> In practice this therefore reduces the need for hardware flow control,
> meaning the sending side doesn't have to buffer as much either.
>
> Signed-off-by: Frans Klaver
> ---
> drivers/tty/serial/omap-serial.c | 30 +++---
> 1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c
> b/drivers/tty/serial/omap-serial.c
> index 7d3f557..398139a 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -575,6 +575,20 @@ static void serial_omap_rdi(struct uart_omap_port *up,
> unsigned int lsr)
> }
>
> /**
> + * serial_omap_fast_irq() - schedule interrupt handling
> + */
> +static irqreturn_t serial_omap_fast_irq(int irq, void *dev_id)
> +{
> + struct uart_omap_port *up = dev_id;
> + unsigned int iir = serial_in(up, UART_IIR);
> +
> + if (iir & UART_IIR_NO_INT)
> + return IRQ_NONE;
> +
> + return IRQ_WAKE_THREAD;
> +}
> +
> +/**
> * serial_omap_irq() - This handles the interrupt from one port
> * @irq: uart port irq number
> * @dev_id: uart port info
> @@ -584,7 +598,6 @@ static irqreturn_t serial_omap_irq(int irq, void *dev_id)
> struct uart_omap_port *up = dev_id;
> unsigned int iir, lsr;
> unsigned int type;
> - irqreturn_t ret = IRQ_NONE;
> int max_count = 256;
>
> spin_lock(&up->port.lock);
> @@ -595,7 +608,6 @@ static irqreturn_t serial_omap_irq(int irq, void *dev_id)
> if (iir & UART_IIR_NO_INT)
> break;
>
> - ret = IRQ_HANDLED;
> lsr = serial_in(up, UART_LSR);
>
> /* extract IRQ type from IIR register */
> @@ -634,7 +646,7 @@ static irqreturn_t serial_omap_irq(int irq, void *dev_id)
> pm_runtime_put_autosuspend(up->dev);
> up->port_activity = jiffies;
>
> - return ret;
> + return IRQ_HANDLED;
> }
>
> static unsigned int serial_omap_tx_empty(struct uart_port *port)
> @@ -731,15 +743,19 @@ static int serial_omap_startup(struct uart_port *port)
> /*
>* Allocate the IRQ
>*/
> - retval = request_irq(up->port.irq, serial_omap_irq, up->port.irqflags,
> - up->name, up);
> + retval = request_threaded_irq(up->port.irq, serial_omap_fast_irq,
> + serial_omap_irq,
> + IRQF_ONESHOT |
> up->port.irqflags,
> + up->name, up);
> if (retval)
> return retval;
>
> /* Optional wake-up IRQ */
> if (up->wakeirq) {
> - retval = request_irq(up->wakeirq, serial_omap_irq,
> - up->port.irqflags, up->name, up);
> + retval = request_threaded_irq(up->wakeirq, serial_omap_fast_irq,
> + serial_omap_irq,
> + IRQF_ONESHOT |
> up->port.irqflags,
> + up->name, up);
> if (retval) {
> free_irq(up->port.irq, up);
> return retval;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
On 09/11/2014 08:50 AM, Sebastian Andrzej Siewior wrote:
> On 09/11/2014 02:32 PM, Peter Hurley wrote:
>> On 09/11/2014 07:42 AM, Sebastian Andrzej Siewior wrote:
>>> I also need a watchdog timer for TX since it seems that on omap3 the
>>> DMA engine suddenly forgets to continue with DMA…
>>
>> One difference I noticed between the omap driver and the 8250 driver is
>> the way modem status interrupts are handled.
>>
>> The omap driver only checks modem status for the UART_IIR_MSI interrupt type.
>> The 8250 driver checks modem status at every interrupt (other than NO_INT).
>>
>> I think the UART_MSR_DCTS bit always reflects that the CTS input has changed
>> between reads of the MSR _even if auto CTS is on_. So perhaps the hardware
>> is being stopped by uart_handle_cts_change() when auto CTS is on?
>
> I doubt that. What I see from a timer debug is that the TX-FIFO level
> is at 0, the DMA transfer for say 1024 bytes start.
> The FIFO is filled to 64bytes and refilled so it doesn't drop below 50.
> At the time of the stall I see that the DMA engine has outstanding
> bytes which it should transfer and the TX FIFO is empty. If hardware
> flow control stops the transfer, I would expect that the DMA engine
> still fills the TX-FIFO until 64 and then waits. But it doesn't.
> Writing bytes into the FIFO leads to bytes beeing sent (and I see them
> on the other side) but the DMA transfer is still on hold. Canceling the
> DMA transfer and re-programming the remaining bytes transfers the
> remaining bytes.
>
> The odd thing is that I only triggered it with "less file". It doesn't
> happen on regular console interaction or "cat large-file". And it only
> triggers on beagle board xm (omap34xx) and I wasn't able to reproduce
> it on am335x or dra7. The latter shares the same DMA engine as beagle
> board xm.
>
> I remember also that I disabled the HW/SW float control just to make
> sure it is not it.
Ok.
I do need to find out if omap hardware sets UART_MSR_DCTS when auto CTS
is on. Would you mind running the debug patch below with HW flow control on?
Regards,
Peter Hurley
--- >% ---
Subject: [DEBUG] serial: does OMAP set UART_LSR_DCTS while autoCTS is on?
** debug patch only **
Signed-off-by: Peter Hurley
---
drivers/tty/serial/serial_core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 5a78f69..1579a20 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2783,6 +2783,9 @@ void uart_handle_cts_change(struct uart_port *uport,
unsigned int status)
uport->icount.cts++;
if (tty_port_cts_enabled(port)) {
+
+ WARN_ON_ONCE(uport->flags & UPF_HARD_FLOW);
+
if (tty->hw_stopped) {
if (status) {
tty->hw_stopped = 0;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
On 09/11/2014 07:42 AM, Sebastian Andrzej Siewior wrote: > I also need a watchdog timer for TX since it seems that on omap3 the > DMA engine suddenly forgets to continue with DMA… One difference I noticed between the omap driver and the 8250 driver is the way modem status interrupts are handled. The omap driver only checks modem status for the UART_IIR_MSI interrupt type. The 8250 driver checks modem status at every interrupt (other than NO_INT). I think the UART_MSR_DCTS bit always reflects that the CTS input has changed between reads of the MSR _even if auto CTS is on_. So perhaps the hardware is being stopped by uart_handle_cts_change() when auto CTS is on? Regards, Peter Hurley [The UPF_HARD_FLOW thing was pretty much just done for omap even though 8250 already had auto CTS/auto RTS. Serial core hardware flow control support needs a redo as drivers have pretty much tacked stuff on randomly.] -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/16] tty: serial: Add 8250-core based omap driver
port->uartclk / 16 / 0x,
> + port->uartclk / 13);
> + omap_8250_get_divisor(port, baud, priv);
> +
> + /*
> + * Ok, we're now changing the port state. Do it with
> + * interrupts disabled.
> + */
> + pm_runtime_get_sync(port->dev);
> + spin_lock_irqsave(&port->lock, flags);
^^^
spin_lock_irq(&port->lock);
The serial core calls the ->set_termios() method with interrupts enabled.
> +
> + /*
> + * Update the per-port timeout.
> + */
> + uart_update_timeout(port, termios->c_cflag, baud);
> +
> + up->port.read_status_mask = UART_LSR_OE | UART_LSR_THRE | UART_LSR_DR;
> + if (termios->c_iflag & INPCK)
> + up->port.read_status_mask |= UART_LSR_FE | UART_LSR_PE;
> + if (termios->c_iflag & (BRKINT | PARMRK))
^
IGNBRK |
Otherwise, the read_status_mask will mask out the BI condition, so
uart_insert_char() will send '\0' byte as TTY_NORMAL.
The 8250 and omap RX path differed so the omap driver didn't need this
change, whereas the 8250 driver does.
> + up->port.read_status_mask |= UART_LSR_BI;
> +
> + /*
> + * Characters to ignore
> + */
> + up->port.ignore_status_mask = 0;
> + if (termios->c_iflag & IGNPAR)
> + up->port.ignore_status_mask |= UART_LSR_PE | UART_LSR_FE;
> + if (termios->c_iflag & IGNBRK) {
> + up->port.ignore_status_mask |= UART_LSR_BI;
> + /*
> + * If we're ignoring parity and break indicators,
> + * ignore overruns too (for real raw support).
> + */
> + if (termios->c_iflag & IGNPAR)
> + up->port.ignore_status_mask |= UART_LSR_OE;
> + }
> +
> + /*
> + * ignore all characters if CREAD is not set
> + */
> + if ((termios->c_cflag & CREAD) == 0)
> + up->port.ignore_status_mask |= UART_LSR_DR;
> +
> + /*
> + * Modem status interrupts
> + */
> + up->ier &= ~UART_IER_MSI;
> + if (UART_ENABLE_MS(&up->port, termios->c_cflag))
> + up->ier |= UART_IER_MSI;
> +
> + up->lcr = cval;
> + /* Up to here it was mostly serial8250_do_set_termios() */
> +
> + /*
> + * We enable TRIG_GRANU for RX and TX and additionaly we set
> + * SCR_TX_EMPTY bit. The result is the following:
> + * - RX_TRIGGER amount of bytes in the FIFO will cause an interrupt.
> + * - less than RX_TRIGGER number of bytes will also cause an interrupt
> + * once the UART decides that there no new bytes arriving.
> + * - Once THRE is enabled, the interrupt will be fired once the FIFO is
> + * empty - the trigger level is ignored here.
> + *
> + * Once DMA is enabled:
> + * - UART will assert the TX DMA line once there is room for TX_TRIGGER
> + * bytes in the TX FIFO. On each assert the DMA engine will move
> + * TX_TRIGGER bytes into the FIFO.
> + * - UART will assert the RX DMA line once there are RX_TRIGGER bytes in
> + * the FIFO and move RX_TRIGGER bytes.
> + * This is because treshold and trigger values are the same.
> + */
> + up->fcr = UART_FCR_ENABLE_FIFO;
> + up->fcr |= TRIGGER_FCR_MASK(TX_TRIGGER) << OMAP_UART_FCR_TX_TRIG;
> + up->fcr |= TRIGGER_FCR_MASK(RX_TRIGGER) << OMAP_UART_FCR_RX_TRIG;
> +
> + priv->scr = OMAP_UART_SCR_RX_TRIG_GRANU1_MASK | OMAP_UART_SCR_TX_EMPTY |
> + OMAP_UART_SCR_TX_TRIG_GRANU1_MASK;
> +
> + priv->xon = termios->c_cc[VSTART];
> + priv->xoff = termios->c_cc[VSTOP];
> +
> + priv->efr = 0;
> + if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW) {
> + /* Enable AUTORTS and AUTOCTS */
> + priv->efr |= UART_EFR_CTS | UART_EFR_RTS;
> +
> + /* Ensure MCR RTS is asserted */
> + up->mcr |= UART_MCR_RTS;
> + }
> +
> + if (up->port.flags & UPF_SOFT_FLOW) {
I'm aware that this is basically from the omap driver but can someone clear
up if omap hardware can actually do UPF_HARD_FLOW and UPF_SOFT_FLOW
simultaneously? The datasheets that I've looked at say no.
Regards,
Peter Hurley
> + /*
> + * IXON Flag:
> + * Enable XON/XOFF flow control on input.
> + * Receiver compares XON1, XOFF1.
> + */
> + if (termios->c_iflag & IXON)
> +
Re: [PATCH v7] 8250-core based serial driver for OMAP + DMA
On 08/15/2014 04:28 PM, Tony Lindgren wrote: > * Sebastian Andrzej Siewior [140815 12:16]: >> On 08/15/2014 08:17 PM, Lennart Sorensen wrote: >> >>> Are you saying that with the new driver you have to respond to the RX >>> irq faster than before to avoid overflows? It is not quite clear. >> >> Yes. The irq fires 46 bytes giving you 16 bytes buffer before overflow >> vs 63 bytes buffer the old one had. >> >>> I do think 4 interrupts to handle 4 bytes of date does seem a >>> tad inefficient, so dropping to 854 looks a lot nicer. Was the omap >>> driver not using the fifo trigger levels at all? >> >> It configured the trigger levels to 1 for RX and 16 for TX. > > Hmm that weird RX trigger level is a workaround for lost characters. > > See commit 0ba5f66836 (tty: serial: OMAP: use a 1-byte RX FIFO > threshold in PIO mode :) That commit looks like it should have been specific to the silicon exhibiting the rx timeout bug. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm
On 07/18/2014 11:31 AM, Felipe Balbi wrote: On Fri, Jul 18, 2014 at 10:35:10AM +0200, Sebastian Andrzej Siewior wrote: >On 07/17/2014 06:18 PM, Felipe Balbi wrote: > > >>No, this is okay. If you look, it checks for "up->ier & > >>UART_IER_THRI". On the second invocation it will see that this > >>bit is already set and therefore won't call get_sync() for the > >>second time. That bit is removed in the _stop_tx() path. > > > >oh, right. But that's actually unnecessary. Calling > >pm_runtime_get() multiple times will just increment the usage > >counter multiple times, which means you can call __stop_tx() > >multiple times too and everything gets balanced, right ? > >No. start_tx() will be called multiple times but only the first >invocation invoke pm_runtime_get(). Now I noticed that I forgot to right, but that's unnecessary. You can pm_runtime_get() every time start_tx() is called. Just make sure to put everytime stop_tx() is called too. The interface is asymmetric. start_tx() may be invoked multiple times for which only 1 interrupt will occur, and thus only invoke __stop_tx() once. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm
On 07/16/2014 12:06 PM, Felipe Balbi wrote: On Wed, Jul 16, 2014 at 05:54:56PM +0200, Sebastian Andrzej Siewior wrote: On 07/16/2014 05:16 PM, Felipe Balbi wrote: I wonder if you should get_sync() on start_tx() and only put_autosuspend() at stop_tx(). I guess the outcome would be largely the same, no ? I just opened minicom on ttyS0 and gave a try. start_tx() was invoked each time I pressed a key (sent a character). I haven't seen stop_tx() even after after I closed minicom. I guess stop_tx() is invoked if you switch half-duplex communication. that's bad, I expected stop to be called also after each character. The 8250 core auto-stops tx when the tx ring buffer is empty (except in the case of dma, where stopping tx isn't necessary). Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/11] Revert "serial: omap: unlock the port lock"
On 03/25/2014 02:28 PM, Tony Lindgren wrote: * Felipe Balbi [140320 12:39]: This reverts commit 0324a821029e1f54e7a7f8fed48693cfce42dc0e. That commit tried to fix a deadlock problem when using hci_ldisc, but it turns out the bug was in hci_ldsic all along where it was calling ->write() from within ->write_wakeup() callback. The problem is that ->write_wakeup() was called with port lock held and ->write() tried to grab the same port lock. Should this and the next patch be earlier in the series as a fix for the v3.15-rc cycle? Should they be cc: stable as well? Well, right now the other fix has had _zero_ testing so not really a -stable candidate just yet. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/11] Revert "serial: omap: unlock the port lock"
On 03/26/2014 10:10 PM, Felipe Balbi wrote: Hi, On Wed, Mar 26, 2014 at 08:39:11PM -0400, Peter Hurley wrote: On 03/25/2014 02:28 PM, Tony Lindgren wrote: * Felipe Balbi [140320 12:39]: This reverts commit 0324a821029e1f54e7a7f8fed48693cfce42dc0e. That commit tried to fix a deadlock problem when using hci_ldisc, but it turns out the bug was in hci_ldsic all along where it was calling ->write() from within ->write_wakeup() callback. The problem is that ->write_wakeup() was called with port lock held and ->write() tried to grab the same port lock. Should this and the next patch be earlier in the series as a fix for the v3.15-rc cycle? Should they be cc: stable as well? Well, right now the other fix has had _zero_ testing so not really a -stable candidate just yet. how can you even say that ? I misunderstood when you wrote: On 03/20/2014 02:11 PM, Felipe Balbi wrote: > here's a build-tested only patch which is waiting for testing from other > colleagues who've got a platform to reproduce the problem: and then the version I reviewed had no Tested-by: tags. Unless you work for some 3 letter acronym organizations, you have no clue about the fact that this was tested on a keystone 2 platform. Ok. How else would we have found the issue to start with ? Bug report? Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/11] bluetooth: hci_ldisc: fix deadlock condition
On 03/26/2014 10:09 PM, Felipe Balbi wrote: I just noticed this patch wasn't addressed to Marcel; seems like this should go through the bluetooth tree (but not through bluetooth-next because it fixes an oops). read the archives: http://marc.info/?l=linux-bluetooth&m=139534449409583&w=2 Sorry. I did actually get Marcel's reply but Thunderbird didn't parent the reply properly in my inbox and I forgot about it. Marcel, You may want to build on top of this patch split handling; I noticed some of the protocol drivers are calling hci_uart_tx_wakeup() from work functions already (so don't need to schedule another work...) I don't think that should be part of $subject, though. I don't understand what you mean here. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/11] bluetooth: hci_ldisc: fix deadlock condition
[ +to Marcel Holtmann ]
On 03/20/2014 03:30 PM, Felipe Balbi wrote:
LDISCs shouldn't call tty->ops->write() from within
->write_wakeup().
->write_wakeup() is called with port lock taken and
IRQs disabled, tty->ops->write() will try to acquire
the same port lock and we will deadlock.
Reviewed-by: Peter Hurley
Reported-by: Huang Shijie
Signed-off-by: Felipe Balbi
I just noticed this patch wasn't addressed to Marcel;
seems like this should go through the bluetooth tree (but not
through bluetooth-next because it fixes an oops).
Marcel,
You may want to build on top of this patch split handling;
I noticed some of the protocol drivers are calling
hci_uart_tx_wakeup() from work functions already (so don't
need to schedule another work...)
Regards,
Peter Hurley
---
drivers/bluetooth/hci_ldisc.c | 24 +++-
drivers/bluetooth/hci_uart.h | 1 +
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 6e06f6f..77af52f 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -118,10 +118,6 @@ static inline struct sk_buff *hci_uart_dequeue(struct
hci_uart *hu)
int hci_uart_tx_wakeup(struct hci_uart *hu)
{
- struct tty_struct *tty = hu->tty;
- struct hci_dev *hdev = hu->hdev;
- struct sk_buff *skb;
-
if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
return 0;
@@ -129,6 +125,22 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
BT_DBG("");
+ schedule_work(&hu->write_work);
+
+ return 0;
+}
+
+static void hci_uart_write_work(struct work_struct *work)
+{
+ struct hci_uart *hu = container_of(work, struct hci_uart, write_work);
+ struct tty_struct *tty = hu->tty;
+ struct hci_dev *hdev = hu->hdev;
+ struct sk_buff *skb;
+
+ /* REVISIT: should we cope with bad skbs or ->write() returning
+* and error value ?
+*/
+
restart:
clear_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
@@ -153,7 +165,6 @@ restart:
goto restart;
clear_bit(HCI_UART_SENDING, &hu->tx_state);
- return 0;
}
static void hci_uart_init_work(struct work_struct *work)
@@ -281,6 +292,7 @@ static int hci_uart_tty_open(struct tty_struct *tty)
tty->receive_room = 65536;
INIT_WORK(&hu->init_ready, hci_uart_init_work);
+ INIT_WORK(&hu->write_work, hci_uart_write_work);
spin_lock_init(&hu->rx_lock);
@@ -318,6 +330,8 @@ static void hci_uart_tty_close(struct tty_struct *tty)
if (hdev)
hci_uart_close(hdev);
+ cancel_work_sync(&hu->write_work);
+
if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
if (hdev) {
if (test_bit(HCI_UART_REGISTERED, &hu->flags))
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index fffa61f..12df101 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -68,6 +68,7 @@ struct hci_uart {
unsigned long hdev_flags;
struct work_struct init_ready;
+ struct work_struct write_work;
struct hci_uart_proto *proto;
void*priv;
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
