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.
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) {
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>;
};
}
--
regards,
Stan
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?
--
regards,
Stan
On Tue, Jul 29, 2014 at 9:06 AM, Stanimir Varbanov <[email protected]> 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 <[email protected]> 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 <[email protected]> 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>;
}
};
};
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Tue, Jul 29, 2014 at 8:07 PM, Stephen Boyd <[email protected]> wrote:
> On 07/29/14 16:45, Grant Likely wrote:
>> On Tue, 29 Jul 2014 17:06:42 +0300, Stanimir Varbanov <[email protected]> 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 <[email protected]> wrote:
> > On 07/29/14 16:45, Grant Likely wrote:
> >> On Tue, 29 Jul 2014 17:06:42 +0300, Stanimir Varbanov <[email protected]> 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.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation