| Message ID | 1431452337-19280-1-git-send-email-mwelling@ieee.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/12/2015 12:38 PM, Michael Welling wrote: > GPIO chip select patch series appears to have broken the native chip select > support. This patch pulls the manual native chip select toggling out of > the transfer_one routine and adds a set_cs routine. > > Tested natively on AM3354 with SPI serial flash on spi0cs0. > > Signed-off-by: Michael Welling <mwelling@ieee.org> > --- > drivers/spi/spi-omap2-mcspi.c | 33 +++++++++++---------------------- > 1 file changed, 11 insertions(+), 22 deletions(-) > > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c > index 90cf7e7..a7d85c5 100644 > --- a/drivers/spi/spi-omap2-mcspi.c > +++ b/drivers/spi/spi-omap2-mcspi.c > @@ -243,17 +243,20 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable) > mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHCTRL0); > } > > -static void omap2_mcspi_force_cs(struct spi_device *spi, int cs_active) > +static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > { > u32 l; > > - l = mcspi_cached_chconf0(spi); > - if (cs_active) > - l |= OMAP2_MCSPI_CHCONF_FORCE; > - else > - l &= ~OMAP2_MCSPI_CHCONF_FORCE; > + if (spi->controller_state) { > + l = mcspi_cached_chconf0(spi); > > - mcspi_write_chconf0(spi, l); > + if (enable) > + l &= ~OMAP2_MCSPI_CHCONF_FORCE; > + else > + l |= OMAP2_MCSPI_CHCONF_FORCE; > + > + mcspi_write_chconf0(spi, l); > + } > } > > static void omap2_mcspi_set_master_mode(struct spi_master *master) > @@ -1075,7 +1078,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi, > > struct spi_master *master; > struct omap2_mcspi_dma *mcspi_dma; > - int cs_active = 0; > struct omap2_mcspi_cs *cs; > struct omap2_mcspi_device_config *cd; > int par_override = 0; > @@ -1118,11 +1120,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi, > mcspi_read_cs_reg(spi, OMAP2_MCSPI_MODULCTRL); > } > > - if (!cs_active) { > - omap2_mcspi_force_cs(spi, 1); > - cs_active = 1; > - } > - > chconf = mcspi_cached_chconf0(spi); > chconf &= ~OMAP2_MCSPI_CHCONF_TRM_MASK; > chconf &= ~OMAP2_MCSPI_CHCONF_TURBO; > @@ -1169,12 +1166,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi, > if (t->delay_usecs) > udelay(t->delay_usecs); > > - /* ignore the "leave it on after last xfer" hint */ > - if (t->cs_change) { > - omap2_mcspi_force_cs(spi, 0); > - cs_active = 0; > - } > - > omap2_mcspi_set_enable(spi, 0); > > if (mcspi->fifo_depth > 0) > @@ -1187,9 +1178,6 @@ out: > status = omap2_mcspi_setup_transfer(spi, NULL); > } > > - if (cs_active) > - omap2_mcspi_force_cs(spi, 0); > - > if (cd && cd->cs_per_word) { > chconf = mcspi->ctx.modulctrl; > chconf |= OMAP2_MCSPI_MODULCTRL_SINGLE; > @@ -1334,6 +1322,7 @@ static int omap2_mcspi_probe(struct platform_device *pdev) > master->setup = omap2_mcspi_setup; > master->auto_runtime_pm = true; > master->transfer_one = omap2_mcspi_transfer_one; > + master->set_cs = omap2_mcspi_set_cs; > master->cleanup = omap2_mcspi_cleanup; > master->dev.of_node = node; > master->max_speed_hz = OMAP2_MCSPI_MAX_FREQ; > Tested on next-20150512 http://paste.ubuntu.org.cn/2600748 Since the original issue was reported by me in http://marc.info/?t=143136312900001&r=1&w=2 Reported-by: Nishanth Menon <nm@ti.com> Tested-by: Nishanth Menon <nm@ti.com>
On Tue, May 12, 2015 at 12:38:57PM -0500, Michael Welling wrote: > GPIO chip select patch series appears to have broken the native chip select > support. This patch pulls the manual native chip select toggling out of > the transfer_one routine and adds a set_cs routine. Applied, thanks
On Tue, May 12, 2015 at 01:57:36PM -0500, Nishanth Menon wrote: > On 05/12/2015 12:38 PM, Michael Welling wrote: > > GPIO chip select patch series appears to have broken the native chip select > > support. This patch pulls the manual native chip select toggling out of > > the transfer_one routine and adds a set_cs routine. Please remember to delete unneeded context from your replies, the reader shouldn't need to page through the entire patch to find out you reviewed it.
On 05/12/2015 02:19 PM, Mark Brown wrote: > On Tue, May 12, 2015 at 01:57:36PM -0500, Nishanth Menon wrote: >> On 05/12/2015 12:38 PM, Michael Welling wrote: >>> GPIO chip select patch series appears to have broken the native chip select >>> support. This patch pulls the manual native chip select toggling out of >>> the transfer_one routine and adds a set_cs routine. > > Please remember to delete unneeded context from your replies, the reader > shouldn't need to page through the entire patch to find out you reviewed > it. Will do so. Anyways, I did test it to be accurate - have'nt reviewed it.
On Tue, May 12, 2015 at 08:17:58PM +0100, Mark Brown wrote: > On Tue, May 12, 2015 at 12:38:57PM -0500, Michael Welling wrote: > > GPIO chip select patch series appears to have broken the native chip select > > support. This patch pulls the manual native chip select toggling out of > > the transfer_one routine and adds a set_cs routine. > > Applied, thanks It appears that in haste, this fix for the native cs support broke the GPIO chip select support that I was originally shooting for. [ 2.653658] mcp23s08 spi1.1: TXS timed out [ 2.657961] mcp23s08 spi1.1: SPI transfer failed: -5 [ 2.663305] spi_master spi1: failed to transfer one message from queue [ 2.670172] mcp23s08 spi1.1: can't setup chip 64, --> -5 [ 2.675784] GPIO chip mcp23s08: gpiochip_unexport: status -19 My guess is that the set_cs needs to be called even when toggling as GPIO. How should I handle this?
On Wed, May 20, 2015 at 09:07:09PM -0500, Michael Welling wrote: > My guess is that the set_cs needs to be called even when toggling as GPIO. > How should I handle this? It shouldn't be part of a set_cs() operation but rather part of the main transfer operation.
On Thu, May 21, 2015 at 11:18:57AM +0100, Mark Brown wrote: > On Wed, May 20, 2015 at 09:07:09PM -0500, Michael Welling wrote: > > > My guess is that the set_cs needs to be called even when toggling as GPIO. > > > How should I handle this? > > It shouldn't be part of a set_cs() operation but rather part of the main > transfer operation. Okay then this patch should be reverted. Do you want to revert the patch and apply a new one or should I provide a patch that reverts the changes and fixes it all in one? Sorry for this mess.
On Thu, May 21, 2015 at 04:04:11PM -0500, Michael Welling wrote: > Do you want to revert the patch and apply a new one or should I provide a > patch that reverts the changes and fixes it all in one? Can you please send me separate revert and re-add patches, that's probably going to be easier to review.
On Thu, May 21, 2015 at 10:16:38PM +0100, Mark Brown wrote: > On Thu, May 21, 2015 at 04:04:11PM -0500, Michael Welling wrote: > > > Do you want to revert the patch and apply a new one or should I provide a > > patch that reverts the changes and fixes it all in one? > > Can you please send me separate revert and re-add patches, that's > probably going to be easier to review. So after reverting this patch I found there is a issue in that it is difficult to determine when a transfer is complete to properly drive the chipselect from within the transfer_one function. Then I figured that we could handle the case when GPIOs are being used with some conditional calls to omap2_mcspi_set_cs in the omap2_mcspi_work_one function. Near the beginning of the function I added: if (gpio_is_valid(spi->cs_gpio)) omap2_mcspi_set_cs(spi, 0); Near the end of the function I added: if (gpio_is_valid(spi->cs_gpio)) omap2_mcspi_set_cs(spi, 1); This makes GPIO chip select support work while leaving the native working as previous. Is this solution acceptible? In the process of reviewing the changes I found a few other things that should be changed as well. Here you will see a delay that is already handled by the core spi driver: https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/spi/spi-omap2-mcspi.c#n1166 In the set_cs function the inversion of the enable that occurs in the core spi_set_cs function based on SPI_CS_HIGH needs to revert as the controller handles the inversion with OMAP2_MCSPI_CHCONF_EPOL bit in the CHCONF register. If you approve of the fix for the GPIO support, I will provide a patch series with all of these above mentioned fixes.
On Thu, May 21, 2015 at 06:48:33PM -0500, Michael Welling wrote: > So after reverting this patch I found there is a issue in that it is difficult > to determine when a transfer is complete to properly drive the chipselect from > within the transfer_one function. Is unprepare_message() a suitable place here? I've got a feeling the answer is no... > Then I figured that we could handle the case when GPIOs are being used with > some conditional calls to omap2_mcspi_set_cs in the omap2_mcspi_work_one > function. > > Near the beginning of the function I added: > if (gpio_is_valid(spi->cs_gpio)) > omap2_mcspi_set_cs(spi, 0); > > Near the end of the function I added: > if (gpio_is_valid(spi->cs_gpio)) > omap2_mcspi_set_cs(spi, 1); > > This makes GPIO chip select support work while leaving the native working > as previous. > > Is this solution acceptible? I think that's probably OK as well, it's not ideal though (and risky if the chip select is routed somewhere...). > In the process of reviewing the changes I found a few other things that > should be changed as well. Please send fixes for these as separate patches (ideally without any dependency on your new work so we can send them to Linus as fixes). > Here you will see a delay that is already handled by the core spi driver: > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/spi/spi-omap2-mcspi.c#n1166 I can't actually see that since I have no internet access right now!
On Fri, May 22, 2015 at 01:25:44PM +0100, Mark Brown wrote: > On Thu, May 21, 2015 at 06:48:33PM -0500, Michael Welling wrote: > > > So after reverting this patch I found there is a issue in that it is difficult > > to determine when a transfer is complete to properly drive the chipselect from > > within the transfer_one function. > > Is unprepare_message() a suitable place here? I've got a feeling the > answer is no... > > > Then I figured that we could handle the case when GPIOs are being used with > > some conditional calls to omap2_mcspi_set_cs in the omap2_mcspi_work_one > > function. > > > > Near the beginning of the function I added: > > if (gpio_is_valid(spi->cs_gpio)) > > omap2_mcspi_set_cs(spi, 0); > > > > Near the end of the function I added: > > if (gpio_is_valid(spi->cs_gpio)) > > omap2_mcspi_set_cs(spi, 1); > > > > This makes GPIO chip select support work while leaving the native working > > as previous. > > > > Is this solution acceptible? > > I think that's probably OK as well, it's not ideal though (and risky if > the chip select is routed somewhere...). > > > In the process of reviewing the changes I found a few other things that > > should be changed as well. > > Please send fixes for these as separate patches (ideally without any > dependency on your new work so we can send them to Linus as fixes). > Well actually these fixes are needed as the results of the first three patches pushed into next. When switching from transfer_one_message to tranfer_one I did not realize that the delay was handled in the core. When adding the set_cs function I did not notice that the enable was complemented by the core driver. > > Here you will see a delay that is already handled by the core spi driver: > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/spi/spi-omap2-mcspi.c#n1166 > > I can't actually see that since I have no internet access right now! That's like a fish out of water.
The recent update of the OMAP2 McSPI driver left some unresolved issues. These patches should take care them and again allow for GPIO chip selects and native chip selects. Michael Welling (4): spi: omap2-mcspi: Remove unnecessary delay spi: omap2-mcspi: Fix set_cs function for active high spi: omap2-mcspi: Fix GPIO chip select support spi: omap2-mcspi: Handle error on gpio_request drivers/spi/spi-omap2-mcspi.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
On Sat, May 23, 2015 at 09:13:41PM -0500, Michael Welling wrote: > The recent update of the OMAP2 McSPI driver left some unresolved issues. > These patches should take care them and again allow for GPIO chip selects > and native chip selects. Applied all, thanks.
diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c index 90cf7e7..a7d85c5 100644 --- a/drivers/spi/spi-omap2-mcspi.c +++ b/drivers/spi/spi-omap2-mcspi.c @@ -243,17 +243,20 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable) mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHCTRL0); } -static void omap2_mcspi_force_cs(struct spi_device *spi, int cs_active) +static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) { u32 l; - l = mcspi_cached_chconf0(spi); - if (cs_active) - l |= OMAP2_MCSPI_CHCONF_FORCE; - else - l &= ~OMAP2_MCSPI_CHCONF_FORCE; + if (spi->controller_state) { + l = mcspi_cached_chconf0(spi); - mcspi_write_chconf0(spi, l); + if (enable) + l &= ~OMAP2_MCSPI_CHCONF_FORCE; + else + l |= OMAP2_MCSPI_CHCONF_FORCE; + + mcspi_write_chconf0(spi, l); + } } static void omap2_mcspi_set_master_mode(struct spi_master *master) @@ -1075,7 +1078,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi, struct spi_master *master; struct omap2_mcspi_dma *mcspi_dma; - int cs_active = 0; struct omap2_mcspi_cs *cs; struct omap2_mcspi_device_config *cd; int par_override = 0; @@ -1118,11 +1120,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi, mcspi_read_cs_reg(spi, OMAP2_MCSPI_MODULCTRL); } - if (!cs_active) { - omap2_mcspi_force_cs(spi, 1); - cs_active = 1; - } - chconf = mcspi_cached_chconf0(spi); chconf &= ~OMAP2_MCSPI_CHCONF_TRM_MASK; chconf &= ~OMAP2_MCSPI_CHCONF_TURBO; @@ -1169,12 +1166,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi, if (t->delay_usecs) udelay(t->delay_usecs); - /* ignore the "leave it on after last xfer" hint */ - if (t->cs_change) { - omap2_mcspi_force_cs(spi, 0); - cs_active = 0; - } - omap2_mcspi_set_enable(spi, 0); if (mcspi->fifo_depth > 0) @@ -1187,9 +1178,6 @@ out: status = omap2_mcspi_setup_transfer(spi, NULL); } - if (cs_active) - omap2_mcspi_force_cs(spi, 0); - if (cd && cd->cs_per_word) { chconf = mcspi->ctx.modulctrl; chconf |= OMAP2_MCSPI_MODULCTRL_SINGLE; @@ -1334,6 +1322,7 @@ static int omap2_mcspi_probe(struct platform_device *pdev) master->setup = omap2_mcspi_setup; master->auto_runtime_pm = true; master->transfer_one = omap2_mcspi_transfer_one; + master->set_cs = omap2_mcspi_set_cs; master->cleanup = omap2_mcspi_cleanup; master->dev.of_node = node; master->max_speed_hz = OMAP2_MCSPI_MAX_FREQ;
GPIO chip select patch series appears to have broken the native chip select support. This patch pulls the manual native chip select toggling out of the transfer_one routine and adds a set_cs routine. Tested natively on AM3354 with SPI serial flash on spi0cs0. Signed-off-by: Michael Welling <mwelling@ieee.org> --- drivers/spi/spi-omap2-mcspi.c | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-)