Re: [PATCH v2 5/9] powerpc/microwatt: Use standard 16550 UART for console

[Paul Mackerras <paulus@ozlabs.org>]

On Fri, Jun 18, 2021 at 05:40:40PM +1000, Nicholas Piggin wrote:
> Excerpts from Paul Mackerras's message of June 18, 2021 1:46 pm:
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > 
> > This adds support to the Microwatt platform to use the standard
> > 16550-style UART which available in the standalone Microwatt FPGA.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
...
> > +#ifdef CONFIG_PPC_EARLY_DEBUG_MICROWATT
> > +
> > +#define UDBG_UART_MW_ADDR	((void __iomem *)0xc0002000)
> > +
> > +static u8 udbg_uart_in_isa300_rm(unsigned int reg)
> > +{
> > +	uint64_t msr = mfmsr();
> > +	uint8_t  c;
> > +
> > +	mtmsr(msr & ~(MSR_EE|MSR_DR));
> > +	isync();
> > +	eieio();
> > +	c = __raw_rm_readb(UDBG_UART_MW_ADDR + (reg << 2));
> > +	mtmsr(msr);
> > +	isync();
> > +	return c;
> > +}
> 
> Why is realmode required? No cache inhibited mappings yet?

Because it's EARLY debug, for use in the very early stages of boot
when the kernel's radix tree may or may not have been initialized.
The easiest way to make a function that works correctly whether or not
the radix tree has been initialized and the MMU turned on is to
temporarily turn off the MMU for data accesses and use lbzcix/stbcix
(which Microwatt has, even though it doesn't implement hypervisor
mode).

(I don't know which "yet" you meant - "yet" in the process of booting a
kernel, or "yet" in the process of Microwatt's development?  Microwatt
certainly does have cache-inhibited mappings and has done since the
MMU was first introduced.)

In fact the defconfig I add later in the series doesn't enable
CONFIG_PPC_EARLY_DEBUG_MICROWATT, but it's there if it's needed for
debugging.

> mtmsrd with L=0 is defined to be context synchronizing in isa 3, so I 
> don't think the isync would be required. There is a bit of code around 
> arch/powerpc that does this, maybe it used to be needed or some other
> implementations needed it?
> 
> That's just for my curiosity, it doesn't really hurt to have them
> there.

Right, and in fact mtmsrd is marked as a single-issue instruction in
Microwatt, so it should work with no isyncs or eieios.  Presumably Ben
copied the isync/eieio pattern from somewhere else.

Paul.