Re: [PATCH V2 00/14] net: ks8851: Unify KS8851 SPI and MLL drivers

[Marek Vasut <marex@denx.de>]

On 3/26/20 8:02 PM, Lukas Wunner wrote:
> On Wed, Mar 25, 2020 at 04:05:29PM +0100, Marek Vasut wrote:
>> The KS8851SNL/SNLI and KS8851-16MLL/MLLI/MLLU are very much the same pieces
>> of silicon, except the former has an SPI interface, while the later has a
>> parallel bus interface. Thus far, Linux has two separate drivers for each
>> and they are diverging considerably.
>>
>> This series unifies them into a single driver with small SPI and parallel
>> bus specific parts. The approach here is to first separate out the SPI
>> specific parts into a separate file, then add parallel bus accessors in
>> another separate file and then finally remove the old parallel bus driver.
>> The reason for replacing the old parallel bus driver is because the SPI
>> bus driver is much higher quality.
> 
> With this series, ks8851.ko (SPI variant) failed to compile as a module.
> I got it working by renaming ks8851.c to ks8851_common.c and applying
> the following change to the Makefile:
> 
> --- a/drivers/net/ethernet/micrel/Makefile
> +++ b/drivers/net/ethernet/micrel/Makefile
> @@ -5,6 +5,8 @@
>  
>  obj-$(CONFIG_ARM_KS8695_ETHER) += ks8695net.o
>  obj-$(CONFIG_KS8842) += ks8842.o
> -obj-$(CONFIG_KS8851) += ks8851.o ks8851_spi.o
> -obj-$(CONFIG_KS8851_MLL) += ks8851.o ks8851_par.o
> +obj-$(CONFIG_KS8851) += ks8851.o
> +ks8851-objs = ks8851_common.o ks8851_spi.o
> +obj-$(CONFIG_KS8851_MLL) += ks8851_mll.o
> +ks8851_mll-objs = ks8851_common.o ks8851_par.o
>  obj-$(CONFIG_KSZ884X_PCI) += ksz884x.o
> 
> This series breaks reading the MAC address from an EEPROM attached to
> the KSZ8851SNLI:
> The MAC address stored in the EEPROM was c8:3e:a7:99:ef:aa.
> The MAC address was read as 3e:c8:99:a7:ef:aa with this series.
> Note: The MAC address starts at the third byte in the EEPROM and is
> stored as aa:ef:99:a7:3e:c8, i.e. in reverse order.  (I think the
> spec says something else but it appears to be wrong.)
> 
> Assigning a MAC address with "ifconfig eth1 hw ether <mac>" (which I
> believe ends up calling ks8851_write_mac_addr()) worked fine.

I added fixes for those.

> The performance degredation with this series is as follows:
> 
> Latency (ping) without this series:
>   rtt min/avg/max/mdev = 0.982/1.776/3.756/0.027 ms, ipg/ewma 2.001/1.761 ms
> With this series:
>   rtt min/avg/max/mdev = 1.084/1.811/3.546/0.040 ms, ipg/ewma 2.020/1.814 ms
> 
> Throughput (scp) without this series:
>   Transferred: sent 369780976, received 66088 bytes, in 202.0 seconds
>   Bytes per second: sent 1830943.5, received 327.2
> With this series:
>   Transferred: sent 369693896, received 67588 bytes, in 210.5 seconds
>   Bytes per second: sent 1755952.6, received 321.0

Maybe some iperf would be better here ?

> SPI clock is 25 MHz.  The chip would allow up to 40 MHz, but the board
> layout limits that.
> 
> I suspect the performance regression is not only caused by the
> suboptimal 16 byte instead of 8 byte accesses (and 2x16 byte instead
> of 32 byte accesses), but also because the accessor functions cannot
> be inlined.  It would be better if they were included from a header
> file as static inlines.  The performance regression would then likely
> disappear.

I did another measurement today and I found out that while RX on the old
KS8851-MLL driver runs at ~50 Mbit/s , TX runs at ~80 Mbit/s . With this
new driver, RX still runs at ~50 Mbit/s, but TX runs also at 50 Mbit/s .
That's real bad. Any ideas how to debug/profile this one ?

> I guess the good news is that it otherwise worked out of the box.

Great