[U-Boot] [PATCH v4 0/9] dm: imx: Add driver model support for GPIO and serial on cm_fx6

[U-Boot] [PATCH v4 0/9] dm: imx: Add driver model support for GPIO and serial on cm_fx6

[Simon Glass]

This series adjusts the IMX serial and GPIO drivers to support driver model.
As an example of its use, the recently-added cm_fx6 board is converted over
to driver model.

Some minor driver model core changed are required to make this work and
these are included with this series.

Thanks to the Compulab people for helping with this.

Changes in v4:
- Adjust error checking to permit calling gpio_request() multiple times
- Avoid doing low-level SATA init multiple times
- Move SATA changes into the next patch
- Move SATA changes into this patch

Changes in v3:
- Add a check for the Ethernet gpio_request() also
- Add a comment for the CONFIG_SPL_BUILD #ifdef
- Just warn when one of the board init stages fails
- Use gpio_is_requested() in one more place

Changes in v2:
- Add an internal function to check if a GPIO is requested
- Add new patch to add error checking to setup_i2c()
- Add patch to display error number when an error occurs in initcall
- Change 'reserved' to 'requested'
- Check return values of gpio_request()
- Tidy up confusing code that creates names for gpio_request()
- Use the correct namespace for the platform data

Simon Glass (9):
  dm: linker_lists: Add a way to declare multiple objects
  dm: core: Allow a list of devices to be declared in one step
  initcall: Display error number when an error occurs
  dm: serial: Put common code into separate functions
  imx: Add error checking to setup_i2c()
  dm: imx: Use gpio_request() to request GPIOs
  dm: imx: gpio: Support driver model in MXC gpio driver
  dm: imx: serial: Support driver model in the MXC serial driver
  dm: imx: Move cm_fx6 to use driver model for serial and GPIO

 arch/arm/imx-common/i2c-mxv7.c            |  48 ++++-
 arch/arm/include/asm/imx-common/mxc_i2c.h |   4 +-
 board/compulab/cm_fx6/cm_fx6.c            | 111 +++++++++--
 board/compulab/cm_fx6/common.c            |  14 +-
 drivers/gpio/mxc_gpio.c                   | 304 +++++++++++++++++++++++++++++-
 drivers/serial/serial-uclass.c            |  32 ++--
 drivers/serial/serial_mxc.c               | 170 ++++++++++++++---
 include/configs/cm_fx6.h                  |  11 ++
 include/dm/platdata.h                     |   4 +
 include/linker_lists.h                    |  21 +++
 include/serial_mxc.h                      |  14 ++
 lib/initcall.c                            |   8 +-
 12 files changed, 669 insertions(+), 72 deletions(-)
 create mode 100644 include/serial_mxc.h

-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH v4 1/9] dm: linker_lists: Add a way to declare multiple objects

[Simon Glass]

The existing ll_entry_declare() permits a single element of the list to
be added to a linker list. Sometimes we want to add several objects at
once. To avoid lots of messy declarations, add a macro to support this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 include/linker_lists.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/linker_lists.h b/include/linker_lists.h
index 507d61b..6e568bc 100644
--- a/include/linker_lists.h
+++ b/include/linker_lists.h
@@ -141,6 +141,27 @@
 			section(".u_boot_list_2_"#_list"_2_"#_name)))
 
 /**
+ * ll_entry_declare_list() - Declare a list of link-generated array entries
+ * @_type:	Data type of each entry
+ * @_name:	Name of the entry
+ * @_list:	name of the list. Should contain only characters allowed
+ *		in a C variable name!
+ *
+ * This is like ll_entry_declare() but creates multiple entries. It should
+ * be assigned to an array.
+ *
+ * ll_entry_declare_list(struct my_sub_cmd, my_sub_cmd, cmd_sub, cmd.sub) = {
+ *	{ .x = 3, .y = 4 },
+ *	{ .x = 8, .y = 2 },
+ *	{ .x = 1, .y = 7 }
+ * };
+ */
+#define ll_entry_declare_list(_type, _name, _list)			\
+	_type _u_boot_list_2_##_list##_2_##_name[] __aligned(4)		\
+			__attribute__((unused,				\
+			section(".u_boot_list_2_"#_list"_2_"#_name)))
+
+/**
  * We need a 0-byte-size type for iterator symbols, and the compiler
  * does not allow defining objects of C type 'void'. Using an empty
  * struct is allowed by the compiler, but causes gcc versions 4.4 and
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH v4 2/9] dm: core: Allow a list of devices to be declared in one step

[Simon Glass]

The U_BOOT_DEVICE macro allows the declaration of a single U-Boot device.
Add an equivalent macro to declare an array of devices, for convenience.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 include/dm/platdata.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/dm/platdata.h b/include/dm/platdata.h
index 2bc8b14..9e47e51 100644
--- a/include/dm/platdata.h
+++ b/include/dm/platdata.h
@@ -25,4 +25,8 @@ struct driver_info {
 #define U_BOOT_DEVICE(__name)						\
 	ll_entry_declare(struct driver_info, __name, driver_info)
 
+/* Declare a list of devices. The argument is a driver_info[] array */
+#define U_BOOT_DEVICES(__name)						\
+	ll_entry_declare_list(struct driver_info, __name, driver_info)
+
 #endif
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH v4 3/9] initcall: Display error number when an error occurs

[Simon Glass]

Now that some initcall functions return a useful error number, display it
when something goes wrong.

Signed-off-by: Simon Glass <sjg@chromium.org>
Acked-by: Igor Grinberg <grinberg@compulab.co.il>
---

Changes in v4: None
Changes in v3: None
Changes in v2:
- Add patch to display error number when an error occurs in initcall

 lib/initcall.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/initcall.c b/lib/initcall.c
index 7597bad..39f4b3f 100644
--- a/lib/initcall.c
+++ b/lib/initcall.c
@@ -15,14 +15,16 @@ int initcall_run_list(const init_fnc_t init_sequence[])
 
 	for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
 		unsigned long reloc_ofs = 0;
+		int ret;
 
 		if (gd->flags & GD_FLG_RELOC)
 			reloc_ofs = gd->reloc_off;
 		debug("initcall: %p\n", (char *)*init_fnc_ptr - reloc_ofs);
-		if ((*init_fnc_ptr)()) {
-			printf("initcall sequence %p failed at call %p\n",
+		ret = (*init_fnc_ptr)();
+		if (ret) {
+			printf("initcall sequence %p failed at call %p (err=%d)\n",
 			       init_sequence,
-			       (char *)*init_fnc_ptr - reloc_ofs);
+			       (char *)*init_fnc_ptr - reloc_ofs, ret);
 			return -1;
 		}
 	}
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH v4 4/9] dm: serial: Put common code into separate functions

[Simon Glass]

Avoid duplicating the code which deals with getc() and putc(). It is fairly
simple, but may expand later.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/serial/serial-uclass.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 1ac943f..e93c624 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -71,7 +71,7 @@ void serial_initialize(void)
 	serial_find_console_or_panic();
 }
 
-void serial_putc(char ch)
+static void serial_putc_dev(struct udevice *dev, char ch)
 {
 	struct dm_serial_ops *ops = serial_get_ops(cur_dev);
 	int err;
@@ -83,6 +83,11 @@ void serial_putc(char ch)
 		serial_putc('\r');
 }
 
+void serial_putc(char ch)
+{
+	serial_putc_dev(cur_dev, ch);
+}
+
 void serial_setbrg(void)
 {
 	struct dm_serial_ops *ops = serial_get_ops(cur_dev);
@@ -107,28 +112,32 @@ int serial_tstc(void)
 	return 1;
 }
 
-int serial_getc(void)
+static int serial_getc_dev(struct udevice *dev)
 {
-	struct dm_serial_ops *ops = serial_get_ops(cur_dev);
+	struct dm_serial_ops *ops = serial_get_ops(dev);
 	int err;
 
 	do {
-		err = ops->getc(cur_dev);
+		err = ops->getc(dev);
 	} while (err == -EAGAIN);
 
 	return err >= 0 ? err : 0;
 }
 
+int serial_getc(void)
+{
+	return serial_getc_dev(cur_dev);
+}
+
 void serial_stdio_init(void)
 {
 }
 
-void serial_stub_putc(struct stdio_dev *sdev, const char ch)
+static void serial_stub_putc(struct stdio_dev *sdev, const char ch)
 {
 	struct udevice *dev = sdev->priv;
-	struct dm_serial_ops *ops = serial_get_ops(dev);
 
-	ops->putc(dev, ch);
+	serial_putc_dev(dev, ch);
 }
 
 void serial_stub_puts(struct stdio_dev *sdev, const char *str)
@@ -140,15 +149,8 @@ void serial_stub_puts(struct stdio_dev *sdev, const char *str)
 int serial_stub_getc(struct stdio_dev *sdev)
 {
 	struct udevice *dev = sdev->priv;
-	struct dm_serial_ops *ops = serial_get_ops(dev);
-
-	int err;
 
-	do {
-		err = ops->getc(dev);
-	} while (err == -EAGAIN);
-
-	return err >= 0 ? err : 0;
+	return serial_getc_dev(dev);
 }
 
 int serial_stub_tstc(struct stdio_dev *sdev)
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH v4 5/9] imx: Add error checking to setup_i2c()

[Simon Glass]

Since this function can fail, check its return value.

Signed-off-by: Simon Glass <sjg@chromium.org>
Tested-by: Nikita Kiryanov <nikita@compulab.co.il>
---

Changes in v4:
- Move SATA changes into the next patch

Changes in v3:
- Just warn when one of the board init stages fails

Changes in v2:
- Add new patch to add error checking to setup_i2c()

 arch/arm/imx-common/i2c-mxv7.c            | 24 +++++++++++++++----
 arch/arm/include/asm/imx-common/mxc_i2c.h |  4 ++--
 board/compulab/cm_fx6/cm_fx6.c            | 40 ++++++++++++++++++++++++++-----
 3 files changed, 55 insertions(+), 13 deletions(-)

diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c
index a580873..70cff5c 100644
--- a/arch/arm/imx-common/i2c-mxv7.c
+++ b/arch/arm/imx-common/i2c-mxv7.c
@@ -69,15 +69,29 @@ static void * const i2c_bases[] = {
 };
 
 /* i2c_index can be from 0 - 2 */
-void setup_i2c(unsigned i2c_index, int speed, int slave_addr,
-		struct i2c_pads_info *p)
+int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
+	      struct i2c_pads_info *p)
 {
+	int ret;
+
 	if (i2c_index >= ARRAY_SIZE(i2c_bases))
-		return;
+		return -EINVAL;
 	/* Enable i2c clock */
-	enable_i2c_clk(1, i2c_index);
+	ret = enable_i2c_clk(1, i2c_index);
+	if (ret)
+		goto err_clk;
+
 	/* Make sure bus is idle */
-	force_idle_bus(p);
+	ret = force_idle_bus(p);
+	if (ret)
+		goto err_idle;
+
 	bus_i2c_init(i2c_bases[i2c_index], speed, slave_addr,
 			force_idle_bus, p);
+
+	return 0;
+
+err_idle:
+err_clk:
+	return ret;
 }
diff --git a/arch/arm/include/asm/imx-common/mxc_i2c.h b/arch/arm/include/asm/imx-common/mxc_i2c.h
index 182c2f3..af86163 100644
--- a/arch/arm/include/asm/imx-common/mxc_i2c.h
+++ b/arch/arm/include/asm/imx-common/mxc_i2c.h
@@ -52,8 +52,8 @@ struct i2c_pads_info {
 					&mx6q_##name : &mx6s_##name
 #endif
 
-void setup_i2c(unsigned i2c_index, int speed, int slave_addr,
-		struct i2c_pads_info *p);
+int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
+	      struct i2c_pads_info *p);
 void bus_i2c_init(void *base, int speed, int slave_addr,
 		int (*idle_bus_fn)(void *p), void *p);
 int bus_i2c_read(void *base, uchar chip, uint addr, int alen, uchar *buf,
diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
index fdb8ebf..9c6e686 100644
--- a/board/compulab/cm_fx6/cm_fx6.c
+++ b/board/compulab/cm_fx6/cm_fx6.c
@@ -141,14 +141,36 @@ I2C_PADS(i2c2_pads,
 	 IMX_GPIO_NR(1, 6));
 
 
-static void cm_fx6_setup_i2c(void)
+static int cm_fx6_setup_one_i2c(int busnum, struct i2c_pads_info *pads)
 {
-	setup_i2c(0, CONFIG_SYS_I2C_SPEED, 0x7f, I2C_PADS_INFO(i2c0_pads));
-	setup_i2c(1, CONFIG_SYS_I2C_SPEED, 0x7f, I2C_PADS_INFO(i2c1_pads));
-	setup_i2c(2, CONFIG_SYS_I2C_SPEED, 0x7f, I2C_PADS_INFO(i2c2_pads));
+	int ret;
+
+	ret = setup_i2c(busnum, CONFIG_SYS_I2C_SPEED, 0x7f, pads);
+	if (ret)
+		printf("Warning: I2C%d setup failed: %d\n", busnum, ret);
+
+	return ret;
+}
+
+static int cm_fx6_setup_i2c(void)
+{
+	int ret = 0, err;
+
+	/* i2c<x>_pads are wierd macro variables; we can't use an array */
+	err = cm_fx6_setup_one_i2c(0, I2C_PADS_INFO(i2c0_pads));
+	if (err)
+		ret = err;
+	err = cm_fx6_setup_one_i2c(1, I2C_PADS_INFO(i2c1_pads));
+	if (err)
+		ret = err;
+	err = cm_fx6_setup_one_i2c(2, I2C_PADS_INFO(i2c2_pads));
+	if (err)
+		ret = err;
+
+	return ret;
 }
 #else
-static void cm_fx6_setup_i2c(void) { }
+static int cm_fx6_setup_i2c(void) { return 0; }
 #endif
 
 #ifdef CONFIG_USB_EHCI_MX6
@@ -409,9 +431,15 @@ void ft_board_setup(void *blob, bd_t *bd)
 
 int board_init(void)
 {
+	int ret;
+
 	gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
 	cm_fx6_setup_gpmi_nand();
-	cm_fx6_setup_i2c();
+
+	/* Warn on failure but do not abort boot */
+	ret = cm_fx6_setup_i2c();
+	if (ret)
+		printf("Warning: I2C setup failed: %d\n", ret);
 
 	return 0;
 }
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH v4 6/9] dm: imx: Use gpio_request() to request GPIOs

