Re: [PATCH 2/5] hw/arm/aspeed: Select console UART from machine

[Peter Delevoryas <pdel@fb.com>]

> On Aug 31, 2021, at 6:34 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> On 8/31/21 1:23 PM, Andrew Jeffery wrote:
>> Hi Cédric, Peter,
>> 
>> On Tue, 31 Aug 2021, at 20:09, Cédric Le Goater wrote:
>>> On 8/28/21 5:58 PM, Peter Delevoryas wrote:
>>>> I think I’m a little confused on this part. What I meant by “most machines just use UART5” was that most DTS’s use “stdout-path=&uart5”, but fuji uses “stdout-path=&uart1”. I /do/ see that SCU510 includes a bit related to UART, but it’s for disabling booting from UART1 and UART5. I just care about the console aspect, not booting.
>>> 
>>> The UART can be switched with SCU70[29] on the AST2500, btw.
>> 
>> If it helps, neither the AST2600's "Boot from UART" feature nor the 
>> AST2[456]00's "Debug UART" feature are related to which UART is used as 
>> the BMC console by u-boot and/or the kernel - the latter is entirely a 
>> software thing.
> 
> ok. 
> 
> I don't think we should initialize all 5 UARTs of SoC and let the user define 
> all the expected devices on the command. Unless we want to do something like
> 'macs_mask' ? but at the SoC level. It might be overkill for the need.
> 
> My suggestion is have the Aspeed board tell the SoC which uart was selected 
> for the console. That can be done with an extra "serial-dev" int property at 
> the SoC level, defaults to ASPEED_DEV_UART5, like for the machine. 
> 
> The serial init needs a change  : 
> 
>    /* UART - attach an 8250 to the IO space as our UART5 */
>    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400,
>                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
> 
> but it stays where it is currently, under the SoC.

Oh ok, yeah that design sounds good to me, I can submit a patch like that.

I’ll make sure to resubmit PATCH 5, the register fixes, separately though.

> 
>> The "Debug UART" is a hardware backdoor, a UART-to-AHB bridge 
>> implemented by the SoC. It provides a shell environment that allows you 
>> to issue transactions directly on the AHB if you perform a magic knock. 
>> I have a driver for it implemented here:
>> 
>> https://github.com/amboar/cve-2019-6260/blob/master/src/debug.c
>> 
>> SCU70[29] on the AST2500 selects whether this backdoor is exposed on 
>> UART1 or UART5.
>> 
>> The "Boot from UART" feature is implemented in the AST2600 ROM code as 
>> a fallback for loading the SPL if fetching it from SPI-NOR or the eMMC 
>> fails, or the SPL is incorrectly signed for secure-boot.
>> 
>> I think Peter is on the right track with this patch?
> 
> Yes. nearly. Sorry for the confusion on how to handle this Peter. A machine 
> *and* a SoC property should to the trick. 
> 
> 'amc->serial_dev' is a good idea. You need a similar one under the SoC.
> 
> Thanks for the feedback Andrew,
> 
> C. 

Ah, thanks for clarifying Andrew! I was looking at the datasheet again yesterday,
and it seemed like the “debug” and “boot” features might be distinct from rx/tx,
but to be honest I had no idea and came to this opinion mostly by accident.

So, I’ll resubmit PATCH 5 separately, and submit another patch with this
machine + SoC property design by itself.

Thanks for your consideration, I really appreciate it!

Thanks,
Peter