* [PATCH] tty: serial: uartlite: avoid null pointer dereference during rmmod
From: Kefeng Wang @ 2019-05-14 3:32 UTC (permalink / raw)
To: linux-kernel, linux-serial
Cc: Kefeng Wang, Peter Korsgaard, Shubhrajyoti Datta,
Greg Kroah-Hartman, Hulk Robot
After commit 415b43bdb008 "tty: serial: uartlite: Move uart register to
probe", calling uart_unregister_driver unconditionally will trigger a
null pointer dereference due to ulite_uart_driver may not registed.
CPU: 1 PID: 3755 Comm: syz-executor.0 Not tainted 5.1.0+ #28
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xa9/0x10e lib/dump_stack.c:113
__kasan_report+0x171/0x18d mm/kasan/report.c:321
kasan_report+0xe/0x20 mm/kasan/common.c:614
tty_unregister_driver+0x19/0x100 drivers/tty/tty_io.c:3383
uart_unregister_driver+0x30/0xc0 drivers/tty/serial/serial_core.c:2579
__do_sys_delete_module kernel/module.c:1027 [inline]
__se_sys_delete_module kernel/module.c:970 [inline]
__x64_sys_delete_module+0x244/0x330 kernel/module.c:970
do_syscall_64+0x72/0x2a0 arch/x86/entry/common.c:298
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Call uart_unregister_driver only if ulite_uart_driver.state not null to
fix it.
Cc: Peter Korsgaard <jacmet@sunsite.dk>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe")
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/tty/serial/uartlite.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index b8b912b5a8b9..06e79c11141d 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -897,7 +897,8 @@ static int __init ulite_init(void)
static void __exit ulite_exit(void)
{
platform_driver_unregister(&ulite_platform_driver);
- uart_unregister_driver(&ulite_uart_driver);
+ if (ulite_uart_driver.state)
+ uart_unregister_driver(&ulite_uart_driver);
}
module_init(ulite_init);
--
2.20.1
^ permalink raw reply related
* [PATCH v2] serial: sh-sci: disable DMA for uart_console
From: George G. Davis @ 2019-05-13 15:47 UTC (permalink / raw)
To: Eugeniu Rosca, Geert Uytterhoeven, Simon Horman, Wolfram Sang,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
open list
Cc: Chris Brandt, Ulrich Hecht, Andy Lowe, Linux-Renesas,
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Magnus Damm,
Rob Herring, Mark Rutland, George G. Davis, stable
As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
console UART"), UART console lines use low-level PIO only access functions
which will conflict with use of the line when DMA is enabled, e.g. when
the console line is also used for systemd messages. So disable DMA
support for UART console lines.
Fixes: https://patchwork.kernel.org/patch/10929511/
Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: stable@vger.kernel.org
Signed-off-by: George G. Davis <george_davis@mentor.com>
---
v2: Clarify comment regarding DMA support on kernel console,
add {Tested,Reviewed}-by:, and Cc: linux-stable lines.
---
drivers/tty/serial/sh-sci.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 3cd139752d3f..abc705716aa0 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1557,6 +1557,13 @@ static void sci_request_dma(struct uart_port *port)
dev_dbg(port->dev, "%s: port %d\n", __func__, port->line);
+ /*
+ * DMA on console may interfere with Kernel log messages which use
+ * plain putchar(). So, simply don't use it with a console.
+ */
+ if (uart_console(port))
+ return;
+
if (!port->dev->of_node)
return;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] serial: sh-sci: disable DMA for uart_console
From: George G. Davis @ 2019-05-13 15:43 UTC (permalink / raw)
To: Wolfram Sang
Cc: George G. Davis, Eugeniu Rosca, Geert Uytterhoeven,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
open list, Simon Horman, Chris Brandt, Wolfram Sang, Ulrich Hecht,
Andy Lowe, Linux-Renesas,
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Magnus Damm,
Rob Herring, Mark Rutland
In-Reply-To: <20190513135114.GA20443@kunai>
Hello Wolfram,
On Mon, May 13, 2019 at 03:51:14PM +0200, Wolfram Sang wrote:
> On Thu, May 09, 2019 at 10:43:30AM -0400, George G. Davis wrote:
> > As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> > console UART"), UART console lines use low-level PIO only access functions
> > which will conflict with use of the line when DMA is enabled, e.g. when
> > the console line is also used for systemd messages. So disable DMA
> > support for UART console lines.
> >
> > Fixes: https://patchwork.kernel.org/patch/10929511/
> > Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> > Cc: Eugeniu Rosca <erosca@de.adit-jv.com>
> > Signed-off-by: George G. Davis <george_davis@mentor.com>
> > ---
> > drivers/tty/serial/sh-sci.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> > index 3cd139752d3f..885b56b1d4e4 100644
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -1557,6 +1557,9 @@ static void sci_request_dma(struct uart_port *port)
> >
> > dev_dbg(port->dev, "%s: port %d\n", __func__, port->line);
> >
> > + if (uart_console(port))
> > + return; /* Cannot use DMA on console */
>
> Minor nit: maybe the comment can be made more specific?
>
> /*
> * DMA on console may interfere with Kernel log messages which use
> * plain putchar(). So, simply don't use it with a console.
> */
I'll submit v2 with the above recommended change.
Thanks!
> Other than that:
>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Much better than dropping the properties, as Geert noted.
--
Regards,
George
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: disable DMA for uart_console
From: Wolfram Sang @ 2019-05-13 13:51 UTC (permalink / raw)
To: George G. Davis
Cc: Eugeniu Rosca, Geert Uytterhoeven, Greg Kroah-Hartman, Jiri Slaby,
open list:SERIAL DRIVERS, open list, Simon Horman, Chris Brandt,
Wolfram Sang, Ulrich Hecht, Andy Lowe, Linux-Renesas,
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Magnus Damm,
Rob Herring, Mark Rutland, George G. Davis
In-Reply-To: <1557413011-1662-1-git-send-email-george_davis@mentor.com>
[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]
On Thu, May 09, 2019 at 10:43:30AM -0400, George G. Davis wrote:
> As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> console UART"), UART console lines use low-level PIO only access functions
> which will conflict with use of the line when DMA is enabled, e.g. when
> the console line is also used for systemd messages. So disable DMA
> support for UART console lines.
>
> Fixes: https://patchwork.kernel.org/patch/10929511/
> Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> Cc: Eugeniu Rosca <erosca@de.adit-jv.com>
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> ---
> drivers/tty/serial/sh-sci.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 3cd139752d3f..885b56b1d4e4 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1557,6 +1557,9 @@ static void sci_request_dma(struct uart_port *port)
>
> dev_dbg(port->dev, "%s: port %d\n", __func__, port->line);
>
> + if (uart_console(port))
> + return; /* Cannot use DMA on console */
Minor nit: maybe the comment can be made more specific?
/*
* DMA on console may interfere with Kernel log messages which use
* plain putchar(). So, simply don't use it with a console.
*/
Other than that:
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Much better than dropping the properties, as Geert noted.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: disable DMA for uart_console
From: Simon Horman @ 2019-05-13 11:42 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: George G. Davis, Eugeniu Rosca, Greg Kroah-Hartman, Jiri Slaby,
open list:SERIAL DRIVERS, open list, Chris Brandt, Wolfram Sang,
Ulrich Hecht, Andy Lowe, Linux-Renesas,
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Magnus Damm,
Rob Herring, Mark Rutland, George G. Davis
In-Reply-To: <CAMuHMdUCcxfVdY1PqfYRZMjHN2eP_-NAsniCY39XyrDysAu1Pw@mail.gmail.com>
On Mon, May 13, 2019 at 01:13:16PM +0200, Geert Uytterhoeven wrote:
> Hi George,
>
> On Thu, May 9, 2019 at 4:44 PM George G. Davis <ggdavisiv@gmail.com> wrote:
> > As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> > console UART"), UART console lines use low-level PIO only access functions
> > which will conflict with use of the line when DMA is enabled, e.g. when
> > the console line is also used for systemd messages. So disable DMA
> > support for UART console lines.
> >
> > Fixes: https://patchwork.kernel.org/patch/10929511/
> > Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> > Cc: Eugeniu Rosca <erosca@de.adit-jv.com>
> > Signed-off-by: George G. Davis <george_davis@mentor.com>
>
> I think this makes sense. In addition to OMAP 8250, the same approach
> is used in the Mediatek 8250 and iMX serial drivers.
>
> Regardless, this is definitely better than removing the "dmas" properties
> from DT, as DT describes hardware, not usage policies.
+1
> Anyone else with a comment?
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: disable DMA for uart_console
From: Geert Uytterhoeven @ 2019-05-13 11:13 UTC (permalink / raw)
To: George G. Davis
Cc: Eugeniu Rosca, Greg Kroah-Hartman, Jiri Slaby,
open list:SERIAL DRIVERS, open list, Simon Horman, Chris Brandt,
Wolfram Sang, Ulrich Hecht, Andy Lowe, Linux-Renesas,
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Magnus Damm,
Rob Herring, Mark Rutland, George G. Davis
In-Reply-To: <1557413011-1662-1-git-send-email-george_davis@mentor.com>
Hi George,
On Thu, May 9, 2019 at 4:44 PM George G. Davis <ggdavisiv@gmail.com> wrote:
> As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> console UART"), UART console lines use low-level PIO only access functions
> which will conflict with use of the line when DMA is enabled, e.g. when
> the console line is also used for systemd messages. So disable DMA
> support for UART console lines.
>
> Fixes: https://patchwork.kernel.org/patch/10929511/
> Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> Cc: Eugeniu Rosca <erosca@de.adit-jv.com>
> Signed-off-by: George G. Davis <george_davis@mentor.com>
I think this makes sense. In addition to OMAP 8250, the same approach
is used in the Mediatek 8250 and iMX serial drivers.
Regardless, this is definitely better than removing the "dmas" properties
from DT, as DT describes hardware, not usage policies.
Anyone else with a comment?
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] serial: sh-sci: disable DMA for uart_console
From: Eugeniu Rosca @ 2019-05-13 10:28 UTC (permalink / raw)
To: George G. Davis
Cc: George G. Davis, Geert Uytterhoeven, Greg Kroah-Hartman,
Jiri Slaby, open list:SERIAL DRIVERS, open list, Simon Horman,
Chris Brandt, Wolfram Sang, Ulrich Hecht, Andy Lowe,
Linux-Renesas, OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Magnus Damm, Rob Herring, Mark Rutland, Eugeniu Rosca
In-Reply-To: <20190510183847.GB28648@mam-gdavis-lt>
Hi George,
On Fri, May 10, 2019 at 02:38:47PM -0400, George G. Davis wrote:
> Hello Eugeniu,
>
> On Fri, May 10, 2019 at 07:10:21PM +0200, Eugeniu Rosca wrote:
> > Hi George,
> >
> > I am able to reproduce the SCIF2 console freeze described in the
> > referenced patchwork link using M3-ES1.1-Salvator-XS and recent
> > v5.1-9573-gb970afcfcabd kernel.
> >
> > I confirm the behavior is healed with this patch. Thanks!
> > Hope to see it accepted soon, since it fixes a super annoying
> > console breakage every fourth boot or so on lots of R-Car3 targets.
> >
> > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
>
> Thanks for testing.
>
> Also note, for the record, that the problem is not limited to SCIF2, e.g. try
> setting console=ttySC<n> wheren <n> is not SCIF2 on any other board which
> includes support for other serial ports, e.g. r8a7795-salvator-x, and you will
> observe the same problem on other SCIF ports too. It's just a concidence that
> most boards use SCIF2 as the default serial console where the console hangs
> (resolved by this patch) have been observed on multiple boards.
Thanks for the additional level of detail.
FTR, trying to track the origin of the problem, it looks to me that the
issue was _unmasked_ by v4.16-rc1 commit be7e251d20e6c8 ("tty: serial:
sh-sci: Hide DMA config question") which turned on DMA on SCIF by
default.
I wonder if it'd be helpful to resend the patch w/o using --in-reply-to,
so that it appears as standalone entry in linux-renesas-soc patchwork.
Currently, assuming that the R-Car maintainers filter out any "Rejected"
patches (which is the default patchwork behavior), your patch would be
hidden from their eye.
--
Best Regards,
Eugeniu.
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: disable DMA for uart_console
From: George G. Davis @ 2019-05-10 18:38 UTC (permalink / raw)
To: Eugeniu Rosca
Cc: George G. Davis, Geert Uytterhoeven, Greg Kroah-Hartman,
Jiri Slaby, open list:SERIAL DRIVERS, open list, Simon Horman,
Chris Brandt, Wolfram Sang, Ulrich Hecht, Andy Lowe,
Linux-Renesas, OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Magnus Damm, Rob Herring, Mark Rutland, Eugeniu Rosca
In-Reply-To: <20190510171021.GA22691@vmlxhi-102.adit-jv.com>
Hello Eugeniu,
On Fri, May 10, 2019 at 07:10:21PM +0200, Eugeniu Rosca wrote:
> Hi George,
>
> I am able to reproduce the SCIF2 console freeze described in the
> referenced patchwork link using M3-ES1.1-Salvator-XS and recent
> v5.1-9573-gb970afcfcabd kernel.
>
> I confirm the behavior is healed with this patch. Thanks!
> Hope to see it accepted soon, since it fixes a super annoying
> console breakage every fourth boot or so on lots of R-Car3 targets.
>
> Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Thanks for testing.
Also note, for the record, that the problem is not limited to SCIF2, e.g. try
setting console=ttySC<n> wheren <n> is not SCIF2 on any other board which
includes support for other serial ports, e.g. r8a7795-salvator-x, and you will
observe the same problem on other SCIF ports too. It's just a concidence that
most boards use SCIF2 as the default serial console where the console hangs
(resolved by this patch) have been observed on multiple boards.
>
> On Thu, May 09, 2019 at 10:43:30AM -0400, George G. Davis wrote:
> > As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> > console UART"), UART console lines use low-level PIO only access functions
> > which will conflict with use of the line when DMA is enabled, e.g. when
> > the console line is also used for systemd messages. So disable DMA
> > support for UART console lines.
> >
> > Fixes: https://patchwork.kernel.org/patch/10929511/
> > Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> > Cc: Eugeniu Rosca <erosca@de.adit-jv.com>
> > Signed-off-by: George G. Davis <george_davis@mentor.com>
> > ---
> > drivers/tty/serial/sh-sci.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> > index 3cd139752d3f..885b56b1d4e4 100644
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -1557,6 +1557,9 @@ static void sci_request_dma(struct uart_port *port)
> >
> > dev_dbg(port->dev, "%s: port %d\n", __func__, port->line);
> >
> > + if (uart_console(port))
> > + return; /* Cannot use DMA on console */
> > +
> > if (!port->dev->of_node)
> > return;
> >
> > --
> > 2.7.4
> >
>
> --
> Best Regards,
> Eugeniu.
--
Regards,
George
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: disable DMA for uart_console
From: Eugeniu Rosca @ 2019-05-10 17:10 UTC (permalink / raw)
To: George G. Davis
Cc: Geert Uytterhoeven, Greg Kroah-Hartman, Jiri Slaby,
open list:SERIAL DRIVERS, open list, Simon Horman, Chris Brandt,
Wolfram Sang, Ulrich Hecht, Andy Lowe, Linux-Renesas,
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Magnus Damm,
Rob Herring, Mark Rutland, George G. Davis, Eugeniu Rosca
In-Reply-To: <1557413011-1662-1-git-send-email-george_davis@mentor.com>
Hi George,
I am able to reproduce the SCIF2 console freeze described in the
referenced patchwork link using M3-ES1.1-Salvator-XS and recent
v5.1-9573-gb970afcfcabd kernel.
I confirm the behavior is healed with this patch. Thanks!
Hope to see it accepted soon, since it fixes a super annoying
console breakage every fourth boot or so on lots of R-Car3 targets.
Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
On Thu, May 09, 2019 at 10:43:30AM -0400, George G. Davis wrote:
> As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> console UART"), UART console lines use low-level PIO only access functions
> which will conflict with use of the line when DMA is enabled, e.g. when
> the console line is also used for systemd messages. So disable DMA
> support for UART console lines.
>
> Fixes: https://patchwork.kernel.org/patch/10929511/
> Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> Cc: Eugeniu Rosca <erosca@de.adit-jv.com>
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> ---
> drivers/tty/serial/sh-sci.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 3cd139752d3f..885b56b1d4e4 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1557,6 +1557,9 @@ static void sci_request_dma(struct uart_port *port)
>
> dev_dbg(port->dev, "%s: port %d\n", __func__, port->line);
>
> + if (uart_console(port))
> + return; /* Cannot use DMA on console */
> +
> if (!port->dev->of_node)
> return;
>
> --
> 2.7.4
>
--
Best Regards,
Eugeniu.
^ permalink raw reply
* Re: [PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip
From: Lee Jones @ 2019-05-10 7:14 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Ralf Baechle, Paul Burton, James Hogan, Dmitry Torokhov,
David S. Miller, Alessandro Zummo, Alexandre Belloni,
Greg Kroah-Hartman, Jiri Slaby, linux-mips, linux-kernel,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190509160220.bb5382df931e5bd0972276df@suse.de>
On Thu, 09 May 2019, Thomas Bogendoerfer wrote:
> On Wed, 8 May 2019 11:23:13 +0100
> Lee Jones <lee.jones@linaro.org> wrote:
>
> > On Tue, 09 Apr 2019, Thomas Bogendoerfer wrote:
> >
> > > +static u32 crc8_addr(u64 addr)
> > > +{
> > > + u32 crc = 0;
> > > + int i;
> > > +
> > > + for (i = 0; i < 64; i += 8)
> > > + crc8_byte(&crc, addr >> i);
> > > + return crc;
> > > +}
> >
> > Not looked into these in any detail, but are you not able to use the
> > CRC functions already provided by the kernel?
>
> they are using a different polynomial, so I can't use it.
Would it be worth moving support out to somewhere more central so
others can use this "polynomial"?
> > > + }
> > > + pr_err("ioc3: CRC error in NIC address\n");
> > > +}
> >
> > This all looks like networking code. If this is the case, it should
> > be moved to drivers/networking or similar.
>
> no it's not. nic stands for number in a can produced by Dallas Semi also
> known under the name 1-Wire (https://en.wikipedia.org/wiki/1-Wire).
> SGI used them to provide partnumber, serialnumber and mac addresses.
> By placing the code to read the NiCs inside ioc3 driver there is no need
> for locking and adding library code for accessing these informations.
Great. So it looks like you should be using this, no?
drivers/base/regmap/regmap-w1.c
> > > +static struct resource ioc3_uarta_resources[] = {
> > > + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
> >
> > You are the first user of offsetof() in MFD. Could you tell me why
> > it's required please?
>
> to get the offsets of different chip functions out of a struct.
I can see what it does on a coding level.
What are you using it for in practical/real terms?
Why wouldn't any other MFD driver require this, but you do?
> > Please drop all of these and statically create the MFD cells like
> > almost all other MFD drivers do.
>
> I started that way and it blew up the driver and create a bigger mess
> than I wanted to have. What's your concern with my approach ?
>
> I could use static mfd_cell arrays, if there would be a init/startup
> method per cell, which is called before setting up the platform device.
> That way I could do the dynamic setup for ethernet and serial devices.
You can set platform data later. There are plenty of examples of
this in the MFD subsystem. Statically define what you can, and add
the dynamic stuff later.
> > > +static void ioc3_create_devices(struct ioc3_priv_data *ipd)
> > > +{
> > > + struct mfd_cell *cell;
> > > +
> > > + memset(ioc3_mfd_cells, 0, sizeof(ioc3_mfd_cells));
> > > + cell = ioc3_mfd_cells;
> > > +
> > > + if (ipd->info->funcs & IOC3_ETH) {
> > > + memcpy(ioc3_eth_platform_data.mac_addr, ipd->nic_mac,
> > > + sizeof(ioc3_eth_platform_data.mac_addr));
> >
> > Better to pull the MAC address from within the Ethernet driver.
>
> the NiC where the MAC address is provided is connected to the ioc3
> chip outside of the ethernet register set. And there is another
> NiC connected to the same 1-W bus. So moving reading of the MAC
> address to the ethernet driver duplicates code and adds complexity
> (locking). Again what's your concern here ?
Does this go away if you use the already provided 1-wire API?
> > > + if (ipd->info->funcs & IOC3_SER) {
> > > + writel(GPCR_UARTA_MODESEL | GPCR_UARTB_MODESEL,
> > > + &ipd->regs->gpcr_s);
> > > + writel(0, &ipd->regs->gppr[6]);
> > > + writel(0, &ipd->regs->gppr[7]);
> > > + udelay(100);
> > > + writel(readl(&ipd->regs->port_a.sscr) & ~SSCR_DMA_EN,
> > > + &ipd->regs->port_a.sscr);
> > > + writel(readl(&ipd->regs->port_b.sscr) & ~SSCR_DMA_EN,
> > > + &ipd->regs->port_b.sscr);
> > > + udelay(1000);
> >
> > No idea what any of this does.
> >
> > It looks like it belongs in the serial driver (and needs comments).
>
> it configures the IOC3 chip for serial usage. This is not part of
> the serial register set, so it IMHO belongs in the MFD driver.
So it does serial things, but doesn't belong in the serial driver?
Could you please go into a bit more detail as to why you think that?
Why is it better here?
It's also totally unreadable by the way!
> > > + }
> > > +#if defined(CONFIG_SGI_IP27)
> >
> > What is this? Can't you obtain this dynamically by probing the H/W?
>
> that's the machine type and the #ifdef CONFIG_xxx are just for saving space,
> when compiled for other machines and it's easy to remove.
Please find other ways to save the space. #ifery can get very messy,
very quickly and is almost always avoidable.
> > > + if (ipd->info->irq_offset) {
> >
> > What does this really signify?
>
> IOC3 ASICs are most of the time connected to a SGI bridge chip. IOC3 can
> provide two interrupt lines, which are wired to the bridge chip. The first
> interrupt is assigned via the PCI core, but since IOC3 is not a PCI multi
> function device the second interrupt must be treated here. And the used
> interrupt line on the bridge chip differs between boards.
Please provide a MACRO, function or something else which results in
more readable code. Whatever you choose to use, please add this text
above, it will be helpful for future readers.
> Thank you for your review. I'll address all other comments not cited in
> my mail.
NP
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH] tty: amba-pl011: allow shared interrupt
From: Florian Fainelli @ 2019-05-09 21:11 UTC (permalink / raw)
To: linux-kernel
Cc: bcm-kernel-feedback-list, Doug Berger, Florian Fainelli,
Russell King, Greg Kroah-Hartman, Jiri Slaby,
open list:SERIAL DRIVERS
From: Doug Berger <opendmb@gmail.com>
The PL011 register space includes all necessary status bits to
determine whether a device instance requires handling in response
to an interrupt. Therefore, multiple instances of the device could
be serviced by a single shared interrupt, which is the case on BCM7211.
Signed-off-by: Doug Berger <opendmb@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/tty/serial/amba-pl011.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 89ade213a1a9..5921a33b2a07 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1717,7 +1717,7 @@ static int pl011_allocate_irq(struct uart_amba_port *uap)
{
pl011_write(uap->im, uap, REG_IMSC);
- return request_irq(uap->port.irq, pl011_int, 0, "uart-pl011", uap);
+ return request_irq(uap->port.irq, pl011_int, IRQF_SHARED, "uart-pl011", uap);
}
/*
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip
From: Thomas Bogendoerfer @ 2019-05-09 14:02 UTC (permalink / raw)
To: Lee Jones
Cc: Ralf Baechle, Paul Burton, James Hogan, Dmitry Torokhov,
David S. Miller, Alessandro Zummo, Alexandre Belloni,
Greg Kroah-Hartman, Jiri Slaby, linux-mips, linux-kernel,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190508102313.GG3995@dell>
On Wed, 8 May 2019 11:23:13 +0100
Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 09 Apr 2019, Thomas Bogendoerfer wrote:
>
> > +static u32 crc8_addr(u64 addr)
> > +{
> > + u32 crc = 0;
> > + int i;
> > +
> > + for (i = 0; i < 64; i += 8)
> > + crc8_byte(&crc, addr >> i);
> > + return crc;
> > +}
>
> Not looked into these in any detail, but are you not able to use the
> CRC functions already provided by the kernel?
they are using a different polynomial, so I can't use it.
> > + }
> > + pr_err("ioc3: CRC error in NIC address\n");
> > +}
>
> This all looks like networking code. If this is the case, it should
> be moved to drivers/networking or similar.
no it's not. nic stands for number in a can produced by Dallas Semi also
known under the name 1-Wire (https://en.wikipedia.org/wiki/1-Wire).
SGI used them to provide partnumber, serialnumber and mac addresses.
By placing the code to read the NiCs inside ioc3 driver there is no need
for locking and adding library code for accessing these informations.
> > +static struct resource ioc3_uarta_resources[] = {
> > + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
>
> You are the first user of offsetof() in MFD. Could you tell me why
> it's required please?
to get the offsets of different chip functions out of a struct.
> Please drop all of these and statically create the MFD cells like
> almost all other MFD drivers do.
I started that way and it blew up the driver and create a bigger mess
than I wanted to have. What's your concern with my approach ?
I could use static mfd_cell arrays, if there would be a init/startup
method per cell, which is called before setting up the platform device.
That way I could do the dynamic setup for ethernet and serial devices.
> > +static void ioc3_create_devices(struct ioc3_priv_data *ipd)
> > +{
> > + struct mfd_cell *cell;
> > +
> > + memset(ioc3_mfd_cells, 0, sizeof(ioc3_mfd_cells));
> > + cell = ioc3_mfd_cells;
> > +
> > + if (ipd->info->funcs & IOC3_ETH) {
> > + memcpy(ioc3_eth_platform_data.mac_addr, ipd->nic_mac,
> > + sizeof(ioc3_eth_platform_data.mac_addr));
>
> Better to pull the MAC address from within the Ethernet driver.
the NiC where the MAC address is provided is connected to the ioc3
chip outside of the ethernet register set. And there is another
NiC connected to the same 1-W bus. So moving reading of the MAC
address to the ethernet driver duplicates code and adds complexity
(locking). Again what's your concern here ?
> > + if (ipd->info->funcs & IOC3_SER) {
> > + writel(GPCR_UARTA_MODESEL | GPCR_UARTB_MODESEL,
> > + &ipd->regs->gpcr_s);
> > + writel(0, &ipd->regs->gppr[6]);
> > + writel(0, &ipd->regs->gppr[7]);
> > + udelay(100);
> > + writel(readl(&ipd->regs->port_a.sscr) & ~SSCR_DMA_EN,
> > + &ipd->regs->port_a.sscr);
> > + writel(readl(&ipd->regs->port_b.sscr) & ~SSCR_DMA_EN,
> > + &ipd->regs->port_b.sscr);
> > + udelay(1000);
>
> No idea what any of this does.
>
> It looks like it belongs in the serial driver (and needs comments).
it configures the IOC3 chip for serial usage. This is not part of
the serial register set, so it IMHO belongs in the MFD driver.
> > + }
> > +#if defined(CONFIG_SGI_IP27)
>
> What is this? Can't you obtain this dynamically by probing the H/W?
that's the machine type and the #ifdef CONFIG_xxx are just for saving space,
when compiled for other machines and it's easy to remove.
> > + if (ipd->info->irq_offset) {
>
> What does this really signify?
IOC3 ASICs are most of the time connected to a SGI bridge chip. IOC3 can
provide two interrupt lines, which are wired to the bridge chip. The first
interrupt is assigned via the PCI core, but since IOC3 is not a PCI multi
function device the second interrupt must be treated here. And the used
interrupt line on the bridge chip differs between boards.
Thank you for your review. I'll address all other comments not cited in
my mail.
Thomas.
--
SUSE Linux GmbH
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* [PATCH] tty: serial_core: Fix the incorrect configuration of baud rate and data length at the console serial port resume
From: Lanqing Liu @ 2019-05-09 5:42 UTC (permalink / raw)
To: baolin.wang, baolin.wang, gregkh, jslaby
Cc: lanqing.liu, liuhhome, robh, linux-serial, linux-kernel,
chunyan.zhang, orson.zhai, zhang.lyra
When userspace opens a serial port for console, uart_port_startup()
is called. This function assigns the uport->cons->cflag value to
TTY->termios.c_cflag, then it is cleared to 0. When the user space
closes this serial port, the TTY structure will be released, and at
this time uport->cons->cflag has also been cleared.
On the Spreadtrum platform, in some special scenarios, like charging mode,
userspace needs to close the console, which means the uport->cons->cflag
has also been cleared. But printing logs is still needed in the kernel. So
when system enters suspend and resume, the console needs to be configure
the baud rate and data length of the serial port according to its own cflag
when resuming the console port. At this time, the cflag is 0, which will
cause serial port to produce configuration errors that do not meet user
expectations.
To fix this, assigning the TTY->termios.c_cflag value to uport->cons->cflag
before the userspace closes this console serial port. It will ensure that
the correct cflag value can be gotten when the console serial port was
resumed.
Signed-off-by: Lanqing Liu <liuhhome@gmail.com>
---
drivers/tty/serial/serial_core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 351843f..f233cf8 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1539,6 +1539,7 @@ static void uart_set_termios(struct tty_struct *tty,
static void uart_close(struct tty_struct *tty, struct file *filp)
{
struct uart_state *state = tty->driver_data;
+ struct uart_port *uport;
if (!state) {
struct uart_driver *drv = tty->driver->driver_state;
@@ -1553,6 +1554,9 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
}
pr_debug("uart_close(%d) called\n", tty->index);
+ uport = uart_port_check(state);
+ if (uport && uart_console(uport))
+ uport->cons->cflag = tty->termios.c_cflag;
tty_port_close(tty->port, tty, filp);
}
--
1.9.1
^ permalink raw reply related
* Re: [GIT PULL] TTY/Serial patches for 5.2-rc1
From: pr-tracker-bot @ 2019-05-08 18:30 UTC (permalink / raw)
To: Greg KH
Cc: Linus Torvalds, Jiri Slaby, Stephen Rothwell, Andrew Morton,
linux-kernel, linux-serial
In-Reply-To: <20190508103146.GA14542@kroah.com>
The pull request you sent on Wed, 8 May 2019 12:31:46 +0200:
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tags/tty-5.2-rc1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/b3a5e648f5917ea508ecab9a629028b186d38eae
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: serial: Add Milbeaut serial control
From: Sugaya, Taichi @ 2019-05-08 12:55 UTC (permalink / raw)
To: Colin Ian King
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial,
linux-kernel@vger.kernel.org
In-Reply-To: <246f81ba-0ed1-d5bc-1a48-bcf0fb2cc05e@canonical.com>
Hi,
Thank you for pointing out.
On 2019/05/02 20:47, Colin Ian King wrote:
> Hi,
>
> Static analysis with Coverity has picked up an issue in commit:
>
> commit ba44dc04300441b47618f9933bf36e75a280e5fe
> Author: Sugaya Taichi <sugaya.taichi@socionext.com>
> Date: Mon Apr 15 20:31:40 2019 +0900
>
> serial: Add Milbeaut serial control
>
> In function mlb_usio_rx_chars() the u8 status is being bit-wise AND'd
> with MLB_USIO_SSR_BRK (which is 1UL << 8) and hence the result is always
> false, which looks incorrect to me. Is this intentional?
>
No. It is always false so should be dropped.
I will send a fixes patch.
Thanks,
Sugaya Taichi
> Colin
>
^ permalink raw reply
* Re: [PATCH v3] serial: Add Milbeaut serial control
From: Sugaya, Taichi @ 2019-05-08 12:22 UTC (permalink / raw)
To: Alan Cox
Cc: Greg Kroah-Hartman, Jiri Slaby, Arnd Bergmann, Takao Orito,
Kazuhiro Kasai, Shinji Kanematsu, Jassi Brar, Masami Hiramatsu,
linux-kernel, linux-serial
In-Reply-To: <20190426191515.757e6015@alans-desktop>
Hi,
Thank you for pointing out.
On 2019/04/27 3:15, Alan Cox wrote:
> O
>> +static void mlb_usio_set_termios(struct uart_port *port,
>> + struct ktermios *termios, struct ktermios *old)
>> +{
>> + unsigned int escr, smr = MLB_USIO_SMR_SOE;
>> + unsigned long flags, baud, quot;
>> +
>> + switch (termios->c_cflag & CSIZE) {
>> + case CS5:
>> + escr = MLB_USIO_ESCR_L_5BIT;
>> + break;
>> + case CS6:
>> + escr = MLB_USIO_ESCR_L_6BIT;
>> + break;
>> + case CS7:
>> + escr = MLB_USIO_ESCR_L_7BIT;
>> + break;
>> + case CS8:
>> + default:
>> + escr = MLB_USIO_ESCR_L_8BIT;
>> + break;
>> + }
>> +
>> + if (termios->c_cflag & CSTOPB)
>> + smr |= MLB_USIO_SMR_SBL;
>> +
>> + if (termios->c_cflag & PARENB) {
>> + escr |= MLB_USIO_ESCR_PEN;
>> + if (termios->c_cflag & PARODD)
>> + escr |= MLB_USIO_ESCR_P;
>> + }
>
> If you don't suport CMSPAR then clear that bit in termios as well
>
OK, clear the bit because of not supported.
>> + /* Set hard flow control */
>> + if (of_property_read_bool(port->dev->of_node, "auto-flow-control") ||
>> + (termios->c_cflag & CRTSCTS))
>> + escr |= MLB_USIO_ESCR_FLWEN;
>
> That's just broken. The termios bits are the definitive things for the
> port, and in addition even if they are forced you need to correct the
> termios data.
>
> You might want to control flow control *at boot* with an OF property but
> doing it post boot is just busted.
>
Ah, Yes.
I think OF property should not be here, and it may only be used to determine
the characteristics of the port.
I try to make a fixes patch.
Thanks,
Sugaya Taichi
> Alan
>
^ permalink raw reply
* [PATCH] tty: serial_core: Set port active bit in uart_port_activate
From: Serge Semin @ 2019-05-08 10:44 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: Serge Semin, linux-serial, linux-kernel
A bug was introduced by commit b3b576461864 ("tty: serial_core: convert
uart_open to use tty_port_open"). It caused a constant warning printed
into the system log regarding the tty and port counter mismatch:
[ 21.644197] ttyS ttySx: tty_port_close_start: tty->count = 1 port count = 2
in case if session hangup was detected so the warning is printed starting
from the second open-close iteration.
Particularly the problem was discovered in situation when there is a
serial tty device without hardware back-end being setup. It is considered
by the tty-serial subsystems as a hardware problem with session hang up.
In this case uart_startup() will return a positive value with TTY_IO_ERROR
flag set in corresponding tty_struct instance. The same value will get
passed to be returned from the activate() callback and then being returned
from tty_port_open(). But since in this case tty_port_block_til_ready()
isn't called the TTY_PORT_ACTIVE flag isn't set (while the method had been
called before tty_port_open conversion was introduced and the rest of the
subsystem code expected the bit being set in this case), which prevents the
uart_hangup() method to perform any cleanups including the tty port
counter setting to zero. So the next attempt to open/close the tty device
will discover the counters mismatch.
In order to fix the problem we need to manually set the TTY_PORT_ACTIVE
flag in case if uart_startup() returned a positive value. In this case
the hang up procedure will perform a full set of cleanup actions including
the port ref-counter resetting.
Fixes: b3b576461864 "tty: serial_core: convert uart_open to use tty_port_open"
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
drivers/tty/serial/serial_core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 351843f847c0..9113e07952d1 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1776,6 +1776,7 @@ static int uart_port_activate(struct tty_port *port, struct tty_struct *tty)
{
struct uart_state *state = container_of(port, struct uart_state, port);
struct uart_port *uport;
+ int ret;
uport = uart_port_check(state);
if (!uport || uport->flags & UPF_DEAD)
@@ -1786,7 +1787,11 @@ static int uart_port_activate(struct tty_port *port, struct tty_struct *tty)
/*
* Start up the serial port.
*/
- return uart_startup(tty, state, 0);
+ ret = uart_startup(tty, state, 0);
+ if (ret > 0)
+ tty_port_set_active(port, 1);
+
+ return ret;
}
static const char *uart_type(struct uart_port *port)
--
2.21.0
^ permalink raw reply related
* [GIT PULL] TTY/Serial patches for 5.2-rc1
From: Greg KH @ 2019-05-08 10:31 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jiri Slaby, Stephen Rothwell, Andrew Morton, linux-kernel,
linux-serial
The following changes since commit 085b7755808aa11f78ab9377257e1dad2e6fa4bb:
Linux 5.1-rc6 (2019-04-21 10:45:57 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tags/tty-5.2-rc1
for you to fetch changes up to 45c054d0815b1530d7c7ff8441389a0421dd05e7:
tty: serial: add driver for the SiFive UART (2019-04-29 16:30:59 +0200)
----------------------------------------------------------------
TTY/Serial patches for 5.2-rc1
Here is the "big" set of tty/serial driver patches for 5.2-rc1.
It's really pretty small, not much happening in this portion of the
kernel at the moment. When the "highlight" is the movement of the
documentation from .txt to .rst files, it's a good merge window.
There's a number of small fixes and updates over the various serial
drivers, and a new "tty null" driver for those embedded systems that
like to make things even smaller and not break things.
All of these have been in linux-next for a while with no reported
issues.
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
----------------------------------------------------------------
Andy Shevchenko (4):
dt-bindings: sc16is7xx: Add alternative clock-frequency property
serial: sc16is7xx: Respect clock-frequency property
serial: sc16is7xx: Switch to use device_get_match_data()
serial: sc16is7xx: Drop of_match_ptr() use
Bartlomiej Zolnierkiewicz (1):
tty: remove redundant 'default n' from Kconfig-s
Colin Ian King (1):
n_tty: check for negative and zero space return from tty_write_room
David Emett (1):
tty: fix read of tty->pgrp outside of ctrl_lock
Fuqian Huang (2):
tty: rocket: Remove RCPK_GET_STRUCT ioctl
tty: rocket: deprecate the rp_ioctl
Greg Kroah-Hartman (5):
Revert "tty: pty: Fix race condition between release_one_tty and pty_write"
Merge 5.1-rc3 into tty-next
tty: add SPDX identifiers to Kconfig and Makefiles
tty: fix up a few remaining files without SPDX identifiers
Merge 5.1-rc6 into tty-next
Hariprasad Kelam (1):
tty:serial_core: Spelling mistake
Jakub Wilk (1):
vt: use /dev/vcs (not /dev/vcs0) in comment
Jiri Slaby (1):
TTY: serial_core, add ->install
Johan Hovold (2):
Revert "tty: fix NULL pointer issue when tty_port ops is not set"
tty: update obsolete termios comment
Julien Grall (1):
tty/sysrq: Convert show_lock to raw_spinlock_t
Kangjie Lu (1):
tty: ipwireless: fix missing checks for ioremap
Konstantin Khorenko (1):
tty/vt: avoid high order pages allocation on GIO_UNIMAP ioctl
Lanqing Liu (4):
dt-bindings: serial: sprd: Add clocks and clocks-names properties
serial: sprd: Add power management for the Spreadtrum serial controller
dt-bindings: serial: sprd: Add dma properties to support DMA mode
serial: sprd: Add DMA mode support
Long Cheng (2):
serial: 8250-mtk: add follow control
serial: 8250-mtk: modify baudrate setting
Mauro Carvalho Chehab (1):
docs: serial: convert docs to ReST and rename to *.rst
Pankaj Gupta (1):
virtio_console: initialize vtermno value for ports
Paul Walmsley (2):
dt-bindings: serial: add documentation for the SiFive UART driver
tty: serial: add driver for the SiFive UART
Reinis Danne (1):
tty: vt: keyboard: Allow Unicode compose base char
Sahara (1):
tty: pty: Fix race condition between release_one_tty and pty_write
Sergei Trofimovich (1):
tty/vt: fix write/write race in ioctl(KDSKBSENT) handler
Sergey Organov (1):
tty: serial_core: fix error code returned by uart_register_driver()
Shubhrajyoti Datta (2):
dt-bindings: xilinx-uartps: Add support for cts-override
serial: uartps: Add support for cts-override
Su Bao Cheng (1):
serial: 8250_exar: Adjust IOT2000 matching
Sugaya Taichi (2):
serial: Add Milbeaut serial control
serial: Fix using plain integer instead of Null pointer
Valdis Kletnieks (1):
drivers/tty/tty_jobctrl.c - fix non-kerneldoc comment
Vincent Whitchurch (1):
tty: Add NULL TTY driver
Wei Yongjun (1):
serial: milbeaut_usio: Fix error handling in probe and remove
Yifeng Li (1):
tty: vt.c: Fix TIOCL_BLANKSCREEN console blanking if blankinterval == 0
YueHaibing (2):
serial: 8250_fintek: Make fintek_8250_set_termios static
serial: sprd: Fix a copy-paste err in sprd_request_dma()
.../devicetree/bindings/serial/cdns,uart.txt | 5 +
.../devicetree/bindings/serial/nxp,sc16is7xx.txt | 2 +
.../devicetree/bindings/serial/sifive-serial.txt | 33 +
.../devicetree/bindings/serial/sprd-uart.txt | 17 +-
.../serial/{README.cycladesZ => cyclades_z.rst} | 5 +-
Documentation/serial/{driver => driver.rst} | 115 ++-
Documentation/serial/index.rst | 32 +
Documentation/serial/moxa-smartio | 523 ----------
Documentation/serial/moxa-smartio.rst | 615 ++++++++++++
Documentation/serial/n_gsm.rst | 103 ++
Documentation/serial/n_gsm.txt | 96 --
Documentation/serial/{rocket.txt => rocket.rst} | 152 ++-
.../{serial-iso7816.txt => serial-iso7816.rst} | 21 +-
.../serial/{serial-rs485.txt => serial-rs485.rst} | 22 +-
Documentation/serial/{tty.txt => tty.rst} | 111 +-
MAINTAINERS | 4 +-
drivers/char/virtio_console.c | 3 +-
drivers/tty/Kconfig | 22 +-
drivers/tty/Makefile | 1 +
drivers/tty/hvc/Kconfig | 3 +-
drivers/tty/ipwireless/Makefile | 1 +
drivers/tty/ipwireless/main.c | 8 +
drivers/tty/n_tty.c | 4 +-
drivers/tty/rocket.c | 14 +-
drivers/tty/rocket.h | 1 -
drivers/tty/serdev/Kconfig | 1 +
drivers/tty/serdev/Makefile | 1 +
drivers/tty/serial/8250/8250_exar.c | 7 +-
drivers/tty/serial/8250/8250_fintek.c | 5 +-
drivers/tty/serial/8250/8250_men_mcb.c | 1 +
drivers/tty/serial/8250/8250_mtk.c | 162 ++-
drivers/tty/serial/8250/Kconfig | 1 +
drivers/tty/serial/Kconfig | 54 +-
drivers/tty/serial/Makefile | 2 +
drivers/tty/serial/cpm_uart/Makefile | 1 +
drivers/tty/serial/jsm/Makefile | 1 +
drivers/tty/serial/milbeaut_usio.c | 614 ++++++++++++
drivers/tty/serial/sc16is7xx.c | 34 +-
drivers/tty/serial/serial_core.c | 30 +-
drivers/tty/serial/sifive.c | 1056 ++++++++++++++++++++
drivers/tty/serial/sn_console.c | 1 +
drivers/tty/serial/sprd_serial.c | 501 +++++++++-
drivers/tty/serial/ucc_uart.c | 2 +-
drivers/tty/serial/xilinx_uartps.c | 12 +
drivers/tty/sysrq.c | 6 +-
drivers/tty/tty_io.c | 2 +-
drivers/tty/tty_jobctrl.c | 4 +-
drivers/tty/tty_port.c | 10 +-
drivers/tty/ttynull.c | 109 ++
drivers/tty/vcc.c | 1 +
drivers/tty/vt/.gitignore | 1 +
drivers/tty/vt/consolemap.c | 8 +-
drivers/tty/vt/cp437.uni | 1 +
drivers/tty/vt/defkeymap.c_shipped | 1 +
drivers/tty/vt/defkeymap.map | 1 +
drivers/tty/vt/keyboard.c | 35 +-
drivers/tty/vt/vc_screen.c | 2 +-
drivers/tty/vt/vt.c | 2 -
include/linux/serial_core.h | 2 +-
include/uapi/linux/serial_core.h | 6 +
60 files changed, 3679 insertions(+), 911 deletions(-)
create mode 100644 Documentation/devicetree/bindings/serial/sifive-serial.txt
rename Documentation/serial/{README.cycladesZ => cyclades_z.rst} (85%)
rename Documentation/serial/{driver => driver.rst} (92%)
create mode 100644 Documentation/serial/index.rst
delete mode 100644 Documentation/serial/moxa-smartio
create mode 100644 Documentation/serial/moxa-smartio.rst
create mode 100644 Documentation/serial/n_gsm.rst
delete mode 100644 Documentation/serial/n_gsm.txt
rename Documentation/serial/{rocket.txt => rocket.rst} (68%)
rename Documentation/serial/{serial-iso7816.txt => serial-iso7816.rst} (85%)
rename Documentation/serial/{serial-rs485.txt => serial-rs485.rst} (89%)
rename Documentation/serial/{tty.txt => tty.rst} (74%)
create mode 100644 drivers/tty/serial/milbeaut_usio.c
create mode 100644 drivers/tty/serial/sifive.c
create mode 100644 drivers/tty/ttynull.c
^ permalink raw reply
* Re: [PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip
From: Lee Jones @ 2019-05-08 10:23 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Ralf Baechle, Paul Burton, James Hogan, Dmitry Torokhov,
David S. Miller, Alessandro Zummo, Alexandre Belloni,
Greg Kroah-Hartman, Jiri Slaby, linux-mips, linux-kernel,
linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190409154610.6735-3-tbogendoerfer@suse.de>
On Tue, 09 Apr 2019, Thomas Bogendoerfer wrote:
> SGI IOC3 chip has integrated ethernet, keyboard and mouse interface.
> It also supports connecting a SuperIO chip for serial and parallel
> interfaces. IOC3 is used inside various SGI systemboards and add-on
> cards with different equipped external interfaces.
>
> Support for ethernet and serial interfaces were implemented inside
> the network driver. This patchset moves out the not network related
> parts to a new MFD driver, which takes care of card detection,
> setup of platform devices and interrupt distribution for the subdevices.
>
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> ---
> arch/mips/include/asm/sn/ioc3.h | 345 +++---
> arch/mips/sgi-ip27/ip27-timer.c | 20 -
> drivers/mfd/Kconfig | 13 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/ioc3.c | 802 ++++++++++++++
> drivers/net/ethernet/sgi/Kconfig | 4 +-
> drivers/net/ethernet/sgi/ioc3-eth.c | 1867 ++++++++++++---------------------
> drivers/tty/serial/8250/8250_ioc3.c | 98 ++
> drivers/tty/serial/8250/Kconfig | 11 +
> drivers/tty/serial/8250/Makefile | 1 +
> include/linux/platform_data/ioc3eth.h | 15 +
> 11 files changed, 1762 insertions(+), 1415 deletions(-)
> create mode 100644 drivers/mfd/ioc3.c
> create mode 100644 drivers/tty/serial/8250/8250_ioc3.c
> create mode 100644 include/linux/platform_data/ioc3eth.h
[...]
> +config SGI_MFD_IOC3
> + tristate "SGI IOC3 core driver"
> + depends on PCI && MIPS
> + select MFD_CORE
> + help
> + This option enables basic support for the SGI IOC3-based
> + controller cards. This option does not enable any specific
> + functions on such a card, but provides necessary infrastructure
> + for other drivers to utilize.
> +
> + If you have an SGI Origin, Octane, or a PCI IOC3 card,
> + then say Y. Otherwise say N.
> +
> endmenu
> endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index b4569ed7f3f3..07255e499129 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -246,4 +246,5 @@ obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
> obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
> obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o
> +obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o
>
> diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c
> new file mode 100644
> index 000000000000..ad715805b16e
> --- /dev/null
> +++ b/drivers/mfd/ioc3.c
> @@ -0,0 +1,802 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SGI IOC3 multifunction device driver
> + *
> + * Copyright (C) 2018, 2019 Thomas Bogendoerfer <tbogendoerfer@suse.de>
> + *
> + * Based on work by:
> + * Stanislaw Skowronek <skylark@unaligned.org>
> + * Joshua Kinard <kumba@gentoo.org>
> + * Brent Casavant <bcasavan@sgi.com> - IOC4 master driver
> + * Pat Gefre <pfg@sgi.com> - IOC3 serial port IRQ demuxer
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/core.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/ioc3eth.h>
These should be in alphabetical order.
> +#include <asm/sn/ioc3.h>
> +#include <asm/pci/bridge.h>
> +
> +#define IOC3_ETH BIT(0)
> +#define IOC3_SER BIT(1)
> +#define IOC3_PAR BIT(2)
> +#define IOC3_KBD BIT(3)
> +#define IOC3_M48T35 BIT(4)
> +
> +static int ioc3_serial_id = 1;
> +static int ioc3_eth_id = 1;
> +static int ioc3_kbd_id = 1;
> +static struct mfd_cell ioc3_mfd_cells[5];
> +
> +struct ioc3_board_info {
> + const char *name;
> + int irq_offset;
> + int funcs;
> +};
> +
> +struct ioc3_priv_data {
> + struct ioc3_board_info *info;
> + struct irq_domain *domain;
> + struct ioc3 __iomem *regs;
> + struct pci_dev *pdev;
> + char nic_part[32];
> + char nic_mac[6];
> + int irq_io;
> +};
> +
> +#define MCR_PACK(pulse, sample) (((pulse) << 10) | ((sample) << 2))
> +
> +static int ioc3_nic_wait(u32 __iomem *mcr)
> +{
> + u32 mcr_val;
> +
> + do {
> + mcr_val = readl(mcr);
> + } while (!(mcr_val & 2));
Why 2? Is bit 2 always set on a successful read?
Either way, you should provide a comment to improve readability.
> + return (mcr_val & 1);
Reads just a bit at a time? Again, a comment please.
> +}
> +
> +static int ioc3_nic_reset(u32 __iomem *mcr)
> +{
> + int presence;
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + writel(MCR_PACK(520, 65), mcr);
What do these magic values mean? Best to define them.
> + presence = ioc3_nic_wait(mcr);
> + local_irq_restore(flags);
> +
> + udelay(500);
What are you waiting for? Why 500us?
> + return presence;
> +}
> +
> +static int ioc3_nic_read_bit(u32 __iomem *mcr)
> +{
> + int result;
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + writel(MCR_PACK(6, 13), mcr);
Why 6 and 13?
Define all magic values please.
> + result = ioc3_nic_wait(mcr);
> + local_irq_restore(flags);
> +
> + udelay(100);
Why 100?
> + return result;
> +}
> +
> +static u32 ioc3_nic_read_byte(u32 __iomem *mcr)
> +{
> + u32 result = 0;
> + int i;
> +
> + for (i = 0; i < 8; i++)
> + result = ((result >> 1) | (ioc3_nic_read_bit(mcr) << 7));
> +
> + return result;
> +}
> +
> +static void ioc3_nic_write_bit(u32 __iomem *mcr, int bit)
> +{
> + if (bit)
> + writel(MCR_PACK(6, 110), mcr);
> + else
> + writel(MCR_PACK(80, 30), mcr);
> +
> + ioc3_nic_wait(mcr);
> +}
> +
> +static void ioc3_nic_write_byte(u32 __iomem *mcr, int byte)
> +{
> + int i;
> +
> + for (i = 0; i < 8; i++) {
> + ioc3_nic_write_bit(mcr, byte & 1);
> + byte >>= 1;
> + }
> +}
> +
> +static u64 ioc3_nic_find(u32 __iomem *mcr, int *last, u64 addr)
> +{
> + int a, b, index, disc;
> +
> + ioc3_nic_reset(mcr);
> +
> + /* Search ROM. */
> + ioc3_nic_write_byte(mcr, 0xf0);
What does 0xf0 mean?
Please define it and all magic numbers that follow.
> + /* Algorithm from ``Book of iButton Standards''. */
That's great, but what is it actually doing. Please provide suitable
comments such that the reader doesn't have to painstakingly work it
all out.
> + for (index = 0, disc = 0; index < 64; index++) {
> + a = ioc3_nic_read_bit(mcr);
> + b = ioc3_nic_read_bit(mcr);
> +
> + if (unlikely(a && b)) {
> + pr_warn("ioc3: NIC search failed.\n");
> + *last = 0;
> + return 0;
> + }
> +
> + if (!a && !b) {
> + if (index == *last)
> + addr |= 1UL << index;
> + else if (index > *last) {
> + addr &= ~(1UL << index);
> + disc = index;
> + } else if ((addr & (1UL << index)) == 0)
> + disc = index;
> + ioc3_nic_write_bit(mcr, (addr >> index) & 1);
> + continue;
> + } else {
> + if (a)
> + addr |= (1UL << index);
> + else
> + addr &= ~(1UL << index);
> + ioc3_nic_write_bit(mcr, a);
> + continue;
> + }
> + }
> + *last = disc;
> + return addr;
> +}
> +
> +static void ioc3_nic_addr(u32 __iomem *mcr, u64 addr)
> +{
> + int index;
> +
> + ioc3_nic_reset(mcr);
> + ioc3_nic_write_byte(mcr, 0xf0);
> +
> + for (index = 0; index < 64; index++) {
> + ioc3_nic_read_bit(mcr);
> + ioc3_nic_read_bit(mcr);
> + ioc3_nic_write_bit(mcr, ((addr >> index) & 1));
> + }
What is this function doing? Setting the NIC address?
Why are the 2 sequential reads required before each bit write?
> +}
> +
> +static void crc16_byte(u32 *crc, u8 db)
> +{
> + int i;
> +
> + for (i = 0; i < 8; i++) {
> + *crc <<= 1;
> + if ((db ^ (*crc >> 16)) & 1)
> + *crc ^= 0x8005;
> + db >>= 1;
> + }
> + *crc &= 0xffff;
> +}
> +
> +static u32 crc16_area(u8 *dbs, int size, u32 crc)
> +{
> + while (size--)
> + crc16_byte(&crc, *(dbs++));
> +
> + return crc;
> +}
> +
> +static void crc8_byte(u32 *crc, u8 db)
> +{
> + int i, f;
> +
> + for (i = 0; i < 8; i++) {
> + f = ((*crc ^ db) & 1);
> + *crc >>= 1;
> + db >>= 1;
> + if (f)
> + *crc ^= 0x8c;
> + }
> + *crc &= 0xff;
> +}
> +
> +static u32 crc8_addr(u64 addr)
> +{
> + u32 crc = 0;
> + int i;
> +
> + for (i = 0; i < 64; i += 8)
> + crc8_byte(&crc, addr >> i);
> + return crc;
> +}
Not looked into these in any detail, but are you not able to use the
CRC functions already provided by the kernel?
> +static void ioc3_read_redir_page(u32 __iomem *mcr, u64 addr, int page,
What is a redir page?
> + u8 *redir, u8 *data)
> +{
> + int loops = 16, i;
> +
> + while (redir[page] != 0xff) {
> + page = (redir[page] ^ 0xff);
> + loops--;
> + if (unlikely(loops < 0)) {
> + pr_err("ioc3: NIC circular redirection\n");
> + return;
> + }
> + }
> +
> + loops = 3;
> + while (loops > 0) {
> + ioc3_nic_addr(mcr, addr);
> + ioc3_nic_write_byte(mcr, 0xf0);
> + ioc3_nic_write_byte(mcr, (page << 5) & 0xe0);
> + ioc3_nic_write_byte(mcr, (page >> 3) & 0x1f);
> +
> + for (i = 0; i < 0x20; i++)
> + data[i] = ioc3_nic_read_byte(mcr);
> +
> + if (crc16_area(data, 0x20, 0) == 0x800d)
> + return;
> +
> + loops--;
> + }
> +
> + pr_err("ioc3: CRC error in data page\n");
> + for (i = 0; i < 0x20; i++)
> + data[i] = 0x00;
Comments required throughout.
> +}
> +
> +static void ioc3_read_redir_map(u32 __iomem *mcr, u64 addr, u8 *redir)
> +{
> + int i, j, crc_ok, loops = 3;
> + u32 crc;
> +
> + while (loops > 0) {
> + crc_ok = 1;
> + ioc3_nic_addr(mcr, addr);
> + ioc3_nic_write_byte(mcr, 0xaa);
> + ioc3_nic_write_byte(mcr, 0x00);
> + ioc3_nic_write_byte(mcr, 0x01);
> +
> + for (i = 0; i < 64; i += 8) {
> + for (j = 0; j < 8; j++)
> + redir[i + j] = ioc3_nic_read_byte(mcr);
> +
> + crc = crc16_area(redir + i, 8, i == 0 ? 0x8707 : 0);
> +
> + crc16_byte(&crc, ioc3_nic_read_byte(mcr));
> + crc16_byte(&crc, ioc3_nic_read_byte(mcr));
> +
> + if (crc != 0x800d)
> + crc_ok = 0;
> + }
> + if (crc_ok)
> + return;
> + loops--;
> + }
> + pr_err("ioc3: CRC error in redirection page\n");
> + for (i = 0; i < 64; i++)
> + redir[i] = 0xff;
As above.
> +}
> +
> +static void ioc3_read_nic(struct ioc3_priv_data *ipd, u32 __iomem *mcr,
> + u64 addr)
> +{
> + u8 redir[64];
> + u8 data[64], part[32];
> + int i, j;
> +
> + /* Read redirections */
> + ioc3_read_redir_map(mcr, addr, redir);
> +
> + /* Read data pages */
> + ioc3_read_redir_page(mcr, addr, 0, redir, data);
> + ioc3_read_redir_page(mcr, addr, 1, redir, (data + 32));
> +
> + /* Assemble the part # */
> + j = 0;
> + for (i = 0; i < 19; i++)
> + if (data[i + 11] != ' ')
> + part[j++] = data[i + 11];
> +
> + for (i = 0; i < 6; i++)
> + if (data[i + 32] != ' ')
> + part[j++] = data[i + 32];
> +
> + part[j] = 0;
> +
> + /* Skip Octane (IP30) power supplies */
> + if (!(strncmp(part, "060-0035-", 9)) ||
> + !(strncmp(part, "060-0038-", 9)) ||
> + !(strncmp(part, "060-0028-", 9)))
> + return;
> +
> + strlcpy(ipd->nic_part, part, sizeof(ipd->nic_part));
> +}
> +
> +static void ioc3_read_mac(struct ioc3_priv_data *ipd, u64 addr)
> +{
> + int i, loops = 3;
> + u8 data[13];
> + u32 __iomem *mcr = &ipd->regs->mcr;
> +
> + while (loops > 0) {
> + ioc3_nic_addr(mcr, addr);
> + ioc3_nic_write_byte(mcr, 0xf0);
> + ioc3_nic_write_byte(mcr, 0x00);
> + ioc3_nic_write_byte(mcr, 0x00);
> + ioc3_nic_read_byte(mcr);
> +
> + for (i = 0; i < 13; i++)
> + data[i] = ioc3_nic_read_byte(mcr);
> +
> + if (crc16_area(data, 13, 0) == 0x800d) {
> + for (i = 10; i > 4; i--)
> + ipd->nic_mac[10 - i] = data[i];
> + return;
> + }
> + loops--;
> + }
> +
> + pr_err("ioc3: CRC error in MAC address\n");
> + for (i = 0; i < 6; i++)
> + ipd->nic_mac[i] = 0x00;
> +}
> +
> +static void ioc3_probe_nic(struct ioc3_priv_data *ipd, u32 __iomem *mcr)
> +{
> + int save = 0, loops = 3;
> + u64 first, addr;
> +
> + while (loops > 0) {
> + ipd->nic_part[0] = 0;
> + first = ioc3_nic_find(mcr, &save, 0);
> + addr = first;
> +
> + if (unlikely(!first))
> + return;
> +
> + while (1) {
> + if (crc8_addr(addr))
> + break;
> +
> + switch (addr & 0xff) {
> + case 0x0b:
> + ioc3_read_nic(ipd, mcr, addr);
> + break;
> + case 0x09:
> + case 0x89:
> + case 0x91:
> + ioc3_read_mac(ipd, addr);
> + break;
> + }
> +
> + addr = ioc3_nic_find(mcr, &save, addr);
> + if (addr == first)
> + return;
> + }
> + loops--;
> + }
> + pr_err("ioc3: CRC error in NIC address\n");
> +}
This all looks like networking code. If this is the case, it should
be moved to drivers/networking or similar.
> +static void ioc3_irq_ack(struct irq_data *d)
> +{
> + struct ioc3_priv_data *ipd = irq_data_get_irq_chip_data(d);
> + unsigned int hwirq = irqd_to_hwirq(d);
> +
> + writel(BIT(hwirq), &ipd->regs->sio_ir);
> +}
> +
> +static void ioc3_irq_mask(struct irq_data *d)
> +{
> + struct ioc3_priv_data *ipd = irq_data_get_irq_chip_data(d);
> + unsigned int hwirq = irqd_to_hwirq(d);
> +
> + writel(BIT(hwirq), &ipd->regs->sio_iec);
> +}
> +
> +static void ioc3_irq_unmask(struct irq_data *d)
> +{
> + struct ioc3_priv_data *ipd = irq_data_get_irq_chip_data(d);
> + unsigned int hwirq = irqd_to_hwirq(d);
> +
> + writel(BIT(hwirq), &ipd->regs->sio_ies);
> +}
> +
> +static struct irq_chip ioc3_irq_chip = {
> + .name = "IOC3",
> + .irq_ack = ioc3_irq_ack,
> + .irq_mask = ioc3_irq_mask,
> + .irq_unmask = ioc3_irq_unmask,
> +};
> +
> +#define IOC3_LVL_MASK (BIT(0) | BIT(2) | BIT(6) | BIT(9) | BIT(11) | BIT(15))
You need to define what each of these bits are. BIT(2) doesn't tell
the reader anything, meaning the code is unreadable.
> +static int ioc3_irq_domain_map(struct irq_domain *d, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + if (BIT(hwirq) & IOC3_LVL_MASK)
Comment needed.
> + irq_set_chip_and_handler(irq, &ioc3_irq_chip, handle_level_irq);
> + else
> + irq_set_chip_and_handler(irq, &ioc3_irq_chip, handle_edge_irq);
> +
> + irq_set_chip_data(irq, d->host_data);
> + return 0;
> +}
> +
> +
Drop the superfluous '\n'.
> +static const struct irq_domain_ops ioc3_irq_domain_ops = {
> + .map = ioc3_irq_domain_map,
> +};
> +
> +static void ioc3_irq_handler(struct irq_desc *desc)
> +{
> + struct irq_domain *domain = irq_desc_get_handler_data(desc);
> + struct ioc3_priv_data *ipd = domain->host_data;
> + struct ioc3 __iomem *regs = ipd->regs;
> + unsigned int irq = 0;
Why does these need to be pre-assigned?
> + u32 pending;
> +
> + pending = readl(®s->sio_ir);
> + pending &= readl(®s->sio_ies);
Comment required.
> + if (pending)
> + irq = irq_find_mapping(domain, __ffs(pending));
> + else if (!ipd->info->irq_offset &&
> + (readl(®s->eth.eisr) & readl(®s->eth.eier)))
Comment required.
> + irq = irq_find_mapping(domain, 23);
> +
> + if (irq)
> + generic_handle_irq(irq);
> + else
> + spurious_interrupt();
> +}
> +
> +static struct resource ioc3_uarta_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
You are the first user of offsetof() in MFD. Could you tell me why
it's required please?
> + sizeof_field(struct ioc3, sregs.uarta)),
> + DEFINE_RES_IRQ(6)
Please define all magic numbers.
> +};
> +
> +static struct resource ioc3_uartb_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uartb),
> + sizeof_field(struct ioc3, sregs.uartb)),
> + DEFINE_RES_IRQ(15)
> +};
> +
> +static struct resource ioc3_kbd_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, serio),
> + sizeof_field(struct ioc3, serio)),
> + DEFINE_RES_IRQ(22)
> +};
> +
> +static struct resource ioc3_eth_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, eth),
> + sizeof_field(struct ioc3, eth)),
> + DEFINE_RES_MEM(offsetof(struct ioc3, ssram),
> + sizeof_field(struct ioc3, ssram)),
> + DEFINE_RES_IRQ(0)
> +};
> +
> +static struct ioc3eth_platform_data ioc3_eth_platform_data;
I doubt you'll need this in here. The MAC address info will need to
be moved out to a networking driver of some sort.
> +#ifdef CONFIG_SGI_IP27
#ifery in C files is highly discouraged. Please remove them.
> +static struct resource ioc3_rtc_resources[] = {
> + DEFINE_RES_MEM(IOC3_BYTEBUS_DEV0, 32768)
Define all magic numbers please.
> +};
> +
> +static struct ioc3_board_info ip27_baseio_info = {
> + .name = "IP27 BaseIO",
> + .funcs = IOC3_ETH | IOC3_SER | IOC3_M48T35,
> + .irq_offset = 2
> +};
> +
> +static struct ioc3_board_info ip27_baseio6g_info = {
> + .name = "IP27 BaseIO6G",
> + .funcs = IOC3_ETH | IOC3_SER | IOC3_KBD | IOC3_M48T35,
> + .irq_offset = 2
> +};
> +
> +static struct ioc3_board_info ip27_mio_info = {
> + .name = "MIO",
> + .funcs = IOC3_SER | IOC3_PAR | IOC3_KBD,
> + .irq_offset = 0
> +};
> +
> +static struct ioc3_board_info ip29_baseio_info = {
> + .name = "IP29 System Board",
> + .funcs = IOC3_ETH | IOC3_SER | IOC3_PAR | IOC3_KBD | IOC3_M48T35,
> + .irq_offset = 1
> +};
> +
> +#endif /* CONFIG_SGI_IP27 */
> +
> +static struct ioc3_board_info ioc3_menet_info = {
> + .name = "MENET",
> + .funcs = IOC3_ETH | IOC3_SER,
> + .irq_offset = 4
> +};
> +
> +static struct ioc3_board_info ioc3_cad_duo_info = {
> + .name = "CAD DUO",
> + .funcs = IOC3_ETH | IOC3_KBD,
> + .irq_offset = 0
> +};
Please drop all of these and statically create the MFD cells like
almost all other MFD drivers do.
> +#define IOC3_BOARD(_partno, _info) { .info = _info, .match = _partno }
> +
> +static struct {
> + struct ioc3_board_info *info;
> + const char *match;
> +} ioc3_boards[] = {
> +#ifdef CONFIG_SGI_IP27
> + IOC3_BOARD("030-0734-", &ip27_baseio6g_info),
> + IOC3_BOARD("030-1023-", &ip27_baseio_info),
> + IOC3_BOARD("030-1124-", &ip27_baseio_info),
> + IOC3_BOARD("030-1025-", &ip29_baseio_info),
> + IOC3_BOARD("030-1244-", &ip29_baseio_info),
> + IOC3_BOARD("030-1389-", &ip29_baseio_info),
> + IOC3_BOARD("030-0880-", &ip27_mio_info),
> +#endif
> + IOC3_BOARD("030-0873-", &ioc3_menet_info),
> + IOC3_BOARD("030-1155-", &ioc3_cad_duo_info),
> +};
> +
> +static int ioc3_identify(struct ioc3_priv_data *idp)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ioc3_boards); i++)
> + if (!strncmp(idp->nic_part, ioc3_boards[i].match,
> + strlen(ioc3_boards[i].match))) {
> + idp->info = ioc3_boards[i].info;
> + return 0;
> + }
> +
> + return -1;
Please return a proper Linux error code.
> +}
> +
> +static void ioc3_create_devices(struct ioc3_priv_data *ipd)
> +{
> + struct mfd_cell *cell;
> +
> + memset(ioc3_mfd_cells, 0, sizeof(ioc3_mfd_cells));
> + cell = ioc3_mfd_cells;
> +
> + if (ipd->info->funcs & IOC3_ETH) {
> + memcpy(ioc3_eth_platform_data.mac_addr, ipd->nic_mac,
> + sizeof(ioc3_eth_platform_data.mac_addr));
Better to pull the MAC address from within the Ethernet driver.
> + cell->name = "ioc3-eth";
> + cell->id = ioc3_eth_id++;
> + cell->resources = ioc3_eth_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_eth_resources);
> + cell->platform_data = &ioc3_eth_platform_data;
> + cell->pdata_size = sizeof(ioc3_eth_platform_data);
Please define all of this in a static struct, external to this
function.
> + if (ipd->info->irq_offset) {
> + /*
> + * Ethernet interrupt is on an extra interrupt
> + * not inside the irq domain, so we need an
> + * extra mfd_add_devices without the domain
> + * argument
> + */
> + ioc3_eth_resources[2].start = ipd->pdev->irq;
> + ioc3_eth_resources[2].end = ipd->pdev->irq;
Using '2' is fragile.
In this case, seeing as you're using the parent's IRQ, you can obtain
the IRQ using the usual platform_*() handlers, using pdev->parent.
> + mfd_add_devices(&ipd->pdev->dev, -1, cell, 1,
Don't use -1, use the defines instead.
Instead of 1, use ARRAY_SIZE() once the cells are defined statically.
> + &ipd->pdev->resource[0], 0, NULL);
> + memset(cell, 0, sizeof(*cell));
> + } else {
> + /* fake hwirq in domain */
What does this mean?
> + ioc3_eth_resources[2].start = 23;
> + ioc3_eth_resources[2].end = 23;
> + cell++;
> + }
> + }
> + if (ipd->info->funcs & IOC3_SER) {
> + writel(GPCR_UARTA_MODESEL | GPCR_UARTB_MODESEL,
> + &ipd->regs->gpcr_s);
> + writel(0, &ipd->regs->gppr[6]);
> + writel(0, &ipd->regs->gppr[7]);
> + udelay(100);
> + writel(readl(&ipd->regs->port_a.sscr) & ~SSCR_DMA_EN,
> + &ipd->regs->port_a.sscr);
> + writel(readl(&ipd->regs->port_b.sscr) & ~SSCR_DMA_EN,
> + &ipd->regs->port_b.sscr);
> + udelay(1000);
No idea what any of this does.
It looks like it belongs in the serial driver (and needs comments).
> + cell->name = "ioc3-serial8250";
> + cell->id = ioc3_serial_id++;
> + cell->resources = ioc3_uarta_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_uarta_resources);
> + cell++;
> + cell->name = "ioc3-serial8250";
> + cell->id = ioc3_serial_id++;
> + cell->resources = ioc3_uartb_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_uartb_resources);
> + cell++;
Please define this statically.
> + }
> + if (ipd->info->funcs & IOC3_KBD) {
> + cell->name = "ioc3-kbd",
> + cell->id = ioc3_kbd_id++;
> + cell->resources = ioc3_kbd_resources,
> + cell->num_resources = ARRAY_SIZE(ioc3_kbd_resources),
> + cell++;
As above.
> + }
> +#if defined(CONFIG_SGI_IP27)
What is this? Can't you obtain this dynamically by probing the H/W?
> + if (ipd->info->funcs & IOC3_M48T35) {
> + cell->name = "rtc-m48t35";
> + cell->id = -1;
> + cell->resources = ioc3_rtc_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_rtc_resources);
> + cell++;
Static please.
> + }
> +#endif
> + mfd_add_devices(&ipd->pdev->dev, -1, ioc3_mfd_cells,
Use the defines.
> + cell - ioc3_mfd_cells, &ipd->pdev->resource[0],
?
> + 0, ipd->domain);
> +}
> +
> +static int ioc3_mfd_probe(struct pci_dev *pdev,
> + const struct pci_device_id *pci_id)
> +{
> + struct ioc3_priv_data *ipd;
> + int err, ret = -ENODEV, io_irqno;
> + struct ioc3 __iomem *regs;
> + struct irq_domain *domain;
> + struct fwnode_handle *fn;
> +
> + err = pci_enable_device(pdev);
> + if (err)
> + return err;
> +
> + pci_write_config_byte(pdev, PCI_LATENCY_TIMER, 64);
> + pci_set_master(pdev);
> +
> + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> + if (ret) {
> + dev_warn(&pdev->dev, "Warning: couldn_t set 64-bit DMA mask\n");
> + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> + if (ret) {
> + dev_err(&pdev->dev, "Can't set DMA mask, aborting\n");
> + return ret;
> + }
> + }
> +
> + /* Set up per-IOC3 data */
> + ipd = kzalloc(sizeof(struct ioc3_priv_data), GFP_KERNEL);
Does PCI not provide managed (devm_*() like) helpers?
> + if (!ipd) {
> + ret = -ENOMEM;
> + goto out_disable_device;
> + }
> + ipd->pdev = pdev;
'\n'
> + /*
> + * Map all IOC3 registers. These are shared between subdevices
> + * so the main IOC3 module manages them.
> + */
> + regs = pci_ioremap_bar(pdev, 0);
> + if (!regs) {
> + pr_warn("ioc3: Unable to remap PCI BAR for %s.\n",
> + pci_name(pdev));
> + goto out_free_ipd;
> + }
> + ipd->regs = regs;
> +
> + /* Track PCI-device specific data */
> + pci_set_drvdata(pdev, ipd);
Do you don't need 'ipd->pdev = pdev' then.
> + writel(GPCR_MLAN_EN, &ipd->regs->gpcr_s);
> + ioc3_probe_nic(ipd, ®s->mcr);
This should probably be moved to the networking driver.
> +#ifdef CONFIG_SGI_IP27
No #ifery in MFD c-files please.
> + /* BaseIO NIC is attached to bridge */
> + if (!ipd->nic_part[0]) {
> + struct bridge_controller *bc = BRIDGE_CONTROLLER(pdev->bus);
> +
> + ioc3_probe_nic(ipd, &bc->base->b_nic);
> + }
> +#endif
> +
> + if (ioc3_identify(ipd)) {
> + pr_err("ioc3: part: [%s] unknown card\n", ipd->nic_part);
> + goto out_iounmap;
> + }
> +
> + pr_info("ioc3: part: [%s] %s\n", ipd->nic_part, ipd->info->name);
> +
> + /* Clear IRQs */
A comment! I just fell off my chair! =;-)
> + writel(~0, ®s->sio_iec);
> + writel(~0, ®s->sio_ir);
> + writel(0, ®s->eth.eier);
> + writel(~0, ®s->eth.eisr);
> +
> + if (ipd->info->irq_offset) {
What does this really signify?
> + struct pci_host_bridge *hbrg = pci_find_host_bridge(pdev->bus);
> +
> + io_irqno = hbrg->map_irq(pdev,
> + PCI_SLOT(pdev->devfn) + ipd->info->irq_offset, 0);
> + } else {
> + io_irqno = pdev->irq;
> + }
> + ipd->irq_io = io_irqno;
> +
> + fn = irq_domain_alloc_named_fwnode("IOC3");
> + if (!fn)
> + goto out_iounmap;
> +
> + domain = irq_domain_create_linear(fn, 24, &ioc3_irq_domain_ops, ipd);
> + irq_domain_free_fwnode(fn);
> + if (!domain)
> + goto out_iounmap;
> + ipd->domain = domain;
> +
> + irq_set_chained_handler_and_data(io_irqno, ioc3_irq_handler, domain);
> +
> + ioc3_create_devices(ipd);
> +
> + return 0;
> +
> +out_iounmap:
> + iounmap(ipd->regs);
> +
> +out_free_ipd:
> + kfree(ipd);
> +
> +out_disable_device:
> + pci_disable_device(pdev);
> + return ret;
> +}
> +
> +static void ioc3_mfd_remove(struct pci_dev *pdev)
> +{
> + struct ioc3_priv_data *ipd;
> +
> + ipd = pci_get_drvdata(pdev);
> +
> + /* Clear and disable all IRQs */
> + writel(~0, &ipd->regs->sio_iec);
> + writel(~0, &ipd->regs->sio_ir);
> +
> + /* Release resources */
> + irq_domain_remove(ipd->domain);
> + free_irq(ipd->irq_io, (void *)ipd);
> + iounmap(ipd->regs);
> +
> + pci_disable_device(pdev);
> +
> + kfree(ipd);
> +}
> +
> +static struct pci_device_id ioc3_mfd_id_table[] = {
> + { PCI_VENDOR_ID_SGI, PCI_DEVICE_ID_SGI_IOC3, PCI_ANY_ID, PCI_ANY_ID },
> + { 0, },
> +};
> +MODULE_DEVICE_TABLE(pci, ioc3_mfd_id_table);
> +
> +static struct pci_driver ioc3_mfd_driver = {
> + .name = "IOC3",
> + .id_table = ioc3_mfd_id_table,
> + .probe = ioc3_mfd_probe,
> + .remove = ioc3_mfd_remove,
> +};
> +
> +module_pci_driver(ioc3_mfd_driver);
> +
> +MODULE_AUTHOR("Thomas Bogendoerfer <tbogendoerfer@suse.de>");
> +MODULE_DESCRIPTION("SGI IOC3 MFD driver");
> +MODULE_LICENSE("GPL");
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Andy Shevchenko @ 2019-05-07 15:08 UTC (permalink / raw)
To: Esben Haabendal
Cc: Lee Jones, 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: <87k1f2jvyd.fsf@haabendal.dk>
On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > On Tue, May 07, 2019 at 01:35:58PM +0200, Esben Haabendal wrote:
> >> Lee Jones <lee.jones@linaro.org> writes:
> >> > On Thu, 02 May 2019, Esben Haabendal wrote:
> >> >
> >> >> Could you help clarify whether or not this patch is trying to do
> >> >> something odd/wrong?
> >> >>
> >> >> I might be misunderstanding Andy (probably is), but the discussion
> >> >> revolves around the changes I propose where I change the serial8250
> >> >> driver to use platform_get_resource() in favour of
> >> >> request_mem_region()/release_mem_region().
> >> >
> >> > Since 'serial8250' is registered as a platform device, I don't see any
> >> > reason why it shouldn't have the capability to obtain its memory
> >> > regions from the platform_get_*() helpers.
> >>
> >> Good to hear. That is exactly what I am trying do with this patch.
> >>
> >> @Andy: If you still don't like my approach, could you please advice an
> >> acceptable method for improving the serial8250 driver to allow the use
> >> of platform_get_*() helpers?
> >
> > I still don't get why you need this.
>
> Because platform_get_resource() is a generally available and useful
> helper function for working with platform_device resources, that the
> current standard serial8250 driver does not support.
>
> I am uncertain if I still haven't convinced you that current serial8250
> driver does not work with platform_get_resource(), or if you believe
> that it really should not support it.
I believe there is no need to do this support.
Most of the platform code that uses it is quite legacy, and all under arch/
ideally should be converted to use Device Tree.
> > If it's MFD, you may use "serial8250" with a given platform data like
> > dozens of current users do.
>
> There is only one in-tree mfd driver using "serial8250", the sm501.c
> driver. And that driver predates the mfd framework (mfd-core.c) by a
> year, and does not use any of the mfd-core functionality.
So, does it have an issue?
> I want to use the mfd-core provided handling of resource splitting,
> because it makes it easier to handle splitting of a single memory
> resource as defined by a PCI BAR in this case. And the other drivers I
> need to use all support/use platform_get_resource(), so it would even
> have an impact on the integration of that if I cannot use mfd resource
> splitting with serial8250.
I tired to repeat, that is OKAY! You *may* split and supply resources to the
drivers, nothing prevents you to do that with current code base.
Do you see any problem with that? What is that problem?
If you would like utilize serial8250, just provide a platform data for it.
> > Another approach is to use 8250 library, thus, creating a specific glue driver
> > (like all 8250_* do).
>
> As mentioned, I think this is a bad approach, and I would prefer to
> improve the "serial8250" driver instead. But if you insist, what should
> I call such a driver? It needs a platform_driver name, for use when
> matching with platform_device devices. And it would support exactly the
> same hardware as the current "serial8250" driver.
If you need some specifics, you create a driver with whatever name
suits the IP in question. Nevertheless, if it's simple generic 8250, nothing
needs to be added, except platform data, see above.
> > Yes, I understand that 8250 driver is full of quirks and not modern approaches
> > to do one or another thing. Unfortunately it's not too easy to fix it without
> > uglifying code and doing some kind of ping-pong thru the conversion. I don't
> > think it worth to do it in the current state of affairs. Though, cleaning up
> > the core part from the quirks and custom pieces would make this task
> > achievable.
>
> I think it should be possible and worthwhile to improve serial8250
> driver with support for using platform_device resources
> (platform_get_resource() helper).
I simple can't understand why it's needed. What problem would it solve which
can't be solved with existing code base?
> If we could stop discussing if it is a proper thing to do, we could try
> to find a good way to do it instead.
> > Btw, what exact IP of UART do you have implemented there?
>
> It is an XPS 16550 UART (v3.00a).
> https://www.xilinx.com/support/documentation/ip_documentation/xps_uart16550.pdf
So, briefly looking at it I didn't find any deviations from a standard 16550a.
Also there are two drivers mentioned Xilinx, though I'm pretty sure it's not
your case.
Since you have more than one of them, it's even smaller to use current
infrastructure to enumerate them using only one serial8250 description.
See plenty examples in the Linux kernel, such as 8250_exar_st16c554.c.
That is what you may just modify for your needs and put inside your MFD.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function
From: Lee Jones @ 2019-05-07 13:38 UTC (permalink / raw)
To: Esben Haabendal
Cc: linux-serial, Greg Kroah-Hartman, Jiri Slaby, Nishanth Menon,
Vignesh R, Tony Lindgren, Lokesh Vutla, Florian Fainelli,
linux-kernel
In-Reply-To: <87o94ejwrx.fsf@haabendal.dk>
On Tue, 07 May 2019, Esben Haabendal wrote:
> Lee Jones <lee.jones@linaro.org> writes:
>
> > On Fri, 26 Apr 2019, Esben Haabendal wrote:
> >
> >> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
> >> to an MFD driver.
> >>
> >> When calling mfd_add_device(), platform_data should be a pointer to a
> >> struct plat_serial8250_port, with proper settings like .flags, .type,
> >> .iotype, .regshift and .uartclk. Memory (or ioport) and IRQ should be
> >> passed as cell resources.
> >
> > What? No, please!
> >
> > If you *must* create a whole driver just to be able to use
> > platform_*() helpers (which I don't think you should), then please
> > call it something else. This doesn't have anything to do with MFD.
>
> True.
>
> I really don't think it is a good idea to create a whole driver just to
> be able to use platform_get_*() helpers. And if I am forced to do this,
> because I am unable to convince Andy to improve the standard serial8250
> driver to support that, it should be called MFD. The driver would be
I assume you mean "shouldn't"?
> generally usable for all usecases where platform_get_*() works.
>
> I don't have any idea what to call such a driver. It really would just
> be a fork of the current serial8250 driver, just allowing use of
> platform_get_*(), supporting exactly the same hardware.
>
> I am still hoping that we can find a way to improve serial8250 to be
> usable in these cases.
Me too.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-05-07 12:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, 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: <20190507115325.GV9224@smile.fi.intel.com>
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> On Tue, May 07, 2019 at 01:35:58PM +0200, Esben Haabendal wrote:
>> Lee Jones <lee.jones@linaro.org> writes:
>> > On Thu, 02 May 2019, Esben Haabendal wrote:
>> >
>> >> Could you help clarify whether or not this patch is trying to do
>> >> something odd/wrong?
>> >>
>> >> I might be misunderstanding Andy (probably is), but the discussion
>> >> revolves around the changes I propose where I change the serial8250
>> >> driver to use platform_get_resource() in favour of
>> >> request_mem_region()/release_mem_region().
>> >
>> > Since 'serial8250' is registered as a platform device, I don't see any
>> > reason why it shouldn't have the capability to obtain its memory
>> > regions from the platform_get_*() helpers.
>>
>> Good to hear. That is exactly what I am trying do with this patch.
>>
>> @Andy: If you still don't like my approach, could you please advice an
>> acceptable method for improving the serial8250 driver to allow the use
>> of platform_get_*() helpers?
>
> I still don't get why you need this.
Because platform_get_resource() is a generally available and useful
helper function for working with platform_device resources, that the
current standard serial8250 driver does not support.
I am uncertain if I still haven't convinced you that current serial8250
driver does not work with platform_get_resource(), or if you believe
that it really should not support it.
> If it's MFD, you may use "serial8250" with a given platform data like
> dozens of current users do.
There is only one in-tree mfd driver using "serial8250", the sm501.c
driver. And that driver predates the mfd framework (mfd-core.c) by a
year, and does not use any of the mfd-core functionality.
I want to use the mfd-core provided handling of resource splitting,
because it makes it easier to handle splitting of a single memory
resource as defined by a PCI BAR in this case. And the other drivers I
need to use all support/use platform_get_resource(), so it would even
have an impact on the integration of that if I cannot use mfd resource
splitting with serial8250.
> Another approach is to use 8250 library, thus, creating a specific glue driver
> (like all 8250_* do).
As mentioned, I think this is a bad approach, and I would prefer to
improve the "serial8250" driver instead. But if you insist, what should
I call such a driver? It needs a platform_driver name, for use when
matching with platform_device devices. And it would support exactly the
same hardware as the current "serial8250" driver.
> Yes, I understand that 8250 driver is full of quirks and not modern approaches
> to do one or another thing. Unfortunately it's not too easy to fix it without
> uglifying code and doing some kind of ping-pong thru the conversion. I don't
> think it worth to do it in the current state of affairs. Though, cleaning up
> the core part from the quirks and custom pieces would make this task
> achievable.
I think it should be possible and worthwhile to improve serial8250
driver with support for using platform_device resources
(platform_get_resource() helper).
If we could stop discussing if it is a proper thing to do, we could try
to find a good way to do it instead.
> I'm also puzzled why you don't use FPGA manager which should handle, as far as
> I understand, very flexible configurations of FPGAs.
FPGA manager is for programming FPGA's. The FPGA's used in this project
read their configuration from EEPROM.
I don't see any overlap of FPGA manager with MFD. They server
completely different purposes, and could very well both be used for the
same FPGA's.
> Btw, what exact IP of UART do you have implemented there?
It is an XPS 16550 UART (v3.00a).
https://www.xilinx.com/support/documentation/ip_documentation/xps_uart16550.pdf
There are 5 of them in one FPGA, together with 3 XPS LL TEMAC Ethernet
IP blocks, an IRQ controller, and a number of custom IP blocks.
/Esben
^ permalink raw reply
* Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function
From: Esben Haabendal @ 2019-05-07 12:04 UTC (permalink / raw)
To: Lee Jones
Cc: linux-serial, Greg Kroah-Hartman, Jiri Slaby, Nishanth Menon,
Vignesh R, Tony Lindgren, Lokesh Vutla, Florian Fainelli,
linux-kernel
In-Reply-To: <20190507114905.GB29524@dell>
Lee Jones <lee.jones@linaro.org> writes:
> On Fri, 26 Apr 2019, Esben Haabendal wrote:
>
>> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
>> to an MFD driver.
>>
>> When calling mfd_add_device(), platform_data should be a pointer to a
>> struct plat_serial8250_port, with proper settings like .flags, .type,
>> .iotype, .regshift and .uartclk. Memory (or ioport) and IRQ should be
>> passed as cell resources.
>
> What? No, please!
>
> If you *must* create a whole driver just to be able to use
> platform_*() helpers (which I don't think you should), then please
> call it something else. This doesn't have anything to do with MFD.
True.
I really don't think it is a good idea to create a whole driver just to
be able to use platform_get_*() helpers. And if I am forced to do this,
because I am unable to convince Andy to improve the standard serial8250
driver to support that, it should be called MFD. The driver would be
generally usable for all usecases where platform_get_*() works.
I don't have any idea what to call such a driver. It really would just
be a fork of the current serial8250 driver, just allowing use of
platform_get_*(), supporting exactly the same hardware.
I am still hoping that we can find a way to improve serial8250 to be
usable in these cases.
/Esben
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Andy Shevchenko @ 2019-05-07 11:53 UTC (permalink / raw)
To: Esben Haabendal
Cc: Lee Jones, 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: <87sgtqjy3l.fsf@haabendal.dk>
On Tue, May 07, 2019 at 01:35:58PM +0200, Esben Haabendal wrote:
> Lee Jones <lee.jones@linaro.org> writes:
> > On Thu, 02 May 2019, Esben Haabendal wrote:
> >
> >> Could you help clarify whether or not this patch is trying to do
> >> something odd/wrong?
> >>
> >> I might be misunderstanding Andy (probably is), but the discussion
> >> revolves around the changes I propose where I change the serial8250
> >> driver to use platform_get_resource() in favour of
> >> request_mem_region()/release_mem_region().
> >
> > Since 'serial8250' is registered as a platform device, I don't see any
> > reason why it shouldn't have the capability to obtain its memory
> > regions from the platform_get_*() helpers.
>
> Good to hear. That is exactly what I am trying do with this patch.
>
> @Andy: If you still don't like my approach, could you please advice an
> acceptable method for improving the serial8250 driver to allow the use
> of platform_get_*() helpers?
I still don't get why you need this.
If it's MFD, you may use "serial8250" with a given platform data like dozens of
current users do.
Another approach is to use 8250 library, thus, creating a specific glue driver
(like all 8250_* do).
Yes, I understand that 8250 driver is full of quirks and not modern approaches
to do one or another thing. Unfortunately it's not too easy to fix it without
uglifying code and doing some kind of ping-pong thru the conversion. I don't
think it worth to do it in the current state of affairs. Though, cleaning up
the core part from the quirks and custom pieces would make this task
achievable.
I'm also puzzled why you don't use FPGA manager which should handle, as far as
I understand, very flexible configurations of FPGAs.
Btw, what exact IP of UART do you have implemented there?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function
From: Lee Jones @ 2019-05-07 11:49 UTC (permalink / raw)
To: Esben Haabendal
Cc: linux-serial, Greg Kroah-Hartman, Jiri Slaby, Nishanth Menon,
Vignesh R, Tony Lindgren, Lokesh Vutla, Florian Fainelli,
linux-kernel
In-Reply-To: <20190426084038.6377-3-esben@geanix.com>
On Fri, 26 Apr 2019, Esben Haabendal wrote:
> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
> to an MFD driver.
>
> When calling mfd_add_device(), platform_data should be a pointer to a
> struct plat_serial8250_port, with proper settings like .flags, .type,
> .iotype, .regshift and .uartclk. Memory (or ioport) and IRQ should be
> passed as cell resources.
What? No, please!
If you *must* create a whole driver just to be able to use
platform_*() helpers (which I don't think you should), then please
call it something else. This doesn't have anything to do with MFD.
> Do not include UPF_BOOT_AUTOCONF in platform_data.flags.
>
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
> drivers/tty/serial/8250/8250_mfd.c | 119 +++++++++++++++++++++++++++++++++++++
> drivers/tty/serial/8250/Kconfig | 12 ++++
> drivers/tty/serial/8250/Makefile | 1 +
> 3 files changed, 132 insertions(+)
> create mode 100644 drivers/tty/serial/8250/8250_mfd.c
>
> diff --git a/drivers/tty/serial/8250/8250_mfd.c b/drivers/tty/serial/8250/8250_mfd.c
> new file mode 100644
> index 0000000..eae1566
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_mfd.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Serial Port driver for 8250/16550-type MFD sub devices
> + *
> + * This mimics the serial8250_probe of 8250_core.c, while allowing
> + * use without UPF_BOOT_AUTOCONF, which is problematic for MFD, as
> + * the request_mem_region() will typically fail as the region is
> + * already requested by the MFD device.
> + *
> + * Memory and irq are passed as platform resources, which allows easy
> + * use together with (devm_)mfd_add_devices().
> + *
> + * Other parameters are passed as struct plat_serial8250_port in
> + * device platform_data.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/serial_8250.h>
> +
> +struct serial8250_mfd_data {
> + int line;
> +};
> +
> +static int serial8250_mfd_probe(struct platform_device *pdev)
> +{
> + struct plat_serial8250_port *pdata = dev_get_platdata(&pdev->dev);
> + struct serial8250_mfd_data *data;
> + struct uart_8250_port up;
> + struct resource *r;
> + void __iomem *membase;
> +
> + if (!pdata)
> + return -ENODEV;
> +
> + memset(&up, 0, sizeof(up));
> +
> + switch (pdata->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, 0);
> + if (!r)
> + return -ENODEV;
> + membase = devm_ioremap_nocache(&pdev->dev,
> + r->start, resource_size(r));
> + if (!membase)
> + return -ENOMEM;
> + up.port.mapbase = r->start;
> + up.port.membase = membase;
> + break;
> + case UPIO_HUB6:
> + case UPIO_PORT:
> + r = platform_get_resource(pdev, IORESOURCE_IO, 0);
> + if (!r)
> + return -ENODEV;
> + up.port.iobase = r->start;
> + break;
> + }
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + up.port.irq = platform_get_irq(pdev, 0);
> + if (up.port.irq < 0)
> + up.port.irq = 0; /* no interrupt -> use polling */
> +
> + /* Register with 8250_core.c */
> + up.port.irqflags = pdata->irqflags;
> + up.port.uartclk = pdata->uartclk;
> + up.port.regshift = pdata->regshift;
> + up.port.iotype = pdata->iotype;
> + up.port.flags = pdata->flags;
> + up.port.hub6 = pdata->hub6;
> + up.port.private_data = pdata->private_data;
> + up.port.type = pdata->type;
> + up.port.serial_in = pdata->serial_in;
> + up.port.serial_out = pdata->serial_out;
> + up.port.handle_irq = pdata->handle_irq;
> + up.port.handle_break = pdata->handle_break;
> + up.port.set_termios = pdata->set_termios;
> + up.port.set_ldisc = pdata->set_ldisc;
> + up.port.get_mctrl = pdata->get_mctrl;
> + up.port.pm = pdata->pm;
> + up.port.dev = &pdev->dev;
> + data->line = __serial8250_register_8250_port(&up, 0);
> + if (data->line < 0)
> + return data->line;
> +
> + platform_set_drvdata(pdev, data);
> + return 0;
> +}
> +
> +static int serial8250_mfd_remove(struct platform_device *pdev)
> +{
> + struct serial8250_mfd_data *data = platform_get_drvdata(pdev);
> +
> + serial8250_unregister_port(data->line);
> + return 0;
> +}
> +
> +static struct platform_driver serial8250_mfd_driver = {
> + .probe = serial8250_mfd_probe,
> + .remove = serial8250_mfd_remove,
> + .driver = {
> + .name = "serial8250-mfd",
> + },
> +};
> +
> +module_platform_driver(serial8250_mfd_driver);
> +
> +MODULE_AUTHOR("Esben Haabendal <esben@geanix.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Driver for 8250/16550-type MFD sub devices");
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 15c2c54..ef1572b 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -58,6 +58,18 @@ config SERIAL_8250_PNP
> This builds standard PNP serial support. You may be able to
> disable this feature if you only need legacy serial support.
>
> +config SERIAL_8250_MFD
> + bool "8250/16550 MFD function support"
> + depends on SERIAL_8250 && MFD_CORE
> + default n
> + help
> + This builds support for using 8250/16550-type UARTs as MFD
> + functions.
> +
> + MFD drivers needing this should select it automatically.
> +
> + If unsure, say N.
> +
> config SERIAL_8250_FINTEK
> bool "Support for Fintek F81216A LPC to 4 UART RS485 API"
> depends on SERIAL_8250
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index 18751bc..da8e139 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -6,6 +6,7 @@
> obj-$(CONFIG_SERIAL_8250) += 8250.o 8250_base.o
> 8250-y := 8250_core.o
> 8250-$(CONFIG_SERIAL_8250_PNP) += 8250_pnp.o
> +8250-$(CONFIG_SERIAL_8250_MFD) += 8250_mfd.o
> 8250_base-y := 8250_port.o
> 8250_base-$(CONFIG_SERIAL_8250_DMA) += 8250_dma.o
> 8250_base-$(CONFIG_SERIAL_8250_FINTEK) += 8250_fintek.o
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ 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