[Simon Glass]

GPIOs should be requested before use. Without this, driver model will not
permit the GPIO to be used.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v4:
- Adjust error checking to permit calling gpio_request() multiple times
- Avoid doing low-level SATA init multiple times
- Move SATA changes into this patch

Changes in v3:
- Add a check for the Ethernet gpio_request() also
- Add a comment for the CONFIG_SPL_BUILD #ifdef
- Just warn when one of the board init stages fails

Changes in v2:
- Check return values of gpio_request()

 arch/arm/imx-common/i2c-mxv7.c | 24 +++++++++++++++++
 board/compulab/cm_fx6/cm_fx6.c | 61 ++++++++++++++++++++++++++++++++----------
 board/compulab/cm_fx6/common.c | 14 +++++++++-
 3 files changed, 84 insertions(+), 15 deletions(-)

diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c
index 70cff5c..aaf6936 100644
--- a/arch/arm/imx-common/i2c-mxv7.c
+++ b/arch/arm/imx-common/i2c-mxv7.c
@@ -4,6 +4,7 @@
  * SPDX-License-Identifier:	GPL-2.0+
  */
 #include <common.h>
+#include <malloc.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/imx-regs.h>
 #include <asm/errno.h>
@@ -72,10 +73,26 @@ static void * const i2c_bases[] = {
 int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
 	      struct i2c_pads_info *p)
 {
+	char *name1, *name2;
 	int ret;
 
 	if (i2c_index >= ARRAY_SIZE(i2c_bases))
 		return -EINVAL;
+
+	name1 = malloc(9);
+	name2 = malloc(9);
+	if (!name1 || !name2)
+		return -ENOMEM;
+	sprintf(name1, "i2c_sda%d", i2c_index);
+	sprintf(name2, "i2c_scl%d", i2c_index);
+	ret = gpio_request(p->sda.gp, name1);
+	if (ret)
+		goto err_req1;
+
+	ret = gpio_request(p->scl.gp, name2);
+	if (ret)
+		goto err_req2;
+
 	/* Enable i2c clock */
 	ret = enable_i2c_clk(1, i2c_index);
 	if (ret)
@@ -93,5 +110,12 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
 
 err_idle:
 err_clk:
+	gpio_free(p->scl.gp);
+err_req2:
+	gpio_free(p->sda.gp);
+err_req1:
+	free(name1);
+	free(name2);
+
 	return ret;
 }
diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
index 9c6e686..53681d4 100644
--- a/board/compulab/cm_fx6/cm_fx6.c
+++ b/board/compulab/cm_fx6/cm_fx6.c
@@ -69,24 +69,52 @@ static iomux_v3_cfg_t const sata_pads[] = {
 	IOMUX_PADS(PAD_EIM_BCLK__GPIO6_IO31   | MUX_PAD_CTRL(NO_PAD_CTRL)),
 };
 
-static void cm_fx6_setup_issd(void)
+static int cm_fx6_setup_issd(void)
 {
-	SETUP_IOMUX_PADS(sata_pads);
-	/* Make sure this gpio has logical 0 value */
+	static bool init_done = false;
+	int ret;
+	int i;
+
+	if (!init_done) {
+		SETUP_IOMUX_PADS(sata_pads);
+
+		for (i = 0; i < ARRAY_SIZE(cm_fx6_issd_gpios); i++) {
+			ret = gpio_request(cm_fx6_issd_gpios[i], "sata");
+			if (ret)
+				return ret;
+		}
+
+		/* Make sure this gpio has logical 0 value */
+		ret = gpio_request(CM_FX6_SATA_PWLOSS_INT, "sata_pwloss_int");
+		if (ret)
+			return ret;
+		init_done = true;
+	}
+
 	gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0);
 	udelay(100);
 
 	cm_fx6_sata_power(0);
 	mdelay(250);
 	cm_fx6_sata_power(1);
+
+	return 0;
 }
 
 #define CM_FX6_SATA_INIT_RETRIES	10
 int sata_initialize(void)
 {
-	int err, i;
+	int err, i, ret;
 
-	cm_fx6_setup_issd();
+	/*
+	 * cm-fx6 may have iSSD not assembled and in this case it has
+	 * bypasses for a (m)SATA socket on the baseboard. The socketed
+	 * device is not controlled by those GPIOs. So just print a warning
+	 * if the setup fails.
+	 */
+	ret = cm_fx6_setup_issd();
+	if (ret)
+		printf("Warning: iSSD setup failed!\n");
 	for (i = 0; i < CM_FX6_SATA_INIT_RETRIES; i++) {
 		err = setup_sata();
 		if (err) {
@@ -183,9 +211,9 @@ static int cm_fx6_usb_hub_reset(void)
 	int err;
 
 	err = gpio_request(CM_FX6_USB_HUB_RST, "usb hub rst");
-	if (err) {
+	if (err && err != -EBUSY) {
 		printf("USB hub rst gpio request failed: %d\n", err);
-		return -1;
+		return err;
 	}
 
 	SETUP_IOMUX_PAD(PAD_SD3_RST__GPIO7_IO08 | MUX_PAD_CTRL(NO_PAD_CTRL));
@@ -199,13 +227,13 @@ static int cm_fx6_usb_hub_reset(void)
 
 static int cm_fx6_init_usb_otg(void)
 {
-	int ret;
+	int err;
 	struct iomuxc *iomux = (struct iomuxc *)IOMUXC_BASE_ADDR;
 
-	ret = gpio_request(SB_FX6_USB_OTG_PWR, "usb-pwr");
-	if (ret) {
-		printf("USB OTG pwr gpio request failed: %d\n", ret);
-		return ret;
+	err = gpio_request(SB_FX6_USB_OTG_PWR, "usb-pwr");
+	if (err && err != -EBUSY) {
+		printf("err OTG pwr gpio request failed: %d\n", err);
+		return err;
 	}
 
 	SETUP_IOMUX_PAD(PAD_EIM_D22__GPIO3_IO22 | MUX_PAD_CTRL(NO_PAD_CTRL));
@@ -340,12 +368,17 @@ static int handle_mac_address(void)
 
 int board_eth_init(bd_t *bis)
 {
-	int res = handle_mac_address();
-	if (res)
+	int err;
+
+	err = handle_mac_address();
+	if (err)
 		puts("No MAC address found\n");
 
 	SETUP_IOMUX_PADS(enet_pads);
 	/* phy reset */
+	err = gpio_request(CM_FX6_ENET_NRST, "enet_nrst");
+	if (err)
+		printf("Etnernet NRST gpio request failed: %d\n", err);
 	gpio_direction_output(CM_FX6_ENET_NRST, 0);
 	udelay(500);
 	gpio_set_value(CM_FX6_ENET_NRST, 1);
diff --git a/board/compulab/cm_fx6/common.c b/board/compulab/cm_fx6/common.c
index 1f39679..22ace2f 100644
--- a/board/compulab/cm_fx6/common.c
+++ b/board/compulab/cm_fx6/common.c
@@ -79,6 +79,18 @@ void cm_fx6_set_ecspi_iomux(void)
 
 int board_spi_cs_gpio(unsigned bus, unsigned cs)
 {
-	return (bus == 0 && cs == 0) ? (CM_FX6_ECSPI_BUS0_CS0) : -1;
+	if (bus != 0 || cs != 0)
+		return -EINVAL;
+
+	/* DM does not support SPL yet and this function is not implemented */
+#ifndef CONFIG_SPL_BUILD
+	int ret;
+
+	ret = gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0");
+	if (ret && ret != -EBUSY)
+		return ret;
+#endif
+
+	return CM_FX6_ECSPI_BUS0_CS0;
 }
 #endif
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH v4 7/9] dm: imx: gpio: Support driver model in MXC gpio driver

[Simon Glass]

Add driver model support with this driver. In this case the platform data
is in the driver. It would be better to put this into an SOC-specific file,
but this is best attempted when more boards are moved over to use driver
model.

Signed-off-by: Simon Glass <sjg@chromium.org>
Acked-by: Igor Grinberg <grinberg@compulab.co.il>
---

Changes in v4: None
Changes in v3:
- Use gpio_is_requested() in one more place

Changes in v2:
- Add an internal function to check if a GPIO is requested
- Change 'reserved' to 'requested'
- Tidy up confusing code that creates names for gpio_request()

 drivers/gpio/mxc_gpio.c | 304 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 303 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
index 6a572d5..3f7b7d2 100644
--- a/drivers/gpio/mxc_gpio.c
+++ b/drivers/gpio/mxc_gpio.c
@@ -8,16 +8,31 @@
  * SPDX-License-Identifier:	GPL-2.0+
  */
 #include <common.h>
+#include <errno.h>
+#include <dm.h>
+#include <malloc.h>
 #include <asm/arch/imx-regs.h>
 #include <asm/gpio.h>
 #include <asm/io.h>
-#include <errno.h>
 
 enum mxc_gpio_direction {
 	MXC_GPIO_DIRECTION_IN,
 	MXC_GPIO_DIRECTION_OUT,
 };
 
+#define GPIO_NAME_SIZE			20
+#define GPIO_PER_BANK			32
+
+struct mxc_gpio_plat {
+	struct gpio_regs *regs;
+};
+
+struct mxc_bank_info {
+	char label[GPIO_PER_BANK][GPIO_NAME_SIZE];
+	struct gpio_regs *regs;
+};
+
+#ifndef CONFIG_DM_GPIO
 #define GPIO_TO_PORT(n)		(n / 32)
 
 /* GPIO port description */
@@ -134,3 +149,290 @@ int gpio_direction_output(unsigned gpio, int value)
 
 	return mxc_gpio_direction(gpio, MXC_GPIO_DIRECTION_OUT);
 }
+#endif
+
+#ifdef CONFIG_DM_GPIO
+/**
+ * gpio_is_requested() - check if a GPIO has been requested
+ *
+ * @bank:	Bank to check
+ * @offset:	GPIO offset within bank to check
+ * @return true if marked as requested, false if not
+ */
+static inline bool gpio_is_requested(struct mxc_bank_info *bank, int offset)
+{
+	return *bank->label[offset] != '\0';
+}
+
+static int mxc_gpio_is_output(struct gpio_regs *regs, int offset)
+{
+	u32 val;
+
+	val = readl(&regs->gpio_dir);
+
+	return val & (1 << offset) ? 1 : 0;
+}
+
+static void mxc_gpio_bank_direction(struct gpio_regs *regs, int offset,
+				    enum mxc_gpio_direction direction)
+{
+	u32 l;
+
+	l = readl(&regs->gpio_dir);
+
+	switch (direction) {
+	case MXC_GPIO_DIRECTION_OUT:
+		l |= 1 << offset;
+		break;
+	case MXC_GPIO_DIRECTION_IN:
+		l &= ~(1 << offset);
+	}
+	writel(l, &regs->gpio_dir);
+}
+
+static void mxc_gpio_bank_set_value(struct gpio_regs *regs, int offset,
+				    int value)
+{
+	u32 l;
+
+	l = readl(&regs->gpio_dr);
+	if (value)
+		l |= 1 << offset;
+	else
+		l &= ~(1 << offset);
+	writel(l, &regs->gpio_dr);
+}
+
+static int mxc_gpio_bank_get_value(struct gpio_regs *regs, int offset)
+{
+	return (readl(&regs->gpio_psr) >> offset) & 0x01;
+}
+
+static int mxc_gpio_bank_get_output_value(struct gpio_regs *regs, int offset)
+{
+	return (readl(&regs->gpio_dr) >> offset) & 0x01;
+}
+
+static int check_requested(struct udevice *dev, unsigned offset,
+			   const char *func)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	struct gpio_dev_priv *uc_priv = dev->uclass_priv;
+
+	if (!gpio_is_requested(bank, offset)) {
+		printf("mxc_gpio: %s: error: gpio %s%d not requested\n",
+		       func, uc_priv->bank_name, offset);
+		return -EPERM;
+	}
+
+	return 0;
+}
+
+/* set GPIO pin 'gpio' as an input */
+static int mxc_gpio_direction_input(struct udevice *dev, unsigned offset)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	int ret;
+
+	ret = check_requested(dev, offset, __func__);
+	if (ret)
+		return ret;
+
+	/* Configure GPIO direction as input. */
+	mxc_gpio_bank_direction(bank->regs, offset, MXC_GPIO_DIRECTION_IN);
+
+	return 0;
+}
+
+/* set GPIO pin 'gpio' as an output, with polarity 'value' */
+static int mxc_gpio_direction_output(struct udevice *dev, unsigned offset,
+				       int value)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	int ret;
+
+	ret = check_requested(dev, offset, __func__);
+	if (ret)
+		return ret;
+
+	/* Configure GPIO output value. */
+	mxc_gpio_bank_set_value(bank->regs, offset, value);
+
+	/* Configure GPIO direction as output. */
+	mxc_gpio_bank_direction(bank->regs, offset, MXC_GPIO_DIRECTION_OUT);
+
+	return 0;
+}
+
+/* read GPIO IN value of pin 'gpio' */
+static int mxc_gpio_get_value(struct udevice *dev, unsigned offset)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	int ret;
+
+	ret = check_requested(dev, offset, __func__);
+	if (ret)
+		return ret;
+
+	return mxc_gpio_bank_get_value(bank->regs, offset);
+}
+
+/* write GPIO OUT value to pin 'gpio' */
+static int mxc_gpio_set_value(struct udevice *dev, unsigned offset,
+				 int value)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	int ret;
+
+	ret = check_requested(dev, offset, __func__);
+	if (ret)
+		return ret;
+
+	mxc_gpio_bank_set_value(bank->regs, offset, value);
+
+	return 0;
+}
+
+static int mxc_gpio_get_state(struct udevice *dev, unsigned int offset,
+			      char *buf, int bufsize)
+{
+	struct gpio_dev_priv *uc_priv = dev->uclass_priv;
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	const char *label;
+	bool requested;
+	bool is_output;
+	int size;
+
+	label = bank->label[offset];
+	is_output = mxc_gpio_is_output(bank->regs, offset);
+	size = snprintf(buf, bufsize, "%s%d: ",
+			uc_priv->bank_name ? uc_priv->bank_name : "", offset);
+	buf += size;
+	bufsize -= size;
+	requested = gpio_is_requested(bank, offset);
+	snprintf(buf, bufsize, "%s: %d [%c]%s%s",
+		 is_output ? "out" : " in",
+		 is_output ?
+			mxc_gpio_bank_get_output_value(bank->regs, offset) :
+			mxc_gpio_bank_get_value(bank->regs, offset),
+		 requested ? 'x' : ' ',
+		 requested ? " " : "",
+		 label);
+
+	return 0;
+}
+
+static int mxc_gpio_request(struct udevice *dev, unsigned offset,
+			      const char *label)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+
+	if (gpio_is_requested(bank, offset))
+		return -EBUSY;
+
+	strncpy(bank->label[offset], label, GPIO_NAME_SIZE);
+	bank->label[offset][GPIO_NAME_SIZE - 1] = '\0';
+
+	return 0;
+}
+
+static int mxc_gpio_free(struct udevice *dev, unsigned offset)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	int ret;
+
+	ret = check_requested(dev, offset, __func__);
+	if (ret)
+		return ret;
+	bank->label[offset][0] = '\0';
+
+	return 0;
+}
+
+static int mxc_gpio_get_function(struct udevice *dev, unsigned offset)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+
+	if (!gpio_is_requested(bank, offset))
+		return GPIOF_UNUSED;
+
+	/* GPIOF_FUNC is not implemented yet */
+	if (mxc_gpio_is_output(bank->regs, offset))
+		return GPIOF_OUTPUT;
+	else
+		return GPIOF_INPUT;
+}
+
+static const struct dm_gpio_ops gpio_mxc_ops = {
+	.request		= mxc_gpio_request,
+	.free			= mxc_gpio_free,
+	.direction_input	= mxc_gpio_direction_input,
+	.direction_output	= mxc_gpio_direction_output,
+	.get_value		= mxc_gpio_get_value,
+	.set_value		= mxc_gpio_set_value,
+	.get_function		= mxc_gpio_get_function,
+	.get_state		= mxc_gpio_get_state,
+};
+
+static const struct mxc_gpio_plat mxc_plat[] = {
+	{ (struct gpio_regs *)GPIO1_BASE_ADDR },
+	{ (struct gpio_regs *)GPIO2_BASE_ADDR },
+	{ (struct gpio_regs *)GPIO3_BASE_ADDR },
+#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
+		defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ (struct gpio_regs *)GPIO4_BASE_ADDR },
+#endif
+#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ (struct gpio_regs *)GPIO5_BASE_ADDR },
+	{ (struct gpio_regs *)GPIO6_BASE_ADDR },
+#endif
+#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ (struct gpio_regs *)GPIO7_BASE_ADDR },
+#endif
+};
+
+static int mxc_gpio_probe(struct udevice *dev)
+{
+	struct mxc_bank_info *bank = dev_get_priv(dev);
+	struct mxc_gpio_plat *plat = dev_get_platdata(dev);
+	struct gpio_dev_priv *uc_priv = dev->uclass_priv;
+	int banknum;
+	char name[18], *str;
+
+	banknum = plat - mxc_plat;
+	sprintf(name, "GPIO%d_", banknum + 1);
+	str = strdup(name);
+	if (!str)
+		return -ENOMEM;
+	uc_priv->bank_name = str;
+	uc_priv->gpio_count = GPIO_PER_BANK;
+	bank->regs = plat->regs;
+
+	return 0;
+}
+
+U_BOOT_DRIVER(gpio_mxc) = {
+	.name	= "gpio_mxc",
+	.id	= UCLASS_GPIO,
+	.ops	= &gpio_mxc_ops,
+	.probe	= mxc_gpio_probe,
+	.priv_auto_alloc_size = sizeof(struct mxc_bank_info),
+};
+
+U_BOOT_DEVICES(mxc_gpios) = {
+	{ "gpio_mxc", &mxc_plat[0] },
+	{ "gpio_mxc", &mxc_plat[1] },
+	{ "gpio_mxc", &mxc_plat[2] },
+#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
+		defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ "gpio_mxc", &mxc_plat[3] },
+#endif
+#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ "gpio_mxc", &mxc_plat[4] },
+	{ "gpio_mxc", &mxc_plat[5] },
+#endif
+#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ "gpio_mxc", &mxc_plat[6] },
+#endif
+};
+#endif
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH v4 8/9] dm: imx: serial: Support driver model in the MXC serial driver

