IDE: MMIO IDE device control should be little endian

PATCH In review Valentin Manea 7 participants 14 messages Aug 22, 2014 – Sep 9, 2014

Share & tools

Valentin Manea

author

2014-08-22 13:44 UTC #0 lore raw

PATCH 1/1 IDE: MMIO IDE device control should be little endian

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 <[email protected]>

---
 hw/ide/mmio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/ide/mmio.c +1 -1

index 01c1d0e..334c8cc 100644
@@ -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 = {

Signature · 1 line

1.9.1

Peter Maydell

2014-08-22 09:22 UTC #1 lore raw

You forgot to cc qemu-devel...

Quoted 32 lines · Valentin Manea

Signed-off-by Valentin Manea <[email protected]>

---
 hw/ide/mmio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/ide/mmio.c +2 -2

index 01c1d0e..334c8cc 100644
@@ -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,
 };

Quoted 3 lines

static const VMStateDescription vmstate_ide_mmio = {
--
1.9.1

thanks
-- PMM

Michael Tokarev

2014-08-24 09:24 UTC #2 lore raw

In reply to Valentin Manea

Quoted 4 lines · 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).

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

Quoted 29 lines

Signed-off-by Valentin Manea <[email protected]>

---
 hw/ide/mmio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/ide/mmio.c +2 -2

index 01c1d0e..334c8cc 100644
@@ -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 = {

Stefan Hajnoczi

2014-08-28 11:13 UTC #3 lore raw

Quoted 10 lines · Michael Tokarev

Quoted 4 lines · 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).
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 [email protected] and CC Kevin Wolf <[email protected]> and Stefan Hajnoczi <[email protected]>.

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

Valentin Manea

author

2014-08-31 08:33 UTC #4 lore raw

Quoted 6 lines · Stefan Hajnoczi

Hi Valentin,
Please resend the patch to [email protected] and CC Kevin Wolf <[email protected]> and Stefan Hajnoczi <[email protected]>.
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

Valentin Manea

2014-08-31 08:32 UTC #5 lore raw

PATCH 1/1 IDE: MMIO IDE device control should be little endian

In reply to Valentin Manea

Signed-off-by Valentin Manea <[email protected]>

---
 hw/ide/mmio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/ide/mmio.c +1 -1

index 01c1d0e..334c8cc 100644
@@ -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

static const VMStateDescription vmstate_ide_mmio = {

Signature · 1 line

1.9.1

Valentin Manea

2014-09-07 19:07 UTC #6 lore raw

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

Quoted 35 lines · Valentin Manea

Signed-off-by Valentin Manea <[email protected]>

---
 hw/ide/mmio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/ide/mmio.c +2 -2

index 01c1d0e..334c8cc 100644
@@ -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 = {

Stefan Hajnoczi

2014-09-08 08:52 UTC #7 lore raw

Quoted 3 lines · Valentin Manea

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

Stefan Hajnoczi

2014-09-08 09:08 UTC #8 lore raw

In reply to Valentin Manea

Quoted 33 lines · Valentin Manea

Signed-off-by Valentin Manea <[email protected]>

---
 hw/ide/mmio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/ide/mmio.c +2 -2

index 01c1d0e..334c8cc 100644
@@ -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,
 };

Quoted 1 line

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

Paolo Bonzini

2014-09-08 09:12 UTC #9 lore raw

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

Il 08/09/2014 11:07, Stefan Hajnoczi ha scritto:

Quoted 8 lines

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-----

Michael Tokarev

2014-09-08 10:59 UTC #10 lore raw

In reply to Stefan Hajnoczi

Quoted 8 lines · Stefan Hajnoczi

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

Peter Maydell

2014-09-08 12:14 UTC #11 lore raw

In reply to Stefan Hajnoczi

Quoted 8 lines · Stefan Hajnoczi

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

Kevin Wolf

Applied

2014-09-09 08:59 UTC #12 lore raw

In reply to Valentin Manea

Am 31.08.2014 um 10:32 hat Valentin Manea geschrieben:

Quoted 7 lines

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 <[email protected]>

Thanks, applied to the block branch.

Review static void mmio_ide_cmd_write(void *opaque, hwaddr · line 102

Quoted 3 lines

static uint64_t mmio_ide_status_read(void *opaque, hwaddr addr,
(quoted diff) +0 -0
@@ -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

Valentin Manea

2014-09-09 19:19 UTC #13 lore raw

Hi Kevin

Quoted 1 line · Kevin Wolf

Thanks, applied to the block branch.

Thanks a lot

Quoted 3 lines

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