[patch 2.6.31-rc7 2/3] spi: McSPI saves CHCONFx too

[patch 2.6.31-rc7 2/3] spi: McSPI saves CHCONFx too

[David Brownell]

From: Tero Kristo <tero.kristo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>

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 <tero.kristo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/spi/omap2_mcspi.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -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

Re: [patch 2.6.31-rc7 2/3] spi: McSPI saves CHCONFx too

[Andrew Morton]

On Thu, 27 Aug 2009 20:21:11 -0700
David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:

> From: Tero Kristo <tero.kristo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> 
> 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 <tero.kristo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
>  drivers/spi/omap2_mcspi.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> --- a/drivers/spi/omap2_mcspi.c
> +++ b/drivers/spi/omap2_mcspi.c
> @@ -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;
>  };

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

Re: [patch 2.6.31-rc7 2/3] spi: McSPI saves CHCONFx too

[David Brownell]

On Tuesday 01 September 2009, Andrew Morton wrote:
> On Thu, 27 Aug 2009 20:21:11 -0700
> David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> 
> > From: Tero Kristo <tero.kristo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> > 
> > 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 <tero.kristo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> > Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> > ---
> >  drivers/spi/omap2_mcspi.c |   16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > --- a/drivers/spi/omap2_mcspi.c
> > +++ b/drivers/spi/omap2_mcspi.c
> > @@ -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;
> >  };
> 
> 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

Re: [patch 2.6.31-rc7 2/3] spi: McSPI saves CHCONFx too

[Andrew Morton]

On Wed, 2 Sep 2009 08:53:16 -0700 David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:

> On Tuesday 01 September 2009, Andrew Morton wrote:
> > On Thu, 27 Aug 2009 20:21:11 -0700
> > David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > 
> > > From: Tero Kristo <tero.kristo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> > > 
> > > 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 <tero.kristo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> > > Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> > > ---
> > >  drivers/spi/omap2_mcspi.c |   16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > --- a/drivers/spi/omap2_mcspi.c
> > > +++ b/drivers/spi/omap2_mcspi.c
> > > @@ -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;
> > >  };
> > 
> > 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.
> 

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