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 <valentin.manea@gmail.com>
---
 hw/ide/mmio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

 static const VMStateDescription vmstate_ide_mmio = {

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

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

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

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

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

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

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

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