Re: [PATCH V4 5/7] clk: bcm2835: add missing 22 HW-clocks.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On 08.02.2016, at 12:12, Martin Sperl <kernel@xxxxxxxxxxxxxxxx> wrote:
> 
> 
> 
> On 02.02.2016 02:51, Eric Anholt wrote:
>> kernel@xxxxxxxxxxxxxxxx writes:
>> 
>>> From: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>
>>> 
>>> There were 22 HW clocks missing from the clock driver.
>>> 
>>> These have been included and int_bits and frac_bits
>>> have been set correctly based on information extracted
>>> from the broadcom videocore headers
>>> (http://www.broadcom.com/docs/support/videocore/Brcm_Android_ICS_Graphics_Stack.tar.gz)
>>> 
>>> For an extracted view of the registers please see:
>>> https://github.com/msperl/rpi-registers/blob/master/md/Region_CM.md
>>> 
>>> bcm2835_clock_per_parents has been assigned as the parent
>>> clock for all new clocks, but this may not be correct
>>> in all cases - documentation on this is not publicly
>>> available, so some modifications may be needed in the
>>> future.
>> 
>> We need the parents to be correct if we're going to land the patch.
>> I'll try to update them.
>> 
>> I'm not a fan of this "let's just shove everything we can find in some
>> header file into the .c and hope for the best."  Most of these clocks
>> were left out intentionally.
> 
> Well - I wanted to get them in just in case we need them later.
> 
> If you have got access to documentation which states the correct
> parent mux, then please share them so that we can implement them
> correctly.
> 
> Also listing all allows us then to expose the values of the registers
> via debugfs in case we need it - see separate RFC-patch 9 -  where
> we expose the raw register values as well.
> 
>>> +static const struct bcm2835_clock_data bcm2835_clock_aveo_data = {
>>> +	.name = "aveo",
>>> +	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),
>>> +	.parents = bcm2835_clock_per_parents,
>>> +	.ctl_reg = CM_AVEOCTL,
>>> +	.div_reg = CM_AVEODIV,
>>> +	.int_bits = 4,
>>> +	.frac_bits = 12,
>>> +};
>> 
>> AVEO has 0 fractional bits
> 
> Correct - my issue (copy paste - I assume).
> 
>> 
>>> +static const struct bcm2835_clock_data bcm2835_clock_ccp2_data = {
>>> +	/* this is possibly a gate */
>>> +	.name = "ccp2",
>>> +	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),
>>> +	.parents = bcm2835_clock_per_parents,
>>> +	.ctl_reg = CM_CCP2CTL,
>>> +	.div_reg = CM_CCP2DIV,
>>> +	.int_bits = 1,
>>> +	.frac_bits = 0,
>>> +};
>> 
>> CCP2 is a gate from a different clock source, so this won't work.
> See comment above: please provide parent clock mux.
> See also my comment about 3 clock that may be gates - this applies.
> 
>> 
>>> +static const struct bcm2835_clock_data bcm2835_clock_dsi0p_data = {
>>> +	/* this is possibly a gate */
>>> +	.name = "dsi0p",
>>> +	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),
>>> +	.parents = bcm2835_clock_per_parents,
>>> +	.ctl_reg = CM_DSI0PCTL,
>>> +	.div_reg = CM_DSI0PDIV,
>>> +	.int_bits = 1,
>>> +	.frac_bits = 0,
>>> +};
>>> +
>>> +static const struct bcm2835_clock_data bcm2835_clock_dsi1p_data = {
>>> +	/* this is possibly a gate */
>>> +	.name = "dsi1p",
>>> +	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),
>>> +	.parents = bcm2835_clock_per_parents,
>>> +	.ctl_reg = CM_DSI1PCTL,
>>> +	.div_reg = CM_DSI1PDIV,
>>> +	.int_bits = 1,
>>> +	.frac_bits = 0,
>>> +};
>> 
>> DSI0/1 pixel clocks take different clock sources and are gates off of
>> them, so these definitions don't work.
> See comment above: please provide parent clock.
> See also my comment about 3 clock that may be gates.
> 
>> 
>>> +
>>> +static const struct bcm2835_clock_data bcm2835_clock_gnric_data = {
>>> +	.name = "gnric",
>>> +	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),
>>> +	.parents = bcm2835_clock_per_parents,
>>> +	.ctl_reg = CM_GNRICCTL,
>>> +	.div_reg = CM_GNRICDIV,
>>> +	.int_bits = 12,
>>> +	.frac_bits = 12,
>>> +};
>> 
>> GNRIC isn't an actual clock, it's just what's used for describing the
>> overall structure of clocks.
> 
> Well - there is the corresponding register at 0x7e101000 which reads as:
> ctl = 0x0000636d
> div = 0x0000636d
> 
> So we could remove this one if this is really the case of a dummy -
> even though I wonder why there would be space in io-space reserved for
> this "description" only.
> 
>> 
>>> +static const struct bcm2835_clock_data bcm2835_clock_peria_data = {
>>> +	.name = "peria",
>>> +	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),
>>> +	.parents = bcm2835_clock_per_parents,
>>> +	.ctl_reg = CM_PERIACTL,
>>> +	.div_reg = CM_PERIADIV,
>>> +	.int_bits = 12,
>>> +	.frac_bits = 12,
>>> +};
>> 
>> This register doesn't do anything, because the debug bit in the power
>> manager is not set.  We don't think we should expose a clock gate if it
>> doesn't work, I think.
> maybe we allow setting debug in the PM at a later point in time?
> 
>> 
>>> +static const struct bcm2835_clock_data bcm2835_clock_pulse_data = {
>>> +	.name = "pulse",
>>> +	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),
>>> +	.parents = bcm2835_clock_per_parents,
>>> +	.ctl_reg = CM_PULSECTL,
>>> +	.div_reg = CM_PULSEDIV,
>>> +	.int_bits = 12,
>>> +	.frac_bits = 0,
>>> +};
>> 
>> There's some other divider involved in this clock, won't give correct results.
> See comment above: please provide parent clock.
> 
>> 
>>> +static const struct bcm2835_clock_data bcm2835_clock_td0_data = {
>>> +	.name = "td0",
>>> +	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),
>>> +	.parents = bcm2835_clock_per_parents,
>>> +	.ctl_reg = CM_TD0CTL,
>>> +	.div_reg = CM_TD0DIV,
>>> +	.int_bits = 12,
>>> +	.frac_bits = 12,
>>> +};
>>> +
>>> +static const struct bcm2835_clock_data bcm2835_clock_td1_data = {
>>> +	.name = "td1",
>>> +	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),
>>> +	.parents = bcm2835_clock_per_parents,
>>> +	.ctl_reg = CM_TD1CTL,
>>> +	.div_reg = CM_TD1DIV,
>>> +	.int_bits = 12,
>>> +	.frac_bits = 12,
>>> +};
>> 
>> These are some other clock generator, not the generic or mash ones used
>> elsewhere.  I wouldn't enable them without testing.
> 
> As long as they are not referenced in the DT these only are read only
> and can get read via debugfs - these are disabled anyway:
> 
> root@raspcm:/build/linux# head /sys/kernel/debug/clk/td*/clk_rate
> ==> /sys/kernel/debug/clk/td0/clk_rate <==
> 0
> 
> ==> /sys/kernel/debug/clk/td1/clk_rate <==
> 0
> 
>> 
>>> +static const struct bcm2835_clock_data bcm2835_clock_tec_data = {
>>> +	.name = "tec",
>>> +	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),
>>> +	.parents = bcm2835_clock_per_parents,
>>> +	.ctl_reg = CM_TECCTL,
>>> +	.div_reg = CM_TECDIV,
>>> +	.int_bits = 6,
>>> +	.frac_bits = 0,
>>> +};
>> 
>> TEC should be osc parents.
> I will change that.
> 
>> 
>>> -/*
>>> - * CM_PERIICTL (and CM_PERIACTL, CM_SYSCTL and CM_VPUCTL if
>>> - * you have the debug bit set in the power manager, which we
>>> - * don't bother exposing) are individual gates off of the
>>> - * non-stop vpu clock.
>>> - */
>>>  static const struct bcm2835_gate_data bcm2835_clock_peri_image_data = {
>>>  	.name = "peri_image",
>>>  	.parent = "vpu",
>>>  	.ctl_reg = CM_PERIICTL,
>>>  };
>>> 
>>> +static const struct bcm2835_gate_data bcm2835_clock_sys_data = {
>>> +	.name = "sys",
>>> +	.parent = "vpu",
>>> +	.ctl_reg = CM_SYSCTL,
>>> +};
>> 
>> same concern as peria.
> maybe we allow setting debug in the PM at a later point in time?
> 
> Please propose how to continue to get the clocks in.
> 
> If you want some clocks left out, then that is fine and we
> can accommodate that and just leave the defines in the bindings
> header for later use...
> 
> Just list them.
> 


