[PATCH v3 1/4] firmware: scmi: voltage regulator
Etienne Carriere
etienne.carriere at linaro.org
Fri Mar 5 00:37:38 CET 2021
Hello Patrick,
On Wed, 3 Mar 2021 at 11:09, Patrick DELAUNAY
<patrick.delaunay at 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/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
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 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.
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);
> > + else
> > + ret = device_bind(dev, drv, ofnode_get_name(node),
> > + NULL, node, NULL);
> > +
> > if (ret)
> > break;
> > }
>
> (...)
>
> Regards
>
> Patrick
>
More information about the U-Boot
mailing list