* 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
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-05-07 11:35 UTC (permalink / raw)
To: Lee Jones, Andy Shevchenko
Cc: linux-serial, Enrico Weigelt, Greg Kroah-Hartman, Jiri Slaby,
Darwin Dingel, Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe,
Marek Vasut, Douglas Anderson, Paul Burton, linux-kernel
In-Reply-To: <20190507093239.GB4529@dell>
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?
/Esben
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-05-07 11:32 UTC (permalink / raw)
To: Lee Jones
Cc: Andy Shevchenko, 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: <20190507093631.GC4529@dell>
Lee Jones <lee.jones@linaro.org> writes:
> On Tue, 07 May 2019, Lee Jones wrote:
>> 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.
>
> Not sure which device you're trying to enable, but if it's booted
> using Device Tree, you could always use 'of_serial'.
It is an x86_64 platform, so there is unfortunately no device tree.
> It does seem a little odd that the 'serial8250' IP block has been
> incorporated into an MFD. Which device is it you're trying to enable
> exactly?
It is a Xilinx FPGA, containing a number of different devices, including
5 16550A UART devices (XPS 16550 UART v3.00a). It also contains 3
Ethernet interfaces and a number of custom IP blocks.
The FPGA is connected to the CPU using PCIe, with all devices using
parts of a big common io memory block, specified by a PCI BAR.
/Esben
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Lee Jones @ 2019-05-07 9:36 UTC (permalink / raw)
To: Esben Haabendal
Cc: Andy Shevchenko, 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: <20190507093239.GB4529@dell>
On Tue, 07 May 2019, Lee Jones wrote:
> On Thu, 02 May 2019, Esben Haabendal wrote:
>
> > Hi Lee
> >
> > 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.
Not sure which device you're trying to enable, but if it's booted
using Device Tree, you could always use 'of_serial'.
It does seem a little odd that the 'serial8250' IP block has been
incorporated into an MFD. Which device is it you're trying to enable
exactly?
> > In my understanding, use of platform_get_resource() is the right thing
> > to do in order to integrate properly with with MFD drivers that splits a
> > common memory resource in mfd_add_device() using the mem_base argument.
>
--
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: Lee Jones @ 2019-05-07 9:32 UTC (permalink / raw)
To: Esben Haabendal
Cc: Andy Shevchenko, 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: <87pnp11112.fsf@haabendal.dk>
On Thu, 02 May 2019, Esben Haabendal wrote:
> Hi Lee
>
> 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.
> In my understanding, use of platform_get_resource() is the right thing
> to do in order to integrate properly with with MFD drivers that splits a
> common memory resource in mfd_add_device() using the mem_base argument.
--
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-06 21:04 UTC (permalink / raw)
To: Esben Haabendal
Cc: Andy Shevchenko, Lee Jones, open list:SERIAL DRIVERS,
Enrico Weigelt, Greg Kroah-Hartman, Jiri Slaby, Darwin Dingel,
Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe, Marek Vasut,
Douglas Anderson, Paul Burton, Linux Kernel Mailing List
In-Reply-To: <87o94f32iw.fsf@haabendal.dk>
Let's start from simple things:
- I really failed to find where resources are requested in
mfd_add_device():
https://elixir.bootlin.com/linux/latest/ident/mfd_add_device
- as for 8250_dw.c (as MFD child of intel-lpss-pci.c) see below...
1. 8250_dw.c remaps resources, but doesn't request them.
2. It calls
-> serial8250_register_8250_port(), which sets UPF_BOOT_AUTOCONF
unconditionally, as you noticed
-> uart_add_one_port()
-> uart_configure_port()
-> port->ops->config_port(), if UPF_BOOT_AUTOCONF is set, see above
3. And ->config_port() is defined to serial8250_config_port() in
https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/8250/8250_port.c#L3147
So, it *does* request resources implicitly via 8250 core.
I maybe miss something obvious, though.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-05-06 17:40 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: <20190506164426.GO9224@smile.fi.intel.com>
> On Mon, May 06, 2019 at 05:46:56PM +0200, Esben Haabendal wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> >> > On Wed, May 01, 2019 at 09:17:37AM +0200, Esben Haabendal wrote:
>> >> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>
>> As an example, the sm501.c driver, the only driver in drivers/mfd/ which
>> uses serial8250 driver, does not use any code from mfd-core.
>> Incidentally, it is 1 year older than mfd-core.c, and as never been
>> refactored to use mfd-core functionality.
>
> So, sm501.c should not request resources for its children. This as simple as
> that.
Funny thing. Even though sm501.c does not use mfd-core at all, it does
request resources for all its child devices, except for the uart
children.
sm501_register_usbhost(), sm501_register_display() and
sm501_register_gpio() all creates/requests resources. But
sm501_register_uart() does not.
How many concrete examples are needed to convince you that what I am
trying to do is how it is done everywhere else (than
serial8250_core.c/serial8250_port.c, even in other 8250_*.c drivers)
(obviously not 100% true, there are ofcourse other pieces of code not
working well with resource management) ?
> What you are trying to do here is a hack workaround on the current
> behaviour in the Linux device model (resource management) as I told
> you already.
No. If it was, then all (most) mfd drivers added after 2008 are hacky
workarounds, because the use mfd_add_devices().
There are currently 53 drivers in drivers/mfd/ that calls
mfd_add_devices() with one or more cells with resources attached.
Are they all hacky workarounds?
I am not trying to do anything that they are not already doing.
>> > Why not? Again, *slicing* resources is OK and that's what MFD for,
>> > *requesting*
>> > them in the parent is not.
>>
>> Why we cannot use request_mem_region() for those memory resources again?
>
> Because it's how it was designed. "One device per one resource". If you would
> like to fix this, it should be done obviously not in 8250 driver or any other
> driver, but driver core.
>
> Nevertheless there is one particular exception here,
> i.e. IORESOURCE_MUXED.
I am not trying to fix the problem of having multiple drivers owning the
same resource. I am just trying to make serial8250 driver behave so it
can use the resources that it is handed by mfd-core.
This really is how it (mfd and also device resource management) is
designed. I am not inventing anything, or making a workaround.
Actually, you should take a look at the following specialized 8250
8250_aspeed_vuart.c
8250_bcm2835aux.c
8250_dw.c
8250_em.c
8250_ingenic.c
8250_lpc18xx.c
8250_mtk.c
8250_omap.c
8250_pxa.c
8250_uniphier.c
They all use platform_get_resource(), and will work nicely with mfd.
And of-course, none of them use request_mem_region().
So, if you want to insist that I create a clone of the current standard
serial8250 driver (serial8250_isa_driver in 8520_core.c), even though I
want absolutely nothing specialized, just need it to play nicely with
platform_get_resource(), what should I call the driver? 8520_plat.c ?
>> It fails because the resources are now already owned the mfd driver, on
>> behalf of the child.
>
> Yes. Behaves in order how it's implementer. No issues here.
If that was the case, then mfd-core would be implemented in order to not
work with existing platform drivers. There definitely is an issue
here. And it is in 8250_core.c and 8250_port.c.
>> > Nope, *requesting* resources as you mentioned lock them to the certain user.
>>
>> I still think there is some confusion in relation to your use of the
>> word "requesting". There is no explicit request/lock action in
>> kernel/resource.c.
>
> You have to check IORESOURCE_BUSY. It seems that what you missed in your
> picture.
Point taken. I haven't put much focus on that. But I don't see how
that is going to help making use of serial8250_isa_driver in combination
with mfd_add_devices(). I am not creating/requesting the resources.
That is done by mfd_add_device(), which I fail to see why I would need
to change.
> I didn't comment the rest until we will figure out the IO resource management
> in general.
I believe all my comments were related to this same resource management
discussion.
/Esben
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Andy Shevchenko @ 2019-05-06 16:44 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: <87ef5boaa7.fsf@haabendal.dk>
On Mon, May 06, 2019 at 05:46:56PM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> >> > On Wed, May 01, 2019 at 09:17:37AM +0200, Esben Haabendal wrote:
> >> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> As an example, the sm501.c driver, the only driver in drivers/mfd/ which
> uses serial8250 driver, does not use any code from mfd-core.
> Incidentally, it is 1 year older than mfd-core.c, and as never been
> refactored to use mfd-core functionality.
So, sm501.c should not request resources for its children. This as simple as
that.
What you are trying to do here is a hack workaround on the current behaviour in
the Linux device model (resource management) as I told you already.
> > Why not? Again, *slicing* resources is OK and that's what MFD for, *requesting*
> > them in the parent is not.
>
> Why we cannot use request_mem_region() for those memory resources again?
Because it's how it was designed. "One device per one resource". If you would
like to fix this, it should be done obviously not in 8250 driver or any other
driver, but driver core.
Nevertheless there is one particular exception here, i.e. IORESOURCE_MUXED.
> It fails because the resources are now already owned the mfd driver, on
> behalf of the child.
Yes. Behaves in order how it's implementer. No issues here.
> > Nope, *requesting* resources as you mentioned lock them to the certain user.
>
> I still think there is some confusion in relation to your use of the
> word "requesting". There is no explicit request/lock action in
> kernel/resource.c.
You have to check IORESOURCE_BUSY. It seems that what you missed in your
picture.
I didn't comment the rest until we will figure out the IO resource management
in general.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-05-06 15:46 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: <20190502153124.GA9224@smile.fi.intel.com>
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > On Wed, May 01, 2019 at 09:17:37AM +0200, Esben Haabendal wrote:
>> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> >
>> >> > Hmm... Currently it's done inside individual port drivers, like 8250_dw.c.
>> >> > Each of the drivers can do it differently, for example 8250_lpss.c or
>> >> > 8250_pnp.c.
>> >>
>> >> So, you would prefer to create a new "specialized" port driver that uses
>> >> platform resources? I am not doing anything else different from
>> >> the generic port driver here in 8250_core.c.
>> >
>> > If it's required and using serial8250 directly is not enough.
>>
>> Sorry, I am not sure what you mean by that.
>
> The serial8250 is the name of (generic) platform driver of 8250 which can be
> used as a child for MFD.
The last part is only true, if you don't let MFD manage the memory
resources, which is the main thing that MFD is doing.
As an example, the sm501.c driver, the only driver in drivers/mfd/ which
uses serial8250 driver, does not use any code from mfd-core.
Incidentally, it is 1 year older than mfd-core.c, and as never been
refactored to use mfd-core functionality.
>> >> > I think I understand what is a confusion here.
>> >> >
>> >> > For the IO resources we have two operations:
>> >> > - mapping / re-mapping (may be shared)
>> >> > - requesting (exclusive)
>> >> >
>> >> > In the parenthesis I put a level of access to it. While many device
>> >> > drivers can *share* same resource (mapped or unmapped), the only one
>> >> > can actually request it.
>> >>
>> >> Mostly true. But there is an important twist to the exclusive restriction.
>> >>
>> >> The exclusive part of the request is limited to the the same root/parent
>> >> resource.
>> >>
>> >> When you request a memory resource from the root resource
>> >> (iomem_resource), the resource returned can be used as a new parent
>> >> resource. This new parent can then be used to give exclusive access to
>> >> slices of that resource. When used like that, I expect that the parent
>> >> resource is not supposed to be used for anything else than honoring
>> >> resource requests.
>> >>
>> >> And this is exactly what mfd-core uses the mem_base argument
>> >> in mfd_add_devices().
>> >>
>> >> > So, the parent can take an slice resources as it would be
>> >> > appropriated, but not requesting them.
>> >>
>> >> The parent is not and should not be doing that by itself. The request
>> >> is done on by mfd-core when mfd_add_devices() is called.
>> >
>> > No, MFD *does not* (and actually *may not* in order to allow standalone drivers
>> > to be used as children w/o modifications) request resources. It just passes
>> > them to children as parent suggested.
>>
>> In drivers/mfd/mfd-core.c:mfd_add_device() :
>>
>> for (r = 0; r < cell->num_resources; r++) {
>> res[r].name = cell->resources[r].name;
>> res[r].flags = cell->resources[r].flags;
>>
>> /* Find out base to use */
>> if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
>> res[r].parent = mem_base;
>> res[r].start = mem_base->start +
>> cell->resources[r].start;
>> res[r].end = mem_base->start +
>> cell->resources[r].end;
>> } else if (cell->resources[r].flags & IORESOURCE_IRQ) {
>> if (domain) {
>> /* Unable to create mappings for IRQ ranges. */
>> WARN_ON(cell->resources[r].start !=
>> cell->resources[r].end);
>> res[r].start = res[r].end = irq_create_mapping(
>> domain, cell->resources[r].start);
>> } else {
>> res[r].start = irq_base +
>> cell->resources[r].start;
>> res[r].end = irq_base +
>> cell->resources[r].end;
>> }
>> } else {
>> res[r].parent = cell->resources[r].parent;
>> res[r].start = cell->resources[r].start;
>> res[r].end = cell->resources[r].end;
>> }
>>
>> if (!cell->ignore_resource_conflicts) {
>> if (has_acpi_companion(&pdev->dev)) {
>> ret = acpi_check_resource_conflict(&res[r]);
>> if (ret)
>> goto fail_alias;
>> }
>> }
>> }
>>
>> ret = platform_device_add_resources(pdev, res, cell->num_resources);
>>
>> This creates the child resources. Whether we call that requesting the
>> resources or not, is a matter of word. But it is what it is. When it
>> is done, you cannot use request_mem_region() for those memory resources,
>> they are now locked/exclusive for the mfd parent *and* for the
>> respective mfd child device.
>
> Why not? Again, *slicing* resources is OK and that's what MFD for, *requesting*
> them in the parent is not.
Why we cannot use request_mem_region() for those memory resources again?
It fails because the resources are now already owned the mfd driver, on
behalf of the child.
>> In order to use them, child devices simply use platform_get_resource(),
>> and everything works nicely. It works fine for normal (non-mfd)
>> devices, as they get (requests) the resources from the root resource
>> (iomem_resource), and works fine for mfd devices as well. So no changes
>> are needed for drivers to work with mfd.
>>
>> Whether you call the thing that mfd_add_device() does for "request
>> resources" or just "pass them to children" is a matter of words.
>
> Nope, *requesting* resources as you mentioned lock them to the certain user.
I still think there is some confusion in relation to your use of the
word "requesting". There is no explicit request/lock action in
kernel/resource.c.
There are basically only two steps involved in resource management in
kernel/resource.c:
1. You create a struct resource.
2. You add it to the child list of another resource.
Requesting a reasource is just another word for 2. And
request_mem_region() is just a that, hardcoded to the iomem_resource
struct.
And mfd_add_device() implements step 2 for the resources included in the
cell.resources field. The purpose is a generic resource slicing
implementation, and is also what you call requesting.
>> The mfd (parent) has a resource which it cuts up into slices for its
>> children, and these slices are passed to the child devices. The
>> drivers for these child devices must then pickup the resource(s)
>> using platform_get_resource(). At no point is any "request_*"
>> function called.
>>
>> Looking at in another way.
>>
>> The request_mem_region() macro call __request_resource(), which which
>> simply creates a new 'struct resource' in the iomem_resource resource.
>>
>> In mfd_add_device(), almost the same happens. A new 'struct resource'
>> is created in the mem_base resource.
>>
>> In both cases, a 'struct resource' is created, representing exclusive
>> access to the resource. And like it or not, this is something that MFD
>> already *do*, and I think it is way out of scope of this patch to change
>> that.
>>
>> I just try to make serial8250 driver work nicely in that (mfd) context,
>> without changing how mfd is working.
>
> There is nothing to change. It's already working. I don't see a problem here.
No. It is not working.
Why are you saying that? There is no existing usage of the serial8250
driver in combination of mfd_add_devices(). And it cannot be done with
the current serial8250 driver, as it will fail in the
request_mem_region() call.
I really think it is a shame if we cannot use mfd-core for mfd drivers
that just happens to need serial8250 driver.
/Esben
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-05-06 15:19 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: linux-serial, Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko,
Darwin Dingel, Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe,
Marek Vasut, Douglas Anderson, Paul Burton, linux-kernel
In-Reply-To: <a535c7b6-54e0-ab58-7626-f7f631773c18@metux.net>
"Enrico Weigelt, metux IT consult" <lkml@metux.net> writes:
> On 30.04.19 16:04, Esben Haabendal wrote:
>> Allow getting memory resource (mapbase or iobase) as well as irq from
>> platform_device resources.
>>
>> The UPF_DEV_RESOURCES flag must be set for devices where platform_device
>> resources are to be used. When not set, driver behaves as before.
>>
>> This allows use of the serial8250 driver together with devices with
>> resources added by platform_device_add_resources(), such as mfd child
>> devices added with mfd_add_devices().
>
> I like the idea (actually, quite the direction I'd like to go), but
> unfortunately it's more compilicated than that.
>
> Some drivers don't use these fields, eg. 8250 determines the mapsize
> based on several factors, at the time of the mapping is done. That's
> one of the things my patches shall clean up.
Could you take a quick look at my patch again. The patch only changes
the probe method in the serial8250_isa_driver in 8250_core.c file.
So other drivers are not affected by this change.
And with the addition of the new UPF_DEV_RESOURCES flag, no existing
platforms should be affected either.
The patch merely makes it possible to start using plain "serial8250"
driver (serial8250_isa_driver) with standard platform resources, fx. as
implemented by mfd-core.
/Esben
^ permalink raw reply
* Re: [PATCH v5 0/2] tty: serial: add DT bindings and serial driver for the SiFive FU540 UART
From: Paul Walmsley @ 2019-05-03 19:05 UTC (permalink / raw)
To: Kevin Hilman
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
Atish Patra, linux-serial@vger.kernel.org, Paul Walmsley,
linux-riscv@lists.infradead.org
In-Reply-To: <7hsgtwlm5t.fsf@baylibre.com>
On Thu, 2 May 2019, Kevin Hilman wrote:
> Paul Walmsley <paul.walmsley@sifive.com> writes:
>
> > I'd recommend testing the DT patches with BBL and the open-source FSBL.
> > That's the traditional way of booting RISC-V Linux systems.
>
> OK, but as you know, not the tradiaional way of booting most other linux
> systems. ;)
>
> I'm working on getting RISC-V supported in kernelCI in a fully-automated
> way, and I don't currently have the time to add add support for BBL+FSBL
> to kernelCI automation tooling, so having u-boot support is the best way
> to get support in kernelCI, IMO.
That's great. Please keep hacking away on RISC-V support for kernelCI.
My point is just that the U-boot and OpenSBI software stack you're working
with is not going to be useful for automatic tests of some kernel patches
yet. That stack is still very new, and was written around a non-upstream
set of DT data. We are in the process of posting and merging patches to
fix that, but it's going to take a few releases of both the kernel and
those other boot stack components until things are sorted out in a more
durable way.
- Paul
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Enrico Weigelt, metux IT consult @ 2019-05-02 19:41 UTC (permalink / raw)
To: Esben Haabendal, linux-serial
Cc: Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko, Darwin Dingel,
Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe, Marek Vasut,
Douglas Anderson, Paul Burton, linux-kernel
In-Reply-To: <20190430140416.4707-1-esben@geanix.com>
On 30.04.19 16:04, Esben Haabendal wrote:
> Allow getting memory resource (mapbase or iobase) as well as irq from
> platform_device resources.
>
> The UPF_DEV_RESOURCES flag must be set for devices where platform_device
> resources are to be used. When not set, driver behaves as before.
>
> This allows use of the serial8250 driver together with devices with
> resources added by platform_device_add_resources(), such as mfd child
> devices added with mfd_add_devices().
I like the idea (actually, quite the direction I'd like to go), but
unfortunately it's more compilicated than that.
Some drivers don't use these fields, eg. 8250 determines the mapsize
based on several factors, at the time of the mapping is done. That's
one of the things my patches shall clean up.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: [PATCH v5 2/2] tty: serial: add driver for the SiFive UART
From: Kevin Hilman @ 2019-05-02 18:58 UTC (permalink / raw)
To: linux-kernel, linux-serial, linux-riscv, gregkh
Cc: Paul Walmsley, Paul Walmsley, Jiri Slaby, Palmer Dabbelt,
Wesley Terpstra, Julia Lawall, Emil Renner Berthing,
Andreas Schwab
In-Reply-To: <20190413020111.23400-3-paul.walmsley@sifive.com>
Paul Walmsley <paul.walmsley@sifive.com> writes:
> Add a serial driver for the SiFive UART, found on SiFive FU540 devices
> (among others).
>
> The underlying serial IP block is relatively basic, and currently does
> not support serial break detection. Further information on the IP
> block can be found in the documentation and Chisel sources:
>
> https://static.dev.sifive.com/FU540-C000-v1.0.pdf
>
> https://github.com/sifive/sifive-blocks/tree/master/src/main/scala/devices/uart
>
> This driver was written in collaboration with Wesley Terpstra
> <wesley@sifive.com>.
>
> Tested on a SiFive HiFive Unleashed A00 board, using BBL and the open-
> source FSBL (using a DT file based on what's targeted for mainline).
>
> This revision incorporates changes based on comments by Julia Lawall
> <julia.lawall@lip6.fr>, Emil Renner Berthing <kernel@esmil.dk>, and
> Andreas Schwab <schwab@suse.de>. Thanks also to Andreas for testing
> the driver with his userspace and reporting a bug with the
> set_termios implementation.
>
> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Cc: Wesley Terpstra <wesley@sifive.com>
> Cc: linux-serial@vger.kernel.org
> Cc: linux-riscv@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Cc: Emil Renner Berthing <kernel@esmil.dk>
> Cc: Andreas Schwab <schwab@suse.de>
Tested-by: Kevin Hilman <khilman@baylibre.com>
^ permalink raw reply
* Re: [PATCH v5 0/2] tty: serial: add DT bindings and serial driver for the SiFive FU540 UART
From: Kevin Hilman @ 2019-05-02 18:57 UTC (permalink / raw)
Cc: Atish Patra, Paul Walmsley, linux-kernel@vger.kernel.org,
linux-serial@vger.kernel.org, linux-riscv@lists.infradead.org,
gregkh@linuxfoundation.org
In-Reply-To: <alpine.DEB.2.21.9999.1904191407310.5118@viisi.sifive.com>
Paul Walmsley <paul.walmsley@sifive.com> writes:
> On Fri, 19 Apr 2019, Kevin Hilman wrote:
>
>> Looks like Paul has so far only tested this with BBL + FSBL, so I think
>> I'll wait to hear from him how that setup might be different from using
>> OpenSBI + u-boot.
>
> I'd recommend testing the DT patches with BBL and the open-source FSBL.
> That's the traditional way of booting RISC-V Linux systems.
OK, but as you know, not the tradiaional way of booting most other linux
systems. ;)
I'm working on getting RISC-V supported in kernelCI in a fully-automated
way, and I don't currently have the time to add add support for BBL+FSBL
to kernelCI automation tooling, so having u-boot support is the best way
to get support in kernelCI, IMO.
Kevin
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Andy Shevchenko @ 2019-05-02 15:31 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: <87pnp11112.fsf@haabendal.dk>
On Thu, May 02, 2019 at 02:41:45PM +0200, Esben Haabendal wrote:
> Hi Lee
>
> 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().
>
> In my understanding, use of platform_get_resource() is the right thing
> to do in order to integrate properly with with MFD drivers that splits a
> common memory resource in mfd_add_device() using the mem_base argument.
>
> Discussion follows:
>
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>
> > On Wed, May 01, 2019 at 09:17:37AM +0200, Esben Haabendal wrote:
> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> >
> >> > Hmm... Currently it's done inside individual port drivers, like 8250_dw.c.
> >> > Each of the drivers can do it differently, for example 8250_lpss.c or
> >> > 8250_pnp.c.
> >>
> >> So, you would prefer to create a new "specialized" port driver that uses
> >> platform resources? I am not doing anything else different from
> >> the generic port driver here in 8250_core.c.
> >
> > If it's required and using serial8250 directly is not enough.
>
> Sorry, I am not sure what you mean by that.
The serial8250 is the name of (generic) platform driver of 8250 which can be
used as a child for MFD.
> >> >> + if (!(port->flags & UPF_DEV_RESOURCES))
> >> >> + release_mem_region(port->mapbase, size);
> >> >
> >> > This is again same issue. The parent should not request resource it
> >> > doesn't use.
> >>
> >> Yes, this is same issue.
> >>
> >> But the last part is not true. A parent mfd driver might "use" a memory
> >> resource for the sole purpose of splitting it up for it's mfd child
> >> devices. This is a core part of mfd framework, and not something I am
> >> inventing with this patch. I am just trying to make it possible to use
> >> 8250 driver in that context.
> >>
> >> > I think I understand what is a confusion here.
> >> >
> >> > For the IO resources we have two operations:
> >> > - mapping / re-mapping (may be shared)
> >> > - requesting (exclusive)
> >> >
> >> > In the parenthesis I put a level of access to it. While many device
> >> > drivers can *share* same resource (mapped or unmapped), the only one
> >> > can actually request it.
> >>
> >> Mostly true. But there is an important twist to the exclusive restriction.
> >>
> >> The exclusive part of the request is limited to the the same root/parent
> >> resource.
> >>
> >> When you request a memory resource from the root resource
> >> (iomem_resource), the resource returned can be used as a new parent
> >> resource. This new parent can then be used to give exclusive access to
> >> slices of that resource. When used like that, I expect that the parent
> >> resource is not supposed to be used for anything else than honoring
> >> resource requests.
> >>
> >> And this is exactly what mfd-core uses the mem_base argument
> >> in mfd_add_devices().
> >>
> >> > So, the parent can take an slice resources as it would be
> >> > appropriated, but not requesting them.
> >>
> >> The parent is not and should not be doing that by itself. The request
> >> is done on by mfd-core when mfd_add_devices() is called.
> >
> > No, MFD *does not* (and actually *may not* in order to allow standalone drivers
> > to be used as children w/o modifications) request resources. It just passes
> > them to children as parent suggested.
>
> In drivers/mfd/mfd-core.c:mfd_add_device() :
>
> for (r = 0; r < cell->num_resources; r++) {
> res[r].name = cell->resources[r].name;
> res[r].flags = cell->resources[r].flags;
>
> /* Find out base to use */
> if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
> res[r].parent = mem_base;
> res[r].start = mem_base->start +
> cell->resources[r].start;
> res[r].end = mem_base->start +
> cell->resources[r].end;
> } else if (cell->resources[r].flags & IORESOURCE_IRQ) {
> if (domain) {
> /* Unable to create mappings for IRQ ranges. */
> WARN_ON(cell->resources[r].start !=
> cell->resources[r].end);
> res[r].start = res[r].end = irq_create_mapping(
> domain, cell->resources[r].start);
> } else {
> res[r].start = irq_base +
> cell->resources[r].start;
> res[r].end = irq_base +
> cell->resources[r].end;
> }
> } else {
> res[r].parent = cell->resources[r].parent;
> res[r].start = cell->resources[r].start;
> res[r].end = cell->resources[r].end;
> }
>
> if (!cell->ignore_resource_conflicts) {
> if (has_acpi_companion(&pdev->dev)) {
> ret = acpi_check_resource_conflict(&res[r]);
> if (ret)
> goto fail_alias;
> }
> }
> }
>
> ret = platform_device_add_resources(pdev, res, cell->num_resources);
>
> This creates the child resources. Whether we call that requesting the
> resources or not, is a matter of word. But it is what it is. When it
> is done, you cannot use request_mem_region() for those memory resources,
> they are now locked/exclusive for the mfd parent *and* for the
> respective mfd child device.
Why not? Again, *slicing* resources is OK and that's what MFD for, *requesting*
them in the parent is not.
> In order to use them, child devices simply use platform_get_resource(),
> and everything works nicely. It works fine for normal (non-mfd)
> devices, as they get (requests) the resources from the root resource
> (iomem_resource), and works fine for mfd devices as well. So no changes
> are needed for drivers to work with mfd.
>
> Whether you call the thing that mfd_add_device() does for "request
> resources" or just "pass them to children" is a matter of words.
Nope, *requesting* resources as you mentioned lock them to the certain user.
> The
> mfd (parent) has a resource which it cuts up into slices for its
> children, and these slices are passed to the child devices. The drivers
> for these child devices must then pickup the resource(s) using
> platform_get_resource(). At no point is any "request_*" function
> called.
>
> Looking at in another way.
>
> The request_mem_region() macro call __request_resource(), which which
> simply creates a new 'struct resource' in the iomem_resource resource.
>
> In mfd_add_device(), almost the same happens. A new 'struct resource'
> is created in the mem_base resource.
>
> In both cases, a 'struct resource' is created, representing exclusive
> access to the resource. And like it or not, this is something that MFD
> already *do*, and I think it is way out of scope of this patch to change
> that.
>
> I just try to make serial8250 driver work nicely in that (mfd) context,
> without changing how mfd is working.
There is nothing to change. It's already working. I don't see a problem here.
> >> > OTOH, it's possible to have a (weird) MFD case where parent *requested*
> >> > resources, and *all* of its children are aware of that.
> >>
> >> I am not sure what you mean with this, but mfd drivers should not pass
> >> along it's intire requested memory resource(s) to child devices. The
> >> child devices will get the requested resource slices, as implemented by
> >> mfd_add_devices().
> >>
> >> I hope you can see that I am not violating any fundamental design
> >> decissions here, but actually try adhere to them (resource management,
> >> platform_device resource management, and mfd-core).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-05-02 12:41 UTC (permalink / raw)
To: Lee Jones, Andy Shevchenko
Cc: linux-serial, Enrico Weigelt, Greg Kroah-Hartman, Jiri Slaby,
Darwin Dingel, Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe,
Marek Vasut, Douglas Anderson, Paul Burton, linux-kernel
In-Reply-To: <20190502104556.GS9224@smile.fi.intel.com>
Hi Lee
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().
In my understanding, use of platform_get_resource() is the right thing
to do in order to integrate properly with with MFD drivers that splits a
common memory resource in mfd_add_device() using the mem_base argument.
Discussion follows:
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> On Wed, May 01, 2019 at 09:17:37AM +0200, Esben Haabendal wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>
>> > Hmm... Currently it's done inside individual port drivers, like 8250_dw.c.
>> > Each of the drivers can do it differently, for example 8250_lpss.c or
>> > 8250_pnp.c.
>>
>> So, you would prefer to create a new "specialized" port driver that uses
>> platform resources? I am not doing anything else different from
>> the generic port driver here in 8250_core.c.
>
> If it's required and using serial8250 directly is not enough.
Sorry, I am not sure what you mean by that.
>> >> + if (!(port->flags & UPF_DEV_RESOURCES))
>> >> + release_mem_region(port->mapbase, size);
>> >
>> > This is again same issue. The parent should not request resource it
>> > doesn't use.
>>
>> Yes, this is same issue.
>>
>> But the last part is not true. A parent mfd driver might "use" a memory
>> resource for the sole purpose of splitting it up for it's mfd child
>> devices. This is a core part of mfd framework, and not something I am
>> inventing with this patch. I am just trying to make it possible to use
>> 8250 driver in that context.
>>
>> > I think I understand what is a confusion here.
>> >
>> > For the IO resources we have two operations:
>> > - mapping / re-mapping (may be shared)
>> > - requesting (exclusive)
>> >
>> > In the parenthesis I put a level of access to it. While many device
>> > drivers can *share* same resource (mapped or unmapped), the only one
>> > can actually request it.
>>
>> Mostly true. But there is an important twist to the exclusive restriction.
>>
>> The exclusive part of the request is limited to the the same root/parent
>> resource.
>>
>> When you request a memory resource from the root resource
>> (iomem_resource), the resource returned can be used as a new parent
>> resource. This new parent can then be used to give exclusive access to
>> slices of that resource. When used like that, I expect that the parent
>> resource is not supposed to be used for anything else than honoring
>> resource requests.
>>
>> And this is exactly what mfd-core uses the mem_base argument
>> in mfd_add_devices().
>>
>> > So, the parent can take an slice resources as it would be
>> > appropriated, but not requesting them.
>>
>> The parent is not and should not be doing that by itself. The request
>> is done on by mfd-core when mfd_add_devices() is called.
>
> No, MFD *does not* (and actually *may not* in order to allow standalone drivers
> to be used as children w/o modifications) request resources. It just passes
> them to children as parent suggested.
In drivers/mfd/mfd-core.c:mfd_add_device() :
for (r = 0; r < cell->num_resources; r++) {
res[r].name = cell->resources[r].name;
res[r].flags = cell->resources[r].flags;
/* Find out base to use */
if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
res[r].parent = mem_base;
res[r].start = mem_base->start +
cell->resources[r].start;
res[r].end = mem_base->start +
cell->resources[r].end;
} else if (cell->resources[r].flags & IORESOURCE_IRQ) {
if (domain) {
/* Unable to create mappings for IRQ ranges. */
WARN_ON(cell->resources[r].start !=
cell->resources[r].end);
res[r].start = res[r].end = irq_create_mapping(
domain, cell->resources[r].start);
} else {
res[r].start = irq_base +
cell->resources[r].start;
res[r].end = irq_base +
cell->resources[r].end;
}
} else {
res[r].parent = cell->resources[r].parent;
res[r].start = cell->resources[r].start;
res[r].end = cell->resources[r].end;
}
if (!cell->ignore_resource_conflicts) {
if (has_acpi_companion(&pdev->dev)) {
ret = acpi_check_resource_conflict(&res[r]);
if (ret)
goto fail_alias;
}
}
}
ret = platform_device_add_resources(pdev, res, cell->num_resources);
This creates the child resources. Whether we call that requesting the
resources or not, is a matter of word. But it is what it is. When it
is done, you cannot use request_mem_region() for those memory resources,
they are now locked/exclusive for the mfd parent *and* for the
respective mfd child device.
In order to use them, child devices simply use platform_get_resource(),
and everything works nicely. It works fine for normal (non-mfd)
devices, as they get (requests) the resources from the root resource
(iomem_resource), and works fine for mfd devices as well. So no changes
are needed for drivers to work with mfd.
Whether you call the thing that mfd_add_device() does for "request
resources" or just "pass them to children" is a matter of words. The
mfd (parent) has a resource which it cuts up into slices for its
children, and these slices are passed to the child devices. The drivers
for these child devices must then pickup the resource(s) using
platform_get_resource(). At no point is any "request_*" function
called.
Looking at in another way.
The request_mem_region() macro call __request_resource(), which which
simply creates a new 'struct resource' in the iomem_resource resource.
In mfd_add_device(), almost the same happens. A new 'struct resource'
is created in the mem_base resource.
In both cases, a 'struct resource' is created, representing exclusive
access to the resource. And like it or not, this is something that MFD
already *do*, and I think it is way out of scope of this patch to change
that.
I just try to make serial8250 driver work nicely in that (mfd) context,
without changing how mfd is working.
>> > OTOH, it's possible to have a (weird) MFD case where parent *requested*
>> > resources, and *all* of its children are aware of that.
>>
>> I am not sure what you mean with this, but mfd drivers should not pass
>> along it's intire requested memory resource(s) to child devices. The
>> child devices will get the requested resource slices, as implemented by
>> mfd_add_devices().
>>
>> I hope you can see that I am not violating any fundamental design
>> decissions here, but actually try adhere to them (resource management,
>> platform_device resource management, and mfd-core).
/Esben
^ permalink raw reply
* re: serial: Add Milbeaut serial control
From: Colin Ian King @ 2019-05-02 11:47 UTC (permalink / raw)
To: Sugaya Taichi
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial,
linux-kernel@vger.kernel.org
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?
Colin
^ permalink raw reply
* Re: cpu power up timing changes causes UART rx character loss on imx6ull
From: Fabio Estevam @ 2019-05-02 11:31 UTC (permalink / raw)
To: Christoph Niedermaier
Cc: linux-serial@vger.kernel.org, Anson Huang, Sascha Hauer,
linux-imx@nxp.com, kernel@pengutronix.de,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <kcis.DC82F67FDB724ECD9A98DE07DACAC907@DHPLMX01>
Hi Christoph,
On Thu, May 2, 2019 at 6:40 AM Christoph Niedermaier
<cniedermaier@dh-electronics.com> wrote:
>
> I have tested the patch with kernel version 5.1-rc7 and I could not reproduce the loss of characters any more.
Excellent. I have just sent a formal patch.
> Couldn't it perhaps be better to name the define SW2ISO_IMX6SX instead of SW2ISO_IMX6X?
Yes, I have changed it in the forma patch. Please reply with your
Tested-by when you have a chance.
Thanks
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Andy Shevchenko @ 2019-05-02 10:45 UTC (permalink / raw)
To: Esben Haabendal
Cc: linux-serial, Enrico Weigelt, Greg Kroah-Hartman, Jiri Slaby,
Darwin Dingel, Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe,
Marek Vasut, Douglas Anderson, Paul Burton, linux-kernel
In-Reply-To: <874l6efxta.fsf@haabendal.dk>
On Wed, May 01, 2019 at 09:17:37AM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > Hmm... Currently it's done inside individual port drivers, like 8250_dw.c.
> > Each of the drivers can do it differently, for example 8250_lpss.c or
> > 8250_pnp.c.
>
> So, you would prefer to create a new "specialized" port driver that uses
> platform resources? I am not doing anything else different from
> the generic port driver here in 8250_core.c.
If it's required and using serial8250 directly is not enough.
> >> + if (!(port->flags & UPF_DEV_RESOURCES))
> >> + release_mem_region(port->mapbase, size);
> >
> > This is again same issue. The parent should not request resource it
> > doesn't use.
>
> Yes, this is same issue.
>
> But the last part is not true. A parent mfd driver might "use" a memory
> resource for the sole purpose of splitting it up for it's mfd child
> devices. This is a core part of mfd framework, and not something I am
> inventing with this patch. I am just trying to make it possible to use
> 8250 driver in that context.
>
> > I think I understand what is a confusion here.
> >
> > For the IO resources we have two operations:
> > - mapping / re-mapping (may be shared)
> > - requesting (exclusive)
> >
> > In the parenthesis I put a level of access to it. While many device
> > drivers can *share* same resource (mapped or unmapped), the only one
> > can actually request it.
>
> Mostly true. But there is an important twist to the exclusive restriction.
>
> The exclusive part of the request is limited to the the same root/parent
> resource.
>
> When you request a memory resource from the root resource
> (iomem_resource), the resource returned can be used as a new parent
> resource. This new parent can then be used to give exclusive access to
> slices of that resource. When used like that, I expect that the parent
> resource is not supposed to be used for anything else than honoring
> resource requests.
>
> And this is exactly what mfd-core uses the mem_base argument
> in mfd_add_devices().
>
> > So, the parent can take an slice resources as it would be
> > appropriated, but not requesting them.
>
> The parent is not and should not be doing that by itself. The request
> is done on by mfd-core when mfd_add_devices() is called.
No, MFD *does not* (and actually *may not* in order to allow standalone drivers
to be used as children w/o modifications) request resources. It just passes
them to children as parent suggested.
> > OTOH, it's possible to have a (weird) MFD case where parent *requested*
> > resources, and *all* of its children are aware of that.
>
> I am not sure what you mean with this, but mfd drivers should not pass
> along it's intire requested memory resource(s) to child devices. The
> child devices will get the requested resource slices, as implemented by
> mfd_add_devices().
>
> I hope you can see that I am not violating any fundamental design
> decissions here, but actually try adhere to them (resource management,
> platform_device resource management, and mfd-core).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] PM / core: Propagate dev->power.wakeup_path when no callbacks
From: Rafael J. Wysocki @ 2019-05-01 10:21 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-pm, Geert Uytterhoeven, Loic Pallardy, Linus Walleij,
Alexandre Torgue, Rob Herring, Greg Kroah-Hartman, Johan Hovold,
linux-gpio, linux-serial, linux-kernel
In-Reply-To: <20190410095516.6170-1-ulf.hansson@linaro.org>
On Wednesday, April 10, 2019 11:55:16 AM CEST Ulf Hansson wrote:
> The dev->power.direct_complete flag may become set in device_prepare() in
> case the device don't have any PM callbacks (dev->power.no_pm_callbacks is
> set). This leads to a broken behaviour, when there is child having wakeup
> enabled and relies on its parent to be used in the wakeup path.
>
> More precisely, when the direct complete path becomes selected for the
> child in __device_suspend(), the propagation of the dev->power.wakeup_path
> becomes skipped as well.
>
> Let's address this problem, by checking if the device is a part the wakeup
> path or has wakeup enabled, then prevent the direct complete path from
> being used.
>
> Reported-by: Loic Pallardy <loic.pallardy@st.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> More background:
>
> This problem was reported by Loic Pallardy, offlist, while he was working
> on enabling wakeup for a tty serial console driver.
>
> When I looked more closely, I noticed that uart_suspend_port() calls
> device_may_wakeup() for the tty child devices, and then also the used serial
> driver check its device (parent) for device_may_wakeup(). To me this looks like
> workarounds to fix a behaviour that really should be dealt with from the PM
> core, no matter of whether the child have PM callbacks assigned or not.
>
> In other words, it seems like the serial driver(s) should be checking the
> wakeup_path flag for the parent, solely, instead.
>
> I haven't digested further behaviours for other subsystem, but recently
> reviewed a patch for a gpio driver [1], that seems to be suffering from the
> similar problems.
>
> Kind regards
> Ulf Hansson
>
> [1]
> https://lkml.org/lkml/2019/4/4/1283
>
> ---
> drivers/base/power/main.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 41eba82ee7b9..f9cfdeee8288 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1747,6 +1747,10 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
> if (dev->power.syscore)
> goto Complete;
>
> + /* Avoid direct_complete, to let wakeup_path being propagated. */
> + if (device_may_wakeup(dev) || dev->power.wakeup_path)
> + dev->power.direct_complete = false;
> +
> if (dev->power.direct_complete) {
> if (pm_runtime_status_suspended(dev)) {
> pm_runtime_disable(dev);
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-05-01 7:17 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-serial, Enrico Weigelt, Greg Kroah-Hartman, Jiri Slaby,
Darwin Dingel, Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe,
Marek Vasut, Douglas Anderson, Paul Burton, linux-kernel
In-Reply-To: <20190430153736.GL9224@smile.fi.intel.com>
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> On Tue, Apr 30, 2019 at 04:04:13PM +0200, Esben Haabendal wrote:
>> Remaining platform_data fields (other than mapbase, iobase, mapsize and
>> irq) are used just as before. Note
>
> Note what?
Note nothing. I will remove it, sorry about that.
>> +static int serial8250_probe_resources(struct platform_device *pdev,
>> + unsigned int num,
>> + struct plat_serial8250_port *p,
>> + struct uart_8250_port *uart)
>> +{
>> + struct resource *r;
>> + int irq;
>> +
>> + switch (p->iotype) {
>> + case UPIO_AU:
>> + case UPIO_TSI:
>> + case UPIO_MEM32:
>> + case UPIO_MEM32BE:
>> + case UPIO_MEM16:
>> + case UPIO_MEM:
>> + r = platform_get_resource(pdev, IORESOURCE_MEM, num);
>> + if (!r)
>> + return -ENODEV;
>> + uart->port.mapbase = r->start;
>> + uart->port.mapsize = resource_size(r);
>> + uart->port.flags |= UPF_IOREMAP;
>> + break;
>> + case UPIO_HUB6:
>> + case UPIO_PORT:
>> + r = platform_get_resource(pdev, IORESOURCE_IO, num);
>> + if (!r)
>> + return -ENODEV;
>> + uart->port.iobase = r->start;
>> + uart->port.mapsize = resource_size(r);
>> + break;
>> + }
>> +
>> + irq = platform_get_irq(pdev, num);
>> + if (irq == -ENXIO)
>> + uart->port.irq = 0; /* no interrupt -> use polling */
>> + else if (irq < 0)
>> + return irq;
>> + uart->port.irq = irq;
>> +
>> + return 0;
>> +}
>
> Hmm... Currently it's done inside individual port drivers, like 8250_dw.c.
> Each of the drivers can do it differently, for example 8250_lpss.c or
> 8250_pnp.c.
So, you would prefer to create a new "specialized" port driver that uses
platform resources? I am not doing anything else different from
the generic port driver here in 8250_core.c.
>> + if (!(port->flags & UPF_DEV_RESOURCES))
>> + release_mem_region(port->mapbase, size);
>
> This is again same issue. The parent should not request resource it
> doesn't use.
Yes, this is same issue.
But the last part is not true. A parent mfd driver might "use" a memory
resource for the sole purpose of splitting it up for it's mfd child
devices. This is a core part of mfd framework, and not something I am
inventing with this patch. I am just trying to make it possible to use
8250 driver in that context.
> I think I understand what is a confusion here.
>
> For the IO resources we have two operations:
> - mapping / re-mapping (may be shared)
> - requesting (exclusive)
>
> In the parenthesis I put a level of access to it. While many device
> drivers can *share* same resource (mapped or unmapped), the only one
> can actually request it.
Mostly true. But there is an important twist to the exclusive restriction.
The exclusive part of the request is limited to the the same root/parent
resource.
When you request a memory resource from the root resource
(iomem_resource), the resource returned can be used as a new parent
resource. This new parent can then be used to give exclusive access to
slices of that resource. When used like that, I expect that the parent
resource is not supposed to be used for anything else than honoring
resource requests.
And this is exactly what mfd-core uses the mem_base argument
in mfd_add_devices().
> So, the parent can take an slice resources as it would be
> appropriated, but not requesting them.
The parent is not and should not be doing that by itself. The request
is done on by mfd-core when mfd_add_devices() is called.
> OTOH, it's possible to have a (weird) MFD case where parent *requested*
> resources, and *all* of its children are aware of that.
I am not sure what you mean with this, but mfd drivers should not pass
along it's intire requested memory resource(s) to child devices. The
child devices will get the requested resource slices, as implemented by
mfd_add_devices().
I hope you can see that I am not violating any fundamental design
decissions here, but actually try adhere to them (resource management,
platform_device resource management, and mfd-core).
/Esben
^ permalink raw reply
* Re: [PATCH 06/41] drivers: tty: serial: sb1250-duart: use dev_err() instead of printk()
From: Maciej W. Rozycki @ 2019-05-01 2:29 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: linux-kernel, Greg Kroah-Hartman, andrew, andriy.shevchenko, vz,
slemieux.tyco, khilman, liviu.dudau, sudeep.holla,
lorenzo.pieralisi, David S. Miller, jacmet, linux, matthias.bgg,
linux-mips, linux-serial, linux-ia64, linux-amlogic, linuxppc-dev,
sparclinux
In-Reply-To: <1556369542-13247-7-git-send-email-info@metux.net>
On Sat, 27 Apr 2019, Enrico Weigelt, metux IT consult wrote:
> diff --git a/drivers/tty/serial/sb1250-duart.c b/drivers/tty/serial/sb1250-duart.c
> index 329aced..655961c 100644
> --- a/drivers/tty/serial/sb1250-duart.c
> +++ b/drivers/tty/serial/sb1250-duart.c
> @@ -663,7 +663,6 @@ static void sbd_release_port(struct uart_port *uport)
>
> static int sbd_map_port(struct uart_port *uport)
> {
> - const char *err = KERN_ERR "sbd: Cannot map MMIO\n";
> struct sbd_port *sport = to_sport(uport);
> struct sbd_duart *duart = sport->duart;
>
> @@ -671,7 +670,7 @@ static int sbd_map_port(struct uart_port *uport)
> uport->membase = ioremap_nocache(uport->mapbase,
> DUART_CHANREG_SPACING);
> if (!uport->membase) {
> - printk(err);
> + dev_err(uport->dev, "Cannot map MMIO (base)\n");
> return -ENOMEM;
> }
>
> @@ -679,7 +678,7 @@ static int sbd_map_port(struct uart_port *uport)
> sport->memctrl = ioremap_nocache(duart->mapctrl,
> DUART_CHANREG_SPACING);
> if (!sport->memctrl) {
> - printk(err);
> + dev_err(uport->dev, "Cannot map MMIO (ctrl)\n");
> iounmap(uport->membase);
> uport->membase = NULL;
> return -ENOMEM;
Hmm, what's the point to have separate messages, which consume extra
memory, for a hardly if at all possible error condition?
Maciej
^ permalink raw reply
* Re: [PATCH 01/41] drivers: tty: serial: dz: use dev_err() instead of printk()
From: Maciej W. Rozycki @ 2019-05-01 2:20 UTC (permalink / raw)
To: Greg KH
Cc: Enrico Weigelt, metux IT consult,
Enrico Weigelt, metux IT consult, linux-kernel, andrew,
andriy.shevchenko, vz, slemieux.tyco, khilman, liviu.dudau,
sudeep.holla, lorenzo.pieralisi, David S. Miller, jacmet, linux,
matthias.bgg, linux-mips, linux-serial, linux-ia64, linux-amlogic,
linuxppc-dev, sparclinux
In-Reply-To: <20190429131224.GA27385@kroah.com>
On Mon, 29 Apr 2019, Greg KH wrote:
> > >> drivers/tty/serial/dz.c | 8 ++++----
> > >
> > > Do you have this hardware to test any of these changes with?
> >
> > Unfortunately not :(
>
> Then I can take the "basic" types of patches for the driver (like this
> one), but not any others, sorry.
I can verify changes to dz.c, sb1250-duart.c and zs.c with real hardware,
but regrettably not right away: the hardware is in a remote location and
while I have it wired for remote operation unfortunately its connectivity
has been cut off by an unfriendly ISP.
I'm not sure if all the changes make sense though: if there is a compiler
warning or a usability issue, then a patch is surely welcome, otherwise:
"If it ain't broke, don't fix it".
Maciej
^ permalink raw reply
* Re: [PATCH 41/41] drivers: tty: serial: lpc32xx_hs: fill mapsize and use it
From: Vladimir Zapolskiy @ 2019-04-30 20:52 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult, linux-kernel
Cc: gregkh, andrew, andriy.shevchenko, macro, slemieux.tyco, khilman,
liviu.dudau, sudeep.holla, lorenzo.pieralisi, davem, jacmet,
linux, matthias.bgg, linux-mips, linux-serial, linux-ia64,
linux-amlogic, linuxppc-dev, sparclinux
In-Reply-To: <1556369542-13247-42-git-send-email-info@metux.net>
Hi Enrico,
On 04/27/2019 03:52 PM, Enrico Weigelt, metux IT consult wrote:
> Fill the struct uart_port->mapsize field and use it, insteaf of
typo, s/insteaf/instead/
> hardcoded values in many places. This makes the code layout a bit
> more consistent and easily allows using generic helpers for the
> io memory handling.
>
> Candidates for such helpers could be eg. the request+ioremap and
> iounmap+release combinations.
>
> Signed-off-by: Enrico Weigelt <info@metux.net>
Acked-by: Vladimir Zapolskiy <vz@mleia.com>
--
Best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Andy Shevchenko @ 2019-04-30 15:37 UTC (permalink / raw)
To: Esben Haabendal
Cc: linux-serial, Enrico Weigelt, Greg Kroah-Hartman, Jiri Slaby,
Darwin Dingel, Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe,
Marek Vasut, Douglas Anderson, Paul Burton, linux-kernel
In-Reply-To: <20190430140416.4707-1-esben@geanix.com>
On Tue, Apr 30, 2019 at 04:04:13PM +0200, Esben Haabendal wrote:
> Allow getting memory resource (mapbase or iobase) as well as irq from
> platform_device resources.
>
> The UPF_DEV_RESOURCES flag must be set for devices where platform_device
> resources are to be used. When not set, driver behaves as before.
>
> This allows use of the serial8250 driver together with devices with
> resources added by platform_device_add_resources(), such as mfd child
> devices added with mfd_add_devices().
>
> Remaining platform_data fields (other than mapbase, iobase, mapsize and
> irq) are used just as before. Note
Note what?
> +static int serial8250_probe_resources(struct platform_device *pdev,
> + unsigned int num,
> + struct plat_serial8250_port *p,
> + struct uart_8250_port *uart)
> +{
> + struct resource *r;
> + int irq;
> +
> + switch (p->iotype) {
> + case UPIO_AU:
> + case UPIO_TSI:
> + case UPIO_MEM32:
> + case UPIO_MEM32BE:
> + case UPIO_MEM16:
> + case UPIO_MEM:
> + r = platform_get_resource(pdev, IORESOURCE_MEM, num);
> + if (!r)
> + return -ENODEV;
> + uart->port.mapbase = r->start;
> + uart->port.mapsize = resource_size(r);
> + uart->port.flags |= UPF_IOREMAP;
> + break;
> + case UPIO_HUB6:
> + case UPIO_PORT:
> + r = platform_get_resource(pdev, IORESOURCE_IO, num);
> + if (!r)
> + return -ENODEV;
> + uart->port.iobase = r->start;
> + uart->port.mapsize = resource_size(r);
> + break;
> + }
> +
> + irq = platform_get_irq(pdev, num);
> + if (irq == -ENXIO)
> + uart->port.irq = 0; /* no interrupt -> use polling */
> + else if (irq < 0)
> + return irq;
> + uart->port.irq = irq;
> +
> + return 0;
> +}
Hmm... Currently it's done inside individual port drivers, like 8250_dw.c.
Each of the drivers can do it differently, for example 8250_lpss.c or
8250_pnp.c.
> + if (!(port->flags & UPF_DEV_RESOURCES))
> + release_mem_region(port->mapbase, size);
This is again same issue. The parent should not request resource it doesn't use.
I think I understand what is a confusion here.
For the IO resources we have two operations:
- mapping / re-mapping (may be shared)
- requesting (exclusive)
In the parenthesis I put a level of access to it. While many device drivers can
*share* same resource (mapped or unmapped), the only one can actually request
it.
So, the parent can take an slice resources as it would be appropriated, but not
requesting them.
OTOH, it's possible to have a (weird) MFD case where parent *requested*
resources, and *all* of its children are aware of that.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
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