[PATCH] ns16550: misc minor adjustments

[PATCH] ns16550: misc minor adjustments

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 5392 bytes --]

First and foremost: fix documentation: The use of "clock_hz", when
"base_baud" was meant, has taken me several hours (suspecting a more
complicated problem with the PCIe card I've been trying to get
working). At once correct the "gdb" option, which is more like
"console", not like "com<N>".

Next, fix the types of ns_{read,write}_reg(): Especially the former
having had a signed return type so far caused quite interesting effects
when determining to baud rate if "auto" was specified. In that same
code, also avoid dividing by zero when in fact the baud rate was not
previously set up.

Further, accept I/O port based serial PCI cards with a port range wider
than 8 bytes.

Finally, slightly rearrange struct ns16550 to reduce holes.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -277,13 +277,13 @@ Flag to indicate whether to probe for a
 ACPI indicating none to be there.
 
 ### com1,com2
-> `= <baud>[/<clock_hz>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
+> `= <baud>[/<base_baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
 
 Both option `com1` and `com2` follow the same format.
 
 * `<baud>` may be either an integer baud rate, or the string `auto` if
   the bootloader or other earlier firmware has already set it up.
-* Optionally, a clock speed measured in hz can be specified.
+* Optionally, the base baud rate can be specified.
 * `DPS` represents the number of data bits, the parity, and the number
   of stop bits.
   * `D` is an integer between 5 and 8 for the number of data bits.
@@ -730,9 +730,11 @@ Controls EPT related features.
 >> Have hardware keep accessed/dirty (A/D) bits updated.
 
 ### gdb
-> `= <baud>[/<clock_hz>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]] | pci | amt ] `
+> `= com1[H,L] | com2[H,L] | dbgp`
 
-Specify the serial parameters for the GDB stub.
+> Default: ``
+
+Specify which console gdbstub should use. See `console`.
 
 ### gnttab\_max\_frames
 > `= <integer>`
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -33,7 +33,7 @@
 
 /*
  * Configure serial port with a string:
- *   <baud>[/<clock_hz>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]]].
+ *   <baud>[/<base_baud>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]]].
  * The tail of the string can be omitted if platform defaults are sufficient.
  * If the baud rate is pre-configured, perhaps by a bootloader, then 'auto'
  * can be specified in place of a numeric baud rate. Polled mode is specified
@@ -66,10 +66,10 @@ static struct ns16550 {
     bool_t dw_usr_bsy;
 #ifdef HAS_PCI
     /* PCI card parameters. */
-    unsigned int pb_bdf[3]; /* pci bridge BDF */
-    unsigned int ps_bdf[3]; /* pci serial port BDF */
     bool_t pb_bdf_enable;   /* if =1, pb-bdf effective, port behind bridge */
     bool_t ps_bdf_enable;   /* if =1, ps_bdf effective, port on pci card */
+    unsigned int pb_bdf[3]; /* pci bridge BDF */
+    unsigned int ps_bdf[3]; /* pci serial port BDF */
     u32 bar;
     u32 bar64;
     u16 cr;
@@ -345,7 +345,7 @@ static const struct ns16550_config_mmio
 
 static void ns16550_delayed_resume(void *data);
 
-static char ns_read_reg(struct ns16550 *uart, int reg)
+static u8 ns_read_reg(struct ns16550 *uart, unsigned int reg)
 {
     void __iomem *addr = uart->remapped_io_base + (reg << uart->reg_shift);
 #ifdef HAS_IOPORTS
@@ -363,7 +363,7 @@ static char ns_read_reg(struct ns16550 *
     }
 }
 
-static void ns_write_reg(struct ns16550 *uart, int reg, char c)
+static void ns_write_reg(struct ns16550 *uart, unsigned int reg, u8 c)
 {
     void __iomem *addr = uart->remapped_io_base + (reg << uart->reg_shift);
 #ifdef HAS_IOPORTS
@@ -386,7 +386,7 @@ static void ns_write_reg(struct ns16550
 
 static int ns16550_ioport_invalid(struct ns16550 *uart)
 {
-    return (unsigned char)ns_read_reg(uart, UART_IER) == 0xff;
+    return ns_read_reg(uart, UART_IER) == 0xff;
 }
 
 static void ns16550_interrupt(
@@ -399,7 +399,8 @@ static void ns16550_interrupt(
 
     while ( !(ns_read_reg(uart, UART_IIR) & UART_IIR_NOINT) )
     {
-        char lsr = ns_read_reg(uart, UART_LSR);
+        u8 lsr = ns_read_reg(uart, UART_LSR);
+
         if ( (lsr & uart->lsr_mask) == uart->lsr_mask )
             serial_tx_interrupt(port, regs);
         if ( lsr & UART_LSR_DR )
@@ -530,7 +531,8 @@ static void ns16550_setup_preirq(struct
         /* Baud rate already set: read it out from the divisor latch. */
         divisor  = ns_read_reg(uart, UART_DLL);
         divisor |= ns_read_reg(uart, UART_DLM) << 8;
-        uart->baud = uart->clock_hz / (divisor << 4);
+        if ( divisor )
+            uart->baud = uart->clock_hz / (divisor << 4);
     }
     ns_write_reg(uart, UART_LCR, lcr);
 
@@ -945,8 +947,8 @@ pci_uart_config (struct ns16550 *uart, i
                     size = len & PCI_BASE_ADDRESS_IO_MASK;
                     size &= -size;
 
-                    /* Not 8 bytes */
-                    if ( size != 0x8 )
+                    /* Not at least 8 bytes */
+                    if ( size < 8 )
                         continue;
 
                     uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;



[-- Attachment #2: ns16550-misc.patch --]
[-- Type: text/plain, Size: 5423 bytes --]

ns16550: misc minor adjustments

First and foremost: fix documentation: The use of "clock_hz", when
"base_baud" was meant, has taken me several hours (suspecting a more
complicated problem with the PCIe card I've been trying to get
working). At once correct the "gdb" option, which is more like
"console", not like "com<N>".

Next, fix the types of ns_{read,write}_reg(): Especially the former
having had a signed return type so far caused quite interesting effects
when determining to baud rate if "auto" was specified. In that same
code, also avoid dividing by zero when in fact the baud rate was not
previously set up.

Further, accept I/O port based serial PCI cards with a port range wider
than 8 bytes.

Finally, slightly rearrange struct ns16550 to reduce holes.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -277,13 +277,13 @@ Flag to indicate whether to probe for a
 ACPI indicating none to be there.
 
 ### com1,com2
-> `= <baud>[/<clock_hz>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
+> `= <baud>[/<base_baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
 
 Both option `com1` and `com2` follow the same format.
 
 * `<baud>` may be either an integer baud rate, or the string `auto` if
   the bootloader or other earlier firmware has already set it up.
-* Optionally, a clock speed measured in hz can be specified.
+* Optionally, the base baud rate can be specified.
 * `DPS` represents the number of data bits, the parity, and the number
   of stop bits.
   * `D` is an integer between 5 and 8 for the number of data bits.
@@ -730,9 +730,11 @@ Controls EPT related features.
 >> Have hardware keep accessed/dirty (A/D) bits updated.
 
 ### gdb
-> `= <baud>[/<clock_hz>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]] | pci | amt ] `
+> `= com1[H,L] | com2[H,L] | dbgp`
 
-Specify the serial parameters for the GDB stub.
+> Default: ``
+
+Specify which console gdbstub should use. See `console`.
 
 ### gnttab\_max\_frames
 > `= <integer>`
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -33,7 +33,7 @@
 
 /*
  * Configure serial port with a string:
- *   <baud>[/<clock_hz>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]]].
+ *   <baud>[/<base_baud>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]]].
  * The tail of the string can be omitted if platform defaults are sufficient.
  * If the baud rate is pre-configured, perhaps by a bootloader, then 'auto'
  * can be specified in place of a numeric baud rate. Polled mode is specified
@@ -66,10 +66,10 @@ static struct ns16550 {
     bool_t dw_usr_bsy;
 #ifdef HAS_PCI
     /* PCI card parameters. */
-    unsigned int pb_bdf[3]; /* pci bridge BDF */
-    unsigned int ps_bdf[3]; /* pci serial port BDF */
     bool_t pb_bdf_enable;   /* if =1, pb-bdf effective, port behind bridge */
     bool_t ps_bdf_enable;   /* if =1, ps_bdf effective, port on pci card */
+    unsigned int pb_bdf[3]; /* pci bridge BDF */
+    unsigned int ps_bdf[3]; /* pci serial port BDF */
     u32 bar;
     u32 bar64;
     u16 cr;
@@ -345,7 +345,7 @@ static const struct ns16550_config_mmio
 
 static void ns16550_delayed_resume(void *data);
 
-static char ns_read_reg(struct ns16550 *uart, int reg)
+static u8 ns_read_reg(struct ns16550 *uart, unsigned int reg)
 {
     void __iomem *addr = uart->remapped_io_base + (reg << uart->reg_shift);
 #ifdef HAS_IOPORTS
@@ -363,7 +363,7 @@ static char ns_read_reg(struct ns16550 *
     }
 }
 
-static void ns_write_reg(struct ns16550 *uart, int reg, char c)
+static void ns_write_reg(struct ns16550 *uart, unsigned int reg, u8 c)
 {
     void __iomem *addr = uart->remapped_io_base + (reg << uart->reg_shift);
 #ifdef HAS_IOPORTS
@@ -386,7 +386,7 @@ static void ns_write_reg(struct ns16550
 
 static int ns16550_ioport_invalid(struct ns16550 *uart)
 {
-    return (unsigned char)ns_read_reg(uart, UART_IER) == 0xff;
+    return ns_read_reg(uart, UART_IER) == 0xff;
 }
 
 static void ns16550_interrupt(
@@ -399,7 +399,8 @@ static void ns16550_interrupt(
 
     while ( !(ns_read_reg(uart, UART_IIR) & UART_IIR_NOINT) )
     {
-        char lsr = ns_read_reg(uart, UART_LSR);
+        u8 lsr = ns_read_reg(uart, UART_LSR);
+
         if ( (lsr & uart->lsr_mask) == uart->lsr_mask )
             serial_tx_interrupt(port, regs);
         if ( lsr & UART_LSR_DR )
@@ -530,7 +531,8 @@ static void ns16550_setup_preirq(struct
         /* Baud rate already set: read it out from the divisor latch. */
         divisor  = ns_read_reg(uart, UART_DLL);
         divisor |= ns_read_reg(uart, UART_DLM) << 8;
-        uart->baud = uart->clock_hz / (divisor << 4);
+        if ( divisor )
+            uart->baud = uart->clock_hz / (divisor << 4);
     }
     ns_write_reg(uart, UART_LCR, lcr);
 
@@ -945,8 +947,8 @@ pci_uart_config (struct ns16550 *uart, i
                     size = len & PCI_BASE_ADDRESS_IO_MASK;
                     size &= -size;
 
-                    /* Not 8 bytes */
-                    if ( size != 0x8 )
+                    /* Not at least 8 bytes */
+                    if ( size < 8 )
                         continue;
 
                     uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] ns16550: misc minor adjustments

[Ian Campbell]

On Thu, 2015-11-12 at 03:31 -0700, Jan Beulich wrote:
> First and foremost: fix documentation: The use of "clock_hz", when
> "base_baud" was meant, has taken me several hours (suspecting a more
> complicated problem with the PCIe card I've been trying to get
> working). At once correct the "gdb" option, which is more like
> "console", not like "com<N>".
> 
> Next, fix the types of ns_{read,write}_reg(): Especially the former
> having had a signed return type so far caused quite interesting effects
> when determining to baud rate if "auto" was specified. In that same
> code, also avoid dividing by zero when in fact the baud rate was not
> previously set up.
> 
> Further, accept I/O port based serial PCI cards with a port range wider
> than 8 bytes.
> 
> Finally, slightly rearrange struct ns16550 to reduce holes.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -277,13 +277,13 @@ Flag to indicate whether to probe for a
>  ACPI indicating none to be there.
>  
>  ### com1,com2
> -> `= <baud>[/<clock_hz>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-
> bdf>][,[<bridge-bdf>]]]]]]`
> +> `= <baud>[/<base_baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-
> bdf>][,[<bridge-bdf>]]]]]]`
>  
>  Both option `com1` and `com2` follow the same format.
>  
>  * `<baud>` may be either an integer baud rate, or the string `auto` if
>    the bootloader or other earlier firmware has already set it up.
> -* Optionally, a clock speed measured in hz can be specified.
> +* Optionally, the base baud rate can be specified.

I do wonder if everyone trying to use this (which will include less-savy
users who are trying to produce a useful bug report by our request) will
know what this means?

On the other hand I'm not exactly sure how to better describe it other than
as the "basic baud rate, i.e. the one you get with divisor == 1", which is
no more likely to enlighten anyone who didn't already know that.

Is it true (enough) to say something like "typically the fastest baud rate
supported by the device"?

> @@ -530,7 +531,8 @@ static void ns16550_setup_preirq(struct
>          /* Baud rate already set: read it out from the divisor latch. */
>          divisor  = ns_read_reg(uart, UART_DLL);
>          divisor |= ns_read_reg(uart, UART_DLM) << 8;
> -        uart->baud = uart->clock_hz / (divisor << 4);
> +        if ( divisor )
> +            uart->baud = uart->clock_hz / (divisor << 4);

Will following code cope with uart->baud == BAUD_AUTO? Or should we pick a
static fallback rate (115200?) and set the divisor appropriately?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] ns16550: misc minor adjustments

[Jan Beulich]

>>> On 12.11.15 at 12:07, <ian.campbell@citrix.com> wrote:
> On Thu, 2015-11-12 at 03:31 -0700, Jan Beulich wrote:
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -277,13 +277,13 @@ Flag to indicate whether to probe for a
>>  ACPI indicating none to be there.
>>  
>>  ### com1,com2
>> -> `= <baud>[/<clock_hz>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-
>> bdf>][,[<bridge-bdf>]]]]]]`
>> +> `= <baud>[/<base_baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-
>> bdf>][,[<bridge-bdf>]]]]]]`
>>  
>>  Both option `com1` and `com2` follow the same format.
>>  
>>  * `<baud>` may be either an integer baud rate, or the string `auto` if
>>    the bootloader or other earlier firmware has already set it up.
>> -* Optionally, a clock speed measured in hz can be specified.
>> +* Optionally, the base baud rate can be specified.
> 
> I do wonder if everyone trying to use this (which will include less-savy
> users who are trying to produce a useful bug report by our request) will
> know what this means?
> 
> On the other hand I'm not exactly sure how to better describe it other than
> as the "basic baud rate, i.e. the one you get with divisor == 1", which is
> no more likely to enlighten anyone who didn't already know that.
> 
> Is it true (enough) to say something like "typically the fastest baud rate
> supported by the device"?

Yes, I think so.

>> @@ -530,7 +531,8 @@ static void ns16550_setup_preirq(struct
>>          /* Baud rate already set: read it out from the divisor latch. */
>>          divisor  = ns_read_reg(uart, UART_DLL);
>>          divisor |= ns_read_reg(uart, UART_DLM) << 8;
>> -        uart->baud = uart->clock_hz / (divisor << 4);
>> +        if ( divisor )
>> +            uart->baud = uart->clock_hz / (divisor << 4);
> 
> Will following code cope with uart->baud == BAUD_AUTO? Or should we pick a
> static fallback rate (115200?) and set the divisor appropriately?

The device won't work with it left as BAUD_AUTO. Setting a guessed
baud rate alone won't help, as we'd then also have to write it into the
respective registers. And I don't think anyway we should do any
guessing here - if the command line option was wrong (regardless of
whether due to using auto or just a wrong baud rate), communication
won't work. All I think we should really care about is to not crash, the
more in a way not diagnosable over serial console.

Jan

Re: [PATCH] ns16550: misc minor adjustments

[Ian Campbell]

On Thu, 2015-11-12 at 04:52 -0700, Jan Beulich wrote:

> > > @@ -530,7 +531,8 @@ static void ns16550_setup_preirq(struct
> > >          /* Baud rate already set: read it out from the divisor
> > > latch. */
> > >          divisor  = ns_read_reg(uart, UART_DLL);
> > >          divisor |= ns_read_reg(uart, UART_DLM) << 8;
> > > -        uart->baud = uart->clock_hz / (divisor << 4);
> > > +        if ( divisor )
> > > +            uart->baud = uart->clock_hz / (divisor << 4);
> > 
> > Will following code cope with uart->baud == BAUD_AUTO? Or should we
> > pick a
> > static fallback rate (115200?) and set the divisor appropriately?
> 
> The device won't work with it left as BAUD_AUTO. Setting a guessed
> baud rate alone won't help, as we'd then also have to write it into the
> respective registers. And I don't think anyway we should do any
> guessing here - if the command line option was wrong (regardless of
> whether due to using auto or just a wrong baud rate), communication
> won't work. All I think we should really care about is to not crash, the
> more in a way not diagnosable over serial console.

BAUD_AUTO is the default in device-tree based situations, so it isn't
necessarily the case that the command line is wrong. For the non-DT case I
think it also ends up with an implicit BAUD_AUTO in some cases?

That said I don't think it is worth going to the effort to distinguish
implicit from explicit-auto configuration, and your argument about not
writing to the registers certainly applies to the latter, so lets just
worry about not crashing as you say.

Is a printk worthwhile, due to the fact there might be a second console
present?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] ns16550: misc minor adjustments

[Andrew Cooper]

On 12/11/15 12:10, Ian Campbell wrote:
> On Thu, 2015-11-12 at 04:52 -0700, Jan Beulich wrote:
>
>>>> @@ -530,7 +531,8 @@ static void ns16550_setup_preirq(struct
>>>>          /* Baud rate already set: read it out from the divisor
>>>> latch. */
>>>>          divisor  = ns_read_reg(uart, UART_DLL);
>>>>          divisor |= ns_read_reg(uart, UART_DLM) << 8;
>>>> -        uart->baud = uart->clock_hz / (divisor << 4);
>>>> +        if ( divisor )
>>>> +            uart->baud = uart->clock_hz / (divisor << 4);
>>> Will following code cope with uart->baud == BAUD_AUTO? Or should we
>>> pick a
>>> static fallback rate (115200?) and set the divisor appropriately?
>> The device won't work with it left as BAUD_AUTO. Setting a guessed
>> baud rate alone won't help, as we'd then also have to write it into the
>> respective registers. And I don't think anyway we should do any
>> guessing here - if the command line option was wrong (regardless of
>> whether due to using auto or just a wrong baud rate), communication
>> won't work. All I think we should really care about is to not crash, the
>> more in a way not diagnosable over serial console.
> BAUD_AUTO is the default in device-tree based situations, so it isn't
> necessarily the case that the command line is wrong. For the non-DT case I
> think it also ends up with an implicit BAUD_AUTO in some cases?
>
> That said I don't think it is worth going to the effort to distinguish
> implicit from explicit-auto configuration, and your argument about not
> writing to the registers certainly applies to the latter, so lets just
> worry about not crashing as you say.
>
> Is a printk worthwhile, due to the fact there might be a second console
> present?

Yes.  Even if there isn't a second serial console, the system might boot
and `xl dmesg` should contain a record.

~Andrew

Re: [PATCH] ns16550: misc minor adjustments

[Jan Beulich]

>>> On 12.11.15 at 13:10, <ian.campbell@citrix.com> wrote:
> Is a printk worthwhile, due to the fact there might be a second console
> present?

As Andrew also asks for it, I'll add one.

Jan