* 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
* Re: [PATCH v5 1/2] dmaengine: 8250_mtk_dma: add Mediatek uart DMA support
From: Sean Wang @ 2018-12-14 20:09 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: <1544700985.28871.26.camel@mhfsdcap03>
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.
> > > + 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.
> > > +
> > > + 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?
> > > + 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.
> > > +
> > > +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] serial: sh-sci: Document r8a774c0 bindings
From: Simon Horman @ 2018-12-14 16:39 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Greg Kroah-Hartman, Rob Herring, Mark Rutland, linux-serial,
devicetree, linux-kernel, Geert Uytterhoeven, Chris Paterson,
Biju Das, linux-renesas-soc
In-Reply-To: <1544732274-5904-1-git-send-email-fabrizio.castro@bp.renesas.com>
On Thu, Dec 13, 2018 at 08:17:54PM +0000, Fabrizio Castro wrote:
> RZ/G2E (R8A774C0) SoC also has the R-Car Gen3 compatible SCIF and
> HSCIF ports, so document the SoC specific bindings.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
^ permalink raw reply
* [PATCH 4/4] arm64: dts: exynos: Add Bluetooth chip to TM2(e) boards
From: Marek Szyprowski @ 2018-12-14 11:34 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: <20181214113410.22848-1-m.szyprowski@samsung.com>
TM2(e) boards have a Broadcom Bluetooth chip connected to 3rd UART port.
Add a device tree node describing it and its resources (control GPIO lines
and clock).
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
.../boot/dts/exynos/exynos5433-tm2-common.dtsi | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
index f3ed4c078ba5..d88e2f0e179a 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
@@ -1202,6 +1202,20 @@
status = "okay";
};
+&serial_3 {
+ status = "okay";
+
+ bluetooth {
+ compatible = "brcm,bcm43438-bt";
+ max-speed = <3000000>;
+ shutdown-gpios = <&gpd4 0 GPIO_ACTIVE_HIGH>;
+ device-wakeup-gpios = <&gpr3 7 GPIO_ACTIVE_HIGH>;
+ host-wakeup-gpios = <&gpa2 2 GPIO_ACTIVE_HIGH>;
+ clocks = <&s2mps13_osc S2MPS11_CLK_BT>;
+ clock-names = "extclk";
+ };
+};
+
&spi_1 {
cs-gpios = <&gpd6 3 GPIO_ACTIVE_HIGH>;
status = "okay";
--
2.17.1
^ permalink raw reply related
* [PATCH 3/4] tty: serial: samsung: Increase maximum baudrate
From: Marek Szyprowski @ 2018-12-14 11:34 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: <20181214113410.22848-1-m.szyprowski@samsung.com>
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);
quot = s3c24xx_serial_getclk(ourport, baud, &clk, &clk_sel);
if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST)
quot = port->custom_divisor;
--
2.17.1
^ permalink raw reply related
* [PATCH 2/4] tty: serial: samsung: Properly set flags in autoCTS mode
From: Marek Szyprowski @ 2018-12-14 11:34 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: <20181214113410.22848-1-m.szyprowski@samsung.com>
From: Beomho Seo <beomho.seo@samsung.com>
Commit 391f93f2ec9f ("serial: core: Rework hw-assited flow control support")
has changed the way the autoCTS mode is handled.
According to that change, serial drivers which enable H/W autoCTS mode must
set UPSTAT_AUTOCTS to prevent the serial core from inadvertently disabling
TX. This patch adds proper handling of UPSTAT_AUTOCTS flag.
Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
[mszyprow: rephrased commit message]
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/tty/serial/samsung.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index da1bd4bba8a9..2a49b6d876b8 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -1365,11 +1365,14 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
wr_regl(port, S3C2410_ULCON, ulcon);
wr_regl(port, S3C2410_UBRDIV, quot);
+ port->status &= ~UPSTAT_AUTOCTS;
+
umcon = rd_regl(port, S3C2410_UMCON);
if (termios->c_cflag & CRTSCTS) {
umcon |= S3C2410_UMCOM_AFC;
/* Disable RTS when RX FIFO contains 63 bytes */
umcon &= ~S3C2412_UMCON_AFC_8;
+ port->status = UPSTAT_AUTOCTS;
} else {
umcon &= ~S3C2410_UMCOM_AFC;
}
--
2.17.1
^ permalink raw reply related
* [PATCH 1/4] mfd: exynos-lpass: Enable UART module support
From: Marek Szyprowski @ 2018-12-14 11:34 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: <20181214113410.22848-1-m.szyprowski@samsung.com>
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.
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)
--
2.17.1
^ permalink raw reply related
* [PATCH 0/4] Samsung TM2(e): add Bluetooth support (resend)
From: Marek Szyprowski @ 2018-12-14 11:34 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: <CGME20181214113415eucas1p16dd61a4312d88216cb0d737887c751b7@eucas1p1.samsung.com>
Hi All
This patchset enables Bluetooth support on TM2/TM2e boards using mainline
HCI_BCM driver. It requires a few fixes to Samsung serial driver and
adding a proper node to device tree description of the mentioned boards.
Patches can be applied independently to each subsystem.
Best regards
Marek Szyprowski
Samsung R&D Institute Poland
(resend reason: added respective maintainers and lists to cc)
Patch summary:
Beomho Seo (2):
mfd: exynos-lpass: Enable UART module support
tty: serial: samsung: Properly set flags in autoCTS mode
Marek Szyprowski (1):
arm64: dts: exynos: Add Bluetooth chip to TM2(e) boards
Seung-Woo Kim (1):
tty: serial: samsung: Increase maximum baudrate
.../boot/dts/exynos/exynos5433-tm2-common.dtsi | 14 ++++++++++++++
drivers/mfd/exynos-lpass.c | 4 +++-
drivers/tty/serial/samsung.c | 5 ++++-
3 files changed, 21 insertions(+), 2 deletions(-)
--
2.17.1
^ permalink raw reply
* RE: [PATCH] serial: sh-sci: Document r8a774c0 bindings
From: Fabrizio Castro @ 2018-12-14 10:23 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, Linux-Renesas
In-Reply-To: <CAMuHMdWbBPue+n=Z9GOJVFFKr-stRV4-ozH0sxWjUbRmjDqN6g@mail.gmail.com>
Hello Geert,
Thank you for your feedback!
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 14 December 2018 10:05
> Subject: Re: [PATCH] serial: sh-sci: Document r8a774c0 bindings
>
> On Thu, Dec 13, 2018 at 9:18 PM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
>
> dt-bindings: ...
Will send a v2 to fix this
Thanks,
Fab
>
> > RZ/G2E (R8A774C0) SoC also has the R-Car Gen3 compatible SCIF and
> > HSCIF ports, so document the SoC specific bindings.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
[https://www2.renesas.eu/media/email/unicef.jpg]
This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: Document r8a774c0 bindings
From: Geert Uytterhoeven @ 2018-12-14 10:05 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Simon Horman, Geert Uytterhoeven,
Chris Paterson, Biju Das, Linux-Renesas
In-Reply-To: <1544732274-5904-1-git-send-email-fabrizio.castro@bp.renesas.com>
On Thu, Dec 13, 2018 at 9:18 PM Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
dt-bindings: ...
> RZ/G2E (R8A774C0) SoC also has the R-Car Gen3 compatible SCIF and
> HSCIF ports, so document the SoC specific bindings.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v2] tty: serial: qcom_geni_serial: Remove interrupt storm
From: Doug Anderson @ 2018-12-14 0:59 UTC (permalink / raw)
To: Ryan Case
Cc: Greg Kroah-Hartman, Jiri Slaby, LKML, linux-serial, Stephen Boyd
In-Reply-To: <20181213194321.240148-1-ryandcase@chromium.org>
Hi,
On Thu, Dec 13, 2018 at 11:43 AM Ryan Case <ryandcase@chromium.org> wrote:
>
> Disable M_TX_FIFO_WATERMARK_EN after we've sent all data for a given
> transaction so we don't continue to receive a flurry of free space
> interrupts while waiting for the M_CMD_DONE notification. Re-enable the
> watermark when establishing the next transaction.
>
> Also clear the watermark interrupt after filling the FIFO so we do not
> receive notification again prior to actually having free space.
>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
> Changes in v2:
> Addressed Doug's comments
> - Avoid M_TX_WATERMARK_EN writes when values already match
> - Added note about M_TX_WATERMARK_EN triggering and latching
>
> drivers/tty/serial/qcom_geni_serial.c | 25 +++++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
Thanks!
Reviewed-by: Douglas Anderson <dianders@chromium.org>
If I echo something out the serial port I now see 2 interrupts fire.
That seems about right compared to before where we'd get boatloads.
Thus:
Tested-by: Douglas Anderson <dianders@chromium.org>
-Doug
^ permalink raw reply
* [PATCH] serial: sh-sci: Document r8a774c0 bindings
From: Fabrizio Castro @ 2018-12-13 20:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Mark Rutland
Cc: Fabrizio Castro, linux-serial, devicetree, linux-kernel,
Simon Horman, Geert Uytterhoeven, Chris Paterson, Biju Das,
linux-renesas-soc
RZ/G2E (R8A774C0) SoC also has the R-Car Gen3 compatible SCIF and
HSCIF ports, so document the SoC specific bindings.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
This patch depends on:
https://patchwork.kernel.org/patch/10565543/
---
Documentation/devicetree/bindings/serial/renesas,sci-serial.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
index 21526e8..20232ad 100644
--- a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
+++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
@@ -26,6 +26,8 @@ Required properties:
- "renesas,hscif-r8a77470" for R8A77470 (RZ/G1C) HSCIF compatible UART.
- "renesas,scif-r8a774a1" for R8A774A1 (RZ/G2M) SCIF compatible UART.
- "renesas,hscif-r8a774a1" for R8A774A1 (RZ/G2M) HSCIF compatible UART.
+ - "renesas,scif-r8a774c0" for R8A774C0 (RZ/G2E) SCIF compatible UART.
+ - "renesas,hscif-r8a774c0" for R8A774C0 (RZ/G2E) HSCIF compatible UART.
- "renesas,scif-r8a7778" for R8A7778 (R-Car M1) SCIF compatible UART.
- "renesas,scif-r8a7779" for R8A7779 (R-Car H1) SCIF compatible UART.
- "renesas,scif-r8a7790" for R8A7790 (R-Car H2) SCIF compatible UART.
--
2.7.4
^ permalink raw reply related
* [PATCH v2] tty: serial: qcom_geni_serial: Remove interrupt storm
From: Ryan Case @ 2018-12-13 19:43 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Doug Anderson, linux-kernel, linux-serial, Stephen Boyd,
Ryan Case
Disable M_TX_FIFO_WATERMARK_EN after we've sent all data for a given
transaction so we don't continue to receive a flurry of free space
interrupts while waiting for the M_CMD_DONE notification. Re-enable the
watermark when establishing the next transaction.
Also clear the watermark interrupt after filling the FIFO so we do not
receive notification again prior to actually having free space.
Signed-off-by: Ryan Case <ryandcase@chromium.org>
---
Changes in v2:
Addressed Doug's comments
- Avoid M_TX_WATERMARK_EN writes when values already match
- Added note about M_TX_WATERMARK_EN triggering and latching
drivers/tty/serial/qcom_geni_serial.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 6e38498362ef..0c93beb69e73 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -724,6 +724,7 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
size_t pending;
int i;
u32 status;
+ u32 irq_en;
unsigned int chunk;
int tail;
@@ -752,6 +753,11 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
if (!port->tx_remaining) {
qcom_geni_serial_setup_tx(uport, pending);
port->tx_remaining = pending;
+
+ irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
+ if (!(irq_en & M_TX_FIFO_WATERMARK_EN))
+ writel_relaxed(irq_en | M_TX_FIFO_WATERMARK_EN,
+ uport->membase + SE_GENI_M_IRQ_EN);
}
remaining = chunk;
@@ -775,7 +781,23 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
}
xmit->tail = tail & (UART_XMIT_SIZE - 1);
+
+ /*
+ * The tx fifo watermark is level triggered and latched. Though we had
+ * cleared it in qcom_geni_serial_isr it will have already reasserted
+ * so we must clear it again here after our writes.
+ */
+ writel_relaxed(M_TX_FIFO_WATERMARK_EN,
+ uport->membase + SE_GENI_M_IRQ_CLEAR);
+
out_write_wakeup:
+ if (!port->tx_remaining) {
+ irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
+ if (irq_en & M_TX_FIFO_WATERMARK_EN)
+ writel_relaxed(irq_en & ~M_TX_FIFO_WATERMARK_EN,
+ uport->membase + SE_GENI_M_IRQ_EN);
+ }
+
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup(uport);
}
@@ -811,8 +833,7 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
tty_insert_flip_char(tport, 0, TTY_OVERRUN);
}
- if (m_irq_status & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN) &&
- m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
+ if (m_irq_status & m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
qcom_geni_serial_handle_tx(uport, m_irq_status & M_CMD_DONE_EN,
geni_status & M_GENI_CMD_ACTIVE);
--
2.20.0.rc2.403.gdbc3b29805-goog
^ permalink raw reply related
* Re: [PATCH] serial: 8250: Fix clearing FIFOs in RS485 mode again
From: Paul Burton @ 2018-12-13 17:48 UTC (permalink / raw)
To: Marek Vasut
Cc: Greg Kroah-Hartman, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org
In-Reply-To: <372cccd7-a175-2737-5af8-3072606c6b1c@denx.de>
Hi Marek,
On Thu, Dec 13, 2018 at 03:51:02PM +0100, Marek Vasut wrote:
> >>>>> I wonder whether the issue may be the JZ4780 UART not automatically
> >>>>> resetting the FIFOs when FCR[0] changes. This is a guess, but the manual
> >>>>> doesn't explicitly say it'll happen & the programming example it gives
> >>>>> says to explicitly clear the FIFOs using FCR[2:1]. Since this is what
> >>>>> the kernel has been doing for at least the whole git era I wouldn't be
> >>>>> surprised if other devices are bitten by the change as people start
> >>>>> trying 4.20 on them.
> >>>>
> >>>> The patch you're complaining about is doing exactly that -- it sets
> >>>> UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT in FCR , and then clears it.
> >>>> Those two bits are exactly FCR[2:1]. It also explicitly does not touch
> >>>> any other bits in FCR.
> >>>
> >>> No - your patch does that *only* if the FIFO enable bit is set, and
> >>> presumes that clearing FIFOs is a no-op when they're disabled. I don't
> >>> believe that's true on my system. I also believe that not touching the
> >>> FIFO enable bit is problematic, since some callers clearly expect that
> >>> to be cleared.
> >>
> >> Why would you disable FIFO to clear it ? This doesn't make sense to me,
> >> the FIFO must be operational for it to be cleared. Moreover, it
> >> contradicts your previous statement, "the programming example it gives
> >> says to explicitly clear the FIFOs using FCR[2:1]" .
> >
> > No, no, not at all... I'm saying that my theory is:
> >
> > - The JZ4780 requires that the FIFO be reset using FCR[2:1] in order
> > to not read garbage.
>
> Which we do
No - we don't... We used to, but your patch stops that from happening if
the FIFO enable bit is clear.... And the FIFO enable bit is clear during
serial8250_do_startup() if the UART was used with earlycon, otherwise it
depends on state left behind by the bootloader. I don't know how many
ways I can say this.
> > - Prior to your patch serial8250_clear_fifos() did this,
> > unconditionally.
>
> btw originally, this also cleared the UME bit. Could this be what made
> the difference on the JZ4780 ?
It did not, you are wrong. serial_out() calls ingenic_uart_serial_out()
which unconditionally or's in the UME bit for writes to FCR.
> > - After your patch serial8250_clear_fifos() only clears the FIFOs if
> > the FIFO is already enabled.
>
> I'd suppose this is OK, since clearing a disabled FIFO makes no sense ?
This is where the brokenness seems to come from. As I said, this would
be fine according to the PC16550D documentation but I can say based on
experimentation that it is *not* fine on the JZ4780.
Resetting a disabled FIFO makes perfect sense if it's going to be
enabled later. That used to happen, and after your patch it doesn't.
> > - When called from serial8250_do_startup() during boot, the FIFO may
> > not be enabled - for example if the bootloader didn't use the FIFO,
> > or if the UART is used with 8250_early which zeroes FCR.
>
> If it's not enabled, do you need to clear it ?
>
> > - Therefore after your patch the FIFO reset bits are never set if the
> > UART was used with 8250_early, or if it wasn't but the bootloader
> > didn't enable the FIFO.
>
> Hence my question above, do you need to clear the FIFO even if it was
> not enabled ?
Yes.
What you asked before though was whether we need to disable the FIFO in
order to clear it, which is a different question.
> > I suspect that you get away with that because according to the PC16550D
> > documentation the FIFOs should reset when the FIFO enable bit changes,
> > so when the FIFO is later enabled it gets reset. I suspect that in the
> > JZ4780 this does not happen, and the FIFO contains garbage. Our previous
> > behaviour (always set FCR[2:1] to reset the FIFO) has been around for a
> > long time, so I would not be surprised to find other devices relying
> > upon it.
>
> It is well possible, but I'd like to understand what really happens
> here, not just guess.
I can only tell you what little the JZ4780 manual says & what actually
works when I run it on my system. I used the word supect to explain my
theory, but that's the best we can do with the documentation available.
> > I'm also saying that if the FIFOs are enabled during boot then your
> > patch will leave them enabled after serial8250_do_startup() calls
> > serial8250_clear_fifos(), which a comment in serial8250_do_startup()
> > clearly states is not the expected behaviour:
>
> In that case, that needs to be fixed. But serial8250_clear_fifos()
> should only do what the name says -- clear FIFOs -- not disable them.
...which is why I effectively renamed it
serial8250_clear_and_disable_fifos() in my suggested patch. Did you test
it? I have no OMAP3 system to check that I didn't break your setup, and
at this point if I can't be sure it works I'll be submitting a revert of
your broken patch.
> >> Clear the FIFO buffers and disable them.
> >> (they will be reenabled in set_termios())
> >
> > (From https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/8250/8250_port.c?h=v4.20-rc6#n2209)
> >
> > And further, with your patch serial8250_do_shutdown() will leave the
> > FIFO enabled which again does not match what comments suggest it expects
> > ("Disable break condition and FIFOs").
> >
> >> What exactly does your programming example for the JZ4780 say about the
> >> FIFO clearing ? And does it work in that example ?
> >
> > It says to set FCR[2:1] to reset the FIFO after enabling it, which as
> > far as I can tell from the PC16550D documentation would not be necessary
> > there because when you enable the FIFO it would automatically be reset.
> > Linux did this until your patch.
>
> Linux sets the FCR[2:1] if the FIFO is enabled even now.
Correct, but not when the FIFO is disabled in serial8250_do_startup(),
nor when the FIFO is later enabled in serial8250_do_set_termios().
> >> I just remembered U-Boot has this patch for JZ4780 UART [1], which
> >> touches the FCR UME bit, so the FCR behavior on JZ4780 does differ from
> >> the generic 8250 core. Could it be that this is what you're hitting ?
> >
> > No - we already set the UME bit & this has all worked fine until your
> > patch changed the FIFO reset behaviour.
>
> The UME bit is in the FCR, serial8250_clear_fifos() originally cleared
> it, cfr:
> - serial_out(p, UART_FCR, 0);
> in f6aa5beb45be27968a4df90176ca36dfc4363d37 . So the code was originally
> completely disabling the UART on JZ4780 .
Again, wrong. Look at what serial_out() actually does & see
ingenic_uart_serial_out(). Again, the Ingenic UARTs have worked fine
until your patch, and work with mainline after reverting it - the
problem is *entirely* with the change you made & the UME bit has nothing
to do with it.
Thanks,
Paul
^ permalink raw reply
* Re: [PATCH] tty: serial: qcom_geni_serial: Remove interrupt storm
From: Doug Anderson @ 2018-12-13 17:36 UTC (permalink / raw)
To: Ryan Case
Cc: Greg Kroah-Hartman, Jiri Slaby, LKML, linux-serial, Stephen Boyd
In-Reply-To: <20181212003522.239189-1-ryandcase@chromium.org>
Hi,
On Tue, Dec 11, 2018 at 4:36 PM Ryan Case <ryandcase@chromium.org> wrote:
>
> Disable M_TX_FIFO_WATERMARK_EN after we've sent all data for a given
> transaction so we don't continue to receive a flurry of free space
> interrupts while waiting for the M_CMD_DONE notification. Re-enable the
> watermark when establishing the next transaction.
>
> Also clear the watermark interrupt after filling the FIFO so we do not
> receive notification again prior to actually having free space.
>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
> drivers/tty/serial/qcom_geni_serial.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 6e38498362ef..965aefa54114 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -724,10 +724,12 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
> size_t pending;
> int i;
> u32 status;
> + u32 irq_en;
> unsigned int chunk;
> int tail;
>
> status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
>
> /* Complete the current tx command before taking newly added data */
> if (active)
> @@ -752,6 +754,9 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
> if (!port->tx_remaining) {
> qcom_geni_serial_setup_tx(uport, pending);
> port->tx_remaining = pending;
> +
> + irq_en |= M_TX_FIFO_WATERMARK_EN;
> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
I notice other places that turn on the watermark only do so if
"xfer_mode == GENI_SE_FIFO". ...but it looks as if the mode is always
FIFO mode, so you should be fine. Probably the right thing to do is
that someone should do a future patch to kill all the "if xfer_mode ==
GENI_SE_FIFO" stuff and if/when someone wants to add DMA then they can
do it from scratch.
> }
>
> remaining = chunk;
> @@ -775,7 +780,15 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
> }
>
> xmit->tail = tail & (UART_XMIT_SIZE - 1);
> + writel_relaxed(M_TX_FIFO_WATERMARK_EN,
> + uport->membase + SE_GENI_M_IRQ_CLEAR);
Worth a comment saying that the watermark is "level-triggered and
latched" so it's obvious why it's not a race (because it will just
re-assert itself) and also why we need to ACK it at the end of the
function (because it'll keep re-latching itself until you put
something in the FIFO)?
> +
> out_write_wakeup:
> + if (!port->tx_remaining) {
> + irq_en &= ~M_TX_FIFO_WATERMARK_EN;
> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
I think you should change this (and probably the other write to
irq_en) to a read/modify/write rather than using the value you read at
the top of the function. Specifically there are enough helper
functions called that might also be touching irq_en and I'd be worried
that the value you read at the top of the function might not be truth
anymore. I also wonder if it's worth an "if" test to only do the
writel_relaxed() if the value wasn't already right since sometimes IO
writes can be slow.
If I followed the code correctly, doing the read/modify/write actually
might matter. The code calls qcom_geni_serial_stop_tx() which _does_
modify irq_en. Then the code does "goto out_write_wakeup" where it'll
clobber qcom_geni_serial_stop_tx()'s modifications.
> + }
> +
> if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> uart_write_wakeup(uport);
> }
> @@ -811,8 +824,7 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
> tty_insert_flip_char(tport, 0, TTY_OVERRUN);
> }
>
> - if (m_irq_status & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN) &&
> - m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
> + if (m_irq_status & m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
Unrelated to everything else in this patch, but seems OK to me.
-Doug
^ permalink raw reply
* Re: [PATCH] serial: 8250: Fix clearing FIFOs in RS485 mode again
From: Marek Vasut @ 2018-12-13 14:51 UTC (permalink / raw)
To: Paul Burton
Cc: Greg Kroah-Hartman, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org
In-Reply-To: <20181213051154.57h2ddfcbrgh65gy@pburton-laptop>
On 12/13/2018 06:11 AM, Paul Burton wrote:
> Hi Marek,
Hi,
> On Thu, Dec 13, 2018 at 05:18:19AM +0100, Marek Vasut wrote:
>>>>> I wonder whether the issue may be the JZ4780 UART not automatically
>>>>> resetting the FIFOs when FCR[0] changes. This is a guess, but the manual
>>>>> doesn't explicitly say it'll happen & the programming example it gives
>>>>> says to explicitly clear the FIFOs using FCR[2:1]. Since this is what
>>>>> the kernel has been doing for at least the whole git era I wouldn't be
>>>>> surprised if other devices are bitten by the change as people start
>>>>> trying 4.20 on them.
>>>>
>>>> The patch you're complaining about is doing exactly that -- it sets
>>>> UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT in FCR , and then clears it.
>>>> Those two bits are exactly FCR[2:1]. It also explicitly does not touch
>>>> any other bits in FCR.
>>>
>>> No - your patch does that *only* if the FIFO enable bit is set, and
>>> presumes that clearing FIFOs is a no-op when they're disabled. I don't
>>> believe that's true on my system. I also believe that not touching the
>>> FIFO enable bit is problematic, since some callers clearly expect that
>>> to be cleared.
>>
>> Why would you disable FIFO to clear it ? This doesn't make sense to me,
>> the FIFO must be operational for it to be cleared. Moreover, it
>> contradicts your previous statement, "the programming example it gives
>> says to explicitly clear the FIFOs using FCR[2:1]" .
>
> No, no, not at all... I'm saying that my theory is:
>
> - The JZ4780 requires that the FIFO be reset using FCR[2:1] in order
> to not read garbage.
Which we do
> - Prior to your patch serial8250_clear_fifos() did this,
> unconditionally.
btw originally, this also cleared the UME bit. Could this be what made
the difference on the JZ4780 ?
Can you try this patch on the JZ4780 , just out of curiosity ?
diff --git a/drivers/tty/serial/8250/8250_port.c
b/drivers/tty/serial/8250/8250_port.c
index c39482b961110..3dce99f4c6802 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -553,7 +553,7 @@ 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;
+ unsigned char clr_mask = UART_FCR_CLEAR_RCVR |
UART_FCR_CLEAR_XMIT | BIT(4) /* UME */;
if (p->capabilities & UART_CAP_FIFO) {
> - After your patch serial8250_clear_fifos() only clears the FIFOs if
> the FIFO is already enabled.
I'd suppose this is OK, since clearing a disabled FIFO makes no sense ?
> - When called from serial8250_do_startup() during boot, the FIFO may
> not be enabled - for example if the bootloader didn't use the FIFO,
> or if the UART is used with 8250_early which zeroes FCR.
If it's not enabled, do you need to clear it ?
> - Therefore after your patch the FIFO reset bits are never set if the
> UART was used with 8250_early, or if it wasn't but the bootloader
> didn't enable the FIFO.
Hence my question above, do you need to clear the FIFO even if it was
not enabled ?
> I suspect that you get away with that because according to the PC16550D
> documentation the FIFOs should reset when the FIFO enable bit changes,
> so when the FIFO is later enabled it gets reset. I suspect that in the
> JZ4780 this does not happen, and the FIFO contains garbage. Our previous
> behaviour (always set FCR[2:1] to reset the FIFO) has been around for a
> long time, so I would not be surprised to find other devices relying
> upon it.
It is well possible, but I'd like to understand what really happens
here, not just guess.
> I'm also saying that if the FIFOs are enabled during boot then your
> patch will leave them enabled after serial8250_do_startup() calls
> serial8250_clear_fifos(), which a comment in serial8250_do_startup()
> clearly states is not the expected behaviour:
In that case, that needs to be fixed. But serial8250_clear_fifos()
should only do what the name says -- clear FIFOs -- not disable them.
>> Clear the FIFO buffers and disable them.
>> (they will be reenabled in set_termios())
>
> (From https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/8250/8250_port.c?h=v4.20-rc6#n2209)
>
> And further, with your patch serial8250_do_shutdown() will leave the
> FIFO enabled which again does not match what comments suggest it expects
> ("Disable break condition and FIFOs").
>
>> What exactly does your programming example for the JZ4780 say about the
>> FIFO clearing ? And does it work in that example ?
>
> It says to set FCR[2:1] to reset the FIFO after enabling it, which as
> far as I can tell from the PC16550D documentation would not be necessary
> there because when you enable the FIFO it would automatically be reset.
> Linux did this until your patch.
Linux sets the FCR[2:1] if the FIFO is enabled even now.
>>>>> @@ -558,25 +558,26 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>>>>> 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.
>>>>> + * 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);
>>>>> + serial_out(p, UART_FCR, fcr | clr_mask);
>>>>> + serial_out(p, UART_FCR, fcr & ~clr);
>>>>
>>>> Note that, if I understand the patch correctly, this will _not_ restore
>>>> the original (broken) behavior. The original behavior cleared the entire
>>>> FCR at the end, which cleared even bits that were not supposed to be
>>>> cleared .
>>>
>>> It will restore the original behaviour with regards to disabling the
>>> FIFOs, so long as callers that expect that use
>>> serial8250_clear_and_disable_fifos().
>>
>> Does your platform need the FIFO to be explicitly disabled for it to be
>> cleared, can you confirm that ? And if so, does this apply to other
>> platforms with 8250 UART or is this specific to JZ4780 implementation of
>> the UART block ?
>>
>> I just remembered U-Boot has this patch for JZ4780 UART [1], which
>> touches the FCR UME bit, so the FCR behavior on JZ4780 does differ from
>> the generic 8250 core. Could it be that this is what you're hitting ?
>
> No - we already set the UME bit & this has all worked fine until your
> patch changed the FIFO reset behaviour.
The UME bit is in the FCR, serial8250_clear_fifos() originally cleared
it, cfr:
- serial_out(p, UART_FCR, 0);
in f6aa5beb45be27968a4df90176ca36dfc4363d37 . So the code was originally
completely disabling the UART on JZ4780 .
--
Best regards,
Marek Vasut
^ permalink raw reply related
* Re: [PATCH v5 1/2] dmaengine: 8250_mtk_dma: add Mediatek uart DMA support
From: Long Cheng @ 2018-12-13 11:36 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: <CAGp9LzqB=hzkOHZqa-ZzRLcJujU--7gxYZczXiEj3DiH+FmhbQ@mail.gmail.com>
On Tue, 2018-12-11 at 15:12 -0800, Sean Wang wrote:
> Sorry for that I didn't have a full review at one time in the earlier version
>
> On Mon, Dec 10, 2018 at 9:37 PM Long Cheng
> <long.cheng@mediatek.com> wrote:
> >
> > In DMA engine framework, add 8250 mtk dma to support it.
>
> It looks like there are still many rooms to improve the description,
> especially it's a totally new driver.
>
> >
> > Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> > ---
> > drivers/dma/mediatek/8250_mtk_dma.c | 830 +++++++++++++++++++++++++++++++++++
> > drivers/dma/mediatek/Kconfig | 11 +
> > drivers/dma/mediatek/Makefile | 1 +
> > 3 files changed, 842 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..f79d180
> > --- /dev/null
> > +++ b/drivers/dma/mediatek/8250_mtk_dma.c
> > @@ -0,0 +1,830 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Mediatek 8250 DMA driver.
>
> MediaTek
>
> > + *
> > + * 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_APDMA_DEFAULT_REQUESTS 127
> > +#define MTK_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_DMA_RING_SIZE 0xffffU
> > +/* invert this bit when wrap ring head again*/
> > +#define MTK_DMA_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_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.
> [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];
> > +};
> > +
> > +struct mtk_chan {
> > + struct virt_dma_chan vc;
> > + struct list_head node;
> > + struct dma_slave_config cfg;
> > + void __iomem *base;
> > + struct mtk_dma_desc *desc;
> > +
> > + bool stop;
> > + bool requested;
> > +
> > + unsigned int rx_status;
> > +};
> > +
> > +struct mtk_dma_sg {
> > + dma_addr_t addr;
> > + unsigned int en; /* number of elements (24-bit) */
> > + unsigned int fn; /* number of frames (16-bit) */
> > +};
> > +
> > +struct mtk_dma_desc {
> > + struct virt_dma_desc vd;
> > + enum dma_transfer_direction dir;
> > +
> > + unsigned int sglen;
> > + struct mtk_dma_sg sg[0];
> > +
> > + unsigned int len;
> > +};
> > +
> > +static inline struct mtk_dmadev *to_mtk_dma_dev(struct dma_device *d)
> > +{
> > + return container_of(d, struct mtk_dmadev, ddev);
> > +}
> > +
> > +static inline struct mtk_chan *to_mtk_dma_chan(struct dma_chan *c)
> > +{
> > + return container_of(c, struct mtk_chan, vc.chan);
> > +}
> > +
> > +static inline struct mtk_dma_desc *to_mtk_dma_desc
> > + (struct dma_async_tx_descriptor *t)
> > +{
> > + return container_of(t, struct mtk_dma_desc, vd.tx);
> > +}
> > +
> > +static void mtk_dma_chan_write(struct mtk_chan *c,
> > + unsigned int reg, unsigned int val)
> > +{
> > + writel(val, c->base + reg);
> > +}
> > +
> > +static unsigned int mtk_dma_chan_read(struct mtk_chan *c, unsigned int reg)
> > +{
> > + return readl(c->base + reg);
> > +}
> > +
> > +static void mtk_dma_desc_free(struct virt_dma_desc *vd)
> > +{
> > + struct dma_chan *chan = vd->tx.chan;
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > +
> > + kfree(c->desc);
> > + c->desc = NULL;
> > +}
> > +
> > +static void mtk_dma_tx_flush(struct dma_chan *chan)
> > +{
>
> If the user is only one, let's span the content into where the user is.
>
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > +
> > + if (mtk_dma_chan_read(c, VFF_FLUSH) == 0U)
> > + mtk_dma_chan_write(c, VFF_FLUSH, VFF_FLUSH_B);
> > +}
> > +
> > +static void mtk_dma_tx_write(struct dma_chan *chan)
> > +{
>
> If the user is only one, let's span the content into where the user is.
>
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + unsigned int txcount = c->desc->len;
> > + unsigned int len, send, left, wpt, wrap;
> > +
> > + len = mtk_dma_chan_read(c, VFF_LEN);
> > +
> > + while ((left = mtk_dma_chan_read(c, VFF_LEFT_SIZE)) > 0U) {
> > + if (c->desc->len == 0U)
>
> merge the condition back into the condition in while
>
> > + break;
> > + send = min_t(unsigned int, left, c->desc->len);
> > + wpt = mtk_dma_chan_read(c, VFF_WPT);
> > + wrap = wpt & MTK_DMA_RING_WRAP ? 0U : MTK_DMA_RING_WRAP;
> > +
> > + if ((wpt & (len - 1U)) + send < len)
> > + mtk_dma_chan_write(c, VFF_WPT, wpt + send);
> > + else
> > + mtk_dma_chan_write(c, VFF_WPT,
> > + ((wpt + send) & (len - 1U))
> > + | wrap);
> > +
> > + c->desc->len -= send;
>
> ->len can be renamed to ->avail_len to say it's variable during the work
>
> > + }
> > +
> > + if (txcount != c->desc->len) {
> > + mtk_dma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > + mtk_dma_tx_flush(chan);
> > + }
> > +}
> > +
> > +static void mtk_dma_start_tx(struct mtk_chan *c)
> > +{
> > + if (mtk_dma_chan_read(c, VFF_LEFT_SIZE) == 0U)
> > + mtk_dma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> > + else
> > + mtk_dma_tx_write(&c->vc.chan);
> > +
> > + c->stop = false;
> > +}
> > +
> > +static void mtk_dma_get_rx_size(struct mtk_chan *c)
> > +{
>
> If the user is only one, let's span the content into where the user is.
>
> > + unsigned int rx_size = mtk_dma_chan_read(c, VFF_LEN);
> > + unsigned int rdptr, wrptr, wrreg, rdreg, count;
>
> too much variable seems a little lousy, two variables are enough
>
> unsigned int rd, wr;
>
> > +
> > + rdreg = mtk_dma_chan_read(c, VFF_RPT);
> > + wrreg = mtk_dma_chan_read(c, VFF_WPT);
> > + rdptr = rdreg & MTK_DMA_RING_SIZE;
> > + wrptr = wrreg & MTK_DMA_RING_SIZE;
>
> rd = mtk_dma_chan_read(c, VFF_RPT) & MTK_DMA_RING_SIZE;
> wr = mtk_dma_chan_read(c, VFF_WPT) & MTK_DMA_RING_SIZE
>
> > + count = ((rdreg ^ wrreg) & MTK_DMA_RING_WRAP) ?
> > + (wrptr + rx_size - rdptr) : (wrptr - rdptr);
> > +
> > + c->rx_status = count;
>
> drop the variable count and have a direct assignment
>
> > +
> > + mtk_dma_chan_write(c, VFF_RPT, wrreg);
> > +}
> > +
> > +static void mtk_dma_start_rx(struct mtk_chan *c)
> > +{
> > + struct dma_chan *chan = &c->vc.chan;
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_dma_desc *d = c->desc;
> > +
> > + if (mtk_dma_chan_read(c, VFF_VALID_SIZE) == 0U)
> > + return;
> > +
> > + if (d && vchan_next_desc(&c->vc)) {
> > + mtk_dma_get_rx_size(c);
> > + list_del(&d->vd.node);
> > + vchan_cookie_complete(&d->vd);
> > + } else {
> > + spin_lock(&mtkd->lock);
> > + if (list_empty(&mtkd->pending))
> > + list_add_tail(&c->node, &mtkd->pending);
> > + spin_unlock(&mtkd->lock);
> > + tasklet_schedule(&mtkd->task);
> > + }
> > +}
> > +
> > +static void mtk_dma_reset(struct mtk_chan *c)
> > +{
>
> If the user is only one, let's span the content into where the user is.
>
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(c->vc.chan.device);
> > + u32 status;
> > + int ret;
> > +
> > + mtk_dma_chan_write(c, VFF_ADDR, 0);
> > + mtk_dma_chan_write(c, VFF_THRE, 0);
> > + mtk_dma_chan_write(c, VFF_LEN, 0);
> > + mtk_dma_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");
> > + return;
> > + }
> > +
> > + if (c->cfg.direction == DMA_DEV_TO_MEM)
> > + mtk_dma_chan_write(c, VFF_RPT, 0);
> > + else if (c->cfg.direction == DMA_MEM_TO_DEV)
> > + mtk_dma_chan_write(c, VFF_WPT, 0);
>
> using switch and case statement
>
> > +
> > + if (mtkd->support_33bits)
> > + mtk_dma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
> > +}
> > +
> > +static void mtk_dma_stop(struct mtk_chan *c)
>
> If the user is only one, let's span the content into where the user is.
>
> > +{
> > + u32 status;
> > + int ret;
> > +
> > + mtk_dma_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_dma_chan_read(c, VFF_DEBUG_STATUS));
> > +
> > + /*set stop as 1 -> wait until en is 0 -> set stop as 0*/
> > + mtk_dma_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_dma_chan_read(c, VFF_DEBUG_STATUS));
> > +
> > + mtk_dma_chan_write(c, VFF_STOP, VFF_STOP_CLR_B);
> > + mtk_dma_chan_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> > +
> > + if (c->cfg.direction == DMA_DEV_TO_MEM)
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > + else
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
>
> using switch and case statement
>
> > +
> > + c->stop = true;
> > +}
> > +
> > +/*
> > + * This callback schedules all pending channels. We could be more
> > + * clever here by postponing allocation of the real DMA channels to
> > + * this point, and freeing them when our virtual channel becomes idle.
> > + *
> > + * We would then need to deal with 'all channels in-use'
> > + */
> > +static void mtk_dma_sched(unsigned long data)
> > +{
>
> As at the initial be said, try to make descriptors submit as fast as
> possible without involving in a tasklet. The same improvement had been
> done at mtk-hsdma.c so you could have a reference to it first if you
> have no much idea of how to begin to improve.
>
> > + struct mtk_dmadev *mtkd = (struct mtk_dmadev *)data;
> > + struct virt_dma_desc *vd;
> > + struct mtk_chan *c;
> > + unsigned long flags;
> > + LIST_HEAD(head);
> > +
> > + spin_lock_irq(&mtkd->lock);
> > + list_splice_tail_init(&mtkd->pending, &head);
> > + spin_unlock_irq(&mtkd->lock);
> > +
> > + if (!list_empty(&head)) {
> > + c = list_first_entry(&head, struct mtk_chan, node);
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> > + list_del_init(&c->node);
> > + mtk_dma_start_rx(c);
> > + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> > + vd = vchan_next_desc(&c->vc);
> > + c->desc = to_mtk_dma_desc(&vd->tx);
> > + list_del_init(&c->node);
> > + mtk_dma_start_tx(c);
> > + }
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > + }
> > +}
> > +
> > +static int mtk_dma_alloc_chan_resources(struct dma_chan *chan)
> > +{
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + int ret = -EBUSY;
> > +
> > + pm_runtime_get_sync(mtkd->ddev.dev);
> > +
> > + if (!mtkd->ch[chan->chan_id]) {
> > + c->base = mtkd->mem_base[chan->chan_id]
>
> mtkd->mem_base is unnecessary, we can directly decide c->base in the
> driver probe stage
>
> > + mtkd->ch[chan->chan_id] = c;
>
> mtkd->ch is also unnecessary, the core always pass struct dma_chan
> *chan to each callback function
>
> > + ret = 1;
>
> ret be 1 seems be wrong
>
> > + }
> > + c->requested = false;
> > + mtk_dma_reset(c);
> > +
> > + return ret;
> > +}
> > +
> > +static void mtk_dma_free_chan_resources(struct dma_chan *chan)
> > +{
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > +
> > + if (c->requested) {
> > + c->requested = false;
> > + free_irq(mtkd->dma_irq[chan->chan_id], chan);
>
> it makes not consistent because there are not request_irq present at
> mtk_dma_alloc_chan_resources. but I'd prefer a devm_request_irq is
> being done
> as the driver got probe.
>
> > + }
> > +
> > + tasklet_kill(&mtkd->task);
> > + tasklet_kill(&c->vc.task);
> > +
> > + c->base = NULL;
> > + mtkd->ch[chan->chan_id] = NULL;
> > + vchan_free_chan_resources(&c->vc);
> > +
> > + pm_runtime_put_sync(mtkd->ddev.dev);
> > +}
> > +
> > +static enum dma_status mtk_dma_tx_status(struct dma_chan *chan,
> > + dma_cookie_t cookie,
> > + struct dma_tx_state *txstate)
> > +{
> > + struct mtk_chan *c = to_mtk_dma_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_dma_chan_read(c, VFF_RPT)
> > + & MTK_DMA_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;
> > +}
> > +
> > +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.
> > + 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
>
> > + if (vchan_issue_pending(&c->vc) && !c->desc) {
> > + vd = vchan_next_desc(&c->vc);
> > + c->desc = to_mtk_dma_desc(&vd->tx);
> > + }
> > + } 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_dma_desc(&vd->tx);
> > + mtk_dma_start_tx(c);
> > + }
> > + }
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +}
> > +
> > +static irqreturn_t mtk_dma_rx_interrupt(int irq, void *dev_id)
> > +{
> > + struct dma_chan *chan = (struct dma_chan *)dev_id;
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > +
> > + mtk_dma_start_rx(c);
> > +
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t mtk_dma_tx_interrupt(int irq, void *dev_id)
> > +{
> > + struct dma_chan *chan = (struct dma_chan *)dev_id;
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + struct mtk_dma_desc *d = c->desc;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (d->len != 0U) {
> > + list_add_tail(&c->node, &mtkd->pending);
> > + tasklet_schedule(&mtkd->task);
> > + } else {
> > + list_del(&d->vd.node);
> > + vchan_cookie_complete(&d->vd);
> > + }
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +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.
> > +
> > + 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.
> > +
> > + 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.
> > + 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.
> > +
> > +static int mtk_dma_device_resume(struct dma_chan *chan)
> > +{
> > + /* just for check caps pass */
> > + return -EINVAL;
>
> similar to the above
>
> > +}
> > +
> > +static void mtk_dma_free(struct mtk_dmadev *mtkd)
> > +{
> > + tasklet_kill(&mtkd->task);
> > + while (list_empty(&mtkd->ddev.channels) == 0) {
> !list_empty(&mtkd->ddev.channels)
> > + 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);
> > + devm_kfree(mtkd->ddev.dev, c);
>
> no need to call devm_kfree, the core would help do this
>
> > + }
> > +}
> > +
> > +static const struct of_device_id mtk_uart_dma_match[] = {
> > + { .compatible = "mediatek,mt6577-uart-dma", },
> > + { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_uart_dma_match);
> > +
> > +static int mtk_apdma_probe(struct platform_device *pdev)
> > +{
> > + struct mtk_dmadev *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;
> > +
> > + for (i = 0; i < MTK_APDMA_CHANNELS; i++) {
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > + if (!res)
> > + return -ENODEV;
> > + mtkd->mem_base[i] = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(mtkd->mem_base[i]))
> > + return PTR_ERR(mtkd->mem_base[i]);
> > + }
> > +
> > + for (i = 0; i < MTK_APDMA_CHANNELS; i++) {
> > + 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);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + mtkd->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(mtkd->clk)) {
> > + dev_err(&pdev->dev, "No clock specified\n");
> > + return PTR_ERR(mtkd->clk);
> > + }
> > +
> > + if (of_property_read_bool(pdev->dev.of_node, "dma-33bits")) {
> > + dev_info(&pdev->dev, "Support dma 33bits\n");
> > + mtkd->support_33bits = true;
> > + }
> > +
> > + if (mtkd->support_33bits)
> > + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(33));
> > + else
> > + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>
> rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32 |
> mtkd->support_33bits));
>
> > + if (rc)
> > + return rc;
> > +
> > + dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
> > + mtkd->ddev.device_alloc_chan_resources = mtk_dma_alloc_chan_resources;
> > + mtkd->ddev.device_free_chan_resources = mtk_dma_free_chan_resources;
> > + mtkd->ddev.device_tx_status = mtk_dma_tx_status;
> > + mtkd->ddev.device_issue_pending = mtk_dma_issue_pending;
> > + mtkd->ddev.device_prep_slave_sg = mtk_dma_prep_slave_sg;
> > + mtkd->ddev.device_config = mtk_dma_slave_config;
> > + mtkd->ddev.device_pause = mtk_dma_device_pause;
> > + mtkd->ddev.device_resume = mtk_dma_device_resume;
> > + mtkd->ddev.device_terminate_all = mtk_dma_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);
> > + INIT_LIST_HEAD(&mtkd->pending);
> > +
> > + spin_lock_init(&mtkd->lock);
> > + tasklet_init(&mtkd->task, mtk_dma_sched, (unsigned long)mtkd);
> > +
> > + mtkd->dma_requests = MTK_APDMA_DEFAULT_REQUESTS;
> > + if (of_property_read_u32(pdev->dev.of_node,
> > + "dma-requests", &mtkd->dma_requests)) {
> > + dev_info(&pdev->dev,
> > + "Missing dma-requests property, using %u.\n",
> > + MTK_APDMA_DEFAULT_REQUESTS);
> > + }
> > +
> > + for (i = 0; i < MTK_APDMA_CHANNELS; i++) {
> > + c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
> > + if (!c)
> > + goto err_no_dma;
> > +
> > + c->vc.desc_free = mtk_dma_desc_free;
> > + vchan_init(&c->vc, &mtkd->ddev);
> > + INIT_LIST_HEAD(&c->node);
> > + }
> > +
> > + 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_dma_free(mtkd);
> > + return rc;
> > +}
> > +
> > +static int mtk_apdma_remove(struct platform_device *pdev)
> > +{
> > + struct mtk_dmadev *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_dma_free(mtkd);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int mtk_dma_suspend(struct device *dev)
> > +{
> > + struct mtk_dmadev *mtkd = dev_get_drvdata(dev);
> > +
> > + if (!pm_runtime_suspended(dev))
> > + clk_disable_unprepare(mtkd->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_dma_resume(struct device *dev)
> > +{
> > + int ret;
> > + struct mtk_dmadev *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_dma_runtime_suspend(struct device *dev)
> > +{
> > + struct mtk_dmadev *mtkd = dev_get_drvdata(dev);
> > +
> > + clk_disable_unprepare(mtkd->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_dma_runtime_resume(struct device *dev)
> > +{
> > + int ret;
> > + struct mtk_dmadev *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_dma_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mtk_dma_suspend, mtk_dma_resume)
> > + SET_RUNTIME_PM_OPS(mtk_dma_runtime_suspend,
> > + mtk_dma_runtime_resume, NULL)
> > +};
> > +
> > +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.
> > + .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] tty: serial: don't do termios for BTIF
From: Russell King - ARM Linux @ 2018-12-13 11:00 UTC (permalink / raw)
To: Ryder Lee
Cc: Andy Shevchenko, Greg Kroah-Hartman, Sean Wang, Sean Wang,
Weijie Gao, linux-kernel, Roy Luo, linux-mediatek, linux-serial,
Matthias Brugger, linux-arm-kernel
In-Reply-To: <972976519be8e28ee600651fe14d1f6d5f23e697.1544679557.git.ryder.lee@mediatek.com>
On Thu, Dec 13, 2018 at 02:24:03PM +0800, Ryder Lee wrote:
> The MediaTek BTIF controller doesn't need to set termios so add
> a new capability 'UART_CAP_NMOD' for the unsupported case.
>
>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
> drivers/tty/serial/8250/8250.h | 1 +
> drivers/tty/serial/8250/8250_port.c | 7 ++++++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index ebfb0bd..2b9ba39 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -80,6 +80,7 @@ struct serial8250_config {
> #define UART_CAP_MINI (1 << 17) /* Mini UART on BCM283X family lacks:
> * STOP PARITY EPAR SPAR WLEN5 WLEN6
> */
> +#define UART_CAP_NMOD (1 << 18) /* UART doesn't do termios */
>
> #define UART_BUG_QUOT (1 << 0) /* UART has buggy quot LSB */
> #define UART_BUG_TXEN (1 << 1) /* UART has buggy TX IIR status */
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index c39482b..e4a69a0 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -297,7 +297,7 @@
> .tx_loadsz = 16,
> .fcr = UART_FCR_ENABLE_FIFO |
> UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT,
> - .flags = UART_CAP_FIFO,
> + .flags = UART_CAP_FIFO | UART_CAP_NMOD,
> },
> [PORT_NPCM] = {
> .name = "Nuvoton 16550",
> @@ -2663,6 +2663,11 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port,
> unsigned long flags;
> unsigned int baud, quot, frac = 0;
>
> + if (up->capabilities & UART_CAP_NMOD) {
> + termios->c_cflag = 0;
So your port is limited to 5-bits and is unable to receive characters?
That's what a zero cflag means, and that's what you'll be telling
userspace here.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH] tty/serial: do not free trasnmit buffer page under port lock
From: Petr Mladek @ 2018-12-13 9:42 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Sergey Senozhatsky, Andrew Morton, Peter Zijlstra, Waiman Long,
Dmitry Safonov, Steven Rostedt
In-Reply-To: <20181213045839.9692-1-sergey.senozhatsky@gmail.com>
On Thu 2018-12-13 13:58:39, Sergey Senozhatsky wrote:
> LKP has hit yet another circular locking dependency between uart
> console drivers and debugobjects [1]:
>
> The patch fixes transmit buffer page free() in uart_shutdown() and,
> additionally, in uart_port_startup() (as was suggested by Dmitry Safonov).
>
> [1] https://lore.kernel.org/lkml/20181211091154.GL23332@shao2-debian/T/#u
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH] tty/serial: do not free trasnmit buffer page under port lock
From: Peter Zijlstra @ 2018-12-13 9:17 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Sergey Senozhatsky, Andrew Morton, Waiman Long, Dmitry Safonov,
Steven Rostedt, Petr Mladek
In-Reply-To: <20181213045839.9692-1-sergey.senozhatsky@gmail.com>
On Thu, Dec 13, 2018 at 01:58:39PM +0900, Sergey Senozhatsky wrote:
> LKP has hit yet another circular locking dependency between uart
> console drivers and debugobjects [1]:
>
> CPU0 CPU1
>
> rhltable_init()
> __init_work()
> debug_object_init
> uart_shutdown() /* db->lock */
> /* uart_port->lock */ debug_print_object()
> free_page() printk()
> call_console_drivers()
> debug_check_no_obj_freed() /* uart_port->lock */
> /* db->lock */
> debug_print_object()
>
> So there are two dependency chains:
> uart_port->lock -> db->lock
> And
> db->lock -> uart_port->lock
>
> This particular circular locking dependency can be addressed in several
> ways:
>
> a) One way would be to move debug_print_object() out of db->lock scope
> and, thus, break the db->lock -> uart_port->lock chain.
> b) Another one would be to free() transmit buffer page out of db->lock
> in UART code; which is what this patch does.
>
> It makes sense to apply a) and b) independently: there are too many things
> going on behind free(), none of which depend on uart_port->lock.
>
> The patch fixes transmit buffer page free() in uart_shutdown() and,
> additionally, in uart_port_startup() (as was suggested by Dmitry Safonov).
>
> [1] https://lore.kernel.org/lkml/20181211091154.GL23332@shao2-debian/T/#u
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Waiman Long <longman@redhat.com>
> Cc: Dmitry Safonov <dima@arista.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Petr Mladek <pmladek@suse.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> drivers/tty/serial/serial_core.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c439a5a1e6c0..d4cca5bdaf1c 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -205,10 +205,15 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
> if (!state->xmit.buf) {
> state->xmit.buf = (unsigned char *) page;
> uart_circ_clear(&state->xmit);
> + uart_port_unlock(uport, flags);
> } else {
> + uart_port_unlock(uport, flags);
> + /*
> + * Do not free() the page under the port lock, see
> + * uart_shutdown().
> + */
> free_page(page);
> }
> - uart_port_unlock(uport, flags);
>
> retval = uport->ops->startup(uport);
> if (retval == 0) {
> @@ -268,6 +273,7 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
> struct uart_port *uport = uart_port_check(state);
> struct tty_port *port = &state->port;
> unsigned long flags = 0;
> + char *xmit_buf = NULL;
>
> /*
> * Set the TTY IO error marker
> @@ -298,14 +304,18 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
> tty_port_set_suspended(port, 0);
>
> /*
> - * Free the transmit buffer page.
> + * Do not free() the transmit buffer page under the port lock since
> + * this can create various circular locking scenarios. For instance,
> + * console driver may need to allocate/free a debug object, which
> + * can endup in printk() recursion.
> */
> uart_port_lock(state, flags);
> - if (state->xmit.buf) {
> - free_page((unsigned long)state->xmit.buf);
> - state->xmit.buf = NULL;
> - }
> + xmit_buf = state->xmit.buf;
> + state->xmit.buf = NULL;
> uart_port_unlock(uport, flags);
> +
> + if (xmit_buf)
> + free_page((unsigned long)xmit_buf);
> }
>
> /**
> --
> 2.20.0
>
^ permalink raw reply
* [PATCH] tty: serial: don't do termios for BTIF
From: Ryder Lee @ 2018-12-13 6:24 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman
Cc: Sean Wang, Matthias Brugger, Weijie Gao, Roy Luo, linux-serial,
linux-kernel, linux-arm-kernel, linux-mediatek, Ryder Lee,
Sean Wang
The MediaTek BTIF controller doesn't need to set termios so add
a new capability 'UART_CAP_NMOD' for the unsupported case.
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
drivers/tty/serial/8250/8250.h | 1 +
drivers/tty/serial/8250/8250_port.c | 7 ++++++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ebfb0bd..2b9ba39 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -80,6 +80,7 @@ struct serial8250_config {
#define UART_CAP_MINI (1 << 17) /* Mini UART on BCM283X family lacks:
* STOP PARITY EPAR SPAR WLEN5 WLEN6
*/
+#define UART_CAP_NMOD (1 << 18) /* UART doesn't do termios */
#define UART_BUG_QUOT (1 << 0) /* UART has buggy quot LSB */
#define UART_BUG_TXEN (1 << 1) /* UART has buggy TX IIR status */
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index c39482b..e4a69a0 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -297,7 +297,7 @@
.tx_loadsz = 16,
.fcr = UART_FCR_ENABLE_FIFO |
UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT,
- .flags = UART_CAP_FIFO,
+ .flags = UART_CAP_FIFO | UART_CAP_NMOD,
},
[PORT_NPCM] = {
.name = "Nuvoton 16550",
@@ -2663,6 +2663,11 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port,
unsigned long flags;
unsigned int baud, quot, frac = 0;
+ if (up->capabilities & UART_CAP_NMOD) {
+ termios->c_cflag = 0;
+ return;
+ }
+
if (up->capabilities & UART_CAP_MINI) {
termios->c_cflag &= ~(CSTOPB | PARENB | PARODD | CMSPAR);
if ((termios->c_cflag & CSIZE) == CS5 ||
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v5 2/2] arm: dts: mt2712: add uart APDMA to device tree
From: Yingjoe Chen @ 2018-12-13 6:18 UTC (permalink / raw)
To: Long Cheng
Cc: Vinod Koul, Rob Herring, Mark Rutland, Ryder Lee,
Matthias Brugger, Dan Williams, Greg Kroah-Hartman, Jiri Slaby,
Sean Wang, Sean Wang, dmaengine, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, linux-serial, srv_heupstream,
YT Shen
In-Reply-To: <1544506645-27979-3-git-send-email-long.cheng@mediatek.com>
On Tue, 2018-12-11 at 13:37 +0800, Long Cheng wrote:
> 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..a59728b 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";
> };
>
> @@ -378,6 +381,38 @@
> status = "disabled";
> };
>
> + apdma: dma-controller@11000400 {
> + compatible = "mediatek,mt2712-uart-dma",
> + "mediatek,mt6577-uart-dma";
Sorting, please make sure this is before
auxadc: adc@11001000 {
> + 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>;
> + };
> +
> uart0: serial@11002000 {
> compatible = "mediatek,mt2712-uart",
> "mediatek,mt6577-uart";
> @@ -385,6 +420,9 @@
...deleted
^ permalink raw reply
* Re: [LKP] [tty] c96cf923a9: WARNING:possible_circular_locking_dependency_detected
From: Sergey Senozhatsky @ 2018-12-13 5:38 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Sergey Senozhatsky, Dmitry Safonov, Jiri Slaby, kernel test robot,
lkp, Greg Kroah-Hartman, Mikulas Patocka, LKML, linux-serial,
Sergey Senozhatsky, Petr Mladek, Steven Rostedt, Waiman Long
In-Reply-To: <20181212090751.GU5289@hirez.programming.kicks-ass.net>
On (12/12/18 10:07), Peter Zijlstra wrote:
> > + uart_port_lock(state, flags);
> > + xmit_buf = state->xmit.buf;
> > + state->xmit.buf = NULL;
> > + uart_port_unlock(uport, flags);
> > +
> > /*
> > * Free the transmit buffer page.
> > */
> > - uart_port_lock(state, flags);
> > - if (state->xmit.buf) {
> > - free_page((unsigned long)state->xmit.buf);
> > - state->xmit.buf = NULL;
> > - }
> > - uart_port_unlock(uport, flags);
> > + if (xmit_buf)
> > + free_page((unsigned long)xmit_buf);
> > }
>
> That looks sensible anyhow. One should strive to not do alloc or free
> under locks as much as possible anyway.
Right, good point.
-ss
^ permalink raw reply
* Re: [PATCH] serial: 8250: Fix clearing FIFOs in RS485 mode again
From: Paul Burton @ 2018-12-13 5:11 UTC (permalink / raw)
To: Marek Vasut
Cc: Greg Kroah-Hartman, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org
In-Reply-To: <b8525991-f539-a180-2e88-a70b29319413@denx.de>
Hi Marek,
On Thu, Dec 13, 2018 at 05:18:19AM +0100, Marek Vasut wrote:
> >>> I wonder whether the issue may be the JZ4780 UART not automatically
> >>> resetting the FIFOs when FCR[0] changes. This is a guess, but the manual
> >>> doesn't explicitly say it'll happen & the programming example it gives
> >>> says to explicitly clear the FIFOs using FCR[2:1]. Since this is what
> >>> the kernel has been doing for at least the whole git era I wouldn't be
> >>> surprised if other devices are bitten by the change as people start
> >>> trying 4.20 on them.
> >>
> >> The patch you're complaining about is doing exactly that -- it sets
> >> UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT in FCR , and then clears it.
> >> Those two bits are exactly FCR[2:1]. It also explicitly does not touch
> >> any other bits in FCR.
> >
> > No - your patch does that *only* if the FIFO enable bit is set, and
> > presumes that clearing FIFOs is a no-op when they're disabled. I don't
> > believe that's true on my system. I also believe that not touching the
> > FIFO enable bit is problematic, since some callers clearly expect that
> > to be cleared.
>
> Why would you disable FIFO to clear it ? This doesn't make sense to me,
> the FIFO must be operational for it to be cleared. Moreover, it
> contradicts your previous statement, "the programming example it gives
> says to explicitly clear the FIFOs using FCR[2:1]" .
No, no, not at all... I'm saying that my theory is:
- The JZ4780 requires that the FIFO be reset using FCR[2:1] in order
to not read garbage.
- Prior to your patch serial8250_clear_fifos() did this,
unconditionally.
- After your patch serial8250_clear_fifos() only clears the FIFOs if
the FIFO is already enabled.
- When called from serial8250_do_startup() during boot, the FIFO may
not be enabled - for example if the bootloader didn't use the FIFO,
or if the UART is used with 8250_early which zeroes FCR.
- Therefore after your patch the FIFO reset bits are never set if the
UART was used with 8250_early, or if it wasn't but the bootloader
didn't enable the FIFO.
I suspect that you get away with that because according to the PC16550D
documentation the FIFOs should reset when the FIFO enable bit changes,
so when the FIFO is later enabled it gets reset. I suspect that in the
JZ4780 this does not happen, and the FIFO contains garbage. Our previous
behaviour (always set FCR[2:1] to reset the FIFO) has been around for a
long time, so I would not be surprised to find other devices relying
upon it.
I'm also saying that if the FIFOs are enabled during boot then your
patch will leave them enabled after serial8250_do_startup() calls
serial8250_clear_fifos(), which a comment in serial8250_do_startup()
clearly states is not the expected behaviour:
> Clear the FIFO buffers and disable them.
> (they will be reenabled in set_termios())
(From https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/8250/8250_port.c?h=v4.20-rc6#n2209)
And further, with your patch serial8250_do_shutdown() will leave the
FIFO enabled which again does not match what comments suggest it expects
("Disable break condition and FIFOs").
> What exactly does your programming example for the JZ4780 say about the
> FIFO clearing ? And does it work in that example ?
It says to set FCR[2:1] to reset the FIFO after enabling it, which as
far as I can tell from the PC16550D documentation would not be necessary
there because when you enable the FIFO it would automatically be reset.
Linux did this until your patch.
> >>> @@ -558,25 +558,26 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
> >>> 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.
> >>> + * 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);
> >>> + serial_out(p, UART_FCR, fcr | clr_mask);
> >>> + serial_out(p, UART_FCR, fcr & ~clr);
> >>
> >> Note that, if I understand the patch correctly, this will _not_ restore
> >> the original (broken) behavior. The original behavior cleared the entire
> >> FCR at the end, which cleared even bits that were not supposed to be
> >> cleared .
> >
> > It will restore the original behaviour with regards to disabling the
> > FIFOs, so long as callers that expect that use
> > serial8250_clear_and_disable_fifos().
>
> Does your platform need the FIFO to be explicitly disabled for it to be
> cleared, can you confirm that ? And if so, does this apply to other
> platforms with 8250 UART or is this specific to JZ4780 implementation of
> the UART block ?
>
> I just remembered U-Boot has this patch for JZ4780 UART [1], which
> touches the FCR UME bit, so the FCR behavior on JZ4780 does differ from
> the generic 8250 core. Could it be that this is what you're hitting ?
No - we already set the UME bit & this has all worked fine until your
patch changed the FIFO reset behaviour.
Thanks,
Paul
^ permalink raw reply
* [PATCH] tty/serial: do not free trasnmit buffer page under port lock
From: Sergey Senozhatsky @ 2018-12-13 4:58 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: linux-serial, linux-kernel, Sergey Senozhatsky, Andrew Morton,
Peter Zijlstra, Waiman Long, Dmitry Safonov, Steven Rostedt,
Petr Mladek
LKP has hit yet another circular locking dependency between uart
console drivers and debugobjects [1]:
CPU0 CPU1
rhltable_init()
__init_work()
debug_object_init
uart_shutdown() /* db->lock */
/* uart_port->lock */ debug_print_object()
free_page() printk()
call_console_drivers()
debug_check_no_obj_freed() /* uart_port->lock */
/* db->lock */
debug_print_object()
So there are two dependency chains:
uart_port->lock -> db->lock
And
db->lock -> uart_port->lock
This particular circular locking dependency can be addressed in several
ways:
a) One way would be to move debug_print_object() out of db->lock scope
and, thus, break the db->lock -> uart_port->lock chain.
b) Another one would be to free() transmit buffer page out of db->lock
in UART code; which is what this patch does.
It makes sense to apply a) and b) independently: there are too many things
going on behind free(), none of which depend on uart_port->lock.
The patch fixes transmit buffer page free() in uart_shutdown() and,
additionally, in uart_port_startup() (as was suggested by Dmitry Safonov).
[1] https://lore.kernel.org/lkml/20181211091154.GL23332@shao2-debian/T/#u
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Dmitry Safonov <dima@arista.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Petr Mladek <pmladek@suse.com>
---
drivers/tty/serial/serial_core.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c439a5a1e6c0..d4cca5bdaf1c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -205,10 +205,15 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
if (!state->xmit.buf) {
state->xmit.buf = (unsigned char *) page;
uart_circ_clear(&state->xmit);
+ uart_port_unlock(uport, flags);
} else {
+ uart_port_unlock(uport, flags);
+ /*
+ * Do not free() the page under the port lock, see
+ * uart_shutdown().
+ */
free_page(page);
}
- uart_port_unlock(uport, flags);
retval = uport->ops->startup(uport);
if (retval == 0) {
@@ -268,6 +273,7 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
struct uart_port *uport = uart_port_check(state);
struct tty_port *port = &state->port;
unsigned long flags = 0;
+ char *xmit_buf = NULL;
/*
* Set the TTY IO error marker
@@ -298,14 +304,18 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
tty_port_set_suspended(port, 0);
/*
- * Free the transmit buffer page.
+ * Do not free() the transmit buffer page under the port lock since
+ * this can create various circular locking scenarios. For instance,
+ * console driver may need to allocate/free a debug object, which
+ * can endup in printk() recursion.
*/
uart_port_lock(state, flags);
- if (state->xmit.buf) {
- free_page((unsigned long)state->xmit.buf);
- state->xmit.buf = NULL;
- }
+ xmit_buf = state->xmit.buf;
+ state->xmit.buf = NULL;
uart_port_unlock(uport, flags);
+
+ if (xmit_buf)
+ free_page((unsigned long)xmit_buf);
}
/**
--
2.20.0
^ permalink raw reply related
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