[PATCH v5 18/27] misc: am33xx: add control module driver
Dario Binacchi
dariobin at libero.it
Sun Nov 8 11:50:12 CET 2020
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 at chromium.org> ha scritto:
>
>
> Hi Dario,
>
> On Sun, 1 Nov 2020 at 02:13, Dario Binacchi <dariobin at libero.it> wrote:
> >
> > Hi Simon,
> >
> > > Il 28/10/2020 03:10 Simon Glass <sjg at chromium.org> ha scritto:
> > >
> > >
> > > 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.
> >
> > 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 at 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) \
> 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
Regards,
Dario
More information about the U-Boot
mailing list