Hi Mark, Finn.

This respin aims go group all the fixes sent/suggested on the list
the last weeks around the dp8393x device.

Mark, can you send your S-o-b for patches 4 & 6?

The last 2 patches are included for Mark, but I don't plan to merge
them without Finn's Ack, and apparently they require more work.

Tested with NetBSD guest on Magnum Jazz.

Based-on mips-next.

Mark Cave-Ayland (1):
  NOTFORMERGE dp8393x: don't force 32-bit register access

Philippe Mathieu-Daudé (7):
  dp8393x: Replace address_space_rw(is_write=1) by address_space_write()
  dp8393x: Replace 0x40 magic value by SONIC_REG16_COUNT definition
  dp8393x: Only shift the device registers mapping by 1 bit
  dp8393x: Store CAM registers as 16-bit
  dp8393x: Migrate registers as array of uint16
  dp8393x: Store CRC using device configured endianess
  NOTFORMERGE dp8393x: Rewrite dp8393x_get() / dp8393x_put()

 hw/m68k/q800.c   |   2 +-
 hw/mips/jazz.c   |   2 +-
 hw/net/dp8393x.c | 219 +++++++++++++++++++++--------------------------
 3 files changed, 99 insertions(+), 124 deletions(-)

-- 
2.31.1

On Sat, 10 Jul 2021, Philippe Mathieu-Daudé wrote:

> 
> The last 2 patches are included for Mark, but I don't plan to merge
> 
> them without Finn's Ack, and apparently they require more work.
> 


I tested the patch series both with and without the last 2 patches. Both 
builds worked fine with my NetBSD/arc, Linux/mipsel and Linux/m68k guests.

Tested-by: Finn Thain <fthain@linux-m68k.org>

I have no objection to patch 8/8 ("dp8393x: don't force 32-bit register 
access"). I asked Mark to explain why it was a bug fix (since it didn't 
change QEMU behaviour in my tests) but when I looked into it I found that 
he is quite right, the patch does fix a theoretical bug.

