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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
