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

From: Mauro Carvalho Chehab
Date: Tue Aug 18 2020 - 05:14:00 EST

Next message: Jiri Olsa: "Re: Kernel build error on BTFIDS vmlinux"
Previous message: Xu Wang: "[PATCH] qed_main: Remove unnecessary cast in kfree"
In reply to: Rob Herring: "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 ]

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.

Maybe we use this for compatible:

hisilicon,hi6421v600-spmi

>
> > +
> > + 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.

> (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.

>
> > + 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?

>
> > +
> > + 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

>
> > +
> > + 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: Jiri Olsa: "Re: Kernel build error on BTFIDS vmlinux"
Previous message: Xu Wang: "[PATCH] qed_main: Remove unnecessary cast in kfree"
In reply to: Rob Herring: "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 ]