[Qemu-devel] [PATCH] Corrected memory regions

[Qemu-devel] [PATCH] Corrected memory regions

[Seth K]

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.
This is my first patch, I hope I did it correctly,
Seth Kintigh
---
 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);
-- 
2.11.0

Re: [Qemu-devel] [PATCH] Corrected memory regions

[Philippe Mathieu-Daudé]

Hi Seth,

On Wed, Nov 14, 2018 at 8:25 AM Seth K <skintigh@gmail.com> 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.
> This is my first patch, I hope I did it correctly,
> Seth Kintigh

Welcome to QEMU, and thanks for your patch :)

I checked the DocID15818 (Rev 15) datasheet for this chip and your
patch is technically correct.

I suggest the patch title as "hw/arm/stm32f205: Fix the UART and Timer
region size" (note: use present rather than past).

However for the legal stuff, the QEMU project states all patches "must
include a Signed-off-by: line".
Please have a look at the following page which explains everything you
need to know in order to send useful contributions:
https://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line

> ---

Note than the commit message stay in the repository history forever.
You can add information not related to the patch below the previous
"---" line, it will be cut out when your patch is applied to the
repository.
Here would go your "This is my first patch, I hope I did it correctly"
and signature.

Regards,

Phil.

>  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);
> --
> 2.11.0

Re: [Qemu-devel] [PATCH] Corrected memory regions

[Peter Maydell]

On 4 November 2018 at 07:42, Seth K <skintigh@gmail.com> 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

[Qemu-devel] [PATCH] hw/arm/stm32f205: Fix the UART and Timer region size

[Seth K]

From: Seth Kintigh <skintigh@gmail.com>

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 <skintigh@gmail.com>
---
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(-)

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 <peter.maydell@linaro.org>
wrote:

> On 4 November 2018 at 07:42, Seth K <skintigh@gmail.com> 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
>

Re: [Qemu-devel] [PATCH] hw/arm/stm32f205: Fix the UART and Timer region size

[Philippe Mathieu-Daudé]

Hi Seth,

On Mon, Nov 19, 2018 at 4:17 AM Seth K <skintigh@gmail.com> wrote:
>
> From: Seth Kintigh <skintigh@gmail.com>
>
> 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 <skintigh@gmail.com>
> ---
> 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 <peter.maydell@linaro.org>
> wrote:
>
> > On 4 November 2018 at 07:42, Seth K <skintigh@gmail.com> 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
> >

Re: [Qemu-devel] [PATCH] hw/arm/stm32f205: Fix the UART and Timer region size

[Peter Maydell]

On 19 November 2018 at 10:43, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Seth,
>
> On Mon, Nov 19, 2018 at 4:17 AM Seth K <skintigh@gmail.com> wrote:
>>
>> From: Seth Kintigh <skintigh@gmail.com>
>>
>> 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 <skintigh@gmail.com>
>> ---
>> 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

Re: [Qemu-devel] [PATCH] hw/arm/stm32f205: Fix the UART and Timer region size

[Philippe Mathieu-Daudé]

On Mon, Nov 19, 2018 at 12:08 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 November 2018 at 10:43, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > Hi Seth,
> >
> > On Mon, Nov 19, 2018 at 4:17 AM Seth K <skintigh@gmail.com> wrote:
> >>
> >> From: Seth Kintigh <skintigh@gmail.com>
> >>
> >> 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 <skintigh@gmail.com>
> >> ---
> >> 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é <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

Re: [Qemu-devel] [PATCH] hw/arm/stm32f205: Fix the UART and Timer region size

[Alistair Francis]

On Mon, Nov 19, 2018 at 3:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On Mon, Nov 19, 2018 at 12:08 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 19 November 2018 at 10:43, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > > Hi Seth,
> > >
> > > On Mon, Nov 19, 2018 at 4:17 AM Seth K <skintigh@gmail.com> wrote:
> > >>
> > >> From: Seth Kintigh <skintigh@gmail.com>
> > >>
> > >> 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 <skintigh@gmail.com>
> > >> ---
> > >> 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é <f4bug@amsat.org>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> 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
>

Re: [Qemu-devel] [PATCH] hw/arm/stm32f205: Fix the UART and Timer region size

[Seth K]

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

On Tue, Nov 20, 2018 at 12:15 PM Alistair Francis <alistair23@gmail.com>
wrote:

> On Mon, Nov 19, 2018 at 3:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
> >
> > On Mon, Nov 19, 2018 at 12:08 PM Peter Maydell <peter.maydell@linaro.org>
> wrote:
> > > On 19 November 2018 at 10:43, Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
> > > > Hi Seth,
> > > >
> > > > On Mon, Nov 19, 2018 at 4:17 AM Seth K <skintigh@gmail.com> wrote:
> > > >>
> > > >> From: Seth Kintigh <skintigh@gmail.com>
> > > >>
> > > >> 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 <skintigh@gmail.com>
> > > >> ---
> > > >> 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é <f4bug@amsat.org>
> > Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >
> > 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
> >
>