linux-kernel - Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADyBb7sJfY4=5NKKPGosOGv4sq4Tg2_TBW-XbWYoqfz4JNE-LA@mail.gmail.com>
Date:   Wed, 26 Oct 2016 23:24:32 +0800
From:   Fu Wei <fu.wei@...aro.org>
To:     Mark Rutland <mark.rutland@....com>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <marc.zyngier@....com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Sudeep Holla <sudeep.holla@....com>,
        Hanjun Guo <hanjun.guo@...aro.org>,
        linux-arm-kernel@...ts.infradead.org,
        Linaro ACPI Mailman List <linaro-acpi@...ts.linaro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        rruigrok@...eaurora.org, "Abdulhamid, Harb" <harba@...eaurora.org>,
        Christopher Covington <cov@...eaurora.org>,
        Timur Tabi <timur@...eaurora.org>,
        G Gregory <graeme.gregory@...aro.org>,
        Al Stone <al.stone@...aro.org>, Jon Masters <jcm@...hat.com>,
        Wei Huang <wei@...hat.com>, Arnd Bergmann <arnd@...db.de>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>,
        Leo Duran <leo.duran@....com>,
        Wim Van Sebroeck <wim@...ana.be>,
        Guenter Roeck <linux@...ck-us.net>,
        linux-watchdog@...r.kernel.org, Tomasz Nowicki <tn@...ihalf.com>,
        Christoffer Dall <christoffer.dall@...aro.org>,
        Julien Grall <julien.grall@....com>
Subject: Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the
 timer init code to prepare for GTDT

Hi Mark,

On 21 October 2016 at 19:32, Mark Rutland <mark.rutland@....com> wrote:
> On Thu, Sep 29, 2016 at 02:17:15AM +0800, fu.wei@...aro.org wrote:
>> From: Fu Wei <fu.wei@...aro.org>
>>
>> The patch refactor original memory-mapped timer init code:
>> (1) extract some subfunction for reusing some common code
>>     a. get_cnttidr
>>     b. is_best_frame
>> (2) move base address and irq code for arch_timer_mem to
>> arch_timer_mem_register
>>
>> Signed-off-by: Fu Wei <fu.wei@...aro.org>
>> ---
>>  drivers/clocksource/arm_arch_timer.c | 159 +++++++++++++++++++++--------------
>>  1 file changed, 96 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index c7b0040..e78095f 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -57,6 +57,7 @@
>>  static unsigned arch_timers_present __initdata;
>>
>>  static void __iomem *arch_counter_base;
>> +static void __iomem *cntctlbase __initdata;
>>
>>  struct arch_timer {
>>       void __iomem *base;
>> @@ -656,15 +657,49 @@ out:
>>       return err;
>>  }
>>
>> -static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
>> +static int __init arch_timer_mem_register(struct device_node *np, void *frame)
>>  {
>> -     int ret;
>> -     irq_handler_t func;
>> +     struct device_node *frame_node = NULL;
>>       struct arch_timer *t;
>> +     void __iomem *base;
>> +     irq_handler_t func;
>> +     unsigned int irq;
>> +     int ret;
>> +
>> +     if (!frame)
>> +             return -EINVAL;
>
> Why would we call this without a frame?

Sorry, I just verify it , make sure frame is not NULL,
Because it is a "static" function, so we do need this check?

>
>> +
>> +     if (np) {
>
> ... or without a node?

For "np", for now, we just  just verify it, but it is just paperation
for GTDT support,
Because in next patch, if np == NULL, that means we call this function
from GTDT, but not DT.

>
>> +             frame_node = (struct device_node *)frame;
>> +             base = of_iomap(frame_node, 0);
>> +             arch_timer_detect_rate(base, np);
>
> ... BANG! (we check base too late, below).
>
> Please as Marc requested several versions ago: split the FW parsing
> (ACPI and DT) so that happens first, *then* once we have the data in a
> common format, use that to drive poking the HW, requesting IRQs, etc,
> completely independent of the source.
>
> In patches, do this by:
>
> (1) adding the data structures
> (2) splitting the existing DT probing to use them
> (3) Adding ACPI functionality atop

this patch is a preparation for GTDT support, I have splitted some
functions for reusing them in next patch(GTDT support)

if np == NULL, that means we call this function from GTDT, but
if np != NULL, that means we call this function from DT


>
>> -static int __init arch_timer_mem_init(struct device_node *np)
>> +static int __init get_cnttidr(struct device_node *np, u32 *cnttidr)
>>  {
>> -     struct device_node *frame, *best_frame = NULL;
>> -     void __iomem *cntctlbase, *base;
>> -     unsigned int irq, ret = -EINVAL;
>> -     u32 cnttidr;
>> +     if (!cnttidr)
>> +             return -EINVAL;
>> +
>> +     if (np)
>> +             cntctlbase = of_iomap(np, 0);
>> +     else
>> +             return -EINVAL;
>
> We want to check this for ACPI too, no?

just like I said above, this is a preparation for GTDT support,

So please correct me if I am doing this in the wrong way, thanks :-)

>
> Thanks,
> Mark.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat