diff mbox series

drivers/of: Add devm_of_iomap()

Message ID 23be2945b593a36d8fa1970bb579389c0f892a3e.camel@kernel.crashing.org
State Changes Requested, archived
Headers show
Series drivers/of: Add devm_of_iomap() | expand

Commit Message

Benjamin Herrenschmidt June 12, 2018, 12:01 a.m. UTC 
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

Comments

Andy Shevchenko June 12, 2018, 8:35 a.m. UTC | #1 
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?
Benjamin Herrenschmidt June 12, 2018, 10:28 a.m. UTC | #2 
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
Andy Shevchenko June 12, 2018, 4:53 p.m. UTC | #3 
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).
Rob Herring (Arm) June 12, 2018, 5:42 p.m. UTC | #4 
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
Benjamin Herrenschmidt June 12, 2018, 10:53 p.m. UTC | #5 
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
Benjamin Herrenschmidt June 12, 2018, 10:58 p.m. UTC | #6 
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
Andy Shevchenko June 12, 2018, 11:16 p.m. UTC | #7 
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?
Benjamin Herrenschmidt June 13, 2018, 12:18 a.m. UTC | #8 
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
Andy Shevchenko June 13, 2018, 8:18 a.m. UTC | #9 
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.
Rob Herring (Arm) June 13, 2018, 3 p.m. UTC | #10 
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
Benjamin Herrenschmidt June 13, 2018, 11:07 p.m. UTC | #11 
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
Geert Uytterhoeven June 14, 2018, 8:27 a.m. UTC | #12 
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
kernel test robot June 14, 2018, 1:30 p.m. UTC | #13 
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
Benjamin Herrenschmidt June 14, 2018, 11:50 p.m. UTC | #14 
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
Geert Uytterhoeven June 15, 2018, 6:16 a.m. UTC | #15 
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
Benjamin Herrenschmidt June 15, 2018, 6:52 a.m. UTC | #16 
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 mbox series

Patch

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