hw/arm/stm32f205: Fix the UART and Timer region size

Commit Message

Seth K Nov. 19, 2018, 3:15 a.m. UTC

From: Seth Kintigh <[e-mail]>

I corrected these 2 memory regions based on specifications from the chip
manufacturer. The existing ranges seem to overlap and and cause odd
behavior and/or crashes when trying to set up multiple UARTs,

Signed-off-by: Seth Kintigh <[e-mail]>
---
Phil, I hope this is the right format.

PMM, my original changes were made on an old version, but I made the patch
from the latest version per the instructions. I'm glad to hear that serial
port limit is gone!

 hw/char/stm32f2xx_usart.c  | 2 +-
 hw/timer/stm32f2xx_timer.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

s);


On Thu, Nov 15, 2018 at 7:05 AM Peter Maydell <[e-mail]>
wrote:

> On 4 November 2018 at 07:42, Seth K <[e-mail]> wrote:
> > I corrected these 2 memory regions based on specifications from the chip
> > manufacturer. The existing ranges seem to overlap and and cause odd
> > behavior and/or crashes when trying to set up multiple UARTs,
> > I also played with changing MAX_SERIAL_PORTS to 8 to match the hardware,
> > but I did not include that in this patch as I never fully tested its
> > effects.
>
> Hi; thanks for the patch. As Philippe says, it looks good,
> but the one thing we definitely need is a Signed-off-by:
> line from you.
>
> A minor note -- there is no "MAX_SERIAL_PORTS" definition
> in QEMU any more: we removed that artificial limitation
> earlier this year. Maybe you're basing your patch on an
> older version of QEMU? It's best to use git master for
> development. Our SoC model defines 6 uarts (STM_NUM_USARTS)
> in hw/arm/stm32f205_soc.c, which should all now be connectable
> on the command line, though I haven't tested that or checked
> whether the hardware has 6 or some other number...
>
> thanks
> -- PMM
>

Comments

Philippe Mathieu-Daudé Nov. 19, 2018, 10:43 a.m. UTC | #1

Hi Seth,

On Mon, Nov 19, 2018 at 4:17 AM Seth K <[e-mail]> wrote:
>
> From: Seth Kintigh <[e-mail]>
>
> I corrected these 2 memory regions based on specifications from the chip
> manufacturer. The existing ranges seem to overlap and and cause odd
> behavior and/or crashes when trying to set up multiple UARTs,
>
> Signed-off-by: Seth Kintigh <[e-mail]>
> ---
> Phil, I hope this is the right format.

Better but still incorrect.

This is the 2nd version of your patch, if you add "v2" in the patch
subject, it helps reviewers to see this is not the same as your
previous patch.
I think your next revision should have "v3" in the subject line.

You should not send new patches in-reply to previous one, it is
clearer to start a new thread directly. (same than previous point,
reviewers get confuse in the thread to see which patch they are
referring to).

I tried to apply your patch but get an error:

$ git am seth_stm32f2xx_regsize.mbox
Applying: hw/arm/stm32f205: Fix the UART and Timer region size
error: patch failed: hw/timer/stm32f2xx_timer.c:308
error: hw/timer/stm32f2xx_timer.c: patch does not apply
Patch failed at 0001 hw/arm/stm32f205: Fix the UART and Timer region size
Use 'git am --show-current-patch' to see the failed patch

I suppose the problem is you inserted your patch in the middle of a
mail, or you appended the review comments to your patch.
Please have a look at the "Submit a patch" wiki:
https://wiki.qemu.org/Contribute/SubmitAPatch#Use_git_format-patch
"Use the right diff format. git format-patch will produce patch emails
in the right format"

If you plan to contribute again (for which you are welcome!) I
recommend you to correctly setup git using the previous link, and for
GMail you can also refer to:
https://git-scm.com/docs/git-send-email#_examples
(don't forget to generate an app-specific password).

Regards,

Phil.

>
> PMM, my original changes were made on an old version, but I made the patch
> from the latest version per the instructions. I'm glad to hear that serial
> port limit is gone!
>
>  hw/char/stm32f2xx_usart.c  | 2 +-
>  hw/timer/stm32f2xx_timer.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
> index 032b5fda13..f3363a2952 100644
> --- a/hw/char/stm32f2xx_usart.c
> +++ b/hw/char/stm32f2xx_usart.c
> @@ -202,7 +202,7 @@ static void stm32f2xx_usart_init(Object *obj)
>      sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
>
>      memory_region_init_io(&s->mmio, obj, &stm32f2xx_usart_ops, s,
> -                          TYPE_STM32F2XX_USART, 0x2000);
> +                          TYPE_STM32F2XX_USART, 0x400);
>      sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
>  }
>
> diff --git a/hw/timer/stm32f2xx_timer.c b/hw/timer/stm32f2xx_timer.c
> index 58fc7b1188..ae744d1642 100644
> --- a/hw/timer/stm32f2xx_timer.c
> +++ b/hw/timer/stm32f2xx_timer.c
> @@ -308,7 +308,7 @@ static void stm32f2xx_timer_init(Object *obj)
>      sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
>
>      memory_region_init_io(&s->iomem, obj, &stm32f2xx_timer_ops, s,
> -                          "stm32f2xx_timer", 0x4000);
> +                          "stm32f2xx_timer", 0x400);
>      sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>
>      s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, stm32f2xx_timer_interrupt,
> s);
>
>
> On Thu, Nov 15, 2018 at 7:05 AM Peter Maydell <[e-mail]>
> wrote:
>
> > On 4 November 2018 at 07:42, Seth K <[e-mail]> wrote:
> > > I corrected these 2 memory regions based on specifications from the chip
> > > manufacturer. The existing ranges seem to overlap and and cause odd
> > > behavior and/or crashes when trying to set up multiple UARTs,
> > > I also played with changing MAX_SERIAL_PORTS to 8 to match the hardware,
> > > but I did not include that in this patch as I never fully tested its
> > > effects.
> >
> > Hi; thanks for the patch. As Philippe says, it looks good,
> > but the one thing we definitely need is a Signed-off-by:
> > line from you.
> >
> > A minor note -- there is no "MAX_SERIAL_PORTS" definition
> > in QEMU any more: we removed that artificial limitation
> > earlier this year. Maybe you're basing your patch on an
> > older version of QEMU? It's best to use git master for
> > development. Our SoC model defines 6 uarts (STM_NUM_USARTS)
> > in hw/arm/stm32f205_soc.c, which should all now be connectable
> > on the command line, though I haven't tested that or checked
> > whether the hardware has 6 or some other number...
> >
> > thanks
> > -- PMM
> >

Peter Maydell Nov. 19, 2018, 11:08 a.m. UTC | #2

On 19 November 2018 at 10:43, Philippe Mathieu-Daudé <[e-mail]> wrote:
> Hi Seth,
>
> On Mon, Nov 19, 2018 at 4:17 AM Seth K <[e-mail]> wrote:
>>
>> From: Seth Kintigh <[e-mail]>
>>
>> I corrected these 2 memory regions based on specifications from the chip
>> manufacturer. The existing ranges seem to overlap and and cause odd
>> behavior and/or crashes when trying to set up multiple UARTs,
>>
>> Signed-off-by: Seth Kintigh <[e-mail]>
>> ---
>> Phil, I hope this is the right format.
>
> Better but still incorrect.

