[PATCH 0/2] ARM: mvebu: fix PCIe/mvebu-soc-id issue

[PATCH 0/2] ARM: mvebu: fix PCIe/mvebu-soc-id issue

[Thomas Petazzoni]

Hello,

This set of two patches (and more specifically the second patch) fix a
PCIe detection problem caused by the introduction of the mvebu-soc-id
and a bad interaction between the clock manipulation done by the
mvebu-soc-id code and the PCIe driver. The details are all in the
commit log of the second patch.

This second patch fixes a PCIe detection problem noticed by several
persons. The first patch is just a related preparation patch.

Both patches are meant for 3.15-rc, and should be backported to 3.14
as part of the stable process.

Best regards,

Thomas

Thomas Petazzoni (2):
  ARM: mvebu: mvebu-soc-id: add missing clk_put() call
  ARM: mvebu: mvebu-soc-id: keep clock enabled if PCIe unit is enabled

 arch/arm/mach-mvebu/mvebu-soc-id.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
1.9.2

[PATCH 1/2] ARM: mvebu: mvebu-soc-id: add missing clk_put() call

[Thomas Petazzoni]

The mvebu-soc-id code in mach-mvebu/ needs to enable a clock to read
the SoC device ID and revision number. To do so, it does a clk_get(),
then a clk_prepare_enable(), reads the value, and disables the clock
with clk_disable_unprepare(). However, it forgets to clk_put() the
clock. This commit fixes this issue.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: <stable@vger.kernel.org> # 3.14+
Fixes: af8d1c63afcbf36eea06789c92e22d4af118d2fb ('ARM: mvebu: Add support to get the ID and the revision of a SoC')
---
 arch/arm/mach-mvebu/mvebu-soc-id.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c
index f3d4cf5..b52af6f 100644
--- a/arch/arm/mach-mvebu/mvebu-soc-id.c
+++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
@@ -109,6 +109,7 @@ static int __init mvebu_soc_id_init(void)
 
 res_ioremap:
 	clk_disable_unprepare(clk);
+	clk_put(clk);
 
 clk_err:
 	of_node_put(child);
-- 
1.9.2

[PATCH 1/2] ARM: mvebu: mvebu-soc-id: add missing clk_put() call

[Gregory CLEMENT]

Hi Thomas,

On 12/05/2014 16:11, Thomas Petazzoni wrote:
> The mvebu-soc-id code in mach-mvebu/ needs to enable a clock to read
> the SoC device ID and revision number. To do so, it does a clk_get(),
> then a clk_prepare_enable(), reads the value, and disables the clock
> with clk_disable_unprepare(). However, it forgets to clk_put() the
> clock. This commit fixes this issue.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: <stable@vger.kernel.org> # 3.14+
> Fixes: af8d1c63afcbf36eea06789c92e22d4af118d2fb ('ARM: mvebu: Add support to get the ID and the revision of a SoC')

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>


Thanks,

Gregory

> ---
>  arch/arm/mach-mvebu/mvebu-soc-id.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c
> index f3d4cf5..b52af6f 100644
> --- a/arch/arm/mach-mvebu/mvebu-soc-id.c
> +++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
> @@ -109,6 +109,7 @@ static int __init mvebu_soc_id_init(void)
>  
>  res_ioremap:
>  	clk_disable_unprepare(clk);
> +	clk_put(clk);
>  
>  clk_err:
>  	of_node_put(child);
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/2] ARM: mvebu: mvebu-soc-id: keep clock enabled if PCIe unit is enabled

[Thomas Petazzoni]

Since the mvebu-soc-id code in mach-mvebu/ was introduced, several
users have noticed a regression: the PCIe card connected in the first
PCIe interface is not detected properly.

This is due to the fact that the mvebu-soc-id code enables the PCIe
clock of the first PCIe interface, reads the SoC device ID and
revision number (yes this information is made available as part of
PCIe registers), and then disables the clock. However, by doing this,
we gate the clock and therefore loose the complex PCIe configuration
that was done by the bootloader.

Unfortunately, as of today, the kernel is not capable of doing this
complex configuration by itself, so we really need to keep the PCIe
clock enabled. However, we don't want to keep it enabled
unconditionally: if the PCIe interface is not enabled or PCI support
is not compiled into the kernel, there is no reason to keep the PCIe
clock running.

This issue was discussed with Kevin Hilman, and the suggested solution
was to make the mvebu-soc-id code keep the clock enabled in case it
will be needed for PCIe. This is therefore the solution implemented in
this patch.

Long term, we hope to make the kernel more capable in terms of PCIe
configuration for this platform, which will anyway be needed to
support the compilation of the PCIe host controller driver as a
module. In the mean time however, we don't have much other choice than
to implement the currently proposed solution.

Reported-by: Neil Greatorex <neil@fatboyfat.co.uk>
Cc: Neil Greatorex <neil@fatboyfat.co.uk>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: <stable@vger.kernel.org> # 3.14+
Fixes: af8d1c63afcbf36eea06789c92e22d4af118d2fb ('ARM: mvebu: Add support to get the ID and the revision of a SoC')
---
 arch/arm/mach-mvebu/mvebu-soc-id.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c
