Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties

From: Rob Herring
Date: Tue Aug 18 2020 - 13:08:09 EST

Next message: Mark Brown: "Re: [PATCH v3] ASoC: hdac_hdmi: support 'ELD' mixer"
Previous message: Jonathan Cameron: "[PATCH v8 6/6] docs: mm: numaperf.rst Add brief description for access class 1."
In reply to: Mauro Carvalho Chehab: "Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties"
Next in thread: Mauro Carvalho Chehab: "Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Mark Brown: "Re: [PATCH v3] ASoC: hdac_hdmi: support 'ELD' mixer"
Previous message: Jonathan Cameron: "[PATCH v8 6/6] docs: mm: numaperf.rst Add brief description for access class 1."
In reply to: Mauro Carvalho Chehab: "Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties"
Next in thread: Mauro Carvalho Chehab: "Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]