| Message ID | 1389900750-27625-1-git-send-email-nm@ti.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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?
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 :(.
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 :)
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?
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.
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?
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
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.
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.
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.
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.
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);
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(-)