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@xxxxxxxxxxxxxxx> writes:
>
> > On Wed, May 01, 2019 at 09:17:37AM +0200, Esben Haabendal wrote:
> >> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 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
