diff mbox

RFC: add function for localbus address

Message ID 1409672700-21697-1-git-send-email-svarbanov@mm-sol.com (mailing list archive)
State New, archived
Headers show

Commit Message

Stanimir Varbanov Sept. 2, 2014, 3:45 p.m. UTC
Hi Grant,

I came down to this. Could you review? Is that
implementation closer to the suggestion made by you.

---
 drivers/of/address.c       |   49 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/of/platform.c      |   20 ++++++++++++++---
 include/linux/of_address.h |   19 +++++++++++++++++
 3 files changed, 84 insertions(+), 4 deletions(-)

Comments

Stephen Boyd Sept. 5, 2014, 11:29 p.m. UTC | #1
On 09/02/14 08:45, Stanimir Varbanov wrote:
> Hi Grant,
>
> I came down to this. Could you review? Is that
> implementation closer to the suggestion made by you.

I like this patch (but I'm biased because I want it to exist). Feel free
to add my Tested-by.


> ---
>  drivers/of/address.c       |   49 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/of/platform.c      |   20 ++++++++++++++---
>  include/linux/of_address.h |   19 +++++++++++++++++
>  3 files changed, 84 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index e371825..86c2166 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -601,6 +601,32 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
>  }
>  EXPORT_SYMBOL(of_get_address);
>  
> +const __be32 *of_get_localbus_address(struct device_node *np, int index,
> +				      u64 *size)
> +{
> +	struct device_node *root, *parent;
> +	const __be32 *ranges, *prop = NULL;
> +
> +	parent = of_get_parent(np);
> +	if (!parent)
> +		return NULL;
> +
> +	root = of_find_node_by_path("/");
> +
> +	if (parent == root) {
> +		of_node_put(parent);
> +		return NULL;
> +	}

I don't get this part though. Perhaps it needs a comment to say why we
don't allow the node to live in the root.

> +
> +	ranges = of_get_property(parent, "ranges", NULL);
> +	of_node_put(parent);
> +
> +	if (!ranges)
> +		prop = of_get_address(np, index, size, NULL);
> +
> +	return prop;
> +}
> +
>
grant.likely@linaro.org Sept. 8, 2014, 2:52 p.m. UTC | #2
On Tue,  2 Sep 2014 18:45:00 +0300, Stanimir Varbanov <svarbanov@mm-sol.com> wrote:
> Hi Grant,
> 
> I came down to this. Could you review? Is that
> implementation closer to the suggestion made by you.
> 
> ---
>  drivers/of/address.c       |   49 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/of/platform.c      |   20 ++++++++++++++---
>  include/linux/of_address.h |   19 +++++++++++++++++
>  3 files changed, 84 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index e371825..86c2166 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -601,6 +601,32 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
>  }
>  EXPORT_SYMBOL(of_get_address);
>  
> +const __be32 *of_get_localbus_address(struct device_node *np, int index,
> +				      u64 *size)
> +{
> +	struct device_node *root, *parent;
> +	const __be32 *ranges, *prop = NULL;
> +
> +	parent = of_get_parent(np);
> +	if (!parent)
> +		return NULL;
> +
> +	root = of_find_node_by_path("/");
> +
> +	if (parent == root) {
> +		of_node_put(parent);
> +		return NULL;
> +	}
> +
> +	ranges = of_get_property(parent, "ranges", NULL);
> +	of_node_put(parent);
> +
> +	if (!ranges)
> +		prop = of_get_address(np, index, size, NULL);
> +
> +	return prop;
> +}

So, the above doesn't make much sense to me. It looks like the function
merely decodes the local address, and the below function will stuff it
into a resource structure, but the tests for if the parent is root or
the parent has a ranges property are nonsensical. That shouldn't matter
for the functionality (except for automatically decoding them.. more
below)

