On Tue, Aug 18, 2020 at 11:13:51AM +0200, Mauro Carvalho Chehab wrote: > Em Mon, 17 Aug 2020 14:12:11 -0600 > Rob Herring <robh@xxxxxxxxxx> escreveu: > > > On Mon, Aug 17, 2020 at 09:11:02AM +0200, Mauro Carvalho Chehab wrote: > > > Add documentation for the properties needed by the HiSilicon > > > 6421v600 driver, and by the SPMI controller used to access > > > the chipset. > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> > > > --- > > > .../mfd/hisilicon,hi6421-spmi-pmic.yaml | 182 ++++++++++++++++++ > > > .../spmi/hisilicon,hisi-spmi-controller.yaml | 54 ++++++ > > > 2 files changed, 236 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml > > > create mode 100644 Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml > > > new file mode 100644 > > > index 000000000000..95494114554d > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml > > > @@ -0,0 +1,182 @@ > > > +# SPDX-License-Identifier: GPL-2.0 > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/mfd/hisilicon,hi6421-spmi-pmic.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: HiSilicon 6421v600 SPMI PMIC > > > + > > > +maintainers: > > > + - Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> > > > + > > > +description: | > > > + HiSilicon 6421v600 uses a MIPI System Power Management (SPMI) bus in order > > > + to provide interrupts and power supply. > > > + > > > + The GPIO and interrupt settings are represented as part of the top-level PMIC > > > + node. > > > + > > > + The SPMI controller part is provided by > > > + Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml. > > > + > > > +properties: > > > + $nodename: > > > + pattern: "pmic@[0-9a-f]" > > > + > > > + compatible: > > > + const: hisilicon,hi6421-spmi-pmic > > > > -spmi-pmic is redundant. Can the hi6421 be anything else? > > There are other HiSilicom 6421 variants that don't use SPMI bus: > > Documentation/devicetree/bindings/mfd/hi6421.txt: "hisilicon,hi6421-pmic"; > Documentation/devicetree/bindings/mfd/hi6421.txt: "hisilicon,hi6421v530-pmic"; > > The DT file on Kernel 4.9 uses hi6421v600 (although the schematics > from 96boards name it as hi6421v610). > > While I don't mind much,would prefer to keep "spmi" on its name, in order > to distinguish this one from the non-spmi variants. Fine, though since probing is bus specific it works fine if the same compatible is used with different buses. Devices with multiple bus options are pretty common. > Maybe we use this for compatible: > > hisilicon,hi6421v600-spmi Okay. > > > + reg: > > > + maxItems: 1 > > > + > > > + spmi-channel: > > > + description: number of the SPMI channel where the PMIC is connected > > > > This looks like a common (to SPMI), but it's not something defined in > > spmi.txt > > This one is not part of the SPMI core. It is stored inside a private > structure inside at the HiSilicon spmi controller driver. It is stored > there as ctrl_dev->channel, and it is used to calculate the register offset > for readl(): > > offset = SPMI_APB_SPMI_STATUS_BASE_ADDR; > offset += SPMI_CHANNEL_OFFSET * ctrl_dev->channel + SPMI_SLAVE_OFFSET * sid; > do { > status = readl(base + offset); > ... > > The SPMI bus is somewhat similar to I2C: it is a 2-wire serial bus > with up to 16 devices connected to it. > > Now, most modern I2C chipsets provide multiple independent I2C > channels, on different pins. Also, on some chipsets, certain > GPIO pins can be used either as GPIO or as I2C. > > I strongly suspect that this is the case here: according with > the Hikey 970 schematics: > > https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf > > The pins used by SPMI clock/data can also be used as GPIO. > > While I don't have access to the datasheets for Kirin 970 (or any other > chipsets on this board), for me, it sounds that different GPIO pins > are allowed to use SPMI. The "spmi-channel" property specifies > what pins will be used for SPMI, among the ones that are > compatible with MIPI SPMI specs. Based on this, I think it should be called 'hisilicon,spmi-channel' as it is vendor specific. > > (which should ideally be converted to schema first). > > I can try porting spmi schema to yaml on a separate patch, > and submit it independently of this series. > > > Minimally, > > it needs a better explanation and determination if it should be common > > or is HiSilicon specific. > > What about: > > spmi-channel: > description: | > number of the SPMI channel at the HiSilicon SoC that will > be used for the MIPI SPMI controller. > > > > > > + > > > + '#interrupt-cells': > > > + const: 2 > > > + > > > + interrupt-controller: > > > + description: > > > + Identify that the PMIC is capable of behaving as an interrupt controller. > > > > No need to redefine common properties if nothing specific to this device > > to say. Just: > > > > interrupt-controller: true > > Ok. > > > > > > + > > > + gpios: > > > + maxItems: 1 > > > + > > > + irq-num: > > > + description: Interrupt request number > > > + > > > + 'irq-array': > > > + description: Interrupt request array > > > + > > > + 'irq-mask-addr': > > > + description: Address for the interrupt request mask > > > + > > > + 'irq-addr': > > > + description: Address for the interrupt request > > > > What's all these non-standard interrupt properties? > > After doing a deeper look at the code which handles IRQs on this PMIC, > I'm considering to get rid of two properties: irq-num and irq-array. > > See, the code does this: > > /* During probe time */ > pmic->irqs = devm_kzalloc(dev, pmic->irqnum * sizeof(int), GFP_KERNEL); > > /* While handling IRQs */ > for (i = 0; i < pmic->irqarray; i++) { > pending = hi6421_spmi_pmic_read(pmic, (i + pmic->irq_addr)); > pending &= 0xff; > > for_each_set_bit(offset, &pending, 8) > generic_handle_irq(pmic->irqs[offset + i * 8]); > > } > > Right now, Hikey 970 sets: > > irq-num = <16>; > irq-array = <2>; > irq-mask-addr = <0x202>; > irq-addr = <0x212>; > > From the above code, it sounds to me that irq-array is the number of > bytes used for IRQ, while irq-num is the number of bits. E. g: > > irq_num = irqarray * 8; > > So, we can get rid of at least one of them. > > Going further, the code provides an special treatment for some IRQs: > > #define HISI_IRQ_KEY_NUM 0 > #define HISI_IRQ_KEY_VALUE 0xc0 > #define HISI_IRQ_KEY_DOWN 7 > #define HISI_IRQ_KEY_UP 6 > > for (i = 0; i < pmic->irqarray; i++) { > pending = hi6421_spmi_pmic_read(pmic, (i + pmic->irq_addr)); > > ... > /* solve powerkey order */ > if ((i == HISI_IRQ_KEY_NUM) && > ((pending & HISI_IRQ_KEY_VALUE) == HISI_IRQ_KEY_VALUE)) { > generic_handle_irq(pmic->irqs[HISI_IRQ_KEY_DOWN]); > generic_handle_irq(pmic->irqs[HISI_IRQ_KEY_UP]); > pending &= (~HISI_IRQ_KEY_VALUE); > } > > As the values for HISI_IRQ_KEY_DOWN and HISI_IRQ_KEY_UP don't > depend on irqarray, it sounds to me that this is actually hardcoded > for irqarray == 2. > > So, I'll just get rid of those, replacing them by some defines inside > the code. If needed later, this patch can always be reverted. > > > > + 'irq-mask-addr': > > > + description: Address for the interrupt request mask > > > + > > > + 'irq-addr': > > > + description: Address for the interrupt request > > Those two seems more standard to me: irq-mask-addr is the address to > enable/disable IRQs, while irq-addr is where the pending IRQs are > stored. > > What would be the standard way to specify them both? > > > > + > > > + regulators: > > > + type: object > > > > additionalProperties: false > > > > > + > > > + properties: > > > + '#address-cells': > > > + const: 1 > > > + > > > + '#size-cells': > > > + const: 0 > > > + > > > + patternProperties: > > > + '^ldo@[0-9]+$': > > > > Unit-addresses are hex. > > > > Also, doesn't match the example. > > True. This should be, instead: > > patternProperties: > '^ldo[0-9]+@[0-9a-f]+$': > > The name part of the property would better to stay in decimal, > as it makes a in order to match the public schematics: > > https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf > > Using decimal values, the dmesg matches the schematics helps a lot when > dealing issues related to PM, as the names of the LDO lines will match > page 12 the schematics: > > ldo3: 1500 <--> 2000 mV at 1800 mV normal > ldo4: 1725 <--> 1900 mV at 1800 mV normal idle > ldo9: 1750 <--> 3300 mV at 2950 mV normal idle > ldo15: 1800 <--> 3000 mV at 2950 mV normal idle > ldo16: 1800 <--> 3000 mV at 2950 mV normal idle > ldo17: 2500 <--> 3300 mV at 2500 mV normal idle > ldo33: 2500 <--> 3300 mV at 2500 mV normal > ldo34: 2600 <--> 3300 mV at 2600 mV normal > ldo4: disabling > ldo33: disabling > > So, from above, looking at the datasheet, it is clear that > ldo33 - e. g. PCIe Switch VDD25 - is disabled. Makes sense. > > > + type: object > > > + > > > + $ref: "/schemas/regulator/regulator.yaml#" > > > + > > > + properties: > > > + reg: > > > + description: Enable register. > > > + > > > > > + '#address-cells': > > > + const: 1 > > > + > > > + '#size-cells': > > > + const: 0 > > > > No child nodes, you don't need these. > > It is needed. However, this is at the second file of the DT. > > See, as SPMI is actually a bus, the entire DT setting has 3 > parts: > - the SPMI controller; > - the PMICs; > - the regulators. > > A complete example is: > > spmi: spmi@fff24000 { > compatible = "hisilicon,spmi-controller"; > #address-cells = <2>; > #size-cells = <0>; > status = "ok"; > reg = <0x0 0xfff24000 0x0 0x1000>; > spmi-channel = <2>; > > pmic: pmic@0 { > compatible = "hisilicon,hi6421-spmi-pmic"; > slave_id = <0>; > reg = <0 SPMI_USID>; > > #interrupt-cells = <2>; > interrupt-controller; > gpios = <&gpio28 0 0>; > irq-mask-addr = <0x202>; > irq-addr = <0x212>; > > regulators { > #address-cells = <1>; > #size-cells = <0>; > > ldo3: ldo3@16 { > reg = <0x16>; > vsel-reg = <0x51>; > > regulator-name = "ldo3"; > regulator-min-microvolt = <1500000>; > regulator-max-microvolt = <2000000>; > regulator-boot-on; > > enable-mask = <0x01>; > > voltage-table = <1500000>, <1550000>, > <1600000>, <1650000>, > <1700000>, <1725000>, > <1750000>, <1775000>, > <1800000>, <1825000>, > <1850000>, <1875000>, > <1900000>, <1925000>, > <1950000>, <2000000>; > off-on-delay-us = <20000>; > startup-delay-us = <120>; > }; > ... > }; > }; > }; > > The child nodes are at the regulator DT properties. > > Well, I can drop those from here, adding them only at the regulator's > part, using "bus { ... };". > > > > + > > > + vsel-reg: > > > + description: Voltage selector register. > > > > 'reg' can have multiple entries if you want. > > Yes, I know. I was in doubt if I should either place vsel-reg on > a separate property or together with reg. I ended keeping it > in separate on the submitted patch series. > > What makes more sense? Really, not putting it in DT. Like other things, it's fixed for the chip. > > > + > > > + enable-mask: > > > + description: Bitmask used to enable the regulator. > > > > But if there's a shared enable reg, then you shouldn't have duplicate > > addresses (same 'reg' value in multiple nodes). > > At least for the LDOs supported on HiKey 970, values for > "reg" and "vsel-reg" are unique: each LDO has their own. > > Right now, enable-mask is 0x01 for all LDOs at the Hikey 970 > DTS. However, only 8 LDOs are currently present at the DTS. From > the schematics, it sounds that HiSilicon 6421v600 supports > at least 37 lines. I've no idea if enable-mask remains the same > for the other ones, nor if "reg" and "vsel-reg" won't be > unique in the general case. > > > These perhaps should be driver data rather than in DT as it's all fixed > > for this chip. We don't try to parameterize everything in DT. > > I considered that. However, I've no idea about the values and > ranges for the other 29 LDOs. So, without knowing better about > this silicon, I prefer to keep those at DT. > > > > > > + > > > + voltage-table: > > > + description: Table with the selector items for the voltage regulator. > > > + minItems: 2 > > > + maxItems: 16 > > > > Needs a type $ref. > > Ok. I'll add: > > $ref: /schemas/types.yaml#/definitions/uint32 > > > > + > > > + off-on-delay-us: > > > + description: Time required for changing state to enabled in microseconds. > > > + > > > + startup-delay-us: > > > + description: Startup time in microseconds. > > > + > > > + idle-mode-mask: > > > + description: Bitmask used to put the regulator on idle mode. > > > + > > > + eco-microamp: > > > + description: Maximum current while on idle mode. > > > + > > > + required: > > > + - reg > > > + - vsel-reg > > > + - enable-mask > > > + - voltage-table > > > + - off-on-delay-us > > > + - startup-delay-us > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - regulators > > > > Add: > > > > additionalProperties: false > > Ok. > > > > > > + > > > +examples: > > > + - | > > > + /* pmic properties */ > > > + > > > + pmic: pmic@0 { > > > + compatible = "hisilicon,hi6421-spmi-pmic"; > > > + slave_id = <0>; > > > > Not documented. I believe this is part of 'reg'. > > Good point. I'll double-check this one, but I guess you're right. > > > > > > + reg = <0 0>; > > > + > > > + #interrupt-cells = <2>; > > > + interrupt-controller; > > > + gpios = <&gpio28 0 0>; > > > + irq-num = <16>; > > > + irq-array = <2>; > > > + irq-mask-addr = <0x202 2>; > > > + irq-addr = <0x212 2>; > > > + > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + regulators { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + ldo3: ldo3@16 { > > > + reg = <0x16>; > > > + vsel-reg = <0x51>; > > > + > > > + regulator-name = "ldo3"; > > > + regulator-min-microvolt = <1500000>; > > > + regulator-max-microvolt = <2000000>; > > > + regulator-boot-on; > > > + > > > + enable-mask = <0x01>; > > > + > > > + voltage-table = <1500000>, <1550000>, <1600000>, <1650000>, > > > + <1700000>, <1725000>, <1750000>, <1775000>, > > > + <1800000>, <1825000>, <1850000>, <1875000>, > > > + <1900000>, <1925000>, <1950000>, <2000000>; > > > + off-on-delay-us = <20000>; > > > + startup-delay-us = <120>; > > > + }; > > > + > > > + ldo4: ldo4@17 { /* 40 PIN */ > > > + reg = <0x17>; > > > + vsel-reg = <0x52>; > > > + > > > + regulator-name = "ldo4"; > > > + regulator-min-microvolt = <1725000>; > > > + regulator-max-microvolt = <1900000>; > > > + regulator-boot-on; > > > + > > > + enable-mask = <0x01>; > > > + idle-mode-mask = <0x10>; > > > + eco-microamp = <10000>; > > > + > > > + hi6421-vsel = <0x52 0x07>; > > > > Not documented. > > This is a left-over. I dropped this one, in favor of "vsel-reg" > (plus a mask for the voltage-table size). > > > > > > + voltage-table = <1725000>, <1750000>, <1775000>, <1800000>, > > > + <1825000>, <1850000>, <1875000>, <1900000>; > > > + off-on-delay-us = <20000>; > > > + startup-delay-us = <120>; > > > + }; > > > + }; > > > + }; > > > diff --git a/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml b/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml > > > new file mode 100644 > > > index 000000000000..5aeb2ae12024 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml > > > @@ -0,0 +1,54 @@ > > > +# SPDX-License-Identifier: GPL-2.0 > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/spmi/hisilicon,hisi-spmi-controller.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: HiSilicon SPMI controller > > > + > > > +maintainers: > > > + - Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> > > > + > > > +description: | > > > + The HiSilicon SPMI controller is found on some Kirin-based designs. > > > + It is a MIPI System Power Management (SPMI) controller. > > > + > > > + The PMIC part is provided by > > > + Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml. > > > + > > > +properties: > > > + $nodename: > > > + pattern: "spmi@[0-9a-f]" > > > + > > > + compatible: > > > + const: hisilicon,spmi-controller > > > > Needs an SoC specific compatible. > > What about: > hisilicon,kirin970-spmi-controller Is 'kirin970' really the SoC name? The older ones are all 'hi[0-9]+'. > > > > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + "#address-cells": > > > + const: 2 > > > + > > > + "#size-cells": > > > + const: 0 > > > + > > > + spmi-channel: > > > + description: number of the SPMI channel where the PMIC is connected > > > + > > > +patternProperties: > > > + "^pmic@[0-9a-f]$": > > > + $ref: "/schemas/mfd/hisilicon,hi6421-spmi-pmic.yaml#" > > > + > > > +examples: > > > + - | > > > + spmi: spmi@fff24000 { > > > + compatible = "hisilicon,spmi-controller"; > > > + #address-cells = <2>; > > > + #size-cells = <0>; > > > + status = "ok"; > > > + reg = <0x0 0xfff24000 0x0 0x1000>; > > > + spmi-channel = <2>; > > > > Does this go in the SPMI controller or child (pmic)? > > Those belong to the SPMI controller. Maybe I did some mess trying to > split up DT in order to place the Kirin970 SPMI bus controller on > one file, and the HiSilicon 6421v600 on another one. > > I ended needing to duplicate some things, as otherwise the DT checks fail. > > Basically, the full DT is: > > spmi: spmi@fff24000 { > /* Kirin 970 SPMI controller props */ > > pmic: pmic@0 { > /* HiSilicon 6421v600 PMIC props */ > > regulators { > ldo3: ldo3@16 { > /* HiSilicon 6421v600 ldo3 regulator props */ > }; > ldo4: ldo3@17 { > /* HiSilicon 6421v600 ldo4 regulator props */ > }; > ... > ldo34: ldo3@33 { > /* HiSilicon 6421v600 ldo34 regulator props */ > }; > }; > }; > }; > > Thanks, > Mauro
