| Message ID | 20110621135558.GB9228@S2101-09.ap.freescale.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I wonder if it makes sense to create a new device node "/linux-devices" to express a desired mapping from device nodes to /dev entries? The properties could be the names of device special files and the values the corresponding node phandles. On 6/21/2011 3:55 AM, Shawn Guo 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. > > 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; > + }; > + > 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++; > + } > + > + 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; > > - 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); >
On Tue, Jun 21, 2011 at 12:32 PM, Mitch Bradley <wmb@firmworks.com> wrote:
> I wonder if it makes sense to create a new device node "/linux-devices" to express a desired mapping from device nodes to /dev entries? The properties could be the names of device special files and the values the corresponding node phandles.
I've been trying /really/ hard to avoid doing something like that
because a lot of the time the desired Linux dev name is a
implementation detail, and a potentially unstable one at that. If
Linux requires certain devices to have certain names because that is
how it hooks up clocks (which is the current situation on some
platforms), then I'd rather have Linux encode a lookup of the
preferred name, at least until the that particular implementation
detail goes away.
As for enumerating devices, I don't think this is a Linux-specific
thing. In this case it is entirely reasonable to want to say /this
node/ is the second serial port, and /that node/ is the third, which
is information needed regardless of the client OS.
g.
On Tue, Jun 21, 2011 at 12:42:14PM -0600, Grant Likely wrote: > On Tue, Jun 21, 2011 at 12:32 PM, Mitch Bradley <wmb@firmworks.com> wrote: > > I wonder if it makes sense to create a new device node "/linux-devices" to express a desired mapping from device nodes to /dev entries? The properties could be the names of device special files and the values the corresponding node phandles. > > I've been trying /really/ hard to avoid doing something like that > because a lot of the time the desired Linux dev name is a > implementation detail, and a potentially unstable one at that. If > Linux requires certain devices to have certain names because that is > how it hooks up clocks As I keep on saying, we don't _have_ to have to match on device name. If DT can come up with a better way to bind a clock to a particular device/connection name then DT can provide its own clk_get() implementation which does that. clk_get() has the struct device, and therefore can get at the DT information itself to read out whatever properties are required. But, we must not get away from the fact that clk_get()'s second argument should _never_ be used as a unique clock name itself. At the moment, until we do have some kind of DT based clk_get(), the easiest way to move clk-API using drivers over is to use the device name in the static clk_lookup tables. It's all an implementation detail, one which is (thankfully) hidden behind a proper API which will be _completely_ transparent to drivers using the clk API in the _proper_ way.
On Tue, Jun 21, 2011 at 12:53 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Jun 21, 2011 at 12:42:14PM -0600, Grant Likely wrote: >> On Tue, Jun 21, 2011 at 12:32 PM, Mitch Bradley <wmb@firmworks.com> wrote: >> > I wonder if it makes sense to create a new device node "/linux-devices" to express a desired mapping from device nodes to /dev entries? The properties could be the names of device special files and the values the corresponding node phandles. >> >> I've been trying /really/ hard to avoid doing something like that >> because a lot of the time the desired Linux dev name is a >> implementation detail, and a potentially unstable one at that. If >> Linux requires certain devices to have certain names because that is >> how it hooks up clocks > > As I keep on saying, we don't _have_ to have to match on device name. > If DT can come up with a better way to bind a clock to a particular > device/connection name then DT can provide its own clk_get() > implementation which does that. Sorry, I overstated the situation. My point is only that I don't want encode how Linux currently views the world into the DT, because implementation details can and do change. g.
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 >
On Tue, Jun 21, 2011 at 4:08 PM, Mitch Bradley <wmb@firmworks.com> wrote: > On 6/21/2011 9:38 AM, Grant Likely wrote: >> >> 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? > > > The point is that if you refer to the node explicitly by its string name, > the need for a label disappears and the problem of overriding a default > alias disappears (assuming that a later redefinition of a property takes > precedence over an earlier one, as is the OFW convention). Ah, we're having an impedance mismatch. I'm thinking specifically about the device tree compiler and some syntactic sugar for using the label definition to generate /also/ create alias properties. The hairiness is related to that and the way that dtc is implemented, not with the final aliases themselves. g.
> Ah, we're having an impedance mismatch. I'm thinking specifically > about the device tree compiler and some syntactic sugar for using the > label definition to generate /also/ create alias properties. The > hairiness is related to that and the way that dtc is implemented, not > with the final aliases themselves. You can generate DTC-style aliases from OFW-style aliases instead (or as well), it has other advantages (like being more readable, and having the aliases grouped together). Segher
On Tue, Jun 21, 2011 at 4:52 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote: >> Ah, we're having an impedance mismatch. I'm thinking specifically >> about the device tree compiler and some syntactic sugar for using the >> label definition to generate /also/ create alias properties. The >> hairiness is related to that and the way that dtc is implemented, not >> with the final aliases themselves. > > You can generate DTC-style aliases from OFW-style aliases instead (or > as well), it has other advantages (like being more readable, and having > the aliases grouped together). There is no difference between OFW and DTC aliases as far as I'm aware. g.
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@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? > 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.
On Wed, Jun 22, 2011 at 9:33 AM, Shawn Guo <shawn.guo@freescale.com> wrote: > 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@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? >> > Another dependency the patch has to wait for? Or we can go ahead and > utilize the facility later when it gets ready? No, this isn't something you need to wait for. Just musing on future enhancements. >> 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? heh, that happens sometimes. I tend to be a bit scattered when replying to email. Just ignore the last sentence fragment. >> > - 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. Are you sure? It doesn't look like the driver behaviour uses id for anything other than an index into the statically allocated serial port instance table. I don't see any change of behaviour based on the port number anywhere. g.
On Wed, Jun 22, 2011 at 09:52:11AM -0600, Grant Likely wrote: [...] > > >> > - 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. > > Are you sure? It doesn't look like the driver behaviour uses id for > anything other than an index into the statically allocated serial port > instance table. I don't see any change of behaviour based on the port > number anywhere. > Sorry, I did not make this clear. In serial_imx_probe(), the port gets created and then saved as below. imx_ports[sport->port.line] = sport; While in imx_console_setup(), it addresses the port as following. sport = imx_ports[co->index]; When users specify their console as ttymxc0, they mean they are using the first i.mx uart hardware port, in turn ttymxc1 for the second port ... That said, imx_port[0] has to be the first hardware port, imx_port[1] has to be the second one ... That's why port id sport->port.line can not be dynamically assigned, otherwise console may not work.
On Wed, Jun 22, 2011 at 6:12 PM, Shawn Guo <shawn.guo@freescale.com> wrote: > On Wed, Jun 22, 2011 at 09:52:11AM -0600, Grant Likely wrote: > [...] >> >> >> > - 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. >> >> Are you sure? It doesn't look like the driver behaviour uses id for >> anything other than an index into the statically allocated serial port >> instance table. I don't see any change of behaviour based on the port >> number anywhere. >> > Sorry, I did not make this clear. In serial_imx_probe(), the port > gets created and then saved as below. > > imx_ports[sport->port.line] = sport; > > While in imx_console_setup(), it addresses the port as following. > > sport = imx_ports[co->index]; > > When users specify their console as ttymxc0, they mean they are > using the first i.mx uart hardware port, in turn ttymxc1 for the > second port ... > > That said, imx_port[0] has to be the first hardware port, imx_port[1] > has to be the second one ... That's why port id sport->port.line > can not be dynamically assigned, otherwise console may not work. My point still stands. If there is no number specifically assigned to a device, then numbering should be dynamic. That's standard behaviour for Linux device drivers, and I get nervous every time I see a driver that make an assumption to the contrary, especially when there is no good reason for it. Besides, you still can ensure the console is reliable by having an alias for the console device. g.
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; + }; + 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++; + } + + 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; - 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);