regulator: Add regmap support to regulator-fixed device tree binding

Add properties for regmap to the regulator-fixed device tree binding:
* Reference the regmap phandle
* Specify the enable offset and enable mask
* Reuse enable-active-high for regmap

This is required for Broadcom BCM63xx SoCs that enable power to individual peripherals by clearing a bit in the miscIddqCtrl register.

---
 .../bindings/regulator/fixed-regulator.txt          | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

index 4fae41d..b499a65 100644
@@ -5,13 +5,18 @@ Required properties:
 
 Optional properties:
 - gpio: gpio to use for enable control
+- regmap: regmap phandle to use for enable control
+- regmap-offset: register offset when using regmap for enable control
+- regmap-mask: register enable mask when using regmap for enable control
 - startup-delay-us: startup time in microseconds
-- enable-active-high: Polarity of GPIO is Active high
+- enable-active-high: Polarity of GPIO/regmap is Active high
 If this property is missing, the default assumed is Active low.
 - gpio-open-drain: GPIO is open drain type.
   If this property is missing then default assumption is false.
 -vin-supply: Input supply name.
 
+Only one of gpio/regmap should be specified.
+
 Any property defined as part of the core regulator
 binding, defined in regulator.txt, can also be used.
 However a fixed voltage regulator is expected to have the

@@ -32,3 +37,17 @@ Example:
 		gpio-open-drain;
 		vin-supply = <&parent_reg>;
 	};
+
+	xyz: fixedregulator@1 {
+		compatible = "regulator-fixed";
+		regulator-name = "fixed-supply";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		regmap = <&syscon>;
+		regmap-offset = <0x4>;
+		regmap-mask = <0x200>;
+		startup-delay-us = <70000>;
+		enable-active-high;
+		regulator-boot-on;
+		vin-supply = <&parent_reg>;
+	};

2.1.4

-- 
Simon Arlott

Use the device tree properties for regmap:
* Lookup the regmap phandle
* Use the enable offset and enable mask
* Reuse enable-active-high value

Use an alternative set of regulator_ops when the regmap is set, specifying the regmap helper functions.

As syscon_regmap_lookup_by_phandle() can only return -ENODEV or -ENOSYS as errors and -EPROBE_DEFER doesn't make sense for a register, the specific error is never propagated.

This is required for Broadcom BCM63xx SoCs that enable power to individual peripherals by clearing a bit in the miscIddqCtrl register.

---
 drivers/regulator/fixed.c       | 30 +++++++++++++++++++++++++++++-
 include/linux/regulator/fixed.h |  9 ++++++++-
 2 files changed, 37 insertions(+), 2 deletions(-)

index ff62d69..5a3fa44 100644
@@ -30,6 +30,7 @@
 #include <linux/of_gpio.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/machine.h>
