Re: [PATCH 1/2] regulator: pbias: use untranslated address to program pbias regulator

[Kishon Vijay Abraham I <kishon@ti.com>]

Now fixed Lee Jones mail address!

On Tuesday 01 September 2015 03:10 PM, Kishon Vijay Abraham I wrote:
> Hi Mark,
> 
> On Monday 31 August 2015 08:22 PM, Mark Brown wrote:
>> On Mon, Aug 31, 2015 at 04:14:07PM +0530, Kishon Vijay Abraham I wrote:
>>> On Tuesday 25 August 2015 07:20 PM, Mark Brown wrote:
>>>> On Tue, Aug 25, 2015 at 01:03:04PM +0300, Grygorii Strashko wrote:
>>>>> On 08/19/2015 09:11 PM, Mark Brown wrote:
>>
>>>>>> So substract this address from the start of the resource to get the
>>>>>> offset?  Or provide a wrapper function in the resource code which does
>>>>>> that.  
>>>>
>>>>> I'd be very appreciated if you have and can share any thought on
>>>>> How can we get this absolute base address to substract?
>>>>
>>>> Ask the syscon device for its resource?  Or have it provide an absolute
>>>
>>> Even if we get the absolute address of syscon, we have to do the
>>> subtraction only for the newer dtbs (previous dtbs already have only the
>>> offset). Do you recommend adding a new property to differentiate between
>>> older dtbs and newer dtbs? Any other suggestions here?
>>
>> Hang on.  This is the first I've heard of any DTs not just having
>> absolute addresses.  How does any of this work - has it ever worked, and
>> is the situation completely understood?  My original concern with this
> 
> Before commit d919501feffa8715147582c3ffce96fad0c7016f ARM: dts: dra7:
> add minimal l4 bus layout with control module support, the dt was like
> 
> ocp {
> 	dra7_ctrl_general: tisyscon@4a002e00 {
> 		compatible = "syscon";
> 		reg = <0x4a002e00 0x7c>;
> 	};
> 
> 	pbias_regulator: pbias_regulator {
> 		compatible = "ti,pbias-omap";
> 		reg = <0 0x4>;
> 		syscon = <&dra7_ctrl_general>;
> 	};
> };
> 
> Here platform_get_resource in pbias driver returns '0' which is
> populated in vsel_reg and enable_reg. And regulator_enable_regmap uses
> the base address from tisyscon@4a002e00 which is '0x4a002e00' and offset
> from enable_reg which is '0' inorder to write to the pbias register.
> 
> But after commit d919501fef, the dt became like this (after a couple of
> fixes)
> 
> ocp {
> 	l4_cfg: l4@4a000000 {
> 		compatible = "ti,dra7-l4-cfg", "simple-bus";
> 		ranges = <0 0x4a000000 0x22c000>;
> 
> 		scm: scm@2000 {
> 			compatible = "ti,dra7-scm-core", "simple-bus";
> 			reg = <0x2000 0x2000>;
> 			ranges = <0 0x2000 0x2000>;
> 
> 			scm_conf: scm_conf@0 {
> 				compatible = "syscon", "simple-bus";
> 				reg = <0x0 0x1400>;
> 				ranges = <0 0x0 0x1400>;
> 
> 				pbias_regulator: pbias_regulator {
> 					compatible = "ti,pbias-omap";
> 					reg = <0xe00 0x4>;
> 					syscon = <&scm_conf>;
>                                 };
> 			};
> 		};
> 	};
> };
> 
> Here platform_get_resource in pbias driver returns '4a002e00' which is
> populated in vsel_reg and enable_reg. And regulator_enable_regmap uses
> the base address from scm_conf@0 which is '0x4a002000' and offset from
> enable_reg which is '4a002e00' inorder to write to the pbias register
> and it results in a abort.
> 
>> was that I coudn't understand the commit log and your original response
>> seemed to indicate that we always have the absolute address :(  Perhaps
> 
> We started having the absolute address only after the dt change (commit
> d919501fef and a couple of more dt fixes).
> 
>> this is something to do with the brief mention of having moved the DT
>> node for some reason?
>>
>> We at least need some sort of coherent explanation of the problem and a
>> comprehensible fix to go with it.  Right now it seems like things are
>> just being moved about to hide problems without either of these things
>> which seems like it makes the code less clear and more fragile.
> 
> hmm.. IMO the actual problem was modelling the 'offset' as a resource
> (by populating the offset in 'reg' property) which was added when the
> initial pbias dt node was created. And now since the pbias dt node is
> being moved around, it's causing inadvertent address translation
> breaking the pbias driver. IMHO the value in 'reg' property of pbias dt
> node should be treated as 'offset' instead of address 'resource' and
> that's what my patch tried to do.
>>
>>>> address based interface for that matter?
>>
>>> Syscon doesn't directly expose any API's to write to it's register.
>>> Rather it uses regmap APIs to read/write to it's register. I'm not sure
>>> if it's possible to add regmap APIs to write to a register with absolute
>>> address. Any hints?
>>
>> Yes, I'm aware that it is regmap based!  What I am suggesting is that if
>> people are making DTs like yours with devices that are children of the
>> syscon then presumably it might be useful for it to allow client drivers
>> to pass absolute addresses in so that it can translate for them.
> 
> Arnd, Lee?
> 
> Thanks
> Kishon
>