index b52af6f..09520e1 100644
--- a/arch/arm/mach-mvebu/mvebu-soc-id.c
+++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
@@ -108,8 +108,18 @@ static int __init mvebu_soc_id_init(void)
 	iounmap(pci_base);
 
 res_ioremap:
-	clk_disable_unprepare(clk);
-	clk_put(clk);
+	/*
+	 * If the PCIe unit is actually enabled and we have PCI
+	 * support in the kernel, we intentionally do not release the
+	 * reference to the clock. We want to keep it running since
+	 * the bootloader does some PCIe link configuration that the
+	 * kernel is for now unable to do, and gating the clock would
+	 * make us loose this precious configuration.
+	 */
+	if (!of_device_is_available(child) || !IS_ENABLED(CONFIG_PCI_MVEBU)) {
+		clk_disable_unprepare(clk);
+		clk_put(clk);
+	}
 
 clk_err:
 	of_node_put(child);
-- 
1.9.2

[PATCH 2/2] ARM: mvebu: mvebu-soc-id: keep clock enabled if PCIe unit is enabled

[Gregory CLEMENT]

Hi Thomas,

On 12/05/2014 16:11, Thomas Petazzoni wrote:
> Since the mvebu-soc-id code in mach-mvebu/ was introduced, several
> users have noticed a regression: the PCIe card connected in the first
> PCIe interface is not detected properly.
> 
> This is due to the fact that the mvebu-soc-id code enables the PCIe
> clock of the first PCIe interface, reads the SoC device ID and
> revision number (yes this information is made available as part of
> PCIe registers), and then disables the clock. However, by doing this,
> we gate the clock and therefore loose the complex PCIe configuration
> that was done by the bootloader.
> 
> Unfortunately, as of today, the kernel is not capable of doing this
> complex configuration by itself, so we really need to keep the PCIe
> clock enabled. However, we don't want to keep it enabled
> unconditionally: if the PCIe interface is not enabled or PCI support
> is not compiled into the kernel, there is no reason to keep the PCIe
> clock running.
> 
> This issue was discussed with Kevin Hilman, and the suggested solution
> was to make the mvebu-soc-id code keep the clock enabled in case it
> will be needed for PCIe. This is therefore the solution implemented in
> this patch.
> 
> Long term, we hope to make the kernel more capable in terms of PCIe
> configuration for this platform, which will anyway be needed to
> support the compilation of the PCIe host controller driver as a
> module. In the mean time however, we don't have much other choice than
> to implement the currently proposed solution.

It fixed the detection issue I had on the Armada 385 DB board:
Tetsed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

And as I also agree with the code:

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>


Thanks,

Gregory

> 
> Reported-by: Neil Greatorex <neil@fatboyfat.co.uk>
> Cc: Neil Greatorex <neil@fatboyfat.co.uk>
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: <stable@vger.kernel.org> # 3.14+
> Fixes: af8d1c63afcbf36eea06789c92e22d4af118d2fb ('ARM: mvebu: Add support to get the ID and the revision of a SoC')
> ---
>  arch/arm/mach-mvebu/mvebu-soc-id.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c
> index b52af6f..09520e1 100644
> --- a/arch/arm/mach-mvebu/mvebu-soc-id.c
> +++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
> @@ -108,8 +108,18 @@ static int __init mvebu_soc_id_init(void)
>  	iounmap(pci_base);
>  
>  res_ioremap:
> -	clk_disable_unprepare(clk);
> -	clk_put(clk);
> +	/*
> +	 * If the PCIe unit is actually enabled and we have PCI
> +	 * support in the kernel, we intentionally do not release the
> +	 * reference to the clock. We want to keep it running since
> +	 * the bootloader does some PCIe link configuration that the
> +	 * kernel is for now unable to do, and gating the clock would
> +	 * make us loose this precious configuration.
> +	 */
> +	if (!of_device_is_available(child) || !IS_ENABLED(CONFIG_PCI_MVEBU)) {
> +		clk_disable_unprepare(clk);
> +		clk_put(clk);
> +	}
>  
>  clk_err:
>  	of_node_put(child);
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/2] ARM: mvebu: mvebu-soc-id: keep clock enabled if PCIe unit is enabled

[Willy Tarreau]

Hi Thomas,

On Mon, May 12, 2014 at 04:11:40PM +0200, Thomas Petazzoni wrote:
(...)
> This issue was discussed with Kevin Hilman, and the suggested solution
> was to make the mvebu-soc-id code keep the clock enabled in case it
> will be needed for PCIe. This is therefore the solution implemented in
> this patch.

Great, I'll give it a try ASAP both on the mirabox and the XP-GP
board.

Willy

[PATCH 0/2] ARM: mvebu: fix PCIe/mvebu-soc-id issue

[Andrew Lunn]

On Mon, May 12, 2014 at 04:11:38PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> This set of two patches (and more specifically the second patch) fix a
> PCIe detection problem caused by the introduction of the mvebu-soc-id
> and a bad interaction between the clock manipulation done by the
> mvebu-soc-id code and the PCIe driver. The details are all in the
> commit log of the second patch.
> 
> This second patch fixes a PCIe detection problem noticed by several
> persons.

