Hi Dario, On 08/11/20 4:20 pm, Dario Binacchi 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 <s...@chromium.org> ha scritto: >> >> >> Hi Dario, >> >> On Sun, 1 Nov 2020 at 02:13, Dario Binacchi <dario...@libero.it> wrote: >>> >>> Hi Simon, >>> >>>> Il 28/10/2020 03:10 Simon Glass <s...@chromium.org> ha scritto: >>>> >>>> >>>> On Sun, 25 Oct 2020 at 06:40, Dario Binacchi <dario...@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 <dario...@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 <dario...@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:
I prefer option 3 as this is a SoC property and it should be applied to all SoCs. Thanks and regards, Lokesh