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

Gabriel L. Somlo gsomlo at gmail.com
Wed Oct 31 14:31:56 GMT 2018

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?
> > 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)

	- 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? :)

	- 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).

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 :)

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 upsream 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? 

> > 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* :)

Thanks again,

More information about the lowrisc-dev mailing list