[PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Nishanth Menon]

Certain platforms such as DRA7 have quirky memory maps such as:
PRM_ABBLDO_DSPEVE_CTRL	0x4ae07e20
PRM_ABBLDO_IVA_CTRL	0x4ae07e24
other-registers
PRM_ABBLDO_DSPEVE_SETUP	0x4ae07e30
PRM_ABBLDO_IVA_SETUP	0x4ae07e34

These need the address allocation to be either shared OR unique
allocation per register instance.

So, introduce v3 type of ABB for these instances to allocate based on
shared address ranges.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
Baseline off: v3.13-rc8

The alternate approach is to introduce registers as seperate
reg-names options, which means all existing abb-v1,v2 will probably have to
change as well.

The reason for this mishmash of register allocation is due to SoC integration
lack of discipline of ensuring that device instances need contigous register
allocation :(.. but too late to fix :(

 .../bindings/regulator/ti-abb-regulator.txt        |    1 +
 drivers/regulator/ti-abb-regulator.c               |   26 +++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt b/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
index 2e57a33..96ae151 100644
--- a/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
@@ -4,6 +4,7 @@ Required Properties:
 - compatible: Should be one of:
   - "ti,abb-v1" for older SoCs like OMAP3
   - "ti,abb-v2" for newer SoCs like OMAP4, OMAP5
+  - "ti,abb-v3" for newer SoCs like DRA7 quirky addressing
 - reg: Address and length of the register set for the device. It contains
   the information of registers in the same order as described by reg-names
 - reg-names: Should contain the reg names
diff --git a/drivers/regulator/ti-abb-regulator.c b/drivers/regulator/ti-abb-regulator.c
index b187b6b..5f9cfd4 100644
--- a/drivers/regulator/ti-abb-regulator.c
+++ b/drivers/regulator/ti-abb-regulator.c
@@ -673,9 +673,23 @@ static const struct ti_abb_reg abb_regs_v2 = {
 	.opp_sel_mask		= (0x03 << 0),
 };
 
+static const struct ti_abb_reg abb_regs_v3 = {
+	.control_reg		= 0x00,
+	.setup_reg		= 0x10,
+
+	.sr2_wtcnt_value_mask	= (0xff << 8),
+	.fbb_sel_mask		= (0x01 << 2),
+	.rbb_sel_mask		= (0x01 << 1),
+	.sr2_en_mask		= (0x01 << 0),
+
+	.opp_change_mask	= (0x01 << 2),
+	.opp_sel_mask		= (0x03 << 0),
+};
+
 static const struct of_device_id ti_abb_of_match[] = {
 	{.compatible = "ti,abb-v1", .data = &abb_regs_v1},
 	{.compatible = "ti,abb-v2", .data = &abb_regs_v2},
+	{.compatible = "ti,abb-v3", .data = &abb_regs_v3},
 	{ },
 };
 
@@ -724,7 +738,17 @@ static int ti_abb_probe(struct platform_device *pdev)
 	/* Map ABB resources */
 	pname = "base-address";
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
-	abb->base = devm_ioremap_resource(dev, res);
+	if (!res) {
+		dev_err(dev, "Missing '%s' IO resource\n", pname);
+		return -ENODEV;
+	}
+
+	/*
+	 * On some platforms such as DRA7, the register range of ABB LDOx is
+	 * interleaved with registers of another ABB LDO and non-LDO registers!
+	 * We cannot use reserved allocation in these cases.
+	 */
+	abb->base = devm_ioremap_nocache(dev, res->start, resource_size(res));
 	if (IS_ERR(abb->base))
 		return PTR_ERR(abb->base);
 
-- 
1.7.9.5

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Mark Brown]

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

On Thu, Jan 16, 2014 at 01:32:30PM -0600, Nishanth Menon wrote:
> Certain platforms such as DRA7 have quirky memory maps such as:
> PRM_ABBLDO_DSPEVE_CTRL	0x4ae07e20
> PRM_ABBLDO_IVA_CTRL	0x4ae07e24
> other-registers
> PRM_ABBLDO_DSPEVE_SETUP	0x4ae07e30
> PRM_ABBLDO_IVA_SETUP	0x4ae07e34
> 
> These need the address allocation to be either shared OR unique
> allocation per register instance.

Is there a system controller involved here by any chance?

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

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Nishanth Menon]

On 01/17/2014 10:08 AM, Mark Brown wrote:
> On Thu, Jan 16, 2014 at 01:32:30PM -0600, Nishanth Menon wrote:
>> Certain platforms such as DRA7 have quirky memory maps such as:
>> PRM_ABBLDO_DSPEVE_CTRL	0x4ae07e20
>> PRM_ABBLDO_IVA_CTRL	0x4ae07e24
>> other-registers
>> PRM_ABBLDO_DSPEVE_SETUP	0x4ae07e30
>> PRM_ABBLDO_IVA_SETUP	0x4ae07e34
>>
>> These need the address allocation to be either shared OR unique
>> allocation per register instance.
> 
> Is there a system controller involved here by any chance?
> 
Nope. ABB LDO module is a standalone instance whose register set
happens to be part of a memory range, as I recently got to know, is a
favorite for our hardware designers to put "misc" modules at SoC
integration time - Sigh! I guess I have made more than enough stink
about this internally already :(.

-- 
Regards,
Nishanth Menon

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Mark Brown]

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

On Fri, Jan 17, 2014 at 10:47:29AM -0600, Nishanth Menon wrote:
> On 01/17/2014 10:08 AM, Mark Brown wrote:

> > Is there a system controller involved here by any chance?

> Nope. ABB LDO module is a standalone instance whose register set
> happens to be part of a memory range, as I recently got to know, is a
> favorite for our hardware designers to put "misc" modules at SoC
> integration time - Sigh! I guess I have made more than enough stink
> about this internally already :(.

That sounds exactly like a system controller :)

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

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Nishanth Menon]

On 01/17/2014 12:00 PM, Mark Brown wrote:
> On Fri, Jan 17, 2014 at 10:47:29AM -0600, Nishanth Menon wrote:
>> On 01/17/2014 10:08 AM, Mark Brown wrote:
> 
>>> Is there a system controller involved here by any chance?
> 
>> Nope. ABB LDO module is a standalone instance whose register set
>> happens to be part of a memory range, as I recently got to know, is a
>> favorite for our hardware designers to put "misc" modules at SoC
>> integration time - Sigh! I guess I have made more than enough stink
>> about this internally already :(.
> 
> That sounds exactly like a system controller :)
Hmm.. I have considered something like PRCM in OMAP as a system
controller... anyways.. I think I miss your suggestion here. Is there
something you might suggest to improve in the patch or take an
alternative route?


-- 
Regards,
Nishanth Menon

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Mark Brown]

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

On Fri, Jan 17, 2014 at 12:11:44PM -0600, Nishanth Menon wrote:
> On 01/17/2014 12:00 PM, Mark Brown wrote:

> > That sounds exactly like a system controller :)

> Hmm.. I have considered something like PRCM in OMAP as a system
> controller... anyways.. I think I miss your suggestion here. Is there
> something you might suggest to improve in the patch or take an
> alternative route?

What people have done for these things is have a device for the
controller that maps the registers and then have the other devices talk
through that to get the access they need.  You still need to specify
which interrupts but it's simpler with the I/O mapping and is needed for
arbitration if the hardware has register bits for multiple functions in
the same register.

Not thought enough about the patch itself yet, that's more a separate
thing.

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

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Mark Brown]

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

On Thu, Jan 16, 2014 at 01:32:30PM -0600, Nishanth Menon wrote:

> -	abb->base = devm_ioremap_resource(dev, res);

> +	abb->base = devm_ioremap_nocache(dev, res->start, resource_size(res));

devm_ioremap_resouce() should do the right thing if the memory region is
marked as uncacheable (with IORESOURCE_CACHEABLE not set).  Since I
can't see the OF code actually setting that flag on the resources unless
I'm missing something this change isn't needed?

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

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Nishanth Menon]

