Re: [PATCH 4/4] serial/arc-uart: switch to devicetree based probing
On Sat, 9 Feb 2013 12:15:10 +0530, Vineet Gupta
wrote:
> On Saturday 09 February 2013 04:31 AM, Grant Likely wrote:
> > On Fri, 11 Jan 2013 11:50:23 +0530, Vineet Gupta
> > wrote:
> >> +- clock-frequency : the input clock frequency for the UART
> >> +- baud: baud rate for UART
> > change 'baud' to 'current-speed'. There is already precedence for this
> > with other serial devices.
>
> While I'm OK with this - I can only see of_serial.c following the rule :-)
> More importantly I'm not clear about the logistics of this fix. Obviously
> this has
> a bearing on DT files in arch/arc/boot/*. So are such changes (platform +
> driver)
> routed thru the subsystem tree or the arch tree or bits from both with
> bisectability not considered - which feels wrong. We have to also consider the
> fact that Greg has closed the tty/serial for 3.9. So while I have no
> objection to
> your comment, it seems that the it needs to wait till 3.9-rc1 - or is there an
> alternate way.
I would consider it a bug fix. The binding isn't what it should be and
it needs to be addressed before appearing in a released kernel. If this
patch has already been merged, then write and post a fixup patch.
> >
> >> @@ -673,8 +693,18 @@ static int __init arc_serial_probe_earlyprintk(struct
> >> platform_device *pdev)
> >> static int arc_serial_probe(struct platform_device *pdev)
> >> {
> >>int rc, dev_id;
> >> + struct device_node *np = pdev->dev.of_node;
> >> +
> >> + /* no device tree device */
> >> + if (!np)
> >> + return -ENODEV;
> > This breaks non-DT users. Is this what you intend? It creates a flag day
> > where users have to switch from non-DT to DT cold-turkey.
>
> Not supporting non-DT user was not the idea - it just simplifies the code a
> bit
> given that it would only even be runtime used in a ARC Linux port based
> platform -
> which unconditionally enables OF. Further - the ARC port itself is not yet
> upstream so there are no "official" user of this in tree driver.
> FWIW, ARC Linux port was recently reviewed on lkml/arch mailing lists and is
> now
> in linux-next for a possible 3.9 merge.
If there are no users, then nobody is broken. If this driver only
supports DT platforms then my comment can be ignored.
> >> + dev_id = of_alias_get_id(np, "serial");
> >> + if (dev_id < 0) {
> >> + dev_err(&pdev->dev, "failed to get alias id: %d\n", dev_id);
> >> + return dev_id;
> >> + }
> > Don't fail on this. If you can't get an id then choose one dynamically.
>
> You mean just assume 0.
No, I mean dynamically assign an ID from ids that are available. Say you
had two of these devices in a system, and neither had an alias; they
couldn't both be '0'. :-)
g.
>
> Thanks for reviewing.
> -Vineet
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] serial/arc-uart: switch to devicetree based probing
On Saturday 09 February 2013, Vineet Gupta wrote: > On Saturday 09 February 2013 04:31 AM, Grant Likely wrote: > > On Fri, 11 Jan 2013 11:50:23 +0530, Vineet Gupta > > wrote: > >> +- clock-frequency : the input clock frequency for the UART > >> +- baud : baud rate for UART > > change 'baud' to 'current-speed'. There is already precedence for this > > with other serial devices. > > While I'm OK with this - I can only see of_serial.c following the rule :-) > More importantly I'm not clear about the logistics of this fix. Obviously > this has > a bearing on DT files in arch/arc/boot/*. So are such changes (platform + > driver) > routed thru the subsystem tree or the arch tree or bits from both with > bisectability not considered - which feels wrong. We have to also consider the > fact that Greg has closed the tty/serial for 3.9. So while I have no > objection to > your comment, it seems that the it needs to wait till 3.9-rc1 - or is there an > alternate way. I'd consider this one a bug fix, so while Greg is not accepting any new features for the serial tree, I think it should still get in that way and should not be controversial as an add-on change. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] serial/arc-uart: switch to devicetree based probing
On Saturday 09 February 2013 04:31 AM, Grant Likely wrote:
> On Fri, 11 Jan 2013 11:50:23 +0530, Vineet Gupta
> wrote:
>> +- clock-frequency : the input clock frequency for the UART
>> +- baud : baud rate for UART
> change 'baud' to 'current-speed'. There is already precedence for this
> with other serial devices.
While I'm OK with this - I can only see of_serial.c following the rule :-)
More importantly I'm not clear about the logistics of this fix. Obviously this
has
a bearing on DT files in arch/arc/boot/*. So are such changes (platform +
driver)
routed thru the subsystem tree or the arch tree or bits from both with
bisectability not considered - which feels wrong. We have to also consider the
fact that Greg has closed the tty/serial for 3.9. So while I have no objection
to
your comment, it seems that the it needs to wait till 3.9-rc1 - or is there an
alternate way.
>
>> @@ -673,8 +693,18 @@ static int __init arc_serial_probe_earlyprintk(struct
>> platform_device *pdev)
>> static int arc_serial_probe(struct platform_device *pdev)
>> {
>> int rc, dev_id;
>> +struct device_node *np = pdev->dev.of_node;
>> +
>> +/* no device tree device */
>> +if (!np)
>> +return -ENODEV;
> This breaks non-DT users. Is this what you intend? It creates a flag day
> where users have to switch from non-DT to DT cold-turkey.
Not supporting non-DT user was not the idea - it just simplifies the code a bit
given that it would only even be runtime used in a ARC Linux port based
platform -
which unconditionally enables OF. Further - the ARC port itself is not yet
upstream so there are no "official" user of this in tree driver.
FWIW, ARC Linux port was recently reviewed on lkml/arch mailing lists and is now
in linux-next for a possible 3.9 merge.
>> +dev_id = of_alias_get_id(np, "serial");
>> +if (dev_id < 0) {
>> +dev_err(&pdev->dev, "failed to get alias id: %d\n", dev_id);
>> +return dev_id;
>> +}
> Don't fail on this. If you can't get an id then choose one dynamically.
You mean just assume 0.
Thanks for reviewing.
-Vineet
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] serial/arc-uart: switch to devicetree based probing
On Fri, 11 Jan 2013 11:50:23 +0530, Vineet Gupta
wrote:
> * DT binding for arc-uart
> * With alll the bits in place we can now use DT probing.
>
> Note that there's a bit of kludge right now because earlyprintk portion
> of driver can't use the DT infrastrcuture to get resoures/plat_data.
> This requires some infrastructre changes to of_flat_ framework
>
> Signed-off-by: Vineet Gupta
> Cc: Grant Likely
> Cc: Arnd Bergmann
> Cc: linux-ser...@vger.kernel.org
> Cc: Alan Cox
> Cc: Greg Kroah-Hartman
> Cc: devicetree-disc...@lists.ozlabs.org
> Cc: Rob Herring
> Cc: Rob Landley
> Cc: linux-ser...@vger.kernel.org
> ---
> .../devicetree/bindings/tty/serial/arc-uart.txt| 26
> drivers/tty/serial/arc_uart.c | 43 ++-
> 2 files changed, 66 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/tty/serial/arc-uart.txt
>
> diff --git a/Documentation/devicetree/bindings/tty/serial/arc-uart.txt
> b/Documentation/devicetree/bindings/tty/serial/arc-uart.txt
> new file mode 100644
> index 000..c3bd8f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty/serial/arc-uart.txt
> @@ -0,0 +1,26 @@
> +* Synopsys ARC UART : Non standard UART used in some of the ARC FPGA boards
> +
> +Required properties:
> +- compatible : "snps,arc-uart"
> +- reg: offset and length of the register set for the
> device.
> +- interrupts : device interrupt
> +- clock-frequency: the input clock frequency for the UART
> +- baud : baud rate for UART
change 'baud' to 'current-speed'. There is already precedence for this
with other serial devices.
g.
> @@ -673,8 +693,18 @@ static int __init arc_serial_probe_earlyprintk(struct
> platform_device *pdev)
> static int arc_serial_probe(struct platform_device *pdev)
> {
> int rc, dev_id;
> + struct device_node *np = pdev->dev.of_node;
> +
> + /* no device tree device */
> + if (!np)
> + return -ENODEV;
This breaks non-DT users. Is this what you intend? It creates a flag day
where users have to switch from non-DT to DT cold-turkey.
> + dev_id = of_alias_get_id(np, "serial");
> + if (dev_id < 0) {
> + dev_err(&pdev->dev, "failed to get alias id: %d\n", dev_id);
> + return dev_id;
> + }
Don't fail on this. If you can't get an id then choose one dynamically.
>
> - dev_id = pdev->id < 0 ? 0 : pdev->id;
> rc = arc_uart_init_one(pdev, dev_id);
> if (rc)
> return rc;
> @@ -689,12 +719,19 @@ static int arc_serial_remove(struct platform_device
> *pdev)
> return 0;
> }
>
> +static const struct of_device_id arc_uart_dt_ids[] = {
> + { .compatible = "snps,arc-uart" },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, arc_uart_dt_ids);
> +
> static struct platform_driver arc_platform_driver = {
> .probe = arc_serial_probe,
> .remove = arc_serial_remove,
> .driver = {
> .name = DRIVER_NAME,
> .owner = THIS_MODULE,
> + .of_match_table = arc_uart_dt_ids,
>},
> };
>
> --
> 1.7.4.1
>
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] serial/arc-uart: switch to devicetree based probing
On Friday 11 January 2013, Vineet Gupta wrote: > On Friday 11 January 2013 05:03 PM, Arnd Bergmann wrote: > > Quick question about the name though: is this UART only used > > on ARC, or is it something that synopsys licenses to other > > parties as well? If the latter is true, we might want to > > add a more generic "compatible" value for those that use > > it on another architecture. > > It's not licensed as standalone IP - since its not a standard 8250. It is only > (but actively) used on the internal FPGA flows from ARC days. > Ok, thanks for the confirmation. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] serial/arc-uart: switch to devicetree based probing
On Friday 11 January 2013 05:03 PM, Arnd Bergmann wrote: > On Friday 11 January 2013, Vineet Gupta wrote: >> * DT binding for arc-uart >> * With alll the bits in place we can now use DT probing. >> >> Note that there's a bit of kludge right now because earlyprintk portion >> of driver can't use the DT infrastrcuture to get resoures/plat_data. >> This requires some infrastructre changes to of_flat_ framework >> >> Signed-off-by: Vineet Gupta >> Cc: Grant Likely >> Cc: Arnd Bergmann >> Cc: linux-ser...@vger.kernel.org >> Cc: Alan Cox >> Cc: Greg Kroah-Hartman >> Cc: devicetree-disc...@lists.ozlabs.org >> Cc: Rob Herring >> Cc: Rob Landley >> Cc: linux-ser...@vger.kernel.org > Acked-by: Arnd Bergmann > > Quick question about the name though: is this UART only used > on ARC, or is it something that synopsys licenses to other > parties as well? If the latter is true, we might want to > add a more generic "compatible" value for those that use > it on another architecture. It's not licensed as standalone IP - since its not a standard 8250. It is only (but actively) used on the internal FPGA flows from ARC days. Thx, Vineet > Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] serial/arc-uart: switch to devicetree based probing
On Friday 11 January 2013, Vineet Gupta wrote: > * DT binding for arc-uart > * With alll the bits in place we can now use DT probing. > > Note that there's a bit of kludge right now because earlyprintk portion > of driver can't use the DT infrastrcuture to get resoures/plat_data. > This requires some infrastructre changes to of_flat_ framework > > Signed-off-by: Vineet Gupta > Cc: Grant Likely > Cc: Arnd Bergmann > Cc: linux-ser...@vger.kernel.org > Cc: Alan Cox > Cc: Greg Kroah-Hartman > Cc: devicetree-disc...@lists.ozlabs.org > Cc: Rob Herring > Cc: Rob Landley > Cc: linux-ser...@vger.kernel.org Acked-by: Arnd Bergmann Quick question about the name though: is this UART only used on ARC, or is it something that synopsys licenses to other parties as well? If the latter is true, we might want to add a more generic "compatible" value for those that use it on another architecture. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
