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

From: Shawn Guo
Date: Wed Jun 22 2011 - 11:28:15 EST


On Tue, Jun 21, 2011 at 01:13:50PM -0600, Grant Likely wrote:
> On Tue, Jun 21, 2011 at 7:55 AM, Shawn Guo <shawn.guo@xxxxxxxxxxxxx> 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?
>
Another dependency the patch has to wait for? Or we can go ahead and
utilize the facility later when it gets ready?

> >        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.
>
Ok, I'm thinking about it. Will probably ask something on details
later.

> 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
>
Some words not finished?

> \> +
> > +       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.
>
We probably can not. The driver works with being told the correct
port id which is defined by soc. Instead of dynamically assigning
a port id, we have to tell the driver the exact hardware port id of
the device that is being probed.

--
Regards,
Shawn

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/