[RFC PATCH 0/2] MSM/QCOM: Fix few race conditions when DEBUG_LL is enabled

[RFC PATCH 0/2] MSM/QCOM: Fix few race conditions when DEBUG_LL is enabled

[Srinivas Kandagatla]

When I started using earlyprintk on IFC6410 I noticed console lockups.

These two patches fix the issues in gsbi and serial driver.
One of the patch is to do with disabling clk while bootconsole is using it and
other is reseting IP while bootconsole is still throwing up characters.

I marked them as RFC for more comments and wanted to know if there are any
other better ways to fix this issue.


--srini

Srinivas Kandagatla (2):
  soc: qcom: do not disable the iface clock in probe
  tty:msm_serial: Do not reset IP if we use bootconsole

 drivers/soc/qcom/qcom_gsbi.c    | 46 +++++++++++++++++++++++++++++------------
 drivers/tty/serial/msm_serial.c | 18 +++++++++++++++-
 2 files changed, 50 insertions(+), 14 deletions(-)

-- 
1.9.1

[RFC PATCH 1/2] soc: qcom: do not disable the iface clock in probe

[Srinivas Kandagatla]

The use case here is when we have a bootconsole which is printing the
characters on serial console and gsbi driver comes up after some time.
As gsbi driver disables the clock in probe the bootconsole locks up.

This patch fixes the problem by disabling the clock in platform remove
rather than in probe.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 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 ab7b441..64fb298 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

[RFC PATCH 2/2] tty:msm_serial: Do not reset IP if we use bootconsole

[Srinivas Kandagatla]

The use case is when we boot the platform with bootconsole enabled. What
I noticed is that the console gets locked sometimes up before the bootconsole
is disabled.

As part of console setup in serial driver it resets that hardware which
is a race condition to bootconsole using the same hardware. This
patch adds a check to see if there is bootconsole before reseting the hardware.

Am sure there are better ways to solve this, so marking this patch as
RFC. Any suggestions are welcome.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/tty/serial/msm_serial.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index 778e376..7078153 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -913,6 +913,17 @@ static void msm_console_write(struct console *co, const char *s,
 	spin_unlock(&port->lock);
 }
 
+static int have_boot_console(void)
+{
+	struct console *con;
+
+	for_each_console(con)
+		if (con->flags & CON_BOOT)
+			return 1;
+
+	return 0;
+}
+
 static int __init msm_console_setup(struct console *co, char *options)
 {
 	struct uart_port *port;
@@ -943,7 +954,12 @@ static int __init msm_console_setup(struct console *co, char *options)
 		baud = 115200;
 	msm_set_baud_rate(port, baud);
 
-	msm_reset(port);
+	/*
+	 * do not reset if we are the boot console
+	 * can result in a lockup from bootconsole
+	 */
+	if (have_boot_console())
+		msm_reset(port);
 
 	if (msm_port->is_uartdm) {
 		msm_write(port, UART_CR_CMD_PROTECTION_EN, UART_CR);
-- 
1.9.1

Re: [RFC PATCH 2/2] tty:msm_serial: Do not reset IP if we use bootconsole

[Srinivas Kandagatla]

On 10/06/14 15:32, Srinivas Kandagatla wrote:
> +	 * do not reset if we are the boot console
> +	 * can result in a lockup from bootconsole
> +	 */
> +	if (have_boot_console())
> +		msm_reset(port);
Oops..

I think I sent a wrong changeset I this patch... this should be.

	if (!have_boot_console())
		msm_reset(port);


Will fix it in next version..

Re: [RFC PATCH 1/2] soc: qcom: do not disable the iface clock in probe

[Kumar Gala]

On Jun 10, 2014, at 9:31 AM, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:

> The use case here is when we have a bootconsole which is printing the
> characters on serial console and gsbi driver comes up after some time.
> As gsbi driver disables the clock in probe the bootconsole locks up.
> 
> This patch fixes the problem by disabling the clock in platform remove
> rather than in probe.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> drivers/soc/qcom/qcom_gsbi.c | 46 +++++++++++++++++++++++++++++++-------------
> 1 file changed, 33 insertions(+), 13 deletions(-)

It seems like we shouldn’t need this change.  Adding Stephen to see if there is a reason we don’t have the clk’s enable_count adjusted for how the bootloader setup clks.

- k

> 
> diff --git a/drivers/soc/qcom/qcom_gsbi.c b/drivers/soc/qcom/qcom_gsbi.c
> index ab7b441..64fb298 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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [RFC PATCH 1/2] soc: qcom: do not disable the iface clock in probe

[Stephen Boyd]

On 06/10/14 08:20, Kumar Gala wrote:
> On Jun 10, 2014, at 9:31 AM, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
>
>> The use case here is when we have a bootconsole which is printing the
>> characters on serial console and gsbi driver comes up after some time.
>> As gsbi driver disables the clock in probe the bootconsole locks up.
>>
>> This patch fixes the problem by disabling the clock in platform remove
>> rather than in probe.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> drivers/soc/qcom/qcom_gsbi.c | 46 +++++++++++++++++++++++++++++++-------------
>> 1 file changed, 33 insertions(+), 13 deletions(-)
> It seems like we shouldn’t need this change.  Adding Stephen to see if there is a reason we don’t have the clk’s enable_count adjusted for how the bootloader setup clks.

This is a long standing problem with the clock framework. In our vendor
tree we've added something called "handoff" which basically detects the
state of all clocks upon registration and keeps clocks enabled until
late_init() if the clocks were enabled at the time of registration.

For this case though "handoff" doesn't seem necessary. It's easier to
just disable the clock when the driver is removed. With finer grained
power management this driver can participate in runtime_pm and disable
the ahb clock when the device is runtime suspended; which would only
happen when the child devices (uart/spi/i2c) are also runtime suspended.

> - k
>
>> diff --git a/drivers/soc/qcom/qcom_gsbi.c b/drivers/soc/qcom/qcom_gsbi.c
>> index ab7b441..64fb298 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;
>> +};

What does mode and crci have to do with this patch? Can't we just put
the clock into the platform data?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC PATCH 1/2] soc: qcom: do not disable the iface clock in probe

