Re: [PATCH v2 2/2] clocksource/drivers/timer-microchip-pit64b: add Microchip PIT64B support

[Claudiu.Beznea]

On 04.11.2019 21:05, Thomas Gleixner wrote:
> On Mon, 4 Nov 2019, Claudiu Beznea wrote:
>> +struct mchp_pit64b_common_data {
>> +	void __iomem *base;
>> +	struct clk *pclk;
>> +	struct clk *gclk;
>> +	u64 cycles;
>> +	u8 pres;

[...]

>> +static int __init mchp_pit64b_pres_compute(u32 *pres, u32 clk_rate,
>> +					   u32 max_rate)
>> +{
>> +	u32 tmp;
>> +
>> +	for (*pres = 0; *pres < MCHP_PIT64B_PRES_MAX; (*pres)++) {
>> +		tmp = clk_rate / (*pres + 1);
>> +		if (tmp <= max_rate)
>> +			break;
>> +	}
>> +
>> +	if (*pres == MCHP_PIT64B_PRES_MAX)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int __init mchp_pit64b_pres_prepare(struct mchp_pit64b_common_data *cd,
>> +					   unsigned long max_rate)
>> +{
>> +	unsigned long pclk_rate, diff = 0, best_diff = ULONG_MAX;
>> +	long gclk_round = 0;
>> +	u32 pres, best_pres = 0;
>> +	int ret = 0;
>> +
>> +	pclk_rate = clk_get_rate(cd->pclk);
>> +	if (!pclk_rate)
>> +		return -EINVAL;
>> +
>> +	if (cd->gclk) {
>> +		gclk_round = clk_round_rate(cd->gclk, max_rate);
>> +		if (gclk_round < 0)
>> +			goto pclk;
>> +
>> +		if (pclk_rate / gclk_round < 3)
>> +			goto pclk;
>> +
>> +		ret = mchp_pit64b_pres_compute(&pres, gclk_round, max_rate);
>> +		if (ret)
>> +			best_diff = abs(gclk_round - max_rate);
>> +		else
>> +			best_diff = abs(gclk_round / (pres + 1) - max_rate);
>> +		best_pres = pres;
>> +	}
>> +
>> +pclk:
>> +	/* Check if requested rate could be obtained using PCLK. */
>> +	ret = mchp_pit64b_pres_compute(&pres, pclk_rate, max_rate);
>> +	if (ret)
>> +		diff = abs(pclk_rate - max_rate);
>> +	else
>> +		diff = abs(pclk_rate / (pres + 1) - max_rate);
>> +
>> +	if (best_diff > diff) {
>> +		/* Use PCLK. */
>> +		cd->gclk = NULL;
>> +		best_pres = pres;
>> +	} else {
>> +		clk_set_rate(cd->gclk, gclk_round);
>> +	}
>> +
>> +	cd->pres = best_pres;
>> +
>> +	pr_info("PIT64B: using clk=%s with prescaler %u, freq=%lu [Hz]\n",
>> +		cd->gclk ? "gclk" : "pclk", cd->pres,
>> +		cd->gclk ? gclk_round / (cd->pres + 1)
>> +			 : pclk_rate / (cd->pres + 1));
>> +
>> +	return 0;
> 
> Lots of undocumented functionality which open codes stuff which exists
> already in the clk framework AFAICT.
> 
> Why are you not simply implementing this as clk framework components?
> 
> 
>             |-----|
>   gclk ---->|     |    |---------|
>             | MUX |--->| Divider |->
>   pclk ---->|     |    |---------|
>             |-----|
> 
> which is exaxtly how your hardware looks like. The clk framework has all
> the selection mechanisms in place and all this conditional clock stuff can
> be removed all over the place simply because you just ask for the desired
> frequency on init. Also suspend/resume and all the other stuff just works
> without all the mess involved.
> 

With regards to this: gclk and pclk are clock inputs to PIT64B hardware
block, GCLK rate could be requested and set from clock subsystem, PCLK rate
is fixed and could not be set via clock subsystem.
PCLK is the one that is feeding the PIT64B hardware block. The hardware
block would not work without it. At the same time it could be selected as
clock source for PIT64B's timer.
GCLK could only be selected as clock source for PIT64B's timer. PIT64B
hardware block with its timer functionality could work with only PCLK but
could not work only with GCLK.

The block diagram is as you pointed it:

                               PIT64B
PMC             +------------------------------------+
----+           |   |-----|                          |
    |-->gclk -->|-->|     |    |---------|  +-----+  |
    |           |   | MUX |--->| Divider |->|timer|  |
    |-->pclk -->|-->|     |    |---------|  +-----+  |
----+           |   |-----|                          |
                |      ^                             |
                |     sel                            |
                +------------------------------------+

The divider could be b/w 1 and 16.
Peripheral clock rate, on the platform that I'm using, is fixed at 200MHz.
In this case the minimum clock rate that could be used for peripheral clock
is 200MHz/16 = 12,5Mhz.
Generic clock rate vary depending on its clock source and dividers.
Lowest clock source of generic clock could be 32Khz, highest clock source
for generic clock could be 600Mhz (on the platform that I'm using).

Implementing a clock driver for this as you pointed it would involve
breaking the timer's driver in 2 parts: one
related to clock selection, one related to timer's functionalities.

Since, I'm thinking, the right place to put this driver is at91 clock tree
(drivers/clk/at91/) I should take a syscon to PIT64B in there so that to be
able to set the proper PIT64B's bits for MUX selection and divider.
Breaking this in multiple blocks will complicate a bit the things (e.g. I
would need to select 2 config flags for PIT64B block).

In the DT bindings I should anyway need to have a phandle to the peripheral
clock and one related to the MUX.

pmc: pmc@fffffc00 {
	compatible = "microchip,sam9x60-pmc", "syscon";
	reg = <0xfffffc00 0x200>;
	interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
	#clock-cells = <2>;
	clocks = <&clk32k 1>, <&clk32k 0>, <&main_xtal>;
	clock-names = "td_slck", "md_slck", "main_xtal";
};

pit64b: timer@f0028000 {
	compatible = "microchip,sam9x60-pit64b";
	reg = <0xf0028000 0x100>;
	interrupts = <37 IRQ_TYPE_LEVEL_HIGH 7>;
	clocks = <&pmc PMC_TYPE_PERIPHERAL 37>, <&pmc PMC_TYPE_MUX>;
	clock-names = "pclk", "mux";
};

One aspect here is that PCLK should be enabled all the time and the mux
should only select b/w PCLK and GCLK. And the mux is not actually part of
the PMC.

One other simpler thing that I can do is to assign GCLK rate directly from
device tree with "assigned-clock-rates" property and go all the time
directly with GCLK (assuming PCLK rate/GCLK rate ratio is all the time the
good one (it should be at least 3 for this IP)) and in this driver to
select all the time the GCLK clock.

Thank you,
Claudiu Beznea


[...]

> 
> So the first invocation of this init function is supposed to init the clock
> event device and the second one inits the clock source. And both allocate
> common data. How is that common?
> 
> Thanks,
> 
> 	tglx
> 
>