Skip to contentSkip to conversation

spi: McSPI saves CHCONFx too

PATCH In review David Brownell 2 participants 4 messages Aug 28, 2009 – Sep 2, 2009 3 messages not in the archive SPI

lore
Share & tools
lore

David Brownell

author
#0 lore raw

PATCH 2/3 spi: McSPI saves CHCONFx too

From: Tero Kristo <[email protected]>

Previous restore was lazy and only restored CHxCONF when it was needed by a specific chip select. This could cause occasional errors on an SPI bus where multiple chip selects are in use.

Signed-off-by Tero Kristo <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>
Signed-off-by David Brownell <[email protected]>
---
 drivers/spi/omap2_mcspi.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/spi/omap2_mcspi.c +16 -0
@@ -134,6 +134,7 @@ struct omap2_mcspi_cs {
 	void __iomem		*base;
 	unsigned long		phys;
 	int			word_len;
+	struct list_head	node;
 	/* Context save and restore shadow register */
 	u32			chconf0;
 };

@@ -145,6 +146,7 @@ struct omap2_mcspi_regs {
 	u32 sysconfig;
 	u32 modulctrl;
 	u32 wakeupenable;
+	struct list_head cs;
 };
 
 static struct omap2_mcspi_regs omap2_mcspi_ctx[OMAP2_MCSPI_MAX_CTRL];

