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

[U-Boot] [PATCH 0/10] 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.


Simon Glass (10):
  dm: linker_lists: Add a way to declare multiple objects
  dm: core: Allow a list of devices to be declared in one step
  dm: core: Allow device_bind() to used without CONFIG_OF_CONTROL
  dm: serial: Don't require device tree to configure a console
  dm: serial: Put common code into separate functions
  dm: imx: Use gpio_request() to request GPIOs
  imximage.cfg: Remove copyright header
  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     |  14 ++
 board/compulab/cm_fx6/cm_fx6.c     |  19 +++
 board/compulab/cm_fx6/common.c     |   3 +
 board/compulab/cm_fx6/imximage.cfg |   6 -
 drivers/core/device.c              |   7 +-
 drivers/gpio/mxc_gpio.c            | 291 ++++++++++++++++++++++++++++++++++++-
 drivers/serial/serial-uclass.c     |  35 +++--
 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 ++
 12 files changed, 545 insertions(+), 50 deletions(-)
 create mode 100644 include/serial_mxc.h

-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH 01/10] 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>
---

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

diff --git a/include/linker_lists.h b/include/linker_lists.h
index 557e627..32cc9fc 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 02/10] 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>
---

 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 03/10] dm: core: Allow device_bind() to used without CONFIG_OF_CONTROL

[Simon Glass]

The sequence number support in driver model requires device tree control.
It should be skipped if CONFIG_OF_CONTROL is not defined, and should not
require functions from fdtdec.

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

 drivers/core/device.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/core/device.c b/drivers/core/device.c
index 166b073..ef41a9b 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -106,13 +106,16 @@ int device_bind(struct udevice *parent, struct driver *drv, const char *name,
 	 * a 'requested' sequence, and will be resolved (and ->seq updated)
 	 * when the device is probed.
 	 */
-	dev->req_seq = fdtdec_get_int(gd->fdt_blob, of_offset, "reg", -1);
 	dev->seq = -1;
+#ifdef CONFIG_OF_CONTROL
+	dev->req_seq = fdtdec_get_int(gd->fdt_blob, of_offset, "reg", -1);
 	if (uc->uc_drv->name && of_offset != -1) {
 		fdtdec_get_alias_seq(gd->fdt_blob, uc->uc_drv->name, of_offset,
 				     &dev->req_seq);
 	}
-
+#else
+	dev->req_seq = -1;
+#endif
 	if (!dev->platdata && drv->platdata_auto_alloc_size)
 		dev->flags |= DM_FLAG_ALLOC_PDATA;
 
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH 04/10] dm: serial: Don't require device tree to configure a console

[Simon Glass]

Allow serial_find_console_or_panic() to work without a device tree.

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

 drivers/serial/serial-uclass.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index d04104e..1ac943f 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -25,6 +25,7 @@ struct udevice *cur_dev __attribute__ ((section(".data")));
 
 static void serial_find_console_or_panic(void)
 {
+#ifdef CONFIG_OF_CONTROL
 	int node;
 
 	/* Check for a chosen console */
@@ -44,7 +45,7 @@ static void serial_find_console_or_panic(void)
 			return;
 		cur_dev = NULL;
 	}
-
+#endif
 	/*
 	 * Failing that, get the device with sequence number 0, or in extremis
 	 * just the first serial device we can find. But we insist on having
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH 05/10] 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>
---

 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 06/10] 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>
---

 arch/arm/imx-common/i2c-mxv7.c | 14 ++++++++++++++
 board/compulab/cm_fx6/cm_fx6.c |  9 +++++++++
 board/compulab/cm_fx6/common.c |  3 +++
 3 files changed, 26 insertions(+)

diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c
index a580873..7cea024 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,8 +73,21 @@ static void * const i2c_bases[] = {
 void setup_i2c(unsigned i2c_index, int speed, int slave_addr,
 		struct i2c_pads_info *p)
 {
+	char *name;
+
 	if (i2c_index >= ARRAY_SIZE(i2c_bases))
 		return;
+
+	name = malloc(9);
+	assert(name);
+	sprintf(name, "i2c_sda%d", i2c_index);
+	gpio_request(p->sda.gp, name);
+
+	name = malloc(9);
+	assert(name);
+	sprintf(name, "i2c_scl%d", i2c_index);
+	gpio_request(p->scl.gp, name);
+
 	/* Enable i2c clock */
 	enable_i2c_clk(1, i2c_index);
 	/* Make sure bus is idle */
diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
index fdb8ebf..80a123d 100644
--- a/board/compulab/cm_fx6/cm_fx6.c
+++ b/board/compulab/cm_fx6/cm_fx6.c
@@ -71,8 +71,15 @@ static iomux_v3_cfg_t const sata_pads[] = {
 
 static void cm_fx6_setup_issd(void)
 {
+	int i;
+
 	SETUP_IOMUX_PADS(sata_pads);
+
+	for (i = 0; i < ARRAY_SIZE(cm_fx6_issd_gpios); i++)
+		gpio_request(cm_fx6_issd_gpios[i], "sata");
+
 	/* Make sure this gpio has logical 0 value */
+	gpio_request(CM_FX6_SATA_PWLOSS_INT, "sata_pwloss_int");
 	gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0);
 	udelay(100);
 
@@ -167,6 +174,7 @@ static int cm_fx6_usb_hub_reset(void)
 	}
 
 	SETUP_IOMUX_PAD(PAD_SD3_RST__GPIO7_IO08 | MUX_PAD_CTRL(NO_PAD_CTRL));
+	gpio_request(CM_FX6_USB_HUB_RST, "usb_hub_rst");
 	gpio_direction_output(CM_FX6_USB_HUB_RST, 0);
 	udelay(10);
 	gpio_direction_output(CM_FX6_USB_HUB_RST, 1);
@@ -324,6 +332,7 @@ int board_eth_init(bd_t *bis)
 
 	SETUP_IOMUX_PADS(enet_pads);
 	/* phy reset */
+	gpio_request(CM_FX6_ENET_NRST, "enet_nrst");
 	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..562313b 100644
--- a/board/compulab/cm_fx6/common.c
+++ b/board/compulab/cm_fx6/common.c
@@ -79,6 +79,9 @@ void cm_fx6_set_ecspi_iomux(void)
 
 int board_spi_cs_gpio(unsigned bus, unsigned cs)
 {
+#ifndef CONFIG_SPL_BUILD
+	gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0");
+#endif
 	return (bus == 0 && cs == 0) ? (CM_FX6_ECSPI_BUS0_CS0) : -1;
 }
 #endif
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH 07/10] imximage.cfg: Remove copyright header

[Simon Glass]

This seems to break mkimage:

Invalid imximage commands Type - valid names are: BOOT_FROM, BOOT_OFFSET, DATA, CSF, IMAGE_VERSION
Error: board/compulab/cm_fx6/imximage.cfg[1] - Invalid command(/*)

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

 board/compulab/cm_fx6/imximage.cfg | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/board/compulab/cm_fx6/imximage.cfg b/board/compulab/cm_fx6/imximage.cfg
index 420947e..95e27be 100644
--- a/board/compulab/cm_fx6/imximage.cfg
+++ b/board/compulab/cm_fx6/imximage.cfg
@@ -1,8 +1,2 @@
-/*
- * Copyright (C) 2014, Compulab Ltd - http://compulab.co.il/
- *
- * SPDX-License-Identifier:	GPL-2.0+
- */
-
 IMAGE_VERSION 2
 BOOT_FROM	sd
-- 
2.1.0.rc2.206.gedb03e5

[U-Boot] [PATCH 08/10] 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>
---

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

diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
index 6a572d5..8669cf0 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,277 @@ int gpio_direction_output(unsigned gpio, int value)
 
 	return mxc_gpio_direction(gpio, MXC_GPIO_DIRECTION_OUT);
 }
