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

Simon Glass sjg at chromium.org
Tue Nov 17 00:52:51 CET 2020


Hi Dario,

On Sun, 8 Nov 2020 at 03:50, Dario Binacchi <dariobin at 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 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) \

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


More information about the U-Boot mailing list