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

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

[Chanho Park]

Internal loopback mode can be supported by setting
S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since
s3c2410 and later SoCs. We can test it by linux-serial-test program[1]
with -k option. It will set TIOCM_LOOP mode during test.

-k, --loopback     Use internal hardware loop back

[1]: https://github.com/cbrake/linux-serial-test
Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
 drivers/tty/serial/samsung_tty.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index d5ca904def34..d7d035cd95c0 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1002,16 +1002,22 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
 {
 	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
+	unsigned int ucon = rd_reg(port, S3C2410_UCON);
+	unsigned int mctrl = TIOCM_CAR | TIOCM_DSR;
 
 	if (umstat & S3C2410_UMSTAT_CTS)
-		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
-	else
-		return TIOCM_CAR | TIOCM_DSR;
+		mctrl |= TIOCM_CTS;
+
+	if (ucon & S3C2443_UCON_LOOPBACK)
+		mctrl |= TIOCM_LOOP;
+
+	return mctrl;
 }
 
 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 +1025,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 |= S3C2443_UCON_LOOPBACK;
+	else
+		ucon &= ~S3C2443_UCON_LOOPBACK;
+
+	wr_regl(port, S3C2410_UCON, ucon);
 }
 
 static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
-- 
2.36.1

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

[Chanho Park]

Internal loopback mode can be supported by setting
S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since
s3c2410 and later SoCs. We can test it by linux-serial-test program[1]
with -k option. It will set TIOCM_LOOP mode during test.

-k, --loopback     Use internal hardware loop back

[1]: https://github.com/cbrake/linux-serial-test
Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
 drivers/tty/serial/samsung_tty.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index d5ca904def34..d7d035cd95c0 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1002,16 +1002,22 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
 {
 	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
+	unsigned int ucon = rd_reg(port, S3C2410_UCON);
+	unsigned int mctrl = TIOCM_CAR | TIOCM_DSR;
 
 	if (umstat & S3C2410_UMSTAT_CTS)
-		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
-	else
-		return TIOCM_CAR | TIOCM_DSR;
+		mctrl |= TIOCM_CTS;
+
+	if (ucon & S3C2443_UCON_LOOPBACK)
+		mctrl |= TIOCM_LOOP;
+
+	return mctrl;
 }
 
 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 +1025,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 |= S3C2443_UCON_LOOPBACK;
+	else
+		ucon &= ~S3C2443_UCON_LOOPBACK;
+
+	wr_regl(port, S3C2410_UCON, ucon);
 }
 
 static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: serial: samsung_tty: loopback mode support

[Krzysztof Kozlowski]

On 27/06/2022 05:23, Chanho Park wrote:
> Internal loopback mode can be supported by setting
> S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since
> s3c2410 and later SoCs. We can test it by linux-serial-test program[1]
> with -k option. It will set TIOCM_LOOP mode during test.
> 
> -k, --loopback     Use internal hardware loop back
> 
> [1]: https://github.com/cbrake/linux-serial-test
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> ---
>  drivers/tty/serial/samsung_tty.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: serial: samsung_tty: loopback mode support

[Krzysztof Kozlowski]

On 27/06/2022 05:23, Chanho Park wrote:
> Internal loopback mode can be supported by setting
> S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since
> s3c2410 and later SoCs. We can test it by linux-serial-test program[1]
> with -k option. It will set TIOCM_LOOP mode during test.
> 
> -k, --loopback     Use internal hardware loop back
> 
> [1]: https://github.com/cbrake/linux-serial-test
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> ---
>  drivers/tty/serial/samsung_tty.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

Re: [PATCH] tty: serial: samsung_tty: loopback mode support

[Ilpo Järvinen]

On Mon, 27 Jun 2022, Chanho Park wrote:

> Internal loopback mode can be supported by setting
> S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since
> s3c2410 and later SoCs.

In that case, why is the  LOOPBACK define named 2443 and not 2410???

The change looks fine otherwise.

I note though that many of the current drivers won't return TOICM_LOOP 
from ->get_mctrl() but I don't think it's exactly wrong to return it 
either. Perhaps lack of returning it is due to 
Documentation/driver-api/serial/driver.rst not including TOICM_LOOP in 
get_mctrl's list of information but only in set_mctrl's one.

-- 
 i.

