[PATCH] serial: 8250: Add support for using platform_device resources

[PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

Allow getting memory resource (mapbase or iobase) as well as irq from
platform_device resources.

The UPF_DEV_RESOURCES flag must be set for devices where platform_device
resources are to be used.  When not set, driver behaves as before.

This allows use of the serial8250 driver together with devices with
resources added by platform_device_add_resources(), such as mfd child
devices added with mfd_add_devices().

Remaining platform_data fields (other than mapbase, iobase, mapsize and
irq) are used just as before.  Note

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/tty/serial/8250/8250_core.c | 56 +++++++++++++++++++++++++++++++++----
 drivers/tty/serial/8250/8250_port.c | 15 ++++++----
 include/linux/serial_core.h         |  1 +
 3 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index e441221..9df6a98 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -788,6 +788,48 @@ void serial8250_resume_port(int line)
 }
 EXPORT_SYMBOL(serial8250_resume_port);
 
+static int serial8250_probe_resources(struct platform_device *pdev,
+				      unsigned int num,
+				      struct plat_serial8250_port *p,
+				      struct uart_8250_port *uart)
+{
+	struct resource *r;
+	int irq;
+
+	switch (p->iotype) {
+	case UPIO_AU:
+	case UPIO_TSI:
+	case UPIO_MEM32:
+	case UPIO_MEM32BE:
+	case UPIO_MEM16:
+	case UPIO_MEM:
+		r = platform_get_resource(pdev, IORESOURCE_MEM, num);
+		if (!r)
+			return -ENODEV;
+		uart->port.mapbase = r->start;
+		uart->port.mapsize = resource_size(r);
+		uart->port.flags |= UPF_IOREMAP;
+		break;
+	case UPIO_HUB6:
+	case UPIO_PORT:
+		r = platform_get_resource(pdev, IORESOURCE_IO, num);
+		if (!r)
+			return -ENODEV;
+		uart->port.iobase = r->start;
+		uart->port.mapsize = resource_size(r);
+		break;
+	}
+
+	irq = platform_get_irq(pdev, num);
+	if (irq == -ENXIO)
+		uart->port.irq = 0; /* no interrupt -> use polling */
+	else if (irq < 0)
+		return irq;
+	uart->port.irq = irq;
+
+	return 0;
+}
+
 /*
  * Register a set of serial devices attached to a platform device.  The
  * list is terminated with a zero flags entry, which means we expect
@@ -805,15 +847,19 @@ static int serial8250_probe(struct platform_device *dev)
 		irqflag = IRQF_SHARED;
 
 	for (i = 0; p && p->flags != 0; p++, i++) {
-		uart.port.iobase	= p->iobase;
-		uart.port.membase	= p->membase;
-		uart.port.irq		= p->irq;
+		uart.port.flags		= p->flags;
+		if (p->flags & UPF_DEV_RESOURCES) {
+			serial8250_probe_resources(dev, i, p, &uart);
+		} else {
+			uart.port.iobase	= p->iobase;
+			uart.port.mapbase	= p->mapbase;
+			uart.port.membase	= p->membase;
+			uart.port.irq		= p->irq;
+		}
 		uart.port.irqflags	= p->irqflags;
 		uart.port.uartclk	= p->uartclk;
 		uart.port.regshift	= p->regshift;
 		uart.port.iotype	= p->iotype;
-		uart.port.flags		= p->flags;
-		uart.port.mapbase	= p->mapbase;
 		uart.port.hub6		= p->hub6;
 		uart.port.private_data	= p->private_data;
 		uart.port.type		= p->type;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index d2f3310..7fa1e49 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2863,7 +2863,8 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 		if (!port->mapbase)
 			break;
 
-		if (!request_mem_region(port->mapbase, size, "serial")) {
+		if (!(port->flags & UPF_DEV_RESOURCES) &&
+		    !request_mem_region(port->mapbase, size, "serial")) {
 			ret = -EBUSY;
 			break;
 		}
@@ -2871,7 +2872,8 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 		if (port->flags & UPF_IOREMAP) {
 			port->membase = ioremap_nocache(port->mapbase, size);
 			if (!port->membase) {
-				release_mem_region(port->mapbase, size);
+				if (!(port->flags & UPF_DEV_RESOURCES))
+					release_mem_region(port->mapbase, size);
 				ret = -ENOMEM;
 			}
 		}
@@ -2879,7 +2881,8 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 
 	case UPIO_HUB6:
 	case UPIO_PORT:
-		if (!request_region(port->iobase, size, "serial"))
+		if (!(port->flags & UPF_DEV_RESOURCES) &&
+		    !request_region(port->iobase, size, "serial"))
 			ret = -EBUSY;
 		break;
 	}
@@ -2906,12 +2909,14 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
 			port->membase = NULL;
 		}
 
-		release_mem_region(port->mapbase, size);
+		if (!(port->flags & UPF_DEV_RESOURCES))
+			release_mem_region(port->mapbase, size);
 		break;
 
 	case UPIO_HUB6:
 	case UPIO_PORT:
-		release_region(port->iobase, size);
+		if (!(port->flags & UPF_DEV_RESOURCES))
+			release_region(port->iobase, size);
 		break;
 	}
 }
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 5fe2b03..87b4ed3 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -207,6 +207,7 @@ struct uart_port {
 #define UPF_BUGGY_UART		((__force upf_t) ASYNC_BUGGY_UART     /* 14 */ )
 #define UPF_MAGIC_MULTIPLIER	((__force upf_t) ASYNC_MAGIC_MULTIPLIER /* 16 */ )
 
+#define UPF_DEV_RESOURCES	((__force upf_t) (1 << 18))
 #define UPF_NO_THRE_TEST	((__force upf_t) (1 << 19))
 /* Port has hardware-assisted h/w flow control */
 #define UPF_AUTO_CTS		((__force upf_t) (1 << 20))
-- 
2.4.11

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

> 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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Lee Jones]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Lee Jones]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

On Tue, May 07, 2019 at 01:35:58PM +0200, Esben Haabendal wrote:
> Lee Jones <lee.jones@linaro.org> writes:
> > On Thu, 02 May 2019, Esben Haabendal wrote:
> >
> >> Could you help clarify whether or not this patch is trying to do
> >> something odd/wrong?
> >> 
> >> I might be misunderstanding Andy (probably is), but the discussion
> >> revolves around the changes I propose where I change the serial8250
> >> driver to use platform_get_resource() in favour of
> >> request_mem_region()/release_mem_region().
> >
> > Since 'serial8250' is registered as a platform device, I don't see any
> > reason why it shouldn't have the capability to obtain its memory
> > regions from the platform_get_*() helpers.
> 
> Good to hear.  That is exactly what I am trying do with this patch.
> 
> @Andy: If you still don't like my approach, could you please advice an
> acceptable method for improving the serial8250 driver to allow the use
> of platform_get_*() helpers?

