qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Peter Delevoryas <pdel@fb.com>
To: "Cédric Le Goater" <clg@kaod.org>
Cc: Andrew Jeffery <andrew@aj.id.au>, Joel Stanley <joel@jms.id.au>,
	"Cameron Esfahani via" <qemu-devel@nongnu.org>,
	"qemu-arm@nongnu.org" <qemu-arm@nongnu.org>
Subject: Re: [PATCH 2/5] hw/arm/aspeed: Select console UART from machine
Date: Tue, 31 Aug 2021 14:07:13 +0000	[thread overview]
Message-ID: <1B01A3F5-1D6A-4976-B7C1-A4AD3259CB1C@fb.com> (raw)
In-Reply-To: <a802ecb1-aa49-fd4c-5bd2-2bb19af56ac9@kaod.org>

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


  reply	other threads:[~2021-08-31 14:12 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-27 21:04 [PATCH 0/5] hw/arm/aspeed: Add fuji machine type pdel
2021-08-27 21:04 ` [PATCH 1/5] hw/arm/aspeed: Add get_irq to AspeedSoCClass pdel
2021-08-28  0:30   ` Peter Delevoryas
2021-08-28  8:27   ` Cédric Le Goater
2021-08-27 21:04 ` [PATCH 2/5] hw/arm/aspeed: Select console UART from machine pdel
2021-08-28  8:25   ` Cédric Le Goater
2021-08-28 15:58     ` Peter Delevoryas
2021-08-31  8:15       ` Cédric Le Goater
2021-08-31 13:51         ` Peter Delevoryas
2021-08-31 14:06           ` Cédric Le Goater
2021-08-31 10:39       ` Cédric Le Goater
2021-08-31 11:23         ` Andrew Jeffery
2021-08-31 13:34           ` Cédric Le Goater
2021-08-31 14:07             ` Peter Delevoryas [this message]
2021-08-31 15:57             ` Philippe Mathieu-Daudé
2021-08-31 16:37               ` Cédric Le Goater
2021-08-27 21:04 ` [PATCH 3/5] hw/arm/aspeed: Add fuji machine type pdel
2021-08-28  8:28   ` Cédric Le Goater
2021-08-28 16:00     ` Peter Delevoryas
2021-08-31 16:00       ` Philippe Mathieu-Daudé
2021-08-31 16:38         ` Peter Delevoryas
2021-08-27 21:04 ` [PATCH 4/5] hw/arm/aspeed: Fix AST2600_CLK_SEL3 address pdel
2021-08-28  8:15   ` Cédric Le Goater
2021-08-28 15:13     ` Peter Delevoryas
2021-08-27 21:04 ` [PATCH 5/5] hw/arm/aspeed: Initialize AST2600 clock selection registers pdel
2021-08-28  8:19   ` Cédric Le Goater

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1B01A3F5-1D6A-4976-B7C1-A4AD3259CB1C@fb.com \
    --to=pdel@fb.com \
    --cc=andrew@aj.id.au \
    --cc=clg@kaod.org \
    --cc=joel@jms.id.au \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).