Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1505807 > unrolled thread
| Started by | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| First post | 2016-10-21 13:40 +0200 |
| Last post | 2016-10-26 18:10 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT Mark Rutland <mark.rutland@arm.com> - 2016-10-21 13:40 +0200
Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT Fu Wei <fu.wei@linaro.org> - 2016-10-26 17:30 +0200
Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT Mark Rutland <mark.rutland@arm.com> - 2016-10-26 17:50 +0200
Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT Fu Wei <fu.wei@linaro.org> - 2016-10-26 18:10 +0200
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2016-10-21 13:40 +0200 |
| Subject | Re: [PATCH v14 7/9] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT |
| Message-ID | <suGl5-4T9-37@gated-at.bofh.it> |
On Thu, Sep 29, 2016 at 02:17:15AM +0800, fu.wei@linaro.org wrote:
> From: Fu Wei <fu.wei@linaro.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@linaro.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?
> +
> + if (np) {
... or without a node?
> + 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
> -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?
Thanks,
Mark.
[toc] | [next] | [standalone]
| From | Fu Wei <fu.wei@linaro.org> |
|---|---|
| Date | 2016-10-26 17:30 +0200 |
| Message-ID | <swyjo-5Fl-3@gated-at.bofh.it> |
| In reply to | #1505807 |
Hi Mark,
On 21 October 2016 at 19:32, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Sep 29, 2016 at 02:17:15AM +0800, fu.wei@linaro.org wrote:
>> From: Fu Wei <fu.wei@linaro.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@linaro.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
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2016-10-26 17:50 +0200 |
| Message-ID | <swyCK-5Mj-35@gated-at.bofh.it> |
| In reply to | #1509527 |
On Wed, Oct 26, 2016 at 11:24:32PM +0800, Fu Wei wrote:
> On 21 October 2016 at 19:32, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Sep 29, 2016 at 02:17:15AM +0800, fu.wei@linaro.org wrote:
> >> +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?
I'd rather we simply don't call the function rather than passing a NULL
frame in.
> >> +
> >> + 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.
Please don't do that. More on that 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
As above, please structure the patches such that that never happens.
We currently have:
+--------------------------+
| DT Parsing + Common code |
+--------------------------+
Per (1 and 2) make this:
+------------+ +-------------+
| DT parsing |--(common structure)-->| Common code |
+------------+ +-------------+
Then per (3) make this:
+------------+
| DT parsing |--(common structure)------+
+------------+ |
v
+-------------+
| Common code |
+-------------+
^
+--------------+ |
| ACPI parsing |--(common structure)----+
+--------------+
Thanks,
Mark.
[toc] | [prev] | [next] | [standalone]
| From | Fu Wei <fu.wei@linaro.org> |
|---|---|
| Date | 2016-10-26 18:10 +0200 |
| Message-ID | <swyW6-6ba-41@gated-at.bofh.it> |
| In reply to | #1509543 |
Hi Mark,
On 26 October 2016 at 23:46, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Oct 26, 2016 at 11:24:32PM +0800, Fu Wei wrote:
>> On 21 October 2016 at 19:32, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Thu, Sep 29, 2016 at 02:17:15AM +0800, fu.wei@linaro.org wrote:
>> >> +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?
>
> I'd rather we simply don't call the function rather than passing a NULL
> frame in.
>
OK, NP, will do
>> >> +
>> >> + 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.
>
> Please don't do that. More on that 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
>
> As above, please structure the patches such that that never happens.
>
> We currently have:
>
> +--------------------------+
> | DT Parsing + Common code |
> +--------------------------+
>
> Per (1 and 2) make this:
>
> +------------+ +-------------+
> | DT parsing |--(common structure)-->| Common code |
> +------------+ +-------------+
>
> Then per (3) make this:
>
> +------------+
> | DT parsing |--(common structure)------+
> +------------+ |
> v
> +-------------+
> | Common code |
> +-------------+
> ^
> +--------------+ |
> | ACPI parsing |--(common structure)----+
Thanks for your suggestion , I will do this way in my next patchset
Thanks :-)
> +--------------+
>
> Thanks,
> Mark.
--
Best regards,
Fu Wei
Software Engineer
Red Hat
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web