| Message ID | 1368010660-31465-1-git-send-email-jagarwal@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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 */ };
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(-)