Date: Tue, 22 Aug 2017 22:00:44 +0300
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Aleksandar Markovic <aleksandar.markovic@rt-rk.com>
Cc: linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
        jslaby@suse.com, alan@linux.intel.com, jinqian@android.com,
        aleksandar.markovic@imgtec.com, miodrag.dinic@imgtec.com,
        petar.jovanovic@imgtec.com, raghu.gandham@imgtec.com,
        james.hogan@imgtec.com, ralf@linux-mips.org
Subject: Re: [PATCH 1/2] tty: goldfish: Use streaming DMA for r/w operations
 on Ranchu platforms
Message-ID: <20170822185938.6stpf75qko6mt6el@mwanda>
References: <20170822155637.53204-1-aleksandar.markovic@rt-rk.com>
 <20170822155637.53204-2-aleksandar.markovic@rt-rk.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170822155637.53204-2-aleksandar.markovic@rt-rk.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6911
Lines: 206

On Tue, Aug 22, 2017 at 05:56:36PM +0200, Aleksandar Markovic wrote:
>  drivers/tty/goldfish.c | 120 ++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 108 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/tty/goldfish.c b/drivers/tty/goldfish.c
> index 996bd47..ac98d5a 100644
> --- a/drivers/tty/goldfish.c
> +++ b/drivers/tty/goldfish.c
> @@ -22,6 +22,8 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/goldfish.h>
> +#include <linux/mm.h>
> +#include <linux/dma-mapping.h>
>  
>  enum {
>  	GOLDFISH_TTY_PUT_CHAR       = 0x00,
> @@ -32,6 +34,8 @@ enum {
>  	GOLDFISH_TTY_DATA_LEN       = 0x14,
>  	GOLDFISH_TTY_DATA_PTR_HIGH  = 0x18,
>  
> +	GOLDFISH_TTY_VERSION		= 0x20,

This is an offset, right?  It's not version 32?  The name is misleading.
Maybe call it GOLDFISH_VERSION_REG.

> +
>  	GOLDFISH_TTY_CMD_INT_DISABLE    = 0,
>  	GOLDFISH_TTY_CMD_INT_ENABLE     = 1,
>  	GOLDFISH_TTY_CMD_WRITE_BUFFER   = 2,
> @@ -45,6 +49,8 @@ struct goldfish_tty {
>  	u32 irq;
>  	int opencount;
>  	struct console console;
> +	u32 version;
> +	struct device *dev;
>  };
>  
>  static DEFINE_MUTEX(goldfish_tty_lock);
> @@ -53,24 +59,93 @@ static u32 goldfish_tty_line_count = 8;
>  static u32 goldfish_tty_current_line_count;
>  static struct goldfish_tty *goldfish_ttys;
>  
> -static void goldfish_tty_do_write(int line, const char *buf, unsigned count)
> +static inline void do_rw_io(struct goldfish_tty *qtty,
          ^^^^^^
Don't make functions inline.  Leave that for the compiler to decide.
It's smarter than we are and it ignores our input anyway.

> +			    unsigned long address,
> +			    unsigned int count,
> +			    int is_write)
>  {
>  	unsigned long irq_flags;
> -	struct goldfish_tty *qtty = &goldfish_ttys[line];
>  	void __iomem *base = qtty->base;
> +
>  	spin_lock_irqsave(&qtty->lock, irq_flags);
> -	gf_write_ptr(buf, base + GOLDFISH_TTY_DATA_PTR,
> -				base + GOLDFISH_TTY_DATA_PTR_HIGH);
> +	gf_write_ptr((void *)address, base + GOLDFISH_TTY_DATA_PTR,
> +		     base + GOLDFISH_TTY_DATA_PTR_HIGH);
>  	writel(count, base + GOLDFISH_TTY_DATA_LEN);
> -	writel(GOLDFISH_TTY_CMD_WRITE_BUFFER, base + GOLDFISH_TTY_CMD);
> +
> +	if (is_write)
> +		writel(GOLDFISH_TTY_CMD_WRITE_BUFFER, base + GOLDFISH_TTY_CMD);
> +	else
> +		writel(GOLDFISH_TTY_CMD_READ_BUFFER, base + GOLDFISH_TTY_CMD);
> +
>  	spin_unlock_irqrestore(&qtty->lock, irq_flags);
>  }
>  
> +static inline void goldfish_tty_rw(struct goldfish_tty *qtty,
> +				   unsigned long addr,
> +				   unsigned int count,
> +				   int is_write)
> +{
> +	dma_addr_t dma_handle;
> +	enum dma_data_direction dma_dir;
> +
> +	dma_dir = (is_write ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
> +	if (qtty->version) {

It's cleaner to write be "if (qtty->version > 0)" because the version
numbers are numbers and not booleans.

> +		/*
> +		 * Goldfish TTY for Ranchu platform uses
> +		 * physical addresses and DMA for read/write operations
> +		 */
> +		unsigned long addr_end = addr + count;
> +
> +		while (addr < addr_end) {
> +			unsigned long pg_end = (addr & PAGE_MASK) + PAGE_SIZE;
> +			unsigned long next =
> +					pg_end < addr_end ? pg_end : addr_end;
> +			unsigned long avail = next - addr;
> +
> +			/*
> +			 * Map the buffer's virtual address to the DMA address
> +			 * so the buffer can be accessed by the device.
> +			 */
> +			dma_handle = dma_map_single(qtty->dev, (void *)addr,
> +						    avail, dma_dir);
> +
> +			if (dma_mapping_error(qtty->dev, dma_handle)) {
> +				dev_err(qtty->dev, "tty: DMA mapping error.\n");
> +				return;
> +			}
> +			do_rw_io(qtty, dma_handle, avail, is_write);
> +
> +			/*
> +			 * Unmap the previously mapped region after
> +			 * the completion of the read/write operation.
> +			 */
> +			dma_unmap_single(qtty->dev, dma_handle, avail, dma_dir);
> +
> +			addr += avail;
> +		}
> +	} else {
> +		/*
> +		 * Old style Goldfish TTY used on the Goldfish platform
> +		 * uses virtual addresses.
> +		 */
> +		do_rw_io(qtty, addr, count, is_write);
> +	}
> +}
> +
> +static void goldfish_tty_do_write(int line, const char *buf,
> +				  unsigned int count)
> +{
> +	struct goldfish_tty *qtty = &goldfish_ttys[line];
> +	unsigned long address = (unsigned long)(void *)buf;
> +
> +	goldfish_tty_rw(qtty, address, count, 1);
> +}
> +
>  static irqreturn_t goldfish_tty_interrupt(int irq, void *dev_id)
>  {
>  	struct goldfish_tty *qtty = dev_id;
>  	void __iomem *base = qtty->base;
> -	unsigned long irq_flags;
> +	unsigned long address;
>  	unsigned char *buf;
>  	u32 count;
>  
> @@ -79,12 +154,10 @@ static irqreturn_t goldfish_tty_interrupt(int irq, void *dev_id)
>  		return IRQ_NONE;
>  
>  	count = tty_prepare_flip_string(&qtty->port, &buf, count);
> -	spin_lock_irqsave(&qtty->lock, irq_flags);
> -	gf_write_ptr(buf, base + GOLDFISH_TTY_DATA_PTR,
> -				base + GOLDFISH_TTY_DATA_PTR_HIGH);
> -	writel(count, base + GOLDFISH_TTY_DATA_LEN);
> -	writel(GOLDFISH_TTY_CMD_READ_BUFFER, base + GOLDFISH_TTY_CMD);
> -	spin_unlock_irqrestore(&qtty->lock, irq_flags);
> +
> +	address = (unsigned long)(void *)buf;
> +	goldfish_tty_rw(qtty, address, count, 0);
> +
>  	tty_schedule_flip(&qtty->port);
>  	return IRQ_HANDLED;
>  }
> @@ -271,6 +344,29 @@ static int goldfish_tty_probe(struct platform_device *pdev)
>  	qtty->port.ops = &goldfish_port_ops;
>  	qtty->base = base;
>  	qtty->irq = irq;
> +	qtty->dev = &pdev->dev;
> +
> +	/* Goldfish TTY device used by the Goldfish emulator
> +	 * should identify itself with 0, forcing the driver
> +	 * to use virtual addresses. Goldfish TTY device
> +	 * on Ranchu emulator (qemu2) returns 1 here and
> +	 * driver will use physical addresses.
> +	 */
> +	qtty->version = readl(base + GOLDFISH_TTY_VERSION);
> +
> +	/* Goldfish TTY device on Ranchu emulator (qemu2)
> +	 * will use DMA for read/write IO operations.
> +	 */
> +	if (qtty->version > 0) {
> +		/* Initialize dma_mask to 32-bits.
> +		 */
> +		if (!pdev->dev.dma_mask)
> +			pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> +		if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(32))) {
> +			dev_err(&pdev->dev, "No suitable DMA available.\n");
> +			goto err_create_driver_failed;

This goto is wrong and also "ret" isn't necessarily set correctly.
It's better to preserve the error code from dma_set_mask().

"Goto err_create_driver_failed" basically says that we just called
create_driver().  It's a come-from label name which is rubbish because
I can see just from looking at the patch, without any other context,
that actually dma_set_mask() failed and not create_driver()... Label
names should say what the goto does.  In this case what we want to do is
decrement goldfish_tty_current_line_count and call
goldfish_tty_delete_driver() if we're the last user.  A good label name
for this would be "goto dec_line_count;".  But actually the label which
does this is called "goto err_request_irq_failed;"...


regards,
dan carpenter