use IORESOURCE_REG resource type for non-translatable addresses in DT

use IORESOURCE_REG resource type for non-translatable addresses in DT

[Stanimir Varbanov]

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

Re: use IORESOURCE_REG resource type for non-translatable addresses in DT

[Arnd Bergmann]

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

Re: use IORESOURCE_REG resource type for non-translatable addresses in DT

[Stanimir Varbanov]

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

Re: use IORESOURCE_REG resource type for non-translatable addresses in DT

[Rob Herring]

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

Re: use IORESOURCE_REG resource type for non-translatable addresses in DT

[Grant Likely]

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

Re: use IORESOURCE_REG resource type for non-translatable addresses in DT

[Stephen Boyd]

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>;
        }
    };
};

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: use IORESOURCE_REG resource type for non-translatable addresses in DT

[Rob Herring]

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
>

Re: use IORESOURCE_REG resource type for non-translatable addresses in DT

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: use IORESOURCE_REG resource type for non-translatable addresses in DT

[Stanimir Varbanov]

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.

-- 
regards,
Stan

Re: use IORESOURCE_REG resource type for non-translatable addresses in DT

[Bjorn Andersson]

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

Re: use IORESOURCE_REG resource type for non-translatable addresses in DT

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: use IORESOURCE_REG resource type for non-translatable addresses in DT

[Bjorn Andersson]

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

Re: use IORESOURCE_REG resource type for non-translatable addresses in DT

[Stanimir Varbanov]

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. :(


-- 
regards,
Stan

[PATCH] RFC: add function for localbus address

[Stanimir Varbanov]

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;
+}
+
 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;
-- 
1.7.0.4

Re: [PATCH] RFC: add function for localbus address

[Stephen Boyd]

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;
> +}
> +
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] RFC: add function for localbus address

[Grant Likely]

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.

Re: [PATCH] RFC: add function for localbus address

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] RFC: add function for localbus address

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] RFC: add function for localbus address

[Grant Likely]

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.

Re: [PATCH] RFC: add function for localbus address

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] RFC: add function for localbus address

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] RFC: add function for localbus address

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] RFC: add function for localbus address

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2078 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] RFC: add function for localbus address

[Stanimir Varbanov]

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

-- 
regards,
Stan