Hi Mark,
On 21 October 2016 at 19:32, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> On Thu, Sep 29, 2016 at 02:17:15AM +0800, fu.wei@xxxxxxxxxx wrote:
>> From: Fu Wei <fu.wei@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
