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

Excerpts from Paul Mackerras's message of June 18, 2021 10:12 pm:

On Fri, Jun 18, 2021 at 05:40:40PM +1000, Nicholas Piggin wrote:

quoted

Excerpts from Paul Mackerras's message of June 18, 2021 1:46 pm:

quoted

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 <redacted>

...

quoted

quoted

+#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

Ah makes sense.

(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.)

I did mean mappings to the UART, but good to get both answers :D

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.

quoted

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.

Makes sense. Well I don't have any objection to the series.

Thanks,
Nick