Hello Patrick,
On Wed, 3 Mar 2021 at 11:09, Patrick DELAUNAY patrick.delaunay@foss.st.com wrote:
Hi Etienne,
On 2/22/21 8:27 AM, Etienne Carriere wrote:
Implement voltage regulators interfaced by the SCMI voltage domain protocol. The DT bindings are defined in the Linux kernel since SCMI voltage domain and regulators patches [1] and [2] integration in v5.11-rc7.
Link: [1] https://github.com/torvalds/linux/commit/0f80fcec08e9c50b8d2992cf26495673765... Link: [2] https://github.com/torvalds/linux/commit/2add5cacff3531e54c50b0832128299faa9... Signed-off-by: Etienne Carriere etienne.carriere@linaro.org Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Jaehoon Chung jh80.chung@samsung.com
Changes in v3:
- applied review tags
Changes in v2:
- no change
doc/device-tree-bindings/arm/arm,scmi.txt | 34 +++++ drivers/firmware/scmi/scmi_agent-uclass.c | 35 ++++- drivers/power/regulator/Kconfig | 8 + drivers/power/regulator/Makefile | 1 + drivers/power/regulator/scmi_regulator.c | 170 ++++++++++++++++++++++ include/scmi_protocols.h | 113 ++++++++++++++ 6 files changed, 359 insertions(+), 2 deletions(-) create mode 100644 drivers/power/regulator/scmi_regulator.c
diff --git a/doc/device-tree-bindings/arm/arm,scmi.txt b/doc/device-tree-bindings/arm/arm,scmi.txt index 1f293ea24..a76124f4a 100644 --- a/doc/device-tree-bindings/arm/arm,scmi.txt +++ b/doc/device-tree-bindings/arm/arm,scmi.txt @@ -62,6 +62,20 @@ Required properties:
- #power-domain-cells : Should be 1. Contains the device or the power domain ID value used by SCMI commands.
+Regulator bindings for the SCMI Regulator based on SCMI Message Protocol +------------------------------------------------------------ +An SCMI Regulator is permanently bound to a well defined SCMI Voltage Domain, +and should be always positioned as a root regulator. +It does not support any current operation.
+SCMI Regulators are grouped under a 'regulators' node which in turn is a child +of the SCMI Voltage protocol node inside the desired SCMI instance node.
+This binding uses the common regulator binding[6].
+Required properties:
- reg : shall identify an existent SCMI Voltage Domain.
Sensor bindings for the sensors based on SCMI Message Protocol
SCMI provides an API to access the various sensors on the SoC.@@ -105,6 +119,7 @@ Required sub-node properties: [3] Documentation/devicetree/bindings/thermal/thermal.txt [4] Documentation/devicetree/bindings/sram/sram.yaml [5] Documentation/devicetree/bindings/reset/reset.txt +[6] Documentation/devicetree/bindings/regulator/regulator.yaml
Example:
@@ -169,6 +184,25 @@ firmware { reg = <0x16>; #reset-cells = <1>; };
scmi_voltage: protocol@17 {reg = <0x17>;regulators {regulator_devX: regulator@0 {reg = <0x0>;regulator-max-microvolt = <3300000>;};regulator_devY: regulator@9 {reg = <0x9>;regulator-min-microvolt = <500000>;regulator-max-microvolt = <4200000>;};...};}; };diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c b/drivers/firmware/scmi/scmi_agent-uclass.c index 516e690ac..03d236426 100644 --- a/drivers/firmware/scmi/scmi_agent-uclass.c +++ b/drivers/firmware/scmi/scmi_agent-uclass.c @@ -50,6 +50,29 @@ int scmi_to_linux_errno(s32 scmi_code) return -EPROTO; }
+static int regulator_devices_bind(struct udevice *dev, struct driver *drv,
ofnode protocol_node)+{
ofnode regulators_node;ofnode node;int ret;regulators_node = ofnode_find_subnode(protocol_node, "regulators");if (!ofnode_valid(regulators_node)) {dev_err(dev, "regulators subnode not found\n");return -ENXIO;}ofnode_for_each_subnode(node, regulators_node) {ret = device_bind(dev, drv, ofnode_get_name(node), NULL, node,NULL);if (ret)return ret;}return 0;+}
This parsing function can be moved in
drivers/power/regulator/scmi_regulator.c
That is a good idea. I didn't want to export a function from regu/scmi-regu.c to scmi/... The intermediate device would nicely do the binding job. At the cost of an additional udevice. Is that worth it?
I agree it would reflect more the DT since scmi regus are in a child node of the SCMI protocol. But should the software align with the DT?
That said, if no one argues against your id, i'll make the proposed changes and post a v4.
Thanks, etienne
By adding the intermediate NOP uclass, binded with "protocol@17" node
for example
U_BOOT_DRIVER(scmi_regulator) = { .name = "scmi_regulator", .id = UCLASS_REGULATOR, .ops = &scmi_voltd_ops, .probe = scmi_regulator_probe, .ofdata_to_platdata = scmi_regulator_ofdata_to_platdata, .platdata_auto_alloc_size = sizeof(struct scmi_regulator_platdata), };
static int scmi_regulator_bind(struct udevice *dev) { struct driver *drv; ofnode node, subnode; int ret;
node = dev_read_subnode(dev, "regulators"); if (!ofnode_valid(node)) { dev_err(dev, "regulators subnode not found\n"); return -ENXIO; } drv = DM_GET_DRIVER(scmi_regulator); ofnode_for_each_subnode(subnode, node) { ret = device_bind_ofnode(dev, drv, ofnode_get_name(subnode), NULL, subnode, NULL); if (ret) return ret; } return 0;}
U_BOOT_DRIVER(scmi_voltage_domain) = { .name = "scmi_voltage_domain", .id = UCLASS_NOP, .bind = scmi_regulator_bind, };
I think it is more elegant and more alligned with existing behavior
for SCMI clock and SCMI reset.
SCMI clocks and reset are different in that there is only 1 device per clock grapes and reset controller series. For SCMI regulator, there is 1 device per regulator.
warning:
in this case the tree of U-Boot class change, SCMI agent become the grand-father
of each regulator and all the call "devm_scmi_process_msg(dev->parent, &msg);"
should be replaced by "devm_scmi_process_msg(dev->parent->parent, &msg);"
/*
- SCMI agent devices binds devices of various uclasses depeding on
- the FDT description. scmi_bind_protocol() is a generic bind sequence
@@ -79,6 +102,10 @@ static int scmi_bind_protocols(struct udevice *dev) if (IS_ENABLED(CONFIG_RESET_SCMI)) drv = DM_DRIVER_GET(scmi_reset_domain); break;
case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI))drv = DM_DRIVER_GET(scmi_voltage_domain);break; default: break; }@@ -89,8 +116,12 @@ static int scmi_bind_protocols(struct udevice *dev) continue; }
ret = device_bind(dev, drv, ofnode_get_name(node), NULL, node,NULL);
if (protocol_id == SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN)ret = regulator_devices_bind(dev, drv, node);elseret = device_bind(dev, drv, ofnode_get_name(node),NULL, node, NULL);if (ret) break; }(...)
Regards
Patrick