On 01/21/2014 12:55 PM, Mark Brown wrote:
> On Thu, Jan 16, 2014 at 01:32:30PM -0600, Nishanth Menon wrote:
> 
>> -	abb->base = devm_ioremap_resource(dev, res);
> 
>> +	abb->base = devm_ioremap_nocache(dev, res->start, resource_size(res));
> 
> devm_ioremap_resouce() should do the right thing if the memory region is
> marked as uncacheable (with IORESOURCE_CACHEABLE not set).  Since I
> can't see the OF code actually setting that flag on the resources unless
> I'm missing something this change isn't needed?
> 
Without this change, on DRA7 I get:
[    0.579500] abb_mpu: 1060 <--> 1210 mV
[    0.580321] abb_ivahd: 1055 <--> 1250 mV
[    0.580583] ti_abb 4ae07e20.regulator-abb-dspeve: can't request
region for resource [mem 0x4ae07e20-0x4ae07e2f]
[    0.580610] ti_abb: probe of 4ae07e20.regulator-abb-dspeve failed
with error -16
[    0.581216] abb_gpu: 1090 <--> 1280 mV

with the change in the patch, I get:
[    0.589750] abb_mpu: 1060 <--> 1210 mV
[    0.590522] abb_ivahd: 1055 <--> 1250 mV
[    0.591331] abb_dspeve: 1055 <--> 1250 mV
[    0.592097] abb_gpu: 1090 <--> 1280 mV

reference dts:
https://github.com/nmenon/linux-2.6-playground/blob/abb-rev-v3.14-rc1-vnext-20140121/arch/arm/boot/dts/dra7.dtsi#L562
-- 
Regards,
Nishanth Menon

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Mark Brown]

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

On Tue, Jan 21, 2014 at 02:06:25PM -0600, Nishanth Menon wrote:

> Without this change, on DRA7 I get:
> [    0.579500] abb_mpu: 1060 <--> 1210 mV
> [    0.580321] abb_ivahd: 1055 <--> 1250 mV
> [    0.580583] ti_abb 4ae07e20.regulator-abb-dspeve: can't request
> region for resource [mem 0x4ae07e20-0x4ae07e2f]
> [    0.580610] ti_abb: probe of 4ae07e20.regulator-abb-dspeve failed
> with error -16
> [    0.581216] abb_gpu: 1090 <--> 1280 mV

OK, that's not to do with nocache, that's to do with duplicate
request_mem_region() calls.  Of course the trick here is that if the
other thing that requests the memory region doesn't do it then we have a
problem.  

I really can't help thinking that a system controller node as the parent
is going to make things happier for devices that share this register
bank.  I guess it might be possible to also do it with single register
memory regions but I'd not be surprised if that wasn't possible and it
doesn't seem as idiomatic.

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

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Nishanth Menon]

On 01/21/2014 03:56 PM, Mark Brown wrote:
> On Tue, Jan 21, 2014 at 02:06:25PM -0600, Nishanth Menon wrote:
> 
>> Without this change, on DRA7 I get:
>> [    0.579500] abb_mpu: 1060 <--> 1210 mV
>> [    0.580321] abb_ivahd: 1055 <--> 1250 mV
>> [    0.580583] ti_abb 4ae07e20.regulator-abb-dspeve: can't request
>> region for resource [mem 0x4ae07e20-0x4ae07e2f]
>> [    0.580610] ti_abb: probe of 4ae07e20.regulator-abb-dspeve failed
>> with error -16
>> [    0.581216] abb_gpu: 1090 <--> 1280 mV
> 
> OK, that's not to do with nocache, that's to do with duplicate
> request_mem_region() calls.  Of course the trick here is that if the
> other thing that requests the memory region doesn't do it then we have a
> problem.  
Yes. Both mappings are for ABB driver itself due to the interleaved
addresses for two ABB instances :(

> 
> I really can't help thinking that a system controller node as the parent
> is going to make things happier for devices that share this register
> bank.  I guess it might be possible to also do it with single register
> memory regions but I'd not be surprised if that wasn't possible and it
> doesn't seem as idiomatic.
> 
I am tempted by syscon and using regmap. but doing a syscon on entire
control module has impact to a bunch of drivers as you can expect.

The other alternative will be to use reg-names and map individual
registers (there are just setup and control registers to deal with per
abb instance). That will coexist with other pinctrl, bandgap and other
drivers which are not based off syscon.


-- 
Regards,
Nishanth Menon

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Mark Brown]

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

On Tue, Jan 21, 2014 at 04:32:12PM -0600, Nishanth Menon wrote:

> The other alternative will be to use reg-names and map individual
> registers (there are just setup and control registers to deal with per
> abb instance). That will coexist with other pinctrl, bandgap and other
> drivers which are not based off syscon.

Yes, I suspect that's going to be a lot easier to deploy given that
there are existing drivers using the current non-syscon setup.  I'm
totally fine with that approach.

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

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Nishanth Menon]

On 01/22/2014 02:19 PM, Mark Brown wrote:
> On Tue, Jan 21, 2014 at 04:32:12PM -0600, Nishanth Menon wrote:
> 
>> The other alternative will be to use reg-names and map individual
>> registers (there are just setup and control registers to deal with per
>> abb instance). That will coexist with other pinctrl, bandgap and other
>> drivers which are not based off syscon.
> 
> Yes, I suspect that's going to be a lot easier to deploy given that
> there are existing drivers using the current non-syscon setup.  I'm
> totally fine with that approach.
> 
ok - thanks for confirming - this also means that existing compatible
flags will have to change - but they were present solely to encode the
offset of setup and control from each other.

Considering that we never had any dtsi in upstream (since they were
all pending clock dts support for TI platforms), I will go with the
assumption of not needing to maintain backward compatible for dtbs
that could never have been generated in upstream kernel.

-- 
Regards,
Nishanth Menon

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Nishanth Menon]

On 14:48-20140122, Nishanth Menon wrote:
> On 01/22/2014 02:19 PM, Mark Brown wrote:
> > On Tue, Jan 21, 2014 at 04:32:12PM -0600, Nishanth Menon wrote:
> > 
> >> The other alternative will be to use reg-names and map individual
> >> registers (there are just setup and control registers to deal with per
> >> abb instance). That will coexist with other pinctrl, bandgap and other
> >> drivers which are not based off syscon.
> > 
> > Yes, I suspect that's going to be a lot easier to deploy given that
> > there are existing drivers using the current non-syscon setup.  I'm
> > totally fine with that approach.
> > 
> ok - thanks for confirming - this also means that existing compatible
> flags will have to change - but they were present solely to encode the
> offset of setup and control from each other.
> 
> Considering that we never had any dtsi in upstream (since they were
> all pending clock dts support for TI platforms), I will go with the
> assumption of not needing to maintain backward compatible for dtbs
> that could never have been generated in upstream kernel.

How about something like the following? If this is acceptable, I can do
a complete round of retest and ensure everything is functional on all
platforms and post it as a formal patch:

>From 510812eeb10d5dffe44aa896746468bcb7f17fe2 Mon Sep 17 00:00:00 2001
From: Nishanth Menon <nm@ti.com>
Date: Wed, 22 Jan 2014 16:06:57 -0600
Subject: [RFC PATCH] regulator: ti-abb: Add support for interleaved LDO registers

Certain platforms such as DRA7 have quirky memory maps such as:
PRM_ABBLDO_DSPEVE_CTRL      0x4ae07e20
PRM_ABBLDO_IVA_CTRL 0x4ae07e24
other-registers
PRM_ABBLDO_DSPEVE_SETUP     0x4ae07e30
PRM_ABBLDO_IVA_SETUP        0x4ae07e34

These need the address range allocation to be either not reserved OR unique
allocation per register instance or use something like syscon based
solution.

