[PATCH v5 18/27] misc: am33xx: add control module driver
Lokesh Vutla
lokeshvutla at ti.com
Sun Nov 15 10:52:11 CET 2020
Hi Dario,
On 08/11/20 4:20 pm, Dario Binacchi 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:
I prefer option 3 as this is a SoC property and it should be applied to all SoCs.
Thanks and regards,
Lokesh
More information about the U-Boot
mailing list