[Andy Gross]

On Tue, Jun 10, 2014 at 10:39:02AM -0700, Stephen Boyd wrote:

<snip>

> >> diff --git a/drivers/soc/qcom/qcom_gsbi.c b/drivers/soc/qcom/qcom_gsbi.c
> >> index ab7b441..64fb298 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;
> >> +};
> 
> What does mode and crci have to do with this patch? Can't we just put
> the clock into the platform data?

I second this notion.

-- 
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC PATCH 1/2] soc: qcom: do not disable the iface clock in probe

[Srinivas Kandagatla]

On 10/06/14 18:39, Stephen Boyd wrote:
> On 06/10/14 08:20, Kumar Gala wrote:
>> On Jun 10, 2014, at 9:31 AM, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote:
>>
>>> The use case here is when we have a bootconsole which is printing the
>>> characters on serial console and gsbi driver comes up after some time.
>>> As gsbi driver disables the clock in probe the bootconsole locks up.
>>>
>>> This patch fixes the problem by disabling the clock in platform remove
>>> rather than in probe.
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> ---
>>> drivers/soc/qcom/qcom_gsbi.c | 46 +++++++++++++++++++++++++++++++-------------
>>> 1 file changed, 33 insertions(+), 13 deletions(-)
>> It seems like we shouldn’t need this change.  Adding Stephen to see if there is a reason we don’t have the clk’s enable_count adjusted for how the bootloader setup clks.
>
> This is a long standing problem with the clock framework. In our vendor
> tree we've added something called "handoff" which basically detects the
> state of all clocks upon registration and keeps clocks enabled until
> late_init() if the clocks were enabled at the time of registration.
>
> For this case though "handoff" doesn't seem necessary. It's easier to
> just disable the clock when the driver is removed. With finer grained
> power management this driver can participate in runtime_pm and disable
> the ahb clock when the device is runtime suspended; which would only
> happen when the child devices (uart/spi/i2c) are also runtime suspended.

I had same thought about gsbi participating in PM.
>
>> - k
>>
>>> diff --git a/drivers/soc/qcom/qcom_gsbi.c b/drivers/soc/qcom/qcom_gsbi.c
>>> index ab7b441..64fb298 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;
>>> +};
>
> What does mode and crci have to do with this patch? Can't we just put
> the clock into the platform data?
It has nothing to do with this but, for completeness and we might need 
this if we are doing PM in future. for example pm resume might want to 
reconfigure the gsbi.

Am Ok with either approaches.


--srini

>

Re: [RFC PATCH 1/2] soc: qcom: do not disable the iface clock in probe

[Andy Gross]

On Tue, Jun 10, 2014 at 06:47:29PM +0100, Srinivas Kandagatla wrote:

<snip>

> >What does mode and crci have to do with this patch? Can't we just put
> >the clock into the platform data?
> It has nothing to do with this but, for completeness and we might
> need this if we are doing PM in future. for example pm resume might
> want to reconfigure the gsbi.

Yes, setting idle mode would save some power.


-- 
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC PATCH 2/2] tty:msm_serial: Do not reset IP if we use bootconsole

[Stephen Boyd]

On 06/10/14 07:32, Srinivas Kandagatla wrote:
> The use case is when we boot the platform with bootconsole enabled. What
> I noticed is that the console gets locked sometimes up before the bootconsole
> is disabled.
>
> As part of console setup in serial driver it resets that hardware which
> is a race condition to bootconsole using the same hardware. This
> patch adds a check to see if there is bootconsole before reseting the hardware.
>
> Am sure there are better ways to solve this, so marking this patch as
> RFC. Any suggestions are welcome.
>
>

Isn't there some way to check and wait for the hardware to be unused
before we reset it?

I recall being able to overcome this "race condition" by removing the
printks in the serial driver setup. Does that work for you?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC PATCH 2/2] tty:msm_serial: Do not reset IP if we use bootconsole

[Srinivas Kandagatla]

On 17/06/14 22:09, Stephen Boyd wrote:
> On 06/10/14 07:32, Srinivas Kandagatla wrote:
>> The use case is when we boot the platform with bootconsole enabled. What
>> I noticed is that the console gets locked sometimes up before the bootconsole
>> is disabled.
>>
>> As part of console setup in serial driver it resets that hardware which
>> is a race condition to bootconsole using the same hardware. This
>> patch adds a check to see if there is bootconsole before reseting the hardware.
>>
>> Am sure there are better ways to solve this, so marking this patch as
>> RFC. Any suggestions are welcome.
>>
>>
>
> Isn't there some way to check and wait for the hardware to be unused
> before we reset it?
>
> I recall being able to overcome this "race condition" by removing the
> printks in the serial driver setup. Does that work for you?
>
Good point,
Its possible that removing prints during the transition fixes the issue. 
I will give it a try.

Re: [RFC PATCH 2/2] tty:msm_serial: Do not reset IP if we use bootconsole

[Srinivas Kandagatla]

Hi Stephen,

Did bit more testing on this.

I could not reproduce this issue (across 40+ reboots) once I removed 
some of my debug, so we can ignore this patch for now, But we still need 
gsbi patch for clock.



Thanks,
srini

On 17/06/14 22:09, Stephen Boyd wrote:
> On 06/10/14 07:32, Srinivas Kandagatla wrote:
>> The use case is when we boot the platform with bootconsole enabled. What
>> I noticed is that the console gets locked sometimes up before the bootconsole
>> is disabled.
>>
>> As part of console setup in serial driver it resets that hardware which
>> is a race condition to bootconsole using the same hardware. This
>> patch adds a check to see if there is bootconsole before reseting the hardware.
>>
>> Am sure there are better ways to solve this, so marking this patch as
>> RFC. Any suggestions are welcome.
>>
>>
>
> Isn't there some way to check and wait for the hardware to be unused
> before we reset it?
>
> I recall being able to overcome this "race condition" by removing the
> printks in the serial driver setup. Does that work for you?
>

Re: [RFC PATCH 1/2] soc: qcom: do not disable the iface clock in probe

[Srinivas Kandagatla]

Hi Andy,
Do you know who is going to queue this patch for 3.17?

thanks,
srini


On 10/06/14 15:31, Srinivas Kandagatla wrote:
> The use case here is when we have a bootconsole which is printing the
> characters on serial console and gsbi driver comes up after some time.
> As gsbi driver disables the clock in probe the bootconsole locks up.
>
> This patch fixes the problem by disabling the clock in platform remove
> rather than in probe.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   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 ab7b441..64fb298 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);
>

Re: [RFC PATCH 1/2] soc: qcom: do not disable the iface clock in probe

[Srinivas Kandagatla]

Hi Andy/Kumar,

If its not too late,
Can we queue this fix for 3.17-rc1 or rc2?


thanks,
srini
On 10/06/14 15:31, Srinivas Kandagatla wrote:
> The use case here is when we have a bootconsole which is printing the
> characters on serial console and gsbi driver comes up after some time.
> As gsbi driver disables the clock in probe the bootconsole locks up.
>
> This patch fixes the problem by disabling the clock in platform remove
> rather than in probe.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   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 ab7b441..64fb298 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);
>