I still don't get why you need this.

If it's MFD, you may use "serial8250" with a given platform data like dozens of
current users do.

Another approach is to use 8250 library, thus, creating a specific glue driver
(like all 8250_* do).

Yes, I understand that 8250 driver is full of quirks and not modern approaches
to do one or another thing. Unfortunately it's not too easy to fix it without
uglifying code and doing some kind of ping-pong thru the conversion. I don't
think it worth to do it in the current state of affairs. Though, cleaning up
the core part from the quirks and custom pieces would make this task
achievable.

I'm also puzzled why you don't use FPGA manager which should handle, as far as
I understand, very flexible configurations of FPGAs.

Btw, what exact IP of UART do you have implemented there?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Tue, May 07, 2019 at 01:35:58PM +0200, Esben Haabendal wrote:
>> Lee Jones <lee.jones@linaro.org> writes:
>> > On Thu, 02 May 2019, Esben Haabendal wrote:
>> >
>> >> Could you help clarify whether or not this patch is trying to do
>> >> something odd/wrong?
>> >> 
>> >> I might be misunderstanding Andy (probably is), but the discussion
>> >> revolves around the changes I propose where I change the serial8250
>> >> driver to use platform_get_resource() in favour of
>> >> request_mem_region()/release_mem_region().
>> >
>> > Since 'serial8250' is registered as a platform device, I don't see any
>> > reason why it shouldn't have the capability to obtain its memory
>> > regions from the platform_get_*() helpers.
>> 
>> Good to hear.  That is exactly what I am trying do with this patch.
>> 
>> @Andy: If you still don't like my approach, could you please advice an
>> acceptable method for improving the serial8250 driver to allow the use
>> of platform_get_*() helpers?
>
> I still don't get why you need this.

Because platform_get_resource() is a generally available and useful
helper function for working with platform_device resources, that the
current standard serial8250 driver does not support.

I am uncertain if I still haven't convinced you that current serial8250
driver does not work with platform_get_resource(), or if you believe
that it really should not support it.

> If it's MFD, you may use "serial8250" with a given platform data like
> dozens of current users do.

There is only one in-tree mfd driver using "serial8250", the sm501.c
driver.  And that driver predates the mfd framework (mfd-core.c) by a
year, and does not use any of the mfd-core functionality.

I want to use the mfd-core provided handling of resource splitting,
because it makes it easier to handle splitting of a single memory
resource as defined by a PCI BAR in this case.  And the other drivers I
need to use all support/use platform_get_resource(), so it would even
have an impact on the integration of that if I cannot use mfd resource
splitting with serial8250.

> Another approach is to use 8250 library, thus, creating a specific glue driver
> (like all 8250_* do).

As mentioned, I think this is a bad approach, and I would prefer to
improve the "serial8250" driver instead.  But if you insist, what should
I call such a driver?  It needs a platform_driver name, for use when
matching with platform_device devices.  And it would support exactly the
same hardware as the current "serial8250" driver.

> Yes, I understand that 8250 driver is full of quirks and not modern approaches
> to do one or another thing. Unfortunately it's not too easy to fix it without
> uglifying code and doing some kind of ping-pong thru the conversion. I don't
> think it worth to do it in the current state of affairs. Though, cleaning up
> the core part from the quirks and custom pieces would make this task
> achievable.

I think it should be possible and worthwhile to improve serial8250
driver with support for using platform_device resources
(platform_get_resource() helper).

If we could stop discussing if it is a proper thing to do, we could try
to find a good way to do it instead.

> I'm also puzzled why you don't use FPGA manager which should handle, as far as
> I understand, very flexible configurations of FPGAs.

FPGA manager is for programming FPGA's.  The FPGA's used in this project
read their configuration from EEPROM.

I don't see any overlap of FPGA manager with MFD.  They server
completely different purposes, and could very well both be used for the
same FPGA's.

> Btw, what exact IP of UART do you have implemented there?

It is an XPS 16550 UART (v3.00a).
https://www.xilinx.com/support/documentation/ip_documentation/xps_uart16550.pdf

There are 5 of them in one FPGA, together with 3 XPS LL TEMAC Ethernet
IP blocks, an IRQ controller, and a number of custom IP blocks.

/Esben

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > On Tue, May 07, 2019 at 01:35:58PM +0200, Esben Haabendal wrote:
> >> Lee Jones <lee.jones@linaro.org> writes:
> >> > On Thu, 02 May 2019, Esben Haabendal wrote:
> >> >
> >> >> Could you help clarify whether or not this patch is trying to do
> >> >> something odd/wrong?
> >> >> 
> >> >> I might be misunderstanding Andy (probably is), but the discussion
> >> >> revolves around the changes I propose where I change the serial8250
> >> >> driver to use platform_get_resource() in favour of
> >> >> request_mem_region()/release_mem_region().
> >> >
> >> > Since 'serial8250' is registered as a platform device, I don't see any
> >> > reason why it shouldn't have the capability to obtain its memory
> >> > regions from the platform_get_*() helpers.
> >> 
> >> Good to hear.  That is exactly what I am trying do with this patch.
> >> 
> >> @Andy: If you still don't like my approach, could you please advice an
> >> acceptable method for improving the serial8250 driver to allow the use
> >> of platform_get_*() helpers?
> >
> > I still don't get why you need this.
> 
> Because platform_get_resource() is a generally available and useful
> helper function for working with platform_device resources, that the
> current standard serial8250 driver does not support.
> 
> I am uncertain if I still haven't convinced you that current serial8250
> driver does not work with platform_get_resource(), or if you believe
> that it really should not support it.

I believe there is no need to do this support.

Most of the platform code that uses it is quite legacy, and all under arch/
ideally should be converted to use Device Tree.

> > If it's MFD, you may use "serial8250" with a given platform data like
> > dozens of current users do.
> 
> There is only one in-tree mfd driver using "serial8250", the sm501.c
> driver.  And that driver predates the mfd framework (mfd-core.c) by a
> year, and does not use any of the mfd-core functionality.

So, does it have an issue?

> I want to use the mfd-core provided handling of resource splitting,
> because it makes it easier to handle splitting of a single memory
> resource as defined by a PCI BAR in this case.  And the other drivers I
> need to use all support/use platform_get_resource(), so it would even
> have an impact on the integration of that if I cannot use mfd resource
> splitting with serial8250.

I tired to repeat, that is OKAY! You *may* split and supply resources to the
drivers, nothing prevents you to do that with current code base.

Do you see any problem with that? What is that problem?

If you would like utilize serial8250, just provide a platform data for it.

> > Another approach is to use 8250 library, thus, creating a specific glue driver
> > (like all 8250_* do).
> 
> As mentioned, I think this is a bad approach, and I would prefer to
> improve the "serial8250" driver instead.  But if you insist, what should
> I call such a driver?  It needs a platform_driver name, for use when
> matching with platform_device devices.  And it would support exactly the
> same hardware as the current "serial8250" driver.