> We can test it by linux-serial-test program[1]
> with -k option. It will set TIOCM_LOOP mode during test.
> 
> -k, --loopback     Use internal hardware loop back
> 
> [1]: https://github.com/cbrake/linux-serial-test
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> ---
>  drivers/tty/serial/samsung_tty.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index d5ca904def34..d7d035cd95c0 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -1002,16 +1002,22 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
>  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
>  {
>  	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
> +	unsigned int ucon = rd_reg(port, S3C2410_UCON);
> +	unsigned int mctrl = TIOCM_CAR | TIOCM_DSR;
>  
>  	if (umstat & S3C2410_UMSTAT_CTS)
> -		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
> -	else
> -		return TIOCM_CAR | TIOCM_DSR;
> +		mctrl |= TIOCM_CTS;
> +
> +	if (ucon & S3C2443_UCON_LOOPBACK)
> +		mctrl |= TIOCM_LOOP;
> +
> +	return mctrl;
>  }
>  
>  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 +1025,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 |= S3C2443_UCON_LOOPBACK;
> +	else
> +		ucon &= ~S3C2443_UCON_LOOPBACK;
> +
> +	wr_regl(port, S3C2410_UCON, ucon);
>  }
>  
>  static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: serial: samsung_tty: loopback mode support

[Ilpo Järvinen]

On Mon, 27 Jun 2022, Chanho Park wrote:

> Internal loopback mode can be supported by setting
> S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since
> s3c2410 and later SoCs.

In that case, why is the  LOOPBACK define named 2443 and not 2410???

The change looks fine otherwise.

I note though that many of the current drivers won't return TOICM_LOOP 
from ->get_mctrl() but I don't think it's exactly wrong to return it 
either. Perhaps lack of returning it is due to 
Documentation/driver-api/serial/driver.rst not including TOICM_LOOP in 
get_mctrl's list of information but only in set_mctrl's one.

-- 
 i.

> We can test it by linux-serial-test program[1]
> with -k option. It will set TIOCM_LOOP mode during test.
> 
> -k, --loopback     Use internal hardware loop back
> 
> [1]: https://github.com/cbrake/linux-serial-test
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> ---
>  drivers/tty/serial/samsung_tty.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index d5ca904def34..d7d035cd95c0 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -1002,16 +1002,22 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
>  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
>  {
>  	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
> +	unsigned int ucon = rd_reg(port, S3C2410_UCON);
> +	unsigned int mctrl = TIOCM_CAR | TIOCM_DSR;
>  
>  	if (umstat & S3C2410_UMSTAT_CTS)
> -		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
> -	else
> -		return TIOCM_CAR | TIOCM_DSR;
> +		mctrl |= TIOCM_CTS;
> +
> +	if (ucon & S3C2443_UCON_LOOPBACK)
> +		mctrl |= TIOCM_LOOP;
> +
> +	return mctrl;
>  }
>  
>  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 +1025,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 |= S3C2443_UCON_LOOPBACK;
> +	else
> +		ucon &= ~S3C2443_UCON_LOOPBACK;
> +
> +	wr_regl(port, S3C2410_UCON, ucon);
>  }
>  
>  static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
> 

Re: [PATCH] tty: serial: samsung_tty: loopback mode support

[Greg Kroah-Hartman]

On Mon, Jun 27, 2022 at 12:23:53PM +0900, Chanho Park wrote:
> Internal loopback mode can be supported by setting
> S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since
> s3c2410 and later SoCs. We can test it by linux-serial-test program[1]
> with -k option. It will set TIOCM_LOOP mode during test.
> 
> -k, --loopback     Use internal hardware loop back

I do not understand this line in the changelog text, what is it for?

> 
> [1]: https://github.com/cbrake/linux-serial-test
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>

Our tools require a blank line before the signed-off-by line.  Did you
not get a warning when checking this patch before sending it out?

Also, no need for a footnote in a changelog that is so short, just put
the link in the body of the text itself.

thanks,

greg k-h

Re: [PATCH] tty: serial: samsung_tty: loopback mode support

[Greg Kroah-Hartman]

On Mon, Jun 27, 2022 at 12:23:53PM +0900, Chanho Park wrote:
> Internal loopback mode can be supported by setting
> S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since
> s3c2410 and later SoCs. We can test it by linux-serial-test program[1]
> with -k option. It will set TIOCM_LOOP mode during test.
> 
> -k, --loopback     Use internal hardware loop back

I do not understand this line in the changelog text, what is it for?

> 
> [1]: https://github.com/cbrake/linux-serial-test
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>

Our tools require a blank line before the signed-off-by line.  Did you
not get a warning when checking this patch before sending it out?

Also, no need for a footnote in a changelog that is so short, just put
the link in the body of the text itself.

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] tty: serial: samsung_tty: loopback mode support

[Chanho Park]

> Subject: Re: [PATCH] tty: serial: samsung_tty: loopback mode support
> 
> On Mon, Jun 27, 2022 at 12:23:53PM +0900, Chanho Park wrote:
> > Internal loopback mode can be supported by setting
> > S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since
> > s3c2410 and later SoCs. We can test it by linux-serial-test program[1]
> > with -k option. It will set TIOCM_LOOP mode during test.
> >
> > -k, --loopback     Use internal hardware loop back
> 
> I do not understand this line in the changelog text, what is it for?

