[PATCH] Updated version of OXPCIe serial port support

[Stefan Reinauer]

See patch

[Kevin O'Connor]

On Fri, Feb 11, 2011 at 01:18:03PM -0800, Stefan Reinauer wrote:
> add support for OXPCIe 952 card and clean up debug abstraction layer
>
> Signed-off-by: Stefan Reinauer

Thanks.  See comments below.

[...]
> +#define OXPCIE_COM1 (oxpcie_bar + 0x1000)
> +u32 oxpcie_bar VAR16VISIBLE;

I don't think it's a good idea to have an all caps define that
references a variable.

[...]
>    oxpcie_bar = pci_config_readl(oxpcie_bdf, 0x10);

s/0x10/PCI_BASE_ADDRESS_0/
> +// Write a character to the serial port.
> +static void
> +debug_serial_oxford(char c)
> +{
> if (!CONFIG_DEBUG_SERIAL_OXFORD)
>    return;
> if (!oxpcie_bar)
>    return;
> int timeout = DEBUG_TIMEOUT;
> while ((readb((void *)OXPCIE_COM1+SEROFF_LSR) & 0x60) != 0x60)

I'm not sure how this will work.  The bar is likely to be in high
memory, yet this code can be run from 16bit mode.

There is now pci_readl/pci_writel for trampolining into 32bit mode to
read/write BARs.  Would you want to trampoline to 32bit mode for a
debug device though?

The rest of the patch looks okay to me.

-Kevin

[Stefan Reinauer]

On Sat, Feb 12, 2011 at 2:30 PM, Kevin O'Connor  wrote:
> > while ((readb((void *)OXPCIE_COM1+SEROFF_LSR) & 0x60) != 0x60)
>
> I'm not sure how this will work.  The bar is likely to be in high
> memory, yet this code can be run from 16bit mode.
>
> There is now pci_readl/pci_writel for trampolining into 32bit mode to
> read/write BARs.  Would you want to trampoline to 32bit mode for a
> debug device though?

Ok, does it sound reasonable to add pci_readw and pci_readb for that, too?

Also, the functionality of those functions is somewhat pci unrelated,
it just happens to be useful for access to pci bars. I suggest
renaming them to pmode_readl or something like that in order to make
it easier to understand what the function really does. Sounds ok? Then
I would go ahead and implement it this way.

Stefan

[Kevin O'Connor]

On Sun, Feb 13, 2011 at 06:26:32PM -0800, Stefan Reinauer wrote:
> On Sat, Feb 12, 2011 at 2:30 PM, Kevin O'Connor  wrote:
> > > while ((readb((void *)OXPCIE_COM1+SEROFF_LSR) & 0x60) != 0x60)
> >
> > I'm not sure how this will work.  The bar is likely to be in high
> > memory, yet this code can be run from 16bit mode.
> >
> > There is now pci_readl/pci_writel for trampolining into 32bit mode to
> > read/write BARs.  Would you want to trampoline to 32bit mode for a
> > debug device though?
>
> Ok, does it sound reasonable to add pci_readw and pci_readb for that, too?

Adding the extra functions should be fine.

However, the trampoline is not transparent - there's no way to know
which flavor of 16bit mode to return to.  So, having debug functions
trampoline could cause "heisenbugs".

I suggest disabling the debugging when MODESEGMENT is true, so we can
commit that.  Then an incremental patch that adds debugging via the
trampoline functions can be added on top if you wish.
> Also, the functionality of those functions is somewhat pci unrelated,
> it just happens to be useful for access to pci bars. I suggest
> renaming them to pmode_readl or something like that in order to make
> it easier to understand what the function really does. Sounds ok? Then
> I would go ahead and implement it this way.

If you wish.  Please send the rename as a separate patch though.

-Kevin