If you need some specifics, you create a driver with whatever name
suits the IP in question. Nevertheless, if it's simple generic 8250, nothing
needs to be added, except platform data, see above.

> > Yes, I understand that 8250 driver is full of quirks and not modern approaches
> > to do one or another thing. Unfortunately it's not too easy to fix it without
> > uglifying code and doing some kind of ping-pong thru the conversion. I don't
> > think it worth to do it in the current state of affairs. Though, cleaning up
> > the core part from the quirks and custom pieces would make this task
> > achievable.
> 
> I think it should be possible and worthwhile to improve serial8250
> driver with support for using platform_device resources
> (platform_get_resource() helper).

I simple can't understand why it's needed. What problem would it solve which
can't be solved with existing code base?

> If we could stop discussing if it is a proper thing to do, we could try
> to find a good way to do it instead.

> > Btw, what exact IP of UART do you have implemented there?
> 
> It is an XPS 16550 UART (v3.00a).
> https://www.xilinx.com/support/documentation/ip_documentation/xps_uart16550.pdf

So, briefly looking at it I didn't find any deviations from a standard 16550a.

Also there are two drivers mentioned Xilinx, though I'm pretty sure it's not
your case.

Since you have more than one of them, it's even smaller to use current
infrastructure to enumerate them using only one serial8250 description.
See plenty examples in the Linux kernel, such as 8250_exar_st16c554.c.
That is what you may just modify for your needs and put inside your MFD.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > On Tue, May 07, 2019 at 01:35:58PM +0200, Esben Haabendal wrote:
>> >> Lee Jones <lee.jones@linaro.org> writes:
>> >> > On Thu, 02 May 2019, Esben Haabendal wrote:
>> >> >
>> >> >> Could you help clarify whether or not this patch is trying to do
>> >> >> something odd/wrong?
>> >> >> 
>> >> >> I might be misunderstanding Andy (probably is), but the discussion
>> >> >> revolves around the changes I propose where I change the serial8250
>> >> >> driver to use platform_get_resource() in favour of
>> >> >> request_mem_region()/release_mem_region().
>> >> >
>> >> > Since 'serial8250' is registered as a platform device, I don't see any
>> >> > reason why it shouldn't have the capability to obtain its memory
>> >> > regions from the platform_get_*() helpers.
>> >> 
>> >> Good to hear.  That is exactly what I am trying do with this patch.
>> >> 
>> >> @Andy: If you still don't like my approach, could you please advice an
>> >> acceptable method for improving the serial8250 driver to allow the use
>> >> of platform_get_*() helpers?
>> >
>> > I still don't get why you need this.
>> 
>> Because platform_get_resource() is a generally available and useful
>> helper function for working with platform_device resources, that the
>> current standard serial8250 driver does not support.
>> 
>> I am uncertain if I still haven't convinced you that current serial8250
>> driver does not work with platform_get_resource(), or if you believe
>> that it really should not support it.
>
> I believe there is no need to do this support.
>
> Most of the platform code that uses it is quite legacy,

So all code that use/support platform_get_resource() is legacy code?

commit 7945f929f1a77a1c8887a97ca07f87626858ff42
Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Date:   Wed Feb 20 11:12:39 2019 +0000

    drivers: provide devm_platform_ioremap_resource()
    
    There are currently 1200+ instances of using platform_get_resource()
    and devm_ioremap_resource() together in the kernel tree.
    
    This patch wraps these two calls in a single helper. Thanks to that
    we don't have to declare a local variable for struct resource * and can
    omit the redundant argument for resource type. We also have one
    function call less.
    
    Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
    Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

It does not looks quite dead to me.

> and all under arch/
> ideally should be converted to use Device Tree.

When do you expect arch/x86 to be converted to device tree?

>> > If it's MFD, you may use "serial8250" with a given platform data like
>> > dozens of current users do.
>> 
>> There is only one in-tree mfd driver using "serial8250", the sm501.c
>> driver.  And that driver predates the mfd framework (mfd-core.c) by a
>> year, and does not use any of the mfd-core functionality.
>
> So, does it have an issue?

I don't have hardware so I can test it, but I assume that it is
working.

It is ignoring framework code (mfd-core), that is implemented to await
re-inventing the wheel for each and every mfd driver.  If that is an
issue, then yes, sm501.c does have an issue and could be improved/fixed.

>> I want to use the mfd-core provided handling of resource splitting,
>> because it makes it easier to handle splitting of a single memory
>> resource as defined by a PCI BAR in this case.  And the other drivers I
>> need to use all support/use platform_get_resource(), so it would even
>> have an impact on the integration of that if I cannot use mfd resource
>> splitting with serial8250.
>
> I tired to repeat, that is OKAY! You *may* split and supply resources to the
> drivers, nothing prevents you to do that with current code base.
>
> Do you see any problem with that? What is that problem?
>
> If you would like utilize serial8250, just provide a platform data for
> it.

I fear we are coming to an end here.

I don't seem to be able to break through to you, to get you to
understand the issue here.

I want to write a simple and elegant mfd driver, using mfd-core
framework (the mfd_add_devices() function call to be specific).  I don't
want to reimplement similar functionality in the mfd driver.

The other drivers I need all work fine with this, but serial8250 does
not.

As I understand Lee Jones, he seem to agree with me, so could you
please, please consider that I might not be totally on crack, and might
actually have brough forward a valid proposition.

>> > Another approach is to use 8250 library, thus, creating a specific
>> > glue driver (like all 8250_* do).
>> 
>> As mentioned, I think this is a bad approach, and I would prefer to
>> improve the "serial8250" driver instead.  But if you insist, what should
>> I call such a driver?  It needs a platform_driver name, for use when
>> matching with platform_device devices.  And it would support exactly the
>> same hardware as the current "serial8250" driver.
>
> If you need some specifics, you create a driver with whatever name
> suits the IP in question. Nevertheless, if it's simple generic 8250, nothing
> needs to be added, except platform data, see above.

We are on repeat here.  I don't agree with you here.  I have a simple
generic 8250 (16550A) compatible device, and cannot use it in a mfd
driver using the standard mfd-core framework.

The lacking of support for platform_get_resource() in the generic
serial8250 driver is not a feature.  It should be supported, just as it
is in several of the specialized 8250 drivers.

>> > Yes, I understand that 8250 driver is full of quirks and not modern
>> > approaches
>> > to do one or another thing. Unfortunately it's not too easy to fix it
>> > without
>> > uglifying code and doing some kind of ping-pong thru the conversion. I don't
>> > think it worth to do it in the current state of affairs. Though, cleaning up
>> > the core part from the quirks and custom pieces would make this task
>> > achievable.
>> 
>> I think it should be possible and worthwhile to improve serial8250
>> driver with support for using platform_device resources
>> (platform_get_resource() helper).
>
> I simple can't understand why it's needed. What problem would it solve which
> can't be solved with existing code base?

