Hi Dario,
On Sun, 8 Nov 2020 at 03:50, Dario Binacchi <dario...@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 <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:
> @@ -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
