* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Andy Shevchenko @ 2019-04-30 15:37 UTC (permalink / raw)
To: Esben Haabendal
Cc: linux-serial, Enrico Weigelt, Greg Kroah-Hartman, Jiri Slaby,
Darwin Dingel, Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe,
Marek Vasut, Douglas Anderson, Paul Burton, linux-kernel
In-Reply-To: <20190430140416.4707-1-esben@geanix.com>
On Tue, Apr 30, 2019 at 04:04:13PM +0200, Esben Haabendal wrote:
> Allow getting memory resource (mapbase or iobase) as well as irq from
> platform_device resources.
>
> The UPF_DEV_RESOURCES flag must be set for devices where platform_device
> resources are to be used. When not set, driver behaves as before.
>
> This allows use of the serial8250 driver together with devices with
> resources added by platform_device_add_resources(), such as mfd child
> devices added with mfd_add_devices().
>
> Remaining platform_data fields (other than mapbase, iobase, mapsize and
> irq) are used just as before. Note
Note what?
> +static int serial8250_probe_resources(struct platform_device *pdev,
> + unsigned int num,
> + struct plat_serial8250_port *p,
> + struct uart_8250_port *uart)
> +{
> + struct resource *r;
> + int irq;
> +
> + switch (p->iotype) {
> + case UPIO_AU:
> + case UPIO_TSI:
> + case UPIO_MEM32:
> + case UPIO_MEM32BE:
> + case UPIO_MEM16:
> + case UPIO_MEM:
> + r = platform_get_resource(pdev, IORESOURCE_MEM, num);
> + if (!r)
> + return -ENODEV;
> + uart->port.mapbase = r->start;
> + uart->port.mapsize = resource_size(r);
> + uart->port.flags |= UPF_IOREMAP;
> + break;
> + case UPIO_HUB6:
> + case UPIO_PORT:
> + r = platform_get_resource(pdev, IORESOURCE_IO, num);
> + if (!r)
> + return -ENODEV;
> + uart->port.iobase = r->start;
> + uart->port.mapsize = resource_size(r);
> + break;
> + }
> +
> + irq = platform_get_irq(pdev, num);
> + if (irq == -ENXIO)
> + uart->port.irq = 0; /* no interrupt -> use polling */
> + else if (irq < 0)
> + return irq;
> + uart->port.irq = irq;
> +
> + return 0;
> +}
Hmm... Currently it's done inside individual port drivers, like 8250_dw.c.
Each of the drivers can do it differently, for example 8250_lpss.c or
8250_pnp.c.
> + if (!(port->flags & UPF_DEV_RESOURCES))
> + release_mem_region(port->mapbase, size);
This is again same issue. The parent should not request resource it doesn't use.
I think I understand what is a confusion here.
For the IO resources we have two operations:
- mapping / re-mapping (may be shared)
- requesting (exclusive)
In the parenthesis I put a level of access to it. While many device drivers can
*share* same resource (mapped or unmapped), the only one can actually request
it.
So, the parent can take an slice resources as it would be appropriated, but not
requesting them.
OTOH, it's possible to have a (weird) MFD case where parent *requested*
resources, and *all* of its children are aware of that.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: cpu power up timing changes causes UART rx character loss on imx6ull
From: Fabio Estevam @ 2019-04-30 15:35 UTC (permalink / raw)
To: Christoph Niedermaier
Cc: linux-serial@vger.kernel.org, Anson Huang, Sascha Hauer,
linux-imx@nxp.com, kernel@pengutronix.de,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <kcis.E7DDD7DFA27C49D7987A7EAB797CB891@DHPLMX01>
Hi Christoph,
On Tue, Apr 30, 2019 at 11:10 AM Christoph Niedermaier
<cniedermaier@dh-electronics.com> wrote:
>
> Hello,
>
> I have found out that the commit 1e434b703248 ("ARM: imx: update the cpu power up timing setting on i.mx6sx") causes UART rx character loss on imx6ull.
> The commit is designed for imx6sx, but it also changes the cpu power up timing of the imx6ull, because function imx6sx_cpuidle_init() is also used for imx6ull.
>
> After receiving of 32 characters correctly, the following characters 33-36 gets lost if there is no delay in the transmission.
> I connect the imx6ull with a native PC COM port.
>
> If I revert the commit I will receive all characters correctly.
Thanks for the report.
It seems we need to change sw2iso only for mx6sx and not for imx6ul.
Could you please test the following patch?
http://code.bulix.org/crpbf9-684699
Thanks
^ permalink raw reply
* Re: [PATCH 22/41] drivers: tty: serial: cpm_uart: fix logging calls
From: Andy Shevchenko @ 2019-04-30 14:10 UTC (permalink / raw)
To: Christophe Leroy
Cc: Enrico Weigelt, metux IT consult, linux-kernel, lorenzo.pieralisi,
linux-ia64, linux-serial, andrew, gregkh, sudeep.holla,
liviu.dudau, linux-mips, vz, linux, sparclinux, khilman, macro,
slemieux.tyco, matthias.bgg, jacmet, linux-amlogic, linuxppc-dev,
davem
In-Reply-To: <a00ba23b-e73e-c964-a6d0-347cb605b8c8@c-s.fr>
On Mon, Apr 29, 2019 at 05:59:04PM +0200, Christophe Leroy wrote:
> Le 27/04/2019 à 14:52, Enrico Weigelt, metux IT consult a écrit :
> > Fix checkpatch warnings by using pr_err():
> >
> > WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
> > #109: FILE: drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c:109:
> > + printk(KERN_ERR
> >
> > WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
> > #128: FILE: drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c:128:
> > + printk(KERN_ERR
> >
> > WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
> > + printk(KERN_ERR
> >
> > WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
> > + printk(KERN_ERR
> >
> > Signed-off-by: Enrico Weigelt <info@metux.net>
>
> Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
>
> But is that really worth doing those changes ?
>
> If we want to do something useful, wouldn't it make more sense to introduce
> the use of dev_err() in order to identify the faulting device in the message
> ?
+1 for switching to dev_*().
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-04-30 14:04 UTC (permalink / raw)
To: linux-serial
Cc: Enrico Weigelt, Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko,
Darwin Dingel, Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe,
Marek Vasut, Douglas Anderson, Paul Burton, linux-kernel
Allow getting memory resource (mapbase or iobase) as well as irq from
platform_device resources.
The UPF_DEV_RESOURCES flag must be set for devices where platform_device
resources are to be used. When not set, driver behaves as before.
This allows use of the serial8250 driver together with devices with
resources added by platform_device_add_resources(), such as mfd child
devices added with mfd_add_devices().
Remaining platform_data fields (other than mapbase, iobase, mapsize and
irq) are used just as before. Note
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
drivers/tty/serial/8250/8250_core.c | 56 +++++++++++++++++++++++++++++++++----
drivers/tty/serial/8250/8250_port.c | 15 ++++++----
include/linux/serial_core.h | 1 +
3 files changed, 62 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index e441221..9df6a98 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -788,6 +788,48 @@ void serial8250_resume_port(int line)
}
EXPORT_SYMBOL(serial8250_resume_port);
+static int serial8250_probe_resources(struct platform_device *pdev,
+ unsigned int num,
+ struct plat_serial8250_port *p,
+ struct uart_8250_port *uart)
+{
+ struct resource *r;
+ int irq;
+
+ switch (p->iotype) {
+ case UPIO_AU:
+ case UPIO_TSI:
+ case UPIO_MEM32:
+ case UPIO_MEM32BE:
+ case UPIO_MEM16:
+ case UPIO_MEM:
+ r = platform_get_resource(pdev, IORESOURCE_MEM, num);
+ if (!r)
+ return -ENODEV;
+ uart->port.mapbase = r->start;
+ uart->port.mapsize = resource_size(r);
+ uart->port.flags |= UPF_IOREMAP;
+ break;
+ case UPIO_HUB6:
+ case UPIO_PORT:
+ r = platform_get_resource(pdev, IORESOURCE_IO, num);
+ if (!r)
+ return -ENODEV;
+ uart->port.iobase = r->start;
+ uart->port.mapsize = resource_size(r);
+ break;
+ }
+
+ irq = platform_get_irq(pdev, num);
+ if (irq == -ENXIO)
+ uart->port.irq = 0; /* no interrupt -> use polling */
+ else if (irq < 0)
+ return irq;
+ uart->port.irq = irq;
+
+ return 0;
+}
+
/*
* Register a set of serial devices attached to a platform device. The
* list is terminated with a zero flags entry, which means we expect
@@ -805,15 +847,19 @@ static int serial8250_probe(struct platform_device *dev)
irqflag = IRQF_SHARED;
for (i = 0; p && p->flags != 0; p++, i++) {
- uart.port.iobase = p->iobase;
- uart.port.membase = p->membase;
- uart.port.irq = p->irq;
+ uart.port.flags = p->flags;
+ if (p->flags & UPF_DEV_RESOURCES) {
+ serial8250_probe_resources(dev, i, p, &uart);
+ } else {
+ uart.port.iobase = p->iobase;
+ uart.port.mapbase = p->mapbase;
+ uart.port.membase = p->membase;
+ uart.port.irq = p->irq;
+ }
uart.port.irqflags = p->irqflags;
uart.port.uartclk = p->uartclk;
uart.port.regshift = p->regshift;
uart.port.iotype = p->iotype;
- uart.port.flags = p->flags;
- uart.port.mapbase = p->mapbase;
uart.port.hub6 = p->hub6;
uart.port.private_data = p->private_data;
uart.port.type = p->type;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index d2f3310..7fa1e49 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2863,7 +2863,8 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
if (!port->mapbase)
break;
- if (!request_mem_region(port->mapbase, size, "serial")) {
+ if (!(port->flags & UPF_DEV_RESOURCES) &&
+ !request_mem_region(port->mapbase, size, "serial")) {
ret = -EBUSY;
break;
}
@@ -2871,7 +2872,8 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
if (port->flags & UPF_IOREMAP) {
port->membase = ioremap_nocache(port->mapbase, size);
if (!port->membase) {
- release_mem_region(port->mapbase, size);
+ if (!(port->flags & UPF_DEV_RESOURCES))
+ release_mem_region(port->mapbase, size);
ret = -ENOMEM;
}
}
@@ -2879,7 +2881,8 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
case UPIO_HUB6:
case UPIO_PORT:
- if (!request_region(port->iobase, size, "serial"))
+ if (!(port->flags & UPF_DEV_RESOURCES) &&
+ !request_region(port->iobase, size, "serial"))
ret = -EBUSY;
break;
}
@@ -2906,12 +2909,14 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
port->membase = NULL;
}
- release_mem_region(port->mapbase, size);
+ if (!(port->flags & UPF_DEV_RESOURCES))
+ release_mem_region(port->mapbase, size);
break;
case UPIO_HUB6:
case UPIO_PORT:
- release_region(port->iobase, size);
+ if (!(port->flags & UPF_DEV_RESOURCES))
+ release_region(port->iobase, size);
break;
}
}
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 5fe2b03..87b4ed3 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -207,6 +207,7 @@ struct uart_port {
#define UPF_BUGGY_UART ((__force upf_t) ASYNC_BUGGY_UART /* 14 */ )
#define UPF_MAGIC_MULTIPLIER ((__force upf_t) ASYNC_MAGIC_MULTIPLIER /* 16 */ )
+#define UPF_DEV_RESOURCES ((__force upf_t) (1 << 18))
#define UPF_NO_THRE_TEST ((__force upf_t) (1 << 19))
/* Port has hardware-assisted h/w flow control */
#define UPF_AUTO_CTS ((__force upf_t) (1 << 20))
--
2.4.11
^ permalink raw reply related
* cpu power up timing changes causes UART rx character loss on imx6ull
From: Christoph Niedermaier @ 2019-04-30 13:45 UTC (permalink / raw)
To: linux-arm-kernel@lists.infradead.org, linux-imx@nxp.com,
linux-serial@vger.kernel.org
Cc: Sascha Hauer, Anson Huang, kernel@pengutronix.de
Hello,
I have found out that the commit 1e434b703248 ("ARM: imx: update the cpu power up timing setting on i.mx6sx") causes UART rx character loss on imx6ull.
The commit is designed for imx6sx, but it also changes the cpu power up timing of the imx6ull, because function imx6sx_cpuidle_init() is also used for imx6ull.
After receiving of 32 characters correctly, the following characters 33-36 gets lost if there is no delay in the transmission.
I connect the imx6ull with a native PC COM port.
If I revert the commit I will receive all characters correctly.
Has anyone discovered this behavior as well?
Thank you,
Christoph
^ permalink raw reply
* Re: [PATCH 13/41] drivers: tty: serial: uartlite: fill mapsize and use it
From: Enrico Weigelt, metux IT consult @ 2019-04-29 18:26 UTC (permalink / raw)
To: Peter Korsgaard, Enrico Weigelt, metux IT consult
Cc: linux-kernel, lorenzo.pieralisi, linux-ia64, linux-serial, andrew,
gregkh, sudeep.holla, liviu.dudau, linux-mips, vz, linux,
sparclinux, khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <87muk8rg82.fsf@dell.be.48ers.dk>
On 29.04.19 17:19, Peter Korsgaard wrote:
>>>>>> "Enrico" == Enrico Weigelt, metux IT consult <info@metux.net> writes:
>
> > Fill the struct uart_port->mapsize field and use it, insteaf of
>
> s/insteaf/instead/
Fixed.
> > hardcoded values in many places. This makes the code layout a bit
> > more consistent and easily allows using generic helpers for the
> > io memory handling.
>
> > Candidates for such helpers could be eg. the request+ioremap and
> > iounmap+release combinations.
>
> > Signed-off-by: Enrico Weigelt <info@metux.net>
>
> Acked-by: Peter Korsgaard <peter@korsgaard.com>
thanks for review.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: serial drivers polishing
From: Enrico Weigelt, metux IT consult @ 2019-04-29 16:50 UTC (permalink / raw)
To: Christophe Leroy, Enrico Weigelt, metux IT consult, linux-kernel
Cc: lorenzo.pieralisi, linux-ia64, linux-serial, andrew, gregkh,
sudeep.holla, liviu.dudau, linux-mips, vz, linux, sparclinux,
khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <7471c418-4058-db7b-b2ed-af9a67fff201@c-s.fr>
On 29.04.19 18:16, Christophe Leroy wrote:
Hi,
> Got the following build error while compiling for my powerpc board with
> your full series applied. No time to investigate though.
thanks, fixed it. That was the unclean patch where i've forgotten to
add 'rfc' into the title ... turned out that this one needs some
more rework :o
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* [PATCH v3] tty: serial: 8250: Fix type field in format string
From: Hao Lee @ 2019-04-29 16:24 UTC (permalink / raw)
To: gregkh; +Cc: jslaby, haolee.swjtu, linux-serial, linux-kernel
The dev_dbg statement should print the value of uart.port.mapbase instead
of its address. Besides that, uart.port.irq and uart.port.iotype are all
unsigned types, so using %u is more appropriate.
Signed-off-by: Hao Lee <haolee.swjtu@gmail.com>
---
drivers/tty/serial/8250/8250_pnp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_pnp.c b/drivers/tty/serial/8250/8250_pnp.c
index 431e69a5a6a0..dfca33141fcc 100644
--- a/drivers/tty/serial/8250/8250_pnp.c
+++ b/drivers/tty/serial/8250/8250_pnp.c
@@ -462,8 +462,8 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
return -ENODEV;
dev_dbg(&dev->dev,
- "Setup PNP port: port %lx, mem %pa, irq %d, type %d\n",
- uart.port.iobase, &uart.port.mapbase,
+ "Setup PNP port: port %#lx, mem %#llx, irq %u, type %u\n",
+ uart.port.iobase, (unsigned long long)uart.port.mapbase,
uart.port.irq, uart.port.iotype);
if (flags & CIR_PORT) {
--
2.14.5
^ permalink raw reply related
* Re: [PATCH v2] tty: serial: 8250: Fix type field in format string
From: Hao Lee @ 2019-04-29 16:23 UTC (permalink / raw)
To: Greg KH; +Cc: jslaby, linux-serial, linux-kernel
In-Reply-To: <20190429143117.GA1474@kroah.com>
On Mon, 29 Apr 2019 at 22:31, Greg KH <gregkh@linuxfoundation.org> wrote:
> This causes build warnings when applied, I'm having to drop it now.
>
> Please be more careful, when submitting patches, always test-build them
> first.
I have found my mistake. Although I have built a kernel to test my
patch, I forget to turn on the 8250 configurations which are turned
off during another kernel test. As a result, 8250_pnp.c was not
compiled at all, so I didn't see any warnings. Sorry for that and
thanks for your guidance. I will submit my patch v3.
^ permalink raw reply
* Re: [PATCH 22/41] drivers: tty: serial: cpm_uart: fix logging calls
From: Enrico Weigelt, metux IT consult @ 2019-04-29 16:20 UTC (permalink / raw)
To: Christophe Leroy, Enrico Weigelt, metux IT consult, linux-kernel
Cc: lorenzo.pieralisi, linux-ia64, linux-serial, andrew, gregkh,
sudeep.holla, liviu.dudau, linux-mips, vz, linux, sparclinux,
khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <a00ba23b-e73e-c964-a6d0-347cb605b8c8@c-s.fr>
On 29.04.19 17:59, Christophe Leroy wrote:
> If we want to do something useful, wouldn't it make more sense to
> introduce the use of dev_err() in order to identify the faulting device
> in the message ?
Well, I could get the struct device* pointer via pinfo.port->dev,
but I wasn't entirely sure that it's always defined before these
functions could be called.
Shall I change it to dev_*() ?
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: serial drivers polishing
From: Christophe Leroy @ 2019-04-29 16:16 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult, linux-kernel
Cc: lorenzo.pieralisi, linux-ia64, linux-serial, andrew, gregkh,
sudeep.holla, liviu.dudau, linux-mips, vz, linux, sparclinux,
khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <1556369542-13247-1-git-send-email-info@metux.net>
Hi,
On 04/27/2019 12:51 PM, Enrico Weigelt, metux IT consult wrote:
> Hello folks,
>
>
> here's another attempt of polishing the serial drivers:
>
> * lots of minor cleanups to make checkpatch happier
> (eg. formatting, includes, inttypes, ...)
>
> * use appropriate logging helpers instead of printk()
>
> * consequent use of mapsize/mapbase fields:
> the basic idea is, all drivers should fill mapbase/mapbase fields at
> init time and later only use those fields, instead of hardcoded values
> (later on, we can add generic helpers for the map/unmap stuff, etc)
>
> * untwisting serial8250_port_size() at all:
> move the iomem size probing to initialization time, move out some
> platform specific magic to corresponding platform code, etc.
>
>
> Unfortunately, I don't have the actual hardware to really test all
> the code, so please let me know if there's something broken in here.
>
>
> have fun,
>
> --mtx
>
Got the following build error while compiling for my powerpc board with
your full series applied. No time to investigate though.
CC arch/powerpc/kernel/setup-common.o
In file included from ./include/linux/serial_8250.h:14:0,
from arch/powerpc/kernel/setup-common.c:33:
./include/linux/serial_core.h: In function ‘uart_memres_set_res’:
./include/linux/serial_core.h:446:18: error: ‘resource’ undeclared
(first use in this function)
port->iobase = resource->start;
^
./include/linux/serial_core.h:446:18: note: each undeclared identifier
is reported only once for each function it appears in
./include/linux/serial_core.h:450:2: error: ‘uart’ undeclared (first use
in this function)
uart->mapbase = res->start;
^
./include/linux/serial_core.h: In function ‘uart_memres_set_start_len’:
./include/linux/serial_core.h:464:6: error: ‘struct uart_driver’ has no
member named ‘mapbase’
uart->mapbase = start;
^
./include/linux/serial_core.h:465:6: error: ‘struct uart_driver’ has no
member named ‘mapsize’
uart->mapsize = len;
^
./include/linux/serial_core.h:466:6: error: ‘struct uart_driver’ has no
member named ‘iotype’
uart->iotype = UPIO_MEM;
^
make[3]: *** [arch/powerpc/kernel/setup-common.o] Error 1
Christophe
^ permalink raw reply
* Re: [PATCH 21/41] drivers: tty: serial: cpm_uart: fix includes
From: Christophe Leroy @ 2019-04-29 16:02 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult, linux-kernel
Cc: lorenzo.pieralisi, linux-ia64, linux-serial, andrew, gregkh,
sudeep.holla, liviu.dudau, linux-mips, vz, linux, sparclinux,
khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <1556369542-13247-22-git-send-email-info@metux.net>
Le 27/04/2019 à 14:52, Enrico Weigelt, metux IT consult a écrit :
> Fixing checkpatch warning:
>
> WARNING: Use #include <linux/io.h> instead of <asm/io.h>
> #25: FILE: drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c:25:
> +#include <asm/io.h>
>
> WARNING: Use #include <linux/io.h> instead of <asm/io.h>
> +#include <asm/io.h>
>
> WARNING: Use #include <linux/delay.h> instead of <asm/delay.h>
> +#include <asm/delay.h>
>
> Signed-off-by: Enrico Weigelt <info@metux.net>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> drivers/tty/serial/cpm_uart/cpm_uart_core.c | 4 ++--
> drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_core.c b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
> index 374b8bb..c831d31 100644
> --- a/drivers/tty/serial/cpm_uart/cpm_uart_core.c
> +++ b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
> @@ -33,10 +33,10 @@
> #include <linux/gpio.h>
> #include <linux/of_gpio.h>
> #include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
>
> -#include <asm/io.h>
> #include <asm/irq.h>
> -#include <asm/delay.h>
> #include <asm/fs_pd.h>
> #include <asm/udbg.h>
>
> diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c b/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c
> index ef1ae08..40cfcf4 100644
> --- a/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c
> +++ b/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c
> @@ -21,8 +21,8 @@
> #include <linux/device.h>
> #include <linux/memblock.h>
> #include <linux/dma-mapping.h>
> +#include <linux/io.h>
>
> -#include <asm/io.h>
> #include <asm/irq.h>
> #include <asm/fs_pd.h>
> #include <asm/prom.h>
>
^ permalink raw reply
* Re: [PATCH 20/41] drivers: tty: serial: cpm_uart: use dev_err()/dev_warn() instead of printk()
From: Christophe Leroy @ 2019-04-29 16:02 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult, linux-kernel
Cc: lorenzo.pieralisi, linux-ia64, linux-serial, andrew, gregkh,
sudeep.holla, liviu.dudau, linux-mips, vz, linux, sparclinux,
khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <1556369542-13247-21-git-send-email-info@metux.net>
Le 27/04/2019 à 14:52, Enrico Weigelt, metux IT consult a écrit :
> Using dev_err()/dev_warn() instead of printk() for more consistent output.
> (prints device name, etc).
>
> Signed-off-by: Enrico Weigelt <info@metux.net>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> drivers/tty/serial/cpm_uart/cpm_uart_core.c | 6 +++---
> drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_core.c b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
> index b929c7a..374b8bb 100644
> --- a/drivers/tty/serial/cpm_uart/cpm_uart_core.c
> +++ b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
> @@ -265,7 +265,7 @@ static void cpm_uart_int_rx(struct uart_port *port)
> * later, which will be the next rx-interrupt or a timeout
> */
> if (tty_buffer_request_room(tport, i) < i) {
> - printk(KERN_WARNING "No room in flip buffer\n");
> + dev_warn(port->dev, "No room in flip buffer\n");
> return;
> }
>
> @@ -1155,7 +1155,7 @@ static int cpm_uart_init_port(struct device_node *np,
> if (!pinfo->clk) {
> data = of_get_property(np, "fsl,cpm-brg", &len);
> if (!data || len != 4) {
> - printk(KERN_ERR "CPM UART %pOFn has no/invalid "
> + dev_err(port->dev, "CPM UART %pOFn has no/invalid "
> "fsl,cpm-brg property.\n", np);
> return -EINVAL;
> }
> @@ -1164,7 +1164,7 @@ static int cpm_uart_init_port(struct device_node *np,
>
> data = of_get_property(np, "fsl,cpm-command", &len);
> if (!data || len != 4) {
> - printk(KERN_ERR "CPM UART %pOFn has no/invalid "
> + dev_err(port->dev, "CPM UART %pOFn has no/invalid "
> "fsl,cpm-command property.\n", np);
> return -EINVAL;
> }
> diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c b/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c
> index 6a1cd03..ef1ae08 100644
> --- a/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c
> +++ b/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c
> @@ -67,7 +67,7 @@ void __iomem *cpm_uart_map_pram(struct uart_cpm_port *port,
> return pram;
>
> if (len != 2) {
> - printk(KERN_WARNING "cpm_uart[%d]: device tree references "
> + dev_warn(port->dev, "cpm_uart[%d]: device tree references "
> "SMC pram, using boot loader/wrapper pram mapping. "
> "Please fix your device tree to reference the pram "
> "base register instead.\n",
>
^ permalink raw reply
* Re: [PATCH 22/41] drivers: tty: serial: cpm_uart: fix logging calls
From: Christophe Leroy @ 2019-04-29 15:59 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult, linux-kernel
Cc: lorenzo.pieralisi, linux-ia64, linux-serial, andrew, gregkh,
sudeep.holla, liviu.dudau, linux-mips, vz, linux, sparclinux,
khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <1556369542-13247-23-git-send-email-info@metux.net>
Le 27/04/2019 à 14:52, Enrico Weigelt, metux IT consult a écrit :
> Fix checkpatch warnings by using pr_err():
>
> WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
> #109: FILE: drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c:109:
> + printk(KERN_ERR
>
> WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
> #128: FILE: drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c:128:
> + printk(KERN_ERR
>
> WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
> + printk(KERN_ERR
>
> WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
> + printk(KERN_ERR
>
> Signed-off-by: Enrico Weigelt <info@metux.net>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
But is that really worth doing those changes ?
If we want to do something useful, wouldn't it make more sense to
introduce the use of dev_err() in order to identify the faulting device
in the message ?
Christophe
> ---
> drivers/tty/serial/cpm_uart/cpm_uart_cpm1.c | 6 ++----
> drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c | 6 ++----
> 2 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_cpm1.c b/drivers/tty/serial/cpm_uart/cpm_uart_cpm1.c
> index 56fc527..aed61e9 100644
> --- a/drivers/tty/serial/cpm_uart/cpm_uart_cpm1.c
> +++ b/drivers/tty/serial/cpm_uart/cpm_uart_cpm1.c
> @@ -71,8 +71,7 @@ int cpm_uart_allocbuf(struct uart_cpm_port *pinfo, unsigned int is_con)
> dpmemsz = sizeof(cbd_t) * (pinfo->rx_nrfifos + pinfo->tx_nrfifos);
> dp_offset = cpm_dpalloc(dpmemsz, 8);
> if (IS_ERR_VALUE(dp_offset)) {
> - printk(KERN_ERR
> - "cpm_uart_cpm1.c: could not allocate buffer descriptors\n");
> + pr_err("cpm_uart_cpm1.c: could not allocate buffer descriptors\n");
> return -ENOMEM;
> }
> dp_mem = cpm_dpram_addr(dp_offset);
> @@ -90,8 +89,7 @@ int cpm_uart_allocbuf(struct uart_cpm_port *pinfo, unsigned int is_con)
>
> if (mem_addr == NULL) {
> cpm_dpfree(dp_offset);
> - printk(KERN_ERR
> - "cpm_uart_cpm1.c: could not allocate coherent memory\n");
> + pr_err("cpm_uart_cpm1.c: could not allocate coherent memory\n");
> return -ENOMEM;
> }
>
> diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c b/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c
> index 40cfcf4..a0fccda 100644
> --- a/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c
> +++ b/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c
> @@ -106,8 +106,7 @@ int cpm_uart_allocbuf(struct uart_cpm_port *pinfo, unsigned int is_con)
> dpmemsz = sizeof(cbd_t) * (pinfo->rx_nrfifos + pinfo->tx_nrfifos);
> dp_offset = cpm_dpalloc(dpmemsz, 8);
> if (IS_ERR_VALUE(dp_offset)) {
> - printk(KERN_ERR
> - "cpm_uart_cpm.c: could not allocate buffer descriptors\n");
> + pr_err("cpm_uart_cpm.c: could not allocate buffer descriptors\n");
> return -ENOMEM;
> }
>
> @@ -125,8 +124,7 @@ int cpm_uart_allocbuf(struct uart_cpm_port *pinfo, unsigned int is_con)
>
> if (mem_addr == NULL) {
> cpm_dpfree(dp_offset);
> - printk(KERN_ERR
> - "cpm_uart_cpm.c: could not allocate coherent memory\n");
> + pr_err("cpm_uart_cpm.c: could not allocate coherent memory\n");
> return -ENOMEM;
> }
>
>
^ permalink raw reply
* Re: [PATCH 37/41] drivers: tty: serial: 8250: simplify io resource size computation
From: Enrico Weigelt, metux IT consult @ 2019-04-29 15:58 UTC (permalink / raw)
To: John Paul Adrian Glaubitz, Enrico Weigelt, metux IT consult,
linux-kernel
Cc: gregkh, andrew, andriy.shevchenko, macro, vz, slemieux.tyco,
khilman, liviu.dudau, sudeep.holla, lorenzo.pieralisi, davem,
jacmet, linux, matthias.bgg, linux-mips, linux-serial, linux-ia64,
linux-amlogic, linuxppc-dev, sparclinux
In-Reply-To: <ba6dd5fa-36f1-902d-1ab4-c99e6a5ea3c2@physik.fu-berlin.de>
On 27.04.19 15:03, John Paul Adrian Glaubitz wrote:
> On 4/27/19 2:52 PM, Enrico Weigelt, metux IT consult wrote:
>> Simpily io resource size computation by setting mapsize field.
> ^^^^
> Here's a typo
thx. fixed.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: [PATCH 23/41] drivers: tty: serial: cpm_uart: fix styling issues
From: Christophe Leroy @ 2019-04-29 15:56 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult, linux-kernel
Cc: lorenzo.pieralisi, linux-ia64, linux-serial, andrew, gregkh,
sudeep.holla, liviu.dudau, linux-mips, vz, linux, sparclinux,
khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <1556369542-13247-24-git-send-email-info@metux.net>
Le 27/04/2019 à 14:52, Enrico Weigelt, metux IT consult a écrit :
> Fix checkpatch errors:
What the main purpose of this change ?
If we apply this, any fix to stable will be a nightmare to backport. Is
it really worth it ?
Anyway, a couple of comments in the patch below
[...]
>
>
> Signed-off-by: Enrico Weigelt <info@metux.net>
> ---
> drivers/tty/serial/cpm_uart/cpm_uart.h | 10 +--
> drivers/tty/serial/cpm_uart/cpm_uart_core.c | 95 ++++++++++++++++-------------
> drivers/tty/serial/cpm_uart/cpm_uart_cpm1.h | 4 +-
> drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c | 6 +-
> 4 files changed, 64 insertions(+), 51 deletions(-)
[...]
>
> @@ -1048,9 +1058,10 @@ static void cpm_uart_early_write(struct uart_cpm_port *pinfo,
> static int poll_wait_key(char *obuf, struct uart_cpm_port *pinfo)
> {
> u_char c, *cp;
> - volatile cbd_t *bdp;
> int i;
>
> + volatile cbd_t *bdp;
> +
This was likely a false positive from checkpatch. The formatting was
good, and now it is wrong as it adds an unnecessary blank line.
> /* Get the address of the host memory buffer.
> */
> bdp = pinfo->rx_cur;
[...]
> diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c b/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c
> index a0fccda..154ac19 100644
> --- a/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c
> +++ b/drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c
> @@ -117,8 +117,7 @@ int cpm_uart_allocbuf(struct uart_cpm_port *pinfo, unsigned int is_con)
> if (is_con) {
> mem_addr = kzalloc(memsz, GFP_NOWAIT);
> dma_addr = virt_to_bus(mem_addr);
> - }
> - else
> + } else
> mem_addr = dma_alloc_coherent(pinfo->port.dev, memsz, &dma_addr,
> GFP_KERNEL);
Checkpatch should have told you that in case first leg has braces,
second leg must have braces too even if it's a single line.
Christophe
>
> @@ -148,7 +147,8 @@ void cpm_uart_freebuf(struct uart_cpm_port *pinfo)
> dma_free_coherent(pinfo->port.dev, L1_CACHE_ALIGN(pinfo->rx_nrfifos *
> pinfo->rx_fifosize) +
> L1_CACHE_ALIGN(pinfo->tx_nrfifos *
> - pinfo->tx_fifosize), (void __force *)pinfo->mem_addr,
> + pinfo->tx_fifosize),
> + (void __force *)pinfo->mem_addr,
> pinfo->dma_addr);
>
> cpm_dpfree(pinfo->dp_addr);
>
^ permalink raw reply
* Re: [PATCH 36/41] drivers: tty: serial: 8250: store mmio resource size in port struct
From: Andy Shevchenko @ 2019-04-29 15:39 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: Enrico Weigelt, metux IT consult, linux-kernel, gregkh, andrew,
macro, vz, slemieux.tyco, khilman, liviu.dudau, sudeep.holla,
lorenzo.pieralisi, davem, jacmet, linux, matthias.bgg, linux-mips,
linux-serial, linux-ia64, linux-amlogic, linuxppc-dev, sparclinux
In-Reply-To: <4bab941a-c2f2-7f1c-9bc2-86c63f171c25@metux.net>
On Mon, Apr 29, 2019 at 04:55:05PM +0200, Enrico Weigelt, metux IT consult wrote:
> On 28.04.19 17:18, Andy Shevchenko wrote:
> > On Sat, Apr 27, 2019 at 02:52:17PM +0200, Enrico Weigelt, metux IT consult wrote:
> >> - int ret = 0;
> >
> > This and Co is a separate change that can be done in its own patch.
>
> I don't really understand :(
> Do you mean the splitting off the retval part from the rest ?
You do two things here: one of them is removing ret and other relative changes.
This should be split to a separate patch.
> > You may increase readability by introducing temporary variables
> >
> > ... mapbase = port->mapbase;
> > ... mapsize = port->mapsize;
> > ...
> > port->membase = ioremap_nocache(mapbase, mapsize);
> > ...
>
> Is that really necessary ? Maybe it's just my personal taste, but I
> don't feel the more more verbose one is really easier to read.
Up to Greg. For me it's harder to read all those port-> in several parameters.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 12/41] drivers: tty: serial: uartlite: use dev_dbg() instead of pr_debug()
From: Peter Korsgaard @ 2019-04-29 15:26 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: linux-kernel, lorenzo.pieralisi, linux-ia64, linux-serial, andrew,
gregkh, sudeep.holla, liviu.dudau, linux-mips, vz, linux,
sparclinux, khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <1556369542-13247-13-git-send-email-info@metux.net>
>>>>> "Enrico" == Enrico Weigelt, metux IT consult <info@metux.net> writes:
> Using dev_dbg() instead of pr_debg() for more consistent output.
> (prints device name, etc).
> Signed-off-by: Enrico Weigelt <info@metux.net>
Acked-by: Peter Korsgaard <peter@korsgaard.com>
--
Bye, Peter Korsgaard
^ permalink raw reply
* Re: [PATCH 16/41] drivers: tty: serial: uartlite: fix overlong lines
From: Peter Korsgaard @ 2019-04-29 15:24 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: linux-kernel, lorenzo.pieralisi, linux-ia64, linux-serial, andrew,
gregkh, sudeep.holla, liviu.dudau, linux-mips, vz, linux,
sparclinux, khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <1556369542-13247-17-git-send-email-info@metux.net>
>>>>> "Enrico" == Enrico Weigelt, metux IT consult <info@metux.net> writes:
> Fix checkpatch warnings:
> WARNING: line over 80 characters
> #283: FILE: drivers/tty/serial/uartlite.c:283:
> + ret = request_irq(port->irq, ulite_isr, IRQF_SHARED | IRQF_TRIGGER_RISING,
> WARNING: Missing a blank line after declarations
> #577: FILE: drivers/tty/serial/uartlite.c:577:
> + struct earlycon_device *device = console->data;
> + uart_console_write(&device->port, s, n, early_uartlite_putc);
> WARNING: line over 80 characters
> #590: FILE: drivers/tty/serial/uartlite.c:590:
> +OF_EARLYCON_DECLARE(uartlite_b, "xlnx,opb-uartlite-1.00.b", early_uartlite_setup);
> WARNING: line over 80 characters
> #591: FILE: drivers/tty/serial/uartlite.c:591:
> +OF_EARLYCON_DECLARE(uartlite_a, "xlnx,xps-uartlite-1.00.a", early_uartlite_setup);
Given that these are just a few characters more than 80 I don't really
think these changes improve readability.
> Signed-off-by: Enrico Weigelt <info@metux.net>
> ---
> drivers/tty/serial/uartlite.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index 6f79353..0140cec 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -280,7 +280,8 @@ static int ulite_startup(struct uart_port *port)
> return ret;
> }
> - ret = request_irq(port->irq, ulite_isr, IRQF_SHARED | IRQF_TRIGGER_RISING,
> + ret = request_irq(port->irq, ulite_isr,
> + IRQF_SHARED | IRQF_TRIGGER_RISING,
> "uartlite", port);
> if (ret)
> return ret;
> @@ -574,6 +575,7 @@ static void early_uartlite_write(struct console *console,
> const char *s, unsigned int n)
> {
> struct earlycon_device *device = console->data;
> +
> uart_console_write(&device->port, s, n, early_uartlite_putc);
Unrelated change?
--
Bye, Peter Korsgaard
^ permalink raw reply
* Re: [PATCH 15/41] drivers: tty: serial: uartlite: fix use fix bare 'unsigned'
From: Peter Korsgaard @ 2019-04-29 15:21 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: linux-kernel, lorenzo.pieralisi, linux-ia64, linux-serial, andrew,
gregkh, sudeep.holla, liviu.dudau, linux-mips, vz, linux,
sparclinux, khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <1556369542-13247-16-git-send-email-info@metux.net>
>>>>> "Enrico" == Enrico Weigelt, metux IT consult <info@metux.net> writes:
> Fix checkpatch warnings:
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #562: FILE: drivers/tty/serial/uartlite.c:562:
> + unsigned retries = 1000000;
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #574: FILE: drivers/tty/serial/uartlite.c:574:
> + const char *s, unsigned n)
s/fix use fix/fix use of/ in Subject. Other than that:
Acked-by: Peter Korsgaard <peter@korsgaard.com>
--
Bye, Peter Korsgaard
^ permalink raw reply
* Re: [PATCH 14/41] drivers: tty: serial: uartlite: remove unnecessary braces
From: Peter Korsgaard @ 2019-04-29 15:20 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: linux-kernel, lorenzo.pieralisi, linux-ia64, linux-serial, andrew,
gregkh, sudeep.holla, liviu.dudau, linux-mips, vz, linux,
sparclinux, khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <1556369542-13247-15-git-send-email-info@metux.net>
>>>>> "Enrico" == Enrico Weigelt, metux IT consult <info@metux.net> writes:
> checkpatch complains:
> WARNING: braces {} are not necessary for any arm of this statement
> #489: FILE: drivers/tty/serial/uartlite.c:489:
> + if (oops_in_progress) {
> [...]
> + } else
> [...]
> Signed-off-by: Enrico Weigelt <info@metux.net>
Acked-by: Peter Korsgaard <peter@korsgaard.com>
--
Bye, Peter Korsgaard
^ permalink raw reply
* Re: [PATCH 13/41] drivers: tty: serial: uartlite: fill mapsize and use it
From: Peter Korsgaard @ 2019-04-29 15:19 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: linux-kernel, lorenzo.pieralisi, linux-ia64, linux-serial, andrew,
gregkh, sudeep.holla, liviu.dudau, linux-mips, vz, linux,
sparclinux, khilman, macro, slemieux.tyco, matthias.bgg, jacmet,
linux-amlogic, andriy.shevchenko, linuxppc-dev, davem
In-Reply-To: <1556369542-13247-14-git-send-email-info@metux.net>
>>>>> "Enrico" == Enrico Weigelt, metux IT consult <info@metux.net> writes:
> Fill the struct uart_port->mapsize field and use it, insteaf of
s/insteaf/instead/
> hardcoded values in many places. This makes the code layout a bit
> more consistent and easily allows using generic helpers for the
> io memory handling.
> Candidates for such helpers could be eg. the request+ioremap and
> iounmap+release combinations.
> Signed-off-by: Enrico Weigelt <info@metux.net>
Acked-by: Peter Korsgaard <peter@korsgaard.com>
--
Bye, Peter Korsgaard
^ permalink raw reply
* Re: [PATCH 36/41] drivers: tty: serial: 8250: store mmio resource size in port struct
From: Enrico Weigelt, metux IT consult @ 2019-04-29 14:55 UTC (permalink / raw)
To: Andy Shevchenko, Enrico Weigelt, metux IT consult
Cc: linux-kernel, gregkh, andrew, macro, vz, slemieux.tyco, khilman,
liviu.dudau, sudeep.holla, lorenzo.pieralisi, davem, jacmet,
linux, matthias.bgg, linux-mips, linux-serial, linux-ia64,
linux-amlogic, linuxppc-dev, sparclinux
In-Reply-To: <20190428151848.GO9224@smile.fi.intel.com>
On 28.04.19 17:18, Andy Shevchenko wrote:
> On Sat, Apr 27, 2019 at 02:52:17PM +0200, Enrico Weigelt, metux IT consult wrote:
>> The io resource size is currently recomputed when it's needed but this
>> actually needs to be done once (or drivers could specify fixed values)
>
> io -> IO
fixed.
>> Simplify that by doing this computation only once and storing the result
>> into the mapsize field. serial8250_register_8250_port() is now called
>> only once on driver init, the previous call sites now just fetch the
>> value from the mapsize field.
>
> Do I understand correctly that this has no side effects?
I don't know of any. (except someting changes things like regshift after
the initialization phase ... :o)
>> @@ -979,6 +979,9 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>> if (up->port.uartclk == 0)
>> return -EINVAL;
>>
>> + /* compute the mapsize in case the driver didn't specify one */
>> + up->mapsize = serial8250_port_size(up);
>
> I don't know all quirks in 8250 drivers by heart, though can you guarantee that
> at this point the device reports correct IO resource size? (If I'm not mistaken
> some broken hardware needs some magic to be done before card can be properly
> handled)
Actually, I don't see anything talking to the hardware at all here.
It's all derived from values that are set up before
serial8250_register_8250_port() is called.
>> - unsigned int size = serial8250_port_size(up);
>> struct uart_port *port = &up->port;
>
>> - int ret = 0;
>
> This and Co is a separate change that can be done in its own patch.
I don't really understand :(
Do you mean the splitting off the retval part from the rest ?
>> + port->membase = ioremap_nocache(port->mapbase,
>> + port->mapsize);
>
> You may increase readability by introducing temporary variables
>
> ... mapbase = port->mapbase;
> ... mapsize = port->mapsize;
> ...
> port->membase = ioremap_nocache(mapbase, mapsize);
> ...
Is that really necessary ? Maybe it's just my personal taste, but I
don't feel the more more verbose one is really easier to read.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: [PATCH v2] tty: serial: 8250: Fix type field in format string
From: Greg KH @ 2019-04-29 14:31 UTC (permalink / raw)
To: Hao Lee; +Cc: jslaby, linux-serial, linux-kernel
In-Reply-To: <20190427091943.GA3810@haolee.io>
On Sat, Apr 27, 2019 at 05:19:43PM +0800, Hao Lee wrote:
> The dev_dbg statement should print the value of uart.port.mapbase instead
> of its address. Besides that, uart.port.irq and uart.port.iotype are all
> unsigned types, so using %u is more appropriate.
>
> Signed-off-by: Hao Lee <haolee.swjtu@gmail.com>
> ---
> drivers/tty/serial/8250/8250_pnp.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_pnp.c b/drivers/tty/serial/8250/8250_pnp.c
> index 431e69a5a6a0..9dea11baf479 100644
> --- a/drivers/tty/serial/8250/8250_pnp.c
> +++ b/drivers/tty/serial/8250/8250_pnp.c
> @@ -462,8 +462,8 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
> return -ENODEV;
>
> dev_dbg(&dev->dev,
> - "Setup PNP port: port %lx, mem %pa, irq %d, type %d\n",
> - uart.port.iobase, &uart.port.mapbase,
> + "Setup PNP port: port %#lx, mem %#lx, irq %u, type %u\n",
> + uart.port.iobase, uart.port.mapbase,
> uart.port.irq, uart.port.iotype);
>
> if (flags & CIR_PORT) {
> --
> 2.14.5
This causes build warnings when applied, I'm having to drop it now.
Please be more careful, when submitting patches, always test-build them
first.
greg k-h
^ permalink raw reply
* Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF
From: Esben Haabendal @ 2019-04-29 14:25 UTC (permalink / raw)
To: Andy Shevchenko
Cc: open list:SERIAL DRIVERS, Greg Kroah-Hartman, Jiri Slaby,
Darwin Dingel, He Zhe, Jisheng Zhang, Sebastian Andrzej Siewior,
Linux Kernel Mailing List
In-Reply-To: <20190429133535.GG9224@smile.fi.intel.com>
Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> On Mon, Apr 29, 2019 at 11:29:05AM +0200, Esben Haabendal wrote:
>> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>> > On Mon, Apr 29, 2019 at 9:27 AM Esben Haabendal <esben@haabendal.dk> wrote:
>> >> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>> >> > On Sat, Apr 27, 2019 at 12:01 PM Esben Haabendal <esben@haabendal.dk>
>> >> > wrote:
>> >> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> >> >> > On Fri, Apr 26, 2019 at 06:54:05PM +0200, Esben Haabendal wrote:
>> >> >> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>
>> So maybe we should go down that direction intead, extending 8250 driver
>> to replace mapbase with a resource struct instead?
>>
>> > Btw, we have PCI MFD driver which enumerates 8250 (more precisely
>> > 8250_dw) w/o any issues.
>>
>> I am aware of that (sm501.c). It avoids the problem by not requesting
>> the parent memory region (sm->io_res), and requesting all child memory
>> regions directly in root instead of relative to the sm->io_res parent.
>>
>> But as resoure management is designed for managing a parent/child
>> resource tree, this looks much more like a workaround than the right
>> solution.
>>
>> >> It would be nice if child drivers requesting memory would pass the
>> >> parent memory resource. Maybe 8250 driver could be changed to accept a
>> >> struct resource pointer instead of a simple mapbase value, allowing to
>> >> setup the resource with parent pointing to the MFD memory resource.
>> >
>> > I don't see the problem in certain driver, I guess you are trying to
>> > workaround existin Linux device resource model.
>>
>> No, I actually try to do the right thing in relation to Linux device
>> resource model. But 8250 is just not behaving very well in that
>> respect, not having been made really aware of the resource model.
>
> The point here is that. MFD driver can re-use existing platform drivers which
> may be used standalone. They and only they are the right owners of the
> requesting *their* resources.
>
> When parent request resources on the behalf of its child it simple will break
> this flexibility.
I hear what you say. And I agree, parent/mfd drivers should not request
resources on behalf of it's children.
But on the other side, something is broken by design in mfd, if it is
not possible for parent/mfd driver to request the resources for itself,
which naturally contains the resources for all it's mfd
functions/childs.
Please take a look at mfd_add_device() in drivers/mfd/mfd-core.c, and
the use of the cell and mem_base arguments in particular.
for (r = 0; r < cell->num_resources; r++) {
res[r].name = cell->resources[r].name;
res[r].flags = cell->resources[r].flags;
/* Find out base to use */
if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
res[r].parent = mem_base;
res[r].start = mem_base->start +
cell->resources[r].start;
res[r].end = mem_base->start +
cell->resources[r].end;
It really is a core part of mfd drivers that you request a memory
resource for the mfd driver, and then have one or more child memory
resources requsted with the parent memory resource as base.
Many mfd drivers handle resources like this, like: htc/pasic3.c,
sta2x11-mfd.c and intel-lpss.c.
Ofcourse, the sm501.c driver does not, as it does not use the mfd API at
all, and is thus not a good example of a mfd driver, and there is
therefore no good examples of using 8250 with an mfd driver.
/Esben
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox