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
