qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Cédric Le Goater" <clg@kaod.org>
To: Peter Delevoryas <pdel@fb.com>
Cc: "qemu-arm@nongnu.org" <qemu-arm@nongnu.org>,
	"joel@jms.id.au" <joel@jms.id.au>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: Re: [PATCH 2/5] hw/arm/aspeed: Select console UART from machine
Date: Tue, 31 Aug 2021 10:15:16 +0200	[thread overview]
Message-ID: <a67c22a2-eb11-87f5-54e7-14c2d771861b@kaod.org> (raw)
In-Reply-To: <BYAPR15MB3032BA6C3556797AC2A3461CACC99@BYAPR15MB3032.namprd15.prod.outlook.com>

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.

OK. 

We want to keep the initialization of the SoC devices under the SoC and not 
under the board. That's the main reason behind my suggestion. 
 
>  
> 
> This is the commit that changed the serial console from UART5 to UART1 in fuji’s DTS: https://github.com/facebook/openbmc-uboot/commit/afeddd6e27b5f094bbc4805dc8c1c22b3b7fb203 <https://github.com/facebook/openbmc-uboot/commit/afeddd6e27b5f094bbc4805dc8c1c22b3b7fb203>
> 
>  
> 
> I don’t know what the platform.S AST_SCU_MFP_CTRL7 changes do (maybe setting some GPIO for UART1?), but as far as I understand, I don’t think using UART1 should require any extra registers from the datasheet.
> 
>  
> 
> An alternate design I considered was UART5=serial_hd(0) and UART1=serial_hd(1),
> maybe that would be more appropriate? I don’t think anybody uses both UART’s 
> simultaneously though, so I didn’t pursue that design.

I would prefer that. This patch has been in my tree for years :

  https://github.com/legoater/qemu/commit/138713ee1d3d84682b85c1b01577a7e86ab3fed4

See

  https://github.com/legoater/qemu/commits/aspeed-6.2

May be this is what you simply need ? You can use the ast2600-evb machine for 
your tests. Tell me how it goes. 

AFAICT, you will need to resend PATCH 5 only.

Thanks,

C.


>  
> 
> Some link references:
> 
> Elbert DTS uses “stdout-path=&uart5” https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-elbert.dts#L17
> 
> Fuji DTS uses “stdout-path=&uart1” https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-fuji.dts#L17
> 
>  
> 
> *From: *Cédric Le Goater <clg@kaod.org>
> *Date: *Saturday, August 28, 2021 at 1:26 AM
> *To: *Peter Delevoryas <pdel@fb.com>
> *Cc: *joel@jms.id.au <joel@jms.id.au>, qemu-devel@nongnu.org <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
> 
> On 8/27/21 11:04 PM, pdel@fb.com wrote:
>> From: Peter Delevoryas <pdel@fb.com>
>>
>> This change replaces the UART serial device initialization code with machine
>> configuration data, making it so that we have a single code path for console
>> UART initialization, but allowing different machines to use different
>> UART's. This is relevant because the Aspeed chips have 2 debug UART's, UART5
>> and UART1, and while most machines just use UART5, some use UART1.
> 
> I think this is controlled by SCU510. If so, we should have a different HW
> strapping for the new machine and check the configuration at the SoC level,
> in aspeed_ast2600.c, to change the serial initialization.
> 
> 
> Thanks,
> 
> C.
>  
>>
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>>  hw/arm/aspeed.c         | 7 +++++++
>>  hw/arm/aspeed_ast2600.c | 5 -----
>>  hw/arm/aspeed_soc.c     | 5 -----
>>  include/hw/arm/aspeed.h | 1 +
>>  4 files changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 9d43e26c51..ff53d12395 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -14,6 +14,7 @@
>>  #include "hw/arm/boot.h"
>>  #include "hw/arm/aspeed.h"
>>  #include "hw/arm/aspeed_soc.h"
>> +#include "hw/char/serial.h"
>>  #include "hw/i2c/i2c_mux_pca954x.h"
>>  #include "hw/i2c/smbus_eeprom.h"
>>  #include "hw/misc/pca9552.h"
>> @@ -21,6 +22,7 @@
>>  #include "hw/misc/led.h"
>>  #include "hw/qdev-properties.h"
>>  #include "sysemu/block-backend.h"
>> +#include "sysemu/sysemu.h"
>>  #include "hw/loader.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/units.h"
>> @@ -352,6 +354,10 @@ static void aspeed_machine_init(MachineState *machine)
>>      }
>>      qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
>> 
>> +    serial_mm_init(get_system_memory(), sc->memmap[amc->serial_dev], 2,
>> +                   sc->get_irq(&bmc->soc, amc->serial_dev), 38400,
>> +                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> +
>>      memory_region_add_subregion(get_system_memory(),
>>                                  sc->memmap[ASPEED_DEV_SDRAM],
>>                                  &bmc->ram_container);
>> @@ -804,6 +810,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>>      mc->no_parallel = 1;
>>      mc->default_ram_id = "ram";
>>      amc->macs_mask = ASPEED_MAC0_ON;
>> +    amc->serial_dev = ASPEED_DEV_UART5;
>> 
>>      aspeed_machine_class_props_init(oc);
>>  }
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index 56e1adb343..a27b0de482 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -322,11 +322,6 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>>      }
>> 
>> -    /* 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);
>> -
>>      /* I2C */
>>      object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>>                               &error_abort);
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index c373182299..0c09d1e5b4 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -287,11 +287,6 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>>      }
>> 
>> -    /* 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);
>> -
>>      /* I2C */
>>      object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>>                               &error_abort);
>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
>> index c9747b15fc..9f5b9f04d6 100644
>> --- a/include/hw/arm/aspeed.h
>> +++ b/include/hw/arm/aspeed.h
>> @@ -38,6 +38,7 @@ struct AspeedMachineClass {
>>      uint32_t num_cs;
>>      uint32_t macs_mask;
>>      void (*i2c_init)(AspeedMachineState *bmc);
>> +    uint32_t serial_dev;
>>  };
>> 
>> 
>>
> 



  reply	other threads:[~2021-08-31  8:17 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 [this message]
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
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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=a67c22a2-eb11-87f5-54e7-14c2d771861b@kaod.org \
    --to=clg@kaod.org \
    --cc=joel@jms.id.au \
    --cc=pdel@fb.com \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

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