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

[Stephen Warren]

MASK_BITS_31_30On 10/15/2013 04:54 PM, Tom Warren wrote:
> Some T114 peripherals can take up to 8 different clock
> sources (parents), including 4 new ones that don't exist
> on previous chips (PLLC2/C3/MEM2/SRC2). Expand clock/pll
> code/tables to support these additional bits/sources.

I would really like Peter De Schrijver to review this patch, since he
wrote the upstream Tegra124 clock driver, which involved a lot of driver
unification with previous SoCs. I'd like him to take a look at the mux
mask widths in particular, w.r.t. making sure that U-Boot, the kernel,
and the TRM all agree on which peripherals have which size mux field.

In particular, clk-tegra-periph.c in Peter's patches contains clocks
with mux fields in bits 31:30 or bits 31:29; there is no clock with a
mux field in bits 31:28. Yet, OUT_CLK_SOURCE4_* in U-Boot (before this
patch) represent a mux field in bits 31:28. If this is wrong, I believe
we need to fix it before applying this patch. If the TRM is wrong, we
need to file a bug agaist it.

> Changes were made to some common code. Testing on T30/T20
> showed no changes in periph clock sources/divisors.
> 
> Also, peripheral clock sources that no longer exist on T114
> were removed from the clock_periph_type table (CVE, TVDAC, etc.),
> and periphs that are gone or not needed in early init are
> no longer brought out of reset/enabled (FUSE, IRAMA/B/C/D, etc.).

As I mentioned in the response I just sent to V1, removing things seems
like it should be in a separate patch, so each patch does just one
logical thing.

> diff --git a/arch/arm/cpu/tegra-common/clock.c b/arch/arm/cpu/tegra-common/clock.c
> index 268fb91..62a2191 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) {

mux_bits is a parameter to this function. I don't see this patch
changing the way this function is called, so I have to assume that the
same values are passed to the function before and after this patch.
Based on the switch statement that's added below, I assume that the
defines MASK_BITS_* are passed into this function as mux_bits.

In that case, the "4" in that if expression means MASK_BITS_29_28.
However, OUT_CLK_SOURCE4_MASK/SHIFT means a mask in bits 31:28. Perhaps
the "4" wasn't originally meant to indicate "number of bits in mux
field", but rather "number of possible values representable in the mux
field"?

While this may be a pre-existing issue, I believe it is imperative that
we fix this before confusing the matter further by building more patches
on top of it.

> -		clrsetbits_le32(reg, OUT_CLK_SOURCE4_MASK,
> -			source << OUT_CLK_SOURCE4_SHIFT);
> -	} else {
> +
> +	switch (mux_bits) {
> +	case MASK_BITS_31_30:
>  		clrsetbits_le32(reg, OUT_CLK_SOURCE_MASK,
>  			source << OUT_CLK_SOURCE_SHIFT);

OUT_CLK_SOURCE_* do indeed represent a mask/shift for bits 31:30, so
that's probably OK.

> +		break;
> +
> +	case MASK_BITS_31_29:
> +		clrsetbits_le32(reg, OUT_CLK_SOURCE3_MASK,
> +			source << OUT_CLK_SOURCE3_SHIFT);

OUT_CLK_SOURCE3_* do indeed represent a mask/shift for bits 31:30, so
that's probably OK.

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

OUT_CLK_SOURCE4_* do NOT represent a mask/shift for bits 29:28, but
rather for bits 31:28. Again, I think the meaning, value, and name of
MASK_BITS_29_28 and OUT_CLK_SOURCE4_* need to be fixed and made
consistent prior to this patch.

I would also suggest making separate patches for the following so
they're all much simpler:

* Removing clock definitions.

* Removing reset twiddling for some clocks.

* The assert fix.

* Updating adjust_periph_pll() to support different mux mask location/size.

* Adding new clocks that rely on the the new mux mask location/size. Or,
perhaps just adding new clocks, period.