| Message ID | 1433136067-20644-3-git-send-email-bhuvanchandra.dv@toradex.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Mon, Jun 01, 2015 at 10:51:06AM +0530, Bhuvanchandra DV wrote: > The LPUART does not provide manual control of RTS/CTS signals, > those can only be controlled by the hardware directly. Therefore > manual control of those signals through mctrl can not be provided. > The current implementation enables/disables the automatic control, > which is not what mctrl should do, hence remove the incorrect > implementation. > > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com> > --- > drivers/tty/serial/fsl_lpuart.c | 63 +++++------------------------------------ > 1 file changed, 7 insertions(+), 56 deletions(-) > > diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c > index 08ce76f..532cfb7 100644 > --- a/drivers/tty/serial/fsl_lpuart.c > +++ b/drivers/tty/serial/fsl_lpuart.c > @@ -822,64 +822,15 @@ static unsigned int lpuart32_tx_empty(struct uart_port *port) > > static unsigned int lpuart_get_mctrl(struct uart_port *port) > { > - unsigned int temp = 0; > - unsigned char reg; > - > - reg = readb(port->membase + UARTMODEM); > - if (reg & UARTMODEM_TXCTSE) > - temp |= TIOCM_CTS; > - > - if (reg & UARTMODEM_RXRTSE) > - temp |= TIOCM_RTS; > - > - return temp; From reading the commit log I would expect that you only touch the set_mctrl function, but not get_mctrl. Assuming your code change is right, can you mention this in the commit log please? The bits UARTMODEM_TXCTSE and UARTMODEM_RXRTSE only control the automatic mode? What is the problem you're fixing here? I'm not sure how such an UART should be handled, but I imagine that you want to make use of automatic mode in some cases. Greg? Best regards Uwe
Hello, On 06/08/2015 12:11 PM, Uwe Kleine-König wrote: > Hello, > > On Mon, Jun 01, 2015 at 10:51:06AM +0530, Bhuvanchandra DV wrote: >> The LPUART does not provide manual control of RTS/CTS signals, >> those can only be controlled by the hardware directly. Therefore >> manual control of those signals through mctrl can not be provided. >> The current implementation enables/disables the automatic control, >> which is not what mctrl should do, hence remove the incorrect >> implementation. >> >> Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com> >> --- >> drivers/tty/serial/fsl_lpuart.c | 63 +++++------------------------------------ >> 1 file changed, 7 insertions(+), 56 deletions(-) >> >> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c >> index 08ce76f..532cfb7 100644 >> --- a/drivers/tty/serial/fsl_lpuart.c >> +++ b/drivers/tty/serial/fsl_lpuart.c >> @@ -822,64 +822,15 @@ static unsigned int lpuart32_tx_empty(struct uart_port *port) >> >> static unsigned int lpuart_get_mctrl(struct uart_port *port) >> { >> - unsigned int temp = 0; >> - unsigned char reg; >> - >> - reg = readb(port->membase + UARTMODEM); >> - if (reg & UARTMODEM_TXCTSE) >> - temp |= TIOCM_CTS; >> - >> - if (reg & UARTMODEM_RXRTSE) >> - temp |= TIOCM_RTS; >> - >> - return temp; > From reading the commit log I would expect that you only touch the > set_mctrl function, but not get_mctrl. Assuming your code change is > right, can you mention this in the commit log please? The bits > UARTMODEM_TXCTSE and UARTMODEM_RXRTSE only control the automatic mode? OK, will use 'get/set_mctrl' instead of 'mctrl' in the commit log. Yes, those bits are only responsible for enabling/disabling the auto hardware flow control, not for controlling the RTS/CTS signals directly. > > What is the problem you're fixing here? I'm not sure how such an UART > should be handled, but I imagine that you want to make use of automatic > mode in some cases. Greg? > Fixing the implementation of hardware flow control wrt LPUART hardware. > Best regards > Uwe > Best regards, Bhuvan
On Mon, Jun 01, 2015 at 10:51:06AM +0530, Bhuvanchandra DV wrote: > + /* > + * CTS/RTS can _only_ be handled > + * automatically by the hardware. > + */ That's really not acceptable. You need to do something here so that you can _force_ RTS to be deasserted when the kernel buffers fill up - be that by stopping reading characters and letting the FIFO fill up or something of that ilk. Ignoring the rest of the flow control system in the kernel is not really acceptable.
On Mon, Jun 08, 2015 at 08:41:13AM +0200, Uwe Kleine-König wrote: > Hello, > > On Mon, Jun 01, 2015 at 10:51:06AM +0530, Bhuvanchandra DV wrote: > > The LPUART does not provide manual control of RTS/CTS signals, > > those can only be controlled by the hardware directly. Therefore > > manual control of those signals through mctrl can not be provided. > > The current implementation enables/disables the automatic control, > > which is not what mctrl should do, hence remove the incorrect > > implementation. > > > > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com> > > --- > > drivers/tty/serial/fsl_lpuart.c | 63 +++++------------------------------------ > > 1 file changed, 7 insertions(+), 56 deletions(-) > > > > diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c > > index 08ce76f..532cfb7 100644 > > --- a/drivers/tty/serial/fsl_lpuart.c > > +++ b/drivers/tty/serial/fsl_lpuart.c > > @@ -822,64 +822,15 @@ static unsigned int lpuart32_tx_empty(struct uart_port *port) > > > > static unsigned int lpuart_get_mctrl(struct uart_port *port) > > { > > - unsigned int temp = 0; > > - unsigned char reg; > > - > > - reg = readb(port->membase + UARTMODEM); > > - if (reg & UARTMODEM_TXCTSE) > > - temp |= TIOCM_CTS; > > - > > - if (reg & UARTMODEM_RXRTSE) > > - temp |= TIOCM_RTS; > > - > > - return temp; > >From reading the commit log I would expect that you only touch the > set_mctrl function, but not get_mctrl. Assuming your code change is > right, can you mention this in the commit log please? The bits > UARTMODEM_TXCTSE and UARTMODEM_RXRTSE only control the automatic mode? Dumb serial writers common fault #1 is to make get_mctrl return TIOCM_RTS and/or TIOCM_DTR. It should never return these. This is a sure sign that the serial writer doesn't know what they're doing, and doesn't understand the interface they're implementing. That's a good enough reason IMHO to reject a driver prior to merging.
On Mon, Jun 08, 2015 at 08:41:13AM +0200, Uwe Kleine-König wrote: > >From reading the commit log I would expect that you only touch the > set_mctrl function, but not get_mctrl. Assuming your code change is > right, can you mention this in the commit log please? The bits > UARTMODEM_TXCTSE and UARTMODEM_RXRTSE only control the automatic mode? And... another point. UARTMODEM_TXCTSE and UARTMODEM_RXRTSE are the auto flow control _enable_ signals. That's not something that get_mctrl() should ever return. get_mctrl() is about reporting the actual current state of the CTS, DSR, DCD and RI signals _only_. If the hardware doesn't let you read the state, then reporting that CTS, DSR and DCD are all asserted is recommended, as that's the state needed to make the UART do useful stuff. So that part of this patch is the sane thing to do. What isn't sane is to use the auto-cts enable bit to report a ficticious current CTS state.
On 06/08/2015 02:42 PM, Russell King - ARM Linux wrote: > On Mon, Jun 01, 2015 at 10:51:06AM +0530, Bhuvanchandra DV wrote: >> + /* >> + * CTS/RTS can _only_ be handled >> + * automatically by the hardware. >> + */ > > That's really not acceptable. You need to do something here so that you > can _force_ RTS to be deasserted when the kernel buffers fill up - be > that by stopping reading characters and letting the FIFO fill up or > something of that ilk. Agreed, will fix this. > > Ignoring the rest of the flow control system in the kernel is not really > acceptable. > Best regards, Bhuvan
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c index 08ce76f..532cfb7 100644 --- a/drivers/tty/serial/fsl_lpuart.c +++ b/drivers/tty/serial/fsl_lpuart.c @@ -822,64 +822,15 @@ static unsigned int lpuart32_tx_empty(struct uart_port *port) static unsigned int lpuart_get_mctrl(struct uart_port *port) { - unsigned int temp = 0; - unsigned char reg; - - reg = readb(port->membase + UARTMODEM); - if (reg & UARTMODEM_TXCTSE) - temp |= TIOCM_CTS; - - if (reg & UARTMODEM_RXRTSE) - temp |= TIOCM_RTS; - - return temp; -} - -static unsigned int lpuart32_get_mctrl(struct uart_port *port) -{ - unsigned int temp = 0; - unsigned long reg; - - reg = lpuart32_read(port->membase + UARTMODIR); - if (reg & UARTMODIR_TXCTSE) - temp |= TIOCM_CTS; - - if (reg & UARTMODIR_RXRTSE) - temp |= TIOCM_RTS; - - return temp; + return TIOCM_CAR | TIOCM_CTS | TIOCM_DSR; } static void lpuart_set_mctrl(struct uart_port *port, unsigned int mctrl) { - unsigned char temp; - - temp = readb(port->membase + UARTMODEM) & - ~(UARTMODEM_RXRTSE | UARTMODEM_TXCTSE); - - if (mctrl & TIOCM_RTS) - temp |= UARTMODEM_RXRTSE; - - if (mctrl & TIOCM_CTS) - temp |= UARTMODEM_TXCTSE; - - writeb(temp, port->membase + UARTMODEM); -} - -static void lpuart32_set_mctrl(struct uart_port *port, unsigned int mctrl) -{ - unsigned long temp; - - temp = lpuart32_read(port->membase + UARTMODIR) & - ~(UARTMODIR_RXRTSE | UARTMODIR_TXCTSE); - - if (mctrl & TIOCM_RTS) - temp |= UARTMODIR_RXRTSE; - - if (mctrl & TIOCM_CTS) - temp |= UARTMODIR_TXCTSE; - - lpuart32_write(temp, port->membase + UARTMODIR); + /* + * CTS/RTS can _only_ be handled + * automatically by the hardware. + */ } static void lpuart_break_ctl(struct uart_port *port, int break_state) @@ -1515,8 +1466,8 @@ static struct uart_ops lpuart_pops = { static struct uart_ops lpuart32_pops = { .tx_empty = lpuart32_tx_empty, - .set_mctrl = lpuart32_set_mctrl, - .get_mctrl = lpuart32_get_mctrl, + .set_mctrl = lpuart_set_mctrl, + .get_mctrl = lpuart_get_mctrl, .stop_tx = lpuart32_stop_tx, .start_tx = lpuart32_start_tx, .stop_rx = lpuart32_stop_rx,
The LPUART does not provide manual control of RTS/CTS signals, those can only be controlled by the hardware directly. Therefore manual control of those signals through mctrl can not be provided. The current implementation enables/disables the automatic control, which is not what mctrl should do, hence remove the incorrect implementation. Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com> --- drivers/tty/serial/fsl_lpuart.c | 63 +++++------------------------------------ 1 file changed, 7 insertions(+), 56 deletions(-)