Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 29 May 2020 20:10:32 +0300
From:   Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
To:     Andreas =?iso-8859-1?Q?F=E4rber?= <afaerber@suse.de>
Cc:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>, linux-serial@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-actions@lists.infradead.org
Subject: Re: [PATCH 1/1] tty: serial: owl: Add support for kernel debugger
Message-ID: <20200529171032.GA26724@BV030612LT>
References: <6ee88060c129715980592a1ae33c93923916a14b.1590766726.git.cristian.ciocaltea@gmail.com>
 <16ff435f-9172-e01d-dfe6-7aa8575c4bd6@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <16ff435f-9172-e01d-dfe6-7aa8575c4bd6@suse.de>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, May 29, 2020 at 05:56:47PM +0200, Andreas F?rber wrote:
> Hi,
> 
> Am 29.05.20 um 17:50 schrieb Cristian Ciocaltea:
> > Implement poll_put_char and poll_get_char callbacks in struct uart_ops
> > that enables OWL UART to be used for KGDB debugging over serial line.
> > 
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > ---
> >   drivers/tty/serial/owl-uart.c | 45 ++++++++++++++++++++++++++++++-----
> >   1 file changed, 39 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/owl-uart.c b/drivers/tty/serial/owl-uart.c
> > index c2fa2f15d50a..26dcc374dec5 100644
> > --- a/drivers/tty/serial/owl-uart.c
> > +++ b/drivers/tty/serial/owl-uart.c
> > @@ -12,6 +12,7 @@
> >   #include <linux/console.h>
> >   #include <linux/delay.h>
> >   #include <linux/io.h>
> > +#include <linux/iopoll.h>
> >   #include <linux/module.h>
> >   #include <linux/of.h>
> >   #include <linux/platform_device.h>
> > @@ -20,13 +21,13 @@
> >   #include <linux/tty.h>
> >   #include <linux/tty_flip.h>
> > -#define OWL_UART_PORT_NUM 7
> > -#define OWL_UART_DEV_NAME "ttyOWL"
> > +#define OWL_UART_PORT_NUM		7
> > +#define OWL_UART_DEV_NAME		"ttyOWL"
> > -#define OWL_UART_CTL	0x000
> > -#define OWL_UART_RXDAT	0x004
> > -#define OWL_UART_TXDAT	0x008
> > -#define OWL_UART_STAT	0x00c
> > +#define OWL_UART_CTL			0x000
> > +#define OWL_UART_RXDAT			0x004
> > +#define OWL_UART_TXDAT			0x008
> > +#define OWL_UART_STAT			0x00c
> 
> Please do not unnecessarily re-indent kernel code. You can do so when you're
> actually adding something new.
> 

Hi Andreas,

Thank you for reviewing!

Sure, I will revert unnecessary changes.

>
> >   #define OWL_UART_CTL_DWLS_MASK		GENMASK(1, 0)
> >   #define OWL_UART_CTL_DWLS_5BITS		(0x0 << 0)
> > @@ -461,6 +462,34 @@ static void owl_uart_config_port(struct uart_port *port, int flags)
> >   	}
> >   }
> > +#ifdef CONFIG_CONSOLE_POLL
> > +
> > +static int owl_uart_poll_get_char(struct uart_port *port)
> > +{
> > +	u32 c = NO_POLL_CHAR;
> > +
> > +	if (!(owl_uart_read(port, OWL_UART_STAT) & OWL_UART_STAT_RFEM))
> > +		c = owl_uart_read(port, OWL_UART_RXDAT);
> > +
> > +	return c;
> > +}
> > +
> > +static void owl_uart_poll_put_char(struct uart_port *port, unsigned char c)
> > +{
> > +	/* Wait while TX FIFO is full */
> > +	while (owl_uart_read(port, OWL_UART_STAT) & OWL_UART_STAT_TFFU)
> > +		cpu_relax();
> > +
> > +	/* Send the character out */
> > +	owl_uart_write(port, c, OWL_UART_TXDAT);
> > +
> > +	/* Wait for transmitter to become empty */
> > +	while (owl_uart_read(port, OWL_UART_STAT) & OWL_UART_STAT_TRFL_MASK)
> > +		cpu_relax();
> > +}
> 
> How is this different from earlycon? I dislike that this is being
> open-coded. Please try to reuse existing functions for this.
>

I actually tried initially to reuse the existing code, but I found
a few (possible) issues:

- owl_uart_port_write() does more things than I think it's really
needed here, i.e. I'm not sure if the locking stuff and the IRQ
setup are required. From what I've noticed, most serial drivers provide
a very simple implementation (and lock free) for the callbacks, but
I couldn't figure out if locking could be required in some
circumstances.

- owl_console_putchar() could be a better alternative, but it depends
on CONFIG_SERIAL_OWL_CONSOLE which might not be enabled if the user
only chooses CONFIG_KGDB_SERIAL_CONSOLE, although this is probably
not a valid scenario.

Kind regards,
Cristi

> 
> Regards,
> Andreas
> 
> > +
> > +#endif /* CONFIG_CONSOLE_POLL */
> > +
> >   static const struct uart_ops owl_uart_ops = {
> >   	.set_mctrl = owl_uart_set_mctrl,
> >   	.get_mctrl = owl_uart_get_mctrl,
> > @@ -476,6 +505,10 @@ static const struct uart_ops owl_uart_ops = {
> >   	.request_port = owl_uart_request_port,
> >   	.release_port = owl_uart_release_port,
> >   	.verify_port = owl_uart_verify_port,
> > +#ifdef CONFIG_CONSOLE_POLL
> > +	.poll_get_char	= owl_uart_poll_get_char,
> > +	.poll_put_char	= owl_uart_poll_put_char,
> > +#endif
> >   };
> >   #ifdef CONFIG_SERIAL_OWL_CONSOLE
> > 
> 
> 
> -- 
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 N?rnberg, Germany
> GF: Felix Imend?rffer
> HRB 36809 (AG N?rnberg)