@@ -255,6 +257,7 @@ static void omap2_mcspi_set_master_mode(
 static void omap2_mcspi_restore_ctx(struct omap2_mcspi *mcspi)
 {
 	struct spi_master *spi_cntrl;
+	struct omap2_mcspi_cs *cs;
 	spi_cntrl = mcspi->master;
 
 	/* McSPI: context restore */

@@ -266,6 +269,10 @@ static void omap2_mcspi_restore_ctx(stru
 
 	mcspi_write_reg(spi_cntrl, OMAP2_MCSPI_WAKEUPENABLE,
 			omap2_mcspi_ctx[spi_cntrl->bus_num - 1].wakeupenable);
+
+	list_for_each_entry(cs, &omap2_mcspi_ctx[spi_cntrl->bus_num - 1].cs,
+			node)
+		__raw_writel(cs->chconf0, cs->base + OMAP2_MCSPI_CHCONF0);
 }
 static void omap2_mcspi_disable_clocks(struct omap2_mcspi *mcspi)
 {

@@ -714,6 +721,9 @@ static int omap2_mcspi_setup(struct spi_
 		cs->phys = mcspi->phys + spi->chip_select * 0x14;
 		cs->chconf0 = 0;
 		spi->controller_state = cs;
+		/* Link this to context save list */
+		list_add_tail(&cs->node,
+			&omap2_mcspi_ctx[mcspi->master->bus_num - 1].cs);
 	}
 
 	if (mcspi_dma->dma_rx_channel == -1

@@ -736,10 +746,15 @@ static void omap2_mcspi_cleanup(struct s
 {
 	struct omap2_mcspi	*mcspi;
 	struct omap2_mcspi_dma	*mcspi_dma;
+	struct omap2_mcspi_cs	*cs;
 
 	mcspi = spi_master_get_devdata(spi->master);
 	mcspi_dma = &mcspi->dma_channels[spi->chip_select];
 
+	/* Unlink controller state from context save list */
+	cs = spi->controller_state;
+	list_del(&cs->node);
+
 	kfree(spi->controller_state);
 
 	if (mcspi_dma->dma_rx_channel != -1) {

@@ -1104,6 +1119,7 @@ static int __init omap2_mcspi_probe(stru
 
 	spin_lock_init(&mcspi->lock);
 	INIT_LIST_HEAD(&mcspi->msg_queue);
+	INIT_LIST_HEAD(&omap2_mcspi_ctx[master->bus_num - 1].cs);
 
 	mcspi->ick = clk_get(&pdev->dev, "ick");
 	if (IS_ERR(mcspi->ick)) {

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day trial. Simplify your report design, integration and deployment - and focus on what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july

A message that is not in the archive (sent off-list or not yet received): 200908272021.12070.david-b-yBeKhBN/[email protected]

#2 lore raw
Quoted 24 lines · On Thu, 27 Aug 2009 20:21:11 -0700 David Brownell…

From: Tero Kristo <[email protected]>

Previous restore was lazy and only restored CHxCONF when it was needed by a specific chip select. This could cause occasional errors on an SPI bus where multiple chip selects are in use.

Signed-off-by Tero Kristo <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>
Signed-off-by David Brownell <[email protected]>
---
 drivers/spi/omap2_mcspi.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/spi/omap2_mcspi.c +1 -0
@@ -134,6 +134,7 @@ struct omap2_mcspi_cs {
 	void __iomem		*base;
 	unsigned long		phys;
 	int			word_len;
+	struct list_head	node;
 	/* Context save and restore shadow register */
 	u32			chconf0;
 };

drivers/spi/omap2_mcspi.c struct omap2_mcspi_regs { · line 150

Quoted 5 lines
drivers/spi/omap2_mcspi.c +1 -0
@@ -145,6 +146,7 @@ struct omap2_mcspi_regs {
 	u32 sysconfig;
 	u32 modulctrl;
 	u32 wakeupenable;
+	struct list_head cs;
 };

Which locking protects these new lists?

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day trial. Simplify your report design, integration and deployment - and focus on what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july

A message that is not in the archive (sent off-list or not yet received): 20090901135726.96173f7e.akpm-de/[email protected]

author
#4 lore raw
Quoted 26 lines · Andrew Morton
Quoted 23 lines · On Thu, 27 Aug 2009 20:21:11 -0700 David Brownell…

From: Tero Kristo <[email protected]>

Previous restore was lazy and only restored CHxCONF when it was needed by a specific chip select. This could cause occasional errors on an SPI bus where multiple chip selects are in use.

Signed-off-by Tero Kristo <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>
Signed-off-by David Brownell <[email protected]>
---
 drivers/spi/omap2_mcspi.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/spi/omap2_mcspi.c +1 -0
@@ -134,6 +134,7 @@ struct omap2_mcspi_cs {
 	void __iomem		*base;
 	unsigned long		phys;
 	int			word_len;
+	struct list_head	node;
 	/* Context save and restore shadow register */
 	u32			chconf0;
 };
Quoted 8 lines

drivers/spi/omap2_mcspi.c struct omap2_mcspi_regs { · line 150

Quoted 6 lines
drivers/spi/omap2_mcspi.c +1 -0
@@ -145,6 +146,7 @@ struct omap2_mcspi_regs {
 	u32 sysconfig;
 	u32 modulctrl;
 	u32 wakeupenable;
+	struct list_head cs;
 };

Which locking protects these new lists?

One's the head, one's the instance ... the list is used in activation and deactivation paths, so the same locks which already protect those paths.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day trial. Simplify your report design, integration and deployment - and focus on what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july

A message that is not in the archive (sent off-list or not yet received): 200909020853.17212.david-b-yBeKhBN/[email protected]

#6 lore raw
Quoted 36 lines · On Wed, 2 Sep 2009 08:53:16 -0700 David Brownell…
Quoted 26 lines · Andrew Morton
Quoted 23 lines · On Thu, 27 Aug 2009 20:21:11 -0700 David Brownell…

From: Tero Kristo <[email protected]>

Previous restore was lazy and only restored CHxCONF when it was needed by a specific chip select. This could cause occasional errors on an SPI bus where multiple chip selects are in use.

Signed-off-by Tero Kristo <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>
Signed-off-by David Brownell <[email protected]>
---
 drivers/spi/omap2_mcspi.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/spi/omap2_mcspi.c +1 -0
@@ -134,6 +134,7 @@ struct omap2_mcspi_cs {
 	void __iomem		*base;
 	unsigned long		phys;
 	int			word_len;
+	struct list_head	node;
 	/* Context save and restore shadow register */
 	u32			chconf0;
 };
Quoted 8 lines

drivers/spi/omap2_mcspi.c struct omap2_mcspi_regs { · line 150

Quoted 6 lines
drivers/spi/omap2_mcspi.c +1 -0
@@ -145,6 +146,7 @@ struct omap2_mcspi_regs {
 	u32 sysconfig;
 	u32 modulctrl;
 	u32 wakeupenable;
+	struct list_head cs;
 };

Which locking protects these new lists?

Quoted 4 lines

One's the head, one's the instance ... the list is used in activation and deactivation paths, so the same locks which already protect those paths.

Which locks are those?

<looks for 10 seconds>

omap2_mcspi_work() calls omap2_mcspi_enable_clocks() under mcspi->lock

omap2_mcspi_setup() calls omap2_mcspi_enable_clocks() not under mcspi->lock

The locking here is insufficiently clear and possibly buggy.

Please revisit. If it's missing, add it. If it's unneeded, document it. If it's actually present and correct, document it.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day trial. Simplify your report design, integration and deployment - and focus on what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july