[U-Boot] [PATCH 05/19] riscv: Add a SYSCON driver for Core Local Interruptor

Thu Dec 6 12:30:55 UTC 2018

Hi Bin,

On Thu, 2018-12-06 at 18:07 +0800, Bin Meng wrote:
> Hi Lukas,
> 
> On Thu, Dec 6, 2018 at 7:11 AM Auer, Lukas
> <lukas.auer at aisec.fraunhofer.de> wrote:
> > 
> > Hi Bin,
> > 
> > On Wed, 2018-12-05 at 17:59 +0800, Bin Meng wrote:
> > > Hi Lukas,
> > > 
> > > On Wed, Nov 14, 2018 at 6:33 PM Auer, Lukas
> > > <lukas.auer at aisec.fraunhofer.de> wrote:
> > > > 
> > > > Hi Bin,
> > > > 
> > > > On Wed, 2018-11-14 at 09:48 +0800, Bin Meng wrote:
> > > > > Hi Lukas,
> > > > > 
> > > > > On Tue, Nov 13, 2018 at 10:45 PM Auer, Lukas
> > > > > <lukas.auer at aisec.fraunhofer.de> wrote:
> > > > > > 
> > > > > > Hi Bin,
> > > > > > 
> > > > > > On Tue, 2018-11-13 at 00:21 -0800, Bin Meng wrote:
> > > > > > > This adds U-Boot syscon driver for RISC-V Core Local
> > > > > > > Interruptor
> > > > > > > (CLINT). The CLINT block holds memory-mapped control and
> > > > > > > status
> > > > > > > registers associated with software and timer interrupts.
> > > > > > > 
> > > > > > > 3 APIs are provided for U-Boot to implement Supervisor
> > > > > > > Binary
> > > > > > > Interface (SBI) as defined by the RISC-V privileged
> > > > > > > architecture
> > > > > > > spec v1.10.
> > > > > > > 
> > > > > > > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> > > > > > > ---
> > > > > > 
> > > > > > Would it make sense to also abstract the functions provided
> > > > > > by
> > > > > > the
> > > > > > CLINT more? The reason why I am asking is because the CLINT
> > > > > > is
> > > > > > (unfortunately) not specified as part of RISC-V. It is
> > > > > > developing
> > > > > > into
> > > > > > a de facto standard since other platforms are following
> > > > > > SiFive's
> > > > > > implementation, but there is nothing that would prevent
> > > > > > them
> > > > > > from
> > > > > > implementing something else.
> > > > > > 
> > > > > 
> > > > > I think your observation is correct about CLINT. Rick, does
> > > > > Andes's
> > > > > RISC-V processor also follow SiFive's CLINT model?
> > > > > 
> > > > > > Two immediate examples I can think of would be mtime and
> > > > > > the
> > > > > > IPI
> > > > > > implementation. Many SoC vendors will likely already have a
> > > > > > suitable
> > > > > > timer IP block for mtime. I can imagine that they would
> > > > > > choose
> > > > > > to
> > > > > > re-
> > > > > > use their memory map instead of following that of the
> > > > > > CLINT.
> > > > > > For the IPI implementation there is already an alternative,
> > > > > > the
> > > > > > SBI. If
> > > > > > U-Boot should be able to run in supervisor mode, it would
> > > > > > be
> > > > > > helpful to
> > > > > > support both the SBI and the CLINT interface.
> > > > > > 
> > > > > 
> > > > > I am not sure I followed you here. This driver provides 3
> > > > > APIs:
> > > > > riscv_send_ipi() is for IPI, and the other 2 are for
> > > > > mtime/mtimecmp.
> > > > > 
> > > > 
> > > > It does, but I am not sure how easy it is to support different
> > > > devices.
> > > > Supporting the SBI is not going to be an issue, more
> > > > problematic
> > > > would
> > > > be if we have two different devices and device tree nodes to
> > > > implement
> > > > the functionality of the APIs. Now we have to bind this driver
> > > > to
> > > > two
> > > > devices and call the APIs from the correct instantiation, which
> > > > would
> > > > not work.
> > > > 
> > > > Thinking about this a little more, I think the only issue is
> > > > that
> > > > we
> > > > have both IPI- and mtime-related APIs in one driver. How about
> > > > something like this? Instead of binding this driver to
> > > > riscv,clint0, we
> > > > add a new misc driver for the clint0. The only thing the driver
> > > > does,
> > > > is to bind the syscon driver and the timer driver (see for
> > > > example
> > > > tegra-car.c). Other architectures with separate device tree
> > > > nodes
> > > > for
> > > > the API functionality won't need the misc driver and can just
> > > > bind
> > > > the
> > > > devices to the syscon driver and a timer driver.
> > > > 
> > > 
> > > Sorry it took a long time before replying this. I did have a look
> > > at
> > > the tegra-car.c driver, and also tried various experiments. As
> > > Rick
> > > pointed out we have to handle mixed IP blocks like Andes chip for
> > > mtimer and IPIs. So it looks we need be able to flexibly handle
> > > the
> > > following cases (sigh):
> > > 
> > > - SiFive's clint model which implements mtimer and IPI
> > > - mtimer following SiFive's clint model, but IPI does not (Andes
> > > chip)
> > > - IPI following SiFive's clint model, but mtimer follows
> > > (hypothetical
> > > model which does not exist today)
> > > - completely different mtimer and IPI models from SiFive's clint
> > > model
> > > 
> > 
> > This really is not ideal. I don't think there is a nice way of
> > handling
> > this since there is no way to detect which version we have. A
> > cleaner
> > solution would be to get everyone to use separate compatible
> > strings so
> > that we can load the correct driver or use the correct register
> > offsets.
> > I can't actually find a device node for the CLINT in the device
> > tree of
> > Andes' chip. If they already use a different compatible string then
> > this should work.
> > 
> 
> I cannot find the clint compatible string for Andes chip too ...
> 
> > >  I don't quite follow your idea of implementing clint as a misc
> > > driver, then binding syscon and timer devices in the misc driver.
> > > But
> > > I think we can only have the misc driver, and use misc uclass's
> > > ioctl() to implement the SBI required calls (set_timecmp,
> > > get_time,
> > > send_ipi), and we can have another ioctl code to report its
> > > capability
> > > to support mtimer and/or IPI functionality. This can support
> > > flexibility. However I believe this may affect performance if we
> > > go
> > > through many uclass function calls when doing SBI.
> > > 
> > > The solution does not look clean to me :(
> > > 
> > 
> > Yes I agree, that seems too complex. My idea was to only use the
> > misc
> > driver to bind different sub-devices, so that we can separate the
> > functionality of the CLINT into separate drivers.
> > If you are interested, I have pushed my WIP patch series for SMP
> > support to Github [1]. It works, but is not finished yet and, in
> > particular, must be rebased on top of your series. The misc driver
> > for
> > the CLINT0 is added in [2].
> > 
> > Thinking about this a bit more, I think your implementation might
> > be
> > best. RISC-V specifies that all implementation must have an mtime
> > CSR.
> > So it makes sense to have just one timer driver for RISC-V, which
> > accesses mtime using an API. We can then have different syscon
> > drivers
> > to implement the IPI and timer related APIs. So for qemu, we would
> > have
> > one syscon driver for the CLINT0 (when U-Boot is running in machine
> > mode) and one using the SBI functions (when U-Boot is running in
> > supervisor mode).
> 
> Yes, having just one timer driver for RISC-V seems doable. I will try
> to refine current implementation in v2.
> 

Sounds good, looking forward to it. One more question, can you also add
an API to clear the IPI bit?

> > We would need more error handling though to, for example, handle
> > the
> > case where no syscon driver is bound. Essentially an interface for
> > the
> > syscon drivers to implement, similar to a uclass.
> > 
> > Thanks,
> > Lukas
> > 
> > 
> > [1]: https://github.com/lukasauer/u-boot/commits/riscv-smp
> > [2]:
> > 
https://github.com/lukasauer/u-boot/commit/7901e68ed25ac8e3008f76a8f2c5a11e1b8e2952
> 
> BTW: I think I can drop my last patch in my series and let you handle
> the SMP boot in your series. Good work!
> 

Thanks!

Lukas