[U-Boot] [PATCH v3 0/4] dm:gpio:mxc add DT support

[U-Boot] [PATCH v3 0/4] dm:gpio:mxc add DT support

[Peng Fan]

This patch set is to add DT support for mxc_gpio driver.

patch 1/4 and 2/4, a new dev_get_addr interface is abstracted to
		   improve driver who want to get device address.
patch 3/4, add a new bank_index entry in platdata to avoid `plat - mxc_plat`
	   pointer subtract usage.
patch 4/4, add compatible ids and implement bind function. Also commented
	   out U_BOOT_DEVICES and mxc_plat, since they are not needed
	   if using DT.


Changes v3:
 1. split bank_index patch
 2. abstract dev_get_addr for driver

Changes v2:
 1. remove uneccessary #ifdef
 2. add more stuff in commit log
 3. include a new function mxc_get_gpio_addr to get register base.
    This function is different for DT and not DT, by `#ifdef`.
    If using one implementation for DT and not DT, final image will be big.
 4. include a new entry in platdata, named bank_index. it can simplify DT
    support. To no DT, bank_index is static initilized; to DT, bank_index
    is get from device's req_seq.

Peng Fan (4):
  dm: introduce dev_get_addr interface
  dm: add dev_get_addr prototype
  dm:gpio:mxc add a bank_index entry in platdata
  dm:gpio:mxc add DT support

 drivers/core/device.c   | 19 +++++++++++++
 drivers/gpio/mxc_gpio.c | 72 ++++++++++++++++++++++++++++++++++++-------------
 include/dm/device.h     |  9 +++++++
 3 files changed, 82 insertions(+), 18 deletions(-)

-- 
1.8.4

[U-Boot] [PATCH v3 1/4] dm: introduce dev_get_addr interface

[Peng Fan]

Abstracting dev_get_addr can improve drivers that want to
get device's address.

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
---
 drivers/core/device.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/core/device.c b/drivers/core/device.c
index 963b16f..0ba5c76 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -12,6 +12,7 @@
 #include <common.h>
 #include <fdtdec.h>
 #include <malloc.h>
+#include <libfdt.h>
 #include <dm/device.h>
 #include <dm/device-internal.h>
 #include <dm/lists.h>
@@ -390,3 +391,21 @@ ulong dev_get_of_data(struct udevice *dev)
 {
 	return dev->of_id->data;
 }
+
+#ifdef CONFIG_OF_CONTROL
+void *dev_get_addr(struct udevice *dev)
+{
+	fdt_addr_t addr;
+
+	addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
+	if (addr == FDT_ADDR_T_NONE)
+		return NULL;
+	else
+		return (void *)addr;
+}
+#else
+void *dev_get_addr(struct udevice *dev)
+{
+	return NULL;
+}
+#endif
-- 
1.8.4

[U-Boot] [PATCH v3 2/4] dm: add dev_get_addr prototype

[Peng Fan]

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
---
 include/dm/device.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/dm/device.h b/include/dm/device.h
index 13598a1..ee00c4d 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -322,4 +322,13 @@ int device_find_first_child(struct udevice *parent, struct udevice **devp);
  */
 int device_find_next_child(struct udevice **devp);
 
+
+/**
+ * dev_get_addr() - Get the reg property of a device
+ *
+ * @dev: Pointer to a device. Returns reg address, or NULL if no DT
+ *
+ * @return addr, or NULL if NO DT
+ */
+void *dev_get_addr(struct udevice *dev);
 #endif
-- 
1.8.4

[U-Boot] [PATCH v3 3/4] dm:gpio:mxc add a bank_index entry in platdata

[Peng Fan]

Add a new entry in platdata structure and intialize
bank_index in mxc_plat array.
This new entry can avoid using `plat - mxc_plat` by using
`plat->bank_index`.

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
---
 drivers/gpio/mxc_gpio.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
index 8bb9e39..c52dd19 100644
--- a/drivers/gpio/mxc_gpio.c
+++ b/drivers/gpio/mxc_gpio.c
@@ -23,6 +23,7 @@ enum mxc_gpio_direction {
 #define GPIO_PER_BANK			32
 
 struct mxc_gpio_plat {
+	int bank_index;
 	struct gpio_regs *regs;
 };
 
@@ -259,19 +260,19 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
 };
 
 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 },
