[PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

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.

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
-- 
2.4.11

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Enrico Weigelt, metux IT consult]

On 21.05.19 16:43, Greg Kroah-Hartman wrote:

Hi,

> Sometimes you need to go tell the hardware/firmware people not to do
> foolish things.  You can not always fix their problems in software.
> Please push back on this.

I've often been in the same situation. It's hopeless with those folks.
Even worse: you give'em a clear spec on how the register interface
shall look like, and they make something really weird out of it
(shuffled byte orders, multiplexing irqs over a single gpio line and
leave the other's empty instead of just one gpio per irq, ...)

The kernel is full of buggy hardware, because hw folks seem not really
capable of doing their homework :(

Actually, the whole existance of these hundreds of different uart
devices, IMHO, is a clear sing of hw folks not doing the homework.


By the way: I've somewhat lost track of what the patch was actually
about ... :o


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Greg Kroah-Hartman]

On Tue, May 21, 2019 at 04:31:52PM +0200, Esben Haabendal wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Tue, May 21, 2019 at 01:50:25PM +0200, Esben Haabendal wrote:
> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >> 
> >> > On Tue, May 21, 2019 at 01:11:08PM +0200, Esben Haabendal wrote:
> >> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >> >> 
> >> >> >> I will try ad hold back with this thread until you get back to it.
> >> >> >
> >> >> > Ok, I have no idea what is going on here, sorry.  This is a really long
> >> >> > and meandering thread, and I can't even find the original patches in my
> >> >> > queue.
> >> >> >
> >> >> > So can you resend things and we can start over?  :)
> >> >> 
> >> >> Will do.
> >> >> 
> >> >> > But note, using a mfd for a uart seems VERY odd to me...
> >> >> 
> >> >> Ok.  In my case, I have a pcie card with an fpga which includes 5 uart
> >> >> ports, 3 ethernet interfaces and a number of custom IP blocks.
> >> >> I believe that an mfd driver for that pcie card in that case.
> >> >
> >> > I believe you need to fix that fpga to expose individual pci devices
> >> > such that you can properly bind the individual devices to the expected
> >> > drivers :)
> >> 
> >> Well, that is really out-of-scope of what I am doing here.
> >
> > Not really, if you have control over the fpga firmware (and odds are you
> > do), just fix that and instantly your device works with all kernels, no
> > need to change anything.
> >
> > Why not do this?
> 
> Because I do not have control over fpga firmware.

Who does?  Why did they create it this way if it can not be accessed by
an operating system as-is?  Has it passed the PCI tests?  Do you have a
link to where you can get this crazy device?

> >> > Seriously, who makes such a broken fpga device that goes against the PCI
> >> > spec that way?  Well, not so much as "goes against it", as "ignores all
> >> > of the proper ideas of the past 20 years for working with PCI devices".
> >> 
> >> Might be.  But that is the firmware I have to work with here, and I
> >> still hope we can find a good solution for implementing a driver without
> >> having to maintain out-of-tree patches.
> >
> > As this hardware will not work on any operating system as-is, why not
> > fix the firmware to keep from having to support a one-off device that no
> > one else would be crazy enough to create?  :)
> 
> Clearly, someone has been crazy enough.  Hopefully, we can be smart
> enough to make Linux fit to it.

Sometimes you need to go tell the hardware/firmware people not to do
foolish things.  You can not always fix their problems in software.
Please push back on this.

thanks,

greg k-h

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Tue, May 21, 2019 at 01:50:25PM +0200, Esben Haabendal wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> > On Tue, May 21, 2019 at 01:11:08PM +0200, Esben Haabendal wrote:
>> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> >> 
>> >> >> I will try ad hold back with this thread until you get back to it.
>> >> >
>> >> > Ok, I have no idea what is going on here, sorry.  This is a really long
>> >> > and meandering thread, and I can't even find the original patches in my
>> >> > queue.
>> >> >
>> >> > So can you resend things and we can start over?  :)
>> >> 
>> >> Will do.
>> >> 
>> >> > But note, using a mfd for a uart seems VERY odd to me...
>> >> 
>> >> Ok.  In my case, I have a pcie card with an fpga which includes 5 uart
>> >> ports, 3 ethernet interfaces and a number of custom IP blocks.
>> >> I believe that an mfd driver for that pcie card in that case.
>> >
>> > I believe you need to fix that fpga to expose individual pci devices
>> > such that you can properly bind the individual devices to the expected
>> > drivers :)
>> 
>> Well, that is really out-of-scope of what I am doing here.
>
> Not really, if you have control over the fpga firmware (and odds are you
> do), just fix that and instantly your device works with all kernels, no
> need to change anything.
>
> Why not do this?

