On Wednesday 08 May 2013, Srinivas KANDAGATLA wrote:
> From: Srinivas Kandagatla <[email protected]>
> +*st-asc(Serial Port)
> +
> +Required properties:
> +- compatible : Should be "st,asc".
Are there any hardware revision numbers for the asc? If there are potentially
incompatible or backwards-compatible variants, it would be good to include
the version in this string.
> +- reg, reg-names, interrupts, interrupt-names : Standard way to define device
> + resources with names. look in
> + Documentation/devicetree/bindings/resource-names.txt
> +
> +Optional properties:
> +- st,hw-flow-ctrl bool flag to enable hardware flow control.
> +- st,force-m1 bool flat to force asc to be in Mode-1 recommeded
> + for high bit rates (above 19.2K)
> +Example:
> +serial@fe440000{
> + compatible = "st,asc";
> + reg = <0xfe440000 0x2c>;
> + interrupts = <0 209 0>;
> +};
I would also recommed adding a way to set the default baud rate through
a property. Following the example of the 8250 driver, you should probably
call that "current-speed".
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 7e7006f..346f325 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1484,6 +1484,25 @@ config SERIAL_RP2_NR_UARTS
> If multiple cards are present, the default limit of 32 ports may
> need to be increased.
>
> +config SERIAL_ST_ASC
> + tristate "ST ASC serial port support"
> + depends on PLAT_STIXXXX
> + default y
> + select SERIAL_CORE
> + help
> + This driver is for the on-chip Asychronous Serial Controller on
> + STMicroelectronics STixxxx SoCs.
> + ASC is embedded in ST COMMS IP block. It supports Rx & Tx functionality.
> + It support all industry standard baud rates.
> +
> + If unsure, say N.
I would not make it "default y".
> +config SERIAL_ST_ASC_CONSOLE
> + bool "Support for console on ST ASC"
> + depends on SERIAL_ST_ASC
> + default y
> + select SERIAL_CORE_CONSOLE
> +
> endmenu
This needs to be "depends on SERIAL_ST_ASC=y". You would get a link error
if you try to make SERIAL_ST_ASC a loadable module and SERIAL_ST_ASC_CONSOLE
built-in.
> +
> +static struct asc_port asc_ports[ASC_MAX_PORTS];
> +static struct uart_driver asc_uart_driver;
> +
> +/*---- Forward function declarations---------------------------*/
> +static irqreturn_t asc_interrupt(int irq, void *ptr);
> +static void asc_transmit_chars(struct uart_port *);
> +static int asc_set_baud(struct asc_port *ascport, int baud);
> +
Please remove all forward declarations, by reordering the functions in
the way they are called.
> diff --git a/drivers/tty/serial/st-asc.h b/drivers/tty/serial/st-asc.h
> new file mode 100644
> index 0000000..e59f818
> --- /dev/null
> +++ b/drivers/tty/serial/st-asc.h
> +#ifndef _ST_ASC_H
> +#define _ST_ASC_H
> +
> +#include <linux/serial_core.h>
> +#include <linux/clk.h>
> +
> +struct asc_port {
> + struct uart_port port;
> + struct clk *clk;
> + unsigned int hw_flow_control:1;
> + unsigned int check_parity:1;
> + unsigned int force_m1:1;
> +};
Since this header file is only used in one place, just merge it into
the driver itself.
> +#define ASC_MAJOR 204
> +#define ASC_MINOR_START 40
I don't know what the current policy is on allocating major/minor numbers,
but I'm sure you cannot just reuse one that is already used.
Documentation/devices.txt lists the ones that are officially assigned.
Can't you use dynamic allocation here?
Arnd
On 16:34 Wed 08 May , Arnd Bergmann wrote:
> On Wednesday 08 May 2013, Srinivas KANDAGATLA wrote:
> > From: Srinivas Kandagatla <[email protected]>
>
> > +*st-asc(Serial Port)
> > +
> > +Required properties:
> > +- compatible : Should be "st,asc".
>
> Are there any hardware revision numbers for the asc? If there are potentially
> incompatible or backwards-compatible variants, it would be good to include
> the version in this string.
The st,asc come from st20 or st200 IIRC which was not ARM but a ST arch
and then used on st40 series
an the st,asc for st200 and st40 are not compatible completly
>
> > +- reg, reg-names, interrupts, interrupt-names : Standard way to define device
> > + resources with names. look in
> > + Documentation/devicetree/bindings/resource-names.txt
> > +
> > +Optional properties:
> > +- st,hw-flow-ctrl bool flag to enable hardware flow control.
> > +- st,force-m1 bool flat to force asc to be in Mode-1 recommeded
> > + for high bit rates (above 19.2K)
> > +Example:
> > +serial@fe440000{
> > + compatible = "st,asc";
> > + reg = <0xfe440000 0x2c>;
> > + interrupts = <0 209 0>;
> > +};
>
> I would also recommed adding a way to set the default baud rate through
> a property. Following the example of the 8250 driver, you should probably
> call that "current-speed".
on st it has always be 38k
>
> > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> > index 7e7006f..346f325 100644
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -1484,6 +1484,25 @@ config SERIAL_RP2_NR_UARTS
> > If multiple cards are present, the default limit of 32 ports may
> > need to be increased.
> >
> > +config SERIAL_ST_ASC
> > + tristate "ST ASC serial port support"
> > + depends on PLAT_STIXXXX
> > + default y
> > + select SERIAL_CORE
> > + help
> > + This driver is for the on-chip Asychronous Serial Controller on
> > + STMicroelectronics STixxxx SoCs.
> > + ASC is embedded in ST COMMS IP block. It supports Rx & Tx functionality.
> > + It support all industry standard baud rates.
> > +
> > + If unsure, say N.
>
> I would not make it "default y".
me too
>
> > +config SERIAL_ST_ASC_CONSOLE
> > + bool "Support for console on ST ASC"
> > + depends on SERIAL_ST_ASC
> > + default y
> > + select SERIAL_CORE_CONSOLE
> > +
> > endmenu
>
> This needs to be "depends on SERIAL_ST_ASC=y". You would get a link error
> if you try to make SERIAL_ST_ASC a loadable module and SERIAL_ST_ASC_CONSOLE
> built-in.
>
> > +
> > +static struct asc_port asc_ports[ASC_MAX_PORTS];
> > +static struct uart_driver asc_uart_driver;
> > +
> > +/*---- Forward function declarations---------------------------*/
> > +static irqreturn_t asc_interrupt(int irq, void *ptr);
> > +static void asc_transmit_chars(struct uart_port *);
> > +static int asc_set_baud(struct asc_port *ascport, int baud);
> > +
>
> Please remove all forward declarations, by reordering the functions in
> the way they are called.
>
and drop the dummy functions
>
> > diff --git a/drivers/tty/serial/st-asc.h b/drivers/tty/serial/st-asc.h
> > new file mode 100644
> > index 0000000..e59f818
> > --- /dev/null
> > +++ b/drivers/tty/serial/st-asc.h
> > +#ifndef _ST_ASC_H
> > +#define _ST_ASC_H
> > +
> > +#include <linux/serial_core.h>
> > +#include <linux/clk.h>
> > +
> > +struct asc_port {
> > + struct uart_port port;
> > + struct clk *clk;
> > + unsigned int hw_flow_control:1;
> > + unsigned int check_parity:1;
> > + unsigned int force_m1:1;
> > +};
>
> Since this header file is only used in one place, just merge it into
> the driver itself.
>
> > +#define ASC_MAJOR 204
> > +#define ASC_MINOR_START 40
>
> I don't know what the current policy is on allocating major/minor numbers,
> but I'm sure you cannot just reuse one that is already used.
>
> Documentation/devices.txt lists the ones that are officially assigned.
> Can't you use dynamic allocation here?
>
> Arnd
> _______________________________________________
> devicetree-discuss mailing list
> [email protected]
> https://lists.ozlabs.org/listinfo/devicetree-discuss
Thankyou for the comments.
On 08/05/13 15:34, Arnd Bergmann wrote:
> On Wednesday 08 May 2013, Srinivas KANDAGATLA wrote:
>> From: Srinivas Kandagatla <[email protected]>
>> +*st-asc(Serial Port)
>> +
>> +Required properties:
>> +- compatible : Should be "st,asc".
> Are there any hardware revision numbers for the asc? If there are potentially
> incompatible or backwards-compatible variants, it would be good to include
> the version in this string.
Unfortunately, there is no version numbering for this IP.
>
>> +- reg, reg-names, interrupts, interrupt-names : Standard way to define device
>> + resources with names. look in
>> + Documentation/devicetree/bindings/resource-names.txt
>> +
>> +Optional properties:
>> +- st,hw-flow-ctrl bool flag to enable hardware flow control.
>> +- st,force-m1 bool flat to force asc to be in Mode-1 recommeded
>> + for high bit rates (above 19.2K)
>> +Example:
>> +serial@fe440000{
>> + compatible = "st,asc";
>> + reg = <0xfe440000 0x2c>;
>> + interrupts = <0 209 0>;
>> +};
> I would also recommed adding a way to set the default baud rate through
> a property. Following the example of the 8250 driver, you should probably
> call that "current-speed".
Yes, I will add this in the next version.
>
>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>> index 7e7006f..346f325 100644
>> --- a/drivers/tty/serial/Kconfig
>> +++ b/drivers/tty/serial/Kconfig
>> @@ -1484,6 +1484,25 @@ config SERIAL_RP2_NR_UARTS
>> If multiple cards are present, the default limit of 32 ports may
>> need to be increased.
>>
>> +config SERIAL_ST_ASC
>> + tristate "ST ASC serial port support"
>> + depends on PLAT_STIXXXX
>> + default y
>> + select SERIAL_CORE
>> + help
>> + This driver is for the on-chip Asychronous Serial Controller on
>> + STMicroelectronics STixxxx SoCs.
>> + ASC is embedded in ST COMMS IP block. It supports Rx & Tx functionality.
>> + It support all industry standard baud rates.
>> +
>> + If unsure, say N.
> I would not make it "default y".
Yep.
>> +config SERIAL_ST_ASC_CONSOLE
>> + bool "Support for console on ST ASC"
>> + depends on SERIAL_ST_ASC
>> + default y
>> + select SERIAL_CORE_CONSOLE
>> +
>> endmenu
> This needs to be "depends on SERIAL_ST_ASC=y". You would get a link error
> if you try to make SERIAL_ST_ASC a loadable module and SERIAL_ST_ASC_CONSOLE
> built-in.
Ok, got it.
>
>> +
>> +static struct asc_port asc_ports[ASC_MAX_PORTS];
>> +static struct uart_driver asc_uart_driver;
>> +
>> +/*---- Forward function declarations---------------------------*/
>> +static irqreturn_t asc_interrupt(int irq, void *ptr);
>> +static void asc_transmit_chars(struct uart_port *);
>> +static int asc_set_baud(struct asc_port *ascport, int baud);
>> +
> Please remove all forward declarations, by reordering the functions in
> the way they are called.
Will do.
>
>
>> diff --git a/drivers/tty/serial/st-asc.h b/drivers/tty/serial/st-asc.h
>> new file mode 100644
>> index 0000000..e59f818
>> --- /dev/null
>> +++ b/drivers/tty/serial/st-asc.h
>> +#ifndef _ST_ASC_H
>> +#define _ST_ASC_H
>> +
>> +#include <linux/serial_core.h>
>> +#include <linux/clk.h>
>> +
>> +struct asc_port {
>> + struct uart_port port;
>> + struct clk *clk;
>> + unsigned int hw_flow_control:1;
>> + unsigned int check_parity:1;
>> + unsigned int force_m1:1;
>> +};
> Since this header file is only used in one place, just merge it into
> the driver itself.
Ok, will do it.
>
>> +#define ASC_MAJOR 204
>> +#define ASC_MINOR_START 40
> I don't know what the current policy is on allocating major/minor numbers,
> but I'm sure you cannot just reuse one that is already used.
>
> Documentation/devices.txt lists the ones that are officially assigned.
> Can't you use dynamic allocation here?
>
> Arnd
>
On 08/05/13 15:39, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> +
>> >
>> > Please remove all forward declarations, by reordering the functions in
>> > the way they are called.
>> >
> and drop the dummy functions
We can not remove the dummy functions, as the serial-core does not check
it before using them.
>> >
>>> > > diff --git a/drivers/tty/serial/st-asc.h b/drivers/tty/serial/st-asc.h
>>> > > new file mode 100644
>>> > > index 0000000..e59f818
>>> > > --- /dev/null
>>> > > +++ b/drivers/tty/serial/st-asc.h
On Wednesday 08 May 2013, Srinivas KANDAGATLA wrote:
>
> On 08/05/13 15:39, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >>> +
> >> >
> >> > Please remove all forward declarations, by reordering the functions in
> >> > the way they are called.
> >> >
> > and drop the dummy functions
>
> We can not remove the dummy functions, as the serial-core does not check
> it before using them.
None of them seem performance critical, so it sounds reasonable to change
this in the core so you don't have to provide them. It's up to Greg to decide
whether he'd like that change or not.
Arnd
Hi Arnd,
We have pretty much completed reworking the patch set we sent recently, but
there is one comment you made which seemed to make perfect sense
but after investigating it has left me totally confused, which was:
>I would also recommed adding a way to set the default baud rate through
> a property. Following the example of the 8250 driver, you should probably
> call that "current-speed".
I note that you are listed as the author of of_serial.c so I was assuming
that looking at this would make sense, but it doesn't at all, and I would
be really grateful if you could explain what you were trying to achieve
and how you thought it worked. The code does:
/* If current-speed was set, then try not to change it. */
if (of_property_read_u32(np, "current-speed", &spd) == 0)
port->custom_divisor = clk / (16 * spd);
The "spd" variable is not used again and I do not understand why you
thought setting port->custom_divisor would have any impact on the default
or current baud rate of the UART, or that this information was useful to
any other part of the system. A search has only revealed that this port
field has only one purpose, which is to provide a "custom" speed
(unsurprisingly) when the user requests 38.4K _and_ the port flag
UPF_SPD_CUST has been set through a TIOCSSERIAL ioctl call, instead of
actually getting 38.4K. The key part of that assessment being the
implementation of uart_get_divisor() :
if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST)
quot = port->custom_divisor;
else
quot = DIV_ROUND_CLOSEST(port->uartclk, 16 * baud);
Also all of the custom_divisor functionality is basically commented as "old"
or has a kernel warning saying it is deprecated (see uart_set_info), so as
far as I can see for our (and I suspect most) hardware it is completely
irrelevant functionality.
If you really wanted to specify a default rate for a TTY then surely you need
to change the driver's init_termios, or change the termios on a per port basis
after they get registered. Only a handful of drivers do not use the default
uart_register_driver() and change the init_termios (statically) to something
other than 9600 8N1. I have been unable so far to find any evidence of drivers
changing the termios after a port has been registered based on either platform
data or OF attributes. So I do not see why we would want to provide that
functionality explicitly in the ASC driver if nobody else does; if you want
to change the TTY speed, use the tty IOCTL interfaces (or stty).
So I don't see why you think the concept of a default or current speed belongs
in the device tree description of the uart at all, unless there is something
special about the hardware. One possible example of that seems to be the ARC
driver which only appears to support one serial speed and uses "current-speed"
attribute to specify that speed; or the ARC driver is broken and they have
made a mistake in their set_termios implementation, we cannot quite work out
which of those two things is true. But that isn't the case for the ASC
hardware so it again would not be relevant in this driver.
So basically we would like some help understanding what it is you would
really like us to do. Clearly we can just duplicate what you did in
of_serial.c for the 8250 and move on, but we would like to understand why
that wouldn't just be adding redundant code.
Regards,
-stephen
On Monday 20 May 2013, Stephen GALLIMORE wrote:
> We have pretty much completed reworking the patch set we sent recently, but
> there is one comment you made which seemed to make perfect sense
> but after investigating it has left me totally confused, which was:
>
> >I would also recommed adding a way to set the default baud rate through
> > a property. Following the example of the 8250 driver, you should probably
> > call that "current-speed".
>
> I note that you are listed as the author of of_serial.c so I was assuming
> that looking at this would make sense, but it doesn't at all, and I would
> be really grateful if you could explain what you were trying to achieve
> and how you thought it worked. The code does:
>
> /* If current-speed was set, then try not to change it. */
> if (of_property_read_u32(np, "current-speed", &spd) == 0)
> port->custom_divisor = clk / (16 * spd);
>
> The "spd" variable is not used again and I do not understand why you
> thought setting port->custom_divisor would have any impact on the default
> or current baud rate of the UART, or that this information was useful to
> any other part of the system. A search has only revealed that this port
> field has only one purpose, which is to provide a "custom" speed
> (unsurprisingly) when the user requests 38.4K and the port flag
> UPF_SPD_CUST has been set through a TIOCSSERIAL ioctl call, instead of
> actually getting 38.4K. The key part of that assessment being the
> implementation of uart_get_divisor() :
>
> if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST)
> quot = port->custom_divisor;
> else
> quot = DIV_ROUND_CLOSEST(port->uartclk, 16 * baud);
>
> Also all of the custom_divisor functionality is basically commented as "old"
> or has a kernel warning saying it is deprecated (see uart_set_info), so as
> far as I can see for our (and I suspect most) hardware it is completely
> irrelevant functionality.
What may have happened here is that custom_divisor was used in a different
way when I added that code than it is today, and the change was not propagated
into the of_serial driver. However, going back to 2.6.20 shows no different
code than what we have today in this regard. It may simply have been a mistake
on my side.
> So I don't see why you think the concept of a default or current speed belongs
> in the device tree description of the uart at all, unless there is something
> special about the hardware.
I looked it up in the original serial port binding at
http://www.openfirmware.org/1275/bindings/devices/html/serial.html, which does
not specify the property, and in ePAPR, which does have it in the section about
"serial class devices":
18 6.2.1.2 current-speed
19 Property: current-speed
20 Value type: <u32>
21 Description:
22 Specifies the current speed of a serial device in bits per second. A boot program should set
23 this property if it has initialized the serial device.
24 Example:
25 current - speed = <115200>; # 115200 baud
The reason you want this is so that the driver can initialize the hardware from
scratch more easily and get back to the same settings. Why they only specified
the baud rate but not also start/stop bits and flow control I don't understand
though.
I guess you can ignore my original comment.
Arnd
On Wednesday 22 May 2013, Arnd Bergmann wrote:
> > Also all of the custom_divisor functionality is basically commented as "old"
> > or has a kernel warning saying it is deprecated (see uart_set_info), so as
> > far as I can see for our (and I suspect most) hardware it is completely
> > irrelevant functionality.
>
> What may have happened here is that custom_divisor was used in a different
> way when I added that code than it is today, and the change was not propagated
> into the of_serial driver. However, going back to 2.6.20 shows no different
> code than what we have today in this regard. It may simply have been
> a mistake on my side.
Thanks for looking into this and digging around in the history. At least I didn't
miss something really obvious.
> I looked it up in the original serial port binding at
> http://www.openfirmware.org/1275/bindings/devices/html/serial.ht
> ml, which does
> not specify the property, and in ePAPR, which does have it in the
> section about
> "serial class devices":
>
> 18 6.2.1.2 current-speed
> 19 Property: current-speed
> 20 Value type: <u32>
> 21 Description:
> 22 Specifies the current speed of a serial device in bits per second. A
> boot program should set
> 23 this property if it has initialized the serial device.
> 24 Example:
> 25 current - speed = <115200>; # 115200 baud
>
> The reason you want this is so that the driver can initialize the hardware from
> scratch more easily and get back to the same settings. Why they only specified
> the baud rate but not also start/stop bits and flow control I don't understand
> though.
Yes, I had been made aware of the ePAPR definition by Srini after I sent the post.
In fact I had exactly the same question in my head about the other settings as
well.
>
> I guess you can ignore my original comment.
>
OK.
Thanks again for your time.
Regards,
-stephen