Re: [PATCH 2/2] net: Add driver for LiteX's LiteETH network interface

From: Joel Stanley
Date: Fri Aug 20 2021 - 03:46:45 EST


Hi Florent and Gabriel,

On Fri, 6 Aug 2021 at 12:27, Florent Kermarrec <florent@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Gabriel, Joel,
>
> for 1), the polling mode has been useful recently with Linux-On-LiteX-Vexriscv to bring up the SMP version before enabling the IRQs and can probably be useful in the future for similar purposes. So if it's not too complicated to maintain, this could be useful to keep it.

I would prefer to get a basic working driver for the straight forward
Linux-on-FPGA use case merged first. We can then work out what should
be done next. I would like to see hardware improvements to make it go
faster, and we can look at other features like polling if you still
need them.

>
> Le ven. 6 août 2021 à 13:48, Gabriel L. Somlo <gsomlo@xxxxxxxxx> a écrit :
>>
>> Hi Joel,
>>
>> Thanks for pushing this upstream (and for writing it to begin with)!
>>
>> Would you mind diff-ing your version of litex_liteeth.c against what
>> is currently in
>> https://github.com/litex-hub/linux/blob/litex-rebase/drivers/net/ethernet/litex/litex_liteeth.c ?
>>
>> Two main differences we should discuss:
>>
>> 1. there's a polling mode (added by Antony Pavlov), and if we
>> decide *not* to keep it around, I want to ensure we do that
>> deliberately, with an explanation as to why;

I assumed this was only for broken systems, or systems that were still
being brought up. Haven't the risc-v socs improved to the point where
they have working interrupts now?

>>
>> 2. LiteX CSRs are accessed using `litex_[read|write][8|16|32]()`
>> as opposed to simply `[read|write][b|w|l]()`. The former set
>> are defined in `include/linux/litex.h` and are needed to
>> ensure correct accesses regardless of endianness, since by
>> default LiteX registers' endianness mirrors that of the
>> configured CPU.

I don't like that they get the parameters in the wrong order compared
to the rest of the kernel, and that they force 32-bit access no matter
the width of the access being performed. They also mean we can't use
_releaxed variants of accessors to eg. read a set of registers without
having a barrier in between.

That said, I have used the csr accessors so we can support the mor1k,
which afaict is the only litex CPU that defaults to big endian.

If you could help review the patch so we can get it merged this cycle,
then we can work on future enhancements.

Cheers,

Joel