Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <mhng-0c491e9e-41fd-4f76-a048-55c03d9359f0@palmerdabbelt-glaptop1>
 <66121f9a-48f3-d3a5-7c96-d71397e12aed@linaro.org> <0bc3eb36-7b9d-7c86-130c-68b566e85c10@huawei.com>
 <e1916079-c209-bfad-6b0c-ccfb2e136ca4@linaro.org> <29dc112e-d8c2-2749-7f5d-7c0c19aa9092@huawei.com>
 <8c5ecbd3-c23a-ccd4-b5d8-2e7d2bd10699@linaro.org>
In-Reply-To: <8c5ecbd3-c23a-ccd4-b5d8-2e7d2bd10699@linaro.org>
From:   Anup Patel <anup@brainfault.org>
Date:   Wed, 20 May 2020 06:44:44 +0530
Message-ID: <CAAhSdy2wsnkYMLpT1_2OAjr334t6dAy9LCPNefC=h+aPuRqFrg@mail.gmail.com>
Subject: Re: [PATCH 09/10] timer-riscv: Fix undefined riscv_time_val
To:     Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     Kefeng Wang <wangkefeng.wang@huawei.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>,
        hulkci@huawei.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.co=
m
> >>>>> 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=3Dy and CONFIG_RISCV_=
SBI
> >>> is not,
> >>>
> >>> see patch "4f9bbcefa142 riscv: add support for MMIO access to the tim=
er
> >>> 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 tha=
t
> > 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 =E2=94=82 Open source software for AR=
M SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog