On 10/15/2013 04:42 PM, Tom Warren wrote:
On Tue, Oct 8, 2013 at 2:08 PM, Stephen Warren <swarren@wwwdotorg.org mailto:swarren@wwwdotorg.org> wrote:
> 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.I didn't do the original patch (this code has been in there for awhile - I'm only adding the new clock sources table/periph clocks for T114+), so I can't say why this naming was chosen. Perhaps you can run it down upstream (or in our internal T30 repo) using git-blame and query the original author (Jimmy, Allen, etc.).
That's the job of the patch submitter, not the reviewer. It'd be best if the original author upstreamed the patch; everyone needs to get fully involved in upstreaming, rather than relying on others to do their work for them.
Regardless, as far as I can tell, only CLK_SOURCE_PWM uses bits 29:28, and may be a typo in the T30 TRM. T114+ have changed it to bits 31:29. All other periphs use 2-bit (31:30) and 3-bit (31:29) CLK_SRC fields (4 or 8 possible sources) in my searches of the Tegra30/114/124 TRMs. We've never set a clock source for PWM in any version of U-Boot AFAICT.
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.No new macros here, just using the MASK bits in a switch statement,
The patch adds the following, which doesn't look like it's named consistently:
+#define OUT_CLK_SOURCE3_SHIFT 29 +#define OUT_CLK_SOURCE3_MASK (7U << OUT_CLK_SOURCE3_SHIFT)
because the 31:29 case wasn't covered. I'm loath to change it now since this is only supposed to be a patch to add additional clock source tables that came into being w/T114, and this is common code. I'll leave it as-is and if you want to submit a cleanup patch later, I'll Ack it.
> 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.The precedent in this file seemed to be to use a hex number suffix for 'empty' slots, so I replaced PERIPHC_NONE with PERIPHC_05h to be consistent. Since I was removing clocks/regs that had been removed in the T114 TRM (see below), this seemed a good time for it. I can add a note about it in the commit msg.
The patch description talks about adding support for more clock sources, not fixing bugs with existing clock sources. Those two things sound like they should be different patches; one fix/cleanup, one new feature. Then, each patch would be simpler.
Anyway, I'll take another look at V2 and put more detailed comments there.