diff mbox series

[v3] tty: serial: samsung_tty: loopback mode support

Message ID 20220629004141.51484-1-chanho61.park@samsung.com (mailing list archive)
State New, archived
Headers show
Series [v3] tty: serial: samsung_tty: loopback mode support | expand

Commit Message

Chanho Park June 29, 2022, 12:41 a.m. UTC
Internal loopback mode can be supported by setting
UCON register's Loopback Mode bit. The mode & bit can be supported
since s3c2410 and later SoCs. The prefix of LOOPBACK / BIT(5) naming
should be also changed to S3C2410_ in order to avoid confusion.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
Changes from v2:
- Add Ilop and Krzysztof's R-B tags
- Drop linux-serial-test related commit messages
- Wrap commit messages at 72 columns

Changes from v1:
- Drop TIOCM_LOOP return from s3c24xx_serial_get_mctrl as pointed out
  by Ilpo. Documentation/driver-api/serial/driver.rst indicates the
  bit is only for set_mctrl.
- Change the loopback bit definition from S3C2443_UCON_LOOPBACK to
  S3C2410_UCON_LOOPBACK because it has been supported since s3c2410.
- Remove the unnecessary footnote and put a blank line before
  Signed-off-by

 drivers/tty/serial/samsung_tty.c | 8 ++++++++
 include/linux/serial_s3c.h       | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

Comments

Marek Szyprowski July 11, 2022, 12:31 p.m. UTC | #1
Hi Chanho,

On 29.06.2022 02:41, Chanho Park wrote:
> Internal loopback mode can be supported by setting
> UCON register's Loopback Mode bit. The mode & bit can be supported
> since s3c2410 and later SoCs. The prefix of LOOPBACK / BIT(5) naming
> should be also changed to S3C2410_ in order to avoid confusion.
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> ---
> Changes from v2:
> - Add Ilop and Krzysztof's R-B tags
> - Drop linux-serial-test related commit messages
> - Wrap commit messages at 72 columns
>
> Changes from v1:
> - Drop TIOCM_LOOP return from s3c24xx_serial_get_mctrl as pointed out
>    by Ilpo. Documentation/driver-api/serial/driver.rst indicates the
>    bit is only for set_mctrl.
> - Change the loopback bit definition from S3C2443_UCON_LOOPBACK to
>    S3C2410_UCON_LOOPBACK because it has been supported since s3c2410.
> - Remove the unnecessary footnote and put a blank line before
>    Signed-off-by
>
>   drivers/tty/serial/samsung_tty.c | 8 ++++++++
>   include/linux/serial_s3c.h       | 2 +-
>   2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index d5ca904def34..03ef4ff506fd 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -1012,6 +1012,7 @@ static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
>   static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
>   {
>   	unsigned int umcon = rd_regl(port, S3C2410_UMCON);
> +	unsigned int ucon = rd_reg(port, S3C2410_UCON);
>   
>   	if (mctrl & TIOCM_RTS)
>   		umcon |= S3C2410_UMCOM_RTS_LOW;
> @@ -1019,6 +1020,13 @@ static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
>   		umcon &= ~S3C2410_UMCOM_RTS_LOW;
>   
>   	wr_regl(port, S3C2410_UMCON, umcon);
> +
> +	if (mctrl & TIOCM_LOOP)
> +		ucon |= S3C2410_UCON_LOOPBACK;
> +	else
> +		ucon &= ~S3C2410_UCON_LOOPBACK;
> +
> +	wr_regl(port, S3C2410_UCON, ucon);

S3C2410_UCON must be modified at least under the local_irq_save() or port lock, otherwise it breaks kernel console operation on older Exynos SoCs (especially when DMA mode is used).

The above read-modify-write sequence should be replaced with the following pattern:

         if (mctrl & TIOCM_LOOP)
                 s3c24xx_set_bit(port, S3C2410_UCON_LOOPBACK, S3C2410_UCON);
         else
                 s3c24xx_clear_bit(port, S3C2410_UCON_LOOPBACK, S3C2410_UCON);


>   }
>   
>   static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
> diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
> index dec15f5b3dec..1672cf0810ef 100644
> --- a/include/linux/serial_s3c.h
> +++ b/include/linux/serial_s3c.h
> @@ -83,7 +83,7 @@
>   #define S3C2410_UCON_RXIRQMODE	  (1<<0)
>   #define S3C2410_UCON_RXFIFO_TOI	  (1<<7)
>   #define S3C2443_UCON_RXERR_IRQEN  (1<<6)
> -#define S3C2443_UCON_LOOPBACK	  (1<<5)
> +#define S3C2410_UCON_LOOPBACK	  (1<<5)
>   
>   #define S3C2410_UCON_DEFAULT	  (S3C2410_UCON_TXILEVEL  | \
>   				   S3C2410_UCON_RXILEVEL  | \

