Messages in this thread | | | From | Anup Patel <> | Date | Wed, 20 May 2020 06:44:44 +0530 | Subject | Re: [PATCH 09/10] timer-riscv: Fix undefined riscv_time_val |
| |
On Tue, May 19, 2020 at 7:21 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 19/05/2020 14:39, Kefeng Wang wrote: > > > > On 2020/5/19 4:23, Daniel Lezcano wrote: > >> Hi Kefeng, > >> > >> On 18/05/2020 17:40, Kefeng Wang wrote: > >>> On 2020/5/18 22:09, Daniel Lezcano wrote: > >>>> On 13/05/2020 23:14, Palmer Dabbelt wrote: > >>>>> On Sun, 10 May 2020 19:20:00 PDT (-0700), wangkefeng.wang@huawei.com > >>>>> wrote: > >>>>>> ERROR: modpost: "riscv_time_val" [crypto/tcrypt.ko] undefined! > >>>>>> > >>>>>> Reported-by: Hulk Robot <hulkci@huawei.com> > >>>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > >>>>>> --- > >>>>>> drivers/clocksource/timer-riscv.c | 1 + > >>>>>> 1 file changed, 1 insertion(+) > >>>>>> > >>>>>> diff --git a/drivers/clocksource/timer-riscv.c > >>>>>> b/drivers/clocksource/timer-riscv.c > >>>>>> index c4f15c4068c0..071b8c144027 100644 > >>>>>> --- a/drivers/clocksource/timer-riscv.c > >>>>>> +++ b/drivers/clocksource/timer-riscv.c > >>>>>> @@ -19,6 +19,7 @@ > >>>>>> > >>>>>> u64 __iomem *riscv_time_cmp; > >>>>>> u64 __iomem *riscv_time_val; > >>>>>> +EXPORT_SYMBOL(riscv_time_val); > >>>>>> > >>>>>> static inline void mmio_set_timer(u64 val) > >>>>>> { > >>>>> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com> > >>>>> Acked-by: Palmer Dabbelt <palmerdabbelt@google.com> > >>>>> > >>>>> Adding the clocksource maintainers. Let me know if you want this > >>>>> through my > >>>>> tree, I'm assuming you want it through your tree. > >>>> How can we end up by an export symbol here ?! > >>> Hi Danile, > >> s/Danile/Daniel/ > > Sorry for typing error. > >> > >>> Found this build error when CONFIG_RISCV_M_MODE=y and CONFIG_RISCV_SBI > >>> is not, > >>> > >>> see patch "4f9bbcefa142 riscv: add support for MMIO access to the timer > >>> registers" > >> Thanks for the pointer. > >> > >> The question still remains, how do we end up with this EXPORT_SYMBOL? > >> > >> There is something wrong if the fix is an EXPORT_SYMBOL for a global > >> variable. > > > > Not very clear, there are some global variable( eg, acpi_disabled, > > memstart_addr in arm64,) is exported by EXPORT_SYMBOL, do you mean that > > export riscv_time_val is wrong way? > > I do not maintain acpi neither arm64.mm. > > AFAICT, riscv_time_val is globally declared in > drivers/clocksource/timer-riscv.c > > The driver does not use this variable at all. Then there is a readl on > it in the header file arch/riscv/include/asm/timex.h > > And finally it is initialized in arch/riscv/kernel/clint.c > > Same thing for riscv_time_cmp. > > The correct fix is to initialize the variables in the place where they > belong to (drivers/clocksource/timer-riscv.c), create a function to read > their content and export-symbol-gpl the function.
I agree with Daniel. Exporting riscv_time_val is a temporary fix.
The problem is timer-riscv.c is pretty convoluted right now. It is implementing two different clocksources and clockevents in one-place.
I think we need two separate drivers for RISC-V world.
1. timer-riscv: This for regular S-mode kernel with MMU. The clocksource will use TIME CSR and the clockevent device will use SBI calls.
2. timer-clint: This for M-mode kernel without MMU (or NoMMU kernel). The clocksource will use MMIO counter for clocksource and the clockevent device will use MMIO compare registers.
I will send a patch to have a separate timer-clint driver under drivers/clocksource. (@Daniel, I hope you will be fine with this?)
Regards, Anup
> > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog
| |