[Simon Glass]

Add driver model support with this driver. Boards which use this driver
should define platform data in their board files.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/serial/serial_mxc.c | 170 +++++++++++++++++++++++++++++++++++++-------
 include/serial_mxc.h        |  14 ++++
 2 files changed, 159 insertions(+), 25 deletions(-)
 create mode 100644 include/serial_mxc.h

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index 313d560..9ce24f9 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -5,37 +5,15 @@
  */
 
 #include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <serial_mxc.h>
 #include <watchdog.h>
 #include <asm/arch/imx-regs.h>
 #include <asm/arch/clock.h>
 #include <serial.h>
 #include <linux/compiler.h>
 
-#define __REG(x)     (*((volatile u32 *)(x)))
-
-#ifndef CONFIG_MXC_UART_BASE
-#error "define CONFIG_MXC_UART_BASE to use the MXC UART driver"
-#endif
-
-#define UART_PHYS	CONFIG_MXC_UART_BASE
-
-/* Register definitions */
-#define URXD  0x0  /* Receiver Register */
-#define UTXD  0x40 /* Transmitter Register */
-#define UCR1  0x80 /* Control Register 1 */
-#define UCR2  0x84 /* Control Register 2 */
-#define UCR3  0x88 /* Control Register 3 */
-#define UCR4  0x8c /* Control Register 4 */
-#define UFCR  0x90 /* FIFO Control Register */
-#define USR1  0x94 /* Status Register 1 */
-#define USR2  0x98 /* Status Register 2 */
-#define UESC  0x9c /* Escape Character Register */
-#define UTIM  0xa0 /* Escape Timer Register */
-#define UBIR  0xa4 /* BRM Incremental Register */
-#define UBMR  0xa8 /* BRM Modulator Register */
-#define UBRC  0xac /* Baud Rate Count Register */
-#define UTS   0xb4 /* UART Test Register (mx31) */
-
 /* UART Control Register Bit Fields.*/
 #define  URXD_CHARRDY    (1<<15)
 #define  URXD_ERR        (1<<14)
@@ -128,6 +106,33 @@
 #define  UTS_RXFULL	 (1<<3)	 /* RxFIFO full */
 #define  UTS_SOFTRST	 (1<<0)	 /* Software reset */
 
+#ifndef CONFIG_DM_SERIAL
+
+#ifndef CONFIG_MXC_UART_BASE
+#error "define CONFIG_MXC_UART_BASE to use the MXC UART driver"
+#endif
+
+#define UART_PHYS	CONFIG_MXC_UART_BASE
+
+#define __REG(x)     (*((volatile u32 *)(x)))
+
+/* Register definitions */
+#define URXD  0x0  /* Receiver Register */
+#define UTXD  0x40 /* Transmitter Register */
+#define UCR1  0x80 /* Control Register 1 */
+#define UCR2  0x84 /* Control Register 2 */
+#define UCR3  0x88 /* Control Register 3 */
+#define UCR4  0x8c /* Control Register 4 */
+#define UFCR  0x90 /* FIFO Control Register */
+#define USR1  0x94 /* Status Register 1 */
+#define USR2  0x98 /* Status Register 2 */
+#define UESC  0x9c /* Escape Character Register */
+#define UTIM  0xa0 /* Escape Timer Register */
+#define UBIR  0xa4 /* BRM Incremental Register */
+#define UBMR  0xa8 /* BRM Modulator Register */
+#define UBRC  0xac /* Baud Rate Count Register */
+#define UTS   0xb4 /* UART Test Register (mx31) */
+
 DECLARE_GLOBAL_DATA_PTR;
 
 static void mxc_serial_setbrg(void)
@@ -222,3 +227,118 @@ __weak struct serial_device *default_serial_console(void)
 {
 	return &mxc_serial_drv;
 }
+#endif
+
+#ifdef CONFIG_DM_SERIAL
+
+struct mxc_uart {
+	u32 rxd;
+	u32 spare0[15];
+
+	u32 txd;
+	u32 spare1[15];
+
+	u32 cr1;
+	u32 cr2;
+	u32 cr3;
+	u32 cr4;
+
+	u32 fcr;
+	u32 sr1;
+	u32 sr2;
+	u32 esc;
+
+	u32 tim;
+	u32 bir;
+	u32 bmr;
+	u32 brc;
+
+	u32 onems;
+	u32 ts;
+};
+
+int mxc_serial_setbrg(struct udevice *dev, int baudrate)
+{
+	struct mxc_serial_platdata *plat = dev->platdata;
+	struct mxc_uart *const uart = plat->reg;
+	u32 clk = imx_get_uartclk();
+
+	writel(4 << 7, &uart->fcr); /* divide input clock by 2 */
+	writel(0xf, &uart->bir);
+	writel(clk / (2 * baudrate), &uart->bmr);
+
+	writel(UCR2_WS | UCR2_IRTS | UCR2_RXEN | UCR2_TXEN | UCR2_SRST,
+	       &uart->cr2);
+	writel(UCR1_UARTEN, &uart->cr1);
+
+	return 0;
+}
+
+static int mxc_serial_probe(struct udevice *dev)
+{
+	struct mxc_serial_platdata *plat = dev->platdata;
+	struct mxc_uart *const uart = plat->reg;
+
+	writel(0, &uart->cr1);
+	writel(0, &uart->cr2);
+	while (!(readl(&uart->cr2) & UCR2_SRST));
+	writel(0x704 | UCR3_ADNIMP, &uart->cr3);
+	writel(0x8000, &uart->cr4);
+	writel(0x2b, &uart->esc);
+	writel(0, &uart->tim);
+	writel(0, &uart->ts);
+
+	return 0;
+}
+
+static int mxc_serial_getc(struct udevice *dev)
+{
+	struct mxc_serial_platdata *plat = dev->platdata;
+	struct mxc_uart *const uart = plat->reg;
+
+	if (readl(&uart->ts) & UTS_RXEMPTY)
+		return -EAGAIN;
+
+	return readl(&uart->rxd) & URXD_RX_DATA;
+}
+
+static int mxc_serial_putc(struct udevice *dev, const char ch)
+{
+	struct mxc_serial_platdata *plat = dev->platdata;
+	struct mxc_uart *const uart = plat->reg;
+
+	if (!(readl(&uart->ts) & UTS_TXEMPTY))
+		return -EAGAIN;
+
+	writel(ch, &uart->txd);
+
+	return 0;
+}
+
+static int mxc_serial_pending(struct udevice *dev, bool input)
+{
+	struct mxc_serial_platdata *plat = dev->platdata;
+	struct mxc_uart *const uart = plat->reg;
+	uint32_t sr2 = readl(&uart->sr2);
+
+	if (input)
+		return sr2 & USR2_RDR ? 1 : 0;
+	else
+		return sr2 & USR2_TXDC ? 0 : 1;
+}
+
+static const struct dm_serial_ops mxc_serial_ops = {
+	.putc = mxc_serial_putc,
+	.pending = mxc_serial_pending,
+	.getc = mxc_serial_getc,
+	.setbrg = mxc_serial_setbrg,
+};
+
+U_BOOT_DRIVER(serial_mxc) = {
+	.name	= "serial_mxc",
+	.id	= UCLASS_SERIAL,
+	.probe = mxc_serial_probe,
+	.ops	= &mxc_serial_ops,
+	.flags = DM_FLAG_PRE_RELOC,
+};
+#endif
diff --git a/include/serial_mxc.h b/include/serial_mxc.h
new file mode 100644
index 0000000..7d3ace2
--- /dev/null
+++ b/include/serial_mxc.h
@@ -0,0 +1,14 @@
+/*
+ * Copyright (c) 2014 Google, Inc
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef __serial_mxc_h
+#define __serial_mxc_h
+
+/* Information about a serial port */
+struct mxc_serial_platdata {
+	struct mxc_uart *reg;  /* address of registers in physical memory */
+};
+
+#endif
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH v4 9/9] dm: imx: Move cm_fx6 to use driver model for serial and GPIO

[Simon Glass]

Now that serial and GPIO are available for iMX.6, move cm_fx6 over as an
example.

Acked-by: Igor Grinberg <grinberg@compulab.co.il>
Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2:
- Use the correct namespace for the platform data

 board/compulab/cm_fx6/cm_fx6.c | 10 ++++++++++
 include/configs/cm_fx6.h       | 11 +++++++++++
 2 files changed, 21 insertions(+)

diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
index 53681d4..dd5698f 100644
--- a/board/compulab/cm_fx6/cm_fx6.c
+++ b/board/compulab/cm_fx6/cm_fx6.c
@@ -9,11 +9,13 @@
  */
 
 #include <common.h>
