| Message ID | 565CB1CF.5040306@simon.arlott.org.uk |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Mon, Nov 30, 2015 at 08:38:30PM +0000, Simon Arlott wrote: > The BCM63xx has one or more registers with bits that act as regulators > to enable/disable power to individual chip peripherals. 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.
On Mon, Nov 30, 2015 at 08:30:07PM +0000, Simon Arlott wrote: > +- offset: register offset > +- mask: register enable mask > +- startup-delay-us: startup time in microseconds 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.
On Tue, December 1, 2015 22:16, Mark Brown wrote: > On Mon, Nov 30, 2015 at 08:30:07PM +0000, Simon Arlott wrote: > >> +- offset: register offset >> +- mask: register enable mask >> +- startup-delay-us: startup time in microseconds > > 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>; }; };
On Wed, Dec 02, 2015 at 12:45:50PM -0000, Simon Arlott wrote: > On Tue, December 1, 2015 22:16, Mark Brown wrote: > > 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 */ 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. > 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: I wouldn't expect to see it in the device tree at all for a device specific driver.
On 02/12/15 12:53, Mark Brown wrote: > On Wed, Dec 02, 2015 at 12:45:50PM -0000, Simon Arlott wrote: >> On Tue, December 1, 2015 22:16, Mark Brown wrote: > >> > 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 */ > > 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. 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? >> 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: > > I wouldn't expect to see it in the device tree at all for a device > specific driver. If there isn't an individual entry in DT for each regulator, how is it supposed to work? There's no #regulator-cells property.
On Wed, Dec 02, 2015 at 08:26:36PM +0000, Simon Arlott wrote: > On 02/12/15 12:53, Mark Brown wrote: > > 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. > 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? 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. > >> 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: > > I wouldn't expect to see it in the device tree at all for a device > > specific driver. > If there isn't an individual entry in DT for each regulator, how is it > supposed to work? There's no #regulator-cells property. There could be one if it would help, but we do normally manage to do this without - look at how other regulator drivers work.
On 03/12/15 00:06, Mark Brown wrote: > On Wed, Dec 02, 2015 at 08:26:36PM +0000, Simon Arlott wrote: >> On 02/12/15 12:53, Mark Brown wrote: > >> > 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. > >> 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? > > 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 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. > 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. Will you accept a generic driver for a simple enable regulator device on a regmap? What should its compatible string be? > 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. > >> >> 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: > >> > I wouldn't expect to see it in the device tree at all for a device >> > specific driver. > >> If there isn't an individual entry in DT for each regulator, how is it >> supposed to work? There's no #regulator-cells property. > > There could be one if it would help, but we do normally manage to do > this without - look at how other regulator drivers work. 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.
On Thu, Dec 03, 2015 at 08:14:33AM +0000, Simon Arlott wrote: > On 03/12/15 00:06, Mark Brown wrote: > > 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. > Will you accept a generic driver for a simple enable regulator device on > a regmap? What should its compatible string be? Perhaps. I really don't like putting the entire driver into DT, it's not a pattern I want to encourage. > > There could be one if it would help, but we do normally manage to do > > this without - look at how other regulator drivers work. > Several of them have a fixed list of supported regulator names in the Yes, that's the way this is handled. > 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. Nonsense. The names are useful to identify which supply is being referred to. Having voltage control is irrelevant to identifying regulators. > 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. 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).
On 03/12/15 15:05, Mark Brown wrote: > On Thu, Dec 03, 2015 at 08:14:33AM +0000, Simon Arlott wrote: >> On 03/12/15 00:06, Mark Brown wrote: > >> > 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. > >> Will you accept a generic driver for a simple enable regulator device on >> a regmap? What should its compatible string be? > > Perhaps. I really don't like putting the entire driver into DT, it's > not a pattern I want to encourage. 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. >> > There could be one if it would help, but we do normally manage to do >> > this without - look at how other regulator drivers work. > >> Several of them have a fixed list of supported regulator names in the > > Yes, that's the way this is handled. 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. >> 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. > > Nonsense. The names are useful to identify which supply is being > referred to. Having voltage control is irrelevant to identifying > regulators. 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. >> 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. > > 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). Ok, I hadn't considered that regulator names could be in DT without any enable bit information when this was in the driver.
On Thu, Dec 03, 2015 at 11:38:28PM +0000, Simon Arlott wrote: > #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) Are you *sure* these are regulators and not power domains? These names look a lot like they could be power domains.
On 03/12/15 23:45, Mark Brown wrote: > On Thu, Dec 03, 2015 at 11:38:28PM +0000, Simon Arlott wrote: > >> #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) > > 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).
On Thu, Dec 03, 2015 at 11:51:16PM +0000, Simon Arlott wrote: > On 03/12/15 23:45, Mark Brown wrote: > > 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). 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?
On Fri, December 4, 2015 11:00, Mark Brown wrote: > On Thu, Dec 03, 2015 at 11:51:16PM +0000, Simon Arlott wrote: >> On 03/12/15 23:45, Mark Brown wrote: > >> > 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). > > 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.
On Fri, Dec 04, 2015 at 12:26:58PM -0000, Simon Arlott wrote: > On Fri, December 4, 2015 11:00, Mark Brown wrote: > > 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. OK, great - runtime PM is just generally a more idiomatic interface for this.
diff --git a/Documentation/devicetree/bindings/regulator/brcm,bcm63xx-regulator.txt b/Documentation/devicetree/bindings/regulator/brcm,bcm63xx-regulator.txt new file mode 100644 index 0000000..e905241 --- /dev/null +++ b/Documentation/devicetree/bindings/regulator/brcm,bcm63xx-regulator.txt @@ -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>; +};
The BCM63xx has one or more registers with bits that act as regulators to enable/disable power to individual chip peripherals. Signed-off-by: Simon Arlott <simon@fire.lp0.eu> --- .../bindings/regulator/brcm,bcm63xx-regulator.txt | 33 ++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 Documentation/devicetree/bindings/regulator/brcm,bcm63xx-regulator.txt