Thanks for this.
Note that we already have a driver for dwc4 which is named eqos (imported from NetBSD).
When I started to split dwc it was to support dwc4/5 but eqos was imported before I started working on it.
Could you test how does eqos compare to your driver ?
TBH I would have prefer that dwc4 support was done this way since the beginning.

Oh, I did not know eqos(4) was imported from NetBSD!

I have spent a few days on eqos(4) but did not get it working. I have got a new RISC-V platform so I was also dealing with the base support and, in particular, generic MSI interrupts support in RISC-V, which makes it hard to debug new drivers. Eventually end up extending dwc(4) as I'm more familiar with that (we all have been using it for 10 years!)

What I found with respect to eqos(4) during my attempt

• eqos does not attach on anything except versions 0x51 & 0x52
• eqos does bus_dma_sync on uncached descriptor memory, not sure how safe is that
• bus_dma_sync operations are done in incorrect places
• bus_dma_sync operations touch all the descriptors at once, which is not correct. Invalidation flushes have to be done on individual descriptors touched by CPU or HW, so that HW could use other descriptors at the same time
• SpaceMit K3 is non-cache coherent SoC with 64 byte cache line, while the descriptors are 16 byte. One flush covers 4 descriptors. Either descriptors have to be extended to 64 byte (using internal padding mechanism) and invalidated on individual basis, or mapping should be uncached
• eqos busdma tags do not support memory addresses > 32bit, while Spacemit has all its DRAM in the upper 64 bit memory region
• eqos comes with some glue for rockchip platforms in the "if_eqos_fdt.c", which looks like a generic file name
• eqos driver committed without any review ? (no information in the original commit message)

In D59191#1357815, @br wrote:

I have spent a few days on eqos(4) but did not get it working.

I also think that rqos should be merged back into dwc.

In D59191#1357815, @br wrote:

What I found with respect to eqos(4) during my attempt

• eqos does bus_dma_sync on uncached descriptor memory, not sure how safe is that
• bus_dma_sync operations touch all the descriptors at once, which is not correct. Invalidation flushes have to be done on individual descriptors touched by CPU or HW, so that HW could use other descriptors at the same time
• SpaceMit K3 is non-cache coherent SoC with 64 byte cache line, while the descriptors are 16 byte. One flush covers 4 descriptors. Either descriptors have to be extended to 64 byte (using internal padding mechanism) and invalidated > on individual basis, or mapping should be uncached

That's not true at all:

it's not the driver's job to determine the kind of memory used for descriptors. Because both the DMA and the CPU may write to descriptors simultaneously, the driver must require BUS_DMA_COHERENT memory. It is the responsibility of the platform bus DMA implementation to provide the driver with memory that has the given characteristics. It doesn't matter if this is done using uncached, not write-combined, or anything else. This must be fully transparent to the driver.

As descriptors operate on coherent memory, bus_dma_sync() can access them at any time without issue. Using a shorter bus_dma_sync() per descriptor doesn't make sense because bus_dma_sync() on coherent memory is a pure read or write barrier, nothing more.

However, I have another major issue. Please don't use linuxism like 'ops'. In FreeBSD, we have subclasses, which are much more flexible and make code more readable and maintainable. Please use them - you make life much easier for followers. Thanks.

In D59191#1358222, @mmel wrote:

Because both the DMA and the CPU may write to descriptors simultaneously, the driver must require BUS_DMA_COHERENT memory.

In D59191#1358222, @mmel wrote:

In D59191#1357815, @br wrote:

I have spent a few days on eqos(4) but did not get it working.

I also think that rqos should be merged back into dwc.

In D59191#1357815, @br wrote:

What I found with respect to eqos(4) during my attempt

• eqos does bus_dma_sync on uncached descriptor memory, not sure how safe is that
• bus_dma_sync operations touch all the descriptors at once, which is not correct. Invalidation flushes have to be done on individual descriptors touched by CPU or HW, so that HW could use other descriptors at the same time
• SpaceMit K3 is non-cache coherent SoC with 64 byte cache line, while the descriptors are 16 byte. One flush covers 4 descriptors. Either descriptors have to be extended to 64 byte (using internal padding mechanism) and invalidated > on individual basis, or mapping should be uncached

That's not true at all:

it's not the driver's job to determine the kind of memory used for descriptors. Because both the DMA and the CPU may write to descriptors simultaneously, the driver must require BUS_DMA_COHERENT memory. It is the responsibility of the platform bus DMA implementation to provide the driver with memory that has the given characteristics. It doesn't matter if this is done using uncached, not write-combined, or anything else. This must be fully transparent to the driver.