+#endif
+
+#ifdef CONFIG_DM_GPIO
+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_reserved(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 (!*bank->label[offset]) {
+		printf("mxc_gpio: %s: error: gpio %s%d not reserved\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_reserved(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_reserved(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_reserved(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_reserved(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 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;
+	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),
+		 *label ? 'x' : ' ',
+		 *label ? " " : "",
+		 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 (*bank->label[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_reserved(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 (!*bank->label[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 = "";
+
+	bank->regs = plat->regs;
+	uc_priv->gpio_count = 32;
+	name = malloc(8);
+	if (!name)
+		return -ENOMEM;
+	banknum = plat - mxc_plat;
+	if (banknum < 98)
+		sprintf(name, "GPIO%d_", banknum + 1);
+	uc_priv->bank_name = name;
+
+	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 09/10] 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>
---

 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 10/10] 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.

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

 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 80a123d..41222cb 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>
@@ -490,3 +492,11 @@ u32 get_board_rev(void)
 	return cl_eeprom_get_board_rev();
 }
 
+struct mxc_serial_platdata serial_mxc_plat = {
+	.reg = (struct mxc_uart *)UART4_BASE,
+};
+
+U_BOOT_DEVICE(mxc_serial) = {
+	.name	= "serial_mxc",
+	.platdata = &serial_mxc_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 06/10] dm: imx: Use gpio_request() to request GPIOs

[Igor Grinberg]

Hi Simon,

On 09/15/14 15:57, Simon Glass wrote:
> GPIOs should be requested before use. Without this, driver model will not
> permit the GPIO to be used.

Right. That should have been done from the start... Sorry for that...
A question below though..

> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  arch/arm/imx-common/i2c-mxv7.c | 14 ++++++++++++++
>  board/compulab/cm_fx6/cm_fx6.c |  9 +++++++++
>  board/compulab/cm_fx6/common.c |  3 +++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c
> index a580873..7cea024 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,8 +73,21 @@ static void * const i2c_bases[] = {
>  void setup_i2c(unsigned i2c_index, int speed, int slave_addr,
>  		struct i2c_pads_info *p)
>  {
> +	char *name;
> +
>  	if (i2c_index >= ARRAY_SIZE(i2c_bases))
>  		return;
> +
> +	name = malloc(9);
> +	assert(name);
> +	sprintf(name, "i2c_sda%d", i2c_index);
> +	gpio_request(p->sda.gp, name);
> +
> +	name = malloc(9);
> +	assert(name);
> +	sprintf(name, "i2c_scl%d", i2c_index);
> +	gpio_request(p->scl.gp, name);
> +
>  	/* Enable i2c clock */
>  	enable_i2c_clk(1, i2c_index);
>  	/* Make sure bus is idle */
> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
> index fdb8ebf..80a123d 100644
> --- a/board/compulab/cm_fx6/cm_fx6.c
> +++ b/board/compulab/cm_fx6/cm_fx6.c
> @@ -71,8 +71,15 @@ static iomux_v3_cfg_t const sata_pads[] = {
>  
>  static void cm_fx6_setup_issd(void)
>  {
> +	int i;
> +
>  	SETUP_IOMUX_PADS(sata_pads);
> +
> +	for (i = 0; i < ARRAY_SIZE(cm_fx6_issd_gpios); i++)
> +		gpio_request(cm_fx6_issd_gpios[i], "sata");
> +
>  	/* Make sure this gpio has logical 0 value */
> +	gpio_request(CM_FX6_SATA_PWLOSS_INT, "sata_pwloss_int");
>  	gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0);
>  	udelay(100);
>  
> @@ -167,6 +174,7 @@ static int cm_fx6_usb_hub_reset(void)
>  	}
>  
>  	SETUP_IOMUX_PAD(PAD_SD3_RST__GPIO7_IO08 | MUX_PAD_CTRL(NO_PAD_CTRL));
> +	gpio_request(CM_FX6_USB_HUB_RST, "usb_hub_rst");
>  	gpio_direction_output(CM_FX6_USB_HUB_RST, 0);
>  	udelay(10);
>  	gpio_direction_output(CM_FX6_USB_HUB_RST, 1);
> @@ -324,6 +332,7 @@ int board_eth_init(bd_t *bis)
>  
>  	SETUP_IOMUX_PADS(enet_pads);
>  	/* phy reset */
> +	gpio_request(CM_FX6_ENET_NRST, "enet_nrst");
>  	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..562313b 100644
> --- a/board/compulab/cm_fx6/common.c
> +++ b/board/compulab/cm_fx6/common.c
> @@ -79,6 +79,9 @@ void cm_fx6_set_ecspi_iomux(void)
>  
>  int board_spi_cs_gpio(unsigned bus, unsigned cs)
>  {
> +#ifndef CONFIG_SPL_BUILD
> +	gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0");
> +#endif
>  	return (bus == 0 && cs == 0) ? (CM_FX6_ECSPI_BUS0_CS0) : -1;
>  }
>  #endif
> 

In all the above gpio_request() calls, I think we should check for
the return value.
Because after patch 8 in the series it can fail if if someone
double requests the same gpio.

-- 
Regards,
Igor.

[U-Boot] [PATCH 07/10] imximage.cfg: Remove copyright header

[Igor Grinberg]

On 09/15/14 15:57, Simon Glass wrote:
> This seems to break mkimage:
> 
> Invalid imximage commands Type - valid names are: BOOT_FROM, BOOT_OFFSET, DATA, CSF, IMAGE_VERSION
> Error: board/compulab/cm_fx6/imximage.cfg[1] - Invalid command(/*)

That is really strange, because there are multiple imximage.cfg
files with headers like this and I don't remember any complaints.

Also, I've checked right now on v2014.10-rc2 + Nikita's patches on top
and no problem has occurred.

What are you basing at?

> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  board/compulab/cm_fx6/imximage.cfg | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/board/compulab/cm_fx6/imximage.cfg b/board/compulab/cm_fx6/imximage.cfg
> index 420947e..95e27be 100644
> --- a/board/compulab/cm_fx6/imximage.cfg
> +++ b/board/compulab/cm_fx6/imximage.cfg
> @@ -1,8 +1,2 @@
> -/*
> - * Copyright (C) 2014, Compulab Ltd - http://compulab.co.il/
> - *
> - * SPDX-License-Identifier:	GPL-2.0+
> - */
> -
>  IMAGE_VERSION 2
>  BOOT_FROM	sd
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH 06/10] dm: imx: Use gpio_request() to request GPIOs

[Simon Glass]

Hi Igor,

On 15 September 2014 11:13, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Simon,
>
> On 09/15/14 15:57, Simon Glass wrote:
>> GPIOs should be requested before use. Without this, driver model will not
>> permit the GPIO to be used.
>
> Right. That should have been done from the start... Sorry for that...
> A question below though..
>
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  arch/arm/imx-common/i2c-mxv7.c | 14 ++++++++++++++
>>  board/compulab/cm_fx6/cm_fx6.c |  9 +++++++++
>>  board/compulab/cm_fx6/common.c |  3 +++
>>  3 files changed, 26 insertions(+)
>>
>> diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c
>> index a580873..7cea024 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,8 +73,21 @@ static void * const i2c_bases[] = {
>>  void setup_i2c(unsigned i2c_index, int speed, int slave_addr,
>>               struct i2c_pads_info *p)
>>  {
>> +     char *name;
>> +
>>       if (i2c_index >= ARRAY_SIZE(i2c_bases))
>>               return;
>> +
>> +     name = malloc(9);
>> +     assert(name);
>> +     sprintf(name, "i2c_sda%d", i2c_index);
>> +     gpio_request(p->sda.gp, name);
>> +
>> +     name = malloc(9);
>> +     assert(name);
>> +     sprintf(name, "i2c_scl%d", i2c_index);
>> +     gpio_request(p->scl.gp, name);
>> +
>>       /* Enable i2c clock */
>>       enable_i2c_clk(1, i2c_index);
>>       /* Make sure bus is idle */
>> diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
>> index fdb8ebf..80a123d 100644
>> --- a/board/compulab/cm_fx6/cm_fx6.c
>> +++ b/board/compulab/cm_fx6/cm_fx6.c
>> @@ -71,8 +71,15 @@ static iomux_v3_cfg_t const sata_pads[] = {
>>
>>  static void cm_fx6_setup_issd(void)
>>  {
>> +     int i;
>> +
>>       SETUP_IOMUX_PADS(sata_pads);
>> +
>> +     for (i = 0; i < ARRAY_SIZE(cm_fx6_issd_gpios); i++)
>> +             gpio_request(cm_fx6_issd_gpios[i], "sata");
>> +
>>       /* Make sure this gpio has logical 0 value */
>> +     gpio_request(CM_FX6_SATA_PWLOSS_INT, "sata_pwloss_int");
>>       gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0);
>>       udelay(100);
>>
>> @@ -167,6 +174,7 @@ static int cm_fx6_usb_hub_reset(void)
>>       }
>>
>>       SETUP_IOMUX_PAD(PAD_SD3_RST__GPIO7_IO08 | MUX_PAD_CTRL(NO_PAD_CTRL));
>> +     gpio_request(CM_FX6_USB_HUB_RST, "usb_hub_rst");
>>       gpio_direction_output(CM_FX6_USB_HUB_RST, 0);
>>       udelay(10);
>>       gpio_direction_output(CM_FX6_USB_HUB_RST, 1);
>> @@ -324,6 +332,7 @@ int board_eth_init(bd_t *bis)
>>
>>       SETUP_IOMUX_PADS(enet_pads);
>>       /* phy reset */
>> +     gpio_request(CM_FX6_ENET_NRST, "enet_nrst");
>>       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..562313b 100644
>> --- a/board/compulab/cm_fx6/common.c
>> +++ b/board/compulab/cm_fx6/common.c
>> @@ -79,6 +79,9 @@ void cm_fx6_set_ecspi_iomux(void)
>>
>>  int board_spi_cs_gpio(unsigned bus, unsigned cs)
>>  {
>> +#ifndef CONFIG_SPL_BUILD
>> +     gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0");
>> +#endif
>>       return (bus == 0 && cs == 0) ? (CM_FX6_ECSPI_BUS0_CS0) : -1;
>>  }
>>  #endif
>>
>
> In all the above gpio_request() calls, I think we should check for
> the return value.
> Because after patch 8 in the series it can fail if if someone
> double requests the same gpio.

That's true, although for a particular board you presumably know what
you are doing. The problem happens more when we move this sort of
thing to drivers, and there is a conflict.

In some cases there is not way to report an error (void functions),
and in others it needs additional plumbing. But I agree we should
start to fix this sort of thing.

Regards,
Simon

[U-Boot] [PATCH 08/10] dm: imx: gpio: Support driver model in MXC gpio driver

[Igor Grinberg]

Hi Simon,

On 09/15/14 15:57, Simon Glass 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>
> ---
> 
>  drivers/gpio/mxc_gpio.c | 291 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 290 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
> index 6a572d5..8669cf0 100644
> --- a/drivers/gpio/mxc_gpio.c
> +++ b/drivers/gpio/mxc_gpio.c

[...]

> +static int check_reserved(struct udevice *dev, unsigned offset,
> +			  const char *func)

Wouldn't "check_requested" be a better name here?
And then also in the error message below?

> +{
> +	struct mxc_bank_info *bank = dev_get_priv(dev);
> +	struct gpio_dev_priv *uc_priv = dev->uclass_priv;
> +
> +	if (!*bank->label[offset]) {

Can we have here a more explicit (and descriptive) check for '\0'?

> +		printf("mxc_gpio: %s: error: gpio %s%d not reserved\n",
> +		       func, uc_priv->bank_name, offset);
> +		return -EPERM;
> +	}
> +
> +	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 (*bank->label[offset])

Same here?

> +		return -EBUSY;
> +
> +	strncpy(bank->label[offset], label, GPIO_NAME_SIZE);
> +	bank->label[offset][GPIO_NAME_SIZE - 1] = '\0';
> +
> +	return 0;
> +}

[...]

> +static int mxc_gpio_get_function(struct udevice *dev, unsigned offset)
> +{
> +	struct mxc_bank_info *bank = dev_get_priv(dev);
> +
> +	if (!*bank->label[offset])

and here.

> +		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 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 = "";

I think you can skip this initialization,
malloc will write into this pointer anyway.

> +
> +	bank->regs = plat->regs;
> +	uc_priv->gpio_count = 32;

Isn't this GPIO_PER_BANK is defined for?

> +	name = malloc(8);
> +	if (!name)
> +		return -ENOMEM;
> +	banknum = plat - mxc_plat;
> +	if (banknum < 98)
> +		sprintf(name, "GPIO%d_", banknum + 1);

The logic here (and the magic 98) is unclear.
Can we have a bit more explanation here please?

> +	uc_priv->bank_name = name;
> +
> +	return 0;
> +}

[...]


-- 
Regards,
Igor.

[U-Boot] [PATCH 06/10] dm: imx: Use gpio_request() to request GPIOs

[Igor Grinberg]

On 09/15/14 21:04, Simon Glass wrote:
> Hi Igor,
> 
> On 15 September 2014 11:13, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> Hi Simon,
>>
>> On 09/15/14 15:57, Simon Glass wrote:
>>> GPIOs should be requested before use. Without this, driver model will not
>>> permit the GPIO to be used.

[...]

>>
>> In all the above gpio_request() calls, I think we should check for
>> the return value.
>> Because after patch 8 in the series it can fail if if someone
>> double requests the same gpio.
> 
> That's true, although for a particular board you presumably know what
> you are doing. The problem happens more when we move this sort of
> thing to drivers, and there is a conflict.
> 
> In some cases there is not way to report an error (void functions),
> and in others it needs additional plumbing. But I agree we should
> start to fix this sort of thing.

Ok. So for the board code, which is the current user of
gpio_request() function, I would at least add an error message and
stop trying to access the "failed to request" gpio.

-- 
Regards,
Igor.

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

[Igor Grinberg]

On 09/15/14 15:57, Simon Glass wrote:
> Now that serial and GPIO are available for iMX.6, move cm_fx6 over as an
> example.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  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 80a123d..41222cb 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>
> @@ -490,3 +492,11 @@ u32 get_board_rev(void)
>  	return cl_eeprom_get_board_rev();
>  }
>  
> +struct mxc_serial_platdata serial_mxc_plat = {

This isn't referenced outside, right?
If so, should it be static?

Also, can we please have it in the same name space like
all (well.. almost all) other structures/functions names
e.g. cm_fx6_mxc_serial_plat ?

> +	.reg = (struct mxc_uart *)UART4_BASE,
> +};
> +
> +U_BOOT_DEVICE(mxc_serial) = {
> +	.name	= "serial_mxc",
> +	.platdata = &serial_mxc_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
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH 07/10] imximage.cfg: Remove copyright header

[Simon Glass]

Hi Igor,

On 15 September 2014 12:00, Igor Grinberg <grinberg@compulab.co.il> wrote:

> On 09/15/14 15:57, Simon Glass wrote:
> > This seems to break mkimage:
> >
> > Invalid imximage commands Type - valid names are: BOOT_FROM,
> BOOT_OFFSET, DATA, CSF, IMAGE_VERSION
> > Error: board/compulab/cm_fx6/imximage.cfg[1] - Invalid command(/*)
>
> That is really strange, because there are multiple imximage.cfg
> files with headers like this and I don't remember any complaints.
>
> Also, I've checked right now on v2014.10-rc2 + Nikita's patches on top
> and no problem has occurred.
>
> What are you basing at?
>

I am using master at u-boot-imx. I'll drop this patch for v2, but I need it
locally for now.


>
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  board/compulab/cm_fx6/imximage.cfg | 6 ------
> >  1 file changed, 6 deletions(-)
> >
> > diff --git a/board/compulab/cm_fx6/imximage.cfg
> b/board/compulab/cm_fx6/imximage.cfg
> > index 420947e..95e27be 100644
> > --- a/board/compulab/cm_fx6/imximage.cfg
> > +++ b/board/compulab/cm_fx6/imximage.cfg
> > @@ -1,8 +1,2 @@
> > -/*
> > - * Copyright (C) 2014, Compulab Ltd - http://compulab.co.il/
> > - *
> > - * SPDX-License-Identifier:  GPL-2.0+
> > - */
> > -
> >  IMAGE_VERSION 2
> >  BOOT_FROM    sd
> >
>
> --
> Regards,
> Igor.
>

Regards,
Simon

[U-Boot] [PATCH 08/10] dm: imx: gpio: Support driver model in MXC gpio driver

[Simon Glass]

Hi Igor,

On 15 September 2014 12:32, Igor Grinberg <grinberg@compulab.co.il> wrote:

> Hi Simon,
>
> On 09/15/14 15:57, Simon Glass 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>
> > ---
> >
> >  drivers/gpio/mxc_gpio.c | 291
> +++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 290 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
> > index 6a572d5..8669cf0 100644
> > --- a/drivers/gpio/mxc_gpio.c
> > +++ b/drivers/gpio/mxc_gpio.c
>
> [...]
>
> > +static int check_reserved(struct udevice *dev, unsigned offset,
> > +                       const char *func)
>
> Wouldn't "check_requested" be a better name here?
> And then also in the error message below?
>

The problem is that we then get into 'is_request' which is really not quite
as good a name as 'is_reserved'. Still, I'll change it to make things
consistent.

>
> > +{
> > +     struct mxc_bank_info *bank = dev_get_priv(dev);
> > +     struct gpio_dev_priv *uc_priv = dev->uclass_priv;
> > +
> > +     if (!*bank->label[offset]) {
>
> Can we have here a more explicit (and descriptive) check for '\0'?
>

I'll add a function.

>
> > +             printf("mxc_gpio: %s: error: gpio %s%d not reserved\n",
> > +                    func, uc_priv->bank_name, offset);
> > +             return -EPERM;
> > +     }
> > +
> > +     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 (*bank->label[offset])
>
> Same here?
>
> > +             return -EBUSY;
> > +
> > +     strncpy(bank->label[offset], label, GPIO_NAME_SIZE);
> > +     bank->label[offset][GPIO_NAME_SIZE - 1] = '\0';
> > +
> > +     return 0;
> > +}
>
> [...]
>
> > +static int mxc_gpio_get_function(struct udevice *dev, unsigned offset)
> > +{
> > +     struct mxc_bank_info *bank = dev_get_priv(dev);
> > +
> > +     if (!*bank->label[offset])
>
> and here.
>
> > +             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 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 = "";
>
> I think you can skip this initialization,
> malloc will write into this pointer anyway.
>

OK


>
> > +
> > +     bank->regs = plat->regs;
> > +     uc_priv->gpio_count = 32;
>
> Isn't this GPIO_PER_BANK is defined for?
>

OK

>
> > +     name = malloc(8);
> > +     if (!name)
> > +             return -ENOMEM;
> > +     banknum = plat - mxc_plat;
> > +     if (banknum < 98)
> > +             sprintf(name, "GPIO%d_", banknum + 1);
>
> The logic here (and the magic 98) is unclear.
> Can we have a bit more explanation here please?
>

It's icky, trying to avoid a string overflow. I'll rewrite it.


>
> > +     uc_priv->bank_name = name;
> > +
> > +     return 0;
> > +}
>
> [...]
>
>
> --
> Regards,
> Igor.
>

Regards,
Simon

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

[Simon Glass]

Hi Igor,

On 15 September 2014 12:50, Igor Grinberg <grinberg@compulab.co.il> wrote:
> On 09/15/14 15:57, Simon Glass wrote:
>> Now that serial and GPIO are available for iMX.6, move cm_fx6 over as an
>> example.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  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 80a123d..41222cb 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>
>> @@ -490,3 +492,11 @@ u32 get_board_rev(void)
>>       return cl_eeprom_get_board_rev();
>>  }
>>
>> +struct mxc_serial_platdata serial_mxc_plat = {
>
> This isn't referenced outside, right?
> If so, should it be static?

OK

>
> Also, can we please have it in the same name space like
> all (well.. almost all) other structures/functions names
> e.g. cm_fx6_mxc_serial_plat ?

OK

Regards,
Simon