[lowrisc-dev] Re: lowRISC kernel changes rebased over stock 4.19

Dr Jonathan Kimmitt jrrk2 at cam.ac.uk
Wed Oct 31 15:12:19 GMT 2018


On 31/10/2018 14:31, Gabriel L. Somlo wrote:
> On Tue, Oct 30, 2018 at 09:04:36PM +0000, Dr Jonathan Kimmitt wrote:
>>> I've managed to successfully use https://github.com/riscv/riscv-gnu-toolchain
>>> directly to build every other component of lowRISC v0.6 (specifically
>>> busybox, the kernel, and bbl). That makes it possible to ignore any
>>> current differences between the toolchain fork included in
>>> rocket-chip/riscv-tools and "upstream" :)
>>>
>> The Rocket core repository has a dire warning against using anything other
>> than their tools, but I guess that dates back to the days when CSR numbers
>> were rather unstable.
> I haven't been on the risc-v scene long enough to understand the
> distinction between Rocket's repository and the "riscv" account on
> github. The latter instinctively feels closer to the notion of "upstream",
> although they too have forked submodules of all the gnu stuff. But maybe
> they (riscv) are the established path for changes trickle up into GNU as
> well, same way they appear to be doing for the Linux kernel?

The rocket-chip repo contains links to a certain version of 
riscv-gnu-toolchain, not necessarily

the latest, that is known to work. Obviously it is a pain to keep 
retesting everything. It also

has links to riscv-pk, riscv-isa-sim etc. If you use Debian-sid as the 
build environment, you do

not have to worry about tool versions because gcc-8 for RISCV is default.

>   
>>> Next, I looked at the kernel, and tried to figure out how I can ignore
>>> everything that is *not* lowRISC, and focus only on the specific bits that
>>> *are*! So I came up with the following set of six patches on top of the
>>> latest (4.19) upstream kernel:
>> The LowRISC-QuickStart repo has some patches similar to yours based on
>> the 4.18 kernel. The 4.19 was not out when those were prepared.
> Right, there's three of them:
>
> 	- riscv.patch, appearing to contain various "in-flight" changes
> 	  from the riscv Linux tree, which I assume are either already
> 	  in 4.19 or they don't matter (at least going by the fact that
> 	  after ignoring them completely my 4.19 kernel still builds and
> 	  runs fine on the nexys4ddr)

riscv.patch are differences between 4.18 and 4.18-riscv. All of those were

intended to make it into the 4.19 merge window. I haven't tested whether 
this is so.

>
> 	- lowrisc.patch, containing all peripheral drivers (keyboard,
> 	  video, ethernet, microSD), as well as the timer hacks I
> 	  avoided by modifying the DT, and this:
> =========================================================
> diff -urN linux-4.18-patched/kernel/dma/swiotlb.c riscv-linux-riscv-linux-4.18/kernel/dma/swiotlb.c
> --- linux-4.18-patched/kernel/dma/swiotlb.c     2018-08-12 21:41:04.000000000 +0100
> +++ riscv-linux-riscv-linux-4.18/kernel/dma/swiotlb.c 2018-09-17 15:23:49.410795216 +0100
> @@ -147,7 +147,7 @@
>   }
>   
>   /* default to 64MB */
> -#define IO_TLB_DEFAULT_SIZE (64UL<<20)
> +#define IO_TLB_DEFAULT_SIZE (1UL<<20)
>   unsigned long swiotlb_size_or_default(void)
>   {
>          unsigned long size;
> =========================================================
> 	  ... which again, I ignored without any apparent ill effects.
> 	  Any idea why it was in there, and how bad it is that I'm trying
> 	  to ignore it? :)

The Nexys4DDR only has 128M of ram, so if you boot without this patch 
you will get a nasty surprise

when you use a free command for the first time. What! half of memory 
grabbed for a software I/O TLB!

> 	- gpio.patch, which modifies the microSD source to also add the
> 	  GPIO driver (not sure why it's pasted into the microSD .c file,
> 	  but OK with me for now).

Due to laziness, the GPIO was attached to the SD driver Verilog, hence 
it does not

have an independent DTB entry (and neither does it have a separately 
mapped register window in supervisor space)

