Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1717533 > unrolled thread
| Started by | Aleksandar Markovic <aleksandar.markovic@rt-rk.com> |
|---|---|
| First post | 2017-08-22 18:00 +0200 |
| Last post | 2017-08-23 13:10 +0200 |
| Articles | 3 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 1/2] tty: goldfish: Use streaming DMA for r/w operations on Ranchu platforms Aleksandar Markovic <aleksandar.markovic@rt-rk.com> - 2017-08-22 18:00 +0200
Re: [PATCH 1/2] tty: goldfish: Use streaming DMA for r/w operations on Ranchu platforms Dan Carpenter <dan.carpenter@oracle.com> - 2017-08-22 21:10 +0200
RE: [PATCH 1/2] tty: goldfish: Use streaming DMA for r/w operations on Ranchu platforms Miodrag Dinic <Miodrag.Dinic@imgtec.com> - 2017-08-23 13:10 +0200
| From | Aleksandar Markovic <aleksandar.markovic@rt-rk.com> |
|---|---|
| Date | 2017-08-22 18:00 +0200 |
| Subject | [PATCH 1/2] tty: goldfish: Use streaming DMA for r/w operations on Ranchu platforms |
| Message-ID | <uhjKW-4r8-27@gated-at.bofh.it> |
From: Miodrag Dinic <miodrag.dinic@imgtec.com>
Implement tty r/w operations using streaming DMA.
Goldfish tty for Ranchu platforms has been modified to use
streaming DMA mappings for read/write operations. This change
eliminates the need for snooping through the TLB in QEMU using
cpu_get_phys_page_debug() which does not guarantee that it will
return the valid va -> pa mapping.
The streaming DMA mapping is implemented using dma_map_single() per
transfer, while dma_unmap_single() is used for unmapping right after
the DMA transfer.
Using DMA API is the proper way for handling r/w transfers and
makes this driver more portable, thus effectively eliminating
the need for virt_to_page() and page_to_phys() conversions.
This change does not affect the old style Goldfish tty behaviour
which is still used by the Goldfish emulator. Version register has
been added and probed to see which platform is running this driver.
Reading from the new GOLDFISH_TTY_VERSION register using the Goldfish
emulator will return 0 and driver will work with virtual addresses.
Whereas if run on Ranchu it returns 1, and thus DMA is used.
(Goldfish and Ranchu are code names for the first and the second
generation of virtual boards used by Android emulator.)
Signed-off-by: Miodrag Dinic <miodrag.dinic@imgtec.com>
Signed-off-by: Goran Ferenc <goran.ferenc@imgtec.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtech.com>
---
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,
+
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,
+ 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) {
+ /*
+ * 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;
+ }
+ }
writel(GOLDFISH_TTY_CMD_INT_DISABLE, base + GOLDFISH_TTY_CMD);
--
2.9.3
[toc] | [next] | [standalone]
| From | Dan Carpenter <dan.carpenter@oracle.com> |
|---|---|
| Date | 2017-08-22 21:10 +0200 |
| Subject | Re: [PATCH 1/2] tty: goldfish: Use streaming DMA for r/w operations on Ranchu platforms |
| Message-ID | <uhmIO-6Dr-27@gated-at.bofh.it> |
| In reply to | #1717533 |
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
[toc] | [prev] | [next] | [standalone]
| From | Miodrag Dinic <Miodrag.Dinic@imgtec.com> |
|---|---|
| Date | 2017-08-23 13:10 +0200 |
| Subject | RE: [PATCH 1/2] tty: goldfish: Use streaming DMA for r/w operations on Ranchu platforms |
| Message-ID | <uhBHQ-8bC-7@gated-at.bofh.it> |
| In reply to | #1717674 |
Hello Dan,
thank you for your comments. Please find the answers inline:
> >
> > + GOLDFISH_TTY_VERSION = 0x20,
>
> This is an offset, right? It's not version 32? The name is misleading.
> Maybe call it GOLDFISH_VERSION_REG.> -static void goldfish_tty_do_write(int line, const char *buf, unsigned count)
Yes I guess it is a little bit misleading.
We will refactor this according to your suggestion in the next version.
Thanks.
> > +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.
It will be fixed in next version.
> + if (qtty->version) {
>
> It's cleaner to write be "if (qtty->version > 0)" because the version
> numbers are numbers and not booleans.
Ditto.
> > + 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;"...
Thank you for the suggestions. We will make appropriate changes and submit
the next patch version soon.
Kind regards,
Miodrag
________________________________________
From: Dan Carpenter [dan.carpenter@oracle.com]
Sent: Tuesday, August 22, 2017 9:00 PM
To: Aleksandar Markovic
Cc: linux-kernel@vger.kernel.org; gregkh@linuxfoundation.org; jslaby@suse.com; alan@linux.intel.com; jinqian@android.com; Aleksandar Markovic; Miodrag Dinic; Petar Jovanovic; Raghu Gandham; James Hogan; ralf@linux-mips.org
Subject: Re: [PATCH 1/2] tty: goldfish: Use streaming DMA for r/w operations on Ranchu platforms
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web