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