+#include <linux/mfd/syscon.h>
 
 struct fixed_voltage_data {
 	struct regulator_desc desc;

@@ -92,6 +93,19 @@ of_get_fixed_voltage_config(struct device *dev,
 	if ((config->gpio == -ENODEV) || (config->gpio == -EPROBE_DEFER))
 		return ERR_PTR(-EPROBE_DEFER);
 
+	config->regmap = syscon_regmap_lookup_by_phandle(np, "regmap");
+	if (!IS_ERR(config->regmap)) {
+		u32 val;
+
+		if (of_property_read_u32(np, "regmap-offset", &val))
+			return ERR_PTR(-EINVAL);
+		config->enable_reg = val;
+
+		if (of_property_read_u32(np, "regmap-mask", &val))
+			return ERR_PTR(-EINVAL);
+		config->enable_mask = val;
+	}
+
 	of_property_read_u32(np, "startup-delay-us", &config->startup_delay);
 
 	config->enable_high = of_property_read_bool(np, "enable-active-high");

@@ -107,6 +121,12 @@ of_get_fixed_voltage_config(struct device *dev,
 static struct regulator_ops fixed_voltage_ops = {
 };
 
+static struct regulator_ops fixed_voltage_regmap_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+};
+
 static int reg_fixed_voltage_probe(struct platform_device *pdev)
 {
 	struct fixed_voltage_config *config;

@@ -140,7 +160,15 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 	}
 	drvdata->desc.type = REGULATOR_VOLTAGE;
 	drvdata->desc.owner = THIS_MODULE;
-	drvdata->desc.ops = &fixed_voltage_ops;
+	if (!IS_ERR_OR_NULL(config->regmap)) {
+		drvdata->desc.ops = &fixed_voltage_regmap_ops;
+		cfg.regmap = config->regmap;
+		drvdata->desc.enable_reg = config->enable_reg;
+		drvdata->desc.enable_mask = config->enable_mask;
+		drvdata->desc.enable_is_inverted = !config->enable_high;
+	} else {
+		drvdata->desc.ops = &fixed_voltage_ops;
+	}
 
 	drvdata->desc.enable_time = config->startup_delay;
 

index 48918be..7cec0d1 100644
@@ -26,6 +26,10 @@ struct regulator_init_data;
  * @microvolts:		Output voltage of regulator
  * @gpio:		GPIO to use for enable control
  * 			set to -EINVAL if not used
+ * @regmap:		Regmap to use for enable control
+ *			set to -ENODEV if not used
+ * @enable_reg:		Register for control when using regmap
+ * @enable_mask:	Mask for control when using regmap
  * @startup_delay:	Start-up time in microseconds
  * @gpio_is_open_drain: Gpio pin is open drain or normal type.
  *			If it is open drain type then HIGH will be set

@@ -33,7 +37,7 @@ struct regulator_init_data;
  *			and low will be set as gpio-output with driven
  *			to low. For non-open-drain case, the gpio will
  *			will be in output and drive to low/high accordingly.
- * @enable_high:	Polarity of enable GPIO
+ * @enable_high:	Polarity of enable GPIO/regmap
  *			1 = Active high, 0 = Active low
  * @enabled_at_boot:	Whether regulator has been enabled at
  * 			boot or not. 1 = Yes, 0 = No

@@ -50,6 +54,9 @@ struct fixed_voltage_config {
 	const char *input_supply;
 	int microvolts;
 	int gpio;
+	struct regmap *regmap;
+	unsigned int enable_reg;
+	unsigned int enable_mask;
 	unsigned startup_delay;
 	unsigned gpio_is_open_drain:1;
 	unsigned enable_high:1;

2.1.4

-- 
Simon Arlott
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Just write a driver for this, the fixed driver is really intended for things like discrete regulators and if we were going to do a generic regmap driver (which I'm not terribly keen on as it means we're not able to later add additional information or custom handling in the driver) we should just do that rather than shoehorning things into the fixed driver.

The BCM63xx has one or more registers with bits that act as regulators to enable/disable power to individual chip peripherals.

---
 .../bindings/regulator/brcm,bcm63xx-regulator.txt  | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/brcm,bcm63xx-regulator.txt

new file mode 100644
index 0000000..e905241
@@ -0,0 +1,33 @@
+BCM63xx regulators
+
+The BCM63xx has one or more registers with bits that act as regulators
+to enable/disable power to individual chip peripherals.
+
+Required properties:
+- compatible: Must be "brcm,bcm<soc>-regulator", "brcm,bcm63xx-regulator";
+- regmap: regmap phandle to use for enable control
+- offset: register offset
+- mask: register enable mask
+- startup-delay-us: startup time in microseconds
+
+Any property defined as part of the core regulator
+binding, defined in regulator.txt, can also be used.
+
+However the regulator is expected to have the same values
+for regulator-min-microvolt and regulator-max-microvolt.
+
+Example:
+
+misc: syscon@10001800 {
+	compatible = "syscon";
+	reg = <0x10001800 0xd0>;
+};
+
+robosw_power: robosw {
+	compatible = "brcm,bcm63168-regulator", "brcm,bcm63xx-regulator";
+	regulator-name = "robosw_power";
+	regmap = <&misc>;
+	offset = <0x4c>;
+	mask = <0x40>;
+	startup-delay-us = <100000>;
+};

2.1.4

-- 
Simon Arlott

The BCM63xx has one or more registers with bits that act as regulators to enable/disable power to individual chip peripherals.

---
 MAINTAINERS                           |   1 +
 drivers/regulator/Kconfig             |   9 +++
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/bcm63xx-regulator.c | 125 ++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+)
 create mode 100644 drivers/regulator/bcm63xx-regulator.c

index a381176..29e0589 100644
@@ -2376,6 +2376,7 @@ F:	arch/mips/boot/dts/brcm/bcm*.dts*
 F:	drivers/irqchip/irq-bcm63*
 F:	drivers/irqchip/irq-bcm7*
 F:	drivers/irqchip/irq-brcmstb*
+F:	drivers/regulator/bcm63*
 F:	include/linux/bcm63xx_wdt.h
 
 BROADCOM TG3 GIGABIT ETHERNET DRIVER

index 8df0b0e..8a2d5db 100644
@@ -146,6 +146,15 @@ config REGULATOR_AXP20X
 	  This driver provides support for the voltage regulators on the
 	  AXP20X PMIC.
 
+config REGULATOR_BCM63XX
+	tristate "Broadcom BCM63xx Regulators"
+	depends on OF
+	depends on MFD_SYSCON
+	depends on BMIPS_GENERIC
+	help
+	  This driver provides support for the fixed regulators on the
+	  BCM63xx SoCs.
+
 config REGULATOR_BCM590XX
 	tristate "Broadcom BCM590xx PMU Regulators"
 	depends on MFD_BCM590XX

index 0f81749..23e8a4b 100644
@@ -21,6 +21,7 @@ obj-$(CONFIG_REGULATOR_ARIZONA) += arizona-micsupp.o arizona-ldo1.o
 obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
 obj-$(CONFIG_REGULATOR_AS3722) += as3722-regulator.o
 obj-$(CONFIG_REGULATOR_AXP20X) += axp20x-regulator.o
+obj-$(CONFIG_REGULATOR_BCM63XX) += bcm63xx-regulator.o
 obj-$(CONFIG_REGULATOR_BCM590XX) += bcm590xx-regulator.o
 obj-$(CONFIG_REGULATOR_DA903X)	+= da903x.o
 obj-$(CONFIG_REGULATOR_DA9052)	+= da9052-regulator.o

2.1.4

-- 
Simon Arlott

The BCM63xx has one or more registers with bits that act as regulators to enable/disable power to individual chip peripherals.

---

 MAINTAINERS                           |   1 +
 drivers/regulator/Kconfig             |   9 +++
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/bcm63xx-regulator.c | 125 ++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+)
 create mode 100644 drivers/regulator/bcm63xx-regulator.c

index a381176..29e0589 100644
@@ -2376,6 +2376,7 @@ F:	arch/mips/boot/dts/brcm/bcm*.dts*
 F:	drivers/irqchip/irq-bcm63*
 F:	drivers/irqchip/irq-bcm7*
 F:	drivers/irqchip/irq-brcmstb*
+F:	drivers/regulator/bcm63*
 F:	include/linux/bcm63xx_wdt.h
 
 BROADCOM TG3 GIGABIT ETHERNET DRIVER

index 8df0b0e..8a2d5db 100644
@@ -146,6 +146,15 @@ config REGULATOR_AXP20X
 	  This driver provides support for the voltage regulators on the
 	  AXP20X PMIC.
 
+config REGULATOR_BCM63XX
+	tristate "Broadcom BCM63xx Regulators"
+	depends on OF
+	depends on MFD_SYSCON
+	depends on BMIPS_GENERIC
+	help
+	  This driver provides support for the fixed regulators on the
+	  BCM63xx SoCs.
+
 config REGULATOR_BCM590XX
 	tristate "Broadcom BCM590xx PMU Regulators"
 	depends on MFD_BCM590XX

index 0f81749..23e8a4b 100644
@@ -21,6 +21,7 @@ obj-$(CONFIG_REGULATOR_ARIZONA) += arizona-micsupp.o arizona-ldo1.o
 obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
 obj-$(CONFIG_REGULATOR_AS3722) += as3722-regulator.o
 obj-$(CONFIG_REGULATOR_AXP20X) += axp20x-regulator.o
+obj-$(CONFIG_REGULATOR_BCM63XX) += bcm63xx-regulator.o
 obj-$(CONFIG_REGULATOR_BCM590XX) += bcm590xx-regulator.o
 obj-$(CONFIG_REGULATOR_DA903X)	+= da903x.o
 obj-$(CONFIG_REGULATOR_DA9052)	+= da9052-regulator.o

2.1.4

-- 
Simon Arlott

Please don't submit new revisions of single patches in followup to an existing series, it just makes everything more confusing. If you need to update a series please just post an update to the series.

Why are these in the DT, I would expect that if this is a driver for a specific SoC all these properties would be known as a result of that.

This is a driver for multiple SoCs with the same regulator control in different places on different SoCs, so the location of it within the misc register needs to be provided in the DT:

BCM6362:
#define MISC_BASE 0xb0001800 /* Miscellaneous Registers */ uint32 miscIddqCtrl; /* 0x48 */

BCM63268/BCM63168:
#define MISC_BASE 0xb0001800 /* Miscellaneous Registers */ uint32 miscIddqCtrl; /* 0x4c */

I'll remove the startup-delay-us property and put a default in the driver.

The mask is used as there's one bit per regulator in the register, but there's more than one way to express this in the DT:

It currently looks like this:

  regulators {
    compatible = "simple-bus";
    #address-cells = <0>;
    #size-cells = <0>;

    sar {
      compatible = "brcm,bcm63168-regulator", "brcm,bcm6345-regulator";
      regulator-name = "sar_power";
      regmap = <&misc>;
      offset = <0x4c>;
      mask = <0x1>;
      startup-delay-us = <100000>;
    };

    ipsec {
      compatible = "brcm,bcm63168-regulator", "brcm,bcm6345-regulator";
      regulator-name = "ipsec_power";
      regmap = <&misc>;
      offset = <0x4c>;
      mask = <0x2>;
      startup-delay-us = <100000>;
    };

    ...

};

  ubus {
    compatible = "brcm,ubus", "simple-bus";
    #address-cells = <1>;
    #size-cells = <1>;
    ranges;

    misc: syscon@10001800 {
      compatible = "syscon";
      reg = <0x10001800 0xd0>;
    };

};

Would this be more appropriate?

  regulators {
    compatible = "simple-bus";
    #address-cells = <0>;
    #size-cells = <0>;

    misc_iddq_ctrl {
      compatible = "brcm,bcm63168-regulator", "brcm,bcm6345-regulator";
      regmap = <&misc>;
      offset = <0x4c>;
      #address-cells = <1>;
      #size-cells = <0>;

      sar@0 {
        regulator-name = "sar_power";
      };

      ipsec@1 {
        regulator-name = "ipsec_power";
      };

      ...
    };

};

  ubus {
    compatible = "brcm,ubus", "simple-bus";
    #address-cells = <1>;
    #size-cells = <1>;
    ranges;

    misc: syscon@10001800 {
      compatible = "syscon";
      reg = <0x10001800 0xd0>;
    };

};

Simon Arlott

This is the sort of thing you can pick up from the SoC compatible strings. As things stand there is zero content in this driver that relates to this SoC.

I wouldn't expect to see it in the device tree at all for a device specific driver.

There's always going to be very little content in the driver that relates to this SoC, given that a single bit flip enables/disables power.

All other device tree drivers allow a register address to be specified for the device, how is an offset in the regmap any different?

If there isn't an individual entry in DT for each regulator, how is it supposed to work? There's no #regulator-cells property.

Simon Arlott
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

It's not that it's an offset in regmap, it's that you're trying to make a device driver with literally *no* content that is specific to the actual device. If you're making a driver for a specific device like this it should know at least something about how to control the device from the compatible string. If you're making a generic driver it should not make reference to specific devices.

In general I would prefer to have a driver with a SoC specific compatible string and the data tables in the kernel, that way we keep the device trees stable and our ABI more robust.

There could be one if it would help, but we do normally manage to do this without - look at how other regulator drivers work.

The device that only has a single register, I don't want to make it specific to the device by hard coding a register address in the driver that changes when the same device is used on different SoCs.

Will you accept a generic driver for a simple enable regulator device on a regmap? What should its compatible string be?

Several of them have a fixed list of supported regulator names in the driver. The regulator names for this device are meaningless to the driver because all of the regulators look the same. They don't have a known or controllable voltage and can only be turned on or off.

Any table mapping regulator names to bits in the register would be different for each SoC making the list of regulator names in the device tree redundant. If they're not listed in the device tree then I can't use them as a phandle for other devices.

Simon Arlott
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Perhaps. I really don't like putting the entire driver into DT, it's not a pattern I want to encourage.

Yes, that's the way this is handled.

Nonsense. The names are useful to identify which supply is being referred to. Having voltage control is irrelevant to identifying regulators.

The list of regulator nodes in device tree is not redundant, it is as you say used to connect things together. The question is where to put the control data for those regulators (in this case the enable time and the switch).

I don't like putting the list of which bit is which regulator name into a driver because it means that new hardware that adds a regulator or moves them around requires a change to the driver.

Documentation/devicetree/usage-model.txt:
2.1 High Level View
-------------------
[...] Using
it allows board and device support to become data driven; to make setup decisions based on data passed into the kernel instead of on per-machine hard coded selections.

I agree that an entire driver shouldn't be put in the DT (especially given that you then need the DT to debug any issue), but this isn't much of a driver when it involves flipping a single bit.

The key thing would be to avoid it growing into something too complex (e.g. set register X to 42 to enable and set register Y to 90 to disable, would not belong in DT).

Should I be looking at trying to use generic_pm_domain instead of a regulator? Those don't appear to require a name so a phandle with an argument for the bit would work.

I see two variants of the hardware bits (three if you consider that the GMAC isn't on every SoC):

#define MISC_IDDQ_CTRL_GMAC         (1<<18)
#define MISC_IDDQ_CTRL_WLAN_PADS    (1<<13)
#define MISC_IDDQ_CTRL_PCIE         (1<<12)
#define MISC_IDDQ_CTRL_FAP          (1<<11)
#define MISC_IDDQ_CTRL_VDSL_MIPS    (1<<10)
#define MISC_IDDQ_CTRL_VDSL_PHY     (1<<9)
#define MISC_IDDQ_CTRL_PERIPH       (1<<8)
#define MISC_IDDQ_CTRL_PCM          (1<<7)
#define MISC_IDDQ_CTRL_ROBOSW       (1<<6)
#define MISC_IDDQ_CTRL_USBD         (1<<5)
#define MISC_IDDQ_CTRL_USBH         (1<<4)
#define MISC_IDDQ_CTRL_DECT         (1<<3)
#define MISC_IDDQ_CTRL_MIPS         (1<<2)
#define MISC_IDDQ_CTRL_IPSEC        (1<<1)
#define MISC_IDDQ_CTRL_SAR          (1<<0)

#define MISC_IDDQ_CTRL_EPHY         (1<<9)
#define MISC_IDDQ_CTRL_ROBOSW       (1<<8)
#define MISC_IDDQ_CTRL_PCIE         (1<<7)
#define MISC_IDDQ_CTRL_USBH         (1<<6)
#define MISC_IDDQ_CTRL_USBD         (1<<5)
#define MISC_IDDQ_CTRL_PCM          (1<<4)
#define MISC_IDDQ_CTRL_SAR          (1<<3)
#define MISC_IDDQ_CTRL_ADSL2_AFE    (1<<2)
#define MISC_IDDQ_CTRL_ADSL2_PHY    (1<<1)
#define MISC_IDDQ_CTRL_ADSL2_MIPS   (1<<0)

I could put these lists of bits in a driver (associated with the names) but I'd strongly prefer to put the list of bits in the DT.

If they are in the driver, then there is nothing to stop someone from deciding to force it to fit a different new piece of hardware by deliberately using the wrong names in the DT just because they match the bits they want to use, and they think they can't wait for a new list of bits to be added to the kernel.

e.g. someone adds an LTE chip and reuses the obsolete DECT regulator:

misc_iddq_ctrl@42 {
compatible = "...-regulator";
reg = <0x42>;

  regulators {
    lte_power: dect {
      regulator-name = "LTE";
    };

};
};

Associating the bits with the names in the DT avoids this problem at the expense of having a very generic driver.

A lot of the existing regulator drivers have mostly numbered regulator pins which could have been connected to anything on a hardware variant.

Ok, but from the driver's point of view there's nothing different about any of the regulators on this hardware. I could have numbered them "BIT0" to "BIT31" in the driver and used a meaningful alias in DT.

Ok, I hadn't considered that regulator names could be in DT without any enable bit information when this was in the driver.

Simon Arlott
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Are you *sure* these are regulators and not power domains? These names look a lot like they could be power domains.

No, I'm not sure. Some of them are may actually be regulators (the "PHY" ones) while others are almost definitely power domains (like the "FAP" Forwarding Assist Processor).

Simon Arlott

OK, so the power domains should be being represented and managed as such rather than using regulators - it's a better fit (doing things like support atomic context) and it also sidesteps this. For the things that you say are clearly regulators should we have more information about those?

I'd prefer to handle them all as power domains since it fits better. Even if some of them are regulators there's no extra control or status interface and they're used like power domains to disable unused functionality.

Simon Arlott

OK, great - runtime PM is just generally a more idiomatic interface for this.