As I am trying to complete the list of clocks here my “best” summary
so far of the clock tree for the bcm2835 taken from various sources:
* Datasheet
* broadcom provided VC4 headers (via https://github.com/msperl/rpi-registers)
* mailing list

And I have come up with the following description that
you can put in graphviz or http://www.webgraphviz.com/

--- cut here ---
digraph clocks {
	graph [ordering="out"];

	subgraph cluster_osc {
		label = "Oscillators";

                "GND";
                "OSC";
                "testdebug0";
                "testdebug1";
	}

	subgraph cluster_pll {
		label = "PLLs";

		subgraph cluster_plla {
			label = "PLLA";

			OSC -> PLLA;

			PLLA -> PLLA_core;
			PLLA -> PLLA_per;
			PLLA -> PLLA_dsi0;
			PLLA -> PLLA_ccp2;
		}

		subgraph cluster_pllb {
			label = "PLLB";

			OSC -> PLLB;

			PLLB -> PLLB_ARM;
			PLLB -> PLLB_SP0;
			PLLB -> PLLB_SP1;
			PLLB -> PLLB_SP2;
		}

		subgraph cluster_pllc {
			label = "PLLC";

			OSC -> PLLC;

			PLLC -> PLLC_core0;
			PLLC -> PLLC_core1;
			PLLC -> PLLC_core2;
			PLLC -> PLLC_per;
		}

		subgraph cluster_plld {
			label = "PLLD";

			OSC -> PLLD;

			PLLD -> PLLD_core;
			PLLD -> PLLD_per;
			PLLD -> PLLD_dsi0;
			PLLD -> PLLD_dsi1;
		}

		subgraph cluster_pllh {
			label = "PLLH";

			OSC -> PLLH;

			PLLH -> PLLH_aux;
			PLLH -> PLLH_pix;
			PLLH -> PLLH_rcal;
		}
	}

	subgraph cluster_mux {
		label = "clocks";

		subgraph cluster_vpu_clocks {
			label = "VPU-clocks";

			subgraph cluster_vpu_mux {
				label = "VPU-mux";

				vGND        [label="0: GND"];
				vOSC        [label="1: OSC"];
				vtestdebug0 [label="2: testdebug0"];
				vtestdebug1 [label="3: testdebug1"];
				vPLLA_core  [label="4: PLLA_core"];
				vPLLC_core0 [label="5: PLLC_core0"];
				vPLLD_core  [label="6: PLLD_core"];
				vPLLH_aux   [label="7: PLLH_aux"];
				vPLLC_core1 [label="8: PLLC_core1"];
				vPLLC_core2 [label="9: PLLC_core2"];

				GND        -> vGND        -> vpu_mux;
				OSC        -> vOSC        -> vpu_mux;
				testdebug0 -> vtestdebug0 -> vpu_mux;
				testdebug1 -> vtestdebug1 -> vpu_mux;
				PLLA_core  -> vPLLA_core  -> vpu_mux;
				PLLC_core0 -> vPLLC_core1 -> vpu_mux;
				PLLD_core  -> vPLLD_core  -> vpu_mux;
				PLLH_aux   -> vPLLH_aux   -> vpu_mux;
				PLLC_core1 -> vPLLC_core1 -> vpu_mux;
				PLLC_core2 -> vPLLC_core2 -> vpu_mux;
			}

			vpu_mux -> vpu;
			vpu_mux -> v3d;
			vpu_mux -> isp;
			vpu_mux -> h264;
			vpu_mux -> sdram;
		}

		subgraph cluster_per_clocks {
			label = "Periperial-clocks";

			subgraph cluster_per_mux {
				label = "Periperal-mux";

				pGND        [label="0: GND"];
				pOSC        [label="1: OSC"];
				ptestdebug0 [label="2: testdebug0"];
				ptestdebug1 [label="3: testdebug1"];
				pPLLA_per   [label="4: PLLA_per"];
				pPLLC_per   [label="5: PLLC_per"];
				pPLLD_per   [label="5: PLLD_per"];
				pPLLH_aux   [label="5: PLLH_aux"];

				GND        -> pGND        -> per_mux;
				OSC        -> pOSC        -> per_mux;
				testdebug0 -> ptestdebug0 -> per_mux;
				testdebug1 -> ptestdebug1 -> per_mux;
				PLLA_per   -> pPLLA_per   -> per_mux;
				PLLC_per   -> pPLLC_per   -> per_mux;
				PLLD_per   -> pPLLD_per   -> per_mux;
				PLLH_aux   -> pPLLH_aux   -> per_mux;
			}

			per_mux -> vec;
			per_mux -> uart;
			per_mux -> hsm;
			per_mux -> emmc;
			per_mux -> pwm;
			per_mux -> pcm;
			per_mux -> aveo;
			per_mux -> cam0;
			per_mux -> cam1;
			per_mux -> dft;
			per_mux -> dpi;
			per_mux -> dsi0e;
			per_mux -> dsi1e;
			per_mux -> gp0;
			per_mux -> gp1;
			per_mux -> gp2;
			per_mux -> slim;
			per_mux -> smi;
		}

		subgraph cluster_osc_clocks {
			label = "osc-clocks";

			subgraph cluster_osc_mux {
				label = "osc-mux";

				oGND        [label="0: GND"];
				oOSC        [label="1: OSC"];
				otestdebug0 [label="2: testdebug0"];
				otestdebug1 [label="3: testdebug1"];

				GND        -> oGND        -> osc_mux;
				OSC        -> oOSC        -> osc_mux;
				testdebug0 -> otestdebug0 -> osc_mux;
				testdebug1 -> otestdebug1 -> osc_mux;
			}

			osc_mux -> tsens;
			osc_mux -> tec;
			osc_mux -> otp;
		}

		subgraph cluster_unknown_mux_clocks {
			label = "unknown-parent-mux-clocks";

			ukn_mux -> ccp2;
			ukn_mux -> dsi0pix;
			ukn_mux -> dsi1pix;
			ukn_mux -> pulse;
			ukn_mux -> td0;
			ukn_mux -> td1;
		}

		subgraph cluster_debug_clocks {
			label = "debug-clocks";

			debug_mux -> peria;
			debug_mux -> sys;
		}
	}

	subgraph cluster_aux {
		label = “auxiliar-clocks";

		vpu -> spi1;
		vpu -> spi2;
		vpu -> uart1;
	}
}
--- cut here ---

I have no idea where we could put this information
in the kernel tree - maybe this would help.

>From this you can see that we have:
* PLL that is not configured now:
  * PLLB
* a few more PLL-dividers that are not configured:
  * PLLB_ARM
  * PLLB_SP0
  * PLLB_SP1
  * PLLB_SP2
  * PLLD_dsi0
  * PLLD_dsi1
  * PLLH_rcal
* a few clocks which Eric has said are wrong and where
  the corresponding parents need to get found:
  * ccp2 (gate)
  * dsi0pix (gate - maybe PLLD_dis0?)
  * dis1pix (gate - maybe PLLD_dis1)
  * pulse (gate)
  * td0
  * td1
* debug clocks that require a running debug power domain:
  * peria
  * sys
* missing clocks in the current driver:
  * aveo
  * cam0
  * cam1
  * ccp2 (see note above)
  * dtf
  * dpi
  * dsi0e   (maybe this also uses a different parent clock mux?)
  * dsi0pix (maybe this also uses a different parent clock mux?)
  * dsi1e   (maybe this also uses a different parent clock mux?)
  * dsi1pix (maybe this also uses a different parent clock mux?)
  * gp0
  * gp1
  * gp2
  * peria (see note above)
  * pulse (see note above)
  * slim
  * smi
  * td0 (see note above)
  * td1 (see note above)
  * td2 (see note above)
  * tec
  * sys (see note above)
* generic clock that is supposedly not in use

I can create a new patch that will include all those
missing clocks that are undisputed in their settings
(hence without any comments/notes) - I hope this is acceptable.

If someone has any ideas about those clocks where we 
miss the correct parents/mux, then please add your
wisdom.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux