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

Fri Dec 7 14:00:02 UTC 2018

Hi Lukas,

On Thu, Dec 6, 2018 at 8:30 PM Auer, Lukas
<lukas.auer at aisec.fraunhofer.de> wrote:
>
> 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?
>

Will do in v2.

> > > 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!

Regards,
Bin