[PATCH v5 18/27] misc: am33xx: add control module driver
Dario Binacchi
dariobin at libero.it
Sun Nov 1 10:13:53 CET 2020
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?
Regards,
Dario
More information about the U-Boot
mailing list