| Message ID | 20200511022001.179767-10-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: make riscv build happier | expand |
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. Thanks!
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 ?!
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, 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. > >
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/ > 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.
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? > > >
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.
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
On 2020/5/20 9:14, Anup Patel wrote: > 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. ok, it's better. thanks for your explanation. > I agree with Daniel. Exporting riscv_time_val is a temporary fix. yes. it's only for build, let's wait for Anup's patch. > > 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 > . >
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) {
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(+)