Patchwork [v2,11/27] clk: mvebu: create parent-child relation for PCIe clocks on Armada 370

login
register
mail settings
Submitter Thomas Petazzoni
Date Jan. 28, 2013, 6:56 p.m.
Message ID <1359399397-29729-12-git-send-email-thomas.petazzoni@free-electrons.com>
Download mbox | patch
Permalink /patch/216313/
State Not Applicable
Headers show

Comments

Thomas Petazzoni - Jan. 28, 2013, 6:56 p.m.
The Armada 370 has two gatable clocks for each PCIe interface, and we
want both of them to be enabled. We therefore make one of the two
clocks a child of the other, as we did for the sataX and sataXlnk
clocks on Armada XP.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Mike Turquette <mturquette@linaro.org>
---
 drivers/clk/mvebu/clk-gating-ctrl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Stephen Warren - Jan. 28, 2013, 10:08 p.m.
On 01/28/2013 11:56 AM, Thomas Petazzoni wrote:
> The Armada 370 has two gatable clocks for each PCIe interface, and we
> want both of them to be enabled. We therefore make one of the two
> clocks a child of the other, as we did for the sataX and sataXlnk
> clocks on Armada XP.

> diff --git a/drivers/clk/mvebu/clk-gating-ctrl.c b/drivers/clk/mvebu/clk-gating-ctrl.c

> @@ -119,8 +119,8 @@ static const struct mvebu_soc_descr __initconst armada_370_gating_descr[] = {
>  	{ "pex1_en", NULL,  2 },
>  	{ "ge1", NULL, 3 },
>  	{ "ge0", NULL, 4 },
> -	{ "pex0", NULL, 5 },
> -	{ "pex1", NULL, 9 },
> +	{ "pex0", "pex0_en", 5 },
> +	{ "pex1", "pex1_en", 9 },

I must admit, I know nothing about struct mvebu_soc_descr, but I'm
having a hard time seeing how that code change makes one of those clock
a parent of the other, since the pex0 entry doesn't reference anything
"pex1"-related, nor vice-versa. Is more explanation in the commit
message warranted here?
--
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
Thomas Petazzoni - Jan. 28, 2013, 10:21 p.m.
Dear Stephen Warren,

On Mon, 28 Jan 2013 15:08:46 -0700, Stephen Warren wrote:

> I must admit, I know nothing about struct mvebu_soc_descr, but I'm
> having a hard time seeing how that code change makes one of those clock
> a parent of the other, since the pex0 entry doesn't reference anything
> "pex1"-related, nor vice-versa. Is more explanation in the commit
> message warranted here?

See the definition of mvebu_soc_descr:

struct mvebu_soc_descr {
        const char *name;
        const char *parent;
        int bit_idx;
};

It simply registers the pex0 clock with the pex0_en clock as its
parents. Those clocks are normal gatable clocks, registered with
clk_register_gate(). This ensures that whenever the pex0 clock is
enabled, its parent clock pex0_en gets enabled as well. We do the same
for SATA clocks on Armada XP, for example:

static const struct mvebu_soc_descr __initconst
armada_xp_gating_descr[] = { { "audio", NULL, 0 },
[...]
        { "sata0lnk", NULL, 14 },
        { "sata0", "sata0lnk", 15 },
[...]
        { "sata1lnk", NULL, 29 },
        { "sata1", "sata1lnk", 30 },
        { }
};

Best regards,

Thomas
Stephen Warren - Jan. 28, 2013, 10:27 p.m.
On 01/28/2013 03:21 PM, Thomas Petazzoni wrote:
> Dear Stephen Warren,
> 
> On Mon, 28 Jan 2013 15:08:46 -0700, Stephen Warren wrote:
> 
>> I must admit, I know nothing about struct mvebu_soc_descr, but I'm
>> having a hard time seeing how that code change makes one of those clock
>> a parent of the other, since the pex0 entry doesn't reference anything
>> "pex1"-related, nor vice-versa. Is more explanation in the commit
>> message warranted here?
> 
> See the definition of mvebu_soc_descr:
> 
> struct mvebu_soc_descr {
>         const char *name;
>         const char *parent;
>         int bit_idx;
> };
> 
> It simply registers the pex0 clock with the pex0_en clock as its
> parents. Those clocks are normal gatable clocks, registered with
> clk_register_gate(). This ensures that whenever the pex0 clock is
> enabled, its parent clock pex0_en gets enabled as well.

Oh I see; I was confused by the patch description. The two clocks being
made child/parent are the two clocks for a port, and this relationship
is set up for each port; for some reason I thought there was a
requirement to make one port's clock a child of the other port's clock.
--
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
Thomas Petazzoni - Jan. 28, 2013, 10:44 p.m.
Dear Stephen Warren,

On Mon, 28 Jan 2013 15:27:19 -0700, Stephen Warren wrote:

> Oh I see; I was confused by the patch description. The two clocks being
> made child/parent are the two clocks for a port, and this relationship
> is set up for each port; for some reason I thought there was a
> requirement to make one port's clock a child of the other port's clock.

Aah, ok, I understand the confusion now. Re-reading my commit log, I'm
not sure where the confusion comes from, but english is not my native
language, so maybe something that sounds clear to me is not clear in
reality. I'll rephrase the commit log to make sure this confusion is
clarified.

Thanks!

Thomas

Patch

diff --git a/drivers/clk/mvebu/clk-gating-ctrl.c b/drivers/clk/mvebu/clk-gating-ctrl.c
index 8fa5408..fd52b5f 100644
--- a/drivers/clk/mvebu/clk-gating-ctrl.c
+++ b/drivers/clk/mvebu/clk-gating-ctrl.c
@@ -119,8 +119,8 @@  static const struct mvebu_soc_descr __initconst armada_370_gating_descr[] = {
 	{ "pex1_en", NULL,  2 },
 	{ "ge1", NULL, 3 },
 	{ "ge0", NULL, 4 },
-	{ "pex0", NULL, 5 },
-	{ "pex1", NULL, 9 },
+	{ "pex0", "pex0_en", 5 },
+	{ "pex1", "pex1_en", 9 },
 	{ "sata0", NULL, 15 },
 	{ "sdio", NULL, 17 },
 	{ "tdm", NULL, 25 },