Re: [PATCH 1/3] serial/imx: add device tree support

[Grant Likely <grant.likely@secretlab.ca>]

On Tue, Jun 21, 2011 at 1:35 PM, Mitch Bradley <wmb@firmworks.com> wrote:
> What is the problem with
>
> aliases{
>   serial0 = "/uart@7000c000";
> }
>
> Properties in the alias node are supposed to have string values.

?

Not sure I follow.  Indeed, properties in the aliases node are string values.

Are you referring to how I was proposing some dtc syntax for
generating the alias strings?

g.

>
>
> On 6/21/2011 9:13 AM, Grant Likely wrote:
>>
>> On Tue, Jun 21, 2011 at 7:55 AM, Shawn Guo<shawn.guo@freescale.com>
>>  wrote:
>>>
>>> Hi Grant,
>>>
>>> I just gave a try to use aliases node for identify the device index.
>>> Please take a look and let me know if it's what you expect.
>>
>> Thanks Shawn.  This is definitely on track.  Comments below...
>>
>>>
>>> On Sun, Jun 19, 2011 at 03:30:02PM +0800, Shawn Guo wrote:
>>> [...]
>>>>>>
>>>>>> +#ifdef CONFIG_OF
>>>>>> +static int serial_imx_probe_dt(struct imx_port *sport,
>>>>>> +         struct platform_device *pdev)
>>>>>> +{
>>>>>> + struct device_node *node = pdev->dev.of_node;
>>>>>> + const __be32 *line;
>>>>>> +
>>>>>> + if (!node)
>>>>>> +         return -ENODEV;
>>>>>> +
>>>>>> + line = of_get_property(node, "id", NULL);
>>>>>> + if (!line)
>>>>>> +         return -ENODEV;
>>>>>> +
>>>>>> + sport->port.line = be32_to_cpup(line) - 1;
>>>>>
>>>>> Hmmm, I really would like to be rid of this.  Instead, if uarts must
>>>>> be enumerated, the driver should look for a /aliases/uart* property
>>>>> that matches the of_node.  Doing it that way is already established in
>>>>> the OpenFirmware documentation, and it ensures there are no overlaps
>>>>> in the global namespace.
>>>>>
>>>>
>>>> I just gave one more try to avoid using 'aliases', and you gave a
>>>> 'no' again.  Now, I know how hard you are on this.  Okay, I start
>>>> thinking about your suggestion seriously :)
>>>>
>>>>> We do need some infrastructure to make that easier though.  Would you
>>>>> have time to help put that together?
>>>>>
>>>> Ok, I will give it a try.
>>>>
>>>
>>> diff --git a/arch/arm/boot/dts/imx51-babbage.dts
>>> b/arch/arm/boot/dts/imx51-babbage.dts
>>> index da0381a..f4a5c3c 100644
>>> --- a/arch/arm/boot/dts/imx51-babbage.dts
>>> +++ b/arch/arm/boot/dts/imx51-babbage.dts
>>> @@ -18,6 +18,12 @@
>>>        compatible = "fsl,imx51-babbage", "fsl,imx51";
>>>        interrupt-parent =<&tzic>;
>>>
>>> +       aliases {
>>> +               serial0 =&uart0;
>>> +               serial1 =&uart1;
>>> +               serial2 =&uart2;
>>> +       };
>>> +
>>
>> Hmmm.  David Gibson had tossed out an idea of automatically generating
>> aliases from labels.  It may be time to revisit that idea.
>>
>> David, perhaps using this format for a label should turn it into an
>> alias (prefix label with an asterisk):
>>
>>         *thealias: i2c@12340000 { /*...*/ };
>>
>> .... although that approach gets *really* hairy when considering that
>> different boards will want different enumeration.  How would one
>> override an automatic alias defined by an included .dts file?
>>
>>>        chosen {
>>>                bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3
>>> rootwait";
>>>        };
>>> @@ -47,29 +53,29 @@
>>>                        reg =<0x70000000 0x40000>;
>>>                        ranges;
>>>
>>> -                       uart@7000c000 {
>>> +                       uart2: uart@7000c000 {
>>>                                compatible = "fsl,imx51-uart",
>>> "fsl,imx21-uart";
>>>                                reg =<0x7000c000 0x4000>;
>>>                                interrupts =<33>;
>>>                                id =<3>;
>>> -                               fsl,has-rts-cts;
>>> +                               fsl,uart-has-rtscts;
>>>                        };
>>>                };
>>>
>>> -               uart@73fbc000 {
>>> +               uart0: uart@73fbc000 {
>>>                        compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>>>                        reg =<0x73fbc000 0x4000>;
>>>                        interrupts =<31>;
>>>                        id =<1>;
>>> -                       fsl,has-rts-cts;
>>> +                       fsl,uart-has-rtscts;
>>>                };
>>>
>>> -               uart@73fc0000 {
>>> +               uart1: uart@73fc0000 {
>>>                        compatible = "fsl,imx51-uart", "fsl,imx21-uart";
>>>                        reg =<0x73fc0000 0x4000>;
>>>                        interrupts =<32>;
>>>                        id =<2>;
>>> -                       fsl,has-rts-cts;
>>> +                       fsl,uart-has-rtscts;
>>>                };
>>>        };
>>>
>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>> index 632ebae..13df5d2 100644
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>> @@ -737,6 +737,37 @@ err0:
>>>  EXPORT_SYMBOL(of_parse_phandles_with_args);
>>>
>>>  /**
>>> + *     of_get_device_index - Get device index by looking up "aliases"
>>> node
>>> + *     @np:    Pointer to device node that asks for device index
>>> + *     @name:  The device alias without index number
>>> + *
>>> + *     Returns the device index if find it, else returns -ENODEV.
>>> + */
>>> +int of_get_device_index(struct device_node *np, const char *alias)
>>> +{
>>> +       struct device_node *aliases = of_find_node_by_name(NULL,
>>> "aliases");
>>> +       struct property *prop;
>>> +       char name[32];
>>> +       int index = 0;
>>> +
>>> +       if (!aliases)
>>> +               return -ENODEV;
>>> +
>>> +       while (1) {
>>> +               snprintf(name, sizeof(name), "%s%d", alias, index);
>>> +               prop = of_find_property(aliases, name, NULL);
>>> +               if (!prop)
>>> +                       return -ENODEV;
>>> +               if (np == of_find_node_by_path(prop->value))
>>> +                       break;
>>> +               index++;
>>> +       }
>>
>> Rather than parsing the alias strings everytime, it would probably be
>> better to preprocess all the properties in the aliases node and create
>> a lookup table of alias->node references that can be walked quickly
>> and trivially.
>>
>> Also, when obtaining an enumeration for a device, you'll need to be
>> careful about what number gets returned.  If the node doesn't match a
>> given alias, but aliases do exist for other devices of like type, then
>> you need to be careful not to assign a number already assigned to
>> another device via an alias (this of course assumes the driver
>> supports dynamics enumeration, which many drivers will).  It would be
>>
>> \>  +
>>>
>>> +       return index;
>>> +}
>>> +EXPORT_SYMBOL(of_get_device_index);
>>> +
>>> +/**
>>>  * prom_add_property - Add a property to a node
>>>  */
>>>  int prom_add_property(struct device_node *np, struct property *prop)
>>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>>> index da436e0..852668f 100644
>>> --- a/drivers/tty/serial/imx.c
>>> +++ b/drivers/tty/serial/imx.c
>>> @@ -1271,18 +1271,18 @@ static int serial_imx_probe_dt(struct imx_port
>>> *sport,
>>>        struct device_node *node = pdev->dev.of_node;
>>>        const struct of_device_id *of_id =
>>>                        of_match_device(imx_uart_dt_ids,&pdev->dev);
>>> -       const __be32 *line;
>>> +       int line;
>>>
>>>        if (!node)
>>>                return -ENODEV;
>>>
>>> -       line = of_get_property(node, "id", NULL);
>>> -       if (!line)
>>> +       line = of_get_device_index(node, "serial");
>>> +       if (IS_ERR_VALUE(line))
>>>                return -ENODEV;
>>
>> Personally, it an alias isn't present, then I'd dynamically assign a port
>> id.
>>
>>>
>>> -       sport->port.line = be32_to_cpup(line) - 1;
>>> +       sport->port.line = line;
>>>
>>> -       if (of_get_property(node, "fsl,has-rts-cts", NULL))
>>> +       if (of_get_property(node, "fsl,uart-has-rtscts", NULL))
>>>                sport->have_rtscts = 1;
>>>
>>>        if (of_get_property(node, "fsl,irda-mode", NULL))
>>> diff --git a/include/linux/of.h b/include/linux/of.h
>>> index bfc0ed1..3153752 100644
>>> --- a/include/linux/of.h
>>> +++ b/include/linux/of.h
>>> @@ -213,6 +213,8 @@ extern int of_parse_phandles_with_args(struct
>>> device_node *np,
>>>        const char *list_name, const char *cells_name, int index,
>>>        struct device_node **out_node, const void **out_args);
>>>
>>> +extern int of_get_device_index(struct device_node *np, const char
>>> *alias);
>>> +
>>>  extern int of_machine_is_compatible(const char *compat);
>>>
>>>  extern int prom_add_property(struct device_node* np, struct property*
>>> prop);
>>>
>>> --
>>> Regards,
>>> Shawn
>>>
>>>
>>
>>
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.