Re: [PATCH 3/3] clk: tegra: Properly setup PWM clock on Tegra30

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3317 bytes --]

On Wed, Oct 30, 2013 at 03:38:23PM -0600, Stephen Warren wrote:
> On 10/30/2013 03:00 PM, Thierry Reding wrote:
> > On Tue, Oct 29, 2013 at 01:45:38PM -0600, Stephen Warren wrote:
> >> On 10/29/2013 09:51 AM, Thierry Reding wrote:
> >>> The clock for the PWM controller is slightly different from
> >>> other peripheral clocks on Tegra30. The clock source mux field
> >>> start at bit position 28 rather than 30.
> 
> >>> diff --git a/drivers/clk/tegra/clk-tegra30.c
> >>> b/drivers/clk/tegra/clk-tegra30.c
> >> 
> >>> @@ -836,7 +837,6 @@ static struct tegra_clk
> >>> tegra30_clks[tegra_clk_max] __initdata = { [tegra_clk_extern1]
> >>> = { .dt_id = TEGRA30_CLK_EXTERN1, .present = true }, 
> >>> [tegra_clk_extern2] = { .dt_id = TEGRA30_CLK_EXTERN2, .present
> >>> = true }, [tegra_clk_extern3] = { .dt_id = TEGRA30_CLK_EXTERN3,
> >>> .present = true }, -	[tegra_clk_pwm] = { .dt_id =
> >>> TEGRA30_CLK_PWM, .present = true },
> >> 
> >> I think you still need an entry in this table; isn't it used by
> >> the DT->internal clock ID translation function?
> > 
> > As far as I can tell, this is used by the generic code to
> > determine which of the generic clocks to register. If
> > TEGRA30_CLK_PWM is kept within this list, tegra_periph_clk_init()
> > will register the clock a second time.
> > 
> >> Either way, it seems like this patch might want to add a 
> >> tegra_clk_pwm_tegra30 so that the common C files can still
> >> implement this clock, just with different parameters?
> > 
> > That's pretty much what this patch does, isn't it? It adds a
> > custom entry for the PWM clock to the Tegra30-specific
> > tegra_periph_clk_list and keeps the common one from being
> > registered by dropping the entry from tegra30_clks.
> 
> I was hoping for the new clock that gets added to be added to the
> common file that defines the common clocks rather than to be added to
> the Tegra30-specific files. However, I suppose if it really is a
> Tegra30-specific clock, then keeping it isolated to the
> Tegra30-specific file might make sense.
> 
> That said, I wonder if we can't keep the PWM clock definition in the
> common file, and just make it a bit parameterized? Perhaps that
> defeats the purpose of a common definition though.
> 
> > The same is already done on Tegra20, where the PWM clock is
> > similarly weird.
> 
> OK, perhaps that's reasonable precedent for this then.
> 
> > On Tegra114 and Tegra124 the clock is still weird, but it can be 
> > tweaked into behaving more commonly by lying about the actual
> > position and size of the mux field.
> 
> That doesn't sound good... What if someone actually wants to use the
> correct position/size (e.g. use mux options that presumably can't be
> selected now we've lied)? We really shouldn't lie about things.

Perhaps I've misinterpreted the code. At least comparing to the register
definitions the field should still be 3 bits wide, but the code clamps
it to the upper 2, which makes it compatible with the regular peripheral
clocks. The mux options that are left out are apparently very uncommonly
used (PLLC2 and PLLC3).

I suppose it's always possible that the register definition is wrong or
there was perhaps another good reason to do it this way? Maybe Peter
knows.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]