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 > <[email protected]> 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 > > > <[email protected]> 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 > > > > > <[email protected]> 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 <[email protected]> > > > > > > > --- > > > > > > > > > > > > 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
