Re: [PATCH] clocksource/drivers/tango-xtal: Replace code by clocksource_mmio_init

[Marc Gonzalez]

On 13/11/2015 15:16, Daniel Lezcano wrote:
> On 11/13/2015 01:20 PM, Marc Gonzalez wrote:
>> On 13/11/2015 11:58, Daniel Lezcano wrote:
>>
>>> The current code to initialize, register and read the clocksource is
>>> already factored out in mmio.c via the clocksource_mmio_init function.
>>>
>>> Factor out the code with the clocksource_mmio_init function.
>>
>> The reason I didn't like clocksource_mmio_init() is because it exports
>> 4 generic accessors.
>>
>> I guess this function makes more sense when all platforms are using it,
>> in an ARCH_MULTIPLATFORM kernel. (Also the accessors are probably quite
>> small, so the waste is probably minimal.)
> 
> Right.
> 
>> In my opinion, defining struct clocksource_mmio with reg "outside"
>> struct clocksource leads to less efficient(1) and less clear(2) code.
>> 1) because of the padding from ____cacheline_aligned
>> 2) because of the container_of() gymnastics
> 
> I am not sure that would impact the cacheline.

I'm saying that, because of the alignment, the compiler has to make
"struct clocksource_mmio" bigger than a "struct clocksource" with one
more field, because of the padding required.

>> Should the reg field be considered "hot-path data"?
> 
> Yes.
> 
>> One problem with my patch: if some ports define CLKSRC_MMIO but
>> have lots of static struct clocksource, the extra reg field might
>> waste memory / worsen cache locality?
> 
> Yes. But the current situation is we have the base address always 
> defined in different drivers, so that won't change the situation too much.
> 
> The clocksource and the clock_event_device have some commons fields.
> 
> I am wondering if we can create a common structure for both containing 
> those fields and use them, as the network stack does with the routes, we 
> should have a common structure to deal with, without using the container of.
> 
> For example:
> 
> struct clockcommon {
> 	u32 mult;
> 	u32 shift;
> 	int rating;
> 	void __iomem *base;
> 	char *name;
> 	int irq;
> };
> 
> struct clocksource {
> 	struct clockcommon common; /* MUST be the first field */
> 	cycle_t (*read)(struct clocksource *cs);
> 	cycle_t mask;
> 	...
> };

According to my notes, commit 369db4c952 grouped hot-path data
into a single cache line (hence ____cacheline_aligned).

(AFAIR, ARMv7 ARCH_MULTIPLATFORM assumes CACHE_LINE=64)

Not sure how to make the two concepts (common base struct and
grouping hot data) play nicely, without wasting a lot of space
on padding.


> struct clockevent {
> 	struct clockcommon common; /* MUST be the first field */
> 	ktime_t next_event;
> 	...
> };
> 
> int clocksource_init(struct clockcommon *clock)
> {
> 	struct clocksource *clksrc = (struct clocksource *)clock;
> }
> 
> int clockevent_init(struct clockcommon *clock)
> {
> 	struct clockevent *clkevt = (struct clockevent *)clock;
> }
> 
> Hence we have the base address for both and we can remove the base@ from 
> all the drivers.
> 
> The good thing with the mmio is, as you mentioned, instead of having 
> multiple clocksource structure defined, we have just one allocated at 
> init time. The rest falls in the __init section and unloaded.
> 
> This approach is valid for the multiplatform and I believe all SoC will 
> migrate to it.
> 
>> Also, maybe the fields should be copied in ascending order?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/