On repeat again.  I have explained it way to many times, so I guess I
must assume by now that you do not think that being able to use
serial8250 together with mfd-core is something that should be solved.

>> If we could stop discussing if it is a proper thing to do, we could try
>> to find a good way to do it instead.
>
>> > Btw, what exact IP of UART do you have implemented there?
>> 
>> It is an XPS 16550 UART (v3.00a).
>> https://www.xilinx.com/support/documentation/ip_documentation/xps_uart16550.pdf
>
> So, briefly looking at it I didn't find any deviations from a standard 16550a.

Exactly. I am pretty sure I have said this more than once in this
thread.  This IP is perfectly standard.  It would be completely wrong to
write a specialized 8250 driver for this.

But if I fail to find a way to get something merged which allows using
the generic serial8250 driver with platform_get_resource(), I guess I
need to handle this out-of-tree.  And anybody else needing/wanting to do
the same would have to do that as well.

> Also there are two drivers mentioned Xilinx, though I'm pretty sure it's not
> your case.
>
> Since you have more than one of them, it's even smaller to use current
> infrastructure to enumerate them using only one serial8250 description.
> See plenty examples in the Linux kernel, such as 8250_exar_st16c554.c.
> That is what you may just modify for your needs and put inside your MFD.

It would still mean that I would have revert to not using convenient and
otherwise fully appropriate API calls like pci_request_regions() and
mfd_add_devices().

The mfd driver in question is for a PCI device.  Not being able to
request the PCI regions seems silly.

Not being able to register all child devices with the call introduced
for that sole purpose also seems silly.

/Esben

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:

> We are on repeat here.  I don't agree with you here.  I have a simple
> generic 8250 (16550A) compatible device, and cannot use it in a mfd
> driver using the standard mfd-core framework.

> The lacking of support for platform_get_resource() in the generic
> serial8250 driver is not a feature.  It should be supported, just as it
> is in several of the specialized 8250 drivers.

We are going circles here.
What exactly prevents you to use it? Presence of request_mem_region()?

> It would still mean that I would have revert to not using convenient and
> otherwise fully appropriate API calls like pci_request_regions() and
> mfd_add_devices().

Yes, here is the issue. 8250 requires the parent not to *request*
resources. Because child handles IO access itself.

> The mfd driver in question is for a PCI device.  Not being able to
> request the PCI regions seems silly.

Nope. Otherwise, the parent which *doesn't handle* IO on behalf of
child should not request its resources.

> Not being able to register all child devices with the call introduced
> for that sole purpose also seems silly.

> Please take a look at https://lkml.org/lkml/2019/4/9/576
> ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")

Thank you for this link.
Now, look at this comment:

+ /*
+ * Map all IOC3 registers.  These are shared between subdevices
+ * so the main IOC3 module manages them.
+ */

Is it your case? Can we see the code?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

On Tue, May 14, 2019 at 12:23 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:

> > Please take a look at https://lkml.org/lkml/2019/4/9/576
> > ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
>
> Thank you for this link.
> Now, look at this comment:
>
> + /*
> + * Map all IOC3 registers.  These are shared between subdevices
> + * so the main IOC3 module manages them.
> + */
>
> Is it your case? Can we see the code?

They do not request resources by the way.
You may do the same, I told you this several times.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Tue, May 14, 2019 at 12:23 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
>
>> > Please take a look at https://lkml.org/lkml/2019/4/9/576
>> > ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
>>
>> Thank you for this link.
>> Now, look at this comment:
>>
>> + /*
>> + * Map all IOC3 registers.  These are shared between subdevices
>> + * so the main IOC3 module manages them.
>> + */
>>
>> Is it your case? Can we see the code?
>
> They do not request resources by the way.

Actually, that looks like a bug in ioc3.c driver.

It is using mfd_add_devices() with a mem_base that has not been properly
requested, and the platform_get_resource() calls made by child drivers
does not guarantee exclusive access to the memory resources, as they are
not inserted in the root memory resource tree.

> You may do the same, I told you this several times.

In drivers/mfd/ioc3.c:

First, the uart resources are defined.  The register memory resource is
defined relative to the mfd driver memory resource.

+static struct resource ioc3_uarta_resources[] = {
+	DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
+		       sizeof_field(struct ioc3, sregs.uarta)),
+	DEFINE_RES_IRQ(6)
+};

This is then used when creating the uart cell.

+		cell->name = "ioc3-serial8250";
+		cell->id = ioc3_serial_id++;
+		cell->resources = ioc3_uarta_resources;
+		cell->num_resources = ARRAY_SIZE(ioc3_uarta_resources);

Finally, the mfd_add_devices() call is made, giving the resource for the
BAR0 region (&ipd->pdev->resource[0]) as mem_base argument:

+	mfd_add_devices(&ipd->pdev->dev, -1, ioc3_mfd_cells,
+			cell - ioc3_mfd_cells, &ipd->pdev->resource[0],
+			0, ipd->domain);

This is just what I want to do.

But in order to guarantee exclusive access to the memory resource, I
need to have it requested.

/Esben

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Tue, May 14, 2019 at 12:23 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
>
>> > Please take a look at https://lkml.org/lkml/2019/4/9/576
>> > ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
>>
>> Thank you for this link.
>> Now, look at this comment:
>>
>> + /*
>> + * Map all IOC3 registers.  These are shared between subdevices
>> + * so the main IOC3 module manages them.
>> + */
>>
>> Is it your case? Can we see the code?
>
> They do not request resources by the way.

Actually, that looks like a bug in ioc3.c driver.

It is using mfd_add_devices() with a mem_base that has not been properly
requested, and the platform_get_resource() calls made by child drivers
does not guarantee exclusive access to the memory resources, as they are
not inserted in the root memory resource tree.

> You may do the same, I told you this several times.

In drivers/mfd/ioc3.c:

First, the uart resources are defined.  The register memory resource is
defined relative to the mfd driver memory resource.

+static struct resource ioc3_uarta_resources[] = {
+	DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
+		       sizeof_field(struct ioc3, sregs.uarta)),
+	DEFINE_RES_IRQ(6)
+};

This is then used when creating the uart cell.

+		cell->name = "ioc3-serial8250";
+		cell->id = ioc3_serial_id++;
+		cell->resources = ioc3_uarta_resources;
+		cell->num_resources = ARRAY_SIZE(ioc3_uarta_resources);

Finally, the mfd_add_devices() call is made, giving the resource for the
BAR0 region (&ipd->pdev->resource[0]) as mem_base argument:

