[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/4] dp8393x: don't force 32-bit register access
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH 1/4] dp8393x: don't force 32-bit register access |
Date: |
Tue, 6 Jul 2021 23:00:48 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 |
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(-)
[PATCH 2/4] dp8393x: Replace address_space_rw(is_write=1) by address_space_write(), Mark Cave-Ayland, 2021/07/05
[PATCH 3/4] dp8393x: Store CAM registers as 16-bit, Mark Cave-Ayland, 2021/07/05
[PATCH 4/4] dp8393x: Rewrite dp8393x_get() / dp8393x_put(), Mark Cave-Ayland, 2021/07/05