Because I do not have control over fpga firmware.

>> > Seriously, who makes such a broken fpga device that goes against the PCI
>> > spec that way?  Well, not so much as "goes against it", as "ignores all
>> > of the proper ideas of the past 20 years for working with PCI devices".
>> 
>> Might be.  But that is the firmware I have to work with here, and I
>> still hope we can find a good solution for implementing a driver without
>> having to maintain out-of-tree patches.
>
> As this hardware will not work on any operating system as-is, why not
> fix the firmware to keep from having to support a one-off device that no
> one else would be crazy enough to create?  :)

Clearly, someone has been crazy enough.  Hopefully, we can be smart
enough to make Linux fit to it.

/Esben

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Greg Kroah-Hartman]

On Tue, May 21, 2019 at 01:50:25PM +0200, Esben Haabendal wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Tue, May 21, 2019 at 01:11:08PM +0200, Esben Haabendal wrote:
> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >> 
> >> >> I will try ad hold back with this thread until you get back to it.
> >> >
> >> > Ok, I have no idea what is going on here, sorry.  This is a really long
> >> > and meandering thread, and I can't even find the original patches in my
> >> > queue.
> >> >
> >> > So can you resend things and we can start over?  :)
> >> 
> >> Will do.
> >> 
> >> > But note, using a mfd for a uart seems VERY odd to me...
> >> 
> >> Ok.  In my case, I have a pcie card with an fpga which includes 5 uart
> >> ports, 3 ethernet interfaces and a number of custom IP blocks.
> >> I believe that an mfd driver for that pcie card in that case.
> >
> > I believe you need to fix that fpga to expose individual pci devices
> > such that you can properly bind the individual devices to the expected
> > drivers :)
> 
> Well, that is really out-of-scope of what I am doing here.

Not really, if you have control over the fpga firmware (and odds are you
do), just fix that and instantly your device works with all kernels, no
need to change anything.

Why not do this?

> > Seriously, who makes such a broken fpga device that goes against the PCI
> > spec that way?  Well, not so much as "goes against it", as "ignores all
> > of the proper ideas of the past 20 years for working with PCI devices".
> 
> Might be.  But that is the firmware I have to work with here, and I
> still hope we can find a good solution for implementing a driver without
> having to maintain out-of-tree patches.

As this hardware will not work on any operating system as-is, why not
fix the firmware to keep from having to support a one-off device that no
one else would be crazy enough to create?  :)

thanks,

greg k-h

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Tue, May 21, 2019 at 01:11:08PM +0200, Esben Haabendal wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> >> I will try ad hold back with this thread until you get back to it.
>> >
>> > Ok, I have no idea what is going on here, sorry.  This is a really long
>> > and meandering thread, and I can't even find the original patches in my
>> > queue.
>> >
>> > So can you resend things and we can start over?  :)
>> 
>> Will do.
>> 
>> > But note, using a mfd for a uart seems VERY odd to me...
>> 
>> Ok.  In my case, I have a pcie card with an fpga which includes 5 uart
>> ports, 3 ethernet interfaces and a number of custom IP blocks.
>> I believe that an mfd driver for that pcie card in that case.
>
> I believe you need to fix that fpga to expose individual pci devices
> such that you can properly bind the individual devices to the expected
> drivers :)

Well, that is really out-of-scope of what I am doing here.

> Seriously, who makes such a broken fpga device that goes against the PCI
> spec that way?  Well, not so much as "goes against it", as "ignores all
> of the proper ideas of the past 20 years for working with PCI devices".

Might be.  But that is the firmware I have to work with here, and I
still hope we can find a good solution for implementing a driver without
having to maintain out-of-tree patches.

/Esben

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Greg Kroah-Hartman]

On Tue, May 21, 2019 at 01:11:08PM +0200, Esben Haabendal wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> >> I will try ad hold back with this thread until you get back to it.
> >
> > Ok, I have no idea what is going on here, sorry.  This is a really long
> > and meandering thread, and I can't even find the original patches in my
> > queue.
> >
> > So can you resend things and we can start over?  :)
> 
> Will do.
> 
> > But note, using a mfd for a uart seems VERY odd to me...
> 
> Ok.  In my case, I have a pcie card with an fpga which includes 5 uart
> ports, 3 ethernet interfaces and a number of custom IP blocks.
> I believe that an mfd driver for that pcie card in that case.

