| Message ID | 1409672700-21697-1-git-send-email-svarbanov@mm-sol.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > +} > + >
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
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.
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.
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
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
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.
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.
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.
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 --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;