[PATCH v2] soc: qcom: do not disable the iface clock in probe

[PATCH v2] soc: qcom: do not disable the iface clock in probe

[Srinivas Kandagatla]

since commit 31964ffebbb9 ("tty: serial: msm: Remove direct access to GSBI")'
serial hangs if earlyprintk are enabled.

This hang is noticed only when the GSBI driver is probed and all the
earlyprintks before gsbi probe are seen on the console.
The reason why it hangs is because GSBI driver disables hclk in its
probe function without realizing that the serial IP might be in use by
a bootconsole. As gsbi driver disables the clock in probe the
bootconsole locks up.

Turning off hclk's could be dangerous if there are system components
like earlyprintk using the hclk.

This patch fixes the issue by delegating the clock management to
probe and remove functions in gsbi rather than disabling the clock in probe.

More detailed problem description can be found here:
http://www.spinics.net/lists/linux-arm-msm/msg10589.html

Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
Hi Kevin, 

Am resending this patch with reference to the problem and adding more
details to the log.

Could you pick this fix for the next rc, as this fixes a serial console
hang with earlyprintk on SOCs like APQ8064.

Changes since v1:
	- added more info in the change log as requested by Kumar and Kevin.

thanks,
srini

 drivers/soc/qcom/qcom_gsbi.c | 46 +++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/soc/qcom/qcom_gsbi.c b/drivers/soc/qcom/qcom_gsbi.c
index 447458e..7e1f120 100644
--- a/drivers/soc/qcom/qcom_gsbi.c
+++ b/drivers/soc/qcom/qcom_gsbi.c
@@ -22,44 +22,63 @@
 #define GSBI_CTRL_REG		0x0000
 #define GSBI_PROTOCOL_SHIFT	4
 
