Re: [Qemu-devel] [PATCH] IDE: MMIO IDE device control should be little endian

Re: [Qemu-devel] [PATCH] IDE: MMIO IDE device control should be little endian

[Peter Maydell]

You forgot to cc qemu-devel...

On 22 August 2014 09:52, Valentin Manea <qemu@mrs.ro> wrote:
> Set the IDE MMIO memory type to little endian. The ATA specs identify
> words part of the control commands encoded as little endian.
> While this has no impact on little endian systems, it's required for big
> endian systems(eg OpenRisc).
>
> Signed-off-by: Valentin Manea <valentin.manea@gmail.com>
> ---
>  hw/ide/mmio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/mmio.c b/hw/ide/mmio.c
> index 01c1d0e..334c8cc 100644
> --- a/hw/ide/mmio.c
> +++ b/hw/ide/mmio.c
> @@ -82,7 +82,7 @@ static void mmio_ide_write(void *opaque, hwaddr addr,
>  static const MemoryRegionOps mmio_ide_ops = {
>      .read = mmio_ide_read,
>      .write = mmio_ide_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>
>  static uint64_t mmio_ide_status_read(void *opaque, hwaddr addr,
> @@ -102,7 +102,7 @@ static void mmio_ide_cmd_write(void *opaque, hwaddr
> addr,
>  static const MemoryRegionOps mmio_ide_cs_ops = {
>      .read = mmio_ide_status_read,
>      .write = mmio_ide_cmd_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>
>  static const VMStateDescription vmstate_ide_mmio = {
> --
> 1.9.1

thanks
-- PMM

Re: [Qemu-devel] [Qemu-trivial] [PATCH] IDE: MMIO IDE device control should be little endian

[Michael Tokarev]

22.08.2014 12:52, Valentin Manea wrote:
> Set the IDE MMIO memory type to little endian. The ATA specs identify
> words part of the control commands encoded as little endian.
> While this has no impact on little endian systems, it's required for big
> endian systems(eg OpenRisc).

While the patch itself is simple, I'm not sure it is "trivial enough".
And you didn't Cc neither qemu-devel@ nor the IDE subsystem "odd fixer"
maintainers (kwolf@ and stefanha@).  I'm not applying this for now to
trivial tree.

Thanks,

/mjt

> Signed-off-by: Valentin Manea <valentin.manea@gmail.com>
> ---
>  hw/ide/mmio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/mmio.c b/hw/ide/mmio.c
> index 01c1d0e..334c8cc 100644
> --- a/hw/ide/mmio.c
> +++ b/hw/ide/mmio.c
> @@ -82,7 +82,7 @@ static void mmio_ide_write(void *opaque, hwaddr addr,
>  static const MemoryRegionOps mmio_ide_ops = {
>      .read = mmio_ide_read,
>      .write = mmio_ide_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
> 
>  static uint64_t mmio_ide_status_read(void *opaque, hwaddr addr,
> @@ -102,7 +102,7 @@ static void mmio_ide_cmd_write(void *opaque, hwaddr
> addr,
>  static const MemoryRegionOps mmio_ide_cs_ops = {
>      .read = mmio_ide_status_read,
>      .write = mmio_ide_cmd_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
> 
>  static const VMStateDescription vmstate_ide_mmio = {
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH] IDE: MMIO IDE device control should be little endian

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 905 bytes --]

On Sun, Aug 24, 2014 at 01:24:29PM +0400, Michael Tokarev wrote:
> 22.08.2014 12:52, Valentin Manea wrote:
> > Set the IDE MMIO memory type to little endian. The ATA specs identify
> > words part of the control commands encoded as little endian.
> > While this has no impact on little endian systems, it's required for big
> > endian systems(eg OpenRisc).
> 
> While the patch itself is simple, I'm not sure it is "trivial enough".
> And you didn't Cc neither qemu-devel@ nor the IDE subsystem "odd fixer"
> maintainers (kwolf@ and stefanha@).  I'm not applying this for now to
> trivial tree.

Hi Valentin,
Please resend the patch to qemu-devel@nongnu.org and CC Kevin Wolf
<kwolf@redhat.com> and Stefan Hajnoczi <stefanha@redhat.com>.

All patches must go through the mailing list so the community has a
chance to review and comment before a committer applies them.

Thanks,
Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-trivial] [PATCH] IDE: MMIO IDE device control should be little endian

[Valentin Manea]

On 2014-08-28 14:13, Stefan Hajnoczi wrote:
> Hi Valentin,
> Please resend the patch to qemu-devel@nongnu.org and CC Kevin Wolf
> <kwolf@redhat.com> and Stefan Hajnoczi <stefanha@redhat.com>.
> 
> All patches must go through the mailing list so the community has a
> chance to review and comment before a committer applies them.

Hi Stefan,

  Thanks for the tips, I have resent the patch.

Thanks,
Valentin

[Qemu-devel] [PATCH] IDE: MMIO IDE device control should be little endian

[Valentin Manea]

Set the IDE MMIO memory type to little endian. The ATA specs identify
words part of the control commands encoded as little endian.
While this has no impact on little endian systems, it's required for big
endian systems(eg OpenRisc).

Signed-off-by: Valentin Manea <valentin.manea@gmail.com>
---
 hw/ide/mmio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/mmio.c b/hw/ide/mmio.c
index 01c1d0e..334c8cc 100644
--- a/hw/ide/mmio.c
+++ b/hw/ide/mmio.c
@@ -82,7 +82,7 @@ static void mmio_ide_write(void *opaque, hwaddr addr,
 static const MemoryRegionOps mmio_ide_ops = {
     .read = mmio_ide_read,
     .write = mmio_ide_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };

 static uint64_t mmio_ide_status_read(void *opaque, hwaddr addr,
@@ -102,7 +102,7 @@ static void mmio_ide_cmd_write(void *opaque, hwaddr
addr,
 static const MemoryRegionOps mmio_ide_cs_ops = {
     .read = mmio_ide_status_read,
     .write = mmio_ide_cmd_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };

 static const VMStateDescription vmstate_ide_mmio = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH] IDE: MMIO IDE device control should be little endian

[Valentin Manea]

Hi,

  Did anybody get the chance to review this patch?
  It would be quite nice to integrate it before all the other openrisc changes, to get
the IDE working also.

Thanks,
Valentin
On 2014-08-31 11:32, Valentin Manea wrote:
> 
> Set the IDE MMIO memory type to little endian. The ATA specs identify
> words part of the control commands encoded as little endian.
> While this has no impact on little endian systems, it's required for big
> endian systems(eg OpenRisc).
> 
> Signed-off-by: Valentin Manea <valentin.manea@gmail.com>
> ---
>  hw/ide/mmio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/mmio.c b/hw/ide/mmio.c
> index 01c1d0e..334c8cc 100644
> --- a/hw/ide/mmio.c
> +++ b/hw/ide/mmio.c
> @@ -82,7 +82,7 @@ static void mmio_ide_write(void *opaque, hwaddr addr,
>  static const MemoryRegionOps mmio_ide_ops = {
>      .read = mmio_ide_read,
>      .write = mmio_ide_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
> 
>  static uint64_t mmio_ide_status_read(void *opaque, hwaddr addr,
> @@ -102,7 +102,7 @@ static void mmio_ide_cmd_write(void *opaque, hwaddr
> addr,
>  static const MemoryRegionOps mmio_ide_cs_ops = {
>      .read = mmio_ide_status_read,
>      .write = mmio_ide_cmd_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
> 
>  static const VMStateDescription vmstate_ide_mmio = {
> 

Re: [Qemu-devel] [PATCH] IDE: MMIO IDE device control should be little endian

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On Sun, Sep 07, 2014 at 10:07:10PM +0300, Valentin Manea wrote:
>   Did anybody get the chance to review this patch?
>   It would be quite nice to integrate it before all the other openrisc changes, to get
> the IDE working also.

Resending a patch as a reply in an existing thread increases the chance
of it getting overlooked.  Threaded mail clients bury the mail as part
of an old conversation.  Please send patches as top-level emails instead
so they don't get missed.

Anyway...I've spotted it now and will review.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] IDE: MMIO IDE device control should be little endian

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]

On Sun, Aug 31, 2014 at 11:32:08AM +0300, Valentin Manea wrote:
> 
> Set the IDE MMIO memory type to little endian. The ATA specs identify
> words part of the control commands encoded as little endian.
> While this has no impact on little endian systems, it's required for big
> endian systems(eg OpenRisc).
> 
> Signed-off-by: Valentin Manea <valentin.manea@gmail.com>
> ---
>  hw/ide/mmio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/mmio.c b/hw/ide/mmio.c
> index 01c1d0e..334c8cc 100644
> --- a/hw/ide/mmio.c
> +++ b/hw/ide/mmio.c
> @@ -82,7 +82,7 @@ static void mmio_ide_write(void *opaque, hwaddr addr,
>  static const MemoryRegionOps mmio_ide_ops = {
>      .read = mmio_ide_read,
>      .write = mmio_ide_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
> 
>  static uint64_t mmio_ide_status_read(void *opaque, hwaddr addr,
> @@ -102,7 +102,7 @@ static void mmio_ide_cmd_write(void *opaque, hwaddr
> addr,
>  static const MemoryRegionOps mmio_ide_cs_ops = {
>      .read = mmio_ide_status_read,
>      .write = mmio_ide_cmd_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
> 
>  static const VMStateDescription vmstate_ide_mmio = {

This patch changes the semantics of the sh4eb target (with the r2d
machine type), where the mmio-ide registers currently don't need
byteswapping.

I guess a real big-endian SH4 with MMIO IDE *should* byteswap but want
to double-check.

Does anyone know what the expected behavior here is?

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] IDE: MMIO IDE device control should be little endian

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 08/09/2014 11:07, Stefan Hajnoczi ha scritto:
> This patch changes the semantics of the sh4eb target (with the r2d
>  machine type), where the mmio-ide registers currently don't need 
> byteswapping.
> 
> I guess a real big-endian SH4 with MMIO IDE *should* byteswap but 
> want to double-check.
> 
> Does anyone know what the expected behavior here is?

Nope, but I agree that it would likely be a bug fix for sh4eb.

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJUDXLUAAoJEBvWZb6bTYbyELoP/jwo9SbUupQDDVlvEF1ToB+M
MrhOy5umG3qIbrJrl3biiTvxkSpQ9TrfHT1jtk1caGsMos6lRfajVOp1L1JFG4el
l/QSMAwLv53yWwGkFeKOH/OtJuraM79iSTxq042mE6w6FP95vRYP+IYLBCbwJY59
qyHFk9/uP0UztdnwzoNcjeyJkGtQAET7seU4Xf25te2q3DK2cO+/jA3Ew16ebQYd
nCf3I7KeXNZT1kSZvctwvt0OM4De8IjHXgU76rZXAy1z/sYSEmd/QUzQkHwFHFXA
Hpza+LgLvIheEWcOtluyOlZH533JTmLSj2TdNDvcDq+W7pPXO2ncf64y8q+F3roi
CH4xHElv88D3bLiy2aE57k1xxuwnJ/xnpxQ0isMosR9fs99dd/k2sHnnaKuBYYVI
B4fM+dRfqO5zxNaPUA1zJ3VpQl66C8CXI/iLJbMfgqHYCnAEHgHZA0oLuL8o2RFJ
Nl+eulnlcE4XUvzkCsSe19c2g+9iG13ZfVBK4RIth23jSrgw6EhmRCb1Fj4Rf6Ej
oArfBcFQr9EZZ8acMbo7wFIwH2Y73+tW65IauFGVDAoOAeLcVyHZ+fudPEWYCLb6
ak9BcXnt5zkAzZgoc5CZS9hLeGKuMsMqkrKaXFu5RybqdaH6LCJfyYNkbdqn37LM
HOAIkqKZ+LES8fXJBoZe
=pOCE
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH] IDE: MMIO IDE device control should be little endian

[Michael Tokarev]

08.09.2014 13:07, Stefan Hajnoczi wrote:

> This patch changes the semantics of the sh4eb target (with the r2d
> machine type), where the mmio-ide registers currently don't need
> byteswapping.
> 
> I guess a real big-endian SH4 with MMIO IDE *should* byteswap but want
> to double-check.
> 
> Does anyone know what the expected behavior here is?

That's exactly the reason I don't think this is a -trivial material :)

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] IDE: MMIO IDE device control should be little endian

[Peter Maydell]

On 8 September 2014 10:07, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> This patch changes the semantics of the sh4eb target (with the r2d
> machine type), where the mmio-ide registers currently don't need
> byteswapping.
>
> I guess a real big-endian SH4 with MMIO IDE *should* byteswap but want
> to double-check.
>
> Does anyone know what the expected behavior here is?

I had a look for the documentation, and the SH7751R
user's manual (hardware) is freely downloadable from
the Renesas website. It says:
 "The PCMCIA interface is supported only in little-endian mode."

So I think we don't need to care...

-- PMM

Re: [Qemu-devel] [PATCH] IDE: MMIO IDE device control should be little endian

[Kevin Wolf]

Am 31.08.2014 um 10:32 hat Valentin Manea geschrieben:
> 
> Set the IDE MMIO memory type to little endian. The ATA specs identify
> words part of the control commands encoded as little endian.
> While this has no impact on little endian systems, it's required for big
> endian systems(eg OpenRisc).
> 
> Signed-off-by: Valentin Manea <valentin.manea@gmail.com>

Thanks, applied to the block branch.

>  static uint64_t mmio_ide_status_read(void *opaque, hwaddr addr,
> @@ -102,7 +102,7 @@ static void mmio_ide_cmd_write(void *opaque, hwaddr
> addr,

Please make sure that your patch isn't corrupted by your mail client,
like by this line wrap. I had to manually fix the patch before I could
apply it. For larger patches, I would simply have rejected it.

Kevin

Re: [Qemu-devel] [PATCH] IDE: MMIO IDE device control should be little endian

[Valentin Manea]

Hi Kevin
On 2014-09-09 11:58, Kevin Wolf wrote:
> Thanks, applied to the block branch.

Thanks a lot

> Please make sure that your patch isn't corrupted by your mail client,
> like by this line wrap. I had to manually fix the patch before I could
> apply it. For larger patches, I would simply have rejected it.


Really sorry, my mail client was playing tricks on me. I will make sure to triple check
next time.

Regards,
Valentin