Message ID | 23be2945b593a36d8fa1970bb579389c0f892a3e.camel@kernel.crashing.org |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | drivers/of: Add devm_of_iomap() | expand |
On Tue, Jun 12, 2018 at 3:01 AM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > There are still quite a few cases where a device might want to get to a > different node of the device-tree, obtain the resources and map them. > > Drivers doing that currently open code the whole thing, which is error > proe. prone > > We have of_iomap() and of_io_request_and_map() but they both have shortcomings, > such as not returning the size of the resource found (which can be necessary) > and not being "managed". > > This adds a devm_of_iomap() that provides all of these and should probably > replace uses of the above in most drivers. It feels like a wrong approach. Can OF graph help here? Would it be better approach?
On Tue, 2018-06-12 at 11:35 +0300, Andy Shevchenko wrote: > On Tue, Jun 12, 2018 at 3:01 AM, Benjamin Herrenschmidt > <benh@kernel.crashing.org> wrote: > > There are still quite a few cases where a device might want to get to a > > different node of the device-tree, obtain the resources and map them. > > > > Drivers doing that currently open code the whole thing, which is error > > proe. > > prone > > > > > We have of_iomap() and of_io_request_and_map() but they both have shortcomings, > > such as not returning the size of the resource found (which can be necessary) > > and not being "managed". > > > > This adds a devm_of_iomap() that provides all of these and should probably > > replace uses of the above in most drivers. > > It feels like a wrong approach. > Can OF graph help here? Would it be better approach? I don't quite understand what your objection is nor what "OF graph" is... This is a direct replacement for the open coded equivalent that a number of drivers do, almost always without using devm_* or forgetting to request the resources etc... Ie, a less bug-prone tool in the toolbox. So there's a real use case here. In fact a driver I'm going to submit soon uses it, which is why I wrote it in the first place, rather than adding yet another open-coded case. And to reply to the inevitable next reaction, NO this is not a case for creating yet another 237 layers of abstractions. Sometimes, a driver needs to directly access (no regmap overhead please) some regions represented by a specific DT node (it could be a child of the device for example representing a portion of its register space, or it could be a separate piece of HW that needs to be used by the device but doesn't fit in any abstract model and shouldn't). Ben. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 12, 2018 at 1:28 PM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Tue, 2018-06-12 at 11:35 +0300, Andy Shevchenko wrote: >> On Tue, Jun 12, 2018 at 3:01 AM, Benjamin Herrenschmidt >> <benh@kernel.crashing.org> wrote: >> > There are still quite a few cases where a device might want to get to a >> > different node of the device-tree, obtain the resources and map them. >> > >> > Drivers doing that currently open code the whole thing, which is error >> > proe. >> >> prone >> >> > >> > We have of_iomap() and of_io_request_and_map() but they both have shortcomings, >> > such as not returning the size of the resource found (which can be necessary) >> > and not being "managed". >> > >> > This adds a devm_of_iomap() that provides all of these and should probably >> > replace uses of the above in most drivers. >> >> It feels like a wrong approach. >> Can OF graph help here? Would it be better approach? > > I don't quite understand what your objection is nor what "OF graph" > is... There is no objection per se, just a doubt that this is a right thing to do. I might be wrong, of course. OF graph nodes is a special API that allows you to access like you said "different node of device-tree". https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/graph.txt > This is a direct replacement for the open coded equivalent that a > number of drivers do, almost always without using devm_* or forgetting > to request the resources etc... Ie, a less bug-prone tool in the > toolbox. > > So there's a real use case here. I believe you, though as I mentioned that simplification doesn't feel right. Like jumping out of the frying pan into the fire. > In fact a driver I'm going to submit soon uses it, which is why I wrote > it in the first place, rather than adding yet another open-coded case. Good, but check with graphs first. If it's not suitable would be nice to know why. > And to reply to the inevitable next reaction, NO this is not a case for > creating yet another 237 layers of abstractions. Sometimes, a driver > needs to directly access (no regmap overhead please) some regions > represented by a specific DT node (it could be a child of the device > for example representing a portion of its register space, or it could > be a separate piece of HW that needs to be used by the device but > doesn't fit in any abstract model and shouldn't).
On Mon, Jun 11, 2018 at 6:01 PM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > There are still quite a few cases where a device might want to get to a > different node of the device-tree, obtain the resources and map them. > > Drivers doing that currently open code the whole thing, which is error > proe. > > We have of_iomap() and of_io_request_and_map() but they both have shortcomings, > such as not returning the size of the resource found (which can be necessary) > and not being "managed". > > This adds a devm_of_iomap() that provides all of these and should probably > replace uses of the above in most drivers. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > > I'm cooking a driver that uses this, if there's no objection I'd like > to carry it in my pull request for that driver (it can also exist in > the DT tree of course). Just let me know. We generally only use of_iomap when there is no struct device for any new driver. Why can't you use devm_ioremap_resource? Is this a non-platform bus device? > > drivers/of/address.c | 35 +++++++++++++++++++++++++++++++++++ > include/linux/of_address.h | 5 +++++ > 2 files changed, 40 insertions(+) > > diff --git a/drivers/of/address.c b/drivers/of/address.c > index cf83c05f5650..b7d49ee7b690 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -807,6 +807,41 @@ void __iomem *of_io_request_and_map(struct device_node *np, int index, > } > EXPORT_SYMBOL(of_io_request_and_map); > > +/* > + * devm_of_iomap - Requests a resource and maps the memory mapped IO > + * for a given device_node managed by a given device > + * > + * Checks that a resource is a valid memory region, requests the memory > + * region and ioremaps it. All operations are managed and will be undone > + * on driver detach of the device. > + * > + * This is to be used when a device requests/maps resources described > + * by other device tree nodes (children or otherwise). > + * > + * @dev: The device "managing" the resource > + * @node: The device-tree node where the resource resides > + * @index: index of the MMIO range in the "reg" property > + * @size: Returns the size of the resource (pass NULL if not needed) > + * Returns a pointer to the requested and mapped memory or an ERR_PTR() encoded > + * error code on failure. Usage example: > + * > + * base = devm_of_iomap(&pdev->dev, node, 0, NULL); > + * if (IS_ERR(base)) > + * return PTR_ERR(base); > + */ > +void __iomem *devm_of_iomap(struct device *dev, struct device_node *node, int index, > + resource_size_t *size) of/address.c generally doesn't depend on struct device. I'd like to keep it that way. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2018-06-12 at 11:42 -0600, Rob Herring wrote: > On Mon, Jun 11, 2018 at 6:01 PM, Benjamin Herrenschmidt > <benh@kernel.crashing.org> wrote: > > There are still quite a few cases where a device might want to get to a > > different node of the device-tree, obtain the resources and map them. > > > > Drivers doing that currently open code the whole thing, which is error > > proe. > > > > We have of_iomap() and of_io_request_and_map() but they both have shortcomings, > > such as not returning the size of the resource found (which can be necessary) > > and not being "managed". > > > > This adds a devm_of_iomap() that provides all of these and should probably > > replace uses of the above in most drivers. > > > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > --- > > > > I'm cooking a driver that uses this, if there's no objection I'd like > > to carry it in my pull request for that driver (it can also exist in > > the DT tree of course). Just let me know. > > We generally only use of_iomap when there is no struct device for any > new driver. Why can't you use devm_ioremap_resource? Is this a > non-platform bus device? This is just a wrapper on devm_ioremap_resource :-) Basically it's a "fixed" version of of_iomap, that has the devm* management and will mark the resource busy. My thinking was to then replace most of_iomap users with this. As for the specific case of the driver I'm cooking, it's a case where the SoC contains a little coprocessor (a ColdFire even !) alongside the main ARM core. I have a driver that offloads the bitbanging of some GPIOs to it (to implement the FSI bus). I use devm_of_iomap() to map the registers of the interrupt controller of the coprocessor, it's not really part of the interrupt tree, it doesn't distribute interrupts to the ARM or to Linux, it's just a device-node pointed to by a handle. BTW. Another thing that I find a bit annoying is "allocated" reserved- memory, there's no API to get to it other than via the DMA APIs or a CMA, which is overkill in a few circumstances (such as the one here where I just want to dedicate a bit of memory to the coprocessor). Right now I'm using a fixed reservation (with a reg property) and go to it "manually" but that somewhat sucks. Cheers, Ben. > > > > > drivers/of/address.c | 35 +++++++++++++++++++++++++++++++++++ > > include/linux/of_address.h | 5 +++++ > > 2 files changed, 40 insertions(+) > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > index cf83c05f5650..b7d49ee7b690 100644 > > --- a/drivers/of/address.c > > +++ b/drivers/of/address.c > > @@ -807,6 +807,41 @@ void __iomem *of_io_request_and_map(struct device_node *np, int index, > > } > > EXPORT_SYMBOL(of_io_request_and_map); > > > > +/* > > + * devm_of_iomap - Requests a resource and maps the memory mapped IO > > + * for a given device_node managed by a given device > > + * > > + * Checks that a resource is a valid memory region, requests the memory > > + * region and ioremaps it. All operations are managed and will be undone > > + * on driver detach of the device. > > + * > > + * This is to be used when a device requests/maps resources described > > + * by other device tree nodes (children or otherwise). > > + * > > + * @dev: The device "managing" the resource > > + * @node: The device-tree node where the resource resides > > + * @index: index of the MMIO range in the "reg" property > > + * @size: Returns the size of the resource (pass NULL if not needed) > > + * Returns a pointer to the requested and mapped memory or an ERR_PTR() encoded > > + * error code on failure. Usage example: > > + * > > + * base = devm_of_iomap(&pdev->dev, node, 0, NULL); > > + * if (IS_ERR(base)) > > + * return PTR_ERR(base); > > + */ > > +void __iomem *devm_of_iomap(struct device *dev, struct device_node *node, int index, > > + resource_size_t *size) > > of/address.c generally doesn't depend on struct device. I'd like to > keep it that way. > > Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2018-06-12 at 19:53 +0300, Andy Shevchenko wrote: > > > > It feels like a wrong approach. > > > Can OF graph help here? Would it be better approach? > > > > I don't quite understand what your objection is nor what "OF graph" > > is... > > There is no objection per se, just a doubt that this is a right thing to do. > I might be wrong, of course. > > OF graph nodes is a special API that allows you to access like you > said "different node of device-tree". > https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/graph.txt So I had a look and this is just an example on how to use phandles to link ports and endpoints... I fail to see how that relates to what this patch does. In the driver I'm doing for example, I do use a similar technique to "point" to the other node. In this case, this is a coprocessor in the SoC and I'm linking to the node that represent its interrupt controller (and its not a full fledged OS running there so we don't have a full interrupt tree for it). But once you have such a "graph", the question of mapping whatever memory resources (ie. "reg" properties) remains. Today, people will use of_address_to_resource() with ioremap, or of_iomap () to do that ... This patch just provides a devm_ variant of the latter, which also does a request_mem_resource() on it (which is missing from of_iomap), so generally is a better alternative. Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 13, 2018 at 1:58 AM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Tue, 2018-06-12 at 19:53 +0300, Andy Shevchenko wrote: >> >> > > It feels like a wrong approach. >> > > Can OF graph help here? Would it be better approach? >> > >> > I don't quite understand what your objection is nor what "OF graph" >> > is... >> >> There is no objection per se, just a doubt that this is a right thing to do. >> I might be wrong, of course. >> >> OF graph nodes is a special API that allows you to access like you >> said "different node of device-tree". >> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/graph.txt > > So I had a look and this is just an example on how to use phandles to > link ports and endpoints... I fail to see how that relates to what this > patch does. Because your patch does nothing except another layring of the existing APIs. > In the driver I'm doing for example, I do use a similar technique to > "point" to the other node. In this case, this is a coprocessor in the > SoC and I'm linking to the node that represent its interrupt controller > (and its not a full fledged OS running there so we don't have a full > interrupt tree for it). Hmm... So, you are trying to solve problem with other methods which might be not so suitable at all?
On Wed, 2018-06-13 at 02:16 +0300, Andy Shevchenko wrote: > On Wed, Jun 13, 2018 at 1:58 AM, Benjamin Herrenschmidt > <benh@kernel.crashing.org> wrote: > > On Tue, 2018-06-12 at 19:53 +0300, Andy Shevchenko wrote: > > > > > > > > It feels like a wrong approach. > > > > > Can OF graph help here? Would it be better approach? > > > > > > > > I don't quite understand what your objection is nor what "OF graph" > > > > is... > > > > > > There is no objection per se, just a doubt that this is a right thing to do. > > > I might be wrong, of course. > > > > > > OF graph nodes is a special API that allows you to access like you > > > said "different node of device-tree". > > > https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/graph.txt > > > > So I had a look and this is just an example on how to use phandles to > > link ports and endpoints... I fail to see how that relates to what this > > patch does. > > Because your patch does nothing except another layring of the existing APIs. I'm really having a hard time understanding what you are going on about.. Yes, it's a helper that combines two existing API functions, the goal being to generally replace the use of the existing of_iomap whenever possible. It makes sense and makes callers simpler and less bug prone. > > > In the driver I'm doing for example, I do use a similar technique to > > "point" to the other node. In this case, this is a coprocessor in the > > SoC and I'm linking to the node that represent its interrupt controller > > (and its not a full fledged OS running there so we don't have a full > > interrupt tree for it). > > Hmm... So, you are trying to solve problem with other methods which > might be not so suitable at all? Again, I cannot understand what you are going on about, what is "not suitable" to what purpose ? It's fairly common for nodes to point to each other. We've been doing that since the dawn of the device-tree. In this case, we have a coprocessor bound to a device and pointing to its interrupt controller, and we need to get to that and map it, I fail to see what the issue is and in what way this is "not suitable". But there are many other uses of things like of_iomap() which could benefit from switching to devm_of_iomap() and thus getting the automated cleanup on exit and appropriate request of the memory resource. (hint: I wrote of_iomap and a good bulk of what's in drivers/of/address.c... Ben. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 13, 2018 at 3:18 AM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > But there are many other uses of things like of_iomap() which could > benefit from switching to devm_of_iomap() and thus getting the > automated cleanup on exit and appropriate request of the memory > resource. Fine, fine.
On Tue, Jun 12, 2018 at 4:53 PM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Tue, 2018-06-12 at 11:42 -0600, Rob Herring wrote: >> On Mon, Jun 11, 2018 at 6:01 PM, Benjamin Herrenschmidt >> <benh@kernel.crashing.org> wrote: >> > There are still quite a few cases where a device might want to get to a >> > different node of the device-tree, obtain the resources and map them. >> > >> > Drivers doing that currently open code the whole thing, which is error >> > proe. >> > >> > We have of_iomap() and of_io_request_and_map() but they both have shortcomings, >> > such as not returning the size of the resource found (which can be necessary) >> > and not being "managed". >> > >> > This adds a devm_of_iomap() that provides all of these and should probably >> > replace uses of the above in most drivers. >> > >> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> > --- >> > >> > I'm cooking a driver that uses this, if there's no objection I'd like >> > to carry it in my pull request for that driver (it can also exist in >> > the DT tree of course). Just let me know. >> >> We generally only use of_iomap when there is no struct device for any >> new driver. Why can't you use devm_ioremap_resource? Is this a >> non-platform bus device? > > This is just a wrapper on devm_ioremap_resource :-) Basically it's a > "fixed" version of of_iomap, that has the devm* management and will > mark the resource busy. > > My thinking was to then replace most of_iomap users with this. > > As for the specific case of the driver I'm cooking, it's a case where > the SoC contains a little coprocessor (a ColdFire even !) alongside the Wow. Must be the 1 licensee. > main ARM core. I have a driver that offloads the bitbanging of some > GPIOs to it (to implement the FSI bus). I use devm_of_iomap() to map > the registers of the interrupt controller of the coprocessor, it's not > really part of the interrupt tree, it doesn't distribute interrupts to > the ARM or to Linux, it's just a device-node pointed to by a handle. Accessing another processor's interrupt controller. What could go wrong with that. I guess this is fine. There's another problem though. This doesn't work on Sparc because address.c is not built. I'd suggest moving to of/device.c or a new file. > BTW. Another thing that I find a bit annoying is "allocated" reserved- > memory, there's no API to get to it other than via the DMA APIs or a > CMA, which is overkill in a few circumstances (such as the one here > where I just want to dedicate a bit of memory to the coprocessor). > Right now I'm using a fixed reservation (with a reg property) and go to > it "manually" but that somewhat sucks. But that's not really a DT problem. It's a kernel problem if you can't reserve a contiguous range of unmapped pages. But why not just get coherent allocation and ignore that it is mapped. That seems better to me than working around it in DT. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2018-06-13 at 09:00 -0600, Rob Herring wrote: > > > My thinking was to then replace most of_iomap users with this. > > > > As for the specific case of the driver I'm cooking, it's a case where > > the SoC contains a little coprocessor (a ColdFire even !) alongside the > > Wow. Must be the 1 licensee. Haha probably :-) It was fun to play with for sure, lots of old memories of m68k asm coming back to the surface ;-) > > main ARM core. I have a driver that offloads the bitbanging of some > > GPIOs to it (to implement the FSI bus). I use devm_of_iomap() to map > > the registers of the interrupt controller of the coprocessor, it's not > > really part of the interrupt tree, it doesn't distribute interrupts to > > the ARM or to Linux, it's just a device-node pointed to by a handle. > > Accessing another processor's interrupt controller. What could go > wrong with that. We poke at one of the registers to trigger an IPI to the corproessor :- ) Sadly the HW doesn't cleanly separate the registers for the "consumer" side (the coprocessor) from those used to poke at it by from ARM into different banks (though at least the "clear IPI" is a separate register so the coprocessor and the ARM don't race, ie, it actually works fine :-). > I guess this is fine. There's another problem though. This doesn't > work on Sparc because address.c is not built. I'd suggest moving to > of/device.c or a new file. Ah sure. lib/devres.c maybe, where devm_ioremap is already. > > BTW. Another thing that I find a bit annoying is "allocated" reserved- > > memory, there's no API to get to it other than via the DMA APIs or a > > CMA, which is overkill in a few circumstances (such as the one here > > where I just want to dedicate a bit of memory to the coprocessor). > > Right now I'm using a fixed reservation (with a reg property) and go to > > it "manually" but that somewhat sucks. > > But that's not really a DT problem. Correct. At the moment I just use a fixed DT reserved entry and go directly for it's "reg" property (well, via of_address_to_resource) but that's not very nice. It would be better if I could have linux "allocate" the space for me but then just give me an API to find it without going via a CMA or some DMA ops. > It's a kernel problem if you can't > reserve a contiguous range of unmapped pages. But why not just get > coherent allocation and ignore that it is mapped. That seems better to > me than working around it in DT. The kernel won't get me a 1M (or 2M on the AST2400) aligned allocation successfully at runtime. So a reserve entry is the way to go here (though being mapped or not is not a big deal, I can just flush the cache after loading the ucode into it). Basically, I want one of the "allocated" reserved-memory entry but I don't want to bother with a CMA or a shared DMA pool for it, which are both completely overkill for what I need. (Also the code is hard to follow :-) Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ben, (the "m68k" later in the thread caught my attention ;-) On Tue, Jun 12, 2018 at 2:02 AM Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > There are still quite a few cases where a device might want to get to a > different node of the device-tree, obtain the resources and map them. > > Drivers doing that currently open code the whole thing, which is error > proe. > > We have of_iomap() and of_io_request_and_map() but they both have shortcomings, > such as not returning the size of the resource found (which can be necessary) > and not being "managed". > > This adds a devm_of_iomap() that provides all of these and should probably > replace uses of the above in most drivers. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> Thanks for your patch! > --- a/include/linux/of_address.h > +++ b/include/linux/of_address.h > @@ -40,6 +40,11 @@ extern void __iomem *of_iomap(struct device_node *device, int index); > void __iomem *of_io_request_and_map(struct device_node *device, > int index, const char *name); > > +/* Request and map, wrapper on devm_ioremap_resource */ > +extern void __iomem *devm_of_iomap(struct device *dev, > + struct device_node *node, int index, > + resource_size_t *size); > + > /* Extract an address from a device, returns the region size and > * the address space flags too. The PCI version uses a BAR number > * instead of an absolute index Do you need a dummy for !CONFIG_OF_ADDRESS, to aid compile-testing? Gr{oetje,eeting}s, Geert
Hi Benjamin,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.17 next-20180614]
[cannot apply to glikely/devicetree/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Benjamin-Herrenschmidt/drivers-of-Add-devm_of_iomap/20180612-080800
config: um-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=um
All errors (new ones prefixed by >>):
arch/um/drivers/vde.o: In function `vde_open_real':
(.text+0x951): warning: Using 'getgrnam' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
(.text+0x79c): warning: Using 'getpwuid' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
arch/um/drivers/vector_user.o: In function `user_init_socket_fds':
vector_user.c:(.text+0x334): warning: Using 'getaddrinfo' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
arch/um/drivers/pcap.o: In function `pcap_nametoaddr':
(.text+0xdee5): warning: Using 'gethostbyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
arch/um/drivers/pcap.o: In function `pcap_nametonetaddr':
(.text+0xdf85): warning: Using 'getnetbyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
arch/um/drivers/pcap.o: In function `pcap_nametoproto':
(.text+0xe1a5): warning: Using 'getprotobyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
arch/um/drivers/pcap.o: In function `pcap_nametoport':
(.text+0xdfd7): warning: Using 'getservbyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
drivers/of/address.o: In function `devm_of_iomap':
>> (.text+0x1882): undefined reference to `devm_ioremap_resource'
collect2: error: ld returned 1 exit status
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, 2018-06-14 at 10:27 +0200, Geert Uytterhoeven wrote: > > --- a/include/linux/of_address.h > > +++ b/include/linux/of_address.h > > @@ -40,6 +40,11 @@ extern void __iomem *of_iomap(struct device_node *device, int index); > > void __iomem *of_io_request_and_map(struct device_node *device, > > int index, const char *name); > > > > +/* Request and map, wrapper on devm_ioremap_resource */ > > +extern void __iomem *devm_of_iomap(struct device *dev, > > + struct device_node *node, int index, > > + resource_size_t *size); > > + > > /* Extract an address from a device, returns the region size and > > * the address space flags too. The PCI version uses a BAR number > > * instead of an absolute index > > Do you need a dummy for !CONFIG_OF_ADDRESS, to aid compile-testing? I didn't think so, as of_address_to_resource() already has a dummy, so it should build fine. Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ben, On Fri, Jun 15, 2018 at 1:51 AM Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Thu, 2018-06-14 at 10:27 +0200, Geert Uytterhoeven wrote: > > > --- a/include/linux/of_address.h > > > +++ b/include/linux/of_address.h > > > @@ -40,6 +40,11 @@ extern void __iomem *of_iomap(struct device_node *device, int index); > > > void __iomem *of_io_request_and_map(struct device_node *device, > > > int index, const char *name); > > > > > > +/* Request and map, wrapper on devm_ioremap_resource */ > > > +extern void __iomem *devm_of_iomap(struct device *dev, > > > + struct device_node *node, int index, > > > + resource_size_t *size); > > > + > > > /* Extract an address from a device, returns the region size and > > > * the address space flags too. The PCI version uses a BAR number > > > * instead of an absolute index > > > > Do you need a dummy for !CONFIG_OF_ADDRESS, to aid compile-testing? > > I didn't think so, as of_address_to_resource() already has a dummy, so > it should build fine. That dummy doesn't matter, as drivers/of/address won't be built if !CONFIG_OF_ADDRESS anyway. I mean for compile-testing users of devm_of_iomap(). And according to kbuild test robot <lkp@intel.com>: >> (.text+0x1882): undefined reference to `devm_ioremap_resource' devm_ioremap_resource() has no dummy for !HAS_IOMEM and OF_ADDRESS depends on !SPARC && (HAS_IOMEM || UML), bypassing the HAS_IOMEM check on UML. Gr{oetje,eeting}s, Geert
On Fri, 2018-06-15 at 08:16 +0200, Geert Uytterhoeven wrote: > Hi Ben, > > On Fri, Jun 15, 2018 at 1:51 AM Benjamin Herrenschmidt > <benh@kernel.crashing.org> wrote: > > On Thu, 2018-06-14 at 10:27 +0200, Geert Uytterhoeven wrote: > > > > --- a/include/linux/of_address.h > > > > +++ b/include/linux/of_address.h > > > > @@ -40,6 +40,11 @@ extern void __iomem *of_iomap(struct device_node *device, int index); > > > > void __iomem *of_io_request_and_map(struct device_node *device, > > > > int index, const char *name); > > > > > > > > +/* Request and map, wrapper on devm_ioremap_resource */ > > > > +extern void __iomem *devm_of_iomap(struct device *dev, > > > > + struct device_node *node, int index, > > > > + resource_size_t *size); > > > > + > > > > /* Extract an address from a device, returns the region size and > > > > * the address space flags too. The PCI version uses a BAR number > > > > * instead of an absolute index > > > > > > Do you need a dummy for !CONFIG_OF_ADDRESS, to aid compile-testing? > > > > I didn't think so, as of_address_to_resource() already has a dummy, so > > it should build fine. > > That dummy doesn't matter, as drivers/of/address won't be built if > !CONFIG_OF_ADDRESS anyway. Right but: - I moved it to lib/devres.c in my respun patch (because otherwise sparc won't get it) - The dummy is in include/linux/of_address.h, so devm_of_ioremap will compile fine either way. > I mean for compile-testing users of devm_of_iomap(). > > And according to kbuild test robot <lkp@intel.com>: > > > > (.text+0x1882): undefined reference to `devm_ioremap_resource' > > devm_ioremap_resource() has no dummy for !HAS_IOMEM and > OF_ADDRESS depends on !SPARC && (HAS_IOMEM || UML), > bypassing the HAS_IOMEM check on UML. > > Gr{oetje,eeting}s, > > Geert > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/of/address.c b/drivers/of/address.c index cf83c05f5650..b7d49ee7b690 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -807,6 +807,41 @@ void __iomem *of_io_request_and_map(struct device_node *np, int index, } EXPORT_SYMBOL(of_io_request_and_map); +/* + * devm_of_iomap - Requests a resource and maps the memory mapped IO + * for a given device_node managed by a given device + * + * Checks that a resource is a valid memory region, requests the memory + * region and ioremaps it. All operations are managed and will be undone + * on driver detach of the device. + * + * This is to be used when a device requests/maps resources described + * by other device tree nodes (children or otherwise). + * + * @dev: The device "managing" the resource + * @node: The device-tree node where the resource resides + * @index: index of the MMIO range in the "reg" property + * @size: Returns the size of the resource (pass NULL if not needed) + * Returns a pointer to the requested and mapped memory or an ERR_PTR() encoded + * error code on failure. Usage example: + * + * base = devm_of_iomap(&pdev->dev, node, 0, NULL); + * if (IS_ERR(base)) + * return PTR_ERR(base); + */ +void __iomem *devm_of_iomap(struct device *dev, struct device_node *node, int index, + resource_size_t *size) +{ + struct resource res; + + if (of_address_to_resource(node, index, &res)) + return IOMEM_ERR_PTR(-EINVAL); + if (size) + *size = resource_size(&res); + return devm_ioremap_resource(dev, &res); +} +EXPORT_SYMBOL(devm_of_iomap); + /** * of_dma_get_range - Get DMA range info * @np: device node to get DMA range info diff --git a/include/linux/of_address.h b/include/linux/of_address.h index 37864734ca50..2649232a2b26 100644 --- a/include/linux/of_address.h +++ b/include/linux/of_address.h @@ -40,6 +40,11 @@ extern void __iomem *of_iomap(struct device_node *device, int index); void __iomem *of_io_request_and_map(struct device_node *device, int index, const char *name); +/* Request and map, wrapper on devm_ioremap_resource */ +extern void __iomem *devm_of_iomap(struct device *dev, + struct device_node *node, int index, + resource_size_t *size); + /* Extract an address from a device, returns the region size and * the address space flags too. The PCI version uses a BAR number * instead of an absolute index
There are still quite a few cases where a device might want to get to a different node of the device-tree, obtain the resources and map them. Drivers doing that currently open code the whole thing, which is error proe. We have of_iomap() and of_io_request_and_map() but they both have shortcomings, such as not returning the size of the resource found (which can be necessary) and not being "managed". This adds a devm_of_iomap() that provides all of these and should probably replace uses of the above in most drivers. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- I'm cooking a driver that uses this, if there's no objection I'd like to carry it in my pull request for that driver (it can also exist in the DT tree of course). Just let me know. drivers/of/address.c | 35 +++++++++++++++++++++++++++++++++++ include/linux/of_address.h | 5 +++++ 2 files changed, 40 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html