S3C24XX: Ensure GPIO pins are configure properly on init and resume.
Subsystems (2)
Add initialisation for the necessary GPIO lines on driver probe and resume. This is needed as we need to place the GPIO lines into inputs over suspend as the controller is reset during the resume path and the initial state is to act as a SPI slave which drivers the MISO line. With the MISO line driven, there is the likelyhood that the chip will be driving against the SPI slave device (not all SPI devices tristate their MISO line when their chipselect is inactive)
drivers/spi/spi_s3c24xx.c +36 -0
@@ -238,6 +238,8 @@ static irqreturn_t s3c24xx_spi_irq(int istatic void s3c24xx_spi_initialsetup (struct s3c24xx_spi *hw) { +struct platform_device *pdev =to_platform_device (hw->dev); +/* for the moment, permanently enable the clock */ clk_enable (hw->clk);
@@ -247,6 +249,20 @@ static void s3c24xx_spi_initialsetup(strwriteb (0xff , hw->regs +S3C2410_SPPRE );writeb (SPPIN_DEFAULT , hw->regs +S3C2410_SPPIN );writeb (SPCON_DEFAULT , hw->regs +S3C2410_SPCON ); + +if (pdev->id ==0 ) { +s3c2410_gpio_cfgpin (S3C2410_GPE13 ,S3C2410_GPE13_SPICLK0 ); +s3c2410_gpio_cfgpin (S3C2410_GPE12 ,S3C2410_GPE12_SPIMOSI0 ); +s3c2410_gpio_cfgpin (S3C2410_GPE11 ,S3C2410_GPE11_SPIMISO0 ); +s3c2410_gpio_pullup (S3C2410_GPE11 ,0 ); +s3c2410_gpio_pullup (S3C2410_GPE12 ,0 ); + }else { +s3c2410_gpio_cfgpin (S3C2410_GPG7 ,S3C2410_GPG7_SPICLK1 ); +s3c2410_gpio_cfgpin (S3C2410_GPG6 ,S3C2410_GPG6_SPIMOSI1 ); +s3c2410_gpio_cfgpin (S3C2410_GPG5 ,S3C2410_GPG5_SPIMISO1 ); +s3c2410_gpio_pullup (S3C2410_GPG5 ,0 ); +s3c2410_gpio_pullup (S3C2410_GPG6 ,0 ); + } }static int __init s3c24xx_spi_probe (struct platform_device *pdev)
@@ -412,6 +428,26 @@ static int s3c24xx_spi_suspend(struct pl {struct s3c24xx_spi *hw =platform_get_drvdata (pdev); +/* Place the gpio associated with the controller into input over +* suspend as on resume the controller is reset and will end up +* as a slave until we reconfigure it in the resume path. This +* means that we cannot allow the controller to drive MISO line +* during this period in case it drives against something on the +* board. +*/ + +if (pdev->id ==0 ) { +s3c2410_gpio_pullup (S3C2410_GPE12 ,1 ); +s3c2410_gpio_pullup (S3C2410_GPE13 ,1 ); +s3c2410_gpio_cfgpin (S3C2410_GPE12 ,S3C2410_GPIO_INPUT ); +s3c2410_gpio_cfgpin (S3C2410_GPE11 ,S3C2410_GPIO_INPUT ); + }else { +s3c2410_gpio_pullup (S3C2410_GPG5 ,1 ); +s3c2410_gpio_pullup (S3C2410_GPG6 ,1 ); +s3c2410_gpio_cfgpin (S3C2410_GPG6 ,S3C2410_GPIO_INPUT ); +s3c2410_gpio_cfgpin (S3C2410_GPG5 ,S3C2410_GPIO_INPUT ); + } +clk_disable (hw->clk);return 0 ; }
Signature · 9 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes' ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/
A message that is not in the archive (sent off-list or not yet received): [email protected]
drivers/spi/spi_s3c24xx.c static int s3c24xx_spi_suspend(struct pl · line 431
Quoted 5 lines · Ben Dooks
drivers/spi/spi_s3c24xx.c +1
-0@@ -412,6 +428,26 @@ static int s3c24xx_spi_suspend(struct pl {struct s3c24xx_spi *hw =platform_get_drvdata (pdev); +/* Place the gpio associated with the controller into input over
Well, input-with-pullup. As usual, we don't want inputs to float. :)
Review static int s3c24xx_spi_suspend(struct pl · line 433
Quoted 2 lines
(quoted diff) +2
-0+ * suspend as on resume the controller is reset and will end up + * as a slave until we reconfigure it in the resume path.
Kind of unfriendly, and not something I've seen much on ARM, FWIW.
I generally like to see pinmux stuff kept out of drivers, since it tends to change between revisions more than the controller design. I take it that's not yet an issue with this controller ...
drivers/spi/spi_s3c24xx.c static int s3c24xx_spi_suspend(struct pl · line 438
Quoted 6 lines
drivers/spi/spi_s3c24xx.c +5
-0This +* means that we cannot allow the controller to drive MISO line +* during this period in case it drives against something on the +* board. +*/ +
What I don't get is that there are two outputs other than chipselect (SCK on GPE13 and GPG7, and MOSI on GPE12 and GPG6), and you seem to remux those on bus 0 but *NOT* bus 1:
drivers/spi/spi_s3c24xx.c static int s3c24xx_spi_suspend(struct pl · line 448
Quoted 10 lines
drivers/spi/spi_s3c24xx.c +10
-0+if (pdev->id ==0 ) { +s3c2410_gpio_pullup (S3C2410_GPE12 ,1 ); +s3c2410_gpio_pullup (S3C2410_GPE13 ,1 ); +s3c2410_gpio_cfgpin (S3C2410_GPE12 ,S3C2410_GPIO_INPUT ); +s3c2410_gpio_cfgpin (S3C2410_GPE11 ,S3C2410_GPIO_INPUT ); + }else { +s3c2410_gpio_pullup (S3C2410_GPG5 ,1 ); +s3c2410_gpio_pullup (S3C2410_GPG6 ,1 ); +s3c2410_gpio_cfgpin (S3C2410_GPG6 ,S3C2410_GPIO_INPUT ); +s3c2410_gpio_cfgpin (S3C2410_GPG5 ,S3C2410_GPIO_INPUT );
... where you do GPG6/MOSI alright, but GPG5/MISO not GPG7/SCK.
Or is the suspend side of your patch inconsistent with the resume side?
Quoted 7 lines
drivers/spi/spi_s3c24xx.c +2
-0+ } +clk_disable (hw->clk);return 0 ;}
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
A message that is not in the archive (sent off-list or not yet received): 200807310114.42424.david-b-yBeKhBN/[email protected]
Quoted 9 lines · David Brownell
drivers/spi/spi_s3c24xx.c static int s3c24xx_spi_suspend(struct pl · line 431
Quoted 5 lines · Ben Dooks
drivers/spi/spi_s3c24xx.c +1
-0@@ -412,6 +428,26 @@ static int s3c24xx_spi_suspend(struct pl {struct s3c24xx_spi *hw =platform_get_drvdata (pdev); +/* Place the gpio associated with the controller into input over Well, input-with-pullup. As usual, we don't want inputs to float. :)
Ok, the comment needs changing, we do ensure the weak-pin-pullups are enabled.
Quoted 5 lines
Review static int s3c24xx_spi_suspend(struct pl · line 433
Quoted 3 lines
(quoted diff) +2
-0+ * suspend as on resume the controller is reset and will end up + * as a slave until we reconfigure it in the resume path.Kind of unfriendly, and not something I've seen much on ARM, FWIW.
I'm not sure, this tends to be the case in the Samsung range that blocks are powered down over suspend, and get reset whilst resuming. It is just unfortunate in this case that the defaults are not what we need.
Quoted 3 lines
I generally like to see pinmux stuff kept out of drivers, since it tends to change between revisions more than the controller design. I take it that's not yet an issue with this controller ...
Yes, unfortunately this case needs fixing, even if it is to add a callback to set the pin state to/from the spi pins and add some common code in arch/arm/plat-s3c24xx to deal with this. Not all devices come up in the state that you can directly connect them to their pin blocks.
I'll check through all the range that has this block in them, iirc S3C2410, S3C2412, S3C2440 and S3C2442... newer designs have a different controller in them, so do not need to be consideres for this.
Quoted 12 lines
drivers/spi/spi_s3c24xx.c static int s3c24xx_spi_suspend(struct pl · line 438
Quoted 7 lines
drivers/spi/spi_s3c24xx.c +5
-0This +* means that we cannot allow the controller to drive MISO line +* during this period in case it drives against something on the +* board. +*/ +What I don't get is that there are two outputs other than chipselect (SCK on GPE13 and GPG7, and MOSI on GPE12 and GPG6), and you seem to remux those on bus 0 but *NOT* bus 1:
Quoted 12 lines
drivers/spi/spi_s3c24xx.c static int s3c24xx_spi_suspend(struct pl · line 448
Quoted 10 lines
drivers/spi/spi_s3c24xx.c +10
-0+if (pdev->id ==0 ) { +s3c2410_gpio_pullup (S3C2410_GPE12 ,1 ); +s3c2410_gpio_pullup (S3C2410_GPE13 ,1 ); +s3c2410_gpio_cfgpin (S3C2410_GPE12 ,S3C2410_GPIO_INPUT ); +s3c2410_gpio_cfgpin (S3C2410_GPE11 ,S3C2410_GPIO_INPUT ); + }else { +s3c2410_gpio_pullup (S3C2410_GPG5 ,1 ); +s3c2410_gpio_pullup (S3C2410_GPG6 ,1 ); +s3c2410_gpio_cfgpin (S3C2410_GPG6 ,S3C2410_GPIO_INPUT ); +s3c2410_gpio_cfgpin (S3C2410_GPG5 ,S3C2410_GPIO_INPUT );... where you do GPG6/MOSI alright, but GPG5/MISO not GPG7/SCK.
SCK will become an input when the controller pops into slave mode, it is just the data pins that swap their meanings when the controller changes.
Signature · 11 lines
Ben Q: What's a light-year? A: One-third less calories than a regular year. ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/