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

[Philippe Mathieu-Daudé]

On 7/6/21 9:22 PM, Mark Cave-Ayland wrote:
> On 06/07/2021 18:18, Philippe Mathieu-Daudé wrote:
>> 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.
> 
> As per the comment, the removal of .impl.max_access_size was to imply
> that the ultimate limit is determined by a dynamic property more than
> the hard-coded limit i.e if you wanted to increase the stride you would
> increase it_shift first and then adjust the .impl.max_access_size to
> match accordingly.
> 
> At this point we're probably heading into personal preference territory,
> so if you are happy to merge this via mips-next then I'm happy for you
> to make the final decision :)

No, I'll take your patch. I missed the it_shift use, and I plan to kill
it next dev cycle because I'm tired by its misuses. Probably with a
new memory device similar to:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg795421.html

$ git grep -w it_shift | wc -l
50

Doable.

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