| Message ID | 1342805751-18048-3-git-send-email-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 20, 2012 at 7:35 PM, Andrew Lunn <andrew@lunn.ch> wrote: > Replace the deprecated master->transfer with transfer_one_message() > and allow the SPI subsystem handle all the queuing of messages. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@googlemail.com> Send these with Mark Brown on the To: line right now, I think he's busy collecting SPI patches for the next merge window. Yours, Linus Walleij
On Sun, Jul 22, 2012 at 09:22:09AM +0200, Linus Walleij wrote: > On Fri, Jul 20, 2012 at 7:35 PM, Andrew Lunn <andrew@lunn.ch> wrote: > > > Replace the deprecated master->transfer with transfer_one_message() > > and allow the SPI subsystem handle all the queuing of messages. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@googlemail.com> > > Send these with Mark Brown on the To: line right now, I think he's > busy collecting SPI patches for the next merge window. Hi Linus, Mark and now Arnd. I was planning sending all these via arm-soc. This patch is part of a bigger collection of Orion changes. The rest are contributions from others which have already been ack-by, etc, on the lists. Somewhere near the beginning of this series is: 4385fbf SPI: Refactor spi-orion to use SPI framework queue. d9d8293 spi-orion: remove uneeded spi_info ee13b49 spi-orion: add device tree binding 76f0b84 ARM: kirkwood: use devicetree for SPI on dreamplug 05f504f ARM: kirkwood: use devicetree for orion-spi You can see the chain of dependencies here. So at minimum i would need to send the first three via Mark. I then need to cross my fingers and hope he really picks them up, because without them, we are going to have a number of boards which fail to find their root file system on an SPI flash. Now, maybe i've done all this wrong, since i'm a newbie to collecting patches together for submission upstream.... You can see the complete set of patches here: git://github.com/lunn/linux.git v3.5-rc7-for-next-v5 There is one exception and that is the very first patch: I2C: MV64XXX: Add Device Tree support which Wolfram Sang insists goes via him. I see this less critical. If for some reason it does not make it in, all it means is temperature sensors and RTCs are missing, but at least the boards still boot. Andrew
On Sun, Jul 22, 2012 at 11:53:28AM +0200, Andrew Lunn wrote: > You can see the chain of dependencies here. So at minimum i would need > to send the first three via Mark. I then need to cross my fingers and > hope he really picks them up, because without them, we are going to > have a number of boards which fail to find their root file system on > an SPI flash. Well, I'd like to think that generally I'm reasonably responsive to patch submissions and obviously there's no great urgency here since we're in the merge window - nothing is likely to go in until 3.7 anyway, though it'd be handy if the DT updates went in early (and they're very low risk...). > Now, maybe i've done all this wrong, since i'm a newbie to collecting > patches together for submission upstream.... The general pattern with these things should be that the patches get sent via the normal route and any unusual requirements discussed then. To be honest it doesn't seem like there's much problem here, you've got no build time dependencies or anything.
On Sun, Jul 22, 2012 at 08:07:01PM +0100, Mark Brown wrote: > On Sun, Jul 22, 2012 at 11:53:28AM +0200, Andrew Lunn wrote: > > > You can see the chain of dependencies here. So at minimum i would need > > to send the first three via Mark. I then need to cross my fingers and > > hope he really picks them up, because without them, we are going to > > have a number of boards which fail to find their root file system on > > an SPI flash. > > Well, I'd like to think that generally I'm reasonably responsive to > patch submissions Well, this particular patch was first submitted May 19th and Ack-by Linus on May 14th. http://comments.gmane.org/gmane.linux.kernel.spi.devel/9937 It was reposted 10 Jun and Acked-by again in Jun 10 by Linus. It was also reposted July 3. Andrew
On Sun, Jul 22, 2012 at 09:32:43PM +0200, Andrew Lunn wrote: > On Sun, Jul 22, 2012 at 08:07:01PM +0100, Mark Brown wrote: > > Well, I'd like to think that generally I'm reasonably responsive to > > patch submissions > Well, this particular patch was first submitted May 19th and Ack-by > Linus on May 14th. > http://comments.gmane.org/gmane.linux.kernel.spi.devel/9937 > It was reposted 10 Jun and Acked-by again in Jun 10 by Linus. > It was also reposted July 3. None of which were CCed to me; they were all before I started looking after SPI so that's not surprising but still the number of reposts is more a comment on why I'm helping out here than on what I said above.
On Sun, Jul 22, 2012 at 11:42:57PM +0100, Mark Brown wrote: > On Sun, Jul 22, 2012 at 09:32:43PM +0200, Andrew Lunn wrote: > > On Sun, Jul 22, 2012 at 08:07:01PM +0100, Mark Brown wrote: > > > > Well, I'd like to think that generally I'm reasonably responsive to > > > patch submissions > > > Well, this particular patch was first submitted May 19th and Ack-by > > Linus on May 14th. > > > http://comments.gmane.org/gmane.linux.kernel.spi.devel/9937 > > > It was reposted 10 Jun and Acked-by again in Jun 10 by Linus. > > > It was also reposted July 3. > > None of which were CCed to me; they were all before I started looking > after SPI so that's not surprising but still the number of reposts is > more a comment on why I'm helping out here than on what I said above. Hi Mark O.K., fair enough. Will you accept any patches for this merge window? As you said, the DT enablement is low risk. It should be possible to separate this one patch from the other two. https://github.com/lunn/linux/commit/794e259e3e23f02250ac12c74dd507a4b340b257 Andrew
On Mon, Jul 23, 2012 at 09:27:49AM +0200, Andrew Lunn wrote: > Will you accept any patches for this merge window? As you said, the DT > enablement is low risk. It should be possible to separate this one > patch from the other two. Can you send the patches please? The bit I was saying was most low risk was the actual DT changes, as opposed to the bits that parse and use the DT.
On Mon, Jul 23, 2012 at 10:37:47AM +0100, Mark Brown wrote: > On Mon, Jul 23, 2012 at 09:27:49AM +0200, Andrew Lunn wrote: > > > Will you accept any patches for this merge window? As you said, the DT > > enablement is low risk. It should be possible to separate this one > > patch from the other two. > > Can you send the patches please? I just sent the DT enabling patch. The other two, "Refactor spi-orion to use SPI framework queue." and "remove uneeded spi_info" can wait for the next merge window. For me, they are lower priority than getting DT working. > The bit I was saying was most low risk was the actual DT changes, as > opposed to the bits that parse and use the DT. Ah, O.K. I understood you wrongly. I actually think they are the most dangerous part. A board booting with these DT changes, and without the driver enablement for DT is likely to fail to find its root filesystem. Thanks Andrew
On Mon, Jul 23, 2012 at 12:16:41PM +0200, Andrew Lunn wrote: > On Mon, Jul 23, 2012 at 10:37:47AM +0100, Mark Brown wrote: > > The bit I was saying was most low risk was the actual DT changes, as > > opposed to the bits that parse and use the DT. > Ah, O.K. I understood you wrongly. I actually think they are the most > dangerous part. A board booting with these DT changes, and without the > driver enablement for DT is likely to fail to find its root > filesystem. Why would that be? If there's no DT support for the driver it'll just ignore the DT.
On Mon, Jul 23, 2012 at 11:23:36AM +0100, Mark Brown wrote: > On Mon, Jul 23, 2012 at 12:16:41PM +0200, Andrew Lunn wrote: > > On Mon, Jul 23, 2012 at 10:37:47AM +0100, Mark Brown wrote: > > > > The bit I was saying was most low risk was the actual DT changes, as > > > opposed to the bits that parse and use the DT. > > > Ah, O.K. I understood you wrongly. I actually think they are the most > > dangerous part. A board booting with these DT changes, and without the > > driver enablement for DT is likely to fail to find its root > > filesystem. > > Why would that be? If there's no DT support for the driver it'll just > ignore the DT. Yes, it ignores the DT. However, SPI driver never gets instantiated. The FLASH chip on the SPI bus never gets instantiated. MTD partitions are probably created, but since the underlying device is missing, they don't work and the root filesystem is not available. There is no logic in the board code to first try DT, and if that fails, use old fashioned platform_device instantiating of the drivers. I've also had bad experiences when we have the old platform_device instantiating plus DT also instantiating the same device. The only way i've found to convert an existing board from platform_device to DT is to first add DT support to the driver, and then atomically swap platform_device structures for DT descriptions. Andrew
Hello. On 22-07-2012 23:32, Andrew Lunn wrote: >>> You can see the chain of dependencies here. So at minimum i would need >>> to send the first three via Mark. I then need to cross my fingers and >>> hope he really picks them up, because without them, we are going to >>> have a number of boards which fail to find their root file system on >>> an SPI flash. >> Well, I'd like to think that generally I'm reasonably responsive to >> patch submissions > Well, this particular patch was first submitted May 19th and Ack-by > Linus on May 14th. It was acked even before submitted? ;-) > http://comments.gmane.org/gmane.linux.kernel.spi.devel/9937 > It was reposted 10 Jun and Acked-by again in Jun 10 by Linus. > It was also reposted July 3. WBR, Sergei
On Mon, Jul 23, 2012 at 02:53:54PM +0400, Sergei Shtylyov wrote: > Hello. > > On 22-07-2012 23:32, Andrew Lunn wrote: > > >>>You can see the chain of dependencies here. So at minimum i would need > >>>to send the first three via Mark. I then need to cross my fingers and > >>>hope he really picks them up, because without them, we are going to > >>>have a number of boards which fail to find their root file system on > >>>an SPI flash. > > >>Well, I'd like to think that generally I'm reasonably responsive to > >>patch submissions > > >Well, this particular patch was first submitted May 19th and Ack-by > >Linus on May 14th. > > It was acked even before submitted? ;-) I have my dates wrong. See: http://comments.gmane.org/gmane.linux.kernel.spi.devel/9937 First posted 9 May, acked 14th May. Andrew
On Mon, Jul 23, 2012 at 12:49:11PM +0200, Andrew Lunn wrote: > There is no logic in the board code to first try DT, and if that > fails, use old fashioned platform_device instantiating of the drivers. Well, I'd really expect this to be done the other way around. > I've also had bad experiences when we have the old platform_device > instantiating plus DT also instantiating the same device. Hopefully we can arrange for them to collide with each other... > The only way i've found to convert an existing board from > platform_device to DT is to first add DT support to the driver, and > then atomically swap platform_device structures for DT descriptions. I guess if it's not working then we need to do that, but it's really sad that the DT deployment is this painful.
diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c index dfd04e9..547d983 100644 --- a/drivers/spi/spi-orion.c +++ b/drivers/spi/spi-orion.c @@ -35,12 +35,6 @@ #define ORION_SPI_CLK_PRESCALE_MASK 0x1F struct orion_spi { - struct work_struct work; - - /* Lock access to transfer list. */ - spinlock_t lock; - - struct list_head msg_queue; struct spi_master *master; void __iomem *base; unsigned int max_speed; @@ -49,8 +43,6 @@ struct orion_spi { struct clk *clk; }; -static struct workqueue_struct *orion_spi_wq; - static inline void __iomem *spi_reg(struct orion_spi *orion_spi, u32 reg) { return orion_spi->base + reg; @@ -277,73 +269,78 @@ out: } -static void orion_spi_work(struct work_struct *work) +static int orion_spi_transfer_one_message(struct spi_master *master, + struct spi_message *m) { - struct orion_spi *orion_spi = - container_of(work, struct orion_spi, work); - - spin_lock_irq(&orion_spi->lock); - while (!list_empty(&orion_spi->msg_queue)) { - struct spi_message *m; - struct spi_device *spi; - struct spi_transfer *t = NULL; - int par_override = 0; - int status = 0; - int cs_active = 0; - - m = container_of(orion_spi->msg_queue.next, struct spi_message, - queue); + struct orion_spi *orion_spi = spi_master_get_devdata(master); + struct spi_device *spi = m->spi; + struct spi_transfer *t = NULL; + int par_override = 0; + int status = 0; + int cs_active = 0; - list_del_init(&m->queue); - spin_unlock_irq(&orion_spi->lock); + /* Load defaults */ + status = orion_spi_setup_transfer(spi, NULL); - spi = m->spi; + if (status < 0) + goto msg_done; - /* Load defaults */ - status = orion_spi_setup_transfer(spi, NULL); + list_for_each_entry(t, &m->transfers, transfer_list) { + /* make sure buffer length is even when working in 16 + * bit mode*/ + if ((t->bits_per_word == 16) && (t->len & 1)) { + dev_err(&spi->dev, + "message rejected : " + "odd data length %d while in 16 bit mode\n", + t->len); + status = -EIO; + goto msg_done; + } - if (status < 0) + if (t->speed_hz && t->speed_hz < orion_spi->min_speed) { + dev_err(&spi->dev, + "message rejected : " + "device min speed (%d Hz) exceeds " + "required transfer speed (%d Hz)\n", + orion_spi->min_speed, t->speed_hz); + status = -EIO; goto msg_done; + } - list_for_each_entry(t, &m->transfers, transfer_list) { - if (par_override || t->speed_hz || t->bits_per_word) { - par_override = 1; - status = orion_spi_setup_transfer(spi, t); - if (status < 0) - break; - if (!t->speed_hz && !t->bits_per_word) - par_override = 0; - } - - if (!cs_active) { - orion_spi_set_cs(orion_spi, 1); - cs_active = 1; - } - - if (t->len) - m->actual_length += - orion_spi_write_read(spi, t); - - if (t->delay_usecs) - udelay(t->delay_usecs); - - if (t->cs_change) { - orion_spi_set_cs(orion_spi, 0); - cs_active = 0; - } + if (par_override || t->speed_hz || t->bits_per_word) { + par_override = 1; + status = orion_spi_setup_transfer(spi, t); + if (status < 0) + break; + if (!t->speed_hz && !t->bits_per_word) + par_override = 0; } -msg_done: - if (cs_active) - orion_spi_set_cs(orion_spi, 0); + if (!cs_active) { + orion_spi_set_cs(orion_spi, 1); + cs_active = 1; + } - m->status = status; - m->complete(m->context); + if (t->len) + m->actual_length += orion_spi_write_read(spi, t); - spin_lock_irq(&orion_spi->lock); + if (t->delay_usecs) + udelay(t->delay_usecs); + + if (t->cs_change) { + orion_spi_set_cs(orion_spi, 0); + cs_active = 0; + } } - spin_unlock_irq(&orion_spi->lock); +msg_done: + if (cs_active) + orion_spi_set_cs(orion_spi, 0); + + m->status = status; + spi_finalize_current_message(master); + + return 0; } static int __init orion_spi_reset(struct orion_spi *orion_spi) @@ -376,75 +373,6 @@ static int orion_spi_setup(struct spi_device *spi) return 0; } -static int orion_spi_transfer(struct spi_device *spi, struct spi_message *m) -{ - struct orion_spi *orion_spi; - struct spi_transfer *t = NULL; - unsigned long flags; - - m->actual_length = 0; - m->status = 0; - - /* reject invalid messages and transfers */ - if (list_empty(&m->transfers) || !m->complete) - return -EINVAL; - - orion_spi = spi_master_get_devdata(spi->master); - - list_for_each_entry(t, &m->transfers, transfer_list) { - unsigned int bits_per_word = spi->bits_per_word; - - if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) { - dev_err(&spi->dev, - "message rejected : " - "invalid transfer data buffers\n"); - goto msg_rejected; - } - - if (t->bits_per_word) - bits_per_word = t->bits_per_word; - - if ((bits_per_word != 8) && (bits_per_word != 16)) { - dev_err(&spi->dev, - "message rejected : " - "invalid transfer bits_per_word (%d bits)\n", - bits_per_word); - goto msg_rejected; - } - /*make sure buffer length is even when working in 16 bit mode*/ - if ((t->bits_per_word == 16) && (t->len & 1)) { - dev_err(&spi->dev, - "message rejected : " - "odd data length (%d) while in 16 bit mode\n", - t->len); - goto msg_rejected; - } - - if (t->speed_hz && t->speed_hz < orion_spi->min_speed) { - dev_err(&spi->dev, - "message rejected : " - "device min speed (%d Hz) exceeds " - "required transfer speed (%d Hz)\n", - orion_spi->min_speed, t->speed_hz); - goto msg_rejected; - } - } - - - spin_lock_irqsave(&orion_spi->lock, flags); - list_add_tail(&m->queue, &orion_spi->msg_queue); - queue_work(orion_spi_wq, &orion_spi->work); - spin_unlock_irqrestore(&orion_spi->lock, flags); - - return 0; -msg_rejected: - /* Message rejected and not queued */ - m->status = -EINVAL; - if (m->complete) - m->complete(m->context); - return -EINVAL; -} - static int __init orion_spi_probe(struct platform_device *pdev) { struct spi_master *master; @@ -469,7 +397,7 @@ static int __init orion_spi_probe(struct platform_device *pdev) master->mode_bits = 0; master->setup = orion_spi_setup; - master->transfer = orion_spi_transfer; + master->transfer_one_message = orion_spi_transfer_one_message; master->num_chipselect = ORION_NUM_CHIPSELECTS; dev_set_drvdata(&pdev->dev, master); @@ -503,11 +431,6 @@ static int __init orion_spi_probe(struct platform_device *pdev) } spi->base = ioremap(r->start, SZ_1K); - INIT_WORK(&spi->work, orion_spi_work); - - spin_lock_init(&spi->lock); - INIT_LIST_HEAD(&spi->msg_queue); - if (orion_spi_reset(spi) < 0) goto out_rel_mem; @@ -531,14 +454,12 @@ out: static int __exit orion_spi_remove(struct platform_device *pdev) { struct spi_master *master; - struct orion_spi *spi; struct resource *r; + struct orion_spi *spi; master = dev_get_drvdata(&pdev->dev); spi = spi_master_get_devdata(master); - cancel_work_sync(&spi->work); - clk_disable_unprepare(spi->clk); clk_put(spi->clk); @@ -562,21 +483,13 @@ static struct platform_driver orion_spi_driver = { static int __init orion_spi_init(void) { - orion_spi_wq = create_singlethread_workqueue( - orion_spi_driver.driver.name); - if (orion_spi_wq == NULL) - return -ENOMEM; - return platform_driver_probe(&orion_spi_driver, orion_spi_probe); } module_init(orion_spi_init); static void __exit orion_spi_exit(void) { - flush_workqueue(orion_spi_wq); platform_driver_unregister(&orion_spi_driver); - - destroy_workqueue(orion_spi_wq); } module_exit(orion_spi_exit);