Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Yinbo Zhu <zhuyinbo@loongson.cn>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Huacai Chen <chenhuacai@kernel.org>,
        WANG Xuerui <kernel@xen0n.name>,
        Jiaxun Yang <jiaxun.yang@flygoat.com>,
        Jianmin Lv <lvjianmin@loongson.cn>,
        Yun Liu <liuyun@loongson.cn>,
        Yang Li <yang.lee@linux.alibaba.com>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        loongarch@lists.linux.dev, zhuyinbo@loongson.cn
Subject: Re: [PATCH v11 1/3] clocksource: loongson2_hpet: add hpet driver
 support
In-Reply-To: <c46e5ebf-5293-5123-52d3-b3594c6e9244@loongson.cn>
References: <20221129030925.14074-1-zhuyinbo@loongson.cn>
 <87k03bs6pj.ffs@tglx> <c46e5ebf-5293-5123-52d3-b3594c6e9244@loongson.cn>
Date:   Fri, 02 Dec 2022 10:04:12 +0100
Message-ID: <87bkomqir7.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Dec 02 2022 at 12:36, Yinbo Zhu wrote:
> =E5=9C=A8 2022/12/1 19:29, Thomas Gleixner =E5=86=99=E9=81=93:
>>
>>> +static DEFINE_SPINLOCK(hpet_lock);
>> This wants to be a raw spinlock if at all. But first you have to explain
>> the purpose of this lock.
>>
>>> +DEFINE_PER_CPU(struct clock_event_device, hpet_clockevent_device);
>> Why needs this to be global and why is it needed at all?
>>
>> This code does support exactly _ONE_ clock event device.
>
> This is consider that the one hardware clock_event_device is used for=20
> multiple cpu cores,
>
> and earch cpu cores has a device from its perspective, so add=20
> DEFINE_SPINLOCK(hpet_lock)
>
> and DEFINE_PER_CPU(struct clock_event_device, hpet_clockevent_device),
>
> the use of locks described below is also this reason .

You cannot use _ONE_ clock event device as per CPU clock event device
for multiple CPUs. That simply cannot work ever.

There are two types of clockevent devices:

      1) strictly per CPU, i.e. one distinct device per CPU

      2) global broadcast device

The global broadcast device is used if there are less physical devices
than CPUs or to handle the cases where the per CPU device stops in
deep idle states.

For the case that there are less physical devices than CPUs, you have to
install dummy per CPU devices. Grep for CLOCK_EVT_FEAT_DUMMY.

The core code will use the broadcast device to provide timer interrupts
and it propagates them to the CPUs which are backed by a dummy per CPU
device via IPIs.

None of this needs a lock in the driver code unless the hardware is
really dumb designed and has a register shared with something else.

The serialization for all clockevent devices is completely provided by
the core code.

>> Seriously, this is not how it works. Instead of copy & paste, we create
>> shared infrastructure and just keep the real architecture specific
>> pieces separate.
>
> I don't find the shared infrastructure in LoongArch, I want to support=C2=
=A0=20
> hpet for LoongArch

Of course you can't find shared infrastructure because there is none.

That's the whole point. Instead of creating copies of code, you rework
the code so that the common parts can be shared between x86, longson and
loongarch. Then you have the architecture/platform specific pieces which
deal with the specific enumeration/initialization and use the shared
infrastructure.

Thanks,

        tglx