+#include <dm.h>
 #include <fsl_esdhc.h>
 #include <miiphy.h>
 #include <netdev.h>
 #include <fdt_support.h>
 #include <sata.h>
+#include <serial_mxc.h>
 #include <asm/arch/crm_regs.h>
 #include <asm/arch/sys_proto.h>
 #include <asm/arch/iomux.h>
@@ -542,3 +544,11 @@ u32 get_board_rev(void)
 	return cl_eeprom_get_board_rev();
 }
 
+static struct mxc_serial_platdata cm_fx6_mxc_serial_plat = {
+	.reg = (struct mxc_uart *)UART4_BASE,
+};
+
+U_BOOT_DEVICE(cm_fx6_serial) = {
+	.name	= "serial_mxc",
+	.platdata = &cm_fx6_mxc_serial_plat,
+};
diff --git a/include/configs/cm_fx6.h b/include/configs/cm_fx6.h
index 10d02b4..1f55150 100644
--- a/include/configs/cm_fx6.h
+++ b/include/configs/cm_fx6.h
@@ -21,6 +21,17 @@
 #define CONFIG_MACH_TYPE		4273
 #define CONFIG_SYS_HZ			1000
 
+#ifndef CONFIG_SPL_BUILD
+#define CONFIG_DM
+#define CONFIG_CMD_DM
+
+#define CONFIG_DM_GPIO
+#define CONFIG_CMD_GPIO
+
+#define CONFIG_DM_SERIAL
+#define CONFIG_SYS_MALLOC_F_LEN		(1 << 10)
+#endif
+
 /* Display information on boot */
 #define CONFIG_DISPLAY_CPUINFO
 #define CONFIG_DISPLAY_BOARDINFO
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH v4 0/9] dm: imx: Add driver model support for GPIO and serial on cm_fx6

[Simon Glass]

Hi Nikita,

On 1 October 2014 19:57, Simon Glass <sjg@chromium.org> wrote:
> This series adjusts the IMX serial and GPIO drivers to support driver model.
> As an example of its use, the recently-added cm_fx6 board is converted over
> to driver model.
>
> Some minor driver model core changed are required to make this work and
> these are included with this series.
>
> Thanks to the Compulab people for helping with this.
>
> Changes in v4:
> - Adjust error checking to permit calling gpio_request() multiple times
> - Avoid doing low-level SATA init multiple times
> - Move SATA changes into the next patch
> - Move SATA changes into this patch
>

Hopefully this is closer. I found out why I couldn't do what I
suggested for the gpio_request() handling. It seems easier to check
for -EBUSY. Feel free to adjust these patches as you wish - I was
planning on pushing them to dm/next when ready.

Regards,
Simon

[U-Boot] [PATCH v4 9/9] dm: imx: Move cm_fx6 to use driver model for serial and GPIO

[Nikita Kiryanov]

Hi Simon,

On 02/10/14 04:57, Simon Glass wrote:
> Now that serial and GPIO are available for iMX.6, move cm_fx6 over as an
> example.
>
> Acked-by: Igor Grinberg <grinberg@compulab.co.il>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Acked-by: Nikita Kiryanov <nikita@compulab.co.il>

-- 
Regards,
Nikita Kiryanov

[U-Boot] [PATCH 0/2] Split "dm: imx: Use gpio_request() to request GPIOs"

[Nikita Kiryanov]

This series splits the patch "dm: imx: Use gpio_request() to request GPIOs" into
two patches, one for the i2c-mxv7.c stuff retaining Simon Glass as the author,
and the other a rework of cm_fx6 subsystem init functions to accomodate calls
to gpio_request().

Cc: Igor Grinberg <grinberg@compulab.co.il>
Cc: Simon Glass <sjg@chromium.org>

Nikita Kiryanov (1):
  arm: mx6: cm_fx6: use gpio request

Simon Glass (1):
  dm: imx: i2c: Use gpio_request() to request GPIOs

 arch/arm/imx-common/i2c-mxv7.c |  25 ++++++++
 board/compulab/cm_fx6/cm_fx6.c | 133 +++++++++++++++++++++++++++++------------
 2 files changed, 119 insertions(+), 39 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH 1/2] dm: imx: i2c: Use gpio_request() to request GPIOs

[Nikita Kiryanov]

From: Simon Glass <sjg@chromium.org>

GPIOs should be requested before use. Without this, driver model will
not permit the GPIO to be used.

Cc: Igor Grinberg <grinberg@compulab.co.il>
Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
---
 arch/arm/imx-common/i2c-mxv7.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c
index 70cff5c..34f5387 100644
--- a/arch/arm/imx-common/i2c-mxv7.c
+++ b/arch/arm/imx-common/i2c-mxv7.c
@@ -4,6 +4,7 @@
  * SPDX-License-Identifier:	GPL-2.0+
  */
 #include <common.h>
+#include <malloc.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/imx-regs.h>
 #include <asm/errno.h>
@@ -72,10 +73,27 @@ static void * const i2c_bases[] = {
 int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
 	      struct i2c_pads_info *p)
 {
+	char *name1, *name2;
 	int ret;
 
 	if (i2c_index >= ARRAY_SIZE(i2c_bases))
 		return -EINVAL;
+
+	name1 = malloc(9);
+	name2 = malloc(9);
+	if (!name1 || !name2)
+		return -ENOMEM;
+
+	sprintf(name1, "i2c_sda%d", i2c_index);
+	sprintf(name2, "i2c_scl%d", i2c_index);
+	ret = gpio_request(p->sda.gp, name1);
+	if (ret)
+		goto err_req1;
+
+	ret = gpio_request(p->scl.gp, name2);
+	if (ret)
+		goto err_req2;
+
 	/* Enable i2c clock */
 	ret = enable_i2c_clk(1, i2c_index);
 	if (ret)
@@ -93,5 +111,12 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
 
 err_idle:
 err_clk:
+	gpio_free(p->scl.gp);
+err_req2:
+	gpio_free(p->sda.gp);
+err_req1:
+	free(name1);
+	free(name2);
+
 	return ret;
 }
-- 
1.9.1

[U-Boot] [PATCH 2/2] arm: mx6: cm_fx6: use gpio request

[Nikita Kiryanov]

Use gpio_request for all the gpios that are utilized by various
subsystems in cm-fx6, and refactor the relevant init functions
so that all gpios are requested during board_init(), not during
subsystem init, thus avoiding the need to manage gpio ownership
each time a subsystem is initialized.

The new division of labor is:
During board_init() muxes are setup and gpios are requested.
During subsystem init gpios are toggled.

Cc: Igor Grinberg <grinberg@compulab.co.il>
Cc: Simon Glass <sjg@chromium.org>
Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
---
 board/compulab/cm_fx6/cm_fx6.c | 133 +++++++++++++++++++++++++++++------------
 1 file changed, 94 insertions(+), 39 deletions(-)

diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
index 9c6e686..f0edc8f 100644
--- a/board/compulab/cm_fx6/cm_fx6.c
+++ b/board/compulab/cm_fx6/cm_fx6.c
@@ -69,16 +69,23 @@ static iomux_v3_cfg_t const sata_pads[] = {
 	IOMUX_PADS(PAD_EIM_BCLK__GPIO6_IO31   | MUX_PAD_CTRL(NO_PAD_CTRL)),
 };
 
-static void cm_fx6_setup_issd(void)
+static int cm_fx6_setup_issd(void)
 {
+	int ret, i;
+
 	SETUP_IOMUX_PADS(sata_pads);
-	/* Make sure this gpio has logical 0 value */
-	gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0);
-	udelay(100);
 
-	cm_fx6_sata_power(0);
-	mdelay(250);
-	cm_fx6_sata_power(1);
+	for (i = 0; i < ARRAY_SIZE(cm_fx6_issd_gpios); i++) {
+		ret = gpio_request(cm_fx6_issd_gpios[i], "sata");
+		if (ret)
+			return ret;
+	}
+
+	ret = gpio_request(CM_FX6_SATA_PWLOSS_INT, "sata_pwloss_int");
+	if (ret)
+		return ret;
+
+	return 0;
 }
 
 #define CM_FX6_SATA_INIT_RETRIES	10
@@ -86,7 +93,14 @@ int sata_initialize(void)
 {
 	int err, i;
 
-	cm_fx6_setup_issd();
+	/* Make sure this gpio has logical 0 value */
+	gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0);
+	udelay(100);
+
+	cm_fx6_sata_power(0);
+	mdelay(250);
+	cm_fx6_sata_power(1);
+
 	for (i = 0; i < CM_FX6_SATA_INIT_RETRIES; i++) {
 		err = setup_sata();
 		if (err) {
@@ -109,6 +123,8 @@ int sata_initialize(void)
 
 	return err;
 }
+#else
+static int cm_fx6_setup_issd(void) { return 0; }
 #endif
 
 #ifdef CONFIG_SYS_I2C_MXC
@@ -177,35 +193,32 @@ static int cm_fx6_setup_i2c(void) { return 0; }
 #define WEAK_PULLDOWN	(PAD_CTL_PUS_100K_DOWN |		\
 			PAD_CTL_SPEED_MED | PAD_CTL_DSE_40ohm |	\
 			PAD_CTL_HYS | PAD_CTL_SRE_SLOW)
+#define MX6_USBNC_BASEADDR	0x2184800
+#define USBNC_USB_H1_PWR_POL	(1 << 9)
 
-static int cm_fx6_usb_hub_reset(void)
+static int cm_fx6_setup_usb_host(void)
 {
 	int err;
 
 	err = gpio_request(CM_FX6_USB_HUB_RST, "usb hub rst");
-	if (err) {
-		printf("USB hub rst gpio request failed: %d\n", err);
-		return -1;
-	}
+	if (err)
+		return err;
 
+	SETUP_IOMUX_PAD(PAD_GPIO_0__USB_H1_PWR | MUX_PAD_CTRL(NO_PAD_CTRL));
 	SETUP_IOMUX_PAD(PAD_SD3_RST__GPIO7_IO08 | MUX_PAD_CTRL(NO_PAD_CTRL));
-	gpio_direction_output(CM_FX6_USB_HUB_RST, 0);
-	udelay(10);
-	gpio_direction_output(CM_FX6_USB_HUB_RST, 1);
-	mdelay(1);
 
 	return 0;
 }
 
-static int cm_fx6_init_usb_otg(void)
+static int cm_fx6_setup_usb_otg(void)
 {
-	int ret;
+	int err;
 	struct iomuxc *iomux = (struct iomuxc *)IOMUXC_BASE_ADDR;
 
-	ret = gpio_request(SB_FX6_USB_OTG_PWR, "usb-pwr");
-	if (ret) {
-		printf("USB OTG pwr gpio request failed: %d\n", ret);
-		return ret;
+	err = gpio_request(SB_FX6_USB_OTG_PWR, "usb-pwr");
+	if (err) {
+		printf("USB OTG pwr gpio request failed: %d\n", err);
+		return err;
 	}
 
 	SETUP_IOMUX_PAD(PAD_EIM_D22__GPIO3_IO22 | MUX_PAD_CTRL(NO_PAD_CTRL));
@@ -216,25 +229,27 @@ static int cm_fx6_init_usb_otg(void)
 	return gpio_direction_output(SB_FX6_USB_OTG_PWR, 0);
 }
 
-#define MX6_USBNC_BASEADDR	0x2184800
-#define USBNC_USB_H1_PWR_POL	(1 << 9)
 int board_ehci_hcd_init(int port)
 {
+	int ret;
 	u32 *usbnc_usb_uh1_ctrl = (u32 *)(MX6_USBNC_BASEADDR + 4);
 
-	switch (port) {
-	case 0:
-		return cm_fx6_init_usb_otg();
-	case 1:
-		SETUP_IOMUX_PAD(PAD_GPIO_0__USB_H1_PWR |
-				MUX_PAD_CTRL(NO_PAD_CTRL));
+	/* Only 1 host controller in use. port 0 is OTG & needs no attention */
+	if (port != 1)
+		return 0;
 
-		/* Set PWR polarity to match power switch's enable polarity */
-		setbits_le32(usbnc_usb_uh1_ctrl, USBNC_USB_H1_PWR_POL);
-		return cm_fx6_usb_hub_reset();
-	default:
-		break;
-	}
+	/* Set PWR polarity to match power switch's enable polarity */
+	setbits_le32(usbnc_usb_uh1_ctrl, USBNC_USB_H1_PWR_POL);
+	ret = gpio_direction_output(CM_FX6_USB_HUB_RST, 0);
+	if (ret)
+		return ret;
+
+	udelay(10);
+	ret = gpio_direction_output(CM_FX6_USB_HUB_RST, 1);
+	if (ret)
+		return ret;
+
+	mdelay(1);
 
 	return 0;
 }
@@ -246,6 +261,9 @@ int board_ehci_power(int port, int on)
 
 	return 0;
 }
+#else
+static int cm_fx6_setup_usb_otg(void) { return 0; }
+static int cm_fx6_setup_usb_host(void) { return 0; }
 #endif
 
 #ifdef CONFIG_FEC_MXC
@@ -340,12 +358,17 @@ static int handle_mac_address(void)
 
 int board_eth_init(bd_t *bis)
 {
-	int res = handle_mac_address();
-	if (res)
+	int err;
+
+	err = handle_mac_address();
+	if (err)
 		puts("No MAC address found\n");
 
 	SETUP_IOMUX_PADS(enet_pads);
 	/* phy reset */
+	err = gpio_request(CM_FX6_ENET_NRST, "enet_nrst");
+	if (err)
+		printf("Etnernet NRST gpio request failed: %d\n", err);
 	gpio_direction_output(CM_FX6_ENET_NRST, 0);
 	udelay(500);
 	gpio_set_value(CM_FX6_ENET_NRST, 1);
@@ -416,6 +439,16 @@ int board_mmc_init(bd_t *bis)
 }
 #endif
 
