Patchwork [1/4] ARM: tegra30: clocks: Fix pciex clock registration

login
register
mail settings
Submitter Jay Agarwal
Date May 8, 2013, 10:57 a.m.
Message ID <1368010660-31465-1-git-send-email-jagarwal@nvidia.com>
Download mbox | patch
Permalink /patch/242550/
State Not Applicable
Headers show

Comments

Jay Agarwal - May 8, 2013, 10:57 a.m.
Registering pciex as peripheral clock instead of fixed clock
as tegra_perih_reset_assert(deassert) api of this clock api 
gives warning and ultimately does not succeed to assert(deassert).
Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
and should be applied on top of this.

Signed-off-by: Jay Agarwal <jagarwal@nvidia.com>
---
 drivers/clk/tegra/clk-tegra30.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
Stephen Warren - May 8, 2013, 4:36 p.m.
On 05/08/2013 04:57 AM, Jay Agarwal wrote:
> Registering pciex as peripheral clock instead of fixed clock
> as tegra_perih_reset_assert(deassert) api of this clock api 
> gives warning and ultimately does not succeed to assert(deassert).

A few procedural comments: I believe this is at least the second version
of these patches that have been posted. As such, the email subject
should say e.e. "PATCH V2" not just "PATCH". You can pass
--subject-prefix='PATCH V2' to git format-patch to achieve this.

Since this is an updated version of the patches, each patch should
describe what changed in the patch, so that people know what issues
you've fixed in this version. Most people believe that the description
of changes should be below the --- line, so that it doesn't get included
in the commit description when the patch is applied.

> Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
> and should be applied on top of this.

Those two lines should be below the --- line so that they don't get
included in the commit description when the patch is applied. git
metadata will provide clues re: who applied the patch and to which
branch, so there's no need to duplicate this information in the commit
description itself, but rather include it below --- so that it's still
present and people can see it.

Some comments on the patch and series below...

> diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c

> +	/* pciex */
> +	clk = tegra_clk_register_periph_gate("pciex", "pll_e", 0, clk_base, 0,
> +				    74, &periph_u_regs, periph_clk_enb_refcnt);
> +	clk_register_clkdev(clk, "pciex", NULL);
> +	clks[pciex] = clk;

Hmmm. This change perhaps isn't technically correct, but is the best we
can do for now, so it's fine. It's also consistent with how the Tegra20
clock driver is written.

This clock has a "reset" bit in the CLK_RST_* registers, but not an
"enable" bit in the CLK_ENB_* registers. Hence, representing this as a
gated clock isn't correct, since there is not gate control in HW. The
correct way to handle this would be to implement the new reset
controller API for Tegra, and expose the reset control only, and not
touch the clock registration at all. However, we do this exact same
thing for a number of Tegra clocks right now, hence I think this change
is fine for now; we'll clean this up, along with some other instances of
the same issue, once the reset API is implemented for Tegra. Of course,
if that happens before the PCI code is checked in, then my opinion will
change.

> @@ -1930,7 +1931,6 @@ static struct tegra_clk_duplicate tegra_clk_duplicates[] = {
>  	TEGRA_CLK_DUPLICATE(bsea, "nvavp", "bsea"),
>  	TEGRA_CLK_DUPLICATE(cml1, "tegra_sata_cml", NULL),
>  	TEGRA_CLK_DUPLICATE(cml0, "tegra_pcie", "cml"),
> -	TEGRA_CLK_DUPLICATE(pciex, "tegra_pcie", "pciex"),

That seems like an unrelated change, and isn't mentioned in the commit
description. Is the change strictly necessary? Is it cleanup that can
happen as a separate patch later in the series? Aren't you able to
remove the CLK_DUPLICATE() entries for other PCIe-related clocks too?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index ba6f51b..6a80b40 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -1592,6 +1592,12 @@  static void __init tegra30_periph_clk_init(void)
 	clk_register_clkdev(clk, "afi", "tegra-pcie");
 	clks[afi] = clk;
 
+	/* pciex */
+	clk = tegra_clk_register_periph_gate("pciex", "pll_e", 0, clk_base, 0,
+				    74, &periph_u_regs, periph_clk_enb_refcnt);
+	clk_register_clkdev(clk, "pciex", NULL);
+	clks[pciex] = clk;
+
 	/* kfuse */
 	clk = tegra_clk_register_periph_gate("kfuse", "clk_m",
 				    TEGRA_PERIPH_ON_APB,
@@ -1710,11 +1716,6 @@  static void __init tegra30_fixed_clk_init(void)
 				1, 0, &cml_lock);
 	clk_register_clkdev(clk, "cml1", NULL);
 	clks[cml1] = clk;
-
-	/* pciex */
-	clk = clk_register_fixed_rate(NULL, "pciex", "pll_e", 0, 100000000);
-	clk_register_clkdev(clk, "pciex", NULL);
-	clks[pciex] = clk;
 }
 
 static void __init tegra30_osc_clk_init(void)
@@ -1930,7 +1931,6 @@  static struct tegra_clk_duplicate tegra_clk_duplicates[] = {
 	TEGRA_CLK_DUPLICATE(bsea, "nvavp", "bsea"),
 	TEGRA_CLK_DUPLICATE(cml1, "tegra_sata_cml", NULL),
 	TEGRA_CLK_DUPLICATE(cml0, "tegra_pcie", "cml"),
-	TEGRA_CLK_DUPLICATE(pciex, "tegra_pcie", "pciex"),
 	TEGRA_CLK_DUPLICATE(vcp, "nvavp", "vcp"),
 	TEGRA_CLK_DUPLICATE(clk_max, NULL, NULL), /* MUST be the last entry */
 };