+	mfd_add_devices(&ipd->pdev->dev, -1, ioc3_mfd_cells,
+			cell - ioc3_mfd_cells, &ipd->pdev->resource[0],
+			0, ipd->domain);

This is just what I want to do.

But in order to guarantee exclusive access to the memory resource, I
need to have it requested.

/Esben

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

On Tue, May 14, 2019 at 02:02:40PM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> 
> > On Tue, May 14, 2019 at 12:23 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
> >
> >> > Please take a look at https://lkml.org/lkml/2019/4/9/576
> >> > ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
> >>
> >> Thank you for this link.
> >> Now, look at this comment:
> >>
> >> + /*
> >> + * Map all IOC3 registers.  These are shared between subdevices
> >> + * so the main IOC3 module manages them.
> >> + */
> >>
> >> Is it your case? Can we see the code?
> >
> > They do not request resources by the way.
> 
> Actually, that looks like a bug in ioc3.c driver.

Nope. This is the right thing to do.

> It is using mfd_add_devices() with a mem_base that has not been properly
> requested, and the platform_get_resource() calls made by child drivers
> does not guarantee exclusive access to the memory resources, as they are
> not inserted in the root memory resource tree.

Should platform_get_resource() guarantee that? I think no, otherwise entire MFD
and other logic will collapse.

> > You may do the same, I told you this several times.
> 
> In drivers/mfd/ioc3.c:
> 
> First, the uart resources are defined.  The register memory resource is
> defined relative to the mfd driver memory resource.
> 
> +static struct resource ioc3_uarta_resources[] = {
> +	DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
> +		       sizeof_field(struct ioc3, sregs.uarta)),
> +	DEFINE_RES_IRQ(6)
> +};
> 
> This is then used when creating the uart cell.
> 
> +		cell->name = "ioc3-serial8250";
> +		cell->id = ioc3_serial_id++;
> +		cell->resources = ioc3_uarta_resources;
> +		cell->num_resources = ARRAY_SIZE(ioc3_uarta_resources);
> 
> Finally, the mfd_add_devices() call is made, giving the resource for the
> BAR0 region (&ipd->pdev->resource[0]) as mem_base argument:
> 
> +	mfd_add_devices(&ipd->pdev->dev, -1, ioc3_mfd_cells,
> +			cell - ioc3_mfd_cells, &ipd->pdev->resource[0],
> +			0, ipd->domain);
> 
> This is just what I want to do.
> 

> But in order to guarantee exclusive access to the memory resource, I
> need to have it requested.

Here the root of our misunderstanding each other.

Every driver till now works fine and entire model works fine without resources
being requested.

I told you already that if you want your way that has to be done not in 8250
driver, but in generic code (driver core or even resource framework).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:
>
>> We are on repeat here.  I don't agree with you here.  I have a simple
>> generic 8250 (16550A) compatible device, and cannot use it in a mfd
>> driver using the standard mfd-core framework.
>
>> The lacking of support for platform_get_resource() in the generic
>> serial8250 driver is not a feature.  It should be supported, just as it
>> is in several of the specialized 8250 drivers.
>
> We are going circles here.
> What exactly prevents you to use it? Presence of request_mem_region()?

Exactly.

>> It would still mean that I would have revert to not using convenient and
>> otherwise fully appropriate API calls like pci_request_regions() and
>> mfd_add_devices().
>
> Yes, here is the issue. 8250 requires the parent not to *request*
> resources. Because child handles IO access itself.

Ok, clearly we are not discussing the actual IO access.  The only issue
is how to handle the memory resource management.

And yes, serial8250 requires "the parent" to not request the resources.
But by doing so, it gets in the way of the mfd-core way of splitting a
properly requested resource.

>> The mfd driver in question is for a PCI device.  Not being able to
>> request the PCI regions seems silly.
>
> Nope. Otherwise, the parent which *doesn't handle* IO on behalf of
> child should not request its resources.

If I may, could I get you to discuss this with Lee Jones?

As I read both of your comments in this thread, you are not aligned on
how mfd drivers should handle resources.  And in that case, one of you
are most likely more right than the other, and if Lee is right, I seem
to be unable to convince you about that.

>> Not being able to register all child devices with the call introduced
>> for that sole purpose also seems silly.
>
>> Please take a look at https://lkml.org/lkml/2019/4/9/576
>> ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
>
> Thank you for this link.
> Now, look at this comment:
>
> + /*
> + * Map all IOC3 registers.  These are shared between subdevices
> + * so the main IOC3 module manages them.
> + */
>
> Is it your case? Can we see the code?

That comment seems quite misleading.  I am quote certain that the uart
registers which are part of BAR 0 is not more shared between subdevices
than the uart registers in BAR 0 in my case.

But BAR 0 as a whole is shared between subdevices.  But BAR 0 can be
(is) split in parts that are exclusive to one subdevice.  The only
difference I see is that I don't have any registers accessed directly by
the mfd driver.

/Esben

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:
>
>> We are on repeat here.  I don't agree with you here.  I have a simple
>> generic 8250 (16550A) compatible device, and cannot use it in a mfd
>> driver using the standard mfd-core framework.
>
>> The lacking of support for platform_get_resource() in the generic
>> serial8250 driver is not a feature.  It should be supported, just as it
>> is in several of the specialized 8250 drivers.
>
> We are going circles here.
> What exactly prevents you to use it? Presence of request_mem_region()?

Exactly.

>> It would still mean that I would have revert to not using convenient and
>> otherwise fully appropriate API calls like pci_request_regions() and
>> mfd_add_devices().
>
> Yes, here is the issue. 8250 requires the parent not to *request*
> resources. Because child handles IO access itself.

Ok, clearly we are not discussing the actual IO access.  The only issue
is how to handle the memory resource management.

And yes, serial8250 requires "the parent" to not request the resources.
But by doing so, it gets in the way of the mfd-core way of splitting a
properly requested resource.

>> The mfd driver in question is for a PCI device.  Not being able to
>> request the PCI regions seems silly.
>
> Nope. Otherwise, the parent which *doesn't handle* IO on behalf of
> child should not request its resources.

If I may, could I get you to discuss this with Lee Jones?

As I read both of your comments in this thread, you are not aligned on
how mfd drivers should handle resources.  And in that case, one of you
are most likely more right than the other, and if Lee is right, I seem
to be unable to convince you about that.

>> Not being able to register all child devices with the call introduced
>> for that sole purpose also seems silly.
>
>> Please take a look at https://lkml.org/lkml/2019/4/9/576
>> ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
>
> Thank you for this link.
> Now, look at this comment:
>
> + /*
> + * Map all IOC3 registers.  These are shared between subdevices
> + * so the main IOC3 module manages them.
> + */
>
> Is it your case? Can we see the code?

That comment seems quite misleading.  I am quote certain that the uart
registers which are part of BAR 0 is not more shared between subdevices
than the uart registers in BAR 0 in my case.

