Hi Akshay, On Mon, Feb 25, 2013 at 10:04 AM, Akshay Saraswat <aksha...@samsung.com> wrote: > Hi Simon, > >>Hi Akshay, >> >>On Thu, Feb 21, 2013 at 11:05 AM, Akshay Saraswat <aksha...@samsung.com> >>wrote: >>> This patch adds fdt nodes for peripherals which require >>> pin muxing and configuration. Device tree bindings for pinctrl >>> are kept same as required for Linux. Existing pinmux code >>> modified to retrieve gpio range and function related info from fdt. >>> >>> Depends-on: [U-Boot] [PATCH 0/4 V3] EXYNOS5: Add GPIO numbering feature >>> URL: http://lists.denx.de/pipermail/u-boot/2013-February/146151.html >>> >>> Signed-off-by: Akshay Saraswat <aksha...@samsung.com> >>> --- >>> Changes since v2: >>> - Rebased as per new version of GPIO numbering patch-set. >> >>This looks pretty reasonable to me. Please find some comment below. >> >>As a general comment, it seems to read from the FDT each time anything >>is changed. I suppose that is efficient on space, and allows it to >>work prior to malloc() being available, but isn't it slow? Perhaps the >>data should be cached? >> > > Malloc is not present initially with us which you have already highlighted, > adding to that we dont use same value more than once. > Hence, I found it better to stick with FDT reading instead of creating a > linked list of all nodes. > Please suggest a location where we shall create the list if we need to > maintain the cache.
I think this is OK for a start. > >>> >>> arch/arm/cpu/armv7/exynos/pinmux.c | 379 +++++++++++---- >>> arch/arm/dts/exynos5250-pinctrl.dtsi | 675 >>> ++++++++++++++++++++++++++ >>> arch/arm/dts/exynos5250.dtsi | 1 + >>> doc/device-tree-bindings/samsung-pinctrl.txt | 253 ++++++++++ >>> include/fdtdec.h | 1 + >>> lib/fdtdec.c | 1 + >>> 6 files changed, 1204 insertions(+), 106 deletions(-) >>> create mode 100644 arch/arm/dts/exynos5250-pinctrl.dtsi >>> create mode 100644 doc/device-tree-bindings/samsung-pinctrl.txt >>> >>> diff --git a/arch/arm/cpu/armv7/exynos/pinmux.c >>> b/arch/arm/cpu/armv7/exynos/pinmux.c >>> index a01ce0c..f4dccad 100644 >>> --- a/arch/arm/cpu/armv7/exynos/pinmux.c >>> +++ b/arch/arm/cpu/armv7/exynos/pinmux.c >>> @@ -27,6 +27,18 @@ >>> #include <asm/arch/pinmux.h> >>> #include <asm/arch/sromc.h> >>> >>> +DECLARE_GLOBAL_DATA_PTR; >>> + >>> +/* Struct for storing pin function and gpio related info */ >>> +struct pin_group { >>> + void *dev_name; >> >>What is this for? > > This is the device name to search for corresponding node. > Since we cannot declare a compatible property for every node, > so I used names to search for nodes. OK I see - the comment help thank you. > >> >>> + int npins; >>> + int function; >>> + int pull_mode; >>> + int drive_strength; >> >>Should add comments about the S5P_GPIO... defines to use in each case. >> >>> + enum exynos5_gpio_pin gpio[]; >> >>This array has no members? > > This is a dynamic array because we do not know exact number of pins in every > node. > >> >> >>> +}; >>> + >>> struct gpio_name_num_table exynos5_gpio_table[] = { >>> { 'a', GPIO_A00 }, >>> { 'b', GPIO_B00 }, >>> @@ -42,87 +54,202 @@ struct gpio_name_num_table exynos5_gpio_table[] = { >>> { 'z', GPIO_Z0 }, >>> }; >>> >>> -static void exynos5_uart_config(int peripheral) >>> +static void get_pins(const struct fdt_property *fprop, struct pin_group >>> *pingrp) >>> { >>> - int i, start, count; >>> + int i; >>> + char gpio[5]; >>> + >>> + pingrp->npins = 0; >>> + >>> + for (i = 0; !(fprop->data[i] == (int)NULL && >>> + fprop->data[i-1] == (int)NULL); i += 7) { >> >>This is a bit obtuse - please add a comment >> >>> + int pin_num = -1; >>> + >>> + gpio[0] = fprop->data[i]; >>> + gpio[1] = fprop->data[i + 1]; >>> + gpio[2] = fprop->data[i + 2]; >>> + gpio[3] = fprop->data[i + 3]; >>> + gpio[4] = fprop->data[i + 5]; >> >>What is this doing? Please add a comment. >> >>> + >>> + pin_num = name_to_gpio(gpio); >>> + >>> + if (pin_num >= 0) { >>> + pingrp->gpio[pingrp->npins] = pin_num; >> >>This seems to be assigning to something with no members. > > Actually I tried to make a dynamic array because we dont know the exact > number of pins for every node. > Please tell me if this introduces any bug. I'll try to figure some other > solution. > >> >>> + pingrp->npins++; >>> + } >>> + } >>> +} >>> + >>> +static int get_fdt_values(struct pin_group *pingrp) >> >>Please add a function comment. >> >>> +{ >>> + int i; >>> + int node, subnode; >>> + const struct fdt_property *fprop; >>> + >>> + for (i = 0; i < 4; i++) { >> >>Why 4? I think you should just continue until node returns -ve. >> >>> + /* Get the node from FDT for pinctrl */ >>> + node = fdtdec_next_compatible(gd->fdt_blob, 0, >>> + COMPAT_SAMSUNG_PINCTRL); >>> + if (node < 0) { >>> + printf("PINCTRL: No node for pinctrl in device >>> tree\n"); >> >>debug() >> >>Only give this error if you don't find any match >> >>> + return -1; >>> + } >>> + >>> + subnode = fdt_subnode_offset(gd->fdt_blob, >>> + node, pingrp->dev_name); >>> + if (subnode < 0) >>> + continue; >>> + >>> + fprop = fdt_get_property(gd->fdt_blob, >>> + subnode, "samsung,pins", NULL); >> >>Suggest you use fdt_getprop() here. > > I tried that earlier. Both use fdt_get_property_namelen underneath. "prop" > check in case of > fdt_getprop_namelen hinders boot instead of the fact that pointer is returned > everytime > in case of fdt_get_property. So I used fdt_get_property. Sorry I don't understand this. fdt_getprop() returns a pointer to fprop->data, so you can just check for NULL Here. > >> >>> + pingrp->function = fdtdec_get_int(gd->fdt_blob, >>> + subnode, "samsung,pin-function", >>> -1); >>> + pingrp->pull_mode = fdtdec_get_int(gd->fdt_blob, >>> + subnode, "samsung,pin-pud", -1); >>> + pingrp->drive_strength = fdtdec_get_int(gd->fdt_blob, >>> + subnode, "samsung,pin-drv", -1); >> >>Error checking on these? Is -1 a valid value? >> >>> + get_pins(fprop, pingrp); >>> + >>> + return 0; >>> + } >>> + >>> + debug("PINCTRL: No subnode for %s\n", (char *)pingrp->dev_name); >> >>Can dev_name be const char *? >> >>> + >>> + return -1; >>> +} >>> + >>> +static void pin_config_group_set(struct pin_group *pingrp) >>> +{ >>> + int i; >>> + >>> + for (i = 0; i < pingrp->npins; i++) { >>> + gpio_cfg_pin(pingrp->gpio[i], >>> S5P_GPIO_FUNC(pingrp->function)); >>> + gpio_set_pull(pingrp->gpio[i], pingrp->pull_mode); >>> + gpio_set_drv(pingrp->gpio[i], pingrp->drive_strength); >>> + } >>> +} >>> + >>> +static int exynos5_uart_config(int peripheral) >>> +{ >>> + char *data, *fctl = NULL; >>> + struct pin_group pingrp; >>> >>> switch (peripheral) { >>> case PERIPH_ID_UART0: >>> - start = GPIO_A00; >>> - count = 4; >>> + data = "uart0-data"; >>> + fctl = "uart0-fctl"; >>> break; >>> case PERIPH_ID_UART1: >>> - start = GPIO_D00; >>> - count = 4; >>> + data = "uart1-data"; >>> + fctl = "uart1-fctl"; >>> break; >>> case PERIPH_ID_UART2: >>> - start = GPIO_A10; >>> - count = 4; >>> + data = "uart2-data"; >>> + fctl = "uart2-fctl"; >>> break; >>> case PERIPH_ID_UART3: >>> - start = GPIO_A14; >>> - count = 2; >>> + data = "uart3-data"; >>> break; >>> } >>> - for (i = start; i < start + count; i++) { >>> - gpio_set_pull(i, S5P_GPIO_PULL_NONE); >>> - gpio_cfg_pin(i, S5P_GPIO_FUNC(0x2)); >>> + >>> + pingrp.dev_name = data; >>> + >>> + /* >>> + * Retrieve and apply pin config details >>> + * from fdt for this UART's data line. >>> + */ >>> + if (get_fdt_values(&pingrp)) >>> + return -1; >>> + >>> + pin_config_group_set(&pingrp); >> >>Suggest you add a function which does both of thees things - i.e. >>get_fdt_values() and pin_config_group_set(). >> >>Also probably should have a pinmux_ prefix on the latter, given the >>name of this file. > > We need to separate these functions, because on the basis of flags we need to > increment pin numbers at times. > Like in SROM we do it for CS1. I think you can still keep the separate functions, but create a new one in addition which does both things. You mostly call the two together. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot