[PATCH v5 15/27] clk: move clk-ti-sci driver to 'ti' directory

[PATCH v5 15/27] clk: move clk-ti-sci driver to 'ti' directory

[Dario Binacchi]

The patch moves the clk-ti-sci.c file to the 'ti' directory along with
all the other TI's drivers, and renames it clk-sci.c.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

(no changes since v1)

 drivers/clk/Kconfig                        | 8 --------
 drivers/clk/Makefile                       | 1 -
 drivers/clk/ti/Kconfig                     | 8 ++++++++
 drivers/clk/ti/Makefile                    | 1 +
 drivers/clk/{clk-ti-sci.c => ti/clk-sci.c} | 0
 5 files changed, 9 insertions(+), 9 deletions(-)
 rename drivers/clk/{clk-ti-sci.c => ti/clk-sci.c} (100%)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 9e54929039..db06f276ec 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -98,14 +98,6 @@ config CLK_STM32F
 	  This clock driver adds support for RCC clock management
 	  for STM32F4 and STM32F7 SoCs.
 
-config CLK_TI_SCI
-	bool "TI System Control Interface (TI SCI) clock driver"
-	depends on CLK && TI_SCI_PROTOCOL && OF_CONTROL
-	help
-	  This enables the clock driver support over TI System Control Interface
-	  available on some new TI's SoCs. If you wish to use clock resources
-	  managed by the TI System Controller, say Y here. Otherwise, say N.
-
 config CLK_HSDK
 	bool "Enable cgu clock driver for HSDK boards"
 	depends on CLK && TARGET_HSDK
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 2581fe0a19..f8383e523d 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -48,6 +48,5 @@ obj-$(CONFIG_SANDBOX) += clk_sandbox.o
 obj-$(CONFIG_SANDBOX) += clk_sandbox_test.o
 obj-$(CONFIG_SANDBOX_CLK_CCF) += clk_sandbox_ccf.o
 obj-$(CONFIG_STM32H7) += clk_stm32h7.o
-obj-$(CONFIG_CLK_TI_SCI) += clk-ti-sci.o
 obj-$(CONFIG_CLK_VERSAL) += clk_versal.o
 obj-$(CONFIG_CLK_CDCE9XX) += clk-cdce9xx.o
diff --git a/drivers/clk/ti/Kconfig b/drivers/clk/ti/Kconfig
index 9e257a2eb7..2dc86d44a9 100644
--- a/drivers/clk/ti/Kconfig
+++ b/drivers/clk/ti/Kconfig
@@ -33,3 +33,11 @@ config CLK_TI_MUX
 	depends on CLK && OF_CONTROL && CLK_CCF
 	help
 	  This enables the mux clock driver support on TI's SoCs.
+
+config CLK_TI_SCI
+	bool "TI System Control Interface (TI SCI) clock driver"
+	depends on CLK && TI_SCI_PROTOCOL && OF_CONTROL
+	help
+	  This enables the clock driver support over TI System Control Interface
+	  available on some new TI's SoCs. If you wish to use clock resources
+	  managed by the TI System Controller, say Y here. Otherwise, say N.
diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile
index c929fe4e28..3d6e0cd79d 100644
--- a/drivers/clk/ti/Makefile
+++ b/drivers/clk/ti/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_CLK_TI_CTRL) += clk-ctrl.o
 obj-$(CONFIG_CLK_TI_DIVIDER) += clk-divider.o
 obj-$(CONFIG_CLK_TI_GATE) += clk-gate.o
 obj-$(CONFIG_CLK_TI_MUX) += clk-mux.o
+obj-$(CONFIG_CLK_TI_SCI) += clk-sci.o
diff --git a/drivers/clk/clk-ti-sci.c b/drivers/clk/ti/clk-sci.c
similarity index 100%
rename from drivers/clk/clk-ti-sci.c
rename to drivers/clk/ti/clk-sci.c
-- 
2.17.1

[PATCH v5 16/27] fdt: translate address if #size-cells = <0>

[Dario Binacchi]

The __of_translate_address routine translates an address from the
device tree into a CPU physical address. A note in the description of
the routine explains that the crossing of any level with
#size-cells = <0> is to be considered an error not by specification but
since inherited from IBM. This does not happen for Texas Instruments, or
at least for the beaglebone device tree. Without this patch, in fact,
the translation into physical addresses of the registers contained in the
am33xx-clocks.dtsi nodes would not be possible. They all have a parent
with #size-cells = <0>.

The CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol makes translation
possible even in the case of crossing levels with #size-cells = <0>.

The patch acts conservatively on address translation, except for
removing a check within the of_translate_one function in the
drivers/core/of_addr.c file:

+
        ranges = of_get_property(parent, rprop, &rlen);
-       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
-               debug("no ranges; cannot translate\n");
-               return 1;
-       }
        if (ranges == NULL || rlen == 0) {
                offset = of_read_number(addr, na);
                memset(addr, 0, pna * 4);
		debug("empty ranges; 1:1 translation\n");

There are two reasons:
1 The function of_empty_ranges_quirk always returns false, invalidating
  the following if statement in case of null ranges. Therefore one of
  the two checks is useless.

2 The implementation of the of_translate_one function found in the
  common/fdt_support.c file has removed this check while keeping the one
  about the 1:1 translation.

The patch adds a test and modifies a check for the correctness of an
address in the case of enabling translation also for zero size cells.
The added test checks translations of addresses generated by nodes of
a device tree similar to those you can find in the files am33xx.dtsi
and am33xx-clocks.dtsi for which the patch was created.

The patch was also tested on a beaglebone black board. The addresses
generated for the registers of the loaded drivers are those specified
by the AM335x reference manual.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
Tested-by: Dario Binacchi <dariobin@libero.it>
Reviewed-by: Simon Glass <sjg@chromium.org>

---

(no changes since v4)

Changes in v4:
- Add Sphinx documentation for dm_flags.
- Convert GD_DM_FLG_* to enum.
- Include device_compat.h header in test/dm/test-fdt.c for dev_xxx macros.

Changes in v3:
- Comment dm_flags field in the global_data structure.

Changes in v2:
- Fix a missing line in the commit message.
- Add dm_flags to global_data structure and GD_DM_FLG_SIZE_CELLS_0 macro
  to test without recompiling.
- Update the OF_CHECK_COUNTS macro in order to have just one
  #define by bringing the GD_DM_FLG_SIZE_CELLS_0 into the expression.
- Lower-case the 0xC019 hex number.

 arch/sandbox/dts/test.dts         | 21 ++++++++++
 common/fdt_support.c              |  6 ++-
 drivers/core/Kconfig              | 12 ++++++
 drivers/core/fdtaddr.c            |  2 +-
 drivers/core/of_addr.c            | 14 ++-----
 drivers/core/ofnode.c             |  7 +++-
 drivers/core/root.c               |  3 ++
 include/asm-generic/global_data.h | 18 ++++++++
 test/dm/test-fdt.c                | 69 ++++++++++++++++++++++++++++++-
 9 files changed, 136 insertions(+), 16 deletions(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index fa84b2c10f..4a7a28559a 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -40,6 +40,7 @@
 		fdt-dummy1 = "/translation-test at 8000/dev at 1,100";
 		fdt-dummy2 = "/translation-test at 8000/dev at 2,200";
 		fdt-dummy3 = "/translation-test at 8000/noxlatebus at 3,300/dev at 42";
+		fdt-dummy4 = "/translation-test at 8000/xlatebus at 4,400/devs/dev at 19";
 		usb0 = &usb_0;
 		usb1 = &usb_1;
 		usb2 = &usb_2;
@@ -1028,6 +1029,7 @@
 			  1 0x100 0x9000 0x1000
 			  2 0x200 0xA000 0x1000
 			  3 0x300 0xB000 0x1000
+			  4 0x400 0xC000 0x1000
 			 >;
 
 		dma-ranges = <0 0x000 0x10000000 0x1000
@@ -1064,6 +1066,25 @@
 				reg = <0x42>;
 			};
 		};
+
+		xlatebus at 4,400 {
+			compatible = "sandbox,zero-size-cells-bus";
+			reg = <4 0x400 0x1000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 4 0x400 0x1000>;
+
+			devs {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				dev at 19 {
+					compatible = "denx,u-boot-fdt-dummy";
+					reg = <0x19>;
+				};
+			};
+		};
+
 	};
 
 	osd {
diff --git a/common/fdt_support.c b/common/fdt_support.c
index a565b470f8..d8cf6f5725 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -20,6 +20,8 @@
 #include <exports.h>
 #include <fdtdec.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 /**
  * fdt_getprop_u32_default_node - Return a node's property or a default
  *
@@ -1001,8 +1003,8 @@ void fdt_del_node_and_alias(void *blob, const char *alias)
 /* Max address size we deal with */
 #define OF_MAX_ADDR_CELLS	4
 #define OF_BAD_ADDR	FDT_ADDR_T_NONE
-#define OF_CHECK_COUNTS(na, ns)	((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
-			(ns) > 0)
+#define OF_CHECK_COUNTS(na, ns) (((na) > 0 && (na) <= OF_MAX_ADDR_CELLS) && \
+			 ((ns) > 0 || (gd->dm_flags & GD_DM_FLG_SIZE_CELLS_0)))
 
 /* Debug utility */
 #ifdef DEBUG
diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
index 07d3a6a7a4..d4ea202b16 100644
--- a/drivers/core/Kconfig
+++ b/drivers/core/Kconfig
@@ -217,6 +217,18 @@ config OF_TRANSLATE
 	  used for the address translation. This function is faster and
 	  smaller in size than fdt_translate_address().
 
+config OF_TRANSLATE_ZERO_SIZE_CELLS
+	bool "Enable translation for zero size cells"
+	depends on OF_TRANSLATE
+	default n
+	help
+	  The routine used to translate an FDT address into a physical CPU
+	  address was developed by IBM. It considers that crossing any level
+	  with #size-cells = <0> makes translation impossible, even if it is
+	  not the way it was specified.
+	  Enabling this option makes translation possible even in the case
+	  of crossing levels with #size-cells = <0>.
+
 config SPL_OF_TRANSLATE
 	bool "Translate addresses using fdt_translate_address in SPL"
 	depends on SPL_DM && SPL_OF_CONTROL
diff --git a/drivers/core/fdtaddr.c b/drivers/core/fdtaddr.c
index 8b48aa5bc5..51a0093d65 100644
--- a/drivers/core/fdtaddr.c
+++ b/drivers/core/fdtaddr.c
@@ -49,7 +49,7 @@ fdt_addr_t devfdt_get_addr_index(const struct udevice *dev, int index)
 
 		reg += index * (na + ns);
 
