* Re: [PATCH v4 11/15] clocksource: Add clock driver for RDA8810PL SoC
From: Manivannan Sadhasivam @ 2018-12-15 5:58 UTC (permalink / raw)
To: Daniel Lezcano
Cc: olof, arnd, robh+dt, tglx, jason, marc.zyngier, gregkh, jslaby,
linux-unisoc, afaerber, linux-arm-kernel, linux-kernel,
devicetree, linux-serial, amit.kucheria, linus.walleij,
zhao_steven
In-Reply-To: <affe5ffd-66de-526a-f568-52ad5f0c0083@linaro.org>
On Wed, Dec 12, 2018 at 04:52:58PM +0100, Daniel Lezcano wrote:
> On 12/12/2018 16:47, Manivannan Sadhasivam wrote:
> > Hi Daniel,
> >
> > On Wed, Dec 12, 2018 at 04:07:53PM +0100, Daniel Lezcano wrote:
> >> On 10/12/2018 18:35, Manivannan Sadhasivam wrote:
> >>> Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER
> >>> and HWTIMER.
> >>>
> >>> RDA8810PL has two independent timers: OSTIMER (56 bit) and HWTIMER
> >>> (64 bit). Each timer provides optional interrupt support. In this
> >>> driver, OSTIMER is used for clockevents and HWTIMER is used for
> >>> clocksource.
> >>>
> >>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >>
> >> The driver looks good to me. Do you want me to pick it up via my tree?
> >>
> >
> > Yes, please do. Marc is going to pick up the irqchip driver but I'm not
> > sure about the serial driver. The rest of the patches can be picked up
> > by the ARM maintainers (I need to send another version for dropping
> > Andreas from MAINTAINERS).
>
> Ok, applied.
>
Just to be sure before spinning next version, have you also picked up the
bindings patch? I can't find the commit(s) in your tree!
https://git.linaro.org/people/daniel.lezcano/linux.git/
Thanks,
Mani
> Thanks
>
> -- Daniel
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>
^ permalink raw reply
* [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Paul Burton @ 2018-12-16 20:10 UTC (permalink / raw)
To: linux-serial@vger.kernel.org, Greg Kroah-Hartman
Cc: linux-kernel@vger.kernel.org, Paul Burton, Daniel Jedrychowski,
Marek Vasut, linux-mips@vger.kernel.org, stable
In-Reply-To: <20181213174834.kxdy6fphaeoivqgh@pburton-laptop>
Commit f6aa5beb45be ("serial: 8250: Fix clearing FIFOs in RS485 mode
again") makes a change to FIFO clearing code which its commit message
suggests was intended to be specific to use with RS485 mode, however:
1) The change made does not just affect __do_stop_tx_rs485(), it also
affects other uses of serial8250_clear_fifos() including paths for
starting up, shutting down or auto-configuring a port regardless of
whether it's an RS485 port or not.
2) It makes the assumption that resetting the FIFOs is a no-op when
FIFOs are disabled, and as such it checks for this case & explicitly
avoids setting the FIFO reset bits when the FIFO enable bit is
clear. A reading of the PC16550D manual would suggest that this is
OK since the FIFO should automatically be reset if it is later
enabled, but we support many 16550-compatible devices and have never
required this auto-reset behaviour for at least the whole git era.
Starting to rely on it now seems risky, offers no benefit, and
indeed breaks at least the Ingenic JZ4780's UARTs which reads
garbage when the RX FIFO is enabled if we don't explicitly reset it.
3) By only resetting the FIFOs if they're enabled, the behaviour of
serial8250_do_startup() during boot now depends on what the value of
FCR is before the 8250 driver is probed. This in itself seems
questionable and leaves us with FCR=0 & no FIFO reset if the UART
was used by 8250_early, otherwise it depends upon what the
bootloader left behind.
4) Although the naming of serial8250_clear_fifos() may be unclear, it
is clear that callers of it expect that it will disable FIFOs. Both
serial8250_do_startup() & serial8250_do_shutdown() contain comments
to that effect, and other callers explicitly re-enable the FIFOs
after calling serial8250_clear_fifos(). The premise of that patch
that disabling the FIFOs is incorrect therefore seems wrong.
For these reasons, this reverts commit f6aa5beb45be ("serial: 8250: Fix
clearing FIFOs in RS485 mode again").
Signed-off-by: Paul Burton <paul.burton@mips.com>
Fixes: f6aa5beb45be ("serial: 8250: Fix clearing FIFOs in RS485 mode again").
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Daniel Jedrychowski <avistel@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: linux-mips@vger.kernel.org
Cc: linux-serial@vger.kernel.org
Cc: stable <stable@vger.kernel.org> # 4.10+
---
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.
---
drivers/tty/serial/8250/8250_port.c | 29 +++++------------------------
1 file changed, 5 insertions(+), 24 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index f776b3eafb96..3f779d25ec0c 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -552,30 +552,11 @@ static unsigned int serial_icr_read(struct uart_8250_port *up, int offset)
*/
static void serial8250_clear_fifos(struct uart_8250_port *p)
{
- unsigned char fcr;
- unsigned char clr_mask = UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT;
-
if (p->capabilities & UART_CAP_FIFO) {
- /*
- * Make sure to avoid changing FCR[7:3] and ENABLE_FIFO bits.
- * In case ENABLE_FIFO is not set, there is nothing to flush
- * so just return. Furthermore, on certain implementations of
- * the 8250 core, the FCR[7:3] bits may only be changed under
- * specific conditions and changing them if those conditions
- * are not met can have nasty side effects. One such core is
- * the 8250-omap present in TI AM335x.
- */
- fcr = serial_in(p, UART_FCR);
-
- /* FIFO is not enabled, there's nothing to clear. */
- if (!(fcr & UART_FCR_ENABLE_FIFO))
- return;
-
- fcr |= clr_mask;
- serial_out(p, UART_FCR, fcr);
-
- fcr &= ~clr_mask;
- serial_out(p, UART_FCR, fcr);
+ serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO);
+ serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO |
+ UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
+ serial_out(p, UART_FCR, 0);
}
}
@@ -1467,7 +1448,7 @@ static void __do_stop_tx_rs485(struct uart_8250_port *p)
* Enable previously disabled RX interrupts.
*/
if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
- serial8250_clear_fifos(p);
+ serial8250_clear_and_reinit_fifos(p);
p->ier |= UART_IER_RLSI | UART_IER_RDI;
serial_port_out(&p->port, UART_IER, p->ier);
--
2.20.0
^ permalink raw reply related
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Marek Vasut @ 2018-12-16 20:32 UTC (permalink / raw)
To: Paul Burton, linux-serial@vger.kernel.org, Greg Kroah-Hartman
Cc: linux-kernel@vger.kernel.org, Paul Burton, Daniel Jedrychowski,
linux-mips@vger.kernel.org, stable
In-Reply-To: <20181216200833.27928-1-paul.burton@mips.com>
On 12/16/2018 09:10 PM, Paul Burton wrote:
> Commit f6aa5beb45be ("serial: 8250: Fix clearing FIFOs in RS485 mode
> again") makes a change to FIFO clearing code which its commit message
> suggests was intended to be specific to use with RS485 mode, however:
>
> 1) The change made does not just affect __do_stop_tx_rs485(), it also
> affects other uses of serial8250_clear_fifos() including paths for
> starting up, shutting down or auto-configuring a port regardless of
> whether it's an RS485 port or not.
>
> 2) It makes the assumption that resetting the FIFOs is a no-op when
> FIFOs are disabled, and as such it checks for this case & explicitly
> avoids setting the FIFO reset bits when the FIFO enable bit is
> clear. A reading of the PC16550D manual would suggest that this is
> OK since the FIFO should automatically be reset if it is later
> enabled, but we support many 16550-compatible devices and have never
> required this auto-reset behaviour for at least the whole git era.
> Starting to rely on it now seems risky, offers no benefit, and
> indeed breaks at least the Ingenic JZ4780's UARTs which reads
> garbage when the RX FIFO is enabled if we don't explicitly reset it.
>
> 3) By only resetting the FIFOs if they're enabled, the behaviour of
> serial8250_do_startup() during boot now depends on what the value of
> FCR is before the 8250 driver is probed. This in itself seems
> questionable and leaves us with FCR=0 & no FIFO reset if the UART
> was used by 8250_early, otherwise it depends upon what the
> bootloader left behind.
>
> 4) Although the naming of serial8250_clear_fifos() may be unclear, it
> is clear that callers of it expect that it will disable FIFOs. Both
> serial8250_do_startup() & serial8250_do_shutdown() contain comments
> to that effect, and other callers explicitly re-enable the FIFOs
> after calling serial8250_clear_fifos(). The premise of that patch
> that disabling the FIFOs is incorrect therefore seems wrong.
>
> For these reasons, this reverts commit f6aa5beb45be ("serial: 8250: Fix
> clearing FIFOs in RS485 mode again").
>
> Signed-off-by: Paul Burton <paul.burton@mips.com>
> Fixes: f6aa5beb45be ("serial: 8250: Fix clearing FIFOs in RS485 mode again").
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Daniel Jedrychowski <avistel@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: linux-mips@vger.kernel.org
> Cc: linux-serial@vger.kernel.org
> Cc: stable <stable@vger.kernel.org> # 4.10+
I am unable to test it on such a short notice as I'm currently ill, so I
cannot tell if your change breaks the OMAP3/AM335x boards or not. Given
that there are very few CI20 boards in use, I'd like to ask you for some
extra time to investigate this on the OMAP3 too.
btw what strikes me as curious is that this patch emerged shortly after
Ezequiel re-posted the CI20 U-Boot patches after an year-long hiatus, is
it somehow related ?
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Marek Vasut @ 2018-12-16 21:08 UTC (permalink / raw)
To: Paul Burton, linux-serial@vger.kernel.org, Greg Kroah-Hartman
Cc: linux-kernel@vger.kernel.org, Paul Burton, Daniel Jedrychowski,
linux-mips@vger.kernel.org, stable
In-Reply-To: <20181216200833.27928-1-paul.burton@mips.com>
On 12/16/2018 09:10 PM, Paul Burton wrote:
> Commit f6aa5beb45be ("serial: 8250: Fix clearing FIFOs in RS485 mode
> again") makes a change to FIFO clearing code which its commit message
> suggests was intended to be specific to use with RS485 mode, however:
>
> 1) The change made does not just affect __do_stop_tx_rs485(), it also
> affects other uses of serial8250_clear_fifos() including paths for
> starting up, shutting down or auto-configuring a port regardless of
> whether it's an RS485 port or not.
>
> 2) It makes the assumption that resetting the FIFOs is a no-op when
> FIFOs are disabled, and as such it checks for this case & explicitly
> avoids setting the FIFO reset bits when the FIFO enable bit is
> clear. A reading of the PC16550D manual would suggest that this is
> OK since the FIFO should automatically be reset if it is later
> enabled, but we support many 16550-compatible devices and have never
> required this auto-reset behaviour for at least the whole git era.
> Starting to rely on it now seems risky, offers no benefit, and
> indeed breaks at least the Ingenic JZ4780's UARTs which reads
> garbage when the RX FIFO is enabled if we don't explicitly reset it.
>
> 3) By only resetting the FIFOs if they're enabled, the behaviour of
> serial8250_do_startup() during boot now depends on what the value of
> FCR is before the 8250 driver is probed. This in itself seems
> questionable and leaves us with FCR=0 & no FIFO reset if the UART
> was used by 8250_early, otherwise it depends upon what the
> bootloader left behind.
>
> 4) Although the naming of serial8250_clear_fifos() may be unclear, it
> is clear that callers of it expect that it will disable FIFOs. Both
> serial8250_do_startup() & serial8250_do_shutdown() contain comments
> to that effect, and other callers explicitly re-enable the FIFOs
> after calling serial8250_clear_fifos(). The premise of that patch
> that disabling the FIFOs is incorrect therefore seems wrong.
>
> For these reasons, this reverts commit f6aa5beb45be ("serial: 8250: Fix
> clearing FIFOs in RS485 mode again").
>
> Signed-off-by: Paul Burton <paul.burton@mips.com>
> Fixes: f6aa5beb45be ("serial: 8250: Fix clearing FIFOs in RS485 mode again").
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Daniel Jedrychowski <avistel@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: linux-mips@vger.kernel.org
> Cc: linux-serial@vger.kernel.org
> Cc: stable <stable@vger.kernel.org> # 4.10+
> ---
> 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.
> ---
> drivers/tty/serial/8250/8250_port.c | 29 +++++------------------------
> 1 file changed, 5 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index f776b3eafb96..3f779d25ec0c 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -552,30 +552,11 @@ static unsigned int serial_icr_read(struct uart_8250_port *up, int offset)
> */
> static void serial8250_clear_fifos(struct uart_8250_port *p)
> {
> - unsigned char fcr;
> - unsigned char clr_mask = UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT;
> -
> if (p->capabilities & UART_CAP_FIFO) {
> - /*
> - * Make sure to avoid changing FCR[7:3] and ENABLE_FIFO bits.
> - * In case ENABLE_FIFO is not set, there is nothing to flush
> - * so just return. Furthermore, on certain implementations of
> - * the 8250 core, the FCR[7:3] bits may only be changed under
> - * specific conditions and changing them if those conditions
> - * are not met can have nasty side effects. One such core is
> - * the 8250-omap present in TI AM335x.
> - */
> - fcr = serial_in(p, UART_FCR);
> -
> - /* FIFO is not enabled, there's nothing to clear. */
> - if (!(fcr & UART_FCR_ENABLE_FIFO))
> - return;
> -
> - fcr |= clr_mask;
> - serial_out(p, UART_FCR, fcr);
> -
> - fcr &= ~clr_mask;
> - serial_out(p, UART_FCR, fcr);
> + serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO);
> + serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO |
> + UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
> + serial_out(p, UART_FCR, 0);
> }
> }
>
> @@ -1467,7 +1448,7 @@ static void __do_stop_tx_rs485(struct uart_8250_port *p)
> * Enable previously disabled RX interrupts.
> */
> if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> - serial8250_clear_fifos(p);
> + serial8250_clear_and_reinit_fifos(p);
>
> p->ier |= UART_IER_RLSI | UART_IER_RDI;
> serial_port_out(&p->port, UART_IER, p->ier);
>
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Paul Burton @ 2018-12-16 21:31 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
In-Reply-To: <f5a76d73-862f-3ebc-cd07-effc5c432103@denx.de>
Hi Marek,
On Sun, Dec 16, 2018 at 09:32:19PM +0100, Marek Vasut wrote:
> I am unable to test it on such a short notice as I'm currently ill, so I
> cannot tell if your change breaks the OMAP3/AM335x boards or not. Given
> that there are very few CI20 boards in use, I'd like to ask you for some
> extra time to investigate this on the OMAP3 too.
I'm sorry to hear that you're ill, but your patch is getting awfully
close to becoming part of a stable kernel release & it causes
regressions. Even if it didn't break a board I use, I think the patch
would be broken & risky for the reasons I outlined in my revert's commit
message.
Ultimately it's Greg's decision but it sounds like you're asking me to
say it's OK to break the JZ4780 in a stable kernel with a patch that I
think would be risky anyway, and I won't do that.
> btw what strikes me as curious is that this patch emerged shortly after
> Ezequiel re-posted the CI20 U-Boot patches after an year-long hiatus, is
> it somehow related ?
Not at all - I regularly test on Ci20 & found this breakage whilst
testing Paul Cercueil's Ingenic TCU patchset v8 [1]. Using a Ci20 with
mainline kernels doesn't rely on Ezequiel's U-Boot work, and indeed I
generally boot using my ci20-usb-boot tool [2] so U-Boot isn't involved
at all.
Now my Ci20 testing isn't automated so it tends to happen mostly when
there are obvious changes to the board or SoC support, but it should
become more automated soon. Kevin Hilman just got a Ci40 working with
kernelci.org infrastructure & I hope we can get Ci20 & other boards
included soon too, either via existing kernelci.org labs or by setting
up a new one.
Thanks,
Paul
[1] https://lore.kernel.org/linux-mips/20181212220922.18759-1-paul@crapouillou.net/T/#t
[2] https://github.com/paulburton/ci20-tools
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Paul Burton @ 2018-12-16 21:39 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
In-Reply-To: <93a3c76b-f4ba-57ae-9d80-6e945b4f3181@denx.de>
Hi Marek,
On Sun, Dec 16, 2018 at 10:08:48PM +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.
Either way, commit f6aa5beb45be ("serial: 8250: Fix clearing FIFOs in
RS485 mode again") breaks systems that worked before it so at this late
stage in the 4.20 cycle a revert would make sense to me. If that breaks
RS85 on OMAP3 then my question would be how much can anyone really care
if nobody noticed since v4.10? And why should that lead to you breaking
the JZ4780 which has been discovered before a stable kernel release
includes the breakage?
Thanks,
Paul
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Marek Vasut @ 2018-12-16 21:45 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: <20181216213133.kwe24pif3v4wcgwp@pburton-laptop>
On 12/16/2018 10:31 PM, Paul Burton wrote:
> Hi Marek,
Hi,
> On Sun, Dec 16, 2018 at 09:32:19PM +0100, Marek Vasut wrote:
>> I am unable to test it on such a short notice as I'm currently ill, so I
>> cannot tell if your change breaks the OMAP3/AM335x boards or not. Given
>> that there are very few CI20 boards in use, I'd like to ask you for some
>> extra time to investigate this on the OMAP3 too.
>
> I'm sorry to hear that you're ill, but your patch is getting awfully
> close to becoming part of a stable kernel release & it causes
> regressions. Even if it didn't break a board I use, I think the patch
> would be broken & risky for the reasons I outlined in my revert's commit
> message.
That's what the incremental releases are for, so that minor problems can
get fixed there. Sure, it's great to have things perfect in the first
release, but if that breaks other systems, too bad.
> Ultimately it's Greg's decision but it sounds like you're asking me to
> say it's OK to break the JZ4780 in a stable kernel with a patch that I
> think would be risky anyway, and I won't do that.
I am saying this revert breaks AM335x, so this is a stalemate. I had a
discussion with Ezequiel (on CC) and he seems to have a different
smaller patch coming for this problem.
[...]
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Ezequiel Garcia @ 2018-12-16 21:52 UTC (permalink / raw)
To: Marek Vasut
Cc: Paul Burton, 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: <949fdd3d-535e-d235-f406-d5bde4658c5e@denx.de>
On Sun, 16 Dec 2018 at 18:45, Marek Vasut <marex@denx.de> wrote:
[skips discussion]
>
> > Ultimately it's Greg's decision but it sounds like you're asking me to
> > say it's OK to break the JZ4780 in a stable kernel with a patch that I
> > think would be risky anyway, and I won't do that.
>
> I am saying this revert breaks AM335x, so this is a stalemate. I had a
> discussion with Ezequiel (on CC) and he seems to have a different
> smaller patch coming for this problem.
>
Can you guys test this? Note that serial8250_do_startup has a comment
stating clearly that it has the intention of disabling the FIFOs,
so it seems this is the right thing to do.
Paul, this removes the garbage on my CI20 (rev.1)
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index c39482b96111..fac19cbc51d1 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2209,10 +2209,11 @@ int serial8250_do_startup(struct uart_port *port)
/*
* Clear the FIFO buffers and disable them.
* (they will be reenabled in set_termios())
*/
serial8250_clear_fifos(up);
+ serial_out(up, UART_FCR, 0);
/*
* Clear the interrupt registers.
*/
serial_port_in(port, UART_LSR);
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply related
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Marek Vasut @ 2018-12-16 22:01 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
In-Reply-To: <20181216213901.hpll7wqzhqvfgkfy@pburton-laptop>
On 12/16/2018 10:39 PM, Paul Burton wrote:
> Hi Marek,
Hi,
> On Sun, Dec 16, 2018 at 10:08:48PM +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.
> Either way, commit f6aa5beb45be ("serial: 8250: Fix clearing FIFOs in
> RS485 mode again") breaks systems that worked before it so at this late
> stage in the 4.20 cycle a revert would make sense to me. If that breaks
> RS85 on OMAP3 then my question would be how much can anyone really care
> if nobody noticed since v4.10? And why should that lead to you breaking
> the JZ4780 which has been discovered before a stable kernel release
> includes the breakage?
There's always a .y release where this can be properly investigated and
solved, instead of breaking one platform or the other.
Then again, see the patch from Ezequiel that was just posted, I think it
might be a far better solution.
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Marek Vasut @ 2018-12-16 22:11 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Paul Burton, 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: <CAAEAJfAad75bHX39ETCdVv9vP0dF7PLz2vvFLLqgtyikPHqJyA@mail.gmail.com>
On 12/16/2018 10:52 PM, Ezequiel Garcia wrote:
> On Sun, 16 Dec 2018 at 18:45, Marek Vasut <marex@denx.de> wrote:
> [skips discussion]
>>
>>> Ultimately it's Greg's decision but it sounds like you're asking me to
>>> say it's OK to break the JZ4780 in a stable kernel with a patch that I
>>> think would be risky anyway, and I won't do that.
>>
>> I am saying this revert breaks AM335x, so this is a stalemate. I had a
>> discussion with Ezequiel (on CC) and he seems to have a different
>> smaller patch coming for this problem.
>>
>
> Can you guys test this? Note that serial8250_do_startup has a comment
> stating clearly that it has the intention of disabling the FIFOs,
> so it seems this is the right thing to do.
>
> Paul, this removes the garbage on my CI20 (rev.1)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c
> b/drivers/tty/serial/8250/8250_port.c
> index c39482b96111..fac19cbc51d1 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2209,10 +2209,11 @@ int serial8250_do_startup(struct uart_port *port)
> /*
> * Clear the FIFO buffers and disable them.
> * (they will be reenabled in set_termios())
> */
> serial8250_clear_fifos(up);
> + serial_out(up, UART_FCR, 0);
>
> /*
> * Clear the interrupt registers.
> */
> serial_port_in(port, UART_LSR);
>
>
On AM335x pocketbeagle
Tested-by: Marek Vasut <marex@denx.de>
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Paul Burton @ 2018-12-16 22:16 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: <949fdd3d-535e-d235-f406-d5bde4658c5e@denx.de>
On Sun, Dec 16, 2018 at 10:45:23PM +0100, Marek Vasut wrote:
> >> I am unable to test it on such a short notice as I'm currently ill, so I
> >> cannot tell if your change breaks the OMAP3/AM335x boards or not. Given
> >> that there are very few CI20 boards in use, I'd like to ask you for some
> >> extra time to investigate this on the OMAP3 too.
> >
> > I'm sorry to hear that you're ill, but your patch is getting awfully
> > close to becoming part of a stable kernel release & it causes
> > regressions. Even if it didn't break a board I use, I think the patch
> > would be broken & risky for the reasons I outlined in my revert's commit
> > message.
>
> That's what the incremental releases are for, so that minor problems can
> get fixed there. Sure, it's great to have things perfect in the first
> release, but if that breaks other systems, too bad.
I don't think the purpose of stable kernels is to intentionally break
systems in a stable release & backport fixes later...
> > Ultimately it's Greg's decision but it sounds like you're asking me to
> > say it's OK to break the JZ4780 in a stable kernel with a patch that I
> > think would be risky anyway, and I won't do that.
>
> I am saying this revert breaks AM335x, so this is a stalemate. I had a
> discussion with Ezequiel (on CC) and he seems to have a different
> smaller patch coming for this problem.
Well, no. Your patch says commit 2bed8a8e7072 ("Clearing FIFOs in RS485
emulation mode causes subsequent transmits to break") broke RS485 on
AM33x in v4.10, and it took 10 release cycles for someone to notice &
fix it. You're asking to break a system that is used & working in order
to fix a problem that seemingly wasn't even noticed for nearly 2 years.
Your fix breaks my system, but I outlined 4 reasons that I believe your
patch to be wrong anyway - breaking my system is just one part of that.
I'll rely to Ezequiel's patch now, but it again addresses just one part
of one of the 4 points I listed in the reasoning for my revert.
Thanks,
Paul
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Paul Burton @ 2018-12-16 22:24 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Marek Vasut, 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: <CAAEAJfAad75bHX39ETCdVv9vP0dF7PLz2vvFLLqgtyikPHqJyA@mail.gmail.com>
Hi Ezequiel,
On Sun, Dec 16, 2018 at 06:52:53PM -0300, Ezequiel Garcia wrote:
> diff --git a/drivers/tty/serial/8250/8250_port.c
> b/drivers/tty/serial/8250/8250_port.c
> index c39482b96111..fac19cbc51d1 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2209,10 +2209,11 @@ int serial8250_do_startup(struct uart_port *port)
> /*
> * Clear the FIFO buffers and disable them.
> * (they will be reenabled in set_termios())
> */
> serial8250_clear_fifos(up);
> + serial_out(up, UART_FCR, 0);
>
> /*
> * Clear the interrupt registers.
> */
> serial_port_in(port, UART_LSR);
>
This helps, but it only addresses one part of one of the 4 reasons I
listed as motivation for my revert. For example serial8250_do_shutdown()
also clearly intends to disable the FIFOs.
Thanks,
Paul
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Paul Burton @ 2018-12-16 22:28 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
In-Reply-To: <28a1d4ae-493d-8bbc-46f7-ad41ca04d782@denx.de>
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.
Thanks,
Paul
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Ezequiel Garcia @ 2018-12-16 22:28 UTC (permalink / raw)
To: Paul Burton
Cc: Marek Vasut, 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: <20181216222411.5jkexuaqxpfudj7b@pburton-laptop>
On Sun, 16 Dec 2018 at 19:24, Paul Burton <paul.burton@mips.com> wrote:
>
> Hi Ezequiel,
>
> On Sun, Dec 16, 2018 at 06:52:53PM -0300, Ezequiel Garcia wrote:
> > diff --git a/drivers/tty/serial/8250/8250_port.c
> > b/drivers/tty/serial/8250/8250_port.c
> > index c39482b96111..fac19cbc51d1 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -2209,10 +2209,11 @@ int serial8250_do_startup(struct uart_port *port)
> > /*
> > * Clear the FIFO buffers and disable them.
> > * (they will be reenabled in set_termios())
> > */
> > serial8250_clear_fifos(up);
> > + serial_out(up, UART_FCR, 0);
> >
> > /*
> > * Clear the interrupt registers.
> > */
> > serial_port_in(port, UART_LSR);
> >
>
> This helps, but it only addresses one part of one of the 4 reasons I
> listed as motivation for my revert. For example serial8250_do_shutdown()
> also clearly intends to disable the FIFOs.
>
OK. So, let's fix that :-)
By all means, it would be really nice to push forward and fix the garbage
issue on JZ4780, as well as the transmission issue on AM335x.
AM335x is a wildly popular platform, and it's not funny to break it.
So, let's please stop discussing which board we'll break and just fix both.
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Paul Burton @ 2018-12-16 22:35 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Marek Vasut, 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: <CAAEAJfAQ9=B6sm=Ard+YTDN5g5r03o=t9xU3Nu2QaKrXXZ4pGw@mail.gmail.com>
Hi Ezequiel,
On Sun, Dec 16, 2018 at 07:28:22PM -0300, Ezequiel Garcia wrote:
> On Sun, 16 Dec 2018 at 19:24, Paul Burton <paul.burton@mips.com> wrote:
> > This helps, but it only addresses one part of one of the 4 reasons I
> > listed as motivation for my revert. For example serial8250_do_shutdown()
> > also clearly intends to disable the FIFOs.
> >
>
> OK. So, let's fix that :-)
I already did, or at least tried to, on Thursday [1].
> By all means, it would be really nice to push forward and fix the garbage
> issue on JZ4780, as well as the transmission issue on AM335x.
>
> AM335x is a wildly popular platform, and it's not funny to break it.
Well, clearly not if it was broken in v4.10 & only just fixed..? And
from Marek's commit message the patch in v4.10 doesn't break the whole
system just RS485.
> So, let's please stop discussing which board we'll break and just fix both.
I completely agree that would be ideal and I wrote a patch hoping to do
that on Thursday, but didn't get any response on testing. It's late in
the cycle hence a revert made sense. Simple as that.
Thanks,
Paul
[1] https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
^ permalink raw reply
* Re: [PATCH v5 1/2] dmaengine: 8250_mtk_dma: add Mediatek uart DMA support
From: Long Cheng @ 2018-12-17 8:39 UTC (permalink / raw)
To: Sean Wang
Cc: vkoul, robh+dt, mark.rutland,
Ryder Lee (李庚諺), Matthias Brugger,
dan.j.williams, gregkh, jslaby,
Sean Wang (王志亘), dmaengine, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, linux-serial,
srv_heupstream, yingjoe.chen, YT Shen
In-Reply-To: <CAGp9LzpGfVgjL9MMQyh2iZRD9bJiuH0B0GTg_bahaa5ju2wmzw@mail.gmail.com>
On Fri, 2018-12-14 at 12:09 -0800, Sean Wang wrote:
thanks for your reply.
> On Thu, Dec 13, 2018 at 3:36 AM Long Cheng <long.cheng@mediatek.com> wrote:
>
> Hope those comments did not get a response that means they're fine with you.
>
> < ... >
>
> > > > +struct mtk_dmadev {
> > > > + struct dma_device ddev;
> > > > + void __iomem *mem_base[MTK_APDMA_CHANNELS];
> > > > + spinlock_t lock; /* dma dev lock */
> > > > + struct tasklet_struct task;
> > >
> > > we can drop tasklet and instead allows descriptors to be handled as
> > > fast as possible.
> > > similar suggestions have been made in the other dmaengine [1] and mtk-hsdma.c
> > >
> >
> > OK, i will remove these, and improve it.
> >
>
> Thanks, that would be great.
>
> > > [1] https://lkml.org/lkml/2018/11/11/146
> > >
> > > > + struct list_head pending;
> > > > + struct clk *clk;
> > > > + unsigned int dma_requests;
> > > > + bool support_33bits;
> > > > + unsigned int dma_irq[MTK_APDMA_CHANNELS];
> > > > + struct mtk_chan *ch[MTK_APDMA_CHANNELS];
> > > > +};
> > > > +
>
> < ... >
>
> > > > +static struct dma_async_tx_descriptor *mtk_dma_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_dma_chan(chan);
> > > > + struct scatterlist *sgent;
> > > > + struct mtk_dma_desc *d;
> > > > + struct mtk_dma_sg *sg;
> > > > + unsigned int size, i, j, en;
> > > > +
> > > > + en = 1;
> > > > +
> > > > + 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) + sglen * sizeof(d->sg[0]), GFP_ATOMIC);
> > > > + if (!d)
> > > > + return NULL;
> > > > +
> > > > + d->dir = dir;
> > > > +
> > > > + j = 0;
> > > > + for_each_sg(sgl, sgent, sglen, i) {
> > > > + d->sg[j].addr = sg_dma_address(sgent);
> > > > + d->sg[j].en = en;
> > > > + d->sg[j].fn = sg_dma_len(sgent) / en;
> > > > + j++;
> > > > + }
> > > > +
> > > > + d->sglen = j;
> > > > +
> > > > + if (dir == DMA_MEM_TO_DEV) {
> > > > + for (size = i = 0; i < d->sglen; i++) {
> > > > + sg = &d->sg[i];
> > > > + size += sg->en * sg->fn;
> > > > + }
> > > > + d->len = size;
> > > > + }
> > > > +
> > >
> > > The driver always only handles data move for the single contiguous
> > > area, but it seems the callback must provide the scatter-gather
> > > function to the dmaegine. otherwise, why is the callback be called
> > > device_prep_slave_sg?
> > >
> >
> > because in 8250 UART native code, call the device_prep_slave_sg. we just
> > use one ring buffer.
> >
>
> If it really did specifically for UART, you should show the function
> only can handle only one entry in sg for the DMA in a few comments and
> a sanity check for these invalid requests (more one entries in sg).
> Otherwise, the hardware will get a failure and even function doesn't
> complain or warn anything if more one entries in sg requested in.
> Additionally, the code can be simplified much if it's just for the
> specific use case.
>
ok. i will add some comments. and let the code be simplified.
> > > > + return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
> > > > +}
> > > > +
> > > > +static void mtk_dma_issue_pending(struct dma_chan *chan)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > > > + struct virt_dma_desc *vd;
> > > > + struct mtk_dmadev *mtkd;
> > > > + unsigned long flags;
> > > > +
> > > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > > + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> > > > + mtkd = to_mtk_dma_dev(chan->device);
> > >
> > > mtkd can be dropped as it seems no users
> > >
>
> < ... >
>
> > > > +static int mtk_dma_slave_config(struct dma_chan *chan,
> > > > + struct dma_slave_config *cfg)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > > > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(c->vc.chan.device);
> > > > + int ret;
> > > > +
> > > > + c->cfg = *cfg;
> > > > +
> > > > + if (cfg->direction == DMA_DEV_TO_MEM) {
> > > > + unsigned int rx_len = cfg->src_addr_width * 1024;
> > >
> > > it seems you should use cfg->src_port_window_size as the comments explains
> > >
> > > * @src_port_window_size: The length of the register area in words the data need
> > > * to be accessed on the device side. It is only used for devices which is using
> > > * an area instead of a single register to receive the data. Typically the DMA
> > > * loops in this area in order to transfer the data.
> > > * @dst_port_window_size: same as src_port_window_size but for the destination
> > > * port.
> > >
> >
> > in 8250 UART native code, will set the parameter. if want to modify
> > this, i think that maybe at next kernel release, we can modify it. i
> > suggest that not modify it at this patch. because it relate of 8250 uart
> > code. thanks.
> >
>
> You can fix it in the next version and a separate follow-up patch for
> the client driver.
>
> > > > +
> > > > + mtk_dma_chan_write(c, VFF_ADDR, cfg->src_addr);
> > > > + mtk_dma_chan_write(c, VFF_LEN, rx_len);
> > > > + mtk_dma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> > > > + mtk_dma_chan_write(c,
> > > > + VFF_INT_EN, VFF_RX_INT_EN0_B
> > > > + | VFF_RX_INT_EN1_B);
> > > > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > > > + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> > >
> > > I'd prefer to move those channel interrupt enablement to
> > > .device_alloc_chan_resources
> > > and related disablement to .device_free_chan_resources
> > >
> >
> > i think that, first need set the config to HW, and the enable the DMA.
> >
>
> I've read through the client driver and the dmaengine, I probably know
> how interaction they work and find out there is something you seem
> completely make a wrong.
>
> You can't do enable DMA with enabling VFF here. That causes a big
> problem, DMA would self decide to move data and not managed and issued
> by the dmaengine framework. For instance in DMA Tx path, because you
> enable the DMA and DMA buffer (VFF) and UART Tx ring uses the same
> memory area, DMA would self start to move data once data from
> userland go in Tx ring even without being issued by dmaengine.
>
> Instead, you should ensure all descriptors are being batched by
> .device_prep_slave_sg and DMA does not start moving data until
> .device_issue_pending done and once descriptors are issued, DMA
> hardware or software have to do it as fast as possible.
>
the VFF enable just enable the DMA function. DMA can't move data here.
Now, the code get length of the data in '.device_prep_slave_sg'.
And let DMA move data in '.device_issue_pending function'. in here, just
enable the function.
> > > > +
> > > > + if (!c->requested) {
> > > > + c->requested = true;
> > > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > > + mtk_dma_rx_interrupt,
> > > > + IRQF_TRIGGER_NONE,
> > > > + KBUILD_MODNAME, chan);
> > >
> > > ISR registration usually happens as the driver got probe, it can give
> > > the system more flexibility to manage such IRQ affinity on the fly.
> > >
> >
> > i will move the request irq to alloc channel.
> >
>
> why don't let it done in driver probe?
>
there are many uart ports, like UART[0-5]. in probe, just get the all
irq of ports. which port is using, who request irq. example, UART1 is
using. we just request irq of uart1. uart0, uart[2-5] don't need request
irq.
> > > > + if (ret < 0) {
> > > > + dev_err(chan->device->dev, "Can't request rx dma IRQ\n");
> > > > + return -EINVAL;
> > > > + }
> > > > + }
> > > > + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> > > > + unsigned int tx_len = cfg->dst_addr_width * 1024;
> > >
> > > Ditto as above, it seems you should use cfg->dst_port_window_size
> > >
> > > > +
> > > > + mtk_dma_chan_write(c, VFF_ADDR, cfg->dst_addr);
> > > > + mtk_dma_chan_write(c, VFF_LEN, tx_len);
> > > > + mtk_dma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> > > > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > > > + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> > >
> > > ditto, I'd prefer to move those channel interrupt enablement to
> > > .device_alloc_chan_resources and related disablement to
> > > .device_free_chan_resources
> > >
> > > > +
> > > > + if (!c->requested) {
> > > > + c->requested = true;
> > > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > > + mtk_dma_tx_interrupt,
> > > > + IRQF_TRIGGER_NONE,
> > > > + KBUILD_MODNAME, chan);
> > >
> > > ditto, we can request ISR with devm_request_irq in the driver got
> > > probe and trim the c->request member
> > >
> > > > + if (ret < 0) {
> > > > + dev_err(chan->device->dev, "Can't request tx dma IRQ\n");
> > > > + return -EINVAL;
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > + if (mtkd->support_33bits)
> > > > + mtk_dma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> > > > +
> > > > + if (mtk_dma_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_dma_terminate_all(struct dma_chan *chan)
> > > > +{
> > > > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > > > + unsigned long flags;
> > > > +
> > > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > > + list_del_init(&c->node);
> > > > + mtk_dma_stop(c);
> > > > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int mtk_dma_device_pause(struct dma_chan *chan)
> > > > +{
> > > > + /* just for check caps pass */
> > > > + return -EINVAL;
> > >
> > > always return error code seems not the client driver wants us to do.
> > >
> > > maybe if the hardware doesn't support pause, we can make a software
> > > pause, that waits until all active descriptors in hardware done, then
> > > disable interrupt and then stop handling the following vd in the
> > > vchan.
> > >
> > > > +}
> >
> > the code can't run. just for 8250 native code to check the function
> > pointer. in the future, if the function useful, we can realize. thanks.
> >
>
> Always return an error code looks like it's a faked function just to
> pass the criteria check. It seems not a good idea.
>
yes, the function is fake. i just review the 8250 uart framework. there
no check the return value of the function. so i can modify it to 'return
0'
> > > > +
> > > > +static int mtk_dma_device_resume(struct dma_chan *chan)
> > > > +{
> > > > + /* just for check caps pass */
> > > > + return -EINVAL;
>
> < ... >
>
> > > > +static struct platform_driver mtk_dma_driver = {
> > >
> > > mtk_dma is much general and all functions and structures in the driver
> > > should be all consistent. I'd prefer to have all naming starts with
> > > mtk_uart_apdma.
> > >
> >
> > the function name and parameters' name, i will modify it. but before the
> > 8250_mtk.c and the Doc. are recorded. if modify file name and Kconfig,
> > will bring about the disorder. i suggest that after the patch is
> > recorded, modify this. thanks.
> >
>
> We can do it in separate patches and in a logical order for the
> changes required across different subsystems.
>
> > > > + .probe = mtk_apdma_probe,
> > >
> > > such as
> > > mtk_uart_apdma_probe
> > >
> > > > + .remove = mtk_apdma_remove,
> > >
> > > mtk_uart_apdma_remove
> > >
> > > > + .driver = {
> > > > + .name = KBUILD_MODNAME,
> > > > + .pm = &mtk_dma_pm_ops,
> > >
> > > mtk_uart_apdma_pm_ops
> > >
> > > > + .of_match_table = of_match_ptr(mtk_uart_dma_match),
> > >
> > > mtk_uart_apdma_match
> > >
> > > > + },
> > > > +};
> > > > +
> > > > +module_platform_driver(mtk_dma_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..d399624 100644
> > > > --- a/drivers/dma/mediatek/Kconfig
> > > > +++ b/drivers/dma/mediatek/Kconfig
> > > > @@ -1,4 +1,15 @@
> > > >
> > > > +config DMA_MTK_UART
> > >
> > > MTK_UART_APDMA to align the other drivers
> > >
> > > > + 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 8250 mtk uart is enabled, and if you want to using DMA,
> > >
> > > 8250 mtk uart should be changed to SERIAL_8250_MT6577 to be more intuitive
> > >
> > > > + you can enable the config. the DMA engine just only be used
> > > > + with MediaTek Socs.
> > >
> > > 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_UART_APDMA) += mtk-uart-apdma.o
> > > to align the other dirvers
> > >
> > > > obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
> > > > --
> > > > 1.7.9.5
> > > >
> >
> >
^ permalink raw reply
* Re: [PATCH v5 1/2] dmaengine: 8250_mtk_dma: add Mediatek uart DMA support
From: Sean Wang @ 2018-12-17 10:07 UTC (permalink / raw)
To: long.cheng
Cc: vkoul, robh+dt, mark.rutland,
Ryder Lee (李庚諺), Matthias Brugger,
dan.j.williams, gregkh, jslaby,
Sean Wang (王志亘), dmaengine, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, linux-serial,
srv_heupstream, yingjoe.chen, YT Shen
In-Reply-To: <1545035989.28871.41.camel@mhfsdcap03>
On Mon, Dec 17, 2018 at 12:40 AM Long Cheng <long.cheng@mediatek.com> wrote:
>
> On Fri, 2018-12-14 at 12:09 -0800, Sean Wang wrote:
< ... >
> > > > > +
> > > > > + mtk_dma_chan_write(c, VFF_ADDR, cfg->src_addr);
> > > > > + mtk_dma_chan_write(c, VFF_LEN, rx_len);
> > > > > + mtk_dma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> > > > > + mtk_dma_chan_write(c,
> > > > > + VFF_INT_EN, VFF_RX_INT_EN0_B
> > > > > + | VFF_RX_INT_EN1_B);
> > > > > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > > > > + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> > > >
> > > > I'd prefer to move those channel interrupt enablement to
> > > > .device_alloc_chan_resources
> > > > and related disablement to .device_free_chan_resources
> > > >
> > >
> > > i think that, first need set the config to HW, and the enable the DMA.
> > >
> >
> > I've read through the client driver and the dmaengine, I probably know
> > how interaction they work and find out there is something you seem
> > completely make a wrong.
> >
> > You can't do enable DMA with enabling VFF here. That causes a big
> > problem, DMA would self decide to move data and not managed and issued
> > by the dmaengine framework. For instance in DMA Tx path, because you
> > enable the DMA and DMA buffer (VFF) and UART Tx ring uses the same
> > memory area, DMA would self start to move data once data from
> > userland go in Tx ring even without being issued by dmaengine.
> >
> > Instead, you should ensure all descriptors are being batched by
> > .device_prep_slave_sg and DMA does not start moving data until
> > .device_issue_pending done and once descriptors are issued, DMA
> > hardware or software have to do it as fast as possible.
> >
>
> the VFF enable just enable the DMA function. DMA can't move data here.
> Now, the code get length of the data in '.device_prep_slave_sg'.
> And let DMA move data in '.device_issue_pending function'. in here, just
> enable the function.
>
My all curiosity are all from the descriptor programmed in
.device_issue_pending in the other drivers at least includes
information such data length, target address, and hardware control
code, but in the driver only includes data length and even the target
address is set up at device_config, that seems unusual.
And, It does too for DMA_DEV_TO_MEM? What I see is there is almost no
any code be programmed for kick off the hardware for the direction but
DMA still can move. That is another point I got confused.
> > > > > +
> > > > > + if (!c->requested) {
> > > > > + c->requested = true;
> > > > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > > > + mtk_dma_rx_interrupt,
> > > > > + IRQF_TRIGGER_NONE,
> > > > > + KBUILD_MODNAME, chan);
> > > >
> > > > ISR registration usually happens as the driver got probe, it can give
> > > > the system more flexibility to manage such IRQ affinity on the fly.
> > > >
> > >
> > > i will move the request irq to alloc channel.
> > >
> >
> > why don't let it done in driver probe?
> >
> there are many uart ports, like UART[0-5]. in probe, just get the all
> irq of ports. which port is using, who request irq. example, UART1 is
> using. we just request irq of uart1. uart0, uart[2-5] don't need request
> irq.
>
> > > > > + if (ret < 0) {
> > > > > + dev_err(chan->device->dev, "Can't request rx dma IRQ\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > + }
> > > > > + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> > > > > + unsigned int tx_len = cfg->dst_addr_width * 1024;
> > > >
> > > > Ditto as above, it seems you should use cfg->dst_port_window_size
> > > >
> > > > > +
> > > > > + mtk_dma_chan_write(c, VFF_ADDR, cfg->dst_addr);
> > > > > + mtk_dma_chan_write(c, VFF_LEN, tx_len);
> > > > > + mtk_dma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> > > > > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > > > > + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> > > >
> > > > ditto, I'd prefer to move those channel interrupt enablement to
> > > > .device_alloc_chan_resources and related disablement to
> > > > .device_free_chan_resources
> > > >
> > > > > +
> > > > > + if (!c->requested) {
> > > > > + c->requested = true;
> > > > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > > > + mtk_dma_tx_interrupt,
> > > > > + IRQF_TRIGGER_NONE,
> > > > > + KBUILD_MODNAME, chan);
> > > >
> > > > ditto, we can request ISR with devm_request_irq in the driver got
> > > > probe and trim the c->request member
> > > >
> > > > > + if (ret < 0) {
> > > > > + dev_err(chan->device->dev, "Can't request tx dma IRQ\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + if (mtkd->support_33bits)
> > > > > + mtk_dma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> > > > > +
> > > > > + if (mtk_dma_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_dma_terminate_all(struct dma_chan *chan)
> > > > > +{
> > > > > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > > > > + unsigned long flags;
> > > > > +
> > > > > + spin_lock_irqsave(&c->vc.lock, flags);
> > > > > + list_del_init(&c->node);
> > > > > + mtk_dma_stop(c);
> > > > > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int mtk_dma_device_pause(struct dma_chan *chan)
> > > > > +{
> > > > > + /* just for check caps pass */
> > > > > + return -EINVAL;
> > > >
> > > > always return error code seems not the client driver wants us to do.
> > > >
> > > > maybe if the hardware doesn't support pause, we can make a software
> > > > pause, that waits until all active descriptors in hardware done, then
> > > > disable interrupt and then stop handling the following vd in the
> > > > vchan.
> > > >
> > > > > +}
> > >
> > > the code can't run. just for 8250 native code to check the function
> > > pointer. in the future, if the function useful, we can realize. thanks.
> > >
> >
> > Always return an error code looks like it's a faked function just to
> > pass the criteria check. It seems not a good idea.
> >
>
> yes, the function is fake. i just review the 8250 uart framework. there
> no check the return value of the function. so i can modify it to 'return
> 0'
>
> > > > > +
> > > > > +static int mtk_dma_device_resume(struct dma_chan *chan)
> > > > > +{
> > > > > + /* just for check caps pass */
> > > > > + return -EINVAL;
> >
> > < ... >
> >
> > > > > +static struct platform_driver mtk_dma_driver = {
> > > >
> > > > mtk_dma is much general and all functions and structures in the driver
> > > > should be all consistent. I'd prefer to have all naming starts with
> > > > mtk_uart_apdma.
> > > >
> > >
> > > the function name and parameters' name, i will modify it. but before the
> > > 8250_mtk.c and the Doc. are recorded. if modify file name and Kconfig,
> > > will bring about the disorder. i suggest that after the patch is
> > > recorded, modify this. thanks.
> > >
> >
> > We can do it in separate patches and in a logical order for the
> > changes required across different subsystems.
> >
> > > > > + .probe = mtk_apdma_probe,
> > > >
> > > > such as
> > > > mtk_uart_apdma_probe
> > > >
> > > > > + .remove = mtk_apdma_remove,
> > > >
> > > > mtk_uart_apdma_remove
> > > >
> > > > > + .driver = {
> > > > > + .name = KBUILD_MODNAME,
> > > > > + .pm = &mtk_dma_pm_ops,
> > > >
> > > > mtk_uart_apdma_pm_ops
> > > >
> > > > > + .of_match_table = of_match_ptr(mtk_uart_dma_match),
> > > >
> > > > mtk_uart_apdma_match
> > > >
> > > > > + },
> > > > > +};
> > > > > +
> > > > > +module_platform_driver(mtk_dma_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..d399624 100644
> > > > > --- a/drivers/dma/mediatek/Kconfig
> > > > > +++ b/drivers/dma/mediatek/Kconfig
> > > > > @@ -1,4 +1,15 @@
> > > > >
> > > > > +config DMA_MTK_UART
> > > >
> > > > MTK_UART_APDMA to align the other drivers
> > > >
> > > > > + 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 8250 mtk uart is enabled, and if you want to using DMA,
> > > >
> > > > 8250 mtk uart should be changed to SERIAL_8250_MT6577 to be more intuitive
> > > >
> > > > > + you can enable the config. the DMA engine just only be used
> > > > > + with MediaTek Socs.
> > > >
> > > > 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_UART_APDMA) += mtk-uart-apdma.o
> > > > to align the other dirvers
> > > >
> > > > > obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
> > > > > --
> > > > > 1.7.9.5
> > > > >
> > >
> > >
>
>
^ permalink raw reply
* Re: [PATCH v5 1/2] dmaengine: 8250_mtk_dma: add Mediatek uart DMA support
From: Long Cheng @ 2018-12-17 11:57 UTC (permalink / raw)
To: Sean Wang
Cc: vkoul, robh+dt, mark.rutland,
Ryder Lee (李庚諺), Matthias Brugger,
dan.j.williams, gregkh, jslaby,
Sean Wang (王志亘), dmaengine, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, linux-serial,
srv_heupstream, yingjoe.chen, YT Shen, Long Cheng
In-Reply-To: <CAGp9LzqFS5ku8-WE0Xc2xwztw9DumeU0DE4ZXbpH8w8u12LnOA@mail.gmail.com>
On Mon, 2018-12-17 at 02:07 -0800, Sean Wang wrote:
> On Mon, Dec 17, 2018 at 12:40 AM Long Cheng <long.cheng@mediatek.com> wrote:
> >
> > On Fri, 2018-12-14 at 12:09 -0800, Sean Wang wrote:
>
> < ... >
>
> > > > > > +
> > > > > > + mtk_dma_chan_write(c, VFF_ADDR, cfg->src_addr);
> > > > > > + mtk_dma_chan_write(c, VFF_LEN, rx_len);
> > > > > > + mtk_dma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> > > > > > + mtk_dma_chan_write(c,
> > > > > > + VFF_INT_EN, VFF_RX_INT_EN0_B
> > > > > > + | VFF_RX_INT_EN1_B);
> > > > > > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > > > > > + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> > > > >
> > > > > I'd prefer to move those channel interrupt enablement to
> > > > > .device_alloc_chan_resources
> > > > > and related disablement to .device_free_chan_resources
> > > > >
> > > >
> > > > i think that, first need set the config to HW, and the enable the DMA.
> > > >
> > >
> > > I've read through the client driver and the dmaengine, I probably know
> > > how interaction they work and find out there is something you seem
> > > completely make a wrong.
> > >
> > > You can't do enable DMA with enabling VFF here. That causes a big
> > > problem, DMA would self decide to move data and not managed and issued
> > > by the dmaengine framework. For instance in DMA Tx path, because you
> > > enable the DMA and DMA buffer (VFF) and UART Tx ring uses the same
> > > memory area, DMA would self start to move data once data from
> > > userland go in Tx ring even without being issued by dmaengine.
> > >
> > > Instead, you should ensure all descriptors are being batched by
> > > .device_prep_slave_sg and DMA does not start moving data until
> > > .device_issue_pending done and once descriptors are issued, DMA
> > > hardware or software have to do it as fast as possible.
> > >
> >
> > the VFF enable just enable the DMA function. DMA can't move data here.
> > Now, the code get length of the data in '.device_prep_slave_sg'.
> > And let DMA move data in '.device_issue_pending function'. in here, just
> > enable the function.
> >
>
> My all curiosity are all from the descriptor programmed in
> .device_issue_pending in the other drivers at least includes
> information such data length, target address, and hardware control
> code, but in the driver only includes data length and even the target
> address is set up at device_config, that seems unusual.
>
> And, It does too for DMA_DEV_TO_MEM? What I see is there is almost no
> any code be programmed for kick off the hardware for the direction but
> DMA still can move. That is another point I got confused.
>
8250_dma in tty, mapping xmit buffer to DMA buffer. and the tx just use
the only one buffer. it's ring buffer. 8250_dma set the begin address
and length of the buffer by means of '.deivce_config' function.
when TX happen, tty_write will write data to xmit buffer. in 8250_dma,
will set the address and length of the data by means of
'.device_prep_slave_sg'. but the address is in the xmit buffer rang. the
WPT(write pointer), RPT(read pointer) registers are recored the DMA data
transfer status, include the current location of transmission. and the
apdma is only for UART. So don't need recored the the address of data ,
target address in '.device_prep_slave_sg'.
in '.device_issue_pending', just using the data length from descriptor,
WPT , we can figure out what's data need to move. then update the WPT
and flush TX.
RX too. RX use other buffer in instead of XMIT buffer. when RX interrupt
coming, will update the RPT, and 8250_dma will get the length from vchan
complete. then 8250_dma can get the data.
> > > > > > +
> > > > > > + if (!c->requested) {
> > > > > > + c->requested = true;
> > > > > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > > > > + mtk_dma_rx_interrupt,
> > > > > > + IRQF_TRIGGER_NONE,
> > > > > > + KBUILD_MODNAME, chan);
> > > > >
> > > > > ISR registration usually happens as the driver got probe, it can give
> > > > > the system more flexibility to manage such IRQ affinity on the fly.
> > > > >
> > > >
> > > > i will move the request irq to alloc channel.
> > > >
> > >
> > > why don't let it done in driver probe?
> > >
> > there are many uart ports, like UART[0-5]. in probe, just get the all
> > irq of ports. which port is using, who request irq. example, UART1 is
> > using. we just request irq of uart1. uart0, uart[2-5] don't need request
> > irq.
> >
> > > > > > + if (ret < 0) {
> > > > > > + dev_err(chan->device->dev, "Can't request rx dma IRQ\n");
> > > > > > + return -EINVAL;
> > > > > > + }
> > > > > > + }
> > > > > > + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> > > > > > + unsigned int tx_len = cfg->dst_addr_width * 1024;
> > > > >
> > > > > Ditto as above, it seems you should use cfg->dst_port_window_size
> > > > >
> > > > > > +
> > > > > > + mtk_dma_chan_write(c, VFF_ADDR, cfg->dst_addr);
> > > > > > + mtk_dma_chan_write(c, VFF_LEN, tx_len);
> > > > > > + mtk_dma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> > > > > > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > > > > > + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> > > > >
> > > > > ditto, I'd prefer to move those channel interrupt enablement to
> > > > > .device_alloc_chan_resources and related disablement to
> > > > > .device_free_chan_resources
> > > > >
> > > > > > +
> > > > > > + if (!c->requested) {
> > > > > > + c->requested = true;
> > > > > > + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> > > > > > + mtk_dma_tx_interrupt,
> > > > > > + IRQF_TRIGGER_NONE,
> > > > > > + KBUILD_MODNAME, chan);
> > > > >
> > > > > ditto, we can request ISR with devm_request_irq in the driver got
> > > > > probe and trim the c->request member
> > > > >
> > > > > > + if (ret < 0) {
> > > > > > + dev_err(chan->device->dev, "Can't request tx dma IRQ\n");
> > > > > > + return -EINVAL;
> > > > > > + }
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + if (mtkd->support_33bits)
> > > > > > + mtk_dma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> > > > > > +
> > > > > > + if (mtk_dma_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;
> > > > > > +}
> > > > > > +
< ... >
^ permalink raw reply
* [PATCH V5 1/4] dt-bindings: serial: lpuart: add imx8qxp compatible string
From: Aisheng Dong @ 2018-12-17 15:00 UTC (permalink / raw)
To: linux-arm-kernel@lists.infradead.org
Cc: Aisheng Dong, Mark Rutland, dongas86@gmail.com,
devicetree@vger.kernel.org, catalin.marinas@arm.com,
will.deacon@arm.com, Greg Kroah-Hartman, robh+dt@kernel.org,
dl-linux-imx, kernel@pengutronix.de, linux-serial@vger.kernel.org,
Fabio Estevam, shawnguo@kernel.org
In-Reply-To: <1545058517-18643-1-git-send-email-aisheng.dong@nxp.com>
Add imx8qxp compatible string
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-serial@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
Documentation/devicetree/bindings/serial/fsl-lpuart.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/serial/fsl-lpuart.txt b/Documentation/devicetree/bindings/serial/fsl-lpuart.txt
index 6bd3f2e..21483ba 100644
--- a/Documentation/devicetree/bindings/serial/fsl-lpuart.txt
+++ b/Documentation/devicetree/bindings/serial/fsl-lpuart.txt
@@ -8,6 +8,8 @@ Required properties:
on LS1021A SoC with 32-bit big-endian register organization
- "fsl,imx7ulp-lpuart" for lpuart compatible with the one integrated
on i.MX7ULP SoC with 32-bit little-endian register organization
+ - "fsl,imx8qxp-lpuart" for lpuart compatible with the one integrated
+ on i.MX8QXP SoC with 32-bit little-endian register organization
- reg : Address and length of the register set for the device
- interrupts : Should contain uart interrupt
- clocks : phandle + clock specifier pairs, one for each entry in clock-names
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] serial: uartps: Add the device_init_wakeup
From: Greg Kroah-Hartman @ 2018-12-17 15:15 UTC (permalink / raw)
To: Michal Simek
Cc: linux-kernel, monstr, Shubhrajyoti Datta, Jiri Slaby,
linux-serial, linux-arm-kernel
In-Reply-To: <ec9fdbdf1c444dc60edfc75a31583630e76a9b72.1544016009.git.michal.simek@xilinx.com>
On Wed, Dec 05, 2018 at 02:20:11PM +0100, Michal Simek wrote:
> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
>
> Initialise the device wakeup.
Why? This says what happened, which you can see from the patch itself,
but it does not say why this is needed at all.
Also, you sent 4 patches, without any numbering, so I have no idea what
order to apply them in.
Please fix the changelog entries up and send this as a patch series so
that I can correctly apply them.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Greg Kroah-Hartman @ 2018-12-17 15:18 UTC (permalink / raw)
To: Paul Burton
Cc: Ezequiel Garcia, Marek Vasut, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org, Paul Burton, Daniel Jedrychowski,
linux-mips@vger.kernel.org, stable, Ezequiel Garcia
In-Reply-To: <20181216223510.hxsdotf332ousinh@pburton-laptop>
On Sun, Dec 16, 2018 at 10:35:12PM +0000, Paul Burton wrote:
> Hi Ezequiel,
>
> On Sun, Dec 16, 2018 at 07:28:22PM -0300, Ezequiel Garcia wrote:
> > On Sun, 16 Dec 2018 at 19:24, Paul Burton <paul.burton@mips.com> wrote:
> > > This helps, but it only addresses one part of one of the 4 reasons I
> > > listed as motivation for my revert. For example serial8250_do_shutdown()
> > > also clearly intends to disable the FIFOs.
> > >
> >
> > OK. So, let's fix that :-)
>
> I already did, or at least tried to, on Thursday [1].
>
> > By all means, it would be really nice to push forward and fix the garbage
> > issue on JZ4780, as well as the transmission issue on AM335x.
> >
> > AM335x is a wildly popular platform, and it's not funny to break it.
>
> Well, clearly not if it was broken in v4.10 & only just fixed..? And
> from Marek's commit message the patch in v4.10 doesn't break the whole
> system just RS485.
>
> > So, let's please stop discussing which board we'll break and just fix both.
>
> I completely agree that would be ideal and I wrote a patch hoping to do
> that on Thursday, but didn't get any response on testing. It's late in
> the cycle hence a revert made sense. Simple as that.
A revert makes sense now, I'll go queue this up, thanks.
greg k-h
^ permalink raw reply
* Re: [PATCH] serial: uartps: Add the device_init_wakeup
From: Michal Simek @ 2018-12-17 15:29 UTC (permalink / raw)
To: Greg Kroah-Hartman, Michal Simek
Cc: monstr, Shubhrajyoti Datta, linux-kernel, linux-serial,
Jiri Slaby, linux-arm-kernel
In-Reply-To: <20181217151520.GA12453@kroah.com>
On 17. 12. 18 16:15, Greg Kroah-Hartman wrote:
> On Wed, Dec 05, 2018 at 02:20:11PM +0100, Michal Simek wrote:
>> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
>>
>> Initialise the device wakeup.
>
> Why? This says what happened, which you can see from the patch itself,
> but it does not say why this is needed at all.
We will fix this.
>
> Also, you sent 4 patches, without any numbering, so I have no idea what
> order to apply them in.
>
> Please fix the changelog entries up and send this as a patch series so
> that I can correctly apply them.
They are all independent that's why I have sent them separately as
single patches.
Thanks,
Michal
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Marek Vasut @ 2018-12-17 15:43 UTC (permalink / raw)
To: Greg Kroah-Hartman, Paul Burton
Cc: Ezequiel Garcia, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org, Paul Burton, Daniel Jedrychowski,
linux-mips@vger.kernel.org, stable, Ezequiel Garcia
In-Reply-To: <20181217151851.GA21564@kroah.com>
On 12/17/2018 04:18 PM, Greg Kroah-Hartman wrote:
> On Sun, Dec 16, 2018 at 10:35:12PM +0000, Paul Burton wrote:
>> Hi Ezequiel,
>>
>> On Sun, Dec 16, 2018 at 07:28:22PM -0300, Ezequiel Garcia wrote:
>>> On Sun, 16 Dec 2018 at 19:24, Paul Burton <paul.burton@mips.com> wrote:
>>>> This helps, but it only addresses one part of one of the 4 reasons I
>>>> listed as motivation for my revert. For example serial8250_do_shutdown()
>>>> also clearly intends to disable the FIFOs.
>>>>
>>>
>>> OK. So, let's fix that :-)
>>
>> I already did, or at least tried to, on Thursday [1].
>>
>>> By all means, it would be really nice to push forward and fix the garbage
>>> issue on JZ4780, as well as the transmission issue on AM335x.
>>>
>>> AM335x is a wildly popular platform, and it's not funny to break it.
>>
>> Well, clearly not if it was broken in v4.10 & only just fixed..? And
>> from Marek's commit message the patch in v4.10 doesn't break the whole
>> system just RS485.
>>
>>> So, let's please stop discussing which board we'll break and just fix both.
>>
>> I completely agree that would be ideal and I wrote a patch hoping to do
>> that on Thursday, but didn't get any response on testing. It's late in
>> the cycle hence a revert made sense. Simple as that.
>
> A revert makes sense now, I'll go queue this up, thanks.
I don't like this for multiple reasons.
1) There is a better patch posted which doesn't break the AM335x and
clearly identifies and fixes the problem on the JZ4780 / CI20
2) The JZ4780 8250 core is not a standard 8250 core, since it has extra
bits in the FCR register (like the UME bit, which disables the whole
UART block), so the revert IMO would break that core too, it just
hides the breakage. I'm still trying to understand the implications
of that in detail, but the discussion wasn't quite constructive.
I'd much rather see Ezequiel's patch applied, since that's far less
destructive approach to fixing the problem than the revert.
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH] serial: uartps: Add the device_init_wakeup
From: Greg Kroah-Hartman @ 2018-12-17 15:44 UTC (permalink / raw)
To: Michal Simek
Cc: linux-kernel, monstr, Shubhrajyoti Datta, Jiri Slaby,
linux-serial, linux-arm-kernel
In-Reply-To: <333f4be8-a4ae-4276-e2ac-725d1ae4c9b0@xilinx.com>
On Mon, Dec 17, 2018 at 04:29:56PM +0100, Michal Simek wrote:
> On 17. 12. 18 16:15, Greg Kroah-Hartman wrote:
> > On Wed, Dec 05, 2018 at 02:20:11PM +0100, Michal Simek wrote:
> >> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> >>
> >> Initialise the device wakeup.
> >
> > Why? This says what happened, which you can see from the patch itself,
> > but it does not say why this is needed at all.
>
> We will fix this.
>
> >
> > Also, you sent 4 patches, without any numbering, so I have no idea what
> > order to apply them in.
> >
> > Please fix the changelog entries up and send this as a patch series so
> > that I can correctly apply them.
>
> They are all independent that's why I have sent them separately as
> single patches.
But when you modify the same file, how am I supposed to know that?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
From: Ezequiel Garcia @ 2018-12-17 16:30 UTC (permalink / raw)
To: Paul Burton
Cc: Marek Vasut, 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: <20181216223510.hxsdotf332ousinh@pburton-laptop>
On Sun, 16 Dec 2018 at 19:35, Paul Burton <paul.burton@mips.com> wrote:
>
> Hi Ezequiel,
>
> On Sun, Dec 16, 2018 at 07:28:22PM -0300, Ezequiel Garcia wrote:
> > On Sun, 16 Dec 2018 at 19:24, Paul Burton <paul.burton@mips.com> wrote:
> > > This helps, but it only addresses one part of one of the 4 reasons I
> > > listed as motivation for my revert. For example serial8250_do_shutdown()
> > > also clearly intends to disable the FIFOs.
> > >
> >
> > OK. So, let's fix that :-)
>
> I already did, or at least tried to, on Thursday [1].
>
> > By all means, it would be really nice to push forward and fix the garbage
> > issue on JZ4780, as well as the transmission issue on AM335x.
> >
> > AM335x is a wildly popular platform, and it's not funny to break it.
>
> Well, clearly not if it was broken in v4.10 & only just fixed..? And
> from Marek's commit message the patch in v4.10 doesn't break the whole
> system just RS485.
>
Careful here. It's naive to consider v4.10 as old in this context.
AM335x is used in hundreds of thousands of products, probably
"industrial" products.
Manufacturers don't always follow the kernel, and it's entirely
likely that they notice a regression only when developing a new product,
or when rebasing on top of a newer longterm kernel.
Then again, I don't have any details about what is Marek's original fix
actually fixing.
Marek: could you please post the test case that you used to validate your fix?
I can't find that anywhere.
> > So, let's please stop discussing which board we'll break and just fix both.
>
> I completely agree that would be ideal and I wrote a patch hoping to do
> that on Thursday, but didn't get any response on testing. It's late in
> the cycle hence a revert made sense. Simple as that.
>
> Thanks,
> Paul
>
> [1] https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
Well, I think this patch a lot of sense. But since Greg wants to
revert Marek's fix,
it would make sense to collate both Marek's and Paul's in a single commit.
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
^ 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