Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] hw/arm/aspeed: directly map the serial device to the system address space

[Philippe Mathieu-Daudé]

On 02/08/2018 09:40 PM, Andrew Jeffery wrote:
> On Thu, 2018-02-08 at 14:40 -0300, Philippe Mathieu-Daudé wrote:
>> On 02/08/2018 02:30 PM, Peter Maydell wrote:
>>> On 8 February 2018 at 17:22, Philippe Mathieu-Daudé <address@hidden> wrote:
>>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>>> ---
>>>>  hw/arm/aspeed_soc.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>>>> index c83b7e207b..a786750e14 100644
>>>> --- a/hw/arm/aspeed_soc.c
>>>> +++ b/hw/arm/aspeed_soc.c
>>>> @@ -257,7 +257,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
>>>> **errp)
>>>>      /* UART - attach an 8250 to the IO space as our UART5 */
>>>>      if (serial_hds[0]) {
>>>>          qemu_irq uart5 = qdev_get_gpio_in(DEVICE(&s->vic), uart_irqs[4]);
>>>> -        serial_mm_init(&s->iomem, ASPEED_SOC_UART_5_BASE, 2,
>>>> +        serial_mm_init(get_system_memory(), ASPEED_SOC_UART_5_BASE, 2,
>>>>                         uart5, 38400, serial_hds[0], DEVICE_LITTLE_ENDIAN);
>>>>      }
>>>
>>> Is this a bug fix? It certainly changes behaviour, which
>>> suggests that a fuller commit message would be useful.
>>
>> This patch is buggy indeed, using system_memory the serial address
>> should be ASPEED_SOC_IOMEM_BASE + ASPEED_SOC_UART_5_BASE.
>>
> 
> If you'd like to test, you can grab images from here:
> 
> https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=romulus/
> 
> For example (without the patch):
> 
>     $ wget     
> https://openpower.xyz/job/openbmc-build/1240/distro=ubuntu,target=romulus/artifact/deploy/images/romulus/image-bmc

Thanks for this useful link!

> ...
> $ qemu-system-arm -M romulus-bmc -m 512 -drive 
> file=image-bmc,if=mtd,format=raw -nographic

good candidate for an Avocado / pyexpect test...

> 
> 
> U-Boot 2016.07 (Jan 25 2018 - 16:31:27 +0000)   
> 
>        Watchdog enabled                         
> DRAM:  496 MiB                                  
> Flash: 32 MiB                                   
> *** Warning - bad CRC, using default environment
> 
> In:    serial                                   
> Out:   serial                                   
> Err:   serial                                   
> Net:   aspeednic#0                              
> Error: aspeednic#0 address not set.             
> 
> Hit any key to stop autoboot:  0                
> ## Loading kernel from FIT Image at 20080000 ...
> ...
> 
> And yeah, this patch breaks output.

Sorry I didn't care enough testing this series :|

> 
> Thanks for the cleanup though.
> 
> Andrew
>

signature.asc
Description: OpenPGP digital signature