[U-Boot] [PATCH v3] Exynos5: Pinmux: Add fdt for pinmux
Simon Glass
sjg at chromium.org
Thu Feb 28 02:08:57 CET 2013
Hi Akshay,
On Mon, Feb 25, 2013 at 10:04 AM, Akshay Saraswat <akshay.s at samsung.com> wrote:
> Hi Simon,
>
>>Hi Akshay,
>>
>>On Thu, Feb 21, 2013 at 11:05 AM, Akshay Saraswat <akshay.s at 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 <akshay.s at 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
More information about the U-Boot
mailing list