diff mbox

use IORESOURCE_REG resource type for non-translatable addresses in DT

Message ID 53D788A7.4020303@mm-sol.com (mailing list archive)
State New, archived
Headers show

Commit Message

Stanimir Varbanov July 29, 2014, 11:42 a.m. UTC
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>;
	};
}

Comments

Arnd Bergmann July 29, 2014, noon UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stanimir Varbanov July 29, 2014, 2:06 p.m. UTC | #2
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?
Rob Herring July 29, 2014, 3:29 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grant Likely July 29, 2014, 11:45 p.m. UTC | #4
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

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd July 30, 2014, 1:07 a.m. UTC | #5
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>;
        }
    };
};
Rob Herring July 30, 2014, 2:53 a.m. UTC | #6
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd July 30, 2014, 6:06 a.m. UTC | #7
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.
Stanimir Varbanov Aug. 27, 2014, 4:27 p.m. UTC | #8
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.
Bjorn Andersson Aug. 27, 2014, 6:24 p.m. UTC | #9
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Aug. 27, 2014, 9:55 p.m. UTC | #10
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.
Stanimir Varbanov Aug. 28, 2014, 7:58 a.m. UTC | #11
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. :(
Bjorn Andersson Aug. 29, 2014, 4:09 a.m. UTC | #12
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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) {