diff mbox series

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

Message ID 2f88fed96d67b05fc033356fdbb7e3227955ab34.1597647359.git.mchehab+huawei@kernel.org (mailing list archive)
State New, archived
Headers show
Series SPMI patches needed by Hikey 970 | expand

Commit Message

Mauro Carvalho Chehab Aug. 17, 2020, 7:11 a.m. UTC
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@kernel.org>
---
 .../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

Comments

Rob Herring (Arm) Aug. 17, 2020, 8:12 p.m. UTC | #1
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@kernel.org>
> ---
>  .../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@kernel.org>
> +
> +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?

> +
> +  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 (which should ideally be converted to schema first). Minimally, 
it needs a better explanation and determination if it should be common 
or is HiSilicon specific.

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

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

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

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

> +
> +          vsel-reg:
> +            description: Voltage selector register.

'reg' can have multiple entries if you want.

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

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.

> +
> +          voltage-table:
> +            description: Table with the selector items for the voltage regulator.
> +            minItems: 2
> +            maxItems: 16

Needs a type $ref.

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

> +
> +examples:
> +  - |
> +    /* pmic properties */
> +
> +    pmic: pmic@0 {
> +      compatible = "hisilicon,hi6421-spmi-pmic";
> +      slave_id = <0>;

Not documented. I believe this is part of 'reg'.

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

> +          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@kernel.org>
> +
> +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.

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

> +
> +      /* pmic properties */
> +
> +    };
> -- 
> 2.26.2
>
Mauro Carvalho Chehab Aug. 18, 2020, 9:13 a.m. UTC | #2
Em Mon, 17 Aug 2020 14:12:11 -0600
Rob Herring <robh@kernel.org> 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@kernel.org>
> > ---
> >  .../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@kernel.org>
> > +
> > +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@kernel.org>
> > +
> > +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
Mauro Carvalho Chehab Aug. 18, 2020, 10:37 a.m. UTC | #3
Em Tue, 18 Aug 2020 11:13:51 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Mon, 17 Aug 2020 14:12:11 -0600
> Rob Herring <robh@kernel.org> escreveu:
> 

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

After another look at the driver, both seems to be fixed
address. There are even some comments that implies that:

	/* SOC_PMIC_IRQ_MASK_0_ADDR */
	ret = of_property_read_u32(np, "irq-mask-addr", &pmic->irq_mask_addr);

	/* SOC_PMIC_IRQ0_ADDR */
	ret = of_property_read_u32(np, "irq-addr", &pmic->irq_addr);

So, I'll just drop the entire set of irq-* properties. The PMIC part
should be just this:

	#include <dt-bindings/spmi/spmi.h>
...
		pmic: pmic@0 {
			compatible = "hisilicon,hi6421v600-spmi";
			reg = <0 SPMI_USID>;

			#interrupt-cells = <2>;
			interrupt-controller;
			gpios = <&gpio28 0 0>;
...
		};



Thanks,
Mauro
Rob Herring (Arm) Aug. 18, 2020, 5:07 p.m. UTC | #4
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@kernel.org> 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@kernel.org>
> > > ---
> > >  .../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@kernel.org>
> > > +
> > > +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@kernel.org>
> > > +
> > > +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
Mauro Carvalho Chehab Aug. 18, 2020, 10:18 p.m. UTC | #5
Em Tue, 18 Aug 2020 11:07:55 -0600
Rob Herring <robh@kernel.org> escreveu:

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

I'm fine with "hisilicon,spmi-channel".

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

I agree, but, as I said before, without the datasheet, we can only
hardcode a small subset of the LDO settings.

Due to that, I prefer keeping it at DT - either grouped together at "reg" or 
as two separated properties (reg and vsel-reg).

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

This SoC is named Kirin 970. Yet, you can see places where 3670 is
used, like:

	https://en.wikichip.org/wiki/hisilicon/kirin/970

There, it says that Hi3670 is the part number.

Thanks,
Mauro
Rob Herring (Arm) Aug. 19, 2020, 8:48 p.m. UTC | #6
On Tue, Aug 18, 2020 at 4:18 PM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Tue, 18 Aug 2020 11:07:55 -0600
> Rob Herring <robh@kernel.org> escreveu:
>
> > > > > +  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.
>
> I'm fine with "hisilicon,spmi-channel".

Humm, QCom has a 'qcom,channel' property for SPMI. Seems like maybe it
should be a common thing.

Rob
diff mbox series

Patch

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@kernel.org>
+
+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
+
+  reg:
+    maxItems: 1
+
+  spmi-channel:
+    description: number of the SPMI channel where the PMIC is connected
+
+  '#interrupt-cells':
+    const: 2
+
+  interrupt-controller:
+    description:
+      Identify that the PMIC is capable of behaving as an interrupt controller.
+
+  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
+
+  regulators:
+    type: object
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      '^ldo@[0-9]+$':
+        type: object
+
+        $ref: "/schemas/regulator/regulator.yaml#"
+
+        properties:
+          reg:
+            description: Enable register.
+
+          '#address-cells':
+            const: 1
+
+          '#size-cells':
+            const: 0
+
+          vsel-reg:
+            description: Voltage selector register.
+
+          enable-mask:
+            description: Bitmask used to enable the regulator.
+
+          voltage-table:
+            description: Table with the selector items for the voltage regulator.
+            minItems: 2
+            maxItems: 16
+
+          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
+
+examples:
+  - |
+    /* pmic properties */
+
+    pmic: pmic@0 {
+      compatible = "hisilicon,hi6421-spmi-pmic";
+      slave_id = <0>;
+      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>;
+          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@kernel.org>
+
+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
+
+  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>;
+
+      /* pmic properties */
+
+    };