Re: [PATCH 1/4] dp8393x: don't force 32-bit register access

[Philippe Mathieu-Daudé]

On 7/5/21 11:49 PM, Mark Cave-Ayland wrote:
> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" set 
> .impl.min_access_size
> and .impl.max_access_size to 4 to try and fix the Linux jazzsonic driver 
> which uses
> 32-bit accesses.
> 
> The problem with forcing the register access to 32-bit in this way is that 
> since the
> dp8393x uses 16-bit registers, a manual endian swap is required for devices 
> on big
> endian machines with 32-bit accesses.
> 
> For both access sizes and machine endians the QEMU memory API can do the 
> right thing
> automatically: all that is needed is to set .impl.min_access_size to 2 to 
> declare that
> the dp8393x implements 16-bit registers.
> 
> Normally .impl.max_access_size should also be set to 2, however that doesn't 
> quite
> work in this case since the register stride is specified using a (dynamic) 
> it_shift
> property which is applied during the MMIO access itself. The effect of this 
> is that
> for a 32-bit access the memory API performs 2 x 16-bit accesses, but the use 
> of
> it_shift within the MMIO access itself causes the register value to be 
> repeated in both
> the top 16-bits and bottom 16-bits. The Linux jazzsonic driver expects the 
> stride to be
> zero-extended up to access size and therefore fails to correctly detect the 
> dp8393x
> device due to the extra data in the top 16-bits.
> 
> The solution here is to remove .impl.max_access_size so that the memory API 
> will
> correctly zero-extend the 16-bit registers to the access size up to and 
> including
> it_shift. Since it_shift is never greater than 2 than this will always do the 
> right
> thing for both 16-bit and 32-bit accesses regardless of the machine endian, 
> allowing
> the manual endian swap code to be removed.

Removing .impl.max_access_size means now it has default, which is 4.
See access_with_adjusted_size:

static MemTxResult access_with_adjusted_size(hwaddr addr,
                                      uint64_t *value,
                                      unsigned size,
                                      unsigned access_size_min,
                                      unsigned access_size_max,
    ...
    if (!access_size_min) {
        access_size_min = 1;
    }
    if (!access_size_max) {
        access_size_max = 4;
    }

called as:

    access_with_adjusted_size(addr, &data, size,
                              mr->ops->impl.min_access_size,
                              mr->ops->impl.max_access_size,
                              memory_region_write_with_attrs_accessor,
                              mr, attrs);

So if you don't mind I'll keep .impl.max_access_size = 4 and update
the comment.

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
> ---
>  hw/net/dp8393x.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 11810c9b60..44a1955015 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -602,15 +602,14 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, 
> unsigned int size)
>  
>      trace_dp8393x_read(reg, reg_names[reg], val, size);
>  
> -    return s->big_endian ? val << 16 : val;
> +    return val;
>  }
>  
> -static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
> +static void dp8393x_write(void *opaque, hwaddr addr, uint64_t val,
>                            unsigned int size)
>  {
>      dp8393xState *s = opaque;
>      int reg = addr >> s->it_shift;
> -    uint32_t val = s->big_endian ? data >> 16 : data;
>  
>      trace_dp8393x_write(reg, reg_names[reg], val, size);
>  
> @@ -691,11 +690,16 @@ static void dp8393x_write(void *opaque, hwaddr addr, 
> uint64_t data,
>      }
>  }
>  
> +/*
> + * Since .impl.max_access_size is effectively controlled by the it_shift
> + * property, leave it unspecified for now to allow the memory API to
> + * correctly zero extend the 16-bit register values to the access size up to 
> and
> + * including it_shift.
> + */
>  static const MemoryRegionOps dp8393x_ops = {
>      .read = dp8393x_read,
>      .write = dp8393x_write,
> -    .impl.min_access_size = 4,
> -    .impl.max_access_size = 4,
> +    .impl.min_access_size = 2,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
>