From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Wed, 21 Jan 2015 10:18:42 +0800 Subject: [U-Boot] [PATCH v2] dm:gpio:mxc add DT support In-Reply-To: <54BE689C.7090103@compulab.co.il> References: <1421738127-26421-1-git-send-email-Peng.Fan@freescale.com> <54BE689C.7090103@compulab.co.il> Message-ID: <54BF0C82.7040503@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, Igor On 1/20/2015 10:39 PM, Igor Grinberg wrote: > On 01/20/15 09:15, Peng Fan wrote: >> This patch add DT support for mxc gpio driver. >> >> Include a bank_index entry in platdata. This can avoid using >> `plat - mxc_plat` to calculate bank number. Also this can simplify code. > Although, I don't insist, but I would recommend to split the patch into 2: > the bank_index rework and the DT support addition. Ok. I can split this in v3 patch. > >> There are two places still using CONFIG_OF_CONTROL macro, just to >> shrink code size if only support DM but not support DT. >> 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. >> 2. add a function mxc_get_gpio_addr to get "reg" property if DT support. >> If no DT, this function just returns NULL. > I have some comments on this one, please see below... Reply below. > >> The following situations are tested: >> 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 >> --- >> >> 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. >> >> drivers/gpio/mxc_gpio.c | 89 +++++++++++++++++++++++++++++++++++++++---------- >> 1 file changed, 71 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c >> index 8bb9e39..5826620 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; >> }; >> >> @@ -150,6 +151,9 @@ int gpio_direction_output(unsigned gpio, int value) >> #endif >> >> #ifdef CONFIG_DM_GPIO >> +#include >> +DECLARE_GLOBAL_DATA_PTR; >> + >> static int mxc_gpio_is_output(struct gpio_regs *regs, int offset) >> { >> u32 val; >> @@ -258,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[] = { >> - { (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); >> @@ -283,7 +270,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) >> @@ -295,12 +282,77 @@ static int mxc_gpio_probe(struct udevice *dev) >> return 0; >> } >> >> +#ifdef CONFIG_OF_CONTROL >> +static struct gpio_regs *mxc_get_gpio_addr(struct udevice *device) >> +{ >> + fdt_addr_t addr; >> + addr = fdtdec_get_addr(gd->fdt_blob, device->of_offset, "reg"); >> + if (addr == FDT_ADDR_T_NONE) >> + return NULL; >> + else >> + return (struct gpio_regs *)addr; >> +} >> +#else >> +static struct gpio_regs *mxc_get_gpio_addr(struct udevice *device) >> +{ >> + return NULL; >> +} >> +#endif > In general, I'm fine with this concept, but I believe we should implement > a stub for fdtdec_get_addr() function in the fdtdec.h (say just returning > FDT_ADDR_T_NONE), as otherwise we might end up with multiple drivers > implementing the same noop callback just to work around a poor fdtdec_*() > interface. I tried to implement a stub function in fdtdec.h like this: __weak fdt_addr_t fdtdec_get_addr_wrap(xxxx) { return FDT_ADDR_T_NONE; } And in driver code, implement non weak version as following: #ifdef CONFIG_OF_CONTROL fdt_addr_t fdtdec_get_addr_wrap(xxxx) { .......... } #endif But gcc complains about conficting types, since we have a weak implementation in header file and a strong implementation in c file. If the weak one is in fdtxx c file, no error, but i thinke this is not a good idea to put this in fdtxx c file. If we do not want DT, but only DM, DT code should not be compiled into the final image. I tried another way, add the following piece code in driver/core/device.c and function prototype in device.h, " #ifdef CONFIG_OF_CONTROL void *dev_reg_addr(struct udevice *dev) { fdt_addr_t addr; addr = fdtdev_get_addr(gd->fdt_blob, dev->of_offset, "reg"); if (addr == FDT_ADDR_T_NONE) return NULL; else return (void *)addr } #else void *dev_reg_addr(struct udevice *dev) { return NULL; } #endif " I think `#ifdef` is needed here. I think this way is better that put stub function in fdtdec.h. Using this way, the driver code can just `add = dev_reg_addr(device)` to get reg address. Maybe the upper piece code should be put in a new file named device-util.c in directory device/core but not device.c? Comments? >> + >> +static int mxc_gpio_bind(struct udevice *device) >> +{ >> + struct mxc_gpio_plat *plat = device->platdata; >> + struct gpio_regs *regs; >> + >> + if (plat) >> + return 0; >> + >> + regs = mxc_get_gpio_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) = { >> @@ -320,3 +372,4 @@ U_BOOT_DEVICES(mxc_gpios) = { >> #endif >> }; >> #endif >> +#endif >> Regards, Peng.