But BAR 0 as a whole is shared between subdevices.  But BAR 0 can be
(is) split in parts that are exclusive to one subdevice.  The only
difference I see is that I don't have any registers accessed directly by
the mfd driver.

/Esben

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

On Tue, May 14, 2019 at 02:02:36PM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> > On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> >> > On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:
> >
> >> We are on repeat here.  I don't agree with you here.  I have a simple
> >> generic 8250 (16550A) compatible device, and cannot use it in a mfd
> >> driver using the standard mfd-core framework.
> >
> >> The lacking of support for platform_get_resource() in the generic
> >> serial8250 driver is not a feature.  It should be supported, just as it
> >> is in several of the specialized 8250 drivers.
> >
> > We are going circles here.
> > What exactly prevents you to use it? Presence of request_mem_region()?
> 
> Exactly.

And I completely tired to repeat that this is okay and does not prevent you to
use MFD.

> >> It would still mean that I would have revert to not using convenient and
> >> otherwise fully appropriate API calls like pci_request_regions() and
> >> mfd_add_devices().
> >
> > Yes, here is the issue. 8250 requires the parent not to *request*
> > resources. Because child handles IO access itself.
> 
> Ok, clearly we are not discussing the actual IO access.  The only issue
> is how to handle the memory resource management.
> 
> And yes, serial8250 requires "the parent" to not request the resources.
> But by doing so, it gets in the way of the mfd-core way of splitting a
> properly requested resource.

Which is right thing to do. Requesting resources should be done by their actual
user, no?

Moreover, if you look at /proc/iomem, for example, I bet there will be
a difference with your proposed method and a established one.

> >> The mfd driver in question is for a PCI device.  Not being able to
> >> request the PCI regions seems silly.
> >
> > Nope. Otherwise, the parent which *doesn't handle* IO on behalf of
> > child should not request its resources.
> 
> If I may, could I get you to discuss this with Lee Jones?
> 
> As I read both of your comments in this thread, you are not aligned on
> how mfd drivers should handle resources.  And in that case, one of you
> are most likely more right than the other, and if Lee is right, I seem
> to be unable to convince you about that.

MFD has nothing to do with *requesting* resource. It's about *slicing* them.
*Requesting* resource is orthogonal to the *slicing*.

> >> Not being able to register all child devices with the call introduced
> >> for that sole purpose also seems silly.
> >
> >> Please take a look at https://lkml.org/lkml/2019/4/9/576
> >> ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
> >
> > Thank you for this link.
> > Now, look at this comment:
> >
> > + /*
> > + * Map all IOC3 registers.  These are shared between subdevices
> > + * so the main IOC3 module manages them.
> > + */
> >
> > Is it your case? Can we see the code?
> 
> That comment seems quite misleading.  I am quote certain that the uart
> registers which are part of BAR 0 is not more shared between subdevices
> than the uart registers in BAR 0 in my case.
> 
> But BAR 0 as a whole is shared between subdevices.  But BAR 0 can be
> (is) split in parts that are exclusive to one subdevice.  The only
> difference I see is that I don't have any registers accessed directly by
> the mfd driver.

I stopped here. Let's discuss an actual code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > On Tue, May 07, 2019 at 01:35:58PM +0200, Esben Haabendal wrote:
>> >> Lee Jones <lee.jones@linaro.org> writes:
>> >> > On Thu, 02 May 2019, Esben Haabendal wrote:
>> >> >
>> >> >> Could you help clarify whether or not this patch is trying to do
>> >> >> something odd/wrong?
>> >> >> 
>> >> >> I might be misunderstanding Andy (probably is), but the discussion
>> >> >> revolves around the changes I propose where I change the serial8250
>> >> >> driver to use platform_get_resource() in favour of
>> >> >> request_mem_region()/release_mem_region().
>> >> >
>> >> > Since 'serial8250' is registered as a platform device, I don't see any
>> >> > reason why it shouldn't have the capability to obtain its memory
>> >> > regions from the platform_get_*() helpers.
>> >> 
>> >> Good to hear.  That is exactly what I am trying do with this patch.
>> >> 
>> >> @Andy: If you still don't like my approach, could you please advice an
>> >> acceptable method for improving the serial8250 driver to allow the use
>> >> of platform_get_*() helpers?
>> >
>> > I still don't get why you need this.
>> 
>> Because platform_get_resource() is a generally available and useful
>> helper function for working with platform_device resources, that the
>> current standard serial8250 driver does not support.
>> 
>> I am uncertain if I still haven't convinced you that current serial8250
>> driver does not work with platform_get_resource(), or if you believe
>> that it really should not support it.
>
> I believe there is no need to do this support.
>
> Most of the platform code that uses it is quite legacy, and all under arch/
> ideally should be converted to use Device Tree.

Please take a look at https://lkml.org/lkml/2019/4/9/576
("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")

This is basically what I am trying to do.  I am just so unfortunate that
the serial devices I have are completely generic, so it does not make
sense for me to create a specialized 8250 driver.

Look at how the serial8250_ioc3_driver uses platform_get_resource() to
get the register memory, and how that works together with
mfd_add_devices() in the mfd driver.  Nice and elegant.  Standard
recommended approach for an mfd driver.

/Esben

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Enrico Weigelt, metux IT consult]

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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

"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

Re: [PATCH] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

"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

[PATCH resend] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

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

When UPF_DEV_RESOURCES flag is set, the following platform_data fields should
not be used: mapbase, iobase, mapsize, and irq.  They are superseded by the
resources attached to the device.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/tty/serial/8250/8250_core.c | 56 +++++++++++++++++++++++++++++++++----
 drivers/tty/serial/8250/8250_port.c | 15 ++++++----
 include/linux/serial_core.h         |  1 +
 3 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index e441221..9df6a98 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -788,6 +788,48 @@ void serial8250_resume_port(int line)
 }
 EXPORT_SYMBOL(serial8250_resume_port);
 