I believe you need to fix that fpga to expose individual pci devices
such that you can properly bind the individual devices to the expected
drivers :)

Seriously, who makes such a broken fpga device that goes against the PCI
spec that way?  Well, not so much as "goes against it", as "ignores all
of the proper ideas of the past 20 years for working with PCI devices".

thanks,

greg k-h

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

>> I will try ad hold back with this thread until you get back to it.
>
> Ok, I have no idea what is going on here, sorry.  This is a really long
> and meandering thread, and I can't even find the original patches in my
> queue.
>
> So can you resend things and we can start over?  :)

Will do.

> But note, using a mfd for a uart seems VERY odd to me...

Ok.  In my case, I have a pcie card with an fpga which includes 5 uart
ports, 3 ethernet interfaces and a number of custom IP blocks.
I believe that an mfd driver for that pcie card in that case.

/Esben

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Greg Kroah-Hartman]

On Tue, May 14, 2019 at 02:41:35PM +0200, Esben Haabendal wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Tue, May 14, 2019 at 11:47:41AM +0100, Lee Jones wrote:
> >> On Tue, 14 May 2019, Esben Haabendal wrote:
> >> 
> >> > Lee Jones <lee.jones@linaro.org> writes:
> >> > 
> >> > > 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"?
> >> > 
> >> > Of-course.
> >> > 
> >> > >> 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.
> >> > 
> >> > Unfortunately, I don't seem to be able to convince Andy to accept
> >> > something like that.
> >> 
> >> Andy is not he Maintainer.
> >> 
> >> What are Greg and Jiri's opinions?
> >
> > I've been ignoring all of this at the moment because of the 5.2-rc merge
> > window.  I'll look at it after -rc1 is out.
> >
> > thanks,
> > greg k-h
> 
> Great, thanks!
> 
> I will try ad hold back with this thread until you get back to it.

Ok, I have no idea what is going on here, sorry.  This is a really long
and meandering thread, and I can't even find the original patches in my
queue.

So can you resend things and we can start over?  :)

But note, using a mfd for a uart seems VERY odd to me...

thanks,

greg k-h

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Tue, May 14, 2019 at 11:47:41AM +0100, Lee Jones wrote:
>> On Tue, 14 May 2019, Esben Haabendal wrote:
>> 
>> > Lee Jones <lee.jones@linaro.org> writes:
>> > 
>> > > 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"?
>> > 
>> > Of-course.
>> > 
>> > >> 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.
>> > 
>> > Unfortunately, I don't seem to be able to convince Andy to accept
>> > something like that.
>> 
>> Andy is not he Maintainer.
>> 
>> What are Greg and Jiri's opinions?
>
> I've been ignoring all of this at the moment because of the 5.2-rc merge
> window.  I'll look at it after -rc1 is out.
>
> thanks,
> greg k-h

Great, thanks!

I will try ad hold back with this thread until you get back to it.

/Esben

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Greg Kroah-Hartman]

On Tue, May 14, 2019 at 11:47:41AM +0100, Lee Jones wrote:
> On Tue, 14 May 2019, Esben Haabendal wrote:
> 
> > Lee Jones <lee.jones@linaro.org> writes:
> > 
> > > 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"?
> > 
> > Of-course.
> > 
> > >> 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.
> > 
> > Unfortunately, I don't seem to be able to convince Andy to accept
> > something like that.
> 
> Andy is not he Maintainer.
> 
> What are Greg and Jiri's opinions?

I've been ignoring all of this at the moment because of the 5.2-rc merge
window.  I'll look at it after -rc1 is out.

thanks,

greg k-h

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Lee Jones]

On Tue, 14 May 2019, Esben Haabendal wrote:

> Lee Jones <lee.jones@linaro.org> writes:
> 
> > 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"?
> 
> Of-course.
> 
> >> 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.
> 
> Unfortunately, I don't seem to be able to convince Andy to accept
> something like that.

Andy is not he Maintainer.

What are Greg and Jiri's opinions?

> I might have to do this out-of-tree :(

Well that would suck!

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

Lee Jones <lee.jones@linaro.org> writes:

> 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"?

Of-course.

>> 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.

Unfortunately, I don't seem to be able to convince Andy to accept
something like that.

I might have to do this out-of-tree :(

/Esben

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Lee Jones]

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

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

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

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Lee Jones]

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

[PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

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.

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
-- 
2.4.11