+	{ 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
+	{ 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
+	{ 2, (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 },
+	{ 3, (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 },
+	{ 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
+	{ 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
 #endif
 #if defined(CONFIG_MX53) || defined(CONFIG_MX6)
-	{ (struct gpio_regs *)GPIO7_BASE_ADDR },
+	{ 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
 #endif
 };
 
@@ -283,7 +284,7 @@ static int mxc_gpio_probe(struct udevice *dev)
 	int banknum;
 	char name[18], *str;
 
-	banknum = plat - mxc_plat;
+	banknum = plat->bank_index;
 	sprintf(name, "GPIO%d_", banknum + 1);
 	str = strdup(name);
 	if (!str)
-- 
1.8.4

[U-Boot] [PATCH v3 4/4] dm:gpio:mxc add DT support

[Peng Fan]

This patch add DT support for mxc gpio driver.

There are one place using CONFIG_OF_CONTROL macro.
1. The U_BOOT_DEVICES and mxc_plat array are complied out. To DT,
   platdata is alloced using calloc, so there is no need to use mxc_plat.

The following situations are tested, and all work fine:
1. with DM, without DT
2. with DM and DT
3. without DM
Since device tree has not been upstreamed, if want to test this patch.
The followings need to be done.
 + pieces of code does not gpio_request when using gpio_direction_xxx and
   etc, need to request gpio.
 + move the gpio settings from board_early_init_f to board_init
 + define CONFIG_DM ,CONFIG_DM_GPIO and CONFIG_OF_CONTROL
 + Add device tree file and do related configuration in
   `make ARCH=arm menuconfig`
These will be done in future patches by step.

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
---
 drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
index c52dd19..0766b78 100644
--- a/drivers/gpio/mxc_gpio.c
+++ b/drivers/gpio/mxc_gpio.c
@@ -151,6 +151,9 @@ int gpio_direction_output(unsigned gpio, int value)
 #endif
 
 #ifdef CONFIG_DM_GPIO
+#include <fdtdec.h>
+DECLARE_GLOBAL_DATA_PTR;
+
 static int mxc_gpio_is_output(struct gpio_regs *regs, int offset)
 {
 	u32 val;
@@ -259,23 +262,6 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
 	.get_function		= mxc_gpio_get_function,
 };
 
-static const struct mxc_gpio_plat mxc_plat[] = {
-	{ 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
-	{ 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
-	{ 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
-#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
-		defined(CONFIG_MX53) || defined(CONFIG_MX6)
-	{ 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
-#endif
-#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
-	{ 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
-	{ 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
-#endif
-#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
-	{ 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
-#endif
-};
-
 static int mxc_gpio_probe(struct udevice *dev)
 {
 	struct mxc_bank_info *bank = dev_get_priv(dev);
@@ -296,12 +282,60 @@ static int mxc_gpio_probe(struct udevice *dev)
 	return 0;
 }
 
+static int mxc_gpio_bind(struct udevice *device)
+{
+	struct mxc_gpio_plat *plat = device->platdata;
+	struct gpio_regs *regs;
+
+	if (plat)
+		return 0;
+
+	regs = dev_get_addr(device);
+	if (!regs)
+		return -ENXIO;
+
+	plat = calloc(1, sizeof(*plat));
+	if (!plat)
+		return -ENOMEM;
+
+	plat->regs = regs;
+	plat->bank_index = device->req_seq;
+	device->platdata = plat;
+
+	return 0;
+}
+
+static const struct udevice_id mxc_gpio_ids[] = {
+	{ .compatible = "fsl,imx35-gpio" },
+	{ }
+};
+
 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),
+	.of_match = mxc_gpio_ids,
+	.bind	= mxc_gpio_bind,
+};
+
+#ifndef CONFIG_OF_CONTROL
+static const struct mxc_gpio_plat mxc_plat[] = {
+	{ 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
+	{ 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
+	{ 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
+#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
+		defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
+#endif
+#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
+	{ 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
+#endif
+#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
+	{ 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
+#endif
 };
 
 U_BOOT_DEVICES(mxc_gpios) = {
@@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {
 #endif
 };
 #endif
+#endif
-- 
1.8.4

[U-Boot] [PATCH v3 1/4] dm: introduce dev_get_addr interface

[Igor Grinberg]

On 01/21/15 13:09, Peng Fan wrote:
> Abstracting dev_get_addr can improve drivers that want to
> get device's address.
> 
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>

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


-- 
Regards,
Igor.

[U-Boot] [PATCH v3 2/4] dm: add dev_get_addr prototype

[Igor Grinberg]

On 01/21/15 13:09, Peng Fan wrote:
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>

I think this should be a part of the first patch, anyway:

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

> ---
>  include/dm/device.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 13598a1..ee00c4d 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -322,4 +322,13 @@ int device_find_first_child(struct udevice *parent, struct udevice **devp);
>   */
>  int device_find_next_child(struct udevice **devp);
>  
> +
> +/**
> + * dev_get_addr() - Get the reg property of a device
> + *
> + * @dev: Pointer to a device. Returns reg address, or NULL if no DT
> + *
> + * @return addr, or NULL if NO DT
> + */
> +void *dev_get_addr(struct udevice *dev);
>  #endif
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH v3 3/4] dm:gpio:mxc add a bank_index entry in platdata

[Igor Grinberg]

On 01/21/15 13:09, Peng Fan wrote:
> Add a new entry in platdata structure and intialize
> bank_index in mxc_plat array.
> This new entry can avoid using `plat - mxc_plat` by using
> `plat->bank_index`.
> 
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>

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


-- 
Regards,
Igor.

[U-Boot] [PATCH v3 4/4] dm:gpio:mxc add DT support

[Peng Fan]

Hi Igor,

Just kindly remind, did you miss this one? Since you ack the other 
patches in this patch set.

On 1/21/2015 7:09 PM, Peng Fan wrote:
> This patch add DT support for mxc gpio driver.
>
> There are one place using CONFIG_OF_CONTROL macro.
> 1. The U_BOOT_DEVICES and mxc_plat array are complied out. To DT,
>     platdata is alloced using calloc, so there is no need to use mxc_plat.
>
> The following situations are tested, and all work fine:
> 1. with DM, without DT
> 2. with DM and DT
> 3. without DM
> Since device tree has not been upstreamed, if want to test this patch.
> The followings need to be done.
>   + pieces of code does not gpio_request when using gpio_direction_xxx and
>     etc, need to request gpio.
>   + move the gpio settings from board_early_init_f to board_init
>   + define CONFIG_DM ,CONFIG_DM_GPIO and CONFIG_OF_CONTROL
>   + Add device tree file and do related configuration in
>     `make ARCH=arm menuconfig`
> These will be done in future patches by step.
>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> ---
>   drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 52 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
> index c52dd19..0766b78 100644
> --- a/drivers/gpio/mxc_gpio.c
> +++ b/drivers/gpio/mxc_gpio.c
> @@ -151,6 +151,9 @@ int gpio_direction_output(unsigned gpio, int value)
>   #endif
>   
>   #ifdef CONFIG_DM_GPIO
> +#include <fdtdec.h>
> +DECLARE_GLOBAL_DATA_PTR;
> +
>   static int mxc_gpio_is_output(struct gpio_regs *regs, int offset)
>   {
>   	u32 val;
> @@ -259,23 +262,6 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
>   	.get_function		= mxc_gpio_get_function,
>   };
>   
> -static const struct mxc_gpio_plat mxc_plat[] = {
> -	{ 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
> -	{ 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
> -	{ 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
> -#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
> -		defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -	{ 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
> -#endif
> -#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -	{ 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
> -	{ 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
> -#endif
> -#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -	{ 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
> -#endif
> -};
> -
>   static int mxc_gpio_probe(struct udevice *dev)
>   {
>   	struct mxc_bank_info *bank = dev_get_priv(dev);
> @@ -296,12 +282,60 @@ static int mxc_gpio_probe(struct udevice *dev)
>   	return 0;
>   }
>   
> +static int mxc_gpio_bind(struct udevice *device)
> +{
> +	struct mxc_gpio_plat *plat = device->platdata;
> +	struct gpio_regs *regs;
> +
> +	if (plat)
> +		return 0;
> +
> +	regs = dev_get_addr(device);
> +	if (!regs)
> +		return -ENXIO;
> +
> +	plat = calloc(1, sizeof(*plat));
> +	if (!plat)
> +		return -ENOMEM;
> +
> +	plat->regs = regs;
> +	plat->bank_index = device->req_seq;
> +	device->platdata = plat;
> +
> +	return 0;
> +}
> +
> +static const struct udevice_id mxc_gpio_ids[] = {
> +	{ .compatible = "fsl,imx35-gpio" },
> +	{ }
> +};
> +
>   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),
> +	.of_match = mxc_gpio_ids,
> +	.bind	= mxc_gpio_bind,
> +};
> +
> +#ifndef CONFIG_OF_CONTROL
> +static const struct mxc_gpio_plat mxc_plat[] = {
> +	{ 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
> +	{ 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
> +	{ 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
> +#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
> +		defined(CONFIG_MX53) || defined(CONFIG_MX6)
> +	{ 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
> +#endif
> +#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
> +	{ 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
> +	{ 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
> +#endif
> +#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
> +	{ 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
> +#endif
>   };
>   
>   U_BOOT_DEVICES(mxc_gpios) = {
> @@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {
>   #endif
>   };
>   #endif
> +#endif
Thanks,
Peng.

[U-Boot] [PATCH v3 4/4] dm:gpio:mxc add DT support

[Igor Grinberg]

Hi Peng,

On 01/22/15 03:06, Peng Fan wrote:
> Hi Igor,
> 
> Just kindly remind, did you miss this one? Since you ack the other patches in this patch set.

Nope, I did not miss it.
I just haven't looked at it yet...

> 
> On 1/21/2015 7:09 PM, Peng Fan wrote:
>> This patch add DT support for mxc gpio driver.
>>
>> There are one place using CONFIG_OF_CONTROL macro.
>> 1. The U_BOOT_DEVICES and mxc_plat array are complied out. To DT,
>>     platdata is alloced using calloc, so there is no need to use mxc_plat.
>>
>> The following situations are tested, and all work fine:
>> 1. with DM, without DT
>> 2. with DM and DT
>> 3. without DM
>> Since device tree has not been upstreamed, if want to test this patch.
>> The followings need to be done.
>>   + pieces of code does not gpio_request when using gpio_direction_xxx and
>>     etc, need to request gpio.
>>   + move the gpio settings from board_early_init_f to board_init
>>   + define CONFIG_DM ,CONFIG_DM_GPIO and CONFIG_OF_CONTROL
>>   + Add device tree file and do related configuration in
>>     `make ARCH=arm menuconfig`
>> These will be done in future patches by step.
>>
>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>

Besides the question below, looks good.

>> ---
>>   drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------
>>   1 file changed, 52 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
>> index c52dd19..0766b78 100644
>> --- a/drivers/gpio/mxc_gpio.c
>> +++ b/drivers/gpio/mxc_gpio.c
>> @@ -151,6 +151,9 @@ int gpio_direction_output(unsigned gpio, int value)
>>   #endif
>>     #ifdef CONFIG_DM_GPIO
>> +#include <fdtdec.h>
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>>   static int mxc_gpio_is_output(struct gpio_regs *regs, int offset)
>>   {
>>       u32 val;
>> @@ -259,23 +262,6 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
>>       .get_function        = mxc_gpio_get_function,
>>   };
>>   -static const struct mxc_gpio_plat mxc_plat[] = {
>> -    { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
>> -    { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
>> -    { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
>> -#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
>> -        defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> -    { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
>> -#endif
>> -#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> -    { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
>> -    { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
>> -#endif
>> -#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> -    { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
>> -#endif
>> -};
>> -
>>   static int mxc_gpio_probe(struct udevice *dev)
>>   {
>>       struct mxc_bank_info *bank = dev_get_priv(dev);
>> @@ -296,12 +282,60 @@ static int mxc_gpio_probe(struct udevice *dev)
>>       return 0;
>>   }
>>   +static int mxc_gpio_bind(struct udevice *device)
>> +{
>> +    struct mxc_gpio_plat *plat = device->platdata;
>> +    struct gpio_regs *regs;
>> +
>> +    if (plat)
>> +        return 0;
>> +
>> +    regs = dev_get_addr(device);
>> +    if (!regs)
>> +        return -ENXIO;
>> +
>> +    plat = calloc(1, sizeof(*plat));
>> +    if (!plat)
>> +        return -ENOMEM;
>> +
>> +    plat->regs = regs;
>> +    plat->bank_index = device->req_seq;

Can we assume that in mxc_gpio case, device->req_seq will never equal -1?

>> +    device->platdata = plat;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct udevice_id mxc_gpio_ids[] = {
>> +    { .compatible = "fsl,imx35-gpio" },
>> +    { }
>> +};
>> +
>>   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),
>> +    .of_match = mxc_gpio_ids,
>> +    .bind    = mxc_gpio_bind,
>> +};
>> +
>> +#ifndef CONFIG_OF_CONTROL
>> +static const struct mxc_gpio_plat mxc_plat[] = {
>> +    { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
>> +    { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
>> +    { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
>> +#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
>> +        defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> +    { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
>> +#endif
>> +#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> +    { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
>> +    { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
>> +#endif
>> +#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> +    { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
>> +#endif
>>   };
>>     U_BOOT_DEVICES(mxc_gpios) = {
>> @@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {
>>   #endif
>>   };
>>   #endif
>> +#endif
> Thanks,
> Peng.
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH v3 4/4] dm:gpio:mxc add DT support

[Peng Fan]

Hi, Igor

Reply below.

On 1/22/2015 4:38 PM, Igor Grinberg wrote:
> Hi Peng,
>
> On 01/22/15 03:06, Peng Fan wrote:
>> Hi Igor,
>>
>> Just kindly remind, did you miss this one? Since you ack the other patches in this patch set.
> Nope, I did not miss it.
> I just haven't looked at it yet...
>
>> On 1/21/2015 7:09 PM, Peng Fan wrote:
>>> This patch add DT support for mxc gpio driver.
>>>
>>> There are one place using CONFIG_OF_CONTROL macro.
>>> 1. The U_BOOT_DEVICES and mxc_plat array are complied out. To DT,
>>>      platdata is alloced using calloc, so there is no need to use mxc_plat.
>>>
>>> The following situations are tested, and all work fine:
>>> 1. with DM, without DT
>>> 2. with DM and DT
>>> 3. without DM
>>> Since device tree has not been upstreamed, if want to test this patch.
>>> The followings need to be done.
>>>    + pieces of code does not gpio_request when using gpio_direction_xxx and
>>>      etc, need to request gpio.
>>>    + move the gpio settings from board_early_init_f to board_init
>>>    + define CONFIG_DM ,CONFIG_DM_GPIO and CONFIG_OF_CONTROL
>>>    + Add device tree file and do related configuration in
>>>      `make ARCH=arm menuconfig`
>>> These will be done in future patches by step.
>>>
>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> Besides the question below, looks good.
>
>>> ---
>>>    drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------
>>>    1 file changed, 52 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
>>> index c52dd19..0766b78 100644
>>> --- a/drivers/gpio/mxc_gpio.c
>>> +++ b/drivers/gpio/mxc_gpio.c
>>> @@ -151,6 +151,9 @@ int gpio_direction_output(unsigned gpio, int value)
>>>    #endif
>>>      #ifdef CONFIG_DM_GPIO
>>> +#include <fdtdec.h>
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>>    static int mxc_gpio_is_output(struct gpio_regs *regs, int offset)
>>>    {
>>>        u32 val;
>>> @@ -259,23 +262,6 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
>>>        .get_function        = mxc_gpio_get_function,
>>>    };
>>>    -static const struct mxc_gpio_plat mxc_plat[] = {
>>> -    { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
>>> -    { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
>>> -    { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
>>> -#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
>>> -        defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> -    { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
>>> -#endif
>>> -#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> -    { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
>>> -    { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
>>> -#endif
>>> -#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> -    { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
>>> -#endif
>>> -};
>>> -
>>>    static int mxc_gpio_probe(struct udevice *dev)
>>>    {
>>>        struct mxc_bank_info *bank = dev_get_priv(dev);
>>> @@ -296,12 +282,60 @@ static int mxc_gpio_probe(struct udevice *dev)
>>>        return 0;
>>>    }
>>>    +static int mxc_gpio_bind(struct udevice *device)
>>> +{
>>> +    struct mxc_gpio_plat *plat = device->platdata;
>>> +    struct gpio_regs *regs;
>>> +
>>> +    if (plat)
>>> +        return 0;
>>> +
>>> +    regs = dev_get_addr(device);
>>> +    if (!regs)
>>> +        return -ENXIO;
>>> +
>>> +    plat = calloc(1, sizeof(*plat));
>>> +    if (!plat)
>>> +        return -ENOMEM;
>>> +
>>> +    plat->regs = regs;
>>> +    plat->bank_index = device->req_seq;
> Can we assume that in mxc_gpio case, device->req_seq will never equal -1?
To NO DT situation, "if (plat) return0;" will directly return, because 
plat is staticlly intialized in source file. To DT,  in aliases, 
"gpio0=&gpio1" and etc should be added, to make
req_seq work. If "reg" property or "alises" are not provied, req_seq 
will be -1. Here I want aliases, because want bank_index from 0 to max 
bank, but this can not be guaranteed.

If reg property is not provided, dev_get_addr will return error. And 
plat->bank_index = device->req_seq will not be executed. If reg property 
is provided, but alises "gpiox=&gpioy" are not provied, there is not a 
good way to check req_seq exceeds max bank, since I do not want to 
include `#define xxMAX_BANK yy`. So I did not test this "req_seq < 0 " 
situation.
>
>>> +    device->platdata = plat;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct udevice_id mxc_gpio_ids[] = {
>>> +    { .compatible = "fsl,imx35-gpio" },
>>> +    { }
>>> +};
>>> +
>>>    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),
>>> +    .of_match = mxc_gpio_ids,
>>> +    .bind    = mxc_gpio_bind,
>>> +};
>>> +
>>> +#ifndef CONFIG_OF_CONTROL
>>> +static const struct mxc_gpio_plat mxc_plat[] = {
>>> +    { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
>>> +    { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
>>> +    { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
>>> +#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
>>> +        defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> +    { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
>>> +#endif
>>> +#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> +    { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
>>> +    { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
>>> +#endif
>>> +#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> +    { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
>>> +#endif
>>>    };
>>>      U_BOOT_DEVICES(mxc_gpios) = {
>>> @@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {
>>>    #endif
>>>    };
>>>    #endif
>>> +#endif
>> Thanks,
>> Peng.
>>
Thanks,
Peng.

[U-Boot] [PATCH v3 1/4] dm: introduce dev_get_addr interface

[Simon Glass]

Hi,

On 21 January 2015 at 04:09, Peng Fan <Peng.Fan@freescale.com> wrote:
> Abstracting dev_get_addr can improve drivers that want to
> get device's address.
>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> ---
>  drivers/core/device.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 963b16f..0ba5c76 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -12,6 +12,7 @@
>  #include <common.h>
>  #include <fdtdec.h>
>  #include <malloc.h>
> +#include <libfdt.h>
>  #include <dm/device.h>
>  #include <dm/device-internal.h>
>  #include <dm/lists.h>
> @@ -390,3 +391,21 @@ ulong dev_get_of_data(struct udevice *dev)
>  {
>         return dev->of_id->data;
>  }
> +
> +#ifdef CONFIG_OF_CONTROL
> +void *dev_get_addr(struct udevice *dev)

My approach so far has been to use a ulong for the device address
(e.g. in platform data) and only use a pointer when we know the type
(e.g. struct disp_ctlr *), typically in driver-private data.

So do you think it would be better to return FDT_ADDR_T_NONE?

> +{
> +       fdt_addr_t addr;
> +
> +       addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
> +       if (addr == FDT_ADDR_T_NONE)
> +               return NULL;
> +       else
> +               return (void *)addr;
> +}
> +#else
> +void *dev_get_addr(struct udevice *dev)
> +{
> +       return NULL;
> +}
> +#endif
> --
> 1.8.4
>
>

Regards,
Simon

[U-Boot] [PATCH v3 4/4] dm:gpio:mxc add DT support

[Simon Glass]

Hi Peng,

On 21 January 2015 at 04:09, Peng Fan <Peng.Fan@freescale.com> wrote:
> This patch add DT support for mxc gpio driver.
>
> There are one place using CONFIG_OF_CONTROL macro.
> 1. The U_BOOT_DEVICES and mxc_plat array are complied out. To DT,
>    platdata is alloced using calloc, so there is no need to use mxc_plat.
>
> The following situations are tested, and all work fine:
> 1. with DM, without DT
> 2. with DM and DT
> 3. without DM
> Since device tree has not been upstreamed, if want to test this patch.
> The followings need to be done.
>  + pieces of code does not gpio_request when using gpio_direction_xxx and
>    etc, need to request gpio.
>  + move the gpio settings from board_early_init_f to board_init
>  + define CONFIG_DM ,CONFIG_DM_GPIO and CONFIG_OF_CONTROL
>  + Add device tree file and do related configuration in
>    `make ARCH=arm menuconfig`
> These will be done in future patches by step.
>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> ---
>  drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 52 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
> index c52dd19..0766b78 100644
> --- a/drivers/gpio/mxc_gpio.c
> +++ b/drivers/gpio/mxc_gpio.c
> @@ -151,6 +151,9 @@ int gpio_direction_output(unsigned gpio, int value)
>  #endif
>
>  #ifdef CONFIG_DM_GPIO
> +#include <fdtdec.h>
> +DECLARE_GLOBAL_DATA_PTR;
> +
>  static int mxc_gpio_is_output(struct gpio_regs *regs, int offset)
>  {
>         u32 val;
> @@ -259,23 +262,6 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
>         .get_function           = mxc_gpio_get_function,
>  };
>
> -static const struct mxc_gpio_plat mxc_plat[] = {
> -       { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
> -       { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
> -       { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
> -#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
> -               defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -       { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
> -#endif
> -#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -       { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
> -       { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
> -#endif
> -#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -       { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
> -#endif
> -};
> -
>  static int mxc_gpio_probe(struct udevice *dev)
>  {
>         struct mxc_bank_info *bank = dev_get_priv(dev);
> @@ -296,12 +282,60 @@ static int mxc_gpio_probe(struct udevice *dev)
>         return 0;
>  }
>
> +static int mxc_gpio_bind(struct udevice *device)

s/device/dev/ as that is the convention.

> +{
> +       struct mxc_gpio_plat *plat = device->platdata;
> +       struct gpio_regs *regs;
> +
> +       if (plat)
> +               return 0;

Please add a comment as to why.

> +
> +       regs = dev_get_addr(device);
> +       if (!regs)
> +               return -ENXIO;

-ENODEV I think?

> +
> +       plat = calloc(1, sizeof(*plat));
> +       if (!plat)
> +               return -ENOMEM;

Can you use the auto-alloc feature instead? Trying to keep memory
allocations out of drivers unless it is for buffers, etc.

> +
> +       plat->regs = regs;
> +       plat->bank_index = device->req_seq;

Why store this? You can access it anytime using the device pointer.

> +       device->platdata = plat;
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id mxc_gpio_ids[] = {
> +       { .compatible = "fsl,imx35-gpio" },
> +       { }
> +};
> +
>  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),
> +       .of_match = mxc_gpio_ids,

Masahiro added a function for this.:

       .of_match = of_match_ptr(mxc_gpio_ids),

But I'm not completely sure if this is wanted, since you include this
information even when not using device tree.

> +       .bind   = mxc_gpio_bind,
> +};
> +
> +#ifndef CONFIG_OF_CONTROL
> +static const struct mxc_gpio_plat mxc_plat[] = {
> +       { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
> +       { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
> +       { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
> +#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
> +               defined(CONFIG_MX53) || defined(CONFIG_MX6)
> +       { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
> +#endif
> +#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
> +       { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
> +       { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
> +#endif
> +#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
> +       { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
> +#endif

Can we remove the casts by changing the type to ulong?

>  };
>
>  U_BOOT_DEVICES(mxc_gpios) = {
> @@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {
>  #endif
>  };
>  #endif
> +#endif
> --
> 1.8.4
>
>

Regards,
Simon

[U-Boot] [PATCH v3 3/4] dm:gpio:mxc add a bank_index entry in platdata

[Simon Glass]

Hi,

On 21 January 2015 at 04:09, Peng Fan <Peng.Fan@freescale.com> wrote:
> Add a new entry in platdata structure and intialize
> bank_index in mxc_plat array.
> This new entry can avoid using `plat - mxc_plat` by using
> `plat->bank_index`.
>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> ---
>  drivers/gpio/mxc_gpio.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
> index 8bb9e39..c52dd19 100644
> --- a/drivers/gpio/mxc_gpio.c
> +++ b/drivers/gpio/mxc_gpio.c
> @@ -23,6 +23,7 @@ enum mxc_gpio_direction {
>  #define GPIO_PER_BANK                  32
>
>  struct mxc_gpio_plat {
> +       int bank_index;
>         struct gpio_regs *regs;
>  };
>
> @@ -259,19 +260,19 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
>  };
>
>  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 },
> +       { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
> +       { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
> +       { 2, (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 },
> +       { 3, (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 },
> +       { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
> +       { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
>  #endif
>  #if defined(CONFIG_MX53) || defined(CONFIG_MX6)
> -       { (struct gpio_regs *)GPIO7_BASE_ADDR },
> +       { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },

Is it possible to drop these casts? Or should that be another patch?

>  #endif
>  };
>
> @@ -283,7 +284,7 @@ static int mxc_gpio_probe(struct udevice *dev)
>         int banknum;
>         char name[18], *str;
>
> -       banknum = plat - mxc_plat;
> +       banknum = plat->bank_index;
>         sprintf(name, "GPIO%d_", banknum + 1);
>         str = strdup(name);
>         if (!str)
> --
> 1.8.4
>
>

Regards,
Simon

[U-Boot] [PATCH v3 4/4] dm:gpio:mxc add DT support

[Peng Fan]

Hi Simon,

On 1/23/2015 5:26 AM, Simon Glass wrote:
> Hi Peng,
>
> On 21 January 2015 at 04:09, Peng Fan <Peng.Fan@freescale.com> wrote:
>> This patch add DT support for mxc gpio driver.
>>
>> There are one place using CONFIG_OF_CONTROL macro.
>> 1. The U_BOOT_DEVICES and mxc_plat array are complied out. To DT,
>>     platdata is alloced using calloc, so there is no need to use mxc_plat.
>>
>> The following situations are tested, and all work fine:
>> 1. with DM, without DT
>> 2. with DM and DT
>> 3. without DM
>> Since device tree has not been upstreamed, if want to test this patch.
>> The followings need to be done.
>>   + pieces of code does not gpio_request when using gpio_direction_xxx and
>>     etc, need to request gpio.
>>   + move the gpio settings from board_early_init_f to board_init
>>   + define CONFIG_DM ,CONFIG_DM_GPIO and CONFIG_OF_CONTROL
>>   + Add device tree file and do related configuration in
>>     `make ARCH=arm menuconfig`
>> These will be done in future patches by step.
>>
>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>> ---
>>   drivers/gpio/mxc_gpio.c | 69 +++++++++++++++++++++++++++++++++++++------------
>>   1 file changed, 52 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
>> index c52dd19..0766b78 100644
>> --- a/drivers/gpio/mxc_gpio.c
>> +++ b/drivers/gpio/mxc_gpio.c
>> @@ -151,6 +151,9 @@ int gpio_direction_output(unsigned gpio, int value)
>>   #endif
>>
>>   #ifdef CONFIG_DM_GPIO
>> +#include <fdtdec.h>
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>>   static int mxc_gpio_is_output(struct gpio_regs *regs, int offset)
>>   {
>>          u32 val;
>> @@ -259,23 +262,6 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
>>          .get_function           = mxc_gpio_get_function,
>>   };
>>
>> -static const struct mxc_gpio_plat mxc_plat[] = {
>> -       { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
>> -       { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
>> -       { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
>> -#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
>> -               defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> -       { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
>> -#endif
>> -#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> -       { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
>> -       { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
>> -#endif
>> -#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> -       { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
>> -#endif
>> -};
>> -
>>   static int mxc_gpio_probe(struct udevice *dev)
>>   {
>>          struct mxc_bank_info *bank = dev_get_priv(dev);
>> @@ -296,12 +282,60 @@ static int mxc_gpio_probe(struct udevice *dev)
>>          return 0;
>>   }
>>
>> +static int mxc_gpio_bind(struct udevice *device)
> s/device/dev/ as that is the convention.
Will fix this.
>
>> +{
>> +       struct mxc_gpio_plat *plat = device->platdata;
>> +       struct gpio_regs *regs;
>> +
>> +       if (plat)
>> +               return 0;
> Please add a comment as to why.
Ok.
>
>> +
>> +       regs = dev_get_addr(device);
>> +       if (!regs)
>> +               return -ENXIO;
> -ENODEV I think?
Yeah. Right.
>> +
>> +       plat = calloc(1, sizeof(*plat));
>> +       if (!plat)
>> +               return -ENOMEM;
> Can you use the auto-alloc feature instead? Trying to keep memory
> allocations out of drivers unless it is for buffers, etc.
I want the DM code can support DT and no DT. To no DT, platdata is 
defined in U_BOOT_DEVICES.
If using auto-alloc feature, but DT is not supported, is it conflict 
with platdata in U_BOOT_DEVICES?
>
>> +
>> +       plat->regs = regs;
>> +       plat->bank_index = device->req_seq;
> Why store this? You can access it anytime using the device pointer.
To no DT, bank_index is statically intialized  in mxc_plat array. I do 
not want to introudce `#ifdef CONFIG_OF_CONTROL` in probe function and 
introudce `if (dev->of_offset == -1)`, so
store it to bank_index.
To no DT, `if(plat) return 0;` will return. So plat->bank_index = 
device->req_seq will only effects for DT.
Just want to support DT and no DT.
>
>> +       device->platdata = plat;
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct udevice_id mxc_gpio_ids[] = {
>> +       { .compatible = "fsl,imx35-gpio" },
>> +       { }
>> +};
>> +
>>   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),
>> +       .of_match = mxc_gpio_ids,
> Masahiro added a function for this.:
>
>         .of_match = of_match_ptr(mxc_gpio_ids),
>
> But I'm not completely sure if this is wanted, since you include this
> information even when not using device tree.
Thanks,I'll try this. I am not sure whether using of_match_ptr will make 
compiler complain mxc_gpio_ids `defined but not used` for no DT, since 
`#ifdef xx` is not recommended to compiled out `mxc_gpio_ids` for no DT.
>
>> +       .bind   = mxc_gpio_bind,
>> +};
>> +
>> +#ifndef CONFIG_OF_CONTROL
>> +static const struct mxc_gpio_plat mxc_plat[] = {
>> +       { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
>> +       { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
>> +       { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
>> +#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51) || \
>> +               defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> +       { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
>> +#endif
>> +#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> +       { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
>> +       { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
>> +#endif
>> +#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
>> +       { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
>> +#endif
> Can we remove the casts by changing the type to ulong?
Hmm, this patch is just to make this driver can support DT. This will 
introduce more change. Also, changing the type to ulong will change 
mxc_gpio_plat struct.
If change the type to ulong, functions in this driver that uses platdata 
will casts it to `struct gpio_regs *`. So i'd like not to remove the 
casts, since remove them in mxc_plat will introduce casts in other 
functions which use the platdata.
>
>>   };
>>
>>   U_BOOT_DEVICES(mxc_gpios) = {
>> @@ -321,3 +355,4 @@ U_BOOT_DEVICES(mxc_gpios) = {
>>   #endif
>>   };
>>   #endif
>> +#endif
>> --
>> 1.8.4
>>
>>
> Regards,
> Simon
Regards,
Peng.

[U-Boot] [PATCH v3 4/4] dm:gpio:mxc add DT support

[Simon Glass]

Hi Peng,

On 24 January 2015 at 07:34, Peng Fan <B51431@freescale.com> wrote:
> Hi Simon,
>
>
> On 1/23/2015 5:26 AM, Simon Glass wrote:
>>
>> Hi Peng,
>>
>> On 21 January 2015 at 04:09, Peng Fan <Peng.Fan@freescale.com> wrote:
>>>
>>> This patch add DT support for mxc gpio driver.
>>>
>>> There are one place using CONFIG_OF_CONTROL macro.
>>> 1. The U_BOOT_DEVICES and mxc_plat array are complied out. To DT,
>>>     platdata is alloced using calloc, so there is no need to use
>>> mxc_plat.
>>>
>>> The following situations are tested, and all work fine:
>>> 1. with DM, without DT
>>> 2. with DM and DT
>>> 3. without DM
>>> Since device tree has not been upstreamed, if want to test this patch.
>>> The followings need to be done.
>>>   + pieces of code does not gpio_request when using gpio_direction_xxx
>>> and
>>>     etc, need to request gpio.
>>>   + move the gpio settings from board_early_init_f to board_init
>>>   + define CONFIG_DM ,CONFIG_DM_GPIO and CONFIG_OF_CONTROL
>>>   + Add device tree file and do related configuration in
>>>     `make ARCH=arm menuconfig`
>>> These will be done in future patches by step.
>>>
>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>>> ---
>>>   drivers/gpio/mxc_gpio.c | 69
>>> +++++++++++++++++++++++++++++++++++++------------
>>>   1 file changed, 52 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
>>> index c52dd19..0766b78 100644
>>> --- a/drivers/gpio/mxc_gpio.c
>>> +++ b/drivers/gpio/mxc_gpio.c
>>> @@ -151,6 +151,9 @@ int gpio_direction_output(unsigned gpio, int value)
>>>   #endif
>>>
>>>   #ifdef CONFIG_DM_GPIO
>>> +#include <fdtdec.h>
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>>   static int mxc_gpio_is_output(struct gpio_regs *regs, int offset)
>>>   {
>>>          u32 val;
>>> @@ -259,23 +262,6 @@ static const struct dm_gpio_ops gpio_mxc_ops = {
>>>          .get_function           = mxc_gpio_get_function,
>>>   };
>>>
>>> -static const struct mxc_gpio_plat mxc_plat[] = {
>>> -       { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
>>> -       { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
>>> -       { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
>>> -#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51)
>>> || \
>>> -               defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> -       { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
>>> -#endif
>>> -#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> -       { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
>>> -       { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
>>> -#endif
>>> -#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> -       { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
>>> -#endif
>>> -};
>>> -
>>>   static int mxc_gpio_probe(struct udevice *dev)
>>>   {
>>>          struct mxc_bank_info *bank = dev_get_priv(dev);
>>> @@ -296,12 +282,60 @@ static int mxc_gpio_probe(struct udevice *dev)
>>>          return 0;
>>>   }
>>>
>>> +static int mxc_gpio_bind(struct udevice *device)
>>
>> s/device/dev/ as that is the convention.
>
> Will fix this.
>>
>>
>>> +{
>>> +       struct mxc_gpio_plat *plat = device->platdata;
>>> +       struct gpio_regs *regs;
>>> +
>>> +       if (plat)
>>> +               return 0;
>>
>> Please add a comment as to why.
>
> Ok.
>>
>>
>>> +
>>> +       regs = dev_get_addr(device);
>>> +       if (!regs)
>>> +               return -ENXIO;
>>
>> -ENODEV I think?
>
> Yeah. Right.
>>>
>>> +
>>> +       plat = calloc(1, sizeof(*plat));
>>> +       if (!plat)
>>> +               return -ENOMEM;
>>
>> Can you use the auto-alloc feature instead? Trying to keep memory
>> allocations out of drivers unless it is for buffers, etc.
>
> I want the DM code can support DT and no DT. To no DT, platdata is defined
> in U_BOOT_DEVICES.
> If using auto-alloc feature, but DT is not supported, is it conflict with
> platdata in U_BOOT_DEVICES?

Yes it will. But please add a TODO in the code to remove this when
every board is converted to driver model and you don't need this.

>>
>>
>>> +
>>> +       plat->regs = regs;
>>> +       plat->bank_index = device->req_seq;
>>
>> Why store this? You can access it anytime using the device pointer.
>
> To no DT, bank_index is statically intialized  in mxc_plat array. I do not
> want to introudce `#ifdef CONFIG_OF_CONTROL` in probe function and introudce
> `if (dev->of_offset == -1)`, so
> store it to bank_index.
> To no DT, `if(plat) return 0;` will return. So plat->bank_index =
> device->req_seq will only effects for DT.
> Just want to support DT and no DT.

OK  I think I understand.

>>
>>
>>> +       device->platdata = plat;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static const struct udevice_id mxc_gpio_ids[] = {
>>> +       { .compatible = "fsl,imx35-gpio" },
>>> +       { }
>>> +};
>>> +
>>>   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),
>>> +       .of_match = mxc_gpio_ids,
>>
>> Masahiro added a function for this.:
>>
>>         .of_match = of_match_ptr(mxc_gpio_ids),
>>
>> But I'm not completely sure if this is wanted, since you include this
>> information even when not using device tree.
>
> Thanks,I'll try this. I am not sure whether using of_match_ptr will make
> compiler complain mxc_gpio_ids `defined but not used` for no DT, since
> `#ifdef xx` is not recommended to compiled out `mxc_gpio_ids` for no DT.

Yes it will. What you have is OK.

>>
>>
>>> +       .bind   = mxc_gpio_bind,
>>> +};
>>> +
>>> +#ifndef CONFIG_OF_CONTROL
>>> +static const struct mxc_gpio_plat mxc_plat[] = {
>>> +       { 0, (struct gpio_regs *)GPIO1_BASE_ADDR },
>>> +       { 1, (struct gpio_regs *)GPIO2_BASE_ADDR },
>>> +       { 2, (struct gpio_regs *)GPIO3_BASE_ADDR },
>>> +#if defined(CONFIG_MX25) || defined(CONFIG_MX27) || defined(CONFIG_MX51)
>>> || \
>>> +               defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> +       { 3, (struct gpio_regs *)GPIO4_BASE_ADDR },
>>> +#endif
>>> +#if defined(CONFIG_MX27) || defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> +       { 4, (struct gpio_regs *)GPIO5_BASE_ADDR },
>>> +       { 5, (struct gpio_regs *)GPIO6_BASE_ADDR },
>>> +#endif
>>> +#if defined(CONFIG_MX53) || defined(CONFIG_MX6)
>>> +       { 6, (struct gpio_regs *)GPIO7_BASE_ADDR },
>>> +#endif
>>
>> Can we remove the casts by changing the type to ulong?
>
> Hmm, this patch is just to make this driver can support DT. This will
> introduce more change. Also, changing the type to ulong will change
> mxc_gpio_plat struct.
> If change the type to ulong, functions in this driver that uses platdata
> will casts it to `struct gpio_regs *`. So i'd like not to remove the casts,
> since remove them in mxc_plat will introduce casts in other functions which
> use the platdata.

OK. Something to think about later. In general a long line of casts
indicates something should be fixed!

Regards,
Simon

[U-Boot] [PATCH v3 1/4] dm: introduce dev_get_addr interface

[Peng Fan]

Hi, Simon

On 1/23/2015 5:25 AM, Simon Glass wrote:
> Hi,
>
> On 21 January 2015 at 04:09, Peng Fan <Peng.Fan@freescale.com> wrote:
>> Abstracting dev_get_addr can improve drivers that want to
>> get device's address.
>>
>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>> ---
>>   drivers/core/device.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>> index 963b16f..0ba5c76 100644
>> --- a/drivers/core/device.c
>> +++ b/drivers/core/device.c
>> @@ -12,6 +12,7 @@
>>   #include <common.h>
>>   #include <fdtdec.h>
>>   #include <malloc.h>
>> +#include <libfdt.h>
>>   #include <dm/device.h>
>>   #include <dm/device-internal.h>
>>   #include <dm/lists.h>
>> @@ -390,3 +391,21 @@ ulong dev_get_of_data(struct udevice *dev)
>>   {
>>          return dev->of_id->data;
>>   }
>> +
>> +#ifdef CONFIG_OF_CONTROL
>> +void *dev_get_addr(struct udevice *dev)
> My approach so far has been to use a ulong for the device address
> (e.g. in platform data) and only use a pointer when we know the type
> (e.g. struct disp_ctlr *), typically in driver-private data.
>
> So do you think it would be better to return FDT_ADDR_T_NONE?
Sorry for the long time delay to reply.
Do you agree this way using ulong as the return type?
"
#ifdef CONFIG_OF_CONTROL
unsigned long dev_get_addr(struct udevice *dev)
{
     fdt_addr_t addr;
     addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
     return addr;
}
#else
unsigned long dev_get_addr(struct udevice *dev)
{
     return FDT_ADDR_T_NONE;
}
#endif
"
Is it better to move this piece of code to include/dm/device.h and using 
static inline prototype? or put them still in driver/core/device.c?
>> +{
>> +       fdt_addr_t addr;
>> +
>> +       addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
>> +       if (addr == FDT_ADDR_T_NONE)
>> +               return NULL;
>> +       else
>> +               return (void *)addr;
>> +}
>> +#else
>> +void *dev_get_addr(struct udevice *dev)
>> +{
>> +       return NULL;
>> +}
>> +#endif
>> --
>> 1.8.4
>>
>>
> Regards,
> Simon
Thanks,
Peng.

[U-Boot] [PATCH v3 1/4] dm: introduce dev_get_addr interface

[Simon Glass]

Hi Peng,

On 5 February 2015 at 23:44, Peng Fan <B51431@freescale.com> wrote:
> Hi, Simon
>
>
> On 1/23/2015 5:25 AM, Simon Glass wrote:
>>
>> Hi,
>>
>> On 21 January 2015 at 04:09, Peng Fan <Peng.Fan@freescale.com> wrote:
>>>
>>> Abstracting dev_get_addr can improve drivers that want to
>>> get device's address.
>>>
>>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
>>> ---
>>>   drivers/core/device.c | 19 +++++++++++++++++++
>>>   1 file changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>>> index 963b16f..0ba5c76 100644
>>> --- a/drivers/core/device.c
>>> +++ b/drivers/core/device.c
>>> @@ -12,6 +12,7 @@
>>>   #include <common.h>
>>>   #include <fdtdec.h>
>>>   #include <malloc.h>
>>> +#include <libfdt.h>
>>>   #include <dm/device.h>
>>>   #include <dm/device-internal.h>
>>>   #include <dm/lists.h>
>>> @@ -390,3 +391,21 @@ ulong dev_get_of_data(struct udevice *dev)
>>>   {
>>>          return dev->of_id->data;
>>>   }
>>> +
>>> +#ifdef CONFIG_OF_CONTROL
>>> +void *dev_get_addr(struct udevice *dev)
>>
>> My approach so far has been to use a ulong for the device address
>> (e.g. in platform data) and only use a pointer when we know the type
>> (e.g. struct disp_ctlr *), typically in driver-private data.
>>
>> So do you think it would be better to return FDT_ADDR_T_NONE?
>
> Sorry for the long time delay to reply.
> Do you agree this way using ulong as the return type?
> "
> #ifdef CONFIG_OF_CONTROL
> unsigned long dev_get_addr(struct udevice *dev)
> {
>     fdt_addr_t addr;
>     addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
>     return addr;
> }
> #else
> unsigned long dev_get_addr(struct udevice *dev)
> {
>     return FDT_ADDR_T_NONE;
> }
> #endif
> "

Sure, but can you return fdt_addr_t?

> Is it better to move this piece of code to include/dm/device.h and using
> static inline prototype? or put them still in driver/core/device.c?

I'd suggest:
- static inline in the header for the case where CONFIG_OF_CONTROL is
not defined
- full implementation in device.c when that is defined

>>>
>>> +{
>>> +       fdt_addr_t addr;
>>> +
>>> +       addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
>>> +       if (addr == FDT_ADDR_T_NONE)
>>> +               return NULL;
>>> +       else
>>> +               return (void *)addr;
>>> +}
>>> +#else
>>> +void *dev_get_addr(struct udevice *dev)
>>> +{
>>> +       return NULL;
>>> +}
>>> +#endif
>>> --
>>> 1.8.4

Regards,
Simon