Hi Dario,
On Sun, 8 Nov 2020 at 03:50, Dario Binacchi dariobin@libero.it wrote:
Hi Simon, I still have some doubts and therefore I would like to also add Lokesh on this matter to finally decide what to do.
Il 03/11/2020 16:12 Simon Glass sjg@chromium.org ha scritto:
Hi Dario,
On Sun, 1 Nov 2020 at 02:13, Dario Binacchi dariobin@libero.it wrote:
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?
I'd like to see compatible strings for the subnode so that you don't need to manually call device_bind_driver_to_node(). Driver model will take care of it.
I agree with you.
You can put the compatible strings in the *u-boot.dtsi file I suppose, although it would be better if the binding was accepted upstream.
So, where then to insert the 'simple-bus' compatible string of the clocks node? 1 am335x-evm-u-boot.dtsi: It's simple to implement but on a design level, I think it's the worst. In fact, this change should be replicated on all am335x boards. 2 am33xx-l4.dtsi: It is simple to implement, but it modifies the linux kernel DTS. I wonder if it is possible to think this time that it is okay to patch the linux kernel DTS. 3 am33xx-u-boot.dtsi: You need to patch scripts/Makefile.lib to include it in the final DTS along with am335x-<board>-u-boot.dtsi. It would automatically be applied for every board. Here is the patch I applied to get it: @@ -179,7 +179,7 @@ u_boot_dtsi_options_raw = $(warning Automatic .dtsi inclusion: options: \
# We use the first match u_boot_dtsi = $(strip $(u_boot_dtsi_options_debug) \
$(notdir $(firstword $(u_boot_dtsi_options))))
$(notdir $(u_boot_dtsi_options)))# Modified for U-Boot dtc_cpp_flags = -Wp,-MD,$(depfile).pre.tmp -nostdinc \ @@ -315,11 +315,28 @@ ifeq ($(CONFIG_OF_LIBFDT_OVERLAY),y) DTC_FLAGS += -@ endif
+# Reverse a list +# Usage: +# $(call reverse,list,)
+define reverse
- $(if $(strip $(1)), \
- $(call reverse, \
$(wordlist 2,$(words $(1)),$(1)), \$(firstword $(1)) $(2)), \- $(2))
+endef
+#u_boot_dtsi_reversed = $(call reverse,$(u_boot_dtsi),)
quiet_cmd_dtc = DTC $@ # Modified for U-Boot # Bring in any U-Boot-specific include at the end of the file cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
(cat $<; $(if $(u_boot_dtsi),echo '$(pound)include "$(u_boot_dtsi)"')) > $(pre-tmp); \
(cat $<) > $(pre-tmp); \$(foreach dtsi,$(call reverse,$(u_boot_dtsi),), \$(if $(dtsi),echo '$(pound)include "$(dtsi)"') >> $(pre-tmp);) \ $(CPP) $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $(pre-tmp) ; \ $(DTC) -O dtb -o $@ -b 0 \ -i $(dir $<) $(DTC_FLAGS) \
This means that multiple .dtsi files may be included. So far we have only picked one (the first), and then had that include others if needed.
I don't have any strong objections to allowing multiple includes. But I do worry that it might make things more complicated.
Linux has so much more code associated with setting up a driver. With U-Boot we care a lot about code size, to making use of built-in driver model features like auto binding, auto memory allocation, etc. is important.
Failing that I don't really mind what you do, as this is a vendor-specific driver.
Regards, Simon