[PATCH v2 19/30] pwm: ti: am33xx: add subsystem driver
Lokesh Vutla
lokeshvutla at ti.com
Fri Sep 11 04:31:53 CEST 2020
On 10/09/20 11:52 pm, Dario Binacchi wrote:
> Hi Lokesh,
>
>> Il 07/09/2020 07:32 Lokesh Vutla <lokeshvutla at ti.com> ha scritto:
>>
>>
>> Hi Dario,
>>
>> On 06/09/20 5:41 pm, Dario Binacchi wrote:
>>> The TI PWMSS driver is a simple bus driver for providing clock and power
>>> management for the PWM peripherals on TI AM33xx SoCs, namely eCAP,
>>> eHRPWM and eQEP.
>>>
>>> Based on more recent versions of the device tree inside the linux kernel,
>>> I added the clock domain for each subsystem in am33x.dtsi so it can be
>>> enabled before probing the peripheral child device and disabled after
>>> its removal.
>>>
>>> Signed-off-by: Dario Binacchi <dariobin at libero.it>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>> arch/arm/dts/am33xx.dtsi | 9 +++
>>> doc/device-tree-bindings/pwm/ti,pwmss.txt | 58 ++++++++++++++
>>> drivers/pwm/Kconfig | 9 ++-
>>> drivers/pwm/Makefile | 1 +
>>> drivers/pwm/pwm-ti-pwmss.c | 95 +++++++++++++++++++++++
>>> 5 files changed, 171 insertions(+), 1 deletion(-)
>>> create mode 100644 doc/device-tree-bindings/pwm/ti,pwmss.txt
>>> create mode 100644 drivers/pwm/pwm-ti-pwmss.c
>>>
>>> diff --git a/arch/arm/dts/am33xx.dtsi b/arch/arm/dts/am33xx.dtsi
>>> index d3dd6a16e7..fa7492a9b6 100644
>>> --- a/arch/arm/dts/am33xx.dtsi
>>> +++ b/arch/arm/dts/am33xx.dtsi
>>> @@ -758,6 +758,9 @@
>>> 0x48300180 0x48300180 0x80 /* EQEP */
>>> 0x48300200 0x48300200 0x80>; /* EHRPWM */
>>>
>>> + clocks = <&l4_per_clkctrl AM3_EPWMSS0_CLKCTRL 0>;
>>> + clock-names = "fck";
>>> +
>>> ecap0: ecap at 48300100 {
>>> compatible = "ti,am3352-ecap",
>>> "ti,am33xx-ecap";
>>> @@ -792,6 +795,9 @@
>>> 0x48302180 0x48302180 0x80 /* EQEP */
>>> 0x48302200 0x48302200 0x80>; /* EHRPWM */
>>>
>>> + clocks = <&l4_per_clkctrl AM3_EPWMSS1_CLKCTRL 0>;
>>> + clock-names = "fck";
>>> +
>>> ecap1: ecap at 48302100 {
>>> compatible = "ti,am3352-ecap",
>>> "ti,am33xx-ecap";
>>> @@ -826,6 +832,9 @@
>>> 0x48304180 0x48304180 0x80 /* EQEP */
>>> 0x48304200 0x48304200 0x80>; /* EHRPWM */
>>>
>>> + clocks = <&l4_per_clkctrl AM3_EPWMSS2_CLKCTRL 0>;
>>> + clock-names = "fck";
>>
>>
>> hmm..I do not see these changes in upstream kernel v5.9-rc3 DT. Which version of
>> kernel are you using? I see al PWM nodes under am33xx-l4.dtsi
>>
>
> The kernel device tree for am335x is very different from the one in
> U-boot. The U-boot device tree is certainly older and an alignment
> with the kernel would require heavy code changes and that's not
> what I need.
kernel dts gets synced to U-Boot at some point. Won't that break your use-case?
>
> Let's compare the 'epwmss at 0' node of the kernel device tree with that of U-boot:
>
> /* kernel v5.9-rc4 am33xx-l4.dtsi */
> segment at 300000 { /* 0x48300000 */
> compatible = "simple-bus";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0x00000000 0x00300000 0x001000>, /* ap 66 */
> <0x00001000 0x00301000 0x001000>, /* ap 67 */
> <0x00002000 0x00302000 0x001000>, /* ap 68 */
> <0x00003000 0x00303000 0x001000>, /* ap 69 */
> <0x00004000 0x00304000 0x001000>, /* ap 70 */
> <0x00005000 0x00305000 0x001000>, /* ap 71 */
> <0x0000e000 0x0030e000 0x001000>, /* ap 72 */
> <0x0000f000 0x0030f000 0x001000>, /* ap 73 */
> <0x00018000 0x00318000 0x004000>, /* ap 74 */
> <0x0001c000 0x0031c000 0x001000>, /* ap 75 */
> <0x00010000 0x00310000 0x002000>, /* ap 76 */
> <0x00012000 0x00312000 0x001000>, /* ap 93 */
> <0x00015000 0x00315000 0x001000>, /* ap 94 */
> <0x00016000 0x00316000 0x001000>, /* ap 95 */
> <0x00017000 0x00317000 0x001000>, /* ap 96 */
> <0x00013000 0x00313000 0x001000>, /* ap 97 */
> <0x00014000 0x00314000 0x001000>, /* ap 98 */
> <0x00020000 0x00320000 0x001000>, /* ap 99 */
> <0x00021000 0x00321000 0x001000>, /* ap 100 */
> <0x00022000 0x00322000 0x001000>, /* ap 101 */
> <0x00023000 0x00323000 0x001000>, /* ap 102 */
> <0x00024000 0x00324000 0x001000>, /* ap 103 */
> <0x00025000 0x00325000 0x001000>; /* ap 104 */
>
> target-module at 0 { /* 0x48300000, ap 66 48.0 */
> compatible = "ti,sysc-omap4", "ti,sysc";
> reg = <0x0 0x4>,
> <0x4 0x4>;
> reg-names = "rev", "sysc";
> ti,sysc-midle = <SYSC_IDLE_FORCE>,
> <SYSC_IDLE_NO>,
> <SYSC_IDLE_SMART>,
> <SYSC_IDLE_SMART_WKUP>;
> ti,sysc-sidle = <SYSC_IDLE_FORCE>,
> <SYSC_IDLE_NO>,
> <SYSC_IDLE_SMART>,
> <SYSC_IDLE_SMART_WKUP>;
> /* Domains (P, C): per_pwrdm, l4ls_clkdm */
> clocks = <&l4ls_clkctrl AM3_L4LS_EPWMSS0_CLKCTRL 0>;
> clock-names = "fck";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0x0 0x0 0x1000>;
>
> epwmss0: epwmss at 0 {
> compatible = "ti,am33xx-pwmss";
> reg = <0x0 0x10>;
> #address-cells = <1>;
> #size-cells = <1>;
> status = "disabled";
> ranges = <0 0 0x1000>;
>
> ecap0: ecap at 100 {
> compatible = "ti,am3352-ecap",
> "ti,am33xx-ecap";
> #pwm-cells = <3>;
> reg = <0x100 0x80>;
> clocks = <&l4ls_gclk>;
> clock-names = "fck";
> interrupts = <31>;
> interrupt-names = "ecap0";
> status = "disabled";
> };
>
> ehrpwm0: pwm at 200 {
> compatible = "ti,am3352-ehrpwm",
> "ti,am33xx-ehrpwm";
> #pwm-cells = <3>;
> reg = <0x200 0x80>;
> clocks = <&ehrpwm0_tbclk>, <&l4ls_gclk>;
> clock-names = "tbclk", "fck";
> status = "disabled";
> };
> };
> };
> };
>
> /* u-boot am33xx.dtsi */
> epwmss0: epwmss at 48300000 {
> compatible = "ti,am33xx-pwmss";
> reg = <0x48300000 0x10>;
> ti,hwmods = "epwmss0";
> #address-cells = <1>;
> #size-cells = <1>;
> status = "disabled";
> ranges = <0x48300100 0x48300100 0x80 /* ECAP */
> 0x48300180 0x48300180 0x80 /* EQEP */
> 0x48300200 0x48300200 0x80>; /* EHRPWM */
>
> ecap0: ecap at 48300100 {
> compatible = "ti,am3352-ecap",
> "ti,am33xx-ecap";
> #pwm-cells = <3>;
> reg = <0x48300100 0x80>;
> clocks = <&l4ls_gclk>;
> clock-names = "fck";
> interrupts = <31>;
> interrupt-names = "ecap0";
> status = "disabled";
> };
>
> ehrpwm0: pwm at 48300200 {
> compatible = "ti,am3352-ehrpwm",
> "ti,am33xx-ehrpwm";
> #pwm-cells = <3>;
> reg = <0x48300200 0x80>;
> clocks = <&ehrpwm0_tbclk>, <&l4ls_gclk>;
> clock-names = "tbclk", "fck";
> status = "disabled";
> };
> };
>
> What can I say?
> 1 The epwmss0 node differs for the properties used for the definition
> of the registers addresses ('ranges' and 'reg' properties) due to their
> parents. I don't want to make changes for something that already works.
> The patch will not heavily modify the device tree.
Old dtb cannot be used for soo long. Today syncing the DT doesn't harm anything
but this series is going to break on DT sync. I would suggest we start with
latest DT bindings.
Thanks and regards,
Lokesh
> 2 The target-module at 0 node, parent of epwmss0 in the kernel device tree,
> references the domain clock (clocks = <&l4ls_clkctrl AM3_L4LS_EPWMSS0_CLKCTRL 0>).
> I have developed the clkctrl (compatible = "ti, clkctrl") driver and therefore, I would
> like to enable/disable domain clock using the driver model API. So, I think that
> the best point of the U-boot device tree where to add the reference to the domain
> clock is in the epwmss0 node and in any case outside the ecap0 and ehrpwm0 nodes.
> While it's not in the same location as the kernel device tree, I think it's still
> an improvement. In the future, when uboot's device tree is more like the current
> kernel device tree, we will change the location of the domain clock reference.
>
>>> +
>>> ecap2: ecap at 48304100 {
>>> compatible = "ti,am3352-ecap",
>>> "ti,am33xx-ecap";
>>> diff --git a/doc/device-tree-bindings/pwm/ti,pwmss.txt b/doc/device-tree-bindings/pwm/ti,pwmss.txt
>>> new file mode 100644
>>> index 0000000000..4633697fbd
>>> --- /dev/null
>>> +++ b/doc/device-tree-bindings/pwm/ti,pwmss.txt
>>> @@ -0,0 +1,58 @@
>>> +TI SOC based PWM Subsystem
>>> +
>>> +Required properties:
>>> +- compatible: Must be "ti,<soc>-pwmss".
>>> + for am33xx - compatible = "ti,am33xx-pwmss";
>>> + for am4372 - compatible = "ti,am4372-pwmss","ti,am33xx-pwmss";
>>> + for dra746 - compatible = "ti,dra746-pwmss", "ti,am33xx-pwmss"
>>> +
>>> +- reg: physical base address and size of the registers map.
>>> +- address-cells: Specify the number of u32 entries needed in child nodes.
>>> + Should set to 1.
>>> +- size-cells: specify number of u32 entries needed to specify child nodes size
>>> + in reg property. Should set to 1.
>>> +- ranges: describes the address mapping of a memory-mapped bus. Should set to
>>> + physical address map of child's base address, physical address within
>>> + parent's address space and length of the address map. For am33xx,
>>> + 3 set of child register maps present, ECAP register space, EQEP
>>> + register space, EHRPWM register space.
>>> +
>>> +Also child nodes should also populated under PWMSS DT node.
>>> +
>>> +Example:
>>> +epwmss0: epwmss at 48300000 { /* PWMSS for am33xx */
>>> + compatible = "ti,am33xx-pwmss";
>>> + reg = <0x48300000 0x10>;
>>> + ti,hwmods = "epwmss0";
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>> + ranges = <0x48300100 0x48300100 0x80 /* ECAP */
>>> + 0x48300180 0x48300180 0x80 /* EQEP */
>>> + 0x48300200 0x48300200 0x80>; /* EHRPWM */
>>> +
>>> + /* child nodes go here */
>>> +};
>>> +
>>> +epwmss0: epwmss at 48300000 { /* PWMSS for am4372 */
>>> + compatible = "ti,am4372-pwmss","ti,am33xx-pwmss"
>>> + reg = <0x48300000 0x10>;
>>> + ti,hwmods = "epwmss0";
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>> + ranges = <0x48300100 0x48300100 0x80 /* ECAP */
>>> + 0x48300180 0x48300180 0x80 /* EQEP */
>>> + 0x48300200 0x48300200 0x80>; /* EHRPWM */
>>> +
>>> + /* child nodes go here */
>>> +};
>>> +
>>> +epwmss0: epwmss at 4843e000 { /* PWMSS for DRA7xx */
>>> + compatible = "ti,dra746-pwmss", "ti,am33xx-pwmss";
>>> + reg = <0x4843e000 0x30>;
>>> + ti,hwmods = "epwmss0";
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>> + ranges;
>>> +
>>> + /* child nodes go here */
>>> +};
>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>> index e4ba8b3d80..129f5d77ca 100644
>>> --- a/drivers/pwm/Kconfig
>>> +++ b/drivers/pwm/Kconfig
>>> @@ -69,8 +69,15 @@ config PWM_SUNXI
>>> This PWM is found on H3, A64 and other Allwinner SoCs. It supports a
>>> programmable period and duty cycle. A 16-bit counter is used.
>>>
>>> +config PWM_TI_PWMSS
>>> + bool "Enable support for the AM335x PWM Subsystem"
>>> + depends on DM_PWM
>>> + help
>>> + This enables the AM335x PWM subsystem driver support on TI's SOCs.
>>> +
>>> config PWM_TI_EHRPWM
>>> bool "Enable support for the AM335x EHRPWM"
>>> - depends on DM_PWM
>>> + depends on PWM_TI_PWMSS
>>> + default y
>>> help
>>> This enables the AM335x EHRPWM driver support on TI's SoCs.
>>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>>> index c5f88f7501..df9c54e764 100644
>>> --- a/drivers/pwm/Makefile
>>> +++ b/drivers/pwm/Makefile
>>> @@ -19,3 +19,4 @@ obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
>>> obj-$(CONFIG_PWM_TEGRA) += tegra_pwm.o
>>> obj-$(CONFIG_PWM_SUNXI) += sunxi_pwm.o
>>> obj-$(CONFIG_PWM_TI_EHRPWM) += pwm-ti-ehrpwm.o
>>> +obj-$(CONFIG_PWM_TI_PWMSS) += pwm-ti-pwmss.o
>>> diff --git a/drivers/pwm/pwm-ti-pwmss.c b/drivers/pwm/pwm-ti-pwmss.c
>>> new file mode 100644
>>> index 0000000000..57f86512d6
>>> --- /dev/null
>>> +++ b/drivers/pwm/pwm-ti-pwmss.c
>>> @@ -0,0 +1,95 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Pulse-Width Modulation Subsystem (pwmss)
>>> + *
>>> + * Copyright (C) 2020 Dario Binacchi <dariobin at libero.it>
>>> + */
>>> +#define DEBUG
>>> +#undef CONFIG_LOGLEVEL
>>> +#define CONFIG_LOGLEVEL 8
>>> +
>>> +#include <common.h>
>>> +#include <clk.h>
>>> +#include <dm.h>
>>> +
>>> +struct ti_pwmss_priv {
>>> + int child_count;
>>> + struct clk clkdm;
>>> +};
>>> +
>>> +static const struct udevice_id ti_pwmss_ids[] = {
>>> + {.compatible = "ti,am33xx-pwmss"},
>>> + {}
>>> +};
>>> +
>>> +static int ti_pwmss_child_post_remove(struct udevice *dev)
>>> +{
>>> + struct ti_pwmss_priv *priv = dev_get_priv(dev->parent);
>>> + int err;
>>> +
>>> + if (--priv->child_count > 0)
>>> + return 0;
>>> +
>>> + err = clk_disable(&priv->clkdm);
>>> + if (err) {
>>> + dev_err(dev->parent, "%s: failed to disable clock domain\n",
>>> + __func__);
>>> + goto clkdm_disable_err;
>>> + }
>>> +
>>> + err = clk_release_all(&priv->clkdm, 1);
>>> + if (err) {
>>> + dev_err(dev->parent, "%s: failed to release clock domain\n",
>>> + __func__);
>>> + goto clkdm_release_err;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +clkdm_release_err:
>>> + clk_enable(&priv->clkdm);
>>> +clkdm_disable_err:
>>> + priv->child_count++;
>>> + return err;
>>> +}
>>> +
>>> +static int ti_pwmss_child_pre_probe(struct udevice *dev)
>>> +{
>>> + struct ti_pwmss_priv *priv = dev_get_priv(dev->parent);
>>> + int err;
>>> +
>>> + if (++priv->child_count > 1)
>>> + return 0;
>>> +
>>> + err = clk_get_by_name(dev->parent, "fck", &priv->clkdm);
>>> + if (err) {
>>> + dev_err(dev->parent, "%s: failed to get clock domain\n",
>>> + __func__);
>>> + goto clkdm_get_err;
>>> + }
>>> +
>>> + err = clk_enable(&priv->clkdm);
>>> + if (err) {
>>> + dev_err(dev->parent, "%s: failed to enable clock domain\n",
>>> + __func__);
>>> + goto clkdm_enable_err;
>>> + }
>>
>> Do we need a whole subsystem driver just for enabling the clocks? Can't the same
>> be happen in driver?
>
> We need this driver also and especially to bind (through the dm_scan_fdt_dev) its child devices in the device tree:
>
> U_BOOT_DRIVER(ti_pwmss) = {
> ...
> .bind = dm_scan_fdt_dev,
> ...
> };
>
> Without this driver the nodes ecap0 and ehrpwm0 will not be bound.
>
> Regards,
> Dario
>
>>
>> Thanks and regards,
>> Lokesh
More information about the U-Boot
mailing list