Hi Simon,
Il 28/10/2020 03:10 Simon Glass sjg@chromium.org ha scritto:
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_OMAP2PLUShelpThe control module includes status and control logic not addressedwithin 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.
Ok
}}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.
Yes, some code can 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
After reading your considerations I did some tests and I am convinced that two are the ways to bind the clocks subnodes: 1 Implement this driver as an extension of the simple-bus driver. Like it, it will have to bind its subnodes (dm_scan_fdt_dev), but it will also have to bind the clocks subnodes since 'clocks' node has no compatible string. You're right, some code can be removed. This is the new version of the ti_am3_scm_bind function modified according to your suggestions:
static int ti_am3_scm_bind(struct udevice *dev) { ofnode clocks_node, conf_node; struct udevice *conf_dev; int err;
err = dm_scan_fdt_dev(dev); if (err) { dev_err(dev, "%s: dm_scan_fdt, err=%d\n", __func__, err); return err; }
if (!strcmp("clocks", ofnode_get_name(dev_ofnode(dev)))) return 0;
/* Look for the clocks node */ 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 -ENOENT; }
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 -ENOENT; } }
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 -ENOENT; }
err = device_bind_driver_to_node(conf_dev, "ti_am3_scm", "scm_clocks", clocks_node, NULL); if (err) { dev_err(dev, "%s: failed to bind scm_clocks\n", __func__); return err; }
return 0; }
2 Do not develop any 'ti, am3-scm' driver but add the 'simple-bus' compatible string to the 'clocks' node. This can be done in two ways: 2.1 Add it to the am33xx-l4.dtsi file. Thereby, however, it would no longer be the same as the Linux kernel one. 2.2 Add it through a *-u-boot.dtsi file. In my case, I am using a beaglebone black board, I had to modify the am335x-evm-u-boot.dtsi file. I think it would be better to add it to the am33xx-u-boot.dtsi file but scripts/Makefile.lib only includes the first of the files it found, so in case you find the files am335x-evm-u-boot.dtsi and am33xx-u-boot.dtsi, my case, it includes the file am335x-evm-u-boot.dtsi.
What do you think about it? What do you suggest me to do?
Regards, Dario