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

On Sun, 25 Oct 2020 at 06:40, Dario Binacchi dariobin@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@0' node to bind the 'clocks' node and in turn its sub-nodes. The 'scm@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@210000 { compatible = "ti,am3-scm", "simple-bus"; ...

    scm_conf: scm_conf@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@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@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@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