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. 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.
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
