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

[Stephen Warren]

On 10/15/2013 04:42 PM, Tom Warren wrote:
> On Tue, Oct 8, 2013 at 2:08 PM, Stephen Warren <swarren at wwwdotorg.org
> <mailto:swarren at 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.