What Phil says below is true, but since this is a simple
patch I have applied it by hand to my target-arm.next branch,
so it will go into the next release of QEMU. Thanks for your
contribution! (I rewrote the commit message a bit to make
it fit in with the usual style we use for our commit messages;
I hope that's OK.)

> I tried to apply your patch but get an error:
>
> $ git am seth_stm32f2xx_regsize.mbox

I suspect this is because the email is in dual text/HTML format.

thanks
-- PMM

Philippe Mathieu-Daudé Nov. 19, 2018, 11:35 a.m. UTC | #3

On Mon, Nov 19, 2018 at 12:08 PM Peter Maydell <[e-mail]> wrote:
> On 19 November 2018 at 10:43, Philippe Mathieu-Daudé <[e-mail]> wrote:
> > Hi Seth,
> >
> > On Mon, Nov 19, 2018 at 4:17 AM Seth K <[e-mail]> wrote:
> >>
> >> From: Seth Kintigh <[e-mail]>
> >>
> >> I corrected these 2 memory regions based on specifications from the chip
> >> manufacturer. The existing ranges seem to overlap and and cause odd
> >> behavior and/or crashes when trying to set up multiple UARTs,
> >>
> >> Signed-off-by: Seth Kintigh <[e-mail]>
> >> ---
> >> Phil, I hope this is the right format.
> >
> > Better but still incorrect.
>
> What Phil says below is true, but since this is a simple
> patch I have applied it by hand to my target-arm.next branch,
> so it will go into the next release of QEMU. Thanks for your
> contribution! (I rewrote the commit message a bit to make
> it fit in with the usual style we use for our commit messages;
> I hope that's OK.)

Thanks Peter!

You can also add:
Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
Tested-by: Philippe Mathieu-Daudé <[e-mail]>

Regards,

Phil.

>
> > I tried to apply your patch but get an error:
> >
> > $ git am seth_stm32f2xx_regsize.mbox
>
> I suspect this is because the email is in dual text/HTML format.
>
> thanks
> -- PMM

Alistair Francis Nov. 20, 2018, 5:15 p.m. UTC | #4

On Mon, Nov 19, 2018 at 3:35 AM Philippe Mathieu-Daudé <[e-mail]> wrote:
>
> On Mon, Nov 19, 2018 at 12:08 PM Peter Maydell <[e-mail]> wrote:
> > On 19 November 2018 at 10:43, Philippe Mathieu-Daudé <[e-mail]> wrote:
> > > Hi Seth,
> > >
> > > On Mon, Nov 19, 2018 at 4:17 AM Seth K <[e-mail]> wrote:
> > >>
> > >> From: Seth Kintigh <[e-mail]>
> > >>
> > >> I corrected these 2 memory regions based on specifications from the chip
> > >> manufacturer. The existing ranges seem to overlap and and cause odd
> > >> behavior and/or crashes when trying to set up multiple UARTs,
> > >>
> > >> Signed-off-by: Seth Kintigh <[e-mail]>
> > >> ---
> > >> Phil, I hope this is the right format.
> > >
> > > Better but still incorrect.
> >
> > What Phil says below is true, but since this is a simple
> > patch I have applied it by hand to my target-arm.next branch,
> > so it will go into the next release of QEMU. Thanks for your
> > contribution! (I rewrote the commit message a bit to make
> > it fit in with the usual style we use for our commit messages;
> > I hope that's OK.)
>
> Thanks Peter!
>
> You can also add:
> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> Tested-by: Philippe Mathieu-Daudé <[e-mail]>
>
> Regards,

Thanks for applying this Peter and thanks for the patch Seth.

Alistair

>
> Phil.
>
> >
> > > I tried to apply your patch but get an error:
> > >
> > > $ git am seth_stm32f2xx_regsize.mbox
> >
> > I suspect this is because the email is in dual text/HTML format.
> >
> > thanks
> > -- PMM
>

Seth K Nov. 26, 2018, 1:54 a.m. UTC | #5

Thanks for all your help and I'm glad to contribute.
Seth

On Tue, Nov 20, 2018 at 12:15 PM Alistair Francis <[e-mail]>
wrote:

> On Mon, Nov 19, 2018 at 3:35 AM Philippe Mathieu-Daudé <[e-mail]>
> wrote:
> >
> > On Mon, Nov 19, 2018 at 12:08 PM Peter Maydell <[e-mail]>
> wrote:
> > > On 19 November 2018 at 10:43, Philippe Mathieu-Daudé <[e-mail]>
> wrote:
> > > > Hi Seth,
> > > >
> > > > On Mon, Nov 19, 2018 at 4:17 AM Seth K <[e-mail]> wrote:
> > > >>
> > > >> From: Seth Kintigh <[e-mail]>
> > > >>
> > > >> I corrected these 2 memory regions based on specifications from the
> chip
> > > >> manufacturer. The existing ranges seem to overlap and and cause odd
> > > >> behavior and/or crashes when trying to set up multiple UARTs,
> > > >>
> > > >> Signed-off-by: Seth Kintigh <[e-mail]>
> > > >> ---
> > > >> Phil, I hope this is the right format.
> > > >
> > > > Better but still incorrect.
> > >
> > > What Phil says below is true, but since this is a simple
> > > patch I have applied it by hand to my target-arm.next branch,
> > > so it will go into the next release of QEMU. Thanks for your
> > > contribution! (I rewrote the commit message a bit to make
> > > it fit in with the usual style we use for our commit messages;
> > > I hope that's OK.)
> >
> > Thanks Peter!
> >
> > You can also add:
> > Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> > Tested-by: Philippe Mathieu-Daudé <[e-mail]>
> >
> > Regards,
>
> Thanks for applying this Peter and thanks for the patch Seth.
>
> Alistair
>
> >
> > Phil.
> >
> > >
> > > > I tried to apply your patch but get an error:
> > > >
> > > > $ git am seth_stm32f2xx_regsize.mbox
> > >
> > > I suspect this is because the email is in dual text/HTML format.
> > >
> > > thanks
> > > -- PMM
> >
>

Patch

diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
index 032b5fda13..f3363a2952 100644
--- a/hw/char/stm32f2xx_usart.c
+++ b/hw/char/stm32f2xx_usart.c
@@ -202,7 +202,7 @@  static void stm32f2xx_usart_init(Object *obj)
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);

     memory_region_init_io(&s->mmio, obj, &stm32f2xx_usart_ops, s,
-                          TYPE_STM32F2XX_USART, 0x2000);
+                          TYPE_STM32F2XX_USART, 0x400);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
 }

diff --git a/hw/timer/stm32f2xx_timer.c b/hw/timer/stm32f2xx_timer.c
index 58fc7b1188..ae744d1642 100644
--- a/hw/timer/stm32f2xx_timer.c
+++ b/hw/timer/stm32f2xx_timer.c
@@ -308,7 +308,7 @@  static void stm32f2xx_timer_init(Object *obj)
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);

     memory_region_init_io(&s->iomem, obj, &stm32f2xx_timer_ops, s,
-                          "stm32f2xx_timer", 0x4000);
+                          "stm32f2xx_timer", 0x400);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);

     s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, stm32f2xx_timer_interrupt,