Hi Thomas

Thanks for the patches. The wifi devices on my kirkwood B3 has now
re-appeared.

Tested-by: Andrew Lunn <andrew@lunn.ch>

	   Andrew

[PATCH 0/2] ARM: mvebu: fix PCIe/mvebu-soc-id issue

[Willy Tarreau]

On Mon, May 12, 2014 at 04:11:38PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> This set of two patches (and more specifically the second patch) fix a
> PCIe detection problem caused by the introduction of the mvebu-soc-id
> and a bad interaction between the clock manipulation done by the
> mvebu-soc-id code and the PCIe driver. The details are all in the
> commit log of the second patch.
> 
> This second patch fixes a PCIe detection problem noticed by several
> persons. The first patch is just a related preparation patch.
> 
> Both patches are meant for 3.15-rc, and should be backported to 3.14
> as part of the stable process.

Both tested on 3.14.3, they work perfectly as expected on mirabox with
igb, and on XP-GP with igb, r8169 and myri10ge, including a dual-port
one with a PCIe bridge onboard.

If you want you can add :

   Tested-by: Willy Tarreau <w@1wt.eu>

Good job!
Willy

[PATCH 0/2] ARM: mvebu: fix PCIe/mvebu-soc-id issue

[Jason Cooper]

On Mon, May 12, 2014 at 04:11:38PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> This set of two patches (and more specifically the second patch) fix a
> PCIe detection problem caused by the introduction of the mvebu-soc-id
> and a bad interaction between the clock manipulation done by the
> mvebu-soc-id code and the PCIe driver. The details are all in the
> commit log of the second patch.
> 
> This second patch fixes a PCIe detection problem noticed by several
> persons. The first patch is just a related preparation patch.
> 
> Both patches are meant for 3.15-rc, and should be backported to 3.14
> as part of the stable process.
> 
> Best regards,
> 
> Thomas
> 
> Thomas Petazzoni (2):
>   ARM: mvebu: mvebu-soc-id: add missing clk_put() call
>   ARM: mvebu: mvebu-soc-id: keep clock enabled if PCIe unit is enabled
> 
>  arch/arm/mach-mvebu/mvebu-soc-id.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

Series applied to mvebu/fixes with Gregory's Ack, and Tested-by's from
Gregory, Andrew, and Willy.

Also, as Rob noted, if you are going to add 'Fixes' tags, please do so
in the following format:

  Fixes: af8d1c63afcb ("ARM: mvebu: Add support to get the ID and the revision of a SoC")

Note that it uses double quotes, and only 12 characters for the hash.

Also, the second patch depends on the first, so I've amended the tags as
such for the second patch:

  Fixes: af8d1c63afcb ("ARM: mvebu: Add support to get the ID and the revision of a SoC")
  Cc: <stable@vger.kernel.org> # 3.14+: 42a18d1cf484: ARM: mvebu: mvebu-soc-id: add missing clk_put() call
  Cc: <stable@vger.kernel.org> # 3.14+

Keep in mind, I don't require any of this.  I /ask/ for the commit hash
of the commit introducing the regression, just to make my job easier.
If you are going to put it in the Fixes format, great.

As for the stable tags, you can't very well add the dependency info
since you don't have the commit hash of the first patch yet. :)

Thanks for all the effort diagnosing this problem and finding the fix!

thx,

Jason.

[PATCH 0/2] ARM: mvebu: fix PCIe/mvebu-soc-id issue

[Thomas Petazzoni]

Dear Jason Cooper,

On Tue, 13 May 2014 11:13:54 -0400, Jason Cooper wrote:

> Series applied to mvebu/fixes with Gregory's Ack, and Tested-by's from
> Gregory, Andrew, and Willy.
> 
> Also, as Rob noted, if you are going to add 'Fixes' tags, please do so
> in the following format:
> 
>   Fixes: af8d1c63afcb ("ARM: mvebu: Add support to get the ID and the revision of a SoC")

Ok, no problem.

> Note that it uses double quotes, and only 12 characters for the hash.
> 
> Also, the second patch depends on the first, so I've amended the tags as
> such for the second patch:
> 
>   Fixes: af8d1c63afcb ("ARM: mvebu: Add support to get the ID and the revision of a SoC")
>   Cc: <stable@vger.kernel.org> # 3.14+: 42a18d1cf484: ARM: mvebu: mvebu-soc-id: add missing clk_put() call
>   Cc: <stable@vger.kernel.org> # 3.14+

Right.

> Keep in mind, I don't require any of this.  I /ask/ for the commit hash
> of the commit introducing the regression, just to make my job easier.
> If you are going to put it in the Fixes format, great.

Yes, I find it doesn't cost me much time, and can possibly save you a
good amount of time. It also ensures a kind of double checking on these
informations: I write them, and then you verify them while applying the
patch.

> As for the stable tags, you can't very well add the dependency info
> since you don't have the commit hash of the first patch yet. :)

Indeed.

> Thanks for all the effort diagnosing this problem and finding the fix!

You're welcome.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com