> +
>  unsigned long __weak pci_address_to_pio(phys_addr_t address)
>  {
>  	if (address > IO_SPACE_LIMIT)
> @@ -665,6 +691,29 @@ int of_address_to_resource(struct device_node *dev, int index,
>  }
>  EXPORT_SYMBOL_GPL(of_address_to_resource);
>  
> +int of_localbus_address_to_resource(struct device_node *dev, int index,
> +				    struct resource *r)
> +{
> +	const char *name = NULL;
> +	const __be32 *addrp;
> +	u64 size;
> +
> +	addrp = of_get_localbus_address(dev, index, &size);
> +	if (!addrp)
> +		return -EINVAL;
> +
> +	of_property_read_string_index(dev, "reg-names", index, &name);
> +
> +	memset(r, 0, sizeof(*r));
> +	r->start = be32_to_cpup(addrp);
> +	r->end = r->start + size - 1;
> +	r->flags = IORESOURCE_REG;

This is problematic. A resource is created, but there is absolutely no
indication that the resource represents a localbus address instead of a
CPU address. platform_device reg resources represent CPU addresses.
Trying to overload it will cause confusion in drivers.

> +	r->name = name ? name : dev->full_name;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_localbus_address_to_resource);
> +
>  struct device_node *of_find_matching_node_by_address(struct device_node *from,
>  					const struct of_device_id *matches,
>  					u64 base_address)
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 0197725..36dcbd7 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -106,8 +106,9 @@ struct platform_device *of_device_alloc(struct device_node *np,
>  				  struct device *parent)
>  {
>  	struct platform_device *dev;
> -	int rc, i, num_reg = 0, num_irq;
> +	int rc, i, num_reg = 0, num_localbus_reg = 0, num_irq;
>  	struct resource *res, temp_res;
> +	int num_resources;
>  
>  	dev = platform_device_alloc("", -1);
>  	if (!dev)
> @@ -116,22 +117,33 @@ struct platform_device *of_device_alloc(struct device_node *np,
>  	/* count the io and irq resources */
>  	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
>  		num_reg++;
> +
> +	while (of_localbus_address_to_resource(np,
> +					num_localbus_reg, &temp_res) == 0)
> +		num_localbus_reg++;
> +

No, I don't support doing this. The moment a platform_driver depends on
a local bus address it is doing something special. It needs to decode
its own address in that case, which it can easily do.

Any platform_driver that interprets a IORESOURCE_REG as a localbus
address instead of a CPU address is *BROKEN*. It should be changed to
either decode the address itself, of a new bus type should be created
that can make its own decisions about what address resources mean.

I realize that you want to reuse the platform populate functionality in
this case. We can refactor some of that code to make it usable for
customized platform_bus_type instances.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Sept. 8, 2014, 8:22 p.m. UTC | #3
Adding Mark Brown who finished off introducing IORESOURCE_REG.

On 09/08/14 07:52, Grant Likely wrote:
> On Tue,  2 Sep 2014 18:45:00 +0300, Stanimir Varbanov <svarbanov@mm-sol.com> wrote:
>> +
>>  unsigned long __weak pci_address_to_pio(phys_addr_t address)
>>  {
>>  	if (address > IO_SPACE_LIMIT)
>> @@ -665,6 +691,29 @@ int of_address_to_resource(struct device_node *dev, int index,
>>  }
>>  EXPORT_SYMBOL_GPL(of_address_to_resource);
>>  
>> +int of_localbus_address_to_resource(struct device_node *dev, int index,
>> +				    struct resource *r)
>> +{
>> +	const char *name = NULL;
>> +	const __be32 *addrp;
>> +	u64 size;
>> +
>> +	addrp = of_get_localbus_address(dev, index, &size);
>> +	if (!addrp)
>> +		return -EINVAL;
>> +
>> +	of_property_read_string_index(dev, "reg-names", index, &name);
>> +
>> +	memset(r, 0, sizeof(*r));
>> +	r->start = be32_to_cpup(addrp);
>> +	r->end = r->start + size - 1;
>> +	r->flags = IORESOURCE_REG;
> This is problematic. A resource is created, but there is absolutely no
> indication that the resource represents a localbus address instead of a
> CPU address. platform_device reg resources represent CPU addresses.
> Trying to overload it will cause confusion in drivers.
>
>> +	r->name = name ? name : dev->full_name;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_localbus_address_to_resource);
>> +
>>  struct device_node *of_find_matching_node_by_address(struct device_node *from,
>>  					const struct of_device_id *matches,
>>  					u64 base_address)
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 0197725..36dcbd7 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -106,8 +106,9 @@ struct platform_device *of_device_alloc(struct device_node *np,
>>  				  struct device *parent)
>>  {
>>  	struct platform_device *dev;
>> -	int rc, i, num_reg = 0, num_irq;
>> +	int rc, i, num_reg = 0, num_localbus_reg = 0, num_irq;
>>  	struct resource *res, temp_res;
>> +	int num_resources;
>>  
>>  	dev = platform_device_alloc("", -1);
>>  	if (!dev)
>> @@ -116,22 +117,33 @@ struct platform_device *of_device_alloc(struct device_node *np,
>>  	/* count the io and irq resources */
>>  	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
>>  		num_reg++;
>> +
>> +	while (of_localbus_address_to_resource(np,
>> +					num_localbus_reg, &temp_res) == 0)
>> +		num_localbus_reg++;
>> +
> No, I don't support doing this. The moment a platform_driver depends on
> a local bus address it is doing something special. It needs to decode
> its own address in that case, which it can easily do.
>
> Any platform_driver that interprets a IORESOURCE_REG as a localbus
> address instead of a CPU address is *BROKEN*. It should be changed to
> either decode the address itself, of a new bus type should be created
> that can make its own decisions about what address resources mean.

Where is this described? From the commit text that introduces
IORESOURCE_REG I see:

"Currently a bunch of I2C/SPI MFD drivers are using IORESOURCE_IO for
register address ranges. Since this causes some confusion due to the
primary use of this resource type for PCI/ISA I/O ports create a new
resource type IORESOURCE_REG."

And the comment next to the #define says "Register offsets". I don't see
anywhere where it mentions these are CPU addresses. Certainly the
current IORESOURCE_REG users aren't CPU addresses because they're
SPI/I2C device drivers.
Mark Brown Sept. 8, 2014, 9:21 p.m. UTC | #4
On Mon, Sep 08, 2014 at 01:22:44PM -0700, Stephen Boyd wrote:

> Where is this described? From the commit text that introduces
> IORESOURCE_REG I see:

> "Currently a bunch of I2C/SPI MFD drivers are using IORESOURCE_IO for
> register address ranges. Since this causes some confusion due to the
> primary use of this resource type for PCI/ISA I/O ports create a new
> resource type IORESOURCE_REG."

> And the comment next to the #define says "Register offsets". I don't see
> anywhere where it mentions these are CPU addresses. Certainly the
> current IORESOURCE_REG users aren't CPU addresses because they're
> SPI/I2C device drivers.

Right, the intention was specifically to handle things where
IORESOURCE_MEM wasn't appropriate as it wasn't ever going to be memory
mapped I/O and should be in a separate address space.
Stanimir Varbanov Sept. 9, 2014, 3:07 p.m. UTC | #5
Hi Grant,

Thanks for the comments!

On 09/08/2014 05:52 PM, Grant Likely wrote:
> On Tue,  2 Sep 2014 18:45:00 +0300, Stanimir Varbanov <svarbanov@mm-sol.com> wrote:
>> Hi Grant,
>>
>> I came down to this. Could you review? Is that
>> implementation closer to the suggestion made by you.
>>
>> ---
>>  drivers/of/address.c       |   49 ++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/of/platform.c      |   20 ++++++++++++++---
>>  include/linux/of_address.h |   19 +++++++++++++++++
>>  3 files changed, 84 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>> index e371825..86c2166 100644
>> --- a/drivers/of/address.c
>> +++ b/drivers/of/address.c
>> @@ -601,6 +601,32 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
>>  }
>>  EXPORT_SYMBOL(of_get_address);
>>  
>> +const __be32 *of_get_localbus_address(struct device_node *np, int index,
>> +				      u64 *size)
>> +{
>> +	struct device_node *root, *parent;
>> +	const __be32 *ranges, *prop = NULL;
>> +
>> +	parent = of_get_parent(np);
>> +	if (!parent)
>> +		return NULL;
>> +
>> +	root = of_find_node_by_path("/");
>> +
>> +	if (parent == root) {
>> +		of_node_put(parent);
>> +		return NULL;
>> +	}
>> +
>> +	ranges = of_get_property(parent, "ranges", NULL);
>> +	of_node_put(parent);
>> +
>> +	if (!ranges)
>> +		prop = of_get_address(np, index, size, NULL);
>> +
>> +	return prop;
>> +}
> 
> So, the above doesn't make much sense to me. It looks like the function
> merely decodes the local address, and the below function will stuff it
> into a resource structure, but the tests for if the parent is root or
> the parent has a ranges property are nonsensical. That shouldn't matter
> for the functionality (except for automatically decoding them.. more
> below)
> 
>> +
>>  unsigned long __weak pci_address_to_pio(phys_addr_t address)
>>  {
>>  	if (address > IO_SPACE_LIMIT)
>> @@ -665,6 +691,29 @@ int of_address_to_resource(struct device_node *dev, int index,
>>  }
>>  EXPORT_SYMBOL_GPL(of_address_to_resource);
>>  
>> +int of_localbus_address_to_resource(struct device_node *dev, int index,
>> +				    struct resource *r)
>> +{
>> +	const char *name = NULL;
>> +	const __be32 *addrp;
>> +	u64 size;
>> +
>> +	addrp = of_get_localbus_address(dev, index, &size);
>> +	if (!addrp)
>> +		return -EINVAL;
>> +
>> +	of_property_read_string_index(dev, "reg-names", index, &name);
>> +
>> +	memset(r, 0, sizeof(*r));
>> +	r->start = be32_to_cpup(addrp);
>> +	r->end = r->start + size - 1;
>> +	r->flags = IORESOURCE_REG;
> 
> This is problematic. A resource is created, but there is absolutely no
> indication that the resource represents a localbus address instead of a
> CPU address. platform_device reg resources represent CPU addresses.
> Trying to overload it will cause confusion in drivers.
> 
>> +	r->name = name ? name : dev->full_name;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_localbus_address_to_resource);
>> +
>>  struct device_node *of_find_matching_node_by_address(struct device_node *from,
>>  					const struct of_device_id *matches,
>>  					u64 base_address)
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 0197725..36dcbd7 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -106,8 +106,9 @@ struct platform_device *of_device_alloc(struct device_node *np,
>>  				  struct device *parent)
>>  {
>>  	struct platform_device *dev;
>> -	int rc, i, num_reg = 0, num_irq;
>> +	int rc, i, num_reg = 0, num_localbus_reg = 0, num_irq;
>>  	struct resource *res, temp_res;
>> +	int num_resources;
>>  
>>  	dev = platform_device_alloc("", -1);
>>  	if (!dev)
>> @@ -116,22 +117,33 @@ struct platform_device *of_device_alloc(struct device_node *np,
>>  	/* count the io and irq resources */
>>  	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
>>  		num_reg++;
>> +
>> +	while (of_localbus_address_to_resource(np,
>> +					num_localbus_reg, &temp_res) == 0)
>> +		num_localbus_reg++;
>> +
> 
> No, I don't support doing this. The moment a platform_driver depends on
> a local bus address it is doing something special. It needs to decode
> its own address in that case, which it can easily do.
> 
> Any platform_driver that interprets a IORESOURCE_REG as a localbus
> address instead of a CPU address is *BROKEN*. It should be changed to
> either decode the address itself, of a new bus type should be created
> that can make its own decisions about what address resources mean.

Do you mean new of_bus entry?

> 
> I realize that you want to reuse the platform populate functionality in
> this case. We can refactor some of that code to make it usable for
> customized platform_bus_type instances.

What kind of refactoring? And what you mean by "customized
platform_bus_type"? Please elaborate more.

In fact one of my first attempts to upstream Qualcomm PMIC driver here
[1] I'm using of_get_address() over each child node to get the register
addresses and constructing resources for them manually. After that those
resources are passed to mfd_add_devices().

This implementation does not get accepted from Lee Jones with teh
argument that it re-implement of_platform_populate and he will need an
Ack from OF maintainers.

Do you mean with the statement above "It needs to decode its own address
in that case, which it can easily do" something like what I've done in [1]?

[1] http://www.kernelhub.org/?msg=520233&p=2
grant.likely@linaro.org Sept. 14, 2014, 4:46 a.m. UTC | #6
On Mon, 08 Sep 2014 13:22:44 -0700, Stephen Boyd <sboyd@codeaurora.org> wrote:
> Adding Mark Brown who finished off introducing IORESOURCE_REG.
> 
> On 09/08/14 07:52, Grant Likely wrote:
> > On Tue,  2 Sep 2014 18:45:00 +0300, Stanimir Varbanov <svarbanov@mm-sol.com> wrote:
> >> +
> >>  unsigned long __weak pci_address_to_pio(phys_addr_t address)
> >>  {
> >>  	if (address > IO_SPACE_LIMIT)
> >> @@ -665,6 +691,29 @@ int of_address_to_resource(struct device_node *dev, int index,
> >>  }
> >>  EXPORT_SYMBOL_GPL(of_address_to_resource);
> >>  
> >> +int of_localbus_address_to_resource(struct device_node *dev, int index,
> >> +				    struct resource *r)
> >> +{
> >> +	const char *name = NULL;
> >> +	const __be32 *addrp;
> >> +	u64 size;
> >> +
> >> +	addrp = of_get_localbus_address(dev, index, &size);
> >> +	if (!addrp)
> >> +		return -EINVAL;
> >> +
> >> +	of_property_read_string_index(dev, "reg-names", index, &name);
> >> +
> >> +	memset(r, 0, sizeof(*r));
> >> +	r->start = be32_to_cpup(addrp);
> >> +	r->end = r->start + size - 1;
> >> +	r->flags = IORESOURCE_REG;
> > This is problematic. A resource is created, but there is absolutely no
> > indication that the resource represents a localbus address instead of a
> > CPU address. platform_device reg resources represent CPU addresses.
> > Trying to overload it will cause confusion in drivers.
> >
> >> +	r->name = name ? name : dev->full_name;
> >> +
> >> +	return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(of_localbus_address_to_resource);
> >> +
> >>  struct device_node *of_find_matching_node_by_address(struct device_node *from,
> >>  					const struct of_device_id *matches,
> >>  					u64 base_address)
> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> >> index 0197725..36dcbd7 100644
> >> --- a/drivers/of/platform.c
> >> +++ b/drivers/of/platform.c
> >> @@ -106,8 +106,9 @@ struct platform_device *of_device_alloc(struct device_node *np,
> >>  				  struct device *parent)
> >>  {
> >>  	struct platform_device *dev;
> >> -	int rc, i, num_reg = 0, num_irq;
> >> +	int rc, i, num_reg = 0, num_localbus_reg = 0, num_irq;
> >>  	struct resource *res, temp_res;
> >> +	int num_resources;
> >>  
> >>  	dev = platform_device_alloc("", -1);
> >>  	if (!dev)
> >> @@ -116,22 +117,33 @@ struct platform_device *of_device_alloc(struct device_node *np,
> >>  	/* count the io and irq resources */
> >>  	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> >>  		num_reg++;
> >> +
> >> +	while (of_localbus_address_to_resource(np,
> >> +					num_localbus_reg, &temp_res) == 0)
> >> +		num_localbus_reg++;
> >> +
> > No, I don't support doing this. The moment a platform_driver depends on
> > a local bus address it is doing something special. It needs to decode
> > its own address in that case, which it can easily do.
> >
> > Any platform_driver that interprets a IORESOURCE_REG as a localbus
> > address instead of a CPU address is *BROKEN*. It should be changed to
> > either decode the address itself, of a new bus type should be created
> > that can make its own decisions about what address resources mean.
> 
> Where is this described? From the commit text that introduces
> IORESOURCE_REG I see:
> 
> "Currently a bunch of I2C/SPI MFD drivers are using IORESOURCE_IO for
> register address ranges. Since this causes some confusion due to the
> primary use of this resource type for PCI/ISA I/O ports create a new
> resource type IORESOURCE_REG."

Sorry, I mistook IORESOURCE_REG or IORESOURCE_IO. You're right, this
isn't an issue.

I'm still concerned about the implications of automatically populating
platform_devices with this resource type. I'll talk to Mark about it
face to fact at Connect this week.

g.

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Oct. 22, 2014, 11:01 p.m. UTC | #7
On 09/13/2014 09:46 PM, Grant Likely wrote:
> On Mon, 08 Sep 2014 13:22:44 -0700, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>
>> Where is this described? From the commit text that introduces
>> IORESOURCE_REG I see:
>>
>> "Currently a bunch of I2C/SPI MFD drivers are using IORESOURCE_IO for
>> register address ranges. Since this causes some confusion due to the
>> primary use of this resource type for PCI/ISA I/O ports create a new
>> resource type IORESOURCE_REG."
> Sorry, I mistook IORESOURCE_REG or IORESOURCE_IO. You're right, this
> isn't an issue.
>
> I'm still concerned about the implications of automatically populating
> platform_devices with this resource type. I'll talk to Mark about it
> face to fact at Connect this week.
>
>

Where did this end up? When we talked at Connect I think we settled on
exploring a driver core specific API like dev_get_localbus_address()
that calls of_get_localbus_address() for devices with an of_node and in
the future it could call something like acpi_get_localbus_address() when
there's an acpi_node. I believe the biggest concern is that we're making
an API that is OF or platform bus specific when it doesn't need to be.
Making a driver core specific API avoids this problem by making it bus
agnostic.

That's fine, but I still think we want to have the IORESOURCE_REG
resources given that we have drivers in-flight and some already merged
that are using the IORESOURCE_REG resource. Furthermore, ACPI is using
the platform bus for MFDs so it's not like we're going to be using a
different bus in the future for these pmic sub-device drivers if we
decide to do pmic devices in ACPI (looks like there is at least
precedence for that with Intel's i2c pmic). Supporting this on ACPI is
going to take the same effort if we plumb it into the resource table or
we plumb it into a new driver core API, so the bus agnostic angle isn't
looking all that convincing. Not to say I'm opposed to some driver core
specific API (that's similar to the proposed device_property_read_*()
API) but I don't see any benefit for something that is already "unified"
between ACPI and OF via the platform bus resources table. If the
resources table didn't already exist I'd be more inclined to say we
should use some new driver core API.

So what's the way forward? I see a few options:

1) Take this patch after some minor tweaks
2) Add a driver core API and fixup all drivers to use it
3) Layer the platform resource stuff on top of a driver core API

I'd prefer we went with option #1.
Russell King - ARM Linux Oct. 22, 2014, 11:20 p.m. UTC | #8
On Wed, Oct 22, 2014 at 04:01:26PM -0700, Stephen Boyd wrote:
> Where did this end up? When we talked at Connect I think we settled on
> exploring a driver core specific API like dev_get_localbus_address()
> that calls of_get_localbus_address() for devices with an of_node and in
> the future it could call something like acpi_get_localbus_address() when
> there's an acpi_node. I believe the biggest concern is that we're making
> an API that is OF or platform bus specific when it doesn't need to be.
> Making a driver core specific API avoids this problem by making it bus
> agnostic.

Given how little information there is in the original patch as to exactly
what problem this is addressing, I could be getting the wrong end of the
stick here.

Is this about trying to have a way to obtain the bus local addresses
associated with CPU-view resources?

If so, how about looking towards PCI, which has had this problem for the
last 15+ years, where PCI bus addresses are not necessarily the same as
CPU physical addresses?

There, we don't end up with multiple addresses specified in resources.
We instead have a way to translate between resources and bus-local
addresses, which IMHO is far nicer and less error-prone than having to
specify the same information twice, once with an offset and once without.
Mark Brown Oct. 22, 2014, 11:51 p.m. UTC | #9
On Wed, Oct 22, 2014 at 04:01:26PM -0700, Stephen Boyd wrote:

> >> "Currently a bunch of I2C/SPI MFD drivers are using IORESOURCE_IO for
> >> register address ranges. Since this causes some confusion due to the
> >> primary use of this resource type for PCI/ISA I/O ports create a new
> >> resource type IORESOURCE_REG."

> > Sorry, I mistook IORESOURCE_REG or IORESOURCE_IO. You're right, this
> > isn't an issue.

> > I'm still concerned about the implications of automatically populating
> > platform_devices with this resource type. I'll talk to Mark about it
> > face to fact at Connect this week.

Hrm, missed that discussion.

> Where did this end up? When we talked at Connect I think we settled on
> exploring a driver core specific API like dev_get_localbus_address()
> that calls of_get_localbus_address() for devices with an of_node and in

...

> That's fine, but I still think we want to have the IORESOURCE_REG
> resources given that we have drivers in-flight and some already merged
> that are using the IORESOURCE_REG resource. Furthermore, ACPI is using

Right, IORESOURCE_REG was the original solution to the overlapping use
of IORESOURCE_IO (rather than having multiple IORESOURCE_IO trees since
we do special magic for IORESOURCE_IO for legacy reasons which was
causing issues but the idea of doing it for generic I/O make sense).

> the platform bus for MFDs so it's not like we're going to be using a
> different bus in the future for these pmic sub-device drivers if we
> decide to do pmic devices in ACPI (looks like there is at least
> precedence for that with Intel's i2c pmic). Supporting this on ACPI is
> going to take the same effort if we plumb it into the resource table or
> we plumb it into a new driver core API, so the bus agnostic angle isn't

Even if we do these things in ACPI it's not at all clear to me that the
idea of putting the internals of the device in ACPI would be at all
tasteful there.  Personally I'm not a big fan of always doing it in DT
either but it's more tasteful there and definitely does make sense for
some things.
Stephen Boyd Oct. 22, 2014, 11:53 p.m. UTC | #10
On 10/22/2014 04:20 PM, Russell King - ARM Linux wrote:
> On Wed, Oct 22, 2014 at 04:01:26PM -0700, Stephen Boyd wrote:
>> Where did this end up? When we talked at Connect I think we settled on
>> exploring a driver core specific API like dev_get_localbus_address()
>> that calls of_get_localbus_address() for devices with an of_node and in
>> the future it could call something like acpi_get_localbus_address() when
>> there's an acpi_node. I believe the biggest concern is that we're making
>> an API that is OF or platform bus specific when it doesn't need to be.
>> Making a driver core specific API avoids this problem by making it bus
>> agnostic.
> Given how little information there is in the original patch as to exactly
> what problem this is addressing, I could be getting the wrong end of the
> stick here.
>
> Is this about trying to have a way to obtain the bus local addresses
> associated with CPU-view resources?
>
> If so, how about looking towards PCI, which has had this problem for the
> last 15+ years, where PCI bus addresses are not necessarily the same as
> CPU physical addresses?
>
> There, we don't end up with multiple addresses specified in resources.
> We instead have a way to translate between resources and bus-local
> addresses, which IMHO is far nicer and less error-prone than having to
> specify the same information twice, once with an offset and once without.
>

Not really. This is about giving the address of a sub device on a pmic
to a platform driver for that sub device. There is no CPU view. The
addresses are offsets in a register space for a PMIC or other MFD that
lives on i2c/spi or some similar sort of bus. So perhaps 0x20
corresponds to the start of the register space for an RTC and 0x38
corresponds to the start of the register space for a regulator.
diff mbox

Patch

diff --git a/drivers/of/address.c b/drivers/of/address.c
index e371825..86c2166 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -601,6 +601,32 @@  const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
 }
 EXPORT_SYMBOL(of_get_address);
 
+const __be32 *of_get_localbus_address(struct device_node *np, int index,
+				      u64 *size)
+{
+	struct device_node *root, *parent;
+	const __be32 *ranges, *prop = NULL;
+
+	parent = of_get_parent(np);
+	if (!parent)
+		return NULL;
+
+	root = of_find_node_by_path("/");
+
+	if (parent == root) {
+		of_node_put(parent);
+		return NULL;
+	}
+
+	ranges = of_get_property(parent, "ranges", NULL);
+	of_node_put(parent);
+
+	if (!ranges)
+		prop = of_get_address(np, index, size, NULL);
+
+	return prop;
+}
+
 unsigned long __weak pci_address_to_pio(phys_addr_t address)
 {
 	if (address > IO_SPACE_LIMIT)
@@ -665,6 +691,29 @@  int of_address_to_resource(struct device_node *dev, int index,
 }
 EXPORT_SYMBOL_GPL(of_address_to_resource);
 
+int of_localbus_address_to_resource(struct device_node *dev, int index,
+				    struct resource *r)
+{
+	const char *name = NULL;
+	const __be32 *addrp;
+	u64 size;
+
+	addrp = of_get_localbus_address(dev, index, &size);
+	if (!addrp)
+		return -EINVAL;
+
+	of_property_read_string_index(dev, "reg-names", index, &name);
+
+	memset(r, 0, sizeof(*r));
+	r->start = be32_to_cpup(addrp);
+	r->end = r->start + size - 1;
+	r->flags = IORESOURCE_REG;
+	r->name = name ? name : dev->full_name;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_localbus_address_to_resource);
+
 struct device_node *of_find_matching_node_by_address(struct device_node *from,
 					const struct of_device_id *matches,
 					u64 base_address)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 0197725..36dcbd7 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -106,8 +106,9 @@  struct platform_device *of_device_alloc(struct device_node *np,
 				  struct device *parent)
 {
 	struct platform_device *dev;
-	int rc, i, num_reg = 0, num_irq;
+	int rc, i, num_reg = 0, num_localbus_reg = 0, num_irq;
 	struct resource *res, temp_res;
+	int num_resources;
 
 	dev = platform_device_alloc("", -1);
 	if (!dev)
@@ -116,22 +117,33 @@  struct platform_device *of_device_alloc(struct device_node *np,
 	/* count the io and irq resources */
 	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
 		num_reg++;
+
+	while (of_localbus_address_to_resource(np,
+					num_localbus_reg, &temp_res) == 0)
+		num_localbus_reg++;
+
 	num_irq = of_irq_count(np);
 
+	num_resources = num_reg + num_localbus_reg + num_irq;
+
 	/* Populate the resource table */
-	if (num_irq || num_reg) {
-		res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
+	if (num_resources) {
+		res = kzalloc(sizeof(*res) * num_resources, GFP_KERNEL);
 		if (!res) {
 			platform_device_put(dev);
 			return NULL;
 		}
 
-		dev->num_resources = num_reg + num_irq;
+		dev->num_resources = num_resources;
 		dev->resource = res;
 		for (i = 0; i < num_reg; i++, res++) {
 			rc = of_address_to_resource(np, i, res);
 			WARN_ON(rc);
 		}
+		for (i = 0; i < num_localbus_reg; i++, res++) {
+			rc = of_localbus_address_to_resource(np, i, res);
+			WARN_ON(rc);
+		}
 		if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
 			pr_debug("not all legacy IRQ resources mapped for %s\n",
 				 np->name);
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index fb7b722..10112ea 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -42,6 +42,8 @@  extern u64 of_translate_dma_address(struct device_node *dev,
 extern u64 of_translate_address(struct device_node *np, const __be32 *addr);
 extern int of_address_to_resource(struct device_node *dev, int index,
 				  struct resource *r);
+extern int of_localbus_address_to_resource(struct device_node *dev, int index,
+				  struct resource *r);
 extern struct device_node *of_find_matching_node_by_address(
 					struct device_node *from,
 					const struct of_device_id *matches,
@@ -55,6 +57,9 @@  extern void __iomem *of_iomap(struct device_node *device, int index);
 extern const __be32 *of_get_address(struct device_node *dev, int index,
 			   u64 *size, unsigned int *flags);
 
+extern const __be32 *of_get_localbus_address(struct device_node *np, int index,
+			   u64 *size);
+
 extern unsigned long pci_address_to_pio(phys_addr_t addr);
 
 extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
@@ -80,6 +85,12 @@  static inline const __be32 *of_get_address(struct device_node *dev, int index,
 	return NULL;
 }
 
+static inline const __be32 *of_get_localbus_address(struct device_node *dev,
+						    int index, u64 *size)
+{
+	return NULL;
+}
+
 static inline int of_pci_range_parser_init(struct of_pci_range_parser *parser,
 			struct device_node *node)
 {
@@ -108,6 +119,8 @@  static inline bool of_dma_is_coherent(struct device_node *np)
 #ifdef CONFIG_OF
 extern int of_address_to_resource(struct device_node *dev, int index,
 				  struct resource *r);
+extern int of_localbus_address_to_resource(struct device_node *dev, int index,
+				  struct resource *r);
 void __iomem *of_iomap(struct device_node *node, int index);
 void __iomem *of_io_request_and_map(struct device_node *device,
 					int index, char *name);
@@ -121,6 +134,12 @@  static inline int of_address_to_resource(struct device_node *dev, int index,
 	return -EINVAL;
 }
 
+static inline int of_localbus_address_to_resource(struct device_node *dev,
+						  int index, struct resource *r)
+{
+	return -EINVAL;
+}
+
 static inline void __iomem *of_iomap(struct device_node *device, int index)
 {
 	return NULL;