* [PATCH v3 00/44] SPMI patches needed by Hikey 970
@ 2020-08-17 7:10 Mauro Carvalho Chehab
2020-08-17 7:11 ` [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties Mauro Carvalho Chehab
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2020-08-17 7:10 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Rob Herring,
devicetree, Lee Jones, David S. Miller, linux-arm-kernel,
Rob Herring, linux-arm-msm, Wei Xu, devel, linux-kernel,
Stephen Boyd
Hi Greg,
This patch series is part of a work I'm doing in order to be able to support
a HiKey 970 board that I recently got on my hands.
Regards,
Mauro.
v1:
submitted to drivers/{mfd,spmi,regulator}
v2:
- submitted via staging, mainly to preserve original
authorship and his SoB;
- as requested on previous review, patches were split per
target subsystem.
v3:
- Some coding style changes, due to reviews from Joe Perches
Patches were tested on a Hikey 970 board.
Mauro Carvalho Chehab (41):
staging: spmi: hisi-spmi-controller: coding style fixup
staging: spmi: hisi-spmi-controller: fix it to probe successfully
staging: spmi: hisi-spmi-controller: fix a typo
staging: spmi: hisi-spmi-controller: adjust whitespaces at defines
staging: spmi: hisi-spmi-controller: use le32 macros where needed
staging: spmi: hisi-spmi-controller: add debug when values are
read/write
staging: spmi: hisi-spmi-controller: fix the dev_foo() logic
staging: spmi: hisi-spmi-controller: add it to the building system
staging: spmi: hisi-spmi-controller: do some code cleanups
staging: mfd: hi6421-spmi-pmic: get rid of unused code
staging: mfd: hi6421-spmi-pmic: deal with non-static functions
staging: mfd: hi6421-spmi-pmic: get rid of the static vars
staging: mfd: hi6421-spmi-pmic: cleanup hi6421-spmi-pmic.h header
staging: mfd: hi6421-spmi-pmic: change the binding logic
staging: mfd: hi6421-spmi-pmic: get rid of unused OF properties
staging: mfd: hi6421-spmi-pmic: cleanup OF properties
staging: mfd: hi6421-spmi-pmic: change namespace on its functions
staging: mfd: hi6421-spmi-pmic: fix some coding style issues
staging: mfd: hi6421-spmi-pmic: add it to the building system
staging: mfd: hi6421-spmi-pmic: cleanup the code
staging: regulator: hi6421v600-regulator: get rid of unused code
staging: regulator: hi6421v600-regulator: port it to upstream
staging: regulator: hi6421v600-regulator: coding style fixups
staging: regulator: hi6421v600-regulator: change the binding logic
staging: regulator: hi6421v600-regulator: cleanup struct
hisi_regulator
staging: regulator: hi6421v600-regulator: cleanup debug messages
staging: regulator: hi6421v600-regulator: use shorter names for OF
properties
staging: regulator: hi6421v600-regulator: better handle modes
staging: regulator: hi6421v600-regulator: change namespace
staging: regulator: hi6421v600-regulator: convert to use get/set
voltage_sel
staging: regulator: hi6421v600-regulator: don't use usleep_range for
off_on_delay
staging: regulator: hi6421v600-regulator: add a driver-specific debug
macro
staging: regulator: hi6421v600-regulator: initialize ramp_delay
staging: regulator: hi6421v600-regulator: cleanup DT settings
staging: regulator: hi6421v600-regulator: fix some coding style issues
staging: regulator: hi6421v600-regulator: add it to the building
system
staging: regulator: hi6421v600-regulator: code cleanup
staging: hikey9xx: add a TODO list
MAINTAINERS: add an entry for HiSilicon 6421v600 drivers
dt: document HiSilicon SPMI controller and mfd/regulator properties
dt: hisilicon: add support for the PMIC found on Hikey 970
Mayulong (3):
staging: spmi: add Hikey 970 SPMI controller driver
staging: mfd: add a PMIC driver for HiSilicon 6421 SPMI version
staging: regulator: add a regulator driver for HiSilicon 6421v600 SPMI
PMIC
.../mfd/hisilicon,hi6421-spmi-pmic.yaml | 182 +++++++
.../spmi/hisilicon,hisi-spmi-controller.yaml | 54 ++
MAINTAINERS | 6 +
.../boot/dts/hisilicon/hi3670-hikey970.dts | 22 +-
.../boot/dts/hisilicon/hikey970-pmic.dtsi | 200 ++++++++
drivers/staging/Kconfig | 2 +
drivers/staging/Makefile | 1 +
drivers/staging/hikey9xx/Kconfig | 35 ++
drivers/staging/hikey9xx/Makefile | 5 +
drivers/staging/hikey9xx/TODO | 5 +
drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 382 ++++++++++++++
.../staging/hikey9xx/hi6421v600-regulator.c | 479 ++++++++++++++++++
.../staging/hikey9xx/hisi-spmi-controller.c | 356 +++++++++++++
include/linux/mfd/hi6421-spmi-pmic.h | 68 +++
14 files changed, 1778 insertions(+), 19 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
create mode 100644 Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml
create mode 100644 arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
create mode 100644 drivers/staging/hikey9xx/Kconfig
create mode 100644 drivers/staging/hikey9xx/Makefile
create mode 100644 drivers/staging/hikey9xx/TODO
create mode 100644 drivers/staging/hikey9xx/hi6421-spmi-pmic.c
create mode 100644 drivers/staging/hikey9xx/hi6421v600-regulator.c
create mode 100644 drivers/staging/hikey9xx/hisi-spmi-controller.c
create mode 100644 include/linux/mfd/hi6421-spmi-pmic.h
--
2.26.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties
2020-08-17 7:10 [PATCH v3 00/44] SPMI patches needed by Hikey 970 Mauro Carvalho Chehab
@ 2020-08-17 7:11 ` Mauro Carvalho Chehab
2020-08-17 20:12 ` Rob Herring
2020-08-18 11:10 ` [PATCH v3.1 " Mauro Carvalho Chehab
2020-08-17 7:32 ` [PATCH v3 00/44] SPMI patches needed by Hikey 970 Greg Kroah-Hartman
2020-08-18 14:17 ` Greg Kroah-Hartman
2 siblings, 2 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2020-08-17 7:11 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Lee Jones,
Rob Herring, Stephen Boyd, devicetree, linux-kernel,
linux-arm-msm
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
+
+ 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 */
+
+ };
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 00/44] SPMI patches needed by Hikey 970
2020-08-17 7:10 [PATCH v3 00/44] SPMI patches needed by Hikey 970 Mauro Carvalho Chehab
2020-08-17 7:11 ` [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties Mauro Carvalho Chehab
@ 2020-08-17 7:32 ` Greg Kroah-Hartman
2020-08-18 14:17 ` Greg Kroah-Hartman
2 siblings, 0 replies; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-08-17 7:32 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: devel, devicetree, Rob Herring, linux-arm-msm, linuxarm, Wei Xu,
linux-kernel, Stephen Boyd, Rob Herring, mauro.chehab, Lee Jones,
David S. Miller, linux-arm-kernel
On Mon, Aug 17, 2020 at 09:10:19AM +0200, Mauro Carvalho Chehab wrote:
> Hi Greg,
>
> This patch series is part of a work I'm doing in order to be able to support
> a HiKey 970 board that I recently got on my hands.
Do you feel this is good enough for me to add to my tree now? Or do you
want me to wait a bit?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties
2020-08-17 7:11 ` [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties Mauro Carvalho Chehab
@ 2020-08-17 20:12 ` Rob Herring
2020-08-18 9:13 ` Mauro Carvalho Chehab
2020-08-18 11:10 ` [PATCH v3.1 " Mauro Carvalho Chehab
1 sibling, 1 reply; 13+ messages in thread
From: Rob Herring @ 2020-08-17 20:12 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Greg Kroah-Hartman, linuxarm, mauro.chehab, Lee Jones,
Stephen Boyd, devicetree, linux-kernel, linux-arm-msm
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties
2020-08-17 20:12 ` Rob Herring
@ 2020-08-18 9:13 ` Mauro Carvalho Chehab
2020-08-18 10:37 ` Mauro Carvalho Chehab
2020-08-18 17:07 ` Rob Herring
0 siblings, 2 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2020-08-18 9:13 UTC (permalink / raw)
To: Rob Herring
Cc: Greg Kroah-Hartman, linuxarm, mauro.chehab, Lee Jones,
Stephen Boyd, devicetree, linux-kernel, linux-arm-msm
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties
2020-08-18 9:13 ` Mauro Carvalho Chehab
@ 2020-08-18 10:37 ` Mauro Carvalho Chehab
2020-08-18 17:07 ` Rob Herring
1 sibling, 0 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2020-08-18 10:37 UTC (permalink / raw)
To: Rob Herring
Cc: Greg Kroah-Hartman, linuxarm, mauro.chehab, Lee Jones,
Stephen Boyd, devicetree, linux-kernel, linux-arm-msm
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3.1 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties
2020-08-17 7:11 ` [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties Mauro Carvalho Chehab
2020-08-17 20:12 ` Rob Herring
@ 2020-08-18 11:10 ` Mauro Carvalho Chehab
2020-08-18 14:19 ` Greg Kroah-Hartman
1 sibling, 1 reply; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2020-08-18 11:10 UTC (permalink / raw)
To: Rob Herring
Cc: Greg Kroah-Hartman, linuxarm, mauro.chehab, Lee Jones,
Stephen Boyd, devicetree, linux-kernel, linux-arm-msm
From e464ec2c38c083403b556e60f189ee8ae2f2c9c6 Mon Sep 17 00:00:00 2001
From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Date: Fri, 31 Jul 2020 09:46:02 +0200
Subject: [PATCH] dt: document HiSilicon SPMI controller and mfd/regulator
properties
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>
---
v3.1:
- Changed the DT properties to better match upstream requirements
PS.: I opted to submit just this patch, instead of the entire
series, in order to avoid flooding people's ML.
I'll be posting the full series again after DT specs match
upstream requirements.
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..881bbd83df65
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
@@ -0,0 +1,159 @@
+# 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 should be connected inside a MIPI System Power Management
+ (SPMI) bus. It provides 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,hi6421v600-spmi
+
+ reg:
+ maxItems: 1
+
+ '#interrupt-cells':
+ const: 2
+
+ interrupt-controller:
+ description:
+ Identify that the PMIC is capable of behaving as an interrupt controller.
+
+ gpios:
+ maxItems: 1
+
+ regulators:
+ type: object
+
+ properties:
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+ patternProperties:
+ '^ldo[0-9]+@[0-9a-f]$':
+ 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";
+ reg = <0 0>;
+
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ gpios = <&gpio28 0 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..b1cfa9c3aca6
--- /dev/null
+++ b/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml
@@ -0,0 +1,62 @@
+# 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 BUS 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,kirin970-spmi-controller
+
+ reg:
+ maxItems: 1
+
+ spmi-channel:
+ description: |
+ number of the Kirin 970 SPMI channel where the SPMI devices are connected.
+
+required:
+ - compatible
+ - reg
+ - spmi-channel
+
+patternProperties:
+ "^pmic@[0-9a-f]$":
+ description: |
+ PMIC properties, which are specific to the used SPMI PMIC device(s).
+ When used in combination with HiSilicon 6421v600, the properties
+ are documented at
+ Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml.
+
+examples:
+ - |
+ bus {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ spmi: spmi@fff24000 {
+ compatible = "hisilicon,kirin970-spmi-controller";
+ status = "ok";
+ reg = <0x0 0xfff24000 0x0 0x1000>;
+ spmi-channel = <2>;
+
+ pmic@0 {
+ /* pmic properties */
+ };
+ };
+ };
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 00/44] SPMI patches needed by Hikey 970
2020-08-17 7:10 [PATCH v3 00/44] SPMI patches needed by Hikey 970 Mauro Carvalho Chehab
2020-08-17 7:11 ` [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties Mauro Carvalho Chehab
2020-08-17 7:32 ` [PATCH v3 00/44] SPMI patches needed by Hikey 970 Greg Kroah-Hartman
@ 2020-08-18 14:17 ` Greg Kroah-Hartman
2020-08-18 14:28 ` Mauro Carvalho Chehab
2 siblings, 1 reply; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-08-18 14:17 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: devel, devicetree, Rob Herring, linux-arm-msm, linuxarm, Wei Xu,
linux-kernel, Stephen Boyd, Rob Herring, mauro.chehab, Lee Jones,
David S. Miller, linux-arm-kernel
On Mon, Aug 17, 2020 at 09:10:19AM +0200, Mauro Carvalho Chehab wrote:
> Hi Greg,
>
> This patch series is part of a work I'm doing in order to be able to support
> a HiKey 970 board that I recently got on my hands.
With this applied, I get the following build error:
ERROR: modpost: "__spmi_driver_register" [drivers/staging/hikey9xx/hi6421-spmi-pmic.ko] undefined!
ERROR: modpost: "spmi_ext_register_writel" [drivers/staging/hikey9xx/hi6421-spmi-pmic.ko] undefined!
ERROR: modpost: "spmi_ext_register_readl" [drivers/staging/hikey9xx/hi6421-spmi-pmic.ko] undefined!
ERROR: modpost: "spmi_controller_add" [drivers/staging/hikey9xx/hisi-spmi-controller.ko] undefined!
ERROR: modpost: "spmi_controller_alloc" [drivers/staging/hikey9xx/hisi-spmi-controller.ko] undefined!
ERROR: modpost: "spmi_controller_remove" [drivers/staging/hikey9xx/hisi-spmi-controller.ko] undefined!
I'll take this in my testing tree for now, can you send a follow-on
patch to fix this?
And I only took the first 41 patches in this series, see my comments on
the rest.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3.1 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties
2020-08-18 11:10 ` [PATCH v3.1 " Mauro Carvalho Chehab
@ 2020-08-18 14:19 ` Greg Kroah-Hartman
0 siblings, 0 replies; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-08-18 14:19 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Rob Herring, linuxarm, mauro.chehab, Lee Jones, Stephen Boyd,
devicetree, linux-kernel, linux-arm-msm
On Tue, Aug 18, 2020 at 01:10:24PM +0200, Mauro Carvalho Chehab wrote:
> From e464ec2c38c083403b556e60f189ee8ae2f2c9c6 Mon Sep 17 00:00:00 2001
> From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Date: Fri, 31 Jul 2020 09:46:02 +0200
> Subject: [PATCH] dt: document HiSilicon SPMI controller and mfd/regulator
> properties
>
> 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>
>
Did you mean to enclose the whole patch, headers and all in here?
> ---
>
> v3.1:
> - Changed the DT properties to better match upstream requirements
>
> PS.: I opted to submit just this patch, instead of the entire
> series, in order to avoid flooding people's ML.
>
> I'll be posting the full series again after DT specs match
> upstream requirements.
>
>
> diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
staging drivers should be self-contained, please keep dt files within
the staging driver directory for the driver until they move out of
staging if at all possible.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 00/44] SPMI patches needed by Hikey 970
2020-08-18 14:17 ` Greg Kroah-Hartman
@ 2020-08-18 14:28 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2020-08-18 14:28 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: devel, devicetree, Rob Herring, linux-arm-msm, linuxarm, Wei Xu,
linux-kernel, Stephen Boyd, Rob Herring, mauro.chehab, Lee Jones,
David S. Miller, linux-arm-kernel
Em Tue, 18 Aug 2020 16:17:50 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:
> On Mon, Aug 17, 2020 at 09:10:19AM +0200, Mauro Carvalho Chehab wrote:
> > Hi Greg,
> >
> > This patch series is part of a work I'm doing in order to be able to support
> > a HiKey 970 board that I recently got on my hands.
>
> With this applied, I get the following build error:
> ERROR: modpost: "__spmi_driver_register" [drivers/staging/hikey9xx/hi6421-spmi-pmic.ko] undefined!
> ERROR: modpost: "spmi_ext_register_writel" [drivers/staging/hikey9xx/hi6421-spmi-pmic.ko] undefined!
> ERROR: modpost: "spmi_ext_register_readl" [drivers/staging/hikey9xx/hi6421-spmi-pmic.ko] undefined!
> ERROR: modpost: "spmi_controller_add" [drivers/staging/hikey9xx/hisi-spmi-controller.ko] undefined!
> ERROR: modpost: "spmi_controller_alloc" [drivers/staging/hikey9xx/hisi-spmi-controller.ko] undefined!
> ERROR: modpost: "spmi_controller_remove" [drivers/staging/hikey9xx/hisi-spmi-controller.ko] undefined!
>
>
> I'll take this in my testing tree for now, can you send a follow-on
> patch to fix this?
Surely. That's because it got moved from drivers/spmi/Kconfig.
The Kconfig var was inside a:
if SPMI
...
endif
This driver should "depends on SPMI". I'll send you a patch in a few.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties
2020-08-18 9:13 ` Mauro Carvalho Chehab
2020-08-18 10:37 ` Mauro Carvalho Chehab
@ 2020-08-18 17:07 ` Rob Herring
2020-08-18 22:18 ` Mauro Carvalho Chehab
1 sibling, 1 reply; 13+ messages in thread
From: Rob Herring @ 2020-08-18 17:07 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Greg Kroah-Hartman, linuxarm, mauro.chehab, Lee Jones,
Stephen Boyd, devicetree, linux-kernel, linux-arm-msm
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties
2020-08-18 17:07 ` Rob Herring
@ 2020-08-18 22:18 ` Mauro Carvalho Chehab
2020-08-19 20:48 ` Rob Herring
0 siblings, 1 reply; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2020-08-18 22:18 UTC (permalink / raw)
To: Rob Herring
Cc: Greg Kroah-Hartman, linuxarm, mauro.chehab, Lee Jones,
Stephen Boyd, devicetree, linux-kernel, linux-arm-msm
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties
2020-08-18 22:18 ` Mauro Carvalho Chehab
@ 2020-08-19 20:48 ` Rob Herring
0 siblings, 0 replies; 13+ messages in thread
From: Rob Herring @ 2020-08-19 20:48 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Greg Kroah-Hartman, Linuxarm, mauro.chehab, Lee Jones,
Stephen Boyd, devicetree, linux-kernel, linux-arm-msm
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-08-19 20:48 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-17 7:10 [PATCH v3 00/44] SPMI patches needed by Hikey 970 Mauro Carvalho Chehab
2020-08-17 7:11 ` [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties Mauro Carvalho Chehab
2020-08-17 20:12 ` Rob Herring
2020-08-18 9:13 ` Mauro Carvalho Chehab
2020-08-18 10:37 ` Mauro Carvalho Chehab
2020-08-18 17:07 ` Rob Herring
2020-08-18 22:18 ` Mauro Carvalho Chehab
2020-08-19 20:48 ` Rob Herring
2020-08-18 11:10 ` [PATCH v3.1 " Mauro Carvalho Chehab
2020-08-18 14:19 ` Greg Kroah-Hartman
2020-08-17 7:32 ` [PATCH v3 00/44] SPMI patches needed by Hikey 970 Greg Kroah-Hartman
2020-08-18 14:17 ` Greg Kroah-Hartman
2020-08-18 14:28 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).