[PATCH v3 1/4] firmware: scmi: voltage regulator

[Patrick DELAUNAY]

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/0f80fcec08e9c50b8d2992cf26495673765ebaba
> Link: [2] https://github.com/torvalds/linux/commit/2add5cacff3531e54c50b0832128299faa9f0563
> Signed-off-by: Etienne Carriere <etienne.carriere at linaro.org>
> Reviewed-by: Simon Glass <sjg at chromium.org>
> Reviewed-by: Jaehoon Chung <jh80.chung at 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 at 17 {
> +			reg = <0x17>;
> +
> +			regulators {
> +				regulator_devX: regulator at 0 {
> +					reg = <0x0>;
> +					regulator-max-microvolt = <3300000>;
> +				};
> +
> +				regulator_devY: regulator at 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

By adding the intermediate NOP uclass, binded with "protocol at 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.


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);
> +		else
> +			ret = device_bind(dev, drv, ofnode_get_name(node),
> +					  NULL, node, NULL);
> +
>   		if (ret)
>   			break;
>   	}

(...)

Regards

Patrick