[PATCH v2] ns16550: misc minor adjustments

[PATCH v2] ns16550: misc minor adjustments

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 5752 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>
---
v2: Log a message when reading zero divisor. Make an attempt at
    explaining what "base baud rate" means.

--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -277,13 +277,14 @@ 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 (usually the highest baud rate the
+  device can communicate at) 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 +731,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,12 @@ 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);
+        else
+            printk(XENLOG_ERR
+                   "Automatic baud rate determination was requested,"
+                   " but a baud rate was not set up\n");
     }
     ns_write_reg(uart, UART_LCR, lcr);
 
@@ -945,8 +951,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: 5783 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>
---
v2: Log a message when reading zero divisor. Make an attempt at
    explaining what "base baud rate" means.

--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -277,13 +277,14 @@ 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 (usually the highest baud rate the
+  device can communicate at) 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 +731,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,12 @@ 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);
+        else
+            printk(XENLOG_ERR
+                   "Automatic baud rate determination was requested,"
+                   " but a baud rate was not set up\n");
     }
     ns_write_reg(uart, UART_LCR, lcr);
 
@@ -945,8 +951,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 v2] ns16550: misc minor adjustments

[Ian Campbell]

On Thu, 2015-11-12 at 08:34 -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>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2] ns16550: misc minor adjustments

[Andrew Cooper]

On 12/11/15 15:34, 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>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> with one tweak

> ---
> v2: Log a message when reading zero divisor. Make an attempt at
>     explaining what "base baud rate" means.
>
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -277,13 +277,14 @@ 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 (usually the highest baud rate the
> +  device can communicate at) 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 +731,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`.

Elsewhere in the document, bold is used to refer to other options, so
"see **console**".

~Andrew