Hi,
IMHO PATCHv4 was better, but in the end I don't have strong feelings
about this. Btw. I think this patchset is a good demonstration of
frustrating upstream kernel development can be considering PATCHv5
is basically the same as PATCHv1. Thanks for making us go in
circles :(
Changes since PATCHv4:
* https://lore.kernel.org/lkml/[email protected]/
* use DT property instead of sysfs config option, like the initial patch
version did as requested by Greg.
Changes since PATCHv3:
* https://lore.kernel.org/lkml/[email protected]/
* rewrote commit message to provide a lot more details why this is needed
* rebased to torvalds/master (5.12-rc1-dontuse), also applies on top of linux-next
* use sysfs_emit() instead of sprintf
-- Sebastian
Fabien Lahoudere (2):
serial: imx: Add DMA buffer configuration via DT
ARM: dts: imx53-ppd: add dma-info nodes
.../bindings/serial/fsl-imx-uart.yaml | 12 +++++++++
arch/arm/boot/dts/imx53-ppd.dts | 2 ++
drivers/tty/serial/imx.c | 25 +++++++++++++------
3 files changed, 32 insertions(+), 7 deletions(-)
--
2.30.2
From: Fabien Lahoudere <[email protected]>
In order to optimize serial communication (performance/throughput VS
latency), we may need to tweak DMA period number and size. This adds
DT properties to configure those values before initialising DMA.
The defaults will stay the same as before.
Signed-off-by: Fabien Lahoudere <[email protected]>
[update documentation and commit message, rebase to current master,
switch back to DT instead of sysfs]
Signed-off-by: Sebastian Reichel <[email protected]>
---
.../bindings/serial/fsl-imx-uart.yaml | 12 +++++++++
drivers/tty/serial/imx.c | 25 +++++++++++++------
2 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml b/Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml
index 2b06c6ce4a75..9d949296a142 100644
--- a/Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml
+++ b/Documentation/devicetree/bindings/serial/fsl-imx-uart.yaml
@@ -71,6 +71,18 @@ properties:
received, and that the peripheral should invert its input using the
INVR registers.
+ fsl,dma-info:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 2
+ maxItems: 2
+ description: |
+ First cell contains the size of DMA buffer chunks, second cell contains
+ the amount of chunks used for the device. Multiplying both numbers is
+ the total size of memory used for receiving data.
+ When not being configured the system will use default settings, which
+ are sensible for most use cases. If you need low latency processing on
+ slow connections this needs to be configured appropriately.
+
uart-has-rtscts: true
rs485-rts-delay: true
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 7d5a8dfa3e91..9e72e382a867 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -225,6 +225,8 @@ struct imx_port {
struct scatterlist rx_sgl, tx_sgl[2];
void *rx_buf;
struct circ_buf rx_ring;
+ unsigned int rx_buf_size;
+ unsigned int rx_period_length;
unsigned int rx_periods;
dma_cookie_t rx_cookie;
unsigned int tx_bytes;
@@ -1183,10 +1185,6 @@ static void imx_uart_dma_rx_callback(void *data)
}
}
-/* RX DMA buffer periods */
-#define RX_DMA_PERIODS 16
-#define RX_BUF_SIZE (RX_DMA_PERIODS * PAGE_SIZE / 4)
-
static int imx_uart_start_rx_dma(struct imx_port *sport)
{
struct scatterlist *sgl = &sport->rx_sgl;
@@ -1197,9 +1195,8 @@ static int imx_uart_start_rx_dma(struct imx_port *sport)
sport->rx_ring.head = 0;
sport->rx_ring.tail = 0;
- sport->rx_periods = RX_DMA_PERIODS;
- sg_init_one(sgl, sport->rx_buf, RX_BUF_SIZE);
+ sg_init_one(sgl, sport->rx_buf, sport->rx_buf_size);
ret = dma_map_sg(dev, sgl, 1, DMA_FROM_DEVICE);
if (ret == 0) {
dev_err(dev, "DMA mapping error for RX.\n");
@@ -1316,7 +1313,8 @@ static int imx_uart_dma_init(struct imx_port *sport)
goto err;
}
- sport->rx_buf = kzalloc(RX_BUF_SIZE, GFP_KERNEL);
+ sport->rx_buf_size = sport->rx_period_length * sport->rx_periods;
+ sport->rx_buf = kzalloc(sport->rx_buf_size, GFP_KERNEL);
if (!sport->rx_buf) {
ret = -ENOMEM;
goto err;
@@ -2179,11 +2177,16 @@ static enum hrtimer_restart imx_trigger_stop_tx(struct hrtimer *t)
return HRTIMER_NORESTART;
}
+/* Default RX DMA buffer configuration */
+#define RX_DMA_PERIODS 16
+#define RX_DMA_PERIOD_LEN (PAGE_SIZE / 4)
+
static int imx_uart_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct imx_port *sport;
void __iomem *base;
+ u32 dma_buf_conf[2];
int ret = 0;
u32 ucr1;
struct resource *res;
@@ -2218,6 +2221,14 @@ static int imx_uart_probe(struct platform_device *pdev)
if (of_get_property(np, "fsl,inverted-rx", NULL))
sport->inverted_rx = 1;
+ if (!of_property_read_u32_array(np, "fsl,dma-info", dma_buf_conf, 2)) {
+ sport->rx_period_length = dma_buf_conf[0];
+ sport->rx_periods = dma_buf_conf[1];
+ } else {
+ sport->rx_period_length = RX_DMA_PERIOD_LEN;
+ sport->rx_periods = RX_DMA_PERIODS;
+ }
+
if (sport->port.line >= ARRAY_SIZE(imx_uart_ports)) {
dev_err(&pdev->dev, "serial%d out of range\n",
sport->port.line);
--
2.30.2
Hi Greg,
On Sat, May 01, 2021 at 08:11:52AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
> > IMHO PATCHv4 was better, but in the end I don't have strong feelings
> > about this. Btw. I think this patchset is a good demonstration of
> > frustrating upstream kernel development can be considering PATCHv5
> > is basically the same as PATCHv1. Thanks for making us go in
> > circles :(
> >
> > Changes since PATCHv4:
> > * https://lore.kernel.org/lkml/[email protected]/
> > * use DT property instead of sysfs config option, like the initial patch
> > version did as requested by Greg.
> >
> > Changes since PATCHv3:
> > * https://lore.kernel.org/lkml/[email protected]/
> > * rewrote commit message to provide a lot more details why this is needed
> > * rebased to torvalds/master (5.12-rc1-dontuse), also applies on top of linux-next
> > * use sysfs_emit() instead of sprintf
> >
> > -- Sebastian
> >
> > Fabien Lahoudere (2):
> > serial: imx: Add DMA buffer configuration via DT
> > ARM: dts: imx53-ppd: add dma-info nodes
> >
> > .../bindings/serial/fsl-imx-uart.yaml | 12 +++++++++
> > arch/arm/boot/dts/imx53-ppd.dts | 2 ++
> > drivers/tty/serial/imx.c | 25 +++++++++++++------
> > 3 files changed, 32 insertions(+), 7 deletions(-)
>
> This is the friendly semi-automated patch-bot of Greg Kroah-Hartman.
> You have sent him a patch that has triggered this response.
>
> Right now, the development tree you have sent a patch for is "closed"
> due to the timing of the merge window. Don't worry, the patch(es) you
> have sent are not lost, and will be looked at after the merge window is
> over (after the -rc1 kernel is released by Linus).
>
> So thank you for your patience and your patches will be reviewed at this
> later time, you do not have to do anything further, this is just a short
> note to let you know the patch status and so you don't worry they didn't
> make it through.
>
> thanks,
>
> greg k-h's patch email bot
Any update on this? :)
-- Sebastian
On Fri, May 28, 2021 at 02:49:52AM +0200, Sebastian Reichel wrote:
> Hi Greg,
>
> On Sat, May 01, 2021 at 08:11:52AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
> > > IMHO PATCHv4 was better, but in the end I don't have strong feelings
> > > about this. Btw. I think this patchset is a good demonstration of
> > > frustrating upstream kernel development can be considering PATCHv5
> > > is basically the same as PATCHv1. Thanks for making us go in
> > > circles :(
> > >
> > > Changes since PATCHv4:
> > > * https://lore.kernel.org/lkml/[email protected]/
> > > * use DT property instead of sysfs config option, like the initial patch
> > > version did as requested by Greg.
> > >
> > > Changes since PATCHv3:
> > > * https://lore.kernel.org/lkml/[email protected]/
> > > * rewrote commit message to provide a lot more details why this is needed
> > > * rebased to torvalds/master (5.12-rc1-dontuse), also applies on top of linux-next
> > > * use sysfs_emit() instead of sprintf
> > >
> > > -- Sebastian
> > >
> > > Fabien Lahoudere (2):
> > > serial: imx: Add DMA buffer configuration via DT
> > > ARM: dts: imx53-ppd: add dma-info nodes
> > >
> > > .../bindings/serial/fsl-imx-uart.yaml | 12 +++++++++
> > > arch/arm/boot/dts/imx53-ppd.dts | 2 ++
> > > drivers/tty/serial/imx.c | 25 +++++++++++++------
> > > 3 files changed, 32 insertions(+), 7 deletions(-)
> >
> > This is the friendly semi-automated patch-bot of Greg Kroah-Hartman.
> > You have sent him a patch that has triggered this response.
> >
> > Right now, the development tree you have sent a patch for is "closed"
> > due to the timing of the merge window. Don't worry, the patch(es) you
> > have sent are not lost, and will be looked at after the merge window is
> > over (after the -rc1 kernel is released by Linus).
> >
> > So thank you for your patience and your patches will be reviewed at this
> > later time, you do not have to do anything further, this is just a short
> > note to let you know the patch status and so you don't worry they didn't
> > make it through.
> >
> > thanks,
> >
> > greg k-h's patch email bot
>
> Any update on this? :)
I'm waiting for the DT maintainers to review the new changes before I
can take the driver changes.
thanks,
greg k-h
Hello Greg,
[adding Rob and devicetree@vger to Cc:]
On Fri, May 28, 2021 at 11:03:54AM +0200, Greg Kroah-Hartman wrote:
> On Fri, May 28, 2021 at 02:49:52AM +0200, Sebastian Reichel wrote:
> > On Sat, May 01, 2021 at 08:11:52AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
> > > > IMHO PATCHv4 was better, but in the end I don't have strong feelings
> > > > about this. Btw. I think this patchset is a good demonstration of
> > > > frustrating upstream kernel development can be considering PATCHv5
> > > > is basically the same as PATCHv1. Thanks for making us go in
> > > > circles :(
I was the one who objected the v1 approach. My frustration is that I
wasn't Cc:d in the later iterations of this patch set. And I also liked
v4 better :-\
> > > > Changes since PATCHv4:
> > > > * https://lore.kernel.org/lkml/[email protected]/
> > > > * use DT property instead of sysfs config option, like the initial patch
> > > > version did as requested by Greg.
> > > >
> > > > Changes since PATCHv3:
> > > > * https://lore.kernel.org/lkml/[email protected]/
> > > > * rewrote commit message to provide a lot more details why this is needed
> > > > * rebased to torvalds/master (5.12-rc1-dontuse), also applies on top of linux-next
> > > > * use sysfs_emit() instead of sprintf
> > > >
> > > > -- Sebastian
> > > >
> > > > Fabien Lahoudere (2):
> > > > serial: imx: Add DMA buffer configuration via DT
> > > > ARM: dts: imx53-ppd: add dma-info nodes
> > > >
> > > > .../bindings/serial/fsl-imx-uart.yaml | 12 +++++++++
> > > > arch/arm/boot/dts/imx53-ppd.dts | 2 ++
> > > > drivers/tty/serial/imx.c | 25 +++++++++++++------
> > > > 3 files changed, 32 insertions(+), 7 deletions(-)
> > >
> > > This is the friendly semi-automated patch-bot of Greg Kroah-Hartman.
> > > You have sent him a patch that has triggered this response.
> > >
> > > Right now, the development tree you have sent a patch for is "closed"
> > > due to the timing of the merge window. Don't worry, the patch(es) you
> > > have sent are not lost, and will be looked at after the merge window is
> > > over (after the -rc1 kernel is released by Linus).
> > >
> > > So thank you for your patience and your patches will be reviewed at this
> > > later time, you do not have to do anything further, this is just a short
> > > note to let you know the patch status and so you don't worry they didn't
> > > make it through.
> > >
> > > thanks,
> > >
> > > greg k-h's patch email bot
> >
> > Any update on this? :)
>
> I'm waiting for the DT maintainers to review the new changes before I
> can take the driver changes.
I wonder what the DT maintainers said (and where [1]). The patch was
applied (db0a196bd8ad1d6bb4b1a9e54f54c09f8dc2cc25) and I think that's
wrong (as it was in v1) because fsl,dma-info isn't about hardware
description but about software tuning for different use cases.
Best regards
Uwe
[1] neither my mailbox nor lore.kernel.org know about an answer.
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[Cc += dt maintainers]
On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
> IMHO PATCHv4 was better, but in the end I don't have strong feelings
> about this. Btw. I think this patchset is a good demonstration of
> frustrating upstream kernel development can be considering PATCHv5
> is basically the same as PATCHv1. Thanks for making us go in
> circles :(
I still like v4 better than v1/v5. I'm sorry for the frustration this
created on your side.
I'd ask Greg to reconsider given that dt is less flexible than a sysfs
knob and otherwise shares all downsides of sysfs (people don't want to
have to tune that, so a useful default for most cases is important; you
have to consult documentation to understand how to tune it).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri, Dec 08, 2023 at 10:07:54AM +0100, Uwe Kleine-K?nig wrote:
> [Cc += dt maintainers]
>
> On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
You are responding to a message from 2021???
I can't remember what I wrote in an email last week, let alone years
ago, are you sure any of this is relevant still?
thanks,
greg k-h
On 08/12/2023 11:32, Greg Kroah-Hartman wrote:
> On Fri, Dec 08, 2023 at 10:07:54AM +0100, Uwe Kleine-König wrote:
>> [Cc += dt maintainers]
>>
>> On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
>
> You are responding to a message from 2021???
>
> I can't remember what I wrote in an email last week, let alone years
> ago, are you sure any of this is relevant still?
>
Also, adding DT maintainers as Cc, does not magically allow us to
understand the discussion. Please shortly summarize what do you
need/expect from us.
Best regards,
Krzysztof
On Fri, Dec 08, 2023 at 01:14:50PM +0100, Krzysztof Kozlowski wrote:
>
> On 08/12/2023 11:32, Greg Kroah-Hartman wrote:
> > On Fri, Dec 08, 2023 at 10:07:54AM +0100, Uwe Kleine-K??nig wrote:
> >> [Cc += dt maintainers]
> >>
> >> On Fri, Apr 30, 2021 at 07:50:36PM +0200, Sebastian Reichel wrote:
> >
> > You are responding to a message from 2021???
> >
> > I can't remember what I wrote in an email last week, let alone years
> > ago, are you sure any of this is relevant still?
> >
>
> Also, adding DT maintainers as Cc, does not magically allow us to
> understand the discussion. Please shortly summarize what do you
> need/expect from us.
This thread is indeed old, but we are remain interested in getting
this resolved.
The topic itself is about the tuning of DMA buffers for the i.MX53
platform.
DMA is required (for the 4M baud UART case), and a specific buffer
configuration is required, too.
Two approaches have been attempted:
"V4" sysfs
https://lore.kernel.org/lkml/[email protected]/
"V5" DT
https://lore.kernel.org/lkml/[email protected]/
(this thread)
Uwe recently commented to [1], and (hoping to avoid any further
confusion) let's continue in that thread.
[1] https://lore.kernel.org/lkml/[email protected]/
>
> Best regards,
> Krzysztof
>
>