+struct gsbi_info {
+	struct clk *hclk;
+	u32 mode;
+	u32 crci;
+};
+
 static int gsbi_probe(struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
 	struct resource *res;
 	void __iomem *base;
-	struct clk *hclk;
-	u32 mode, crci = 0;
+	struct gsbi_info *gsbi;
+
+	gsbi = devm_kzalloc(&pdev->dev, sizeof(*gsbi), GFP_KERNEL);
+
+	if (!gsbi)
+		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
-	if (of_property_read_u32(node, "qcom,mode", &mode)) {
+	if (of_property_read_u32(node, "qcom,mode", &gsbi->mode)) {
 		dev_err(&pdev->dev, "missing mode configuration\n");
 		return -EINVAL;
 	}
 
 	/* not required, so default to 0 if not present */
-	of_property_read_u32(node, "qcom,crci", &crci);
+	of_property_read_u32(node, "qcom,crci", &gsbi->crci);
 
-	dev_info(&pdev->dev, "GSBI port protocol: %d crci: %d\n", mode, crci);
+	dev_info(&pdev->dev, "GSBI port protocol: %d crci: %d\n",
+		 gsbi->mode, gsbi->crci);
+	gsbi->hclk = devm_clk_get(&pdev->dev, "iface");
+	if (IS_ERR(gsbi->hclk))
+		return PTR_ERR(gsbi->hclk);
 
-	hclk = devm_clk_get(&pdev->dev, "iface");
-	if (IS_ERR(hclk))
-		return PTR_ERR(hclk);
+	clk_prepare_enable(gsbi->hclk);
 
-	clk_prepare_enable(hclk);
-
-	writel_relaxed((mode << GSBI_PROTOCOL_SHIFT) | crci,
+	writel_relaxed((gsbi->mode << GSBI_PROTOCOL_SHIFT) | gsbi->crci,
 				base + GSBI_CTRL_REG);
 
 	/* make sure the gsbi control write is not reordered */
 	wmb();
 
-	clk_disable_unprepare(hclk);
+	platform_set_drvdata(pdev, gsbi);
+
+	return of_platform_populate(node, NULL, NULL, &pdev->dev);
+}
+
+static int gsbi_remove(struct platform_device *pdev)
+{
+	struct gsbi_info *gsbi = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(gsbi->hclk);
 
-	return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+	return 0;
 }
 
 static const struct of_device_id gsbi_dt_match[] = {
@@ -76,6 +95,7 @@ static struct platform_driver gsbi_driver = {
 		.of_match_table	= gsbi_dt_match,
 	},
 	.probe = gsbi_probe,
+	.remove	= gsbi_remove,
 };
 
 module_platform_driver(gsbi_driver);
-- 
1.9.1

[PATCH v2] soc: qcom: do not disable the iface clock in probe

[Olof Johansson]

On Tue, Sep 23, 2014 at 08:20:54PM +0100, Srinivas Kandagatla wrote:
> since commit 31964ffebbb9 ("tty: serial: msm: Remove direct access to GSBI")'
> serial hangs if earlyprintk are enabled.
> 
> This hang is noticed only when the GSBI driver is probed and all the
> earlyprintks before gsbi probe are seen on the console.
> The reason why it hangs is because GSBI driver disables hclk in its
> probe function without realizing that the serial IP might be in use by
> a bootconsole. As gsbi driver disables the clock in probe the
> bootconsole locks up.
> 
> Turning off hclk's could be dangerous if there are system components
> like earlyprintk using the hclk.
> 
> This patch fixes the issue by delegating the clock management to
> probe and remove functions in gsbi rather than disabling the clock in probe.
> 
> More detailed problem description can be found here:
> http://www.spinics.net/lists/linux-arm-msm/msg10589.html
> 
> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> Hi Kevin, 
> 
> Am resending this patch with reference to the problem and adding more
> details to the log.
> 
> Could you pick this fix for the next rc, as this fixes a serial console
> hang with earlyprintk on SOCs like APQ8064.
> 
> Changes since v1:
> 	- added more info in the change log as requested by Kumar and Kevin.

Applied to fixes.


-Olof

[PATCH v2] soc: qcom: do not disable the iface clock in probe

[Kevin Hilman]

Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:

> since commit 31964ffebbb9 ("tty: serial: msm: Remove direct access to GSBI")'
> serial hangs if earlyprintk are enabled.
>
> This hang is noticed only when the GSBI driver is probed and all the
> earlyprintks before gsbi probe are seen on the console.
> The reason why it hangs is because GSBI driver disables hclk in its
> probe function without realizing that the serial IP might be in use by
> a bootconsole. As gsbi driver disables the clock in probe the
> bootconsole locks up.

> Turning off hclk's could be dangerous if there are system components
> like earlyprintk using the hclk.

This seems rather fragile.  Isn't the right fix for these other
components to use the clk api to ensure the clock does not get enabled?

Kevin

[PATCH v2] soc: qcom: do not disable the iface clock in probe

[Srinivas Kandagatla]

On 24/09/14 16:22, Kevin Hilman wrote:
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
>
>> since commit 31964ffebbb9 ("tty: serial: msm: Remove direct access to GSBI")'
>> serial hangs if earlyprintk are enabled.
>>
>> This hang is noticed only when the GSBI driver is probed and all the
>> earlyprintks before gsbi probe are seen on the console.
>> The reason why it hangs is because GSBI driver disables hclk in its
>> probe function without realizing that the serial IP might be in use by
>> a bootconsole. As gsbi driver disables the clock in probe the
>> bootconsole locks up.
>
>> Turning off hclk's could be dangerous if there are system components
>> like earlyprintk using the hclk.
>
> This seems rather fragile.  Isn't the right fix for these other
> components to use the clk api to ensure the clock does not get enabled?
Here we are depending on the bootloader to setup the clocks.

Am not sure if we can really use clk apis at that early stage of bootstrap.

--srini



>
> Kevin
>

[PATCH v2] soc: qcom: do not disable the iface clock in probe

[Kevin Hilman]

Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:

> On 24/09/14 16:22, Kevin Hilman wrote:
>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
>>
>>> since commit 31964ffebbb9 ("tty: serial: msm: Remove direct access to GSBI")'
>>> serial hangs if earlyprintk are enabled.
>>>
>>> This hang is noticed only when the GSBI driver is probed and all the
>>> earlyprintks before gsbi probe are seen on the console.
>>> The reason why it hangs is because GSBI driver disables hclk in its
>>> probe function without realizing that the serial IP might be in use by
>>> a bootconsole. As gsbi driver disables the clock in probe the
>>> bootconsole locks up.
>>
>>> Turning off hclk's could be dangerous if there are system components
>>> like earlyprintk using the hclk.
>>
>> This seems rather fragile.  Isn't the right fix for these other
>> components to use the clk api to ensure the clock does not get enabled?
> Here we are depending on the bootloader to setup the clocks.
>
> Am not sure if we can really use clk apis at that early stage of bootstrap.

Not that early, but all that matters is that you clk_enable() sometime
before the GSBI driver calls clk_disable().

If the clocks are always enabled by the bootloader, the platforms clock
driver might check for those and ensure they are enabled, so that linux
is aware of it, and bugs like this one are avoided.

IMO, clock enable/disable shuffling in the GSBI driver to avoid UART
console problems is just papering over a bug that's waiting to pop up
elsewhere.

Kevin