Best regards
Chanho Park July 12, 2022, 12:44 a.m. UTC | #2
> > +	if (mctrl & TIOCM_LOOP)
> > +		ucon |= S3C2410_UCON_LOOPBACK;
> > +	else
> > +		ucon &= ~S3C2410_UCON_LOOPBACK;
> > +
> > +	wr_regl(port, S3C2410_UCON, ucon);
> 
> S3C2410_UCON must be modified at least under the local_irq_save() or port
> lock, otherwise it breaks kernel console operation on older Exynos SoCs
> (especially when DMA mode is used).
> 
> The above read-modify-write sequence should be replaced with the following
> pattern:
> 
>          if (mctrl & TIOCM_LOOP)
>                  s3c24xx_set_bit(port, S3C2410_UCON_LOOPBACK, S3C2410_UCON);
>          else
>                  s3c24xx_clear_bit(port, S3C2410_UCON_LOOPBACK, S3C2410_UCON);

All the set_mctrl() call can be protected by spin_[un]lock_irq[save/restore] with port->lock.
So, I think it is not required.

Best Regards,
Chanho Park
Marek Szyprowski July 12, 2022, 1:58 p.m. UTC | #3
On 12.07.2022 02:44, Chanho Park wrote:
>>> +	if (mctrl & TIOCM_LOOP)
>>> +		ucon |= S3C2410_UCON_LOOPBACK;
>>> +	else
>>> +		ucon &= ~S3C2410_UCON_LOOPBACK;
>>> +
>>> +	wr_regl(port, S3C2410_UCON, ucon);
>> S3C2410_UCON must be modified at least under the local_irq_save() or port
>> lock, otherwise it breaks kernel console operation on older Exynos SoCs
>> (especially when DMA mode is used).
>>
>> The above read-modify-write sequence should be replaced with the following
>> pattern:
>>
>>           if (mctrl & TIOCM_LOOP)
>>                   s3c24xx_set_bit(port, S3C2410_UCON_LOOPBACK, S3C2410_UCON);
>>           else
>>                   s3c24xx_clear_bit(port, S3C2410_UCON_LOOPBACK, S3C2410_UCON);
> All the set_mctrl() call can be protected by spin_[un]lock_irq[save/restore] with port->lock.
> So, I think it is not required.

Right. The problem is elsewhere. You used rd_reg(port, S3C2410_UCON) 
instead of rd_regl(port, S3C2410_UCON) what always zeroed upper bits. I 
will send a fix in a few minutes.


Best regards
Chanho Park July 12, 2022, 11:55 p.m. UTC | #4
> Right. The problem is elsewhere. You used rd_reg(port, S3C2410_UCON)
> instead of rd_regl(port, S3C2410_UCON) what always zeroed upper bits. I
> will send a fix in a few minutes.

Oops. You're right. Thanks for pointing it out and send the fix.

Best Regards,
Chanho Park
diff mbox series

Patch

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index d5ca904def34..03ef4ff506fd 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1012,6 +1012,7 @@  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
 static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 	unsigned int umcon = rd_regl(port, S3C2410_UMCON);
+	unsigned int ucon = rd_reg(port, S3C2410_UCON);
 
 	if (mctrl & TIOCM_RTS)
 		umcon |= S3C2410_UMCOM_RTS_LOW;
@@ -1019,6 +1020,13 @@  static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
 		umcon &= ~S3C2410_UMCOM_RTS_LOW;
 
 	wr_regl(port, S3C2410_UMCON, umcon);
+
+	if (mctrl & TIOCM_LOOP)
+		ucon |= S3C2410_UCON_LOOPBACK;
+	else
+		ucon &= ~S3C2410_UCON_LOOPBACK;
+
+	wr_regl(port, S3C2410_UCON, ucon);
 }
 
 static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
index dec15f5b3dec..1672cf0810ef 100644
--- a/include/linux/serial_s3c.h
+++ b/include/linux/serial_s3c.h
@@ -83,7 +83,7 @@ 
 #define S3C2410_UCON_RXIRQMODE	  (1<<0)
 #define S3C2410_UCON_RXFIFO_TOI	  (1<<7)
 #define S3C2443_UCON_RXERR_IRQEN  (1<<6)
-#define S3C2443_UCON_LOOPBACK	  (1<<5)
+#define S3C2410_UCON_LOOPBACK	  (1<<5)
 
 #define S3C2410_UCON_DEFAULT	  (S3C2410_UCON_TXILEVEL  | \
 				   S3C2410_UCON_RXILEVEL  | \