Ah. I'd like to show the option of linux-serial-test tool how we can test
it.

> 
> >
> > [1]:
> > https://protect2.fireeye.com/v1/url?k=a0d4e46c-c15ff149-a0d56f23-74fe4
> > 85cbff6-c2c20e5c0d4f5c92&q=1&e=7c91006c-af87-4dad-8069-360450d31b36&u=
> > https%3A%2F%2Fgithub.com%2Fcbrake%2Flinux-serial-test
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> 
> Our tools require a blank line before the signed-off-by line.  Did you not
> get a warning when checking this patch before sending it out?

I'm able to see below warning. I'll fix it. Thanks.

WARNING: Do not use whitespace before Signed-off-by:

#15:

> 
> Also, no need for a footnote in a changelog that is so short, just put the
> link in the body of the text itself.

Okay. I'll put the link in the body instead of the footnote.

Best Regards,
Chanho Park

RE: [PATCH] tty: serial: samsung_tty: loopback mode support

[Chanho Park]

> Subject: Re: [PATCH] tty: serial: samsung_tty: loopback mode support
> 
> On Mon, Jun 27, 2022 at 12:23:53PM +0900, Chanho Park wrote:
> > Internal loopback mode can be supported by setting
> > S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since
> > s3c2410 and later SoCs. We can test it by linux-serial-test program[1]
> > with -k option. It will set TIOCM_LOOP mode during test.
> >
> > -k, --loopback     Use internal hardware loop back
> 
> I do not understand this line in the changelog text, what is it for?

Ah. I'd like to show the option of linux-serial-test tool how we can test
it.

> 
> >
> > [1]:
> > https://protect2.fireeye.com/v1/url?k=a0d4e46c-c15ff149-a0d56f23-74fe4
> > 85cbff6-c2c20e5c0d4f5c92&q=1&e=7c91006c-af87-4dad-8069-360450d31b36&u=
> > https%3A%2F%2Fgithub.com%2Fcbrake%2Flinux-serial-test
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> 
> Our tools require a blank line before the signed-off-by line.  Did you not
> get a warning when checking this patch before sending it out?

I'm able to see below warning. I'll fix it. Thanks.

WARNING: Do not use whitespace before Signed-off-by:

#15:

> 
> Also, no need for a footnote in a changelog that is so short, just put the
> link in the body of the text itself.

Okay. I'll put the link in the body instead of the footnote.

Best Regards,
Chanho Park


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] tty: serial: samsung_tty: loopback mode support

[Chanho Park]

> Subject: Re: [PATCH] tty: serial: samsung_tty: loopback mode support
> 
> On Mon, 27 Jun 2022, Chanho Park wrote:
> 
> > Internal loopback mode can be supported by setting
> > S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since
> > s3c2410 and later SoCs.
> 
> In that case, why is the  LOOPBACK define named 2443 and not 2410???

It might be copied from mach-s3c24xx.

> 
> The change looks fine otherwise.
> 
> I note though that many of the current drivers won't return TOICM_LOOP
> from ->get_mctrl() but I don't think it's exactly wrong to return it
> either. Perhaps lack of returning it is due to Documentation/driver-
> api/serial/driver.rst not including TOICM_LOOP in get_mctrl's list of
> information but only in set_mctrl's one.

According to the description, TIOCM_LOOP bit might be used only for setting.
I'll drop the bit manipulation from get_mctrl callback.
Thanks for the heads up.

Best Regards,
Chanho Park

RE: [PATCH] tty: serial: samsung_tty: loopback mode support

[Chanho Park]

> Subject: Re: [PATCH] tty: serial: samsung_tty: loopback mode support
> 
> On Mon, 27 Jun 2022, Chanho Park wrote:
> 
> > Internal loopback mode can be supported by setting
> > S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since
> > s3c2410 and later SoCs.
> 
> In that case, why is the  LOOPBACK define named 2443 and not 2410???

It might be copied from mach-s3c24xx.

> 
> The change looks fine otherwise.
> 
> I note though that many of the current drivers won't return TOICM_LOOP
> from ->get_mctrl() but I don't think it's exactly wrong to return it
> either. Perhaps lack of returning it is due to Documentation/driver-
> api/serial/driver.rst not including TOICM_LOOP in get_mctrl's list of
> information but only in set_mctrl's one.

According to the description, TIOCM_LOOP bit might be used only for setting.
I'll drop the bit manipulation from get_mctrl callback.
Thanks for the heads up.

Best Regards,
Chanho Park


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel