[PATCH v5 18/27] misc: am33xx: add control module driver

[Simon Glass]

On Sun, 25 Oct 2020 at 06:40, Dario Binacchi <dariobin at libero.it> wrote:
>
> The implementation of this driver was needed to bind the device tree
> sub-nodes of the 'clocks' node. In fact, the lack of the compatible
> property in the 'clocks' node does not allow the generic 'syscon' or
> 'simple-bus' drivers linked to the 'scm_conf at 0' node to bind the
> 'clocks' node and in turn its sub-nodes.
> The 'scm at 210000' node is therefore the node closest to the 'clocks' node
> whose driver can bind all the 'clocks' sub-nodes. In this way, the
> address translation functions are able to walk along the device tree
> towards the upper nodes until the address composition is completed.
>
> scm: scm at 210000 {
>         compatible = "ti,am3-scm", "simple-bus";
>         ...
>
>         scm_conf: scm_conf at 0 {
>                 compatible = "syscon", "simple-bus";
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 ranges = <0 0 0x800>;
>
>                 scm_clocks: clocks {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>                 };
>         };
> };
>
> For DT binding details see Linux doc:
> - Documentation/devicetree/bindings/arm/omap/ctrl.txt
>
> Signed-off-by: Dario Binacchi <dariobin at libero.it>
>
> ---
>
> (no changes since v4)
>
> Changes in v4:
> - Include device_compat.h header for dev_xxx macros.
>
> Changes in v3:
> - Remove doc/device-tree-bindings/arm/omap,ctrl.txt.
> - Remove doc/device-tree-bindings/pinctrl/pinctrl-single.txt.
> - Add to commit message the references to linux kernel dt binding
>   documentation.
>
> Changes in v2:
> - Remove the 'ti_am3_scm_clocks' driver. Handle 'scm_clocks' node in
>   the 'ti_am3_scm' driver.
> - Update the commit message.
>
>  drivers/misc/Kconfig      |  7 ++++
>  drivers/misc/Makefile     |  1 +
>  drivers/misc/ti-am3-scm.c | 82 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 90 insertions(+)
>  create mode 100644 drivers/misc/ti-am3-scm.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index b67e906a76..9e8b676637 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -500,4 +500,11 @@ config ESM_PMIC
>           Support ESM (Error Signal Monitor) on PMIC devices. ESM is used
>           typically to reboot the board in error condition.
>
> +config TI_AM3_SCM
> +       bool "AM33XX specific control module support (SCM)"
> +       depends on ARCH_OMAP2PLUS
> +       help
> +        The control module includes status and control logic not addressed
> +        within the peripherals or the rest of the device infrastructure.
> +
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 947bd3a647..056fb3b522 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -75,3 +75,4 @@ obj-$(CONFIG_MICROCHIP_FLEXCOM) += microchip_flexcom.o
>  obj-$(CONFIG_K3_AVS0) += k3_avs.o
>  obj-$(CONFIG_ESM_K3) += k3_esm.o
>  obj-$(CONFIG_ESM_PMIC) += esm_pmic.o
> +obj-$(CONFIG_TI_AM3_SCM) += ti-am3-scm.o
> diff --git a/drivers/misc/ti-am3-scm.c b/drivers/misc/ti-am3-scm.c
> new file mode 100644
> index 0000000000..ed886e6916
> --- /dev/null
> +++ b/drivers/misc/ti-am3-scm.c
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AM335x specific control module (scm)
> + *
> + * Copyright (C) 2020 Dario Binacchi <dariobin at libero.it>
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <dm/device_compat.h>
> +#include <dm/lists.h>
> +#include <linux/err.h>
> +
> +static int ti_am3_scm_bind(struct udevice *dev)
> +{
> +       ofnode clocks_node, conf_node, node;
> +       struct udevice *conf_dev;
> +       int err;
> +
> +       if (!strcmp("clocks", ofnode_get_name(dev_ofnode(dev)))) {
> +               ofnode_for_each_subnode(node, dev_ofnode(dev)) {

Is there not a compatible string for these subnodes?

> +                       dev_dbg(dev, "%s: node=%s\n", __func__,
> +                               ofnode_get_name(node));
> +                       err = lists_bind_fdt(dev, node, NULL, false);
> +                       if (err) {
> +                               dev_err(dev, "%s: lists_bind_fdt, err=%d\n",
> +                                       __func__, err);
> +                               return err;
> +                       }
> +               }
> +
> +               return 0;
> +       }
> +
> +       err = dm_scan_fdt_dev(dev);

If there is no compatible string in the subnodes, what does this
function hope to do?

> +       if (err) {
> +               dev_err(dev, "%s: dm_scan_fdt, err=%d\n", __func__, err);
> +               return err;
> +       }
> +
> +       conf_node = dev_read_subnode(dev, "scm_conf at 0");
> +       if (!ofnode_valid(conf_node)) {
> +               dev_err(dev, "%s: failed to get conf sub-node\n", __func__);
> +               return -ENODEV;
> +       }
> +
> +       if (uclass_get_device_by_ofnode(UCLASS_SYSCON, conf_node, &conf_dev)) {
> +               if (uclass_get_device_by_ofnode(UCLASS_SIMPLE_BUS, conf_node,
> +                                               &conf_dev)) {
> +                       dev_err(dev, "%s: failed to get conf device\n",
> +                               __func__);
> +                       return -ENODEV;

You can't use this because there is a device. Perhaps -ENOENT,? Same below.

> +               }
> +       }
> +
> +       clocks_node = dev_read_subnode(conf_dev, "clocks");
> +       if (!ofnode_valid(clocks_node)) {
> +               dev_err(dev, "%s: failed to get clocks sub-node\n", __func__);
> +               return -ENODEV;
> +       }
> +
> +       err = device_bind_driver_to_node(conf_dev, "ti_am3_scm", "scm_clocks",
> +                                        clocks_node, NULL);

Again, can we not rely on a compatible string? There is so much code
here that could be removed.

> +       if (err) {
> +               dev_err(dev, "%s: failed to bind scm_clocks\n", __func__);
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id ti_am3_scm_ids[] = {
> +       {.compatible = "ti,am3-scm"},
> +       {}
> +};
> +
> +U_BOOT_DRIVER(ti_am3_scm) = {
> +       .name = "ti_am3_scm",
> +       .id = UCLASS_SIMPLE_BUS,
> +       .of_match = ti_am3_scm_ids,
> +       .bind = ti_am3_scm_bind,
> +};
> --
> 2.17.1
>

Regards,
Simon