| Message ID | 53D788A7.4020303@mm-sol.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 29 July 2014 14:42:31 Stanimir Varbanov wrote: > taddr = of_translate_address(dev, addrp); > - if (taddr == OF_BAD_ADDR) > - return -EINVAL; > + /* > + * if the address is non-translatable to cpu physical address > + * fallback to a IORESOURCE_REG resource. > + */ > + if (taddr == OF_BAD_ADDR) { > + memset(r, 0, sizeof(*r)); > + taddr = of_read_number(addrp, 1); > + if (taddr == OF_BAD_ADDR) > + return -EINVAL; > + r->start = taddr; > + r->end = taddr + size - 1; > + r->flags = IORESOURCE_REG; > + r->name = name ? name : dev->full_name; > + return 0; > + } > + I don't think that everything returning OF_BAD_ADDR makes sense to turn into IORESOURCE_REG. It could be an e.g. invalid DT representation, a node with #size-cells=<0>, or it could be something that gets translated one or more nodes up in the tree before it reaches a bus without a ranges property. Also, you should not rely on #address-cells being hardcoded to <1> above. How about modifying of_get_address() rather than __of_address_to_resource() instead? You could introduce a new of_bus entry for each bus you expect to return an IORESOURCE_REG, or you could change of_bus_default_get_flags to return IORESOURCE_REG if the parent node has no ranges property and is not the root node. Arnd
Arnd, thanks for the comments. On 07/29/2014 03:00 PM, Arnd Bergmann wrote: > On Tuesday 29 July 2014 14:42:31 Stanimir Varbanov wrote: >> taddr = of_translate_address(dev, addrp); >> - if (taddr == OF_BAD_ADDR) >> - return -EINVAL; >> + /* >> + * if the address is non-translatable to cpu physical address >> + * fallback to a IORESOURCE_REG resource. >> + */ >> + if (taddr == OF_BAD_ADDR) { >> + memset(r, 0, sizeof(*r)); >> + taddr = of_read_number(addrp, 1); >> + if (taddr == OF_BAD_ADDR) >> + return -EINVAL; >> + r->start = taddr; >> + r->end = taddr + size - 1; >> + r->flags = IORESOURCE_REG; >> + r->name = name ? name : dev->full_name; >> + return 0; >> + } >> + > > I don't think that everything returning OF_BAD_ADDR makes sense > to turn into IORESOURCE_REG. It could be an e.g. invalid DT > representation, a node with #size-cells=<0>, or it could be > something that gets translated one or more nodes up in the > tree before it reaches a bus without a ranges property. > > Also, you should not rely on #address-cells being hardcoded > to <1> above. > This was just an example. Of course it has many issues and probaly it is wrong:) The main goal was to understand does IORESOURCE_REG resource type and parsing the *reg* properties for non-translatable addresses are feasible. And also does it acceptable by community and OF platform maintainers. > How about modifying of_get_address() rather than > __of_address_to_resource() instead? You could introduce > a new of_bus entry for each bus you expect to return > an IORESOURCE_REG, or you could change of_bus_default_get_flags > to return IORESOURCE_REG if the parent node has no ranges property > and is not the root node. IMO the clearer solution is to introduce a new of_bus bus. In that case one possible problem will be how to distinguish the non-translatable and the other buses. Also the *device_type* property is deprecated for non PCI devices. The second option will need to change the prototype of .get_flags method to receive device_node structure. Thoughts?
On Tue, Jul 29, 2014 at 9:06 AM, Stanimir Varbanov <svarbanov@mm-sol.com> wrote: > Arnd, thanks for the comments. > > On 07/29/2014 03:00 PM, Arnd Bergmann wrote: >> On Tuesday 29 July 2014 14:42:31 Stanimir Varbanov wrote: >>> taddr = of_translate_address(dev, addrp); >>> - if (taddr == OF_BAD_ADDR) >>> - return -EINVAL; >>> + /* >>> + * if the address is non-translatable to cpu physical address >>> + * fallback to a IORESOURCE_REG resource. >>> + */ >>> + if (taddr == OF_BAD_ADDR) { >>> + memset(r, 0, sizeof(*r)); >>> + taddr = of_read_number(addrp, 1); >>> + if (taddr == OF_BAD_ADDR) >>> + return -EINVAL; >>> + r->start = taddr; >>> + r->end = taddr + size - 1; >>> + r->flags = IORESOURCE_REG; >>> + r->name = name ? name : dev->full_name; >>> + return 0; >>> + } >>> + >> >> I don't think that everything returning OF_BAD_ADDR makes sense >> to turn into IORESOURCE_REG. It could be an e.g. invalid DT >> representation, a node with #size-cells=<0>, or it could be >> something that gets translated one or more nodes up in the >> tree before it reaches a bus without a ranges property. >> >> Also, you should not rely on #address-cells being hardcoded >> to <1> above. >> > > This was just an example. Of course it has many issues and probaly it is > wrong:) The main goal was to understand does IORESOURCE_REG resource > type and parsing the *reg* properties for non-translatable addresses are > feasible. And also does it acceptable by community and OF platform > maintainers. I guess the question I have is what is the advantage of making this a resource? You can't really pass it to other functions. We're moving in the opposite direction for IRQs as now platform_get_irq translates the IRQ directly rather than using the resource (but the resource is still there just to avoid potentially breaking things for now). >> How about modifying of_get_address() rather than >> __of_address_to_resource() instead? You could introduce >> a new of_bus entry for each bus you expect to return >> an IORESOURCE_REG, or you could change of_bus_default_get_flags >> to return IORESOURCE_REG if the parent node has no ranges property >> and is not the root node. > > IMO the clearer solution is to introduce a new of_bus bus. In that case > one possible problem will be how to distinguish the non-translatable and > the other buses. Also the *device_type* property is deprecated for non > PCI devices. You would have to look for the presence or absence of ranges property. Rob
On Tue, 29 Jul 2014 17:06:42 +0300, Stanimir Varbanov <svarbanov@mm-sol.com> wrote: > Arnd, thanks for the comments. > > On 07/29/2014 03:00 PM, Arnd Bergmann wrote: > > On Tuesday 29 July 2014 14:42:31 Stanimir Varbanov wrote: > >> taddr = of_translate_address(dev, addrp); > >> - if (taddr == OF_BAD_ADDR) > >> - return -EINVAL; > >> + /* > >> + * if the address is non-translatable to cpu physical address > >> + * fallback to a IORESOURCE_REG resource. > >> + */ > >> + if (taddr == OF_BAD_ADDR) { > >> + memset(r, 0, sizeof(*r)); > >> + taddr = of_read_number(addrp, 1); > >> + if (taddr == OF_BAD_ADDR) > >> + return -EINVAL; > >> + r->start = taddr; > >> + r->end = taddr + size - 1; > >> + r->flags = IORESOURCE_REG; > >> + r->name = name ? name : dev->full_name; > >> + return 0; > >> + } > >> + > > > > I don't think that everything returning OF_BAD_ADDR makes sense > > to turn into IORESOURCE_REG. It could be an e.g. invalid DT > > representation, a node with #size-cells=<0>, or it could be > > something that gets translated one or more nodes up in the > > tree before it reaches a bus without a ranges property. > > > > Also, you should not rely on #address-cells being hardcoded > > to <1> above. > > > > This was just an example. Of course it has many issues and probaly it is > wrong:) The main goal was to understand does IORESOURCE_REG resource > type and parsing the *reg* properties for non-translatable addresses are > feasible. And also does it acceptable by community and OF platform > maintainers. The use case is actually very different from of_address_to_resource or of_get_address() because those APIs explicitly return physical memory addresses from the CPU perspective. It makes more sense to create a new API that doesn't attempt to translate the reg address. Alternately, a new API that only translates upto a given parent node. You don't want to automagically translate as far up the tree as possible because the code won't know what node the address is associated with. Translated addresses only make sense if the node it was translated to as also known. g. > > > How about modifying of_get_address() rather than > > __of_address_to_resource() instead? You could introduce > > a new of_bus entry for each bus you expect to return > > an IORESOURCE_REG, or you could change of_bus_default_get_flags > > to return IORESOURCE_REG if the parent node has no ranges property > > and is not the root node. > > IMO the clearer solution is to introduce a new of_bus bus. In that case > one possible problem will be how to distinguish the non-translatable and > the other buses. Also the *device_type* property is deprecated for non > PCI devices. > > The second option will need to change the prototype of .get_flags method > to receive device_node structure. > > Thoughts? > > -- > regards, > Stan
On 07/29/14 16:45, Grant Likely wrote: > On Tue, 29 Jul 2014 17:06:42 +0300, Stanimir Varbanov <svarbanov@mm-sol.com> wrote: >> >> This was just an example. Of course it has many issues and probaly it is >> wrong:) The main goal was to understand does IORESOURCE_REG resource >> type and parsing the *reg* properties for non-translatable addresses are >> feasible. And also does it acceptable by community and OF platform >> maintainers. > The use case is actually very different from of_address_to_resource or > of_get_address() because those APIs explicitly return physical memory > addresses from the CPU perspective. It makes more sense to create a new > API that doesn't attempt to translate the reg address. Alternately, a > new API that only translates upto a given parent node. The most important thing is that platform_get_resource{_by_name}(&pdev, IORESOURCE_REG, n) returns the reg property and optional size encoded into a struct resource. I think Rob is suggesting we circumvent the entire of_address_to_resource() path and do some if (IS_ENABLED(CONFIG_OF) && type == IORESOURCE_REG) check in platform_get_resource() to package up the reg property into a struct resource. That should work. It sounds like you think partially translating addresses is risky though. Fair enough. Perhaps we should call WARN() if someone tries to call platform_get_resource() with IORESOURCE_REG and the parent node has a ranges property that isn't a one-to-one conversion. That way if we want to do something like this we can. pmic@0 { #address-cells = <1>; #size-cells = <0>; reg = <0>; regulators { ranges; #address-cells = <1>; #size-cells = <0>; regulator@40 { reg = <0x40>; }; regulator@50 { reg = <0x50>; } }; };
On Tue, Jul 29, 2014 at 8:07 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 07/29/14 16:45, Grant Likely wrote: >> On Tue, 29 Jul 2014 17:06:42 +0300, Stanimir Varbanov <svarbanov@mm-sol.com> wrote: >>> >>> This was just an example. Of course it has many issues and probaly it is >>> wrong:) The main goal was to understand does IORESOURCE_REG resource >>> type and parsing the *reg* properties for non-translatable addresses are >>> feasible. And also does it acceptable by community and OF platform >>> maintainers. >> The use case is actually very different from of_address_to_resource or >> of_get_address() because those APIs explicitly return physical memory >> addresses from the CPU perspective. It makes more sense to create a new >> API that doesn't attempt to translate the reg address. Alternately, a >> new API that only translates upto a given parent node. > > The most important thing is that platform_get_resource{_by_name}(&pdev, > IORESOURCE_REG, n) returns the reg property and optional size encoded > into a struct resource. I think Rob is suggesting we circumvent the > entire of_address_to_resource() path and do some if > (IS_ENABLED(CONFIG_OF) && type == IORESOURCE_REG) check in > platform_get_resource() to package up the reg property into a struct > resource. That should work. No, I'm saying why are you using platform_get_resource at all and adding a new resource type? I don't see any advantage. You might as well do of_property_read_u32 in the below example. Rob > It sounds like you think partially translating addresses is risky > though. Fair enough. Perhaps we should call WARN() if someone tries to > call platform_get_resource() with IORESOURCE_REG and the parent node has > a ranges property that isn't a one-to-one conversion. That way if we > want to do something like this we can. > > pmic@0 { > #address-cells = <1>; > #size-cells = <0>; > reg = <0>; > > regulators { > ranges; > #address-cells = <1>; > #size-cells = <0>; > > regulator@40 { > reg = <0x40>; > }; > > regulator@50 { > reg = <0x50>; > } > }; > }; > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by The Linux Foundation >
On 07/29, Rob Herring wrote: > On Tue, Jul 29, 2014 at 8:07 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > On 07/29/14 16:45, Grant Likely wrote: > >> On Tue, 29 Jul 2014 17:06:42 +0300, Stanimir Varbanov <svarbanov@mm-sol.com> wrote: > >>> > >>> This was just an example. Of course it has many issues and probaly it is > >>> wrong:) The main goal was to understand does IORESOURCE_REG resource > >>> type and parsing the *reg* properties for non-translatable addresses are > >>> feasible. And also does it acceptable by community and OF platform > >>> maintainers. > >> The use case is actually very different from of_address_to_resource or > >> of_get_address() because those APIs explicitly return physical memory > >> addresses from the CPU perspective. It makes more sense to create a new > >> API that doesn't attempt to translate the reg address. Alternately, a > >> new API that only translates upto a given parent node. > > > > The most important thing is that platform_get_resource{_by_name}(&pdev, > > IORESOURCE_REG, n) returns the reg property and optional size encoded > > into a struct resource. I think Rob is suggesting we circumvent the > > entire of_address_to_resource() path and do some if > > (IS_ENABLED(CONFIG_OF) && type == IORESOURCE_REG) check in > > platform_get_resource() to package up the reg property into a struct > > resource. That should work. > > No, I'm saying why are you using platform_get_resource at all and > adding a new resource type? I don't see any advantage. First off, the resource type is not new. IORESOURCE_REG has existed for two years (see commit 72dcb1197228 "resources: Add register address resource type, 2012-08-07"). The main advantage is allowing things like platform_get_resource_by_name() and platform_get_resource() to work seamlessly with devicetree. If we don't have this, drivers are going to open code their reg property parsing and possibly reg-names parsing. There are a handful of drivers that would be doing this duplicate work. Sure, we could consolidate them into an OF helper function, but then these are the only platform drivers that are getting their reg properties via a special non-translatable address function. The drivers don't care that they're using non-translateable addresses as long as they can pass the address returned from platform_get_resource() to their regmap and do I/O. The drivers are written under the assumption that they're a sub-device of some parent device (in this case a PMIC) and so they assume that the regmap has already been setup for them. > > You might as well do of_property_read_u32 in the below example. > Fair enough. The example is probably too simple. Things are sometimes slightly more complicated and a simple of_property_read_u32() isn't going to work in the case of multiple reg values or when reg-names is in play.
On 07/30/2014 09:06 AM, Stephen Boyd wrote: > On 07/29, Rob Herring wrote: >> On Tue, Jul 29, 2014 at 8:07 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>> On 07/29/14 16:45, Grant Likely wrote: >>>> On Tue, 29 Jul 2014 17:06:42 +0300, Stanimir Varbanov <svarbanov@mm-sol.com> wrote: >>>>> >>>>> This was just an example. Of course it has many issues and probaly it is >>>>> wrong:) The main goal was to understand does IORESOURCE_REG resource >>>>> type and parsing the *reg* properties for non-translatable addresses are >>>>> feasible. And also does it acceptable by community and OF platform >>>>> maintainers. >>>> The use case is actually very different from of_address_to_resource or >>>> of_get_address() because those APIs explicitly return physical memory >>>> addresses from the CPU perspective. It makes more sense to create a new >>>> API that doesn't attempt to translate the reg address. Alternately, a >>>> new API that only translates upto a given parent node. >>> >>> The most important thing is that platform_get_resource{_by_name}(&pdev, >>> IORESOURCE_REG, n) returns the reg property and optional size encoded >>> into a struct resource. I think Rob is suggesting we circumvent the >>> entire of_address_to_resource() path and do some if >>> (IS_ENABLED(CONFIG_OF) && type == IORESOURCE_REG) check in >>> platform_get_resource() to package up the reg property into a struct >>> resource. That should work. >> >> No, I'm saying why are you using platform_get_resource at all and >> adding a new resource type? I don't see any advantage. > > First off, the resource type is not new. IORESOURCE_REG has > existed for two years (see commit 72dcb1197228 "resources: Add > register address resource type, 2012-08-07"). > > The main advantage is allowing things like > platform_get_resource_by_name() and platform_get_resource() to > work seamlessly with devicetree. If we don't have this, drivers > are going to open code their reg property parsing and possibly > reg-names parsing. There are a handful of drivers that would be > doing this duplicate work. > > Sure, we could consolidate them into an OF helper function, but > then these are the only platform drivers that are getting their > reg properties via a special non-translatable address function. > The drivers don't care that they're using non-translateable > addresses as long as they can pass the address returned from > platform_get_resource() to their regmap and do I/O. The drivers > are written under the assumption that they're a sub-device of > some parent device (in this case a PMIC) and so they assume that > the regmap has already been setup for them. Starting from the fact that these devices are sub-functions of PMIC (MFD devices) and currently there are no so many users of similar API outside of PMIC's world, does it make sense to implement an API for non-translatable addresses in MFD core? We could pass a null pointer to the mfd_cell->resources which means that the resources will be collected from the DT. There is already code in mfd_add_device() which passed over every child of the parent device node. This way the mfd_add_device() will fill the proper resource type and the sub-function drivers can use platform_get_resource() without worries.
On Tue, Jul 29, 2014 at 11:06 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 07/29, Rob Herring wrote: [..] >> >> You might as well do of_property_read_u32 in the below example. >> > > Fair enough. The example is probably too simple. Things are > sometimes slightly more complicated and a simple > of_property_read_u32() isn't going to work in the case of > multiple reg values or when reg-names is in play. > But do we have such cases in the Qualcomm PMICs? The only case I've hit so far is for gpios and mpps, where it feels like reg should be base, size and not simply base reg of first gpio - but that's a different thing. Also, so far it seems like most drivers just code the base address in the driver, as we have very specific compatibles. How about we stop trying so hard to make this "perfect" and just merge something close to Josh's original proposal and ignore this problem? Currently all we're doing is delaying any possibility of getting drivers for the individual blocks merged. If we have the dt bindings require the reg to be there, we can discuss and change this all we want later! Regards, Bjorn
On 08/27/14 11:24, Bjorn Andersson wrote: > On Tue, Jul 29, 2014 at 11:06 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> On 07/29, Rob Herring wrote: > [..] >>> You might as well do of_property_read_u32 in the below example. >>> >> Fair enough. The example is probably too simple. Things are >> sometimes slightly more complicated and a simple >> of_property_read_u32() isn't going to work in the case of >> multiple reg values or when reg-names is in play. >> > But do we have such cases in the Qualcomm PMICs? At the least we have reg-names users. > > The only case I've hit so far is for gpios and mpps, where it feels > like reg should be base, size and not simply base reg of first gpio - > but that's a different thing. > > Also, so far it seems like most drivers just code the base address in > the driver, as we have very specific compatibles. It would be nice if the drivers didn't have to do this. It annoys me that platform drivers need to know that they're using DT to figure out things that are standard platform features: registers, irqs, etc. > > > How about we stop trying so hard to make this "perfect" and just merge > something close to Josh's original proposal and ignore this problem? > Currently all we're doing is delaying any possibility of getting > drivers for the individual blocks merged. > If we have the dt bindings require the reg to be there, we can discuss > and change this all we want later! What's Josh's original proposal? We've already punted on this for SSBI PMICs and just required them to put registers in DT for use some day and I don't see anyone blocking individual SPMI pmic drivers from merging over this. So I guess I agree with you that we can move forward with the other drivers. I'd still like to see us make this better though, so it seems worthwhile to have the discussion and get to some conclusion.
On 08/27/2014 09:24 PM, Bjorn Andersson wrote: > On Tue, Jul 29, 2014 at 11:06 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> On 07/29, Rob Herring wrote: > [..] >>> >>> You might as well do of_property_read_u32 in the below example. >>> >> >> Fair enough. The example is probably too simple. Things are >> sometimes slightly more complicated and a simple >> of_property_read_u32() isn't going to work in the case of >> multiple reg values or when reg-names is in play. >> > > But do we have such cases in the Qualcomm PMICs? yes, we have few - rtc, iadc and partially regulators. > > The only case I've hit so far is for gpios and mpps, where it feels > like reg should be base, size and not simply base reg of first gpio - > but that's a different thing. > > Also, so far it seems like most drivers just code the base address in > the driver, as we have very specific compatibles. > > > How about we stop trying so hard to make this "perfect" and just merge > something close to Josh's original proposal and ignore this problem? > Currently all we're doing is delaying any possibility of getting > drivers for the individual blocks merged. > If we have the dt bindings require the reg to be there, we can discuss > and change this all we want later! I totally agree with you, the latest PMIC version of the patches sent two weeks ago follow this assumption but I didn't receive any ack/reviewed-by from anyone and we miss the merge window. :(
On Wed, Aug 27, 2014 at 2:55 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > What's Josh's original proposal? We've already punted on this for SSBI > PMICs and just required them to put registers in DT for use some day and > I don't see anyone blocking individual SPMI pmic drivers from merging > over this. So I guess I agree with you that we can move forward with the > other drivers. I'd still like to see us make this better though, so it > seems worthwhile to have the discussion and get to some conclusion. > That sounds totally reasonable, I agree that we should try to sort this out and if we do this should be applicable on pm8xxx as well(?). My concern was that further discussions would hinder the ability of actually committing the pm8x41 container driver and that is in my eyes blocking integration of drivers for the pmic blocks. Regards, Bjorn
diff --git a/drivers/of/address.c b/drivers/of/address.c index 5edfcb0..898741e 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -617,9 +617,24 @@ static int __of_address_to_resource(struct device_node *dev, if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0) return -EINVAL; + taddr = of_translate_address(dev, addrp); - if (taddr == OF_BAD_ADDR) - return -EINVAL; + /* + * if the address is non-translatable to cpu physical address + * fallback to a IORESOURCE_REG resource. + */ + if (taddr == OF_BAD_ADDR) { + memset(r, 0, sizeof(*r)); + taddr = of_read_number(addrp, 1); + if (taddr == OF_BAD_ADDR) + return -EINVAL; + r->start = taddr; + r->end = taddr + size - 1; + r->flags = IORESOURCE_REG; + r->name = name ? name : dev->full_name; + return 0; + } + memset(r, 0, sizeof(struct resource)); if (flags & IORESOURCE_IO) {
Hi, While looking in MFD drivers I saw that few of them (88pm860x-core, max8925-core and wm831x-core) allow use of IORESOURCE_REG as resource type when calling platform_get_resource() by their child drivers. The resources for these child devices are filled by core MFD driver manually and then passed to mfd_add_devices() as mfd_cells. During development and review comments of the MFD core driver for Qualcomm SPMI PMICs we came down to a need to describe PMIC peripheral addresses (the PMIC sub-functions) through *reg* property in DT. The PMIC peripheral drivers will be scattered over the /drivers and they will call platform_get_resource() to extract their peripheral base addresses from resource->start. The issue we have encountered is that these addresses are non-translatable thus of_address_to_resource returns OF_BAD_ADDR. Stephen Boyd have made a suggestion to solve the issue here [1]. Is that approach acceptable? Or do we have better way? How similar issues could be solved. Our DT node for SPMI PMICs can be seen below [2]. Please do comment. PS: I have made a little change in __of_address_to_resource() to illustrate what I meant above. unsigned long port; [1] https://lkml.org/lkml/2014/7/17/680 [2] Simplistic PMIC DT node. spmi@fc4cf000 { compatible = "qcom,spmi-pmic-arb"; reg = <0xfc4cf000 0x1000>, <0xfc4cb000 0x1000>, <0xfc4ca000 0x1000>; reg-names = "core", "intr", "cnfg"; #address-cells = <2>; #size-cells = <0>; interrupt-controller; #interrupt-cells = <4>; pm8941@0 { compatible = "qcom,pm8941"; reg = <0x0 SPMI_USID>; #size-cells = <1>; #address-cells = <1>; rtc { compatible = "qcom,pm8941-rtc"; reg = <0x6000 0x100>, <0x6100 0x100>; reg-names = "rtc", "alarm"; interrupts = <0x0 0x61 0x1 0>; interrupt-names = "alarm"; }; }; pm8941@1 { compatible = "qcom,pm8941"; reg = <0x1 SPMI_USID>; }; }