* [PATCH 0/4] dp8393x: fixes and improvements @ 2021-07-05 21:49 Mark Cave-Ayland 2021-07-05 21:49 ` [PATCH 1/4] dp8393x: don't force 32-bit register access Mark Cave-Ayland ` (3 more replies) 0 siblings, 4 replies; 15+ messages in thread From: Mark Cave-Ayland @ 2021-07-05 21:49 UTC (permalink / raw) To: qemu-devel, jasowang, laurent, fthain, f4bug (was: [RFC PATCH 0/6] dp8393x: Housekeeping) This is an updated version of Phil's patchset previously posted at https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg00440.html which I've tested locally on Linux/m68k, MacOS and NetBSD/arc. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Changes since RFC: - Drop patch 1 "dp8393x: fix CAM descriptor entry index" from this patchset as it has already been queued to mips-next - Update patch 2 "dp8393x: don't force 32-bit register access" including improved comment explaining the current solution - Drop patch 3 "dp8393x: Restrict bus access to 16/32-bit operations" since it causes a regression with MacOS - Fix offsets in patch 6 "dp8393x: Rewrite dp8393x_get() / dp8393x_put()" Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Mark Cave-Ayland (1): dp8393x: don't force 32-bit register access Philippe Mathieu-Daudé (3): dp8393x: Replace address_space_rw(is_write=1) by address_space_write() dp8393x: Store CAM registers as 16-bit dp8393x: Rewrite dp8393x_get() / dp8393x_put() hw/net/dp8393x.c | 195 ++++++++++++++++++++--------------------------- 1 file changed, 81 insertions(+), 114 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/4] dp8393x: don't force 32-bit register access 2021-07-05 21:49 [PATCH 0/4] dp8393x: fixes and improvements Mark Cave-Ayland @ 2021-07-05 21:49 ` Mark Cave-Ayland 2021-07-06 17:18 ` Philippe Mathieu-Daudé 2021-07-06 23:51 ` Finn Thain 2021-07-05 21:49 ` [PATCH 2/4] dp8393x: Replace address_space_rw(is_write=1) by address_space_write() Mark Cave-Ayland ` (2 subsequent siblings) 3 siblings, 2 replies; 15+ messages in thread From: Mark Cave-Ayland @ 2021-07-05 21:49 UTC (permalink / raw) To: qemu-devel, jasowang, laurent, fthain, f4bug 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. 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, }; -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] dp8393x: don't force 32-bit register access 2021-07-05 21:49 ` [PATCH 1/4] dp8393x: don't force 32-bit register access Mark Cave-Ayland @ 2021-07-06 17:18 ` Philippe Mathieu-Daudé 2021-07-06 19:22 ` Mark Cave-Ayland 2021-07-06 23:51 ` Finn Thain 1 sibling, 1 reply; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-06 17:18 UTC (permalink / raw) To: Mark Cave-Ayland, qemu-devel, jasowang, laurent, fthain 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, > }; > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] dp8393x: don't force 32-bit register access 2021-07-06 17:18 ` Philippe Mathieu-Daudé @ 2021-07-06 19:22 ` Mark Cave-Ayland 2021-07-06 21:00 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 15+ messages in thread From: Mark Cave-Ayland @ 2021-07-06 19:22 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel, jasowang, laurent, fthain 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 :) >> 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, >> }; ATB, Mark. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] dp8393x: don't force 32-bit register access 2021-07-06 19:22 ` Mark Cave-Ayland @ 2021-07-06 21:00 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-06 21:00 UTC (permalink / raw) To: Mark Cave-Ayland, qemu-devel, jasowang, laurent, fthain 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(-) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] dp8393x: don't force 32-bit register access 2021-07-05 21:49 ` [PATCH 1/4] dp8393x: don't force 32-bit register access Mark Cave-Ayland 2021-07-06 17:18 ` Philippe Mathieu-Daudé @ 2021-07-06 23:51 ` Finn Thain 2021-07-07 10:02 ` Mark Cave-Ayland 1 sibling, 1 reply; 15+ messages in thread From: Finn Thain @ 2021-07-06 23:51 UTC (permalink / raw) To: Mark Cave-Ayland, Philippe Mathieu-Daudé Cc: jasowang, qemu-devel, laurent On Mon, 5 Jul 2021, 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. > IIUC, this patch replaces an explicit word swap with an implicit byte swap. The explicit word swap was conditional on the big_endian flag. This flag seems to work like the chip's BMODE pin which switches between Intel and Motorola bus modes (not just byte ordering but bus signalling in general). The BMODE pin or big_endian flag should effect a byte swap not a word swap so there must be a bug though it's not clear how that manifests. Regardless of this patch, the big_endian flag also controls byte swapping during DMA by the device. IIUC, the flag is set to indicate that RAM is big_endian, so it's not actually a property of the dp8393x but of the RAM... The Magnum hardware can run in big endian or little endian mode. But the SONIC chip must remain in little endian mode always because asserting BMODE would invoke Motorola signalling and that would contradict Philippe's datasheet which says that the SONIC device is attached to an "i386 compatible bus". This seems contrary to mips_jazz_init(), which sets the dp8393x big_endian flag whenever TARGET_WORDS_BIGENDIAN is defined, i.e. risc/os guest. QEMU's dp8393x device has native endianness, so perhaps a big endian guest or a big endian host could trigger the bug that's being addressed in this patch. Anyway, I think that this patch is heading in the right direction but can't it go further? Shouldn't the big_endian flag disappear altogether so that the memory API can also take care of the byte swapping needed by dp8393x_get() and dp8393x_put() for DMA? > 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, > }; > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] dp8393x: don't force 32-bit register access 2021-07-06 23:51 ` Finn Thain @ 2021-07-07 10:02 ` Mark Cave-Ayland 2021-07-08 0:52 ` Finn Thain 0 siblings, 1 reply; 15+ messages in thread From: Mark Cave-Ayland @ 2021-07-07 10:02 UTC (permalink / raw) To: Finn Thain, Philippe Mathieu-Daudé; +Cc: jasowang, qemu-devel, laurent On 07/07/2021 00:51, Finn Thain wrote: > On Mon, 5 Jul 2021, 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. >> > > IIUC, this patch replaces an explicit word swap with an implicit byte > swap. The explicit word swap was conditional on the big_endian flag. > > This flag seems to work like the chip's BMODE pin which switches between > Intel and Motorola bus modes (not just byte ordering but bus signalling in > general). The BMODE pin or big_endian flag should effect a byte swap not a > word swap so there must be a bug though it's not clear how that manifests. > > Regardless of this patch, the big_endian flag also controls byte swapping > during DMA by the device. IIUC, the flag is set to indicate that RAM is > big_endian, so it's not actually a property of the dp8393x but of the > RAM... > > The Magnum hardware can run in big endian or little endian mode. But the > SONIC chip must remain in little endian mode always because asserting > BMODE would invoke Motorola signalling and that would contradict > Philippe's datasheet which says that the SONIC device is attached to an > "i386 compatible bus". > > This seems contrary to mips_jazz_init(), which sets the dp8393x big_endian > flag whenever TARGET_WORDS_BIGENDIAN is defined, i.e. risc/os guest. > > QEMU's dp8393x device has native endianness, so perhaps a big endian guest > or a big endian host could trigger the bug that's being addressed in this > patch. > > Anyway, I think that this patch is heading in the right direction but > can't it go further? Shouldn't the big_endian flag disappear altogether so > that the memory API can also take care of the byte swapping needed by > dp8393x_get() and dp8393x_put() for DMA? Currently in QEMU the dp8393x device is connected directly to the system bus i.e. in effect the CPU. The CPU issues either a big-endian or little-endian access, the device declares what it would like to receive, and the memory API handles all the intermediate layers. In this case DEVICE_NATIVE_ENDIAN specifies it expects to receive accesses in the same endian as the machine and so everything "just works". For this reason MMIO accesses generally shouldn't need any explicit byte swapping: if you're doing this then it's a good indicator that something is wrong. There are some special cases around for things like virtio, but we can ignore those for now. The dp8393x_get() and dp8393x_put() functions are for bus master accesses from the device to the RAM, and these accesses are controlled by the "big_endian" property and the DW bit. As you point out the "big_endian" property is effectively the BMODE bit: in my datasheet I see nothing about this pin changing the signalling, only that it affects the byte ordering for RAM accesses. Now it is highly likely that the BMODE bit should match the target endian, in which case we could eliminate the "big_endian" property as you suggest. However this conflicts with what you mention above that the SONIC is hard-coded into little-endian mode, in which case we would still need to keep it. I've sent a fix for the CAP registers, but other than that I'm happy that the first patchset works fine here, and the consolidation of most of the endian swaps is a good tidy-up. If this works for you then I suggest we merge the first patchset including the CAP fix in time for freeze next week and go from there. Certainly we can look to improve things in the future, but without anyone having a working big-endian MIPS image to test against, I don't think it's worth guessing what changes are required as we can easily double the length of this thread and still have no idea if any changes we've made are correct. ATB, Mark. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] dp8393x: don't force 32-bit register access 2021-07-07 10:02 ` Mark Cave-Ayland @ 2021-07-08 0:52 ` Finn Thain 2021-07-08 8:50 ` Mark Cave-Ayland 0 siblings, 1 reply; 15+ messages in thread From: Finn Thain @ 2021-07-08 0:52 UTC (permalink / raw) To: Mark Cave-Ayland, Philippe Mathieu-Daudé Cc: jasowang, qemu-devel, laurent On Wed, 7 Jul 2021, Mark Cave-Ayland wrote: > However this conflicts with what you mention above that the SONIC is > hard-coded into little-endian mode, in which case we would still need to > keep it. > If you want to fully implement BMODE in QEMU then you'll need to abandon native endiannes for the device implementation. I was not proposing this as it implies more byte swapping. In a real Magnum the SONIC chip is connected to a bus that's not modelled by QEMU. It follows that BMODE serves different purposes than big_endian. I pointed out several semantic differences between BMODE and big_endian, but I think the most significant of those was that endianness is already a property of the memory device being accessed for DMA. Yet big_endian is a property of the dp8393x device. > Certainly we can look to improve things in the future, but without > anyone having a working big-endian MIPS image to test against, I don't > think it's worth guessing what changes are required as we can easily > double the length of this thread and still have no idea if any changes > we've made are correct. > That argument can be applied to other patches in this series also. Anyway, if we agree that the aim is ultimately to remove the big_endian flag then patch 4/4 should probably be re-evaluated in light of that. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] dp8393x: don't force 32-bit register access 2021-07-08 0:52 ` Finn Thain @ 2021-07-08 8:50 ` Mark Cave-Ayland 0 siblings, 0 replies; 15+ messages in thread From: Mark Cave-Ayland @ 2021-07-08 8:50 UTC (permalink / raw) To: Finn Thain, Philippe Mathieu-Daudé; +Cc: jasowang, qemu-devel, laurent On 08/07/2021 01:52, Finn Thain wrote: > On Wed, 7 Jul 2021, Mark Cave-Ayland wrote: > >> However this conflicts with what you mention above that the SONIC is >> hard-coded into little-endian mode, in which case we would still need to >> keep it. >> > > If you want to fully implement BMODE in QEMU then you'll need to abandon > native endiannes for the device implementation. I was not proposing this > as it implies more byte swapping. > > In a real Magnum the SONIC chip is connected to a bus that's not modelled > by QEMU. It follows that BMODE serves different purposes than big_endian. > > I pointed out several semantic differences between BMODE and big_endian, > but I think the most significant of those was that endianness is already a > property of the memory device being accessed for DMA. Yet big_endian is a > property of the dp8393x device. > >> Certainly we can look to improve things in the future, but without >> anyone having a working big-endian MIPS image to test against, I don't >> think it's worth guessing what changes are required as we can easily >> double the length of this thread and still have no idea if any changes >> we've made are correct. >> > > That argument can be applied to other patches in this series also. > > Anyway, if we agree that the aim is ultimately to remove the big_endian > flag then patch 4/4 should probably be re-evaluated in light of that. Other than fixing up the MMIO accesses to use the memory API, the patches in this series are just tidy-ups and refactorings i.e. no change in functionality related to the big_endian property. This is exactly the case for patch 4. If there were any changes to the big_endian logic required, then those would be handled by a separate patch (or patches) outside of this refactoring work. As I said before I'm not opposed to this, but we're coming up to freeze in less than a week and no-one has been able to provide working MIPS big endian and little endian test images in a thread lasting 3 weeks. Therefore my recommendation would be to merge the series in its current form for 6.1 and then when someone has found time to generate working images and do the required analysis around the big_endian logic, we can consider further changes later. ATB, Mark. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/4] dp8393x: Replace address_space_rw(is_write=1) by address_space_write() 2021-07-05 21:49 [PATCH 0/4] dp8393x: fixes and improvements Mark Cave-Ayland 2021-07-05 21:49 ` [PATCH 1/4] dp8393x: don't force 32-bit register access Mark Cave-Ayland @ 2021-07-05 21:49 ` Mark Cave-Ayland 2021-07-05 21:49 ` [PATCH 3/4] dp8393x: Store CAM registers as 16-bit Mark Cave-Ayland 2021-07-05 21:49 ` [PATCH 4/4] dp8393x: Rewrite dp8393x_get() / dp8393x_put() Mark Cave-Ayland 3 siblings, 0 replies; 15+ messages in thread From: Mark Cave-Ayland @ 2021-07-05 21:49 UTC (permalink / raw) To: qemu-devel, jasowang, laurent, fthain, f4bug From: Philippe Mathieu-Daudé <f4bug@amsat.org> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/net/dp8393x.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 44a1955015..cc7c001edb 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -820,8 +820,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, size = sizeof(uint16_t) * width; address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width; dp8393x_put(s, width, 0, 0); - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED, - (uint8_t *)s->data, size, 1); + address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED, + (uint8_t *)s->data, size); /* Move to next descriptor */ s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; @@ -850,8 +850,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, /* Pad short packets to keep pointers aligned */ if (rx_len < padded_len) { size = padded_len - rx_len; - address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED, - (uint8_t *)"\xFF\xFF\xFF", size, 1); + address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED, + (uint8_t *)"\xFF\xFF\xFF", size); address += size; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/4] dp8393x: Store CAM registers as 16-bit 2021-07-05 21:49 [PATCH 0/4] dp8393x: fixes and improvements Mark Cave-Ayland 2021-07-05 21:49 ` [PATCH 1/4] dp8393x: don't force 32-bit register access Mark Cave-Ayland 2021-07-05 21:49 ` [PATCH 2/4] dp8393x: Replace address_space_rw(is_write=1) by address_space_write() Mark Cave-Ayland @ 2021-07-05 21:49 ` Mark Cave-Ayland 2021-07-06 23:48 ` Finn Thain 2021-07-05 21:49 ` [PATCH 4/4] dp8393x: Rewrite dp8393x_get() / dp8393x_put() Mark Cave-Ayland 3 siblings, 1 reply; 15+ messages in thread From: Mark Cave-Ayland @ 2021-07-05 21:49 UTC (permalink / raw) To: qemu-devel, jasowang, laurent, fthain, f4bug From: Philippe Mathieu-Daudé <f4bug@amsat.org> Per the DP83932C datasheet from July 1995: 4.0 SONIC Registers 4.1 THE CAM UNIT The Content Addressable Memory (CAM) consists of sixteen 48-bit entries for complete address filtering of network packets. Each entry corresponds to a 48-bit destination address that is user programmable and can contain any combination of Multicast or Physical addresses. Each entry is partitioned into three 16-bit CAM cells accessible through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with CAP0 corresponding to the least significant 16 bits of the Destination Address and CAP2 corresponding to the most significant bits. Store the CAM registers as 16-bit as it simplifies the code. There is no change in the migration stream. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/net/dp8393x.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index cc7c001edb..22ceea338c 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -157,7 +157,7 @@ struct dp8393xState { MemoryRegion mmio; /* Registers */ - uint8_t cam[16][6]; + uint16_t cam[16][3]; uint16_t regs[0x40]; /* Temporaries */ @@ -280,15 +280,13 @@ static void dp8393x_do_load_cam(dp8393xState *s) address_space_read(&s->as, dp8393x_cdp(s), MEMTXATTRS_UNSPECIFIED, s->data, size); index = dp8393x_get(s, width, 0) & 0xf; - s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff; - s->cam[index][1] = dp8393x_get(s, width, 1) >> 8; - s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff; - s->cam[index][3] = dp8393x_get(s, width, 2) >> 8; - s->cam[index][4] = dp8393x_get(s, width, 3) & 0xff; - s->cam[index][5] = dp8393x_get(s, width, 3) >> 8; - trace_dp8393x_load_cam(index, s->cam[index][0], s->cam[index][1], - s->cam[index][2], s->cam[index][3], - s->cam[index][4], s->cam[index][5]); + s->cam[index][0] = dp8393x_get(s, width, 1); + s->cam[index][1] = dp8393x_get(s, width, 2); + s->cam[index][2] = dp8393x_get(s, width, 3); + trace_dp8393x_load_cam(index, + s->cam[index][0] >> 8, s->cam[index][0] & 0xff, + s->cam[index][1] >> 8, s->cam[index][1] & 0xff, + s->cam[index][2] >> 8, s->cam[index][2] & 0xff); /* Move to next entry */ s->regs[SONIC_CDC]--; s->regs[SONIC_CDP] += size; @@ -591,8 +589,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size) case SONIC_CAP1: case SONIC_CAP0: if (s->regs[SONIC_CR] & SONIC_CR_RST) { - val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg) + 1] << 8; - val |= s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)]; + val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)]; } break; /* All other registers have no special contraints */ @@ -990,7 +987,7 @@ static const VMStateDescription vmstate_dp8393x = { .version_id = 0, .minimum_version_id = 0, .fields = (VMStateField []) { - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6), + VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2), VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40), VMSTATE_END_OF_LIST() } -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] dp8393x: Store CAM registers as 16-bit 2021-07-05 21:49 ` [PATCH 3/4] dp8393x: Store CAM registers as 16-bit Mark Cave-Ayland @ 2021-07-06 23:48 ` Finn Thain 2021-07-07 9:08 ` Mark Cave-Ayland 0 siblings, 1 reply; 15+ messages in thread From: Finn Thain @ 2021-07-06 23:48 UTC (permalink / raw) To: Mark Cave-Ayland, Philippe Mathieu-Daudé Cc: jasowang, qemu-devel, laurent [-- Attachment #1: Type: text/plain, Size: 4157 bytes --] On Mon, 5 Jul 2021, Mark Cave-Ayland wrote: > From: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Per the DP83932C datasheet from July 1995: > > 4.0 SONIC Registers > 4.1 THE CAM UNIT > > The Content Addressable Memory (CAM) consists of sixteen > 48-bit entries for complete address filtering of network > packets. Each entry corresponds to a 48-bit destination > address that is user programmable and can contain any > combination of Multicast or Physical addresses. Each entry > is partitioned into three 16-bit CAM cells accessible > through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with > CAP0 corresponding to the least significant 16 bits of > the Destination Address and CAP2 corresponding to the > most significant bits. > > Store the CAM registers as 16-bit as it simplifies the code. > There is no change in the migration stream. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/net/dp8393x.c | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c > index cc7c001edb..22ceea338c 100644 > --- a/hw/net/dp8393x.c > +++ b/hw/net/dp8393x.c > @@ -157,7 +157,7 @@ struct dp8393xState { > MemoryRegion mmio; > > /* Registers */ > - uint8_t cam[16][6]; > + uint16_t cam[16][3]; > uint16_t regs[0x40]; > > /* Temporaries */ > @@ -280,15 +280,13 @@ static void dp8393x_do_load_cam(dp8393xState *s) > address_space_read(&s->as, dp8393x_cdp(s), > MEMTXATTRS_UNSPECIFIED, s->data, size); > index = dp8393x_get(s, width, 0) & 0xf; > - s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff; > - s->cam[index][1] = dp8393x_get(s, width, 1) >> 8; > - s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff; > - s->cam[index][3] = dp8393x_get(s, width, 2) >> 8; > - s->cam[index][4] = dp8393x_get(s, width, 3) & 0xff; > - s->cam[index][5] = dp8393x_get(s, width, 3) >> 8; > - trace_dp8393x_load_cam(index, s->cam[index][0], s->cam[index][1], > - s->cam[index][2], s->cam[index][3], > - s->cam[index][4], s->cam[index][5]); > + s->cam[index][0] = dp8393x_get(s, width, 1); > + s->cam[index][1] = dp8393x_get(s, width, 2); > + s->cam[index][2] = dp8393x_get(s, width, 3); > + trace_dp8393x_load_cam(index, > + s->cam[index][0] >> 8, s->cam[index][0] & 0xff, > + s->cam[index][1] >> 8, s->cam[index][1] & 0xff, > + s->cam[index][2] >> 8, s->cam[index][2] & 0xff); > /* Move to next entry */ > s->regs[SONIC_CDC]--; > s->regs[SONIC_CDP] += size; > @@ -591,8 +589,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size) > case SONIC_CAP1: > case SONIC_CAP0: > if (s->regs[SONIC_CR] & SONIC_CR_RST) { > - val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg) + 1] << 8; > - val |= s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)]; > + val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)]; > } > break; > /* All other registers have no special contraints */ This patch incorrectly alters the behaviour of the jazzsonic.c driver which reads the MAC address from the CAP registers in sonic_probe1(). With mainline QEMU, the driver reports: SONIC ethernet @e0001000, MAC 00:00:00:44:33:22, IRQ 28 With this patch: SONIC ethernet @e0001000, MAC 00:00:33:22:00:00, IRQ 28 > @@ -990,7 +987,7 @@ static const VMStateDescription vmstate_dp8393x = { > .version_id = 0, > .minimum_version_id = 0, > .fields = (VMStateField []) { > - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6), > + VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2), > VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40), > VMSTATE_END_OF_LIST() > } > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] dp8393x: Store CAM registers as 16-bit 2021-07-06 23:48 ` Finn Thain @ 2021-07-07 9:08 ` Mark Cave-Ayland 2021-07-07 21:57 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 15+ messages in thread From: Mark Cave-Ayland @ 2021-07-07 9:08 UTC (permalink / raw) To: Finn Thain, Philippe Mathieu-Daudé; +Cc: jasowang, qemu-devel, laurent On 07/07/2021 00:48, Finn Thain wrote: > On Mon, 5 Jul 2021, Mark Cave-Ayland wrote: > >> From: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >> Per the DP83932C datasheet from July 1995: >> >> 4.0 SONIC Registers >> 4.1 THE CAM UNIT >> >> The Content Addressable Memory (CAM) consists of sixteen >> 48-bit entries for complete address filtering of network >> packets. Each entry corresponds to a 48-bit destination >> address that is user programmable and can contain any >> combination of Multicast or Physical addresses. Each entry >> is partitioned into three 16-bit CAM cells accessible >> through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with >> CAP0 corresponding to the least significant 16 bits of >> the Destination Address and CAP2 corresponding to the >> most significant bits. >> >> Store the CAM registers as 16-bit as it simplifies the code. >> There is no change in the migration stream. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/net/dp8393x.c | 23 ++++++++++------------- >> 1 file changed, 10 insertions(+), 13 deletions(-) >> >> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c >> index cc7c001edb..22ceea338c 100644 >> --- a/hw/net/dp8393x.c >> +++ b/hw/net/dp8393x.c >> @@ -157,7 +157,7 @@ struct dp8393xState { >> MemoryRegion mmio; >> >> /* Registers */ >> - uint8_t cam[16][6]; >> + uint16_t cam[16][3]; >> uint16_t regs[0x40]; >> >> /* Temporaries */ >> @@ -280,15 +280,13 @@ static void dp8393x_do_load_cam(dp8393xState *s) >> address_space_read(&s->as, dp8393x_cdp(s), >> MEMTXATTRS_UNSPECIFIED, s->data, size); >> index = dp8393x_get(s, width, 0) & 0xf; >> - s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff; >> - s->cam[index][1] = dp8393x_get(s, width, 1) >> 8; >> - s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff; >> - s->cam[index][3] = dp8393x_get(s, width, 2) >> 8; >> - s->cam[index][4] = dp8393x_get(s, width, 3) & 0xff; >> - s->cam[index][5] = dp8393x_get(s, width, 3) >> 8; >> - trace_dp8393x_load_cam(index, s->cam[index][0], s->cam[index][1], >> - s->cam[index][2], s->cam[index][3], >> - s->cam[index][4], s->cam[index][5]); >> + s->cam[index][0] = dp8393x_get(s, width, 1); >> + s->cam[index][1] = dp8393x_get(s, width, 2); >> + s->cam[index][2] = dp8393x_get(s, width, 3); >> + trace_dp8393x_load_cam(index, >> + s->cam[index][0] >> 8, s->cam[index][0] & 0xff, >> + s->cam[index][1] >> 8, s->cam[index][1] & 0xff, >> + s->cam[index][2] >> 8, s->cam[index][2] & 0xff); >> /* Move to next entry */ >> s->regs[SONIC_CDC]--; >> s->regs[SONIC_CDP] += size; >> @@ -591,8 +589,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size) >> case SONIC_CAP1: >> case SONIC_CAP0: >> if (s->regs[SONIC_CR] & SONIC_CR_RST) { >> - val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg) + 1] << 8; >> - val |= s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)]; >> + val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)]; >> } >> break; >> /* All other registers have no special contraints */ > > This patch incorrectly alters the behaviour of the jazzsonic.c driver > which reads the MAC address from the CAP registers in sonic_probe1(). > > With mainline QEMU, the driver reports: > SONIC ethernet @e0001000, MAC 00:00:00:44:33:22, IRQ 28 > > With this patch: > SONIC ethernet @e0001000, MAC 00:00:33:22:00:00, IRQ 28 > >> @@ -990,7 +987,7 @@ static const VMStateDescription vmstate_dp8393x = { >> .version_id = 0, >> .minimum_version_id = 0, >> .fields = (VMStateField []) { >> - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6), >> + VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2), >> VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40), >> VMSTATE_END_OF_LIST() >> } I was staring at the code in dp8393x_do_load_cam() trying to figure out how on earth this could be reading the wrong values from the CAM descriptors, when I realised the problem is actually in the read back from the CAP registers (it doesn't need the x2 multiplier since the conversion from uint8_t to uint16_t). This should be fixed by the following: diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 22ceea338c..09c34c3584 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -589,7 +589,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size) case SONIC_CAP1: case SONIC_CAP0: if (s->regs[SONIC_CR] & SONIC_CR_RST) { - val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)]; + val = s->cam[s->regs[SONIC_CEP] & 0xf][SONIC_CAP0 - reg]; } break; /* All other registers have no special contraints */ ATB, Mark. ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] dp8393x: Store CAM registers as 16-bit 2021-07-07 9:08 ` Mark Cave-Ayland @ 2021-07-07 21:57 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 15+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-07 21:57 UTC (permalink / raw) To: Mark Cave-Ayland, Finn Thain; +Cc: jasowang, qemu-devel, laurent On 7/7/21 11:08 AM, Mark Cave-Ayland wrote: > On 07/07/2021 00:48, Finn Thain wrote: > >> On Mon, 5 Jul 2021, Mark Cave-Ayland wrote: >> >>> From: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> >>> Per the DP83932C datasheet from July 1995: >>> >>> 4.0 SONIC Registers >>> 4.1 THE CAM UNIT >>> >>> The Content Addressable Memory (CAM) consists of sixteen >>> 48-bit entries for complete address filtering of network >>> packets. Each entry corresponds to a 48-bit destination >>> address that is user programmable and can contain any >>> combination of Multicast or Physical addresses. Each entry >>> is partitioned into three 16-bit CAM cells accessible >>> through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with >>> CAP0 corresponding to the least significant 16 bits of >>> the Destination Address and CAP2 corresponding to the >>> most significant bits. >>> >>> Store the CAM registers as 16-bit as it simplifies the code. >>> There is no change in the migration stream. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> hw/net/dp8393x.c | 23 ++++++++++------------- >>> 1 file changed, 10 insertions(+), 13 deletions(-) >>> >>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c >>> index cc7c001edb..22ceea338c 100644 >>> --- a/hw/net/dp8393x.c >>> +++ b/hw/net/dp8393x.c >>> @@ -157,7 +157,7 @@ struct dp8393xState { >>> MemoryRegion mmio; >>> /* Registers */ >>> - uint8_t cam[16][6]; >>> + uint16_t cam[16][3]; >>> uint16_t regs[0x40]; >>> /* Temporaries */ >>> @@ -280,15 +280,13 @@ static void dp8393x_do_load_cam(dp8393xState *s) >>> address_space_read(&s->as, dp8393x_cdp(s), >>> MEMTXATTRS_UNSPECIFIED, s->data, size); >>> index = dp8393x_get(s, width, 0) & 0xf; >>> - s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff; >>> - s->cam[index][1] = dp8393x_get(s, width, 1) >> 8; >>> - s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff; >>> - s->cam[index][3] = dp8393x_get(s, width, 2) >> 8; >>> - s->cam[index][4] = dp8393x_get(s, width, 3) & 0xff; >>> - s->cam[index][5] = dp8393x_get(s, width, 3) >> 8; >>> - trace_dp8393x_load_cam(index, s->cam[index][0], >>> s->cam[index][1], >>> - s->cam[index][2], s->cam[index][3], >>> - s->cam[index][4], s->cam[index][5]); >>> + s->cam[index][0] = dp8393x_get(s, width, 1); >>> + s->cam[index][1] = dp8393x_get(s, width, 2); >>> + s->cam[index][2] = dp8393x_get(s, width, 3); >>> + trace_dp8393x_load_cam(index, >>> + s->cam[index][0] >> 8, >>> s->cam[index][0] & 0xff, >>> + s->cam[index][1] >> 8, >>> s->cam[index][1] & 0xff, >>> + s->cam[index][2] >> 8, >>> s->cam[index][2] & 0xff); >>> /* Move to next entry */ >>> s->regs[SONIC_CDC]--; >>> s->regs[SONIC_CDP] += size; >>> @@ -591,8 +589,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr >>> addr, unsigned int size) >>> case SONIC_CAP1: >>> case SONIC_CAP0: >>> if (s->regs[SONIC_CR] & SONIC_CR_RST) { >>> - val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - >>> reg) + 1] << 8; >>> - val |= s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 >>> - reg)]; >>> + val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - >>> reg)]; >>> } >>> break; >>> /* All other registers have no special contraints */ >> >> This patch incorrectly alters the behaviour of the jazzsonic.c driver >> which reads the MAC address from the CAP registers in sonic_probe1(). >> >> With mainline QEMU, the driver reports: >> SONIC ethernet @e0001000, MAC 00:00:00:44:33:22, IRQ 28 >> >> With this patch: >> SONIC ethernet @e0001000, MAC 00:00:33:22:00:00, IRQ 28 >> >>> @@ -990,7 +987,7 @@ static const VMStateDescription vmstate_dp8393x = { >>> .version_id = 0, >>> .minimum_version_id = 0, >>> .fields = (VMStateField []) { >>> - VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6), >>> + VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2), >>> VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40), >>> VMSTATE_END_OF_LIST() >>> } > > I was staring at the code in dp8393x_do_load_cam() trying to figure out > how on earth this could be reading the wrong values from the CAM > descriptors, when I realised the problem is actually in the read back > from the CAP registers (it doesn't need the x2 multiplier since the > conversion from uint8_t to uint16_t). > > This should be fixed by the following: > > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c > index 22ceea338c..09c34c3584 100644 > --- a/hw/net/dp8393x.c > +++ b/hw/net/dp8393x.c > @@ -589,7 +589,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr > addr, unsigned int size) > case SONIC_CAP1: > case SONIC_CAP0: > if (s->regs[SONIC_CR] & SONIC_CR_RST) { > - val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - > reg)]; > + val = s->cam[s->regs[SONIC_CEP] & 0xf][SONIC_CAP0 - reg]; Oops, thanks! ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/4] dp8393x: Rewrite dp8393x_get() / dp8393x_put() 2021-07-05 21:49 [PATCH 0/4] dp8393x: fixes and improvements Mark Cave-Ayland ` (2 preceding siblings ...) 2021-07-05 21:49 ` [PATCH 3/4] dp8393x: Store CAM registers as 16-bit Mark Cave-Ayland @ 2021-07-05 21:49 ` Mark Cave-Ayland 3 siblings, 0 replies; 15+ messages in thread From: Mark Cave-Ayland @ 2021-07-05 21:49 UTC (permalink / raw) To: qemu-devel, jasowang, laurent, fthain, f4bug From: Philippe Mathieu-Daudé <f4bug@amsat.org> Instead of accessing N registers via a single address_space API call using a temporary buffer (stored in the device state) and updating each register, move the address_space call in the register put/get. The load/store and word size checks are moved to put/get too. This simplifies a bit, making the code easier to read. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/net/dp8393x.c | 160 +++++++++++++++++++---------------------------- 1 file changed, 63 insertions(+), 97 deletions(-) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 22ceea338c..a03f8f0837 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -162,7 +162,6 @@ struct dp8393xState { /* Temporaries */ uint8_t tx_buffer[0x10000]; - uint16_t data[12]; int loopback_packet; /* Memory access */ @@ -219,34 +218,48 @@ static uint32_t dp8393x_wt(dp8393xState *s) return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0]; } -static uint16_t dp8393x_get(dp8393xState *s, int width, int offset) +static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, int offset) { + const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; uint16_t val; - if (s->big_endian) { - val = be16_to_cpu(s->data[offset * width + width - 1]); + if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { + addr += offset << 2; + if (s->big_endian) { + val = address_space_ldl_be(&s->as, addr, attrs, NULL); + } else { + val = address_space_ldl_le(&s->as, addr, attrs, NULL); + } } else { - val = le16_to_cpu(s->data[offset * width]); + addr += offset << 1; + if (s->big_endian) { + val = address_space_lduw_be(&s->as, addr, attrs, NULL); + } else { + val = address_space_lduw_le(&s->as, addr, attrs, NULL); + } } + return val; } -static void dp8393x_put(dp8393xState *s, int width, int offset, - uint16_t val) +static void dp8393x_put(dp8393xState *s, + hwaddr addr, int offset, uint16_t val) { - if (s->big_endian) { - if (width == 2) { - s->data[offset * 2] = 0; - s->data[offset * 2 + 1] = cpu_to_be16(val); + const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; + + if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { + addr += offset << 2; + if (s->big_endian) { + address_space_stl_be(&s->as, addr, val, attrs, NULL); } else { - s->data[offset] = cpu_to_be16(val); + address_space_stl_le(&s->as, addr, val, attrs, NULL); } } else { - if (width == 2) { - s->data[offset * 2] = cpu_to_le16(val); - s->data[offset * 2 + 1] = 0; + addr += offset << 1; + if (s->big_endian) { + address_space_stw_be(&s->as, addr, val, attrs, NULL); } else { - s->data[offset] = cpu_to_le16(val); + address_space_stw_le(&s->as, addr, val, attrs, NULL); } } } @@ -277,12 +290,10 @@ static void dp8393x_do_load_cam(dp8393xState *s) while (s->regs[SONIC_CDC] & 0x1f) { /* Fill current entry */ - address_space_read(&s->as, dp8393x_cdp(s), - MEMTXATTRS_UNSPECIFIED, s->data, size); - index = dp8393x_get(s, width, 0) & 0xf; - s->cam[index][0] = dp8393x_get(s, width, 1); - s->cam[index][1] = dp8393x_get(s, width, 2); - s->cam[index][2] = dp8393x_get(s, width, 3); + index = dp8393x_get(s, dp8393x_cdp(s), 0) & 0xf; + s->cam[index][0] = dp8393x_get(s, dp8393x_cdp(s), 1); + s->cam[index][1] = dp8393x_get(s, dp8393x_cdp(s), 2); + s->cam[index][2] = dp8393x_get(s, dp8393x_cdp(s), 3); trace_dp8393x_load_cam(index, s->cam[index][0] >> 8, s->cam[index][0] & 0xff, s->cam[index][1] >> 8, s->cam[index][1] & 0xff, @@ -293,9 +304,7 @@ static void dp8393x_do_load_cam(dp8393xState *s) } /* Read CAM enable */ - address_space_read(&s->as, dp8393x_cdp(s), - MEMTXATTRS_UNSPECIFIED, s->data, size); - s->regs[SONIC_CE] = dp8393x_get(s, width, 0); + s->regs[SONIC_CE] = dp8393x_get(s, dp8393x_cdp(s), 0); trace_dp8393x_load_cam_done(s->regs[SONIC_CE]); /* Done */ @@ -311,14 +320,12 @@ static void dp8393x_do_read_rra(dp8393xState *s) /* Read memory */ width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1; size = sizeof(uint16_t) * 4 * width; - address_space_read(&s->as, dp8393x_rrp(s), - MEMTXATTRS_UNSPECIFIED, s->data, size); /* Update SONIC registers */ - s->regs[SONIC_CRBA0] = dp8393x_get(s, width, 0); - s->regs[SONIC_CRBA1] = dp8393x_get(s, width, 1); - s->regs[SONIC_RBWC0] = dp8393x_get(s, width, 2); - s->regs[SONIC_RBWC1] = dp8393x_get(s, width, 3); + s->regs[SONIC_CRBA0] = dp8393x_get(s, dp8393x_rrp(s), 0); + s->regs[SONIC_CRBA1] = dp8393x_get(s, dp8393x_rrp(s), 1); + s->regs[SONIC_RBWC0] = dp8393x_get(s, dp8393x_rrp(s), 2); + s->regs[SONIC_RBWC1] = dp8393x_get(s, dp8393x_rrp(s), 3); trace_dp8393x_read_rra_regs(s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1], s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]); @@ -414,28 +421,22 @@ static void dp8393x_do_receiver_disable(dp8393xState *s) static void dp8393x_do_transmit_packets(dp8393xState *s) { NetClientState *nc = qemu_get_queue(s->nic); - int width, size; int tx_len, len; uint16_t i; - width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1; - while (1) { /* Read memory */ - size = sizeof(uint16_t) * 6 * width; s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA]; trace_dp8393x_transmit_packet(dp8393x_ttda(s)); - address_space_read(&s->as, dp8393x_ttda(s) + sizeof(uint16_t) * width, - MEMTXATTRS_UNSPECIFIED, s->data, size); tx_len = 0; /* Update registers */ - s->regs[SONIC_TCR] = dp8393x_get(s, width, 0) & 0xf000; - s->regs[SONIC_TPS] = dp8393x_get(s, width, 1); - s->regs[SONIC_TFC] = dp8393x_get(s, width, 2); - s->regs[SONIC_TSA0] = dp8393x_get(s, width, 3); - s->regs[SONIC_TSA1] = dp8393x_get(s, width, 4); - s->regs[SONIC_TFS] = dp8393x_get(s, width, 5); + s->regs[SONIC_TCR] = dp8393x_get(s, dp8393x_ttda(s), 1) & 0xf000; + s->regs[SONIC_TPS] = dp8393x_get(s, dp8393x_ttda(s), 2); + s->regs[SONIC_TFC] = dp8393x_get(s, dp8393x_ttda(s), 3); + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), 4); + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), 5); + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), 6); /* Handle programmable interrupt */ if (s->regs[SONIC_TCR] & SONIC_TCR_PINT) { @@ -457,15 +458,12 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) i++; if (i != s->regs[SONIC_TFC]) { /* Read next fragment details */ - size = sizeof(uint16_t) * 3 * width; - address_space_read(&s->as, - dp8393x_ttda(s) - + sizeof(uint16_t) * width * (4 + 3 * i), - MEMTXATTRS_UNSPECIFIED, s->data, - size); - s->regs[SONIC_TSA0] = dp8393x_get(s, width, 0); - s->regs[SONIC_TSA1] = dp8393x_get(s, width, 1); - s->regs[SONIC_TFS] = dp8393x_get(s, width, 2); + s->regs[SONIC_TSA0] = dp8393x_get(s, dp8393x_ttda(s), + 4 + 3 * i); + s->regs[SONIC_TSA1] = dp8393x_get(s, dp8393x_ttda(s), + 5 + 3 * i); + s->regs[SONIC_TFS] = dp8393x_get(s, dp8393x_ttda(s), + 6 + 3 * i); } } @@ -498,22 +496,12 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) s->regs[SONIC_TCR] |= SONIC_TCR_PTX; /* Write status */ - dp8393x_put(s, width, 0, - s->regs[SONIC_TCR] & 0x0fff); /* status */ - size = sizeof(uint16_t) * width; - address_space_write(&s->as, dp8393x_ttda(s), - MEMTXATTRS_UNSPECIFIED, s->data, size); + dp8393x_put(s, dp8393x_ttda(s), 0, s->regs[SONIC_TCR] & 0x0fff); if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) { /* Read footer of packet */ - size = sizeof(uint16_t) * width; - address_space_read(&s->as, - dp8393x_ttda(s) - + sizeof(uint16_t) * width - * (4 + 3 * s->regs[SONIC_TFC]), - MEMTXATTRS_UNSPECIFIED, s->data, - size); - s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0); + s->regs[SONIC_CTDA] = dp8393x_get(s, dp8393x_ttda(s), + 4 + 3 * s->regs[SONIC_TFC]); if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) { /* EOL detected */ break; @@ -765,7 +753,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, dp8393xState *s = qemu_get_nic_opaque(nc); int packet_type; uint32_t available, address; - int width, rx_len, padded_len; + int rx_len, padded_len; uint32_t checksum; int size; @@ -778,10 +766,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, rx_len = pkt_size + sizeof(checksum); if (s->regs[SONIC_DCR] & SONIC_DCR_DW) { - width = 2; padded_len = ((rx_len - 1) | 3) + 1; } else { - width = 1; padded_len = ((rx_len - 1) | 1) + 1; } @@ -802,11 +788,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, /* Check for EOL */ if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) { /* Are we still in resource exhaustion? */ - size = sizeof(uint16_t) * 1 * width; - address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width; - address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED, - s->data, size); - s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0); + s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5); if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) { /* Still EOL ; stop reception */ return -1; @@ -814,11 +796,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, /* Link has been updated by host */ /* Clear in_use */ - size = sizeof(uint16_t) * width; - address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width; - dp8393x_put(s, width, 0, 0); - address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED, - (uint8_t *)s->data, size); + dp8393x_put(s, dp8393x_crda(s), 6, 0x0000); /* Move to next descriptor */ s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; @@ -872,32 +850,20 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, /* Write status to memory */ trace_dp8393x_receive_write_status(dp8393x_crda(s)); - dp8393x_put(s, width, 0, s->regs[SONIC_RCR]); /* status */ - dp8393x_put(s, width, 1, rx_len); /* byte count */ - dp8393x_put(s, width, 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */ - dp8393x_put(s, width, 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */ - dp8393x_put(s, width, 4, s->regs[SONIC_RSC]); /* seq_no */ - size = sizeof(uint16_t) * 5 * width; - address_space_write(&s->as, dp8393x_crda(s), - MEMTXATTRS_UNSPECIFIED, - s->data, size); + dp8393x_put(s, dp8393x_crda(s), 0, s->regs[SONIC_RCR]); /* status */ + dp8393x_put(s, dp8393x_crda(s), 1, rx_len); /* byte count */ + dp8393x_put(s, dp8393x_crda(s), 2, s->regs[SONIC_TRBA0]); /* pkt_ptr0 */ + dp8393x_put(s, dp8393x_crda(s), 3, s->regs[SONIC_TRBA1]); /* pkt_ptr1 */ + dp8393x_put(s, dp8393x_crda(s), 4, s->regs[SONIC_RSC]); /* seq_no */ /* Check link field */ - size = sizeof(uint16_t) * width; - address_space_read(&s->as, - dp8393x_crda(s) + sizeof(uint16_t) * 5 * width, - MEMTXATTRS_UNSPECIFIED, s->data, size); - s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0); + s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5); if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) { /* EOL detected */ s->regs[SONIC_ISR] |= SONIC_ISR_RDE; } else { /* Clear in_use */ - size = sizeof(uint16_t) * width; - address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width; - dp8393x_put(s, width, 0, 0); - address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED, - s->data, size); + dp8393x_put(s, dp8393x_crda(s), 6, 0x0000); /* Move to next descriptor */ s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; -- 2.20.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-07-08 8:51 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-05 21:49 [PATCH 0/4] dp8393x: fixes and improvements Mark Cave-Ayland 2021-07-05 21:49 ` [PATCH 1/4] dp8393x: don't force 32-bit register access Mark Cave-Ayland 2021-07-06 17:18 ` Philippe Mathieu-Daudé 2021-07-06 19:22 ` Mark Cave-Ayland 2021-07-06 21:00 ` Philippe Mathieu-Daudé 2021-07-06 23:51 ` Finn Thain 2021-07-07 10:02 ` Mark Cave-Ayland 2021-07-08 0:52 ` Finn Thain 2021-07-08 8:50 ` Mark Cave-Ayland 2021-07-05 21:49 ` [PATCH 2/4] dp8393x: Replace address_space_rw(is_write=1) by address_space_write() Mark Cave-Ayland 2021-07-05 21:49 ` [PATCH 3/4] dp8393x: Store CAM registers as 16-bit Mark Cave-Ayland 2021-07-06 23:48 ` Finn Thain 2021-07-07 9:08 ` Mark Cave-Ayland 2021-07-07 21:57 ` Philippe Mathieu-Daudé 2021-07-05 21:49 ` [PATCH 4/4] dp8393x: Rewrite dp8393x_get() / dp8393x_put() Mark Cave-Ayland
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.