-		if (ns) {
+		if (ns || (gd->dm_flags & GD_DM_FLG_SIZE_CELLS_0)) {
 			/*
 			 * Use the full-fledged translate function for complex
 			 * bus setups.
diff --git a/drivers/core/of_addr.c b/drivers/core/of_addr.c
index ca34d84922..a590533c8a 100644
--- a/drivers/core/of_addr.c
+++ b/drivers/core/of_addr.c
@@ -18,7 +18,9 @@
 /* Max address size we deal with */
 #define OF_MAX_ADDR_CELLS	4
 #define OF_CHECK_ADDR_COUNT(na)	((na) > 0 && (na) <= OF_MAX_ADDR_CELLS)
-#define OF_CHECK_COUNTS(na, ns)	(OF_CHECK_ADDR_COUNT(na) && (ns) > 0)
+#define OF_CHECK_COUNTS(na, ns)	(OF_CHECK_ADDR_COUNT(na) && \
+				 ((ns) > 0 || \
+				  (gd->dm_flags & GD_DM_FLG_SIZE_CELLS_0)))
 
 static struct of_bus *of_match_bus(struct device_node *np);
 
@@ -162,11 +164,6 @@ const __be32 *of_get_address(const struct device_node *dev, int index,
 }
 EXPORT_SYMBOL(of_get_address);
 
-static int of_empty_ranges_quirk(const struct device_node *np)
-{
-	return false;
-}
-
 static int of_translate_one(const struct device_node *parent,
 			    struct of_bus *bus, struct of_bus *pbus,
 			    __be32 *addr, int na, int ns, int pna,
@@ -193,11 +190,8 @@ static int of_translate_one(const struct device_node *parent,
 	 * As far as we know, this damage only exists on Apple machines, so
 	 * This code is only enabled on powerpc. --gcl
 	 */
+
 	ranges = of_get_property(parent, rprop, &rlen);
-	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
-		debug("no ranges; cannot translate\n");
-		return 1;
-	}
 	if (ranges == NULL || rlen == 0) {
 		offset = of_read_number(addr, na);
 		memset(addr, 0, pna * 4);
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 7d1b89514c..700cee9f56 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -304,7 +304,8 @@ fdt_addr_t ofnode_get_addr_size_index(ofnode node, int index, fdt_size_t *size)
 
 		ns = of_n_size_cells(ofnode_to_np(node));
 
-		if (IS_ENABLED(CONFIG_OF_TRANSLATE) && ns > 0) {
+		if (IS_ENABLED(CONFIG_OF_TRANSLATE) &&
+		    (ns > 0 || (gd->dm_flags & GD_DM_FLG_SIZE_CELLS_0))) {
 			return of_translate_address(ofnode_to_np(node), prop_val);
 		} else {
 			na = of_n_addr_cells(ofnode_to_np(node));
@@ -656,8 +657,10 @@ fdt_addr_t ofnode_get_addr_size(ofnode node, const char *property,
 		ns = of_n_size_cells(np);
 		*sizep = of_read_number(prop + na, ns);
 
-		if (CONFIG_IS_ENABLED(OF_TRANSLATE) && ns > 0)
+		if (CONFIG_IS_ENABLED(OF_TRANSLATE) &&
+		    (ns > 0 || (gd->dm_flags & GD_DM_FLG_SIZE_CELLS_0))) {
 			return of_translate_address(np, prop);
+		}
 		else
 			return of_read_number(prop, na);
 	} else {
diff --git a/drivers/core/root.c b/drivers/core/root.c
index 0726be6b79..193164238a 100644
--- a/drivers/core/root.c
+++ b/drivers/core/root.c
@@ -135,6 +135,9 @@ int dm_init(bool of_live)
 {
 	int ret;
 
+	if (IS_ENABLED(CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS))
+		gd->dm_flags |= GD_DM_FLG_SIZE_CELLS_0;
+
 	if (gd->dm_root) {
 		dm_warn("Virtual root driver already exists!\n");
 		return -EINVAL;
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index ebb740d34f..5b9364f624 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -180,6 +180,12 @@ struct global_data {
 	struct global_data *new_gd;
 
 #ifdef CONFIG_DM
+	/**
+	 * @dm_flags: additional flags for Driver Model
+	 *
+	 * See &enum gd_dm_flags
+	 */
+	unsigned long dm_flags;
 	/**
 	 * @dm_root: root instance for Driver Model
 	 */
@@ -491,6 +497,18 @@ enum gd_flags {
 	GD_FLG_SMP_READY = 0x40000,
 };
 
+/**
+ * enum gd_dm_flags - global data flags for Driver Model
+ *
+ * See field dm_flags of &struct global_data.
+ */
+enum gd_dm_flags {
+	/**
+	 * @GD_DM_FLG_SIZE_CELLS_0: Enable #size-cells=<0> translation
+	 */
+	GD_DM_FLG_SIZE_CELLS_0 = 0x00001,
+};
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __ASM_GENERIC_GBL_DATA_H */
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
index cc12419ea0..dd18160cbe 100644
--- a/test/dm/test-fdt.c
+++ b/test/dm/test-fdt.c
@@ -5,6 +5,7 @@
 
 #include <common.h>
 #include <dm.h>
+#include <dm/device_compat.h>
 #include <errno.h>
 #include <fdtdec.h>
 #include <log.h>
@@ -581,6 +582,64 @@ U_BOOT_DRIVER(fdt_dummy_drv) = {
 	.id	= UCLASS_TEST_DUMMY,
 };
 
+static int zero_size_cells_bus_bind(struct udevice *dev)
+{
+	ofnode child;
+	int err;
+
+	ofnode_for_each_subnode(child, dev_ofnode(dev)) {
+		if (ofnode_get_property(child, "compatible", NULL))
+			continue;
+
+		err = device_bind_driver_to_node(dev,
+						 "zero_size_cells_bus_child_drv",
+						 "zero_size_cells_bus_child",
+						 child, NULL);
+		if (err) {
+			dev_err(dev, "%s: failed to bind %s\n", __func__,
+				ofnode_get_name(child));
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static const struct udevice_id zero_size_cells_bus_ids[] = {
+	{ .compatible = "sandbox,zero-size-cells-bus" },
+	{ }
+};
+
+U_BOOT_DRIVER(zero_size_cells_bus) = {
+	.name = "zero_size_cells_bus_drv",
+	.id = UCLASS_TEST_DUMMY,
+	.of_match = zero_size_cells_bus_ids,
+	.bind = zero_size_cells_bus_bind,
+};
+
+static int zero_size_cells_bus_child_bind(struct udevice *dev)
+{
+	ofnode child;
+	int err;
+
+	ofnode_for_each_subnode(child, dev_ofnode(dev)) {
+		err = lists_bind_fdt(dev, child, NULL, false);
+		if (err) {
+			dev_err(dev, "%s: lists_bind_fdt, err=%d\n",
+				__func__, err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+U_BOOT_DRIVER(zero_size_cells_bus_child_drv) = {
+	.name = "zero_size_cells_bus_child_drv",
+	.id = UCLASS_TEST_DUMMY,
+	.bind = zero_size_cells_bus_child_bind,
+};
+
 static int dm_test_fdt_translation(struct unit_test_state *uts)
 {
 	struct udevice *dev;
@@ -599,11 +658,19 @@ static int dm_test_fdt_translation(struct unit_test_state *uts)
 	ut_asserteq_str("dev at 2,200", dev->name);
 	ut_asserteq(0xA000, dev_read_addr(dev));
 
-	/* No translation for busses with #size-cells == 0 */
 	ut_assertok(uclass_find_device_by_seq(UCLASS_TEST_DUMMY, 3, true, &dev));
 	ut_asserteq_str("dev at 42", dev->name);
+	/* No translation for busses with #size-cells == 0 */
 	ut_asserteq(0x42, dev_read_addr(dev));
 
+	/* Translation for busses with #size-cells == 0 */
+	gd->dm_flags |= GD_DM_FLG_SIZE_CELLS_0;
+	ut_asserteq(0x8042, dev_read_addr(dev));
+	ut_assertok(uclass_find_device_by_seq(UCLASS_TEST_DUMMY, 4, true, &dev));
+	ut_asserteq_str("dev at 19", dev->name);
+	ut_asserteq(0xc019, dev_read_addr(dev));
+	gd->dm_flags &= ~GD_DM_FLG_SIZE_CELLS_0;
+
 	/* dma address translation */
 	ut_assertok(uclass_find_device_by_seq(UCLASS_TEST_DUMMY, 0, true, &dev));
 	dma_addr[0] = cpu_to_be32(0);
-- 
2.17.1

[PATCH v5 17/27] omap: timer: fix the rate setting

[Dario Binacchi]

The prescaler (PTV) setting must be taken into account even when the
timer input clock frequency has been set.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

(no changes since v1)

 drivers/timer/omap-timer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/timer/omap-timer.c b/drivers/timer/omap-timer.c
index 4eecb3e64d..c08cb5ad2e 100644
--- a/drivers/timer/omap-timer.c
+++ b/drivers/timer/omap-timer.c
@@ -19,8 +19,6 @@
 #define TCLR_PRE_EN			BIT(5)	/* Pre-scaler enable */
 #define TCLR_PTV_SHIFT			(2)	/* Pre-scaler shift value */
 
-#define TIMER_CLOCK             (V_SCLK / (2 << CONFIG_SYS_PTV))
-
 struct omap_gptimer_regs {
 	unsigned int tidr;		/* offset 0x00 */
 	unsigned char res1[12];
@@ -61,7 +59,9 @@ static int omap_timer_probe(struct udevice *dev)
 	struct omap_timer_priv *priv = dev_get_priv(dev);
 
 	if (!uc_priv->clock_rate)
-		uc_priv->clock_rate = TIMER_CLOCK;
+		uc_priv->clock_rate = V_SCLK;
+
+	uc_priv->clock_rate /= (2 << CONFIG_SYS_PTV);
 
 	/* start the counter ticking up, reload value on overflow */
 	writel(0, &priv->regs->tldr);
-- 
2.17.1

[PATCH v5 18/27] misc: am33xx: add control module driver

[Dario Binacchi]

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@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@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)) {
+			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 (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;
+		}
+	}
+
+	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);
+	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

[PATCH v5 19/27] pwm: ti: am33xx: add enhanced pwm driver

[Dario Binacchi]

Enhanced high resolution PWM module (EHRPWM) hardware can be used to
generate PWM output over 2 channels. This commit adds PWM driver support
for EHRPWM device present on AM33XX SOC.

The code is based on the drivers/pwm/pwm-tiehrpwm.c driver of the Linux
kernel version 5.9-rc7.
For DT binding details see:
- Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt

Signed-off-by: Dario Binacchi <dariobin@libero.it>

---

(no changes since v4)

Changes in v4:
- Include device_compat.h header for dev_xxx macros.

Changes in v3:
- Adds PWM_TI_EHRPWM dependency on ARCH_OMAP2PLUS in Kconfig.
- Add error message in case of invalid address.
- Remove doc/device-tree-bindings/pwm/ti,ehrpwm.txt.
- Add to commit message the references to linux kernel dt binding
  documentation.

 drivers/pwm/Kconfig         |   7 +
 drivers/pwm/Makefile        |   1 +
 drivers/pwm/pwm-ti-ehrpwm.c | 468 ++++++++++++++++++++++++++++++++++++
 3 files changed, 476 insertions(+)
 create mode 100644 drivers/pwm/pwm-ti-ehrpwm.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b3bd5c6bb7..ccf81abbe9 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -75,3 +75,10 @@ config PWM_SUNXI
 	help
 	  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_EHRPWM
+	bool "Enable support for EHRPWM PWM"
+	depends on DM_PWM && ARCH_OMAP2PLUS
+	default y
+	help
+	  PWM driver support for the EHRPWM controller found on TI SOCs.
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index f21ae7d76e..0b9d2698a3 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_PWM_SANDBOX)	+= sandbox_pwm.o
 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
diff --git a/drivers/pwm/pwm-ti-ehrpwm.c b/drivers/pwm/pwm-ti-ehrpwm.c
new file mode 100644
index 0000000000..df995c8865
--- /dev/null
+++ b/drivers/pwm/pwm-ti-ehrpwm.c
@@ -0,0 +1,468 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * EHRPWM PWM driver
+ *
+ * Copyright (C) 2020 Dario Binacchi <dariobin@libero.it>
+ *
+ * Based on Linux kernel drivers/pwm/pwm-tiehrpwm.c
+ */
+
+#include <common.h>
+#include <clk.h>
+#include <div64.h>
+#include <dm.h>
+#include <dm/device_compat.h>
+#include <pwm.h>
+#include <asm/io.h>
+
+#define NSEC_PER_SEC			        1000000000L
+
+/* Time base module registers */
+#define TI_EHRPWM_TBCTL				0x00
+#define TI_EHRPWM_TBPRD				0x0A
+
+#define TI_EHRPWM_TBCTL_PRDLD_MASK		BIT(3)
+#define TI_EHRPWM_TBCTL_PRDLD_SHDW		0
+#define TI_EHRPWM_TBCTL_PRDLD_IMDT		BIT(3)
+#define TI_EHRPWM_TBCTL_CLKDIV_MASK		GENMASK(12, 7)
+#define TI_EHRPWM_TBCTL_CTRMODE_MASK		GENMASK(1, 0)
+#define TI_EHRPWM_TBCTL_CTRMODE_UP		0
+#define TI_EHRPWM_TBCTL_CTRMODE_DOWN		BIT(0)
+#define TI_EHRPWM_TBCTL_CTRMODE_UPDOWN		BIT(1)
+#define TI_EHRPWM_TBCTL_CTRMODE_FREEZE		GENMASK(1, 0)
+
+#define TI_EHRPWM_TBCTL_HSPCLKDIV_SHIFT		7
+#define TI_EHRPWM_TBCTL_CLKDIV_SHIFT		10
+
+#define TI_EHRPWM_CLKDIV_MAX			7
+#define TI_EHRPWM_HSPCLKDIV_MAX			7
+#define TI_EHRPWM_PERIOD_MAX			0xFFFF
+
+/* Counter compare module registers */
+#define TI_EHRPWM_CMPA				0x12
+#define TI_EHRPWM_CMPB				0x14
+
+/* Action qualifier module registers */
+#define TI_EHRPWM_AQCTLA			0x16
+#define TI_EHRPWM_AQCTLB			0x18
+#define TI_EHRPWM_AQSFRC			0x1A
+#define TI_EHRPWM_AQCSFRC			0x1C
+
+#define TI_EHRPWM_AQCTL_CBU_MASK		GENMASK(9, 8)
+#define TI_EHRPWM_AQCTL_CBU_FRCLOW		BIT(8)
+#define TI_EHRPWM_AQCTL_CBU_FRCHIGH		BIT(9)
+#define TI_EHRPWM_AQCTL_CBU_FRCTOGGLE		GENMASK(9, 8)
+#define TI_EHRPWM_AQCTL_CAU_MASK		GENMASK(5, 4)
+#define TI_EHRPWM_AQCTL_CAU_FRCLOW		BIT(4)
+#define TI_EHRPWM_AQCTL_CAU_FRCHIGH		BIT(5)
+#define TI_EHRPWM_AQCTL_CAU_FRCTOGGLE		GENMASK(5, 4)
+#define TI_EHRPWM_AQCTL_PRD_MASK		GENMASK(3, 2)
+#define TI_EHRPWM_AQCTL_PRD_FRCLOW		BIT(2)
+#define TI_EHRPWM_AQCTL_PRD_FRCHIGH		BIT(3)
+#define TI_EHRPWM_AQCTL_PRD_FRCTOGGLE		GENMASK(3, 2)
+#define TI_EHRPWM_AQCTL_ZRO_MASK		GENMASK(1, 0)
+#define TI_EHRPWM_AQCTL_ZRO_FRCLOW		BIT(0)
+#define TI_EHRPWM_AQCTL_ZRO_FRCHIGH		BIT(1)
+#define TI_EHRPWM_AQCTL_ZRO_FRCTOGGLE		GENMASK(1, 0)
+
+#define TI_EHRPWM_AQCTL_CHANA_POLNORMAL		(TI_EHRPWM_AQCTL_CAU_FRCLOW | \
+						 TI_EHRPWM_AQCTL_PRD_FRCHIGH | \
+						 TI_EHRPWM_AQCTL_ZRO_FRCHIGH)
+#define TI_EHRPWM_AQCTL_CHANA_POLINVERSED	(TI_EHRPWM_AQCTL_CAU_FRCHIGH | \
+						 TI_EHRPWM_AQCTL_PRD_FRCLOW | \
+						 TI_EHRPWM_AQCTL_ZRO_FRCLOW)
+#define TI_EHRPWM_AQCTL_CHANB_POLNORMAL		(TI_EHRPWM_AQCTL_CBU_FRCLOW | \
+						 TI_EHRPWM_AQCTL_PRD_FRCHIGH | \
+						 TI_EHRPWM_AQCTL_ZRO_FRCHIGH)
+#define TI_EHRPWM_AQCTL_CHANB_POLINVERSED	(TI_EHRPWM_AQCTL_CBU_FRCHIGH | \
+						 TI_EHRPWM_AQCTL_PRD_FRCLOW | \
+						 TI_EHRPWM_AQCTL_ZRO_FRCLOW)
+
+#define TI_EHRPWM_AQSFRC_RLDCSF_MASK		GENMASK(7, 6)
+#define TI_EHRPWM_AQSFRC_RLDCSF_ZRO		0
+#define TI_EHRPWM_AQSFRC_RLDCSF_PRD		BIT(6)
+#define TI_EHRPWM_AQSFRC_RLDCSF_ZROPRD		BIT(7)
+#define TI_EHRPWM_AQSFRC_RLDCSF_IMDT		GENMASK(7, 6)
+
+#define TI_EHRPWM_AQCSFRC_CSFB_MASK		GENMASK(3, 2)
+#define TI_EHRPWM_AQCSFRC_CSFB_FRCDIS		0
+#define TI_EHRPWM_AQCSFRC_CSFB_FRCLOW		BIT(2)
+#define TI_EHRPWM_AQCSFRC_CSFB_FRCHIGH		BIT(3)
+#define TI_EHRPWM_AQCSFRC_CSFB_DISSWFRC		GENMASK(3, 2)
+#define TI_EHRPWM_AQCSFRC_CSFA_MASK		GENMASK(1, 0)
+#define TI_EHRPWM_AQCSFRC_CSFA_FRCDIS		0
+#define TI_EHRPWM_AQCSFRC_CSFA_FRCLOW		BIT(0)
+#define TI_EHRPWM_AQCSFRC_CSFA_FRCHIGH		BIT(1)
+#define TI_EHRPWM_AQCSFRC_CSFA_DISSWFRC		GENMASK(1, 0)
+
+#define TI_EHRPWM_NUM_CHANNELS                  2
+
+struct ti_ehrpwm_priv {
+	fdt_addr_t regs;
+	u32 clk_rate;
+	struct clk tbclk;
+	unsigned long period_cycles[TI_EHRPWM_NUM_CHANNELS];
+	bool polarity_reversed[TI_EHRPWM_NUM_CHANNELS];
+};
+
+static void ti_ehrpwm_modify(u16 val, u16 mask, fdt_addr_t reg)
+{
+	unsigned short v;
+
+	v = readw(reg);
+	v &= ~mask;
+	v |= val & mask;
+	writew(v, reg);
+}
+
+static int ti_ehrpwm_set_invert(struct udevice *dev, uint channel,
+				bool polarity)
+{
+	struct ti_ehrpwm_priv *priv = dev_get_priv(dev);
+
+	if (channel >= TI_EHRPWM_NUM_CHANNELS)
+		return -ENOSPC;
+
+	/* Configuration of polarity in hardware delayed, do at enable */
+	priv->polarity_reversed[channel] = polarity;
+	return 0;
+}
+
+/**
+ * set_prescale_div -	Set up the prescaler divider function
+ * @rqst_prescaler:	prescaler value min
+ * @prescale_div:	prescaler value set
+ * @tb_clk_div:		Time Base Control prescaler bits
+ */
+static int set_prescale_div(unsigned long rqst_prescaler, u16 *prescale_div,
+			    u16 *tb_clk_div)
+{
+	unsigned int clkdiv, hspclkdiv;
+
+	for (clkdiv = 0; clkdiv <= TI_EHRPWM_CLKDIV_MAX; clkdiv++) {
+		for (hspclkdiv = 0; hspclkdiv <= TI_EHRPWM_HSPCLKDIV_MAX;
+		     hspclkdiv++) {
+			/*
+			 * calculations for prescaler value :
+			 * prescale_div = HSPCLKDIVIDER * CLKDIVIDER.
+			 * HSPCLKDIVIDER =  2 ** hspclkdiv
+			 * CLKDIVIDER = (1),            if clkdiv == 0 *OR*
+			 *              (2 * clkdiv),   if clkdiv != 0
+			 *
+			 * Configure prescale_div value such that period
+			 * register value is less than 65535.
+			 */
+
+			*prescale_div = (1 << clkdiv) *
+				(hspclkdiv ? (hspclkdiv * 2) : 1);
+			if (*prescale_div > rqst_prescaler) {
+				*tb_clk_div =
+				    (clkdiv << TI_EHRPWM_TBCTL_CLKDIV_SHIFT) |
+				    (hspclkdiv <<
+				     TI_EHRPWM_TBCTL_HSPCLKDIV_SHIFT);
+				return 0;
+			}
+		}
+	}
+
+	return 1;
+}
+
+static void ti_ehrpwm_configure_polarity(struct udevice *dev, uint channel)
+{
+	struct ti_ehrpwm_priv *priv = dev_get_priv(dev);
+	u16 aqctl_val, aqctl_mask;
+	unsigned int aqctl_reg;
+
+	/*
+	 * Configure PWM output to HIGH/LOW level on counter
+	 * reaches compare register value and LOW/HIGH level
+	 * on counter value reaches period register value and
+	 * zero value on counter
+	 */
+	if (channel == 1) {
+		aqctl_reg = TI_EHRPWM_AQCTLB;
+		aqctl_mask = TI_EHRPWM_AQCTL_CBU_MASK;
+
+		if (priv->polarity_reversed[channel])
+			aqctl_val = TI_EHRPWM_AQCTL_CHANB_POLINVERSED;
+		else
+			aqctl_val = TI_EHRPWM_AQCTL_CHANB_POLNORMAL;
+	} else {
+		aqctl_reg = TI_EHRPWM_AQCTLA;
+		aqctl_mask = TI_EHRPWM_AQCTL_CAU_MASK;
+
+		if (priv->polarity_reversed[channel])
+			aqctl_val = TI_EHRPWM_AQCTL_CHANA_POLINVERSED;
+		else
+			aqctl_val = TI_EHRPWM_AQCTL_CHANA_POLNORMAL;
+	}
+
+	aqctl_mask |= TI_EHRPWM_AQCTL_PRD_MASK | TI_EHRPWM_AQCTL_ZRO_MASK;
+	ti_ehrpwm_modify(aqctl_val, aqctl_mask, priv->regs + aqctl_reg);
+}
+
+/*
+ * period_ns = 10^9 * (ps_divval * period_cycles) / PWM_CLK_RATE
+ * duty_ns   = 10^9 * (ps_divval * duty_cycles) / PWM_CLK_RATE
+ */
+static int ti_ehrpwm_set_config(struct udevice *dev, uint channel,
+				uint period_ns, uint duty_ns)
+{
+	struct ti_ehrpwm_priv *priv = dev_get_priv(dev);
+	u32 period_cycles, duty_cycles;
+	u16 ps_divval, tb_divval;
+	unsigned int i, cmp_reg;
+	unsigned long long c;
+
+	if (channel >= TI_EHRPWM_NUM_CHANNELS)
+		return -ENOSPC;
+
+	if (period_ns > NSEC_PER_SEC)
+		return -ERANGE;
+
+	c = priv->clk_rate;
+	c = c * period_ns;
+	do_div(c, NSEC_PER_SEC);
+	period_cycles = (unsigned long)c;
+
+	if (period_cycles < 1) {
+		period_cycles = 1;
+		duty_cycles = 1;
+	} else {
+		c = priv->clk_rate;
+		c = c * duty_ns;
+		do_div(c, NSEC_PER_SEC);
+		duty_cycles = (unsigned long)c;
+	}
+
+	dev_dbg(dev, "channel=%d, period_ns=%d, duty_ns=%d\n",
+		channel, period_ns, duty_ns);
+
+	/*
+	 * Period values should be same for multiple PWM channels as IP uses
+	 * same period register for multiple channels.
+	 */
+	for (i = 0; i < TI_EHRPWM_NUM_CHANNELS; i++) {
+		if (priv->period_cycles[i] &&
+		    priv->period_cycles[i] != period_cycles) {
+			/*
+			 * Allow channel to reconfigure period if no other
+			 * channels being configured.
+			 */
+			if (i == channel)
+				continue;
+
+			dev_err(dev, "period value conflicts with channel %u\n",
+				i);
+			return -EINVAL;
+		}
+	}
+
+	priv->period_cycles[channel] = period_cycles;
+
+	/* Configure clock prescaler to support Low frequency PWM wave */
+	if (set_prescale_div(period_cycles / TI_EHRPWM_PERIOD_MAX, &ps_divval,
+			     &tb_divval)) {
+		dev_err(dev, "unsupported values\n");
+		return -EINVAL;
+	}
+
+	/* Update clock prescaler values */
+	ti_ehrpwm_modify(tb_divval, TI_EHRPWM_TBCTL_CLKDIV_MASK,
+			 priv->regs + TI_EHRPWM_TBCTL);
+
+	/* Update period & duty cycle with presacler division */
+	period_cycles = period_cycles / ps_divval;
+	duty_cycles = duty_cycles / ps_divval;
+
+	/* Configure shadow loading on Period register */
+	ti_ehrpwm_modify(TI_EHRPWM_TBCTL_PRDLD_SHDW, TI_EHRPWM_TBCTL_PRDLD_MASK,
+			 priv->regs + TI_EHRPWM_TBCTL);
+
+	writew(period_cycles, priv->regs + TI_EHRPWM_TBPRD);
+
+	/* Configure ehrpwm counter for up-count mode */
+	ti_ehrpwm_modify(TI_EHRPWM_TBCTL_CTRMODE_UP,
+			 TI_EHRPWM_TBCTL_CTRMODE_MASK,
+			 priv->regs + TI_EHRPWM_TBCTL);
+
+	if (channel == 1)
+		/* Channel 1 configured with compare B register */
+		cmp_reg = TI_EHRPWM_CMPB;
+	else
+		/* Channel 0 configured with compare A register */
+		cmp_reg = TI_EHRPWM_CMPA;
+
+	writew(duty_cycles, priv->regs + cmp_reg);
+	return 0;
+}
+
+static int ti_ehrpwm_disable(struct udevice *dev, uint channel)
+{
+	struct ti_ehrpwm_priv *priv = dev_get_priv(dev);
+	u16 aqcsfrc_val, aqcsfrc_mask;
+	int err;
+
+	if (channel >= TI_EHRPWM_NUM_CHANNELS)
+		return -ENOSPC;
+
+	/* Action Qualifier puts PWM output low forcefully */
+	if (channel) {
+		aqcsfrc_val = TI_EHRPWM_AQCSFRC_CSFB_FRCLOW;
+		aqcsfrc_mask = TI_EHRPWM_AQCSFRC_CSFB_MASK;
+	} else {
+		aqcsfrc_val = TI_EHRPWM_AQCSFRC_CSFA_FRCLOW;
+		aqcsfrc_mask = TI_EHRPWM_AQCSFRC_CSFA_MASK;
+	}
+
+	/* Update shadow register first before modifying active register */
+	ti_ehrpwm_modify(TI_EHRPWM_AQSFRC_RLDCSF_ZRO,
+			 TI_EHRPWM_AQSFRC_RLDCSF_MASK,
+			 priv->regs + TI_EHRPWM_AQSFRC);
+
+	ti_ehrpwm_modify(aqcsfrc_val, aqcsfrc_mask,
+			 priv->regs + TI_EHRPWM_AQCSFRC);
+
+	/*
+	 * Changes to immediate action on Action Qualifier. This puts
+	 * Action Qualifier control on PWM output from next TBCLK
+	 */
+	ti_ehrpwm_modify(TI_EHRPWM_AQSFRC_RLDCSF_IMDT,
+			 TI_EHRPWM_AQSFRC_RLDCSF_MASK,
+			 priv->regs + TI_EHRPWM_AQSFRC);
+
+	ti_ehrpwm_modify(aqcsfrc_val, aqcsfrc_mask,
+			 priv->regs + TI_EHRPWM_AQCSFRC);
+
+	/* Disabling TBCLK on PWM disable */
+	err = clk_disable(&priv->tbclk);
+	if (err) {
+		dev_err(dev, "failed to disable tbclk\n");
+		return err;
+	}
+
+	return 0;
+}
+
+static int ti_ehrpwm_enable(struct udevice *dev, uint channel)
+{
+	struct ti_ehrpwm_priv *priv = dev_get_priv(dev);
+	u16 aqcsfrc_val, aqcsfrc_mask;
+	int err;
+
+	if (channel >= TI_EHRPWM_NUM_CHANNELS)
+		return -ENOSPC;
+
+	/* Disabling Action Qualifier on PWM output */
+	if (channel) {
+		aqcsfrc_val = TI_EHRPWM_AQCSFRC_CSFB_FRCDIS;
+		aqcsfrc_mask = TI_EHRPWM_AQCSFRC_CSFB_MASK;
+	} else {
+		aqcsfrc_val = TI_EHRPWM_AQCSFRC_CSFA_FRCDIS;
+		aqcsfrc_mask = TI_EHRPWM_AQCSFRC_CSFA_MASK;
+	}
+
+	/* Changes to shadow mode */
+	ti_ehrpwm_modify(TI_EHRPWM_AQSFRC_RLDCSF_ZRO,
+			 TI_EHRPWM_AQSFRC_RLDCSF_MASK,
+			 priv->regs + TI_EHRPWM_AQSFRC);
+
+	ti_ehrpwm_modify(aqcsfrc_val, aqcsfrc_mask,
+			 priv->regs + TI_EHRPWM_AQCSFRC);
+
+	/* Channels polarity can be configured from action qualifier module */
+	ti_ehrpwm_configure_polarity(dev, channel);
+
+	err = clk_enable(&priv->tbclk);
+	if (err) {
+		dev_err(dev, "failed to enable tbclk\n");
+		return err;
+	}
+
+	return 0;
+}
+
+static int ti_ehrpwm_set_enable(struct udevice *dev, uint channel, bool enable)
+{
+	if (enable)
+		return ti_ehrpwm_enable(dev, channel);
+
+	return ti_ehrpwm_disable(dev, channel);
+}
+
+static int ti_ehrpwm_ofdata_to_platdata(struct udevice *dev)
+{
+	struct ti_ehrpwm_priv *priv = dev_get_priv(dev);
+
+	priv->regs = dev_read_addr(dev);
+	if (priv->regs == FDT_ADDR_T_NONE) {
+		dev_err(dev, "invalid address\n");
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "regs=0x%08lx\n", priv->regs);
+	return 0;
+}
+
+static int ti_ehrpwm_remove(struct udevice *dev)
+{
+	struct ti_ehrpwm_priv *priv = dev_get_priv(dev);
+
+	clk_release_all(&priv->tbclk, 1);
+	return 0;
+}
+
+static int ti_ehrpwm_probe(struct udevice *dev)
+{
+	struct ti_ehrpwm_priv *priv = dev_get_priv(dev);
+	struct clk clk;
+	int err;
+
+	err = clk_get_by_name(dev, "fck", &clk);
+	if (err) {
+		dev_err(dev, "failed to get clock\n");
+		return err;
+	}
+
+	priv->clk_rate = clk_get_rate(&clk);
+	if (IS_ERR_VALUE(priv->clk_rate) || !priv->clk_rate) {
+		dev_err(dev, "failed to get clock rate\n");
+		if (IS_ERR_VALUE(priv->clk_rate))
+			return priv->clk_rate;
+
+		return -EINVAL;
+	}
+
+	/* Acquire tbclk for Time Base EHRPWM submodule */
+	err = clk_get_by_name(dev, "tbclk", &priv->tbclk);
+	if (err) {
+		dev_err(dev, "failed to get tbclk clock\n");
+		return err;
+	}
+
+	return 0;
+}
+
+static const struct pwm_ops ti_ehrpwm_ops = {
+	.set_config = ti_ehrpwm_set_config,
+	.set_enable = ti_ehrpwm_set_enable,
+	.set_invert = ti_ehrpwm_set_invert,
+};
+
+static const struct udevice_id ti_ehrpwm_ids[] = {
+	{.compatible = "ti,am3352-ehrpwm"},
+	{.compatible = "ti,am33xx-ehrpwm"},
+	{}
+};
+
+U_BOOT_DRIVER(ti_ehrpwm) = {
+	.name = "ti_ehrpwm",
+	.id = UCLASS_PWM,
+	.of_match = ti_ehrpwm_ids,
+	.ops = &ti_ehrpwm_ops,
+	.ofdata_to_platdata = ti_ehrpwm_ofdata_to_platdata,
+	.probe = ti_ehrpwm_probe,
+	.remove = ti_ehrpwm_remove,
+	.priv_auto_alloc_size = sizeof(struct ti_ehrpwm_priv),
+};
-- 
2.17.1

[PATCH v5 20/27] bus: ti: am33xx: add pwm subsystem driver

[Dario Binacchi]

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.

For DT binding details see Linux doc:
- Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt

Signed-off-by: Dario Binacchi <dariobin@libero.it>

---

(no changes since v3)

Changes in v3:
- Move Kconfig symbol from drivers/pwm to drivers/bus.
- Remove the domain clock reference from the pwmss nodes of the device
  tree in am33xx.dtsi. The resync of am33xx.dtsi with Linux 5.9-rc7
  already contains such references.
- Remove domain clock enabling/disabling. Enabling the domain clock is
  performed by the sysc interconnect target module driver during the pwm
  device probing.
- Remove doc/device-tree-bindings/pwm/ti,pwmss.txt.
- Add to commit message the references to linux kernel dt binding
  documentation.

 drivers/bus/Kconfig    |  6 ++++++
 drivers/bus/Makefile   |  1 +
 drivers/bus/ti-pwmss.c | 21 +++++++++++++++++++++
 3 files changed, 28 insertions(+)
 create mode 100644 drivers/bus/ti-pwmss.c

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 733bec5a56..d742ed333b 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -5,6 +5,12 @@
 
 menu "Bus devices"
 
+config TI_PWMSS
+	bool
+	default y if ARCH_OMAP2PLUS && PWM_TI_EHRPWM
+	help
+	  PWM Subsystem driver support for AM33xx SOC.
+
 config TI_SYSC
 	bool "TI sysc interconnect target module driver"
 	depends on ARCH_OMAP2PLUS
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index 875bb4ed42..a2e71c7b3b 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -3,5 +3,6 @@
 # Makefile for the bus drivers.
 #
 
+obj-$(CONFIG_TI_PWMSS)	+= ti-pwmss.o
 obj-$(CONFIG_TI_SYSC)	+= ti-sysc.o
 obj-$(CONFIG_UNIPHIER_SYSTEM_BUS) += uniphier-system-bus.o
diff --git a/drivers/bus/ti-pwmss.c b/drivers/bus/ti-pwmss.c
new file mode 100644
index 0000000000..265b4cf83b
--- /dev/null
+++ b/drivers/bus/ti-pwmss.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Pulse-Width Modulation Subsystem (pwmss)
+ *
+ * Copyright (C) 2020 Dario Binacchi <dariobin@libero.it>
+ */
+
+#include <common.h>
+#include <dm.h>
+
+static const struct udevice_id ti_pwmss_ids[] = {
+	{.compatible = "ti,am33xx-pwmss"},
+	{}
+};
+
+U_BOOT_DRIVER(ti_pwmss) = {
+	.name = "ti_pwmss",
+	.id = UCLASS_SIMPLE_BUS,
+	.of_match = ti_pwmss_ids,
+	.bind = dm_scan_fdt_dev,
+};
-- 
2.17.1

[PATCH v5 21/27] dm: core: add a function to decode display timings

[Dario Binacchi]

The patch adds a function to get display timings from the device tree
node attached to the device.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 arch/sandbox/dts/test.dts | 46 ++++++++++++++++++++++
 drivers/core/read.c       |  6 +++
 include/dm/read.h         | 24 ++++++++++++
 test/dm/test-fdt.c        | 80 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 156 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 4a7a28559a..b3c1a7f2fd 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -134,6 +134,52 @@
 		interrupts-extended = <&irq 3 0>;
 		acpi,name = "GHIJ";
 		phandle-value = <&gpio_c 10>, <0xFFFFFFFF 20>, <&gpio_a 30>;
+		display-timings {
+			timing0: 240x320 {
+				clock-frequency = <6500000>;
+				hactive = <240>;
+				vactive = <320>;
+				hfront-porch = <6>;
+				hback-porch = <7>;
+				hsync-len = <1>;
+				vback-porch = <5>;
+				vfront-porch = <8>;
+				vsync-len = <2>;
+				hsync-active = <1>;
+				vsync-active = <0>;
+				de-active = <1>;
+				pixelclk-active = <1>;
+				interlaced;
+				doublescan;
+				doubleclk;
+			};
+			timing1: 480x800 {
+				clock-frequency = <9000000>;
+				hactive = <480>;
+				vactive = <800>;
+				hfront-porch = <10>;
+				hback-porch = <59>;
+				hsync-len = <12>;
+				vback-porch = <15>;
+				vfront-porch = <17>;
+				vsync-len = <16>;
+				hsync-active = <0>;
+				vsync-active = <1>;
+				de-active = <0>;
+				pixelclk-active = <0>;
+			};
+			timing2: 800x480 {
+				clock-frequency = <33500000>;
+				hactive = <800>;
+				vactive = <480>;
+				hback-porch = <89>;
+				hfront-porch = <164>;
+				vback-porch = <23>;
+				vfront-porch = <10>;
+				hsync-len = <11>;
+				vsync-len = <13>;
+			};
+		};
 	};
 
 	junk {
diff --git a/drivers/core/read.c b/drivers/core/read.c
index 076125824c..7925c09f60 100644
--- a/drivers/core/read.c
+++ b/drivers/core/read.c
@@ -377,3 +377,9 @@ int dev_read_pci_bus_range(const struct udevice *dev,
 
 	return 0;
 }
+
+int dev_decode_display_timing(const struct udevice *dev, int index,
+			      struct display_timing *config)
+{
+	return ofnode_decode_display_timing(dev_ofnode(dev), index, config);
+}
diff --git a/include/dm/read.h b/include/dm/read.h
index 0585eb1228..0ac26d9f1d 100644
--- a/include/dm/read.h
+++ b/include/dm/read.h
@@ -694,6 +694,23 @@ int dev_get_child_count(const struct udevice *dev);
  */
 int dev_read_pci_bus_range(const struct udevice *dev, struct resource *res);
 
+/**
+ * dev_decode_display_timing() - decode display timings
+ *
+ * Decode display timings from the supplied 'display-timings' node.
+ * See doc/device-tree-bindings/video/display-timing.txt for binding
+ * information.
+ *
+ * @dev: device to read DT display timings from. The node linked to the device
+ *       contains a child node called 'display-timings' which in turn contains
+ *       one or more display timing nodes.
+ * @index: index number to read (0=first timing subnode)
+ * @config: place to put timings
+ * @return 0 if OK, -FDT_ERR_NOTFOUND if not found
+ */
+int dev_decode_display_timing(const struct udevice *dev, int index,
+			      struct display_timing *config);
+
 #else /* CONFIG_DM_DEV_READ_INLINE is enabled */
 
 static inline int dev_read_u32(const struct udevice *dev,
@@ -1016,6 +1033,13 @@ static inline int dev_get_child_count(const struct udevice *dev)
 	return ofnode_get_child_count(dev_ofnode(dev));
 }
 
+static inline int dev_decode_display_timing(const struct udevice *dev,
+					    int index,
+					    struct display_timing *config)
+{
+	return ofnode_decode_display_timing(dev_ofnode(dev), index, config);
+}
+
 #endif /* CONFIG_DM_DEV_READ_INLINE */
 
 /**
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
index dd18160cbe..a0aab9e181 100644
--- a/test/dm/test-fdt.c
+++ b/test/dm/test-fdt.c
@@ -1183,3 +1183,83 @@ static int dm_test_ofdata_order(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_ofdata_order, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+/* Test dev_decode_display_timing() */
+static int dm_test_decode_display_timing(struct unit_test_state *uts)
+{
+	struct udevice *dev;
+	struct display_timing timing;
+
+	ut_assertok(uclass_first_device_err(UCLASS_TEST_FDT, &dev));
+	ut_asserteq_str("a-test", dev->name);
+
+	ut_assertok(dev_decode_display_timing(dev, 0, &timing));
+	ut_assert(timing.hactive.typ == 240);
+	ut_assert(timing.hback_porch.typ == 7);
+	ut_assert(timing.hfront_porch.typ == 6);
+	ut_assert(timing.hsync_len.typ == 1);
+	ut_assert(timing.vactive.typ == 320);
+	ut_assert(timing.vback_porch.typ == 5);
+	ut_assert(timing.vfront_porch.typ == 8);
+	ut_assert(timing.vsync_len.typ == 2);
+	ut_assert(timing.pixelclock.typ == 6500000);
+	ut_assert(timing.flags & DISPLAY_FLAGS_HSYNC_HIGH);
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_HSYNC_LOW));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_VSYNC_HIGH));
+	ut_assert(timing.flags & DISPLAY_FLAGS_VSYNC_LOW);
+	ut_assert(timing.flags & DISPLAY_FLAGS_DE_HIGH);
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_DE_LOW));
+	ut_assert(timing.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE);
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE));
+	ut_assert(timing.flags & DISPLAY_FLAGS_INTERLACED);
+	ut_assert(timing.flags & DISPLAY_FLAGS_DOUBLESCAN);
+	ut_assert(timing.flags & DISPLAY_FLAGS_DOUBLECLK);
+
+	ut_assertok(dev_decode_display_timing(dev, 1, &timing));
+	ut_assert(timing.hactive.typ == 480);
+	ut_assert(timing.hback_porch.typ == 59);
+	ut_assert(timing.hfront_porch.typ == 10);
+	ut_assert(timing.hsync_len.typ == 12);
+	ut_assert(timing.vactive.typ == 800);
+	ut_assert(timing.vback_porch.typ == 15);
+	ut_assert(timing.vfront_porch.typ == 17);
+	ut_assert(timing.vsync_len.typ == 16);
+	ut_assert(timing.pixelclock.typ == 9000000);
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_HSYNC_HIGH));
+	ut_assert(timing.flags & DISPLAY_FLAGS_HSYNC_LOW);
+	ut_assert(timing.flags & DISPLAY_FLAGS_VSYNC_HIGH);
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_VSYNC_LOW));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_DE_HIGH));
+	ut_assert(timing.flags & DISPLAY_FLAGS_DE_LOW);
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE));
+	ut_assert(timing.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE);
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_INTERLACED));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_DOUBLESCAN));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_DOUBLECLK));
+
+	ut_assertok(dev_decode_display_timing(dev, 2, &timing));
+	ut_assert(timing.hactive.typ == 800);
+	ut_assert(timing.hback_porch.typ == 89);
+	ut_assert(timing.hfront_porch.typ == 164);
+	ut_assert(timing.hsync_len.typ == 11);
+	ut_assert(timing.vactive.typ == 480);
+	ut_assert(timing.vback_porch.typ == 23);
+	ut_assert(timing.vfront_porch.typ == 10);
+	ut_assert(timing.vsync_len.typ == 13);
+	ut_assert(timing.pixelclock.typ == 33500000);
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_HSYNC_HIGH));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_HSYNC_LOW));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_VSYNC_HIGH));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_VSYNC_LOW));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_DE_HIGH));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_DE_LOW));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_INTERLACED));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_DOUBLESCAN));
+	ut_assert(!(timing.flags & DISPLAY_FLAGS_DOUBLECLK));
+
+	ut_assert(dev_decode_display_timing(dev, 3, &timing));
+	return 0;
+}
+DM_TEST(dm_test_decode_display_timing, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
-- 
2.17.1

[PATCH v5 22/27] video: omap: add panel driver

[Dario Binacchi]

The previous version of am335x-fb.c contained the functionalities of two
drivers that this patch has split. It was a video type driver that used
the same registration compatible string that now registers a panel type
driver. The proof of this is that two compatible strings were referred
to within the same driver.
There are now two drivers, each with its own compatible string,
functions and API.
Furthermore, the panel driver, in addition to decoding the display
timings, is now also able to manage the backlight.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
Reviewed-by: Simon Glass <sjg@chromium.org>

---

(no changes since v4)

Changes in v4:
- Include device_compat.h header for dev_xxx macros.
- Add Simon Glass review.

Changes in v3:
- Update the DTS lcdc node of the am335x boards because of the
  am33xx.dtsi resynced with Linux 5.9-rc7.

 arch/arm/dts/am335x-brppt1-mmc.dts       |  17 +-
 arch/arm/dts/am335x-brppt1-nand.dts      |  17 +-
 arch/arm/dts/am335x-brppt1-spi.dts       |  17 +-
 arch/arm/dts/am335x-brsmarc1.dts         |  20 +-
 arch/arm/dts/am335x-brxre1.dts           |  21 +-
 arch/arm/dts/am335x-evm-u-boot.dtsi      |  15 +-
 arch/arm/dts/am335x-evmsk-u-boot.dtsi    |  14 +-
 arch/arm/dts/am335x-guardian-u-boot.dtsi |  18 +-
 arch/arm/dts/am335x-pdu001-u-boot.dtsi   |  18 +-
 arch/arm/dts/am335x-pxm50-u-boot.dtsi    |  14 +-
 arch/arm/dts/am335x-rut-u-boot.dtsi      |  14 +-
 arch/arm/dts/da850-evm-u-boot.dtsi       |  18 +-
 drivers/video/Makefile                   |   1 +
 drivers/video/am335x-fb.c                | 255 ++++++++++-------------
 drivers/video/am335x-fb.h                |  31 +++
 drivers/video/tilcdc-panel.c             | 172 +++++++++++++++
 drivers/video/tilcdc-panel.h             |  14 ++
 17 files changed, 478 insertions(+), 198 deletions(-)
 create mode 100644 drivers/video/tilcdc-panel.c
 create mode 100644 drivers/video/tilcdc-panel.h

diff --git a/arch/arm/dts/am335x-brppt1-mmc.dts b/arch/arm/dts/am335x-brppt1-mmc.dts
index 6f919711f0..bd2f6c2e3e 100644
--- a/arch/arm/dts/am335x-brppt1-mmc.dts
+++ b/arch/arm/dts/am335x-brppt1-mmc.dts
@@ -53,8 +53,6 @@
 		bkl-pwm = <&pwmbacklight>;
 		bkl-tps = <&tps_bl>;
 
-		u-boot,dm-pre-reloc;
-
 		panel-info {
 			ac-bias		= <255>;
 			ac-bias-intrpt	= <0>;
@@ -238,8 +236,19 @@
 	status = "okay";
 };
 
-&lcdc {
-	status = "disabled";
+&l4_per {
+
+	segment at 300000 {
+
+		target-module at e000 {
+			u-boot,dm-pre-reloc;
+
+			lcdc: lcdc at 0 {
+				u-boot,dm-pre-reloc;
+				status = "disabled";
+			};
+		};
+	};
 };
 
 &elm {
diff --git a/arch/arm/dts/am335x-brppt1-nand.dts b/arch/arm/dts/am335x-brppt1-nand.dts
index 9d4340f591..67c609739f 100644
--- a/arch/arm/dts/am335x-brppt1-nand.dts
+++ b/arch/arm/dts/am335x-brppt1-nand.dts
@@ -53,8 +53,6 @@
 		bkl-pwm = <&pwmbacklight>;
 		bkl-tps = <&tps_bl>;
 
-		u-boot,dm-pre-reloc;
-
 		panel-info {
 			ac-bias		= <255>;
 			ac-bias-intrpt	= <0>;
@@ -228,8 +226,19 @@
 	status = "disabled";
 };
 
-&lcdc {
-	status = "disabled";
+&l4_per {
+
+	segment at 300000 {
+
+		target-module at e000 {
+			u-boot,dm-pre-reloc;
+
+			lcdc: lcdc at 0 {
+				u-boot,dm-pre-reloc;
+				status = "disabled";
+			};
+		};
+	};
 };
 
 &elm {
diff --git a/arch/arm/dts/am335x-brppt1-spi.dts b/arch/arm/dts/am335x-brppt1-spi.dts
index c078af8fba..ce3dce204d 100644
--- a/arch/arm/dts/am335x-brppt1-spi.dts
+++ b/arch/arm/dts/am335x-brppt1-spi.dts
@@ -54,8 +54,6 @@
 		bkl-pwm = <&pwmbacklight>;
 		bkl-tps = <&tps_bl>;
 
-		u-boot,dm-pre-reloc;
-
 		panel-info {
 			ac-bias		= <255>;
 			ac-bias-intrpt	= <0>;
@@ -259,8 +257,19 @@
 	status = "okay";
 };
 
-&lcdc {
-	status = "disabled";
+&l4_per {
+
+	segment at 300000 {
+
+		target-module at e000 {
+			u-boot,dm-pre-reloc;
+
+			lcdc: lcdc at 0 {
+				u-boot,dm-pre-reloc;
+				status = "disabled";
+			};
+		};
+	};
 };
 
 &elm {
diff --git a/arch/arm/dts/am335x-brsmarc1.dts b/arch/arm/dts/am335x-brsmarc1.dts
index 7e9516e8f8..25cdb11164 100644
--- a/arch/arm/dts/am335x-brsmarc1.dts
+++ b/arch/arm/dts/am335x-brsmarc1.dts
@@ -59,7 +59,6 @@
 		/*backlight = <&tps_bl>; */
 		compatible = "ti,tilcdc,panel";
 		status = "okay";
-		u-boot,dm-pre-reloc;
 
 		panel-info {
 			ac-bias		= <255>;
@@ -298,10 +297,21 @@
 	status = "okay";
 };
 
-&lcdc {
-	status = "okay";
-	ti,no-reset-on-init;
-	ti,no-idle-on-init;
+&l4_per {
+
+	segment at 300000 {
+
+		target-module at e000 {
+			u-boot,dm-pre-reloc;
+
+			lcdc: lcdc at 0 {
+				u-boot,dm-pre-reloc;
+				status = "okay";
+				ti,no-reset-on-init;
+				ti,no-idle-on-init;
+			};
+		};
+	};
 };
 
 &elm {
diff --git a/arch/arm/dts/am335x-brxre1.dts b/arch/arm/dts/am335x-brxre1.dts
index 6091a12fb7..485c8e3613 100644
--- a/arch/arm/dts/am335x-brxre1.dts
+++ b/arch/arm/dts/am335x-brxre1.dts
@@ -79,8 +79,6 @@
 
 		backlight = <&tps_bl>;
 
-		u-boot,dm-pre-reloc;
-
 		panel-info {
 			ac-bias		= <255>;
 			ac-bias-intrpt	= <0>;
@@ -254,10 +252,21 @@
 	status = "okay";
 };
 
-&lcdc {
-	status = "okay";
-	ti,no-reset-on-init;
-	ti,no-idle-on-init;
+&l4_per {
+
+	segment at 300000 {
+
+		target-module at e000 {
+			u-boot,dm-pre-reloc;
+
+			lcdc: lcdc at 0 {
+				u-boot,dm-pre-reloc;
+				status = "okay";
+				ti,no-reset-on-init;
+				ti,no-idle-on-init;
+			};
+		};
+	};
 };
 
 &elm {
diff --git a/arch/arm/dts/am335x-evm-u-boot.dtsi b/arch/arm/dts/am335x-evm-u-boot.dtsi
index d7b049ef20..c124cd5829 100644
--- a/arch/arm/dts/am335x-evm-u-boot.dtsi
+++ b/arch/arm/dts/am335x-evm-u-boot.dtsi
@@ -3,13 +3,20 @@
  * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com/
  */
 
-/ {
-	panel {
-		u-boot,dm-pre-reloc;
+&l4_per {
+
+	segment at 300000 {
+
+		target-module at e000 {
+			u-boot,dm-pre-reloc;
+
+			lcdc: lcdc at 0 {
+				u-boot,dm-pre-reloc;
+			};
+		};
 	};
 };
 
-
 &mmc3 {
 	status = "disabled";
 };
diff --git a/arch/arm/dts/am335x-evmsk-u-boot.dtsi b/arch/arm/dts/am335x-evmsk-u-boot.dtsi
index 599fb377e6..115a018abe 100644
--- a/arch/arm/dts/am335x-evmsk-u-boot.dtsi
+++ b/arch/arm/dts/am335x-evmsk-u-boot.dtsi
@@ -5,8 +5,16 @@
  * Copyright (C) 2020 Dario Binacchi <dariobin@libero.it>
  */
 
-/ {
-	panel {
-		u-boot,dm-pre-reloc;
+&l4_per {
+
+	segment at 300000 {
+
+		target-module at e000 {
+			u-boot,dm-pre-reloc;
+
+			lcdc: lcdc at 0 {
+				u-boot,dm-pre-reloc;
+			};
+		};
 	};
 };
diff --git a/arch/arm/dts/am335x-guardian-u-boot.dtsi b/arch/arm/dts/am335x-guardian-u-boot.dtsi
index eae027c541..b1b4e70a6e 100644
--- a/arch/arm/dts/am335x-guardian-u-boot.dtsi
+++ b/arch/arm/dts/am335x-guardian-u-boot.dtsi
@@ -8,16 +8,26 @@
 	ocp {
 		u-boot,dm-pre-reloc;
 	};
-
-	panel {
-		u-boot,dm-pre-reloc;
-	};
 };
 
 &l4_wkup {
 	u-boot,dm-pre-reloc;
 };
 
+&l4_per {
+
+	segment at 300000 {
+
+		target-module at e000 {
+			u-boot,dm-pre-reloc;
+
+			lcdc: lcdc at 0 {
+				u-boot,dm-pre-reloc;
+			};
+		};
+	};
+};
+
 &mmc1 {
 	u-boot,dm-pre-reloc;
 };
diff --git a/arch/arm/dts/am335x-pdu001-u-boot.dtsi b/arch/arm/dts/am335x-pdu001-u-boot.dtsi
index a799fe9bc3..9be4c1e1b1 100644
--- a/arch/arm/dts/am335x-pdu001-u-boot.dtsi
+++ b/arch/arm/dts/am335x-pdu001-u-boot.dtsi
@@ -7,16 +7,26 @@
 	ocp {
 		u-boot,dm-pre-reloc;
 	};
-
-	panel {
-		u-boot,dm-pre-reloc;
-	};
 };
 
 &l4_wkup {
 	u-boot,dm-pre-reloc;
 };
 
+&l4_per {
+
+	segment at 300000 {
+
+		target-module at e000 {
+			u-boot,dm-pre-reloc;
+
+			lcdc: lcdc at 0 {
+				u-boot,dm-pre-reloc;
+			};
+		};
+	};
+};
+
 &scm {
 	u-boot,dm-pre-reloc;
 };
diff --git a/arch/arm/dts/am335x-pxm50-u-boot.dtsi b/arch/arm/dts/am335x-pxm50-u-boot.dtsi
index 77dfe6e262..5621046cb6 100644
--- a/arch/arm/dts/am335x-pxm50-u-boot.dtsi
+++ b/arch/arm/dts/am335x-pxm50-u-boot.dtsi
@@ -5,8 +5,16 @@
  * Copyright (C) 2020 Dario Binacchi <dariobin@libero.it>
  */
 
-/ {
-	panel {
-		u-boot,dm-pre-reloc;
+&l4_per {
+
+	segment at 300000 {
+
+		target-module at e000 {
+			u-boot,dm-pre-reloc;
+
+			lcdc: lcdc at 0 {
+				u-boot,dm-pre-reloc;
+			};
+		};
 	};
 };
diff --git a/arch/arm/dts/am335x-rut-u-boot.dtsi b/arch/arm/dts/am335x-rut-u-boot.dtsi
index b2b4aa596a..47ab06c9d3 100644
--- a/arch/arm/dts/am335x-rut-u-boot.dtsi
+++ b/arch/arm/dts/am335x-rut-u-boot.dtsi
@@ -5,8 +5,16 @@
  * Copyright (C) 2020 Dario Binacchi <dariobin@libero.it>
  */
 
-/ {
-	panel {
-		u-boot,dm-pre-reloc;
+&l4_per {
+
+	segment at 300000 {
+
+		target-module at e000 {
+			u-boot,dm-pre-reloc;
+
+			lcdc: lcdc at 0 {
+				u-boot,dm-pre-reloc;
+			};
+		};
 	};
 };
diff --git a/arch/arm/dts/da850-evm-u-boot.dtsi b/arch/arm/dts/da850-evm-u-boot.dtsi
index d588628641..020b9eb563 100644
--- a/arch/arm/dts/da850-evm-u-boot.dtsi
+++ b/arch/arm/dts/da850-evm-u-boot.dtsi
@@ -14,10 +14,6 @@
 	nand {
 		compatible = "ti,davinci-nand";
 	};
-
-	panel {
-		u-boot,dm-pre-reloc;
-	};
 };
 
 &eth0 {
@@ -28,6 +24,20 @@
 	compatible = "m25p64", "jedec,spi-nor";
 };
 
+&l4_per {
+
+	segment at 300000 {
+
+		target-module at e000 {
+			u-boot,dm-pre-reloc;
+
+			lcdc: lcdc at 0 {
+				u-boot,dm-pre-reloc;
+			};
+		};
+	};
+};
+
 &mmc0 {
 	u-boot,dm-spl;
 };
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 67a492a2d6..132a63ecea 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_DM_VIDEO) += video-uclass.o vidconsole-uclass.o
 obj-$(CONFIG_DM_VIDEO) += video_bmp.o
 obj-$(CONFIG_PANEL) += panel-uclass.o
 obj-$(CONFIG_SIMPLE_PANEL) += simple_panel.o
+obj-$(CONFIG_AM335X_LCD) += tilcdc-panel.o
 endif
 
 obj-${CONFIG_EXYNOS_FB} += exynos/
diff --git a/drivers/video/am335x-fb.c b/drivers/video/am335x-fb.c
index 2707ff59c7..dc959baa27 100644
--- a/drivers/video/am335x-fb.c
+++ b/drivers/video/am335x-fb.c
@@ -15,6 +15,7 @@
 #include <dm.h>
 #include <lcd.h>
 #include <log.h>
+#include <panel.h>
 #include <video.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/hardware.h>
@@ -25,6 +26,7 @@
 #include <linux/delay.h>
 #include <linux/err.h>
 #include "am335x-fb.h"
+#include "tilcdc-panel.h"
 
 #define LCDC_FMAX				200000000
 
@@ -323,7 +325,7 @@ int am335xfb_init(struct am335x_lcdpanel *panel)
 
 #else /* CONFIG_DM_VIDEO */
 
-#define FBSIZE(t, p)	(((t)->hactive.typ * (t)->vactive.typ * (p)->bpp) >> 3)
+#define FBSIZE(t, p)	(((t).hactive.typ * (t).vactive.typ * (p).bpp) >> 3)
 
 enum {
 	LCD_MAX_WIDTH		= 2048,
@@ -331,39 +333,8 @@ enum {
 	LCD_MAX_LOG2_BPP	= VIDEO_BPP32,
 };
 
-/**
- * tilcdc_panel_info: Panel parameters
- *
- * @ac_bias: AC Bias Pin Frequency
- * @ac_bias_intrpt: AC Bias Pin Transitions per Interrupt
- * @dma_burst_sz: DMA burst size
- * @bpp: Bits per pixel
- * @fdd: FIFO DMA Request Delay
- * @tft_alt_mode: TFT Alternative Signal Mapping (Only for active)
- * @invert_pxl_clk: Invert pixel clock
- * @sync_edge: Horizontal and Vertical Sync Edge: 0=rising 1=falling
- * @sync_ctrl: Horizontal and Vertical Sync: Control: 0=ignore
- * @raster_order: Raster Data Order Select: 1=Most-to-least 0=Least-to-most
- * @fifo_th: DMA FIFO threshold
- */
-struct tilcdc_panel_info {
-	u32 ac_bias;
-	u32 ac_bias_intrpt;
-	u32 dma_burst_sz;
-	u32 bpp;
-	u32 fdd;
-	bool tft_alt_mode;
-	bool invert_pxl_clk;
-	u32 sync_edge;
-	u32 sync_ctrl;
-	u32 raster_order;
-	u32 fifo_th;
-};
-
 struct am335x_fb_priv {
 	struct am335x_lcdhw *regs;
-	struct tilcdc_panel_info panel;
-	struct display_timing timing;
 };
 
 static int am335x_fb_remove(struct udevice *dev)
@@ -381,16 +352,71 @@ static int am335x_fb_probe(struct udevice *dev)
 	struct video_priv *uc_priv = dev_get_uclass_priv(dev);
 	struct am335x_fb_priv *priv = dev_get_priv(dev);
 	struct am335x_lcdhw *regs = priv->regs;
-	struct tilcdc_panel_info *panel = &priv->panel;
-	struct display_timing *timing = &priv->timing;
+	struct udevice *panel;
+	struct tilcdc_panel_info info;
+	struct display_timing timing;
 	struct cm_dpll *const cmdpll = (struct cm_dpll *)CM_DPLL;
 	u32 reg;
+	int err;
 
 	/* Before relocation we don't need to do anything */
 	if (!(gd->flags & GD_FLG_RELOC))
 		return 0;
 
-	am335x_fb_set_pixel_clk_rate(regs, timing->pixelclock.typ);
+	err = uclass_get_device(UCLASS_PANEL, 0, &panel);
+	if (err) {
+		dev_err(dev, "failed to get panel\n");
+		return err;
+	}
+
+	err = panel_get_display_timing(panel, &timing);
+	if (err) {
+		dev_err(dev, "failed to get display timing\n");
+		return err;
+	}
+
+	if (timing.pixelclock.typ > (LCDC_FMAX / 2)) {
+		dev_err(dev, "invalid display clock-frequency: %d Hz\n",
+			timing.pixelclock.typ);
+		return -EINVAL;
+	}
+
+	if (timing.hactive.typ > LCD_MAX_WIDTH)
+		timing.hactive.typ = LCD_MAX_WIDTH;
+
+	if (timing.vactive.typ > LCD_MAX_HEIGHT)
+		timing.vactive.typ = LCD_MAX_HEIGHT;
+
+	err = tilcdc_panel_get_display_info(panel, &info);
+	if (err) {
+		dev_err(dev, "failed to get panel info\n");
+		return err;
+	}
+
+	switch (info.bpp) {
+	case 16:
+	case 24:
+	case 32:
+		break;
+	default:
+		dev_err(dev, "invalid seting, bpp: %d\n", info.bpp);
+		return -EINVAL;
+	}
+
+	switch (info.dma_burst_sz) {
+	case 1:
+	case 2:
+	case 4:
+	case 8:
+	case 16:
+		break;
+	default:
+		dev_err(dev, "invalid setting, dma-burst-sz: %d\n",
+			info.dma_burst_sz);
+		return -EINVAL;
+	}
+
+	am335x_fb_set_pixel_clk_rate(regs, timing.pixelclock.typ);
 
 	/* clock source for LCDC from dispPLL M2 */
 	writel(0, &cmdpll->clklcdcpixelclk);
@@ -411,14 +437,14 @@ static int am335x_fb_probe(struct udevice *dev)
 	writel(reg, &regs->ctrl);
 
 	writel(uc_plat->base, &regs->lcddma_fb0_base);
-	writel(uc_plat->base + FBSIZE(timing, panel),
+	writel(uc_plat->base + FBSIZE(timing, info),
 	       &regs->lcddma_fb0_ceiling);
 	writel(uc_plat->base, &regs->lcddma_fb1_base);
-	writel(uc_plat->base + FBSIZE(timing, panel),
+	writel(uc_plat->base + FBSIZE(timing, info),
 	       &regs->lcddma_fb1_ceiling);
 
-	reg = LCDC_DMA_CTRL_FIFO_TH(panel->fifo_th);
-	switch (panel->dma_burst_sz) {
+	reg = LCDC_DMA_CTRL_FIFO_TH(info.fifo_th);
+	switch (info.dma_burst_sz) {
 	case 1:
 		reg |= LCDC_DMA_CTRL_BURST_SIZE(LCDC_DMA_CTRL_BURST_1);
 		break;
@@ -438,155 +464,84 @@ static int am335x_fb_probe(struct udevice *dev)
 
 	writel(reg, &regs->lcddma_ctrl);
 
-	writel(LCDC_RASTER_TIMING_0_HORLSB(timing->hactive.typ) |
-	       LCDC_RASTER_TIMING_0_HORMSB(timing->hactive.typ) |
-	       LCDC_RASTER_TIMING_0_HFPLSB(timing->hfront_porch.typ) |
-	       LCDC_RASTER_TIMING_0_HBPLSB(timing->hback_porch.typ) |
-	       LCDC_RASTER_TIMING_0_HSWLSB(timing->hsync_len.typ),
+	writel(LCDC_RASTER_TIMING_0_HORLSB(timing.hactive.typ) |
+	       LCDC_RASTER_TIMING_0_HORMSB(timing.hactive.typ) |
+	       LCDC_RASTER_TIMING_0_HFPLSB(timing.hfront_porch.typ) |
+	       LCDC_RASTER_TIMING_0_HBPLSB(timing.hback_porch.typ) |
+	       LCDC_RASTER_TIMING_0_HSWLSB(timing.hsync_len.typ),
 	       &regs->raster_timing0);
 
-	writel(LCDC_RASTER_TIMING_1_VBP(timing->vback_porch.typ) |
-	       LCDC_RASTER_TIMING_1_VFP(timing->vfront_porch.typ) |
-	       LCDC_RASTER_TIMING_1_VSW(timing->vsync_len.typ) |
-	       LCDC_RASTER_TIMING_1_VERLSB(timing->vactive.typ),
+	writel(LCDC_RASTER_TIMING_1_VBP(timing.vback_porch.typ) |
+	       LCDC_RASTER_TIMING_1_VFP(timing.vfront_porch.typ) |
+	       LCDC_RASTER_TIMING_1_VSW(timing.vsync_len.typ) |
+	       LCDC_RASTER_TIMING_1_VERLSB(timing.vactive.typ),
 	       &regs->raster_timing1);
 
-	reg = LCDC_RASTER_TIMING_2_ACB(panel->ac_bias) |
-		LCDC_RASTER_TIMING_2_ACBI(panel->ac_bias_intrpt) |
-		LCDC_RASTER_TIMING_2_HSWMSB(timing->hsync_len.typ) |
-		LCDC_RASTER_TIMING_2_VERMSB(timing->vactive.typ) |
-		LCDC_RASTER_TIMING_2_HBPMSB(timing->hback_porch.typ) |
-		LCDC_RASTER_TIMING_2_HFPMSB(timing->hfront_porch.typ);
+	reg = LCDC_RASTER_TIMING_2_ACB(info.ac_bias) |
+		LCDC_RASTER_TIMING_2_ACBI(info.ac_bias_intrpt) |
+		LCDC_RASTER_TIMING_2_HSWMSB(timing.hsync_len.typ) |
+		LCDC_RASTER_TIMING_2_VERMSB(timing.vactive.typ) |
+		LCDC_RASTER_TIMING_2_HBPMSB(timing.hback_porch.typ) |
+		LCDC_RASTER_TIMING_2_HFPMSB(timing.hfront_porch.typ);
 
-	if (timing->flags & DISPLAY_FLAGS_VSYNC_LOW)
+	if (timing.flags & DISPLAY_FLAGS_VSYNC_LOW)
 		reg |= LCDC_RASTER_TIMING_2_VSYNC_INVERT;
 
-	if (timing->flags & DISPLAY_FLAGS_HSYNC_LOW)
+	if (timing.flags & DISPLAY_FLAGS_HSYNC_LOW)
 		reg |= LCDC_RASTER_TIMING_2_HSYNC_INVERT;
 
-	if (panel->invert_pxl_clk)
+	if (info.invert_pxl_clk)
 		reg |= LCDC_RASTER_TIMING_2_PXCLK_INVERT;
 
-	if (panel->sync_edge)
+	if (info.sync_edge)
 		reg |= LCDC_RASTER_TIMING_2_HSVS_RISEFALL;
 
-	if (panel->sync_ctrl)
+	if (info.sync_ctrl)
 		reg |= LCDC_RASTER_TIMING_2_HSVS_CONTROL;
 
 	writel(reg, &regs->raster_timing2);
 
 	reg = LCDC_RASTER_CTRL_PALMODE_RAWDATA | LCDC_RASTER_CTRL_TFT_MODE |
-		LCDC_RASTER_CTRL_ENABLE | LCDC_RASTER_CTRL_REQDLY(panel->fdd);
+		LCDC_RASTER_CTRL_ENABLE | LCDC_RASTER_CTRL_REQDLY(info.fdd);
 
-	if (panel->tft_alt_mode)
+	if (info.tft_alt_mode)
 		reg |= LCDC_RASTER_CTRL_TFT_ALT_ENABLE;
 
-	if (panel->bpp == 24)
+	if (info.bpp == 24)
 		reg |= LCDC_RASTER_CTRL_TFT_24BPP_MODE;
-	else if (panel->bpp == 32)
+	else if (info.bpp == 32)
 		reg |= LCDC_RASTER_CTRL_TFT_24BPP_MODE |
 			LCDC_RASTER_CTRL_TFT_24BPP_UNPACK;
 
-	if (panel->raster_order)
+	if (info.raster_order)
 		reg |= LCDC_RASTER_CTRL_DATA_ORDER;
 
 	writel(reg, &regs->raster_ctrl);
 
-	uc_priv->xsize = timing->hactive.typ;
-	uc_priv->ysize = timing->vactive.typ;
-	uc_priv->bpix = log_2_n_round_up(panel->bpp);
-	return 0;
-}
-
-static int am335x_fb_ofdata_to_platdata(struct udevice *dev)
-{
-	struct am335x_fb_priv *priv = dev_get_priv(dev);
-	struct tilcdc_panel_info *panel = &priv->panel;
-	struct display_timing *timing = &priv->timing;
-	ofnode node;
-	int err;
+	uc_priv->xsize = timing.hactive.typ;
+	uc_priv->ysize = timing.vactive.typ;
+	uc_priv->bpix = log_2_n_round_up(info.bpp);
 
-	node = ofnode_by_compatible(ofnode_null(), "ti,am33xx-tilcdc");
-	if (!ofnode_valid(node)) {
-		dev_err(dev, "missing 'ti,am33xx-tilcdc' node\n");
-		return -ENXIO;
-	}
-
-	priv->regs = (struct am335x_lcdhw *)ofnode_get_addr(node);
-	dev_dbg(dev, "LCD: base address=0x%x\n", (unsigned int)priv->regs);
-
-	err = ofnode_decode_display_timing(dev_ofnode(dev), 0, timing);
+	err = panel_enable_backlight(panel);
 	if (err) {
-		dev_err(dev, "failed to get display timing\n");
+		dev_err(dev, "failed to enable panel backlight\n");
 		return err;
 	}
 
-	if (timing->pixelclock.typ > (LCDC_FMAX / 2)) {
-		dev_err(dev, "invalid display clock-frequency: %d Hz\n",
-			timing->pixelclock.typ);
-		return -EINVAL;
-	}
-
-	if (timing->hactive.typ > LCD_MAX_WIDTH)
-		timing->hactive.typ = LCD_MAX_WIDTH;
-
-	if (timing->vactive.typ > LCD_MAX_HEIGHT)
-		timing->vactive.typ = LCD_MAX_HEIGHT;
-
-	node = ofnode_find_subnode(dev_ofnode(dev), "panel-info");
-	if (!ofnode_valid(node)) {
-		dev_err(dev, "missing 'panel-info' node\n");
-		return -ENXIO;
-	}
-
-	err |= ofnode_read_u32(node, "ac-bias", &panel->ac_bias);
-	err |= ofnode_read_u32(node, "ac-bias-intrpt", &panel->ac_bias_intrpt);
-	err |= ofnode_read_u32(node, "dma-burst-sz", &panel->dma_burst_sz);
-	err |= ofnode_read_u32(node, "bpp", &panel->bpp);
-	err |= ofnode_read_u32(node, "fdd", &panel->fdd);
-	err |= ofnode_read_u32(node, "sync-edge", &panel->sync_edge);
-	err |= ofnode_read_u32(node, "sync-ctrl", &panel->sync_ctrl);
-	err |= ofnode_read_u32(node, "raster-order", &panel->raster_order);
-	err |= ofnode_read_u32(node, "fifo-th", &panel->fifo_th);
-	if (err) {
-		dev_err(dev, "failed to get panel info\n");
-		return err;
-	}
+	return 0;
+}
 
-	switch (panel->bpp) {
-	case 16:
-	case 24:
-	case 32:
-		break;
-	default:
-		dev_err(dev, "invalid seting, bpp: %d\n", panel->bpp);
-		return -EINVAL;
-	}
+static int am335x_fb_ofdata_to_platdata(struct udevice *dev)
+{
+	struct am335x_fb_priv *priv = dev_get_priv(dev);
 
-	switch (panel->dma_burst_sz) {
-	case 1:
-	case 2:
-	case 4:
-	case 8:
-	case 16:
-		break;
-	default:
-		dev_err(dev, "invalid setting, dma-burst-sz: %d\n",
-			panel->dma_burst_sz);
+	priv->regs = (struct am335x_lcdhw *)dev_read_addr(dev);
+	if ((fdt_addr_t)priv->regs == FDT_ADDR_T_NONE) {
+		dev_err(dev, "failed to get base address\n");
 		return -EINVAL;
 	}
 
-	/* optional */
-	panel->tft_alt_mode = ofnode_read_bool(node, "tft-alt-mode");
-	panel->invert_pxl_clk = ofnode_read_bool(node, "invert-pxl-clk");
-
-	dev_dbg(dev, "LCD: %dx%d, bpp=%d, clk=%d Hz\n", timing->hactive.typ,
-		timing->vactive.typ, panel->bpp, timing->pixelclock.typ);
-	dev_dbg(dev, "     hbp=%d, hfp=%d, hsw=%d\n", timing->hback_porch.typ,
-		timing->hfront_porch.typ, timing->hsync_len.typ);
-	dev_dbg(dev, "     vbp=%d, vfp=%d, vsw=%d\n", timing->vback_porch.typ,
-		timing->vfront_porch.typ, timing->vsync_len.typ);
-
+	dev_dbg(dev, "LCD: base address=0x%x\n", (unsigned int)priv->regs);
 	return 0;
 }
 
@@ -602,7 +557,7 @@ static int am335x_fb_bind(struct udevice *dev)
 }
 
 static const struct udevice_id am335x_fb_ids[] = {
-	{ .compatible = "ti,tilcdc,panel" },
+	{ .compatible = "ti,am33xx-tilcdc" },
 	{ }
 };
 
diff --git a/drivers/video/am335x-fb.h b/drivers/video/am335x-fb.h
index c9f92bc389..4952dd96e9 100644
--- a/drivers/video/am335x-fb.h
+++ b/drivers/video/am335x-fb.h
@@ -70,6 +70,37 @@ struct am335x_lcdpanel {
 
 int am335xfb_init(struct am335x_lcdpanel *panel);
 
+#else /* CONFIG_DM_VIDEO */
+
+/**
+ * tilcdc_panel_info: Panel parameters
+ *
+ * @ac_bias: AC Bias Pin Frequency
+ * @ac_bias_intrpt: AC Bias Pin Transitions per Interrupt
+ * @dma_burst_sz: DMA burst size
+ * @bpp: Bits per pixel
+ * @fdd: FIFO DMA Request Delay
+ * @tft_alt_mode: TFT Alternative Signal Mapping (Only for active)
+ * @invert_pxl_clk: Invert pixel clock
+ * @sync_edge: Horizontal and Vertical Sync Edge: 0=rising 1=falling
+ * @sync_ctrl: Horizontal and Vertical Sync: Control: 0=ignore
+ * @raster_order: Raster Data Order Select: 1=Most-to-least 0=Least-to-most
+ * @fifo_th: DMA FIFO threshold
+ */
+struct tilcdc_panel_info {
+	u32 ac_bias;
+	u32 ac_bias_intrpt;
+	u32 dma_burst_sz;
+	u32 bpp;
+	u32 fdd;
+	bool tft_alt_mode;
+	bool invert_pxl_clk;
+	u32 sync_edge;
+	u32 sync_ctrl;
+	u32 raster_order;
+	u32 fifo_th;
+};
+
 #endif  /* CONFIG_DM_VIDEO */
 
 #endif  /* AM335X_FB_H */
diff --git a/drivers/video/tilcdc-panel.c b/drivers/video/tilcdc-panel.c
new file mode 100644
index 0000000000..caf86c8383
--- /dev/null
+++ b/drivers/video/tilcdc-panel.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * OMAP panel support
+ *
+ * Copyright (C) 2020 Dario Binacchi <dariobin@libero.it>
+ */
+
+#include <common.h>
+#include <backlight.h>
+#include <clk.h>
+#include <display.h>
+#include <dm.h>
+#include <dm/device_compat.h>
+#include <log.h>
+#include <panel.h>
+#include <asm/gpio.h>
+#include <linux/err.h>
+#include "am335x-fb.h"
+
+struct tilcdc_panel_priv {
+	struct tilcdc_panel_info info;
+	struct display_timing timing;
+	struct udevice *backlight;
+	struct gpio_desc enable;
+};
+
+static int tilcdc_panel_enable_backlight(struct udevice *dev)
+{
+	struct tilcdc_panel_priv *priv = dev_get_priv(dev);
+
+	if (dm_gpio_is_valid(&priv->enable))
+		dm_gpio_set_value(&priv->enable, 1);
+
+	if (priv->backlight)
+		return backlight_enable(priv->backlight);
+
+	return 0;
+}
+
+static int tilcdc_panel_set_backlight(struct udevice *dev, int percent)
+{
+	struct tilcdc_panel_priv *priv = dev_get_priv(dev);
+
+	if (dm_gpio_is_valid(&priv->enable))
+		dm_gpio_set_value(&priv->enable, 1);
+
+	if (priv->backlight)
+		return backlight_set_brightness(priv->backlight, percent);
+
+	return 0;
+}
+
+int tilcdc_panel_get_display_info(struct udevice *dev,
+				  struct tilcdc_panel_info *info)
+{
+	struct tilcdc_panel_priv *priv = dev_get_priv(dev);
+
+	memcpy(info, &priv->info, sizeof(*info));
+	return 0;
+}
+
+static int tilcdc_panel_get_display_timing(struct udevice *dev,
+					   struct display_timing *timing)
+{
+	struct tilcdc_panel_priv *priv = dev_get_priv(dev);
+
+	memcpy(timing, &priv->timing, sizeof(*timing));
+	return 0;
+}
+
+static int tilcdc_panel_remove(struct udevice *dev)
+{
+	struct tilcdc_panel_priv *priv = dev_get_priv(dev);
+
+	if (dm_gpio_is_valid(&priv->enable))
+		dm_gpio_free(dev, &priv->enable);
+
+	return 0;
+}
+
+static int tilcdc_panel_probe(struct udevice *dev)
+{
+	struct tilcdc_panel_priv *priv = dev_get_priv(dev);
+	int err;
+
+	err = uclass_get_device_by_phandle(UCLASS_PANEL_BACKLIGHT, dev,
+					   "backlight", &priv->backlight);
+	if (err)
+		dev_warn(dev, "failed to get backlight\n");
+
+	err = gpio_request_by_name(dev, "enable-gpios", 0, &priv->enable,
+				   GPIOD_IS_OUT);
+	if (err) {
+		dev_warn(dev, "failed to get enable GPIO\n");
+		if (err != -ENOENT)
+			return err;
+	}
+
+	return 0;
+}
+
+static int tilcdc_panel_ofdata_to_platdata(struct udevice *dev)
+{
+	struct tilcdc_panel_priv *priv = dev_get_priv(dev);
+	ofnode node;
+	int err;
+
+	err = ofnode_decode_display_timing(dev_ofnode(dev), 0, &priv->timing);
+	if (err) {
+		dev_err(dev, "failed to get display timing\n");
+		return err;
+	}
+
+	node = dev_read_subnode(dev, "panel-info");
+	if (!ofnode_valid(node)) {
+		dev_err(dev, "missing 'panel-info' node\n");
+		return -ENXIO;
+	}
+
+	err |= ofnode_read_u32(node, "ac-bias", &priv->info.ac_bias);
+	err |= ofnode_read_u32(node, "ac-bias-intrpt",
+			       &priv->info.ac_bias_intrpt);
+	err |= ofnode_read_u32(node, "dma-burst-sz", &priv->info.dma_burst_sz);
+	err |= ofnode_read_u32(node, "bpp", &priv->info.bpp);
+	err |= ofnode_read_u32(node, "fdd", &priv->info.fdd);
+	err |= ofnode_read_u32(node, "sync-edge", &priv->info.sync_edge);
+	err |= ofnode_read_u32(node, "sync-ctrl", &priv->info.sync_ctrl);
+	err |= ofnode_read_u32(node, "raster-order", &priv->info.raster_order);
+	err |= ofnode_read_u32(node, "fifo-th", &priv->info.fifo_th);
+	if (err) {
+		dev_err(dev, "failed to get panel info\n");
+		return err;
+	}
+
+	/* optional */
+	priv->info.tft_alt_mode = ofnode_read_bool(node, "tft-alt-mode");
+	priv->info.invert_pxl_clk = ofnode_read_bool(node, "invert-pxl-clk");
+
+	dev_dbg(dev, "LCD: %dx%d, bpp=%d, clk=%d Hz\n",
+		priv->timing.hactive.typ, priv->timing.vactive.typ,
+		priv->info.bpp, priv->timing.pixelclock.typ);
+	dev_dbg(dev, "     hbp=%d, hfp=%d, hsw=%d\n",
+		priv->timing.hback_porch.typ, priv->timing.hfront_porch.typ,
+		priv->timing.hsync_len.typ);
+	dev_dbg(dev, "     vbp=%d, vfp=%d, vsw=%d\n",
+		priv->timing.vback_porch.typ, priv->timing.vfront_porch.typ,
+		priv->timing.vsync_len.typ);
+
+	return 0;
+}
+
+static const struct panel_ops tilcdc_panel_ops = {
+	.enable_backlight = tilcdc_panel_enable_backlight,
+	.set_backlight = tilcdc_panel_set_backlight,
+	.get_display_timing = tilcdc_panel_get_display_timing,
+};
+
+static const struct udevice_id tilcdc_panel_ids[] = {
+	{.compatible = "ti,tilcdc,panel"},
+	{}
+};
+
+U_BOOT_DRIVER(tilcdc_panel) = {
+	.name = "tilcdc_panel",
+	.id = UCLASS_PANEL,
+	.of_match = tilcdc_panel_ids,
+	.ops = &tilcdc_panel_ops,
+	.ofdata_to_platdata = tilcdc_panel_ofdata_to_platdata,
+	.probe = tilcdc_panel_probe,
+	.remove = tilcdc_panel_remove,
+	.priv_auto_alloc_size = sizeof(struct tilcdc_panel_priv),
+};
diff --git a/drivers/video/tilcdc-panel.h b/drivers/video/tilcdc-panel.h
new file mode 100644
index 0000000000..6b40731304
--- /dev/null
+++ b/drivers/video/tilcdc-panel.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2020 Dario Binacchi <dariobin@libero.it>
+ */
+
+#ifndef _TILCDC_PANEL_H
+#define _TILCDC_PANEL_H
+
+#include "am335x-fb.h"
+
+int tilcdc_panel_get_display_info(struct udevice *dev,
+				  struct tilcdc_panel_info *info);
+
+#endif /* _TILCDC_PANEL_H */
-- 
2.17.1

[PATCH v5 23/27] video: omap: drop domain clock enabling by SOC api

[Dario Binacchi]

Enabling the domain clock is performed by the sysc interconnect target
module driver during the video device probing.

Signed-off-by: Dario Binacchi <dariobin@libero.it>

---

(no changes since v3)

Changes in v3:
- Remove clock domain enabling/disabling.
- Update the commit message.

 arch/arm/mach-omap2/am33xx/clock_am33xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/am33xx/clock_am33xx.c b/arch/arm/mach-omap2/am33xx/clock_am33xx.c
index 2427933c8b..cf71192360 100644
--- a/arch/arm/mach-omap2/am33xx/clock_am33xx.c
+++ b/arch/arm/mach-omap2/am33xx/clock_am33xx.c
@@ -226,7 +226,7 @@ void enable_basic_clocks(void)
 		&cmper->usb0clkctrl,
 		&cmper->emiffwclkctrl,
 		&cmper->emifclkctrl,
-#if CONFIG_IS_ENABLED(AM335X_LCD)
+#if CONFIG_IS_ENABLED(AM335X_LCD) && !CONFIG_IS_ENABLED(DM_VIDEO)
 		&cmper->lcdclkctrl,
 		&cmper->lcdcclkstctrl,
 #endif
-- 
2.17.1

[PATCH v5 24/27] video: omap: set LCD clock rate through DM API

[Dario Binacchi]

The patch configures the display DPLL using the functions provided by
the driver model API for the clock. The device tree contains everything
needed to get the DPLL clock. The round rate function developed for
calculating the DPLL multiplier and divisor and the platform routines
for accessing the DPLL registers are removed from the LCD driver code
because they are implemented inside the DPLL clock driver.

Signed-off-by: Dario Binacchi <dariobin@libero.it>

---

(no changes since v3)

Changes in v3:
- Add clk.h header.
- Fix an error code returned by the probe function.

 drivers/video/am335x-fb.c | 129 ++++++++++++++++++++++++++++++--------
 1 file changed, 103 insertions(+), 26 deletions(-)

diff --git a/drivers/video/am335x-fb.c b/drivers/video/am335x-fb.c
index dc959baa27..a0a635cc29 100644
--- a/drivers/video/am335x-fb.c
+++ b/drivers/video/am335x-fb.c
@@ -12,6 +12,7 @@
  * - starts output DMA from gd->fb_base buffer
  */
 #include <common.h>
+#include <clk.h>
 #include <dm.h>
 #include <lcd.h>
 #include <log.h>
@@ -112,6 +113,27 @@ struct am335x_lcdhw {
 	unsigned int		clkc_reset;		/* 0x70 */
 };
 
+DECLARE_GLOBAL_DATA_PTR;
+
+#if !CONFIG_IS_ENABLED(DM_VIDEO)
+
+#if !defined(LCD_CNTL_BASE)
+#error "hw-base address of LCD-Controller (LCD_CNTL_BASE) not defined!"
+#endif
+
+/* Macro definitions */
+#define FBSIZE(x)	(((x)->hactive * (x)->vactive * (x)->bpp) >> 3)
+
+#define LCDC_RASTER_TIMING_2_INVMASK(x)		((x) & GENMASK(25, 20))
+
+static struct am335x_lcdhw *lcdhw = (void *)LCD_CNTL_BASE;
+
+int lcd_get_size(int *line_length)
+{
+	*line_length = (panel_info.vl_col * NBITS(panel_info.vl_bpix)) / 8;
+	return *line_length * panel_info.vl_row + 0x20;
+}
+
 struct dpll_data {
 	unsigned long rounded_rate;
 	u16 rounded_m;
@@ -119,8 +141,6 @@ struct dpll_data {
 	u8 rounded_div;
 };
 
-DECLARE_GLOBAL_DATA_PTR;
-
 /**
  * am335x_dpll_round_rate() - Round a target rate for an OMAP DPLL
  *
@@ -199,25 +219,6 @@ static ulong am335x_fb_set_pixel_clk_rate(struct am335x_lcdhw *regs, ulong rate)
 	return round_rate;
 }
 
-#if !CONFIG_IS_ENABLED(DM_VIDEO)
-
-#if !defined(LCD_CNTL_BASE)
-#error "hw-base address of LCD-Controller (LCD_CNTL_BASE) not defined!"
-#endif
-
-/* Macro definitions */
-#define FBSIZE(x)	(((x)->hactive * (x)->vactive * (x)->bpp) >> 3)
-
-#define LCDC_RASTER_TIMING_2_INVMASK(x)		((x) & GENMASK(25, 20))
-
-static struct am335x_lcdhw *lcdhw = (void *)LCD_CNTL_BASE;
-
-int lcd_get_size(int *line_length)
-{
-	*line_length = (panel_info.vl_col * NBITS(panel_info.vl_bpix)) / 8;
-	return *line_length * panel_info.vl_row + 0x20;
-}
-
 int am335xfb_init(struct am335x_lcdpanel *panel)
 {
 	u32 raster_ctrl = 0;
@@ -335,14 +336,58 @@ enum {
 
 struct am335x_fb_priv {
 	struct am335x_lcdhw *regs;
+	struct clk gclk;
+	struct clk dpll_m2_clk;
 };
 
+static ulong tilcdc_set_pixel_clk_rate(struct udevice *dev, ulong rate)
+{
+	struct am335x_fb_priv *priv = dev_get_priv(dev);
+	struct am335x_lcdhw *regs = priv->regs;
+	ulong mult_rate, mult_round_rate, best_err, err;
+	u32 v;
+	int div, i;
+
+	best_err = rate;
+	div = 0;
+	for (i = 2; i <= 255; i++) {
+		mult_rate = rate * i;
+		mult_round_rate = clk_round_rate(&priv->gclk, mult_rate);
+		if (IS_ERR_VALUE(mult_round_rate))
+			return mult_round_rate;
+
+		err = mult_rate - mult_round_rate;
+		if (err < best_err) {
+			best_err = err;
+			div = i;
+			if (err == 0)
+				break;
+		}
+	}
+
+	if (div == 0) {
+		dev_err(dev, "failed to find a divisor\n");
+		return -EFAULT;
+	}
+
+	mult_rate = clk_set_rate(&priv->gclk, rate * div);
+	v = readl(&regs->ctrl) & ~LCDC_CTRL_CLK_DIVISOR_MASK;
+	v |= LCDC_CTRL_CLK_DIVISOR(div);
+	writel(v, &regs->ctrl);
+	rate = mult_rate / div;
+	dev_dbg(dev, "rate=%ld, div=%d, err=%ld\n", rate, div, err);
+	return rate;
+}
+
 static int am335x_fb_remove(struct udevice *dev)
 {
 	struct video_uc_platdata *uc_plat = dev_get_uclass_platdata(dev);
+	struct am335x_fb_priv *priv = dev_get_priv(dev);
 
 	uc_plat->base -= 0x20;
 	uc_plat->size += 0x20;
+	clk_release_all(&priv->gclk, 1);
+	clk_release_all(&priv->dpll_m2_clk, 1);
 	return 0;
 }
 
@@ -352,10 +397,10 @@ static int am335x_fb_probe(struct udevice *dev)
 	struct video_priv *uc_priv = dev_get_uclass_priv(dev);
 	struct am335x_fb_priv *priv = dev_get_priv(dev);
 	struct am335x_lcdhw *regs = priv->regs;
-	struct udevice *panel;
+	struct udevice *panel, *clk_dev;
 	struct tilcdc_panel_info info;
 	struct display_timing timing;
-	struct cm_dpll *const cmdpll = (struct cm_dpll *)CM_DPLL;
+	ulong rate;
 	u32 reg;
 	int err;
 
@@ -416,10 +461,42 @@ static int am335x_fb_probe(struct udevice *dev)
 		return -EINVAL;
 	}
 
-	am335x_fb_set_pixel_clk_rate(regs, timing.pixelclock.typ);
+	err = uclass_get_device_by_name(UCLASS_CLK, "lcd_gclk at 534", &clk_dev);
+	if (err) {
+		dev_err(dev, "failed to get lcd_gclk device\n");
+		return err;
+	}
 
-	/* clock source for LCDC from dispPLL M2 */
-	writel(0, &cmdpll->clklcdcpixelclk);
+	err = clk_request(clk_dev, &priv->gclk);
+	if (err) {
+		dev_err(dev, "failed to get %s clock\n", clk_dev->name);
+		return err;
+	}
+
+	rate = tilcdc_set_pixel_clk_rate(dev, timing.pixelclock.typ);
+	if (IS_ERR_VALUE(rate)) {
+		dev_err(dev, "failed to set pixel clock rate\n");
+		return rate;
+	}
+
+	err = uclass_get_device_by_name(UCLASS_CLK, "dpll_disp_m2_ck at 4a4", &clk_dev);
+	if (err) {
+		dev_err(dev, "failed to get dpll_disp_m2 clock device\n");
+		return err;
+	}
+
+	err = clk_request(clk_dev, &priv->dpll_m2_clk);
+	if (err) {
+		dev_err(dev, "failed to get %s clock\n", clk_dev->name);
+		return err;
+	}
+
+	err = clk_set_parent(&priv->gclk, &priv->dpll_m2_clk);
+	if (err) {
+		dev_err(dev, "failed to set %s clock as %s's parent\n",
+			priv->dpll_m2_clk.dev->name, priv->gclk.dev->name);
+		return err;
+	}
 
 	/* palette default entry */
 	memset((void *)uc_plat->base, 0, 0x20);
-- 
2.17.1

[PATCH v5 25/27] video: omap: split the legacy code from the DM code

[Dario Binacchi]

The schedule for deprecating the features of the pre-driver-model puts
2019.17 as the deadline for the video subsystem. Furthermore, the latest
patches applied to the am335x-fb.c module have decreased the amount of
code shared with the pre-driver-model implementation. Splitting the two
implementations into two modules improves the readability of the code
and will make it easier to drop the pre-driver-model code.
I have not created a header file with the data structures and the
constants for accessing the LCD controller registers, but I preferred to
keep them inside the two c modules. This is a code replication until the
pre-driver-model version is dropped.

Signed-off-by: Dario Binacchi <dariobin@libero.it>

---

(no changes since v4)

Changes in v4:
- Include device_compat.h header for dev_xxx macros.

 drivers/video/Makefile       |   5 +-
 drivers/video/am335x-fb.c    | 336 ---------------------------
 drivers/video/am335x-fb.h    |  35 ---
 drivers/video/tilcdc-panel.c |   2 +-
 drivers/video/tilcdc-panel.h |   2 +-
 drivers/video/tilcdc.c       | 425 +++++++++++++++++++++++++++++++++++
 drivers/video/tilcdc.h       |  38 ++++
 7 files changed, 468 insertions(+), 375 deletions(-)
 create mode 100644 drivers/video/tilcdc.c
 create mode 100644 drivers/video/tilcdc.h

diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 132a63ecea..29f3434f7c 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -16,7 +16,9 @@ obj-$(CONFIG_DM_VIDEO) += video-uclass.o vidconsole-uclass.o
 obj-$(CONFIG_DM_VIDEO) += video_bmp.o
 obj-$(CONFIG_PANEL) += panel-uclass.o
 obj-$(CONFIG_SIMPLE_PANEL) += simple_panel.o
-obj-$(CONFIG_AM335X_LCD) += tilcdc-panel.o
+obj-$(CONFIG_AM335X_LCD) += tilcdc.o tilcdc-panel.o
+else
+obj-$(CONFIG_AM335X_LCD) += am335x-fb.o
 endif
 
 obj-${CONFIG_EXYNOS_FB} += exynos/
@@ -24,7 +26,6 @@ obj-${CONFIG_VIDEO_ROCKCHIP} += rockchip/
 obj-${CONFIG_VIDEO_STM32} += stm32/
 obj-${CONFIG_VIDEO_TEGRA124} += tegra124/
 
-obj-$(CONFIG_AM335X_LCD) += am335x-fb.o
 obj-$(CONFIG_ATI_RADEON_FB) += ati_radeon_fb.o videomodes.o
 obj-$(CONFIG_ATMEL_HLCD) += atmel_hlcdfb.o
 obj-$(CONFIG_ATMEL_LCD) += atmel_lcdfb.o
diff --git a/drivers/video/am335x-fb.c b/drivers/video/am335x-fb.c
index a0a635cc29..eb9d692035 100644
--- a/drivers/video/am335x-fb.c
+++ b/drivers/video/am335x-fb.c
@@ -12,22 +12,15 @@
  * - starts output DMA from gd->fb_base buffer
  */
 #include <common.h>
-#include <clk.h>
-#include <dm.h>
 #include <lcd.h>
 #include <log.h>
-#include <panel.h>
-#include <video.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/hardware.h>
 #include <asm/arch/omap.h>
 #include <asm/arch/sys_proto.h>
 #include <asm/io.h>
-#include <asm/utils.h>
 #include <linux/delay.h>
-#include <linux/err.h>
 #include "am335x-fb.h"
-#include "tilcdc-panel.h"
 
 #define LCDC_FMAX				200000000
 
@@ -115,8 +108,6 @@ struct am335x_lcdhw {
 
 DECLARE_GLOBAL_DATA_PTR;
 
-#if !CONFIG_IS_ENABLED(DM_VIDEO)
-
 #if !defined(LCD_CNTL_BASE)
 #error "hw-base address of LCD-Controller (LCD_CNTL_BASE) not defined!"
 #endif
@@ -323,330 +314,3 @@ int am335xfb_init(struct am335x_lcdpanel *panel)
 
 	return 0;
 }
-
-#else /* CONFIG_DM_VIDEO */
-
-#define FBSIZE(t, p)	(((t).hactive.typ * (t).vactive.typ * (p).bpp) >> 3)
-
-enum {
-	LCD_MAX_WIDTH		= 2048,
-	LCD_MAX_HEIGHT		= 2048,
-	LCD_MAX_LOG2_BPP	= VIDEO_BPP32,
-};
-
-struct am335x_fb_priv {
-	struct am335x_lcdhw *regs;
-	struct clk gclk;
-	struct clk dpll_m2_clk;
-};
-
-static ulong tilcdc_set_pixel_clk_rate(struct udevice *dev, ulong rate)
-{
-	struct am335x_fb_priv *priv = dev_get_priv(dev);
-	struct am335x_lcdhw *regs = priv->regs;
-	ulong mult_rate, mult_round_rate, best_err, err;
-	u32 v;
-	int div, i;
-
-	best_err = rate;
-	div = 0;
-	for (i = 2; i <= 255; i++) {
-		mult_rate = rate * i;
-		mult_round_rate = clk_round_rate(&priv->gclk, mult_rate);
-		if (IS_ERR_VALUE(mult_round_rate))
-			return mult_round_rate;
-
-		err = mult_rate - mult_round_rate;
-		if (err < best_err) {
-			best_err = err;
-			div = i;
-			if (err == 0)
-				break;
-		}
-	}
-
-	if (div == 0) {
-		dev_err(dev, "failed to find a divisor\n");
-		return -EFAULT;
-	}
-
-	mult_rate = clk_set_rate(&priv->gclk, rate * div);
-	v = readl(&regs->ctrl) & ~LCDC_CTRL_CLK_DIVISOR_MASK;
-	v |= LCDC_CTRL_CLK_DIVISOR(div);
-	writel(v, &regs->ctrl);
-	rate = mult_rate / div;
-	dev_dbg(dev, "rate=%ld, div=%d, err=%ld\n", rate, div, err);
-	return rate;
-}
-
-static int am335x_fb_remove(struct udevice *dev)
-{
-	struct video_uc_platdata *uc_plat = dev_get_uclass_platdata(dev);
-	struct am335x_fb_priv *priv = dev_get_priv(dev);
-
-	uc_plat->base -= 0x20;
-	uc_plat->size += 0x20;
-	clk_release_all(&priv->gclk, 1);
-	clk_release_all(&priv->dpll_m2_clk, 1);
-	return 0;
-}
-
-static int am335x_fb_probe(struct udevice *dev)
-{
-	struct video_uc_platdata *uc_plat = dev_get_uclass_platdata(dev);
-	struct video_priv *uc_priv = dev_get_uclass_priv(dev);
-	struct am335x_fb_priv *priv = dev_get_priv(dev);
-	struct am335x_lcdhw *regs = priv->regs;
-	struct udevice *panel, *clk_dev;
-	struct tilcdc_panel_info info;
-	struct display_timing timing;
-	ulong rate;
-	u32 reg;
-	int err;
-
-	/* Before relocation we don't need to do anything */
-	if (!(gd->flags & GD_FLG_RELOC))
-		return 0;
-
-	err = uclass_get_device(UCLASS_PANEL, 0, &panel);
-	if (err) {
-		dev_err(dev, "failed to get panel\n");
-		return err;
-	}
-
-	err = panel_get_display_timing(panel, &timing);
-	if (err) {
-		dev_err(dev, "failed to get display timing\n");
-		return err;
-	}
-
-	if (timing.pixelclock.typ > (LCDC_FMAX / 2)) {
-		dev_err(dev, "invalid display clock-frequency: %d Hz\n",
-			timing.pixelclock.typ);
-		return -EINVAL;
-	}
-
-	if (timing.hactive.typ > LCD_MAX_WIDTH)
-		timing.hactive.typ = LCD_MAX_WIDTH;
-
-	if (timing.vactive.typ > LCD_MAX_HEIGHT)
-		timing.vactive.typ = LCD_MAX_HEIGHT;
-
-	err = tilcdc_panel_get_display_info(panel, &info);
-	if (err) {
-		dev_err(dev, "failed to get panel info\n");
-		return err;
-	}
-
-	switch (info.bpp) {
-	case 16:
-	case 24:
-	case 32:
-		break;
-	default:
-		dev_err(dev, "invalid seting, bpp: %d\n", info.bpp);
-		return -EINVAL;
-	}
-
-	switch (info.dma_burst_sz) {
-	case 1:
-	case 2:
-	case 4:
-	case 8:
-	case 16:
-		break;
-	default:
-		dev_err(dev, "invalid setting, dma-burst-sz: %d\n",
-			info.dma_burst_sz);
-		return -EINVAL;
-	}
-
-	err = uclass_get_device_by_name(UCLASS_CLK, "lcd_gclk at 534", &clk_dev);
-	if (err) {
-		dev_err(dev, "failed to get lcd_gclk device\n");
-		return err;
-	}
-
-	err = clk_request(clk_dev, &priv->gclk);
-	if (err) {
-		dev_err(dev, "failed to get %s clock\n", clk_dev->name);
-		return err;
-	}
-
-	rate = tilcdc_set_pixel_clk_rate(dev, timing.pixelclock.typ);
-	if (IS_ERR_VALUE(rate)) {
-		dev_err(dev, "failed to set pixel clock rate\n");
-		return rate;
-	}
-
-	err = uclass_get_device_by_name(UCLASS_CLK, "dpll_disp_m2_ck at 4a4", &clk_dev);
-	if (err) {
-		dev_err(dev, "failed to get dpll_disp_m2 clock device\n");
-		return err;
-	}
-
-	err = clk_request(clk_dev, &priv->dpll_m2_clk);
-	if (err) {
-		dev_err(dev, "failed to get %s clock\n", clk_dev->name);
-		return err;
-	}
-
-	err = clk_set_parent(&priv->gclk, &priv->dpll_m2_clk);
-	if (err) {
-		dev_err(dev, "failed to set %s clock as %s's parent\n",
-			priv->dpll_m2_clk.dev->name, priv->gclk.dev->name);
-		return err;
-	}
-
-	/* palette default entry */
-	memset((void *)uc_plat->base, 0, 0x20);
-	*(unsigned int *)uc_plat->base = 0x4000;
-	/* point fb behind palette */
-	uc_plat->base += 0x20;
-	uc_plat->size -= 0x20;
-
-	writel(LCDC_CLKC_ENABLE_CORECLKEN | LCDC_CLKC_ENABLE_LIDDCLKEN |
-	       LCDC_CLKC_ENABLE_DMACLKEN, &regs->clkc_enable);
-	writel(0, &regs->raster_ctrl);
-
-	reg = readl(&regs->ctrl) & LCDC_CTRL_CLK_DIVISOR_MASK;
-	reg |= LCDC_CTRL_RASTER_MODE;
-	writel(reg, &regs->ctrl);
-
-	writel(uc_plat->base, &regs->lcddma_fb0_base);
-	writel(uc_plat->base + FBSIZE(timing, info),
-	       &regs->lcddma_fb0_ceiling);
-	writel(uc_plat->base, &regs->lcddma_fb1_base);
-	writel(uc_plat->base + FBSIZE(timing, info),
-	       &regs->lcddma_fb1_ceiling);
-
-	reg = LCDC_DMA_CTRL_FIFO_TH(info.fifo_th);
-	switch (info.dma_burst_sz) {
-	case 1:
-		reg |= LCDC_DMA_CTRL_BURST_SIZE(LCDC_DMA_CTRL_BURST_1);
-		break;
-	case 2:
-		reg |= LCDC_DMA_CTRL_BURST_SIZE(LCDC_DMA_CTRL_BURST_2);
-		break;
-	case 4:
-		reg |= LCDC_DMA_CTRL_BURST_SIZE(LCDC_DMA_CTRL_BURST_4);
-		break;
-	case 8:
-		reg |= LCDC_DMA_CTRL_BURST_SIZE(LCDC_DMA_CTRL_BURST_8);
-		break;
-	case 16:
-		reg |= LCDC_DMA_CTRL_BURST_SIZE(LCDC_DMA_CTRL_BURST_16);
-		break;
-	}
-
-	writel(reg, &regs->lcddma_ctrl);
-
-	writel(LCDC_RASTER_TIMING_0_HORLSB(timing.hactive.typ) |
-	       LCDC_RASTER_TIMING_0_HORMSB(timing.hactive.typ) |
-	       LCDC_RASTER_TIMING_0_HFPLSB(timing.hfront_porch.typ) |
-	       LCDC_RASTER_TIMING_0_HBPLSB(timing.hback_porch.typ) |
-	       LCDC_RASTER_TIMING_0_HSWLSB(timing.hsync_len.typ),
-	       &regs->raster_timing0);
-
-	writel(LCDC_RASTER_TIMING_1_VBP(timing.vback_porch.typ) |
-	       LCDC_RASTER_TIMING_1_VFP(timing.vfront_porch.typ) |
-	       LCDC_RASTER_TIMING_1_VSW(timing.vsync_len.typ) |
-	       LCDC_RASTER_TIMING_1_VERLSB(timing.vactive.typ),
-	       &regs->raster_timing1);
-
-	reg = LCDC_RASTER_TIMING_2_ACB(info.ac_bias) |
-		LCDC_RASTER_TIMING_2_ACBI(info.ac_bias_intrpt) |
-		LCDC_RASTER_TIMING_2_HSWMSB(timing.hsync_len.typ) |
-		LCDC_RASTER_TIMING_2_VERMSB(timing.vactive.typ) |
-		LCDC_RASTER_TIMING_2_HBPMSB(timing.hback_porch.typ) |
-		LCDC_RASTER_TIMING_2_HFPMSB(timing.hfront_porch.typ);
-
-	if (timing.flags & DISPLAY_FLAGS_VSYNC_LOW)
-		reg |= LCDC_RASTER_TIMING_2_VSYNC_INVERT;
-
-	if (timing.flags & DISPLAY_FLAGS_HSYNC_LOW)
-		reg |= LCDC_RASTER_TIMING_2_HSYNC_INVERT;
-
-	if (info.invert_pxl_clk)
-		reg |= LCDC_RASTER_TIMING_2_PXCLK_INVERT;
-
-	if (info.sync_edge)
-		reg |= LCDC_RASTER_TIMING_2_HSVS_RISEFALL;
-
-	if (info.sync_ctrl)
-		reg |= LCDC_RASTER_TIMING_2_HSVS_CONTROL;
-
-	writel(reg, &regs->raster_timing2);
-
-	reg = LCDC_RASTER_CTRL_PALMODE_RAWDATA | LCDC_RASTER_CTRL_TFT_MODE |
-		LCDC_RASTER_CTRL_ENABLE | LCDC_RASTER_CTRL_REQDLY(info.fdd);
-
-	if (info.tft_alt_mode)
-		reg |= LCDC_RASTER_CTRL_TFT_ALT_ENABLE;
-
-	if (info.bpp == 24)
-		reg |= LCDC_RASTER_CTRL_TFT_24BPP_MODE;
-	else if (info.bpp == 32)
-		reg |= LCDC_RASTER_CTRL_TFT_24BPP_MODE |
-			LCDC_RASTER_CTRL_TFT_24BPP_UNPACK;
-
-	if (info.raster_order)
-		reg |= LCDC_RASTER_CTRL_DATA_ORDER;
-
-	writel(reg, &regs->raster_ctrl);
-
-	uc_priv->xsize = timing.hactive.typ;
-	uc_priv->ysize = timing.vactive.typ;
-	uc_priv->bpix = log_2_n_round_up(info.bpp);
-
-	err = panel_enable_backlight(panel);
-	if (err) {
-		dev_err(dev, "failed to enable panel backlight\n");
-		return err;
-	}
-
-	return 0;
-}
-
-static int am335x_fb_ofdata_to_platdata(struct udevice *dev)
-{
-	struct am335x_fb_priv *priv = dev_get_priv(dev);
-
-	priv->regs = (struct am335x_lcdhw *)dev_read_addr(dev);
-	if ((fdt_addr_t)priv->regs == FDT_ADDR_T_NONE) {
-		dev_err(dev, "failed to get base address\n");
-		return -EINVAL;
-	}
-
-	dev_dbg(dev, "LCD: base address=0x%x\n", (unsigned int)priv->regs);
-	return 0;
-}
-
-static int am335x_fb_bind(struct udevice *dev)
-{
-	struct video_uc_platdata *uc_plat = dev_get_uclass_platdata(dev);
-
-	uc_plat->size = ((LCD_MAX_WIDTH * LCD_MAX_HEIGHT *
-			  (1 << LCD_MAX_LOG2_BPP)) >> 3) + 0x20;
-
-	dev_dbg(dev, "frame buffer size 0x%x\n", uc_plat->size);
-	return 0;
-}
-
-static const struct udevice_id am335x_fb_ids[] = {
-	{ .compatible = "ti,am33xx-tilcdc" },
-	{ }
-};
-
-U_BOOT_DRIVER(am335x_fb) = {
-	.name = "am335x_fb",
-	.id = UCLASS_VIDEO,
-	.of_match = am335x_fb_ids,
-	.bind = am335x_fb_bind,
-	.ofdata_to_platdata = am335x_fb_ofdata_to_platdata,
-	.probe = am335x_fb_probe,
-	.remove = am335x_fb_remove,
-	.priv_auto_alloc_size = sizeof(struct am335x_fb_priv),
-};
-
-#endif /* CONFIG_DM_VIDEO */
diff --git a/drivers/video/am335x-fb.h b/drivers/video/am335x-fb.h
index 4952dd96e9..ad9b015e09 100644
--- a/drivers/video/am335x-fb.h
+++ b/drivers/video/am335x-fb.h
@@ -7,8 +7,6 @@
 #ifndef AM335X_FB_H
 #define AM335X_FB_H
 
-#if !CONFIG_IS_ENABLED(DM_VIDEO)
-
 #define HSVS_CONTROL		BIT(25)	/*
 					 * 0 = lcd_lp and lcd_fp are driven on
 					 * opposite edges of pixel clock than
@@ -70,37 +68,4 @@ struct am335x_lcdpanel {
 
 int am335xfb_init(struct am335x_lcdpanel *panel);
 
-#else /* CONFIG_DM_VIDEO */
-
-/**
- * tilcdc_panel_info: Panel parameters
- *
- * @ac_bias: AC Bias Pin Frequency
- * @ac_bias_intrpt: AC Bias Pin Transitions per Interrupt
- * @dma_burst_sz: DMA burst size
- * @bpp: Bits per pixel
- * @fdd: FIFO DMA Request Delay
- * @tft_alt_mode: TFT Alternative Signal Mapping (Only for active)
- * @invert_pxl_clk: Invert pixel clock
- * @sync_edge: Horizontal and Vertical Sync Edge: 0=rising 1=falling
- * @sync_ctrl: Horizontal and Vertical Sync: Control: 0=ignore
- * @raster_order: Raster Data Order Select: 1=Most-to-least 0=Least-to-most
- * @fifo_th: DMA FIFO threshold
- */
-struct tilcdc_panel_info {
-	u32 ac_bias;
-	u32 ac_bias_intrpt;
-	u32 dma_burst_sz;
-	u32 bpp;
-	u32 fdd;
-	bool tft_alt_mode;
-	bool invert_pxl_clk;
-	u32 sync_edge;
-	u32 sync_ctrl;
-	u32 raster_order;
-	u32 fifo_th;
-};
-
-#endif  /* CONFIG_DM_VIDEO */
-
 #endif  /* AM335X_FB_H */
diff --git a/drivers/video/tilcdc-panel.c b/drivers/video/tilcdc-panel.c
index caf86c8383..e9c8e84e3b 100644
--- a/drivers/video/tilcdc-panel.c
+++ b/drivers/video/tilcdc-panel.c
@@ -15,7 +15,7 @@
 #include <panel.h>
 #include <asm/gpio.h>
 #include <linux/err.h>
-#include "am335x-fb.h"
+#include "tilcdc.h"
 
 struct tilcdc_panel_priv {
 	struct tilcdc_panel_info info;
diff --git a/drivers/video/tilcdc-panel.h b/drivers/video/tilcdc-panel.h
index 6b40731304..6bcfbf8a8b 100644
--- a/drivers/video/tilcdc-panel.h
+++ b/drivers/video/tilcdc-panel.h
@@ -6,7 +6,7 @@
 #ifndef _TILCDC_PANEL_H
 #define _TILCDC_PANEL_H
 
-#include "am335x-fb.h"
+#include "tilcdc.h"
 
 int tilcdc_panel_get_display_info(struct udevice *dev,
 				  struct tilcdc_panel_info *info);
diff --git a/drivers/video/tilcdc.c b/drivers/video/tilcdc.c
new file mode 100644
index 0000000000..6228c2399c
--- /dev/null
+++ b/drivers/video/tilcdc.c
@@ -0,0 +1,425 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2020 Dario Binacchi <dariobin@libero.it>
+ */
+
+#include <common.h>
+#include <clk.h>
+#include <dm.h>
+#include <dm/device_compat.h>
+#include <lcd.h>
+#include <log.h>
+#include <panel.h>
+#include <video.h>
+#include <asm/io.h>
+#include <asm/utils.h>
+#include "tilcdc.h"
+#include "tilcdc-panel.h"
+
+#define LCDC_FMAX				200000000
+
+/* LCD Control Register */
+#define LCDC_CTRL_CLK_DIVISOR_MASK		GENMASK(15, 8)
+#define LCDC_CTRL_RASTER_MODE			BIT(0)
+#define LCDC_CTRL_CLK_DIVISOR(x)		(((x) & GENMASK(7, 0)) << 8)
+/* LCD Clock Enable Register */
+#define LCDC_CLKC_ENABLE_CORECLKEN		BIT(0)
+#define LCDC_CLKC_ENABLE_LIDDCLKEN		BIT(1)
+#define LCDC_CLKC_ENABLE_DMACLKEN		BIT(2)
+/* LCD DMA Control Register */
+#define LCDC_DMA_CTRL_BURST_SIZE(x)		(((x) & GENMASK(2, 0)) << 4)
+#define LCDC_DMA_CTRL_BURST_1			0x0
+#define LCDC_DMA_CTRL_BURST_2			0x1
+#define LCDC_DMA_CTRL_BURST_4			0x2
+#define LCDC_DMA_CTRL_BURST_8			0x3
+#define LCDC_DMA_CTRL_BURST_16			0x4
+#define LCDC_DMA_CTRL_FIFO_TH(x)		(((x) & GENMASK(2, 0)) << 8)
+/* LCD Timing_0 Register */
+#define LCDC_RASTER_TIMING_0_HORMSB(x)	((((x) - 1) & BIT(10)) >> 7)
+#define LCDC_RASTER_TIMING_0_HORLSB(x) (((((x) >> 4) - 1) & GENMASK(5, 0)) << 4)
+#define LCDC_RASTER_TIMING_0_HSWLSB(x)	((((x) - 1) & GENMASK(5, 0)) << 10)
+#define LCDC_RASTER_TIMING_0_HFPLSB(x)	((((x) - 1) & GENMASK(7, 0)) << 16)
+#define LCDC_RASTER_TIMING_0_HBPLSB(x)	((((x) - 1) & GENMASK(7, 0)) << 24)
+/* LCD Timing_1 Register */
+#define LCDC_RASTER_TIMING_1_VERLSB(x)		(((x) - 1) & GENMASK(9, 0))
+#define LCDC_RASTER_TIMING_1_VSW(x)	((((x) - 1) & GENMASK(5, 0)) << 10)
+#define LCDC_RASTER_TIMING_1_VFP(x)		(((x) & GENMASK(7, 0)) << 16)
+#define LCDC_RASTER_TIMING_1_VBP(x)		(((x) & GENMASK(7, 0)) << 24)
+/* LCD Timing_2 Register */
+#define LCDC_RASTER_TIMING_2_HFPMSB(x)	((((x) - 1) & GENMASK(9, 8)) >> 8)
+#define LCDC_RASTER_TIMING_2_HBPMSB(x)	((((x) - 1) & GENMASK(9, 8)) >> 4)
+#define LCDC_RASTER_TIMING_2_ACB(x)		(((x) & GENMASK(7, 0)) << 8)
+#define LCDC_RASTER_TIMING_2_ACBI(x)		(((x) & GENMASK(3, 0)) << 16)
+#define LCDC_RASTER_TIMING_2_VSYNC_INVERT	BIT(20)
+#define LCDC_RASTER_TIMING_2_HSYNC_INVERT	BIT(21)
+#define LCDC_RASTER_TIMING_2_PXCLK_INVERT	BIT(22)
+#define LCDC_RASTER_TIMING_2_DE_INVERT		BIT(23)
+#define LCDC_RASTER_TIMING_2_HSVS_RISEFALL	BIT(24)
+#define LCDC_RASTER_TIMING_2_HSVS_CONTROL	BIT(25)
+#define LCDC_RASTER_TIMING_2_VERMSB(x)		((((x) - 1) & BIT(10)) << 16)
+#define LCDC_RASTER_TIMING_2_HSWMSB(x)	((((x) - 1) & GENMASK(9, 6)) << 21)
+/* LCD Raster Ctrl Register */
+#define LCDC_RASTER_CTRL_ENABLE			BIT(0)
+#define LCDC_RASTER_CTRL_TFT_MODE		BIT(7)
+#define LCDC_RASTER_CTRL_DATA_ORDER		BIT(8)
+#define LCDC_RASTER_CTRL_REQDLY(x)		(((x) & GENMASK(7, 0)) << 12)
+#define LCDC_RASTER_CTRL_PALMODE_RAWDATA	(0x02 << 20)
+#define LCDC_RASTER_CTRL_TFT_ALT_ENABLE		BIT(23)
+#define LCDC_RASTER_CTRL_TFT_24BPP_MODE		BIT(25)
+#define LCDC_RASTER_CTRL_TFT_24BPP_UNPACK	BIT(26)
+
+enum {
+	LCDC_MAX_WIDTH = 2048,
+	LCDC_MAX_HEIGHT = 2048,
+	LCDC_MAX_LOG2_BPP = VIDEO_BPP32,
+};
+
+struct tilcdc_regs {
+	u32 pid;
+	u32 ctrl;
+	u32 gap0;
+	u32 lidd_ctrl;
+	u32 lidd_cs0_conf;
+	u32 lidd_cs0_addr;
+	u32 lidd_cs0_data;
+	u32 lidd_cs1_conf;
+	u32 lidd_cs1_addr;
+	u32 lidd_cs1_data;
+	u32 raster_ctrl;
+	u32 raster_timing0;
+	u32 raster_timing1;
+	u32 raster_timing2;
+	u32 raster_subpanel;
+	u32 raster_subpanel2;
+	u32 lcddma_ctrl;
+	u32 lcddma_fb0_base;
+	u32 lcddma_fb0_ceiling;
+	u32 lcddma_fb1_base;
+	u32 lcddma_fb1_ceiling;
+	u32 sysconfig;
+	u32 irqstatus_raw;
+	u32 irqstatus;
+	u32 irqenable_set;
+	u32 irqenable_clear;
+	u32 gap1;
+	u32 clkc_enable;
+	u32 clkc_reset;
+};
+
+struct tilcdc_priv {
+	struct tilcdc_regs *regs;
+	struct clk gclk;
+	struct clk dpll_m2_clk;
+};
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static ulong tilcdc_set_pixel_clk_rate(struct udevice *dev, ulong rate)
+{
+	struct tilcdc_priv *priv = dev_get_priv(dev);
+	struct tilcdc_regs *regs = priv->regs;
+	ulong mult_rate, mult_round_rate, best_err, err;
+	u32 v;
+	int div, i;
+
+	best_err = rate;
+	div = 0;
+	for (i = 2; i <= 255; i++) {
+		mult_rate = rate * i;
+		mult_round_rate = clk_round_rate(&priv->gclk, mult_rate);
+		if (IS_ERR_VALUE(mult_round_rate))
+			return mult_round_rate;
+
+		err = mult_rate - mult_round_rate;
+		if (err < best_err) {
+			best_err = err;
+			div = i;
+			if (err == 0)
+				break;
+		}
+	}
+
+	if (div == 0) {
+		dev_err(dev, "failed to find a divisor\n");
+		return -EFAULT;
+	}
+
+	mult_rate = clk_set_rate(&priv->gclk, rate * div);
+	v = readl(&regs->ctrl) & ~LCDC_CTRL_CLK_DIVISOR_MASK;
+	v |= LCDC_CTRL_CLK_DIVISOR(div);
+	writel(v, &regs->ctrl);
+	rate = mult_rate / div;
+	dev_dbg(dev, "rate=%ld, div=%d, err=%ld\n", rate, div, err);
+	return rate;
+}
+
+static int tilcdc_remove(struct udevice *dev)
+{
+	struct video_uc_platdata *uc_plat = dev_get_uclass_platdata(dev);
+	struct tilcdc_priv *priv = dev_get_priv(dev);
+
+	uc_plat->base -= 0x20;
+	uc_plat->size += 0x20;
+	clk_release_all(&priv->gclk, 1);
+	clk_release_all(&priv->dpll_m2_clk, 1);
+	return 0;
+}
+
+static int tilcdc_probe(struct udevice *dev)
+{
+	struct video_uc_platdata *uc_plat = dev_get_uclass_platdata(dev);
+	struct video_priv *uc_priv = dev_get_uclass_priv(dev);
+	struct tilcdc_priv *priv = dev_get_priv(dev);
+	struct tilcdc_regs *regs = priv->regs;
+	struct udevice *panel, *clk_dev;
+	struct tilcdc_panel_info info;
+	struct display_timing timing;
+	ulong rate;
+	u32 reg;
+	int err;
+
+	/* Before relocation we don't need to do anything */
+	if (!(gd->flags & GD_FLG_RELOC))
+		return 0;
+
+	err = uclass_get_device(UCLASS_PANEL, 0, &panel);
+	if (err) {
+		dev_err(dev, "failed to get panel\n");
+		return err;
+	}
+
+	err = panel_get_display_timing(panel, &timing);
+	if (err) {
+		dev_err(dev, "failed to get display timing\n");
+		return err;
+	}
+
+	if (timing.pixelclock.typ > (LCDC_FMAX / 2)) {
+		dev_err(dev, "invalid display clock-frequency: %d Hz\n",
+			timing.pixelclock.typ);
+		return -EINVAL;
+	}
+
+	if (timing.hactive.typ > LCDC_MAX_WIDTH)
+		timing.hactive.typ = LCDC_MAX_WIDTH;
+
+	if (timing.vactive.typ > LCDC_MAX_HEIGHT)
+		timing.vactive.typ = LCDC_MAX_HEIGHT;
+
+	err = tilcdc_panel_get_display_info(panel, &info);
+	if (err) {
+		dev_err(dev, "failed to get panel info\n");
+		return err;
+	}
+
+	switch (info.bpp) {
+	case 16:
+	case 24:
+	case 32:
+		break;
+	default:
+		dev_err(dev, "invalid seting, bpp: %d\n", info.bpp);
+		return -EINVAL;
+	}
+
+	switch (info.dma_burst_sz) {
+	case 1:
+	case 2:
+	case 4:
+	case 8:
+	case 16:
+		break;
+	default:
+		dev_err(dev, "invalid setting, dma-burst-sz: %d\n",
+			info.dma_burst_sz);
+		return -EINVAL;
+	}
+
+	err = uclass_get_device_by_name(UCLASS_CLK, "lcd_gclk at 534", &clk_dev);
+	if (err) {
+		dev_err(dev, "failed to get lcd_gclk device\n");
+		return err;
+	}
+
+	err = clk_request(clk_dev, &priv->gclk);
+	if (err) {
+		dev_err(dev, "failed to get %s clock\n", clk_dev->name);
+		return err;
+	}
+
+	rate = tilcdc_set_pixel_clk_rate(dev, timing.pixelclock.typ);
+	if (IS_ERR_VALUE(rate)) {
+		dev_err(dev, "failed to set pixel clock rate\n");
+		return rate;
+	}
+
+	err = uclass_get_device_by_name(UCLASS_CLK, "dpll_disp_m2_ck at 4a4",
+					&clk_dev);
+	if (err) {
+		dev_err(dev, "failed to get dpll_disp_m2 clock device\n");
+		return err;
+	}
+
+	err = clk_request(clk_dev, &priv->dpll_m2_clk);
+	if (err) {
+		dev_err(dev, "failed to get %s clock\n", clk_dev->name);
+		return err;
+	}
+
+	err = clk_set_parent(&priv->gclk, &priv->dpll_m2_clk);
+	if (err) {
+		dev_err(dev, "failed to set %s clock as %s's parent\n",
+			priv->dpll_m2_clk.dev->name, priv->gclk.dev->name);
+		return err;
+	}
+
+	/* palette default entry */
+	memset((void *)uc_plat->base, 0, 0x20);
+	*(unsigned int *)uc_plat->base = 0x4000;
+	/* point fb behind palette */
+	uc_plat->base += 0x20;
+	uc_plat->size -= 0x20;
+
+	writel(LCDC_CLKC_ENABLE_CORECLKEN | LCDC_CLKC_ENABLE_LIDDCLKEN |
+	       LCDC_CLKC_ENABLE_DMACLKEN, &regs->clkc_enable);
+	writel(0, &regs->raster_ctrl);
+
+	reg = readl(&regs->ctrl) & LCDC_CTRL_CLK_DIVISOR_MASK;
+	reg |= LCDC_CTRL_RASTER_MODE;
+	writel(reg, &regs->ctrl);
+
+	reg = (timing.hactive.typ * timing.vactive.typ * info.bpp) >> 3;
+	reg += uc_plat->base;
+	writel(uc_plat->base, &regs->lcddma_fb0_base);
+	writel(reg, &regs->lcddma_fb0_ceiling);
+	writel(uc_plat->base, &regs->lcddma_fb1_base);
+	writel(reg, &regs->lcddma_fb1_ceiling);
+
+	reg = LCDC_DMA_CTRL_FIFO_TH(info.fifo_th);
+	switch (info.dma_burst_sz) {
+	case 1:
+		reg |= LCDC_DMA_CTRL_BURST_SIZE(LCDC_DMA_CTRL_BURST_1);
+		break;
+	case 2:
+		reg |= LCDC_DMA_CTRL_BURST_SIZE(LCDC_DMA_CTRL_BURST_2);
+		break;
+	case 4:
+		reg |= LCDC_DMA_CTRL_BURST_SIZE(LCDC_DMA_CTRL_BURST_4);
+		break;
+	case 8:
+		reg |= LCDC_DMA_CTRL_BURST_SIZE(LCDC_DMA_CTRL_BURST_8);
+		break;
+	case 16:
+		reg |= LCDC_DMA_CTRL_BURST_SIZE(LCDC_DMA_CTRL_BURST_16);
+		break;
+	}
+
+	writel(reg, &regs->lcddma_ctrl);
+
+	writel(LCDC_RASTER_TIMING_0_HORLSB(timing.hactive.typ) |
+	       LCDC_RASTER_TIMING_0_HORMSB(timing.hactive.typ) |
+	       LCDC_RASTER_TIMING_0_HFPLSB(timing.hfront_porch.typ) |
+	       LCDC_RASTER_TIMING_0_HBPLSB(timing.hback_porch.typ) |
+	       LCDC_RASTER_TIMING_0_HSWLSB(timing.hsync_len.typ),
+	       &regs->raster_timing0);
+
+	writel(LCDC_RASTER_TIMING_1_VBP(timing.vback_porch.typ) |
+	       LCDC_RASTER_TIMING_1_VFP(timing.vfront_porch.typ) |
+	       LCDC_RASTER_TIMING_1_VSW(timing.vsync_len.typ) |
+	       LCDC_RASTER_TIMING_1_VERLSB(timing.vactive.typ),
+	       &regs->raster_timing1);
+
+	reg = LCDC_RASTER_TIMING_2_ACB(info.ac_bias) |
+		LCDC_RASTER_TIMING_2_ACBI(info.ac_bias_intrpt) |
+		LCDC_RASTER_TIMING_2_HSWMSB(timing.hsync_len.typ) |
+		LCDC_RASTER_TIMING_2_VERMSB(timing.vactive.typ) |
+		LCDC_RASTER_TIMING_2_HBPMSB(timing.hback_porch.typ) |
+		LCDC_RASTER_TIMING_2_HFPMSB(timing.hfront_porch.typ);
+
+	if (timing.flags & DISPLAY_FLAGS_VSYNC_LOW)
+		reg |= LCDC_RASTER_TIMING_2_VSYNC_INVERT;
+
+	if (timing.flags & DISPLAY_FLAGS_HSYNC_LOW)
+		reg |= LCDC_RASTER_TIMING_2_HSYNC_INVERT;
+
+	if (info.invert_pxl_clk)
+		reg |= LCDC_RASTER_TIMING_2_PXCLK_INVERT;
+
+	if (info.sync_edge)
+		reg |= LCDC_RASTER_TIMING_2_HSVS_RISEFALL;
+
+	if (info.sync_ctrl)
+		reg |= LCDC_RASTER_TIMING_2_HSVS_CONTROL;
+
+	writel(reg, &regs->raster_timing2);
+
+	reg = LCDC_RASTER_CTRL_PALMODE_RAWDATA | LCDC_RASTER_CTRL_TFT_MODE |
+		LCDC_RASTER_CTRL_ENABLE | LCDC_RASTER_CTRL_REQDLY(info.fdd);
+
+	if (info.tft_alt_mode)
+		reg |= LCDC_RASTER_CTRL_TFT_ALT_ENABLE;
+
+	if (info.bpp == 24)
+		reg |= LCDC_RASTER_CTRL_TFT_24BPP_MODE;
+	else if (info.bpp == 32)
+		reg |= LCDC_RASTER_CTRL_TFT_24BPP_MODE |
+			LCDC_RASTER_CTRL_TFT_24BPP_UNPACK;
+
+	if (info.raster_order)
+		reg |= LCDC_RASTER_CTRL_DATA_ORDER;
+
+	writel(reg, &regs->raster_ctrl);
+
+	uc_priv->xsize = timing.hactive.typ;
+	uc_priv->ysize = timing.vactive.typ;
+	uc_priv->bpix = log_2_n_round_up(info.bpp);
+
+	err = panel_enable_backlight(panel);
+	if (err) {
+		dev_err(dev, "failed to enable panel backlight\n");
+		return err;
+	}
+
+	return 0;
+}
+
+static int tilcdc_ofdata_to_platdata(struct udevice *dev)
+{
+	struct tilcdc_priv *priv = dev_get_priv(dev);
+
+	priv->regs = (struct tilcdc_regs *)dev_read_addr(dev);
+	if ((fdt_addr_t)priv->regs == FDT_ADDR_T_NONE) {
+		dev_err(dev, "failed to get base address\n");
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "LCD: base address=0x%x\n", (unsigned int)priv->regs);
+	return 0;
+}
+
+static int tilcdc_bind(struct udevice *dev)
+{
+	struct video_uc_platdata *uc_plat = dev_get_uclass_platdata(dev);
+
+	uc_plat->size = ((LCDC_MAX_WIDTH * LCDC_MAX_HEIGHT *
+			  (1 << LCDC_MAX_LOG2_BPP)) >> 3) + 0x20;
+
+	dev_dbg(dev, "frame buffer size 0x%x\n", uc_plat->size);
+	return 0;
+}
+
+static const struct udevice_id tilcdc_ids[] = {
+	{.compatible = "ti,am33xx-tilcdc"},
+	{}
+};
+
+U_BOOT_DRIVER(tilcdc) = {
+	.name = "tilcdc",
+	.id = UCLASS_VIDEO,
+	.of_match = tilcdc_ids,
+	.bind = tilcdc_bind,
+	.ofdata_to_platdata = tilcdc_ofdata_to_platdata,
+	.probe = tilcdc_probe,
+	.remove = tilcdc_remove,
+	.priv_auto_alloc_size = sizeof(struct tilcdc_priv)
+};
diff --git a/drivers/video/tilcdc.h b/drivers/video/tilcdc.h
new file mode 100644
index 0000000000..2645921df6
--- /dev/null
+++ b/drivers/video/tilcdc.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2020 Dario Binacchi <dariobin@libero.it>
+ */
+
+#ifndef _TILCDC_H
+#define _TILCDC_H
+
+/**
+ * tilcdc_panel_info: Panel parameters
+ *
+ * @ac_bias: AC Bias Pin Frequency
+ * @ac_bias_intrpt: AC Bias Pin Transitions per Interrupt
+ * @dma_burst_sz: DMA burst size
+ * @bpp: Bits per pixel
+ * @fdd: FIFO DMA Request Delay
+ * @tft_alt_mode: TFT Alternative Signal Mapping (Only for active)
+ * @invert_pxl_clk: Invert pixel clock
+ * @sync_edge: Horizontal and Vertical Sync Edge: 0=rising 1=falling
+ * @sync_ctrl: Horizontal and Vertical Sync: Control: 0=ignore
+ * @raster_order: Raster Data Order Select: 1=Most-to-least 0=Least-to-most
+ * @fifo_th: DMA FIFO threshold
+ */
+struct tilcdc_panel_info {
+	u32 ac_bias;
+	u32 ac_bias_intrpt;
+	u32 dma_burst_sz;
+	u32 bpp;
+	u32 fdd;
+	bool tft_alt_mode;
+	bool invert_pxl_clk;
+	u32 sync_edge;
+	u32 sync_ctrl;
+	u32 raster_order;
+	u32 fifo_th;
+};
+
+#endif /* _TILCDC_H */
-- 
2.17.1

[PATCH v5 26/27] video: omap: move drivers to 'ti' directory

[Dario Binacchi]

Add drivers/video/ti/ folder and move all TI's code in this folder for
better maintenance.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

(no changes since v1)

 drivers/video/Kconfig                 |  5 +----
 drivers/video/Makefile                |  4 +---
 drivers/video/ti/Kconfig              |  8 ++++++++
 drivers/video/ti/Makefile             | 10 ++++++++++
 drivers/video/{ => ti}/am335x-fb.c    |  0
 drivers/video/{ => ti}/am335x-fb.h    |  0
 drivers/video/{ => ti}/tilcdc-panel.c |  0
 drivers/video/{ => ti}/tilcdc-panel.h |  0
 drivers/video/{ => ti}/tilcdc.c       |  0
 drivers/video/{ => ti}/tilcdc.h       |  0
 10 files changed, 20 insertions(+), 7 deletions(-)
 create mode 100644 drivers/video/ti/Kconfig
 create mode 100644 drivers/video/ti/Makefile
 rename drivers/video/{ => ti}/am335x-fb.c (100%)
 rename drivers/video/{ => ti}/am335x-fb.h (100%)
 rename drivers/video/{ => ti}/tilcdc-panel.c (100%)
 rename drivers/video/{ => ti}/tilcdc-panel.h (100%)
 rename drivers/video/{ => ti}/tilcdc.c (100%)
 rename drivers/video/{ => ti}/tilcdc.h (100%)

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index b1cb745d59..0eec4c2863 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -546,10 +546,7 @@ config ATMEL_HLCD
 	help
 	   HLCDC supports video output to an attached LCD panel.
 
-config AM335X_LCD
-	bool "Enable AM335x video support"
-	help
-	   Supports video output to an attached LCD panel.
+source "drivers/video/ti/Kconfig"
 
 config LOGICORE_DP_TX
 	bool "Enable Logicore DP TX driver"
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 29f3434f7c..38e181a779 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -16,15 +16,13 @@ obj-$(CONFIG_DM_VIDEO) += video-uclass.o vidconsole-uclass.o
 obj-$(CONFIG_DM_VIDEO) += video_bmp.o
 obj-$(CONFIG_PANEL) += panel-uclass.o
 obj-$(CONFIG_SIMPLE_PANEL) += simple_panel.o
-obj-$(CONFIG_AM335X_LCD) += tilcdc.o tilcdc-panel.o
-else
-obj-$(CONFIG_AM335X_LCD) += am335x-fb.o
 endif
 
 obj-${CONFIG_EXYNOS_FB} += exynos/
 obj-${CONFIG_VIDEO_ROCKCHIP} += rockchip/
 obj-${CONFIG_VIDEO_STM32} += stm32/
 obj-${CONFIG_VIDEO_TEGRA124} += tegra124/
+obj-${CONFIG_AM335X_LCD} += ti/
 
 obj-$(CONFIG_ATI_RADEON_FB) += ati_radeon_fb.o videomodes.o
 obj-$(CONFIG_ATMEL_HLCD) += atmel_hlcdfb.o
diff --git a/drivers/video/ti/Kconfig b/drivers/video/ti/Kconfig
new file mode 100644
index 0000000000..3081e9e8c0
--- /dev/null
+++ b/drivers/video/ti/Kconfig
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright (C) 2020 Dario Binacchi <dariobin@libero.it>
+#
+config AM335X_LCD
+	bool "Enable AM335x video support"
+	help
+	   Supports video output to an attached LCD panel.
diff --git a/drivers/video/ti/Makefile b/drivers/video/ti/Makefile
new file mode 100644
index 0000000000..f0410debf4
--- /dev/null
+++ b/drivers/video/ti/Makefile
@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright (C) 2020 Dario Binacchi <dariobin@libero.it>
+#
+
+ifdef CONFIG_DM
+obj-$(CONFIG_AM335X_LCD) += tilcdc.o tilcdc-panel.o
+else
+obj-$(CONFIG_AM335X_LCD) += am335x-fb.o
+endif
diff --git a/drivers/video/am335x-fb.c b/drivers/video/ti/am335x-fb.c
similarity index 100%
rename from drivers/video/am335x-fb.c
rename to drivers/video/ti/am335x-fb.c
diff --git a/drivers/video/am335x-fb.h b/drivers/video/ti/am335x-fb.h
similarity index 100%
rename from drivers/video/am335x-fb.h
rename to drivers/video/ti/am335x-fb.h
diff --git a/drivers/video/tilcdc-panel.c b/drivers/video/ti/tilcdc-panel.c
similarity index 100%
rename from drivers/video/tilcdc-panel.c
rename to drivers/video/ti/tilcdc-panel.c
diff --git a/drivers/video/tilcdc-panel.h b/drivers/video/ti/tilcdc-panel.h
similarity index 100%
rename from drivers/video/tilcdc-panel.h
rename to drivers/video/ti/tilcdc-panel.h
diff --git a/drivers/video/tilcdc.c b/drivers/video/ti/tilcdc.c
similarity index 100%
rename from drivers/video/tilcdc.c
rename to drivers/video/ti/tilcdc.c
diff --git a/drivers/video/tilcdc.h b/drivers/video/ti/tilcdc.h
similarity index 100%
rename from drivers/video/tilcdc.h
rename to drivers/video/ti/tilcdc.h
-- 
2.17.1

[PATCH v5 27/27] board: ti: am335x-ice: get CDCE913 clock device

[Dario Binacchi]

With support for other clock drivers, the potentially supported CDCE913
device can no longer be probed without specifying its DT node name.

Signed-off-by: Dario Binacchi <dariobin@libero.it>

---

(no changes since v1)

 board/ti/am335x/board.c | 2 +-
 board/ti/am43xx/board.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/board/ti/am335x/board.c b/board/ti/am335x/board.c
index 984cc5e3ba..e1f64859cf 100644
--- a/board/ti/am335x/board.c
+++ b/board/ti/am335x/board.c
@@ -879,7 +879,7 @@ int board_late_init(void)
 	}
 
 	/* Just probe the potentially supported cdce913 device */
-	uclass_get_device(UCLASS_CLK, 0, &dev);
+	uclass_get_device_by_name(UCLASS_CLK, "cdce913 at 65", &dev);
 
 	return 0;
 }
diff --git a/board/ti/am43xx/board.c b/board/ti/am43xx/board.c
index de49590031..62ed37cb48 100644
--- a/board/ti/am43xx/board.c
+++ b/board/ti/am43xx/board.c
@@ -744,7 +744,7 @@ int board_late_init(void)
 #endif
 
 	/* Just probe the potentially supported cdce913 device */
-	uclass_get_device(UCLASS_CLK, 0, &dev);
+	uclass_get_device_by_name(UCLASS_CLK, "cdce913 at 65", &dev);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v5 18/27] misc: am33xx: add control module driver

[Simon Glass]

On Sun, 25 Oct 2020 at 06:40, Dario Binacchi <dariobin@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@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@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.

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

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

[PATCH v5 18/27] misc: am33xx: add control module driver

[Dario Binacchi]

Hi Simon,

> Il 28/10/2020 03:10 Simon Glass <sjg@chromium.org> ha scritto:
> 
>  
> On Sun, 25 Oct 2020 at 06:40, Dario Binacchi <dariobin@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@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@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

[PATCH v5 18/27] misc: am33xx: add control module driver

[Simon Glass]

Hi Dario,

On Sun, 1 Nov 2020 at 02:13, Dario Binacchi <dariobin@libero.it> wrote:
>
> Hi Simon,
>
> > Il 28/10/2020 03:10 Simon Glass <sjg@chromium.org> ha scritto:
> >
> >
> > On Sun, 25 Oct 2020 at 06:40, Dario Binacchi <dariobin@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@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@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. 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

[PATCH v5 18/27] misc: am33xx: add control module driver

[Dario Binacchi]

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@chromium.org> ha scritto:
> 
>  
> Hi Dario,
> 
> On Sun, 1 Nov 2020 at 02:13, Dario Binacchi <dariobin@libero.it> wrote:
> >
> > Hi Simon,
> >
> > > Il 28/10/2020 03:10 Simon Glass <sjg@chromium.org> ha scritto:
> > >
> > >
> > > On Sun, 25 Oct 2020 at 06:40, Dario Binacchi <dariobin@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@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@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:
@@ -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) \

> 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

Regards,
Dario

[PATCH v5 18/27] misc: am33xx: add control module driver

[Lokesh Vutla]

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@chromium.org> ha scritto:
>>
>>  
>> Hi Dario,
>>
>> On Sun, 1 Nov 2020 at 02:13, Dario Binacchi <dariobin@libero.it> wrote:
>>>
>>> Hi Simon,
>>>
>>>> Il 28/10/2020 03:10 Simon Glass <sjg@chromium.org> ha scritto:
>>>>
>>>>
>>>> On Sun, 25 Oct 2020 at 06:40, Dario Binacchi <dariobin@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@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@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

[PATCH v5 18/27] misc: am33xx: add control module driver

[Simon Glass]

Hi Dario,

On Sun, 8 Nov 2020 at 03:50, Dario Binacchi <dariobin@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 <sjg@chromium.org> ha scritto:
> >
> >
> > Hi Dario,
> >
> > On Sun, 1 Nov 2020 at 02:13, Dario Binacchi <dariobin@libero.it> wrote:
> > >
> > > Hi Simon,
> > >
> > > > Il 28/10/2020 03:10 Simon Glass <sjg@chromium.org> ha scritto:
> > > >
> > > >
> > > > On Sun, 25 Oct 2020 at 06:40, Dario Binacchi <dariobin@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@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@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:
> @@ -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