+static int serial8250_probe_resources(struct platform_device *pdev,
+				      unsigned int num,
+				      struct plat_serial8250_port *p,
+				      struct uart_8250_port *uart)
+{
+	struct resource *r;
+	int irq;
+
+	switch (p->iotype) {
+	case UPIO_AU:
+	case UPIO_TSI:
+	case UPIO_MEM32:
+	case UPIO_MEM32BE:
+	case UPIO_MEM16:
+	case UPIO_MEM:
+		r = platform_get_resource(pdev, IORESOURCE_MEM, num);
+		if (!r)
+			return -ENODEV;
+		uart->port.mapbase = r->start;
+		uart->port.mapsize = resource_size(r);
+		uart->port.flags |= UPF_IOREMAP;
+		break;
+	case UPIO_HUB6:
+	case UPIO_PORT:
+		r = platform_get_resource(pdev, IORESOURCE_IO, num);
+		if (!r)
+			return -ENODEV;
+		uart->port.iobase = r->start;
+		uart->port.mapsize = resource_size(r);
+		break;
+	}
+
+	irq = platform_get_irq(pdev, num);
+	if (irq == -ENXIO)
+		uart->port.irq = 0; /* no interrupt -> use polling */
+	else if (irq < 0)
+		return irq;
+	uart->port.irq = irq;
+
+	return 0;
+}
+
 /*
  * Register a set of serial devices attached to a platform device.  The
  * list is terminated with a zero flags entry, which means we expect
@@ -805,15 +847,19 @@ static int serial8250_probe(struct platform_device *dev)
 		irqflag = IRQF_SHARED;
 
 	for (i = 0; p && p->flags != 0; p++, i++) {
-		uart.port.iobase	= p->iobase;
-		uart.port.membase	= p->membase;
-		uart.port.irq		= p->irq;
+		uart.port.flags		= p->flags;
+		if (p->flags & UPF_DEV_RESOURCES) {
+			serial8250_probe_resources(dev, i, p, &uart);
+		} else {
+			uart.port.iobase	= p->iobase;
+			uart.port.mapbase	= p->mapbase;
+			uart.port.membase	= p->membase;
+			uart.port.irq		= p->irq;
+		}
 		uart.port.irqflags	= p->irqflags;
 		uart.port.uartclk	= p->uartclk;
 		uart.port.regshift	= p->regshift;
 		uart.port.iotype	= p->iotype;
-		uart.port.flags		= p->flags;
-		uart.port.mapbase	= p->mapbase;
 		uart.port.hub6		= p->hub6;
 		uart.port.private_data	= p->private_data;
 		uart.port.type		= p->type;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index d2f3310..7fa1e49 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2863,7 +2863,8 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 		if (!port->mapbase)
 			break;
 
-		if (!request_mem_region(port->mapbase, size, "serial")) {
+		if (!(port->flags & UPF_DEV_RESOURCES) &&
+		    !request_mem_region(port->mapbase, size, "serial")) {
 			ret = -EBUSY;
 			break;
 		}
@@ -2871,7 +2872,8 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 		if (port->flags & UPF_IOREMAP) {
 			port->membase = ioremap_nocache(port->mapbase, size);
 			if (!port->membase) {
-				release_mem_region(port->mapbase, size);
+				if (!(port->flags & UPF_DEV_RESOURCES))
+					release_mem_region(port->mapbase, size);
 				ret = -ENOMEM;
 			}
 		}
@@ -2879,7 +2881,8 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 
 	case UPIO_HUB6:
 	case UPIO_PORT:
-		if (!request_region(port->iobase, size, "serial"))
+		if (!(port->flags & UPF_DEV_RESOURCES) &&
+		    !request_region(port->iobase, size, "serial"))
 			ret = -EBUSY;
 		break;
 	}
@@ -2906,12 +2909,14 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
 			port->membase = NULL;
 		}
 
-		release_mem_region(port->mapbase, size);
+		if (!(port->flags & UPF_DEV_RESOURCES))
+			release_mem_region(port->mapbase, size);
 		break;
 
 	case UPIO_HUB6:
 	case UPIO_PORT:
-		release_region(port->iobase, size);
+		if (!(port->flags & UPF_DEV_RESOURCES))
+			release_region(port->iobase, size);
 		break;
 	}
 }
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 5fe2b03..87b4ed3 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -207,6 +207,7 @@ struct uart_port {
 #define UPF_BUGGY_UART		((__force upf_t) ASYNC_BUGGY_UART     /* 14 */ )
 #define UPF_MAGIC_MULTIPLIER	((__force upf_t) ASYNC_MAGIC_MULTIPLIER /* 16 */ )
 
+#define UPF_DEV_RESOURCES	((__force upf_t) (1 << 18))
 #define UPF_NO_THRE_TEST	((__force upf_t) (1 << 19))
 /* Port has hardware-assisted h/w flow control */
 #define UPF_AUTO_CTS		((__force upf_t) (1 << 20))
-- 
2.4.11

Re: [PATCH resend] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

On Tue, May 21, 2019 at 01:34:26PM +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().
> 
> When UPF_DEV_RESOURCES flag is set, the following platform_data fields should
> not be used: mapbase, iobase, mapsize, and irq.  They are superseded by the
> resources attached to the device.
> 

Same comment here: Requesting resource is orthogonal to the retrieving or
slicing them.

