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

[Nicholas Piggin <npiggin@gmail.com>]

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

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