serial: pxa: prepare/unprepare clocks

Change clk_enable/disable() calls to clk_prepare_enable() and clk_disable_unprepare().

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 drivers/i2c/busses/i2c-pxa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-pxa.c +0 -0

index be671f7..2e75375 100644

Change clk_enable/disable() calls to clk_prepare_enable() and clk_disable_unprepare().

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 drivers/i2c/busses/i2c-pxa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-pxa.c +2 -2

index be671f7..2e75375 100644
@@ -1297,7 +1297,7 @@ static int i2c_pxa_suspend_noirq(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct pxa_i2c *i2c = platform_get_drvdata(pdev);
 
-	clk_disable(i2c->clk);
+	clk_disable_unprepare(i2c->clk);
 
 	return 0;
 }

@@ -1307,7 +1307,7 @@ static int i2c_pxa_resume_noirq(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct pxa_i2c *i2c = platform_get_drvdata(pdev);
 
-	clk_enable(i2c->clk);
+	clk_prepare_enable(i2c->clk);
 	i2c_pxa_reset(i2c);
 
 	return 0;

Signature · 1 line

2.1.1

Change clk_enable/disable() calls to clk_prepare_enable() and clk_disable_unprepare().

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 drivers/usb/gadget/udc/pxa25x_udc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/usb/gadget/udc/pxa25x_udc.c +2 -2

index 42f7eeb..e4964ee 100644
@@ -103,8 +103,8 @@ static const char ep0name [] = "ep0";
 
 /* IXP doesn't yet support <linux/clk.h> */
 #define clk_get(dev,name)	NULL
-#define clk_enable(clk)		do { } while (0)
-#define clk_disable(clk)	do { } while (0)
+#define clk_prepare_enable(clk)	do { } while (0)
+#define clk_disable_unprepare(clk)	do { } while (0)
 #define clk_put(clk)		do { } while (0)
 
 #endif

Quoted 24 lines · Dmitry Eremin-Solenikov

Change clk_enable/disable() calls to clk_prepare_enable() and clk_disable_unprepare().

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 drivers/usb/gadget/udc/pxa25x_udc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/usb/gadget/udc/pxa25x_udc.c +2 -2

index 42f7eeb..e4964ee 100644
@@ -103,8 +103,8 @@ static const char ep0name [] = "ep0";
 
 /* IXP doesn't yet support <linux/clk.h> */
 #define clk_get(dev,name)    NULL
-#define clk_enable(clk)              do { } while (0)
-#define clk_disable(clk)     do { } while (0)
+#define clk_prepare_enable(clk)      do { } while (0)
+#define clk_disable_unprepare(clk)   do { } while (0)
 #define clk_put(clk)         do { } while (0)
 
 #endif

drivers/usb/gadget/udc/pxa25x_udc.c static int pullup(struct pxa25x_udc *udc) · line 935

Quoted 5 lines

drivers/usb/gadget/udc/pxa25x_udc.c +1 -1

@@ -932,7 +932,7 @@ static int pullup(struct pxa25x_udc *udc)
              if (!udc->active) {
                      udc->active = 1;
                      /* Enable clock for USB device */
-                     clk_enable(udc->clk);
+                     clk_prepare_enable(udc->clk);

Guess what, Russell's remark on i2c and serial made me cross-check. And there is a case where this will be called in irq context too ...

See :
-> do_IRQ
-> lubbock_vbus_irq()

    -> pxa25x_udc_vbus_session()
      -> pullup()
        -> clk_prepare_enable()
          -> CRACK

Note that your patch is not really the faulty one, I think a threaded irq instead of the "raw" irq should do the trick. And it is granted on UDC api that vbus function is called in a "sleeping" context (Felipe correct me if I'm wrong), so a patch to fix this before your current code would be fine.

Hello,

2014-11-17 21:44 GMT+03:00 Robert Jarzmik <[email protected]>:

Quoted 43 lines

Quoted 24 lines · Dmitry Eremin-Solenikov

Change clk_enable/disable() calls to clk_prepare_enable() and clk_disable_unprepare().

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 drivers/usb/gadget/udc/pxa25x_udc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/usb/gadget/udc/pxa25x_udc.c +2 -2

index 42f7eeb..e4964ee 100644
@@ -103,8 +103,8 @@ static const char ep0name [] = "ep0";
 
 /* IXP doesn't yet support <linux/clk.h> */
 #define clk_get(dev,name)    NULL
-#define clk_enable(clk)              do { } while (0)
-#define clk_disable(clk)     do { } while (0)
+#define clk_prepare_enable(clk)      do { } while (0)
+#define clk_disable_unprepare(clk)   do { } while (0)
 #define clk_put(clk)         do { } while (0)
 
 #endif

drivers/usb/gadget/udc/pxa25x_udc.c static int pullup(struct pxa25x_udc *udc) · line 935

Quoted 5 lines

drivers/usb/gadget/udc/pxa25x_udc.c +1 -1

@@ -932,7 +932,7 @@ static int pullup(struct pxa25x_udc *udc)
              if (!udc->active) {
                      udc->active = 1;
                      /* Enable clock for USB device */
-                     clk_enable(udc->clk);
+                     clk_prepare_enable(udc->clk);

Guess what, Russell's remark on i2c and serial made me cross-check. And there is a case where this will be called in irq context too ...

See :
-> do_IRQ
-> lubbock_vbus_irq()

    -> pxa25x_udc_vbus_session()
      -> pullup()
        -> clk_prepare_enable()
          -> CRACK

Quoted 4 lines

Note that your patch is not really the faulty one, I think a threaded irq instead of the "raw" irq should do the trick. And it is granted on UDC api that vbus function is called in a "sleeping" context (Felipe correct me if I'm wrong), so a patch to fix this before your current code would be fine.

OK, I will take a look. It seems the correct way would be to strip this code away to a phy, like gpio-vbus or nop phys. Do you have access to lubbock (or maybe Daniel, Haojian or Russell has?)?

Hello,
OK, I will take a look. It seems the correct way would be to strip this code away to a phy, like gpio-vbus or nop phys. Do you have access to lubbock (or maybe Daniel, Haojian or Russell has?)?

Also, as we are at it. Imre, Krzysztof, IIRC ixp4xx platform does not provide clock api either in a form of COMMON_CLK or in a local hacked form (like sa1100 does). Do you plan to add any in future, or it just does not make sense to support this API for ixp4xx?

2014-11-17 21:44 GMT+03:00 Robert Jarzmik <[email protected]>:

Quoted 11 lines

Guess what, Russell's remark on i2c and serial made me cross-check. And there is a case where this will be called in irq context too ...

See :
-> do_IRQ
-> lubbock_vbus_irq()

    -> pxa25x_udc_vbus_session()
      -> pullup()
        -> clk_prepare_enable()
          -> CRACK

Quoted 4 lines

Note that your patch is not really the faulty one, I think a threaded irq instead of the "raw" irq should do the trick. And it is granted on UDC api that vbus function is called in a "sleeping" context (Felipe correct me if I'm wrong), so a patch to fix this before your current code would be fine.

OK, I will take a look. It seems the correct way would be to strip this code away to a phy, like gpio-vbus or nop phys. Do you have access to lubbock (or maybe Daniel, Haojian or Russell has?)?

I have sketched a compile-tested only PHY for the Lubbock platform. Could you please take a look and test.
git://git.infradead.org/users/dbaryshkov/zaurus.git lubbock

Quoted 3 lines · Dmitry Eremin-Solenikov

I have sketched a compile-tested only PHY for the Lubbock platform. Could you please take a look and test.
git://git.infradead.org/users/dbaryshkov/zaurus.git lubbock

Okay, now my ADSL line is repaired, thank you Mr farmer "I drive a tractor but I don't care about telephone posts", I'll fetch your changes and make a test. Let's say I schedule this for saturday.

Okay, now my ADSL line is repaired, thank you Mr farmer "I drive a tractor but I don't care about telephone posts", I'll fetch your changes and make a test. Let's say I schedule this for saturday.

Quoted 3 lines · Robert Jarzmik

Okay, now my ADSL line is repaired, thank you Mr farmer "I drive a tractor but I don't care about telephone posts", I'll fetch your changes and make a test. Let's say I schedule this for saturday.

Removed people from the list for the tests aspect.

Well, my lubbock board seem to raise several interrupts in the linux kernel, all the LUBBOCK_IRQ interrupts multiplexed on GPIO0 actually, amongst which are the 2 interrupts for the UDC.

2014-11-22 16:56 GMT+03:00 Robert Jarzmik <[email protected]>:

Quoted 10 lines

Quoted 3 lines · Robert Jarzmik

Okay, now my ADSL line is repaired, thank you Mr farmer "I drive a tractor but I don't care about telephone posts", I'll fetch your changes and make a test. Let's say I schedule this for saturday.

Removed people from the list for the tests aspect.

Well, my lubbock board seem to raise several interrupts in the linux kernel, all the LUBBOCK_IRQ interrupts multiplexed on GPIO0 actually, amongst which are the 2 interrupts for the UDC.

This looks normal - GPIO0 is connected to an interrupt pin of the CPLD. lubbock_init_irq function should be setting up a chained handler for the GPIO0. And that chained handler (lubbock_irq_handler) should further decode which IRQ bit is set. Do you see the handler being called? Can you print the (pending) variable in the handler?

Quoted 6 lines · Dmitry Eremin-Solenikov

2014-11-22 16:56 GMT+03:00 Robert Jarzmik <[email protected]>:

Quoted 5 lines

Quoted 3 lines · Robert Jarzmik

Okay, now my ADSL line is repaired, thank you Mr farmer "I drive a tractor but I don't care about telephone posts", I'll fetch your changes and make a test. Let's say I schedule this for saturday.

Quoted 5 lines

Quoted 5 lines

Removed people from the list for the tests aspect.

Well, my lubbock board seem to raise several interrupts in the linux kernel, all the LUBBOCK_IRQ interrupts multiplexed on GPIO0 actually, amongst which are the 2 interrupts for the UDC.

Arg, what I meant to write was "seem to *never* raise several interrupts".

Hello,

2014-11-22 20:18 GMT+03:00 Robert Jarzmik <[email protected]>:

Quoted 8 lines

Quoted 6 lines · Dmitry Eremin-Solenikov

2014-11-22 16:56 GMT+03:00 Robert Jarzmik <[email protected]>:

Quoted 5 lines

Quoted 3 lines · Robert Jarzmik

Okay, now my ADSL line is repaired, thank you Mr farmer "I drive a tractor but I don't care about telephone posts", I'll fetch your changes and make a test. Let's say I schedule this for saturday.

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

Removed people from the list for the tests aspect.

Well, my lubbock board seem to raise several interrupts in the linux kernel, all the LUBBOCK_IRQ interrupts multiplexed on GPIO0 actually, amongst which are the 2 interrupts for the UDC.

Arg, what I meant to write was "seem to *never* raise several interrupts".

Understood. Just to make me sure - does the upstream kernel work?

Quoted 13 lines · Dmitry Eremin-Solenikov

Hello,

2014-11-22 20:18 GMT+03:00 Robert Jarzmik <[email protected]>:

Quoted 10 lines

Quoted 8 lines · Dmitry Eremin-Solenikov

2014-11-22 16:56 GMT+03:00 Robert Jarzmik <[email protected]>:

Quoted 7 lines

Quoted 3 lines · Robert Jarzmik

Okay, now my ADSL line is repaired, thank you Mr farmer "I drive a tractor but I don't care about telephone posts", I'll fetch your changes and make a test. Let's say I schedule this for saturday.

Removed people from the list for the tests aspect.

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

Quoted 3 lines

Well, my lubbock board seem to raise several interrupts in the linux kernel, all the LUBBOCK_IRQ interrupts multiplexed on GPIO0 actually, amongst which are the 2 interrupts for the UDC.

Arg, what I meant to write was "seem to *never* raise several interrupts".

Understood. Just to make me sure - does the upstream kernel work?

Nope, that's the current situation with the upstream kernel.

2014-11-22 20:49 GMT+03:00 Robert Jarzmik <[email protected]>:
Next point would be (from my POV) to make sure that lubbock_unmask_irq() is called
and works as expected.

OK Dmitry, I pulled through, the interrupts are working back.

Actually one of the blockers I have is in pxa25x_udc, and it is also in your phy-lubbock.c. The bottom of the error is that disable_irq() is called from within a irq handler, and it deadlocks. A disable_irq_nosync() should be used
...

... but a better approach would be to use a threaded irq for vbus handling. I think that way disable_irq() can be used, no workqueue is needed anymore in phy-lubbock.

Would you make that change, I'll test it and review it.

Quoted 5 lines · Dmitry Eremin-Solenikov

2014-11-27 1:12 GMT+03:00 Robert Jarzmik <[email protected]>:

Quoted 1 line

OK Dmitry, I pulled through, the interrupts are working back.

What was the problem? Hardware issues? Firmware in CPLD being of old revision?

There were basically 2 problems :
- hardware: switch S13 was in a position that disabled interrupts all the time One other problem which fooled me was the incorrect gate schematics I had, and which make my understanding of linear function foo such as GPIO0 = foo(LUB_IRQ_EN, LUB_SET_CLR) false.

- software: lubbock.c error
Since gpio-pxa was ported to device/gpio, it is probed *after* lubbock_init_irq() is called. As lubbock_init_irq() installs lubbock_irq_handler() and sets the irq to falling edge detect before gpio-pxa is probed, gpio-pxa probe overwrites this by :
- clearing any edge detection (probe state reset)
- installing generic handle_edge_irq() instead of the lubbock irq handler

So the conclusion is that I have to rework lubbock_init_irq(), remove from it the PXA_GPIO_TO_IRQ(0) stuff, and move it to a code in lubbock.c which will provide the guarantee of being executed _after_ gpio-pxa is probed. When I'm happy with my patch I'll submit it.

Hello,

2014-11-17 21:44 GMT+03:00 Robert Jarzmik <[email protected]>:

Quoted 47 lines

Quoted 24 lines · Dmitry Eremin-Solenikov

Change clk_enable/disable() calls to clk_prepare_enable() and clk_disable_unprepare().

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 drivers/usb/gadget/udc/pxa25x_udc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/usb/gadget/udc/pxa25x_udc.c +2 -2

index 42f7eeb..e4964ee 100644
@@ -103,8 +103,8 @@ static const char ep0name [] = "ep0";
 
 /* IXP doesn't yet support <linux/clk.h> */
 #define clk_get(dev,name)    NULL
-#define clk_enable(clk)              do { } while (0)
-#define clk_disable(clk)     do { } while (0)
+#define clk_prepare_enable(clk)      do { } while (0)
+#define clk_disable_unprepare(clk)   do { } while (0)
 #define clk_put(clk)         do { } while (0)
 
 #endif

drivers/usb/gadget/udc/pxa25x_udc.c static int pullup(struct pxa25x_udc *udc) · line 935

Quoted 5 lines

drivers/usb/gadget/udc/pxa25x_udc.c +1 -1

@@ -932,7 +932,7 @@ static int pullup(struct pxa25x_udc *udc)
              if (!udc->active) {
                      udc->active = 1;
                      /* Enable clock for USB device */
-                     clk_enable(udc->clk);
+                     clk_prepare_enable(udc->clk);

Guess what, Russell's remark on i2c and serial made me cross-check. And there is a case where this will be called in irq context too ...

See :
-> do_IRQ
-> lubbock_vbus_irq()

    -> pxa25x_udc_vbus_session()
      -> pullup()
        -> clk_prepare_enable()
          -> CRACK

Note that your patch is not really the faulty one, I think a threaded irq instead of the "raw" irq should do the trick. And it is granted on UDC api that vbus function is called in a "sleeping" context (Felipe correct me if I'm wrong), so a patch to fix this before your current code would be fine.

Change clk_enable/disable() calls to clk_prepare_enable() and clk_disable_unprepare().

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 drivers/mfd/t7l66xb.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/mfd/t7l66xb.c +7 -7

index 9e04a74..439d905 100644
@@ -87,7 +87,7 @@ static int t7l66xb_mmc_enable(struct platform_device *mmc)
 	unsigned long flags;
 	u8 dev_ctl;
 
-	clk_enable(t7l66xb->clk32k);
+	clk_prepare_enable(t7l66xb->clk32k);
 
 	spin_lock_irqsave(&t7l66xb->lock, flags);
 

@@ -118,7 +118,7 @@ static int t7l66xb_mmc_disable(struct platform_device *mmc)
 
 	spin_unlock_irqrestore(&t7l66xb->lock, flags);
 
-	clk_disable(t7l66xb->clk32k);
+	clk_disable_unprepare(t7l66xb->clk32k);
 
 	return 0;
 }

@@ -285,7 +285,7 @@ static int t7l66xb_suspend(struct platform_device *dev, pm_message_t state)
 
 	if (pdata && pdata->suspend)
 		pdata->suspend(dev);
-	clk_disable(t7l66xb->clk48m);
+	clk_disable_unprepare(t7l66xb->clk48m);
 
 	return 0;
 }

@@ -295,7 +295,7 @@ static int t7l66xb_resume(struct platform_device *dev)
 	struct t7l66xb *t7l66xb = platform_get_drvdata(dev);
 	struct t7l66xb_platform_data *pdata = dev_get_platdata(&dev->dev);
 
-	clk_enable(t7l66xb->clk48m);
+	clk_prepare_enable(t7l66xb->clk48m);
 	if (pdata && pdata->resume)
 		pdata->resume(dev);
 

@@ -369,7 +369,7 @@ static int t7l66xb_probe(struct platform_device *dev)
 		goto err_ioremap;
 	}
 
-	clk_enable(t7l66xb->clk48m);
+	clk_prepare_enable(t7l66xb->clk48m);
 
 	if (pdata && pdata->enable)
 		pdata->enable(dev);

@@ -414,9 +414,9 @@ static int t7l66xb_remove(struct platform_device *dev)
 	int ret;
 
 	ret = pdata->disable(dev);
-	clk_disable(t7l66xb->clk48m);
+	clk_disable_unprepare(t7l66xb->clk48m);
 	clk_put(t7l66xb->clk48m);
-	clk_disable(t7l66xb->clk32k);
+	clk_disable_unprepare(t7l66xb->clk32k);
 	clk_put(t7l66xb->clk32k);
 	t7l66xb_detach_irq(dev);
 	iounmap(t7l66xb->scr);

Change clk_enable/disable() calls to clk_prepare_enable() and clk_disable_unprepare().

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 drivers/mfd/tc6387xb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/mfd/tc6387xb.c +5 -5

index e71f880..85fab37 100644
@@ -52,7 +52,7 @@ static int tc6387xb_suspend(struct platform_device *dev, pm_message_t state)
 
 	if (pdata && pdata->suspend)
 		pdata->suspend(dev);
-	clk_disable(tc6387xb->clk32k);
+	clk_disable_unprepare(tc6387xb->clk32k);
 
 	return 0;
 }

@@ -62,7 +62,7 @@ static int tc6387xb_resume(struct platform_device *dev)
 	struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
 	struct tc6387xb_platform_data *pdata = dev_get_platdata(&dev->dev);
 
-	clk_enable(tc6387xb->clk32k);
+	clk_prepare_enable(tc6387xb->clk32k);
 	if (pdata && pdata->resume)
 		pdata->resume(dev);
 

@@ -100,7 +100,7 @@ static int tc6387xb_mmc_enable(struct platform_device *mmc)
 	struct platform_device *dev      = to_platform_device(mmc->dev.parent);
 	struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
 
-	clk_enable(tc6387xb->clk32k);
+	clk_prepare_enable(tc6387xb->clk32k);
 
 	tmio_core_mmc_enable(tc6387xb->scr + 0x200, 0,
 		tc6387xb_mmc_resources[0].start & 0xfffe);

@@ -113,7 +113,7 @@ static int tc6387xb_mmc_disable(struct platform_device *mmc)
 	struct platform_device *dev      = to_platform_device(mmc->dev.parent);
 	struct tc6387xb *tc6387xb = platform_get_drvdata(dev);
 
-	clk_disable(tc6387xb->clk32k);
+	clk_disable_unprepare(tc6387xb->clk32k);
 
 	return 0;
 }

@@ -214,7 +214,7 @@ static int tc6387xb_remove(struct platform_device *dev)
 	mfd_remove_devices(&dev->dev);
 	iounmap(tc6387xb->scr);
 	release_resource(&tc6387xb->rscr);
-	clk_disable(tc6387xb->clk32k);
+	clk_disable_unprepare(tc6387xb->clk32k);
 	clk_put(tc6387xb->clk32k);
 	kfree(tc6387xb);
 

Change clk_enable/disable() calls to clk_prepare_enable() and clk_disable_unprepare().