From: Daniel Lezcano <daniel.lezcano@linaro.org> To: Rich Felker <dalias@libc.org> Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-sh@vger.kernel.org, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Thomas Gleixner <tglx@linutronix.de>, Marc Zyngier <Marc.Zyngier@arm.com> Subject: Re: [PATCH v6 2/2] clocksource: add J-Core timer/clocksource driver Date: Fri, 26 Aug 2016 09:04:57 +0000 [thread overview] Message-ID: <57C00639.7060509@linaro.org> (raw) In-Reply-To: <20160824174001.GW15995@brightrain.aerifal.cx> On 08/24/2016 07:40 PM, Rich Felker wrote: [ ... ] >>> +config CLKSRC_JCORE_PIT >>> + bool "J-Core PIT timer driver" >>> + depends on OF && (SUPERH || COMPILE_TEST) >> >> Even if this is correct, for the sake of consistency, it is preferable >> to change it to: >> >> bool "J-Core PIT timer driver" if COMPILE_TEST >> depends on SUPERH >> select CLKSRC_OF > > Is this functionally equivalent? If so that's non-obvious to me. What > about the dropped OF dependency? The intent is that the option should > always be available for SUPERH targets using OF, otherwise only > available with COMPILE_TEST.ig It is not equivalent but consistent with the other options where it is not possible to manually set/unset the driver config wuthout COMPILE_TEST. >>> +static int jcore_pit_set(unsigned long delta, struct jcore_pit *pit) >>> +{ >>> + jcore_pit_disable(pit); >>> + __raw_writel(delta, pit->base + REG_THROT); >>> + __raw_writel(pit->enable_val, pit->base + REG_PITEN); >>> + return 0; >> >> Why do you need to use __raw_writel ? >> >> s/__raw_writel/writel/ > > I actually tried multiple times to find good resources on policy for > which form to prefer, but didn't have much luck. My understanding is > that __raw_writel/__raw_readl always performs a native-endian > load/store, whereas writel/readl behavior depends on cpu endianness > and whether the arch declares that "pci bus" (that was the term I > found in the source, not relevant here) io is endian-swapped or not. > Can you give me a better explanation of why we might prefer one form > or the other? [ skipping this as it was very well commented by Arnd and Mark ] >>> + freq = DIV_ROUND_CLOSEST(NSEC_PER_SEC, buspd); >>> + pit->periodic_delta = DIV_ROUND_CLOSEST(NSEC_PER_SEC, HZ*buspd); >> >> ---> HZ * buspd > > OK. > >>> + clockevents_config_and_register(&pit->ced, freq, 1, ULONG_MAX); >>> + >>> + return 0; >>> +} >>> + >>> +static int jcore_pit_local_shutdown(unsigned cpu) >>> +{ >>> + return 0; >>> +} >> >> This function is useless I think. AFAIU, cpuhp_setup_state can have a >> NULL function for the cpu teardown. > > OK, I wasn't sure if that was permitted. > >>> + jcore_cs.name = "jcore_pit_cs"; >>> + jcore_cs.rating = 400; >>> + jcore_cs.read = jcore_clocksource_read; >>> + jcore_cs.mult = 1; >>> + jcore_cs.shift = 0; >>> + jcore_cs.mask = CLOCKSOURCE_MASK(32); >>> + jcore_cs.flags = CLOCK_SOURCE_IS_CONTINUOUS; >>> + >>> + err = clocksource_register_hz(&jcore_cs, NSEC_PER_SEC); >>> + if (err) { >>> + pr_err("Error registering clocksource device: %d\n", err); >>> + return err; >>> + } >> >> May be you can consider by replacing the above by: >> >> clocksource_mmio_init(jcore_pit_base, "jcore_pit_cs", >> NSEC_PER_SEC, 32, >> jcore_clocksource_read); > > I think you're missing the rating argument. Otherwise it should work, > but is there a good reason to prefer it? The code is slightly simpler; > I'm not sure if using kzalloc vs static storage is better or worse. Probably clksrc field pointers are pre-calculated at compile time while the allocation results in the code into an indirection to compute the pointers, but I don't think it is noticeable. [ ... ] >>> + /* >>> + * The J-Core PIT is not hard-wired to a particular IRQ, but >>> + * integrated with the interrupt controller such that the IRQ it >>> + * generates is programmable. The programming interface has a >>> + * legacy field which was an interrupt priority for AIC1, but >>> + * which is OR'd onto bits 2-5 of the generated IRQ number when >>> + * used with J-Core AIC2, so set it to match these bits. >>> + */ >>> + hwirq = irq_get_irq_data(pit_irq)->hwirq; >> >> irq_hw_number_t hwirq; >> hwirq = irqd_to_hwirq(irq_get_irq_data(pit_irq)); > > OK. > >>> + irqprio = (hwirq >> 2) & PIT_PRIO_MASK; >>> + enable_val = (1U << PIT_ENABLE_SHIFT) >>> + | (hwirq << PIT_IRQ_SHIFT) >>> + | (irqprio << PIT_PRIO_SHIFT); >> >> >> I'm missing the connection between the description above and the enable >> value computed here. Can you elaborate ? > > The irqprio field is filled in using a value that matches bits 2 and > up of hwirq; this is what the comment says and what the code does. Can > you elaborate on what you don't understand? The API to compute the 'enable_val'. Having a technical reference manual would help a lot. -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Lezcano <daniel.lezcano@linaro.org> To: Rich Felker <dalias@libc.org> Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-sh@vger.kernel.org, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Thomas Gleixner <tglx@linutronix.de>, Marc Zyngier <Marc.Zyngier@arm.com> Subject: Re: [PATCH v6 2/2] clocksource: add J-Core timer/clocksource driver Date: Fri, 26 Aug 2016 11:04:57 +0200 [thread overview] Message-ID: <57C00639.7060509@linaro.org> (raw) In-Reply-To: <20160824174001.GW15995@brightrain.aerifal.cx> On 08/24/2016 07:40 PM, Rich Felker wrote: [ ... ] >>> +config CLKSRC_JCORE_PIT >>> + bool "J-Core PIT timer driver" >>> + depends on OF && (SUPERH || COMPILE_TEST) >> >> Even if this is correct, for the sake of consistency, it is preferable >> to change it to: >> >> bool "J-Core PIT timer driver" if COMPILE_TEST >> depends on SUPERH >> select CLKSRC_OF > > Is this functionally equivalent? If so that's non-obvious to me. What > about the dropped OF dependency? The intent is that the option should > always be available for SUPERH targets using OF, otherwise only > available with COMPILE_TEST.ig It is not equivalent but consistent with the other options where it is not possible to manually set/unset the driver config wuthout COMPILE_TEST. >>> +static int jcore_pit_set(unsigned long delta, struct jcore_pit *pit) >>> +{ >>> + jcore_pit_disable(pit); >>> + __raw_writel(delta, pit->base + REG_THROT); >>> + __raw_writel(pit->enable_val, pit->base + REG_PITEN); >>> + return 0; >> >> Why do you need to use __raw_writel ? >> >> s/__raw_writel/writel/ > > I actually tried multiple times to find good resources on policy for > which form to prefer, but didn't have much luck. My understanding is > that __raw_writel/__raw_readl always performs a native-endian > load/store, whereas writel/readl behavior depends on cpu endianness > and whether the arch declares that "pci bus" (that was the term I > found in the source, not relevant here) io is endian-swapped or not. > Can you give me a better explanation of why we might prefer one form > or the other? [ skipping this as it was very well commented by Arnd and Mark ] >>> + freq = DIV_ROUND_CLOSEST(NSEC_PER_SEC, buspd); >>> + pit->periodic_delta = DIV_ROUND_CLOSEST(NSEC_PER_SEC, HZ*buspd); >> >> ---> HZ * buspd > > OK. > >>> + clockevents_config_and_register(&pit->ced, freq, 1, ULONG_MAX); >>> + >>> + return 0; >>> +} >>> + >>> +static int jcore_pit_local_shutdown(unsigned cpu) >>> +{ >>> + return 0; >>> +} >> >> This function is useless I think. AFAIU, cpuhp_setup_state can have a >> NULL function for the cpu teardown. > > OK, I wasn't sure if that was permitted. > >>> + jcore_cs.name = "jcore_pit_cs"; >>> + jcore_cs.rating = 400; >>> + jcore_cs.read = jcore_clocksource_read; >>> + jcore_cs.mult = 1; >>> + jcore_cs.shift = 0; >>> + jcore_cs.mask = CLOCKSOURCE_MASK(32); >>> + jcore_cs.flags = CLOCK_SOURCE_IS_CONTINUOUS; >>> + >>> + err = clocksource_register_hz(&jcore_cs, NSEC_PER_SEC); >>> + if (err) { >>> + pr_err("Error registering clocksource device: %d\n", err); >>> + return err; >>> + } >> >> May be you can consider by replacing the above by: >> >> clocksource_mmio_init(jcore_pit_base, "jcore_pit_cs", >> NSEC_PER_SEC, 32, >> jcore_clocksource_read); > > I think you're missing the rating argument. Otherwise it should work, > but is there a good reason to prefer it? The code is slightly simpler; > I'm not sure if using kzalloc vs static storage is better or worse. Probably clksrc field pointers are pre-calculated at compile time while the allocation results in the code into an indirection to compute the pointers, but I don't think it is noticeable. [ ... ] >>> + /* >>> + * The J-Core PIT is not hard-wired to a particular IRQ, but >>> + * integrated with the interrupt controller such that the IRQ it >>> + * generates is programmable. The programming interface has a >>> + * legacy field which was an interrupt priority for AIC1, but >>> + * which is OR'd onto bits 2-5 of the generated IRQ number when >>> + * used with J-Core AIC2, so set it to match these bits. >>> + */ >>> + hwirq = irq_get_irq_data(pit_irq)->hwirq; >> >> irq_hw_number_t hwirq; >> hwirq = irqd_to_hwirq(irq_get_irq_data(pit_irq)); > > OK. > >>> + irqprio = (hwirq >> 2) & PIT_PRIO_MASK; >>> + enable_val = (1U << PIT_ENABLE_SHIFT) >>> + | (hwirq << PIT_IRQ_SHIFT) >>> + | (irqprio << PIT_PRIO_SHIFT); >> >> >> I'm missing the connection between the description above and the enable >> value computed here. Can you elaborate ? > > The irqprio field is filled in using a value that matches bits 2 and > up of hwirq; this is what the comment says and what the code does. Can > you elaborate on what you don't understand? The API to compute the 'enable_val'. Having a technical reference manual would help a lot. -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
next prev parent reply other threads:[~2016-08-26 9:04 UTC|newest] Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-08-03 0:18 [PATCH v6 0/2] J-Core timer support Rich Felker 2016-08-03 0:18 ` Rich Felker [not found] ` <cover.1470183518.git.dalias-8zAoT0mYgF4@public.gmane.org> 2016-03-17 23:12 ` [PATCH v6 2/2] clocksource: add J-Core timer/clocksource driver Rich Felker 2016-03-17 23:12 ` Rich Felker 2016-03-17 23:12 ` Rich Felker 2016-05-17 23:18 ` [PATCH v6 1/2] of: add J-Core timer bindings Rich Felker 2016-08-04 4:30 ` [PATCH v6 0/2] J-Core timer support Rich Felker 2016-08-04 4:30 ` Rich Felker 2016-08-04 4:30 ` Rich Felker [not found] ` <cover.147018b3518.git.dalias-8zAoT0mYgF4@public.gmane.org> 2016-08-04 4:30 ` [PATCH v6 1/2] of: add J-Core timer bindings Rich Felker 2016-08-04 4:30 ` Rich Felker 2016-08-04 4:30 ` Rich Felker [not found] ` <5e1d4f3346f69bdd9d840c8b5a855c1f93ff93f6.147018b3518.git.dalias-8zAoT0mYgF4@public.gmane.org> 2016-08-04 18:16 ` Rob Herring 2016-08-04 18:16 ` Rob Herring 2016-08-04 18:16 ` Rob Herring 2016-08-04 4:30 ` [PATCH v6 2/2] clocksource: add J-Core timer/clocksource driver Rich Felker 2016-08-04 4:30 ` Rich Felker 2016-08-04 4:30 ` Rich Felker 2016-08-04 8:24 ` Alexnader Kuleshov 2016-08-04 8:24 ` Alexnader Kuleshov 2016-08-04 19:42 ` Rich Felker 2016-08-04 19:42 ` Rich Felker 2016-08-24 16:42 ` Daniel Lezcano 2016-08-24 16:42 ` Daniel Lezcano 2016-08-24 17:40 ` Rich Felker 2016-08-24 17:40 ` Rich Felker [not found] ` <20160824174001.GW15995-C3MtFaGISjmo6RMmaWD+6Sb1p8zYI1N1@public.gmane.org> 2016-08-24 19:01 ` Marc Zyngier 2016-08-24 19:01 ` Marc Zyngier 2016-08-24 19:01 ` Marc Zyngier 2016-08-24 19:20 ` Rich Felker 2016-08-24 19:20 ` Rich Felker [not found] ` <20160824192009.GX15995-C3MtFaGISjmo6RMmaWD+6Sb1p8zYI1N1@public.gmane.org> 2016-08-24 22:21 ` Mark Rutland 2016-08-24 22:21 ` Mark Rutland 2016-08-24 22:21 ` Mark Rutland 2016-08-24 20:01 ` Arnd Bergmann 2016-08-24 20:01 ` Arnd Bergmann 2016-08-24 20:52 ` Rich Felker 2016-08-24 20:52 ` Rich Felker 2016-08-24 21:22 ` Mark Rutland 2016-08-24 21:22 ` Mark Rutland 2016-08-24 21:44 ` Rich Felker 2016-08-24 21:44 ` Rich Felker 2016-08-24 21:44 ` Rich Felker 2016-08-24 21:57 ` Arnd Bergmann 2016-08-24 21:57 ` Arnd Bergmann 2016-08-25 10:23 ` Arnd Bergmann 2016-08-25 10:23 ` Arnd Bergmann 2016-08-24 22:54 ` Mark Rutland 2016-08-24 22:54 ` Mark Rutland 2016-08-25 8:07 ` Thomas Gleixner 2016-08-25 8:07 ` Thomas Gleixner 2016-08-25 14:56 ` Rich Felker 2016-08-25 14:56 ` Rich Felker 2016-08-25 14:56 ` Rich Felker 2016-08-25 15:41 ` Thomas Gleixner 2016-08-25 15:41 ` Thomas Gleixner 2016-08-25 17:45 ` Rich Felker 2016-08-25 17:45 ` Rich Felker 2016-08-25 16:38 ` Mark Rutland 2016-08-25 16:38 ` Mark Rutland 2016-08-25 17:51 ` Rich Felker 2016-08-25 17:51 ` Rich Felker 2016-08-25 18:21 ` Mark Rutland 2016-08-25 18:21 ` Mark Rutland 2016-08-25 19:20 ` Rich Felker 2016-08-25 19:20 ` Rich Felker 2016-08-26 9:04 ` Daniel Lezcano [this message] 2016-08-26 9:04 ` Daniel Lezcano
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=57C00639.7060509@linaro.org \ --to=daniel.lezcano@linaro.org \ --cc=Marc.Zyngier@arm.com \ --cc=dalias@libc.org \ --cc=devicetree@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-sh@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=robh+dt@kernel.org \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.