+#ifdef CONFIG_MXC_SPI
+int cm_fx6_setup_ecspi(void)
+{
+	cm_fx6_set_ecspi_iomux();
+	return gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0");
+}
+#else
+int cm_fx6_setup_ecspi(void) { return 0; }
+#endif
+
 #ifdef CONFIG_OF_BOARD_SETUP
 void ft_board_setup(void *blob, bd_t *bd)
 {
@@ -436,6 +469,28 @@ int board_init(void)
 	gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
 	cm_fx6_setup_gpmi_nand();
 
+	ret = cm_fx6_setup_ecspi();
+	if (ret)
+		printf("Warning: ECSPI setup failed: %d\n", ret);
+
+	ret = cm_fx6_setup_usb_otg();
+	if (ret)
+		printf("Warning: USB OTG setup failed: %d\n", ret);
+
+	ret = cm_fx6_setup_usb_host();
+	if (ret)
+		printf("Warning: USB host setup failed: %d\n", ret);
+
+	/*
+	 * cm-fx6 may have iSSD not assembled and in this case it has
+	 * bypasses for a (m)SATA socket on the baseboard. The socketed
+	 * device is not controlled by those GPIOs. So just print a warning
+	 * if the setup fails.
+	 */
+	ret = cm_fx6_setup_issd();
+	if (ret)
+		printf("Warning: iSSD setup failed: %d\n", ret);
+
 	/* Warn on failure but do not abort boot */
 	ret = cm_fx6_setup_i2c();
 	if (ret)
-- 
1.9.1

[U-Boot] [PATCH v4 6/9] dm: imx: Use gpio_request() to request GPIOs

[Nikita Kiryanov]

Hi Simon,

On 02/10/14 04:57, Simon Glass wrote:
> GPIOs should be requested before use. Without this, driver model will not
> permit the GPIO to be used.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v4:
> - Adjust error checking to permit calling gpio_request() multiple times
> - Avoid doing low-level SATA init multiple times
> - Move SATA changes into this patch
>
> Changes in v3:
> - Add a check for the Ethernet gpio_request() also
> - Add a comment for the CONFIG_SPL_BUILD #ifdef
> - Just warn when one of the board init stages fails
>
> Changes in v2:
> - Check return values of gpio_request()
>
>   arch/arm/imx-common/i2c-mxv7.c | 24 +++++++++++++++++
>   board/compulab/cm_fx6/cm_fx6.c | 61 ++++++++++++++++++++++++++++++++----------
>   board/compulab/cm_fx6/common.c | 14 +++++++++-
>   3 files changed, 84 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c
> index 70cff5c..aaf6936 100644
> --- a/arch/arm/imx-common/i2c-mxv7.c
> +++ b/arch/arm/imx-common/i2c-mxv7.c
> @@ -4,6 +4,7 @@
>    * SPDX-License-Identifier:	GPL-2.0+
>    */
>   #include <common.h>
> +#include <malloc.h>
>   #include <asm/arch/clock.h>
>   #include <asm/arch/imx-regs.h>
>   #include <asm/errno.h>
> @@ -72,10 +73,26 @@ static void * const i2c_bases[] = {
>   int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
>   	      struct i2c_pads_info *p)
>   {
> +	char *name1, *name2;
>   	int ret;
>
>   	if (i2c_index >= ARRAY_SIZE(i2c_bases))
>   		return -EINVAL;
> +
> +	name1 = malloc(9);
> +	name2 = malloc(9);
> +	if (!name1 || !name2)
> +		return -ENOMEM;

You have a memory leak here if name1 is allocated but name2 is not.

> +	sprintf(name1, "i2c_sda%d", i2c_index);
> +	sprintf(name2, "i2c_scl%d", i2c_index);
> +	ret = gpio_request(p->sda.gp, name1);
> +	if (ret)
> +		goto err_req1;
> +
> +	ret = gpio_request(p->scl.gp, name2);
> +	if (ret)
> +		goto err_req2;
> +
>   	/* Enable i2c clock */
>   	ret = enable_i2c_clk(1, i2c_index);
>   	if (ret)
> @@ -93,5 +110,12 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
>
>   err_idle:
>   err_clk:
> +	gpio_free(p->scl.gp);
> +err_req2:
> +	gpio_free(p->sda.gp);
> +err_req1:
> +	free(name1);
> +	free(name2);
> +
>   	return ret;
>   }
> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
> index 9c6e686..53681d4 100644
> --- a/board/compulab/cm_fx6/cm_fx6.c
> +++ b/board/compulab/cm_fx6/cm_fx6.c

While this patch addresses the errors I mentioned in V3, I think that
the amount of additional checks this required demonstrates that these
init functions (which can be called multiple times) are not the best
place to do gpio requests.

I'm open to the idea that these requests will be handled by the
respective drivers (where applicable), but until that functionality is
implemented I think it's best to do them in board_init().

I'm attaching 2 patches, which split this patch into two, one for
i2c-mxv7.c, and the other for cm_fx6.c.

-- 
Regards,
Nikita Kiryanov

[U-Boot] [PATCH v4 6/9] dm: imx: Use gpio_request() to request GPIOs

[Simon Glass]

Hi Nikita,

On 2 October 2014 08:18, Nikita Kiryanov <nikita@compulab.co.il> wrote:
> Hi Simon,
>
>
> On 02/10/14 04:57, Simon Glass wrote:
>>
>> GPIOs should be requested before use. Without this, driver model will not
>> permit the GPIO to be used.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v4:
>> - Adjust error checking to permit calling gpio_request() multiple times
>> - Avoid doing low-level SATA init multiple times
>> - Move SATA changes into this patch
>>
>> Changes in v3:
>> - Add a check for the Ethernet gpio_request() also
>> - Add a comment for the CONFIG_SPL_BUILD #ifdef
>> - Just warn when one of the board init stages fails
>>
>> Changes in v2:
>> - Check return values of gpio_request()
>>
>>   arch/arm/imx-common/i2c-mxv7.c | 24 +++++++++++++++++
>>   board/compulab/cm_fx6/cm_fx6.c | 61
>> ++++++++++++++++++++++++++++++++----------
>>   board/compulab/cm_fx6/common.c | 14 +++++++++-
>>   3 files changed, 84 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm/imx-common/i2c-mxv7.c
>> b/arch/arm/imx-common/i2c-mxv7.c
>> index 70cff5c..aaf6936 100644
>> --- a/arch/arm/imx-common/i2c-mxv7.c
>> +++ b/arch/arm/imx-common/i2c-mxv7.c
>> @@ -4,6 +4,7 @@
>>    * SPDX-License-Identifier:   GPL-2.0+
>>    */
>>   #include <common.h>
>> +#include <malloc.h>
>>   #include <asm/arch/clock.h>
>>   #include <asm/arch/imx-regs.h>
>>   #include <asm/errno.h>
>> @@ -72,10 +73,26 @@ static void * const i2c_bases[] = {
>>   int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
>>               struct i2c_pads_info *p)
>>   {
>> +       char *name1, *name2;
>>         int ret;
>>
>>         if (i2c_index >= ARRAY_SIZE(i2c_bases))
>>                 return -EINVAL;
>> +
>> +       name1 = malloc(9);
>> +       name2 = malloc(9);
>> +       if (!name1 || !name2)
>> +               return -ENOMEM;
>
>
> You have a memory leak here if name1 is allocated but name2 is not.

Yes. Actually there is also a memory leak if the devices are removed
(I'm not sure if I mentioned that somewhere, maybe on a Tegra thread).
I'm planning to address both of these as part of the gpio_request()
update, now that I think there are enough GPIO drivers to be
reasonably confident of the best approach.

>
>
>> +       sprintf(name1, "i2c_sda%d", i2c_index);
>> +       sprintf(name2, "i2c_scl%d", i2c_index);
>> +       ret = gpio_request(p->sda.gp, name1);
>> +       if (ret)
>> +               goto err_req1;
>> +
>> +       ret = gpio_request(p->scl.gp, name2);
>> +       if (ret)
>> +               goto err_req2;
>> +
>>         /* Enable i2c clock */
>>         ret = enable_i2c_clk(1, i2c_index);
>>         if (ret)
>> @@ -93,5 +110,12 @@ int setup_i2c(unsigned i2c_index, int speed, int
>> slave_addr,
>>
>>   err_idle:
>>   err_clk:
>> +       gpio_free(p->scl.gp);
>> +err_req2:
>> +       gpio_free(p->sda.gp);
>> +err_req1:
>> +       free(name1);
>> +       free(name2);
>> +
>>         return ret;
>>   }
>> diff --git a/board/compulab/cm_fx6/cm_fx6.c
>> b/board/compulab/cm_fx6/cm_fx6.c
>> index 9c6e686..53681d4 100644
>> --- a/board/compulab/cm_fx6/cm_fx6.c
>> +++ b/board/compulab/cm_fx6/cm_fx6.c
>
>
> While this patch addresses the errors I mentioned in V3, I think that
> the amount of additional checks this required demonstrates that these
> init functions (which can be called multiple times) are not the best
> place to do gpio requests.

Well ignoring the names, there are just the two checks for
gpio_request(), which you are going to need anyway I think.

>
> I'm open to the idea that these requests will be handled by the
> respective drivers (where applicable), but until that functionality is
> implemented I think it's best to do them in board_init().

I'm not convinced that swapping this back to the board, only to swap
it back to the driver is a good plan.

>
> I'm attaching 2 patches, which split this patch into two, one for
> i2c-mxv7.c, and the other for cm_fx6.c.

OK will take a look, thanks.

Regards,
Simon

[U-Boot] [PATCH 2/2] arm: mx6: cm_fx6: use gpio request

[Simon Glass]

Hi Nikita,

On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote:
> Use gpio_request for all the gpios that are utilized by various
> subsystems in cm-fx6, and refactor the relevant init functions
> so that all gpios are requested during board_init(), not during
> subsystem init, thus avoiding the need to manage gpio ownership
> each time a subsystem is initialized.
>
> The new division of labor is:
> During board_init() muxes are setup and gpios are requested.
> During subsystem init gpios are toggled.
>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Simon Glass <sjg@chromium.org>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>

Copying my comments from the other patch:

I've thought about that quite a lot as part of the driver model work.
Claiming GPIOs in board code doesn't feel right to me:

1. If using device tree, the GPIOs are in there, and it means the
board code needs to go looking there as well as the driver. The board
code actually needs to sniff around in the driver's device tree nodes.
That just seems honky.

2. In the driver model world, we hope that board init will fade away
to a large extent. Certainly it should be possible to specify most of
what a driver needs in device tree or platform data. Getting rid of
the explicit init calls in U-Boot (board_init_f(), board_init(),
board_late_init(), board_early_init_f(), ...) is a nice effect of
driver model I hope.

3. Even if not using device tree, and using platform data, where the
board code may specify the platform data, it still feels honky for the
board to be parsing its own data (designed for use by the driver) to
claim GPIOs.

4. I don't really see why pre-claiming enforces anything. If two
subsystems claim the same GPIO you are going to get an error somewhere
in any case.

5. If you look at the calls that drivers make to find out information
from the board file, or perform some init (mmc does this, USB,
ethernet, etc.) it is mostly because the driver has no idea of the
specifics of the board. Device tree and platform data fix exactly this
problem. The driver has the best idea of when it needs to start up,
when it needs this resource of that. In some cases the driver have the
ability to operate in two modes (e.g. 4 bit or 8 bit SDIO, UART
with/without flow control) and this means the init depends on the
driver's mode.

Having said that, it's your board and if you really want to go this
way in the interim, then I'm not going to strongly object.

Regards,
Simon

[U-Boot] [PATCH 1/2] dm: imx: i2c: Use gpio_request() to request GPIOs

[Igor Grinberg]

On 10/02/14 17:17, Nikita Kiryanov wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> GPIOs should be requested before use. Without this, driver model will
> not permit the GPIO to be used.
> 
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>

I like the "small functionality oriented changes" much more.

Acked-by: Igor Grinberg <grinberg@compulab.co.il>

-- 
Regards,
Igor.

[U-Boot] [PATCH 2/2] arm: mx6: cm_fx6: use gpio request

[Igor Grinberg]

Hi Nikita,

On 10/02/14 17:17, Nikita Kiryanov wrote:
> Use gpio_request for all the gpios that are utilized by various
> subsystems in cm-fx6, and refactor the relevant init functions
> so that all gpios are requested during board_init(), not during
> subsystem init, thus avoiding the need to manage gpio ownership
> each time a subsystem is initialized.
> 
> The new division of labor is:
> During board_init() muxes are setup and gpios are requested.
> During subsystem init gpios are toggled.
> 
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Simon Glass <sjg@chromium.org>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>

This looks much better.
I really don't like sticking the "has_been_inited" style of plugs,
while the same can be done by design and w/o such plugs.
Thanks Nikita.

Acked-by Igor Grinberg <grinberg@compulab.co.il>


-- 
Regards,
Igor.

[U-Boot] [PATCH 2/2] arm: mx6: cm_fx6: use gpio request

[Igor Grinberg]

Hi Simon,

On 10/02/14 22:22, Simon Glass wrote:
> Hi Nikita,
> 
> On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote:
>> Use gpio_request for all the gpios that are utilized by various
>> subsystems in cm-fx6, and refactor the relevant init functions
>> so that all gpios are requested during board_init(), not during
>> subsystem init, thus avoiding the need to manage gpio ownership
>> each time a subsystem is initialized.
>>
>> The new division of labor is:
>> During board_init() muxes are setup and gpios are requested.
>> During subsystem init gpios are toggled.
>>
>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>> Cc: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> 
> Copying my comments from the other patch:

Please, don't get me wrong, we have of course read and thought about
this also considering your comments.
The thing is that currently, some of those GPIOs are way too board
specific and may be the right thing would be to leave it that way.
Also some are not board specific, and I will be very glad to pass
them to the drivers.
Yet, I think Nikita's patch is very sane for now.
Once we can pass the GPIOs to the drivers we will of course do this.
We will also be glad to help working on this as we always did (when
the schedule permits us).

> 
> I've thought about that quite a lot as part of the driver model work.
> Claiming GPIOs in board code doesn't feel right to me:
> 
> 1. If using device tree, the GPIOs are in there, and it means the
> board code needs to go looking there as well as the driver. The board
> code actually needs to sniff around in the driver's device tree nodes.
> That just seems honky.

I think this is case dependent. Really we're in the boot loader
world, things here are board specific in many cases.

> 
> 2. In the driver model world, we hope that board init will fade away
> to a large extent. Certainly it should be possible to specify most of
> what a driver needs in device tree or platform data. Getting rid of
> the explicit init calls in U-Boot (board_init_f(), board_init(),
> board_late_init(), board_early_init_f(), ...) is a nice effect of
> driver model I hope.

I don't think it is possible.
There will always be boards that are not by the reference design
and there will have to be stuff done in the board files as it will
not concern any other boards or drivers.

> 
> 3. Even if not using device tree, and using platform data, where the
> board code may specify the platform data, it still feels honky for the
> board to be parsing its own data (designed for use by the driver) to
> claim GPIOs.

Why even? Not using DT is not a bad practice at all!
DT has been designed as an API/ABI to the OS and not for the boot loader!
Boot loaders are board specific, period.
I don't mind using DT in the boot loader for ease and abstraction, but
don't be obsessed with it, because it will only lead to another,
pre-bootloader boot loader which will accommodate all the stuff you
are trying to avoid.

Regarding your feeling honky about parsing data by the board code:
There are so many cases, that I don't think you have considered,
where using DT _instead_ of run time initializations is a total
madness.
Here is one:
Same board, different configuration/revision/extension/variation/etc.
Instead of parsing stuff at runtime and adjusting things according
to detection, you propose an army of DT blobs? This sounds insane.

> 
> 4. I don't really see why pre-claiming enforces anything. If two
> subsystems claim the same GPIO you are going to get an error somewhere
> in any case.

Two subsystems should never own the same GPIO at the same time.
If you follow that rule, there will be errors only in case when there
should errors.

> 
> 5. If you look at the calls that drivers make to find out information
> from the board file, or perform some init (mmc does this, USB,
> ethernet, etc.) it is mostly because the driver has no idea of the
> specifics of the board. Device tree and platform data fix exactly this
> problem. The driver has the best idea of when it needs to start up,
> when it needs this resource of that. In some cases the driver have the
> ability to operate in two modes (e.g. 4 bit or 8 bit SDIO, UART
> with/without flow control) and this means the init depends on the
> driver's mode.

This is correct. No doubt about this.
Yet, generic driver may have prerequisite on a custom board and
don't even know about this prerequisite.

> 
> Having said that, it's your board and if you really want to go this
> way in the interim, then I'm not going to strongly object.

Thanks!
I do really like the idea of DM and I think this should be developed
year ago. Yet, any framework should be flexible enough to give some
place on the stage to the board specific code.

-- 
Regards,
Igor.

[U-Boot] [PATCH 2/2] arm: mx6: cm_fx6: use gpio request

[Simon Glass]

Hi Igor,


On 3 October 2014 01:41, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Simon,
>
> On 10/02/14 22:22, Simon Glass wrote:
>> Hi Nikita,
>>
>> On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote:
>>> Use gpio_request for all the gpios that are utilized by various
>>> subsystems in cm-fx6, and refactor the relevant init functions
>>> so that all gpios are requested during board_init(), not during
>>> subsystem init, thus avoiding the need to manage gpio ownership
>>> each time a subsystem is initialized.
>>>
>>> The new division of labor is:
>>> During board_init() muxes are setup and gpios are requested.
>>> During subsystem init gpios are toggled.
>>>
>>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
>>
>> Copying my comments from the other patch:
>
> Please, don't get me wrong, we have of course read and thought about
> this also considering your comments.
> The thing is that currently, some of those GPIOs are way too board
> specific and may be the right thing would be to leave it that way.
> Also some are not board specific, and I will be very glad to pass
> them to the drivers.
> Yet, I think Nikita's patch is very sane for now.
> Once we can pass the GPIOs to the drivers we will of course do this.
> We will also be glad to help working on this as we always did (when
> the schedule permits us).
>
>>
>> I've thought about that quite a lot as part of the driver model work.
>> Claiming GPIOs in board code doesn't feel right to me:
>>
>> 1. If using device tree, the GPIOs are in there, and it means the
>> board code needs to go looking there as well as the driver. The board
>> code actually needs to sniff around in the driver's device tree nodes.
>> That just seems honky.
>
> I think this is case dependent. Really we're in the boot loader
> world, things here are board specific in many cases.

Yes it is important to retain that flexibility.

>
>>
>> 2. In the driver model world, we hope that board init will fade away
>> to a large extent. Certainly it should be possible to specify most of
>> what a driver needs in device tree or platform data. Getting rid of
>> the explicit init calls in U-Boot (board_init_f(), board_init(),
>> board_late_init(), board_early_init_f(), ...) is a nice effect of
>> driver model I hope.
>
> I don't think it is possible.
> There will always be boards that are not by the reference design
> and there will have to be stuff done in the board files as it will
> not concern any other boards or drivers.

Let's see how this pans out. I feel we can more the pendulum a long
way towards less board-specific init. For example, for MMC we have a
card detect. If we just specify which GPIO it is on, then we don't
need all the callbacks.

>
>>
>> 3. Even if not using device tree, and using platform data, where the
>> board code may specify the platform data, it still feels honky for the
>> board to be parsing its own data (designed for use by the driver) to
>> claim GPIOs.
>
> Why even? Not using DT is not a bad practice at all!
> DT has been designed as an API/ABI to the OS and not for the boot loader!
> Boot loaders are board specific, period.
> I don't mind using DT in the boot loader for ease and abstraction, but
> don't be obsessed with it, because it will only lead to another,
> pre-bootloader boot loader which will accommodate all the stuff you
> are trying to avoid.
>
> Regarding your feeling honky about parsing data by the board code:
> There are so many cases, that I don't think you have considered,
> where using DT _instead_ of run time initializations is a total
> madness.
> Here is one:
> Same board, different configuration/revision/extension/variation/etc.
> Instead of parsing stuff at runtime and adjusting things according
> to detection, you propose an army of DT blobs? This sounds insane.

We are talking here about the code/data trade-off. I feel that U-Boot
currently requires lots of code to be written where with device
tree/platform data it could be data, and the benefit is fewer code
paths and easier integration of new boards. What are these revisions
doing? If they are changing the MMC card detect line then you can have
two different platform data blobs for the board, or two device trees.
It's not mandatory but it certainly works.

If it is easier to check a few GPIOs to find the board ID and then
adjust things at runtime then that works too. That sort of code should
live in the board files though, not the drivers.

>
>>
>> 4. I don't really see why pre-claiming enforces anything. If two
>> subsystems claim the same GPIO you are going to get an error somewhere
>> in any case.
>
> Two subsystems should never own the same GPIO at the same time.
> If you follow that rule, there will be errors only in case when there
> should errors.

Yes. My point was that pre-claiming would still result in an error in this case.

>
>>
>> 5. If you look at the calls that drivers make to find out information
>> from the board file, or perform some init (mmc does this, USB,
>> ethernet, etc.) it is mostly because the driver has no idea of the
>> specifics of the board. Device tree and platform data fix exactly this
>> problem. The driver has the best idea of when it needs to start up,
>> when it needs this resource of that. In some cases the driver have the
>> ability to operate in two modes (e.g. 4 bit or 8 bit SDIO, UART
>> with/without flow control) and this means the init depends on the
>> driver's mode.
>
> This is correct. No doubt about this.
> Yet, generic driver may have prerequisite on a custom board and
> don't even know about this prerequisite.

Or in fact the driver may request that the board be set up a
particular way. This is effectively what happens with MMC - 'please
set up the card-detect line for me to use'.

>
>>
>> Having said that, it's your board and if you really want to go this
>> way in the interim, then I'm not going to strongly object.
>
> Thanks!
> I do really like the idea of DM and I think this should be developed
> year ago. Yet, any framework should be flexible enough to give some
> place on the stage to the board specific code.

I strongly agree with this statement and have been careful so far to
maintain this flexibility in the framework. Let's keep an eye on it.

Regards,
Simon

[U-Boot] [PATCH 2/2] arm: mx6: cm_fx6: use gpio request

[Igor Grinberg]

Hi Simon,

On 10/03/14 17:04, Simon Glass wrote:
> Hi Igor,
> 
> 
> On 3 October 2014 01:41, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> Hi Simon,
>>
>> On 10/02/14 22:22, Simon Glass wrote:
>>> Hi Nikita,
>>>
>>> On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote:
>>>> Use gpio_request for all the gpios that are utilized by various
>>>> subsystems in cm-fx6, and refactor the relevant init functions
>>>> so that all gpios are requested during board_init(), not during
>>>> subsystem init, thus avoiding the need to manage gpio ownership
>>>> each time a subsystem is initialized.
>>>>
>>>> The new division of labor is:
>>>> During board_init() muxes are setup and gpios are requested.
>>>> During subsystem init gpios are toggled.
>>>>
>>>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
>>>
>>> Copying my comments from the other patch:
>>
>> Please, don't get me wrong, we have of course read and thought about
>> this also considering your comments.
>> The thing is that currently, some of those GPIOs are way too board
>> specific and may be the right thing would be to leave it that way.
>> Also some are not board specific, and I will be very glad to pass
>> them to the drivers.
>> Yet, I think Nikita's patch is very sane for now.
>> Once we can pass the GPIOs to the drivers we will of course do this.
>> We will also be glad to help working on this as we always did (when
>> the schedule permits us).
>>
>>>
>>> I've thought about that quite a lot as part of the driver model work.
>>> Claiming GPIOs in board code doesn't feel right to me:
>>>
>>> 1. If using device tree, the GPIOs are in there, and it means the
>>> board code needs to go looking there as well as the driver. The board
>>> code actually needs to sniff around in the driver's device tree nodes.
>>> That just seems honky.
>>
>> I think this is case dependent. Really we're in the boot loader
>> world, things here are board specific in many cases.
> 
> Yes it is important to retain that flexibility.
> 
>>
>>>
>>> 2. In the driver model world, we hope that board init will fade away
>>> to a large extent. Certainly it should be possible to specify most of
>>> what a driver needs in device tree or platform data. Getting rid of
>>> the explicit init calls in U-Boot (board_init_f(), board_init(),
>>> board_late_init(), board_early_init_f(), ...) is a nice effect of
>>> driver model I hope.
>>
>> I don't think it is possible.
>> There will always be boards that are not by the reference design
>> and there will have to be stuff done in the board files as it will
>> not concern any other boards or drivers.
> 
> Let's see how this pans out. I feel we can more the pendulum a long
> way towards less board-specific init. For example, for MMC we have a
> card detect. If we just specify which GPIO it is on, then we don't
> need all the callbacks.

That is totally agreed for cases where it is possible.

> 
>>
>>>
>>> 3. Even if not using device tree, and using platform data, where the
>>> board code may specify the platform data, it still feels honky for the
>>> board to be parsing its own data (designed for use by the driver) to
>>> claim GPIOs.
>>
>> Why even? Not using DT is not a bad practice at all!
>> DT has been designed as an API/ABI to the OS and not for the boot loader!
>> Boot loaders are board specific, period.
>> I don't mind using DT in the boot loader for ease and abstraction, but
>> don't be obsessed with it, because it will only lead to another,
>> pre-bootloader boot loader which will accommodate all the stuff you
>> are trying to avoid.
>>
>> Regarding your feeling honky about parsing data by the board code:
>> There are so many cases, that I don't think you have considered,
>> where using DT _instead_ of run time initializations is a total
>> madness.
>> Here is one:
>> Same board, different configuration/revision/extension/variation/etc.
>> Instead of parsing stuff at runtime and adjusting things according
>> to detection, you propose an army of DT blobs? This sounds insane.
> 
> We are talking here about the code/data trade-off. I feel that U-Boot
> currently requires lots of code to be written where with device
> tree/platform data it could be data, and the benefit is fewer code
> paths and easier integration of new boards. What are these revisions
> doing? If they are changing the MMC card detect line then you can have
> two different platform data blobs for the board, or two device trees.
> It's not mandatory but it certainly works.

Right. Yet, you still need code that will detect the revision
and make the correct choice for platform data/DT blob.
What I'm saying is that in this case, you don't really need several
DT blobs or platform data structs, but only one which can be
adjusted by the same (or not the same) revision detection code.

Sometimes, it can be 3, 4, 5 revisions of the CoM, plus even more of
a base board. You can't really have that much DT blobs in a pocket
to pull out...

> 
> If it is easier to check a few GPIOs to find the board ID and then
> adjust things at runtime then that works too. That sort of code should
> live in the board files though, not the drivers.

So, it merely repeats what I'm saying...

> 
>>
>>>
>>> 4. I don't really see why pre-claiming enforces anything. If two
>>> subsystems claim the same GPIO you are going to get an error somewhere
>>> in any case.
>>
>> Two subsystems should never own the same GPIO at the same time.
>> If you follow that rule, there will be errors only in case when there
>> should errors.
> 
> Yes. My point was that pre-claiming would still result in an error in this case.

It will have the benefit of _not_ playing the request - free game.
IMO, request should be done once (for GPIOs that are designed to be used
in only one subsystem) and then the subsystem can toggle the GPIO as
it likes - w/o the need for requesting once again.

> 
>>
>>>
>>> 5. If you look at the calls that drivers make to find out information
>>> from the board file, or perform some init (mmc does this, USB,
>>> ethernet, etc.) it is mostly because the driver has no idea of the
>>> specifics of the board. Device tree and platform data fix exactly this
>>> problem. The driver has the best idea of when it needs to start up,
>>> when it needs this resource of that. In some cases the driver have the
>>> ability to operate in two modes (e.g. 4 bit or 8 bit SDIO, UART
>>> with/without flow control) and this means the init depends on the
>>> driver's mode.
>>
>> This is correct. No doubt about this.
>> Yet, generic driver may have prerequisite on a custom board and
>> don't even know about this prerequisite.
> 
> Or in fact the driver may request that the board be set up a
> particular way. This is effectively what happens with MMC - 'please
> set up the card-detect line for me to use'.
> 
>>
>>>
>>> Having said that, it's your board and if you really want to go this
>>> way in the interim, then I'm not going to strongly object.
>>
>> Thanks!
>> I do really like the idea of DM and I think this should be developed
>> year ago. Yet, any framework should be flexible enough to give some
>> place on the stage to the board specific code.
> 
> I strongly agree with this statement and have been careful so far to
> maintain this flexibility in the framework. Let's keep an eye on it.
> 
> Regards,
> Simon
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH 2/2] arm: mx6: cm_fx6: use gpio request

[Simon Glass]

Hi Igor,

On 5 October 2014 04:52, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Simon,
>
> On 10/03/14 17:04, Simon Glass wrote:
>> Hi Igor,
>>
>>
>> On 3 October 2014 01:41, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>> Hi Simon,
>>>
>>> On 10/02/14 22:22, Simon Glass wrote:
>>>> Hi Nikita,
>>>>
>>>> On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote:
>>>>> Use gpio_request for all the gpios that are utilized by various
>>>>> subsystems in cm-fx6, and refactor the relevant init functions
>>>>> so that all gpios are requested during board_init(), not during
>>>>> subsystem init, thus avoiding the need to manage gpio ownership
>>>>> each time a subsystem is initialized.
>>>>>
>>>>> The new division of labor is:
>>>>> During board_init() muxes are setup and gpios are requested.
>>>>> During subsystem init gpios are toggled.
>>>>>
>>>>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
>>>>
>>>> Copying my comments from the other patch:
>>>
>>> Please, don't get me wrong, we have of course read and thought about
>>> this also considering your comments.
>>> The thing is that currently, some of those GPIOs are way too board
>>> specific and may be the right thing would be to leave it that way.
>>> Also some are not board specific, and I will be very glad to pass
>>> them to the drivers.
>>> Yet, I think Nikita's patch is very sane for now.
>>> Once we can pass the GPIOs to the drivers we will of course do this.
>>> We will also be glad to help working on this as we always did (when
>>> the schedule permits us).
>>>
>>>>
>>>> I've thought about that quite a lot as part of the driver model work.
>>>> Claiming GPIOs in board code doesn't feel right to me:
>>>>
>>>> 1. If using device tree, the GPIOs are in there, and it means the
>>>> board code needs to go looking there as well as the driver. The board
>>>> code actually needs to sniff around in the driver's device tree nodes.
>>>> That just seems honky.
>>>
>>> I think this is case dependent. Really we're in the boot loader
>>> world, things here are board specific in many cases.
>>
>> Yes it is important to retain that flexibility.
>>
>>>
>>>>
>>>> 2. In the driver model world, we hope that board init will fade away
>>>> to a large extent. Certainly it should be possible to specify most of
>>>> what a driver needs in device tree or platform data. Getting rid of
>>>> the explicit init calls in U-Boot (board_init_f(), board_init(),
>>>> board_late_init(), board_early_init_f(), ...) is a nice effect of
>>>> driver model I hope.
>>>
>>> I don't think it is possible.
>>> There will always be boards that are not by the reference design
>>> and there will have to be stuff done in the board files as it will
>>> not concern any other boards or drivers.
>>
>> Let's see how this pans out. I feel we can more the pendulum a long
>> way towards less board-specific init. For example, for MMC we have a
>> card detect. If we just specify which GPIO it is on, then we don't
>> need all the callbacks.
>
> That is totally agreed for cases where it is possible.

OK good.

>
>>
>>>
>>>>
>>>> 3. Even if not using device tree, and using platform data, where the
>>>> board code may specify the platform data, it still feels honky for the
>>>> board to be parsing its own data (designed for use by the driver) to
>>>> claim GPIOs.
>>>
>>> Why even? Not using DT is not a bad practice at all!
>>> DT has been designed as an API/ABI to the OS and not for the boot loader!
>>> Boot loaders are board specific, period.
>>> I don't mind using DT in the boot loader for ease and abstraction, but
>>> don't be obsessed with it, because it will only lead to another,
>>> pre-bootloader boot loader which will accommodate all the stuff you
>>> are trying to avoid.
>>>
>>> Regarding your feeling honky about parsing data by the board code:
>>> There are so many cases, that I don't think you have considered,
>>> where using DT _instead_ of run time initializations is a total
>>> madness.
>>> Here is one:
>>> Same board, different configuration/revision/extension/variation/etc.
>>> Instead of parsing stuff at runtime and adjusting things according
>>> to detection, you propose an army of DT blobs? This sounds insane.
>>
>> We are talking here about the code/data trade-off. I feel that U-Boot
>> currently requires lots of code to be written where with device
>> tree/platform data it could be data, and the benefit is fewer code
>> paths and easier integration of new boards. What are these revisions
>> doing? If they are changing the MMC card detect line then you can have
>> two different platform data blobs for the board, or two device trees.
>> It's not mandatory but it certainly works.
>
> Right. Yet, you still need code that will detect the revision
> and make the correct choice for platform data/DT blob.
> What I'm saying is that in this case, you don't really need several
> DT blobs or platform data structs, but only one which can be
> adjusted by the same (or not the same) revision detection code.
>
> Sometimes, it can be 3, 4, 5 revisions of the CoM, plus even more of
> a base board. You can't really have that much DT blobs in a pocket
> to pull out...

Sure you can. But if you have board rev GPIOs you can do at least three things:

1. Use them only during production to configure the image to flash (an
image that only works with a single board rev)
2. Use them at run time to select which platform data or device tree info to use
3. Use them at run-time to adjust the platform data or device tree
(even if just to change the 'status' property from "okay" to
"disabled")

I feel all of these have their uses depending on the situation. There
a lots of trade-offs to be had.

For base boards (and maybe even board revs) I wonder if the DT merging
feature might be useful?

>
>>
>> If it is easier to check a few GPIOs to find the board ID and then
>> adjust things at runtime then that works too. That sort of code should
>> live in the board files though, not the drivers.
>
> So, it merely repeats what I'm saying...
>
>>
>>>
>>>>
>>>> 4. I don't really see why pre-claiming enforces anything. If two
>>>> subsystems claim the same GPIO you are going to get an error somewhere
>>>> in any case.
>>>
>>> Two subsystems should never own the same GPIO at the same time.
>>> If you follow that rule, there will be errors only in case when there
>>> should errors.
>>
>> Yes. My point was that pre-claiming would still result in an error in this case.
>
> It will have the benefit of _not_ playing the request - free game.
> IMO, request should be done once (for GPIOs that are designed to be used
> in only one subsystem) and then the subsystem can toggle the GPIO as
> it likes - w/o the need for requesting once again.

OK. In general I prefer the device to claim the GPIOs rather than the
board. With driver model this is pretty easy as there is a formal
'probe' function which will only be called once (until after 'remove'
is called at least). I think request/free is is only a problem because
of the ad-hoc driver approach. But I see that as a general direction
to take, and not a hard and fast rule.

>
>>
>>>
>>>>
>>>> 5. If you look at the calls that drivers make to find out information
>>>> from the board file, or perform some init (mmc does this, USB,
>>>> ethernet, etc.) it is mostly because the driver has no idea of the
>>>> specifics of the board. Device tree and platform data fix exactly this
>>>> problem. The driver has the best idea of when it needs to start up,
>>>> when it needs this resource of that. In some cases the driver have the
>>>> ability to operate in two modes (e.g. 4 bit or 8 bit SDIO, UART
>>>> with/without flow control) and this means the init depends on the
>>>> driver's mode.
>>>
>>> This is correct. No doubt about this.
>>> Yet, generic driver may have prerequisite on a custom board and
>>> don't even know about this prerequisite.
>>
>> Or in fact the driver may request that the board be set up a
>> particular way. This is effectively what happens with MMC - 'please
>> set up the card-detect line for me to use'.
>>
>>>
>>>>
>>>> Having said that, it's your board and if you really want to go this
>>>> way in the interim, then I'm not going to strongly object.
>>>
>>> Thanks!
>>> I do really like the idea of DM and I think this should be developed
>>> year ago. Yet, any framework should be flexible enough to give some
>>> place on the stage to the board specific code.
>>
>> I strongly agree with this statement and have been careful so far to
>> maintain this flexibility in the framework. Let's keep an eye on it.

Regards,
Simon

[U-Boot] [PATCH 2/2] arm: mx6: cm_fx6: use gpio request

[Igor Grinberg]

On 10/05/14 21:32, Simon Glass wrote:
> Hi Igor,
> 
> On 5 October 2014 04:52, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> Hi Simon,
>>
>> On 10/03/14 17:04, Simon Glass wrote:
>>> Hi Igor,
>>>
>>>
>>> On 3 October 2014 01:41, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>> Hi Simon,
>>>>
>>>> On 10/02/14 22:22, Simon Glass wrote:
>>>>> Hi Nikita,
>>>>>
>>>>> On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote:

[...]

>>>>> 3. Even if not using device tree, and using platform data, where the
>>>>> board code may specify the platform data, it still feels honky for the
>>>>> board to be parsing its own data (designed for use by the driver) to
>>>>> claim GPIOs.
>>>>
>>>> Why even? Not using DT is not a bad practice at all!
>>>> DT has been designed as an API/ABI to the OS and not for the boot loader!
>>>> Boot loaders are board specific, period.
>>>> I don't mind using DT in the boot loader for ease and abstraction, but
>>>> don't be obsessed with it, because it will only lead to another,
>>>> pre-bootloader boot loader which will accommodate all the stuff you
>>>> are trying to avoid.
>>>>
>>>> Regarding your feeling honky about parsing data by the board code:
>>>> There are so many cases, that I don't think you have considered,
>>>> where using DT _instead_ of run time initializations is a total
>>>> madness.
>>>> Here is one:
>>>> Same board, different configuration/revision/extension/variation/etc.
>>>> Instead of parsing stuff at runtime and adjusting things according
>>>> to detection, you propose an army of DT blobs? This sounds insane.
>>>
>>> We are talking here about the code/data trade-off. I feel that U-Boot
>>> currently requires lots of code to be written where with device
>>> tree/platform data it could be data, and the benefit is fewer code
>>> paths and easier integration of new boards. What are these revisions
>>> doing? If they are changing the MMC card detect line then you can have
>>> two different platform data blobs for the board, or two device trees.
>>> It's not mandatory but it certainly works.
>>
>> Right. Yet, you still need code that will detect the revision
>> and make the correct choice for platform data/DT blob.
>> What I'm saying is that in this case, you don't really need several
>> DT blobs or platform data structs, but only one which can be
>> adjusted by the same (or not the same) revision detection code.
>>
>> Sometimes, it can be 3, 4, 5 revisions of the CoM, plus even more of
>> a base board. You can't really have that much DT blobs in a pocket
>> to pull out...
> 
> Sure you can. But if you have board rev GPIOs you can do at least three things:
> 
> 1. Use them only during production to configure the image to flash (an
> image that only works with a single board rev)
> 2. Use them at run time to select which platform data or device tree info to use
> 3. Use them at run-time to adjust the platform data or device tree
> (even if just to change the 'status' property from "okay" to
> "disabled")
> 
> I feel all of these have their uses depending on the situation. There
> a lots of trade-offs to be had.

Revision GPIOs is only one case of encoding, there are multiple
possibilities for revision encoding.
Nevertheless, from my experience, only 3rd option is scaling well enough
and sorts out various production/field update by customer/etc. needs.

> 
> For base boards (and maybe even board revs) I wonder if the DT merging
> feature might be useful?

Yes. I think it is useful indeed. There were talks about "cape bus" (or
whatever it was called) in Linux kernel and dynamic DT blobs loading for
extending the one the kernel was booted with. Though I don't know what
the status of this, but it can be very useful for extension boards and
hot plugging. Less for the base boards concept (at least as we use it
now). For the base boards DT merging (if I understand the meaning of
this correctly) can prove useful, but should be handled by U-Boot as
base boards might have a serious impact on the boot process.

> 
>>
>>>
>>> If it is easier to check a few GPIOs to find the board ID and then
>>> adjust things at runtime then that works too. That sort of code should
>>> live in the board files though, not the drivers.
>>
>> So, it merely repeats what I'm saying...
>>
>>>
>>>>
>>>>>
>>>>> 4. I don't really see why pre-claiming enforces anything. If two
>>>>> subsystems claim the same GPIO you are going to get an error somewhere
>>>>> in any case.
>>>>
>>>> Two subsystems should never own the same GPIO at the same time.
>>>> If you follow that rule, there will be errors only in case when there
>>>> should errors.
>>>
>>> Yes. My point was that pre-claiming would still result in an error in this case.
>>
>> It will have the benefit of _not_ playing the request - free game.
>> IMO, request should be done once (for GPIOs that are designed to be used
>> in only one subsystem) and then the subsystem can toggle the GPIO as
>> it likes - w/o the need for requesting once again.
> 
> OK. In general I prefer the device to claim the GPIOs rather than the
> board. With driver model this is pretty easy as there is a formal
> 'probe' function which will only be called once (until after 'remove'
> is called at least). I think request/free is is only a problem because
> of the ad-hoc driver approach. But I see that as a general direction
> to take, and not a hard and fast rule.

No objection on this one.
The objection was only on how to handle the current situation with cm-fx6
CoM. e.g. current driver does not handle the GPIO request stuff.
Once it does, in the same patch, we should remove it from the board(s)
code. It is just like Nikita said, until we have these in the drivers,
we prefer to solve the request problem in a better way (IMO, this is
a better way - to request once instead of request and free each time
or depend on a "inited" variable).


-- 
Regards,
Igor.

[U-Boot] [PATCH 1/2] dm: imx: i2c: Use gpio_request() to request GPIOs

[Simon Glass]

Hi Stefan,

On 3 October 2014 08:39, Igor Grinberg <grinberg@compulab.co.il> wrote:
>
> On 10/02/14 17:17, Nikita Kiryanov wrote:
> > From: Simon Glass <sjg@chromium.org>
> >
> > GPIOs should be requested before use. Without this, driver model will
> > not permit the GPIO to be used.
> >
> > Cc: Igor Grinberg <grinberg@compulab.co.il>
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
>
> I like the "small functionality oriented changes" much more.
>
> Acked-by: Igor Grinberg <grinberg@compulab.co.il>

I plan to pull these two patches from Nikita into u-boot-dm along with
my other IMX and DM patches. It needs to come after SPI and there are
about 8 series in the line. Is that OK with you?

Regards,
Simon

[U-Boot] [PATCH 1/2] dm: imx: i2c: Use gpio_request() to request GPIOs

[Igor Grinberg]

Hi Simon,

I'm sorry for that late reply, I've had email problems during the
conference...
Finally, I have the email working again and going through the stuff...
I guess, it is better late than never..

On 10/14/14 10:25, Simon Glass wrote:
> Hi Stefan,
> 
> On 3 October 2014 08:39, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>
>> On 10/02/14 17:17, Nikita Kiryanov wrote:
>>> From: Simon Glass <sjg@chromium.org>
>>>
>>> GPIOs should be requested before use. Without this, driver model will
>>> not permit the GPIO to be used.
>>>
>>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
>>
>> I like the "small functionality oriented changes" much more.
>>
>> Acked-by: Igor Grinberg <grinberg@compulab.co.il>
> 
> I plan to pull these two patches from Nikita into u-boot-dm along with
> my other IMX and DM patches. It needs to come after SPI and there are
> about 8 series in the line. Is that OK with you?

Yep, this is fine. Thanks!


-- 
Regards,
Igor.

[U-Boot] [PATCH v4 1/9] dm: linker_lists: Add a way to declare multiple objects

[Simon Glass]

On 1 October 2014 19:57, Simon Glass <sjg@chromium.org> wrote:
> The existing ll_entry_declare() permits a single element of the list to
> be added to a linker list. Sometimes we want to add several objects at
> once. To avoid lots of messy declarations, add a macro to support this.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>  include/linker_lists.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>

Applied to u-boot-dm/master.

[U-Boot] [PATCH v4 2/9] dm: core: Allow a list of devices to be declared in one step

[Simon Glass]

On 1 October 2014 19:57, Simon Glass <sjg@chromium.org> wrote:
> The U_BOOT_DEVICE macro allows the declaration of a single U-Boot device.
> Add an equivalent macro to declare an array of devices, for convenience.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>  include/dm/platdata.h | 4 ++++
>  1 file changed, 4 insertions(+)
>

Applied to u-boot-dm/master.

[U-Boot] [PATCH v4 3/9] initcall: Display error number when an error occurs

[Simon Glass]

On 1 October 2014 19:57, Simon Glass <sjg@chromium.org> wrote:
> Now that some initcall functions return a useful error number, display it
> when something goes wrong.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Acked-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
> - Add patch to display error number when an error occurs in initcall
>
>  lib/initcall.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>

Applied to u-boot-dm/master.

[U-Boot] [PATCH v4 4/9] dm: serial: Put common code into separate functions

[Simon Glass]

On 1 October 2014 19:57, Simon Glass <sjg@chromium.org> wrote:
> Avoid duplicating the code which deals with getc() and putc(). It is fairly
> simple, but may expand later.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>  drivers/serial/serial-uclass.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)

Applied to u-boot-dm/master.

[U-Boot] [PATCH v4 5/9] imx: Add error checking to setup_i2c()

[Simon Glass]

On 1 October 2014 19:57, Simon Glass <sjg@chromium.org> wrote:
> Since this function can fail, check its return value.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Tested-by: Nikita Kiryanov <nikita@compulab.co.il>
> ---
>
> Changes in v4:
> - Move SATA changes into the next patch

Applied to u-boot-dm/master.

>
> Changes in v3:
> - Just warn when one of the board init stages fails
>
> Changes in v2:
> - Add new patch to add error checking to setup_i2c()
>
>  arch/arm/imx-common/i2c-mxv7.c            | 24 +++++++++++++++----
>  arch/arm/include/asm/imx-common/mxc_i2c.h |  4 ++--
>  board/compulab/cm_fx6/cm_fx6.c            | 40 ++++++++++++++++++++++++++-----
>  3 files changed, 55 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c
> index a580873..70cff5c 100644
> --- a/arch/arm/imx-common/i2c-mxv7.c
> +++ b/arch/arm/imx-common/i2c-mxv7.c
> @@ -69,15 +69,29 @@ static void * const i2c_bases[] = {
>  };
>
>  /* i2c_index can be from 0 - 2 */
> -void setup_i2c(unsigned i2c_index, int speed, int slave_addr,
> -               struct i2c_pads_info *p)
> +int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
> +             struct i2c_pads_info *p)
>  {
> +       int ret;
> +
>         if (i2c_index >= ARRAY_SIZE(i2c_bases))
> -               return;
> +               return -EINVAL;
>         /* Enable i2c clock */
> -       enable_i2c_clk(1, i2c_index);
> +       ret = enable_i2c_clk(1, i2c_index);
> +       if (ret)
> +               goto err_clk;
> +
>         /* Make sure bus is idle */
> -       force_idle_bus(p);
> +       ret = force_idle_bus(p);
> +       if (ret)
> +               goto err_idle;
> +
>         bus_i2c_init(i2c_bases[i2c_index], speed, slave_addr,
>                         force_idle_bus, p);
> +
> +       return 0;
> +
> +err_idle:
> +err_clk:
> +       return ret;
>  }
> diff --git a/arch/arm/include/asm/imx-common/mxc_i2c.h b/arch/arm/include/asm/imx-common/mxc_i2c.h
> index 182c2f3..af86163 100644
> --- a/arch/arm/include/asm/imx-common/mxc_i2c.h
> +++ b/arch/arm/include/asm/imx-common/mxc_i2c.h
> @@ -52,8 +52,8 @@ struct i2c_pads_info {
>                                         &mx6q_##name : &mx6s_##name
>  #endif
>
> -void setup_i2c(unsigned i2c_index, int speed, int slave_addr,
> -               struct i2c_pads_info *p);
> +int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
> +             struct i2c_pads_info *p);
>  void bus_i2c_init(void *base, int speed, int slave_addr,
>                 int (*idle_bus_fn)(void *p), void *p);
>  int bus_i2c_read(void *base, uchar chip, uint addr, int alen, uchar *buf,
> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
> index fdb8ebf..9c6e686 100644
> --- a/board/compulab/cm_fx6/cm_fx6.c
> +++ b/board/compulab/cm_fx6/cm_fx6.c
> @@ -141,14 +141,36 @@ I2C_PADS(i2c2_pads,
>          IMX_GPIO_NR(1, 6));
>
>
> -static void cm_fx6_setup_i2c(void)
> +static int cm_fx6_setup_one_i2c(int busnum, struct i2c_pads_info *pads)
>  {
> -       setup_i2c(0, CONFIG_SYS_I2C_SPEED, 0x7f, I2C_PADS_INFO(i2c0_pads));
> -       setup_i2c(1, CONFIG_SYS_I2C_SPEED, 0x7f, I2C_PADS_INFO(i2c1_pads));
> -       setup_i2c(2, CONFIG_SYS_I2C_SPEED, 0x7f, I2C_PADS_INFO(i2c2_pads));
> +       int ret;
> +
> +       ret = setup_i2c(busnum, CONFIG_SYS_I2C_SPEED, 0x7f, pads);
> +       if (ret)
> +               printf("Warning: I2C%d setup failed: %d\n", busnum, ret);
> +
> +       return ret;
> +}
> +
> +static int cm_fx6_setup_i2c(void)
> +{
> +       int ret = 0, err;
> +
> +       /* i2c<x>_pads are wierd macro variables; we can't use an array */
> +       err = cm_fx6_setup_one_i2c(0, I2C_PADS_INFO(i2c0_pads));
> +       if (err)
> +               ret = err;
> +       err = cm_fx6_setup_one_i2c(1, I2C_PADS_INFO(i2c1_pads));
> +       if (err)
> +               ret = err;
> +       err = cm_fx6_setup_one_i2c(2, I2C_PADS_INFO(i2c2_pads));
> +       if (err)
> +               ret = err;
> +
> +       return ret;
>  }
>  #else
> -static void cm_fx6_setup_i2c(void) { }
> +static int cm_fx6_setup_i2c(void) { return 0; }
>  #endif
>
>  #ifdef CONFIG_USB_EHCI_MX6
> @@ -409,9 +431,15 @@ void ft_board_setup(void *blob, bd_t *bd)
>
>  int board_init(void)
>  {
> +       int ret;
> +
>         gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
>         cm_fx6_setup_gpmi_nand();
> -       cm_fx6_setup_i2c();
> +
> +       /* Warn on failure but do not abort boot */
> +       ret = cm_fx6_setup_i2c();
> +       if (ret)
> +               printf("Warning: I2C setup failed: %d\n", ret);
>
>         return 0;
>  }
> --
> 2.1.0.rc2.206.gedb03e5
>

[U-Boot] [PATCH v4 7/9] dm: imx: gpio: Support driver model in MXC gpio driver

[Simon Glass]

On 1 October 2014 19:57, Simon Glass <sjg@chromium.org> wrote:
> Add driver model support with this driver. In this case the platform data
> is in the driver. It would be better to put this into an SOC-specific file,
> but this is best attempted when more boards are moved over to use driver
> model.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Acked-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>
> Changes in v4: None
> Changes in v3:
> - Use gpio_is_requested() in one more place
>
> Changes in v2:
> - Add an internal function to check if a GPIO is requested
> - Change 'reserved' to 'requested'
> - Tidy up confusing code that creates names for gpio_request()
>
>  drivers/gpio/mxc_gpio.c | 304 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 303 insertions(+), 1 deletion(-)
>

Applied to u-boot-dm/master.

[U-Boot] [PATCH v4 8/9] dm: imx: serial: Support driver model in the MXC serial driver

[Simon Glass]

On 1 October 2014 19:57, Simon Glass <sjg@chromium.org> wrote:
> Add driver model support with this driver. Boards which use this driver
> should define platform data in their board files.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>  drivers/serial/serial_mxc.c | 170 +++++++++++++++++++++++++++++++++++++-------
>  include/serial_mxc.h        |  14 ++++
>  2 files changed, 159 insertions(+), 25 deletions(-)
>  create mode 100644 include/serial_mxc.h
>

Applied to u-boot-dm/master.

[U-Boot] [PATCH v4 9/9] dm: imx: Move cm_fx6 to use driver model for serial and GPIO

[Simon Glass]

On 1 October 2014 19:57, Simon Glass <sjg@chromium.org> wrote:
> Now that serial and GPIO are available for iMX.6, move cm_fx6 over as an
> example.
>
> Acked-by: Igor Grinberg <grinberg@compulab.co.il>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
> - Use the correct namespace for the platform data
>
>  board/compulab/cm_fx6/cm_fx6.c | 10 ++++++++++
>  include/configs/cm_fx6.h       | 11 +++++++++++
>  2 files changed, 21 insertions(+)
>

Applied to u-boot-dm/master.

[U-Boot] [PATCH 1/2] dm: imx: i2c: Use gpio_request() to request GPIOs

[Simon Glass]

On 21 October 2014 03:51, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Simon,
>
> I'm sorry for that late reply, I've had email problems during the
> conference...
> Finally, I have the email working again and going through the stuff...
> I guess, it is better late than never..
>
> On 10/14/14 10:25, Simon Glass wrote:
>> Hi Stefan,
>>
>> On 3 October 2014 08:39, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>
>>> On 10/02/14 17:17, Nikita Kiryanov wrote:
>>>> From: Simon Glass <sjg@chromium.org>
>>>>
>>>> GPIOs should be requested before use. Without this, driver model will
>>>> not permit the GPIO to be used.
>>>>
>>>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
>>>
>>> I like the "small functionality oriented changes" much more.
>>>
>>> Acked-by: Igor Grinberg <grinberg@compulab.co.il>
>>
>> I plan to pull these two patches from Nikita into u-boot-dm along with
>> my other IMX and DM patches. It needs to come after SPI and there are
>> about 8 series in the line. Is that OK with you?
>
> Yep, this is fine. Thanks!

Applied to u-boot-dm/master, thanks!

[U-Boot] [PATCH 2/2] arm: mx6: cm_fx6: use gpio request

[Simon Glass]

On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote:
> Use gpio_request for all the gpios that are utilized by various
> subsystems in cm-fx6, and refactor the relevant init functions
> so that all gpios are requested during board_init(), not during
> subsystem init, thus avoiding the need to manage gpio ownership
> each time a subsystem is initialized.
>
> The new division of labor is:
> During board_init() muxes are setup and gpios are requested.
> During subsystem init gpios are toggled.
>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Simon Glass <sjg@chromium.org>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> ---
>  board/compulab/cm_fx6/cm_fx6.c | 133 +++++++++++++++++++++++++++++------------
>  1 file changed, 94 insertions(+), 39 deletions(-)
>

Applied to u-boot-dm/master, thanks!