[U-Boot] [PATCH] Tegra114: Add support for more clock sources for T1x4 periphs

[Stephen Warren]

On 10/07/2013 01:47 PM, Tom Warren wrote:
> Some T1x4 peripherals can take up to 8 different clock

I would like to avoid "x" in any Tegra naming. Downstream, we've abused
this by calling Tegra114 Tegra11x instead, and I'd like to completely
avoid anything like that abomination upstream. Can we simply say "114"
instead of "1x4" or "114" here. If the "x" really is a wildcard, let's
just write out 114/124 instead, although if such clocks also exist on
Tegra114, there's no need to even mention Tegra124 here, since the
statement is valid even for just Tegra114 alone.

> Change-Id: I396169cd5732ad42aeddefa70fc43f79e969a70d

Upstream doesn't want those.

> diff --git a/arch/arm/cpu/tegra-common/clock.c b/arch/arm/cpu/tegra-common/clock.c
> index 268fb91..c703c40 100644
> --- a/arch/arm/cpu/tegra-common/clock.c
> +++ b/arch/arm/cpu/tegra-common/clock.c
> @@ -304,13 +304,24 @@ static int adjust_periph_pll(enum periph_id periph_id, int source,
>  	/* work out the source clock and set it */
>  	if (source < 0)
>  		return -1;
> -	if (mux_bits == 4) {
> -		clrsetbits_le32(reg, OUT_CLK_SOURCE4_MASK,
> -			source << OUT_CLK_SOURCE4_SHIFT);

That implies 4 bits ...

> +	switch (mux_bits) {

> +	case MASK_BITS_29_28:
> +		clrsetbits_le32(reg, OUT_CLK_SOURCE4_MASK,
> +			source << OUT_CLK_SOURCE4_SHIFT);

... but that implies 2 bits (29, 28). There's some inconsistency in the
naming there.

Can't we use the OUT_CLK_SOURCE4_MASK macros instead of inventing new
MASK_BITS_29_28 macros, or something like that? Or perhaps simply store
the shift and mask values in per-clock data, so there's no need for
conditional code here.

> +int clock_periph_enable(enum periph_id periph_id, enum clock_id parent,
> +		int divisor)

This function doesn't seem to be used anywhere. What's it for?

> +{
> +	int mux_bits, divider_bits, source;
> +
> +	/* Assert reset and enable clock */
> +	reset_set_enable(periph_id, 1);
> +	clock_enable(periph_id);
> +
> +	/* work out the source clock and set it */
> +	source = get_periph_clock_source(periph_id, parent, &mux_bits,
> +					 &divider_bits);
> +
> +	assert(divisor >= 0);
> +	divisor = (divisor - 1) << 1;

Doesn't that assert need to be "> 0" not "> = 0" to avoid underflow in
the "- 1" operation?

> diff --git a/arch/arm/cpu/tegra114-common/clock.c b/arch/arm/cpu/tegra114-common/clock.c

> @@ -122,110 +160,120 @@ static enum clock_type_id clock_periph_type[PERIPHC_COUNT] = {

> -	TYPE(PERIPHC_NONE,	CLOCK_TYPE_NONE),
...
> +	TYPE(PERIPHC_05h,	CLOCK_TYPE_NONE),

Isn't that an unrelated change? At least the need for this should be
mentioned in the commit description.

>  	/* 0x10 */
> -	TYPE(PERIPHC_CVE,	CLOCK_TYPE_PDCT),
...
> +	TYPE(PERIPHC_10h,	CLOCK_TYPE_NONE),

Why remove that clock?

> -	TYPE(PERIPHC_TVDAC,	CLOCK_TYPE_PDCT),
...
> +	TYPE(PERIPHC_25h,	CLOCK_TYPE_NONE),

Same here.

> -	TYPE(PERIPHC_SPEEDO,	CLOCK_TYPE_PCMT),
...
> +	TYPE(PERIPHC_55h,	CLOCK_TYPE_NONE),

And that. etc.