Re: [PATCH 09/19] hw/intc/xilinx_intc: Only expect big-endian accesses

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Tue, Nov 05, 2024 at 02:04:21PM +0100, Philippe Mathieu-Daudé wrote:
> Per the datasheet (reference added in file header, p.9)
> 'Programming Model' -> 'Register Data Types and Organization':
> 
>     "The XPS INTC registers are read as big-endian data"

Hi Phil,

Some of these devices exist in both big and little endian versions.
So far we've reused the same model by using DEVICE_NATIVE_ENDIAN.

Here's the little endian version:
https://docs.amd.com/v/u/en-US/ds747_axi_intc

Can we have add property to select the endianess?
For the Xilinx use-cases I think it may be a good idea to default it
to little endian and have the big-endian machines explicitly set it.

Cheers,
Edgar


> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/intc/xilinx_intc.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
> index 1762b34564e..71f743a1f14 100644
> --- a/hw/intc/xilinx_intc.c
> +++ b/hw/intc/xilinx_intc.c
> @@ -3,6 +3,9 @@
>   *
>   * Copyright (c) 2009 Edgar E. Iglesias.
>   *
> + * https://docs.amd.com/v/u/en-US/xps_intc
> + * DS572: LogiCORE IP XPS Interrupt Controller (v2.01a)
> + *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to deal
>   * in the Software without restriction, including without limitation the rights
> @@ -143,12 +146,20 @@ static void pic_write(void *opaque, hwaddr addr,
>  static const MemoryRegionOps pic_ops = {
>      .read = pic_read,
>      .write = pic_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    /* The XPS INTC registers are read as big-endian data. */
> +    .endianness = DEVICE_BIG_ENDIAN,
>      .impl = {
>          .min_access_size = 4,
>          .max_access_size = 4,
>      },
>      .valid = {
> +        /*
> +         * All XPS INTC registers are accessed through the PLB interface.
> +         * The base address for these registers is provided by the
> +         * configuration parameter, C_BASEADDR. Each register is 32 bits
> +         * although some bits may be unused and is accessed on a 4-byte
> +         * boundary offset from the base address.
> +         */
>          .min_access_size = 4,
>          .max_access_size = 4,
>      },
> -- 
> 2.45.2
>