My only objection to patch 7/8 ("dp8393x: Rewrite dp8393x_get() / 
dp8393x_put()") was that it could be churn.

If I'm right that the big_endian flag should go away, commit b1600ff195 
("hw/mips/jazz: specify correct endian for dp8393x device") has already 
taken mainline in the wrong direction and amounts to churn.

I have the same reservations about patch 6/8 ("dp8393x: Store CRC using 
device configured endianess"). Perhaps that should be NOTFORMERGE too 
(even though it too a theoretical bug fix).

Is there a good way to avoid using big_endian for storing the CRC and the 
other DMA operations?

BTW, if you see "sn0: receive buffers exhausted" occasionally logged by 
the NetBSD 9.2 kernel, accompanied by packet loss, it's not a regression 
in QEMU. I first observed it last year when stress testing dp8393x with 
NetBSD 5.1. I believe this to be an old NetBSD sn driver bug because Linux 
is unaffected.

On 7/11/21 4:08 AM, Finn Thain wrote:
> On Sat, 10 Jul 2021, Philippe Mathieu-Daudé wrote:
> 
>>
>> The last 2 patches are included for Mark, but I don't plan to merge
>>
>> them without Finn's Ack, and apparently they require more work.
>>
> 
> 
> I tested the patch series both with and without the last 2 patches. Both 
> builds worked fine with my NetBSD/arc, Linux/mipsel and Linux/m68k guests.
> 
> Tested-by: Finn Thain <fthain@linux-m68k.org>

Thank you very much :)

> I have no objection to patch 8/8 ("dp8393x: don't force 32-bit register 
> access"). I asked Mark to explain why it was a bug fix (since it didn't 
> change QEMU behaviour in my tests) but when I looked into it I found that 
> he is quite right, the patch does fix a theoretical bug.

OK.

> My only objection to patch 7/8 ("dp8393x: Rewrite dp8393x_get() / 
> dp8393x_put()") was that it could be churn.
> 
> If I'm right that the big_endian flag should go away, commit b1600ff195 
> ("hw/mips/jazz: specify correct endian for dp8393x device") has already 
> taken mainline in the wrong direction and amounts to churn.

We might figure out with a BE guest image, the remove the endian flag.
I don't think the patch is worth removing, because it simplifies and
we'll only have to fix the endianess in 2 places, dp8393x_get/put. I
prefer to restrict the address space accesses there.

> I have the same reservations about patch 6/8 ("dp8393x: Store CRC using 
> device configured endianess"). Perhaps that should be NOTFORMERGE too 
> (even though it too a theoretical bug fix).

OK, dropped.

> Is there a good way to avoid using big_endian for storing the CRC and the 
> other DMA operations?

Could be, but I'd rather see this fixed generically in the MemoryRegion
API, not in this particular device model.

> BTW, if you see "sn0: receive buffers exhausted" occasionally logged by 
> the NetBSD 9.2 kernel, accompanied by packet loss, it's not a regression 
> in QEMU. I first observed it last year when stress testing dp8393x with 
> NetBSD 5.1. I believe this to be an old NetBSD sn driver bug because Linux 
> is unaffected.
> 

On Sun, 11 Jul 2021, Philippe Mathieu-Daudé wrote:

> 
> > If I'm right that the big_endian flag should go away, commit 
> > b1600ff195 ("hw/mips/jazz: specify correct endian for dp8393x device") 
> > has already taken mainline in the wrong direction and amounts to 
> > churn.
> 
> We might figure out with a BE guest image, the remove the endian flag.

Yes, it's hard to make progress without a BE guest. However, for testing 
dp8393x we probably don't need a disk image. I think we only need working 
firmware, since the RISC/os firmware appears to implement BOOTP and TFTP 
and appears to contain a SONIC driver.

This page discusses using the "MIPS Monitor" firmware on a Mips Magnum 
3000 machine to netboot NetBSD/mipsco: 
https://www.ludd.ltu.se/~ragge/htdocs/Ports/mipsco/install.html

Note that the firmware banner message looks like this:
Rx3230 MIPS Monitor: Version 5.43 OPT Mon May 13 17:31:12 PDT 1991 root 

It appears that there may be a similar firmware for MIPS Magnum at, 
https://gunkies.org/wiki/Installing_Windows_NT_4.0_on_Qemu(MIPS)

(I suppose this is the firmware floppy that was once found at ftp.sgi.com, 
after SGI acquired MIPS?)

The file RISCOS.RAW found in SETUP.ZIP appears to contain various string 
constants, both LE and BE, including:

$ swap64 < RISCOS.RAW |strings |egrep "Version|MIPS|Rx|Monitor"
...
Version 5.60 OPT-EB Wed Jun 17 11:23:28 PDT 1992 root
MIPS
Rx4230
%s %s Monitor: %s
...

This looks like the same banner, only for a different machine (as you'd 
expect). Unfortunately, nothing happened when I tried to boot that 
firmware:

$ ln -s RISCOS.RAW mips_bios.bin 
$ qemu-system-mips64 -M magnum -L . -global ds1225y.filename=mips-nvram -serial mon:stdio -serial null -nic bridge,model=dp83932,mac=00:00:00:aa:bb:cc

I don't know enough about this platform or about QEMU to go much further 
with this so I hope that others will be able to help.

I did find this link, which talks a little about the early boot code.
https://web.archive.org/web/20180823065803/http://www.sensi.org/~alec/mips/mips-history.html

On Mon, 12 Jul 2021, Finn Thain wrote:

> On Sun, 11 Jul 2021, Philippe Mathieu-Daudé wrote:
> 
> > 
> > > If I'm right that the big_endian flag should go away, commit 
> > > b1600ff195 ("hw/mips/jazz: specify correct endian for dp8393x 
> > > device") has already taken mainline in the wrong direction and 
> > > amounts to churn.
> > 
> > We might figure out with a BE guest image, the remove the endian flag.
> 
> Yes, it's hard to make progress without a BE guest. However, for testing 
> dp8393x we probably don't need a disk image. I think we only need 
> working firmware, since the RISC/os firmware appears to implement BOOTP 
> and TFTP and appears to contain a SONIC driver.

I think we probably can install RISC/os once the firmware can be made to 
work.

The file "RISCos_5.01.iso", found in the Bitsavers archive, contains 
several kernel binaries, one of which is "unix.r4030eb_std".

From the "r4030" in its name, and from the symbol names and string 
constants it contains, this binary appears to have all the drivers for the 
MIPS Magnum 4000.