Eqos driver does expect that the memory returned is non-cached, otherwise it would be incorrect to flush all the descriptors at once. Bus_dma could not figure out by itself which descriptor to flush if that is fully transparent to driver.
If we must use uncached descriptors for all DMAs, then it is limitation, because having descriptors to reside in cached memory could be the more efficient and faster way to deal with them on heavy usage. That would require individual flushes.

As descriptors operate on coherent memory, bus_dma_sync() can access them at any time without issue. Using a shorter bus_dma_sync() per descriptor doesn't make sense because bus_dma_sync() on coherent memory is a pure read or write barrier, nothing more.

For memory barries don't we prefer to use macroses like mb() instead of calling to busdma sync routines?
Current dwc(4) driver does that instead of busdma_sync. It makes it easier to read the code, and probably a bit faster for CPU.

However, I have another major issue. Please don't use linuxism like 'ops'. In FreeBSD, we have subclasses, which are much more flexible and make code more readable and maintainable. Please use them - you make life much easier for followers. Thanks.

Ok I can do!

In D59191#1358240, @br wrote:

Eqos driver does expect that the memory returned is non-cached, otherwise it would be incorrect to flush all the descriptors at once. Bus_dma could not figure out by itself which descriptor to flush if that is fully transparent to driver.

The EQOS driver expects coherent memory, not uncachable memory. But yes, on ARM, coherent memory can be realised as uncacheable if the device sits on a non-coherent bus, or as normal cacheable memory if the device sits on a DMA coherent bus (i.e. ACE). However, this decision lies with the busdma layer. The driver may request memory with a given characteristic, and the busdma layer is responsible for fulfilling that request.

If we must use uncached descriptors for all DMAs, then it is limitation, because having descriptors to reside in cached memory could be the more efficient and faster way to deal with them on heavy usage. That would require individual flushes.

There is no way to make cached memory coherent for a device that is not on a coherent bus. The golden rule is that the CPU can load a cache line or flush it at any time, irrespective of the executed code. This is the result of SMP, speculative reads and various streaming optimisations. Forget about this. Ask Adrian; MIPS gave us a big lesson in this area.

As descriptors operate on coherent memory, bus_dma_sync() can access them at any time without issue. Using a shorter bus_dma_sync() per descriptor doesn't make sense because bus_dma_sync() on coherent memory is a pure read or write barrier, nothing more.

For memory barries don't we prefer to use macroses like mb() instead of calling to busdma sync routines?
Current dwc(4) driver does that instead of busdma_sync. It makes it easier to read the code, and probably a bit faster for CPU.

bus_dma_sync() should be used here. However, we do not yet have a proper operation for this, so rmb()/wmb() can be tolerated.

However, I have another major issue. Please don't use linuxism like 'ops'. In FreeBSD, we have subclasses, which are much more flexible and make code more readable and maintainable. Please use them - you make life much easier for followers. Thanks.

Ok I can do!

Wait a minute! It wasn't until the second reading that I realised just how different those drivers are. Perhaps it would be better to have them separately, with some shared code. I'm not sure... Sorry.

In D59191#1358304, @mmel wrote:

Wait a minute! It wasn't until the second reading that I realised just how different those drivers are. Perhaps it would be better to have them separately, with some shared code. I'm not sure... Sorry.

Yes the drivers are different when running but the setup is really exactly the same and that's one big advantage of having them co-exists (granted the init the is same as in Linux this is all a big driver for all synopsys eth, even the allwinner one so they all share the same properties etc ...)

I still have a personal problem with ops. It looks too fine-grained to me, but this can be solved with the following commit if we find it necessary.

Anyway, please add "panic" to all unimplemented functions (currently marked with "TODO") and limit the advertised (and check requested) capabilities to the implemented functionality only.

I fully understand that this is first version of driver, but silently ignoring of not yet implemented functionality is not right way, mainly for followers.

The driver advertises the following: IFCAP_VLAN_MTU, IFCAP_HWCSUM and IFCAP_HWCSUM_IPV6. What happens if the user enables one of these? I only see that IFCAP_RXCSUM is implemented, albeit probably only partially. In all other cases, the driver either emits an invalid packet, is unable to receive a valid packet, or it panics.

In D59191#1377476, @mmel wrote:

The driver advertises the following: IFCAP_VLAN_MTU, IFCAP_HWCSUM and IFCAP_HWCSUM_IPV6. What happens if the user enables one of these? I only see that IFCAP_RXCSUM is implemented, albeit probably only partially. In all other cases, the driver either emits an invalid packet, is unable to receive a valid packet, or it panics.

Both RXCSUM and TXCSUM options on the dwc interface seem to work. I am not sure about IFCAP_VLAN_MTU, it does not seem it require any special handling ?

The IFCAP_VLAN_MTU indicates that the hardware can receive and transmit packets of up to 1504 bytes in length, so nothing more is necessary.
So, the only thing missing is the IPv6 CSUM (but it is advertised)