> +		if (p->flags & UPF_DEV_RESOURCES) {
> +			serial8250_probe_resources(dev, i, p, &uart);

This can be easily detected by checking for the resources directly, like

	res = platform_get_resource(...);
	if (res)
		new_scheme();
	else
		old_scheme();

Otherwise looks good.


> -		if (!request_mem_region(port->mapbase, size, "serial")) {
> +		if (!(port->flags & UPF_DEV_RESOURCES) &&
> +		    !request_mem_region(port->mapbase, size, "serial")) {

> -				release_mem_region(port->mapbase, size);
> +				if (!(port->flags & UPF_DEV_RESOURCES))
> +					release_mem_region(port->mapbase, size);

> -		if (!request_region(port->iobase, size, "serial"))
> +		if (!(port->flags & UPF_DEV_RESOURCES) &&
> +		    !request_region(port->iobase, size, "serial"))

> -		release_mem_region(port->mapbase, size);
> +		if (!(port->flags & UPF_DEV_RESOURCES))
> +			release_mem_region(port->mapbase, size);

> -		release_region(port->iobase, size);
> +		if (!(port->flags & UPF_DEV_RESOURCES))
> +			release_region(port->iobase, size);

All these changes are not related to what you describe in the commit message.
is a workaround for the bug in the parent MFD driver of the 8250.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH resend] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Tue, May 21, 2019 at 01:34:26PM +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().
>> 
>> When UPF_DEV_RESOURCES flag is set, the following platform_data fields should
>> not be used: mapbase, iobase, mapsize, and irq.  They are superseded by the
>> resources attached to the device.
>> 
>
> Same comment here: Requesting resource is orthogonal to the retrieving or
> slicing them.

Yes.  But for MFD devices, I do think it makes sense for the MFD parent
device to request the entire memory resource, and then split it.

And for drivers that actually are aware of the struct resource given,
both approaches work.  Throwing away the resource.parent information
and calling out request_mem_region() manually breaks the idea of
managing IORESOURCE_MEM as a tree structure.

Are we not supposed to be using the parent/child part of struct
resource?

>> +		if (p->flags & UPF_DEV_RESOURCES) {
>> +			serial8250_probe_resources(dev, i, p, &uart);
>
> This can be easily detected by checking for the resources directly, like
>
> 	res = platform_get_resource(...);
> 	if (res)
> 		new_scheme();
> 	else
> 		old_scheme();
>
> Otherwise looks good.

Sounds fine with me.  I was afraid that it could cause problems with
existing drivers, where platform_get_resource() would work, but return
something else than desired.  That would probably have gone unnoticed by
now.  But can ofcourse be fixed if it occurs.


>> -		if (!request_mem_region(port->mapbase, size, "serial")) {
>> +		if (!(port->flags & UPF_DEV_RESOURCES) &&
>> +		    !request_mem_region(port->mapbase, size, "serial")) {
>
>> -				release_mem_region(port->mapbase, size);
>> +				if (!(port->flags & UPF_DEV_RESOURCES))
>> +					release_mem_region(port->mapbase, size);
>
>> -		if (!request_region(port->iobase, size, "serial"))
>> +		if (!(port->flags & UPF_DEV_RESOURCES) &&
>> +		    !request_region(port->iobase, size, "serial"))
>
>> -		release_mem_region(port->mapbase, size);
>> +		if (!(port->flags & UPF_DEV_RESOURCES))
>> +			release_mem_region(port->mapbase, size);
>
>> -		release_region(port->iobase, size);
>> +		if (!(port->flags & UPF_DEV_RESOURCES))
>> +			release_region(port->iobase, size);
>
> All these changes are not related to what you describe in the commit message.
> is a workaround for the bug in the parent MFD driver of the 8250.

You are right, this is not adequately described in commit message.
But unless we are not supposed to allow parent/child memory resource
management, I don't think it is a workaround, but a fix.

But I can split it out in a separate patch.  Would be nice if I at least
can get the other part of the change merged.

/Esben

Re: [PATCH resend] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

On Tue, May 21, 2019 at 04:43:18PM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> 
> > On Tue, May 21, 2019 at 01:34:26PM +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().
> >> 
> >> When UPF_DEV_RESOURCES flag is set, the following platform_data fields should
> >> not be used: mapbase, iobase, mapsize, and irq.  They are superseded by the
> >> resources attached to the device.
> >> 
> >
> > Same comment here: Requesting resource is orthogonal to the retrieving or
> > slicing them.
> 
> Yes.  But for MFD devices, I do think it makes sense for the MFD parent
> device to request the entire memory resource, and then split it.

Nope. This is layering violation here: The user of the resources is not
handling them in full.

> And for drivers that actually are aware of the struct resource given,
> both approaches work.  Throwing away the resource.parent information
> and calling out request_mem_region() manually breaks the idea of
> managing IORESOURCE_MEM as a tree structure.

How come? Can you show an example of output without and with your patches?

> Are we not supposed to be using the parent/child part of struct
> resource?

It's about slicing, no-one prevents you to do that. I don't see a problem.
Show the output!

> >> -		if (!request_mem_region(port->mapbase, size, "serial")) {
> >> +		if (!(port->flags & UPF_DEV_RESOURCES) &&
> >> +		    !request_mem_region(port->mapbase, size, "serial")) {
> >
> >> -				release_mem_region(port->mapbase, size);
> >> +				if (!(port->flags & UPF_DEV_RESOURCES))
> >> +					release_mem_region(port->mapbase, size);
> >
> >> -		if (!request_region(port->iobase, size, "serial"))
> >> +		if (!(port->flags & UPF_DEV_RESOURCES) &&
> >> +		    !request_region(port->iobase, size, "serial"))
> >
> >> -		release_mem_region(port->mapbase, size);
> >> +		if (!(port->flags & UPF_DEV_RESOURCES))
> >> +			release_mem_region(port->mapbase, size);
> >
> >> -		release_region(port->iobase, size);
> >> +		if (!(port->flags & UPF_DEV_RESOURCES))
> >> +			release_region(port->iobase, size);
> >
> > All these changes are not related to what you describe in the commit message.
> > is a workaround for the bug in the parent MFD driver of the 8250.
> 
> You are right, this is not adequately described in commit message.
> But unless we are not supposed to allow parent/child memory resource
> management, I don't think it is a workaround, but a fix.
> 
> But I can split it out in a separate patch.  Would be nice if I at least
> can get the other part of the change merged.

Like Lee said, and I agree, nothing prevents us to switch to
platform_get_resource().

The stumbling block here is the *requesting* in parent which I strongly
disagree with (at least in a form of this change, I already told you, that this
has to be "fixed" on generic level, not as a hack in one certain driver).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH resend] serial: 8250: Add support for using platform_device resources

[Greg Kroah-Hartman]

On Tue, May 21, 2019 at 01:34:26PM +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.

Nothing actually sets this flag in this patch, so I can't take this as
you are adding new features that no one uses :(

Where is the driver that sets this?

thanks,

greg k-h

Re: [PATCH resend] serial: 8250: Add support for using platform_device resources

[Esben Haabendal]

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

> On Tue, May 21, 2019 at 01:34:26PM +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.
>
> Nothing actually sets this flag in this patch, so I can't take this as
> you are adding new features that no one uses :(
>
> Where is the driver that sets this?

It sits here.  It is a rather big and clunky mfd driver, not ready for
upstreaming in its current form.  I hope to get around to clean it up.
But it is for a very specific hardware that is really available or
usable for anybody else.  Does it make sense to spend effort on
submitting such a driver?

/Esben

Re: [PATCH resend] serial: 8250: Add support for using platform_device resources

[Greg Kroah-Hartman]

On Tue, May 21, 2019 at 04:45:34PM +0200, Esben Haabendal wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Tue, May 21, 2019 at 01:34:26PM +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.
> >
> > Nothing actually sets this flag in this patch, so I can't take this as
> > you are adding new features that no one uses :(
> >
> > Where is the driver that sets this?
> 
> It sits here.

Where is "here"?

> It is a rather big and clunky mfd driver, not ready for
> upstreaming in its current form.  I hope to get around to clean it up.
> But it is for a very specific hardware that is really available or
> usable for anybody else.  Does it make sense to spend effort on
> submitting such a driver?

I can not take kernel apis/features being added for no in-kernel user,
that's just how Linux kernel development works.  I'll be glad to review
this if we have an actual user for this code, but it also needs to be
submitted at the same time.

That's how we have always worked, nothing new here :)

thanks,

greg k-h

Re: [PATCH resend] serial: 8250: Add support for using platform_device resources

[Enrico Weigelt, metux IT consult]

On 21.05.19 16:45, Esben Haabendal wrote:
> It sits here.  It is a rather big and clunky mfd driver, not ready for
> upstreaming in its current form.  I hope to get around to clean it up.
> But it is for a very specific hardware that is really available or
> usable for anybody else.  Does it make sense to spend effort on
> submitting such a driver?

Maybe you could post your queue w/ "RFC: " prefix and add a proper
introduction, so everybody knows that isn't meant to be upstreamed,
but instead a basis for discussions ?

Personally, I'd like to have a look at it and understand, what's the
actual problem you'd like to solve.


--mtx

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