Re: [Xen-devel] [PATCH RFC 3/8] ns16550: make usable on ARM

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-09-10 at 16:00 +0100, Jan Beulich wrote:
> >>> On 10.09.13 at 16:18, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> > @@ -40,15 +45,22 @@ string_param("com2", opt_com2);
> >  
> >  static struct ns16550 {
> >      int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
> > -    unsigned long io_base;   /* I/O port or memory-mapped I/O address. */
> > +    u64 io_base;   /* I/O port or memory-mapped I/O address. */
> > +    u64 io_size;
> 
> Does the size really need to be 64 bits?

No, I just wrote 64 for both by mistake.

[...]
> > +    default: /* assume single byte */
> 
> Is that really a good idea? I'd recommend returning all 1s here, and
> dropping writes below.

OK.

> > +    case 1:
> > +        return readb(addr);
> > +    case 4:
> > +        return readl(addr);
> 
> This won't work without changing the return type of the function.
> Or is this just mandating the access width, without the upper 24
> bits carrying meaningful data?

The latter, the registers are still 8 bytes, it's just the accesses.
Maybe I should mask.

> > +#ifdef HAS_PCI
> >  static void pci_serial_early_init(struct ns16550 *uart)
> >  {
> >      if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
> > @@ -178,6 +215,9 @@ static void pci_serial_early_init(struct ns16550 *uart)
> >      pci_conf_write16(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2],
> >                       PCI_COMMAND, PCI_COMMAND_IO);
> >  }
> > +#else
> > +static void pci_serial_early_init(struct ns16550 *uart) {}
> > +#endif
> 
> I'd prefer if #ifdef-s of this kind went inside the function body, thus
> avoiding the duplication of the function "header". This particularly
> reduces the churn on eventual future changes to the function type
> (in particular people not building all architectures not noticing the
> need to change more than one place).

Me too normally, no idea why I did it this way!

> 
> > @@ -278,21 +320,27 @@ static void __init ns16550_init_postirq(struct 
> > serial_port *port)
> >      bits = uart->data_bits + uart->stop_bits + !!uart->parity;
> >      uart->timeout_ms = max_t(
> >          unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
> > -
> >      if ( uart->irq > 0 )
> 
> Anything wrong with the blank line above?

A victim of some debugging code I inserted then removed I think. I'll
but it back.

> 
> > +static const char const *ns16550_dt_compat[] __initdata =
> 
> That ought to be __initconst now that committed that patch.

Indeed yes, thanks.

Ian.


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