Andy Shevchenko <andy.shevchenko@xxxxxxxxx> writes: > On Mon, Apr 29, 2019 at 9:27 AM Esben Haabendal <esben@xxxxxxxxxxxx> wrote: >> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> writes: >> > On Sat, Apr 27, 2019 at 12:01 PM Esben Haabendal <esben@xxxxxxxxxxxx> wrote: >> >> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> writes: >> >> > On Fri, Apr 26, 2019 at 06:54:05PM +0200, Esben Haabendal wrote: >> >> >> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> writes: >> >> >> The reason for this patch is to be able to do exactly that (set port >> >> >> type and UPF_FIXED_TYPE) without having UPF_BOOT_AUTOCONF added. >> >> >> >> >> >> In the current serial8250_register_8250_port() there is: >> >> >> >> >> >> uart->port.flags = up->port.flags | UPF_BOOT_AUTOCONF; >> >> >> >> >> >> So, even though I set UPF_FIXED_TYPE, I get >> >> >> UPF_FIXED_TYPE|UPF_BOOT_AUTOCONF. >> >> > >> >> > Yes. >> >> > >> >> >> So I need this patch. >> >> > >> >> > Why? I don't see any problems to have these flags set. >> >> >> >> The problem with having UPF_BOOT_AUTOCONF is the call to >> >> serial8250_request_std_resource(). It calls request_mem_region(), which >> >> fails if the MFD driver already have requested the memory region for the >> >> MFD device. >> > >> > If it's MFD, why it requested the region for its child? >> > Isn't it a bug in MFD driver? >> >> It is a PCI driver, which calls pci_request_regions(). The PCI device >> carries a lot of different functions, which uses small slices of the PCI >> memory region(s). With the resources being a tree structure, I don't >> think it is a bug when a parent driver requests the entire memory >> region. > > If it's MFD driver, it's not its business to do something > child-related on child behalf. The MFD driver is not doing anything on behalf of the child. Being the parent (MFD), it is requesting the memory region of the MFD/parent device, using pci_request_regions(), similar to how it is done in fx. janz-cmodio.c. > In any case, Linux device resource model uses exclusive region > slicing. If you do like above, you call for a problems. Linux device resource model supports a parent/child/sibling structure, specifically to allow for doing something like this. Requesting memory resources are not restricted to use the iomem_resource root. For example, drivers/pcmcia/soc_common.c:soc_pcmcia_add_one() ret = request_resource(&iomem_resource, &skt->res_skt); if (ret) goto out_err_1; ret = request_resource(&skt->res_skt, &skt->res_io); if (ret) goto out_err_2; As an example of a parent/child memory resource request. The problem is that the 8250 driver only allows for requesting memory region from the root (iomem_resource), as it uses a single resource_size_t mapbase value for specifying the memory resource instead of a full struct resource, which would allow passing in a parent. So maybe we should go down that direction intead, extending 8250 driver to replace mapbase with a resource struct instead? > Btw, we have PCI MFD driver which enumerates 8250 (more precisely > 8250_dw) w/o any issues. I am aware of that (sm501.c). It avoids the problem by not requesting the parent memory region (sm->io_res), and requesting all child memory regions directly in root instead of relative to the sm->io_res parent. But as resoure management is designed for managing a parent/child resource tree, this looks much more like a workaround than the right solution. >> It would be nice if child drivers requesting memory would pass the >> parent memory resource. Maybe 8250 driver could be changed to accept a >> struct resource pointer instead of a simple mapbase value, allowing to >> setup the resource with parent pointing to the MFD memory resource. > > I don't see the problem in certain driver, I guess you are trying to > workaround existin Linux device resource model. No, I actually try to do the right thing in relation to Linux device resource model. But 8250 is just not behaving very well in that respect, not having been made really aware of the resource model. And sm501.c MFD driver ignores the way resource model is designed, and just make things work. /Esben