By going with unique allocation per register, we are able to now have a
single compatible driver for all instances on all platforms which use
the IP block.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 .../bindings/regulator/ti-abb-regulator.txt        |    6 +-
 drivers/regulator/ti-abb-regulator.c               |   75 +++++++-------------
 2 files changed, 30 insertions(+), 51 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt b/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
index 2e57a33..395e279 100644
--- a/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
@@ -2,12 +2,12 @@ Adaptive Body Bias(ABB) SoC internal LDO regulator for Texas Instruments SoCs
 
 Required Properties:
 - compatible: Should be one of:
-  - "ti,abb-v1" for older SoCs like OMAP3
-  - "ti,abb-v2" for newer SoCs like OMAP4, OMAP5
+  - "ti,abb-v1" for SoCs like OMAP3,4,5,DRA7
 - reg: Address and length of the register set for the device. It contains
   the information of registers in the same order as described by reg-names
 - reg-names: Should contain the reg names
-  - "base-address"	- contains base address of ABB module
+  - "control-address"	- contains control register address of ABB module
+  - "setup-address"	- contains setup register address of ABB module
   - "int-address"	- contains address of interrupt register for ABB module
   (also see Optional properties)
 - #address-cell: should be 0
diff --git a/drivers/regulator/ti-abb-regulator.c b/drivers/regulator/ti-abb-regulator.c
index b187b6b..aebad81 100644
--- a/drivers/regulator/ti-abb-regulator.c
+++ b/drivers/regulator/ti-abb-regulator.c
@@ -54,8 +54,6 @@ struct ti_abb_info {
 
 /**
  * struct ti_abb_reg - Register description for ABB block
- * @setup_reg:			setup register offset from base
- * @control_reg:		control register offset from base
  * @sr2_wtcnt_value_mask:	setup register- sr2_wtcnt_value mask
  * @fbb_sel_mask:		setup register- FBB sel mask
  * @rbb_sel_mask:		setup register- RBB sel mask
@@ -64,9 +62,6 @@ struct ti_abb_info {
  * @opp_sel_mask:		control register - mask for mode to operate
  */
 struct ti_abb_reg {
-	u32 setup_reg;
-	u32 control_reg;
-
 	/* Setup register fields */
 	u32 sr2_wtcnt_value_mask;
 	u32 fbb_sel_mask;
@@ -82,7 +77,8 @@ struct ti_abb_reg {
  * struct ti_abb - ABB instance data
  * @rdesc:			regulator descriptor
  * @clk:			clock(usually sysclk) supplying ABB block
- * @base:			base address of ABB block
+ * @control_reg:		control register of the ABB module
+ * @setup_reg:			setup register of the ABB module
  * @int_base:			interrupt register base address
  * @efuse_base:			(optional) efuse base address for ABB modes
  * @ldo_base:			(optional) LDOVBB vset override base address
@@ -98,7 +94,8 @@ struct ti_abb_reg {
 struct ti_abb {
 	struct regulator_desc rdesc;
 	struct clk *clk;
-	void __iomem *base;
+	void __iomem *setup_reg;
+	void __iomem *control_reg;
 	void __iomem *int_base;
 	void __iomem *efuse_base;
 	void __iomem *ldo_base;
@@ -118,20 +115,18 @@ struct ti_abb {
  * ti_abb_rmw() - handy wrapper to set specific register bits
  * @mask:	mask for register field
  * @value:	value shifted to mask location and written
- * @offset:	offset of register
- * @base:	base address
+ * @base:	register address
  *
  * Return: final register value (may be unused)
  */
-static inline u32 ti_abb_rmw(u32 mask, u32 value, u32 offset,
-			     void __iomem *base)
+static inline u32 ti_abb_rmw(u32 mask, u32 value, void __iomem *base)
 {
 	u32 val;
 
-	val = readl(base + offset);
+	val = readl(base);
 	val &= ~mask;
 	val |= (value << __ffs(mask)) & mask;
-	writel(val, base + offset);
+	writel(val, base);
 
 	return val;
 }
@@ -263,21 +258,19 @@ static int ti_abb_set_opp(struct regulator_dev *rdev, struct ti_abb *abb,
 	if (ret)
 		goto out;
 
-	ti_abb_rmw(regs->fbb_sel_mask | regs->rbb_sel_mask, 0, regs->setup_reg,
-		   abb->base);
+	ti_abb_rmw(regs->fbb_sel_mask | regs->rbb_sel_mask, 0, abb->setup_reg);
 
 	switch (info->opp_sel) {
 	case TI_ABB_SLOW_OPP:
-		ti_abb_rmw(regs->rbb_sel_mask, 1, regs->setup_reg, abb->base);
+		ti_abb_rmw(regs->rbb_sel_mask, 1, abb->setup_reg);
 		break;
 	case TI_ABB_FAST_OPP:
-		ti_abb_rmw(regs->fbb_sel_mask, 1, regs->setup_reg, abb->base);
+		ti_abb_rmw(regs->fbb_sel_mask, 1, abb->setup_reg);
 		break;
 	}
 
 	/* program next state of ABB ldo */
-	ti_abb_rmw(regs->opp_sel_mask, info->opp_sel, regs->control_reg,
-		   abb->base);
+	ti_abb_rmw(regs->opp_sel_mask, info->opp_sel, abb->control_reg);
 
 	/*
 	 * program LDO VBB vset override if needed for !bypass mode
@@ -288,7 +281,7 @@ static int ti_abb_set_opp(struct regulator_dev *rdev, struct ti_abb *abb,
 		ti_abb_program_ldovbb(dev, abb, info);
 
 	/* Initiate ABB ldo change */
-	ti_abb_rmw(regs->opp_change_mask, 1, regs->control_reg, abb->base);
+	ti_abb_rmw(regs->opp_change_mask, 1, abb->control_reg);
 
 	/* Wait for ABB LDO to complete transition to new Bias setting */
 	ret = ti_abb_wait_txdone(dev, abb);
@@ -490,8 +483,7 @@ static int ti_abb_init_timings(struct device *dev, struct ti_abb *abb)
 	dev_dbg(dev, "%s: Clk_rate=%ld, sr2_cnt=0x%08x\n", __func__,
 		clk_get_rate(abb->clk), sr2_wt_cnt_val);
 
-	ti_abb_rmw(regs->sr2_wtcnt_value_mask, sr2_wt_cnt_val, regs->setup_reg,
-		   abb->base);
+	ti_abb_rmw(regs->sr2_wtcnt_value_mask, sr2_wt_cnt_val, abb->setup_reg);
 
 	return 0;
 }
@@ -645,25 +637,7 @@ static struct regulator_ops ti_abb_reg_ops = {
 	.get_voltage_sel = ti_abb_get_voltage_sel,
 };
 
-/* Default ABB block offsets, IF this changes in future, create new one */
-static const struct ti_abb_reg abb_regs_v1 = {
-	/* WARNING: registers are wrongly documented in TRM */
-	.setup_reg		= 0x04,
-	.control_reg		= 0x00,
-
-	.sr2_wtcnt_value_mask	= (0xff << 8),
-	.fbb_sel_mask		= (0x01 << 2),
-	.rbb_sel_mask		= (0x01 << 1),
-	.sr2_en_mask		= (0x01 << 0),
-
-	.opp_change_mask	= (0x01 << 2),
-	.opp_sel_mask		= (0x03 << 0),
-};
-
-static const struct ti_abb_reg abb_regs_v2 = {
-	.setup_reg		= 0x00,
-	.control_reg		= 0x04,
-
+static const struct ti_abb_reg abb_regs = {
 	.sr2_wtcnt_value_mask	= (0xff << 8),
 	.fbb_sel_mask		= (0x01 << 2),
 	.rbb_sel_mask		= (0x01 << 1),
@@ -674,8 +648,7 @@ static const struct ti_abb_reg abb_regs_v2 = {
 };
 
 static const struct of_device_id ti_abb_of_match[] = {
-	{.compatible = "ti,abb-v1", .data = &abb_regs_v1},
-	{.compatible = "ti,abb-v2", .data = &abb_regs_v2},
+	{.compatible = "ti,abb-v1", .data = &abb_regs},
 	{ },
 };
 
@@ -722,11 +695,17 @@ static int ti_abb_probe(struct platform_device *pdev)
 	abb->regs = match->data;
 
 	/* Map ABB resources */
-	pname = "base-address";
+	pname = "setup-address";
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
+	abb->setup_reg = devm_ioremap_resource(dev, res);
+	if (IS_ERR(abb->setup_reg))
+		return PTR_ERR(abb->setup_reg);
+
+	pname = "control-address";
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
-	abb->base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(abb->base))
-		return PTR_ERR(abb->base);
+	abb->control_reg = devm_ioremap_resource(dev, res);
+	if (IS_ERR(abb->control_reg))
+		return PTR_ERR(abb->control_reg);
 
 	pname = "int-address";
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
@@ -860,7 +839,7 @@ skip_opt:
 	platform_set_drvdata(pdev, rdev);
 
 	/* Enable the ldo if not already done by bootloader */
-	ti_abb_rmw(abb->regs->sr2_en_mask, 1, abb->regs->setup_reg, abb->base);
+	ti_abb_rmw(abb->regs->sr2_en_mask, 1, abb->setup_reg);
 
 	return 0;
 }
-- 
1.7.9.5

-- 
Regards,
Nishanth Menon

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Mark Brown]

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

On Wed, Jan 22, 2014 at 04:25:23PM -0600, Nishanth Menon wrote:

> How about something like the following? If this is acceptable, I can do
> a complete round of retest and ensure everything is functional on all
> platforms and post it as a formal patch:

Looks OK from a quick scan.  It seems nothing in mainline is using the
-v2 binding so it's probably OK to delete it but if it's not too much
bother it might be better to keep it since I expect some downstream
trees might've picked that up already.

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

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Nishanth Menon]

On 12:29-20140123, Mark Brown wrote:
> On Wed, Jan 22, 2014 at 04:25:23PM -0600, Nishanth Menon wrote:
> 
> > How about something like the following? If this is acceptable, I can do
> > a complete round of retest and ensure everything is functional on all
> > platforms and post it as a formal patch:
> 
> Looks OK from a quick scan.  It seems nothing in mainline is using the
> -v2 binding so it's probably OK to delete it but if it's not too much
> bother it might be better to keep it since I expect some downstream
> trees might've picked that up already.
True - how about the following (formal post pending comprehensive
testing and your feedback on approach):

>From 35b0c999f7ef94bf92acc17e1086af22b187943a Mon Sep 17 00:00:00 2001
From: Nishanth Menon <nm@ti.com>
Date: Thu, 23 Jan 2014 10:00:22 -0600
Subject: [PATCH V3] regulator: ti-abb: Add support for interleaved LDO registers

Certain platforms such as DRA7 have quirky memory maps such as:
PRM_ABBLDO_DSPEVE_CTRL      0x4ae07e20
PRM_ABBLDO_IVA_CTRL 0x4ae07e24
other-registers
PRM_ABBLDO_DSPEVE_SETUP     0x4ae07e30
PRM_ABBLDO_IVA_SETUP        0x4ae07e34

These need the address range allocation to be either not reserved OR unique
allocation per register instance or use something like syscon based
solution.

By going with unique allocation per register, we are able to now have a
single compatible driver for all instances on all platforms which use
the IP block.

So, introduce a new "ti,abb-v3" compatible to allow for definitions
where explicit register definitions are provided, while maintaining
backward compatibility of older predefined register offsets provided
by "ti-abb-v1" and "ti-abb-v2".

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 .../bindings/regulator/ti-abb-regulator.txt        |    6 +-
 drivers/regulator/ti-abb-regulator.c               |   91 +++++++++++++-------
 2 files changed, 67 insertions(+), 30 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt b/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
index 2e57a33..0d2dc26 100644
--- a/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
@@ -4,10 +4,14 @@ Required Properties:
 - compatible: Should be one of:
   - "ti,abb-v1" for older SoCs like OMAP3
   - "ti,abb-v2" for newer SoCs like OMAP4, OMAP5
+  - "ti,abb-v3" for a generic definition where setup and control registers are
+     provided (example: DRA7)
 - reg: Address and length of the register set for the device. It contains
   the information of registers in the same order as described by reg-names
 - reg-names: Should contain the reg names
-  - "base-address"	- contains base address of ABB module
+  - "base-address"	- contains base address of ABB module (ti,abb-v1,ti,abb-v2)
+  - "control-address"	- contains base address of ABB module (ti,abb-v3)
+  - "setup-address"	- contains base address of ABB module (ti,abb-v3)
   - "int-address"	- contains address of interrupt register for ABB module
   (also see Optional properties)
 - #address-cell: should be 0
diff --git a/drivers/regulator/ti-abb-regulator.c b/drivers/regulator/ti-abb-regulator.c
index b187b6b..134c481 100644
--- a/drivers/regulator/ti-abb-regulator.c
+++ b/drivers/regulator/ti-abb-regulator.c
@@ -54,8 +54,8 @@ struct ti_abb_info {
 
 /**
  * struct ti_abb_reg - Register description for ABB block
- * @setup_reg:			setup register offset from base
- * @control_reg:		control register offset from base
+ * @setup_off:			setup register offset from base
+ * @control_off:		setup register offset from base
  * @sr2_wtcnt_value_mask:	setup register- sr2_wtcnt_value mask
  * @fbb_sel_mask:		setup register- FBB sel mask
  * @rbb_sel_mask:		setup register- RBB sel mask
@@ -64,8 +64,8 @@ struct ti_abb_info {
  * @opp_sel_mask:		control register - mask for mode to operate
  */
 struct ti_abb_reg {
-	u32 setup_reg;
-	u32 control_reg;
+	u32 setup_off;
+	u32 control_off;
 
 	/* Setup register fields */
 	u32 sr2_wtcnt_value_mask;
@@ -83,6 +83,8 @@ struct ti_abb_reg {
  * @rdesc:			regulator descriptor
  * @clk:			clock(usually sysclk) supplying ABB block
  * @base:			base address of ABB block
+ * @setup_reg:			setup register of ABB block
+ * @control_reg:		setup register of ABB block
  * @int_base:			interrupt register base address
  * @efuse_base:			(optional) efuse base address for ABB modes
  * @ldo_base:			(optional) LDOVBB vset override base address
@@ -99,6 +101,8 @@ struct ti_abb {
 	struct regulator_desc rdesc;
 	struct clk *clk;
 	void __iomem *base;
+	void __iomem *setup_reg;
+	void __iomem *control_reg;
 	void __iomem *int_base;
 	void __iomem *efuse_base;
 	void __iomem *ldo_base;
@@ -118,20 +122,18 @@ struct ti_abb {
  * ti_abb_rmw() - handy wrapper to set specific register bits
  * @mask:	mask for register field
  * @value:	value shifted to mask location and written
- * @offset:	offset of register
- * @base:	base address
+ * @reg:	register address
  *
  * Return: final register value (may be unused)
  */
-static inline u32 ti_abb_rmw(u32 mask, u32 value, u32 offset,
-			     void __iomem *base)
+static inline u32 ti_abb_rmw(u32 mask, u32 value, void __iomem *reg)
 {
 	u32 val;
 
-	val = readl(base + offset);
+	val = readl(reg);
 	val &= ~mask;
 	val |= (value << __ffs(mask)) & mask;
-	writel(val, base + offset);
+	writel(val, reg);
 
 	return val;
 }
@@ -263,21 +265,20 @@ static int ti_abb_set_opp(struct regulator_dev *rdev, struct ti_abb *abb,
 	if (ret)
 		goto out;
 
-	ti_abb_rmw(regs->fbb_sel_mask | regs->rbb_sel_mask, 0, regs->setup_reg,
-		   abb->base);
+	ti_abb_rmw(regs->fbb_sel_mask | regs->rbb_sel_mask, 0,
+		   abb->setup_reg);
 
 	switch (info->opp_sel) {
 	case TI_ABB_SLOW_OPP:
-		ti_abb_rmw(regs->rbb_sel_mask, 1, regs->setup_reg, abb->base);
+		ti_abb_rmw(regs->rbb_sel_mask, 1, abb->setup_reg);
 		break;
 	case TI_ABB_FAST_OPP:
-		ti_abb_rmw(regs->fbb_sel_mask, 1, regs->setup_reg, abb->base);
+		ti_abb_rmw(regs->fbb_sel_mask, 1, abb->setup_reg);
 		break;
 	}
 
 	/* program next state of ABB ldo */
-	ti_abb_rmw(regs->opp_sel_mask, info->opp_sel, regs->control_reg,
-		   abb->base);
+	ti_abb_rmw(regs->opp_sel_mask, info->opp_sel, abb->control_reg);
 
 	/*
 	 * program LDO VBB vset override if needed for !bypass mode
@@ -288,7 +289,7 @@ static int ti_abb_set_opp(struct regulator_dev *rdev, struct ti_abb *abb,
 		ti_abb_program_ldovbb(dev, abb, info);
 
 	/* Initiate ABB ldo change */
-	ti_abb_rmw(regs->opp_change_mask, 1, regs->control_reg, abb->base);
+	ti_abb_rmw(regs->opp_change_mask, 1, abb->control_reg);
 
 	/* Wait for ABB LDO to complete transition to new Bias setting */
 	ret = ti_abb_wait_txdone(dev, abb);
@@ -490,8 +491,8 @@ static int ti_abb_init_timings(struct device *dev, struct ti_abb *abb)
 	dev_dbg(dev, "%s: Clk_rate=%ld, sr2_cnt=0x%08x\n", __func__,
 		clk_get_rate(abb->clk), sr2_wt_cnt_val);
 
-	ti_abb_rmw(regs->sr2_wtcnt_value_mask, sr2_wt_cnt_val, regs->setup_reg,
-		   abb->base);
+	ti_abb_rmw(regs->sr2_wtcnt_value_mask, sr2_wt_cnt_val,
+		   abb->setup_reg);
 
 	return 0;
 }
@@ -648,8 +649,8 @@ static struct regulator_ops ti_abb_reg_ops = {
 /* Default ABB block offsets, IF this changes in future, create new one */
 static const struct ti_abb_reg abb_regs_v1 = {
 	/* WARNING: registers are wrongly documented in TRM */
-	.setup_reg		= 0x04,
-	.control_reg		= 0x00,
+	.setup_off		= 0x04,
+	.control_off		= 0x00,
 
 	.sr2_wtcnt_value_mask	= (0xff << 8),
 	.fbb_sel_mask		= (0x01 << 2),
@@ -661,8 +662,8 @@ static const struct ti_abb_reg abb_regs_v1 = {
 };
 
 static const struct ti_abb_reg abb_regs_v2 = {
-	.setup_reg		= 0x00,
-	.control_reg		= 0x04,
+	.setup_off		= 0x00,
+	.control_off		= 0x04,
 
 	.sr2_wtcnt_value_mask	= (0xff << 8),
 	.fbb_sel_mask		= (0x01 << 2),
@@ -673,9 +674,20 @@ static const struct ti_abb_reg abb_regs_v2 = {
 	.opp_sel_mask		= (0x03 << 0),
 };
 
+static const struct ti_abb_reg abb_regs_generic = {
+	.sr2_wtcnt_value_mask	= (0xff << 8),
+	.fbb_sel_mask		= (0x01 << 2),
+	.rbb_sel_mask		= (0x01 << 1),
+	.sr2_en_mask		= (0x01 << 0),
+
+	.opp_change_mask	= (0x01 << 2),
+	.opp_sel_mask		= (0x03 << 0),
+};
+
 static const struct of_device_id ti_abb_of_match[] = {
 	{.compatible = "ti,abb-v1", .data = &abb_regs_v1},
 	{.compatible = "ti,abb-v2", .data = &abb_regs_v2},
+	{.compatible = "ti,abb-v3", .data = &abb_regs_generic},
 	{ },
 };
 
@@ -719,14 +731,35 @@ static int ti_abb_probe(struct platform_device *pdev)
 	abb = devm_kzalloc(dev, sizeof(struct ti_abb), GFP_KERNEL);
 	if (!abb)
 		return -ENOMEM;
+	abb->regs = devm_kzalloc(dev, sizeof(struct ti_abb_reg), GFP_KERNEL);
+	if (!abb->regs)
+		return -ENOMEM;
 	abb->regs = match->data;
 
 	/* Map ABB resources */
-	pname = "base-address";
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
-	abb->base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(abb->base))
-		return PTR_ERR(abb->base);
+	if (!abb->regs->setup_off && !abb->regs->control_off) {
+		pname = "base-address";
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
+		abb->base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(abb->base))
+			return PTR_ERR(abb->base);
+
+		abb->setup_reg = abb->base + abb->regs->setup_off;
+		abb->control_reg = abb->base + abb->regs->control_off;
+
+	} else {
+		pname = "control-address";
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
+		abb->control_reg = devm_ioremap_resource(dev, res);
+		if (IS_ERR(abb->control_reg))
+			return PTR_ERR(abb->control_reg);
+
+		pname = "setup-address";
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
+		abb->setup_reg = devm_ioremap_resource(dev, res);
+		if (IS_ERR(abb->setup_reg))
+			return PTR_ERR(abb->setup_reg);
+	}
 
 	pname = "int-address";
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
@@ -860,7 +893,7 @@ skip_opt:
 	platform_set_drvdata(pdev, rdev);
 
 	/* Enable the ldo if not already done by bootloader */
-	ti_abb_rmw(abb->regs->sr2_en_mask, 1, abb->regs->setup_reg, abb->base);
+	ti_abb_rmw(abb->regs->sr2_en_mask, 1, abb->setup_reg);
 
 	return 0;
 }
-- 
1.7.9.5

-- 
Regards,
Nishanth Menon

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Nishanth Menon]

On 01/23/2014 10:20 AM, Nishanth Menon wrote:
> On 12:29-20140123, Mark Brown wrote:
>> On Wed, Jan 22, 2014 at 04:25:23PM -0600, Nishanth Menon wrote:
>>
>>> How about something like the following? If this is acceptable, I can do
>>> a complete round of retest and ensure everything is functional on all
>>> platforms and post it as a formal patch:
>>
>> Looks OK from a quick scan.  It seems nothing in mainline is using the
>> -v2 binding so it's probably OK to delete it but if it's not too much
>> bother it might be better to keep it since I expect some downstream
>> trees might've picked that up already.
> True - how about the following (formal post pending comprehensive
> testing and your feedback on approach):
> 
> From 35b0c999f7ef94bf92acc17e1086af22b187943a Mon Sep 17 00:00:00 2001
> From: Nishanth Menon <nm@ti.com>
> Date: Thu, 23 Jan 2014 10:00:22 -0600
> Subject: [PATCH V3] regulator: ti-abb: Add support for interleaved LDO registers
> 
> Certain platforms such as DRA7 have quirky memory maps such as:
> PRM_ABBLDO_DSPEVE_CTRL      0x4ae07e20
> PRM_ABBLDO_IVA_CTRL 0x4ae07e24
> other-registers
> PRM_ABBLDO_DSPEVE_SETUP     0x4ae07e30
> PRM_ABBLDO_IVA_SETUP        0x4ae07e34
> 
> These need the address range allocation to be either not reserved OR unique
> allocation per register instance or use something like syscon based
> solution.
> 
> By going with unique allocation per register, we are able to now have a
> single compatible driver for all instances on all platforms which use
> the IP block.
> 
> So, introduce a new "ti,abb-v3" compatible to allow for definitions
> where explicit register definitions are provided, while maintaining
> backward compatibility of older predefined register offsets provided
> by "ti-abb-v1" and "ti-abb-v2".
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  .../bindings/regulator/ti-abb-regulator.txt        |    6 +-
>  drivers/regulator/ti-abb-regulator.c               |   91 +++++++++++++-------
>  2 files changed, 67 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt b/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
> index 2e57a33..0d2dc26 100644
> --- a/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
> @@ -4,10 +4,14 @@ Required Properties:
>  - compatible: Should be one of:
>    - "ti,abb-v1" for older SoCs like OMAP3
>    - "ti,abb-v2" for newer SoCs like OMAP4, OMAP5
> +  - "ti,abb-v3" for a generic definition where setup and control registers are
> +     provided (example: DRA7)
>  - reg: Address and length of the register set for the device. It contains
>    the information of registers in the same order as described by reg-names
>  - reg-names: Should contain the reg names
> -  - "base-address"	- contains base address of ABB module
> +  - "base-address"	- contains base address of ABB module (ti,abb-v1,ti,abb-v2)
> +  - "control-address"	- contains base address of ABB module (ti,abb-v3)
> +  - "setup-address"	- contains base address of ABB module (ti,abb-v3)
>    - "int-address"	- contains address of interrupt register for ABB module
>    (also see Optional properties)
>  - #address-cell: should be 0
> diff --git a/drivers/regulator/ti-abb-regulator.c b/drivers/regulator/ti-abb-regulator.c
> index b187b6b..134c481 100644
> --- a/drivers/regulator/ti-abb-regulator.c
> +++ b/drivers/regulator/ti-abb-regulator.c
> @@ -54,8 +54,8 @@ struct ti_abb_info {
>  
>  /**
>   * struct ti_abb_reg - Register description for ABB block
> - * @setup_reg:			setup register offset from base
> - * @control_reg:		control register offset from base
> + * @setup_off:			setup register offset from base
> + * @control_off:		setup register offset from base
>   * @sr2_wtcnt_value_mask:	setup register- sr2_wtcnt_value mask
>   * @fbb_sel_mask:		setup register- FBB sel mask
>   * @rbb_sel_mask:		setup register- RBB sel mask
> @@ -64,8 +64,8 @@ struct ti_abb_info {
>   * @opp_sel_mask:		control register - mask for mode to operate
>   */
>  struct ti_abb_reg {
> -	u32 setup_reg;
> -	u32 control_reg;
> +	u32 setup_off;
> +	u32 control_off;
>  
>  	/* Setup register fields */
>  	u32 sr2_wtcnt_value_mask;
> @@ -83,6 +83,8 @@ struct ti_abb_reg {
>   * @rdesc:			regulator descriptor
>   * @clk:			clock(usually sysclk) supplying ABB block
>   * @base:			base address of ABB block
> + * @setup_reg:			setup register of ABB block
> + * @control_reg:		setup register of ABB block
>   * @int_base:			interrupt register base address
>   * @efuse_base:			(optional) efuse base address for ABB modes
>   * @ldo_base:			(optional) LDOVBB vset override base address
> @@ -99,6 +101,8 @@ struct ti_abb {
>  	struct regulator_desc rdesc;
>  	struct clk *clk;
>  	void __iomem *base;
> +	void __iomem *setup_reg;
> +	void __iomem *control_reg;
>  	void __iomem *int_base;
>  	void __iomem *efuse_base;
>  	void __iomem *ldo_base;
> @@ -118,20 +122,18 @@ struct ti_abb {
>   * ti_abb_rmw() - handy wrapper to set specific register bits
>   * @mask:	mask for register field
>   * @value:	value shifted to mask location and written
> - * @offset:	offset of register
> - * @base:	base address
> + * @reg:	register address
>   *
>   * Return: final register value (may be unused)
>   */
> -static inline u32 ti_abb_rmw(u32 mask, u32 value, u32 offset,
> -			     void __iomem *base)
> +static inline u32 ti_abb_rmw(u32 mask, u32 value, void __iomem *reg)
>  {
>  	u32 val;
>  
> -	val = readl(base + offset);
> +	val = readl(reg);
>  	val &= ~mask;
>  	val |= (value << __ffs(mask)) & mask;
> -	writel(val, base + offset);
> +	writel(val, reg);
>  
>  	return val;
>  }
> @@ -263,21 +265,20 @@ static int ti_abb_set_opp(struct regulator_dev *rdev, struct ti_abb *abb,
>  	if (ret)
>  		goto out;
>  
> -	ti_abb_rmw(regs->fbb_sel_mask | regs->rbb_sel_mask, 0, regs->setup_reg,
> -		   abb->base);
> +	ti_abb_rmw(regs->fbb_sel_mask | regs->rbb_sel_mask, 0,
> +		   abb->setup_reg);
>  
>  	switch (info->opp_sel) {
>  	case TI_ABB_SLOW_OPP:
> -		ti_abb_rmw(regs->rbb_sel_mask, 1, regs->setup_reg, abb->base);
> +		ti_abb_rmw(regs->rbb_sel_mask, 1, abb->setup_reg);
>  		break;
>  	case TI_ABB_FAST_OPP:
> -		ti_abb_rmw(regs->fbb_sel_mask, 1, regs->setup_reg, abb->base);
> +		ti_abb_rmw(regs->fbb_sel_mask, 1, abb->setup_reg);
>  		break;
>  	}
>  
>  	/* program next state of ABB ldo */
> -	ti_abb_rmw(regs->opp_sel_mask, info->opp_sel, regs->control_reg,
> -		   abb->base);
> +	ti_abb_rmw(regs->opp_sel_mask, info->opp_sel, abb->control_reg);
>  
>  	/*
>  	 * program LDO VBB vset override if needed for !bypass mode
> @@ -288,7 +289,7 @@ static int ti_abb_set_opp(struct regulator_dev *rdev, struct ti_abb *abb,
>  		ti_abb_program_ldovbb(dev, abb, info);
>  
>  	/* Initiate ABB ldo change */
> -	ti_abb_rmw(regs->opp_change_mask, 1, regs->control_reg, abb->base);
> +	ti_abb_rmw(regs->opp_change_mask, 1, abb->control_reg);
>  
>  	/* Wait for ABB LDO to complete transition to new Bias setting */
>  	ret = ti_abb_wait_txdone(dev, abb);
> @@ -490,8 +491,8 @@ static int ti_abb_init_timings(struct device *dev, struct ti_abb *abb)
>  	dev_dbg(dev, "%s: Clk_rate=%ld, sr2_cnt=0x%08x\n", __func__,
>  		clk_get_rate(abb->clk), sr2_wt_cnt_val);
>  
> -	ti_abb_rmw(regs->sr2_wtcnt_value_mask, sr2_wt_cnt_val, regs->setup_reg,
> -		   abb->base);
> +	ti_abb_rmw(regs->sr2_wtcnt_value_mask, sr2_wt_cnt_val,
> +		   abb->setup_reg);
>  
>  	return 0;
>  }
> @@ -648,8 +649,8 @@ static struct regulator_ops ti_abb_reg_ops = {
>  /* Default ABB block offsets, IF this changes in future, create new one */
>  static const struct ti_abb_reg abb_regs_v1 = {
>  	/* WARNING: registers are wrongly documented in TRM */
> -	.setup_reg		= 0x04,
> -	.control_reg		= 0x00,
> +	.setup_off		= 0x04,
> +	.control_off		= 0x00,
>  
>  	.sr2_wtcnt_value_mask	= (0xff << 8),
>  	.fbb_sel_mask		= (0x01 << 2),
> @@ -661,8 +662,8 @@ static const struct ti_abb_reg abb_regs_v1 = {
>  };
>  
>  static const struct ti_abb_reg abb_regs_v2 = {
> -	.setup_reg		= 0x00,
> -	.control_reg		= 0x04,
> +	.setup_off		= 0x00,
> +	.control_off		= 0x04,
>  
>  	.sr2_wtcnt_value_mask	= (0xff << 8),
>  	.fbb_sel_mask		= (0x01 << 2),
> @@ -673,9 +674,20 @@ static const struct ti_abb_reg abb_regs_v2 = {
>  	.opp_sel_mask		= (0x03 << 0),
>  };
>  
> +static const struct ti_abb_reg abb_regs_generic = {
> +	.sr2_wtcnt_value_mask	= (0xff << 8),
> +	.fbb_sel_mask		= (0x01 << 2),
> +	.rbb_sel_mask		= (0x01 << 1),
> +	.sr2_en_mask		= (0x01 << 0),
> +
> +	.opp_change_mask	= (0x01 << 2),
> +	.opp_sel_mask		= (0x03 << 0),
> +};
> +
>  static const struct of_device_id ti_abb_of_match[] = {
>  	{.compatible = "ti,abb-v1", .data = &abb_regs_v1},
>  	{.compatible = "ti,abb-v2", .data = &abb_regs_v2},
> +	{.compatible = "ti,abb-v3", .data = &abb_regs_generic},
>  	{ },
>  };
>  
> @@ -719,14 +731,35 @@ static int ti_abb_probe(struct platform_device *pdev)
>  	abb = devm_kzalloc(dev, sizeof(struct ti_abb), GFP_KERNEL);
>  	if (!abb)
>  		return -ENOMEM;
> +	abb->regs = devm_kzalloc(dev, sizeof(struct ti_abb_reg), GFP_KERNEL);
> +	if (!abb->regs)
> +		return -ENOMEM;
>  	abb->regs = match->data;
>  
>  	/* Map ABB resources */
> -	pname = "base-address";
> -	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
> -	abb->base = devm_ioremap_resource(dev, res);
> -	if (IS_ERR(abb->base))
> -		return PTR_ERR(abb->base);
> +	if (!abb->regs->setup_off && !abb->regs->control_off) {
Sigh.. the check was supposed to be:
if (abb->regs->setup_off || abb->regs->control_off) {

Darned.. forgot to commit! Grr... anyways.. still looking for feedback
for the remaining.
> +		pname = "base-address";
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
> +		abb->base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(abb->base))
> +			return PTR_ERR(abb->base);
> +
> +		abb->setup_reg = abb->base + abb->regs->setup_off;
> +		abb->control_reg = abb->base + abb->regs->control_off;
> +
> +	} else {
> +		pname = "control-address";
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
> +		abb->control_reg = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(abb->control_reg))
> +			return PTR_ERR(abb->control_reg);
> +
> +		pname = "setup-address";
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
> +		abb->setup_reg = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(abb->setup_reg))
> +			return PTR_ERR(abb->setup_reg);
> +	}
>  
>  	pname = "int-address";
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
> @@ -860,7 +893,7 @@ skip_opt:
>  	platform_set_drvdata(pdev, rdev);
>  
>  	/* Enable the ldo if not already done by bootloader */
> -	ti_abb_rmw(abb->regs->sr2_en_mask, 1, abb->regs->setup_reg, abb->base);
> +	ti_abb_rmw(abb->regs->sr2_en_mask, 1, abb->setup_reg);
>  
>  	return 0;
>  }
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH] regulator: ti-abb: Add support for interleaved LDO registers

[Mark Brown]

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

On Thu, Jan 23, 2014 at 10:26:20AM -0600, Nishanth Menon wrote:
> On 01/23/2014 10:20 AM, Nishanth Menon wrote:

> Sigh.. the check was supposed to be:
> if (abb->regs->setup_off || abb->regs->control_off) {

> Darned.. forgot to commit! Grr... anyways.. still looking for feedback
> for the remaining.

Looks sensible enough from a quick scan again - so long as it passes
your testing...  :)

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

[PATCH V4] regulator: ti-abb: Add support for interleaved LDO registers

[Nishanth Menon]

Certain platforms such as DRA7 have quirky memory maps such as:
PRM_ABBLDO_DSPEVE_CTRL	0x4ae07e20
PRM_ABBLDO_IVA_CTRL	0x4ae07e24
other-registers
PRM_ABBLDO_DSPEVE_SETUP	0x4ae07e30
PRM_ABBLDO_IVA_SETUP	0x4ae07e34

These need the address range allocation to be either not reserved OR
unique allocation per register instance or use something like syscon
based solution.

By going with unique allocation per register, we are able to now have
a single compatible driver for all instances on all platforms which
use the IP block.

So, introduce a new "ti,abb-v3" compatible to allow for definitions
where explicit register definitions are provided, while maintaining
backward compatibility of older predefined register offsets provided
by "ti-abb-v1" and "ti-abb-v2".

As part of this change, we rename a few variables to indicate the
appropriate meaning.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
Testing code and relevant dts changes(pending 3.14-rc1 tag):
	https://github.com/nmenon/linux-2.6-playground/commits/abb-rev-v3.14-rc1-vnext-20140123

changes in v4 (since v3):
	- minor documentation fixes
	- logic fix of v3 to picking up setup and control regs
	- completed testing on OMAP3630, OMAP4430, OMAP4460, OMAP5432, DRA7

v3: https://patchwork.kernel.org/patch/3529851/
v2: https://patchwork.kernel.org/patch/3525751/
v1: https://patchwork.kernel.org/patch/3500281/

 .../bindings/regulator/ti-abb-regulator.txt        |    6 +-
 drivers/regulator/ti-abb-regulator.c               |   86 +++++++++++++-------
 2 files changed, 62 insertions(+), 30 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt b/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
index 2e57a33..c58db75 100644
--- a/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/ti-abb-regulator.txt
@@ -4,10 +4,14 @@ Required Properties:
 - compatible: Should be one of:
   - "ti,abb-v1" for older SoCs like OMAP3
   - "ti,abb-v2" for newer SoCs like OMAP4, OMAP5
+  - "ti,abb-v3" for a generic definition where setup and control registers are
+     provided (example: DRA7)
 - reg: Address and length of the register set for the device. It contains
   the information of registers in the same order as described by reg-names
 - reg-names: Should contain the reg names
-  - "base-address"	- contains base address of ABB module
+  - "base-address"	- contains base address of ABB module (ti,abb-v1,ti,abb-v2)
+  - "control-address"	- contains control register address of ABB module (ti,abb-v3)
+  - "setup-address"	- contains setup register address of ABB module (ti,abb-v3)
   - "int-address"	- contains address of interrupt register for ABB module
   (also see Optional properties)
 - #address-cell: should be 0
diff --git a/drivers/regulator/ti-abb-regulator.c b/drivers/regulator/ti-abb-regulator.c
index b187b6b..a97d0c5 100644
--- a/drivers/regulator/ti-abb-regulator.c
+++ b/drivers/regulator/ti-abb-regulator.c
@@ -54,8 +54,8 @@ struct ti_abb_info {
 
 /**
  * struct ti_abb_reg - Register description for ABB block
- * @setup_reg:			setup register offset from base
- * @control_reg:		control register offset from base
+ * @setup_off:			setup register offset from base
+ * @control_off:		control register offset from base
  * @sr2_wtcnt_value_mask:	setup register- sr2_wtcnt_value mask
  * @fbb_sel_mask:		setup register- FBB sel mask
  * @rbb_sel_mask:		setup register- RBB sel mask
@@ -64,8 +64,8 @@ struct ti_abb_info {
  * @opp_sel_mask:		control register - mask for mode to operate
  */
 struct ti_abb_reg {
-	u32 setup_reg;
-	u32 control_reg;
+	u32 setup_off;
+	u32 control_off;
 
 	/* Setup register fields */
 	u32 sr2_wtcnt_value_mask;
@@ -83,6 +83,8 @@ struct ti_abb_reg {
  * @rdesc:			regulator descriptor
  * @clk:			clock(usually sysclk) supplying ABB block
  * @base:			base address of ABB block
+ * @setup_reg:			setup register of ABB block
+ * @control_reg:		control register of ABB block
  * @int_base:			interrupt register base address
  * @efuse_base:			(optional) efuse base address for ABB modes
  * @ldo_base:			(optional) LDOVBB vset override base address
@@ -99,6 +101,8 @@ struct ti_abb {
 	struct regulator_desc rdesc;
 	struct clk *clk;
 	void __iomem *base;
+	void __iomem *setup_reg;
+	void __iomem *control_reg;
 	void __iomem *int_base;
 	void __iomem *efuse_base;
 	void __iomem *ldo_base;
@@ -118,20 +122,18 @@ struct ti_abb {
  * ti_abb_rmw() - handy wrapper to set specific register bits
  * @mask:	mask for register field
  * @value:	value shifted to mask location and written
- * @offset:	offset of register
- * @base:	base address
+ * @reg:	register address
  *
  * Return: final register value (may be unused)
  */
-static inline u32 ti_abb_rmw(u32 mask, u32 value, u32 offset,
-			     void __iomem *base)
+static inline u32 ti_abb_rmw(u32 mask, u32 value, void __iomem *reg)
 {
 	u32 val;
 
-	val = readl(base + offset);
+	val = readl(reg);
 	val &= ~mask;
 	val |= (value << __ffs(mask)) & mask;
-	writel(val, base + offset);
+	writel(val, reg);
 
 	return val;
 }
@@ -263,21 +265,19 @@ static int ti_abb_set_opp(struct regulator_dev *rdev, struct ti_abb *abb,
 	if (ret)
 		goto out;
 
-	ti_abb_rmw(regs->fbb_sel_mask | regs->rbb_sel_mask, 0, regs->setup_reg,
-		   abb->base);
+	ti_abb_rmw(regs->fbb_sel_mask | regs->rbb_sel_mask, 0, abb->setup_reg);
 
 	switch (info->opp_sel) {
 	case TI_ABB_SLOW_OPP:
-		ti_abb_rmw(regs->rbb_sel_mask, 1, regs->setup_reg, abb->base);
+		ti_abb_rmw(regs->rbb_sel_mask, 1, abb->setup_reg);
 		break;
 	case TI_ABB_FAST_OPP:
-		ti_abb_rmw(regs->fbb_sel_mask, 1, regs->setup_reg, abb->base);
+		ti_abb_rmw(regs->fbb_sel_mask, 1, abb->setup_reg);
 		break;
 	}
 
 	/* program next state of ABB ldo */
-	ti_abb_rmw(regs->opp_sel_mask, info->opp_sel, regs->control_reg,
-		   abb->base);
+	ti_abb_rmw(regs->opp_sel_mask, info->opp_sel, abb->control_reg);
 
 	/*
 	 * program LDO VBB vset override if needed for !bypass mode
@@ -288,7 +288,7 @@ static int ti_abb_set_opp(struct regulator_dev *rdev, struct ti_abb *abb,
 		ti_abb_program_ldovbb(dev, abb, info);
 
 	/* Initiate ABB ldo change */
-	ti_abb_rmw(regs->opp_change_mask, 1, regs->control_reg, abb->base);
+	ti_abb_rmw(regs->opp_change_mask, 1, abb->control_reg);
 
 	/* Wait for ABB LDO to complete transition to new Bias setting */
 	ret = ti_abb_wait_txdone(dev, abb);
@@ -490,8 +490,7 @@ static int ti_abb_init_timings(struct device *dev, struct ti_abb *abb)
 	dev_dbg(dev, "%s: Clk_rate=%ld, sr2_cnt=0x%08x\n", __func__,
 		clk_get_rate(abb->clk), sr2_wt_cnt_val);
 
-	ti_abb_rmw(regs->sr2_wtcnt_value_mask, sr2_wt_cnt_val, regs->setup_reg,
-		   abb->base);
+	ti_abb_rmw(regs->sr2_wtcnt_value_mask, sr2_wt_cnt_val, abb->setup_reg);
 
 	return 0;
 }
@@ -648,8 +647,8 @@ static struct regulator_ops ti_abb_reg_ops = {
 /* Default ABB block offsets, IF this changes in future, create new one */
 static const struct ti_abb_reg abb_regs_v1 = {
 	/* WARNING: registers are wrongly documented in TRM */
-	.setup_reg		= 0x04,
-	.control_reg		= 0x00,
+	.setup_off		= 0x04,
+	.control_off		= 0x00,
 
 	.sr2_wtcnt_value_mask	= (0xff << 8),
 	.fbb_sel_mask		= (0x01 << 2),
@@ -661,8 +660,8 @@ static const struct ti_abb_reg abb_regs_v1 = {
 };
 
 static const struct ti_abb_reg abb_regs_v2 = {
-	.setup_reg		= 0x00,
-	.control_reg		= 0x04,
+	.setup_off		= 0x00,
+	.control_off		= 0x04,
 
 	.sr2_wtcnt_value_mask	= (0xff << 8),
 	.fbb_sel_mask		= (0x01 << 2),
@@ -673,9 +672,20 @@ static const struct ti_abb_reg abb_regs_v2 = {
 	.opp_sel_mask		= (0x03 << 0),
 };
 
+static const struct ti_abb_reg abb_regs_generic = {
+	.sr2_wtcnt_value_mask	= (0xff << 8),
+	.fbb_sel_mask		= (0x01 << 2),
+	.rbb_sel_mask		= (0x01 << 1),
+	.sr2_en_mask		= (0x01 << 0),
+
+	.opp_change_mask	= (0x01 << 2),
+	.opp_sel_mask		= (0x03 << 0),
+};
+
 static const struct of_device_id ti_abb_of_match[] = {
 	{.compatible = "ti,abb-v1", .data = &abb_regs_v1},
 	{.compatible = "ti,abb-v2", .data = &abb_regs_v2},
+	{.compatible = "ti,abb-v3", .data = &abb_regs_generic},
 	{ },
 };
 
@@ -722,11 +732,29 @@ static int ti_abb_probe(struct platform_device *pdev)
 	abb->regs = match->data;
 
 	/* Map ABB resources */
-	pname = "base-address";
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
-	abb->base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(abb->base))
-		return PTR_ERR(abb->base);
+	if (abb->regs->setup_off || abb->regs->control_off) {
+		pname = "base-address";
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
+		abb->base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(abb->base))
+			return PTR_ERR(abb->base);
+
+		abb->setup_reg = abb->base + abb->regs->setup_off;
+		abb->control_reg = abb->base + abb->regs->control_off;
+
+	} else {
+		pname = "control-address";
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
+		abb->control_reg = devm_ioremap_resource(dev, res);
+		if (IS_ERR(abb->control_reg))
+			return PTR_ERR(abb->control_reg);
+
+		pname = "setup-address";
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
+		abb->setup_reg = devm_ioremap_resource(dev, res);
+		if (IS_ERR(abb->setup_reg))
+			return PTR_ERR(abb->setup_reg);
+	}
 
 	pname = "int-address";
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
@@ -860,7 +888,7 @@ skip_opt:
 	platform_set_drvdata(pdev, rdev);
 
 	/* Enable the ldo if not already done by bootloader */
-	ti_abb_rmw(abb->regs->sr2_en_mask, 1, abb->regs->setup_reg, abb->base);
+	ti_abb_rmw(abb->regs->sr2_en_mask, 1, abb->setup_reg);
 
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH V4] regulator: ti-abb: Add support for interleaved LDO registers

[Mark Brown]

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

On Thu, Jan 23, 2014 at 11:57:27AM -0600, Nishanth Menon wrote:
> Certain platforms such as DRA7 have quirky memory maps such as:
> PRM_ABBLDO_DSPEVE_CTRL	0x4ae07e20
> PRM_ABBLDO_IVA_CTRL	0x4ae07e24

Applied, thanks.

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