All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	linux-serial <linux-serial@vger.kernel.org>,
	"Eric Tremblay" <etremblay@distech-controls.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Lukas Wunner" <lukas.wunner@intel.com>,
	kernel@pengutronix.de,
	"Heiko Stuebner" <heiko.stuebner@theobroma-systems.com>,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	"Jiri Slaby" <jirislaby@kernel.org>,
	"Giulio Benetti" <giulio.benetti@micronovasrl.com>
Subject: Re: [PATCH v3 1/3] serial: 8250: Handle UART without interrupt on TEMT using em485
Date: Tue, 19 Apr 2022 11:09:56 +0300 (EEST)	[thread overview]
Message-ID: <893fcf29-20da-2a4-5951-d956472f85f@linux.intel.com> (raw)
In-Reply-To: <174a96fe-8953-6533-34d7-26c15b8f140@linux.intel.com>

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

Hi Greg,

This change now appears in your tty-next tree. As you seem to have missed 
there is an obvious problem with it, I'm asking which direction I should 
take to fix it.

My RS485 patchset requires frame timing information also for other 
purpose besides notemt and it seems some other drivers (and even serial 
core itself) seem to care about frame time so I was aiming to make it 
available more generally rather than per-purpose like it is now.

Should I just submit a patch which makes the div 64-bit fixing this change 
or should I pursue the alternative approach that is part of my RS485 
patchset (patches 1-2) that generalizes availability of the frame timing 
information and effectively reverts the addition of the extra notemt timer 
added by this change?


On Thu, 31 Mar 2022, Ilpo Järvinen wrote:
> On Wed, 30 Mar 2022, Uwe Kleine-König wrote:
> > On Wed, Mar 30, 2022 at 02:20:01PM +0300, Ilpo Järvinen wrote:
> > > On Wed, 30 Mar 2022, Uwe Kleine-König wrote:
> > > > From: Eric Tremblay <etremblay@distech-controls.com>
> > > > 
> > > > Introduce the UART_CAP_NOTEMT capability. The capability indicates that
> > > > the UART doesn't have an interrupt available on TEMT.
> > > > 
> > > > In the case where the device does not support it, we calculate the
> > > > maximum time it could take for the transmitter to empty the
> > > > shift register. When we get in the situation where we get the
> > > > THRE interrupt, we check if the TEMT bit is set. If it's not, we start
> > > > the a timer and recall __stop_tx() after the delay.
> > > > 
> > > > The transmit sequence is a bit modified when the capability is set. The
> > > > new timer is used between the last interrupt(THRE) and a potential
> > > > stop_tx timer.
> > > 
> > > As a general note on this patch, I've also made a version of this patch 
> > > which I intended to send among my dw rs485 v2 patchset once the merge 
> > > window is over. I believe my approach is cleaner than this one. It is 
> > > based on your suggestion on simply taking advantage of stop_tx_timer.
> > > In addition, I added frame_time into uart_port which removes the need
> > > for drivers to calculate the timing per usecase themselves (I believe 
> > > frame_time could replace the timeout in uart_port entirely).
> > >
> > > > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > > [moved to use added UART_CAP_TEMT]
> > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > > [moved to use added UART_CAP_NOTEMT, improve timeout]
> > > > Signed-off-by: Eric Tremblay <etremblay@distech-controls.com>
> > > > [rebased to v5.17, making use of tty_get_frame_size]
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  drivers/tty/serial/8250/8250.h      |  1 +
> > > >  drivers/tty/serial/8250/8250_port.c | 76 ++++++++++++++++++++++++++++-
> > > >  include/linux/serial_8250.h         |  2 +
> > > >  3 files changed, 77 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > > > index db784ace25d8..39ffeb37786f 100644
> > > > --- a/drivers/tty/serial/8250/8250.h
> > > > +++ b/drivers/tty/serial/8250/8250.h
> > > > @@ -83,6 +83,7 @@ struct serial8250_config {
> > > >  #define UART_CAP_MINI	BIT(17)	/* Mini UART on BCM283X family lacks:
> > > >  					 * STOP PARITY EPAR SPAR WLEN5 WLEN6
> > > >  					 */
> > > > +#define UART_CAP_NOTEMT	BIT(18)	/* UART without interrupt on TEMT available */
> > > >  
> > > >  #define UART_BUG_QUOT	BIT(0)	/* UART has buggy quot LSB */
> > > >  #define UART_BUG_TXEN	BIT(1)	/* UART has buggy TX IIR status */
> > > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > > > index 3b12bfc1ed67..0af13b4c76a0 100644
> > > > --- a/drivers/tty/serial/8250/8250_port.c
> > > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > > @@ -563,8 +563,21 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
> > > >  	}
> > > >  }
> > > >  
> > > > +static inline void serial8250_em485_update_temt_delay(struct uart_8250_port *p,
> > > > +			unsigned int cflag, unsigned int baud)
> > > > +{
> > > > +	unsigned int bits;
> > > > +
> > > > +	if (!p->em485)
> > > > +		return;
> > > > +
> > > > +	bits = tty_get_frame_size(cflag);
> > > > +	p->em485->no_temt_delay = DIV_ROUND_UP(bits * NSEC_PER_SEC, baud);
> > > 
> > > This is guaranteed to overflow on some archs?
> > 
> > Hmm, indeed, even overflows for the usual bits=10. Strange it still
> > worked for me in my tests. Maybe the irq latency is big enough to
> > explain this.


-- 
 i.

  reply	other threads:[~2022-04-19  8:10 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-30 10:46 [PATCH v3 0/3] Handle UART without interrupt on TEMT using em485 Uwe Kleine-König
2022-03-30 10:46 ` [PATCH v3 1/3] serial: 8250: " Uwe Kleine-König
2022-03-30 11:20   ` Ilpo Järvinen
2022-03-30 14:21     ` Uwe Kleine-König
2022-03-31  8:03       ` Ilpo Järvinen
2022-04-19  8:09         ` Ilpo Järvinen [this message]
2022-04-19 10:11           ` Greg Kroah-Hartman
2022-04-19 10:36             ` Ilpo Järvinen
2022-04-19 10:41               ` Greg Kroah-Hartman
2022-03-30 10:46 ` [PATCH v3 2/3] serial: 8250: Add UART_CAP_NOTEMT on PORT_16550A_FSL64 Uwe Kleine-König
2022-03-30 10:46 ` [PATCH v3 3/3] serial: 8250: add compatible for fsl,16550-FIFO64 Uwe Kleine-König

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=893fcf29-20da-2a4-5951-d956472f85f@linux.intel.com \
    --to=ilpo.jarvinen@linux.intel.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=etremblay@distech-controls.com \
    --cc=giulio.benetti@micronovasrl.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=heiko.stuebner@theobroma-systems.com \
    --cc=jirislaby@kernel.org \
    --cc=kernel@pengutronix.de \
    --cc=linux-serial@vger.kernel.org \
    --cc=lukas.wunner@intel.com \
    --cc=u.kleine-koenig@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.