* [PATCH] Revert "serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250"
From: Florian Fainelli @ 2018-12-21 4:27 UTC (permalink / raw)
To: linux-vger
Cc: linux-serial, mpe, arnd, gregkh, linux, benh, paulus,
linuxppc-dev, Florian Fainelli, Jiri Slaby, Nishanth Menon,
Tony Lindgren, Vignesh R, Lokesh Vutla, Michael Moese, open list
In-Reply-To: <20181220173844.GA5838@roeck-us.net>
This reverts commit 6d11023c345e369bcb9d5a68b271764e362c1f6e ("serial:
8250: Default SERIAL_OF_PLATFORM to SERIAL_8250") since that breaks at
least mpc8544ds (PowerPC) using arch/powerpc/kernel/legacy_serial.c.
See https://lkml.org/lkml/2018/12/5/1491 for discussion and analysis
Fixes: 6d11023c345e ("serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/tty/serial/8250/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index d7737dca0e48..15c2c5463835 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -484,7 +484,6 @@ config SERIAL_8250_PXA
config SERIAL_OF_PLATFORM
tristate "Devicetree based probing for 8250 ports"
depends on SERIAL_8250 && OF
- default SERIAL_8250
help
This option is used for all 8250 compatible serial ports that
are probed through devicetree, including Open Firmware based
--
2.19.1
^ permalink raw reply related
* Re: [PATCH 1/4] mfd: exynos-lpass: Enable UART module support
From: Lee Jones @ 2018-12-21 10:13 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-kernel, linux-serial, linux-samsung-soc,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Beomho Seo,
Seung-Woo Kim, Sylwester Nawrocki, Greg Kroah-Hartman
In-Reply-To: <20181214113410.22848-2-m.szyprowski@samsung.com>
On Fri, 14 Dec 2018, Marek Szyprowski wrote:
> From: Beomho Seo <beomho.seo@samsung.com>
>
> This patch enables support for UART module in Exynos Audio SubSystem.
> There are boards (for example TM2), which use it for communication with
> bluetooth chip.
Does it though? Or does it enable the interrupt and reset something?
These calls would probably benefit from some documentation by way of
comments.
> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
> [mszyprow: rephrased commit message, added UART reset]
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> drivers/mfd/exynos-lpass.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/exynos-lpass.c b/drivers/mfd/exynos-lpass.c
> index ca829f85672f..2713de989f05 100644
> --- a/drivers/mfd/exynos-lpass.c
> +++ b/drivers/mfd/exynos-lpass.c
> @@ -82,11 +82,13 @@ static void exynos_lpass_enable(struct exynos_lpass *lpass)
> LPASS_INTR_SFR | LPASS_INTR_DMA | LPASS_INTR_I2S);
>
> regmap_write(lpass->top, SFR_LPASS_INTR_CPU_MASK,
> - LPASS_INTR_SFR | LPASS_INTR_DMA | LPASS_INTR_I2S);
> + LPASS_INTR_SFR | LPASS_INTR_DMA | LPASS_INTR_I2S |
> + LPASS_INTR_UART);
>
> exynos_lpass_core_sw_reset(lpass, LPASS_I2S_SW_RESET);
> exynos_lpass_core_sw_reset(lpass, LPASS_DMA_SW_RESET);
> exynos_lpass_core_sw_reset(lpass, LPASS_MEM_SW_RESET);
> + exynos_lpass_core_sw_reset(lpass, LPASS_UART_SW_RESET);
> }
>
> static void exynos_lpass_disable(struct exynos_lpass *lpass)
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 3/4] tty: serial: samsung: Increase maximum baudrate
From: Lee Jones @ 2018-12-21 10:15 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-kernel, linux-serial, linux-samsung-soc,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Beomho Seo,
Seung-Woo Kim, Sylwester Nawrocki, Greg Kroah-Hartman
In-Reply-To: <20181214113410.22848-4-m.szyprowski@samsung.com>
On Fri, 14 Dec 2018, Marek Szyprowski wrote:
> From: Seung-Woo Kim <sw0312.kim@samsung.com>
>
> This driver can be used to communicate with Bluetooth chip in high-speed
> UART mode, so increase the maximum baudrate to 3Mbps.
>
> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> [mszyprow: rephrased commit message]
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/tty/serial/samsung.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> index 2a49b6d876b8..9fc3559f80d9 100644
> --- a/drivers/tty/serial/samsung.c
> +++ b/drivers/tty/serial/samsung.c
> @@ -1287,7 +1287,7 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
> * Ask the core to calculate the divisor for us.
> */
>
> - baud = uart_get_baud_rate(port, termios, old, 0, 115200*8);
> + baud = uart_get_baud_rate(port, termios, old, 0, 3000000);
Does that mean the low speed devices will stop working? It looks like
this should be dynamically configurable based on what device is
connected to it, no?
> quot = s3c24xx_serial_getclk(ourport, baud, &clk, &clk_sel);
> if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST)
> quot = port->custom_divisor;
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 3/4] tty: serial: samsung: Increase maximum baudrate
From: Marek Szyprowski @ 2018-12-21 10:23 UTC (permalink / raw)
To: Lee Jones
Cc: linux-kernel, linux-serial, linux-samsung-soc,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Beomho Seo,
Seung-Woo Kim, Sylwester Nawrocki, Greg Kroah-Hartman
In-Reply-To: <20181221101553.GI13248@dell>
Hi
On 2018-12-21 11:15, Lee Jones wrote:
> On Fri, 14 Dec 2018, Marek Szyprowski wrote:
>> From: Seung-Woo Kim <sw0312.kim@samsung.com>
>>
>> This driver can be used to communicate with Bluetooth chip in high-speed
>> UART mode, so increase the maximum baudrate to 3Mbps.
>>
>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
>> [mszyprow: rephrased commit message]
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>> drivers/tty/serial/samsung.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>> index 2a49b6d876b8..9fc3559f80d9 100644
>> --- a/drivers/tty/serial/samsung.c
>> +++ b/drivers/tty/serial/samsung.c
>> @@ -1287,7 +1287,7 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
>> * Ask the core to calculate the divisor for us.
>> */
>>
>> - baud = uart_get_baud_rate(port, termios, old, 0, 115200*8);
>> + baud = uart_get_baud_rate(port, termios, old, 0, 3000000);
> Does that mean the low speed devices will stop working? It looks like
> this should be dynamically configurable based on what device is
> connected to it, no?
The changed parameter is maximum supported rate, so this change doesn't
affect any low speed device.
>> quot = s3c24xx_serial_getclk(ourport, baud, &clk, &clk_sel);
>> if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST)
>> quot = port->custom_divisor;
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH 1/4] mfd: exynos-lpass: Enable UART module support
From: Marek Szyprowski @ 2018-12-21 10:27 UTC (permalink / raw)
To: Lee Jones
Cc: linux-kernel, linux-serial, linux-samsung-soc,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Beomho Seo,
Seung-Woo Kim, Sylwester Nawrocki, Greg Kroah-Hartman
In-Reply-To: <20181221101306.GH13248@dell>
Hi
On 2018-12-21 11:13, Lee Jones wrote:
> On Fri, 14 Dec 2018, Marek Szyprowski wrote:
>
>> From: Beomho Seo <beomho.seo@samsung.com>
>>
>> This patch enables support for UART module in Exynos Audio SubSystem.
>> There are boards (for example TM2), which use it for communication with
>> bluetooth chip.
> Does it though? Or does it enable the interrupt and reset something?
> These calls would probably benefit from some documentation by way of
> comments.
It only enables routing interrupts out of LPASS HW module. This is
completely transparent for the rest of the system (UART and CPU/GIC).
UART driver will get them via standard ARM/GIC interrupt controller and
UART driver will enable/mask/handle it by itself via standard methods.
>> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
>> [mszyprow: rephrased commit message, added UART reset]
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>> drivers/mfd/exynos-lpass.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/exynos-lpass.c b/drivers/mfd/exynos-lpass.c
>> index ca829f85672f..2713de989f05 100644
>> --- a/drivers/mfd/exynos-lpass.c
>> +++ b/drivers/mfd/exynos-lpass.c
>> @@ -82,11 +82,13 @@ static void exynos_lpass_enable(struct exynos_lpass *lpass)
>> LPASS_INTR_SFR | LPASS_INTR_DMA | LPASS_INTR_I2S);
>>
>> regmap_write(lpass->top, SFR_LPASS_INTR_CPU_MASK,
>> - LPASS_INTR_SFR | LPASS_INTR_DMA | LPASS_INTR_I2S);
>> + LPASS_INTR_SFR | LPASS_INTR_DMA | LPASS_INTR_I2S |
>> + LPASS_INTR_UART);
>>
>> exynos_lpass_core_sw_reset(lpass, LPASS_I2S_SW_RESET);
>> exynos_lpass_core_sw_reset(lpass, LPASS_DMA_SW_RESET);
>> exynos_lpass_core_sw_reset(lpass, LPASS_MEM_SW_RESET);
>> + exynos_lpass_core_sw_reset(lpass, LPASS_UART_SW_RESET);
>> }
>>
>> static void exynos_lpass_disable(struct exynos_lpass *lpass)
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH 3/4] tty: serial: samsung: Increase maximum baudrate
From: Lee Jones @ 2018-12-21 10:34 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-kernel, linux-serial, linux-samsung-soc,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Beomho Seo,
Seung-Woo Kim, Sylwester Nawrocki, Greg Kroah-Hartman
In-Reply-To: <9be9bcd6-5b8a-da40-b232-a650eaa88d38@samsung.com>
On Fri, 21 Dec 2018, Marek Szyprowski wrote:
> On 2018-12-21 11:15, Lee Jones wrote:
> > On Fri, 14 Dec 2018, Marek Szyprowski wrote:
> >> From: Seung-Woo Kim <sw0312.kim@samsung.com>
> >>
> >> This driver can be used to communicate with Bluetooth chip in high-speed
> >> UART mode, so increase the maximum baudrate to 3Mbps.
> >>
> >> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> >> [mszyprow: rephrased commit message]
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> ---
> >> drivers/tty/serial/samsung.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> >> index 2a49b6d876b8..9fc3559f80d9 100644
> >> --- a/drivers/tty/serial/samsung.c
> >> +++ b/drivers/tty/serial/samsung.c
> >> @@ -1287,7 +1287,7 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
> >> * Ask the core to calculate the divisor for us.
> >> */
> >>
> >> - baud = uart_get_baud_rate(port, termios, old, 0, 115200*8);
> >> + baud = uart_get_baud_rate(port, termios, old, 0, 3000000);
> > Does that mean the low speed devices will stop working? It looks like
> > this should be dynamically configurable based on what device is
> > connected to it, no?
>
> The changed parameter is maximum supported rate, so this change doesn't
> affect any low speed device.
Understood. Thanks for the explanation.
> >> quot = s3c24xx_serial_getclk(ourport, baud, &clk, &clk_sel);
> >> if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST)
> >> quot = port->custom_divisor;
>
> Best regards
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 1/4] mfd: exynos-lpass: Enable UART module support
From: Lee Jones @ 2018-12-21 10:39 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-kernel, linux-serial, linux-samsung-soc,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Beomho Seo,
Seung-Woo Kim, Sylwester Nawrocki, Greg Kroah-Hartman
In-Reply-To: <7a43948e-17b9-1fd6-6b11-400016a4a28c@samsung.com>
On Fri, 21 Dec 2018, Marek Szyprowski wrote:
> Hi
>
> On 2018-12-21 11:13, Lee Jones wrote:
> > On Fri, 14 Dec 2018, Marek Szyprowski wrote:
> >
> >> From: Beomho Seo <beomho.seo@samsung.com>
> >>
> >> This patch enables support for UART module in Exynos Audio SubSystem.
> >> There are boards (for example TM2), which use it for communication with
> >> bluetooth chip.
> > Does it though? Or does it enable the interrupt and reset something?
> > These calls would probably benefit from some documentation by way of
> > comments.
>
> It only enables routing interrupts out of LPASS HW module. This is
> completely transparent for the rest of the system (UART and CPU/GIC).
> UART driver will get them via standard ARM/GIC interrupt controller and
> UART driver will enable/mask/handle it by itself via standard methods.
Sounds fine. But that is not what the commit message says.
> >> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
> >> [mszyprow: rephrased commit message, added UART reset]
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >> ---
> >> drivers/mfd/exynos-lpass.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mfd/exynos-lpass.c b/drivers/mfd/exynos-lpass.c
> >> index ca829f85672f..2713de989f05 100644
> >> --- a/drivers/mfd/exynos-lpass.c
> >> +++ b/drivers/mfd/exynos-lpass.c
> >> @@ -82,11 +82,13 @@ static void exynos_lpass_enable(struct exynos_lpass *lpass)
> >> LPASS_INTR_SFR | LPASS_INTR_DMA | LPASS_INTR_I2S);
> >>
> >> regmap_write(lpass->top, SFR_LPASS_INTR_CPU_MASK,
> >> - LPASS_INTR_SFR | LPASS_INTR_DMA | LPASS_INTR_I2S);
> >> + LPASS_INTR_SFR | LPASS_INTR_DMA | LPASS_INTR_I2S |
> >> + LPASS_INTR_UART);
> >>
> >> exynos_lpass_core_sw_reset(lpass, LPASS_I2S_SW_RESET);
> >> exynos_lpass_core_sw_reset(lpass, LPASS_DMA_SW_RESET);
> >> exynos_lpass_core_sw_reset(lpass, LPASS_MEM_SW_RESET);
> >> + exynos_lpass_core_sw_reset(lpass, LPASS_UART_SW_RESET);
> >> }
> >>
> >> static void exynos_lpass_disable(struct exynos_lpass *lpass)
>
> Best regards
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v2 1/4] mfd: exynos-lpass: Enable UART module support
From: Marek Szyprowski @ 2018-12-21 14:32 UTC (permalink / raw)
To: linux-kernel, linux-serial, linux-samsung-soc
Cc: Marek Szyprowski, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
Beomho Seo, Seung-Woo Kim, Sylwester Nawrocki, Lee Jones,
Greg Kroah-Hartman
In-Reply-To: <20181221103907.GK13248@dell>
From: Beomho Seo <beomho.seo@samsung.com>
This patch enables proper interrupts routing between UART module
in Exynos Audio SubSystem and the rest of the SoC. This routing is
completely transparent for UART device and CPU/GIC. UART driver requests
interrupts from the respective controller and enables/masks/handles it
by itself via standard methods.
There are boards (for example TM2), which use UART module in Exynos Audio
SubStem for communication with BlueTooth chip.
Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
[mszyprow: rephrased commit message, added UART reset]
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Changelog
v2:
- rephrased and extended commit message
---
drivers/mfd/exynos-lpass.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/exynos-lpass.c b/drivers/mfd/exynos-lpass.c
index ca829f85672f..2713de989f05 100644
--- a/drivers/mfd/exynos-lpass.c
+++ b/drivers/mfd/exynos-lpass.c
@@ -82,11 +82,13 @@ static void exynos_lpass_enable(struct exynos_lpass *lpass)
LPASS_INTR_SFR | LPASS_INTR_DMA | LPASS_INTR_I2S);
regmap_write(lpass->top, SFR_LPASS_INTR_CPU_MASK,
- LPASS_INTR_SFR | LPASS_INTR_DMA | LPASS_INTR_I2S);
+ LPASS_INTR_SFR | LPASS_INTR_DMA | LPASS_INTR_I2S |
+ LPASS_INTR_UART);
exynos_lpass_core_sw_reset(lpass, LPASS_I2S_SW_RESET);
exynos_lpass_core_sw_reset(lpass, LPASS_DMA_SW_RESET);
exynos_lpass_core_sw_reset(lpass, LPASS_MEM_SW_RESET);
+ exynos_lpass_core_sw_reset(lpass, LPASS_UART_SW_RESET);
}
static void exynos_lpass_disable(struct exynos_lpass *lpass)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2 1/4] mfd: exynos-lpass: Enable UART module support
From: Lee Jones @ 2018-12-21 14:56 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-kernel, linux-serial, linux-samsung-soc,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Beomho Seo,
Seung-Woo Kim, Sylwester Nawrocki, Greg Kroah-Hartman
In-Reply-To: <20181221143228.934-1-m.szyprowski@samsung.com>
On Fri, 21 Dec 2018, Marek Szyprowski wrote:
> From: Beomho Seo <beomho.seo@samsung.com>
>
> This patch enables proper interrupts routing between UART module
> in Exynos Audio SubSystem and the rest of the SoC. This routing is
> completely transparent for UART device and CPU/GIC. UART driver requests
> interrupts from the respective controller and enables/masks/handles it
> by itself via standard methods.
>
> There are boards (for example TM2), which use UART module in Exynos Audio
> SubStem for communication with BlueTooth chip.
>
> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
> [mszyprow: rephrased commit message, added UART reset]
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changelog
> v2:
> - rephrased and extended commit message
> ---
> drivers/mfd/exynos-lpass.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Applied, thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] serial/sunsu: fix refcount leak
From: Frank Lee @ 2018-12-21 16:41 UTC (permalink / raw)
To: davem, Greg Kroah-Hartman, jslaby, sparclinux
Cc: Daniel Lezcano, linux-serial, Linux Kernel Mailing List
In-Reply-To: <CAEExFWuoJZfHeVemcLz9JhrQ_RBMzjsB3CopymBMWhEyWVR=rA@mail.gmail.com>
ping...
^ permalink raw reply
* Re: [PATCH] serial/sunsu: fix refcount leak
From: David Miller @ 2018-12-21 19:25 UTC (permalink / raw)
To: tiny.windzz
Cc: gregkh, jslaby, sparclinux, daniel.lezcano, linux-serial,
linux-kernel
In-Reply-To: <20181212160145.23117-1-tiny.windzz@gmail.com>
From: Yangtao Li <tiny.windzz@gmail.com>
Date: Wed, 12 Dec 2018 11:01:45 -0500
> The function of_find_node_by_path() acquires a reference to the node
> returned by it and that reference needs to be dropped by its caller.
>
> su_get_type() doesn't do that. The match node are used as an identifier
> to compare against the current node, so we can directly drop the refcount
> after getting the node from the path as it is not used as pointer.
>
> Fix this by use a single variable and drop the refcount right after
> of_find_node_by_path().
>
> Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Marek Vasut @ 2018-12-21 20:08 UTC (permalink / raw)
To: Paul Burton
Cc: linux-serial@vger.kernel.org, Greg Kroah-Hartman,
linux-kernel@vger.kernel.org, Paul Burton, Daniel Jedrychowski,
linux-mips@vger.kernel.org, stable, Ezequiel Garcia
In-Reply-To: <20181216222815.4t4xhaiy6rvfaogq@pburton-laptop>
On 12/16/2018 11:28 PM, Paul Burton wrote:
> Hi Marek,
>
> On Sun, Dec 16, 2018 at 11:01:18PM +0100, Marek Vasut wrote:
>>>>> I did suggest an alternative approach which would rename
>>>>> serial8250_clear_fifos() and split it into 2 variants - one that
>>>>> disables FIFOs & one that does not, then use the latter in
>>>>> __do_stop_tx_rs485():
>>>>>
>>>>> https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
>>>>>
>>>>> However I have no access to the OMAP3 hardware that Marek's patch was
>>>>> attempting to fix & have heard nothing back with regards to him testing
>>>>> that approach, so here's a simple revert that fixes the Ingenic JZ4780.
>>>>>
>>>>> I've marked for stable back to v4.10 presuming that this is how far the
>>>>> broken patch may be backported, given that this is where commit
>>>>> 2bed8a8e7072 ("Clearing FIFOs in RS485 emulation mode causes subsequent
>>>>> transmits to break") that it tried to fix was introduced.
>>>>
>>>> OK, I tested this on AM335x / OMAP3 and the system is again broken, so
>>>> that's a NAK.
>>>
>>> To be clear - what did you test? This revert or the patch linked to
>>> above?
>>>
>>> This revert would of course reintroduce your RS485 issue because it just
>>> undoes your change.
>>
>> The revert. Which of the two patches do you need me to test.
>
> The one in the email I sent on Thursday 13th at 01:48:06 UTC, linked to
> at lore.kernel.org in the quote right above:
>
> https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
>
> You replied with comments on the patch, you just never tested it or
> never told me if you did. The lack of response means I don't know
> whether that potential patch even still works for your system, hence the
> revert.
I shared the entire testcase, which now fails on AM335x due to this
revert. Is there any progress on a proper fix from your side which does
not break the AM335x ?
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Paul Burton @ 2018-12-21 21:08 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-serial@vger.kernel.org, Greg Kroah-Hartman,
linux-kernel@vger.kernel.org, Paul Burton, Daniel Jedrychowski,
linux-mips@vger.kernel.org, stable, Ezequiel Garcia
In-Reply-To: <78839e8c-0278-6060-0dd2-a84a19258a8a@denx.de>
Hi Marek,
On Fri, Dec 21, 2018 at 09:08:28PM +0100, Marek Vasut wrote:
> On 12/16/2018 11:28 PM, Paul Burton wrote:
> > On Sun, Dec 16, 2018 at 11:01:18PM +0100, Marek Vasut wrote:
> >>>>> I did suggest an alternative approach which would rename
> >>>>> serial8250_clear_fifos() and split it into 2 variants - one that
> >>>>> disables FIFOs & one that does not, then use the latter in
> >>>>> __do_stop_tx_rs485():
> >>>>>
> >>>>> https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
> >>>>>
> >>>>> However I have no access to the OMAP3 hardware that Marek's patch was
> >>>>> attempting to fix & have heard nothing back with regards to him testing
> >>>>> that approach, so here's a simple revert that fixes the Ingenic JZ4780.
> >>>>>
> >>>>> I've marked for stable back to v4.10 presuming that this is how far the
> >>>>> broken patch may be backported, given that this is where commit
> >>>>> 2bed8a8e7072 ("Clearing FIFOs in RS485 emulation mode causes subsequent
> >>>>> transmits to break") that it tried to fix was introduced.
> >>>>
> >>>> OK, I tested this on AM335x / OMAP3 and the system is again broken, so
> >>>> that's a NAK.
> >>>
> >>> To be clear - what did you test? This revert or the patch linked to
> >>> above?
> >>>
> >>> This revert would of course reintroduce your RS485 issue because it just
> >>> undoes your change.
> >>
> >> The revert. Which of the two patches do you need me to test.
> >
> > The one in the email I sent on Thursday 13th at 01:48:06 UTC, linked to
> > at lore.kernel.org in the quote right above:
> >
> > https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
> >
> > You replied with comments on the patch, you just never tested it or
> > never told me if you did. The lack of response means I don't know
> > whether that potential patch even still works for your system, hence the
> > revert.
>
> I shared the entire testcase, which now fails on AM335x due to this
> revert. Is there any progress on a proper fix from your side which does
> not break the AM335x ?
No.
Let's be clear - I didn't break your AM335x system, your broken patch
says that Daniel did with a commit applied back in v4.10. As such I
don't consider it my responsibility to fix your problem at all, I don't
have any access to the hardware anyway & I won't be buying hardware to
fix a bug for you.
Despite all that I did write a patch which I expect would have solved
the problem for both of us, which is linked *twice* in the quoted emails
above & which as far as I can tell you *still* have not tested. I can
only surmise that you're trying deliberately to be annoying at this
point.
If you want to try the patch I already wrote, and confirm whether it
actually works for you, then let's talk. Otherwise we're done here.
Thanks,
Paul
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Marek Vasut @ 2018-12-21 22:02 UTC (permalink / raw)
To: Paul Burton
Cc: linux-serial@vger.kernel.org, Greg Kroah-Hartman,
linux-kernel@vger.kernel.org, Paul Burton, Daniel Jedrychowski,
linux-mips@vger.kernel.org, stable, Ezequiel Garcia
In-Reply-To: <20181221210818.g3kbv7bnr577dane@pburton-laptop>
On 12/21/2018 10:08 PM, Paul Burton wrote:
> Hi Marek,
>
> On Fri, Dec 21, 2018 at 09:08:28PM +0100, Marek Vasut wrote:
>> On 12/16/2018 11:28 PM, Paul Burton wrote:
>>> On Sun, Dec 16, 2018 at 11:01:18PM +0100, Marek Vasut wrote:
>>>>>>> I did suggest an alternative approach which would rename
>>>>>>> serial8250_clear_fifos() and split it into 2 variants - one that
>>>>>>> disables FIFOs & one that does not, then use the latter in
>>>>>>> __do_stop_tx_rs485():
>>>>>>>
>>>>>>> https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
>>>>>>>
>>>>>>> However I have no access to the OMAP3 hardware that Marek's patch was
>>>>>>> attempting to fix & have heard nothing back with regards to him testing
>>>>>>> that approach, so here's a simple revert that fixes the Ingenic JZ4780.
>>>>>>>
>>>>>>> I've marked for stable back to v4.10 presuming that this is how far the
>>>>>>> broken patch may be backported, given that this is where commit
>>>>>>> 2bed8a8e7072 ("Clearing FIFOs in RS485 emulation mode causes subsequent
>>>>>>> transmits to break") that it tried to fix was introduced.
>>>>>>
>>>>>> OK, I tested this on AM335x / OMAP3 and the system is again broken, so
>>>>>> that's a NAK.
>>>>>
>>>>> To be clear - what did you test? This revert or the patch linked to
>>>>> above?
>>>>>
>>>>> This revert would of course reintroduce your RS485 issue because it just
>>>>> undoes your change.
>>>>
>>>> The revert. Which of the two patches do you need me to test.
>>>
>>> The one in the email I sent on Thursday 13th at 01:48:06 UTC, linked to
>>> at lore.kernel.org in the quote right above:
>>>
>>> https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
>>>
>>> You replied with comments on the patch, you just never tested it or
>>> never told me if you did. The lack of response means I don't know
>>> whether that potential patch even still works for your system, hence the
>>> revert.
>>
>> I shared the entire testcase, which now fails on AM335x due to this
>> revert. Is there any progress on a proper fix from your side which does
>> not break the AM335x ?
>
> No.
>
> Let's be clear - I didn't break your AM335x system, your broken patch
> says that Daniel did with a commit applied back in v4.10. As such I
> don't consider it my responsibility to fix your problem at all, I don't
> have any access to the hardware anyway & I won't be buying hardware to
> fix a bug for you.
>
> Despite all that I did write a patch which I expect would have solved
> the problem for both of us, which is linked *twice* in the quoted emails
> above & which as far as I can tell you *still* have not tested. I can
> only surmise that you're trying deliberately to be annoying at this
> point.
>
> If you want to try the patch I already wrote, and confirm whether it
> actually works for you, then let's talk. Otherwise we're done here.
Understood. However I did test your patch, but it was generating
spurious IRQs and overruns (ttyS ttyS2: 91 input overrun(s)) on the
AM335x, so that's not a proper solution.
I even brought the CI20 V2 I have back to life (yes, I bought one). The
patch Ezequiel posted did fix the problem on the CI20, and did not break
the AM335x, so I'd prefer if it was revisited instead of a heavy-handed
revert.
And I'd prefer to keep this thread alive, since I am still convinced
that the FIFO handling code is wrong. Moreover, considering the UME bit
on JZ4780 in FCR, the original code should confuse the UART on the
JZ4780 too, although this might be hidden by some other surrounding code
specific to the 8250 core on the JZ4780.
I am also on mipslinux IRC channel, in case you want to discuss this.
--
Best regards,
Marek Vasut
^ permalink raw reply
* bug report: serial: 8250: Rate limit serial port rx interrupts during input overruns
From: Colin Ian King @ 2018-12-22 16:43 UTC (permalink / raw)
To: Darwin Dingel
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial,
linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
Hi,
Static analysis with CoverityScan found a potential null pointer
deference issue with a null uart with recent commit
6d7f677a2afa1c82d7fc7af7f9159cbffd5dc010 ("serial: 8250: Rate limit
serial port rx interrupts during input overruns").
Coverity Analysis is below:
983 uart = serial8250_find_match_or_unused(&up->port);
2. Condition uart, taking false branch.
3. var_compare_op: Comparing uart to null implies that uart might be
null.
984 if (uart && uart->port.type != PORT_8250_CIR) {
985 if (uart->port.dev)
...
...
1075 /* Initialise interrupt backoff work if required */
4. Condition up->overrun_backoff_time_ms > 0, taking false branch.
1076 if (up->overrun_backoff_time_ms > 0) {
CID 1475967 (#1 of 2): Dereference after null check (FORWARD_NULL)
1077 uart->overrun_backoff_time_ms =
up->overrun_backoff_time_ms;
1078 INIT_DELAYED_WORK(&uart->overrun_backoff,
1079 serial_8250_overrun_backoff_work);
1080 } else {
CID 1475967 (#2 of 2): Dereference after null check (FORWARD_NULL)5.
var_deref_op: Dereferencing null pointer uart.
1081 uart->overrun_backoff_time_ms = 0;
1082 }
The implication is that uart could be null, so the code block between
lines 1076 and 1082 should be guarded with a sanity null check on uart too.
Colin
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Paul Burton @ 2018-12-24 15:43 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-serial@vger.kernel.org, Greg Kroah-Hartman,
linux-kernel@vger.kernel.org, Paul Burton, Daniel Jedrychowski,
linux-mips@vger.kernel.org, stable, Ezequiel Garcia
In-Reply-To: <0266be47-75c8-6a1b-dfec-129c5b7bbf8f@denx.de>
Hi Marek,
On Fri, Dec 21, 2018 at 11:02:03PM +0100, Marek Vasut wrote:
> >> I shared the entire testcase, which now fails on AM335x due to this
> >> revert. Is there any progress on a proper fix from your side which does
> >> not break the AM335x ?
> >
> > No.
> >
> > Let's be clear - I didn't break your AM335x system, your broken patch
> > says that Daniel did with a commit applied back in v4.10. As such I
> > don't consider it my responsibility to fix your problem at all, I don't
> > have any access to the hardware anyway & I won't be buying hardware to
> > fix a bug for you.
> >
> > Despite all that I did write a patch which I expect would have solved
> > the problem for both of us, which is linked *twice* in the quoted emails
> > above & which as far as I can tell you *still* have not tested. I can
> > only surmise that you're trying deliberately to be annoying at this
> > point.
> >
> > If you want to try the patch I already wrote, and confirm whether it
> > actually works for you, then let's talk. Otherwise we're done here.
>
> Understood. However I did test your patch, but it was generating
> spurious IRQs and overruns (ttyS ttyS2: 91 input overrun(s)) on the
> AM335x, so that's not a proper solution.
OK, thanks for testing, and let's figure out what's going on. Since the
revert is in now I'd suggest starting from there - ie. approximately
from the code we've had since v4.10.
> I even brought the CI20 V2 I have back to life (yes, I bought one). The
> patch Ezequiel posted did fix the problem on the CI20, and did not break
> the AM335x, so I'd prefer if it was revisited instead of a heavy-handed
> revert.
As I described in an earlier email & on IRC, Ezequiel's one-liner does
not address all of the problems listed in my revert's commit message.
But again, since the revert is now in I suggest starting from there.
As neither a maintainer or significant contributor to
drivers/tty/serial, nor the author of the patch applied in v4.10 which
you say broke AM335x, nor someone with access to the affected hardware,
I'm probably not the best placed person to help you here - all I can do
is offer general debug suggestions. With that in mind, here are some
questions & ideas I have:
1) Your message for commit f6aa5beb45be ("serial: 8250: Fix clearing
FIFOs in RS485 mode again") suggests that the problem you're seeing
is specific to the __do_stop_tx_rs485() path. Does changing only
__do_stop_tx_rs485() to clear the FIFOs without disabling them,
without modifying other users of serial8250_clear_fifos(), fix your
problem? ie. does the following work?
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index f776b3eafb96..18d2a1a93f03 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1458,17 +1458,21 @@ static void serial8250_stop_rx(struct uart_port *port)
}
static void __do_stop_tx_rs485(struct uart_8250_port *p)
{
+ unsigned char fcr;
+
serial8250_em485_rts_after_send(p);
/*
* Empty the RX FIFO, we are not interested in anything
* received during the half-duplex transmission.
* Enable previously disabled RX interrupts.
*/
if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
- serial8250_clear_fifos(p);
+ fcr = serial_port_in(&p->port, UART_FCR);
+ serial_port_out(&p->port, UART_FCR,
+ fcr | UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
p->ier |= UART_IER_RLSI | UART_IER_RDI;
serial_port_out(&p->port, UART_IER, p->ier);
}
Based on the comment above maybe leaving UART_FCR_CLEAR_XMIT out of
that might make sense too..?
Having __do_stop_tx_rs485() not share the FIFO clearing code with
other callers may make sense given that so far as I can see
__do_stop_tx_rs485() runs whilst the port is in use & other callers
of serial8250_clear_fifos() do not.
2) In an earlier email you said "The problem the original patch fixed
was that too many bits were cleared in the FCR on OMAP3/AM335x".
Could you clarify which bits we're talking about? From the AM335x
reference manual I can only see the DMA_MODE bit & the
[RT]X_FIFO_TRIG fields. Do you know which are problematic & why? In
your commit message you mention the AM335x UART swallowing &
retransmitting bytes - which field changing causes that?
> And I'd prefer to keep this thread alive, since I am still convinced
> that the FIFO handling code is wrong. Moreover, considering the UME bit
> on JZ4780 in FCR, the original code should confuse the UART on the
> JZ4780 too, although this might be hidden by some other surrounding code
> specific to the 8250 core on the JZ4780.
For completeness, as I said in an earlier email in the thread and as
we've since discussed on IRC, this is incorrect because
ingenic_uart_serial_out() unconditionally sets the UME bit. As such the
UME bit is not a problem, and JZ4780 works fine both before your patch &
after the revert.
Thanks,
Paul
^ permalink raw reply related
* [PATCH v7 0/2] add uart DMA function
From: Long Cheng @ 2018-12-25 1:26 UTC (permalink / raw)
To: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang
Cc: Matthias Brugger, Dan Williams, Greg Kroah-Hartman, Jiri Slaby,
Sean Wang, dmaengine, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, linux-serial, srv_heupstream,
Yingjoe Chen, YT Shen, Long Cheng
In Mediatek SOCs, the uart can support DMA function.
Base on DMA engine formwork, we add the DMA code to support uart. And put the code under drivers/dma.
This series contains document bindings, Kconfig to control the function enable or not,
device tree including interrupt and dma device node, the code of UART DMA
Changes compared to v6:
-Correct spelling
Changes compared to v5:
-move 'requst irqs' to alloc channel
-remove tasklet.
Changes compared to v4:
-modify Kconfig depends on.
Changes compared to v3:
-fix CONFIG_PM, will cause build fail
Changes compared to v2:
-remove unimportant parameters
-instead of cookie, use APIs of virtual channel.
-use of_dma_xlate_by_chan_id.
Changes compared to v1:
-mian revised file, 8250_mtk_dma.c
--parameters renamed for standard
--remove atomic operation
Long Cheng (2):
dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
arm: dts: mt2712: add uart APDMA to device tree
arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 50 +++
drivers/dma/mediatek/8250_mtk_dma.c | 694 +++++++++++++++++++++++++++++
drivers/dma/mediatek/Kconfig | 11 +
drivers/dma/mediatek/Makefile | 1 +
4 files changed, 756 insertions(+)
create mode 100644 drivers/dma/mediatek/8250_mtk_dma.c
--
1.7.9.5
^ permalink raw reply
* [PATCH v7 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
From: Long Cheng @ 2018-12-25 1:26 UTC (permalink / raw)
To: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang
Cc: Matthias Brugger, Dan Williams, Greg Kroah-Hartman, Jiri Slaby,
Sean Wang, dmaengine, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, linux-serial, srv_heupstream,
Yingjoe Chen, YT Shen, Long Cheng
In-Reply-To: <1545701209-6884-1-git-send-email-long.cheng@mediatek.com>
In DMA engine framework, add 8250 uart dma to support MediaTek uart.
If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
the performance, can enable the function.
Signed-off-by: Long Cheng <long.cheng@mediatek.com>
---
drivers/dma/mediatek/8250_mtk_dma.c | 694 +++++++++++++++++++++++++++++++++++
drivers/dma/mediatek/Kconfig | 11 +
drivers/dma/mediatek/Makefile | 1 +
3 files changed, 706 insertions(+)
create mode 100644 drivers/dma/mediatek/8250_mtk_dma.c
diff --git a/drivers/dma/mediatek/8250_mtk_dma.c b/drivers/dma/mediatek/8250_mtk_dma.c
new file mode 100644
index 0000000..c4090f2
--- /dev/null
+++ b/drivers/dma/mediatek/8250_mtk_dma.c
@@ -0,0 +1,694 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MediaTek 8250 DMA driver.
+ *
+ * Copyright (c) 2018 MediaTek Inc.
+ * Author: Long Cheng <long.cheng@mediatek.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/list.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_dma.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/pm_runtime.h>
+#include <linux/iopoll.h>
+
+#include "../virt-dma.h"
+
+#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
+
+#define VFF_EN_B BIT(0)
+#define VFF_STOP_B BIT(0)
+#define VFF_FLUSH_B BIT(0)
+#define VFF_4G_SUPPORT_B BIT(0)
+#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
+#define VFF_RX_INT_EN1_B BIT(1)
+#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
+#define VFF_WARM_RST_B BIT(0)
+#define VFF_RX_INT_FLAG_CLR_B (BIT(0) | BIT(1))
+#define VFF_TX_INT_FLAG_CLR_B 0
+#define VFF_STOP_CLR_B 0
+#define VFF_FLUSH_CLR_B 0
+#define VFF_INT_EN_CLR_B 0
+#define VFF_4G_SUPPORT_CLR_B 0
+
+/* interrupt trigger level for tx */
+#define VFF_TX_THRE(n) ((n) * 7 / 8)
+/* interrupt trigger level for rx */
+#define VFF_RX_THRE(n) ((n) * 3 / 4)
+
+#define MTK_UART_APDMA_RING_SIZE 0xffffU
+/* invert this bit when wrap ring head again*/
+#define MTK_UART_APDMA_RING_WRAP 0x10000U
+
+#define VFF_INT_FLAG 0x00
+#define VFF_INT_EN 0x04
+#define VFF_EN 0x08
+#define VFF_RST 0x0c
+#define VFF_STOP 0x10
+#define VFF_FLUSH 0x14
+#define VFF_ADDR 0x1c
+#define VFF_LEN 0x24
+#define VFF_THRE 0x28
+#define VFF_WPT 0x2c
+#define VFF_RPT 0x30
+/*TX: the buffer size HW can read. RX: the buffer size SW can read.*/
+#define VFF_VALID_SIZE 0x3c
+/*TX: the buffer size SW can write. RX: the buffer size HW can write.*/
+#define VFF_LEFT_SIZE 0x40
+#define VFF_DEBUG_STATUS 0x50
+#define VFF_4G_SUPPORT 0x54
+
+struct mtk_uart_apdmadev {
+ struct dma_device ddev;
+ struct clk *clk;
+ bool support_33bits;
+ unsigned int dma_irq[MTK_UART_APDMA_CHANNELS];
+};
+
+struct mtk_uart_apdma_desc {
+ struct virt_dma_desc vd;
+
+ unsigned int avail_len;
+};
+
+struct mtk_chan {
+ struct virt_dma_chan vc;
+ struct dma_slave_config cfg;
+ void __iomem *base;
+ struct mtk_uart_apdma_desc *desc;
+
+ bool requested;
+
+ unsigned int rx_status;
+};
+
+static inline struct mtk_uart_apdmadev *
+to_mtk_uart_apdma_dev(struct dma_device *d)
+{
+ return container_of(d, struct mtk_uart_apdmadev, ddev);
+}
+
+static inline struct mtk_chan *to_mtk_uart_apdma_chan(struct dma_chan *c)
+{
+ return container_of(c, struct mtk_chan, vc.chan);
+}
+
+static inline struct mtk_uart_apdma_desc *to_mtk_uart_apdma_desc
+ (struct dma_async_tx_descriptor *t)
+{
+ return container_of(t, struct mtk_uart_apdma_desc, vd.tx);
+}
+
+static void mtk_uart_apdma_chan_write(struct mtk_chan *c,
+ unsigned int reg, unsigned int val)
+{
+ writel(val, c->base + reg);
+}
+
+static unsigned int
+mtk_uart_apdma_chan_read(struct mtk_chan *c, unsigned int reg)
+{
+ return readl(c->base + reg);
+}
+
+static void mtk_uart_apdma_desc_free(struct virt_dma_desc *vd)
+{
+ struct dma_chan *chan = vd->tx.chan;
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+
+ kfree(c->desc);
+ c->desc = NULL;
+}
+
+static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
+{
+ unsigned int txcount = c->desc->avail_len;
+ unsigned int len, send, left, wpt, wrap;
+
+ if (mtk_uart_apdma_chan_read(c, VFF_LEFT_SIZE) == 0U) {
+ mtk_uart_apdma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
+ } else {
+ len = mtk_uart_apdma_chan_read(c, VFF_LEN);
+
+ while (((left = mtk_uart_apdma_chan_read(c,
+ VFF_LEFT_SIZE)) > 0U)
+ && (c->desc->avail_len != 0U)) {
+ send = min_t(unsigned int, left, c->desc->avail_len);
+ wpt = mtk_uart_apdma_chan_read(c, VFF_WPT);
+ wrap = wpt & MTK_UART_APDMA_RING_WRAP ?
+ 0U : MTK_UART_APDMA_RING_WRAP;
+
+ if ((wpt & (len - 1U)) + send < len)
+ mtk_uart_apdma_chan_write(c,
+ VFF_WPT, wpt + send);
+ else
+ mtk_uart_apdma_chan_write(c, VFF_WPT,
+ ((wpt + send) & (len - 1U))
+ | wrap);
+
+ c->desc->avail_len -= send;
+ }
+
+ if (txcount != c->desc->avail_len) {
+ mtk_uart_apdma_chan_write(c,
+ VFF_INT_EN, VFF_TX_INT_EN_B);
+ if (mtk_uart_apdma_chan_read(c,
+ VFF_FLUSH) == 0U)
+ mtk_uart_apdma_chan_write(c,
+ VFF_FLUSH, VFF_FLUSH_B);
+ }
+ }
+}
+
+static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
+{
+ struct mtk_uart_apdma_desc *d = c->desc;
+ unsigned int rx_len, wg, rg, count;
+
+ if (mtk_uart_apdma_chan_read(c, VFF_VALID_SIZE) == 0U)
+ return;
+
+ if (d && vchan_next_desc(&c->vc)) {
+ rx_len = mtk_uart_apdma_chan_read(c, VFF_LEN);
+ rg = mtk_uart_apdma_chan_read(c, VFF_RPT);
+ wg = mtk_uart_apdma_chan_read(c, VFF_WPT);
+ count = ((rg ^ wg) & MTK_UART_APDMA_RING_WRAP) ?
+ ((wg & MTK_UART_APDMA_RING_SIZE) +
+ rx_len - (rg & MTK_UART_APDMA_RING_SIZE)) :
+ ((wg & MTK_UART_APDMA_RING_SIZE) -
+ (rg & MTK_UART_APDMA_RING_SIZE));
+
+ c->rx_status = count;
+ mtk_uart_apdma_chan_write(c, VFF_RPT, wg);
+
+ list_del(&d->vd.node);
+ vchan_cookie_complete(&d->vd);
+ }
+}
+
+static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
+{
+ struct dma_chan *chan = (struct dma_chan *)dev_id;
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ struct mtk_uart_apdma_desc *d;
+ unsigned long flags;
+
+ spin_lock_irqsave(&c->vc.lock, flags);
+ switch (c->cfg.direction) {
+ case DMA_DEV_TO_MEM:
+ mtk_uart_apdma_chan_write(c,
+ VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
+ mtk_uart_apdma_start_rx(c);
+ break;
+ case DMA_MEM_TO_DEV:
+ d = c->desc;
+
+ mtk_uart_apdma_chan_write(c,
+ VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
+
+ if (d->avail_len != 0U) {
+ mtk_uart_apdma_start_tx(c);
+ } else {
+ list_del(&d->vd.node);
+ vchan_cookie_complete(&d->vd);
+ }
+ break;
+ default:
+ break;
+ }
+ spin_unlock_irqrestore(&c->vc.lock, flags);
+
+ return IRQ_HANDLED;
+}
+
+static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
+{
+ struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ u32 status;
+ int ret = -EBUSY;
+
+ pm_runtime_get_sync(mtkd->ddev.dev);
+
+ mtk_uart_apdma_chan_write(c, VFF_ADDR, 0);
+ mtk_uart_apdma_chan_write(c, VFF_THRE, 0);
+ mtk_uart_apdma_chan_write(c, VFF_LEN, 0);
+ mtk_uart_apdma_chan_write(c, VFF_RST, VFF_WARM_RST_B);
+
+ ret = readx_poll_timeout(readl,
+ c->base + VFF_EN,
+ status, status == 0, 10, 100);
+ if (ret) {
+ dev_err(c->vc.chan.device->dev,
+ "dma reset: fail, timeout\n");
+ goto exit;
+ }
+
+ if (!c->requested) {
+ c->requested = true;
+ ret = request_irq(mtkd->dma_irq[chan->chan_id],
+ mtk_uart_apdma_irq_handler,
+ IRQF_TRIGGER_NONE,
+ KBUILD_MODNAME, chan);
+ if (ret < 0) {
+ dev_err(chan->device->dev, "Can't request dma IRQ\n");
+ return -EINVAL;
+ }
+ }
+
+ if (mtkd->support_33bits)
+ mtk_uart_apdma_chan_write(c,
+ VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
+
+exit:
+ return ret;
+}
+
+static void mtk_uart_apdma_free_chan_resources(struct dma_chan *chan)
+{
+ struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+
+ if (c->requested) {
+ c->requested = false;
+ free_irq(mtkd->dma_irq[chan->chan_id], chan);
+ }
+
+ tasklet_kill(&c->vc.task);
+
+ vchan_free_chan_resources(&c->vc);
+
+ pm_runtime_put_sync(mtkd->ddev.dev);
+}
+
+static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
+ dma_cookie_t cookie,
+ struct dma_tx_state *txstate)
+{
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ enum dma_status ret;
+ unsigned long flags;
+
+ if (!txstate)
+ return DMA_ERROR;
+
+ ret = dma_cookie_status(chan, cookie, txstate);
+ spin_lock_irqsave(&c->vc.lock, flags);
+ if (ret == DMA_IN_PROGRESS) {
+ c->rx_status = mtk_uart_apdma_chan_read(c, VFF_RPT)
+ & MTK_UART_APDMA_RING_SIZE;
+ dma_set_residue(txstate, c->rx_status);
+ } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
+ dma_set_residue(txstate, c->rx_status);
+ } else {
+ dma_set_residue(txstate, 0);
+ }
+ spin_unlock_irqrestore(&c->vc.lock, flags);
+
+ return ret;
+}
+
+/*
+ * dmaengine_prep_slave_single will call the function. and sglen is 1.
+ * 8250 uart using one ring buffer, and deal with one sg.
+ */
+static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
+ (struct dma_chan *chan, struct scatterlist *sgl,
+ unsigned int sglen, enum dma_transfer_direction dir,
+ unsigned long tx_flags, void *context)
+{
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ struct mtk_uart_apdma_desc *d;
+
+ if ((dir != DMA_DEV_TO_MEM) &&
+ (dir != DMA_MEM_TO_DEV)) {
+ dev_err(chan->device->dev, "bad direction\n");
+ return NULL;
+ }
+
+ /* Now allocate and setup the descriptor */
+ d = kzalloc(sizeof(*d), GFP_ATOMIC);
+ if (!d)
+ return NULL;
+
+ /* sglen is 1 */
+ d->avail_len = sg_dma_len(sgl);
+
+ return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
+}
+
+static void mtk_uart_apdma_issue_pending(struct dma_chan *chan)
+{
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ struct virt_dma_desc *vd;
+ unsigned long flags;
+
+ spin_lock_irqsave(&c->vc.lock, flags);
+ if (c->cfg.direction == DMA_DEV_TO_MEM) {
+ if (vchan_issue_pending(&c->vc) && !c->desc) {
+ vd = vchan_next_desc(&c->vc);
+ c->desc = to_mtk_uart_apdma_desc(&vd->tx);
+ mtk_uart_apdma_start_rx(c);
+ }
+ } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
+ if (vchan_issue_pending(&c->vc) && !c->desc) {
+ vd = vchan_next_desc(&c->vc);
+ c->desc = to_mtk_uart_apdma_desc(&vd->tx);
+ mtk_uart_apdma_start_tx(c);
+ }
+ }
+ spin_unlock_irqrestore(&c->vc.lock, flags);
+}
+
+static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
+ struct dma_slave_config *cfg)
+{
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ struct mtk_uart_apdmadev *mtkd =
+ to_mtk_uart_apdma_dev(c->vc.chan.device);
+
+ c->cfg = *cfg;
+
+ if (cfg->direction == DMA_DEV_TO_MEM) {
+ unsigned int rx_len = cfg->src_addr_width * 1024;
+
+ mtk_uart_apdma_chan_write(c, VFF_ADDR, cfg->src_addr);
+ mtk_uart_apdma_chan_write(c, VFF_LEN, rx_len);
+ mtk_uart_apdma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
+ mtk_uart_apdma_chan_write(c,
+ VFF_INT_EN, VFF_RX_INT_EN0_B
+ | VFF_RX_INT_EN1_B);
+ mtk_uart_apdma_chan_write(c, VFF_RPT, 0);
+ mtk_uart_apdma_chan_write(c,
+ VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
+ mtk_uart_apdma_chan_write(c, VFF_EN, VFF_EN_B);
+ } else if (cfg->direction == DMA_MEM_TO_DEV) {
+ unsigned int tx_len = cfg->dst_addr_width * 1024;
+
+ mtk_uart_apdma_chan_write(c, VFF_ADDR, cfg->dst_addr);
+ mtk_uart_apdma_chan_write(c, VFF_LEN, tx_len);
+ mtk_uart_apdma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
+ mtk_uart_apdma_chan_write(c, VFF_WPT, 0);
+ mtk_uart_apdma_chan_write(c,
+ VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
+ mtk_uart_apdma_chan_write(c, VFF_EN, VFF_EN_B);
+ }
+
+ if (mtkd->support_33bits)
+ mtk_uart_apdma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
+
+ if (mtk_uart_apdma_chan_read(c, VFF_EN) != VFF_EN_B) {
+ dev_err(chan->device->dev,
+ "config dma dir[%d] fail\n", cfg->direction);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
+{
+ struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
+ unsigned long flags;
+ u32 status;
+ int ret;
+
+ spin_lock_irqsave(&c->vc.lock, flags);
+
+ mtk_uart_apdma_chan_write(c, VFF_FLUSH, VFF_FLUSH_CLR_B);
+ /* Wait for flush */
+ ret = readx_poll_timeout(readl,
+ c->base + VFF_FLUSH,
+ status,
+ (status & VFF_FLUSH_B) != VFF_FLUSH_B,
+ 10, 100);
+ if (ret)
+ dev_err(c->vc.chan.device->dev,
+ "dma stop: polling FLUSH fail, DEBUG=0x%x\n",
+ mtk_uart_apdma_chan_read(c, VFF_DEBUG_STATUS));
+
+ /*set stop as 1 -> wait until en is 0 -> set stop as 0*/
+ mtk_uart_apdma_chan_write(c, VFF_STOP, VFF_STOP_B);
+ ret = readx_poll_timeout(readl,
+ c->base + VFF_EN,
+ status, status == 0, 10, 100);
+ if (ret)
+ dev_err(c->vc.chan.device->dev,
+ "dma stop: polling VFF_EN fail, DEBUG=0x%x\n",
+ mtk_uart_apdma_chan_read(c, VFF_DEBUG_STATUS));
+
+ mtk_uart_apdma_chan_write(c, VFF_STOP, VFF_STOP_CLR_B);
+ mtk_uart_apdma_chan_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
+
+ switch (c->cfg.direction) {
+ case DMA_DEV_TO_MEM:
+ mtk_uart_apdma_chan_write(c,
+ VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
+ break;
+ case DMA_MEM_TO_DEV:
+ mtk_uart_apdma_chan_write(c,
+ VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
+ break;
+ default:
+ break;
+ }
+
+ spin_unlock_irqrestore(&c->vc.lock, flags);
+
+ return 0;
+}
+
+static int mtk_uart_apdma_device_pause(struct dma_chan *chan)
+{
+ /* just for check caps pass */
+ return 0;
+}
+
+static int mtk_uart_apdma_device_resume(struct dma_chan *chan)
+{
+ /* just for check caps pass */
+ return 0;
+}
+
+static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
+{
+ while (list_empty(&mtkd->ddev.channels) == 0) {
+ struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
+ struct mtk_chan, vc.chan.device_node);
+
+ list_del(&c->vc.chan.device_node);
+ tasklet_kill(&c->vc.task);
+ }
+}
+
+static const struct of_device_id mtk_uart_apdma_match[] = {
+ { .compatible = "mediatek,mt6577-uart-dma", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mtk_uart_apdma_match);
+
+static int mtk_uart_apdma_probe(struct platform_device *pdev)
+{
+ struct mtk_uart_apdmadev *mtkd;
+ struct resource *res;
+ struct mtk_chan *c;
+ unsigned int i;
+ int rc;
+
+ mtkd = devm_kzalloc(&pdev->dev, sizeof(*mtkd), GFP_KERNEL);
+ if (!mtkd)
+ return -ENOMEM;
+
+ mtkd->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(mtkd->clk)) {
+ dev_err(&pdev->dev, "No clock specified\n");
+ rc = PTR_ERR(mtkd->clk);
+ goto err_no_dma;
+ }
+
+ if (of_property_read_bool(pdev->dev.of_node, "dma-33bits")) {
+ dev_info(&pdev->dev, "Support dma 33bits\n");
+ mtkd->support_33bits = true;
+ }
+
+ rc = dma_set_mask_and_coherent(&pdev->dev,
+ DMA_BIT_MASK(32 | mtkd->support_33bits));
+ if (rc)
+ goto err_no_dma;
+
+ dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
+ mtkd->ddev.device_alloc_chan_resources =
+ mtk_uart_apdma_alloc_chan_resources;
+ mtkd->ddev.device_free_chan_resources =
+ mtk_uart_apdma_free_chan_resources;
+ mtkd->ddev.device_tx_status = mtk_uart_apdma_tx_status;
+ mtkd->ddev.device_issue_pending = mtk_uart_apdma_issue_pending;
+ mtkd->ddev.device_prep_slave_sg = mtk_uart_apdma_prep_slave_sg;
+ mtkd->ddev.device_config = mtk_uart_apdma_slave_config;
+ mtkd->ddev.device_pause = mtk_uart_apdma_device_pause;
+ mtkd->ddev.device_resume = mtk_uart_apdma_device_resume;
+ mtkd->ddev.device_terminate_all = mtk_uart_apdma_terminate_all;
+ mtkd->ddev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
+ mtkd->ddev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
+ mtkd->ddev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
+ mtkd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
+ mtkd->ddev.dev = &pdev->dev;
+ INIT_LIST_HEAD(&mtkd->ddev.channels);
+
+ for (i = 0; i < MTK_UART_APDMA_CHANNELS; i++) {
+ c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
+ if (!c) {
+ rc = -ENODEV;
+ goto err_no_dma;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+ if (!res) {
+ rc = -ENODEV;
+ goto err_no_dma;
+ }
+
+ c->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(c->base)) {
+ rc = PTR_ERR(c->base);
+ goto err_no_dma;
+ }
+ c->requested = false;
+ c->vc.desc_free = mtk_uart_apdma_desc_free;
+ vchan_init(&c->vc, &mtkd->ddev);
+
+ mtkd->dma_irq[i] = platform_get_irq(pdev, i);
+ if ((int)mtkd->dma_irq[i] < 0) {
+ dev_err(&pdev->dev, "failed to get IRQ[%d]\n", i);
+ rc = -EINVAL;
+ goto err_no_dma;
+ }
+ }
+
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+
+ rc = dma_async_device_register(&mtkd->ddev);
+ if (rc)
+ goto rpm_disable;
+
+ platform_set_drvdata(pdev, mtkd);
+
+ if (pdev->dev.of_node) {
+ /* Device-tree DMA controller registration */
+ rc = of_dma_controller_register(pdev->dev.of_node,
+ of_dma_xlate_by_chan_id,
+ mtkd);
+ if (rc)
+ goto dma_remove;
+ }
+
+ return rc;
+
+dma_remove:
+ dma_async_device_unregister(&mtkd->ddev);
+rpm_disable:
+ pm_runtime_disable(&pdev->dev);
+err_no_dma:
+ mtk_uart_apdma_free(mtkd);
+ return rc;
+}
+
+static int mtk_uart_apdma_remove(struct platform_device *pdev)
+{
+ struct mtk_uart_apdmadev *mtkd = platform_get_drvdata(pdev);
+
+ if (pdev->dev.of_node)
+ of_dma_controller_free(pdev->dev.of_node);
+
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_put_noidle(&pdev->dev);
+
+ dma_async_device_unregister(&mtkd->ddev);
+ mtk_uart_apdma_free(mtkd);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int mtk_uart_apdma_suspend(struct device *dev)
+{
+ struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
+
+ if (!pm_runtime_suspended(dev))
+ clk_disable_unprepare(mtkd->clk);
+
+ return 0;
+}
+
+static int mtk_uart_apdma_resume(struct device *dev)
+{
+ int ret;
+ struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
+
+ if (!pm_runtime_suspended(dev)) {
+ ret = clk_prepare_enable(mtkd->clk);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+#ifdef CONFIG_PM
+static int mtk_uart_apdma_runtime_suspend(struct device *dev)
+{
+ struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(mtkd->clk);
+
+ return 0;
+}
+
+static int mtk_uart_apdma_runtime_resume(struct device *dev)
+{
+ int ret;
+ struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
+
+ ret = clk_prepare_enable(mtkd->clk);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops mtk_uart_apdma_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(mtk_uart_apdma_suspend, mtk_uart_apdma_resume)
+ SET_RUNTIME_PM_OPS(mtk_uart_apdma_runtime_suspend,
+ mtk_uart_apdma_runtime_resume, NULL)
+};
+
+static struct platform_driver mtk_uart_apdma_driver = {
+ .probe = mtk_uart_apdma_probe,
+ .remove = mtk_uart_apdma_remove,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .pm = &mtk_uart_apdma_pm_ops,
+ .of_match_table = of_match_ptr(mtk_uart_apdma_match),
+ },
+};
+
+module_platform_driver(mtk_uart_apdma_driver);
+
+MODULE_DESCRIPTION("MediaTek UART APDMA Controller Driver");
+MODULE_AUTHOR("Long Cheng <long.cheng@mediatek.com>");
+MODULE_LICENSE("GPL v2");
+
diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
index 27bac0b..1a523c87 100644
--- a/drivers/dma/mediatek/Kconfig
+++ b/drivers/dma/mediatek/Kconfig
@@ -1,4 +1,15 @@
+config DMA_MTK_UART
+ tristate "MediaTek SoCs APDMA support for UART"
+ depends on OF && SERIAL_8250_MT6577
+ select DMA_ENGINE
+ select DMA_VIRTUAL_CHANNELS
+ help
+ Support for the UART DMA engine found on MediaTek MTK SoCs.
+ when SERIAL_8250_MT6577 is enabled, and if you want to use DMA,
+ you can enable the config. the DMA engine can only be used
+ with MediaTek SoCs.
+
config MTK_HSDMA
tristate "MediaTek High-Speed DMA controller support"
depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/dma/mediatek/Makefile b/drivers/dma/mediatek/Makefile
index 6e778f8..2f2efd9 100644
--- a/drivers/dma/mediatek/Makefile
+++ b/drivers/dma/mediatek/Makefile
@@ -1 +1,2 @@
+obj-$(CONFIG_DMA_MTK_UART) += 8250_mtk_dma.o
obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
--
1.7.9.5
^ permalink raw reply related
* [PATCH v7 2/2] arm: dts: mt2712: add uart APDMA to device tree
From: Long Cheng @ 2018-12-25 1:26 UTC (permalink / raw)
To: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang
Cc: Matthias Brugger, Dan Williams, Greg Kroah-Hartman, Jiri Slaby,
Sean Wang, dmaengine, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, linux-serial, srv_heupstream,
Yingjoe Chen, YT Shen, Long Cheng
In-Reply-To: <1545701209-6884-1-git-send-email-long.cheng@mediatek.com>
1. add uart APDMA controller device node
2. add uart 0/1/2/3/4/5 DMA function
Signed-off-by: Long Cheng <long.cheng@mediatek.com>
---
arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 50 +++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
index 976d92a..be1a22a 100644
--- a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
@@ -300,6 +300,9 @@
interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_LOW>;
clocks = <&baud_clk>, <&sys_clk>;
clock-names = "baud", "bus";
+ dmas = <&apdma 10
+ &apdma 11>;
+ dma-names = "tx", "rx";
status = "disabled";
};
@@ -369,6 +372,38 @@
(GIC_CPU_MASK_RAW(0x13) | IRQ_TYPE_LEVEL_HIGH)>;
};
+ apdma: dma-controller@11000400 {
+ compatible = "mediatek,mt2712-uart-dma",
+ "mediatek,mt6577-uart-dma";
+ reg = <0 0x11000400 0 0x80>,
+ <0 0x11000480 0 0x80>,
+ <0 0x11000500 0 0x80>,
+ <0 0x11000580 0 0x80>,
+ <0 0x11000600 0 0x80>,
+ <0 0x11000680 0 0x80>,
+ <0 0x11000700 0 0x80>,
+ <0 0x11000780 0 0x80>,
+ <0 0x11000800 0 0x80>,
+ <0 0x11000880 0 0x80>,
+ <0 0x11000900 0 0x80>,
+ <0 0x11000980 0 0x80>;
+ interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 104 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 105 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 106 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 107 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 108 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 109 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 110 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 111 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 112 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 113 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_SPI 114 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&pericfg CLK_PERI_AP_DMA>;
+ clock-names = "apdma";
+ #dma-cells = <1>;
+ };
+
auxadc: adc@11001000 {
compatible = "mediatek,mt2712-auxadc";
reg = <0 0x11001000 0 0x1000>;
@@ -385,6 +420,9 @@
interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_LOW>;
clocks = <&baud_clk>, <&sys_clk>;
clock-names = "baud", "bus";
+ dmas = <&apdma 0
+ &apdma 1>;
+ dma-names = "tx", "rx";
status = "disabled";
};
@@ -395,6 +433,9 @@
interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_LOW>;
clocks = <&baud_clk>, <&sys_clk>;
clock-names = "baud", "bus";
+ dmas = <&apdma 2
+ &apdma 3>;
+ dma-names = "tx", "rx";
status = "disabled";
};
@@ -405,6 +446,9 @@
interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_LOW>;
clocks = <&baud_clk>, <&sys_clk>;
clock-names = "baud", "bus";
+ dmas = <&apdma 4
+ &apdma 5>;
+ dma-names = "tx", "rx";
status = "disabled";
};
@@ -415,6 +459,9 @@
interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_LOW>;
clocks = <&baud_clk>, <&sys_clk>;
clock-names = "baud", "bus";
+ dmas = <&apdma 6
+ &apdma 7>;
+ dma-names = "tx", "rx";
status = "disabled";
};
@@ -629,6 +676,9 @@
interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_LOW>;
clocks = <&baud_clk>, <&sys_clk>;
clock-names = "baud", "bus";
+ dmas = <&apdma 8
+ &apdma 9>;
+ dma-names = "tx", "rx";
status = "disabled";
};
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v7 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
From: Nicolas Boichat @ 2018-12-25 7:16 UTC (permalink / raw)
To: Long Cheng
Cc: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang, Matthias Brugger, Dan Williams, Greg Kroah-Hartman,
Jiri Slaby, Sean Wang, dmaengine, devicetree,
linux-arm Mailing List, linux-mediatek, lkml, linux-serial,
srv_heupstream, Yingjoe Chen, YT Shen
In-Reply-To: <1545701209-6884-2-git-send-email-long.cheng@mediatek.com>
Not a full review, a few comments below.
Thanks,
On Tue, Dec 25, 2018 at 9:27 AM Long Cheng <long.cheng@mediatek.com> wrote:
>
> In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> the performance, can enable the function.
>
> Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> ---
> drivers/dma/mediatek/8250_mtk_dma.c | 694 +++++++++++++++++++++++++++++++++++
> drivers/dma/mediatek/Kconfig | 11 +
> drivers/dma/mediatek/Makefile | 1 +
> 3 files changed, 706 insertions(+)
> create mode 100644 drivers/dma/mediatek/8250_mtk_dma.c
>
> diff --git a/drivers/dma/mediatek/8250_mtk_dma.c b/drivers/dma/mediatek/8250_mtk_dma.c
> new file mode 100644
> index 0000000..c4090f2
> --- /dev/null
> +++ b/drivers/dma/mediatek/8250_mtk_dma.c
> @@ -0,0 +1,694 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MediaTek 8250 DMA driver.
> + *
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Long Cheng <long.cheng@mediatek.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/list.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_dma.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/iopoll.h>
Alphabetical order.
> +
> +#include "../virt-dma.h"
> +
> +#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
> +
> +#define VFF_EN_B BIT(0)
> +#define VFF_STOP_B BIT(0)
> +#define VFF_FLUSH_B BIT(0)
> +#define VFF_4G_SUPPORT_B BIT(0)
> +#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
> +#define VFF_RX_INT_EN1_B BIT(1)
> +#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
> +#define VFF_WARM_RST_B BIT(0)
> +#define VFF_RX_INT_FLAG_CLR_B (BIT(0) | BIT(1))
> +#define VFF_TX_INT_FLAG_CLR_B 0
> +#define VFF_STOP_CLR_B 0
> +#define VFF_FLUSH_CLR_B 0
> +#define VFF_INT_EN_CLR_B 0
> +#define VFF_4G_SUPPORT_CLR_B 0
> +
> +/* interrupt trigger level for tx */
> +#define VFF_TX_THRE(n) ((n) * 7 / 8)
> +/* interrupt trigger level for rx */
> +#define VFF_RX_THRE(n) ((n) * 3 / 4)
> +
> +#define MTK_UART_APDMA_RING_SIZE 0xffffU
> +/* invert this bit when wrap ring head again*/
> +#define MTK_UART_APDMA_RING_WRAP 0x10000U
> +
> +#define VFF_INT_FLAG 0x00
> +#define VFF_INT_EN 0x04
> +#define VFF_EN 0x08
> +#define VFF_RST 0x0c
> +#define VFF_STOP 0x10
> +#define VFF_FLUSH 0x14
> +#define VFF_ADDR 0x1c
> +#define VFF_LEN 0x24
> +#define VFF_THRE 0x28
> +#define VFF_WPT 0x2c
> +#define VFF_RPT 0x30
> +/*TX: the buffer size HW can read. RX: the buffer size SW can read.*/
> +#define VFF_VALID_SIZE 0x3c
> +/*TX: the buffer size SW can write. RX: the buffer size HW can write.*/
> +#define VFF_LEFT_SIZE 0x40
> +#define VFF_DEBUG_STATUS 0x50
> +#define VFF_4G_SUPPORT 0x54
> +
> +struct mtk_uart_apdmadev {
> + struct dma_device ddev;
> + struct clk *clk;
> + bool support_33bits;
> + unsigned int dma_irq[MTK_UART_APDMA_CHANNELS];
> +};
> +
> +struct mtk_uart_apdma_desc {
> + struct virt_dma_desc vd;
> +
> + unsigned int avail_len;
> +};
> +
> +struct mtk_chan {
> + struct virt_dma_chan vc;
> + struct dma_slave_config cfg;
> + void __iomem *base;
> + struct mtk_uart_apdma_desc *desc;
> +
> + bool requested;
> +
> + unsigned int rx_status;
> +};
> +
> +static inline struct mtk_uart_apdmadev *
> +to_mtk_uart_apdma_dev(struct dma_device *d)
> +{
> + return container_of(d, struct mtk_uart_apdmadev, ddev);
> +}
> +
> +static inline struct mtk_chan *to_mtk_uart_apdma_chan(struct dma_chan *c)
> +{
> + return container_of(c, struct mtk_chan, vc.chan);
> +}
> +
> +static inline struct mtk_uart_apdma_desc *to_mtk_uart_apdma_desc
> + (struct dma_async_tx_descriptor *t)
> +{
> + return container_of(t, struct mtk_uart_apdma_desc, vd.tx);
> +}
> +
> +static void mtk_uart_apdma_chan_write(struct mtk_chan *c,
> + unsigned int reg, unsigned int val)
> +{
> + writel(val, c->base + reg);
> +}
> +
> +static unsigned int
> +mtk_uart_apdma_chan_read(struct mtk_chan *c, unsigned int reg)
> +{
> + return readl(c->base + reg);
> +}
> +
> +static void mtk_uart_apdma_desc_free(struct virt_dma_desc *vd)
> +{
> + struct dma_chan *chan = vd->tx.chan;
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> +
> + kfree(c->desc);
> + c->desc = NULL;
Why? I'm afraid this may mask double-free.
> +}
> +
> +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> +{
> + unsigned int txcount = c->desc->avail_len;
Variable name is confusing... I'd rather have a boolean `transmitted`
(or something), set to 0 here, and set to 1 in the loop (but again,
maybe this is not necessary at all, see below).
> + unsigned int len, send, left, wpt, wrap;
> +
> + if (mtk_uart_apdma_chan_read(c, VFF_LEFT_SIZE) == 0U) {
> + mtk_uart_apdma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
I'd add a "return;" here and de-indent the rest of the function (that
should help to make it more readable).
> + } else {
> + len = mtk_uart_apdma_chan_read(c, VFF_LEN);
You make an assumption below that len is always a power of 2. I wonder
if we want to validate that with some BUG/WARN call.
Also, I assume VFF_LEN is constant? Maybe we can read it once only in
the probe function?
> +
> + while (((left = mtk_uart_apdma_chan_read(c,
> + VFF_LEFT_SIZE)) > 0U)
> + && (c->desc->avail_len != 0U)) {
Why do we need a while loop here? LEFT_SIZE will always be smaller
than LEN, right? Shouldn't we load as much as we can, then let an
interrupt happen and reload the FIFO at that stage? Or does this help
gain a bit of extra performance?
> + send = min_t(unsigned int, left, c->desc->avail_len);
> + wpt = mtk_uart_apdma_chan_read(c, VFF_WPT);
> + wrap = wpt & MTK_UART_APDMA_RING_WRAP ?
wrap = wrp ^ MTK_UART_APDMA_RING_WRAP ?
> + 0U : MTK_UART_APDMA_RING_WRAP;
> +
> + if ((wpt & (len - 1U)) + send < len)
> + mtk_uart_apdma_chan_write(c,
> + VFF_WPT, wpt + send);
> + else
> + mtk_uart_apdma_chan_write(c, VFF_WPT,
> + ((wpt + send) & (len - 1U))
> + | wrap);
> +
> + c->desc->avail_len -= send;
> + }
> +
> + if (txcount != c->desc->avail_len) {
We know mtk_uart_apdma_chan_read(c, VFF_LEFT_SIZE) > 0 at least once,
so this test should always be true.
Unless we have c->desc->avail_len == 0, but then we won't even call
this function... So this is always true.
> + mtk_uart_apdma_chan_write(c,
> + VFF_INT_EN, VFF_TX_INT_EN_B);
> + if (mtk_uart_apdma_chan_read(c,
> + VFF_FLUSH) == 0U)
> + mtk_uart_apdma_chan_write(c,
> + VFF_FLUSH, VFF_FLUSH_B);
> + }
> + }
> +}
> +
> +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> +{
> + struct mtk_uart_apdma_desc *d = c->desc;
> + unsigned int rx_len, wg, rg, count;
> +
> + if (mtk_uart_apdma_chan_read(c, VFF_VALID_SIZE) == 0U)
> + return;
> +
> + if (d && vchan_next_desc(&c->vc)) {
> + rx_len = mtk_uart_apdma_chan_read(c, VFF_LEN);
> + rg = mtk_uart_apdma_chan_read(c, VFF_RPT);
> + wg = mtk_uart_apdma_chan_read(c, VFF_WPT);
> + count = ((rg ^ wg) & MTK_UART_APDMA_RING_WRAP) ?
> + ((wg & MTK_UART_APDMA_RING_SIZE) +
> + rx_len - (rg & MTK_UART_APDMA_RING_SIZE)) :
> + ((wg & MTK_UART_APDMA_RING_SIZE) -
> + (rg & MTK_UART_APDMA_RING_SIZE));
This is hard to read, please use a if/else.
> +
> + c->rx_status = count;
> + mtk_uart_apdma_chan_write(c, VFF_RPT, wg);
> +
> + list_del(&d->vd.node);
> + vchan_cookie_complete(&d->vd);
> + }
> +}
> +
> +static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
> +{
> + struct dma_chan *chan = (struct dma_chan *)dev_id;
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdma_desc *d;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + switch (c->cfg.direction) {
> + case DMA_DEV_TO_MEM:
You use an if/else below in a similar case, please be consistent (IMHO
if/else is a bit more compact, so I'd use that here).
> + mtk_uart_apdma_chan_write(c,
> + VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> + mtk_uart_apdma_start_rx(c);
> + break;
> + case DMA_MEM_TO_DEV:
> + d = c->desc;
> +
> + mtk_uart_apdma_chan_write(c,
> + VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> +
> + if (d->avail_len != 0U) {
> + mtk_uart_apdma_start_tx(c);
> + } else {
> + list_del(&d->vd.node);
> + vchan_cookie_complete(&d->vd);
> + }
> + break;
> + default:
> + break;
> + }
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> +{
> + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + u32 status;
> + int ret = -EBUSY;
> +
> + pm_runtime_get_sync(mtkd->ddev.dev);
> +
> + mtk_uart_apdma_chan_write(c, VFF_ADDR, 0);
> + mtk_uart_apdma_chan_write(c, VFF_THRE, 0);
> + mtk_uart_apdma_chan_write(c, VFF_LEN, 0);
> + mtk_uart_apdma_chan_write(c, VFF_RST, VFF_WARM_RST_B);
> +
> + ret = readx_poll_timeout(readl,
> + c->base + VFF_EN,
> + status, status == 0, 10, 100);
> + if (ret) {
> + dev_err(c->vc.chan.device->dev,
> + "dma reset: fail, timeout\n");
> + goto exit;
> + }
> +
> + if (!c->requested) {
> + c->requested = true;
> + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> + mtk_uart_apdma_irq_handler,
> + IRQF_TRIGGER_NONE,
> + KBUILD_MODNAME, chan);
> + if (ret < 0) {
> + dev_err(chan->device->dev, "Can't request dma IRQ\n");
> + return -EINVAL;
> + }
> + }
> +
> + if (mtkd->support_33bits)
> + mtk_uart_apdma_chan_write(c,
> + VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
> +
> +exit:
> + return ret;
> +}
> +
> +static void mtk_uart_apdma_free_chan_resources(struct dma_chan *chan)
> +{
> + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> +
> + if (c->requested) {
> + c->requested = false;
> + free_irq(mtkd->dma_irq[chan->chan_id], chan);
> + }
> +
> + tasklet_kill(&c->vc.task);
> +
> + vchan_free_chan_resources(&c->vc);
> +
> + pm_runtime_put_sync(mtkd->ddev.dev);
> +}
> +
> +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + enum dma_status ret;
> + unsigned long flags;
> +
> + if (!txstate)
> + return DMA_ERROR;
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (ret == DMA_IN_PROGRESS) {
> + c->rx_status = mtk_uart_apdma_chan_read(c, VFF_RPT)
> + & MTK_UART_APDMA_RING_SIZE;
> + dma_set_residue(txstate, c->rx_status);
> + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
> + dma_set_residue(txstate, c->rx_status);
> + } else {
> + dma_set_residue(txstate, 0);
> + }
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + return ret;
> +}
> +
> +/*
> + * dmaengine_prep_slave_single will call the function. and sglen is 1.
> + * 8250 uart using one ring buffer, and deal with one sg.
> + */
> +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> + (struct dma_chan *chan, struct scatterlist *sgl,
> + unsigned int sglen, enum dma_transfer_direction dir,
> + unsigned long tx_flags, void *context)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdma_desc *d;
> +
> + if ((dir != DMA_DEV_TO_MEM) &&
> + (dir != DMA_MEM_TO_DEV)) {
> + dev_err(chan->device->dev, "bad direction\n");
> + return NULL;
> + }
> +
> + /* Now allocate and setup the descriptor */
> + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> + if (!d)
> + return NULL;
> +
> + /* sglen is 1 */
> + d->avail_len = sg_dma_len(sgl);
> +
> + return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
> +}
> +
> +static void mtk_uart_apdma_issue_pending(struct dma_chan *chan)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct virt_dma_desc *vd;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> + if (vchan_issue_pending(&c->vc) && !c->desc) {
> + vd = vchan_next_desc(&c->vc);
> + c->desc = to_mtk_uart_apdma_desc(&vd->tx);
> + mtk_uart_apdma_start_rx(c);
> + }
> + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> + if (vchan_issue_pending(&c->vc) && !c->desc) {
> + vd = vchan_next_desc(&c->vc);
> + c->desc = to_mtk_uart_apdma_desc(&vd->tx);
> + mtk_uart_apdma_start_tx(c);
> + }
> + }
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +}
> +
> +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> + struct dma_slave_config *cfg)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdmadev *mtkd =
> + to_mtk_uart_apdma_dev(c->vc.chan.device);
> +
> + c->cfg = *cfg;
> +
> + if (cfg->direction == DMA_DEV_TO_MEM) {
> + unsigned int rx_len = cfg->src_addr_width * 1024;
> +
> + mtk_uart_apdma_chan_write(c, VFF_ADDR, cfg->src_addr);
> + mtk_uart_apdma_chan_write(c, VFF_LEN, rx_len);
> + mtk_uart_apdma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> + mtk_uart_apdma_chan_write(c,
> + VFF_INT_EN, VFF_RX_INT_EN0_B
> + | VFF_RX_INT_EN1_B);
> + mtk_uart_apdma_chan_write(c, VFF_RPT, 0);
> + mtk_uart_apdma_chan_write(c,
> + VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> + mtk_uart_apdma_chan_write(c, VFF_EN, VFF_EN_B);
> + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> + unsigned int tx_len = cfg->dst_addr_width * 1024;
> +
> + mtk_uart_apdma_chan_write(c, VFF_ADDR, cfg->dst_addr);
> + mtk_uart_apdma_chan_write(c, VFF_LEN, tx_len);
> + mtk_uart_apdma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> + mtk_uart_apdma_chan_write(c, VFF_WPT, 0);
> + mtk_uart_apdma_chan_write(c,
> + VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> + mtk_uart_apdma_chan_write(c, VFF_EN, VFF_EN_B);
> + }
> +
> + if (mtkd->support_33bits)
> + mtk_uart_apdma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> +
> + if (mtk_uart_apdma_chan_read(c, VFF_EN) != VFF_EN_B) {
> + dev_err(chan->device->dev,
> + "config dma dir[%d] fail\n", cfg->direction);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + unsigned long flags;
> + u32 status;
> + int ret;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> +
> + mtk_uart_apdma_chan_write(c, VFF_FLUSH, VFF_FLUSH_CLR_B);
> + /* Wait for flush */
> + ret = readx_poll_timeout(readl,
> + c->base + VFF_FLUSH,
> + status,
> + (status & VFF_FLUSH_B) != VFF_FLUSH_B,
> + 10, 100);
> + if (ret)
> + dev_err(c->vc.chan.device->dev,
> + "dma stop: polling FLUSH fail, DEBUG=0x%x\n",
> + mtk_uart_apdma_chan_read(c, VFF_DEBUG_STATUS));
> +
> + /*set stop as 1 -> wait until en is 0 -> set stop as 0*/
> + mtk_uart_apdma_chan_write(c, VFF_STOP, VFF_STOP_B);
> + ret = readx_poll_timeout(readl,
> + c->base + VFF_EN,
> + status, status == 0, 10, 100);
> + if (ret)
> + dev_err(c->vc.chan.device->dev,
> + "dma stop: polling VFF_EN fail, DEBUG=0x%x\n",
> + mtk_uart_apdma_chan_read(c, VFF_DEBUG_STATUS));
> +
> + mtk_uart_apdma_chan_write(c, VFF_STOP, VFF_STOP_CLR_B);
> + mtk_uart_apdma_chan_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> +
> + switch (c->cfg.direction) {
> + case DMA_DEV_TO_MEM:
Ditto, if/else?
> + mtk_uart_apdma_chan_write(c,
> + VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> + break;
> + case DMA_MEM_TO_DEV:
> + mtk_uart_apdma_chan_write(c,
> + VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> + break;
> + default:
> + break;
> + }
> +
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_device_pause(struct dma_chan *chan)
> +{
> + /* just for check caps pass */
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_device_resume(struct dma_chan *chan)
> +{
> + /* just for check caps pass */
> + return 0;
> +}
> +
> +static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> +{
> + while (list_empty(&mtkd->ddev.channels) == 0) {
> + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> + struct mtk_chan, vc.chan.device_node);
> +
> + list_del(&c->vc.chan.device_node);
> + tasklet_kill(&c->vc.task);
> + }
> +}
> +
> +static const struct of_device_id mtk_uart_apdma_match[] = {
> + { .compatible = "mediatek,mt6577-uart-dma", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_uart_apdma_match);
> +
> +static int mtk_uart_apdma_probe(struct platform_device *pdev)
> +{
> + struct mtk_uart_apdmadev *mtkd;
> + struct resource *res;
> + struct mtk_chan *c;
> + unsigned int i;
> + int rc;
> +
> + mtkd = devm_kzalloc(&pdev->dev, sizeof(*mtkd), GFP_KERNEL);
> + if (!mtkd)
> + return -ENOMEM;
> +
> + mtkd->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(mtkd->clk)) {
> + dev_err(&pdev->dev, "No clock specified\n");
> + rc = PTR_ERR(mtkd->clk);
> + goto err_no_dma;
> + }
> +
> + if (of_property_read_bool(pdev->dev.of_node, "dma-33bits")) {
> + dev_info(&pdev->dev, "Support dma 33bits\n");
Drop this, a bit verbose.
> + mtkd->support_33bits = true;
> + }
> +
> + rc = dma_set_mask_and_coherent(&pdev->dev,
> + DMA_BIT_MASK(32 | mtkd->support_33bits));
> + if (rc)
> + goto err_no_dma;
> +
> + dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
> + mtkd->ddev.device_alloc_chan_resources =
> + mtk_uart_apdma_alloc_chan_resources;
> + mtkd->ddev.device_free_chan_resources =
> + mtk_uart_apdma_free_chan_resources;
> + mtkd->ddev.device_tx_status = mtk_uart_apdma_tx_status;
> + mtkd->ddev.device_issue_pending = mtk_uart_apdma_issue_pending;
> + mtkd->ddev.device_prep_slave_sg = mtk_uart_apdma_prep_slave_sg;
> + mtkd->ddev.device_config = mtk_uart_apdma_slave_config;
> + mtkd->ddev.device_pause = mtk_uart_apdma_device_pause;
> + mtkd->ddev.device_resume = mtk_uart_apdma_device_resume;
> + mtkd->ddev.device_terminate_all = mtk_uart_apdma_terminate_all;
> + mtkd->ddev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> + mtkd->ddev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> + mtkd->ddev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> + mtkd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> + mtkd->ddev.dev = &pdev->dev;
> + INIT_LIST_HEAD(&mtkd->ddev.channels);
> +
> + for (i = 0; i < MTK_UART_APDMA_CHANNELS; i++) {
> + c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
> + if (!c) {
> + rc = -ENODEV;
> + goto err_no_dma;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> + if (!res) {
> + rc = -ENODEV;
> + goto err_no_dma;
> + }
> +
> + c->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(c->base)) {
> + rc = PTR_ERR(c->base);
> + goto err_no_dma;
> + }
> + c->requested = false;
> + c->vc.desc_free = mtk_uart_apdma_desc_free;
> + vchan_init(&c->vc, &mtkd->ddev);
> +
> + mtkd->dma_irq[i] = platform_get_irq(pdev, i);
> + if ((int)mtkd->dma_irq[i] < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ[%d]\n", i);
> + rc = -EINVAL;
> + goto err_no_dma;
> + }
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> +
> + rc = dma_async_device_register(&mtkd->ddev);
> + if (rc)
> + goto rpm_disable;
> +
> + platform_set_drvdata(pdev, mtkd);
> +
> + if (pdev->dev.of_node) {
> + /* Device-tree DMA controller registration */
> + rc = of_dma_controller_register(pdev->dev.of_node,
> + of_dma_xlate_by_chan_id,
> + mtkd);
> + if (rc)
> + goto dma_remove;
> + }
> +
> + return rc;
> +
> +dma_remove:
> + dma_async_device_unregister(&mtkd->ddev);
> +rpm_disable:
> + pm_runtime_disable(&pdev->dev);
> +err_no_dma:
> + mtk_uart_apdma_free(mtkd);
> + return rc;
> +}
> +
> +static int mtk_uart_apdma_remove(struct platform_device *pdev)
> +{
> + struct mtk_uart_apdmadev *mtkd = platform_get_drvdata(pdev);
> +
> + if (pdev->dev.of_node)
> + of_dma_controller_free(pdev->dev.of_node);
> +
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_put_noidle(&pdev->dev);
> +
> + dma_async_device_unregister(&mtkd->ddev);
> + mtk_uart_apdma_free(mtkd);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mtk_uart_apdma_suspend(struct device *dev)
> +{
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + if (!pm_runtime_suspended(dev))
> + clk_disable_unprepare(mtkd->clk);
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_resume(struct device *dev)
> +{
> + int ret;
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + if (!pm_runtime_suspended(dev)) {
> + ret = clk_prepare_enable(mtkd->clk);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +#ifdef CONFIG_PM
> +static int mtk_uart_apdma_runtime_suspend(struct device *dev)
> +{
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(mtkd->clk);
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_runtime_resume(struct device *dev)
> +{
> + int ret;
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + ret = clk_prepare_enable(mtkd->clk);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops mtk_uart_apdma_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(mtk_uart_apdma_suspend, mtk_uart_apdma_resume)
> + SET_RUNTIME_PM_OPS(mtk_uart_apdma_runtime_suspend,
> + mtk_uart_apdma_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver mtk_uart_apdma_driver = {
> + .probe = mtk_uart_apdma_probe,
> + .remove = mtk_uart_apdma_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .pm = &mtk_uart_apdma_pm_ops,
> + .of_match_table = of_match_ptr(mtk_uart_apdma_match),
> + },
> +};
> +
> +module_platform_driver(mtk_uart_apdma_driver);
> +
> +MODULE_DESCRIPTION("MediaTek UART APDMA Controller Driver");
> +MODULE_AUTHOR("Long Cheng <long.cheng@mediatek.com>");
> +MODULE_LICENSE("GPL v2");
> +
> diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
> index 27bac0b..1a523c87 100644
> --- a/drivers/dma/mediatek/Kconfig
> +++ b/drivers/dma/mediatek/Kconfig
> @@ -1,4 +1,15 @@
>
> +config DMA_MTK_UART
> + tristate "MediaTek SoCs APDMA support for UART"
> + depends on OF && SERIAL_8250_MT6577
> + select DMA_ENGINE
> + select DMA_VIRTUAL_CHANNELS
> + help
> + Support for the UART DMA engine found on MediaTek MTK SoCs.
> + when SERIAL_8250_MT6577 is enabled, and if you want to use DMA,
> + you can enable the config. the DMA engine can only be used
> + with MediaTek SoCs.
> +
> config MTK_HSDMA
> tristate "MediaTek High-Speed DMA controller support"
> depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/dma/mediatek/Makefile b/drivers/dma/mediatek/Makefile
> index 6e778f8..2f2efd9 100644
> --- a/drivers/dma/mediatek/Makefile
> +++ b/drivers/dma/mediatek/Makefile
> @@ -1 +1,2 @@
> +obj-$(CONFIG_DMA_MTK_UART) += 8250_mtk_dma.o
> obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
> --
> 1.7.9.5
>
^ permalink raw reply
* Re: [PATCH v7 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
From: Long Cheng @ 2018-12-25 12:06 UTC (permalink / raw)
To: Nicolas Boichat
Cc: Mark Rutland, devicetree, Ryder Lee, linux-serial, srv_heupstream,
Greg Kroah-Hartman, Randy Dunlap, lkml, Sean Wang, YT Shen,
dmaengine, Vinod Koul, Rob Herring, linux-mediatek, Sean Wang,
Jiri Slaby, Matthias Brugger, Yingjoe Chen, Dan Williams,
linux-arm Mailing List
In-Reply-To: <CANMq1KA1ywLnQDzXRbLcuu-afXuj_yrprRBC4kejF4-20ztXyA@mail.gmail.com>
Thanks for your comments.
On Tue, 2018-12-25 at 15:16 +0800, Nicolas Boichat wrote:
> Not a full review, a few comments below.
>
> Thanks,
>
would you like help to full review at this version patch? and then i
can modify these at next version patch. thanks.
> On Tue, Dec 25, 2018 at 9:27 AM Long Cheng <long.cheng@mediatek.com> wrote:
> >
> > In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> > If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> > the performance, can enable the function.
> >
> > Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> > ---
> > drivers/dma/mediatek/8250_mtk_dma.c | 694 +++++++++++++++++++++++++++++++++++
> > drivers/dma/mediatek/Kconfig | 11 +
> > drivers/dma/mediatek/Makefile | 1 +
> > 3 files changed, 706 insertions(+)
> > create mode 100644 drivers/dma/mediatek/8250_mtk_dma.c
> >
> > diff --git a/drivers/dma/mediatek/8250_mtk_dma.c b/drivers/dma/mediatek/8250_mtk_dma.c
> > new file mode 100644
> > index 0000000..c4090f2
> > --- /dev/null
> > +++ b/drivers/dma/mediatek/8250_mtk_dma.c
> > @@ -0,0 +1,694 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * MediaTek 8250 DMA driver.
> > + *
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Long Cheng <long.cheng@mediatek.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/list.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_dma.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/iopoll.h>
>
> Alphabetical order.
ok, i will order.
>
> > +
> > +#include "../virt-dma.h"
> > +
> > +#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
> > +
> > +#define VFF_EN_B BIT(0)
> > +#define VFF_STOP_B BIT(0)
> > +#define VFF_FLUSH_B BIT(0)
> > +#define VFF_4G_SUPPORT_B BIT(0)
> > +#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
> > +#define VFF_RX_INT_EN1_B BIT(1)
> > +#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
> > +#define VFF_WARM_RST_B BIT(0)
> > +#define VFF_RX_INT_FLAG_CLR_B (BIT(0) | BIT(1))
> > +#define VFF_TX_INT_FLAG_CLR_B 0
> > +#define VFF_STOP_CLR_B 0
> > +#define VFF_FLUSH_CLR_B 0
> > +#define VFF_INT_EN_CLR_B 0
> > +#define VFF_4G_SUPPORT_CLR_B 0
> > +
> > +/* interrupt trigger level for tx */
> > +#define VFF_TX_THRE(n) ((n) * 7 / 8)
> > +/* interrupt trigger level for rx */
> > +#define VFF_RX_THRE(n) ((n) * 3 / 4)
> > +
> > +#define MTK_UART_APDMA_RING_SIZE 0xffffU
> > +/* invert this bit when wrap ring head again*/
> > +#define MTK_UART_APDMA_RING_WRAP 0x10000U
> > +
> > +#define VFF_INT_FLAG 0x00
> > +#define VFF_INT_EN 0x04
> > +#define VFF_EN 0x08
> > +#define VFF_RST 0x0c
> > +#define VFF_STOP 0x10
> > +#define VFF_FLUSH 0x14
> > +#define VFF_ADDR 0x1c
> > +#define VFF_LEN 0x24
> > +#define VFF_THRE 0x28
> > +#define VFF_WPT 0x2c
> > +#define VFF_RPT 0x30
> > +/*TX: the buffer size HW can read. RX: the buffer size SW can read.*/
> > +#define VFF_VALID_SIZE 0x3c
> > +/*TX: the buffer size SW can write. RX: the buffer size HW can write.*/
> > +#define VFF_LEFT_SIZE 0x40
> > +#define VFF_DEBUG_STATUS 0x50
> > +#define VFF_4G_SUPPORT 0x54
> > +
> > +struct mtk_uart_apdmadev {
> > + struct dma_device ddev;
> > + struct clk *clk;
> > + bool support_33bits;
> > + unsigned int dma_irq[MTK_UART_APDMA_CHANNELS];
> > +};
> > +
> > +struct mtk_uart_apdma_desc {
> > + struct virt_dma_desc vd;
> > +
> > + unsigned int avail_len;
> > +};
> > +
> > +struct mtk_chan {
> > + struct virt_dma_chan vc;
> > + struct dma_slave_config cfg;
> > + void __iomem *base;
> > + struct mtk_uart_apdma_desc *desc;
> > +
> > + bool requested;
> > +
> > + unsigned int rx_status;
> > +};
> > +
> > +static inline struct mtk_uart_apdmadev *
> > +to_mtk_uart_apdma_dev(struct dma_device *d)
> > +{
> > + return container_of(d, struct mtk_uart_apdmadev, ddev);
> > +}
> > +
> > +static inline struct mtk_chan *to_mtk_uart_apdma_chan(struct dma_chan *c)
> > +{
> > + return container_of(c, struct mtk_chan, vc.chan);
> > +}
> > +
> > +static inline struct mtk_uart_apdma_desc *to_mtk_uart_apdma_desc
> > + (struct dma_async_tx_descriptor *t)
> > +{
> > + return container_of(t, struct mtk_uart_apdma_desc, vd.tx);
> > +}
> > +
> > +static void mtk_uart_apdma_chan_write(struct mtk_chan *c,
> > + unsigned int reg, unsigned int val)
> > +{
> > + writel(val, c->base + reg);
> > +}
> > +
> > +static unsigned int
> > +mtk_uart_apdma_chan_read(struct mtk_chan *c, unsigned int reg)
> > +{
> > + return readl(c->base + reg);
> > +}
> > +
> > +static void mtk_uart_apdma_desc_free(struct virt_dma_desc *vd)
> > +{
> > + struct dma_chan *chan = vd->tx.chan;
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > +
> > + kfree(c->desc);
> > + c->desc = NULL;
>
> Why? I'm afraid this may mask double-free.
>
i will remove it.
> > +}
> > +
> > +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> > +{
> > + unsigned int txcount = c->desc->avail_len;
>
> Variable name is confusing... I'd rather have a boolean `transmitted`
> (or something), set to 0 here, and set to 1 in the loop (but again,
> maybe this is not necessary at all, see below).
ok, i modify the function and remove it.
>
> > + unsigned int len, send, left, wpt, wrap;
> > +
> > + if (mtk_uart_apdma_chan_read(c, VFF_LEFT_SIZE) == 0U) {
> > + mtk_uart_apdma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
>
> I'd add a "return;" here and de-indent the rest of the function (that
> should help to make it more readable).
add
>
> > + } else {
> > + len = mtk_uart_apdma_chan_read(c, VFF_LEN);
>
> You make an assumption below that len is always a power of 2. I wonder
> if we want to validate that with some BUG/WARN call.
>
> Also, I assume VFF_LEN is constant? Maybe we can read it once only in
> the probe function?
>
no, the value of VFF_LEN isn't constant.
> > +
> > + while (((left = mtk_uart_apdma_chan_read(c,
> > + VFF_LEFT_SIZE)) > 0U)
> > + && (c->desc->avail_len != 0U)) {
>
> Why do we need a while loop here? LEFT_SIZE will always be smaller
> than LEN, right? Shouldn't we load as much as we can, then let an
> interrupt happen and reload the FIFO at that stage? Or does this help
> gain a bit of extra performance?
i will remove while, use 'if' instead of.
>
> > + send = min_t(unsigned int, left, c->desc->avail_len);
> > + wpt = mtk_uart_apdma_chan_read(c, VFF_WPT);
> > + wrap = wpt & MTK_UART_APDMA_RING_WRAP ?
>
> wrap = wrp ^ MTK_UART_APDMA_RING_WRAP ?
>
wrap = wpt & MTK_UART_APDMA_RING_WRAP ?
i had test it.
> > + 0U : MTK_UART_APDMA_RING_WRAP;
> > +
> > + if ((wpt & (len - 1U)) + send < len)
> > + mtk_uart_apdma_chan_write(c,
> > + VFF_WPT, wpt + send);
> > + else
> > + mtk_uart_apdma_chan_write(c, VFF_WPT,
> > + ((wpt + send) & (len - 1U))
> > + | wrap);
> > +
> > + c->desc->avail_len -= send;
> > + }
> > +
> > + if (txcount != c->desc->avail_len) {
>
> We know mtk_uart_apdma_chan_read(c, VFF_LEFT_SIZE) > 0 at least once,
> so this test should always be true.
>
> Unless we have c->desc->avail_len == 0, but then we won't even call
> this function... So this is always true.
>
yes. you are right. i will remove it.
> > + mtk_uart_apdma_chan_write(c,
> > + VFF_INT_EN, VFF_TX_INT_EN_B);
> > + if (mtk_uart_apdma_chan_read(c,
> > + VFF_FLUSH) == 0U)
> > + mtk_uart_apdma_chan_write(c,
> > + VFF_FLUSH, VFF_FLUSH_B);
> > + }
> > + }
> > +}
> > +
> > +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> > +{
> > + struct mtk_uart_apdma_desc *d = c->desc;
> > + unsigned int rx_len, wg, rg, count;
> > +
> > + if (mtk_uart_apdma_chan_read(c, VFF_VALID_SIZE) == 0U)
> > + return;
> > +
> > + if (d && vchan_next_desc(&c->vc)) {
> > + rx_len = mtk_uart_apdma_chan_read(c, VFF_LEN);
> > + rg = mtk_uart_apdma_chan_read(c, VFF_RPT);
> > + wg = mtk_uart_apdma_chan_read(c, VFF_WPT);
> > + count = ((rg ^ wg) & MTK_UART_APDMA_RING_WRAP) ?
> > + ((wg & MTK_UART_APDMA_RING_SIZE) +
> > + rx_len - (rg & MTK_UART_APDMA_RING_SIZE)) :
> > + ((wg & MTK_UART_APDMA_RING_SIZE) -
> > + (rg & MTK_UART_APDMA_RING_SIZE));
>
> This is hard to read, please use a if/else.
>
ok.
> > +
> > + c->rx_status = count;
> > + mtk_uart_apdma_chan_write(c, VFF_RPT, wg);
> > +
> > + list_del(&d->vd.node);
> > + vchan_cookie_complete(&d->vd);
> > + }
> > +}
> > +
> > +static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
> > +{
> > + struct dma_chan *chan = (struct dma_chan *)dev_id;
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > + struct mtk_uart_apdma_desc *d;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + switch (c->cfg.direction) {
> > + case DMA_DEV_TO_MEM:
>
> You use an if/else below in a similar case, please be consistent (IMHO
> if/else is a bit more compact, so I'd use that here).
>
sean wang had review it. the link
http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016292.html
> > + mtk_uart_apdma_chan_write(c,
> > + VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > + mtk_uart_apdma_start_rx(c);
> > + break;
> > + case DMA_MEM_TO_DEV:
> > + d = c->desc;
> > +
> > + mtk_uart_apdma_chan_write(c,
> > + VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > +
> > + if (d->avail_len != 0U) {
> > + mtk_uart_apdma_start_tx(c);
> > + } else {
> > + list_del(&d->vd.node);
> > + vchan_cookie_complete(&d->vd);
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> > +{
> > + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > + u32 status;
> > + int ret = -EBUSY;
> > +
> > + pm_runtime_get_sync(mtkd->ddev.dev);
> > +
> > + mtk_uart_apdma_chan_write(c, VFF_ADDR, 0);
> > + mtk_uart_apdma_chan_write(c, VFF_THRE, 0);
> > + mtk_uart_apdma_chan_write(c, VFF_LEN, 0);
> > + mtk_uart_apdma_chan_write(c, VFF_RST, VFF_WARM_RST_B);
> > +
> > + ret = readx_poll_timeout(readl,
> > + c->base + VFF_EN,
> > + status, status == 0, 10, 100);
> > + if (ret) {
> > + dev_err(c->vc.chan.device->dev,
> > + "dma reset: fail, timeout\n");
> > + goto exit;
> > + }
> > +
> > + if (!c->requested) {
> > + c->requested = true;
> > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > + mtk_uart_apdma_irq_handler,
> > + IRQF_TRIGGER_NONE,
> > + KBUILD_MODNAME, chan);
> > + if (ret < 0) {
> > + dev_err(chan->device->dev, "Can't request dma IRQ\n");
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + if (mtkd->support_33bits)
> > + mtk_uart_apdma_chan_write(c,
> > + VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
> > +
> > +exit:
> > + return ret;
> > +}
> > +
> > +static void mtk_uart_apdma_free_chan_resources(struct dma_chan *chan)
> > +{
> > + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > +
> > + if (c->requested) {
> > + c->requested = false;
> > + free_irq(mtkd->dma_irq[chan->chan_id], chan);
> > + }
> > +
> > + tasklet_kill(&c->vc.task);
> > +
> > + vchan_free_chan_resources(&c->vc);
> > +
> > + pm_runtime_put_sync(mtkd->ddev.dev);
> > +}
> > +
> > +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> > + dma_cookie_t cookie,
> > + struct dma_tx_state *txstate)
> > +{
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > + enum dma_status ret;
> > + unsigned long flags;
> > +
> > + if (!txstate)
> > + return DMA_ERROR;
> > +
> > + ret = dma_cookie_status(chan, cookie, txstate);
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (ret == DMA_IN_PROGRESS) {
> > + c->rx_status = mtk_uart_apdma_chan_read(c, VFF_RPT)
> > + & MTK_UART_APDMA_RING_SIZE;
> > + dma_set_residue(txstate, c->rx_status);
> > + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
> > + dma_set_residue(txstate, c->rx_status);
> > + } else {
> > + dma_set_residue(txstate, 0);
> > + }
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
> > + return ret;
> > +}
> > +
> > +/*
> > + * dmaengine_prep_slave_single will call the function. and sglen is 1.
> > + * 8250 uart using one ring buffer, and deal with one sg.
> > + */
> > +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> > + (struct dma_chan *chan, struct scatterlist *sgl,
> > + unsigned int sglen, enum dma_transfer_direction dir,
> > + unsigned long tx_flags, void *context)
> > +{
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > + struct mtk_uart_apdma_desc *d;
> > +
> > + if ((dir != DMA_DEV_TO_MEM) &&
> > + (dir != DMA_MEM_TO_DEV)) {
> > + dev_err(chan->device->dev, "bad direction\n");
> > + return NULL;
> > + }
> > +
> > + /* Now allocate and setup the descriptor */
> > + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> > + if (!d)
> > + return NULL;
> > +
> > + /* sglen is 1 */
> > + d->avail_len = sg_dma_len(sgl);
> > +
> > + return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
> > +}
> > +
> > +static void mtk_uart_apdma_issue_pending(struct dma_chan *chan)
> > +{
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > + struct virt_dma_desc *vd;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> > + if (vchan_issue_pending(&c->vc) && !c->desc) {
> > + vd = vchan_next_desc(&c->vc);
> > + c->desc = to_mtk_uart_apdma_desc(&vd->tx);
> > + mtk_uart_apdma_start_rx(c);
> > + }
> > + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> > + if (vchan_issue_pending(&c->vc) && !c->desc) {
> > + vd = vchan_next_desc(&c->vc);
> > + c->desc = to_mtk_uart_apdma_desc(&vd->tx);
> > + mtk_uart_apdma_start_tx(c);
> > + }
> > + }
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +}
> > +
> > +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> > + struct dma_slave_config *cfg)
> > +{
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > + struct mtk_uart_apdmadev *mtkd =
> > + to_mtk_uart_apdma_dev(c->vc.chan.device);
> > +
> > + c->cfg = *cfg;
> > +
> > + if (cfg->direction == DMA_DEV_TO_MEM) {
> > + unsigned int rx_len = cfg->src_addr_width * 1024;
> > +
> > + mtk_uart_apdma_chan_write(c, VFF_ADDR, cfg->src_addr);
> > + mtk_uart_apdma_chan_write(c, VFF_LEN, rx_len);
> > + mtk_uart_apdma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> > + mtk_uart_apdma_chan_write(c,
> > + VFF_INT_EN, VFF_RX_INT_EN0_B
> > + | VFF_RX_INT_EN1_B);
> > + mtk_uart_apdma_chan_write(c, VFF_RPT, 0);
> > + mtk_uart_apdma_chan_write(c,
> > + VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > + mtk_uart_apdma_chan_write(c, VFF_EN, VFF_EN_B);
> > + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> > + unsigned int tx_len = cfg->dst_addr_width * 1024;
> > +
> > + mtk_uart_apdma_chan_write(c, VFF_ADDR, cfg->dst_addr);
> > + mtk_uart_apdma_chan_write(c, VFF_LEN, tx_len);
> > + mtk_uart_apdma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> > + mtk_uart_apdma_chan_write(c, VFF_WPT, 0);
> > + mtk_uart_apdma_chan_write(c,
> > + VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > + mtk_uart_apdma_chan_write(c, VFF_EN, VFF_EN_B);
> > + }
> > +
> > + if (mtkd->support_33bits)
> > + mtk_uart_apdma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> > +
> > + if (mtk_uart_apdma_chan_read(c, VFF_EN) != VFF_EN_B) {
> > + dev_err(chan->device->dev,
> > + "config dma dir[%d] fail\n", cfg->direction);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
> > +{
> > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > + unsigned long flags;
> > + u32 status;
> > + int ret;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > +
> > + mtk_uart_apdma_chan_write(c, VFF_FLUSH, VFF_FLUSH_CLR_B);
> > + /* Wait for flush */
> > + ret = readx_poll_timeout(readl,
> > + c->base + VFF_FLUSH,
> > + status,
> > + (status & VFF_FLUSH_B) != VFF_FLUSH_B,
> > + 10, 100);
> > + if (ret)
> > + dev_err(c->vc.chan.device->dev,
> > + "dma stop: polling FLUSH fail, DEBUG=0x%x\n",
> > + mtk_uart_apdma_chan_read(c, VFF_DEBUG_STATUS));
> > +
> > + /*set stop as 1 -> wait until en is 0 -> set stop as 0*/
> > + mtk_uart_apdma_chan_write(c, VFF_STOP, VFF_STOP_B);
> > + ret = readx_poll_timeout(readl,
> > + c->base + VFF_EN,
> > + status, status == 0, 10, 100);
> > + if (ret)
> > + dev_err(c->vc.chan.device->dev,
> > + "dma stop: polling VFF_EN fail, DEBUG=0x%x\n",
> > + mtk_uart_apdma_chan_read(c, VFF_DEBUG_STATUS));
> > +
> > + mtk_uart_apdma_chan_write(c, VFF_STOP, VFF_STOP_CLR_B);
> > + mtk_uart_apdma_chan_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> > +
> > + switch (c->cfg.direction) {
> > + case DMA_DEV_TO_MEM:
>
> Ditto, if/else?
>
ditto.
http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016292.html
> > + mtk_uart_apdma_chan_write(c,
> > + VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > + break;
> > + case DMA_MEM_TO_DEV:
> > + mtk_uart_apdma_chan_write(c,
> > + VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_uart_apdma_device_pause(struct dma_chan *chan)
> > +{
> > + /* just for check caps pass */
> > + return 0;
> > +}
> > +
> > +static int mtk_uart_apdma_device_resume(struct dma_chan *chan)
> > +{
> > + /* just for check caps pass */
> > + return 0;
> > +}
> > +
> > +static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> > +{
> > + while (list_empty(&mtkd->ddev.channels) == 0) {
> > + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> > + struct mtk_chan, vc.chan.device_node);
> > +
> > + list_del(&c->vc.chan.device_node);
> > + tasklet_kill(&c->vc.task);
> > + }
> > +}
> > +
> > +static const struct of_device_id mtk_uart_apdma_match[] = {
> > + { .compatible = "mediatek,mt6577-uart-dma", },
> > + { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_uart_apdma_match);
> > +
> > +static int mtk_uart_apdma_probe(struct platform_device *pdev)
> > +{
> > + struct mtk_uart_apdmadev *mtkd;
> > + struct resource *res;
> > + struct mtk_chan *c;
> > + unsigned int i;
> > + int rc;
> > +
> > + mtkd = devm_kzalloc(&pdev->dev, sizeof(*mtkd), GFP_KERNEL);
> > + if (!mtkd)
> > + return -ENOMEM;
> > +
> > + mtkd->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(mtkd->clk)) {
> > + dev_err(&pdev->dev, "No clock specified\n");
> > + rc = PTR_ERR(mtkd->clk);
> > + goto err_no_dma;
> > + }
> > +
> > + if (of_property_read_bool(pdev->dev.of_node, "dma-33bits")) {
> > + dev_info(&pdev->dev, "Support dma 33bits\n");
>
> Drop this, a bit verbose.
>
> > + mtkd->support_33bits = true;
> > + }
> > +
> > + rc = dma_set_mask_and_coherent(&pdev->dev,
> > + DMA_BIT_MASK(32 | mtkd->support_33bits));
> > + if (rc)
> > + goto err_no_dma;
> > +
> > + dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
> > + mtkd->ddev.device_alloc_chan_resources =
> > + mtk_uart_apdma_alloc_chan_resources;
> > + mtkd->ddev.device_free_chan_resources =
> > + mtk_uart_apdma_free_chan_resources;
> > + mtkd->ddev.device_tx_status = mtk_uart_apdma_tx_status;
> > + mtkd->ddev.device_issue_pending = mtk_uart_apdma_issue_pending;
> > + mtkd->ddev.device_prep_slave_sg = mtk_uart_apdma_prep_slave_sg;
> > + mtkd->ddev.device_config = mtk_uart_apdma_slave_config;
> > + mtkd->ddev.device_pause = mtk_uart_apdma_device_pause;
> > + mtkd->ddev.device_resume = mtk_uart_apdma_device_resume;
> > + mtkd->ddev.device_terminate_all = mtk_uart_apdma_terminate_all;
> > + mtkd->ddev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> > + mtkd->ddev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> > + mtkd->ddev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> > + mtkd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> > + mtkd->ddev.dev = &pdev->dev;
> > + INIT_LIST_HEAD(&mtkd->ddev.channels);
> > +
> > + for (i = 0; i < MTK_UART_APDMA_CHANNELS; i++) {
> > + c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
> > + if (!c) {
> > + rc = -ENODEV;
> > + goto err_no_dma;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > + if (!res) {
> > + rc = -ENODEV;
> > + goto err_no_dma;
> > + }
> > +
> > + c->base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(c->base)) {
> > + rc = PTR_ERR(c->base);
> > + goto err_no_dma;
> > + }
> > + c->requested = false;
> > + c->vc.desc_free = mtk_uart_apdma_desc_free;
> > + vchan_init(&c->vc, &mtkd->ddev);
> > +
> > + mtkd->dma_irq[i] = platform_get_irq(pdev, i);
> > + if ((int)mtkd->dma_irq[i] < 0) {
> > + dev_err(&pdev->dev, "failed to get IRQ[%d]\n", i);
> > + rc = -EINVAL;
> > + goto err_no_dma;
> > + }
> > + }
> > +
> > + pm_runtime_enable(&pdev->dev);
> > + pm_runtime_set_active(&pdev->dev);
> > +
> > + rc = dma_async_device_register(&mtkd->ddev);
> > + if (rc)
> > + goto rpm_disable;
> > +
> > + platform_set_drvdata(pdev, mtkd);
> > +
> > + if (pdev->dev.of_node) {
> > + /* Device-tree DMA controller registration */
> > + rc = of_dma_controller_register(pdev->dev.of_node,
> > + of_dma_xlate_by_chan_id,
> > + mtkd);
> > + if (rc)
> > + goto dma_remove;
> > + }
> > +
> > + return rc;
> > +
> > +dma_remove:
> > + dma_async_device_unregister(&mtkd->ddev);
> > +rpm_disable:
> > + pm_runtime_disable(&pdev->dev);
> > +err_no_dma:
> > + mtk_uart_apdma_free(mtkd);
> > + return rc;
> > +}
> > +
> > +static int mtk_uart_apdma_remove(struct platform_device *pdev)
> > +{
> > + struct mtk_uart_apdmadev *mtkd = platform_get_drvdata(pdev);
> > +
> > + if (pdev->dev.of_node)
> > + of_dma_controller_free(pdev->dev.of_node);
> > +
> > + pm_runtime_disable(&pdev->dev);
> > + pm_runtime_put_noidle(&pdev->dev);
> > +
> > + dma_async_device_unregister(&mtkd->ddev);
> > + mtk_uart_apdma_free(mtkd);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int mtk_uart_apdma_suspend(struct device *dev)
> > +{
> > + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> > +
> > + if (!pm_runtime_suspended(dev))
> > + clk_disable_unprepare(mtkd->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_uart_apdma_resume(struct device *dev)
> > +{
> > + int ret;
> > + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> > +
> > + if (!pm_runtime_suspended(dev)) {
> > + ret = clk_prepare_enable(mtkd->clk);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +#endif /* CONFIG_PM_SLEEP */
> > +
> > +#ifdef CONFIG_PM
> > +static int mtk_uart_apdma_runtime_suspend(struct device *dev)
> > +{
> > + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> > +
> > + clk_disable_unprepare(mtkd->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_uart_apdma_runtime_resume(struct device *dev)
> > +{
> > + int ret;
> > + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> > +
> > + ret = clk_prepare_enable(mtkd->clk);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +#endif /* CONFIG_PM */
> > +
> > +static const struct dev_pm_ops mtk_uart_apdma_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mtk_uart_apdma_suspend, mtk_uart_apdma_resume)
> > + SET_RUNTIME_PM_OPS(mtk_uart_apdma_runtime_suspend,
> > + mtk_uart_apdma_runtime_resume, NULL)
> > +};
> > +
> > +static struct platform_driver mtk_uart_apdma_driver = {
> > + .probe = mtk_uart_apdma_probe,
> > + .remove = mtk_uart_apdma_remove,
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .pm = &mtk_uart_apdma_pm_ops,
> > + .of_match_table = of_match_ptr(mtk_uart_apdma_match),
> > + },
> > +};
> > +
> > +module_platform_driver(mtk_uart_apdma_driver);
> > +
> > +MODULE_DESCRIPTION("MediaTek UART APDMA Controller Driver");
> > +MODULE_AUTHOR("Long Cheng <long.cheng@mediatek.com>");
> > +MODULE_LICENSE("GPL v2");
> > +
> > diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
> > index 27bac0b..1a523c87 100644
> > --- a/drivers/dma/mediatek/Kconfig
> > +++ b/drivers/dma/mediatek/Kconfig
> > @@ -1,4 +1,15 @@
> >
> > +config DMA_MTK_UART
> > + tristate "MediaTek SoCs APDMA support for UART"
> > + depends on OF && SERIAL_8250_MT6577
> > + select DMA_ENGINE
> > + select DMA_VIRTUAL_CHANNELS
> > + help
> > + Support for the UART DMA engine found on MediaTek MTK SoCs.
> > + when SERIAL_8250_MT6577 is enabled, and if you want to use DMA,
> > + you can enable the config. the DMA engine can only be used
> > + with MediaTek SoCs.
> > +
> > config MTK_HSDMA
> > tristate "MediaTek High-Speed DMA controller support"
> > depends on ARCH_MEDIATEK || COMPILE_TEST
> > diff --git a/drivers/dma/mediatek/Makefile b/drivers/dma/mediatek/Makefile
> > index 6e778f8..2f2efd9 100644
> > --- a/drivers/dma/mediatek/Makefile
> > +++ b/drivers/dma/mediatek/Makefile
> > @@ -1 +1,2 @@
> > +obj-$(CONFIG_DMA_MTK_UART) += 8250_mtk_dma.o
> > obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
> > --
> > 1.7.9.5
> >
^ permalink raw reply
* Re: [PATCH v7 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
From: Nicolas Boichat @ 2018-12-26 0:05 UTC (permalink / raw)
To: Long Cheng
Cc: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang, Matthias Brugger, Dan Williams, Greg Kroah-Hartman,
Jiri Slaby, Sean Wang, dmaengine, devicetree,
linux-arm Mailing List, linux-mediatek, lkml, linux-serial,
srv_heupstream, Yingjoe Chen, YT Shen
In-Reply-To: <1545739611.28871.77.camel@mhfsdcap03>
On Tue, Dec 25, 2018 at 8:06 PM Long Cheng <long.cheng@mediatek.com> wrote:
>
> Thanks for your comments.
>
> On Tue, 2018-12-25 at 15:16 +0800, Nicolas Boichat wrote:
> > Not a full review, a few comments below.
> >
> > Thanks,
> >
>
> would you like help to full review at this version patch? and then i
> can modify these at next version patch. thanks.
Added a few more comments ,-)
> > On Tue, Dec 25, 2018 at 9:27 AM Long Cheng <long.cheng@mediatek.com> wrote:
> > >
> > > In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> > > If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> > > the performance, can enable the function.
> > >
> > > Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> > > ---
> > > drivers/dma/mediatek/8250_mtk_dma.c | 694 +++++++++++++++++++++++++++++++++++
> > > drivers/dma/mediatek/Kconfig | 11 +
> > > drivers/dma/mediatek/Makefile | 1 +
> > > 3 files changed, 706 insertions(+)
> > > create mode 100644 drivers/dma/mediatek/8250_mtk_dma.c
> > >
> > > diff --git a/drivers/dma/mediatek/8250_mtk_dma.c b/drivers/dma/mediatek/8250_mtk_dma.c
> > > new file mode 100644
> > > index 0000000..c4090f2
> > > --- /dev/null
> > > +++ b/drivers/dma/mediatek/8250_mtk_dma.c
> > > @@ -0,0 +1,694 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * MediaTek 8250 DMA driver.
> > > + *
> > > + * Copyright (c) 2018 MediaTek Inc.
> > > + * Author: Long Cheng <long.cheng@mediatek.com>
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/dmaengine.h>
> > > +#include <linux/dma-mapping.h>
> > > +#include <linux/err.h>
> > > +#include <linux/init.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/list.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_dma.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/spinlock.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/iopoll.h>
> >
> > Alphabetical order.
>
> ok, i will order.
>
> >
> > > +
> > > +#include "../virt-dma.h"
> > > +
> > > +#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
> > > +
> > > +#define VFF_EN_B BIT(0)
> > > +#define VFF_STOP_B BIT(0)
> > > +#define VFF_FLUSH_B BIT(0)
> > > +#define VFF_4G_SUPPORT_B BIT(0)
> > > +#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
> > > +#define VFF_RX_INT_EN1_B BIT(1)
> > > +#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
> > > +#define VFF_WARM_RST_B BIT(0)
> > > +#define VFF_RX_INT_FLAG_CLR_B (BIT(0) | BIT(1))
> > > +#define VFF_TX_INT_FLAG_CLR_B 0
> > > +#define VFF_STOP_CLR_B 0
> > > +#define VFF_FLUSH_CLR_B 0
> > > +#define VFF_INT_EN_CLR_B 0
> > > +#define VFF_4G_SUPPORT_CLR_B 0
> > > +
> > > +/* interrupt trigger level for tx */
> > > +#define VFF_TX_THRE(n) ((n) * 7 / 8)
> > > +/* interrupt trigger level for rx */
> > > +#define VFF_RX_THRE(n) ((n) * 3 / 4)
> > > +
> > > +#define MTK_UART_APDMA_RING_SIZE 0xffffU
> > > +/* invert this bit when wrap ring head again*/
> > > +#define MTK_UART_APDMA_RING_WRAP 0x10000U
> > > +
> > > +#define VFF_INT_FLAG 0x00
> > > +#define VFF_INT_EN 0x04
> > > +#define VFF_EN 0x08
> > > +#define VFF_RST 0x0c
> > > +#define VFF_STOP 0x10
> > > +#define VFF_FLUSH 0x14
> > > +#define VFF_ADDR 0x1c
> > > +#define VFF_LEN 0x24
> > > +#define VFF_THRE 0x28
> > > +#define VFF_WPT 0x2c
> > > +#define VFF_RPT 0x30
> > > +/*TX: the buffer size HW can read. RX: the buffer size SW can read.*/
> > > +#define VFF_VALID_SIZE 0x3c
> > > +/*TX: the buffer size SW can write. RX: the buffer size HW can write.*/
> > > +#define VFF_LEFT_SIZE 0x40
> > > +#define VFF_DEBUG_STATUS 0x50
> > > +#define VFF_4G_SUPPORT 0x54
> > > +
> > > +struct mtk_uart_apdmadev {
> > > + struct dma_device ddev;
> > > + struct clk *clk;
> > > + bool support_33bits;
> > > + unsigned int dma_irq[MTK_UART_APDMA_CHANNELS];
> > > +};
> > > +
> > > +struct mtk_uart_apdma_desc {
> > > + struct virt_dma_desc vd;
> > > +
> > > + unsigned int avail_len;
> > > +};
> > > +
> > > +struct mtk_chan {
> > > + struct virt_dma_chan vc;
> > > + struct dma_slave_config cfg;
> > > + void __iomem *base;
> > > + struct mtk_uart_apdma_desc *desc;
> > > +
> > > + bool requested;
> > > +
> > > + unsigned int rx_status;
> > > +};
> > > +
> > > +static inline struct mtk_uart_apdmadev *
> > > +to_mtk_uart_apdma_dev(struct dma_device *d)
> > > +{
> > > + return container_of(d, struct mtk_uart_apdmadev, ddev);
> > > +}
> > > +
> > > +static inline struct mtk_chan *to_mtk_uart_apdma_chan(struct dma_chan *c)
> > > +{
> > > + return container_of(c, struct mtk_chan, vc.chan);
> > > +}
> > > +
> > > +static inline struct mtk_uart_apdma_desc *to_mtk_uart_apdma_desc
> > > + (struct dma_async_tx_descriptor *t)
> > > +{
> > > + return container_of(t, struct mtk_uart_apdma_desc, vd.tx);
> > > +}
> > > +
> > > +static void mtk_uart_apdma_chan_write(struct mtk_chan *c,
> > > + unsigned int reg, unsigned int val)
> > > +{
> > > + writel(val, c->base + reg);
> > > +}
> > > +
> > > +static unsigned int
> > > +mtk_uart_apdma_chan_read(struct mtk_chan *c, unsigned int reg)
> > > +{
> > > + return readl(c->base + reg);
> > > +}
> > > +
> > > +static void mtk_uart_apdma_desc_free(struct virt_dma_desc *vd)
> > > +{
> > > + struct dma_chan *chan = vd->tx.chan;
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > +
> > > + kfree(c->desc);
> > > + c->desc = NULL;
> >
> > Why? I'm afraid this may mask double-free.
> >
>
> i will remove it.
>
> > > +}
> > > +
> > > +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> > > +{
> > > + unsigned int txcount = c->desc->avail_len;
> >
> > Variable name is confusing... I'd rather have a boolean `transmitted`
> > (or something), set to 0 here, and set to 1 in the loop (but again,
> > maybe this is not necessary at all, see below).
>
> ok, i modify the function and remove it.
>
> >
> > > + unsigned int len, send, left, wpt, wrap;
> > > +
> > > + if (mtk_uart_apdma_chan_read(c, VFF_LEFT_SIZE) == 0U) {
> > > + mtk_uart_apdma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> >
> > I'd add a "return;" here and de-indent the rest of the function (that
> > should help to make it more readable).
>
> add
>
> >
> > > + } else {
> > > + len = mtk_uart_apdma_chan_read(c, VFF_LEN);
> >
> > You make an assumption below that len is always a power of 2. I wonder
> > if we want to validate that with some BUG/WARN call.
> >
> > Also, I assume VFF_LEN is constant? Maybe we can read it once only in
> > the probe function?
> >
>
> no, the value of VFF_LEN isn't constant.
>
> > > +
> > > + while (((left = mtk_uart_apdma_chan_read(c,
> > > + VFF_LEFT_SIZE)) > 0U)
> > > + && (c->desc->avail_len != 0U)) {
> >
> > Why do we need a while loop here? LEFT_SIZE will always be smaller
> > than LEN, right? Shouldn't we load as much as we can, then let an
> > interrupt happen and reload the FIFO at that stage? Or does this help
> > gain a bit of extra performance?
>
> i will remove while, use 'if' instead of.
> >
> > > + send = min_t(unsigned int, left, c->desc->avail_len);
> > > + wpt = mtk_uart_apdma_chan_read(c, VFF_WPT);
> > > + wrap = wpt & MTK_UART_APDMA_RING_WRAP ?
> >
> > wrap = wrp ^ MTK_UART_APDMA_RING_WRAP ?
> >
>
> wrap = wpt & MTK_UART_APDMA_RING_WRAP ?
> i had test it.
Sorry, I got it wrong (and my question mark is confusing), what I mean is this:
wrap = (wrp ^ MTK_UART_APDMA_RING_WRAP) & MTK_UART_APDMA_RING_WRAP;
But then it's marginally shorter than your solution, so maybe keep
yours, it's a little simpler to understand.
> > > + 0U : MTK_UART_APDMA_RING_WRAP;
> > > +
> > > + if ((wpt & (len - 1U)) + send < len)
Is it possible for wpt > len? If not, then wpt + send < len should be enough.
Also, wpt + send is used a few times now, so maybe have it a separate variable?
Something like this might be nicer:
next_wpt = wpt + send;
if ((wpt & (len - 1U)) + send >= len) {
next_wpt = next_wpt & (len - 1U);
if (!(wpt & MTK_UART_APDMA_RING_WRAP))
next_wpt |= MTK_UART_APDMA_RING_WRAP;
}
mtk_uart_apdma_chan_write(c, VFF_WPT, next_wpt);
Or if the assumption than wpt < len holds:
if (next_wpt >= len) {
next_wpt = next_wpt - len;
...
> > > + mtk_uart_apdma_chan_write(c,
> > > + VFF_WPT, wpt + send);
> > > + else
> > > + mtk_uart_apdma_chan_write(c, VFF_WPT,
> > > + ((wpt + send) & (len - 1U))
> > > + | wrap);
> > > +
> > > + c->desc->avail_len -= send;
> > > + }
> > > +
> > > + if (txcount != c->desc->avail_len) {
> >
> > We know mtk_uart_apdma_chan_read(c, VFF_LEFT_SIZE) > 0 at least once,
> > so this test should always be true.
> >
> > Unless we have c->desc->avail_len == 0, but then we won't even call
> > this function... So this is always true.
> >
>
> yes. you are right. i will remove it.
>
> > > + mtk_uart_apdma_chan_write(c,
> > > + VFF_INT_EN, VFF_TX_INT_EN_B);
> > > + if (mtk_uart_apdma_chan_read(c,
> > > + VFF_FLUSH) == 0U)
> > > + mtk_uart_apdma_chan_write(c,
> > > + VFF_FLUSH, VFF_FLUSH_B);
> > > + }
> > > + }
> > > +}
> > > +
> > > +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> > > +{
> > > + struct mtk_uart_apdma_desc *d = c->desc;
> > > + unsigned int rx_len, wg, rg, count;
> > > +
> > > + if (mtk_uart_apdma_chan_read(c, VFF_VALID_SIZE) == 0U)
> > > + return;
> > > +
> > > + if (d && vchan_next_desc(&c->vc)) {
I'd negate the test to de-indent the rest of the code:
if (!d || !vchan_next_desc(&c->vc))
return;
> > > + rx_len = mtk_uart_apdma_chan_read(c, VFF_LEN);
> > > + rg = mtk_uart_apdma_chan_read(c, VFF_RPT);
> > > + wg = mtk_uart_apdma_chan_read(c, VFF_WPT);
> > > + count = ((rg ^ wg) & MTK_UART_APDMA_RING_WRAP) ?
> > > + ((wg & MTK_UART_APDMA_RING_SIZE) +
> > > + rx_len - (rg & MTK_UART_APDMA_RING_SIZE)) :
> > > + ((wg & MTK_UART_APDMA_RING_SIZE) -
> > > + (rg & MTK_UART_APDMA_RING_SIZE));
> >
> > This is hard to read, please use a if/else.
> >
>
> ok.
>
> > > +
> > > + c->rx_status = count;
> > > + mtk_uart_apdma_chan_write(c, VFF_RPT, wg);
> > > +
> > > + list_del(&d->vd.node);
> > > + vchan_cookie_complete(&d->vd);
> > > + }
> > > +}
> > > +
> > > +static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
> > > +{
> > > + struct dma_chan *chan = (struct dma_chan *)dev_id;
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + struct mtk_uart_apdma_desc *d;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > + switch (c->cfg.direction) {
> > > + case DMA_DEV_TO_MEM:
> >
> > You use an if/else below in a similar case, please be consistent (IMHO
> > if/else is a bit more compact, so I'd use that here).
> >
> sean wang had review it. the link
> http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016292.html
>
> > > + mtk_uart_apdma_chan_write(c,
> > > + VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > > + mtk_uart_apdma_start_rx(c);
> > > + break;
> > > + case DMA_MEM_TO_DEV:
> > > + d = c->desc;
> > > +
> > > + mtk_uart_apdma_chan_write(c,
> > > + VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > > +
> > > + if (d->avail_len != 0U) {
> > > + mtk_uart_apdma_start_tx(c);
> > > + } else {
> > > + list_del(&d->vd.node);
> > > + vchan_cookie_complete(&d->vd);
> > > + }
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > > +
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> > > +{
> > > + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + u32 status;
> > > + int ret = -EBUSY;
No need to init ret.
> > > +
> > > + pm_runtime_get_sync(mtkd->ddev.dev);
> > > +
> > > + mtk_uart_apdma_chan_write(c, VFF_ADDR, 0);
> > > + mtk_uart_apdma_chan_write(c, VFF_THRE, 0);
> > > + mtk_uart_apdma_chan_write(c, VFF_LEN, 0);
> > > + mtk_uart_apdma_chan_write(c, VFF_RST, VFF_WARM_RST_B);
> > > +
> > > + ret = readx_poll_timeout(readl,
> > > + c->base + VFF_EN,
> > > + status, status == 0, 10, 100);
> > > + if (ret) {
> > > + dev_err(c->vc.chan.device->dev,
> > > + "dma reset: fail, timeout\n");
> > > + goto exit;
Just return ret, since you don't do any cleanup in exit:.
> > > + }
> > > +
> > > + if (!c->requested) {
> > > + c->requested = true;
> > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > + mtk_uart_apdma_irq_handler,
> > > + IRQF_TRIGGER_NONE,
> > > + KBUILD_MODNAME, chan);
> > > + if (ret < 0) {
> > > + dev_err(chan->device->dev, "Can't request dma IRQ\n");
> > > + return -EINVAL;
> > > + }
> > > + }
> > > +
> > > + if (mtkd->support_33bits)
> > > + mtk_uart_apdma_chan_write(c,
> > > + VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
> > > +
> > > +exit:
> > > + return ret;
> > > +}
> > > +
> > > +static void mtk_uart_apdma_free_chan_resources(struct dma_chan *chan)
> > > +{
> > > + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > +
> > > + if (c->requested) {
> > > + c->requested = false;
> > > + free_irq(mtkd->dma_irq[chan->chan_id], chan);
> > > + }
> > > +
> > > + tasklet_kill(&c->vc.task);
> > > +
> > > + vchan_free_chan_resources(&c->vc);
> > > +
> > > + pm_runtime_put_sync(mtkd->ddev.dev);
> > > +}
> > > +
> > > +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> > > + dma_cookie_t cookie,
> > > + struct dma_tx_state *txstate)
> > > +{
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + enum dma_status ret;
> > > + unsigned long flags;
> > > +
> > > + if (!txstate)
> > > + return DMA_ERROR;
> > > +
> > > + ret = dma_cookie_status(chan, cookie, txstate);
> > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > + if (ret == DMA_IN_PROGRESS) {
> > > + c->rx_status = mtk_uart_apdma_chan_read(c, VFF_RPT)
> > > + & MTK_UART_APDMA_RING_SIZE;
> > > + dma_set_residue(txstate, c->rx_status);
> > > + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
> > > + dma_set_residue(txstate, c->rx_status);
> > > + } else {
> > > + dma_set_residue(txstate, 0);
> > > + }
> > > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +/*
> > > + * dmaengine_prep_slave_single will call the function. and sglen is 1.
> > > + * 8250 uart using one ring buffer, and deal with one sg.
> > > + */
> > > +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> > > + (struct dma_chan *chan, struct scatterlist *sgl,
> > > + unsigned int sglen, enum dma_transfer_direction dir,
Too many spaces (or a tab?) between `sglen,` and `enum`
> > > + unsigned long tx_flags, void *context)
> > > +{
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + struct mtk_uart_apdma_desc *d;
> > > +
> > > + if ((dir != DMA_DEV_TO_MEM) &&
> > > + (dir != DMA_MEM_TO_DEV)) {
> > > + dev_err(chan->device->dev, "bad direction\n");
> > > + return NULL;
> > > + }
> > > +
> > > + /* Now allocate and setup the descriptor */
> > > + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> > > + if (!d)
> > > + return NULL;
> > > +
> > > + /* sglen is 1 */
> > > + d->avail_len = sg_dma_len(sgl);
> > > +
> > > + return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
> > > +}
> > > +
> > > +static void mtk_uart_apdma_issue_pending(struct dma_chan *chan)
> > > +{
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + struct virt_dma_desc *vd;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> > > + if (vchan_issue_pending(&c->vc) && !c->desc) {
> > > + vd = vchan_next_desc(&c->vc);
> > > + c->desc = to_mtk_uart_apdma_desc(&vd->tx);
> > > + mtk_uart_apdma_start_rx(c);
> > > + }
> > > + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> > > + if (vchan_issue_pending(&c->vc) && !c->desc) {
> > > + vd = vchan_next_desc(&c->vc);
> > > + c->desc = to_mtk_uart_apdma_desc(&vd->tx);
> > > + mtk_uart_apdma_start_tx(c);
> > > + }
> > > + }
> > > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > > +}
> > > +
> > > +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> > > + struct dma_slave_config *cfg)
> > > +{
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + struct mtk_uart_apdmadev *mtkd =
> > > + to_mtk_uart_apdma_dev(c->vc.chan.device);
> > > +
> > > + c->cfg = *cfg;
> > > +
> > > + if (cfg->direction == DMA_DEV_TO_MEM) {
> > > + unsigned int rx_len = cfg->src_addr_width * 1024;
> > > +
> > > + mtk_uart_apdma_chan_write(c, VFF_ADDR, cfg->src_addr);
> > > + mtk_uart_apdma_chan_write(c, VFF_LEN, rx_len);
> > > + mtk_uart_apdma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> > > + mtk_uart_apdma_chan_write(c,
> > > + VFF_INT_EN, VFF_RX_INT_EN0_B
> > > + | VFF_RX_INT_EN1_B);
> > > + mtk_uart_apdma_chan_write(c, VFF_RPT, 0);
> > > + mtk_uart_apdma_chan_write(c,
> > > + VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > > + mtk_uart_apdma_chan_write(c, VFF_EN, VFF_EN_B);
> > > + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> > > + unsigned int tx_len = cfg->dst_addr_width * 1024;
> > > +
> > > + mtk_uart_apdma_chan_write(c, VFF_ADDR, cfg->dst_addr);
> > > + mtk_uart_apdma_chan_write(c, VFF_LEN, tx_len);
> > > + mtk_uart_apdma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> > > + mtk_uart_apdma_chan_write(c, VFF_WPT, 0);
> > > + mtk_uart_apdma_chan_write(c,
> > > + VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > > + mtk_uart_apdma_chan_write(c, VFF_EN, VFF_EN_B);
> > > + }
> > > +
> > > + if (mtkd->support_33bits)
> > > + mtk_uart_apdma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> > > +
> > > + if (mtk_uart_apdma_chan_read(c, VFF_EN) != VFF_EN_B) {
> > > + dev_err(chan->device->dev,
> > > + "config dma dir[%d] fail\n", cfg->direction);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
> > > +{
> > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > + unsigned long flags;
> > > + u32 status;
> > > + int ret;
> > > +
> > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > +
> > > + mtk_uart_apdma_chan_write(c, VFF_FLUSH, VFF_FLUSH_CLR_B);
> > > + /* Wait for flush */
> > > + ret = readx_poll_timeout(readl,
> > > + c->base + VFF_FLUSH,
> > > + status,
> > > + (status & VFF_FLUSH_B) != VFF_FLUSH_B,
> > > + 10, 100);
> > > + if (ret)
> > > + dev_err(c->vc.chan.device->dev,
> > > + "dma stop: polling FLUSH fail, DEBUG=0x%x\n",
> > > + mtk_uart_apdma_chan_read(c, VFF_DEBUG_STATUS));
> > > +
> > > + /*set stop as 1 -> wait until en is 0 -> set stop as 0*/
> > > + mtk_uart_apdma_chan_write(c, VFF_STOP, VFF_STOP_B);
> > > + ret = readx_poll_timeout(readl,
> > > + c->base + VFF_EN,
> > > + status, status == 0, 10, 100);
> > > + if (ret)
> > > + dev_err(c->vc.chan.device->dev,
> > > + "dma stop: polling VFF_EN fail, DEBUG=0x%x\n",
> > > + mtk_uart_apdma_chan_read(c, VFF_DEBUG_STATUS));
> > > +
> > > + mtk_uart_apdma_chan_write(c, VFF_STOP, VFF_STOP_CLR_B);
> > > + mtk_uart_apdma_chan_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> > > +
> > > + switch (c->cfg.direction) {
> > > + case DMA_DEV_TO_MEM:
> >
> > Ditto, if/else?
> >
> ditto.
> http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016292.html
Well, I disagree, especially in this case: We turn 4 lines of code into 10...
> > > + mtk_uart_apdma_chan_write(c,
> > > + VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > > + break;
> > > + case DMA_MEM_TO_DEV:
> > > + mtk_uart_apdma_chan_write(c,
> > > + VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > +
> > > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mtk_uart_apdma_device_pause(struct dma_chan *chan)
> > > +{
> > > + /* just for check caps pass */
> > > + return 0;
> > > +}
> > > +
> > > +static int mtk_uart_apdma_device_resume(struct dma_chan *chan)
> > > +{
> > > + /* just for check caps pass */
> > > + return 0;
> > > +}
Why do you need these? Seems wrong to advertise device_pause/resume
(and the caps) if we don't actually support that?
> > > +
> > > +static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> > > +{
> > > + while (list_empty(&mtkd->ddev.channels) == 0) {
> > > + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> > > + struct mtk_chan, vc.chan.device_node);
> > > +
> > > + list_del(&c->vc.chan.device_node);
> > > + tasklet_kill(&c->vc.task);
> > > + }
> > > +}
> > > +
> > > +static const struct of_device_id mtk_uart_apdma_match[] = {
> > > + { .compatible = "mediatek,mt6577-uart-dma", },
> > > + { /* sentinel */ },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mtk_uart_apdma_match);
> > > +
> > > +static int mtk_uart_apdma_probe(struct platform_device *pdev)
> > > +{
> > > + struct mtk_uart_apdmadev *mtkd;
> > > + struct resource *res;
> > > + struct mtk_chan *c;
> > > + unsigned int i;
> > > + int rc;
> > > +
> > > + mtkd = devm_kzalloc(&pdev->dev, sizeof(*mtkd), GFP_KERNEL);
> > > + if (!mtkd)
> > > + return -ENOMEM;
> > > +
> > > + mtkd->clk = devm_clk_get(&pdev->dev, NULL);
> > > + if (IS_ERR(mtkd->clk)) {
> > > + dev_err(&pdev->dev, "No clock specified\n");
> > > + rc = PTR_ERR(mtkd->clk);
> > > + goto err_no_dma;
> > > + }
> > > +
> > > + if (of_property_read_bool(pdev->dev.of_node, "dma-33bits")) {
> > > + dev_info(&pdev->dev, "Support dma 33bits\n");
> >
> > Drop this, a bit verbose.
> >
> > > + mtkd->support_33bits = true;
> > > + }
> > > +
> > > + rc = dma_set_mask_and_coherent(&pdev->dev,
> > > + DMA_BIT_MASK(32 | mtkd->support_33bits));
> > > + if (rc)
> > > + goto err_no_dma;
> > > +
> > > + dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
> > > + mtkd->ddev.device_alloc_chan_resources =
> > > + mtk_uart_apdma_alloc_chan_resources;
> > > + mtkd->ddev.device_free_chan_resources =
> > > + mtk_uart_apdma_free_chan_resources;
> > > + mtkd->ddev.device_tx_status = mtk_uart_apdma_tx_status;
> > > + mtkd->ddev.device_issue_pending = mtk_uart_apdma_issue_pending;
> > > + mtkd->ddev.device_prep_slave_sg = mtk_uart_apdma_prep_slave_sg;
> > > + mtkd->ddev.device_config = mtk_uart_apdma_slave_config;
> > > + mtkd->ddev.device_pause = mtk_uart_apdma_device_pause;
> > > + mtkd->ddev.device_resume = mtk_uart_apdma_device_resume;
> > > + mtkd->ddev.device_terminate_all = mtk_uart_apdma_terminate_all;
> > > + mtkd->ddev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> > > + mtkd->ddev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> > > + mtkd->ddev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> > > + mtkd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> > > + mtkd->ddev.dev = &pdev->dev;
> > > + INIT_LIST_HEAD(&mtkd->ddev.channels);
> > > +
> > > + for (i = 0; i < MTK_UART_APDMA_CHANNELS; i++) {
> > > + c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
> > > + if (!c) {
> > > + rc = -ENODEV;
> > > + goto err_no_dma;
> > > + }
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > > + if (!res) {
> > > + rc = -ENODEV;
> > > + goto err_no_dma;
> > > + }
> > > +
> > > + c->base = devm_ioremap_resource(&pdev->dev, res);
> > > + if (IS_ERR(c->base)) {
> > > + rc = PTR_ERR(c->base);
> > > + goto err_no_dma;
> > > + }
> > > + c->requested = false;
> > > + c->vc.desc_free = mtk_uart_apdma_desc_free;
> > > + vchan_init(&c->vc, &mtkd->ddev);
> > > +
> > > + mtkd->dma_irq[i] = platform_get_irq(pdev, i);
> > > + if ((int)mtkd->dma_irq[i] < 0) {
> > > + dev_err(&pdev->dev, "failed to get IRQ[%d]\n", i);
> > > + rc = -EINVAL;
> > > + goto err_no_dma;
> > > + }
> > > + }
> > > +
> > > + pm_runtime_enable(&pdev->dev);
> > > + pm_runtime_set_active(&pdev->dev);
> > > +
> > > + rc = dma_async_device_register(&mtkd->ddev);
> > > + if (rc)
> > > + goto rpm_disable;
> > > +
> > > + platform_set_drvdata(pdev, mtkd);
> > > +
> > > + if (pdev->dev.of_node) {
> > > + /* Device-tree DMA controller registration */
> > > + rc = of_dma_controller_register(pdev->dev.of_node,
> > > + of_dma_xlate_by_chan_id,
> > > + mtkd);
> > > + if (rc)
> > > + goto dma_remove;
> > > + }
> > > +
> > > + return rc;
> > > +
> > > +dma_remove:
> > > + dma_async_device_unregister(&mtkd->ddev);
> > > +rpm_disable:
> > > + pm_runtime_disable(&pdev->dev);
> > > +err_no_dma:
> > > + mtk_uart_apdma_free(mtkd);
It's not very correct to call this before
INIT_LIST_HEAD(&mtkd->ddev.channels);
I'd just call `return rc;` directly for all instances before that line.
> > > + return rc;
> > > +}
> > > +
> > > +static int mtk_uart_apdma_remove(struct platform_device *pdev)
> > > +{
> > > + struct mtk_uart_apdmadev *mtkd = platform_get_drvdata(pdev);
> > > +
> > > + if (pdev->dev.of_node)
> > > + of_dma_controller_free(pdev->dev.of_node);
> > > +
> > > + pm_runtime_disable(&pdev->dev);
> > > + pm_runtime_put_noidle(&pdev->dev);
> > > +
> > > + dma_async_device_unregister(&mtkd->ddev);
> > > + mtk_uart_apdma_free(mtkd);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int mtk_uart_apdma_suspend(struct device *dev)
> > > +{
> > > + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> > > +
> > > + if (!pm_runtime_suspended(dev))
> > > + clk_disable_unprepare(mtkd->clk);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mtk_uart_apdma_resume(struct device *dev)
> > > +{
> > > + int ret;
> > > + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> > > +
> > > + if (!pm_runtime_suspended(dev)) {
> > > + ret = clk_prepare_enable(mtkd->clk);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +#endif /* CONFIG_PM_SLEEP */
> > > +
> > > +#ifdef CONFIG_PM
> > > +static int mtk_uart_apdma_runtime_suspend(struct device *dev)
> > > +{
> > > + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> > > +
> > > + clk_disable_unprepare(mtkd->clk);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mtk_uart_apdma_runtime_resume(struct device *dev)
> > > +{
> > > + int ret;
> > > + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> > > +
> > > + ret = clk_prepare_enable(mtkd->clk);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return 0;
> > > +}
> > > +#endif /* CONFIG_PM */
> > > +
> > > +static const struct dev_pm_ops mtk_uart_apdma_pm_ops = {
> > > + SET_SYSTEM_SLEEP_PM_OPS(mtk_uart_apdma_suspend, mtk_uart_apdma_resume)
> > > + SET_RUNTIME_PM_OPS(mtk_uart_apdma_runtime_suspend,
> > > + mtk_uart_apdma_runtime_resume, NULL)
> > > +};
> > > +
> > > +static struct platform_driver mtk_uart_apdma_driver = {
> > > + .probe = mtk_uart_apdma_probe,
> > > + .remove = mtk_uart_apdma_remove,
> > > + .driver = {
> > > + .name = KBUILD_MODNAME,
> > > + .pm = &mtk_uart_apdma_pm_ops,
> > > + .of_match_table = of_match_ptr(mtk_uart_apdma_match),
> > > + },
> > > +};
> > > +
> > > +module_platform_driver(mtk_uart_apdma_driver);
> > > +
> > > +MODULE_DESCRIPTION("MediaTek UART APDMA Controller Driver");
> > > +MODULE_AUTHOR("Long Cheng <long.cheng@mediatek.com>");
> > > +MODULE_LICENSE("GPL v2");
> > > +
> > > diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
> > > index 27bac0b..1a523c87 100644
> > > --- a/drivers/dma/mediatek/Kconfig
> > > +++ b/drivers/dma/mediatek/Kconfig
> > > @@ -1,4 +1,15 @@
> > >
> > > +config DMA_MTK_UART
> > > + tristate "MediaTek SoCs APDMA support for UART"
> > > + depends on OF && SERIAL_8250_MT6577
> > > + select DMA_ENGINE
> > > + select DMA_VIRTUAL_CHANNELS
> > > + help
> > > + Support for the UART DMA engine found on MediaTek MTK SoCs.
> > > + when SERIAL_8250_MT6577 is enabled, and if you want to use DMA,
> > > + you can enable the config. the DMA engine can only be used
> > > + with MediaTek SoCs.
> > > +
> > > config MTK_HSDMA
> > > tristate "MediaTek High-Speed DMA controller support"
> > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > diff --git a/drivers/dma/mediatek/Makefile b/drivers/dma/mediatek/Makefile
> > > index 6e778f8..2f2efd9 100644
> > > --- a/drivers/dma/mediatek/Makefile
> > > +++ b/drivers/dma/mediatek/Makefile
> > > @@ -1 +1,2 @@
> > > +obj-$(CONFIG_DMA_MTK_UART) += 8250_mtk_dma.o
> > > obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
> > > --
> > > 1.7.9.5
> > >
>
>
^ permalink raw reply
* [PATCH] tty: pass return value of spi_register_driver
From: Kangjie Lu @ 2018-12-26 1:26 UTC (permalink / raw)
To: kjlu; +Cc: pakki001, Greg Kroah-Hartman, Jiri Slaby, linux-serial,
linux-kernel
spi_register_driver() may fail, so let's pass its return value upstream.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
drivers/tty/serial/max310x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 3db48fcd6068..55e73646fcd2 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1470,10 +1470,10 @@ static int __init max310x_uart_init(void)
return ret;
#ifdef CONFIG_SPI_MASTER
- spi_register_driver(&max310x_spi_driver);
+ ret = spi_register_driver(&max310x_spi_driver);
#endif
- return 0;
+ return ret;
}
module_init(max310x_uart_init);
--
2.17.2 (Apple Git-113)
^ permalink raw reply related
* Re: [PATCH 07/14] clock: milbeaut: Add Milbeaut M10V clock control
From: Sugaya, Taichi @ 2018-12-26 1:35 UTC (permalink / raw)
To: Stephen Boyd, devicetree, linux-arm-kernel, linux-clk,
linux-kernel, linux-serial
Cc: Michael Turquette, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
Daniel Lezcano, Thomas Gleixner, Russell King, Jiri Slaby,
Masami Hiramatsu, Jassi Brar
In-Reply-To: <154356669840.88331.4455990896653868594@swboyd.mtv.corp.google.com>
Hi
On 2018/11/30 17:31, Stephen Boyd wrote:
>> + init.num_parents = parents;
>> + init.parent_names = parent_names;
>> +
>> + mcm->cname = clk_name;
>> + mcm->parent = 0;
>> + mcm->hw.init = &init;
>> +
>> + clk = clk_register(NULL, &mcm->hw);
>> + if (IS_ERR(clk))
>> + goto err_clk;
>> +
>> + of_clk_add_provider(node, of_clk_src_simple_get, clk);
>> + return;
>> +
>> +err_clk:
>> + kfree(mcm);
>> +err_mcm:
>> + kfree(parent_names);
>> +}
>> +CLK_OF_DECLARE(m10v_clk_mux, "socionext,milbeaut-m10v-clk-mux",
>> + m10v_clk_mux_setup);
>
> Any chance you can use a platform driver?
>
Excuse me to re-ask you.
Why do you recommend to use a platform driver? Is that current fad?
Thanks
Sugaya Taichi
^ permalink raw reply
* Re: [PATCH v7 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
From: Long Cheng @ 2018-12-26 6:35 UTC (permalink / raw)
To: Nicolas Boichat
Cc: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang, Matthias Brugger, Dan Williams, Greg Kroah-Hartman,
Jiri Slaby, Sean Wang, dmaengine, devicetree,
linux-arm Mailing List, linux-mediatek, lkml, linux-serial,
srv_heupstream, Yingjoe Chen, YT Shen, Z
In-Reply-To: <CANMq1KDtUgOUFRy-TyQK0EmbpXGtLEVo9Mqg1Hvs0JoL9NrFyQ@mail.gmail.com>
On Wed, 2018-12-26 at 08:05 +0800, Nicolas Boichat wrote:
thanks.
> On Tue, Dec 25, 2018 at 8:06 PM Long Cheng <long.cheng@mediatek.com> wrote:
> >
> > Thanks for your comments.
> >
> > On Tue, 2018-12-25 at 15:16 +0800, Nicolas Boichat wrote:
> > > Not a full review, a few comments below.
> > >
> > > Thanks,
> > >
> >
> > would you like help to full review at this version patch? and then i
> > can modify these at next version patch. thanks.
>
> Added a few more comments ,-)
>
> > > On Tue, Dec 25, 2018 at 9:27 AM Long Cheng <long.cheng@mediatek.com> wrote:
> > > >
> > > > In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> > > > If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> > > > the performance, can enable the function.
> > > >
> > > > Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> > > > ---
> > > > drivers/dma/mediatek/8250_mtk_dma.c | 694 +++++++++++++++++++++++++++++++++++
> > > > drivers/dma/mediatek/Kconfig | 11 +
> > > > drivers/dma/mediatek/Makefile | 1 +
> > > > 3 files changed, 706 insertions(+)
> > > > create mode 100644 drivers/dma/mediatek/8250_mtk_dma.c
> > > >
> > > > diff --git a/drivers/dma/mediatek/8250_mtk_dma.c b/drivers/dma/mediatek/8250_mtk_dma.c
> > > > new file mode 100644
> > > > index 0000000..c4090f2
> > > > --- /dev/null
> > > > +++ b/drivers/dma/mediatek/8250_mtk_dma.c
> > > > @@ -0,0 +1,694 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * MediaTek 8250 DMA driver.
> > > > + *
> > > > + * Copyright (c) 2018 MediaTek Inc.
> > > > + * Author: Long Cheng <long.cheng@mediatek.com>
> > > > + */
> > > > +
> > > > +#include <linux/clk.h>
> > > > +#include <linux/dmaengine.h>
> > > > +#include <linux/dma-mapping.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/init.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/list.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of_dma.h>
> > > > +#include <linux/of_device.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/spinlock.h>
> > > > +#include <linux/pm_runtime.h>
> > > > +#include <linux/iopoll.h>
> > >
> > > Alphabetical order.
> >
> > ok, i will order.
> >
> > >
> > > > +
> > > > +#include "../virt-dma.h"
> > > > +
> > > > +#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
> > > > +
> > > > +#define VFF_EN_B BIT(0)
> > > > +#define VFF_STOP_B BIT(0)
> > > > +#define VFF_FLUSH_B BIT(0)
> > > > +#define VFF_4G_SUPPORT_B BIT(0)
> > > > +#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
> > > > +#define VFF_RX_INT_EN1_B BIT(1)
> > > > +#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
> > > > +#define VFF_WARM_RST_B BIT(0)
> > > > +#define VFF_RX_INT_FLAG_CLR_B (BIT(0) | BIT(1))
> > > > +#define VFF_TX_INT_FLAG_CLR_B 0
> > > > +#define VFF_STOP_CLR_B 0
> > > > +#define VFF_FLUSH_CLR_B 0
> > > > +#define VFF_INT_EN_CLR_B 0
> > > > +#define VFF_4G_SUPPORT_CLR_B 0
> > > > +
> > > > +/* interrupt trigger level for tx */
> > > > +#define VFF_TX_THRE(n) ((n) * 7 / 8)
> > > > +/* interrupt trigger level for rx */
> > > > +#define VFF_RX_THRE(n) ((n) * 3 / 4)
> > > > +
> > > > +#define MTK_UART_APDMA_RING_SIZE 0xffffU
> > > > +/* invert this bit when wrap ring head again*/
> > > > +#define MTK_UART_APDMA_RING_WRAP 0x10000U
> > > > +
> > > > +#define VFF_INT_FLAG 0x00
> > > > +#define VFF_INT_EN 0x04
> > > > +#define VFF_EN 0x08
> > > > +#define VFF_RST 0x0c
> > > > +#define VFF_STOP 0x10
> > > > +#define VFF_FLUSH 0x14
> > > > +#define VFF_ADDR 0x1c
> > > > +#define VFF_LEN 0x24
> > > > +#define VFF_THRE 0x28
> > > > +#define VFF_WPT 0x2c
> > > > +#define VFF_RPT 0x30
> > > > +/*TX: the buffer size HW can read. RX: the buffer size SW can read.*/
> > > > +#define VFF_VALID_SIZE 0x3c
> > > > +/*TX: the buffer size SW can write. RX: the buffer size HW can write.*/
> > > > +#define VFF_LEFT_SIZE 0x40
> > > > +#define VFF_DEBUG_STATUS 0x50
> > > > +#define VFF_4G_SUPPORT 0x54
> > > > +
> > > > +struct mtk_uart_apdmadev {
> > > > + struct dma_device ddev;
> > > > + struct clk *clk;
> > > > + bool support_33bits;
> > > > + unsigned int dma_irq[MTK_UART_APDMA_CHANNELS];
> > > > +};
> > > > +
> > > > +struct mtk_uart_apdma_desc {
> > > > + struct virt_dma_desc vd;
> > > > +
> > > > + unsigned int avail_len;
> > > > +};
> > > > +
> > > > +struct mtk_chan {
> > > > + struct virt_dma_chan vc;
> > > > + struct dma_slave_config cfg;
> > > > + void __iomem *base;
> > > > + struct mtk_uart_apdma_desc *desc;
> > > > +
> > > > + bool requested;
> > > > +
> > > > + unsigned int rx_status;
> > > > +};
> > > > +
> > > > +static inline struct mtk_uart_apdmadev *
> > > > +to_mtk_uart_apdma_dev(struct dma_device *d)
> > > > +{
> > > > + return container_of(d, struct mtk_uart_apdmadev, ddev);
> > > > +}
> > > > +
> > > > +static inline struct mtk_chan *to_mtk_uart_apdma_chan(struct dma_chan *c)
> > > > +{
> > > > + return container_of(c, struct mtk_chan, vc.chan);
> > > > +}
> > > > +
> > > > +static inline struct mtk_uart_apdma_desc *to_mtk_uart_apdma_desc
> > > > + (struct dma_async_tx_descriptor *t)
> > > > +{
> > > > + return container_of(t, struct mtk_uart_apdma_desc, vd.tx);
> > > > +}
> > > > +
> > > > +static void mtk_uart_apdma_chan_write(struct mtk_chan *c,
> > > > + unsigned int reg, unsigned int val)
> > > > +{
> > > > + writel(val, c->base + reg);
> > > > +}
> > > > +
> > > > +static unsigned int
> > > > +mtk_uart_apdma_chan_read(struct mtk_chan *c, unsigned int reg)
> > > > +{
> > > > + return readl(c->base + reg);
> > > > +}
> > > > +
> > > > +static void mtk_uart_apdma_desc_free(struct virt_dma_desc *vd)
> > > > +{
> > > > + struct dma_chan *chan = vd->tx.chan;
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > +
> > > > + kfree(c->desc);
> > > > + c->desc = NULL;
> > >
> > > Why? I'm afraid this may mask double-free.
> > >
> >
> > i will remove it.
> >
> > > > +}
> > > > +
> > > > +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> > > > +{
> > > > + unsigned int txcount = c->desc->avail_len;
> > >
> > > Variable name is confusing... I'd rather have a boolean `transmitted`
> > > (or something), set to 0 here, and set to 1 in the loop (but again,
> > > maybe this is not necessary at all, see below).
> >
> > ok, i modify the function and remove it.
> >
> > >
> > > > + unsigned int len, send, left, wpt, wrap;
> > > > +
> > > > + if (mtk_uart_apdma_chan_read(c, VFF_LEFT_SIZE) == 0U) {
> > > > + mtk_uart_apdma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > >
> > > I'd add a "return;" here and de-indent the rest of the function (that
> > > should help to make it more readable).
> >
> > add
> >
> > >
> > > > + } else {
> > > > + len = mtk_uart_apdma_chan_read(c, VFF_LEN);
> > >
> > > You make an assumption below that len is always a power of 2. I wonder
> > > if we want to validate that with some BUG/WARN call.
> > >
> > > Also, I assume VFF_LEN is constant? Maybe we can read it once only in
> > > the probe function?
> > >
> >
> > no, the value of VFF_LEN isn't constant.
> >
> > > > +
> > > > + while (((left = mtk_uart_apdma_chan_read(c,
> > > > + VFF_LEFT_SIZE)) > 0U)
> > > > + && (c->desc->avail_len != 0U)) {
> > >
> > > Why do we need a while loop here? LEFT_SIZE will always be smaller
> > > than LEN, right? Shouldn't we load as much as we can, then let an
> > > interrupt happen and reload the FIFO at that stage? Or does this help
> > > gain a bit of extra performance?
> >
> > i will remove while, use 'if' instead of.
> > >
> > > > + send = min_t(unsigned int, left, c->desc->avail_len);
> > > > + wpt = mtk_uart_apdma_chan_read(c, VFF_WPT);
> > > > + wrap = wpt & MTK_UART_APDMA_RING_WRAP ?
> > >
> > > wrap = wrp ^ MTK_UART_APDMA_RING_WRAP ?
> > >
> >
> > wrap = wpt & MTK_UART_APDMA_RING_WRAP ?
> > i had test it.
>
> Sorry, I got it wrong (and my question mark is confusing), what I mean is this:
>
> wrap = (wrp ^ MTK_UART_APDMA_RING_WRAP) & MTK_UART_APDMA_RING_WRAP;
>
> But then it's marginally shorter than your solution, so maybe keep
> yours, it's a little simpler to understand.
>
i will rename 'MTK_UART_APDMA_RING_WRAP' to 'VFF_RING_WRAP'. let code
shorter. i will keep the code.
> > > > + 0U : MTK_UART_APDMA_RING_WRAP;
> > > > +
> > > > + if ((wpt & (len - 1U)) + send < len)
>
> Is it possible for wpt > len? If not, then wpt + send < len should be enough.
>
> Also, wpt + send is used a few times now, so maybe have it a separate variable?
>
> Something like this might be nicer:
>
> next_wpt = wpt + send;
> if ((wpt & (len - 1U)) + send >= len) {
> next_wpt = next_wpt & (len - 1U);
> if (!(wpt & MTK_UART_APDMA_RING_WRAP))
> next_wpt |= MTK_UART_APDMA_RING_WRAP;
> }
> mtk_uart_apdma_chan_write(c, VFF_WPT, next_wpt);
>
> Or if the assumption than wpt < len holds:
> if (next_wpt >= len) {
> next_wpt = next_wpt - len;
> ...
>
i will modify it like your comments,
next_wpt = wpt + send;
> if ((wpt & (len - 1U)) + send >= len) {
> next_wpt = next_wpt & (len - 1U);
> if (!(wpt & MTK_UART_APDMA_RING_WRAP))
> next_wpt |= MTK_UART_APDMA_RING_WRAP;
>
i think that it's better.
> > > > + mtk_uart_apdma_chan_write(c,
> > > > + VFF_WPT, wpt + send);
> > > > + else
> > > > + mtk_uart_apdma_chan_write(c, VFF_WPT,
> > > > + ((wpt + send) & (len - 1U))
> > > > + | wrap);
> > > > +
> > > > + c->desc->avail_len -= send;
> > > > + }
> > > > +
> > > > + if (txcount != c->desc->avail_len) {
> > >
> > > We know mtk_uart_apdma_chan_read(c, VFF_LEFT_SIZE) > 0 at least once,
> > > so this test should always be true.
> > >
> > > Unless we have c->desc->avail_len == 0, but then we won't even call
> > > this function... So this is always true.
> > >
> >
> > yes. you are right. i will remove it.
> >
> > > > + mtk_uart_apdma_chan_write(c,
> > > > + VFF_INT_EN, VFF_TX_INT_EN_B);
> > > > + if (mtk_uart_apdma_chan_read(c,
> > > > + VFF_FLUSH) == 0U)
> > > > + mtk_uart_apdma_chan_write(c,
> > > > + VFF_FLUSH, VFF_FLUSH_B);
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> > > > +{
> > > > + struct mtk_uart_apdma_desc *d = c->desc;
> > > > + unsigned int rx_len, wg, rg, count;
> > > > +
> > > > + if (mtk_uart_apdma_chan_read(c, VFF_VALID_SIZE) == 0U)
> > > > + return;
> > > > +
> > > > + if (d && vchan_next_desc(&c->vc)) {
>
> I'd negate the test to de-indent the rest of the code:
> if (!d || !vchan_next_desc(&c->vc))
> return;
>
ok. i will modify it like
if ((mtk_uart_apdma_chan_read(c, VFF_VALID_SIZE) == 0U) ||
!d || !vchan_next_desc(&c->vc))
return;
> > > > + rx_len = mtk_uart_apdma_chan_read(c, VFF_LEN);
> > > > + rg = mtk_uart_apdma_chan_read(c, VFF_RPT);
> > > > + wg = mtk_uart_apdma_chan_read(c, VFF_WPT);
> > > > + count = ((rg ^ wg) & MTK_UART_APDMA_RING_WRAP) ?
> > > > + ((wg & MTK_UART_APDMA_RING_SIZE) +
> > > > + rx_len - (rg & MTK_UART_APDMA_RING_SIZE)) :
> > > > + ((wg & MTK_UART_APDMA_RING_SIZE) -
> > > > + (rg & MTK_UART_APDMA_RING_SIZE));
> > >
> > > This is hard to read, please use a if/else.
> > >
> >
> > ok.
> >
> > > > +
> > > > + c->rx_status = count;
> > > > + mtk_uart_apdma_chan_write(c, VFF_RPT, wg);
> > > > +
> > > > + list_del(&d->vd.node);
> > > > + vchan_cookie_complete(&d->vd);
> > > > + }
> > > > +}
> > > > +
> > > > +static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
> > > > +{
> > > > + struct dma_chan *chan = (struct dma_chan *)dev_id;
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + struct mtk_uart_apdma_desc *d;
> > > > + unsigned long flags;
> > > > +
> > > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > > + switch (c->cfg.direction) {
> > > > + case DMA_DEV_TO_MEM:
> > >
> > > You use an if/else below in a similar case, please be consistent (IMHO
> > > if/else is a bit more compact, so I'd use that here).
> > >
> > sean wang had review it. the link
> > http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016292.html
> >
ok, i will use if/else
> > > > + mtk_uart_apdma_chan_write(c,
> > > > + VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > > > + mtk_uart_apdma_start_rx(c);
> > > > + break;
> > > > + case DMA_MEM_TO_DEV:
> > > > + d = c->desc;
> > > > +
> > > > + mtk_uart_apdma_chan_write(c,
> > > > + VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > > > +
> > > > + if (d->avail_len != 0U) {
> > > > + mtk_uart_apdma_start_tx(c);
> > > > + } else {
> > > > + list_del(&d->vd.node);
> > > > + vchan_cookie_complete(&d->vd);
> > > > + }
> > > > + break;
> > > > + default:
> > > > + break;
> > > > + }
> > > > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > > > +
> > > > + return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> > > > +{
> > > > + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + u32 status;
> > > > + int ret = -EBUSY;
>
> No need to init ret.
>
ok.
> > > > +
> > > > + pm_runtime_get_sync(mtkd->ddev.dev);
> > > > +
> > > > + mtk_uart_apdma_chan_write(c, VFF_ADDR, 0);
> > > > + mtk_uart_apdma_chan_write(c, VFF_THRE, 0);
> > > > + mtk_uart_apdma_chan_write(c, VFF_LEN, 0);
> > > > + mtk_uart_apdma_chan_write(c, VFF_RST, VFF_WARM_RST_B);
> > > > +
> > > > + ret = readx_poll_timeout(readl,
> > > > + c->base + VFF_EN,
> > > > + status, status == 0, 10, 100);
> > > > + if (ret) {
> > > > + dev_err(c->vc.chan.device->dev,
> > > > + "dma reset: fail, timeout\n");
> > > > + goto exit;
>
> Just return ret, since you don't do any cleanup in exit:.
>
ok.
> > > > + }
> > > > +
> > > > + if (!c->requested) {
> > > > + c->requested = true;
> > > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > > + mtk_uart_apdma_irq_handler,
> > > > + IRQF_TRIGGER_NONE,
> > > > + KBUILD_MODNAME, chan);
> > > > + if (ret < 0) {
> > > > + dev_err(chan->device->dev, "Can't request dma IRQ\n");
> > > > + return -EINVAL;
> > > > + }
> > > > + }
> > > > +
> > > > + if (mtkd->support_33bits)
> > > > + mtk_uart_apdma_chan_write(c,
> > > > + VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
> > > > +
> > > > +exit:
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static void mtk_uart_apdma_free_chan_resources(struct dma_chan *chan)
> > > > +{
> > > > + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > +
> > > > + if (c->requested) {
> > > > + c->requested = false;
> > > > + free_irq(mtkd->dma_irq[chan->chan_id], chan);
> > > > + }
> > > > +
> > > > + tasklet_kill(&c->vc.task);
> > > > +
> > > > + vchan_free_chan_resources(&c->vc);
> > > > +
> > > > + pm_runtime_put_sync(mtkd->ddev.dev);
> > > > +}
> > > > +
> > > > +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> > > > + dma_cookie_t cookie,
> > > > + struct dma_tx_state *txstate)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + enum dma_status ret;
> > > > + unsigned long flags;
> > > > +
> > > > + if (!txstate)
> > > > + return DMA_ERROR;
> > > > +
> > > > + ret = dma_cookie_status(chan, cookie, txstate);
> > > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > > + if (ret == DMA_IN_PROGRESS) {
> > > > + c->rx_status = mtk_uart_apdma_chan_read(c, VFF_RPT)
> > > > + & MTK_UART_APDMA_RING_SIZE;
> > > > + dma_set_residue(txstate, c->rx_status);
> > > > + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
> > > > + dma_set_residue(txstate, c->rx_status);
> > > > + } else {
> > > > + dma_set_residue(txstate, 0);
> > > > + }
> > > > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +/*
> > > > + * dmaengine_prep_slave_single will call the function. and sglen is 1.
> > > > + * 8250 uart using one ring buffer, and deal with one sg.
> > > > + */
> > > > +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> > > > + (struct dma_chan *chan, struct scatterlist *sgl,
> > > > + unsigned int sglen, enum dma_transfer_direction dir,
>
> Too many spaces (or a tab?) between `sglen,` and `enum`
>
ok. i will modify it
> > > > + unsigned long tx_flags, void *context)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + struct mtk_uart_apdma_desc *d;
> > > > +
> > > > + if ((dir != DMA_DEV_TO_MEM) &&
> > > > + (dir != DMA_MEM_TO_DEV)) {
> > > > + dev_err(chan->device->dev, "bad direction\n");
> > > > + return NULL;
> > > > + }
> > > > +
> > > > + /* Now allocate and setup the descriptor */
> > > > + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> > > > + if (!d)
> > > > + return NULL;
> > > > +
> > > > + /* sglen is 1 */
> > > > + d->avail_len = sg_dma_len(sgl);
> > > > +
> > > > + return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
> > > > +}
> > > > +
> > > > +static void mtk_uart_apdma_issue_pending(struct dma_chan *chan)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + struct virt_dma_desc *vd;
> > > > + unsigned long flags;
> > > > +
> > > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > > + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> > > > + if (vchan_issue_pending(&c->vc) && !c->desc) {
> > > > + vd = vchan_next_desc(&c->vc);
> > > > + c->desc = to_mtk_uart_apdma_desc(&vd->tx);
> > > > + mtk_uart_apdma_start_rx(c);
> > > > + }
> > > > + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> > > > + if (vchan_issue_pending(&c->vc) && !c->desc) {
> > > > + vd = vchan_next_desc(&c->vc);
> > > > + c->desc = to_mtk_uart_apdma_desc(&vd->tx);
> > > > + mtk_uart_apdma_start_tx(c);
> > > > + }
> > > > + }
> > > > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > > > +}
> > > > +
> > > > +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> > > > + struct dma_slave_config *cfg)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + struct mtk_uart_apdmadev *mtkd =
> > > > + to_mtk_uart_apdma_dev(c->vc.chan.device);
> > > > +
> > > > + c->cfg = *cfg;
> > > > +
> > > > + if (cfg->direction == DMA_DEV_TO_MEM) {
> > > > + unsigned int rx_len = cfg->src_addr_width * 1024;
> > > > +
> > > > + mtk_uart_apdma_chan_write(c, VFF_ADDR, cfg->src_addr);
> > > > + mtk_uart_apdma_chan_write(c, VFF_LEN, rx_len);
> > > > + mtk_uart_apdma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> > > > + mtk_uart_apdma_chan_write(c,
> > > > + VFF_INT_EN, VFF_RX_INT_EN0_B
> > > > + | VFF_RX_INT_EN1_B);
> > > > + mtk_uart_apdma_chan_write(c, VFF_RPT, 0);
> > > > + mtk_uart_apdma_chan_write(c,
> > > > + VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > > > + mtk_uart_apdma_chan_write(c, VFF_EN, VFF_EN_B);
> > > > + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> > > > + unsigned int tx_len = cfg->dst_addr_width * 1024;
> > > > +
> > > > + mtk_uart_apdma_chan_write(c, VFF_ADDR, cfg->dst_addr);
> > > > + mtk_uart_apdma_chan_write(c, VFF_LEN, tx_len);
> > > > + mtk_uart_apdma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> > > > + mtk_uart_apdma_chan_write(c, VFF_WPT, 0);
> > > > + mtk_uart_apdma_chan_write(c,
> > > > + VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > > > + mtk_uart_apdma_chan_write(c, VFF_EN, VFF_EN_B);
> > > > + }
> > > > +
> > > > + if (mtkd->support_33bits)
> > > > + mtk_uart_apdma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> > > > +
> > > > + if (mtk_uart_apdma_chan_read(c, VFF_EN) != VFF_EN_B) {
> > > > + dev_err(chan->device->dev,
> > > > + "config dma dir[%d] fail\n", cfg->direction);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> > > > + unsigned long flags;
> > > > + u32 status;
> > > > + int ret;
> > > > +
> > > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > > +
> > > > + mtk_uart_apdma_chan_write(c, VFF_FLUSH, VFF_FLUSH_CLR_B);
> > > > + /* Wait for flush */
> > > > + ret = readx_poll_timeout(readl,
> > > > + c->base + VFF_FLUSH,
> > > > + status,
> > > > + (status & VFF_FLUSH_B) != VFF_FLUSH_B,
> > > > + 10, 100);
> > > > + if (ret)
> > > > + dev_err(c->vc.chan.device->dev,
> > > > + "dma stop: polling FLUSH fail, DEBUG=0x%x\n",
> > > > + mtk_uart_apdma_chan_read(c, VFF_DEBUG_STATUS));
> > > > +
> > > > + /*set stop as 1 -> wait until en is 0 -> set stop as 0*/
> > > > + mtk_uart_apdma_chan_write(c, VFF_STOP, VFF_STOP_B);
> > > > + ret = readx_poll_timeout(readl,
> > > > + c->base + VFF_EN,
> > > > + status, status == 0, 10, 100);
> > > > + if (ret)
> > > > + dev_err(c->vc.chan.device->dev,
> > > > + "dma stop: polling VFF_EN fail, DEBUG=0x%x\n",
> > > > + mtk_uart_apdma_chan_read(c, VFF_DEBUG_STATUS));
> > > > +
> > > > + mtk_uart_apdma_chan_write(c, VFF_STOP, VFF_STOP_CLR_B);
> > > > + mtk_uart_apdma_chan_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> > > > +
> > > > + switch (c->cfg.direction) {
> > > > + case DMA_DEV_TO_MEM:
> > >
> > > Ditto, if/else?
> > >
> > ditto.
> > http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016292.html
>
> Well, I disagree, especially in this case: We turn 4 lines of code into 10...
>
i see, i will using if/else
> > > > + mtk_uart_apdma_chan_write(c,
> > > > + VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > > > + break;
> > > > + case DMA_MEM_TO_DEV:
> > > > + mtk_uart_apdma_chan_write(c,
> > > > + VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > > > + break;
> > > > + default:
> > > > + break;
> > > > + }
> > > > +
> > > > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int mtk_uart_apdma_device_pause(struct dma_chan *chan)
> > > > +{
> > > > + /* just for check caps pass */
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int mtk_uart_apdma_device_resume(struct dma_chan *chan)
> > > > +{
> > > > + /* just for check caps pass */
> > > > + return 0;
> > > > +}
>
> Why do you need these? Seems wrong to advertise device_pause/resume
> (and the caps) if we don't actually support that?
>
in serial 8250_dma.c file, when request DMA, will calll
'dma_get_slave_caps' to get the caps. and the will check caps.cmd_pause.
the pause is device_pause. our device can't support the functions. but
to check pass, need add the fake function. or you can give better
comments. thanks.
> > > > +
> > > > +static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> > > > +{
> > > > + while (list_empty(&mtkd->ddev.channels) == 0) {
> > > > + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> > > > + struct mtk_chan, vc.chan.device_node);
> > > > +
> > > > + list_del(&c->vc.chan.device_node);
> > > > + tasklet_kill(&c->vc.task);
> > > > + }
> > > > +}
> > > > +
> > > > +static const struct of_device_id mtk_uart_apdma_match[] = {
> > > > + { .compatible = "mediatek,mt6577-uart-dma", },
> > > > + { /* sentinel */ },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, mtk_uart_apdma_match);
> > > > +
> > > > +static int mtk_uart_apdma_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct mtk_uart_apdmadev *mtkd;
> > > > + struct resource *res;
> > > > + struct mtk_chan *c;
> > > > + unsigned int i;
> > > > + int rc;
> > > > +
> > > > + mtkd = devm_kzalloc(&pdev->dev, sizeof(*mtkd), GFP_KERNEL);
> > > > + if (!mtkd)
> > > > + return -ENOMEM;
> > > > +
> > > > + mtkd->clk = devm_clk_get(&pdev->dev, NULL);
> > > > + if (IS_ERR(mtkd->clk)) {
> > > > + dev_err(&pdev->dev, "No clock specified\n");
> > > > + rc = PTR_ERR(mtkd->clk);
> > > > + goto err_no_dma;
> > > > + }
> > > > +
> > > > + if (of_property_read_bool(pdev->dev.of_node, "dma-33bits")) {
> > > > + dev_info(&pdev->dev, "Support dma 33bits\n");
> > >
> > > Drop this, a bit verbose.
> > >
> > > > + mtkd->support_33bits = true;
> > > > + }
> > > > +
> > > > + rc = dma_set_mask_and_coherent(&pdev->dev,
> > > > + DMA_BIT_MASK(32 | mtkd->support_33bits));
> > > > + if (rc)
> > > > + goto err_no_dma;
> > > > +
> > > > + dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
> > > > + mtkd->ddev.device_alloc_chan_resources =
> > > > + mtk_uart_apdma_alloc_chan_resources;
> > > > + mtkd->ddev.device_free_chan_resources =
> > > > + mtk_uart_apdma_free_chan_resources;
> > > > + mtkd->ddev.device_tx_status = mtk_uart_apdma_tx_status;
> > > > + mtkd->ddev.device_issue_pending = mtk_uart_apdma_issue_pending;
> > > > + mtkd->ddev.device_prep_slave_sg = mtk_uart_apdma_prep_slave_sg;
> > > > + mtkd->ddev.device_config = mtk_uart_apdma_slave_config;
> > > > + mtkd->ddev.device_pause = mtk_uart_apdma_device_pause;
> > > > + mtkd->ddev.device_resume = mtk_uart_apdma_device_resume;
> > > > + mtkd->ddev.device_terminate_all = mtk_uart_apdma_terminate_all;
> > > > + mtkd->ddev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> > > > + mtkd->ddev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> > > > + mtkd->ddev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> > > > + mtkd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> > > > + mtkd->ddev.dev = &pdev->dev;
> > > > + INIT_LIST_HEAD(&mtkd->ddev.channels);
> > > > +
> > > > + for (i = 0; i < MTK_UART_APDMA_CHANNELS; i++) {
> > > > + c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
> > > > + if (!c) {
> > > > + rc = -ENODEV;
> > > > + goto err_no_dma;
> > > > + }
> > > > +
> > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > > > + if (!res) {
> > > > + rc = -ENODEV;
> > > > + goto err_no_dma;
> > > > + }
> > > > +
> > > > + c->base = devm_ioremap_resource(&pdev->dev, res);
> > > > + if (IS_ERR(c->base)) {
> > > > + rc = PTR_ERR(c->base);
> > > > + goto err_no_dma;
> > > > + }
> > > > + c->requested = false;
> > > > + c->vc.desc_free = mtk_uart_apdma_desc_free;
> > > > + vchan_init(&c->vc, &mtkd->ddev);
> > > > +
> > > > + mtkd->dma_irq[i] = platform_get_irq(pdev, i);
> > > > + if ((int)mtkd->dma_irq[i] < 0) {
> > > > + dev_err(&pdev->dev, "failed to get IRQ[%d]\n", i);
> > > > + rc = -EINVAL;
> > > > + goto err_no_dma;
> > > > + }
> > > > + }
> > > > +
> > > > + pm_runtime_enable(&pdev->dev);
> > > > + pm_runtime_set_active(&pdev->dev);
> > > > +
> > > > + rc = dma_async_device_register(&mtkd->ddev);
> > > > + if (rc)
> > > > + goto rpm_disable;
> > > > +
> > > > + platform_set_drvdata(pdev, mtkd);
> > > > +
> > > > + if (pdev->dev.of_node) {
> > > > + /* Device-tree DMA controller registration */
> > > > + rc = of_dma_controller_register(pdev->dev.of_node,
> > > > + of_dma_xlate_by_chan_id,
> > > > + mtkd);
> > > > + if (rc)
> > > > + goto dma_remove;
> > > > + }
> > > > +
> > > > + return rc;
> > > > +
> > > > +dma_remove:
> > > > + dma_async_device_unregister(&mtkd->ddev);
> > > > +rpm_disable:
> > > > + pm_runtime_disable(&pdev->dev);
> > > > +err_no_dma:
> > > > + mtk_uart_apdma_free(mtkd);
>
> It's not very correct to call this before
> INIT_LIST_HEAD(&mtkd->ddev.channels);
>
> I'd just call `return rc;` directly for all instances before that line.
>
ok, i will modify it.
> > > > + return rc;
> > > > +}
> > > > +
> > > > +static int mtk_uart_apdma_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct mtk_uart_apdmadev *mtkd = platform_get_drvdata(pdev);
> > > > +
> > > > + if (pdev->dev.of_node)
> > > > + of_dma_controller_free(pdev->dev.of_node);
> > > > +
> > > > + pm_runtime_disable(&pdev->dev);
> > > > + pm_runtime_put_noidle(&pdev->dev);
> > > > +
> > > > + dma_async_device_unregister(&mtkd->ddev);
> > > > + mtk_uart_apdma_free(mtkd);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +#ifdef CONFIG_PM_SLEEP
> > > > +static int mtk_uart_apdma_suspend(struct device *dev)
> > > > +{
> > > > + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> > > > +
> > > > + if (!pm_runtime_suspended(dev))
> > > > + clk_disable_unprepare(mtkd->clk);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int mtk_uart_apdma_resume(struct device *dev)
> > > > +{
> > > > + int ret;
> > > > + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> > > > +
> > > > + if (!pm_runtime_suspended(dev)) {
> > > > + ret = clk_prepare_enable(mtkd->clk);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +#endif /* CONFIG_PM_SLEEP */
> > > > +
> > > > +#ifdef CONFIG_PM
> > > > +static int mtk_uart_apdma_runtime_suspend(struct device *dev)
> > > > +{
> > > > + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> > > > +
> > > > + clk_disable_unprepare(mtkd->clk);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int mtk_uart_apdma_runtime_resume(struct device *dev)
> > > > +{
> > > > + int ret;
> > > > + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> > > > +
> > > > + ret = clk_prepare_enable(mtkd->clk);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +#endif /* CONFIG_PM */
> > > > +
> > > > +static const struct dev_pm_ops mtk_uart_apdma_pm_ops = {
> > > > + SET_SYSTEM_SLEEP_PM_OPS(mtk_uart_apdma_suspend, mtk_uart_apdma_resume)
> > > > + SET_RUNTIME_PM_OPS(mtk_uart_apdma_runtime_suspend,
> > > > + mtk_uart_apdma_runtime_resume, NULL)
> > > > +};
> > > > +
> > > > +static struct platform_driver mtk_uart_apdma_driver = {
> > > > + .probe = mtk_uart_apdma_probe,
> > > > + .remove = mtk_uart_apdma_remove,
> > > > + .driver = {
> > > > + .name = KBUILD_MODNAME,
> > > > + .pm = &mtk_uart_apdma_pm_ops,
> > > > + .of_match_table = of_match_ptr(mtk_uart_apdma_match),
> > > > + },
> > > > +};
> > > > +
> > > > +module_platform_driver(mtk_uart_apdma_driver);
> > > > +
> > > > +MODULE_DESCRIPTION("MediaTek UART APDMA Controller Driver");
> > > > +MODULE_AUTHOR("Long Cheng <long.cheng@mediatek.com>");
> > > > +MODULE_LICENSE("GPL v2");
> > > > +
> > > > diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
> > > > index 27bac0b..1a523c87 100644
> > > > --- a/drivers/dma/mediatek/Kconfig
> > > > +++ b/drivers/dma/mediatek/Kconfig
> > > > @@ -1,4 +1,15 @@
> > > >
> > > > +config DMA_MTK_UART
> > > > + tristate "MediaTek SoCs APDMA support for UART"
> > > > + depends on OF && SERIAL_8250_MT6577
> > > > + select DMA_ENGINE
> > > > + select DMA_VIRTUAL_CHANNELS
> > > > + help
> > > > + Support for the UART DMA engine found on MediaTek MTK SoCs.
> > > > + when SERIAL_8250_MT6577 is enabled, and if you want to use DMA,
> > > > + you can enable the config. the DMA engine can only be used
> > > > + with MediaTek SoCs.
> > > > +
> > > > config MTK_HSDMA
> > > > tristate "MediaTek High-Speed DMA controller support"
> > > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > > diff --git a/drivers/dma/mediatek/Makefile b/drivers/dma/mediatek/Makefile
> > > > index 6e778f8..2f2efd9 100644
> > > > --- a/drivers/dma/mediatek/Makefile
> > > > +++ b/drivers/dma/mediatek/Makefile
> > > > @@ -1 +1,2 @@
> > > > +obj-$(CONFIG_DMA_MTK_UART) += 8250_mtk_dma.o
> > > > obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
> > > > --
> > > > 1.7.9.5
> > > >
> >
> >
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox