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.