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

[Bin Meng]

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.

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

Regards,
Bin