> My ideal is to minimize touching the kernel to the simplest, absolutely
> necessary changes only, and to do "one distinct thing per patch", the
> way I expect upstream to want to receive them for review. Not because
> I'm advocating for upstream submission "Right Now", but rather because
> it's easier to understand in the general sense.
>
>>>     https://github.com/gsomlo/riscv-linux/tree/gls-lowrisc-4.19-v00
>>>
>>> This is exactly six commits added to stock upstream:
>>>
>>>     1/6: keyboard driver (uart and ps2-over-usb)
>>>     2/6: vga console driver
>>>     3/6: xilinx/nexys4ddr ethernet driver
>>>     4/6: microSD card reader
>>>     5/6: GPIO driver (inserted on top of microSD driver .c file)
>>>     6/6: lowRISC-specific defconfig (separate from default RISC-V defconfig)
>>>
>>> I specifically avoided cleaning up indentation and running anything
>>> through checkpatch, to give you a chance to ensure these are roughly the
>>> same files as what's currently present in lowrisc's own v0.6 fork of the
>>> linux source tree (okay, I removed some trailing whitespaces, but
>>> managed to contain my OCD from making further changes *beyond* that :)
>>>
>> I’ve been using emacs to edit my source code since 1987. It tends to
>> be over enthusiastic about blanks.
> "git diff" paints them red on the output, guess it does so specifically
> to "trigger" obsessive neat-freaks like myself :)
>
>>> With the following patch against riscv-pk's lowrisc.dts:
>>>
>>> ==========================================================================
>>> diff --git a/machine/lowrisc.dts b/machine/lowrisc.dts
>>> index 6e3c4c2..13a79e5 100644
>>> --- a/machine/lowrisc.dts
>>> +++ b/machine/lowrisc.dts
>>> @@ -14,6 +14,7 @@
>>>     cpus {
>>>         #address-cells = <1>;
>>>         #size-cells = <0>;
>>> +        timebase-frequency = <500000>;
>>>         cpu at 0 {
>>>             clock-frequency = <0>;
>>>             compatible = "sifive,rocket0", "riscv";
>>> @@ -33,7 +34,6 @@
>>>             reg = <0>;
>>>             riscv,isa = "rv64imafdc";
>>>             status = "okay";
>>> -            timebase-frequency = <500000>;
>>>             tlb-split;
>>>             L3: interrupt-controller {
>>>                 #interrupt-cells = <1>;
>>> ==========================================================================
>>>
>> The capability to have different cpus at different clock rates arrived
>> rather late in the day.
> So, is the above patch a good/bad/indifferent thing in your opinion? I
> personally prefer to avoid patching the kernel if I had the power to
> expose RTL (or DT data in this case) that it would find "palatable" without
> modifications :)

Neither good nor bad, just a difference between 4.18-riscv and 4.19

> If/when lowRISC ends up with multiple CPUs at different clock rates, we
> can figure out how to cross that bridge when we get to it?
>
>>> ...it is possible to avoid touching arch/riscv/kernel/time.c and/or
>>> drivers/clocksource/riscv_timer.c altogether, and use the unmodified
>>> upstream clock initialization code path.
>>>
>>> I've built and tested this successfully on the nexys4ddr (with only
>>> busybox for now), using the latest 4.19 kernel and standard
>>> riscv/riscv-gnu-toolchain:
>>>
>>>   make ARCH=riscv lowrisc_defconfig
>>>   cp ../initramfs.cpio .
>>>   make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu-
>>>
>>> ... then built the bbl (boot.bin) file after applying the above
>>> mentioned patch to the device tree source file.
>>>
>>> I'd like to run the newly introduced .[ch] files through
>>> scripts/checkpatch.pl and generally bring them up to kernel coding
>>> standards, but wanted to get your overall opinion, thoughs, and
>>> feedback before I started doing anything like that :)
>>>
>> Your assistance is gratefully received, and I get the idea behind
>> rebasing to 4.19, but what about Synchronization with a particular
>> version of Verilog? Would you want to add a version register in each
>> Verilog device to ensure the Linux driver matches the hardware
>> description during booting?
> As far as I can tell, if we're ok accomodating the timer initialization
> via my proposed DT change, and if we don't need to worry about the
> modified IO_TLB_DEFAULT_SIZE, all that's left is drivers for the
> nexys4ddr peripherals: keyboard, video, network, card-reader, and GPIO.
>
> Adding version registers would be worth considering, but only if/when
> it's time to push for actual upstream inclusion of the patches.
> Otherwise linux driver patches matching the current verilog RTL rebased
> to cleanly apply to the current kernel would be sufficient IMHO.
>
>> For the same reason the kernel maintainer might not be too enthusiastic
>> about integrating and maintaining something as ephemeral as an FPGA net
>> list.
> As long as it's likely for the RTL to change, I agree, it's not worth
> trying to officially push for upstream inclusion. But presenting the
> changes in "upstream-ready" form is IMHO still worth doing for
> external legibility.
>
> Not sure how to best track the development of the individual drivers
> themselves if we're to present them as "upstream-ready", that's a good
> question. Do you expect the keyboard/video/ethernet/etc. drivers to
> still undergo significant development in a way that could benefit from
> tracking changs with git? Maybe having a dedicated repository for
> lowrisc driver files only, and generate "upstream-ready" patches from
> there periodically might be an option?

I think the drivers on this release will not change much (just bug 
fixed). If somebody wants

to boost performance for a hypothetical v0.7 then the hardware and 
driver might well change,

and in this case having a means to test if the acceleration feature was 
available and defaulting to the

current method would be preferable. For example the v0.5 release had one 
incoming

Ethernet buffer, v0.6 has eight. This means juggling the memory map and 
making quite

a few changes, but overall the driver still resembles the v0.5 release 
one. But those who regard the

FPGA code as part of the "firmware" or even the "software" might 
disagree with upstreaming.

The next driver hardware change to come will be emulated VGA graphics.

I guess this will be implemented with /dev/fb, which would be a separate 
patch in your way of thinking,

but could and probably would have an impact on the video driver.




>>> Please let me know what you think.
>> I think cutting down the size of the needed eco system is an excellent
>> notion. If you could share more details of your intended application
>> that would be especially interesting for us.
> I'm trying to build the smallest CPU core that could still boot Linux
> (probably some form of rv32ima[c], maybe from Pulp), and needed to start
> with something that already works, understand it, and try to remove it
> and scale it down as far as possible. The opposite direction (start with
> a tiny core, and add things to it until it boots Linux) seemed like it
> had too many ways to go wrong, so I decided to try and scale *down*
> instead of *up* :)

If you entering a competition for the smallest Linux RTL, then every saving

matters, but as you probably noticed Rocket running at 50MHz makes for a

annoyingy slow Linux box. If you want a usable Linux then the tendency 
is to make

the core larger and add fancy features. You can obviously use Rocket 
Chip generator

to produce small CPUs without floating point and so on, but you will get 
bored with it

faster than you can say 486SX (the famous crippled no FPU Intel 
processor) ...

> Thanks again,
> --Gabriel



More information about the lowrisc-dev mailing list